On Tue, 2020-03-31 at 07:48 +0200, Klaus Birkelund Jensen wrote:
> On Mar 25 12:59, Maxim Levitsky wrote:
> > On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jen...@samsung.com>
> > >
> > > This adds support for multiple namespaces by introducing a new 'nvme-ns'
> > > device model. The nvme device creates a bus named from the device name
> > > ('id'). The nvme-ns devices then connect to this and registers
> > > themselves with the nvme device.
> > >
> > > This changes how an nvme device is created. Example with two namespaces:
> > >
> > > -drive file=nvme0n1.img,if=none,id=disk1
> > > -drive file=nvme0n2.img,if=none,id=disk2
> > > -device nvme,serial=deadbeef,id=nvme0
> > > -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
> > > -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
> > >
> > > The drive property is kept on the nvme device to keep the change
> > > backward compatible, but the property is now optional. Specifying a
> > > drive for the nvme device will always create the namespace with nsid 1.
> > >
> > > Signed-off-by: Klaus Jensen <klaus.jen...@cnexlabs.com>
> > > Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
> > > Reviewed-by: Keith Busch <kbu...@kernel.org>
> > > ---
> > > hw/block/Makefile.objs | 2 +-
> > > hw/block/nvme-ns.c | 157 +++++++++++++++++++++++++++
> > > hw/block/nvme-ns.h | 60 +++++++++++
> > > hw/block/nvme.c | 233 ++++++++++++++++++++++++++---------------
> > > hw/block/nvme.h | 47 ++++-----
> > > hw/block/trace-events | 4 +-
> > > 6 files changed, 389 insertions(+), 114 deletions(-)
> > > create mode 100644 hw/block/nvme-ns.c
> > > create mode 100644 hw/block/nvme-ns.h
> > >
> > > diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> > > index 4b4a2b338dc4..d9141d6a4b9b 100644
> > > --- a/hw/block/Makefile.objs
> > > +++ b/hw/block/Makefile.objs
> > > @@ -2518,9 +2561,6 @@ static void nvme_init_ctrl(NvmeCtrl *n)
> > > id->psd[0].mp = cpu_to_le16(0x9c4);
> > > id->psd[0].enlat = cpu_to_le32(0x10);
> > > id->psd[0].exlat = cpu_to_le32(0x4);
> > > - if (blk_enable_write_cache(n->conf.blk)) {
> > > - id->vwc = 1;
> > > - }
> >
> > Shouldn't that be kept? Assuming that user used the legacy 'drive' option,
> > and it had no write cache enabled.
> >
>
> When using the drive option we still end up calling the same code that
> handles the "new style" namespaces and that code will handle the write
> cache similary.
OK. That makes sense.
>
> > >
> > > n->bar.cap = 0;
> > > NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
> > > @@ -2533,25 +2573,34 @@ static void nvme_init_ctrl(NvmeCtrl *n)
> > > n->bar.intmc = n->bar.intms = 0;
> > > }
> > >
> > > -static int nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error
> > > **errp)
> > > +int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> > > {
> > > - int64_t bs_size;
> > > - NvmeIdNs *id_ns = &ns->id_ns;
> > > + uint32_t nsid = nvme_nsid(ns);
> > >
> > > - bs_size = blk_getlength(n->conf.blk);
> > > - if (bs_size < 0) {
> > > - error_setg_errno(errp, -bs_size, "blk_getlength");
> > > + if (nsid > NVME_MAX_NAMESPACES) {
> > > + error_setg(errp, "invalid nsid (must be between 0 and %d)",
> > > + NVME_MAX_NAMESPACES);
> > > return -1;
> > > }
> > >
> > > - id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> > > - n->ns_size = bs_size;
> > > + if (!nsid) {
> > > + for (int i = 1; i <= n->num_namespaces; i++) {
> > > + NvmeNamespace *ns = nvme_ns(n, i);
> > > + if (!ns) {
> > > + nsid = i;
> > > + break;
> > > + }
> > > + }
> >
> > This misses an edge error case, where all the namespaces are allocated.
> > Yes, it would be insane to allocate all 256 namespaces but still.
> >
>
> Impressive catch! Fixed!
Thanks!
>
> >
> > > + } else {
> > > + if (n->namespaces[nsid - 1]) {
> > > + error_setg(errp, "nsid must be unique");
> >
> > I''l would change that error message to something like
> > "namespace id %d is already in use" or something like that.
> >
>
> Done.
>
Best regards,
Maxim Levitsky
