* [PATCH V3 0/8] hw/block/nvme: support namespace attachment
@ 2021-02-28 16:10 Minwoo Im
2021-02-28 16:10 ` [PATCH V3 1/8] hw/block/nvme: support namespace detach Minwoo Im
` (7 more replies)
0 siblings, 8 replies; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:10 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Keith Busch, Klaus Jensen, Minwoo Im, Kevin Wolf, Max Reitz
Hello,
This series supports namespace attachment: attach and detach. This is
the third version of series with fixing command events and asynchronous
events based on Keith's review.
Since command effects for the namespace attachment command is added in
this version, we no longer need to rescan controller after namespace
attachment command. Kernel will rescan the controller namespaces after
the command successfully done through passthru.
Please review.
Thanks,
Since V2:
- Added command effects (namespace inventory changed) for namespace
attach command. (Keith)
- Added [7/8] patch to support asynchronus event when namespace
inventory is updated. (Keith)
- Added review and tested tag from Klaus to all the patches, but [6/8]
and [7/8].
Since V1:
- Fix to take 'ctrl' which is given from the command rather than 'n'.
(Klaus)
- Add a [7/7] patch to support CNS 12h Identify command (Namespace
Attached Controller list).
Minwoo Im (8):
hw/block/nvme: support namespace detach
hw/block/nvme: fix namespaces array to 1-based
hw/block/nvme: fix allocated namespace list to 256
hw/block/nvme: support allocated namespace type
hw/block/nvme: refactor nvme_select_ns_iocs
hw/block/nvme: support namespace attachment command
hw/block/nvme: support changed namespace asyncrohous event
hw/block/nvme: support Identify NS Attached Controller List
hw/block/nvme-ns.c | 1 +
hw/block/nvme-ns.h | 1 +
hw/block/nvme-subsys.h | 28 +++-
hw/block/nvme.c | 287 ++++++++++++++++++++++++++++++++++++-----
hw/block/nvme.h | 40 ++++++
hw/block/trace-events | 3 +
include/block/nvme.h | 14 ++
7 files changed, 338 insertions(+), 36 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V3 1/8] hw/block/nvme: support namespace detach
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
@ 2021-02-28 16:10 ` Minwoo Im
2021-03-15 4:56 ` Keqian Zhu
2021-02-28 16:10 ` [PATCH V3 2/8] hw/block/nvme: fix namespaces array to 1-based Minwoo Im
` (6 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:10 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Kevin Wolf, Klaus Jensen, Max Reitz, Klaus Jensen, Minwoo Im,
Keith Busch
Given that now we have nvme-subsys device supported, we can manage
namespace allocated, but not attached: detached. This patch introduced
a parameter for nvme-ns device named 'detached'. This parameter
indicates whether the given namespace device is detached from
a entire NVMe subsystem('subsys' given case, shared namespace) or a
controller('bus' given case, private namespace).
- Allocated namespace
1) Shared ns in the subsystem 'subsys0':
-device nvme-ns,id=ns1,drive=blknvme0,nsid=1,subsys=subsys0,detached=true
2) Private ns for the controller 'nvme0' of the subsystem 'subsys0':
-device nvme-subsys,id=subsys0
-device nvme,serial=foo,id=nvme0,subsys=subsys0
-device nvme-ns,id=ns1,drive=blknvme0,nsid=1,bus=nvme0,detached=true
3) (Invalid case) Controller 'nvme0' has no subsystem to manage ns:
-device nvme,serial=foo,id=nvme0
-device nvme-ns,id=ns1,drive=blknvme0,nsid=1,bus=nvme0,detached=true
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
Tested-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme-ns.c | 1 +
hw/block/nvme-ns.h | 1 +
hw/block/nvme-subsys.h | 1 +
hw/block/nvme.c | 41 +++++++++++++++++++++++++++++++++++++++--
hw/block/nvme.h | 22 ++++++++++++++++++++++
5 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 0e8760020483..eda6a0c003a4 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -399,6 +399,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_UINT16("mssrl", NvmeNamespace, params.mssrl, 128),
diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
index 7af6884862b5..b0c00e115d81 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;
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index ccf6a71398d3..890d118117dc 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -23,6 +23,7 @@ typedef struct NvmeSubsystem {
uint8_t subnqn[256];
NvmeCtrl *ctrls[NVME_SUBSYS_MAX_CTRLS];
+ /* Allocated namespaces for this subsystem */
NvmeNamespace *namespaces[NVME_SUBSYS_MAX_NAMESPACES];
} NvmeSubsystem;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index edd0b85c10ce..f6aeae081840 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -23,7 +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> \
+ * subsys=<subsys_id>,detached=<true|false[optional]>
* -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
* zoned=<true|false[optional]>, \
* subsys=<subsys_id>
@@ -82,6 +82,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 attahced to all
+ * controllers in the subsystem by default.
+ * It's mutual exclusive with 'bus' parameter. 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:
@@ -4613,6 +4620,20 @@ 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);
@@ -4644,7 +4665,23 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
trace_pci_nvme_register_namespace(nsid);
- n->namespaces[nsid - 1] = ns;
+ /*
+ * 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);
+ }
+ }
return 0;
}
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index f45ace0cff5b..51b8739b4d1e 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -174,6 +174,10 @@ typedef struct NvmeCtrl {
NvmeSubsystem *subsys;
NvmeNamespace namespace;
+ /*
+ * Attached namespaces to this controller. If subsys is not given, all
+ * namespaces in this list will always be attached.
+ */
NvmeNamespace *namespaces[NVME_MAX_NAMESPACES];
NvmeSQueue **sq;
NvmeCQueue **cq;
@@ -192,6 +196,24 @@ 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)
+{
+ 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)
+{
+ n->namespaces[nvme_nsid(ns) - 1] = ns;
+}
+
static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
{
NvmeSQueue *sq = req->sq;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V3 2/8] hw/block/nvme: fix namespaces array to 1-based
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
2021-02-28 16:10 ` [PATCH V3 1/8] hw/block/nvme: support namespace detach Minwoo Im
@ 2021-02-28 16:10 ` Minwoo Im
2021-02-28 16:10 ` [PATCH V3 3/8] hw/block/nvme: fix allocated namespace list to 256 Minwoo Im
` (5 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:10 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Kevin Wolf, Klaus Jensen, Max Reitz, Klaus Jensen, Minwoo Im,
Keith Busch
subsys->namespaces array used to be sized to NVME_SUBSYS_MAX_NAMESPACES.
But subsys->namespaces are being accessed with 1-based namespace id
which means the very first array entry will always be empty(NULL).
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
Tested-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme-subsys.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 890d118117dc..574774390c4c 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -24,7 +24,7 @@ typedef struct NvmeSubsystem {
NvmeCtrl *ctrls[NVME_SUBSYS_MAX_CTRLS];
/* Allocated namespaces for this subsystem */
- NvmeNamespace *namespaces[NVME_SUBSYS_MAX_NAMESPACES];
+ NvmeNamespace *namespaces[NVME_SUBSYS_MAX_NAMESPACES + 1];
} NvmeSubsystem;
int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V3 3/8] hw/block/nvme: fix allocated namespace list to 256
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
2021-02-28 16:10 ` [PATCH V3 1/8] hw/block/nvme: support namespace detach Minwoo Im
2021-02-28 16:10 ` [PATCH V3 2/8] hw/block/nvme: fix namespaces array to 1-based Minwoo Im
@ 2021-02-28 16:10 ` Minwoo Im
2021-02-28 16:10 ` [PATCH V3 4/8] hw/block/nvme: support allocated namespace type Minwoo Im
` (4 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:10 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Kevin Wolf, Klaus Jensen, Max Reitz, Klaus Jensen, Minwoo Im,
Keith Busch
Expand allocated namespace list (subsys->namespaces) to have 256 entries
which is a value lager than at least NVME_MAX_NAMESPACES which is for
attached namespace list in a controller.
Allocated namespace list should at least larger than attached namespace
list.
n->num_namespaces = NVME_MAX_NAMESPACES;
The above line will set the NN field by id->nn so that the subsystem
should also prepare at least this number of namespace list entries.
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
Tested-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme-subsys.h | 2 +-
hw/block/nvme.h | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 574774390c4c..8a0732b22316 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 32
+#define NVME_SUBSYS_MAX_NAMESPACES 256
typedef struct NvmeCtrl NvmeCtrl;
typedef struct NvmeNamespace NvmeNamespace;
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 51b8739b4d1e..7599d6b1a41b 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -10,6 +10,12 @@
#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 */
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V3 4/8] hw/block/nvme: support allocated namespace type
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
` (2 preceding siblings ...)
2021-02-28 16:10 ` [PATCH V3 3/8] hw/block/nvme: fix allocated namespace list to 256 Minwoo Im
@ 2021-02-28 16:10 ` Minwoo Im
2021-02-28 16:10 ` [PATCH V3 5/8] hw/block/nvme: refactor nvme_select_ns_iocs Minwoo Im
` (3 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:10 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Kevin Wolf, Klaus Jensen, Max Reitz, Klaus Jensen, Minwoo Im,
Keith Busch
From NVMe spec 1.4b "6.1.5. NSID and Namespace Relationships" defines
valid namespace types:
- Unallocated: Not exists in the NVMe subsystem
- Allocated: Exists in the NVMe subsystem
- Inactive: Not attached to the controller
- Active: Attached to the controller
This patch added support for allocated, but not attached namespace type:
!nvme_ns(n, nsid) && nvme_subsys_ns(n->subsys, nsid)
nvme_ns() returns attached namespace instance of the given controller
and nvme_subsys_ns() returns allocated namespace instance in the
subsystem.
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
Tested-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme-subsys.h | 13 +++++++++
hw/block/nvme.c | 63 +++++++++++++++++++++++++++++++-----------
2 files changed, 60 insertions(+), 16 deletions(-)
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 8a0732b22316..14627f9ccb41 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -30,4 +30,17 @@ typedef struct NvmeSubsystem {
int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp);
+/*
+ * Return allocated namespace of the specified nsid in the subsystem.
+ */
+static inline NvmeNamespace *nvme_subsys_ns(NvmeSubsystem *subsys,
+ uint32_t nsid)
+{
+ if (!subsys) {
+ return NULL;
+ }
+
+ return subsys->namespaces[nsid];
+}
+
#endif /* NVME_SUBSYS_H */
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f6aeae081840..53c4d59e09a7 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3225,7 +3225,7 @@ static uint16_t nvme_identify_ctrl_csi(NvmeCtrl *n, NvmeRequest *req)
return NVME_INVALID_FIELD | NVME_DNR;
}
-static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req)
+static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req, bool active)
{
NvmeNamespace *ns;
NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
@@ -3239,7 +3239,14 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req)
ns = nvme_ns(n, nsid);
if (unlikely(!ns)) {
- return nvme_rpt_empty_id_struct(n, req);
+ if (!active) {
+ ns = nvme_subsys_ns(n->subsys, nsid);
+ if (!ns) {
+ return nvme_rpt_empty_id_struct(n, req);
+ }
+ } else {
+ return nvme_rpt_empty_id_struct(n, req);
+ }
}
if (c->csi == NVME_CSI_NVM && nvme_csi_has_nvm_support(ns)) {
@@ -3250,7 +3257,8 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req)
return NVME_INVALID_CMD_SET | NVME_DNR;
}
-static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req)
+static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req,
+ bool active)
{
NvmeNamespace *ns;
NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
@@ -3264,7 +3272,14 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req)
ns = nvme_ns(n, nsid);
if (unlikely(!ns)) {
- return nvme_rpt_empty_id_struct(n, req);
+ if (!active) {
+ ns = nvme_subsys_ns(n->subsys, nsid);
+ if (!ns) {
+ return nvme_rpt_empty_id_struct(n, req);
+ }
+ } else {
+ return nvme_rpt_empty_id_struct(n, req);
+ }
}
if (c->csi == NVME_CSI_NVM && nvme_csi_has_nvm_support(ns)) {
@@ -3277,7 +3292,8 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req)
return NVME_INVALID_FIELD | NVME_DNR;
}
-static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
+static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req,
+ bool active)
{
NvmeNamespace *ns;
NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
@@ -3302,7 +3318,14 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
for (i = 1; i <= n->num_namespaces; i++) {
ns = nvme_ns(n, i);
if (!ns) {
- continue;
+ if (!active) {
+ ns = nvme_subsys_ns(n->subsys, i);
+ if (!ns) {
+ continue;
+ }
+ } else {
+ continue;
+ }
}
if (ns->params.nsid <= min_nsid) {
continue;
@@ -3316,7 +3339,8 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
return nvme_dma(n, list, data_len, DMA_DIRECTION_FROM_DEVICE, req);
}
-static uint16_t nvme_identify_nslist_csi(NvmeCtrl *n, NvmeRequest *req)
+static uint16_t nvme_identify_nslist_csi(NvmeCtrl *n, NvmeRequest *req,
+ bool active)
{
NvmeNamespace *ns;
NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
@@ -3342,7 +3366,14 @@ static uint16_t nvme_identify_nslist_csi(NvmeCtrl *n, NvmeRequest *req)
for (i = 1; i <= n->num_namespaces; i++) {
ns = nvme_ns(n, i);
if (!ns) {
- continue;
+ if (!active) {
+ ns = nvme_subsys_ns(n->subsys, i);
+ if (!ns) {
+ continue;
+ }
+ } else {
+ continue;
+ }
}
if (ns->params.nsid <= min_nsid || c->csi != ns->csi) {
continue;
@@ -3422,25 +3453,25 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
switch (le32_to_cpu(c->cns)) {
case NVME_ID_CNS_NS:
- /* fall through */
+ return nvme_identify_ns(n, req, true);
case NVME_ID_CNS_NS_PRESENT:
- return nvme_identify_ns(n, req);
+ return nvme_identify_ns(n, req, false);
case NVME_ID_CNS_CS_NS:
- /* fall through */
+ return nvme_identify_ns_csi(n, req, true);
case NVME_ID_CNS_CS_NS_PRESENT:
- return nvme_identify_ns_csi(n, req);
+ return nvme_identify_ns_csi(n, req, false);
case NVME_ID_CNS_CTRL:
return nvme_identify_ctrl(n, 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(n, req, true);
case NVME_ID_CNS_NS_PRESENT_LIST:
- return nvme_identify_nslist(n, req);
+ return nvme_identify_nslist(n, req, false);
case NVME_ID_CNS_CS_NS_ACTIVE_LIST:
- /* fall through */
+ return nvme_identify_nslist_csi(n, req, true);
case NVME_ID_CNS_CS_NS_PRESENT_LIST:
- return nvme_identify_nslist_csi(n, req);
+ return nvme_identify_nslist_csi(n, req, false);
case NVME_ID_CNS_NS_DESCR_LIST:
return nvme_identify_ns_descr_list(n, req);
case NVME_ID_CNS_IO_COMMAND_SET:
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V3 5/8] hw/block/nvme: refactor nvme_select_ns_iocs
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
` (3 preceding siblings ...)
2021-02-28 16:10 ` [PATCH V3 4/8] hw/block/nvme: support allocated namespace type Minwoo Im
@ 2021-02-28 16:10 ` Minwoo Im
2021-02-28 16:10 ` [PATCH V3 6/8] hw/block/nvme: support namespace attachment command Minwoo Im
` (2 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:10 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Kevin Wolf, Klaus Jensen, Max Reitz, Klaus Jensen, Minwoo Im,
Keith Busch
This patch has no functional changes. This patch just refactored
nvme_select_ns_iocs() to iterate the attached namespaces of the
controlller and make it invoke __nvme_select_ns_iocs().
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
Tested-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme.c | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 53c4d59e09a7..b18ab0ef810f 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4000,6 +4000,25 @@ static void nvme_ctrl_shutdown(NvmeCtrl *n)
}
}
+static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns)
+{
+ ns->iocs = nvme_cse_iocs_none;
+ switch (ns->csi) {
+ case NVME_CSI_NVM:
+ if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
+ ns->iocs = nvme_cse_iocs_nvm;
+ }
+ break;
+ case NVME_CSI_ZONED:
+ if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_CSI) {
+ ns->iocs = nvme_cse_iocs_zoned;
+ } else if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_NVM) {
+ ns->iocs = nvme_cse_iocs_nvm;
+ }
+ break;
+ }
+}
+
static void nvme_select_ns_iocs(NvmeCtrl *n)
{
NvmeNamespace *ns;
@@ -4010,21 +4029,8 @@ static void nvme_select_ns_iocs(NvmeCtrl *n)
if (!ns) {
continue;
}
- ns->iocs = nvme_cse_iocs_none;
- switch (ns->csi) {
- case NVME_CSI_NVM:
- if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
- ns->iocs = nvme_cse_iocs_nvm;
- }
- break;
- case NVME_CSI_ZONED:
- if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_CSI) {
- ns->iocs = nvme_cse_iocs_zoned;
- } else if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_NVM) {
- ns->iocs = nvme_cse_iocs_nvm;
- }
- break;
- }
+
+ __nvme_select_ns_iocs(n, ns);
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V3 6/8] hw/block/nvme: support namespace attachment command
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
` (4 preceding siblings ...)
2021-02-28 16:10 ` [PATCH V3 5/8] hw/block/nvme: refactor nvme_select_ns_iocs Minwoo Im
@ 2021-02-28 16:10 ` Minwoo Im
2021-02-28 16:10 ` [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event Minwoo Im
2021-02-28 16:11 ` [PATCH V3 8/8] hw/block/nvme: support Identify NS Attached Controller List Minwoo Im
7 siblings, 0 replies; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:10 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Keith Busch, Klaus Jensen, Minwoo Im, Kevin Wolf, Max Reitz
This patch supports Namespace Attachment command for the pre-defined
nvme-ns device nodes. Of course, attach/detach namespace should only be
supported in case 'subsys' is given. This is because if we detach a
namespace from a controller, somebody needs to manage the detached, but
allocated namespace in the NVMe subsystem.
As command effect for the namespace attachment command is registered,
the host will be notified that namespace inventory is changed so that
host will rescan the namespace inventory after this command. For
example, kernel driver manages this command effect via passthru IOCTL.
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
hw/block/nvme-subsys.h | 10 +++++++
hw/block/nvme.c | 61 +++++++++++++++++++++++++++++++++++++++++-
hw/block/nvme.h | 5 ++++
hw/block/trace-events | 2 ++
include/block/nvme.h | 6 +++++
5 files changed, 83 insertions(+), 1 deletion(-)
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 14627f9ccb41..ef4bec928eae 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -30,6 +30,16 @@ typedef struct 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)
+{
+ if (!subsys) {
+ return NULL;
+ }
+
+ return subsys->ctrls[cntlid];
+}
+
/*
* Return allocated namespace of the specified nsid in the subsystem.
*/
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b18ab0ef810f..68c2e63d9412 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -187,6 +187,7 @@ static const uint32_t nvme_cse_acs[256] = {
[NVME_ADM_CMD_SET_FEATURES] = NVME_CMD_EFF_CSUPP,
[NVME_ADM_CMD_GET_FEATURES] = NVME_CMD_EFF_CSUPP,
[NVME_ADM_CMD_ASYNC_EV_REQ] = NVME_CMD_EFF_CSUPP,
+ [NVME_ADM_CMD_NS_ATTACHMENT] = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_NIC,
};
static const uint32_t nvme_cse_iocs_none[256];
@@ -3868,6 +3869,62 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
return NVME_NO_COMPLETE;
}
+static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
+static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
+{
+ NvmeNamespace *ns;
+ NvmeCtrl *ctrl;
+ uint16_t list[NVME_CONTROLLER_LIST_SIZE] = {};
+ uint32_t nsid = le32_to_cpu(req->cmd.nsid);
+ uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
+ bool attach = !(dw10 & 0xf);
+ uint16_t *nr_ids = &list[0];
+ uint16_t *ids = &list[1];
+ uint16_t ret;
+ int i;
+
+ trace_pci_nvme_ns_attachment(nvme_cid(req), dw10 & 0xf);
+
+ ns = nvme_subsys_ns(n->subsys, nsid);
+ if (!ns) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ ret = nvme_dma(n, (uint8_t *)list, 4096,
+ DMA_DIRECTION_TO_DEVICE, req);
+ if (ret) {
+ return ret;
+ }
+
+ if (!*nr_ids) {
+ return NVME_NS_CTRL_LIST_INVALID | NVME_DNR;
+ }
+
+ for (i = 0; i < *nr_ids; i++) {
+ ctrl = nvme_subsys_ctrl(n->subsys, ids[i]);
+ if (!ctrl) {
+ return NVME_NS_CTRL_LIST_INVALID | NVME_DNR;
+ }
+
+ if (attach) {
+ if (nvme_ns_is_attached(ctrl, ns)) {
+ return NVME_NS_ALREADY_ATTACHED | NVME_DNR;
+ }
+
+ nvme_ns_attach(ctrl, ns);
+ __nvme_select_ns_iocs(ctrl, ns);
+ } else {
+ if (!nvme_ns_is_attached(ctrl, ns)) {
+ return NVME_NS_NOT_ATTACHED | NVME_DNR;
+ }
+
+ nvme_ns_detach(ctrl, ns);
+ }
+ }
+
+ return NVME_SUCCESS;
+}
+
static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
{
trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), req->cmd.opcode,
@@ -3899,6 +3956,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
return nvme_get_feature(n, req);
case NVME_ADM_CMD_ASYNC_EV_REQ:
return nvme_aer(n, req);
+ case NVME_ADM_CMD_NS_ATTACHMENT:
+ return nvme_ns_attachment(n, req);
default:
assert(false);
}
@@ -4865,7 +4924,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->mdts = n->params.mdts;
id->ver = cpu_to_le32(NVME_SPEC_VER);
- id->oacs = cpu_to_le16(0);
+ id->oacs = cpu_to_le16(NVME_OACS_NS_MGMT);
id->cntrltype = 0x1;
/*
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 7599d6b1a41b..74a00ab21a55 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -220,6 +220,11 @@ static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
n->namespaces[nvme_nsid(ns) - 1] = ns;
}
+static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
+{
+ n->namespaces[nvme_nsid(ns) - 1] = NULL;
+}
+
static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
{
NvmeSQueue *sq = req->sq;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 25ba51ea5405..98d542c999e2 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -82,6 +82,8 @@ pci_nvme_aer(uint16_t cid) "cid %"PRIu16""
pci_nvme_aer_aerl_exceeded(void) "aerl exceeded"
pci_nvme_aer_masked(uint8_t type, uint8_t mask) "type 0x%"PRIx8" mask 0x%"PRIx8""
pci_nvme_aer_post_cqe(uint8_t typ, uint8_t info, uint8_t log_page) "type 0x%"PRIx8" info 0x%"PRIx8" lid 0x%"PRIx8""
+pci_nvme_ns_attachment(uint16_t cid, uint8_t sel) "cid %"PRIu16", sel=0x%"PRIx8""
+pci_nvme_ns_attachment_attach(uint16_t cntlid, uint32_t nsid) "cntlid=0x%"PRIx16", nsid=0x%"PRIx32""
pci_nvme_enqueue_event(uint8_t typ, uint8_t info, uint8_t log_page) "type 0x%"PRIx8" info 0x%"PRIx8" lid 0x%"PRIx8""
pci_nvme_enqueue_event_noqueue(int queued) "queued %d"
pci_nvme_enqueue_event_masked(uint8_t typ) "type 0x%"PRIx8""
diff --git a/include/block/nvme.h b/include/block/nvme.h
index b23f3ae2279f..339784d9c23a 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -566,6 +566,7 @@ enum NvmeAdminCommands {
NVME_ADM_CMD_ASYNC_EV_REQ = 0x0c,
NVME_ADM_CMD_ACTIVATE_FW = 0x10,
NVME_ADM_CMD_DOWNLOAD_FW = 0x11,
+ NVME_ADM_CMD_NS_ATTACHMENT = 0x15,
NVME_ADM_CMD_FORMAT_NVM = 0x80,
NVME_ADM_CMD_SECURITY_SEND = 0x81,
NVME_ADM_CMD_SECURITY_RECV = 0x82,
@@ -836,6 +837,9 @@ enum NvmeStatusCodes {
NVME_FEAT_NOT_CHANGEABLE = 0x010e,
NVME_FEAT_NOT_NS_SPEC = 0x010f,
NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
+ NVME_NS_ALREADY_ATTACHED = 0x0118,
+ NVME_NS_NOT_ATTACHED = 0x011A,
+ NVME_NS_CTRL_LIST_INVALID = 0x011C,
NVME_CONFLICTING_ATTRS = 0x0180,
NVME_INVALID_PROT_INFO = 0x0181,
NVME_WRITE_TO_RO = 0x0182,
@@ -951,6 +955,7 @@ typedef struct QEMU_PACKED NvmePSD {
uint8_t resv[16];
} NvmePSD;
+#define NVME_CONTROLLER_LIST_SIZE 2048
#define NVME_IDENTIFY_DATA_SIZE 4096
enum NvmeIdCns {
@@ -1045,6 +1050,7 @@ enum NvmeIdCtrlOacs {
NVME_OACS_SECURITY = 1 << 0,
NVME_OACS_FORMAT = 1 << 1,
NVME_OACS_FW = 1 << 2,
+ NVME_OACS_NS_MGMT = 1 << 3,
};
enum NvmeIdCtrlOncs {
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
` (5 preceding siblings ...)
2021-02-28 16:10 ` [PATCH V3 6/8] hw/block/nvme: support namespace attachment command Minwoo Im
@ 2021-02-28 16:10 ` Minwoo Im
2021-03-01 5:56 ` Klaus Jensen
2021-02-28 16:11 ` [PATCH V3 8/8] hw/block/nvme: support Identify NS Attached Controller List Minwoo Im
7 siblings, 1 reply; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:10 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Keith Busch, Klaus Jensen, Minwoo Im, Kevin Wolf, Max Reitz
If namespace inventory is changed due to some reasons (e.g., namespace
attachment/detachment), controller can send out event notifier to the
host to manage namespaces.
This patch sends out the AEN to the host after either attach or detach
namespaces from controllers. To support clear of the event from the
controller, this patch also implemented Get Log Page command for Changed
Namespace List log type. To return namespace id list through the
command, when namespace inventory is updated, id is added to the
per-controller list (changed_ns_list).
To indicate the support of this async event, this patch set
OAES(Optional Asynchronous Events Supported) in Identify Controller data
structure.
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
hw/block/nvme.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
hw/block/nvme.h | 7 +++++++
include/block/nvme.h | 7 +++++++
3 files changed, 58 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 68c2e63d9412..fc06f806e58e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2980,6 +2980,32 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
DMA_DIRECTION_FROM_DEVICE, req);
}
+static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
+ uint64_t off, NvmeRequest *req)
+{
+ uint32_t nslist[1024];
+ uint32_t trans_len;
+ NvmeChangedNs *ns, *next;
+ int i = 0;
+
+ memset(nslist, 0x0, sizeof(nslist));
+ trans_len = MIN(sizeof(nslist) - off, buf_len);
+
+ QTAILQ_FOREACH_SAFE(ns, &n->changed_ns_list, entry, next) {
+ nslist[i++] = ns->nsid;
+
+ QTAILQ_REMOVE(&n->changed_ns_list, ns, entry);
+ g_free(ns);
+ }
+
+ if (!rae) {
+ nvme_clear_events(n, NVME_AER_TYPE_NOTICE);
+ }
+
+ return nvme_dma(n, ((uint8_t *)nslist) + off, trans_len,
+ DMA_DIRECTION_FROM_DEVICE, req);
+}
+
static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
uint64_t off, NvmeRequest *req)
{
@@ -3064,6 +3090,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
return nvme_smart_info(n, rae, len, off, req);
case NVME_LOG_FW_SLOT_INFO:
return nvme_fw_log_info(n, len, off, req);
+ case NVME_LOG_CHANGED_NSLIST:
+ return nvme_changed_nslist(n, rae, len, off, req);
case NVME_LOG_CMD_EFFECTS:
return nvme_cmd_effects(n, csi, len, off, req);
default:
@@ -3882,6 +3910,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
uint16_t *ids = &list[1];
uint16_t ret;
int i;
+ NvmeChangedNs *changed_nsid;
trace_pci_nvme_ns_attachment(nvme_cid(req), dw10 & 0xf);
@@ -3920,6 +3949,18 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
nvme_ns_detach(ctrl, ns);
}
+
+ /*
+ * Add namespace id to the changed namespace id list for event clearing
+ * via Get Log Page command.
+ */
+ changed_nsid = g_new(NvmeChangedNs, 1);
+ changed_nsid->nsid = nsid;
+ QTAILQ_INSERT_TAIL(&ctrl->changed_ns_list, changed_nsid, entry);
+
+ nvme_enqueue_event(ctrl, NVME_AER_TYPE_NOTICE,
+ NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED,
+ NVME_LOG_CHANGED_NSLIST);
}
return NVME_SUCCESS;
@@ -4714,6 +4755,7 @@ static void nvme_init_state(NvmeCtrl *n)
n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING;
n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
+ QTAILQ_INIT(&n->changed_ns_list);
}
static int nvme_attach_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
@@ -4910,6 +4952,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->cntlid = cpu_to_le16(n->cntlid);
+ id->oaes = cpu_to_le32(NVME_OAES_NS_ATTR);
+
id->rab = 6;
if (n->params.use_intel_id) {
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 74a00ab21a55..d5eaea003ea5 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -132,6 +132,11 @@ typedef struct NvmeFeatureVal {
uint32_t async_config;
} NvmeFeatureVal;
+typedef struct NvmeChangedNs {
+ uint32_t nsid;
+ QTAILQ_ENTRY(NvmeChangedNs) entry;
+} NvmeChangedNs;
+
typedef struct NvmeCtrl {
PCIDevice parent_obj;
MemoryRegion bar0;
@@ -177,6 +182,8 @@ typedef struct NvmeCtrl {
QTAILQ_HEAD(, NvmeAsyncEvent) aer_queue;
int aer_queued;
+ QTAILQ_HEAD(, NvmeChangedNs) changed_ns_list; /* Changed NS list log */
+
NvmeSubsystem *subsys;
NvmeNamespace namespace;
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 339784d9c23a..eb0b31e949c2 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -760,6 +760,7 @@ typedef struct QEMU_PACKED NvmeCopySourceRange {
enum NvmeAsyncEventRequest {
NVME_AER_TYPE_ERROR = 0,
NVME_AER_TYPE_SMART = 1,
+ NVME_AER_TYPE_NOTICE = 2,
NVME_AER_TYPE_IO_SPECIFIC = 6,
NVME_AER_TYPE_VENDOR_SPECIFIC = 7,
NVME_AER_INFO_ERR_INVALID_DB_REGISTER = 0,
@@ -771,6 +772,7 @@ enum NvmeAsyncEventRequest {
NVME_AER_INFO_SMART_RELIABILITY = 0,
NVME_AER_INFO_SMART_TEMP_THRESH = 1,
NVME_AER_INFO_SMART_SPARE_THRESH = 2,
+ NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED = 0,
};
typedef struct QEMU_PACKED NvmeAerResult {
@@ -940,6 +942,7 @@ enum NvmeLogIdentifier {
NVME_LOG_ERROR_INFO = 0x01,
NVME_LOG_SMART_INFO = 0x02,
NVME_LOG_FW_SLOT_INFO = 0x03,
+ NVME_LOG_CHANGED_NSLIST = 0x04,
NVME_LOG_CMD_EFFECTS = 0x05,
};
@@ -1046,6 +1049,10 @@ typedef struct NvmeIdCtrlZoned {
uint8_t rsvd1[4095];
} NvmeIdCtrlZoned;
+enum NvmeIdCtrlOaes {
+ NVME_OAES_NS_ATTR = 1 << 8,
+};
+
enum NvmeIdCtrlOacs {
NVME_OACS_SECURITY = 1 << 0,
NVME_OACS_FORMAT = 1 << 1,
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V3 8/8] hw/block/nvme: support Identify NS Attached Controller List
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
` (6 preceding siblings ...)
2021-02-28 16:10 ` [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event Minwoo Im
@ 2021-02-28 16:11 ` Minwoo Im
7 siblings, 0 replies; 14+ messages in thread
From: Minwoo Im @ 2021-02-28 16:11 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Kevin Wolf, Klaus Jensen, Max Reitz, Klaus Jensen, Minwoo Im,
Keith Busch
Support Identify command for Namespace attached controller list. This
command handler will traverse the controller instances in the given
subsystem to figure out whether the specified nsid is attached to the
controllers or not.
The 4096bytes Identify data will return with the first entry (16bits)
indicating the number of the controller id entries. So, the data can
hold up to 2047 entries for the controller ids.
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
Tested-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme.c | 42 ++++++++++++++++++++++++++++++++++++++++++
hw/block/trace-events | 1 +
include/block/nvme.h | 1 +
3 files changed, 44 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc06f806e58e..202fc94d0bb2 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3286,6 +3286,46 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeRequest *req, bool active)
return NVME_INVALID_CMD_SET | NVME_DNR;
}
+static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req)
+{
+ NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
+ uint16_t min_id = le16_to_cpu(c->ctrlid);
+ uint16_t list[NVME_CONTROLLER_LIST_SIZE] = {};
+ uint16_t *ids = &list[1];
+ NvmeNamespace *ns;
+ NvmeCtrl *ctrl;
+ int cntlid, nr_ids = 0;
+
+ trace_pci_nvme_identify_ns_attached_list(min_id);
+
+ if (c->nsid == NVME_NSID_BROADCAST) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ ns = nvme_subsys_ns(n->subsys, c->nsid);
+ if (!ns) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ for (cntlid = min_id; cntlid < ARRAY_SIZE(n->subsys->ctrls); cntlid++) {
+ ctrl = nvme_subsys_ctrl(n->subsys, cntlid);
+ if (!ctrl) {
+ continue;
+ }
+
+ if (!nvme_ns_is_attached(ctrl, ns)) {
+ continue;
+ }
+
+ ids[nr_ids++] = cntlid;
+ }
+
+ list[0] = nr_ids;
+
+ return nvme_dma(n, (uint8_t *)list, sizeof(list),
+ DMA_DIRECTION_FROM_DEVICE, req);
+}
+
static uint16_t nvme_identify_ns_csi(NvmeCtrl *n, NvmeRequest *req,
bool active)
{
@@ -3485,6 +3525,8 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
return nvme_identify_ns(n, req, true);
case NVME_ID_CNS_NS_PRESENT:
return nvme_identify_ns(n, req, false);
+ case NVME_ID_CNS_NS_ATTACHED_CTRL_LIST:
+ return nvme_identify_ns_attached_list(n, req);
case NVME_ID_CNS_CS_NS:
return nvme_identify_ns_csi(n, req, true);
case NVME_ID_CNS_CS_NS_PRESENT:
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 98d542c999e2..2628d69c7879 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -64,6 +64,7 @@ pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
pci_nvme_identify_ctrl(void) "identify controller"
pci_nvme_identify_ctrl_csi(uint8_t csi) "identify controller, csi=0x%"PRIx8""
pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
+pci_nvme_identify_ns_attached_list(uint16_t cntid) "cntid=%"PRIu16""
pci_nvme_identify_ns_csi(uint32_t ns, uint8_t csi) "nsid=%"PRIu32", csi=0x%"PRIx8""
pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
pci_nvme_identify_nslist_csi(uint16_t ns, uint8_t csi) "nsid=%"PRIu16", csi=0x%"PRIx8""
diff --git a/include/block/nvme.h b/include/block/nvme.h
index eb0b31e949c2..b18945913927 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -971,6 +971,7 @@ enum NvmeIdCns {
NVME_ID_CNS_CS_NS_ACTIVE_LIST = 0x07,
NVME_ID_CNS_NS_PRESENT_LIST = 0x10,
NVME_ID_CNS_NS_PRESENT = 0x11,
+ NVME_ID_CNS_NS_ATTACHED_CTRL_LIST = 0x12,
NVME_ID_CNS_CS_NS_PRESENT_LIST = 0x1a,
NVME_ID_CNS_CS_NS_PRESENT = 0x1b,
NVME_ID_CNS_IO_COMMAND_SET = 0x1c,
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event
2021-02-28 16:10 ` [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event Minwoo Im
@ 2021-03-01 5:56 ` Klaus Jensen
2021-03-02 9:26 ` Minwoo Im
0 siblings, 1 reply; 14+ messages in thread
From: Klaus Jensen @ 2021-03-01 5:56 UTC (permalink / raw)
To: Minwoo Im; +Cc: Keith Busch, Kevin Wolf, qemu-devel, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 7023 bytes --]
On Mar 1 01:10, Minwoo Im wrote:
> If namespace inventory is changed due to some reasons (e.g., namespace
> attachment/detachment), controller can send out event notifier to the
> host to manage namespaces.
>
> This patch sends out the AEN to the host after either attach or detach
> namespaces from controllers. To support clear of the event from the
> controller, this patch also implemented Get Log Page command for Changed
> Namespace List log type. To return namespace id list through the
> command, when namespace inventory is updated, id is added to the
> per-controller list (changed_ns_list).
>
> To indicate the support of this async event, this patch set
> OAES(Optional Asynchronous Events Supported) in Identify Controller data
> structure.
>
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
> hw/block/nvme.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> hw/block/nvme.h | 7 +++++++
> include/block/nvme.h | 7 +++++++
> 3 files changed, 58 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 68c2e63d9412..fc06f806e58e 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -2980,6 +2980,32 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> DMA_DIRECTION_FROM_DEVICE, req);
> }
>
> +static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> + uint64_t off, NvmeRequest *req)
> +{
> + uint32_t nslist[1024];
> + uint32_t trans_len;
> + NvmeChangedNs *ns, *next;
> + int i = 0;
> +
> + memset(nslist, 0x0, sizeof(nslist));
> + trans_len = MIN(sizeof(nslist) - off, buf_len);
> +
> + QTAILQ_FOREACH_SAFE(ns, &n->changed_ns_list, entry, next) {
> + nslist[i++] = ns->nsid;
> +
> + QTAILQ_REMOVE(&n->changed_ns_list, ns, entry);
> + g_free(ns);
> + }
> +
> + if (!rae) {
> + nvme_clear_events(n, NVME_AER_TYPE_NOTICE);
> + }
> +
> + return nvme_dma(n, ((uint8_t *)nslist) + off, trans_len,
> + DMA_DIRECTION_FROM_DEVICE, req);
> +}
> +
> static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
> uint64_t off, NvmeRequest *req)
> {
> @@ -3064,6 +3090,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
> return nvme_smart_info(n, rae, len, off, req);
> case NVME_LOG_FW_SLOT_INFO:
> return nvme_fw_log_info(n, len, off, req);
> + case NVME_LOG_CHANGED_NSLIST:
> + return nvme_changed_nslist(n, rae, len, off, req);
> case NVME_LOG_CMD_EFFECTS:
> return nvme_cmd_effects(n, csi, len, off, req);
> default:
> @@ -3882,6 +3910,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
> uint16_t *ids = &list[1];
> uint16_t ret;
> int i;
> + NvmeChangedNs *changed_nsid;
>
> trace_pci_nvme_ns_attachment(nvme_cid(req), dw10 & 0xf);
>
> @@ -3920,6 +3949,18 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>
> nvme_ns_detach(ctrl, ns);
> }
> +
> + /*
> + * Add namespace id to the changed namespace id list for event clearing
> + * via Get Log Page command.
> + */
> + changed_nsid = g_new(NvmeChangedNs, 1);
> + changed_nsid->nsid = nsid;
> + QTAILQ_INSERT_TAIL(&ctrl->changed_ns_list, changed_nsid, entry);
> +
> + nvme_enqueue_event(ctrl, NVME_AER_TYPE_NOTICE,
> + NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED,
> + NVME_LOG_CHANGED_NSLIST);
> }
If one just keeps attaching/detaching we end up with more than 1024
entries here and go out of bounds in nvme_changed_nslist.
How about having the QTAILQ_ENTRY directly on the NvmeNamespace struct
and use QTAILQ_IN_USE to check if the namespace is already in the list?
>
> return NVME_SUCCESS;
> @@ -4714,6 +4755,7 @@ static void nvme_init_state(NvmeCtrl *n)
> n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING;
> n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
> + QTAILQ_INIT(&n->changed_ns_list);
> }
>
> static int nvme_attach_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> @@ -4910,6 +4952,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>
> id->cntlid = cpu_to_le16(n->cntlid);
>
> + id->oaes = cpu_to_le32(NVME_OAES_NS_ATTR);
> +
> id->rab = 6;
>
> if (n->params.use_intel_id) {
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 74a00ab21a55..d5eaea003ea5 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -132,6 +132,11 @@ typedef struct NvmeFeatureVal {
> uint32_t async_config;
> } NvmeFeatureVal;
>
> +typedef struct NvmeChangedNs {
> + uint32_t nsid;
> + QTAILQ_ENTRY(NvmeChangedNs) entry;
> +} NvmeChangedNs;
> +
> typedef struct NvmeCtrl {
> PCIDevice parent_obj;
> MemoryRegion bar0;
> @@ -177,6 +182,8 @@ typedef struct NvmeCtrl {
> QTAILQ_HEAD(, NvmeAsyncEvent) aer_queue;
> int aer_queued;
>
> + QTAILQ_HEAD(, NvmeChangedNs) changed_ns_list; /* Changed NS list log */
> +
> NvmeSubsystem *subsys;
>
> NvmeNamespace namespace;
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 339784d9c23a..eb0b31e949c2 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -760,6 +760,7 @@ typedef struct QEMU_PACKED NvmeCopySourceRange {
> enum NvmeAsyncEventRequest {
> NVME_AER_TYPE_ERROR = 0,
> NVME_AER_TYPE_SMART = 1,
> + NVME_AER_TYPE_NOTICE = 2,
> NVME_AER_TYPE_IO_SPECIFIC = 6,
> NVME_AER_TYPE_VENDOR_SPECIFIC = 7,
> NVME_AER_INFO_ERR_INVALID_DB_REGISTER = 0,
> @@ -771,6 +772,7 @@ enum NvmeAsyncEventRequest {
> NVME_AER_INFO_SMART_RELIABILITY = 0,
> NVME_AER_INFO_SMART_TEMP_THRESH = 1,
> NVME_AER_INFO_SMART_SPARE_THRESH = 2,
> + NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED = 0,
> };
>
> typedef struct QEMU_PACKED NvmeAerResult {
> @@ -940,6 +942,7 @@ enum NvmeLogIdentifier {
> NVME_LOG_ERROR_INFO = 0x01,
> NVME_LOG_SMART_INFO = 0x02,
> NVME_LOG_FW_SLOT_INFO = 0x03,
> + NVME_LOG_CHANGED_NSLIST = 0x04,
> NVME_LOG_CMD_EFFECTS = 0x05,
> };
>
> @@ -1046,6 +1049,10 @@ typedef struct NvmeIdCtrlZoned {
> uint8_t rsvd1[4095];
> } NvmeIdCtrlZoned;
>
> +enum NvmeIdCtrlOaes {
> + NVME_OAES_NS_ATTR = 1 << 8,
> +};
> +
> enum NvmeIdCtrlOacs {
> NVME_OACS_SECURITY = 1 << 0,
> NVME_OACS_FORMAT = 1 << 1,
> --
> 2.25.1
>
--
One of us - No more doubt, silence or taboo about mental illness.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event
2021-03-01 5:56 ` Klaus Jensen
@ 2021-03-02 9:26 ` Minwoo Im
2021-03-02 9:28 ` Klaus Jensen
0 siblings, 1 reply; 14+ messages in thread
From: Minwoo Im @ 2021-03-02 9:26 UTC (permalink / raw)
To: Klaus Jensen; +Cc: Keith Busch, Kevin Wolf, qemu-devel, qemu-block, Max Reitz
On 21-03-01 06:56:02, Klaus Jensen wrote:
> On Mar 1 01:10, Minwoo Im wrote:
> > If namespace inventory is changed due to some reasons (e.g., namespace
> > attachment/detachment), controller can send out event notifier to the
> > host to manage namespaces.
> >
> > This patch sends out the AEN to the host after either attach or detach
> > namespaces from controllers. To support clear of the event from the
> > controller, this patch also implemented Get Log Page command for Changed
> > Namespace List log type. To return namespace id list through the
> > command, when namespace inventory is updated, id is added to the
> > per-controller list (changed_ns_list).
> >
> > To indicate the support of this async event, this patch set
> > OAES(Optional Asynchronous Events Supported) in Identify Controller data
> > structure.
> >
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> > hw/block/nvme.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > hw/block/nvme.h | 7 +++++++
> > include/block/nvme.h | 7 +++++++
> > 3 files changed, 58 insertions(+)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 68c2e63d9412..fc06f806e58e 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -2980,6 +2980,32 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> > DMA_DIRECTION_FROM_DEVICE, req);
> > }
> >
> > +static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> > + uint64_t off, NvmeRequest *req)
> > +{
> > + uint32_t nslist[1024];
> > + uint32_t trans_len;
> > + NvmeChangedNs *ns, *next;
> > + int i = 0;
> > +
> > + memset(nslist, 0x0, sizeof(nslist));
> > + trans_len = MIN(sizeof(nslist) - off, buf_len);
> > +
> > + QTAILQ_FOREACH_SAFE(ns, &n->changed_ns_list, entry, next) {
> > + nslist[i++] = ns->nsid;
> > +
> > + QTAILQ_REMOVE(&n->changed_ns_list, ns, entry);
> > + g_free(ns);
> > + }
> > +
> > + if (!rae) {
> > + nvme_clear_events(n, NVME_AER_TYPE_NOTICE);
> > + }
> > +
> > + return nvme_dma(n, ((uint8_t *)nslist) + off, trans_len,
> > + DMA_DIRECTION_FROM_DEVICE, req);
> > +}
> > +
> > static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
> > uint64_t off, NvmeRequest *req)
> > {
> > @@ -3064,6 +3090,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
> > return nvme_smart_info(n, rae, len, off, req);
> > case NVME_LOG_FW_SLOT_INFO:
> > return nvme_fw_log_info(n, len, off, req);
> > + case NVME_LOG_CHANGED_NSLIST:
> > + return nvme_changed_nslist(n, rae, len, off, req);
> > case NVME_LOG_CMD_EFFECTS:
> > return nvme_cmd_effects(n, csi, len, off, req);
> > default:
> > @@ -3882,6 +3910,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
> > uint16_t *ids = &list[1];
> > uint16_t ret;
> > int i;
> > + NvmeChangedNs *changed_nsid;
> >
> > trace_pci_nvme_ns_attachment(nvme_cid(req), dw10 & 0xf);
> >
> > @@ -3920,6 +3949,18 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
> >
> > nvme_ns_detach(ctrl, ns);
> > }
> > +
> > + /*
> > + * Add namespace id to the changed namespace id list for event clearing
> > + * via Get Log Page command.
> > + */
> > + changed_nsid = g_new(NvmeChangedNs, 1);
> > + changed_nsid->nsid = nsid;
> > + QTAILQ_INSERT_TAIL(&ctrl->changed_ns_list, changed_nsid, entry);
> > +
> > + nvme_enqueue_event(ctrl, NVME_AER_TYPE_NOTICE,
> > + NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED,
> > + NVME_LOG_CHANGED_NSLIST);
> > }
>
> If one just keeps attaching/detaching we end up with more than 1024
> entries here and go out of bounds in nvme_changed_nslist.
>
> How about having the QTAILQ_ENTRY directly on the NvmeNamespace struct
> and use QTAILQ_IN_USE to check if the namespace is already in the list?
QTAILQ_IN_USE might be tough to represent relationship between
controller and namespace itself. So, I will work on this with standard
bitmap rather than the list. I think bitmap will be easier to represent
the relationship.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event
2021-03-02 9:26 ` Minwoo Im
@ 2021-03-02 9:28 ` Klaus Jensen
0 siblings, 0 replies; 14+ messages in thread
From: Klaus Jensen @ 2021-03-02 9:28 UTC (permalink / raw)
To: Minwoo Im; +Cc: Keith Busch, Kevin Wolf, qemu-devel, qemu-block, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 4746 bytes --]
On Mar 2 18:26, Minwoo Im wrote:
> On 21-03-01 06:56:02, Klaus Jensen wrote:
> > On Mar 1 01:10, Minwoo Im wrote:
> > > If namespace inventory is changed due to some reasons (e.g., namespace
> > > attachment/detachment), controller can send out event notifier to the
> > > host to manage namespaces.
> > >
> > > This patch sends out the AEN to the host after either attach or detach
> > > namespaces from controllers. To support clear of the event from the
> > > controller, this patch also implemented Get Log Page command for Changed
> > > Namespace List log type. To return namespace id list through the
> > > command, when namespace inventory is updated, id is added to the
> > > per-controller list (changed_ns_list).
> > >
> > > To indicate the support of this async event, this patch set
> > > OAES(Optional Asynchronous Events Supported) in Identify Controller data
> > > structure.
> > >
> > > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > > ---
> > > hw/block/nvme.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > hw/block/nvme.h | 7 +++++++
> > > include/block/nvme.h | 7 +++++++
> > > 3 files changed, 58 insertions(+)
> > >
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 68c2e63d9412..fc06f806e58e 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -2980,6 +2980,32 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> > > DMA_DIRECTION_FROM_DEVICE, req);
> > > }
> > >
> > > +static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
> > > + uint64_t off, NvmeRequest *req)
> > > +{
> > > + uint32_t nslist[1024];
> > > + uint32_t trans_len;
> > > + NvmeChangedNs *ns, *next;
> > > + int i = 0;
> > > +
> > > + memset(nslist, 0x0, sizeof(nslist));
> > > + trans_len = MIN(sizeof(nslist) - off, buf_len);
> > > +
> > > + QTAILQ_FOREACH_SAFE(ns, &n->changed_ns_list, entry, next) {
> > > + nslist[i++] = ns->nsid;
> > > +
> > > + QTAILQ_REMOVE(&n->changed_ns_list, ns, entry);
> > > + g_free(ns);
> > > + }
> > > +
> > > + if (!rae) {
> > > + nvme_clear_events(n, NVME_AER_TYPE_NOTICE);
> > > + }
> > > +
> > > + return nvme_dma(n, ((uint8_t *)nslist) + off, trans_len,
> > > + DMA_DIRECTION_FROM_DEVICE, req);
> > > +}
> > > +
> > > static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
> > > uint64_t off, NvmeRequest *req)
> > > {
> > > @@ -3064,6 +3090,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
> > > return nvme_smart_info(n, rae, len, off, req);
> > > case NVME_LOG_FW_SLOT_INFO:
> > > return nvme_fw_log_info(n, len, off, req);
> > > + case NVME_LOG_CHANGED_NSLIST:
> > > + return nvme_changed_nslist(n, rae, len, off, req);
> > > case NVME_LOG_CMD_EFFECTS:
> > > return nvme_cmd_effects(n, csi, len, off, req);
> > > default:
> > > @@ -3882,6 +3910,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
> > > uint16_t *ids = &list[1];
> > > uint16_t ret;
> > > int i;
> > > + NvmeChangedNs *changed_nsid;
> > >
> > > trace_pci_nvme_ns_attachment(nvme_cid(req), dw10 & 0xf);
> > >
> > > @@ -3920,6 +3949,18 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
> > >
> > > nvme_ns_detach(ctrl, ns);
> > > }
> > > +
> > > + /*
> > > + * Add namespace id to the changed namespace id list for event clearing
> > > + * via Get Log Page command.
> > > + */
> > > + changed_nsid = g_new(NvmeChangedNs, 1);
> > > + changed_nsid->nsid = nsid;
> > > + QTAILQ_INSERT_TAIL(&ctrl->changed_ns_list, changed_nsid, entry);
> > > +
> > > + nvme_enqueue_event(ctrl, NVME_AER_TYPE_NOTICE,
> > > + NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED,
> > > + NVME_LOG_CHANGED_NSLIST);
> > > }
> >
> > If one just keeps attaching/detaching we end up with more than 1024
> > entries here and go out of bounds in nvme_changed_nslist.
> >
> > How about having the QTAILQ_ENTRY directly on the NvmeNamespace struct
> > and use QTAILQ_IN_USE to check if the namespace is already in the list?
>
> QTAILQ_IN_USE might be tough to represent relationship between
> controller and namespace itself. So, I will work on this with standard
> bitmap rather than the list. I think bitmap will be easier to represent
> the relationship.
OK, sounds reasonable!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 1/8] hw/block/nvme: support namespace detach
2021-02-28 16:10 ` [PATCH V3 1/8] hw/block/nvme: support namespace detach Minwoo Im
@ 2021-03-15 4:56 ` Keqian Zhu
2021-03-16 21:46 ` Klaus Jensen
0 siblings, 1 reply; 14+ messages in thread
From: Keqian Zhu @ 2021-03-15 4:56 UTC (permalink / raw)
To: Minwoo Im, qemu-devel, qemu-block
Cc: Kevin Wolf, Klaus Jensen, Keith Busch, Klaus Jensen, Max Reitz
Hi,
I don't dig into code logic, just some nit below.
On 2021/3/1 0:10, Minwoo Im wrote:
> Given that now we have nvme-subsys device supported, we can manage
> namespace allocated, but not attached: detached. This patch introduced
s/introduced/introduces
> a parameter for nvme-ns device named 'detached'. This parameter
> indicates whether the given namespace device is detached from
> a entire NVMe subsystem('subsys' given case, shared namespace) or a
> controller('bus' given case, private namespace).
>
> - Allocated namespace
>
> 1) Shared ns in the subsystem 'subsys0':
>
> -device nvme-ns,id=ns1,drive=blknvme0,nsid=1,subsys=subsys0,detached=true
>
> 2) Private ns for the controller 'nvme0' of the subsystem 'subsys0':
>
> -device nvme-subsys,id=subsys0
> -device nvme,serial=foo,id=nvme0,subsys=subsys0
> -device nvme-ns,id=ns1,drive=blknvme0,nsid=1,bus=nvme0,detached=true
>
> 3) (Invalid case) Controller 'nvme0' has no subsystem to manage ns:
>
> -device nvme,serial=foo,id=nvme0
> -device nvme-ns,id=ns1,drive=blknvme0,nsid=1,bus=nvme0,detached=true
>
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> Tested-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/block/nvme-ns.c | 1 +
> hw/block/nvme-ns.h | 1 +
> hw/block/nvme-subsys.h | 1 +
> hw/block/nvme.c | 41 +++++++++++++++++++++++++++++++++++++++--
> hw/block/nvme.h | 22 ++++++++++++++++++++++
> 5 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> index 0e8760020483..eda6a0c003a4 100644
> --- a/hw/block/nvme-ns.c
> +++ b/hw/block/nvme-ns.c
> @@ -399,6 +399,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_UINT16("mssrl", NvmeNamespace, params.mssrl, 128),
> diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> index 7af6884862b5..b0c00e115d81 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;
>
> diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
> index ccf6a71398d3..890d118117dc 100644
> --- a/hw/block/nvme-subsys.h
> +++ b/hw/block/nvme-subsys.h
> @@ -23,6 +23,7 @@ typedef struct NvmeSubsystem {
> uint8_t subnqn[256];
>
> NvmeCtrl *ctrls[NVME_SUBSYS_MAX_CTRLS];
> + /* Allocated namespaces for this subsystem */
> NvmeNamespace *namespaces[NVME_SUBSYS_MAX_NAMESPACES];
> } NvmeSubsystem;
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index edd0b85c10ce..f6aeae081840 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -23,7 +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> \
> + * subsys=<subsys_id>,detached=<true|false[optional]>
> * -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
> * zoned=<true|false[optional]>, \
> * subsys=<subsys_id>
> @@ -82,6 +82,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 attahced to all
s/attahced/attached
> + * controllers in the subsystem by default.
> + * It's mutual exclusive with 'bus' parameter. 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:
> @@ -4613,6 +4620,20 @@ 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);
> @@ -4644,7 +4665,23 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
>
> trace_pci_nvme_register_namespace(nsid);
>
> - n->namespaces[nsid - 1] = ns;
> + /*
> + * If subsys is not given, namespae is always attached to the controller
s/namespae/namespace
> + * 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);
> + }
> + }
>
> return 0;
> }
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index f45ace0cff5b..51b8739b4d1e 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -174,6 +174,10 @@ typedef struct NvmeCtrl {
> NvmeSubsystem *subsys;
>
> NvmeNamespace namespace;
> + /*
> + * Attached namespaces to this controller. If subsys is not given, all
> + * namespaces in this list will always be attached.
> + */
> NvmeNamespace *namespaces[NVME_MAX_NAMESPACES];
> NvmeSQueue **sq;
> NvmeCQueue **cq;
> @@ -192,6 +196,24 @@ 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)
> +{
> + 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)
> +{
> + n->namespaces[nvme_nsid(ns) - 1] = ns;
> +}
> +
> static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
> {
> NvmeSQueue *sq = req->sq;
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 1/8] hw/block/nvme: support namespace detach
2021-03-15 4:56 ` Keqian Zhu
@ 2021-03-16 21:46 ` Klaus Jensen
0 siblings, 0 replies; 14+ messages in thread
From: Klaus Jensen @ 2021-03-16 21:46 UTC (permalink / raw)
To: Keqian Zhu
Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz,
Minwoo Im, Keith Busch
[-- Attachment #1: Type: text/plain, Size: 176 bytes --]
On Mar 15 12:56, Keqian Zhu wrote:
> Hi,
>
> I don't dig into code logic, just some nit below.
>
Thanks! I'll cook up a fix with some corrections on these typos.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-03-16 21:53 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-28 16:10 [PATCH V3 0/8] hw/block/nvme: support namespace attachment Minwoo Im
2021-02-28 16:10 ` [PATCH V3 1/8] hw/block/nvme: support namespace detach Minwoo Im
2021-03-15 4:56 ` Keqian Zhu
2021-03-16 21:46 ` Klaus Jensen
2021-02-28 16:10 ` [PATCH V3 2/8] hw/block/nvme: fix namespaces array to 1-based Minwoo Im
2021-02-28 16:10 ` [PATCH V3 3/8] hw/block/nvme: fix allocated namespace list to 256 Minwoo Im
2021-02-28 16:10 ` [PATCH V3 4/8] hw/block/nvme: support allocated namespace type Minwoo Im
2021-02-28 16:10 ` [PATCH V3 5/8] hw/block/nvme: refactor nvme_select_ns_iocs Minwoo Im
2021-02-28 16:10 ` [PATCH V3 6/8] hw/block/nvme: support namespace attachment command Minwoo Im
2021-02-28 16:10 ` [PATCH V3 7/8] hw/block/nvme: support changed namespace asyncrohous event Minwoo Im
2021-03-01 5:56 ` Klaus Jensen
2021-03-02 9:26 ` Minwoo Im
2021-03-02 9:28 ` Klaus Jensen
2021-02-28 16:11 ` [PATCH V3 8/8] hw/block/nvme: support Identify NS Attached Controller List Minwoo Im
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).