[Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces

Klaus Birkelund Jensen posted 16 patches 6 years, 7 months ago
Maintainers: Fam Zheng <fam@euphon.net>, Keith Busch <keith.busch@intel.com>, Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>
There is a newer version of this series
[Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces
Posted by Klaus Birkelund Jensen 6 years, 7 months ago
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

A maximum of 256 namespaces can be configured.

Signed-off-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>
---
 hw/block/Makefile.objs |   2 +-
 hw/block/nvme-ns.c     | 139 +++++++++++++++++++++++++++++++++
 hw/block/nvme-ns.h     |  35 +++++++++
 hw/block/nvme.c        | 169 ++++++++++++++++-------------------------
 hw/block/nvme.h        |  29 ++++---
 hw/block/trace-events  |   1 +
 6 files changed, 255 insertions(+), 120 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 f5f643f0cc06..d44a2f4b780d 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
 
 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..11b594467991
--- /dev/null
+++ b/hw/block/nvme-ns.c
@@ -0,0 +1,139 @@
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qemu/cutils.h"
+#include "qemu/log.h"
+#include "hw/block/block.h"
+#include "hw/pci/msix.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
+#include "qapi/error.h"
+
+#include "hw/qdev-core.h"
+
+#include "nvme.h"
+#include "nvme-ns.h"
+
+static uint64_t nvme_ns_calc_blks(NvmeNamespace *ns)
+{
+    return ns->size / nvme_ns_lbads_bytes(ns);
+}
+
+static void nvme_ns_init_identify(NvmeIdNs *id_ns)
+{
+    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
+}
+
+static int nvme_ns_init(NvmeNamespace *ns)
+{
+    uint64_t ns_blks;
+    NvmeIdNs *id_ns = &ns->id_ns;
+
+    nvme_ns_init_identify(id_ns);
+
+    ns_blks = nvme_ns_calc_blks(ns);
+    id_ns->nuse = id_ns->ncap = id_ns->nsze = cpu_to_le64(ns_blks);
+
+    return 0;
+}
+
+static int nvme_ns_init_blk(NvmeNamespace *ns, NvmeIdCtrl *id, Error **errp)
+{
+    blkconf_blocksizes(&ns->conf);
+
+    if (!blkconf_apply_backend_options(&ns->conf,
+        blk_is_read_only(ns->conf.blk), false, errp)) {
+        return 1;
+    }
+
+    ns->size = blk_getlength(ns->conf.blk);
+    if (ns->size < 0) {
+        error_setg_errno(errp, -ns->size, "blk_getlength");
+        return 1;
+    }
+
+    if (!blk_enable_write_cache(ns->conf.blk)) {
+        id->vwc = 0;
+    }
+
+    return 0;
+}
+
+static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp)
+{
+    if (!ns->conf.blk) {
+        error_setg(errp, "nvme-ns: block backend not configured");
+        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_check_constraints(ns, &local_err)) {
+        error_propagate_prepend(errp, local_err,
+            "nvme_ns_check_constraints: ");
+        return;
+    }
+
+    if (nvme_ns_init_blk(ns, &n->id_ctrl, &local_err)) {
+        error_propagate_prepend(errp, local_err, "nvme_ns_init_blk: ");
+        return;
+    }
+
+    nvme_ns_init(ns);
+    if (nvme_register_namespace(n, ns, &local_err)) {
+        error_propagate_prepend(errp, local_err, "nvme_register_namespace: ");
+        return;
+    }
+}
+
+static Property nvme_ns_props[] = {
+    DEFINE_BLOCK_PROPERTIES(NvmeNamespace, conf),
+    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;
+    dc->props = nvme_ns_props;
+    dc->desc = "virtual nvme namespace";
+}
+
+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->conf.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..f563bb14eceb
--- /dev/null
+++ b/hw/block/nvme-ns.h
@@ -0,0 +1,35 @@
+#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_UINT32("nsid", _state, _props.nsid, 0)
+
+typedef struct NvmeNamespaceParams {
+    uint32_t nsid;
+} NvmeNamespaceParams;
+
+typedef struct NvmeNamespace {
+    DeviceState parent_obj;
+    BlockConf   conf;
+    int64_t     size;
+
+    NvmeIdNs            id_ns;
+    NvmeNamespaceParams params;
+} NvmeNamespace;
+
+static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
+{
+    NvmeIdNs *id = &ns->id_ns;
+    return id->lbaf[NVME_ID_NS_FLBAS_INDEX(id->flbas)].ds;
+}
+
+static inline size_t nvme_ns_lbads_bytes(NvmeNamespace *ns)
+{
+    return 1 << nvme_ns_lbads(ns);
+}
+
+#endif /* NVME_NS_H */
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 6bf62952dd13..6448798132d6 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -17,7 +17,8 @@
 /**
  * Usage: add options:
  *     -drive file=<file>,if=none,id=<drive_id>
- *     -device nvme,drive=<drive_id>,serial=<serial>,id=<id[optional]>
+ *     -device nvme,serial=<serial>,id=nvme0
+ *     -device nvme-ns,drive=<drive_id>,bus=nvme0,nsid=1
  *
  * Advanced optional options:
  *
@@ -41,6 +42,7 @@
 
 #include "trace.h"
 #include "nvme.h"
+#include "nvme-ns.h"
 
 #define NVME_MAX_QS PCI_MSIX_FLAGS_QSIZE
 #define NVME_TEMPERATURE 0x143
@@ -848,15 +850,16 @@ static void nvme_rw_cb(void *opaque, int ret)
     NvmeSQueue *sq = req->sq;
     NvmeCtrl *n = sq->ctrl;
     NvmeCQueue *cq = n->cq[sq->cqid];
+    NvmeNamespace *ns = req->ns;
 
     QTAILQ_REMOVE(&req->blk_req_tailq, blk_req, tailq_entry);
 
-    trace_nvme_rw_cb(req->cqe.cid, req->cmd.nsid);
+    trace_nvme_rw_cb(req->cqe.cid, ns->params.nsid);
 
     if (!ret) {
-        block_acct_done(blk_get_stats(n->conf.blk), &blk_req->acct);
+        block_acct_done(blk_get_stats(ns->conf.blk), &blk_req->acct);
     } else {
-        block_acct_failed(blk_get_stats(n->conf.blk), &blk_req->acct);
+        block_acct_failed(blk_get_stats(ns->conf.blk), &blk_req->acct);
         NVME_GUEST_ERR(nvme_err_internal_dev_error, "block request failed: %s",
             strerror(-ret));
         req->status = NVME_INTERNAL_DEV_ERROR | NVME_DNR;
@@ -871,6 +874,7 @@ static void nvme_rw_cb(void *opaque, int ret)
 
 static uint16_t nvme_flush(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
+    NvmeNamespace *ns = req->ns;
     NvmeBlockBackendRequest *blk_req = nvme_blk_req_get(n, req, NULL);
     if (!blk_req) {
         NVME_GUEST_ERR(nvme_err_internal_dev_error, "nvme_blk_req_get: %s",
@@ -878,9 +882,9 @@ static uint16_t nvme_flush(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         return NVME_INTERNAL_DEV_ERROR;
     }
 
-    block_acct_start(blk_get_stats(n->conf.blk), &blk_req->acct, 0,
+    block_acct_start(blk_get_stats(ns->conf.blk), &blk_req->acct, 0,
          BLOCK_ACCT_FLUSH);
-    blk_req->aiocb = blk_aio_flush(n->conf.blk, nvme_rw_cb, blk_req);
+    blk_req->aiocb = blk_aio_flush(ns->conf.blk, nvme_rw_cb, blk_req);
 
     QTAILQ_INSERT_TAIL(&req->blk_req_tailq, blk_req, tailq_entry);
 
@@ -890,6 +894,7 @@ static uint16_t nvme_flush(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
     NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
+    NvmeNamespace *ns = req->ns;
     NvmeBlockBackendRequest *blk_req;
     const uint8_t lbads = nvme_ns_lbads(req->ns);
     uint64_t slba = le64_to_cpu(rw->slba);
@@ -909,10 +914,10 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         return NVME_INTERNAL_DEV_ERROR;
     }
 
-    block_acct_start(blk_get_stats(n->conf.blk), &blk_req->acct, 0,
+    block_acct_start(blk_get_stats(ns->conf.blk), &blk_req->acct, 0,
         BLOCK_ACCT_WRITE);
 
-    blk_req->aiocb = blk_aio_pwrite_zeroes(n->conf.blk, offset, count,
+    blk_req->aiocb = blk_aio_pwrite_zeroes(ns->conf.blk, offset, count,
         BDRV_REQ_MAY_UNMAP, nvme_rw_cb, blk_req);
 
     QTAILQ_INSERT_TAIL(&req->blk_req_tailq, blk_req, tailq_entry);
@@ -929,7 +934,7 @@ static uint16_t nvme_rw_check_req(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
     uint32_t data_size = req->nlb << nvme_ns_lbads(ns);
 
     if (unlikely((req->slba + req->nlb) > ns->id_ns.nsze)) {
-        block_acct_invalid(blk_get_stats(n->conf.blk), req->is_write ?
+        block_acct_invalid(blk_get_stats(ns->conf.blk), req->is_write ?
             BLOCK_ACCT_WRITE : BLOCK_ACCT_READ);
         trace_nvme_err_invalid_lba_range(req->slba, req->nlb, ns->id_ns.nsze);
         return NVME_LBA_RANGE | NVME_DNR;
@@ -950,18 +955,19 @@ static void nvme_blk_submit_dma(NvmeCtrl *n, NvmeBlockBackendRequest *blk_req,
     BlockCompletionFunc *cb)
 {
     NvmeRequest *req = blk_req->req;
+    NvmeNamespace *ns = req->ns;
 
     if (req->is_write) {
-        dma_acct_start(n->conf.blk, &blk_req->acct, blk_req->qsg,
+        dma_acct_start(ns->conf.blk, &blk_req->acct, blk_req->qsg,
             BLOCK_ACCT_WRITE);
 
-        blk_req->aiocb = dma_blk_write(n->conf.blk, blk_req->qsg,
+        blk_req->aiocb = dma_blk_write(ns->conf.blk, blk_req->qsg,
             blk_req->blk_offset, BDRV_SECTOR_SIZE, cb, blk_req);
     } else {
-        dma_acct_start(n->conf.blk, &blk_req->acct, blk_req->qsg,
+        dma_acct_start(ns->conf.blk, &blk_req->acct, blk_req->qsg,
             BLOCK_ACCT_READ);
 
-        blk_req->aiocb = dma_blk_read(n->conf.blk, blk_req->qsg,
+        blk_req->aiocb = dma_blk_read(ns->conf.blk, blk_req->qsg,
             blk_req->blk_offset, BDRV_SECTOR_SIZE, cb, blk_req);
     }
 }
@@ -970,21 +976,22 @@ static void nvme_blk_submit_cmb(NvmeCtrl *n, NvmeBlockBackendRequest *blk_req,
     BlockCompletionFunc *cb)
 {
     NvmeRequest *req = blk_req->req;
+    NvmeNamespace *ns = req->ns;
 
     qemu_iovec_init(&blk_req->iov, blk_req->qsg->nsg);
     dma_to_cmb(n, blk_req->qsg, &blk_req->iov);
 
     if (req->is_write) {
-        block_acct_start(blk_get_stats(n->conf.blk), &blk_req->acct,
+        block_acct_start(blk_get_stats(ns->conf.blk), &blk_req->acct,
             blk_req->iov.size, BLOCK_ACCT_WRITE);
 
-        blk_req->aiocb = blk_aio_pwritev(n->conf.blk, blk_req->blk_offset,
+        blk_req->aiocb = blk_aio_pwritev(ns->conf.blk, blk_req->blk_offset,
             &blk_req->iov, 0, cb, blk_req);
     } else {
-        block_acct_start(blk_get_stats(n->conf.blk), &blk_req->acct,
+        block_acct_start(blk_get_stats(ns->conf.blk), &blk_req->acct,
             blk_req->iov.size, BLOCK_ACCT_READ);
 
-        blk_req->aiocb = blk_aio_preadv(n->conf.blk, blk_req->blk_offset,
+        blk_req->aiocb = blk_aio_preadv(ns->conf.blk, blk_req->blk_offset,
             &blk_req->iov, 0, cb, blk_req);
     }
 }
@@ -1042,7 +1049,8 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         return NVME_INVALID_NSID | NVME_DNR;
     }
 
-    req->ns = &n->namespace;
+    req->ns = n->namespaces[nsid - 1];
+
     switch (cmd->opcode) {
     case NVME_CMD_FLUSH:
         return nvme_flush(n, cmd, req);
@@ -1302,7 +1310,7 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         return NVME_INVALID_NSID | NVME_DNR;
     }
 
-    ns = &n->namespace;
+    ns = n->namespaces[nsid - 1];
 
     return nvme_dma_read(n, (uint8_t *) &ns->id_ns, sizeof(ns->id_ns), cmd,
         req);
@@ -1444,6 +1452,8 @@ static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd,
 
 static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
+    NvmeNamespace *ns = req->ns;
+
     uint32_t dw10 = le32_to_cpu(cmd->cdw10);
     uint32_t dw11 = le32_to_cpu(cmd->cdw11);
     uint32_t result;
@@ -1464,7 +1474,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         result = cpu_to_le32(n->features.err_rec);
         break;
     case NVME_VOLATILE_WRITE_CACHE:
-        result = blk_enable_write_cache(n->conf.blk);
+        result = blk_enable_write_cache(ns->conf.blk);
         trace_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
         break;
     case NVME_NUMBER_OF_QUEUES:
@@ -1518,6 +1528,8 @@ static uint16_t nvme_set_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd,
 
 static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
+    NvmeNamespace *ns = req->ns;
+
     uint32_t dw10 = le32_to_cpu(cmd->cdw10);
     uint32_t dw11 = le32_to_cpu(cmd->cdw11);
 
@@ -1532,7 +1544,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         }
         break;
     case NVME_VOLATILE_WRITE_CACHE:
-        blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
+        blk_set_enable_write_cache(ns->conf.blk, dw11 & 1);
         break;
     case NVME_NUMBER_OF_QUEUES:
         if (n->qs_created > 2) {
@@ -1835,7 +1847,9 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
     NvmeAsyncEvent *event;
     int i;
 
-    blk_drain(n->conf.blk);
+    for (int i = 0; i < n->num_namespaces; i++) {
+        blk_drain(n->namespaces[i]->conf.blk);
+    }
 
     for (i = 0; i < n->params.num_queues; i++) {
         if (n->sq[i] != NULL) {
@@ -1858,7 +1872,10 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
         g_free(event);
     }
 
-    blk_flush(n->conf.blk);
+    for (int i = 0; i < n->num_namespaces; i++) {
+        blk_flush(n->namespaces[i]->conf.blk);
+    }
+
     n->bar.cc = 0;
     n->outstanding_aers = 0;
 }
@@ -2280,8 +2297,8 @@ static int nvme_check_constraints(NvmeCtrl *n, Error **errp)
 {
     NvmeParams *params = &n->params;
 
-    if (!n->conf.blk) {
-        error_setg(errp, "nvme: block backend not configured");
+    if (!n->parent_obj.qdev.id) {
+        error_setg(errp, "nvme: invalid 'id' parameter");
         return 1;
     }
 
@@ -2298,20 +2315,9 @@ static int nvme_check_constraints(NvmeCtrl *n, Error **errp)
     return 0;
 }
 
-static int nvme_init_blk(NvmeCtrl *n, Error **errp)
-{
-    blkconf_blocksizes(&n->conf);
-    if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
-        false, errp)) {
-        return 1;
-    }
-
-    return 0;
-}
-
 static void nvme_init_state(NvmeCtrl *n)
 {
-    n->num_namespaces = 1;
+    n->num_namespaces = 0;
     n->reg_size = pow2ceil(0x1004 + 2 * (n->params.num_queues + 1) * 4);
     n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
     n->sq = g_new0(NvmeSQueue *, n->params.num_queues);
@@ -2404,11 +2410,7 @@ static void nvme_init_ctrl(NvmeCtrl *n)
     id->cqes = (0x4 << 4) | 0x4;
     id->nn = cpu_to_le32(n->num_namespaces);
     id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
-
-    if (blk_enable_write_cache(n->conf.blk)) {
-        id->vwc = 1;
-    }
-
+    id->vwc = 1;
     id->sgls = cpu_to_le32(0x1);
 
     strcpy((char *) id->subnqn, "nqn.2014-08.org.nvmexpress:uuid:");
@@ -2430,52 +2432,26 @@ static void nvme_init_ctrl(NvmeCtrl *n)
     n->bar.intmc = n->bar.intms = 0;
 }
 
-static uint64_t nvme_ns_calc_blks(NvmeCtrl *n, NvmeNamespace *ns)
-{
-    return n->ns_size / nvme_ns_lbads_bytes(ns);
-}
-
-static void nvme_ns_init_identify(NvmeCtrl *n, NvmeIdNs *id_ns)
-{
-    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
-    id_ns->ncap  = id_ns->nuse = id_ns->nsze =
-        cpu_to_le64(n->ns_size >>
-            id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(id_ns->flbas)].ds);
-}
-
-static int nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
-{
-    uint64_t ns_blks;
-    NvmeIdNs *id_ns = &ns->id_ns;
-
-    nvme_ns_init_identify(n, id_ns);
-
-    ns_blks = nvme_ns_calc_blks(n, ns);
-    id_ns->nuse = id_ns->ncap = id_ns->nsze = cpu_to_le64(ns_blks);
-
-    return 0;
-}
-
-static int nvme_init_namespaces(NvmeCtrl *n, Error **errp)
+int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
 {
-    int64_t bs_size;
-    Error *local_err = NULL;
-    NvmeNamespace *ns = &n->namespace;
+    uint32_t nsid = ns->params.nsid;
 
-    bs_size = blk_getlength(n->conf.blk);
-    if (bs_size < 0) {
-        error_setg_errno(errp, -bs_size, "blk_getlength");
+    if (nsid == 0 || nsid > NVME_MAX_NAMESPACES) {
+        error_setg(errp, "invalid namespace id");
         return 1;
     }
 
-    n->ns_size = bs_size / (uint64_t) n->num_namespaces;
-
-    if (nvme_init_namespace(n, ns, &local_err)) {
-        error_propagate_prepend(errp, local_err,
-            "nvme_init_namespace: ");
+    if (n->namespaces[nsid - 1]) {
+        error_setg(errp, "namespace ids must be unique");
         return 1;
     }
 
+    trace_nvme_register_namespace(nsid);
+
+    n->namespaces[nsid - 1] = ns;
+    n->num_namespaces++;
+    n->id_ctrl.nn++;
+
     return 0;
 }
 
@@ -2489,19 +2465,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
 
-    nvme_init_state(n);
-
-    if (nvme_init_blk(n, &local_err)) {
-        error_propagate_prepend(errp, local_err, "nvme_init_blk: ");
-        return;
-    }
-
-    if (nvme_init_namespaces(n, &local_err)) {
-        error_propagate_prepend(errp, local_err,
-            "nvme_init_namespaces: ");
-        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);
 }
@@ -2524,7 +2491,6 @@ static void nvme_exit(PCIDevice *pci_dev)
 }
 
 static Property nvme_props[] = {
-    DEFINE_BLOCK_PROPERTIES(NvmeCtrl, conf),
     DEFINE_NVME_PROPERTIES(NvmeCtrl, params),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -2552,30 +2518,27 @@ static void nvme_class_init(ObjectClass *oc, void *data)
     dc->vmsd = &nvme_vmstate;
 }
 
-static void nvme_instance_init(Object *obj)
-{
-    NvmeCtrl *s = NVME(obj);
-
-    device_add_bootindex_property(obj, &s->conf.bootindex,
-                                  "bootindex", "/namespace@1,0",
-                                  DEVICE(obj), &error_abort);
-}
-
 static const TypeInfo nvme_info = {
     .name          = TYPE_NVME,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(NvmeCtrl),
     .class_init    = nvme_class_init,
-    .instance_init = nvme_instance_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_PCIE_DEVICE },
         { }
     },
 };
 
+static const TypeInfo nvme_bus_info = {
+    .name = TYPE_NVME_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(NvmeBus),
+};
+
 static void nvme_register_types(void)
 {
     type_register_static(&nvme_info);
+    type_register_static(&nvme_bus_info);
 }
 
 type_init(nvme_register_types)
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 1d52b183d263..9aff5c82a51b 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -2,6 +2,9 @@
 #define HW_NVME_H
 
 #include "block/nvme.h"
+#include "nvme-ns.h"
+
+#define NVME_MAX_NAMESPACES 256
 
 #define DEFINE_NVME_PROPERTIES(_state, _props) \
     DEFINE_PROP_STRING("serial", _state, _props.serial), \
@@ -86,9 +89,12 @@ typedef struct NvmeCQueue {
     QTAILQ_HEAD(, NvmeRequest) req_list;
 } NvmeCQueue;
 
-typedef struct NvmeNamespace {
-    NvmeIdNs        id_ns;
-} NvmeNamespace;
+#define TYPE_NVME_BUS "nvme-bus"
+#define NVME_BUS(obj) OBJECT_CHECK(NvmeBus, (obj), TYPE_NVME_BUS)
+
+typedef struct NvmeBus {
+    BusState parent_bus;
+} NvmeBus;
 
 #define TYPE_NVME "nvme"
 #define NVME(obj) \
@@ -99,7 +105,6 @@ typedef struct NvmeCtrl {
     MemoryRegion iomem;
     MemoryRegion ctrl_mem;
     NvmeBar      bar;
-    BlockConf    conf;
     NvmeParams   params;
 
     uint64_t    starttime_ms;
@@ -112,7 +117,6 @@ typedef struct NvmeCtrl {
     uint32_t    reg_size;
     uint32_t    num_namespaces;
     uint32_t    max_q_ents;
-    uint64_t    ns_size;
     uint8_t     outstanding_aers;
     uint32_t    cmbsz;
     uint32_t    cmbloc;
@@ -128,13 +132,15 @@ typedef struct NvmeCtrl {
     QSIMPLEQ_HEAD(, NvmeAsyncEvent) aer_queue;
 
     NvmeErrorLog    *elpes;
-    NvmeNamespace   namespace;
+    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES];
     NvmeSQueue      **sq;
     NvmeCQueue      **cq;
     NvmeSQueue      admin_sq;
     NvmeCQueue      admin_cq;
     NvmeFeatureVal  features;
     NvmeIdCtrl      id_ctrl;
+
+    NvmeBus bus;
 } NvmeCtrl;
 
 static inline bool nvme_rw_is_write(NvmeRequest *req)
@@ -148,15 +154,6 @@ static inline bool nvme_is_error(uint16_t status, uint16_t err)
     return (status & 0xfff) == err;
 }
 
-static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
-{
-    NvmeIdNs *id = &ns->id_ns;
-    return id->lbaf[NVME_ID_NS_FLBAS_INDEX(id->flbas)].ds;
-}
-
-static inline size_t nvme_ns_lbads_bytes(NvmeNamespace *ns)
-{
-    return 1 << nvme_ns_lbads(ns);
-}
+int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp);
 
 #endif /* HW_NVME_H */
diff --git a/hw/block/trace-events b/hw/block/trace-events
index b239e92294e4..0809c248aa54 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -29,6 +29,7 @@ hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads, uint32_t secs, int t
 
 # nvme.c
 # nvme traces for successful events
+nvme_register_namespace(uint32_t nsid) "nsid %"PRIu32""
 nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u"
 nvme_irq_pin(void) "pulsing IRQ pin"
 nvme_irq_masked(void) "IRQ is masked"
-- 
2.20.1


Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces
Posted by Ross Lagerwall 6 years, 5 months ago
On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
> 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
> 
> A maximum of 256 namespaces can be configured.
> 
snip
>   static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>   {
> +    NvmeNamespace *ns = req->ns;
> +
>       uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>       uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>       uint32_t result;
> @@ -1464,7 +1474,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>           result = cpu_to_le32(n->features.err_rec);
>           break;
>       case NVME_VOLATILE_WRITE_CACHE:
> -        result = blk_enable_write_cache(n->conf.blk);
> +        result = blk_enable_write_cache(ns->conf.blk);
>           trace_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>           break;

I tried this patch series by installing Windows with a single NVME 
controller having two namespaces. QEMU crashed in get_feature / 
NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.

nvme_get_feature / nvme_set_feature look wrong to me since I can't see 
how req->ns would have been set. Should they have similar code to 
nvme_io_cmd to set req->ns from cmd->nsid?

After working around this issue everything else seemed to be working 
well. Thanks for your work on this patch series.

Thanks,
-- 
Ross Lagerwall

Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces
Posted by Klaus Birkelund 6 years, 5 months ago
On Thu, Aug 22, 2019 at 02:18:05PM +0100, Ross Lagerwall wrote:
> On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
> 
> I tried this patch series by installing Windows with a single NVME
> controller having two namespaces. QEMU crashed in get_feature /
> NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.
> 

Hi Ross,

Good catch!

> nvme_get_feature / nvme_set_feature look wrong to me since I can't see how
> req->ns would have been set. Should they have similar code to nvme_io_cmd to
> set req->ns from cmd->nsid?

Definitely. I will fix that for v2.

> 
> After working around this issue everything else seemed to be working well.
> Thanks for your work on this patch series.
> 

And thank you for trying out my patches!


Cheers,
Klaus

Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces
Posted by Ross Lagerwall 6 years, 3 months ago
On 8/23/19 9:10 AM, Klaus Birkelund wrote:
> On Thu, Aug 22, 2019 at 02:18:05PM +0100, Ross Lagerwall wrote:
>> On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
>>
>> I tried this patch series by installing Windows with a single NVME
>> controller having two namespaces. QEMU crashed in get_feature /
>> NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.
>>
> 
> Hi Ross,
> 
> Good catch!
> 
>> nvme_get_feature / nvme_set_feature look wrong to me since I can't see how
>> req->ns would have been set. Should they have similar code to nvme_io_cmd to
>> set req->ns from cmd->nsid?
> 
> Definitely. I will fix that for v2.
> 
>>
>> After working around this issue everything else seemed to be working well.
>> Thanks for your work on this patch series.
>>
> 
> And thank you for trying out my patches!
> 

One more thing... it doesn't handle inactive namespaces properly so if you
have two namespaces with e.g. nsid=1 and nsid=3 QEMU ends up crashing in
certain situations. The patch below adds support for inactive namespaces.

Still hoping to see a v2 some day :-)

Thanks,
Ross

---------------------- 8-< ----------------------
nvme-ns: Allow inactive namespaces

The controller advertises the maximum namespace number but not all of these
slots may have proper namespaces. These are defined as inactive namespaces by
the spec.  Implement support for inactive namespaces instead of crashing.

Changes are needed in a few places:
* When identify_ns is used with an inactive namespace, the controller should
  return all zeroes.
* Only active namespaces should be returned by identify_ns_list.
* When the controller is unplugged, only cleanup active namespaces.
* Keep track of and advertise the maximum valid namespace number rather than
* the number of active namespaces.
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1b8a09853d..29ea5c2023 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1302,6 +1302,7 @@ static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
     NvmeNamespace *ns;
+    NvmeIdNs *id_ns, invalid_ns_id = {0};
     uint32_t nsid = le32_to_cpu(cmd->nsid);
 
     trace_nvme_identify_ns(nsid);
@@ -1312,9 +1313,13 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
     }
 
     ns = n->namespaces[nsid - 1];
+    if (ns) {
+        id_ns = &ns->id_ns;
+    } else {
+        id_ns = &invalid_ns_id;
+    }
 
-    return nvme_dma_read(n, (uint8_t *) &ns->id_ns, sizeof(ns->id_ns), cmd,
-        req);
+    return nvme_dma_read(n, (uint8_t *) id_ns, sizeof(*id_ns), cmd, req);
 }
 
 static uint16_t nvme_identify_ns_list(NvmeCtrl *n, NvmeCmd *cmd,
@@ -1330,7 +1335,7 @@ static uint16_t nvme_identify_ns_list(NvmeCtrl *n, NvmeCmd *cmd,
 
     list = g_malloc0(data_len);
     for (i = 0; i < n->num_namespaces; i++) {
-        if (i < min_nsid) {
+        if (i < min_nsid || !n->namespaces[i]) {
             continue;
         }
         list[j++] = cpu_to_le32(i + 1);
@@ -1861,7 +1866,9 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
     int i;
 
     for (i = 0; i < n->num_namespaces; i++) {
-        blk_drain(n->namespaces[i]->conf.blk);
+        if (n->namespaces[i]) {
+            blk_drain(n->namespaces[i]->conf.blk);
+        }
     }
 
     for (i = 0; i < n->params.num_queues; i++) {
@@ -1886,7 +1893,9 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
     }
 
     for (i = 0; i < n->num_namespaces; i++) {
-        blk_flush(n->namespaces[i]->conf.blk);
+        if (n->namespaces[i]) {
+            blk_flush(n->namespaces[i]->conf.blk);
+        }
     }
 
     n->bar.cc = 0;
@@ -2464,8 +2473,9 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
     trace_nvme_register_namespace(nsid);
 
     n->namespaces[nsid - 1] = ns;
-    n->num_namespaces++;
-    n->id_ctrl.nn++;
+    if (nsid > n->num_namespaces)
+        n->num_namespaces = nsid;
+    n->id_ctrl.nn = n->num_namespaces;
 
     return 0;
 }

Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces
Posted by Klaus Birkelund 6 years, 3 months ago
On Mon, Nov 04, 2019 at 08:46:29AM +0000, Ross Lagerwall wrote:
> On 8/23/19 9:10 AM, Klaus Birkelund wrote:
> > On Thu, Aug 22, 2019 at 02:18:05PM +0100, Ross Lagerwall wrote:
> >> On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
> >>
> >> I tried this patch series by installing Windows with a single NVME
> >> controller having two namespaces. QEMU crashed in get_feature /
> >> NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.
> >>
> > 
> > Hi Ross,
> > 
> > Good catch!
> > 
> >> nvme_get_feature / nvme_set_feature look wrong to me since I can't see how
> >> req->ns would have been set. Should they have similar code to nvme_io_cmd to
> >> set req->ns from cmd->nsid?
> > 
> > Definitely. I will fix that for v2.
> > 
> >>
> >> After working around this issue everything else seemed to be working well.
> >> Thanks for your work on this patch series.
> >>
> > 
> > And thank you for trying out my patches!
> > 
> 
> One more thing... it doesn't handle inactive namespaces properly so if you
> have two namespaces with e.g. nsid=1 and nsid=3 QEMU ends up crashing in
> certain situations. The patch below adds support for inactive namespaces.
> 
> Still hoping to see a v2 some day :-)
> 
 
Hi Ross,

v2[1] is actually out, but only CC'ed Paul. Sorry about that! It fixes
the support for discontiguous nsid's, but does not handle inactive
namespaces correctly in identify.

I'll incorporate that in a v3 along with a couple of other fixes I did.

Thanks!


  [1]: https://patchwork.kernel.org/cover/11190045/

Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces
Posted by Ross Lagerwall 6 years, 3 months ago
On 11/4/19 9:04 AM, Klaus Birkelund wrote:
> On Mon, Nov 04, 2019 at 08:46:29AM +0000, Ross Lagerwall wrote:
>> On 8/23/19 9:10 AM, Klaus Birkelund wrote:
>>> On Thu, Aug 22, 2019 at 02:18:05PM +0100, Ross Lagerwall wrote:
>>>> On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
>>>>
>>>> I tried this patch series by installing Windows with a single NVME
>>>> controller having two namespaces. QEMU crashed in get_feature /
>>>> NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.
>>>>
>>>
>>> Hi Ross,
>>>
>>> Good catch!
>>>
>>>> nvme_get_feature / nvme_set_feature look wrong to me since I can't see how
>>>> req->ns would have been set. Should they have similar code to nvme_io_cmd to
>>>> set req->ns from cmd->nsid?
>>>
>>> Definitely. I will fix that for v2.
>>>
>>>>
>>>> After working around this issue everything else seemed to be working well.
>>>> Thanks for your work on this patch series.
>>>>
>>>
>>> And thank you for trying out my patches!
>>>
>>
>> One more thing... it doesn't handle inactive namespaces properly so if you
>> have two namespaces with e.g. nsid=1 and nsid=3 QEMU ends up crashing in
>> certain situations. The patch below adds support for inactive namespaces.
>>
>> Still hoping to see a v2 some day :-)
>>
>  
> Hi Ross,
> 
> v2[1] is actually out, but only CC'ed Paul. Sorry about that! It fixes
> the support for discontiguous nsid's, but does not handle inactive
> namespaces correctly in identify.
> 
> I'll incorporate that in a v3 along with a couple of other fixes I did.
> 
> Thanks!
> 
> 
>   [1]: https://patchwork.kernel.org/cover/11190045/
> 

Thanks for pointing me at that, I missed it.

Regards,
-- 
Ross Lagerwall

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces
Posted by Klaus Birkelund 6 years, 7 months ago
On Fri, Jul 05, 2019 at 09:23:33AM +0200, Klaus Birkelund Jensen wrote:
> 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
> 
> A maximum of 256 namespaces can be configured.
> 
 
Well that was embarrasing.

This patch breaks nvme-test.c. Which I obviously did not run.

In my defense, the test doesn't do much currently, but I'll of course
fix the test for v2.

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces
Posted by Daniel P. Berrangé 6 years, 7 months ago
On Fri, Jul 05, 2019 at 03:36:17PM +0200, Klaus Birkelund wrote:
> On Fri, Jul 05, 2019 at 09:23:33AM +0200, Klaus Birkelund Jensen wrote:
> > 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
> > 
> > A maximum of 256 namespaces can be configured.
> > 
>  
> Well that was embarrasing.
> 
> This patch breaks nvme-test.c. Which I obviously did not run.
> 
> In my defense, the test doesn't do much currently, but I'll of course
> fix the test for v2.

That highlights a more serious problem.  This series changes the syntx
for configuring the nvme device in a way that is not backwards compatible.
So anyone who is using QEMU with NVME will be broken when they upgrade
to the next QEMU release.

I understand why you wanted to restructure things to have a separate
nvme-ns device, but there needs to be some backcompat support in there
for the existing syntax to avoid breaking current users IMHO.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces
Posted by Klaus Birkelund 6 years, 7 months ago
On Fri, Jul 05, 2019 at 02:49:29PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jul 05, 2019 at 03:36:17PM +0200, Klaus Birkelund wrote:
> > On Fri, Jul 05, 2019 at 09:23:33AM +0200, Klaus Birkelund Jensen wrote:
> > > 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
> > > 
> > > A maximum of 256 namespaces can be configured.
> > > 
> >  
> > Well that was embarrasing.
> > 
> > This patch breaks nvme-test.c. Which I obviously did not run.
> > 
> > In my defense, the test doesn't do much currently, but I'll of course
> > fix the test for v2.
> 
> That highlights a more serious problem.  This series changes the syntx
> for configuring the nvme device in a way that is not backwards compatible.
> So anyone who is using QEMU with NVME will be broken when they upgrade
> to the next QEMU release.
> 
> I understand why you wanted to restructure things to have a separate
> nvme-ns device, but there needs to be some backcompat support in there
> for the existing syntax to avoid breaking current users IMHO.
> 
 
Hi Daniel,

I raised this issue previously. I suggested that we keep the drive
property for the nvme device and only accept either that or an nvme-ns
device to be configured (but not both).

That would keep backward compatibilty, but enforce the use of nvme-ns
for any setup that requires multiple namespaces.

Would that work?

Cheers,
Klaus

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] nvme: support multiple namespaces
Posted by Daniel P. Berrangé 6 years, 7 months ago
On Fri, Jul 05, 2019 at 06:20:14PM +0200, Klaus Birkelund wrote:
> On Fri, Jul 05, 2019 at 02:49:29PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jul 05, 2019 at 03:36:17PM +0200, Klaus Birkelund wrote:
> > > On Fri, Jul 05, 2019 at 09:23:33AM +0200, Klaus Birkelund Jensen wrote:
> > > > 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
> > > > 
> > > > A maximum of 256 namespaces can be configured.
> > > > 
> > >  
> > > Well that was embarrasing.
> > > 
> > > This patch breaks nvme-test.c. Which I obviously did not run.
> > > 
> > > In my defense, the test doesn't do much currently, but I'll of course
> > > fix the test for v2.
> > 
> > That highlights a more serious problem.  This series changes the syntx
> > for configuring the nvme device in a way that is not backwards compatible.
> > So anyone who is using QEMU with NVME will be broken when they upgrade
> > to the next QEMU release.
> > 
> > I understand why you wanted to restructure things to have a separate
> > nvme-ns device, but there needs to be some backcompat support in there
> > for the existing syntax to avoid breaking current users IMHO.
> > 
>  
> Hi Daniel,
> 
> I raised this issue previously. I suggested that we keep the drive
> property for the nvme device and only accept either that or an nvme-ns
> device to be configured (but not both).
> 
> That would keep backward compatibilty, but enforce the use of nvme-ns
> for any setup that requires multiple namespaces.
> 
> Would that work?

Yes, that would be viable, as an existing CLI arg usage would continue
to be supported as before.

We could also list the back compat syntax as a deprecation in the docs
(qemu-deprecated.texi) so that in a few releases in the future, we can
drop the old syntax and then use nvme-ns exclusively thereafter.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|