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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

On Mar 12 13:07, Peter Maydell wrote:
> 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!

I think a cast is in order. I will get a fix out.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]