[RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

[RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

Hello,

This patch series is third one to support multi-controller and namespace
sharing in multi-path.  This series introduced subsystem scheme to
manage controller(s) and namespace(s) in the subsystem.

This series has new patches from the V2:  'detached' parameter has been
added to the nvme-ns device.  This will decide whether to attach the
namespace to controller(s) in the current subsystem or not.  If it's
given with true, then it will be just allocated in the subsystem, but
not attaching to any controllers in the subsystem.  Otherwise, it will
automatically attach to all the controllers in the subsystem.  The other
t hing is that the last patch implemented Identify Active Namespace ID
List command handler apart from the Allocated Namespace ID List.

Run with:
  -device nvme,serial=qux,id=nvme3
  -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3

  -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,detached=true
  -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2

nvme-cli:
  root@vm:~/work# nvme list -v                                                                                                      
  NVM Express Subsystems                                                                                                 
                                                                                                                                     
  Subsystem        Subsystem-NQN                                                                                    Controllers
  ---------------- ------------------------------------------------------------------------------------------------ ----------------
  nvme-subsys0     nqn.2019-08.org.qemu:qux                                                                         nvme0
  nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme1, nvme2, nvme3
                                                                                                                                   
  NVM Express Controllers                                                                                           
                                                                                                                  
  Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
  -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
  nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0
  nvme1    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1
  nvme2    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1
  nvme3    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys1 nvme1n1
                                                                                  
  NVM Express Namespaces                                                 
                                                                       
  Device       NSID     Usage                      Format           Controllers
  ------------ -------- -------------------------- ---------------- ----------------
  nvme0n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme0
  nvme1n1      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme3

Now we have [allocated|not-allocated]/[attached/detached] scheme for
namespaces and controllers.  The next series is going to be to support
namespace management and attachment with few Identify command handlers.

Please review.

Thanks!

Since RFC V2:
  - Rebased on nvme-next branch with trivial patches from the previous
    version(V2) applied. (Klaus)
  - Fix enumeration type name convention with NvmeIdNs prefix. (Klaus)
  - Put 'cntlid' to NvmeCtrl instance in nvme_init_ctrl() which was
    missed in V2.
  - Added 'detached' parameter to nvme-ns device to decide whether to
    attach or not to controller(s) in the subsystem. (Klaus)
  - Implemented Identify Active Namespace ID List aprt from Identify
    Allocated Namespace ID List by removing fall-thru statement.

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

Minwoo Im (8):
  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/nvme: add 'detached' param not to attach namespace
  hw/block/nvme: Add Identify Active Namespace ID List

 hw/block/meson.build   |   2 +-
 hw/block/nvme-ns.c     |  32 ++++++++--
 hw/block/nvme-ns.h     |  13 ++++
 hw/block/nvme-subsys.c | 111 +++++++++++++++++++++++++++++++++
 hw/block/nvme-subsys.h |  32 ++++++++++
 hw/block/nvme.c        | 137 ++++++++++++++++++++++++++++++++++++++---
 hw/block/nvme.h        |  19 ++++++
 include/block/nvme.h   |   8 +++
 8 files changed, 340 insertions(+), 14 deletions(-)
 create mode 100644 hw/block/nvme-subsys.c
 create mode 100644 hw/block/nvme-subsys.h

-- 
2.17.1

[RFC PATCH V3 1/8] 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 309c26db8ff7..4644937a5c50 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 V3 2/8] 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 4644937a5c50..3e3b5451ea3d 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
@@ -4281,11 +4289,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));
@@ -4331,9 +4353,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);
@@ -4417,6 +4437,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 347c149e7905..3fa0e0a15539 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
@@ -158,6 +159,8 @@ typedef struct NvmeCtrl {
 
     uint8_t     zasl;
 
+    NvmeSubsystem   *subsys;
+
     NvmeNamespace   namespace;
     NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES];
     NvmeSQueue      **sq;
-- 
2.17.1

[RFC PATCH V3 3/8] 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 45b2678db1f0..733fb35fedde 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 V3 4/8] 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        | 34 ++++++++++++++++++++++++++++++++--
 hw/block/nvme.h        |  1 +
 4 files changed, 58 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 3e3b5451ea3d..9f8a739fcd8f 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4304,16 +4304,21 @@ 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;
 
+    n->cntlid = cntlid;
+
     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));
     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;
@@ -4359,6 +4364,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);
@@ -4371,11 +4380,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) {
@@ -4391,7 +4417,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 3fa0e0a15539..c158cc873b59 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -133,6 +133,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 V3 5/8] 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 733fb35fedde..28404a62b9d2 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1110,6 +1110,10 @@ enum NvmeNsIdentifierType {
     NVME_NIDT_CSI               = 0x04,
 };
 
+enum NvmeIdNsNmic {
+    NVME_NMIC_NS_SHARED         = 1 << 0,
+};
+
 enum NvmeCsi {
     NVME_CSI_NVM                = 0x00,
     NVME_CSI_ZONED              = 0x02,
-- 
2.17.1

[RFC PATCH V3 6/8] 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.

All the namespace with 'subsys' parameter will attach all controllers in
the subsystem to the namespace by default.

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 | 25 +++++++++++++++++++++++++
 hw/block/nvme-subsys.h |  3 +++
 hw/block/nvme.c        | 10 +++++++++-
 5 files changed, 62 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 293ac990e3f6..929e78861903 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..e7efdcae7d0d 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -43,6 +43,31 @@ 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;
+
+    if (subsys->namespaces[nvme_nsid(ns)]) {
+        error_setg(errp, "namespace %d already registerd to subsy %s",
+                   nvme_nsid(ns), subsys->parent_obj.id);
+        return -1;
+    }
+
+    subsys->namespaces[nvme_nsid(ns)] = ns;
+
+    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..ccf6a71398d3 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -14,6 +14,7 @@
     OBJECT_CHECK(NvmeSubsystem, (obj), TYPE_NVME_SUBSYS)
 
 #define NVME_SUBSYS_MAX_CTRLS   32
+#define NVME_SUBSYS_MAX_NAMESPACES  32
 
 typedef struct NvmeCtrl NvmeCtrl;
 typedef struct NvmeNamespace NvmeNamespace;
@@ -22,8 +23,10 @@ typedef struct NvmeSubsystem {
     uint8_t     subnqn[256];
 
     NvmeCtrl    *ctrls[NVME_SUBSYS_MAX_CTRLS];
+    NvmeNamespace *namespaces[NVME_SUBSYS_MAX_NAMESPACES];
 } 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 9f8a739fcd8f..06bccf1b9e9e 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

[RFC PATCH V3 7/8] hw/block/nvme: add 'detached' param not to attach namespace

[Minwoo Im]

Introduced 'detached' parameter to nvme-ns device.  If given, the
namespace will not be attached to controller(s) in the subsystem.  If
'subsys' is not given with this option, it should be provided with 'bus'
which is for private namespace.

This patch also introduced 'ctrls_bitmap' in NvmeNamespace instance to
represent which controler id(cntlid) is attached to this namespace
device.  A namespace can be attached to multiple controllers in a
subsystem so that this bitmap maps those two relationships.

The ctrls_bitmap bitmap should not be accessed directly, but through the
helpers introduced in this patch: nvme_ns_is_attached(),
nvme_ns_attach(), nvme_ns_detach().

Note that this patch made identify namespace list data not hold
non-attached namespace ID in nvme_identify_nslist.  Currently, this
command handler is for CNS 0x2(Active) and 0x10(Allocated) both.  The
next patch will introduce a handler for later on.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 hw/block/nvme-ns.c     |  9 +++++++++
 hw/block/nvme-ns.h     |  6 ++++++
 hw/block/nvme-subsys.c |  2 ++
 hw/block/nvme.c        | 31 ++++++++++++++++++++++++++++++-
 hw/block/nvme.h        | 15 +++++++++++++++
 5 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 073f65e49cac..70d42c24065c 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -17,6 +17,7 @@
 #include "qemu/cutils.h"
 #include "qemu/log.h"
 #include "qemu/error-report.h"
+#include "qemu/hbitmap.h"
 #include "hw/block/block.h"
 #include "hw/pci/pci.h"
 #include "sysemu/sysemu.h"
@@ -304,6 +305,11 @@ static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp)
     return 0;
 }
 
+static void nvme_ns_init_state(NvmeNamespace *ns)
+{
+    ns->ctrls_bitmap = hbitmap_alloc(NVME_SUBSYS_MAX_CTRLS, 0);
+}
+
 int nvme_ns_setup(NvmeNamespace *ns, Error **errp)
 {
     if (nvme_ns_check_constraints(ns, errp)) {
@@ -314,6 +320,8 @@ int nvme_ns_setup(NvmeNamespace *ns, Error **errp)
         return -1;
     }
 
+    nvme_ns_init_state(ns);
+
     if (nvme_ns_init(ns, errp)) {
         return -1;
     }
@@ -381,6 +389,7 @@ static Property nvme_ns_props[] = {
     DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf),
     DEFINE_PROP_LINK("subsys", NvmeNamespace, subsys, TYPE_NVME_SUBSYS,
                      NvmeSubsystem *),
+    DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false),
     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 929e78861903..ad2f55931d1b 100644
--- a/hw/block/nvme-ns.h
+++ b/hw/block/nvme-ns.h
@@ -26,6 +26,7 @@ typedef struct NvmeZone {
 } NvmeZone;
 
 typedef struct NvmeNamespaceParams {
+    bool     detached;
     uint32_t nsid;
     QemuUUID uuid;
 
@@ -48,6 +49,11 @@ typedef struct NvmeNamespace {
     uint8_t      csi;
 
     NvmeSubsystem   *subsys;
+    /*
+     * Whether this namespace is attached to a controller or not.  This bitmap
+     * is based on controller id.  This is valid only in case 'subsys' != NULL.
+     */
+    HBitmap         *ctrls_bitmap;
 
     NvmeIdNsZoned   *id_ns_zoned;
     NvmeZone        *zone_array;
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
index e7efdcae7d0d..32ad8ef2825a 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -11,6 +11,7 @@
 #include "qemu/uuid.h"
 #include "qemu/iov.h"
 #include "qemu/cutils.h"
+#include "qemu/hbitmap.h"
 #include "qapi/error.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-core.h"
@@ -20,6 +21,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/pci/pci.h"
 #include "nvme.h"
+#include "nvme-ns.h"
 #include "nvme-subsys.h"
 
 int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 06bccf1b9e9e..2b2c07b36c2b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -26,7 +26,7 @@
  *              ,subsys=<subsys_id> \
  *      -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
  *              zoned=<true|false[optional]>, \
- *              subsys=<subsys_id>
+ *              subsys=<subsys_id>,detached=<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
@@ -77,6 +77,13 @@
  *   controllers in the subsystem. Otherwise, `bus` must be given to attach
  *   this namespace to a specified single controller as a non-shared namespace.
  *
+ * - `detached`
+ *   Not to attach the namespace device to controllers in the NVMe subsystem
+ *   during boot-up.  If not given, namespaces are all attached to all
+ *   controllers in the subsystem by default.
+ *   It's mutual exclusive with 'bus' paraemter.  It's only valid in case
+ *   'subsys' is provided.
+ *
  * Setting `zoned` to true selects Zoned Command Set at the namespace.
  * In this case, the following namespace properties are available to configure
  * zoned operation:
@@ -2906,6 +2913,11 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
         if (ns->params.nsid <= min_nsid) {
             continue;
         }
+
+        if (!nvme_ns_is_attached(n, ns)) {
+            continue;
+        }
+
         list_ptr[j++] = cpu_to_le32(ns->params.nsid);
         if (j == data_len / sizeof(uint32_t)) {
             break;
@@ -4146,6 +4158,19 @@ static void nvme_init_state(NvmeCtrl *n)
     n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
 }
 
+static int nvme_attach_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
+{
+    if (nvme_ns_is_attached(n, ns)) {
+        error_setg(errp,
+                   "namespace %d is already attached to controller %d",
+                   nvme_nsid(ns), n->cntlid);
+        return -1;
+    }
+
+    nvme_ns_attach(n, ns);
+    return 0;
+}
+
 int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
 {
     uint32_t nsid = nvme_nsid(ns);
@@ -4179,6 +4204,10 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
 
     n->namespaces[nsid - 1] = ns;
 
+    if (!ns->params.detached) {
+        return nvme_attach_namespace(n, ns, errp);
+    }
+
     return 0;
 }
 
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index c158cc873b59..582e6d4e8c40 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -181,6 +181,21 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
     return n->namespaces[nsid - 1];
 }
 
+static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
+{
+    return hbitmap_get(ns->ctrls_bitmap, n->cntlid);
+}
+
+static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
+{
+    hbitmap_set(ns->ctrls_bitmap, n->cntlid, 1);
+}
+
+static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
+{
+    hbitmap_reset(ns->ctrls_bitmap, n->cntlid, 1);
+}
+
 static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
 {
     NvmeSQueue *sq = req->sq;
-- 
2.17.1

[RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID List

[Minwoo Im]

Spec v1.4b 6.1.4 "Active and Inactive NSID Types" says:

"Active NSIDs for a controller refer to namespaces that are attached to
that controller. Allocated NSIDs that are inactive for a controller refer
to namespaces that are not attached to that controller."

This patch introduced for Identify Active Namespace ID List (CNS 02h).

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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2b2c07b36c2b..7247167b0ee6 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2883,6 +2883,39 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req)
     return NVME_INVALID_FIELD | NVME_DNR;
 }
 
+static uint16_t nvme_identify_nslist_active(NvmeCtrl *n, NvmeRequest *req)
+{
+    NvmeNamespace *ns;
+    NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
+    uint32_t min_nsid = le32_to_cpu(c->nsid);
+    uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
+    static const int data_len = sizeof(list);
+    uint32_t *list_ptr = (uint32_t *)list;
+    int i, j = 0;
+
+    if (min_nsid >= NVME_NSID_BROADCAST - 1) {
+        return NVME_INVALID_NSID | NVME_DNR;
+    }
+
+    for (i = 1; i <= n->num_namespaces; i++) {
+        ns = nvme_ns(n, i);
+        if (!ns || ns->params.nsid <= min_nsid) {
+            continue;
+        }
+
+        if (!nvme_ns_is_attached(n, ns)) {
+            continue;
+        }
+
+        list_ptr[j++] = cpu_to_le32(ns->params.nsid);
+        if (j == data_len / sizeof(uint32_t)) {
+            break;
+        }
+    }
+
+    return nvme_dma(n, list, data_len, DMA_DIRECTION_FROM_DEVICE, req);
+}
+
 static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
 {
     NvmeNamespace *ns;
@@ -2914,10 +2947,6 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
             continue;
         }
 
-        if (!nvme_ns_is_attached(n, ns)) {
-            continue;
-        }
-
         list_ptr[j++] = cpu_to_le32(ns->params.nsid);
         if (j == data_len / sizeof(uint32_t)) {
             break;
@@ -3045,7 +3074,7 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
     case NVME_ID_CNS_CS_CTRL:
         return nvme_identify_ctrl_csi(n, req);
     case NVME_ID_CNS_NS_ACTIVE_LIST:
-         /* fall through */
+         return nvme_identify_nslist_active(n, req);
     case NVME_ID_CNS_NS_PRESENT_LIST:
         return nvme_identify_nslist(n, req);
     case NVME_ID_CNS_CS_NS_ACTIVE_LIST:
-- 
2.17.1

Re: [RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

[Klaus Jensen]

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

On Jan 20 02:01, Minwoo Im wrote:
> Hello,
> 
> This patch series is third one to support multi-controller and namespace
> sharing in multi-path.  This series introduced subsystem scheme to
> manage controller(s) and namespace(s) in the subsystem.
> 
> This series has new patches from the V2:  'detached' parameter has been
> added to the nvme-ns device.  This will decide whether to attach the
> namespace to controller(s) in the current subsystem or not.  If it's
> given with true, then it will be just allocated in the subsystem, but
> not attaching to any controllers in the subsystem.  Otherwise, it will
> automatically attach to all the controllers in the subsystem.  The other
> t hing is that the last patch implemented Identify Active Namespace ID
> List command handler apart from the Allocated Namespace ID List.
> 
> Run with:
>   -device nvme,serial=qux,id=nvme3
>   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> 
>   -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,detached=true
>   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2
> 
> nvme-cli:
>   root@vm:~/work# nvme list -v                                                                                                      
>   NVM Express Subsystems                                                                                                 
>                                                                                                                                      
>   Subsystem        Subsystem-NQN                                                                                    Controllers
>   ---------------- ------------------------------------------------------------------------------------------------ ----------------
>   nvme-subsys0     nqn.2019-08.org.qemu:qux                                                                         nvme0
>   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme1, nvme2, nvme3
>                                                                                                                                    
>   NVM Express Controllers                                                                                           
>                                                                                                                   
>   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
>   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
>   nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0

Shouldn't nvme0n1 be listed under Namespaces for nvme0?

>   nvme1    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1
>   nvme2    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1
>   nvme3    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys1 nvme1n1
>                                                                                   
>   NVM Express Namespaces                                                 
>                                                                        
>   Device       NSID     Usage                      Format           Controllers
>   ------------ -------- -------------------------- ---------------- ----------------
>   nvme0n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme0
>   nvme1n1      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme3
> 
> Now we have [allocated|not-allocated]/[attached/detached] scheme for
> namespaces and controllers.  The next series is going to be to support
> namespace management and attachment with few Identify command handlers.
> 
> Please review.
> 
> Thanks!
> 
> Since RFC V2:
>   - Rebased on nvme-next branch with trivial patches from the previous
>     version(V2) applied. (Klaus)
>   - Fix enumeration type name convention with NvmeIdNs prefix. (Klaus)
>   - Put 'cntlid' to NvmeCtrl instance in nvme_init_ctrl() which was
>     missed in V2.
>   - Added 'detached' parameter to nvme-ns device to decide whether to
>     attach or not to controller(s) in the subsystem. (Klaus)
>   - Implemented Identify Active Namespace ID List aprt from Identify
>     Allocated Namespace ID List by removing fall-thru statement.
> 
> Since RFC V1:
>   - Updated namespace sharing scheme to be based on nvme-subsys
>     hierarchy.
> 
> Minwoo Im (8):
>   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/nvme: add 'detached' param not to attach namespace
>   hw/block/nvme: Add Identify Active Namespace ID List
> 
>  hw/block/meson.build   |   2 +-
>  hw/block/nvme-ns.c     |  32 ++++++++--
>  hw/block/nvme-ns.h     |  13 ++++
>  hw/block/nvme-subsys.c | 111 +++++++++++++++++++++++++++++++++
>  hw/block/nvme-subsys.h |  32 ++++++++++
>  hw/block/nvme.c        | 137 ++++++++++++++++++++++++++++++++++++++---
>  hw/block/nvme.h        |  19 ++++++
>  include/block/nvme.h   |   8 +++
>  8 files changed, 340 insertions(+), 14 deletions(-)
>  create mode 100644 hw/block/nvme-subsys.c
>  create mode 100644 hw/block/nvme-subsys.h
> 
> -- 
> 2.17.1
> 
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.

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

Re: [RFC PATCH V3 7/8] hw/block/nvme: add 'detached' param not to attach namespace

[Klaus Jensen]

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

On Jan 20 02:01, Minwoo Im wrote:
> Introduced 'detached' parameter to nvme-ns device.  If given, the
> namespace will not be attached to controller(s) in the subsystem.  If
> 'subsys' is not given with this option, it should be provided with 'bus'
> which is for private namespace.
> 
> This patch also introduced 'ctrls_bitmap' in NvmeNamespace instance to
> represent which controler id(cntlid) is attached to this namespace
> device.  A namespace can be attached to multiple controllers in a
> subsystem so that this bitmap maps those two relationships.
> 
> The ctrls_bitmap bitmap should not be accessed directly, but through the
> helpers introduced in this patch: nvme_ns_is_attached(),
> nvme_ns_attach(), nvme_ns_detach().
> 
> Note that this patch made identify namespace list data not hold
> non-attached namespace ID in nvme_identify_nslist.  Currently, this
> command handler is for CNS 0x2(Active) and 0x10(Allocated) both.  The
> next patch will introduce a handler for later on.
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>  hw/block/nvme-ns.c     |  9 +++++++++
>  hw/block/nvme-ns.h     |  6 ++++++
>  hw/block/nvme-subsys.c |  2 ++
>  hw/block/nvme.c        | 31 ++++++++++++++++++++++++++++++-
>  hw/block/nvme.h        | 15 +++++++++++++++
>  5 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> index 073f65e49cac..70d42c24065c 100644
> --- a/hw/block/nvme-ns.c
> +++ b/hw/block/nvme-ns.c
> @@ -17,6 +17,7 @@
>  #include "qemu/cutils.h"
>  #include "qemu/log.h"
>  #include "qemu/error-report.h"
> +#include "qemu/hbitmap.h"

Isn't the HBitmap slightly overkill? Can qemu/bitmap.h suffice?

>  #include "hw/block/block.h"
>  #include "hw/pci/pci.h"
>  #include "sysemu/sysemu.h"

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

Re: [RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

[Keith Busch]

On Tue, Jan 19, 2021 at 07:18:16PM +0100, Klaus Jensen wrote:
> On Jan 20 02:01, Minwoo Im wrote:
> > Run with:
> >   -device nvme,serial=qux,id=nvme3
> >   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> > 
> >   -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,detached=true
> >   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2
> > 
> > nvme-cli:
> >   root@vm:~/work# nvme list -v                                                                                                      
> >   NVM Express Subsystems                                                                                                 
> >                                                                                                                                      
> >   Subsystem        Subsystem-NQN                                                                                    Controllers
> >   ---------------- ------------------------------------------------------------------------------------------------ ----------------
> >   nvme-subsys0     nqn.2019-08.org.qemu:qux                                                                         nvme0
> >   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme1, nvme2, nvme3
> >                                                                                                                                    
> >   NVM Express Controllers                                                                                           
> >                                                                                                                   
> >   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
> >   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> >   nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0
> 
> Shouldn't nvme0n1 be listed under Namespaces for nvme0?

Minwoo, try pulling the most current nvme-cli. There was a sysfs
scanning bug for non-mpath drives that should be fixed now.

Re: [RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

On 21-01-19 19:18:16, Klaus Jensen wrote:
> On Jan 20 02:01, Minwoo Im wrote:
> > Hello,
> > 
> > This patch series is third one to support multi-controller and namespace
> > sharing in multi-path.  This series introduced subsystem scheme to
> > manage controller(s) and namespace(s) in the subsystem.
> > 
> > This series has new patches from the V2:  'detached' parameter has been
> > added to the nvme-ns device.  This will decide whether to attach the
> > namespace to controller(s) in the current subsystem or not.  If it's
> > given with true, then it will be just allocated in the subsystem, but
> > not attaching to any controllers in the subsystem.  Otherwise, it will
> > automatically attach to all the controllers in the subsystem.  The other
> > t hing is that the last patch implemented Identify Active Namespace ID
> > List command handler apart from the Allocated Namespace ID List.
> > 
> > Run with:
> >   -device nvme,serial=qux,id=nvme3
> >   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> > 
> >   -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,detached=true
> >   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2
> > 
> > nvme-cli:
> >   root@vm:~/work# nvme list -v                                                                                                      
> >   NVM Express Subsystems                                                                                                 
> >                                                                                                                                      
> >   Subsystem        Subsystem-NQN                                                                                    Controllers
> >   ---------------- ------------------------------------------------------------------------------------------------ ----------------
> >   nvme-subsys0     nqn.2019-08.org.qemu:qux                                                                         nvme0
> >   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme1, nvme2, nvme3
> >                                                                                                                                    
> >   NVM Express Controllers                                                                                           
> >                                                                                                                   
> >   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
> >   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> >   nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0
> 
> Shouldn't nvme0n1 be listed under Namespaces for nvme0?

Oh, I missed that one from the output.  As Keith mentioned, I ran the
list command again based on the latest nvme-cli.git:

Please refer the following result.  I think it's okay not to send the
cover letter again :)

# nvme --version
nvme version 1.13.48.g33c6

# nvme list -v
NVM Express Subsystems

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

NVM Express Controllers

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

NVM Express Namespaces

Device       NSID     Usage                      Format           Controllers     
------------ -------- -------------------------- ---------------- ----------------
nvme0n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme0
nvme1n1      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme3

Re: [RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

> Minwoo, try pulling the most current nvme-cli. There was a sysfs
> scanning bug for non-mpath drives that should be fixed now.

Thank you, Keith!  I've posted list result based on the latest one :)

Re: [RFC PATCH V3 7/8] hw/block/nvme: add 'detached' param not to attach namespace

[Minwoo Im]

> Isn't the HBitmap slightly overkill? Can qemu/bitmap.h suffice?

Definitely, yes, I think.  Current patch series supoprt up to 32
controllers so I think qemu/bitmap.h is enough for us.

Will update the bitmap operations in the next series.

Re: [RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

[Klaus Jensen]

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

On Jan 20 09:44, Minwoo Im wrote:
> On 21-01-19 19:18:16, Klaus Jensen wrote:
> > On Jan 20 02:01, Minwoo Im wrote:
> > > Hello,
> > > 
> > > This patch series is third one to support multi-controller and namespace
> > > sharing in multi-path.  This series introduced subsystem scheme to
> > > manage controller(s) and namespace(s) in the subsystem.
> > > 
> > > This series has new patches from the V2:  'detached' parameter has been
> > > added to the nvme-ns device.  This will decide whether to attach the
> > > namespace to controller(s) in the current subsystem or not.  If it's
> > > given with true, then it will be just allocated in the subsystem, but
> > > not attaching to any controllers in the subsystem.  Otherwise, it will
> > > automatically attach to all the controllers in the subsystem.  The other
> > > t hing is that the last patch implemented Identify Active Namespace ID
> > > List command handler apart from the Allocated Namespace ID List.
> > > 
> > > Run with:
> > >   -device nvme,serial=qux,id=nvme3
> > >   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> > > 
> > >   -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,detached=true
> > >   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2
> > > 
> > > nvme-cli:
> > >   root@vm:~/work# nvme list -v                                                                                                      
> > >   NVM Express Subsystems                                                                                                 
> > >                                                                                                                                      
> > >   Subsystem        Subsystem-NQN                                                                                    Controllers
> > >   ---------------- ------------------------------------------------------------------------------------------------ ----------------
> > >   nvme-subsys0     nqn.2019-08.org.qemu:qux                                                                         nvme0
> > >   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme1, nvme2, nvme3
> > >                                                                                                                                    
> > >   NVM Express Controllers                                                                                           
> > >                                                                                                                   
> > >   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
> > >   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> > >   nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0
> > 
> > Shouldn't nvme0n1 be listed under Namespaces for nvme0?
> 
> Oh, I missed that one from the output.  As Keith mentioned, I ran the
> list command again based on the latest nvme-cli.git:
> 
> Please refer the following result.  I think it's okay not to send the
> cover letter again :)
> 
> # nvme --version
> nvme version 1.13.48.g33c6
> 
> # nvme list -v
> NVM Express Subsystems
> 
> Subsystem        Subsystem-NQN                                                                                    Controllers
> ---------------- ------------------------------------------------------------------------------------------------ ----------------
> nvme-subsys0     nqn.2019-08.org.qemu:qux                                                                         nvme0
> nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme1, nvme2, nvme3
> 
> NVM Express Controllers
> 
> Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces      
> -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0 nvme0n1
> nvme1    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1 
> nvme2    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1 
> nvme3    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys1 nvme1c3n1
> 
> NVM Express Namespaces
> 
> Device       NSID     Usage                      Format           Controllers     
> ------------ -------- -------------------------- ---------------- ----------------
> nvme0n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme0
> nvme1n1      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme3

That looks better, but hmm. Shouldnt the namespace be named `nvme1c3n1`
here has well? Is that also an issue with nvme-cli?

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

Re: [RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

On 21-01-20 08:52:14, Klaus Jensen wrote:
> On Jan 20 09:44, Minwoo Im wrote:
> > On 21-01-19 19:18:16, Klaus Jensen wrote:
> > > On Jan 20 02:01, Minwoo Im wrote:
> > > > Hello,
> > > > 
> > > > This patch series is third one to support multi-controller and namespace
> > > > sharing in multi-path.  This series introduced subsystem scheme to
> > > > manage controller(s) and namespace(s) in the subsystem.
> > > > 
> > > > This series has new patches from the V2:  'detached' parameter has been
> > > > added to the nvme-ns device.  This will decide whether to attach the
> > > > namespace to controller(s) in the current subsystem or not.  If it's
> > > > given with true, then it will be just allocated in the subsystem, but
> > > > not attaching to any controllers in the subsystem.  Otherwise, it will
> > > > automatically attach to all the controllers in the subsystem.  The other
> > > > t hing is that the last patch implemented Identify Active Namespace ID
> > > > List command handler apart from the Allocated Namespace ID List.
> > > > 
> > > > Run with:
> > > >   -device nvme,serial=qux,id=nvme3
> > > >   -device nvme-ns,id=ns3,drive=drv12,nsid=3,bus=nvme3
> > > > 
> > > >   -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,detached=true
> > > >   -device nvme-ns,id=ns2,drive=drv11,nsid=2,bus=nvme2
> > > > 
> > > > nvme-cli:
> > > >   root@vm:~/work# nvme list -v                                                                                                      
> > > >   NVM Express Subsystems                                                                                                 
> > > >                                                                                                                                      
> > > >   Subsystem        Subsystem-NQN                                                                                    Controllers
> > > >   ---------------- ------------------------------------------------------------------------------------------------ ----------------
> > > >   nvme-subsys0     nqn.2019-08.org.qemu:qux                                                                         nvme0
> > > >   nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme1, nvme2, nvme3
> > > >                                                                                                                                    
> > > >   NVM Express Controllers                                                                                           
> > > >                                                                                                                   
> > > >   Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
> > > >   -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> > > >   nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0
> > > 
> > > Shouldn't nvme0n1 be listed under Namespaces for nvme0?
> > 
> > Oh, I missed that one from the output.  As Keith mentioned, I ran the
> > list command again based on the latest nvme-cli.git:
> > 
> > Please refer the following result.  I think it's okay not to send the
> > cover letter again :)
> > 
> > # nvme --version
> > nvme version 1.13.48.g33c6
> > 
> > # nvme list -v
> > NVM Express Subsystems
> > 
> > Subsystem        Subsystem-NQN                                                                                    Controllers
> > ---------------- ------------------------------------------------------------------------------------------------ ----------------
> > nvme-subsys0     nqn.2019-08.org.qemu:qux                                                                         nvme0
> > nvme-subsys1     nqn.2019-08.org.qemu:subsys0                                                                     nvme1, nvme2, nvme3
> > 
> > NVM Express Controllers
> > 
> > Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces      
> > -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
> > nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0 nvme0n1
> > nvme1    foo                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:07.0   nvme-subsys1 
> > nvme2    bar                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:08.0   nvme-subsys1 
> > nvme3    baz                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:09.0   nvme-subsys1 nvme1c3n1
> > 
> > NVM Express Namespaces
> > 
> > Device       NSID     Usage                      Format           Controllers     
> > ------------ -------- -------------------------- ---------------- ----------------
> > nvme0n1      3        268.44  MB / 268.44  MB    512   B +  0 B   nvme0
> > nvme1n1      2        268.44  MB / 268.44  MB    512   B +  0 B   nvme3
> 
> That looks better, but hmm. Shouldnt the namespace be named `nvme1c3n1`
> here has well? Is that also an issue with nvme-cli?

No it isn't in this series.  The 'nvme3' controller has not been
provided with 'subsys' parameter which means it does not support CMIC
multi-controller as nvme_init_ctrl() does not set the flag for
NVME_CMIC_MULTI_CTRL:

  -device nvme,serial=qux,id=nvme3

If this has been given with 'subsys' which means it supports for
multi-controller, then it will be given like:

  Device   SN                   MN                                       FR       TxPort Address        Subsystem    Namespaces
  -------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
  nvme0    qux                  QEMU NVMe Ctrl                           1.0      pcie   0000:00:06.0   nvme-subsys0 nvme0c0n1

In short, if 'nvme' device controller is not given with 'subsys', then
it will not support multi-controller and it will never have
GENHD_FL_HIDDEN (nvmeXcYnZ convention).

Re: [RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID List

[Niklas Cassel]

On Wed, Jan 20, 2021 at 02:01:47AM +0900, Minwoo Im wrote:
> Spec v1.4b 6.1.4 "Active and Inactive NSID Types" says:
> 
> "Active NSIDs for a controller refer to namespaces that are attached to
> that controller. Allocated NSIDs that are inactive for a controller refer
> to namespaces that are not attached to that controller."
> 
> This patch introduced for Identify Active Namespace ID List (CNS 02h).
> 
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
>  hw/block/nvme.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 2b2c07b36c2b..7247167b0ee6 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -2883,6 +2883,39 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req)
>      return NVME_INVALID_FIELD | NVME_DNR;
>  }
>  
> +static uint16_t nvme_identify_nslist_active(NvmeCtrl *n, NvmeRequest *req)
> +{
> +    NvmeNamespace *ns;
> +    NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> +    uint32_t min_nsid = le32_to_cpu(c->nsid);
> +    uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> +    static const int data_len = sizeof(list);
> +    uint32_t *list_ptr = (uint32_t *)list;
> +    int i, j = 0;
> +
> +    if (min_nsid >= NVME_NSID_BROADCAST - 1) {
> +        return NVME_INVALID_NSID | NVME_DNR;
> +    }
> +
> +    for (i = 1; i <= n->num_namespaces; i++) {
> +        ns = nvme_ns(n, i);
> +        if (!ns || ns->params.nsid <= min_nsid) {
> +            continue;
> +        }
> +
> +        if (!nvme_ns_is_attached(n, ns)) {
> +            continue;
> +        }
> +
> +        list_ptr[j++] = cpu_to_le32(ns->params.nsid);
> +        if (j == data_len / sizeof(uint32_t)) {
> +            break;
> +        }
> +    }
> +
> +    return nvme_dma(n, list, data_len, DMA_DIRECTION_FROM_DEVICE, req);
> +}
> +
>  static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
>  {
>      NvmeNamespace *ns;
> @@ -2914,10 +2947,6 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
>              continue;
>          }
>  
> -        if (!nvme_ns_is_attached(n, ns)) {
> -            continue;
> -        }
> -
>          list_ptr[j++] = cpu_to_le32(ns->params.nsid);
>          if (j == data_len / sizeof(uint32_t)) {
>              break;
> @@ -3045,7 +3074,7 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
>      case NVME_ID_CNS_CS_CTRL:
>          return nvme_identify_ctrl_csi(n, req);
>      case NVME_ID_CNS_NS_ACTIVE_LIST:
> -         /* fall through */
> +         return nvme_identify_nslist_active(n, req);
>      case NVME_ID_CNS_NS_PRESENT_LIST:
>          return nvme_identify_nslist(n, req);
>      case NVME_ID_CNS_CS_NS_ACTIVE_LIST:
> -- 
> 2.17.1
> 
> 

Hello Minwoo,

By introducing a detached parameter,
you are also implicitly making the following
NVMe commands no longer be spec compliant:

NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST

When these commands are called on a detached namespace,
they should usually return a zero filled data struct.

Dmitry and I had a patch on V8 on the ZNS series
that tried to fix some the existing NVMe commands
to be spec compliant, by handling detached namespaces
properly. In the end, in order to make it easier to
get the ZNS series accepted, we decided to drop the
detached related stuff from the series.

Feel free to look at that patch for inspiration:
https://github.com/dmitry-fomichev/qemu/commit/251c0ffee5149c739b1347811fa7e32a1c36bf7c

I'm not sure if you want to modify all the functions that
our patch modifies, but I think that you should at least
modify the following nvme functions:

nvme_identify_ns()
nvme_identify_ns_csi()
nvme_identify_nslist()
nvme_identify_nslist_csi()

So they handle detached namespaces correctly for both:
NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
as well as for:
NVME_ID_CNS_NS_PRESENT, NVME_ID_CNS_CS_NS_PRESENT,
NVME_ID_CNS_NS_PRESENT_LIST, NVME_ID_CNS_CS_NS_PRESENT_LIST


Kind regards,
Niklas

Re: [RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID List

[Klaus Jensen]

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

On Jan 20 14:07, Niklas Cassel wrote:
> On Wed, Jan 20, 2021 at 02:01:47AM +0900, Minwoo Im wrote:
> > Spec v1.4b 6.1.4 "Active and Inactive NSID Types" says:
> > 
> > "Active NSIDs for a controller refer to namespaces that are attached to
> > that controller. Allocated NSIDs that are inactive for a controller refer
> > to namespaces that are not attached to that controller."
> > 
> > This patch introduced for Identify Active Namespace ID List (CNS 02h).
> > 
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> >  hw/block/nvme.c | 39 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 2b2c07b36c2b..7247167b0ee6 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -2883,6 +2883,39 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req)
> >      return NVME_INVALID_FIELD | NVME_DNR;
> >  }
> >  
> > +static uint16_t nvme_identify_nslist_active(NvmeCtrl *n, NvmeRequest *req)
> > +{
> > +    NvmeNamespace *ns;
> > +    NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> > +    uint32_t min_nsid = le32_to_cpu(c->nsid);
> > +    uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> > +    static const int data_len = sizeof(list);
> > +    uint32_t *list_ptr = (uint32_t *)list;
> > +    int i, j = 0;
> > +
> > +    if (min_nsid >= NVME_NSID_BROADCAST - 1) {
> > +        return NVME_INVALID_NSID | NVME_DNR;
> > +    }
> > +
> > +    for (i = 1; i <= n->num_namespaces; i++) {
> > +        ns = nvme_ns(n, i);
> > +        if (!ns || ns->params.nsid <= min_nsid) {
> > +            continue;
> > +        }
> > +
> > +        if (!nvme_ns_is_attached(n, ns)) {
> > +            continue;
> > +        }
> > +
> > +        list_ptr[j++] = cpu_to_le32(ns->params.nsid);
> > +        if (j == data_len / sizeof(uint32_t)) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    return nvme_dma(n, list, data_len, DMA_DIRECTION_FROM_DEVICE, req);
> > +}
> > +
> >  static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
> >  {
> >      NvmeNamespace *ns;
> > @@ -2914,10 +2947,6 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
> >              continue;
> >          }
> >  
> > -        if (!nvme_ns_is_attached(n, ns)) {
> > -            continue;
> > -        }
> > -
> >          list_ptr[j++] = cpu_to_le32(ns->params.nsid);
> >          if (j == data_len / sizeof(uint32_t)) {
> >              break;
> > @@ -3045,7 +3074,7 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
> >      case NVME_ID_CNS_CS_CTRL:
> >          return nvme_identify_ctrl_csi(n, req);
> >      case NVME_ID_CNS_NS_ACTIVE_LIST:
> > -         /* fall through */
> > +         return nvme_identify_nslist_active(n, req);
> >      case NVME_ID_CNS_NS_PRESENT_LIST:
> >          return nvme_identify_nslist(n, req);
> >      case NVME_ID_CNS_CS_NS_ACTIVE_LIST:
> > -- 
> > 2.17.1
> > 
> > 
> 
> Hello Minwoo,
> 
> By introducing a detached parameter,
> you are also implicitly making the following
> NVMe commands no longer be spec compliant:
> 
> NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
> NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
> 
> When these commands are called on a detached namespace,
> they should usually return a zero filled data struct.
> 
> Dmitry and I had a patch on V8 on the ZNS series
> that tried to fix some the existing NVMe commands
> to be spec compliant, by handling detached namespaces
> properly. In the end, in order to make it easier to
> get the ZNS series accepted, we decided to drop the
> detached related stuff from the series.
> 
> Feel free to look at that patch for inspiration:
> https://github.com/dmitry-fomichev/qemu/commit/251c0ffee5149c739b1347811fa7e32a1c36bf7c
> 
> I'm not sure if you want to modify all the functions that
> our patch modifies, but I think that you should at least
> modify the following nvme functions:
> 
> nvme_identify_ns()
> nvme_identify_ns_csi()
> nvme_identify_nslist()
> nvme_identify_nslist_csi()
> 
> So they handle detached namespaces correctly for both:
> NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
> NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
> as well as for:
> NVME_ID_CNS_NS_PRESENT, NVME_ID_CNS_CS_NS_PRESENT,
> NVME_ID_CNS_NS_PRESENT_LIST, NVME_ID_CNS_CS_NS_PRESENT_LIST
> 

Definitely - it makes sense to reintroduce your patch here, with a
replacement of `ns->attached` with `nvme_ns_is_attach()`. Looks like
that should be sufficient.

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

Re: [RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID List

[Minwoo Im]

> Hello Minwoo,
> 
> By introducing a detached parameter,
> you are also implicitly making the following
> NVMe commands no longer be spec compliant:
> 
> NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
> NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
> 
> When these commands are called on a detached namespace,
> they should usually return a zero filled data struct.

Agreed.

> Dmitry and I had a patch on V8 on the ZNS series
> that tried to fix some the existing NVMe commands
> to be spec compliant, by handling detached namespaces
> properly. In the end, in order to make it easier to
> get the ZNS series accepted, we decided to drop the
> detached related stuff from the series.
> 
> Feel free to look at that patch for inspiration:
> https://github.com/dmitry-fomichev/qemu/commit/251c0ffee5149c739b1347811fa7e32a1c36bf7c

I've seen this patch and as Klaus said, only thing patches need go with
is to put some of nvme_ns_is_attached() helper among the Identify
handlers.

> I'm not sure if you want to modify all the functions that
> our patch modifies, but I think that you should at least
> modify the following nvme functions:
> 
> nvme_identify_ns()
> nvme_identify_ns_csi()
> nvme_identify_nslist()
> nvme_identify_nslist_csi()

Yes, pretty makes sense to update them.  But now it seems like
'attach/detach' scheme should have been separated out of this series
which just introduced the multi-path for controllers and namespace
sharing.  I will drop this 'detach' scheme out of this series and make
another series to support all of the Identify you mentioned above
cleanly.

> So they handle detached namespaces correctly for both:
> NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
> NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
> as well as for:
> NVME_ID_CNS_NS_PRESENT, NVME_ID_CNS_CS_NS_PRESENT,
> NVME_ID_CNS_NS_PRESENT_LIST, NVME_ID_CNS_CS_NS_PRESENT_LIST
> 
> 
> Kind regards,
> Niklas

Re: [RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID List

[Niklas Cassel]

On Thu, Jan 21, 2021 at 06:58:19AM +0900, Minwoo Im wrote:
> > Hello Minwoo,
> > 
> > By introducing a detached parameter,
> > you are also implicitly making the following
> > NVMe commands no longer be spec compliant:
> > 
> > NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
> > NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
> > 
> > When these commands are called on a detached namespace,
> > they should usually return a zero filled data struct.
> 
> Agreed.
> 
> > Dmitry and I had a patch on V8 on the ZNS series
> > that tried to fix some the existing NVMe commands
> > to be spec compliant, by handling detached namespaces
> > properly. In the end, in order to make it easier to
> > get the ZNS series accepted, we decided to drop the
> > detached related stuff from the series.
> > 
> > Feel free to look at that patch for inspiration:
> > https://github.com/dmitry-fomichev/qemu/commit/251c0ffee5149c739b1347811fa7e32a1c36bf7c
> 
> I've seen this patch and as Klaus said, only thing patches need go with
> is to put some of nvme_ns_is_attached() helper among the Identify
> handlers.
> 
> > I'm not sure if you want to modify all the functions that
> > our patch modifies, but I think that you should at least
> > modify the following nvme functions:
> > 
> > nvme_identify_ns()
> > nvme_identify_ns_csi()
> > nvme_identify_nslist()
> > nvme_identify_nslist_csi()
> 
> Yes, pretty makes sense to update them.  But now it seems like
> 'attach/detach' scheme should have been separated out of this series
> which just introduced the multi-path for controllers and namespace
> sharing.  I will drop this 'detach' scheme out of this series and make
> another series to support all of the Identify you mentioned above
> cleanly.

Hello Minwoo,

thank you for putting in work on this!

Kind regards,
Niklas