[PATCH for-6.0 v2 0/8] hw/block/nvme: misc fixes

[PATCH for-6.0 v2 0/8] hw/block/nvme: misc fixes

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Various fixes for 6.0.

v2:
  - "hw/block/nvme: fix handling of private namespaces"
    update documentation (Gollu)
  - add a patch for missing copyright headers

Klaus Jensen (8):
  hw/block/nvme: fix pi constraint check
  hw/block/nvme: fix missing string representation for ns attachment
  hw/block/nvme: fix the nsid 'invalid' value
  hw/block/nvme: fix controller namespaces array indexing
  hw/block/nvme: fix warning about legacy namespace configuration
  hw/block/nvme: update dmsrl limit on namespace detachment
  hw/block/nvme: fix handling of private namespaces
  hw/block/nvme: add missing copyright headers

 hw/block/nvme-dif.h    |  10 +++
 hw/block/nvme-ns.h     |  12 ++--
 hw/block/nvme-subsys.h |   7 +-
 hw/block/nvme.h        |  45 ++-----------
 include/block/nvme.h   |   1 +
 hw/block/nvme-dif.c    |  10 +++
 hw/block/nvme-ns.c     |  76 ++++++++++++++++++----
 hw/block/nvme-subsys.c |  28 --------
 hw/block/nvme.c        | 142 +++++++++++++++++------------------------
 hw/block/trace-events  |   1 -
 10 files changed, 156 insertions(+), 176 deletions(-)

-- 
2.31.1

[PATCH for-6.0 v2 1/8] hw/block/nvme: fix pi constraint check

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Protection Information can only be enabled if there is at least 8 bytes
of metadata.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme-ns.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 7f8d139a8663..ca04ee1bacfb 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -394,7 +394,7 @@ static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp)
         return -1;
     }
 
-    if (ns->params.pi && !ns->params.ms) {
+    if (ns->params.pi && ns->params.ms < 8) {
         error_setg(errp, "at least 8 bytes of metadata required to enable "
                    "protection information");
         return -1;
-- 
2.31.1

[PATCH for-6.0 v2 2/8] hw/block/nvme: fix missing string representation for ns attachment

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Add the missing nvme_adm_opc_str entry for the Namespace Attachment
command.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 5b0031b11db2..9edc86d79e98 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -86,6 +86,7 @@ static inline const char *nvme_adm_opc_str(uint8_t opc)
     case NVME_ADM_CMD_SET_FEATURES:     return "NVME_ADM_CMD_SET_FEATURES";
     case NVME_ADM_CMD_GET_FEATURES:     return "NVME_ADM_CMD_GET_FEATURES";
     case NVME_ADM_CMD_ASYNC_EV_REQ:     return "NVME_ADM_CMD_ASYNC_EV_REQ";
+    case NVME_ADM_CMD_NS_ATTACHMENT:    return "NVME_ADM_CMD_NS_ATTACHMENT";
     case NVME_ADM_CMD_FORMAT_NVM:       return "NVME_ADM_CMD_FORMAT_NVM";
     default:                            return "NVME_ADM_CMD_UNKNOWN";
     }
-- 
2.31.1

[PATCH for-6.0 v2 3/8] hw/block/nvme: fix the nsid 'invalid' value

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

The `nvme_nsid()` function returns '-1' (FFFFFFFFh) when the given
namespace is NULL. Since FFFFFFFFh is actually a valid namespace
identifier (the "broadcast" value), change this to be '0' since that
actually *is* the invalid value.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme-ns.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
index 9ab7894fc83e..82340c4b2574 100644
--- a/hw/block/nvme-ns.h
+++ b/hw/block/nvme-ns.h
@@ -96,7 +96,7 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns)
         return ns->params.nsid;
     }
 
-    return -1;
+    return 0;
 }
 
 static inline bool nvme_ns_shared(NvmeNamespace *ns)
-- 
2.31.1

[PATCH for-6.0 v2 4/8] hw/block/nvme: fix controller namespaces array indexing

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

The controller namespaces array being 0-indexed requires 'nsid - 1'
everywhere. Something that is easy to miss. Align the controller
namespaces array with the subsystem namespaces array such that both are
1-indexed.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme.h | 8 ++++----
 hw/block/nvme.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 9edc86d79e98..c610ab30dc5c 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -217,7 +217,7 @@ typedef struct NvmeCtrl {
      * Attached namespaces to this controller.  If subsys is not given, all
      * namespaces in this list will always be attached.
      */
-    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES];
+    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES + 1];
     NvmeSQueue      **sq;
     NvmeCQueue      **cq;
     NvmeSQueue      admin_sq;
@@ -232,7 +232,7 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
         return NULL;
     }
 
-    return n->namespaces[nsid - 1];
+    return n->namespaces[nsid];
 }
 
 static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
@@ -253,7 +253,7 @@ static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
     uint32_t nsid = nvme_nsid(ns);
     assert(nsid && nsid <= NVME_MAX_NAMESPACES);
 
-    n->namespaces[nsid - 1] = ns;
+    n->namespaces[nsid] = ns;
 }
 
 static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
@@ -261,7 +261,7 @@ static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
     uint32_t nsid = nvme_nsid(ns);
     assert(nsid && nsid <= NVME_MAX_NAMESPACES);
 
-    n->namespaces[nsid - 1] = NULL;
+    n->namespaces[nsid] = NULL;
 }
 
 static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index c54ec3c9523c..6d31d5b17a0b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -5915,7 +5915,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
             return -1;
         }
     } else {
-        if (n->namespaces[nsid - 1]) {
+        if (n->namespaces[nsid]) {
             error_setg(errp, "namespace id '%d' is already in use", nsid);
             return -1;
         }
-- 
2.31.1

[PATCH for-6.0 v2 5/8] hw/block/nvme: fix warning about legacy namespace configuration

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Remove the unused BlockConf from the controller structure and fix the
constraint checking to actually check the right BlockConf and issue the
warning.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme.h | 1 -
 hw/block/nvme.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index c610ab30dc5c..1570f65989a7 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -166,7 +166,6 @@ typedef struct NvmeCtrl {
     NvmeBar      bar;
     NvmeParams   params;
     NvmeBus      bus;
-    BlockConf    conf;
 
     uint16_t    cntlid;
     bool        qs_created;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 6d31d5b17a0b..de0e726dfdd8 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -5813,7 +5813,7 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
         params->max_ioqpairs = params->num_queues - 1;
     }
 
-    if (n->conf.blk) {
+    if (n->namespace.blkconf.blk) {
         warn_report("drive property is deprecated; "
                     "please use an nvme-ns device instead");
     }
-- 
2.31.1

[PATCH for-6.0 v2 6/8] hw/block/nvme: update dmsrl limit on namespace detachment

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

The Non-MDTS DMSRL limit must be recomputed when namespaces are
detached.

Fixes: 645ce1a70cb6 ("hw/block/nvme: support namespace attachment command")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index de0e726dfdd8..3dc51f407671 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4876,6 +4876,21 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
     return NVME_NO_COMPLETE;
 }
 
+static void __nvme_update_dmrsl(NvmeCtrl *n)
+{
+    int nsid;
+
+    for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
+        NvmeNamespace *ns = nvme_ns(n, nsid);
+        if (!ns) {
+            continue;
+        }
+
+        n->dmrsl = MIN_NON_ZERO(n->dmrsl,
+                                BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
+    }
+}
+
 static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
 static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
 {
@@ -4925,6 +4940,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
             }
 
             nvme_ns_detach(ctrl, ns);
+
+            __nvme_update_dmrsl(ctrl);
         }
 
         /*
-- 
2.31.1

[PATCH for-6.0 v2 7/8] hw/block/nvme: fix handling of private namespaces

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Prior to this patch, if a private nvme-ns device (that is, a namespace
that is not linked to a subsystem) is wired up to an nvme-subsys linked
nvme controller device, the device fails to verify that the namespace id
is unique within the subsystem. NVM Express v1.4b, Section 6.1.6 ("NSID
and Namespace Usage") states that because the device supports Namespace
Management, "NSIDs *shall* be unique within the NVM subsystem".

Additionally, prior to this patch, private namespaces are not known to
the subsystem and the namespace is considered exclusive to the
controller with which it is initially wired up to. However, this is not
the definition of a private namespace; per Section 1.6.33 ("private
namespace"), a private namespace is just a namespace that does not
support multipath I/O or namespace sharing, which means "that it is only
able to be attached to one controller at a time".

Fix this by always allocating namespaces in the subsystem (if one is
linked to the controller), regardsless of the shared/private status of
the namespace. Whether or not the namespace is shareable is controlled
by a new `shared` nvme-ns parameter.

Finally, this fix allows the nvme-ns `subsys` parameter to be removed,
since the `shared` parameter now serves the purpose of attaching the
namespace to all controllers in the subsystem upon device realization.
It is invalid to have an nvme-ns namespace device with a linked
subsystem without the parent nvme controller device also being linked to
one and since the nvme-ns devices will unconditionally be "attached" (in
QEMU terms that is) to an nvme controller device through an NvmeBus, the
nvme-ns namespace device can always get a reference to the subsystem of
the controller it is explicitly (using 'bus=' parametr) or implicitly
attaching to.

Fixes: e570768566b3 ("hw/block/nvme: support for shared namespace in subsystem")
Cc: Minwoo Im <minwoo.im.dev@gmail.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme-ns.h     |  10 +---
 hw/block/nvme-subsys.h |   7 +--
 hw/block/nvme.h        |  39 +------------
 include/block/nvme.h   |   1 +
 hw/block/nvme-ns.c     |  74 +++++++++++++++++++++----
 hw/block/nvme-subsys.c |  28 ----------
 hw/block/nvme.c        | 123 ++++++++++++++---------------------------
 hw/block/trace-events  |   1 -
 8 files changed, 113 insertions(+), 170 deletions(-)

diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
index 82340c4b2574..fb0a41f912e7 100644
--- a/hw/block/nvme-ns.h
+++ b/hw/block/nvme-ns.h
@@ -29,6 +29,7 @@ typedef struct NvmeZone {
 
 typedef struct NvmeNamespaceParams {
     bool     detached;
+    bool     shared;
     uint32_t nsid;
     QemuUUID uuid;
 
@@ -60,8 +61,8 @@ typedef struct NvmeNamespace {
     const uint32_t *iocs;
     uint8_t      csi;
     uint16_t     status;
+    int          attached;
 
-    NvmeSubsystem   *subsys;
     QTAILQ_ENTRY(NvmeNamespace) entry;
 
     NvmeIdNsZoned   *id_ns_zoned;
@@ -99,11 +100,6 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns)
     return 0;
 }
 
-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;
@@ -225,7 +221,7 @@ static inline void nvme_aor_dec_active(NvmeNamespace *ns)
 }
 
 void nvme_ns_init_format(NvmeNamespace *ns);
-int nvme_ns_setup(NvmeNamespace *ns, Error **errp);
+int nvme_ns_setup(NvmeCtrl *n, 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-subsys.h b/hw/block/nvme-subsys.h
index aafa04b84829..24132edd005c 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -14,7 +14,7 @@
     OBJECT_CHECK(NvmeSubsystem, (obj), TYPE_NVME_SUBSYS)
 
 #define NVME_SUBSYS_MAX_CTRLS   32
-#define NVME_SUBSYS_MAX_NAMESPACES  256
+#define NVME_MAX_NAMESPACES     256
 
 typedef struct NvmeCtrl NvmeCtrl;
 typedef struct NvmeNamespace NvmeNamespace;
@@ -24,7 +24,7 @@ typedef struct NvmeSubsystem {
 
     NvmeCtrl    *ctrls[NVME_SUBSYS_MAX_CTRLS];
     /* Allocated namespaces for this subsystem */
-    NvmeNamespace *namespaces[NVME_SUBSYS_MAX_NAMESPACES + 1];
+    NvmeNamespace *namespaces[NVME_MAX_NAMESPACES + 1];
 
     struct {
         char *nqn;
@@ -32,7 +32,6 @@ typedef struct NvmeSubsystem {
 } NvmeSubsystem;
 
 int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
-int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp);
 
 static inline NvmeCtrl *nvme_subsys_ctrl(NvmeSubsystem *subsys,
         uint32_t cntlid)
@@ -54,7 +53,7 @@ static inline NvmeNamespace *nvme_subsys_ns(NvmeSubsystem *subsys,
         return NULL;
     }
 
-    assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES);
+    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
 
     return subsys->namespaces[nsid];
 }
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 1570f65989a7..644143597a0f 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -6,17 +6,9 @@
 #include "nvme-subsys.h"
 #include "nvme-ns.h"
 
-#define NVME_MAX_NAMESPACES 256
-
 #define NVME_DEFAULT_ZONE_SIZE   (128 * MiB)
 #define NVME_DEFAULT_MAX_ZA_SIZE (128 * KiB)
 
-/*
- * Subsystem namespace list for allocated namespaces should be larger than
- * attached namespace list in a controller.
- */
-QEMU_BUILD_BUG_ON(NVME_MAX_NAMESPACES > NVME_SUBSYS_MAX_NAMESPACES);
-
 typedef struct NvmeParams {
     char     *serial;
     uint32_t num_queues; /* deprecated since 5.1 */
@@ -234,35 +226,6 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
     return n->namespaces[nsid];
 }
 
-static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
-{
-    int nsid;
-
-    for (nsid = 1; nsid <= n->num_namespaces; nsid++) {
-        if (nvme_ns(n, nsid) == ns) {
-            return true;
-        }
-    }
-
-    return false;
-}
-
-static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
-{
-    uint32_t nsid = nvme_nsid(ns);
-    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
-
-    n->namespaces[nsid] = ns;
-}
-
-static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
-{
-    uint32_t nsid = nvme_nsid(ns);
-    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
-
-    n->namespaces[nsid] = NULL;
-}
-
 static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
 {
     NvmeSQueue *sq = req->sq;
@@ -291,7 +254,7 @@ typedef enum NvmeTxDirection {
     NVME_TX_DIRECTION_FROM_DEVICE = 1,
 } NvmeTxDirection;
 
-int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp);
+void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns);
 uint16_t nvme_bounce_data(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
                           NvmeTxDirection dir, NvmeRequest *req);
 uint16_t nvme_bounce_mdata(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
diff --git a/include/block/nvme.h b/include/block/nvme.h
index b0a4e4291611..4ac926fbc687 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -847,6 +847,7 @@ enum NvmeStatusCodes {
     NVME_FEAT_NOT_NS_SPEC       = 0x010f,
     NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
     NVME_NS_ALREADY_ATTACHED    = 0x0118,
+    NVME_NS_PRIVATE             = 0x0119,
     NVME_NS_NOT_ATTACHED        = 0x011A,
     NVME_NS_CTRL_LIST_INVALID   = 0x011C,
     NVME_CONFLICTING_ATTRS      = 0x0180,
diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index ca04ee1bacfb..aee43ba0b873 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -73,7 +73,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
     /* support DULBE and I/O optimization fields */
     id_ns->nsfeat |= (0x4 | 0x10);
 
-    if (nvme_ns_shared(ns)) {
+    if (ns->params.shared) {
         id_ns->nmic |= NVME_NMIC_NS_SHARED;
     }
 
@@ -387,7 +387,8 @@ static void nvme_zoned_ns_shutdown(NvmeNamespace *ns)
     assert(ns->nr_open_zones == 0);
 }
 
-static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp)
+static int nvme_ns_check_constraints(NvmeCtrl *n, NvmeNamespace *ns,
+                                     Error **errp)
 {
     if (!ns->blkconf.blk) {
         error_setg(errp, "block backend not configured");
@@ -400,12 +401,32 @@ static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp)
         return -1;
     }
 
+    if (ns->params.nsid > NVME_MAX_NAMESPACES) {
+        error_setg(errp, "invalid namespace id (must be between 0 and %d)",
+                   NVME_MAX_NAMESPACES);
+        return -1;
+    }
+
+    if (!n->subsys) {
+        if (ns->params.detached) {
+            error_setg(errp, "detached requires that the nvme device is "
+                       "linked to an nvme-subsys device");
+            return -1;
+        }
+
+        if (ns->params.shared) {
+            error_setg(errp, "shared requires that the nvme device is "
+                       "linked to an nvme-subsys device");
+            return -1;
+        }
+    }
+
     return 0;
 }
 
-int nvme_ns_setup(NvmeNamespace *ns, Error **errp)
+int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
 {
-    if (nvme_ns_check_constraints(ns, errp)) {
+    if (nvme_ns_check_constraints(n, ns, errp)) {
         return -1;
     }
 
@@ -453,27 +474,60 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
     NvmeNamespace *ns = NVME_NS(dev);
     BusState *s = qdev_get_parent_bus(dev);
     NvmeCtrl *n = NVME(s->parent);
+    NvmeSubsystem *subsys = n->subsys;
+    uint32_t nsid = ns->params.nsid;
+    int i;
 
-    if (nvme_ns_setup(ns, errp)) {
+    if (nvme_ns_setup(n, ns, errp)) {
         return;
     }
 
-    if (ns->subsys) {
-        if (nvme_subsys_register_ns(ns, errp)) {
+    if (!nsid) {
+        for (i = 1; i <= NVME_MAX_NAMESPACES; i++) {
+            if (!nvme_ns(n, i) || nvme_subsys_ns(subsys, i)) {
+                nsid = ns->params.nsid = i;
+                break;
+            }
+        }
+
+        if (!nsid) {
+            error_setg(errp, "no free namespace id");
             return;
         }
     } else {
-        if (nvme_register_namespace(n, ns, errp)) {
+        if (nvme_ns(n, nsid) || nvme_subsys_ns(subsys, nsid)) {
+            error_setg(errp, "namespace id '%d' already allocated", nsid);
             return;
         }
     }
+
+    if (subsys) {
+        subsys->namespaces[nsid] = ns;
+
+        if (ns->params.detached) {
+            return;
+        }
+
+        if (ns->params.shared) {
+            for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) {
+                NvmeCtrl *ctrl = subsys->ctrls[i];
+
+                if (ctrl) {
+                    nvme_attach_ns(ctrl, ns);
+                }
+            }
+
+            return;
+        }
+    }
+
+    nvme_attach_ns(n, ns);
 }
 
 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_BOOL("shared", NvmeNamespace, params.shared, false),
     DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
     DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
     DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0),
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
index 9fadef8cec99..283a97b79d57 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -43,34 +43,6 @@ 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;
-    uint32_t nsid = nvme_nsid(ns);
-    int i;
-
-    assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES);
-
-    if (subsys->namespaces[nsid]) {
-        error_setg(errp, "namespace %d already registerd to subsy %s",
-                   nvme_nsid(ns), subsys->parent_obj.id);
-        return -1;
-    }
-
-    subsys->namespaces[nsid] = 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)
 {
     const char *nqn = subsys->params.nqn ?
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 3dc51f407671..d50f59a03c6a 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -101,10 +101,13 @@
  *
  * nvme namespace device parameters
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- * - `subsys`
- *   If given, the namespace will be attached to all controllers in the
- *   subsystem. Otherwise, `bus` must be given to attach this namespace to a
- *   specific controller as a non-shared namespace.
+ * - `shared`
+ *   When the parent nvme device (as defined explicitly by the 'bus' parameter
+ *   or implicitly by the most recently defined NvmeBus) is linked to an
+ *   nvme-subsys device, the namespace will be attached to all controllers in
+ *   the subsystem. If set to 'off' (the default), the namespace will remain a
+ *   private namespace and may only be attached to a single controller at a
+ *   time.
  *
  * - `detached`
  *   This parameter is only valid together with the `subsys` parameter. If left
@@ -4250,7 +4253,7 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req)
             continue;
         }
 
-        if (!nvme_ns_is_attached(ctrl, ns)) {
+        if (!nvme_ns(ctrl, c->nsid)) {
             continue;
         }
 
@@ -4907,6 +4910,10 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
 
     trace_pci_nvme_ns_attachment(nvme_cid(req), dw10 & 0xf);
 
+    if (!nvme_nsid_valid(n, nsid)) {
+        return NVME_INVALID_NSID | NVME_DNR;
+    }
+
     ns = nvme_subsys_ns(n->subsys, nsid);
     if (!ns) {
         return NVME_INVALID_FIELD | NVME_DNR;
@@ -4928,18 +4935,23 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
         }
 
         if (attach) {
-            if (nvme_ns_is_attached(ctrl, ns)) {
+            if (nvme_ns(ctrl, nsid)) {
                 return NVME_NS_ALREADY_ATTACHED | NVME_DNR;
             }
 
-            nvme_ns_attach(ctrl, ns);
+            if (ns->attached && !ns->params.shared) {
+                return NVME_NS_PRIVATE | NVME_DNR;
+            }
+
+            nvme_attach_ns(ctrl, ns);
             __nvme_select_ns_iocs(ctrl, ns);
         } else {
-            if (!nvme_ns_is_attached(ctrl, ns)) {
+            if (!nvme_ns(ctrl, nsid)) {
                 return NVME_NS_NOT_ATTACHED | NVME_DNR;
             }
 
-            nvme_ns_detach(ctrl, ns);
+            ctrl->namespaces[nsid] = NULL;
+            ns->attached--;
 
             __nvme_update_dmrsl(ctrl);
         }
@@ -5833,6 +5845,12 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
     if (n->namespace.blkconf.blk) {
         warn_report("drive property is deprecated; "
                     "please use an nvme-ns device instead");
+
+        if (n->subsys) {
+            error_setg(errp, "subsystem support is unavailable with legacy "
+                       "namespace ('drive' property)");
+            return;
+        }
     }
 
     if (params->max_ioqpairs < 1 ||
@@ -5895,75 +5913,6 @@ 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);
-
-    if (nsid > NVME_MAX_NAMESPACES) {
-        error_setg(errp, "invalid namespace id (must be between 0 and %d)",
-                   NVME_MAX_NAMESPACES);
-        return -1;
-    }
-
-    if (!nsid) {
-        for (int i = 1; i <= n->num_namespaces; i++) {
-            if (!nvme_ns(n, i)) {
-                nsid = ns->params.nsid = i;
-                break;
-            }
-        }
-
-        if (!nsid) {
-            error_setg(errp, "no free namespace id");
-            return -1;
-        }
-    } else {
-        if (n->namespaces[nsid]) {
-            error_setg(errp, "namespace id '%d' is already in use", nsid);
-            return -1;
-        }
-    }
-
-    trace_pci_nvme_register_namespace(nsid);
-
-    /*
-     * If subsys is not given, namespae is always attached to the controller
-     * because there's no subsystem to manage namespace allocation.
-     */
-    if (!n->subsys) {
-        if (ns->params.detached) {
-            error_setg(errp,
-                       "detached needs nvme-subsys specified nvme or nvme-ns");
-            return -1;
-        }
-
-        return nvme_attach_namespace(n, ns, errp);
-    } else {
-        if (!ns->params.detached) {
-            return nvme_attach_namespace(n, ns, errp);
-        }
-    }
-
-    n->dmrsl = MIN_NON_ZERO(n->dmrsl,
-                            BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
-
-    return 0;
-}
-
 static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
 {
     uint64_t cmb_size = n->params.cmb_size_mb * MiB;
@@ -6193,6 +6142,18 @@ static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
     return 0;
 }
 
+void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns)
+{
+    uint32_t nsid = ns->params.nsid;
+    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
+
+    n->namespaces[nsid] = ns;
+    ns->attached++;
+
+    n->dmrsl = MIN_NON_ZERO(n->dmrsl,
+                            BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
+}
+
 static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 {
     NvmeCtrl *n = NVME(pci_dev);
@@ -6224,13 +6185,11 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         ns = &n->namespace;
         ns->params.nsid = 1;
 
-        if (nvme_ns_setup(ns, errp)) {
+        if (nvme_ns_setup(n, ns, errp)) {
             return;
         }
 
-        if (nvme_register_namespace(n, ns, errp)) {
-            return;
-        }
+        nvme_attach_ns(n, ns);
     }
 }
 
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 22da06986d72..fa12e3a67a75 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -51,7 +51,6 @@ hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads, uint32_t secs, int t
 
 # nvme.c
 # nvme traces for successful events
-pci_nvme_register_namespace(uint32_t nsid) "nsid %"PRIu32""
 pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u"
 pci_nvme_irq_pin(void) "pulsing IRQ pin"
 pci_nvme_irq_masked(void) "IRQ is masked"
-- 
2.31.1

[PATCH for-6.0 v2 8/8] hw/block/nvme: add missing copyright headers

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Add missing license/copyright headers to the nvme-dif.{c,h} files.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme-dif.h | 10 ++++++++++
 hw/block/nvme-dif.c | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/hw/block/nvme-dif.h b/hw/block/nvme-dif.h
index 5a8e37c8525b..524faffbd7a0 100644
--- a/hw/block/nvme-dif.h
+++ b/hw/block/nvme-dif.h
@@ -1,3 +1,13 @@
+/*
+ * QEMU NVM Express End-to-End Data Protection support
+ *
+ * Copyright (c) 2021 Samsung Electronics Co., Ltd.
+ *
+ * Authors:
+ *   Klaus Jensen           <k.jensen@samsung.com>
+ *   Gollu Appalanaidu      <anaidu.gollu@samsung.com>
+ */
+
 #ifndef HW_NVME_DIF_H
 #define HW_NVME_DIF_H
 
diff --git a/hw/block/nvme-dif.c b/hw/block/nvme-dif.c
index e6f04faafb5f..81b0a4cb1382 100644
--- a/hw/block/nvme-dif.c
+++ b/hw/block/nvme-dif.c
@@ -1,3 +1,13 @@
+/*
+ * QEMU NVM Express End-to-End Data Protection support
+ *
+ * Copyright (c) 2021 Samsung Electronics Co., Ltd.
+ *
+ * Authors:
+ *   Klaus Jensen           <k.jensen@samsung.com>
+ *   Gollu Appalanaidu      <anaidu.gollu@samsung.com>
+ */
+
 #include "qemu/osdep.h"
 #include "hw/block/block.h"
 #include "sysemu/dma.h"
-- 
2.31.1

Re: [PATCH for-6.0 v2 0/8] hw/block/nvme: misc fixes

[Keith Busch]

On Mon, Apr 05, 2021 at 07:54:44PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Various fixes for 6.0.
> 
> v2:
>   - "hw/block/nvme: fix handling of private namespaces"
>     update documentation (Gollu)
>   - add a patch for missing copyright headers

Series looks fine.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH for-6.0 v2 7/8] hw/block/nvme: fix handling of private namespaces

[Minwoo Im]

On 21-04-05 19:54:51, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Prior to this patch, if a private nvme-ns device (that is, a namespace
> that is not linked to a subsystem) is wired up to an nvme-subsys linked
> nvme controller device, the device fails to verify that the namespace id
> is unique within the subsystem. NVM Express v1.4b, Section 6.1.6 ("NSID
> and Namespace Usage") states that because the device supports Namespace
> Management, "NSIDs *shall* be unique within the NVM subsystem".
> 
> Additionally, prior to this patch, private namespaces are not known to
> the subsystem and the namespace is considered exclusive to the
> controller with which it is initially wired up to. However, this is not
> the definition of a private namespace; per Section 1.6.33 ("private
> namespace"), a private namespace is just a namespace that does not
> support multipath I/O or namespace sharing, which means "that it is only
> able to be attached to one controller at a time".
> 
> Fix this by always allocating namespaces in the subsystem (if one is
> linked to the controller), regardsless of the shared/private status of
> the namespace. Whether or not the namespace is shareable is controlled
> by a new `shared` nvme-ns parameter.
> 
> Finally, this fix allows the nvme-ns `subsys` parameter to be removed,
> since the `shared` parameter now serves the purpose of attaching the
> namespace to all controllers in the subsystem upon device realization.
> It is invalid to have an nvme-ns namespace device with a linked
> subsystem without the parent nvme controller device also being linked to
> one and since the nvme-ns devices will unconditionally be "attached" (in
> QEMU terms that is) to an nvme controller device through an NvmeBus, the
> nvme-ns namespace device can always get a reference to the subsystem of
> the controller it is explicitly (using 'bus=' parametr) or implicitly
> attaching to.
> 
> Fixes: e570768566b3 ("hw/block/nvme: support for shared namespace in subsystem")
> Cc: Minwoo Im <minwoo.im.dev@gmail.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Thanks for the fix.

Re: [PATCH for-6.0 v2 3/8] hw/block/nvme: fix the nsid 'invalid' value

[Philippe Mathieu-Daudé]

Hi Klaus,

On 4/5/21 7:54 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The `nvme_nsid()` function returns '-1' (FFFFFFFFh) when the given
> namespace is NULL. Since FFFFFFFFh is actually a valid namespace
> identifier (the "broadcast" value), change this to be '0' since that
> actually *is* the invalid value.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> ---
>  hw/block/nvme-ns.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> index 9ab7894fc83e..82340c4b2574 100644
> --- a/hw/block/nvme-ns.h
> +++ b/hw/block/nvme-ns.h
> @@ -96,7 +96,7 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns)
>          return ns->params.nsid;
>      }
>  
> -    return -1;
> +    return 0;

For 6.1 can you add a NVME_NSID_INVALID definition along
NVME_NSID_BROADCAST and use it here?

>  }
>  
>  static inline bool nvme_ns_shared(NvmeNamespace *ns)
> 

Re: [PATCH for-6.0 v2 4/8] hw/block/nvme: fix controller namespaces array indexing

[Philippe Mathieu-Daudé]

On 4/5/21 7:54 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The controller namespaces array being 0-indexed requires 'nsid - 1'
> everywhere. Something that is easy to miss. Align the controller
> namespaces array with the subsystem namespaces array such that both are
> 1-indexed.

TBH I don't understand the justification. Assuming you hit a
bug and try to protect yourself, maybe now you should also
check for

  assert(n->namespaces[0] == NULL);

somewhere. In nvme_ns() maybe?

> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> ---
>  hw/block/nvme.h | 8 ++++----
>  hw/block/nvme.c | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 9edc86d79e98..c610ab30dc5c 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -217,7 +217,7 @@ typedef struct NvmeCtrl {
>       * Attached namespaces to this controller.  If subsys is not given, all
>       * namespaces in this list will always be attached.
>       */
> -    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES];
> +    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES + 1];
>      NvmeSQueue      **sq;
>      NvmeCQueue      **cq;
>      NvmeSQueue      admin_sq;
> @@ -232,7 +232,7 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
>          return NULL;
>      }
>  
> -    return n->namespaces[nsid - 1];
> +    return n->namespaces[nsid];
>  }
>  
>  static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
> @@ -253,7 +253,7 @@ static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
>      uint32_t nsid = nvme_nsid(ns);
>      assert(nsid && nsid <= NVME_MAX_NAMESPACES);
>  
> -    n->namespaces[nsid - 1] = ns;
> +    n->namespaces[nsid] = ns;
>  }
>  
>  static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
> @@ -261,7 +261,7 @@ static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
>      uint32_t nsid = nvme_nsid(ns);
>      assert(nsid && nsid <= NVME_MAX_NAMESPACES);
>  
> -    n->namespaces[nsid - 1] = NULL;
> +    n->namespaces[nsid] = NULL;
>  }
>  
>  static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index c54ec3c9523c..6d31d5b17a0b 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -5915,7 +5915,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
>              return -1;
>          }
>      } else {
> -        if (n->namespaces[nsid - 1]) {
> +        if (n->namespaces[nsid]) {
>              error_setg(errp, "namespace id '%d' is already in use", nsid);
>              return -1;
>          }
> 

Re: [PATCH for-6.0 v2 6/8] hw/block/nvme: update dmsrl limit on namespace detachment

[Philippe Mathieu-Daudé]

On 4/5/21 7:54 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The Non-MDTS DMSRL limit must be recomputed when namespaces are
> detached.
> 
> Fixes: 645ce1a70cb6 ("hw/block/nvme: support namespace attachment command")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> ---
>  hw/block/nvme.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index de0e726dfdd8..3dc51f407671 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -4876,6 +4876,21 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
>      return NVME_NO_COMPLETE;
>  }
>  
> +static void __nvme_update_dmrsl(NvmeCtrl *n)
> +{
> +    int nsid;
> +
> +    for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
> +        NvmeNamespace *ns = nvme_ns(n, nsid);
> +        if (!ns) {
> +            continue;
> +        }
> +
> +        n->dmrsl = MIN_NON_ZERO(n->dmrsl,
> +                                BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
> +    }
> +}
> +
>  static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
>  static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>  {
> @@ -4925,6 +4940,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>              }
>  
>              nvme_ns_detach(ctrl, ns);
> +
> +            __nvme_update_dmrsl(ctrl);
>          }

Why the '__' prefix? It doesn't seem clearer (I'm not sure there is
a convention, it makes me think of a internal macro expansion use
for preprocessor).

There are very few uses of this prefix:

hw/9pfs/cofs.c:21:static ssize_t __readlink(V9fsState *s, V9fsPath
*path, V9fsString *buf)
hw/block/nvme.c:1683:static uint16_t __nvme_zrm_open(NvmeNamespace *ns,
NvmeZone *zone,
hw/block/nvme.c:1742:static void __nvme_advance_zone_wp(NvmeNamespace
*ns, NvmeZone *zone,
hw/block/nvme.c:5213:static void __nvme_select_ns_iocs(NvmeCtrl *n,
NvmeNamespace *ns)
hw/i386/amd_iommu.c:1160:static int __amdvi_int_remap_msi(AMDVIState *iommu,
hw/intc/s390_flic_kvm.c:255:static int __get_all_irqs(KVMS390FLICState
*flic,
hw/net/rocker/rocker_desc.c:199:static bool
__desc_ring_post_desc(DescRing *ring, int err)
hw/net/sungem.c:766:static uint16_t __sungem_mii_read(SunGEMState *s,
uint8_t phy_addr,
hw/ppc/ppc.c:867:static void __cpu_ppc_store_decr(PowerPCCPU *cpu,
uint64_t *nextp,
hw/s390x/pv.c:25:static int __s390_pv_cmd(uint32_t cmd, const char
*cmdname, void *data)
pc-bios/s390-ccw/cio.c:315:static int __do_cio(SubChannelId schid,
uint32_t ccw_addr, int fmt, Irb *irb)
target/ppc/mmu-hash64.c:170:static void __helper_slbie(CPUPPCState *env,
target_ulong addr,

Thomas, Eric, is it worth cleaning these and updating the
'CODESTYLE.rst'?

Phil.

Re: [PATCH for-6.0 v2 3/8] hw/block/nvme: fix the nsid 'invalid' value

[Klaus Jensen]

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

On Apr  6 08:53, Philippe Mathieu-Daudé wrote:
> Hi Klaus,
> 
> On 4/5/21 7:54 PM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > The `nvme_nsid()` function returns '-1' (FFFFFFFFh) when the given
> > namespace is NULL. Since FFFFFFFFh is actually a valid namespace
> > identifier (the "broadcast" value), change this to be '0' since that
> > actually *is* the invalid value.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > ---
> >  hw/block/nvme-ns.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> > index 9ab7894fc83e..82340c4b2574 100644
> > --- a/hw/block/nvme-ns.h
> > +++ b/hw/block/nvme-ns.h
> > @@ -96,7 +96,7 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns)
> >          return ns->params.nsid;
> >      }
> >  
> > -    return -1;
> > +    return 0;
> 
> For 6.1 can you add a NVME_NSID_INVALID definition along
> NVME_NSID_BROADCAST and use it here?
> 

Good idea Philippe, I'll write that up, thanks!

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

Re: [PATCH for-6.0 v2 6/8] hw/block/nvme: update dmsrl limit on namespace detachment

[Klaus Jensen]

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

On Apr  6 09:10, Philippe Mathieu-Daudé wrote:
> On 4/5/21 7:54 PM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > The Non-MDTS DMSRL limit must be recomputed when namespaces are
> > detached.
> > 
> > Fixes: 645ce1a70cb6 ("hw/block/nvme: support namespace attachment command")
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > ---
> >  hw/block/nvme.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index de0e726dfdd8..3dc51f407671 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -4876,6 +4876,21 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
> >      return NVME_NO_COMPLETE;
> >  }
> >  
> > +static void __nvme_update_dmrsl(NvmeCtrl *n)
> > +{
> > +    int nsid;
> > +
> > +    for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
> > +        NvmeNamespace *ns = nvme_ns(n, nsid);
> > +        if (!ns) {
> > +            continue;
> > +        }
> > +
> > +        n->dmrsl = MIN_NON_ZERO(n->dmrsl,
> > +                                BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
> > +    }
> > +}
> > +
> >  static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
> >  static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
> >  {
> > @@ -4925,6 +4940,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
> >              }
> >  
> >              nvme_ns_detach(ctrl, ns);
> > +
> > +            __nvme_update_dmrsl(ctrl);
> >          }
> 
> Why the '__' prefix? It doesn't seem clearer (I'm not sure there is
> a convention, it makes me think of a internal macro expansion use
> for preprocessor).
> 
> There are very few uses of this prefix:
> 
> hw/9pfs/cofs.c:21:static ssize_t __readlink(V9fsState *s, V9fsPath
> *path, V9fsString *buf)
> hw/block/nvme.c:1683:static uint16_t __nvme_zrm_open(NvmeNamespace *ns,
> NvmeZone *zone,
> hw/block/nvme.c:1742:static void __nvme_advance_zone_wp(NvmeNamespace
> *ns, NvmeZone *zone,
> hw/block/nvme.c:5213:static void __nvme_select_ns_iocs(NvmeCtrl *n,
> NvmeNamespace *ns)
> hw/i386/amd_iommu.c:1160:static int __amdvi_int_remap_msi(AMDVIState *iommu,
> hw/intc/s390_flic_kvm.c:255:static int __get_all_irqs(KVMS390FLICState
> *flic,
> hw/net/rocker/rocker_desc.c:199:static bool
> __desc_ring_post_desc(DescRing *ring, int err)
> hw/net/sungem.c:766:static uint16_t __sungem_mii_read(SunGEMState *s,
> uint8_t phy_addr,
> hw/ppc/ppc.c:867:static void __cpu_ppc_store_decr(PowerPCCPU *cpu,
> uint64_t *nextp,
> hw/s390x/pv.c:25:static int __s390_pv_cmd(uint32_t cmd, const char
> *cmdname, void *data)
> pc-bios/s390-ccw/cio.c:315:static int __do_cio(SubChannelId schid,
> uint32_t ccw_addr, int fmt, Irb *irb)
> target/ppc/mmu-hash64.c:170:static void __helper_slbie(CPUPPCState *env,
> target_ulong addr,
> 
> Thomas, Eric, is it worth cleaning these and updating the
> 'CODESTYLE.rst'?
> 

Yeah ok, I think you are right that there is no clear convention on when
to use this or not. I typically just use it for functions that are
normally not supposed to be called directly.

But I don't even think its consistent in the nvme device. For my sake,
we can clean it up, I'll drop it in this case since there is no good
reason for it other than my own idea of "style".

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

Re: [PATCH for-6.0 v2 4/8] hw/block/nvme: fix controller namespaces array indexing

[Klaus Jensen]

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

On Apr  6 09:01, Philippe Mathieu-Daudé wrote:
> On 4/5/21 7:54 PM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > The controller namespaces array being 0-indexed requires 'nsid - 1'
> > everywhere. Something that is easy to miss. Align the controller
> > namespaces array with the subsystem namespaces array such that both are
> > 1-indexed.
> 
> TBH I don't understand the justification.

Justification is mostly to align with the subsystem device. I like the
'1-indexed' approach better. And the -1 causes Coverity to complain
before the assert was added.

> Assuming you hit a
> bug and try to protect yourself, maybe now you should also
> check for
> 
>   assert(n->namespaces[0] == NULL);
> 
> somewhere. In nvme_ns() maybe?
> 

That is definitely a state that should always hold, I guess we can do
that, but we do already guard all "insertions" into the namespace array
by an assert on the nsid. Then again, asserting here makes sure that we
don't introduce something else that inserts on this invalid position.

So, good point, I'll add it.

> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > ---
> >  hw/block/nvme.h | 8 ++++----
> >  hw/block/nvme.c | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 9edc86d79e98..c610ab30dc5c 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -217,7 +217,7 @@ typedef struct NvmeCtrl {
> >       * Attached namespaces to this controller.  If subsys is not given, all
> >       * namespaces in this list will always be attached.
> >       */
> > -    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES];
> > +    NvmeNamespace   *namespaces[NVME_MAX_NAMESPACES + 1];
> >      NvmeSQueue      **sq;
> >      NvmeCQueue      **cq;
> >      NvmeSQueue      admin_sq;
> > @@ -232,7 +232,7 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
> >          return NULL;
> >      }
> >  
> > -    return n->namespaces[nsid - 1];
> > +    return n->namespaces[nsid];
> >  }
> >  
> >  static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
> > @@ -253,7 +253,7 @@ static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
> >      uint32_t nsid = nvme_nsid(ns);
> >      assert(nsid && nsid <= NVME_MAX_NAMESPACES);
> >  
> > -    n->namespaces[nsid - 1] = ns;
> > +    n->namespaces[nsid] = ns;
> >  }
> >  
> >  static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
> > @@ -261,7 +261,7 @@ static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
> >      uint32_t nsid = nvme_nsid(ns);
> >      assert(nsid && nsid <= NVME_MAX_NAMESPACES);
> >  
> > -    n->namespaces[nsid - 1] = NULL;
> > +    n->namespaces[nsid] = NULL;
> >  }
> >  
> >  static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index c54ec3c9523c..6d31d5b17a0b 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -5915,7 +5915,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> >              return -1;
> >          }
> >      } else {
> > -        if (n->namespaces[nsid - 1]) {
> > +        if (n->namespaces[nsid]) {
> >              error_setg(errp, "namespace id '%d' is already in use", nsid);
> >              return -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: [PATCH for-6.0 v2 4/8] hw/block/nvme: fix controller namespaces array indexing

[Klaus Jensen]

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

On Apr  6 09:28, Klaus Jensen wrote:
> On Apr  6 09:01, Philippe Mathieu-Daudé wrote:
> > On 4/5/21 7:54 PM, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jensen@samsung.com>
> > > 
> > > The controller namespaces array being 0-indexed requires 'nsid - 1'
> > > everywhere. Something that is easy to miss. Align the controller
> > > namespaces array with the subsystem namespaces array such that both are
> > > 1-indexed.
> > 
> > TBH I don't understand the justification.
> 
> Justification is mostly to align with the subsystem device. I like the
> '1-indexed' approach better. And the -1 causes Coverity to complain
> before the assert was added.
> 
> > Assuming you hit a
> > bug and try to protect yourself, maybe now you should also
> > check for
> > 
> >   assert(n->namespaces[0] == NULL);
> > 
> > somewhere. In nvme_ns() maybe?
> > 
> 
> That is definitely a state that should always hold, I guess we can do
> that, but we do already guard all "insertions" into the namespace array
> by an assert on the nsid. Then again, asserting here makes sure that we
> don't introduce something else that inserts on this invalid position.
> 
> So, good point, I'll add it.
> 

Then again again.

I don't see the reason for the assert. Even if something ends up there 
by mistake we will never return it. If something ends up there due to 
new code, that nvme_ns() will always return NULL when nsid is zero and 
that should weed out the bug easily.

I'll update the commit message to make it clear that this is about 
making both the subsystem and controller namespaces arrays 1-indexed. 
Them being indexed differently is a recipe for disaster I'd say.

In anycase, I it actually a stretch to call this a bug fix, so I'll drop 
it and queue it up for v6.1.

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

Re: [PATCH for-6.0 v2 6/8] hw/block/nvme: update dmsrl limit on namespace detachment

[Thomas Huth]

On 06/04/2021 09.24, Klaus Jensen wrote:
> On Apr  6 09:10, Philippe Mathieu-Daudé wrote:
>> On 4/5/21 7:54 PM, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> The Non-MDTS DMSRL limit must be recomputed when namespaces are
>>> detached.
>>>
>>> Fixes: 645ce1a70cb6 ("hw/block/nvme: support namespace attachment command")
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>>> ---
>>>   hw/block/nvme.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index de0e726dfdd8..3dc51f407671 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -4876,6 +4876,21 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
>>>       return NVME_NO_COMPLETE;
>>>   }
>>>   
>>> +static void __nvme_update_dmrsl(NvmeCtrl *n)
>>> +{
>>> +    int nsid;
>>> +
>>> +    for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
>>> +        NvmeNamespace *ns = nvme_ns(n, nsid);
>>> +        if (!ns) {
>>> +            continue;
>>> +        }
>>> +
>>> +        n->dmrsl = MIN_NON_ZERO(n->dmrsl,
>>> +                                BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
>>> +    }
>>> +}
>>> +
>>>   static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
>>>   static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>>>   {
>>> @@ -4925,6 +4940,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>>>               }
>>>   
>>>               nvme_ns_detach(ctrl, ns);
>>> +
>>> +            __nvme_update_dmrsl(ctrl);
>>>           }
>>
>> Why the '__' prefix? It doesn't seem clearer (I'm not sure there is
>> a convention, it makes me think of a internal macro expansion use
>> for preprocessor).
>>
>> There are very few uses of this prefix:
>>
>> hw/9pfs/cofs.c:21:static ssize_t __readlink(V9fsState *s, V9fsPath
>> *path, V9fsString *buf)
>> hw/block/nvme.c:1683:static uint16_t __nvme_zrm_open(NvmeNamespace *ns,
>> NvmeZone *zone,
>> hw/block/nvme.c:1742:static void __nvme_advance_zone_wp(NvmeNamespace
>> *ns, NvmeZone *zone,
>> hw/block/nvme.c:5213:static void __nvme_select_ns_iocs(NvmeCtrl *n,
>> NvmeNamespace *ns)
>> hw/i386/amd_iommu.c:1160:static int __amdvi_int_remap_msi(AMDVIState *iommu,
>> hw/intc/s390_flic_kvm.c:255:static int __get_all_irqs(KVMS390FLICState
>> *flic,
>> hw/net/rocker/rocker_desc.c:199:static bool
>> __desc_ring_post_desc(DescRing *ring, int err)
>> hw/net/sungem.c:766:static uint16_t __sungem_mii_read(SunGEMState *s,
>> uint8_t phy_addr,
>> hw/ppc/ppc.c:867:static void __cpu_ppc_store_decr(PowerPCCPU *cpu,
>> uint64_t *nextp,
>> hw/s390x/pv.c:25:static int __s390_pv_cmd(uint32_t cmd, const char
>> *cmdname, void *data)
>> pc-bios/s390-ccw/cio.c:315:static int __do_cio(SubChannelId schid,
>> uint32_t ccw_addr, int fmt, Irb *irb)
>> target/ppc/mmu-hash64.c:170:static void __helper_slbie(CPUPPCState *env,
>> target_ulong addr,
>>
>> Thomas, Eric, is it worth cleaning these and updating the
>> 'CODESTYLE.rst'?
>>
> 
> Yeah ok, I think you are right that there is no clear convention on when
> to use this or not. I typically just use it for functions that are
> normally not supposed to be called directly.
> 
> But I don't even think its consistent in the nvme device. For my sake,
> we can clean it up, I'll drop it in this case since there is no good
> reason for it other than my own idea of "style".

IIRC all identifiers that start with two underscores are reserved by the C 
standard:

  https://busybox.net/~landley/c99-draft.html#7.1.3

Thus you should not use two underscores at the beginning here at all.

  HTH,
   Thomas

Re: [PATCH for-6.0 v2 6/8] hw/block/nvme: update dmsrl limit on namespace detachment

[Klaus Jensen]

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

On Apr  9 19:39, Thomas Huth wrote:
>On 06/04/2021 09.24, Klaus Jensen wrote:
>>On Apr  6 09:10, Philippe Mathieu-Daudé wrote:
>>>On 4/5/21 7:54 PM, Klaus Jensen wrote:
>>>>From: Klaus Jensen <k.jensen@samsung.com>
>>>>
>>>>The Non-MDTS DMSRL limit must be recomputed when namespaces are
>>>>detached.
>>>>
>>>>Fixes: 645ce1a70cb6 ("hw/block/nvme: support namespace attachment command")
>>>>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>>>>---
>>>>  hw/block/nvme.c | 17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>>
>>>>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>>index de0e726dfdd8..3dc51f407671 100644
>>>>--- a/hw/block/nvme.c
>>>>+++ b/hw/block/nvme.c
>>>>@@ -4876,6 +4876,21 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
>>>>      return NVME_NO_COMPLETE;
>>>>  }
>>>>+static void __nvme_update_dmrsl(NvmeCtrl *n)
>>>>+{
>>>>+    int nsid;
>>>>+
>>>>+    for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
>>>>+        NvmeNamespace *ns = nvme_ns(n, nsid);
>>>>+        if (!ns) {
>>>>+            continue;
>>>>+        }
>>>>+
>>>>+        n->dmrsl = MIN_NON_ZERO(n->dmrsl,
>>>>+                                BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
>>>>+    }
>>>>+}
>>>>+
>>>>  static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
>>>>  static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>>>>  {
>>>>@@ -4925,6 +4940,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>>>>              }
>>>>              nvme_ns_detach(ctrl, ns);
>>>>+
>>>>+            __nvme_update_dmrsl(ctrl);
>>>>          }
>>>
>>>Why the '__' prefix? It doesn't seem clearer (I'm not sure there is
>>>a convention, it makes me think of a internal macro expansion use
>>>for preprocessor).
>>>
>>>There are very few uses of this prefix:
>>>
>>>hw/9pfs/cofs.c:21:static ssize_t __readlink(V9fsState *s, V9fsPath
>>>*path, V9fsString *buf)
>>>hw/block/nvme.c:1683:static uint16_t __nvme_zrm_open(NvmeNamespace *ns,
>>>NvmeZone *zone,
>>>hw/block/nvme.c:1742:static void __nvme_advance_zone_wp(NvmeNamespace
>>>*ns, NvmeZone *zone,
>>>hw/block/nvme.c:5213:static void __nvme_select_ns_iocs(NvmeCtrl *n,
>>>NvmeNamespace *ns)
>>>hw/i386/amd_iommu.c:1160:static int __amdvi_int_remap_msi(AMDVIState *iommu,
>>>hw/intc/s390_flic_kvm.c:255:static int __get_all_irqs(KVMS390FLICState
>>>*flic,
>>>hw/net/rocker/rocker_desc.c:199:static bool
>>>__desc_ring_post_desc(DescRing *ring, int err)
>>>hw/net/sungem.c:766:static uint16_t __sungem_mii_read(SunGEMState *s,
>>>uint8_t phy_addr,
>>>hw/ppc/ppc.c:867:static void __cpu_ppc_store_decr(PowerPCCPU *cpu,
>>>uint64_t *nextp,
>>>hw/s390x/pv.c:25:static int __s390_pv_cmd(uint32_t cmd, const char
>>>*cmdname, void *data)
>>>pc-bios/s390-ccw/cio.c:315:static int __do_cio(SubChannelId schid,
>>>uint32_t ccw_addr, int fmt, Irb *irb)
>>>target/ppc/mmu-hash64.c:170:static void __helper_slbie(CPUPPCState *env,
>>>target_ulong addr,
>>>
>>>Thomas, Eric, is it worth cleaning these and updating the
>>>'CODESTYLE.rst'?
>>>
>>
>>Yeah ok, I think you are right that there is no clear convention on when
>>to use this or not. I typically just use it for functions that are
>>normally not supposed to be called directly.
>>
>>But I don't even think its consistent in the nvme device. For my sake,
>>we can clean it up, I'll drop it in this case since there is no good
>>reason for it other than my own idea of "style".
>
>IIRC all identifiers that start with two underscores are reserved by 
>the C standard:
>
> https://busybox.net/~landley/c99-draft.html#7.1.3
>
>Thus you should not use two underscores at the beginning here at all.
>

I'll clean up the remaining double underscores in the next cycle!

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