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

[Maxim Levitsky]

On Thu, 2020-02-06 at 08:24 +0100, Klaus Birkelund Jensen wrote:
> 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 
> > > ---
> > >  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 = >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).
Looks like a good idea.

Best regards,
Maxim Levitsky

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

[Klaus Birkelund Jensen]

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 
> > ---
> >  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 = >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).

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

[Keith Busch]

On Tue, Feb 04, 2020 at 10:52:08AM +0100, Klaus Jensen wrote:
> Signed-off-by: Klaus Jensen 
> ---
>  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 = >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.

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

[Klaus Jensen]

Signed-off-by: Klaus Jensen 
---
 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 = >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)
 
 typedef struct NvmeNamespaceParams {
 uint32_t nsid;
+uint8_t  lbads;
 } NvmeNamespaceParams;
 
 typedef struct NvmeNamespace {
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5fe2e2fe1fa9..67cd8d9d65fe 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2598,6 +2598,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 if (n->namespace.blk) {
 ns = >namespace;
 ns->params.nsid = 1;
+ns->params.lbads = BDRV_SECTOR_BITS;
 
 if (nvme_ns_setup(n, ns, _err)) {
 error_propagate_prepend(errp, local_err, "nvme_ns_setup: ");
-- 
2.25.0