Re: [PATCH v5 26/26] nvme: make lba data size configurable

From: Klaus Birkelund Jensen <k.jensen@samsung.com>
To: Keith Busch <kbusch@kernel.org>
Cc: Kevin Wolf <kwolf@redhat.com>,
	Beata Michalska <beata.michalska@linaro.org>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org,
	Max Reitz <mreitz@redhat.com>, Klaus Jensen <its@irrelevant.dk>,
	Javier Gonzalez <javier.gonz@samsung.com>
Subject: Re: [PATCH v5 26/26] nvme: make lba data size configurable
Date: Thu, 6 Feb 2020 08:24:21 +0100	[thread overview]
Message-ID: <20200206072421.h3vjwelfod2y7nax@apples.localdomain> (raw)
In-Reply-To: <20200204164358.GC6823@redsun51.ssa.fujisawa.hgst.com>

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

On Feb  5 01:43, Keith Busch wrote:
> On Tue, Feb 04, 2020 at 10:52:08AM +0100, Klaus Jensen wrote:
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme-ns.c | 2 +-
> >  hw/block/nvme-ns.h | 4 +++-
> >  hw/block/nvme.c    | 1 +
> >  3 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index 0e5be44486f4..981d7101b8f2 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
> > @@ -18,7 +18,7 @@ static int nvme_ns_init(NvmeNamespace *ns)
> >  {
> >      NvmeIdNs *id_ns = &ns->id_ns;
> >  
> > -    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> > +    id_ns->lbaf[0].ds = ns->params.lbads;
> >      id_ns->nuse = id_ns->ncap = id_ns->nsze =
> >          cpu_to_le64(nvme_ns_nlbas(ns));
> >  
> > diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> > index b564bac25f6d..f1fe4db78b41 100644
> > --- a/hw/block/nvme-ns.h
> > +++ b/hw/block/nvme-ns.h
> > @@ -7,10 +7,12 @@
> >  
> >  #define DEFINE_NVME_NS_PROPERTIES(_state, _props) \
> >      DEFINE_PROP_DRIVE("drive", _state, blk), \
> > -    DEFINE_PROP_UINT32("nsid", _state, _props.nsid, 0)
> > +    DEFINE_PROP_UINT32("nsid", _state, _props.nsid, 0), \
> > +    DEFINE_PROP_UINT8("lbads", _state, _props.lbads, BDRV_SECTOR_BITS)
> 
> I think we need to validate the parameter is between 9 and 12 before
> trusting it can be used safely.
> 
> Alternatively, add supported formats to the lbaf array and let the host
> decide on a live system with the 'format' command.

The device does not yet support Format NVM, but we have a patch ready
for that to be submitted with a new series when this is merged.

For now, while it does not support Format, I will change this patch such
that it defaults to 9 (BRDV_SECTOR_BITS) and only accept 12 as an
alternative (while always keeping the number of formats available to 1).

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]