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

2021-04-06 Thread Minwoo Im
On 21-04-05 19:54:51, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> 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 
> Signed-off-by: Klaus Jensen 
> Reviewed-by: Gollu Appalanaidu 

Reviewed-by: Minwoo Im 

Thanks for the fix.



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

2021-04-05 Thread Klaus Jensen
From: Klaus Jensen 

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 
Signed-off-by: Klaus Jensen 
Reviewed-by: Gollu Appalanaidu 
---
 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 = >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
+++