Re: [Qemu-devel] [PULL 2/5] block/nvme: support larger that 512 bytes sector devices

[Maxim Levitsky]

On Mon, 2019-07-29 at 15:25 +0200, Max Reitz wrote:
> On 29.07.19 15:16, Peter Maydell wrote:
> > On Mon, 22 Jul 2019 at 18:26, Max Reitz <address@hidden> wrote:
> > > 
> > > From: Maxim Levitsky <address@hidden>
> > > 
> > > Currently the driver hardcodes the sector size to 512,
> > > and doesn't check the underlying device. Fix that.
> > > 
> > > Also fail if underlying nvme device is formatted with metadata
> > > as this needs special support.
> > > 
> > > Signed-off-by: Maxim Levitsky <address@hidden>
> > > Message-id: address@hidden
> > > Signed-off-by: Max Reitz <address@hidden>
> > > +static int64_t nvme_get_blocksize(BlockDriverState *bs)
> > > +{
> > > +    BDRVNVMeState *s = bs->opaque;
> > > +    assert(s->blkshift >= BDRV_SECTOR_BITS);
> > > +    return 1 << s->blkshift;
> > > +}
> > 
> > Hi -- Coverity points out here that we calculate the
> > "1 << s->blkshift" as a 32-bit shift, but then return an
> > int64_t type (CID 1403771).
> > 
> > Can the blkshift ever really be 31 or more ?
> > 
> > The types here seem weird anyway -- we return an int64_t,
> > but the only user of this is nvme_probe_blocksizes(),
> > which uses the value only to set BlockSizes::phys and ::log,
> > both of which are of type "uint32_t". That leads me to think
> > that the right return type for the function is uint32_t.
> > 
> > PS: this is the only Coverity issue currently outstanding so
> > if it's a trivial fix it might be nice to put it into rc3.
> 
> Maxim, what do you think?

Fully agree with that.

> 
> How about we let nvme_identify() limit blkshift to something sane and
> then return a uint32_t here?
> 
> In theory it would be limited by page_size, and that has a maximum value
> of 2^27.  In practice, though, that limit is checked by another 32-bit
> shift...

2^27 is the maximum NVME page size, but in theory, but only in theory,
you can have blocks larger that a page size, you will just have to give the 
controller
more that one page, even when you read a single block.

But like I said in the other mail, Linux doesn't support at all block size > 
cpu page size which is almost always 4K,
thus 12 is the practical limit for the block size these days, and of course 
both 27 and 31 is well above this.

So I'll send a patch to fix this today or tomorrow.

Best regards,
        Maxim Levitsky

> Max
>