Re: [PULL 12/30] hw/block/nvme: add support for scatter gather lists

[Max Reitz]

On 27.10.20 11:49, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> For now, support the Data Block, Segment and Last Segment descriptor
> types.
> 
> See NVM Express 1.3d, Section 4.4 ("Scatter Gather List (SGL)").
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> ---
>  include/block/nvme.h  |   6 +-
>  hw/block/nvme.c       | 329 ++++++++++++++++++++++++++++++++++--------
>  hw/block/trace-events |   4 +
>  3 files changed, 279 insertions(+), 60 deletions(-)

[...]

> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index c0f1f8ccd473..63d0a17bc5f0 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -413,13 +413,262 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t 
> prp1, uint64_t prp2,

[...]

> +        if (*len == 0) {
> +            /*
> +             * All data has been mapped, but the SGL contains additional
> +             * segments and/or descriptors. The controller might accept
> +             * ignoring the rest of the SGL.
> +             */
> +            uint16_t sgls = le16_to_cpu(n->id_ctrl.sgls);
> +            if (sgls & NVME_CTRL_SGLS_EXCESS_LENGTH) {

Hi,

Coverity reports that this condition is always false because
NVME_CTRL_SGLS_EXCESS_LENGTH is (1 << 18) and thus won’t fit in a uint16_t.

I think the problem is that n->id_ctrl.sgls is a uint32_t, so sgls
should be uint32_t and we should use le32_to_cpu().  (Which technically
is a second problem, namely to access a 32 bit LE field as a 16 bit
field.  That will give wrong results on big endian hosts.)

Max