| Message ID | 20200204095208.269131-27-k.jensen@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvme: support NVMe v1.3d, SGLs and multiple namespaces | expand |
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.
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).
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 <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). Looks like a good idea. Best regards, Maxim Levitsky
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) 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 = &n->namespace; ns->params.nsid = 1; + ns->params.lbads = BDRV_SECTOR_BITS; if (nvme_ns_setup(n, ns, &local_err)) { error_propagate_prepend(errp, local_err, "nvme_ns_setup: ");
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(-)