Re: [PATCH v7 45/48] nvme: support multiple namespaces

[Klaus Birkelund Jensen <its@irrelevant.dk>]

On Apr 15 09:38, Philippe Mathieu-Daudé wrote:
> On 4/15/20 7:51 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@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.jensen@cnexlabs.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Keith Busch <kbusch@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  |   8 +-
> >   6 files changed, 396 insertions(+), 111 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
> > @@ -7,7 +7,7 @@ common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
> >   common-obj-$(CONFIG_XEN) += xen-block.o
> >   common-obj-$(CONFIG_ECC) += ecc.o
> >   common-obj-$(CONFIG_ONENAND) += onenand.o
> > -common-obj-$(CONFIG_NVME_PCI) += nvme.o
> > +common-obj-$(CONFIG_NVME_PCI) += nvme.o nvme-ns.o
> >   common-obj-$(CONFIG_SWIM) += swim.o
> >   common-obj-$(CONFIG_SH4) += tc58128.o
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > new file mode 100644
> > index 000000000000..bd64d4a94632
> > --- /dev/null
> > +++ b/hw/block/nvme-ns.c
> > @@ -0,0 +1,157 @@
> 
> Missing copyright + license.
> 

Fixed.

> > +
> > +    switch (n->conf.wce) {
> > +    case ON_OFF_AUTO_ON:
> > +        n->features.volatile_wc = 1;
> > +        break;
> > +    case ON_OFF_AUTO_OFF:
> > +        n->features.volatile_wc = 0;
> 
> Missing 'break'?
> 

Ouch. Fixed.

> > +    case ON_OFF_AUTO_AUTO:
> > +        n->features.volatile_wc = blk_enable_write_cache(ns->blk);
> > +        break;
> > +    default:
> > +        abort();
> > +    }
> > +
> > +    blk_set_enable_write_cache(ns->blk, n->features.volatile_wc);
> > +
> > +    return 0;
> > +}
> > +
> > +static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp)
> > +{
> > +    if (!ns->blk) {
> > +        error_setg(errp, "block backend not configured");
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> > +{
> > +    if (nvme_ns_check_constraints(ns, errp)) {
> > +        return -1;
> > +    }
> > +
> > +    if (nvme_ns_init_blk(n, ns, &n->id_ctrl, errp)) {
> > +        return -1;
> > +    }
> > +
> > +    nvme_ns_init(ns);
> > +    if (nvme_register_namespace(n, ns, errp)) {
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static void nvme_ns_realize(DeviceState *dev, Error **errp)
> > +{
> > +    NvmeNamespace *ns = NVME_NS(dev);
> > +    BusState *s = qdev_get_parent_bus(dev);
> > +    NvmeCtrl *n = NVME(s->parent);
> > +    Error *local_err = NULL;
> > +
> > +    if (nvme_ns_setup(n, ns, &local_err)) {
> > +        error_propagate_prepend(errp, local_err,
> > +                                "could not setup namespace: ");
> > +        return;
> > +    }
> > +}
> > +
> > +static Property nvme_ns_props[] = {
> > +    DEFINE_NVME_NS_PROPERTIES(NvmeNamespace, params),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void nvme_ns_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +
> > +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > +
> > +    dc->bus_type = TYPE_NVME_BUS;
> > +    dc->realize = nvme_ns_realize;
> > +    device_class_set_props(dc, nvme_ns_props);
> > +    dc->desc = "virtual nvme namespace";
> 
> "Virtual NVMe namespace"?
> 

Fixed.

> > +}
> > +
> > +static void nvme_ns_instance_init(Object *obj)
> > +{
> > +    NvmeNamespace *ns = NVME_NS(obj);
> > +    char *bootindex = g_strdup_printf("/namespace@%d,0", ns->params.nsid);
> > +
> > +    device_add_bootindex_property(obj, &ns->bootindex, "bootindex",
> > +                                  bootindex, DEVICE(obj), &error_abort);
> > +
> > +    g_free(bootindex);
> > +}
> > +
> > +static const TypeInfo nvme_ns_info = {
> > +    .name = TYPE_NVME_NS,
> > +    .parent = TYPE_DEVICE,
> > +    .class_init = nvme_ns_class_init,
> > +    .instance_size = sizeof(NvmeNamespace),
> > +    .instance_init = nvme_ns_instance_init,
> > +};
> > +
> > +static void nvme_ns_register_types(void)
> > +{
> > +    type_register_static(&nvme_ns_info);
> > +}
> > +
> > +type_init(nvme_ns_register_types)
> > diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> > new file mode 100644
> > index 000000000000..3c3651d485d0
> > --- /dev/null
> > +++ b/hw/block/nvme-ns.h
> > @@ -0,0 +1,60 @@
> 
> Missing copyright + license.
> 

Fixed.

> > +#ifndef NVME_NS_H
> > +#define NVME_NS_H
> > +
> > +#define TYPE_NVME_NS "nvme-ns"
> > +#define NVME_NS(obj) \
> > +    OBJECT_CHECK(NvmeNamespace, (obj), TYPE_NVME_NS)
> > +
> > +#define DEFINE_NVME_NS_PROPERTIES(_state, _props) \
> > +    DEFINE_PROP_DRIVE("drive", _state, blk), \
> > +    DEFINE_PROP_UINT32("nsid", _state, _props.nsid, 0)
> > +
> > +typedef struct NvmeNamespaceParams {
> > +    uint32_t nsid;
> > +} NvmeNamespaceParams;
> > +
> > +typedef struct NvmeNamespace {
> > +    DeviceState  parent_obj;
> > +    BlockBackend *blk;
> > +    int32_t      bootindex;
> > +    int64_t      size;
> > +
> > +    NvmeIdNs            id_ns;
> > +    NvmeNamespaceParams params;
> > +} NvmeNamespace;
> > +
> > +static inline uint32_t nvme_nsid(NvmeNamespace *ns)
> > +{
> > +    if (ns) {
> > +        return ns->params.nsid;
> > +    }
> > +
> > +    return -1;
> 
> This case doesn't seem possible.
> 
> Maybe:
> 
>       assert(ns);
>       return ns->params.nsid;
> 

I have some later patches where this might happen in certain traces.
That's why I'm using a sentinel value. But I will see if I can clean it
up instead.

> > @@ -2607,26 +2666,28 @@ static int nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> >   static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >   {
> >       NvmeCtrl *n = NVME(pci_dev);
> > -    int i;
> > +    NvmeNamespace *ns;
> >       if (nvme_check_constraints(n, errp)) {
> >           return;
> >       }
> > +    qbus_create_inplace(&n->bus, sizeof(NvmeBus), TYPE_NVME_BUS,
> > +                        &pci_dev->qdev, n->parent_obj.qdev.id);
> > +
> >       nvme_init_state(n);
> > +    nvme_init_pci(n, pci_dev);
> > +    nvme_init_ctrl(n);
> 
> Argh you move these functions again... Why not put them in the correct place
> when you refactor nvme_realize()?
> 

"Argh" :) Yeah, I will do an effort to make this better!