Re: [PULL v2 19/38] hw/block/nvme: align zoned.zasl with mdts

[Peter Maydell]

On Tue, 9 Mar 2021 at 11:45, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@samsung.com>
>
> ZASL (Zone Append Size Limit) is defined exactly like MDTS (Maximum Data
> Transfer Size), that is, it is a value in units of the minimum memory
> page size (CAP.MPSMIN) and is reported as a power of two.
>
> The 'mdts' nvme device parameter is specified as in the spec, but the
> 'zoned.append_size_limit' parameter is specified in bytes. This is
> suboptimal for a number of reasons:
>
>   1. It is just plain confusing wrt. the definition of mdts.
>   2. There is a lot of complexity involved in validating the value; it
>      must be a power of two, it should be larger than 4k, if it is zero
>      we set it internally to mdts, but still report it as zero.
>   3. While "hw/block/nvme: improve invalid zasl value reporting"
>      slightly improved the handling of the parameter, the validation is
>      still wrong; it does not depend on CC.MPS, it depends on
>      CAP.MPSMIN. And we are not even checking that it is actually less
>      than or equal to MDTS, which is kinda the *one* condition it must
>      satisfy.
>
> Fix this by defining zasl exactly like mdts and checking the one thing
> that it must satisfy (that it is less than or equal to mdts). Also,
> change the default value from 128KiB to 0 (aka, whatever mdts is).

> @@ -2144,10 +2142,9 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest 
> *req, bool append,
>                  goto invalid;
>              }
>
> -            if (nvme_l2b(ns, nlb) > (n->page_size << n->zasl)) {
> -                trace_pci_nvme_err_append_too_large(slba, nlb, n->zasl);
> -                status = NVME_INVALID_FIELD;
> -                goto invalid;
> +            if (n->params.zasl && data_size > n->page_size << 
> n->params.zasl) {
> +                trace_pci_nvme_err_zasl(data_size);
> +                return NVME_INVALID_FIELD | NVME_DNR;
>              }
>
>              slba = zone->w_ptr;

Hi; Coverity points out a possible overflow here (CID 1450756):
n->page_size is a uint32_t, and n->params.zasl is a uint8_t, so
the "n->page_size << n->params.zasl" will be done as 32-bit arithmetic;
but it is then compared against a uint64_t data_size.

Is this an overflow that can never happen (ie a false positive), or
should the RHS of the comparison be done as 64-bit arithmetic by
adding a cast ?

thanks
-- PMM