[RFC PATCH V2 00/11] hw/block/nvme: support multi-path for ctrl/ns

[RFC PATCH V2 00/11] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

Hello,

This patch series introduces NVMe subsystem device to support multi-path
I/O in NVMe device model.  Two use-cases are supported along with this
patch: Multi-controller, Namespace Sharing.

V1 RFC has been discussed with Klaus and Keith, I really appreciate them
for this patch series to have proper direction [1].

This patch series contains few start-up refactoring pathces from the
first to fifth patches to make nvme-ns device not to rely on the nvme
controller always.  Because nvme-ns shall be able to be mapped to the
subsystem level, not a single controller level so that it should provide
generic initialization code: nvme_ns_setup() with NvmeCtrl.  To do that,
the first five patches are to remove the NvmeCtrl * instance argument
from the nvme_ns_setup().  I'd be happy if they are picked!

For controller and namespace devices, 'subsys' property has been
introduced to map them to a subsystem.  If multi-controller needed, we
can specify 'subsys' to controllers the same.

For namespace deivice, if 'subsys' is not given just like it was, it
will have to be provided with 'bus' parameter to specify a nvme
controller device to attach, it means, they are mutual-exlusive.  To
share a namespace between or among controllers, then nvme-ns should have
'subsys' property to a single nvme subsystem instance.  To make a
namespace private one, then we need to specify 'bus' property rather
than the 'subsys'.

Of course, this series does not require any updates for the run command
for the previos users.

Plase refer the following example with nvme-cli output:

QEMU Run:
  -device nvme-subsys,id=subsys0 \
  -device nvme,serial=foo,id=nvme0,subsys=subsys0 \
  -device nvme,serial=bar,id=nvme1,subsys=subsys0 \
  -device nvme,serial=baz,id=nvme2,subsys=subsys0 \
  -device nvme-ns,id=ns1,drive=drv10,nsid=1,subsys=subsys0 \
  -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2 \
  \
  -device nvme,serial=qux,id=nvme3 \
  -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3

nvme-cli:
  root@vm:~/work# nvme list -v
  NVM Express Subsystems

  Subsystem        Subsystem-NQN                                                                                    Controllers
  ---------------- ------------------------------------------------------------------------------------------------ ----------------
  nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme0, nvme1, nvme2
  nvme-subsys3     nqn.2019-08.org.qemu:qux                                                                         nvme3

  NVM Express Controllers

  Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
  -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
  nvme0    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys1 nvme1n1
  nvme1    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1 nvme1n1
  nvme2    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1 nvme1n1, nvme1n2
  nvme3    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys3

  NVM Express Namespaces

  Device       NSID     Usage                      Format           Controllers
  ------------ -------- -------------------------- ---------------- ----------------
  nvme1n1      1        134.22  MB / 134.22  MB    512   B +  0 B   nvme0, nvme1, nvme2
  nvme1n2      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme2
  nvme3n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme3

Summary:
  - Refactored nvme-ns device not to rely on controller during the
    setup.  [1/11 - 5/11]
  - Introduced a nvme-subsys device model. [6/11]
  - Create subsystem NQN based on subsystem. [7/11]
  - Introduced multi-controller model. [8/11 - 9/11]
  - Updated namespace sharing scheme to be based on nvme-subsys
    hierarchy. [10/11 - 11/11]

Since RFC V1:
  - Updated namespace sharing scheme to be based on nvme-subsys
    hierarchy.

Thanks,

[1] https://lists.gnu.org/archive/html/qemu-block/2021-01/msg00425.html

Minwoo Im (11):
  hw/block/nvme: remove unused argument in nvme_ns_init_zoned
  hw/block/nvme: open code for volatile write cache
  hw/block/nvme: remove unused argument in nvme_ns_init_blk
  hw/block/nvme: split setup and register for namespace
  hw/block/nvme: remove unused argument in nvme_ns_setup
  hw/block/nvme: introduce nvme-subsys device
  hw/block/nvme: support to map controller to a subsystem
  hw/block/nvme: add CMIC enum value for Identify Controller
  hw/block/nvme: support for multi-controller in subsystem
  hw/block/nvme: add NMIC enum value for Identify Namespace
  hw/block/nvme: support for shared namespace in subsystem

 hw/block/meson.build   |   2 +-
 hw/block/nvme-ns.c     |  40 ++++++++++------
 hw/block/nvme-ns.h     |   9 +++-
 hw/block/nvme-subsys.c | 101 +++++++++++++++++++++++++++++++++++++++++
 hw/block/nvme-subsys.h |  30 ++++++++++++
 hw/block/nvme.c        |  92 ++++++++++++++++++++++++++++++++-----
 hw/block/nvme.h        |   5 +-
 include/block/nvme.h   |   8 ++++
 8 files changed, 259 insertions(+), 28 deletions(-)
 create mode 100644 hw/block/nvme-subsys.c
 create mode 100644 hw/block/nvme-subsys.h

-- 
2.17.1

[RFC PATCH V2 01/11] hw/block/nvme: remove unused argument in nvme_ns_init_zoned

[Minwoo Im]

nvme_ns_init_zoned() has no use for given NvmeCtrl object.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme-ns.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 274eaf61b721..32662230130b 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -204,7 +204,7 @@ static void nvme_ns_zoned_init_state(NvmeNamespace *ns)
     }
 }
 
-static void nvme_ns_init_zoned(NvmeCtrl *n, NvmeNamespace *ns, int lba_index)
+static void nvme_ns_init_zoned(NvmeNamespace *ns, int lba_index)
 {
     NvmeIdNsZoned *id_ns_z;
 
@@ -321,7 +321,7 @@ int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
         if (nvme_ns_zoned_check_calc_geometry(ns, errp) != 0) {
             return -1;
         }
-        nvme_ns_init_zoned(n, ns, 0);
+        nvme_ns_init_zoned(ns, 0);
     }
 
     if (nvme_register_namespace(n, ns, errp)) {
-- 
2.17.1

[RFC PATCH V2 02/11] hw/block/nvme: open code for volatile write cache

[Minwoo Im]

Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
initial time.  This feature is related to block device backed,  but this
feature is controlled in controller level via Set/Get Features command.

This patch removed dependency between nvme and nvme-ns to manage the VWC
flag value.  Also, it open coded the Get Features for VWC to check all
namespaces attached to the controller, and if false detected, return
directly false.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme-ns.c |  4 ----
 hw/block/nvme.c    | 15 ++++++++++++---
 hw/block/nvme.h    |  1 -
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 32662230130b..c403cd36b6bd 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
         return -1;
     }
 
-    if (blk_enable_write_cache(ns->blkconf.blk)) {
-        n->features.vwc = 0x1;
-    }
-
     return 0;
 }
 
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cf0fe28fe6eb..b2a9c48a9d81 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
     NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
     uint16_t iv;
     NvmeNamespace *ns;
+    int i;
 
     static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
         [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
@@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
         result = ns->features.err_rec;
         goto out;
     case NVME_VOLATILE_WRITE_CACHE:
-        result = n->features.vwc;
+        for (i = 1; i <= n->num_namespaces; i++) {
+            ns = nvme_ns(n, i);
+            if (!ns) {
+                continue;
+            }
+
+            result = blk_enable_write_cache(ns->blkconf.blk);
+            if (!result) {
+                break;
+            }
+        }
         trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
         goto out;
     case NVME_ASYNCHRONOUS_EVENT_CONF:
@@ -3271,8 +3282,6 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
         ns->features.err_rec = dw11;
         break;
     case NVME_VOLATILE_WRITE_CACHE:
-        n->features.vwc = dw11 & 0x1;
-
         for (i = 1; i <= n->num_namespaces; i++) {
             ns = nvme_ns(n, i);
             if (!ns) {
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index b7fbcca39d9f..5ba83ee77841 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -117,7 +117,6 @@ typedef struct NvmeFeatureVal {
         uint16_t temp_thresh_low;
     };
     uint32_t    async_config;
-    uint32_t    vwc;
 } NvmeFeatureVal;
 
 typedef struct NvmeCtrl {
-- 
2.17.1

[RFC PATCH V2 03/11] hw/block/nvme: remove unused argument in nvme_ns_init_blk

[Minwoo Im]

Removed no longer used aregument NvmeCtrl object in nvme_ns_init_blk().

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme-ns.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index c403cd36b6bd..fc42ae184e01 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -66,7 +66,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
     return 0;
 }
 
-static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
+static int nvme_ns_init_blk(NvmeNamespace *ns, Error **errp)
 {
     if (!blkconf_blocksizes(&ns->blkconf, errp)) {
         return -1;
@@ -306,7 +306,7 @@ int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
         return -1;
     }
 
-    if (nvme_ns_init_blk(n, ns, errp)) {
+    if (nvme_ns_init_blk(ns, errp)) {
         return -1;
     }
 
-- 
2.17.1

[RFC PATCH V2 04/11] hw/block/nvme: split setup and register for namespace

[Minwoo Im]

In NVMe, namespace is being attached to process I/O.  We register NVMe
namespace to a controller via nvme_register_namespace() during
nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
instance to this function to map the namespace to a controller.

To make namespace instance more independent, it should be split into two
parts: setup and register.  This patch split them into two differnt
parts, and finally nvme_ns_setup() does not have nothing to do with
NvmeCtrl instance at all.

This patch is a former patch to introduce NVMe subsystem scheme to the
existing design especially for multi-path.  In that case, it should be
split into two to make namespace independent from a controller.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme-ns.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index fc42ae184e01..3f0e4e461420 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -320,10 +320,6 @@ int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
         nvme_ns_init_zoned(ns, 0);
     }
 
-    if (nvme_register_namespace(n, ns, errp)) {
-        return -1;
-    }
-
     return 0;
 }
 
@@ -361,6 +357,13 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
                                 "could not setup namespace: ");
         return;
     }
+
+    if (nvme_register_namespace(n, ns, errp)) {
+        error_propagate_prepend(errp, local_err,
+                                "could not register namespace: ");
+        return;
+    }
+
 }
 
 static Property nvme_ns_props[] = {
-- 
2.17.1

[RFC PATCH V2 05/11] hw/block/nvme: remove unused argument in nvme_ns_setup

[Minwoo Im]

nvme_ns_setup() finally does not have nothing to do with NvmeCtrl
instance.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme-ns.c | 4 ++--
 hw/block/nvme-ns.h | 2 +-
 hw/block/nvme.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 3f0e4e461420..c8b75fa02138 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -300,7 +300,7 @@ static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp)
     return 0;
 }
 
-int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
+int nvme_ns_setup(NvmeNamespace *ns, Error **errp)
 {
     if (nvme_ns_check_constraints(ns, errp)) {
         return -1;
@@ -352,7 +352,7 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
     NvmeCtrl *n = NVME(s->parent);
     Error *local_err = NULL;
 
-    if (nvme_ns_setup(n, ns, &local_err)) {
+    if (nvme_ns_setup(ns, &local_err)) {
         error_propagate_prepend(errp, local_err,
                                 "could not setup namespace: ");
         return;
diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
index f8f3c28c360b..d87c025b0ab6 100644
--- a/hw/block/nvme-ns.h
+++ b/hw/block/nvme-ns.h
@@ -174,7 +174,7 @@ static inline void nvme_aor_dec_active(NvmeNamespace *ns)
     assert(ns->nr_active_zones >= 0);
 }
 
-int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp);
+int nvme_ns_setup(NvmeNamespace *ns, Error **errp);
 void nvme_ns_drain(NvmeNamespace *ns);
 void nvme_ns_shutdown(NvmeNamespace *ns);
 void nvme_ns_cleanup(NvmeNamespace *ns);
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b2a9c48a9d81..9e890ce84f5e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4313,7 +4313,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         ns = &n->namespace;
         ns->params.nsid = 1;
 
-        if (nvme_ns_setup(n, ns, errp)) {
+        if (nvme_ns_setup(ns, errp)) {
             return;
         }
     }
-- 
2.17.1

[RFC PATCH V2 06/11] hw/block/nvme: introduce nvme-subsys device

[Minwoo Im]

To support multi-path in QEMU NVMe device model, We need to have NVMe
subsystem hierarchy to map controllers and namespaces to a NVMe
subsystem.

This patch introduced a simple nvme-subsys device model.  The subsystem
will be prepared with subsystem NQN with <subsys_id> provided in
nvme-subsys device:

  ex) -device nvme-subsys,id=subsys0: nqn.2019-08.org.qemu:subsys0

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/meson.build   |  2 +-
 hw/block/nvme-subsys.c | 63 ++++++++++++++++++++++++++++++++++++++++++
 hw/block/nvme-subsys.h | 25 +++++++++++++++++
 hw/block/nvme.c        |  3 ++
 4 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 hw/block/nvme-subsys.c
 create mode 100644 hw/block/nvme-subsys.h

diff --git a/hw/block/meson.build b/hw/block/meson.build
index 602ca6c8541d..83ea2d37978d 100644
--- a/hw/block/meson.build
+++ b/hw/block/meson.build
@@ -13,7 +13,7 @@ softmmu_ss.add(when: 'CONFIG_SSI_M25P80', if_true: files('m25p80.c'))
 softmmu_ss.add(when: 'CONFIG_SWIM', if_true: files('swim.c'))
 softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c'))
 softmmu_ss.add(when: 'CONFIG_SH4', if_true: files('tc58128.c'))
-softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('nvme.c', 'nvme-ns.c'))
+softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('nvme.c', 'nvme-ns.c', 'nvme-subsys.c'))
 
 specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
 specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk.c'))
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
new file mode 100644
index 000000000000..f1dc71d588d9
--- /dev/null
+++ b/hw/block/nvme-subsys.c
@@ -0,0 +1,63 @@
+/*
+ * QEMU NVM Express Subsystem: nvme-subsys
+ *
+ * Copyright (c) 2021 Minwoo Im <minwoo.im.dev@gmail.com>
+ *
+ * This code is licensed under the GNU GPL v2.  Refer COPYING.
+ */
+
+#include "qemu/units.h"
+#include "qemu/osdep.h"
+#include "qemu/uuid.h"
+#include "qemu/iov.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-core.h"
+#include "hw/block/block.h"
+#include "block/aio.h"
+#include "block/accounting.h"
+#include "sysemu/sysemu.h"
+#include "hw/pci/pci.h"
+#include "nvme.h"
+#include "nvme-subsys.h"
+
+static void nvme_subsys_setup(NvmeSubsystem *subsys)
+{
+    char *subnqn;
+
+    subnqn = g_strdup_printf("nqn.2019-08.org.qemu:%s", subsys->parent_obj.id);
+    strpadcpy((char *)subsys->subnqn, sizeof(subsys->subnqn), subnqn, '\0');
+    g_free(subnqn);
+}
+
+static void nvme_subsys_realize(DeviceState *dev, Error **errp)
+{
+    NvmeSubsystem *subsys = NVME_SUBSYS(dev);
+
+    nvme_subsys_setup(subsys);
+}
+
+static void nvme_subsys_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+
+    dc->realize = nvme_subsys_realize;
+    dc->desc = "Virtual NVMe subsystem";
+}
+
+static const TypeInfo nvme_subsys_info = {
+    .name = TYPE_NVME_SUBSYS,
+    .parent = TYPE_DEVICE,
+    .class_init = nvme_subsys_class_init,
+    .instance_size = sizeof(NvmeSubsystem),
+};
+
+static void nvme_subsys_register_types(void)
+{
+    type_register_static(&nvme_subsys_info);
+}
+
+type_init(nvme_subsys_register_types)
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
new file mode 100644
index 000000000000..40f06a4c7db0
--- /dev/null
+++ b/hw/block/nvme-subsys.h
@@ -0,0 +1,25 @@
+/*
+ * QEMU NVM Express Subsystem: nvme-subsys
+ *
+ * Copyright (c) 2021 Minwoo Im <minwoo.im.dev@gmail.com>
+ *
+ * This code is licensed under the GNU GPL v2.  Refer COPYING.
+ */
+
+#ifndef NVME_SUBSYS_H
+#define NVME_SUBSYS_H
+
+#define TYPE_NVME_SUBSYS "nvme-subsys"
+#define NVME_SUBSYS(obj) \
+    OBJECT_CHECK(NvmeSubsystem, (obj), TYPE_NVME_SUBSYS)
+
+#define NVME_SUBSYS_MAX_CTRLS   32
+
+typedef struct NvmeCtrl NvmeCtrl;
+typedef struct NvmeNamespace NvmeNamespace;
+typedef struct NvmeSubsystem {
+    DeviceState parent_obj;
+    uint8_t     subnqn[256];
+} NvmeSubsystem;
+
+#endif /* NVME_SUBSYS_H */
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9e890ce84f5e..dde83aafc33d 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -25,6 +25,7 @@
  *              mdts=<N[optional]>,zoned.append_size_limit=<N[optional]> \
  *      -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
  *              zoned=<true|false[optional]>
+ *      -device nvme-subsys,id=<subsys_id>
  *
  * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
  * offset 0 in BAR2 and supports only WDS, RDS and SQS for now.
@@ -37,6 +38,8 @@
  * -object memory-backend-file,id=<mem_id>,share=on,mem-path=<file_path>, \
  *  size=<size> .... -device nvme,...,pmrdev=<mem_id>
  *
+ * To place controller(s) and namespace(s) to a subsystem, then provide
+ * nvme-subsys device as above.
  *
  * nvme device parameters
  * ~~~~~~~~~~~~~~~~~~~~~~
-- 
2.17.1

[RFC PATCH V2 07/11] hw/block/nvme: support to map controller to a subsystem

[Minwoo Im]

nvme controller(nvme) can be mapped to a NVMe subsystem(nvme-subsys).
This patch maps a controller to a subsystem by adding a parameter
'subsys' to the nvme device.

To map a controller to a subsystem, we need to put nvme-subsys first and
then maps the subsystem to the controller:

  -device nvme-subsys,id=subsys0
  -device nvme,serial=foo,id=nvme0,subsys=subsys0

If 'subsys' property is not given to the nvme controller, then subsystem
NQN will be created with serial (e.g., 'foo' in above example),
Otherwise, it will be based on subsys id (e.g., 'subsys0' in above
example).

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme.c | 30 ++++++++++++++++++++++++++----
 hw/block/nvme.h |  3 +++
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index dde83aafc33d..af5b2162e2b5 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -23,6 +23,7 @@
  *              max_ioqpairs=<N[optional]>, \
  *              aerl=<N[optional]>, aer_max_queued=<N[optional]>, \
  *              mdts=<N[optional]>,zoned.append_size_limit=<N[optional]> \
+ *              ,subsys=<subsys_id> \
  *      -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
  *              zoned=<true|false[optional]>
  *      -device nvme-subsys,id=<subsys_id>
@@ -43,6 +44,13 @@
  *
  * nvme device parameters
  * ~~~~~~~~~~~~~~~~~~~~~~
+ * - `subsys`
+ *   NVM Subsystem device. If given, a subsystem NQN will be initialized with
+ *   <subsys_id> given. Otherwise, <serial> will be taken for subsystem NQN.
+ *   Also, it will enable multi controller capability represented in Identify
+ *   Controller data structure in CMIC (Controller Multi-path I/O and Namesapce
+ *   Sharing Capabilities), if given.
+ *
  * - `aerl`
  *   The Asynchronous Event Request Limit (AERL). Indicates the maximum number
  *   of concurrently outstanding Asynchronous Event Request commands support
@@ -4219,11 +4227,25 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
     return 0;
 }
 
+static void nvme_init_subnqn(NvmeCtrl *n)
+{
+    NvmeSubsystem *subsys = n->subsys;
+    NvmeIdCtrl *id = &n->id_ctrl;
+    char *subnqn;
+
+    if (!subsys) {
+        subnqn = g_strdup_printf("nqn.2019-08.org.qemu:%s", n->params.serial);
+        strpadcpy((char *)id->subnqn, sizeof(id->subnqn), subnqn, '\0');
+        g_free(subnqn);
+    } else {
+        pstrcpy((char *)id->subnqn, sizeof(id->subnqn), (char*)subsys->subnqn);
+    }
+}
+
 static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
 {
     NvmeIdCtrl *id = &n->id_ctrl;
     uint8_t *pci_conf = pci_dev->config;
-    char *subnqn;
 
     id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
     id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
@@ -4269,9 +4291,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     id->sgls = cpu_to_le32(NVME_CTRL_SGLS_SUPPORT_NO_ALIGN |
                            NVME_CTRL_SGLS_BITBUCKET);
 
-    subnqn = g_strdup_printf("nqn.2019-08.org.qemu:%s", n->params.serial);
-    strpadcpy((char *)id->subnqn, sizeof(id->subnqn), subnqn, '\0');
-    g_free(subnqn);
+    nvme_init_subnqn(n);
 
     id->psd[0].mp = cpu_to_le16(0x9c4);
     id->psd[0].enlat = cpu_to_le32(0x10);
@@ -4355,6 +4375,8 @@ static void nvme_exit(PCIDevice *pci_dev)
 
 static Property nvme_props[] = {
     DEFINE_BLOCK_PROPERTIES(NvmeCtrl, namespace.blkconf),
+    DEFINE_PROP_LINK("subsys", NvmeCtrl, subsys, TYPE_NVME_SUBSYS,
+                     NvmeSubsystem *),
     DEFINE_PROP_LINK("pmrdev", NvmeCtrl, pmrdev, TYPE_MEMORY_BACKEND,
                      HostMemoryBackend *),
     DEFINE_PROP_STRING("serial", NvmeCtrl, params.serial),
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 5ba83ee77841..2061e53e2cee 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -2,6 +2,7 @@
 #define HW_NVME_H
 
 #include "block/nvme.h"
+#include "nvme-subsys.h"
 #include "nvme-ns.h"
 
 #define NVME_MAX_NAMESPACES 256
@@ -154,6 +155,8 @@ typedef struct NvmeCtrl {
 
     uint8_t     zasl;
 
+    NvmeSubsystem   *subsys;
+
     NvmeNamespace   namespace;
     NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES];
     NvmeSQueue      **sq;
-- 
2.17.1

[RFC PATCH V2 08/11] hw/block/nvme: add CMIC enum value for Identify Controller

[Minwoo Im]

Added Controller Multi-path I/O and Namespace Sharing Capabilities
(CMIC) field to support multi-controller in the following patches.

This field is in Identify Controller data structure in [76].

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 include/block/nvme.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 9494246f1f59..e83ec1e124c6 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -941,6 +941,10 @@ enum NvmeIdCtrlLpa {
     NVME_LPA_EXTENDED = 1 << 2,
 };
 
+enum NvmeIdCtrlCmic {
+    NVME_CMIC_MULTI_CTRL    = 1 << 1,
+};
+
 #define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
 #define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
 #define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
-- 
2.17.1

[RFC PATCH V2 09/11] hw/block/nvme: support for multi-controller in subsystem

[Minwoo Im]

We have nvme-subsys and nvme devices mapped together.  To support
multi-controller scheme to this setup, controller identifier(id) has to
be managed.  Earlier, cntlid(controller id) used to be always 0 because
we didn't have any subsystem scheme that controller id matters.

This patch introduced 'cntlid' attribute to the nvme controller
instance(NvmeCtrl) and make it allocated by the nvme-subsys device
mapped to the controller.  If nvme-subsys is not given to the
controller, then it will always be 0 as it was.

Added 'ctrls' array in the nvme-subsys instance to manage attached
controllers to the subsystem with a limit(32).  This patch didn't take
list for the controllers to make it seamless with nvme-ns device.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme-subsys.c | 21 +++++++++++++++++++++
 hw/block/nvme-subsys.h |  4 ++++
 hw/block/nvme.c        | 32 ++++++++++++++++++++++++++++++--
 hw/block/nvme.h        |  1 +
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
index f1dc71d588d9..a01003136b12 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -22,6 +22,27 @@
 #include "nvme.h"
 #include "nvme-subsys.h"
 
+int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
+{
+    NvmeSubsystem *subsys = n->subsys;
+    int cntlid;
+
+    for (cntlid = 0; cntlid < ARRAY_SIZE(subsys->ctrls); cntlid++) {
+        if (!subsys->ctrls[cntlid]) {
+            break;
+        }
+    }
+
+    if (cntlid == ARRAY_SIZE(subsys->ctrls)) {
+        error_setg(errp, "no more free controller id");
+        return -1;
+    }
+
+    subsys->ctrls[cntlid] = n;
+
+    return cntlid;
+}
+
 static void nvme_subsys_setup(NvmeSubsystem *subsys)
 {
     char *subnqn;
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 40f06a4c7db0..4eba50d96a1d 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -20,6 +20,10 @@ typedef struct NvmeNamespace NvmeNamespace;
 typedef struct NvmeSubsystem {
     DeviceState parent_obj;
     uint8_t     subnqn[256];
+
+    NvmeCtrl    *ctrls[NVME_SUBSYS_MAX_CTRLS];
 } NvmeSubsystem;
 
+int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
+
 #endif /* NVME_SUBSYS_H */
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index af5b2162e2b5..2aeb164dd508 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4242,7 +4242,7 @@ static void nvme_init_subnqn(NvmeCtrl *n)
     }
 }
 
-static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
+static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev, uint16_t cntlid)
 {
     NvmeIdCtrl *id = &n->id_ctrl;
     uint8_t *pci_conf = pci_dev->config;
@@ -4252,6 +4252,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl", ' ');
     strpadcpy((char *)id->fr, sizeof(id->fr), "1.0", ' ');
     strpadcpy((char *)id->sn, sizeof(id->sn), n->params.serial, ' ');
+
+    id->cntlid = cntlid;
+
     id->rab = 6;
     id->ieee[0] = 0x00;
     id->ieee[1] = 0x02;
@@ -4297,6 +4300,10 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     id->psd[0].enlat = cpu_to_le32(0x10);
     id->psd[0].exlat = cpu_to_le32(0x4);
 
+    if (n->subsys) {
+        id->cmic |= NVME_CMIC_MULTI_CTRL;
+    }
+
     NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
     NVME_CAP_SET_CQR(n->bar.cap, 1);
     NVME_CAP_SET_TO(n->bar.cap, 0xf);
@@ -4309,11 +4316,28 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     n->bar.intmc = n->bar.intms = 0;
 }
 
+static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
+{
+    int cntlid;
+
+    if (!n->subsys) {
+        return 0;
+    }
+
+    cntlid = nvme_subsys_register_ctrl(n, errp);
+    if (cntlid < 0) {
+        return -1;
+    }
+
+    return cntlid;
+}
+
 static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 {
     NvmeCtrl *n = NVME(pci_dev);
     NvmeNamespace *ns;
     Error *local_err = NULL;
+    int cntlid;
 
     nvme_check_constraints(n, &local_err);
     if (local_err) {
@@ -4329,7 +4353,11 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
 
-    nvme_init_ctrl(n, pci_dev);
+    cntlid = nvme_init_subsys(n, errp);
+    if (cntlid < 0) {
+        return;
+    }
+    nvme_init_ctrl(n, pci_dev, cntlid);
 
     /* setup a namespace if the controller drive property was given */
     if (n->namespace.blkconf.blk) {
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 2061e53e2cee..329902a9fc04 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -129,6 +129,7 @@ typedef struct NvmeCtrl {
     NvmeBus      bus;
     BlockConf    conf;
 
+    uint16_t    cntlid;
     bool        qs_created;
     uint32_t    page_size;
     uint16_t    page_bits;
-- 
2.17.1

[RFC PATCH V2 10/11] hw/block/nvme: add NMIC enum value for Identify Namespace

[Minwoo Im]

Added Namespace Multi-path I/O and Namespace Sharing Capabilities (NMIC)
field to support shared namespace from controller(s).

This field is in Identify Namespace data structure in [30].

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 include/block/nvme.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index e83ec1e124c6..dd7b5ba9ef4c 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1110,6 +1110,10 @@ enum NvmeNsIdentifierType {
     NVME_NIDT_CSI               = 0x04,
 };
 
+enum NvmeNsNmic {
+    NVME_NMIC_NS_SHARED         = 1 << 0,
+};
+
 enum NvmeCsi {
     NVME_CSI_NVM                = 0x00,
     NVME_CSI_ZONED              = 0x02,
-- 
2.17.1

[RFC PATCH V2 11/11] hw/block/nvme: support for shared namespace in subsystem

[Minwoo Im]

nvme-ns device is registered to a nvme controller device during the
initialization in nvme_register_namespace() in case that 'bus' property
is given which means it's mapped to a single controller.

This patch introduced a new property 'subsys' just like the controller
device instance did to map a namespace to a NVMe subsystem.

If 'subsys' property is given to the nvme-ns device, it will belong to
the specified subsystem and will be attached to all controllers in that
subsystem by enabling shared namespace capability in NMIC(Namespace
Multi-path I/O and Namespace Capabilities) in Identify Namespace.

Usage:

  -device nvme-subsys,id=subsys0
  -device nvme,serial=foo,id=nvme0,subsys=subsys0
  -device nvme,serial=bar,id=nvme1,subsys=subsys0
  -device nvme,serial=baz,id=nvme2,subsys=subsys0
  -device nvme-ns,id=ns1,drive=<drv>,nsid=1,subsys=subsys0  # Shared
  -device nvme-ns,id=ns2,drive=<drv>,nsid=2,bus=nvme2       # Non-shared

  In the above example, 'ns1' will be shared to 'nvme0' and 'nvme1' in
  the same subsystem.  On the other hand, 'ns2' will be attached to the
  'nvme2' only as a private namespace in that subsystem.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme-ns.c     | 23 ++++++++++++++++++-----
 hw/block/nvme-ns.h     |  7 +++++++
 hw/block/nvme-subsys.c | 17 +++++++++++++++++
 hw/block/nvme-subsys.h |  1 +
 hw/block/nvme.c        | 10 +++++++++-
 5 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index c8b75fa02138..073f65e49cac 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -63,6 +63,10 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 
     id_ns->npda = id_ns->npdg = npdg - 1;
 
+    if (nvme_ns_shared(ns)) {
+        id_ns->nmic |= NVME_NMIC_NS_SHARED;
+    }
+
     return 0;
 }
 
@@ -358,16 +362,25 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (nvme_register_namespace(n, ns, errp)) {
-        error_propagate_prepend(errp, local_err,
-                                "could not register namespace: ");
-        return;
+    if (ns->subsys) {
+        if (nvme_subsys_register_ns(ns, errp)) {
+            error_propagate_prepend(errp, local_err,
+                                    "could not setup namespace to subsys: ");
+            return;
+        }
+    } else {
+        if (nvme_register_namespace(n, ns, errp)) {
+            error_propagate_prepend(errp, local_err,
+                                    "could not register namespace: ");
+            return;
+        }
     }
-
 }
 
 static Property nvme_ns_props[] = {
     DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf),
+    DEFINE_PROP_LINK("subsys", NvmeNamespace, subsys, TYPE_NVME_SUBSYS,
+                     NvmeSubsystem *),
     DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
     DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
     DEFINE_PROP_BOOL("zoned", NvmeNamespace, params.zoned, false),
diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
index d87c025b0ab6..3fc7262a7915 100644
--- a/hw/block/nvme-ns.h
+++ b/hw/block/nvme-ns.h
@@ -47,6 +47,8 @@ typedef struct NvmeNamespace {
     const uint32_t *iocs;
     uint8_t      csi;
 
+    NvmeSubsystem   *subsys;
+
     NvmeIdNsZoned   *id_ns_zoned;
     NvmeZone        *zone_array;
     QTAILQ_HEAD(, NvmeZone) exp_open_zones;
@@ -77,6 +79,11 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns)
     return -1;
 }
 
+static inline bool nvme_ns_shared(NvmeNamespace *ns)
+{
+    return !!ns->subsys;
+}
+
 static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns)
 {
     NvmeIdNs *id_ns = &ns->id_ns;
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
index a01003136b12..d67160a0ed6f 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -43,6 +43,23 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
     return cntlid;
 }
 
+int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp)
+{
+    NvmeSubsystem *subsys = ns->subsys;
+    NvmeCtrl *n;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) {
+        n = subsys->ctrls[i];
+
+        if (n && nvme_register_namespace(n, ns, errp)) {
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
 static void nvme_subsys_setup(NvmeSubsystem *subsys)
 {
     char *subnqn;
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 4eba50d96a1d..cae0fbd464e2 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -25,5 +25,6 @@ typedef struct NvmeSubsystem {
 } NvmeSubsystem;
 
 int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
+int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp);
 
 #endif /* NVME_SUBSYS_H */
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2aeb164dd508..7869bd95b099 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -25,7 +25,8 @@
  *              mdts=<N[optional]>,zoned.append_size_limit=<N[optional]> \
  *              ,subsys=<subsys_id> \
  *      -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
- *              zoned=<true|false[optional]>
+ *              zoned=<true|false[optional]>, \
+ *              subsys=<subsys_id>
  *      -device nvme-subsys,id=<subsys_id>
  *
  * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
@@ -69,6 +70,13 @@
  *   data size being in effect. By setting this property to 0, users can make
  *   ZASL to be equal to MDTS. This property only affects zoned namespaces.
  *
+ * nvme namespace device parameters
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * - `subsys`
+ *   NVM Subsystem device.  If given, this namespace will be attached to all
+ *   controllers in the subsystem. Otherwise, `bus` must be given to attach
+ *   this namespace to a specified single controller as a non-shared namespace.
+ *
  * Setting `zoned` to true selects Zoned Command Set at the namespace.
  * In this case, the following namespace properties are available to configure
  * zoned operation:
-- 
2.17.1

Re: [RFC PATCH V2 01/11] hw/block/nvme: remove unused argument in nvme_ns_init_zoned

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

On Jan 17 23:53, Minwoo Im wrote:
> nvme_ns_init_zoned() has no use for given NvmeCtrl object.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 02/11] hw/block/nvme: open code for volatile write cache

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

On Jan 17 23:53, Minwoo Im wrote:
> Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
> initial time.  This feature is related to block device backed,  but this
> feature is controlled in controller level via Set/Get Features command.
> 
> This patch removed dependency between nvme and nvme-ns to manage the VWC
> flag value.  Also, it open coded the Get Features for VWC to check all
> namespaces attached to the controller, and if false detected, return
> directly false.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

The VWC feature really should be namespace specific. I wonder why they
didn't fix that when they added an NSID to the Flush command...

Anyway, this is much better.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 03/11] hw/block/nvme: remove unused argument in nvme_ns_init_blk

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 298 bytes --]

On Jan 17 23:53, Minwoo Im wrote:
> Removed no longer used aregument NvmeCtrl object in nvme_ns_init_blk().
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

I don't think it's too unwieldy for this to be squashed into [02/11],
but

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 04/11] hw/block/nvme: split setup and register for namespace

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On Jan 17 23:53, Minwoo Im wrote:
> In NVMe, namespace is being attached to process I/O.  We register NVMe
> namespace to a controller via nvme_register_namespace() during
> nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
> instance to this function to map the namespace to a controller.
> 
> To make namespace instance more independent, it should be split into two
> parts: setup and register.  This patch split them into two differnt
> parts, and finally nvme_ns_setup() does not have nothing to do with
> NvmeCtrl instance at all.
> 
> This patch is a former patch to introduce NVMe subsystem scheme to the
> existing design especially for multi-path.  In that case, it should be
> split into two to make namespace independent from a controller.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 05/11] hw/block/nvme: remove unused argument in nvme_ns_setup

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

On Jan 17 23:53, Minwoo Im wrote:
> nvme_ns_setup() finally does not have nothing to do with NvmeCtrl
> instance.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

I also think this *could* be squashed into [04/11] without too much
strain on the eye, but

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 10/11] hw/block/nvme: add NMIC enum value for Identify Namespace

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

On Jan 17 23:53, Minwoo Im wrote:
> Added Namespace Multi-path I/O and Namespace Sharing Capabilities (NMIC)
> field to support shared namespace from controller(s).
> 
> This field is in Identify Namespace data structure in [30].
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>  include/block/nvme.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index e83ec1e124c6..dd7b5ba9ef4c 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -1110,6 +1110,10 @@ enum NvmeNsIdentifierType {
>      NVME_NIDT_CSI               = 0x04,
>  };
>  
> +enum NvmeNsNmic {
> +    NVME_NMIC_NS_SHARED         = 1 << 0,
> +};
> +

Let's keep convention (or should be convention...) of using NvmeIdNs
prefix for identify data structure fields on the enum.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 00/11] hw/block/nvme: support multi-path for ctrl/ns

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 5383 bytes --]

On Jan 17 23:53, Minwoo Im wrote:
> Hello,
> 
> This patch series introduces NVMe subsystem device to support multi-path
> I/O in NVMe device model.  Two use-cases are supported along with this
> patch: Multi-controller, Namespace Sharing.
> 
> V1 RFC has been discussed with Klaus and Keith, I really appreciate them
> for this patch series to have proper direction [1].
> 
> This patch series contains few start-up refactoring pathces from the
> first to fifth patches to make nvme-ns device not to rely on the nvme
> controller always.  Because nvme-ns shall be able to be mapped to the
> subsystem level, not a single controller level so that it should provide
> generic initialization code: nvme_ns_setup() with NvmeCtrl.  To do that,
> the first five patches are to remove the NvmeCtrl * instance argument
> from the nvme_ns_setup().  I'd be happy if they are picked!
> 
> For controller and namespace devices, 'subsys' property has been
> introduced to map them to a subsystem.  If multi-controller needed, we
> can specify 'subsys' to controllers the same.
> 
> For namespace deivice, if 'subsys' is not given just like it was, it
> will have to be provided with 'bus' parameter to specify a nvme
> controller device to attach, it means, they are mutual-exlusive.  To
> share a namespace between or among controllers, then nvme-ns should have
> 'subsys' property to a single nvme subsystem instance.  To make a
> namespace private one, then we need to specify 'bus' property rather
> than the 'subsys'.
> 
> Of course, this series does not require any updates for the run command
> for the previos users.
> 
> Plase refer the following example with nvme-cli output:
> 
> QEMU Run:
>   -device nvme-subsys,id=subsys0 \
>   -device nvme,serial=foo,id=nvme0,subsys=subsys0 \
>   -device nvme,serial=bar,id=nvme1,subsys=subsys0 \
>   -device nvme,serial=baz,id=nvme2,subsys=subsys0 \
>   -device nvme-ns,id=ns1,drive=drv10,nsid=1,subsys=subsys0 \
>   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2 \
>   \
>   -device nvme,serial=qux,id=nvme3 \
>   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> 
> nvme-cli:
>   root@vm:~/work# nvme list -v
>   NVM Express Subsystems
> 
>   Subsystem        Subsystem-NQN                                                                                    Controllers
>   ---------------- ------------------------------------------------------------------------------------------------ ----------------
>   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme0, nvme1, nvme2
>   nvme-subsys3     nqn.2019-08.org.qemu:qux                                                                         nvme3
> 
>   NVM Express Controllers
> 
>   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
>   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
>   nvme0    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys1 nvme1n1
>   nvme1    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1 nvme1n1
>   nvme2    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1 nvme1n1, nvme1n2
>   nvme3    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys3
> 
>   NVM Express Namespaces
> 
>   Device       NSID     Usage                      Format           Controllers
>   ------------ -------- -------------------------- ---------------- ----------------
>   nvme1n1      1        134.22  MB / 134.22  MB    512   B +  0 B   nvme0, nvme1, nvme2
>   nvme1n2      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme2
>   nvme3n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme3
> 
> Summary:
>   - Refactored nvme-ns device not to rely on controller during the
>     setup.  [1/11 - 5/11]
>   - Introduced a nvme-subsys device model. [6/11]
>   - Create subsystem NQN based on subsystem. [7/11]
>   - Introduced multi-controller model. [8/11 - 9/11]
>   - Updated namespace sharing scheme to be based on nvme-subsys
>     hierarchy. [10/11 - 11/11]
> 
> Since RFC V1:
>   - Updated namespace sharing scheme to be based on nvme-subsys
>     hierarchy.
> 

Great stuff Minwoo. Thanks!

I'll pick up [01-05/11] directly since they are pretty trivial.

The subsystem model looks pretty much like it should, I don't have a lot
of comments.

One thing that I considered, is if we should reverse the "registration"
and think about it as namespace attachment. The spec is about
controllers attaching to namespaces, not the other way around.
Basically, let the namespaces be configured first and register on the
subsystem (accumulating in a "namespaces" array), then have the
controllers register with the subsystem and attach to all "non-detached"
namespaces. This allows detached namespaces to "linger" in the subsystem
to be attached later on. If there are any private namespaces (like ns2
in your example above), it will be defined after the controller with the
bus=ctrlX parameter like normal.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 10/11] hw/block/nvme: add NMIC enum value for Identify Namespace

[Minwoo Im]

> Let's keep convention (or should be convention...) of using NvmeIdNs
> prefix for identify data structure fields on the enum.

Okay!

Re: [RFC PATCH V2 00/11] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

On 21-01-18 22:14:45, Klaus Jensen wrote:
> On Jan 17 23:53, Minwoo Im wrote:
> > Hello,
> > 
> > This patch series introduces NVMe subsystem device to support multi-path
> > I/O in NVMe device model.  Two use-cases are supported along with this
> > patch: Multi-controller, Namespace Sharing.
> > 
> > V1 RFC has been discussed with Klaus and Keith, I really appreciate them
> > for this patch series to have proper direction [1].
> > 
> > This patch series contains few start-up refactoring pathces from the
> > first to fifth patches to make nvme-ns device not to rely on the nvme
> > controller always.  Because nvme-ns shall be able to be mapped to the
> > subsystem level, not a single controller level so that it should provide
> > generic initialization code: nvme_ns_setup() with NvmeCtrl.  To do that,
> > the first five patches are to remove the NvmeCtrl * instance argument
> > from the nvme_ns_setup().  I'd be happy if they are picked!
> > 
> > For controller and namespace devices, 'subsys' property has been
> > introduced to map them to a subsystem.  If multi-controller needed, we
> > can specify 'subsys' to controllers the same.
> > 
> > For namespace deivice, if 'subsys' is not given just like it was, it
> > will have to be provided with 'bus' parameter to specify a nvme
> > controller device to attach, it means, they are mutual-exlusive.  To
> > share a namespace between or among controllers, then nvme-ns should have
> > 'subsys' property to a single nvme subsystem instance.  To make a
> > namespace private one, then we need to specify 'bus' property rather
> > than the 'subsys'.
> > 
> > Of course, this series does not require any updates for the run command
> > for the previos users.
> > 
> > Plase refer the following example with nvme-cli output:
> > 
> > QEMU Run:
> >   -device nvme-subsys,id=subsys0 \
> >   -device nvme,serial=foo,id=nvme0,subsys=subsys0 \
> >   -device nvme,serial=bar,id=nvme1,subsys=subsys0 \
> >   -device nvme,serial=baz,id=nvme2,subsys=subsys0 \
> >   -device nvme-ns,id=ns1,drive=drv10,nsid=1,subsys=subsys0 \
> >   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2 \
> >   \
> >   -device nvme,serial=qux,id=nvme3 \
> >   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> > 
> > nvme-cli:
> >   root@vm:~/work# nvme list -v
> >   NVM Express Subsystems
> > 
> >   Subsystem        Subsystem-NQN                                                                                    Controllers
> >   ---------------- ------------------------------------------------------------------------------------------------ ----------------
> >   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme0, nvme1, nvme2
> >   nvme-subsys3     nqn.2019-08.org.qemu:qux                                                                         nvme3
> > 
> >   NVM Express Controllers
> > 
> >   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
> >   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> >   nvme0    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys1 nvme1n1
> >   nvme1    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1 nvme1n1
> >   nvme2    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1 nvme1n1, nvme1n2
> >   nvme3    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys3
> > 
> >   NVM Express Namespaces
> > 
> >   Device       NSID     Usage                      Format           Controllers
> >   ------------ -------- -------------------------- ---------------- ----------------
> >   nvme1n1      1        134.22  MB / 134.22  MB    512   B +  0 B   nvme0, nvme1, nvme2
> >   nvme1n2      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme2
> >   nvme3n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme3
> > 
> > Summary:
> >   - Refactored nvme-ns device not to rely on controller during the
> >     setup.  [1/11 - 5/11]
> >   - Introduced a nvme-subsys device model. [6/11]
> >   - Create subsystem NQN based on subsystem. [7/11]
> >   - Introduced multi-controller model. [8/11 - 9/11]
> >   - Updated namespace sharing scheme to be based on nvme-subsys
> >     hierarchy. [10/11 - 11/11]
> > 
> > Since RFC V1:
> >   - Updated namespace sharing scheme to be based on nvme-subsys
> >     hierarchy.
> > 
> 
> Great stuff Minwoo. Thanks!
> 
> I'll pick up [01-05/11] directly since they are pretty trivial.

Thanks! will prepare the next series based on there.

> The subsystem model looks pretty much like it should, I don't have a lot
> of comments.
> 
> One thing that I considered, is if we should reverse the "registration"
> and think about it as namespace attachment. The spec is about
> controllers attaching to namespaces, not the other way around.
> Basically, let the namespaces be configured first and register on the
> subsystem (accumulating in a "namespaces" array), then have the
> controllers register with the subsystem and attach to all "non-detached"
> namespaces. This allows detached namespaces to "linger" in the subsystem
> to be attached later on. If there are any private namespaces (like ns2
> in your example above), it will be defined after the controller with the
> bus=ctrlX parameter like normal.

Revisited spec. again.  5.19 says "The Namespace Attachment command is
used to attach and detach controllers from a namespace.".  and 5.20 says
"Host software uses the Namespace Attachment command to attach or detach
a namespace to or from a controller. The create operation does not attach
the namespace to a controller."

	-device nvme-subsys,id=subsys0
	-device nvme-ns,id=ns1,drive=<drv>,nsid=1,subsys=subsys0
	-device nvme,id=nvme0,serial=foo,subsys=subsys0

In this case, the 'nvme0' controller will have no namespace at the
initial time of the boot-up.  'nvme0' can be attached to the namespace
'ns1' with namespace attach command.  'nvme-ns' device is same as the
'create-ns' operation in a NVMe subsystem.  This makes sense as spec
5.19 says "from a namespace".

	-device nvme,id=nvme1,serial=bar,subsys=subsys0b
	-device nvme-ns,id=ns2,drive=<drv>,nsid=1,bus=nvme1

This case if for private namespace directly attached to controller.
This makes sense as spec 5.20 says "to or from a controller".

All looks fine to me, but one thing I an wondering is that how can we
attach a controller to shared namespace(s) at the initial time?

Re: [RFC PATCH V2 00/11] hw/block/nvme: support multi-path for ctrl/ns

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 7703 bytes --]

On Jan 19 12:21, Minwoo Im wrote:
> On 21-01-18 22:14:45, Klaus Jensen wrote:
> > On Jan 17 23:53, Minwoo Im wrote:
> > > Hello,
> > > 
> > > This patch series introduces NVMe subsystem device to support multi-path
> > > I/O in NVMe device model.  Two use-cases are supported along with this
> > > patch: Multi-controller, Namespace Sharing.
> > > 
> > > V1 RFC has been discussed with Klaus and Keith, I really appreciate them
> > > for this patch series to have proper direction [1].
> > > 
> > > This patch series contains few start-up refactoring pathces from the
> > > first to fifth patches to make nvme-ns device not to rely on the nvme
> > > controller always.  Because nvme-ns shall be able to be mapped to the
> > > subsystem level, not a single controller level so that it should provide
> > > generic initialization code: nvme_ns_setup() with NvmeCtrl.  To do that,
> > > the first five patches are to remove the NvmeCtrl * instance argument
> > > from the nvme_ns_setup().  I'd be happy if they are picked!
> > > 
> > > For controller and namespace devices, 'subsys' property has been
> > > introduced to map them to a subsystem.  If multi-controller needed, we
> > > can specify 'subsys' to controllers the same.
> > > 
> > > For namespace deivice, if 'subsys' is not given just like it was, it
> > > will have to be provided with 'bus' parameter to specify a nvme
> > > controller device to attach, it means, they are mutual-exlusive.  To
> > > share a namespace between or among controllers, then nvme-ns should have
> > > 'subsys' property to a single nvme subsystem instance.  To make a
> > > namespace private one, then we need to specify 'bus' property rather
> > > than the 'subsys'.
> > > 
> > > Of course, this series does not require any updates for the run command
> > > for the previos users.
> > > 
> > > Plase refer the following example with nvme-cli output:
> > > 
> > > QEMU Run:
> > >   -device nvme-subsys,id=subsys0 \
> > >   -device nvme,serial=foo,id=nvme0,subsys=subsys0 \
> > >   -device nvme,serial=bar,id=nvme1,subsys=subsys0 \
> > >   -device nvme,serial=baz,id=nvme2,subsys=subsys0 \
> > >   -device nvme-ns,id=ns1,drive=drv10,nsid=1,subsys=subsys0 \
> > >   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2 \
> > >   \
> > >   -device nvme,serial=qux,id=nvme3 \
> > >   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> > > 
> > > nvme-cli:
> > >   root@vm:~/work# nvme list -v
> > >   NVM Express Subsystems
> > > 
> > >   Subsystem        Subsystem-NQN                                                                                    Controllers
> > >   ---------------- ------------------------------------------------------------------------------------------------ ----------------
> > >   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme0, nvme1, nvme2
> > >   nvme-subsys3     nqn.2019-08.org.qemu:qux                                                                         nvme3
> > > 
> > >   NVM Express Controllers
> > > 
> > >   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
> > >   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> > >   nvme0    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys1 nvme1n1
> > >   nvme1    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1 nvme1n1
> > >   nvme2    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1 nvme1n1, nvme1n2
> > >   nvme3    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys3
> > > 
> > >   NVM Express Namespaces
> > > 
> > >   Device       NSID     Usage                      Format           Controllers
> > >   ------------ -------- -------------------------- ---------------- ----------------
> > >   nvme1n1      1        134.22  MB / 134.22  MB    512   B +  0 B   nvme0, nvme1, nvme2
> > >   nvme1n2      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme2
> > >   nvme3n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme3
> > > 
> > > Summary:
> > >   - Refactored nvme-ns device not to rely on controller during the
> > >     setup.  [1/11 - 5/11]
> > >   - Introduced a nvme-subsys device model. [6/11]
> > >   - Create subsystem NQN based on subsystem. [7/11]
> > >   - Introduced multi-controller model. [8/11 - 9/11]
> > >   - Updated namespace sharing scheme to be based on nvme-subsys
> > >     hierarchy. [10/11 - 11/11]
> > > 
> > > Since RFC V1:
> > >   - Updated namespace sharing scheme to be based on nvme-subsys
> > >     hierarchy.
> > > 
> > 
> > Great stuff Minwoo. Thanks!
> > 
> > I'll pick up [01-05/11] directly since they are pretty trivial.
> 
> Thanks! will prepare the next series based on there.
> 
> > The subsystem model looks pretty much like it should, I don't have a lot
> > of comments.
> > 
> > One thing that I considered, is if we should reverse the "registration"
> > and think about it as namespace attachment. The spec is about
> > controllers attaching to namespaces, not the other way around.
> > Basically, let the namespaces be configured first and register on the
> > subsystem (accumulating in a "namespaces" array), then have the
> > controllers register with the subsystem and attach to all "non-detached"
> > namespaces. This allows detached namespaces to "linger" in the subsystem
> > to be attached later on. If there are any private namespaces (like ns2
> > in your example above), it will be defined after the controller with the
> > bus=ctrlX parameter like normal.
> 
> Revisited spec. again.  5.19 says "The Namespace Attachment command is
> used to attach and detach controllers from a namespace.".  and 5.20 says
> "Host software uses the Namespace Attachment command to attach or detach
> a namespace to or from a controller. The create operation does not attach
> the namespace to a controller."
> 

Yeah ok, that is pretty inconsistent.

> 	-device nvme-subsys,id=subsys0
> 	-device nvme-ns,id=ns1,drive=<drv>,nsid=1,subsys=subsys0
> 	-device nvme,id=nvme0,serial=foo,subsys=subsys0
> 
> In this case, the 'nvme0' controller will have no namespace at the
> initial time of the boot-up.  'nvme0' can be attached to the namespace
> 'ns1' with namespace attach command.  'nvme-ns' device is same as the
> 'create-ns' operation in a NVMe subsystem.  This makes sense as spec
> 5.19 says "from a namespace".
> 
> 	-device nvme,id=nvme1,serial=bar,subsys=subsys0b
> 	-device nvme-ns,id=ns2,drive=<drv>,nsid=1,bus=nvme1
> 
> This case if for private namespace directly attached to controller.
> This makes sense as spec 5.20 says "to or from a controller".
> 
> All looks fine to me, but one thing I an wondering is that how can we
> attach a controller to shared namespace(s) at the initial time?
> 

Ok, nevermind. I think we can get 'detached' functionality in either
case, so no need to increase complexity by requiring a change of define
order.

Supporting CNS 0x12 and 0x13 (Identify, Controller List), we need the
controllers registered and stored in the subsystem anyway.

So, can we add a 'namespaces' array on the subsystem to keep a list of
namespaces and add a 'detached' parameter on the nvme-ns device? If that
parameter is given, the device is not registered with the controllers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 02/11] hw/block/nvme: open code for volatile write cache

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

On Jan 17 23:53, Minwoo Im wrote:
> Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
> initial time.  This feature is related to block device backed,  but this
> feature is controlled in controller level via Set/Get Features command.
> 
> This patch removed dependency between nvme and nvme-ns to manage the VWC
> flag value.  Also, it open coded the Get Features for VWC to check all
> namespaces attached to the controller, and if false detected, return
> directly false.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>  hw/block/nvme-ns.c |  4 ----
>  hw/block/nvme.c    | 15 ++++++++++++---
>  hw/block/nvme.h    |  1 -
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> index 32662230130b..c403cd36b6bd 100644
> --- a/hw/block/nvme-ns.c
> +++ b/hw/block/nvme-ns.c
> @@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
>          return -1;
>      }
>  
> -    if (blk_enable_write_cache(ns->blkconf.blk)) {
> -        n->features.vwc = 0x1;
> -    }
> -
>      return 0;
>  }
>  
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index cf0fe28fe6eb..b2a9c48a9d81 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>      NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
>      uint16_t iv;
>      NvmeNamespace *ns;
> +    int i;
>  
>      static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
>          [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> @@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>          result = ns->features.err_rec;
>          goto out;
>      case NVME_VOLATILE_WRITE_CACHE:
> -        result = n->features.vwc;
> +        for (i = 1; i <= n->num_namespaces; i++) {
> +            ns = nvme_ns(n, i);
> +            if (!ns) {
> +                continue;
> +            }
> +
> +            result = blk_enable_write_cache(ns->blkconf.blk);
> +            if (!result) {
> +                break;
> +            }
> +        }

I did a small tweak here and changed `if (!result)` to `if (result)`. We
want to report that a volatile write cache is present if ANY of the
namespace backing devices have a write cache.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH V2 00/11] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

On 21-01-19 07:04:04, Klaus Jensen wrote:
> On Jan 19 12:21, Minwoo Im wrote:
> > On 21-01-18 22:14:45, Klaus Jensen wrote:
> > > On Jan 17 23:53, Minwoo Im wrote:
> > > > Hello,
> > > > 
> > > > This patch series introduces NVMe subsystem device to support multi-path
> > > > I/O in NVMe device model.  Two use-cases are supported along with this
> > > > patch: Multi-controller, Namespace Sharing.
> > > > 
> > > > V1 RFC has been discussed with Klaus and Keith, I really appreciate them
> > > > for this patch series to have proper direction [1].
> > > > 
> > > > This patch series contains few start-up refactoring pathces from the
> > > > first to fifth patches to make nvme-ns device not to rely on the nvme
> > > > controller always.  Because nvme-ns shall be able to be mapped to the
> > > > subsystem level, not a single controller level so that it should provide
> > > > generic initialization code: nvme_ns_setup() with NvmeCtrl.  To do that,
> > > > the first five patches are to remove the NvmeCtrl * instance argument
> > > > from the nvme_ns_setup().  I'd be happy if they are picked!
> > > > 
> > > > For controller and namespace devices, 'subsys' property has been
> > > > introduced to map them to a subsystem.  If multi-controller needed, we
> > > > can specify 'subsys' to controllers the same.
> > > > 
> > > > For namespace deivice, if 'subsys' is not given just like it was, it
> > > > will have to be provided with 'bus' parameter to specify a nvme
> > > > controller device to attach, it means, they are mutual-exlusive.  To
> > > > share a namespace between or among controllers, then nvme-ns should have
> > > > 'subsys' property to a single nvme subsystem instance.  To make a
> > > > namespace private one, then we need to specify 'bus' property rather
> > > > than the 'subsys'.
> > > > 
> > > > Of course, this series does not require any updates for the run command
> > > > for the previos users.
> > > > 
> > > > Plase refer the following example with nvme-cli output:
> > > > 
> > > > QEMU Run:
> > > >   -device nvme-subsys,id=subsys0 \
> > > >   -device nvme,serial=foo,id=nvme0,subsys=subsys0 \
> > > >   -device nvme,serial=bar,id=nvme1,subsys=subsys0 \
> > > >   -device nvme,serial=baz,id=nvme2,subsys=subsys0 \
> > > >   -device nvme-ns,id=ns1,drive=drv10,nsid=1,subsys=subsys0 \
> > > >   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2 \
> > > >   \
> > > >   -device nvme,serial=qux,id=nvme3 \
> > > >   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> > > > 
> > > > nvme-cli:
> > > >   root@vm:~/work# nvme list -v
> > > >   NVM Express Subsystems
> > > > 
> > > >   Subsystem        Subsystem-NQN                                                                                    Controllers
> > > >   ---------------- ------------------------------------------------------------------------------------------------ ----------------
> > > >   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme0, nvme1, nvme2
> > > >   nvme-subsys3     nqn.2019-08.org.qemu:qux                                                                         nvme3
> > > > 
> > > >   NVM Express Controllers
> > > > 
> > > >   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
> > > >   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> > > >   nvme0    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys1 nvme1n1
> > > >   nvme1    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1 nvme1n1
> > > >   nvme2    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1 nvme1n1, nvme1n2
> > > >   nvme3    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys3
> > > > 
> > > >   NVM Express Namespaces
> > > > 
> > > >   Device       NSID     Usage                      Format           Controllers
> > > >   ------------ -------- -------------------------- ---------------- ----------------
> > > >   nvme1n1      1        134.22  MB / 134.22  MB    512   B +  0 B   nvme0, nvme1, nvme2
> > > >   nvme1n2      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme2
> > > >   nvme3n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme3
> > > > 
> > > > Summary:
> > > >   - Refactored nvme-ns device not to rely on controller during the
> > > >     setup.  [1/11 - 5/11]
> > > >   - Introduced a nvme-subsys device model. [6/11]
> > > >   - Create subsystem NQN based on subsystem. [7/11]
> > > >   - Introduced multi-controller model. [8/11 - 9/11]
> > > >   - Updated namespace sharing scheme to be based on nvme-subsys
> > > >     hierarchy. [10/11 - 11/11]
> > > > 
> > > > Since RFC V1:
> > > >   - Updated namespace sharing scheme to be based on nvme-subsys
> > > >     hierarchy.
> > > > 
> > > 
> > > Great stuff Minwoo. Thanks!
> > > 
> > > I'll pick up [01-05/11] directly since they are pretty trivial.
> > 
> > Thanks! will prepare the next series based on there.
> > 
> > > The subsystem model looks pretty much like it should, I don't have a lot
> > > of comments.
> > > 
> > > One thing that I considered, is if we should reverse the "registration"
> > > and think about it as namespace attachment. The spec is about
> > > controllers attaching to namespaces, not the other way around.
> > > Basically, let the namespaces be configured first and register on the
> > > subsystem (accumulating in a "namespaces" array), then have the
> > > controllers register with the subsystem and attach to all "non-detached"
> > > namespaces. This allows detached namespaces to "linger" in the subsystem
> > > to be attached later on. If there are any private namespaces (like ns2
> > > in your example above), it will be defined after the controller with the
> > > bus=ctrlX parameter like normal.
> > 
> > Revisited spec. again.  5.19 says "The Namespace Attachment command is
> > used to attach and detach controllers from a namespace.".  and 5.20 says
> > "Host software uses the Namespace Attachment command to attach or detach
> > a namespace to or from a controller. The create operation does not attach
> > the namespace to a controller."
> > 
> 
> Yeah ok, that is pretty inconsistent.
> 
> > 	-device nvme-subsys,id=subsys0
> > 	-device nvme-ns,id=ns1,drive=<drv>,nsid=1,subsys=subsys0
> > 	-device nvme,id=nvme0,serial=foo,subsys=subsys0
> > 
> > In this case, the 'nvme0' controller will have no namespace at the
> > initial time of the boot-up.  'nvme0' can be attached to the namespace
> > 'ns1' with namespace attach command.  'nvme-ns' device is same as the
> > 'create-ns' operation in a NVMe subsystem.  This makes sense as spec
> > 5.19 says "from a namespace".
> > 
> > 	-device nvme,id=nvme1,serial=bar,subsys=subsys0b
> > 	-device nvme-ns,id=ns2,drive=<drv>,nsid=1,bus=nvme1
> > 
> > This case if for private namespace directly attached to controller.
> > This makes sense as spec 5.20 says "to or from a controller".
> > 
> > All looks fine to me, but one thing I an wondering is that how can we
> > attach a controller to shared namespace(s) at the initial time?
> > 
> 
> Ok, nevermind. I think we can get 'detached' functionality in either
> case, so no need to increase complexity by requiring a change of define
> order.
> 
> Supporting CNS 0x12 and 0x13 (Identify, Controller List), we need the
> controllers registered and stored in the subsystem anyway.
> 
> So, can we add a 'namespaces' array on the subsystem to keep a list of
> namespaces and add a 'detached' parameter on the nvme-ns device? If that
> parameter is given, the device is not registered with the controllers.

Sure, will do that.  Plese let me have V3 series.  Thanks!

Re: [RFC PATCH V2 02/11] hw/block/nvme: open code for volatile write cache

[Minwoo Im]

On 21-01-19 08:32:21, Klaus Jensen wrote:
> On Jan 17 23:53, Minwoo Im wrote:
> > Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
> > initial time.  This feature is related to block device backed,  but this
> > feature is controlled in controller level via Set/Get Features command.
> > 
> > This patch removed dependency between nvme and nvme-ns to manage the VWC
> > flag value.  Also, it open coded the Get Features for VWC to check all
> > namespaces attached to the controller, and if false detected, return
> > directly false.
> > 
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> >  hw/block/nvme-ns.c |  4 ----
> >  hw/block/nvme.c    | 15 ++++++++++++---
> >  hw/block/nvme.h    |  1 -
> >  3 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index 32662230130b..c403cd36b6bd 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
> > @@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> >          return -1;
> >      }
> >  
> > -    if (blk_enable_write_cache(ns->blkconf.blk)) {
> > -        n->features.vwc = 0x1;
> > -    }
> > -
> >      return 0;
> >  }
> >  
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index cf0fe28fe6eb..b2a9c48a9d81 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
> >      NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> >      uint16_t iv;
> >      NvmeNamespace *ns;
> > +    int i;
> >  
> >      static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> >          [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > @@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
> >          result = ns->features.err_rec;
> >          goto out;
> >      case NVME_VOLATILE_WRITE_CACHE:
> > -        result = n->features.vwc;
> > +        for (i = 1; i <= n->num_namespaces; i++) {
> > +            ns = nvme_ns(n, i);
> > +            if (!ns) {
> > +                continue;
> > +            }
> > +
> > +            result = blk_enable_write_cache(ns->blkconf.blk);
> > +            if (!result) {
> > +                break;
> > +            }
> > +        }
> 
> I did a small tweak here and changed `if (!result)` to `if (result)`. We
> want to report that a volatile write cache is present if ANY of the
> namespace backing devices have a write cache.

Oh.... Thanks for the fix!

Re: [RFC PATCH V2 02/11] hw/block/nvme: open code for volatile write cache

[Sai Pavan Boddu]

Hi Minwoo,

On Sun, Jan 17, 2021 at 11:53:32PM +0900, Minwoo Im wrote:
> Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
> initial time.  This feature is related to block device backed,  but this
> feature is controlled in controller level via Set/Get Features command.
> 
> This patch removed dependency between nvme and nvme-ns to manage the VWC
> flag value.  Also, it open coded the Get Features for VWC to check all
> namespaces attached to the controller, and if false detected, return
> directly false.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>  hw/block/nvme-ns.c |  4 ----
>  hw/block/nvme.c    | 15 ++++++++++++---
>  hw/block/nvme.h    |  1 -
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> index 32662230130b..c403cd36b6bd 100644
> --- a/hw/block/nvme-ns.c
> +++ b/hw/block/nvme-ns.c
> @@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
>          return -1;
>      }
>  
> -    if (blk_enable_write_cache(ns->blkconf.blk)) {
> -        n->features.vwc = 0x1;
> -    }
> -
>      return 0;
>  }
>  
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index cf0fe28fe6eb..b2a9c48a9d81 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>      NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
>      uint16_t iv;
>      NvmeNamespace *ns;
> +    int i;
>  
>      static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
>          [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> @@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>          result = ns->features.err_rec;
>          goto out;
>      case NVME_VOLATILE_WRITE_CACHE:
> -        result = n->features.vwc;
>
> This change breaks the build on ubuntu 16.04, gcc is detecting error
> as below
> ../hw/block/nvme.c: In function ‘nvme_process_sq’:
> ../hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" :
>          "disabled");
>                   ^
>                   ../hw/block/nvme.c:3150:14: note: ‘result’ was
>                   declared here
>                        uint32_t result;
>                                      ^
>GCC version  5.4.0.
>Tested initilizing result variable, build looks good then.
>
>Regards,
>Sai Pavan
>
> +        for (i = 1; i <= n->num_namespaces; i++) {
> +            ns = nvme_ns(n, i);
> +            if (!ns) {
> +                continue;
> +            }
> +
> +            result = blk_enable_write_cache(ns->blkconf.blk);
> +            if (!result) {
> +                break;
> +            }
> +        }
>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>          goto out;
>      case NVME_ASYNCHRONOUS_EVENT_CONF:
> @@ -3271,8 +3282,6 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
>          ns->features.err_rec = dw11;
>          break;
>      case NVME_VOLATILE_WRITE_CACHE:
> -        n->features.vwc = dw11 & 0x1;
> -
>          for (i = 1; i <= n->num_namespaces; i++) {
>              ns = nvme_ns(n, i);
>              if (!ns) {
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index b7fbcca39d9f..5ba83ee77841 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -117,7 +117,6 @@ typedef struct NvmeFeatureVal {
>          uint16_t temp_thresh_low;
>      };
>      uint32_t    async_config;
> -    uint32_t    vwc;
>  } NvmeFeatureVal;
>  
>  typedef struct NvmeCtrl {
> -- 
> 2.17.1
> 
> 

Re: [RFC PATCH V2 02/11] hw/block/nvme: open code for volatile write cache

[Sai Pavan Boddu]

Hi Minwoo,

Please ignore this mail, I see a fix already floating around in the list.

Regards,
Sai Pavan
On Thu, Feb 11, 2021 at 12:15:57PM +0530, Sai Pavan Boddu wrote:
> Hi Minwoo,
> 
> On Sun, Jan 17, 2021 at 11:53:32PM +0900, Minwoo Im wrote:
> > Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
> > initial time.  This feature is related to block device backed,  but this
> > feature is controlled in controller level via Set/Get Features command.
> > 
> > This patch removed dependency between nvme and nvme-ns to manage the VWC
> > flag value.  Also, it open coded the Get Features for VWC to check all
> > namespaces attached to the controller, and if false detected, return
> > directly false.
> > 
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> >  hw/block/nvme-ns.c |  4 ----
> >  hw/block/nvme.c    | 15 ++++++++++++---
> >  hw/block/nvme.h    |  1 -
> >  3 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index 32662230130b..c403cd36b6bd 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
> > @@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> >          return -1;
> >      }
> >  
> > -    if (blk_enable_write_cache(ns->blkconf.blk)) {
> > -        n->features.vwc = 0x1;
> > -    }
> > -
> >      return 0;
> >  }
> >  
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index cf0fe28fe6eb..b2a9c48a9d81 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
> >      NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> >      uint16_t iv;
> >      NvmeNamespace *ns;
> > +    int i;
> >  
> >      static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> >          [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > @@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
> >          result = ns->features.err_rec;
> >          goto out;
> >      case NVME_VOLATILE_WRITE_CACHE:
> > -        result = n->features.vwc;
> >
> > This change breaks the build on ubuntu 16.04, gcc is detecting error
> > as below
> > ../hw/block/nvme.c: In function ‘nvme_process_sq’:
> > ../hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized
> > in this function [-Werror=maybe-uninitialized]
> >          trace_pci_nvme_getfeat_vwcache(result ? "enabled" :
> >          "disabled");
> >                   ^
> >                   ../hw/block/nvme.c:3150:14: note: ‘result’ was
> >                   declared here
> >                        uint32_t result;
> >                                      ^
> >GCC version  5.4.0.
> >Tested initilizing result variable, build looks good then.
> >
> >Regards,
> >Sai Pavan
> >
> > +        for (i = 1; i <= n->num_namespaces; i++) {
> > +            ns = nvme_ns(n, i);
> > +            if (!ns) {
> > +                continue;
> > +            }
> > +
> > +            result = blk_enable_write_cache(ns->blkconf.blk);
> > +            if (!result) {
> > +                break;
> > +            }
> > +        }
> >          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> >          goto out;
> >      case NVME_ASYNCHRONOUS_EVENT_CONF:
> > @@ -3271,8 +3282,6 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
> >          ns->features.err_rec = dw11;
> >          break;
> >      case NVME_VOLATILE_WRITE_CACHE:
> > -        n->features.vwc = dw11 & 0x1;
> > -
> >          for (i = 1; i <= n->num_namespaces; i++) {
> >              ns = nvme_ns(n, i);
> >              if (!ns) {
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index b7fbcca39d9f..5ba83ee77841 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -117,7 +117,6 @@ typedef struct NvmeFeatureVal {
> >          uint16_t temp_thresh_low;
> >      };
> >      uint32_t    async_config;
> > -    uint32_t    vwc;
> >  } NvmeFeatureVal;
> >  
> >  typedef struct NvmeCtrl {
> > -- 
> > 2.17.1
> > 
> > 
>