On Apr 15 09:38, Philippe Mathieu-Daudé wrote:
> On 4/15/20 7:51 AM, 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 | 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!
