[PATCH 0/3] adding ctrl list (cns 0x13) support and random fixes

[PATCH 0/3] adding ctrl list (cns 0x13) support and random fixes

[Gollu Appalanaidu]

This series will add the Identify Controller List (CNS 0x13) support
and NSID endian conversion fixes for CNS 0x12 and CNS 0x13.

Documentation fix for the '-detached' parameter.

Gollu Appalanaidu (3):
  hw/nvme/ctrl: add controller list cns 0x13
  hw/nvme/ctrl: fix endian conversion for nsid in ctrl list
  hw/nvme/ctrl: documentation fix

 hw/nvme/ctrl.c       | 28 +++++++++++++++++-----------
 hw/nvme/trace-events |  2 +-
 include/block/nvme.h |  1 +
 3 files changed, 19 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/3] hw/nvme/ctrl: add controller list cns 0x13

[Gollu Appalanaidu]

Add the controller identifiers list available in NVM Subsystem
that may or may not be attached to namespaces.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/nvme/ctrl.c       | 25 +++++++++++++++----------
 hw/nvme/trace-events |  2 +-
 include/block/nvme.h |  1 +
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2e7498a73e..d08a3350e2 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4251,7 +4251,8 @@ 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)
+static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
+                                        bool attached)
 {
     NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
     uint16_t min_id = le16_to_cpu(c->ctrlid);
@@ -4261,15 +4262,17 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req)
     NvmeCtrl *ctrl;
     int cntlid, nr_ids = 0;
 
-    trace_pci_nvme_identify_ns_attached_list(min_id);
+    trace_pci_nvme_identify_ctrl_list(c->cns, min_id);
 
-    if (c->nsid == NVME_NSID_BROADCAST) {
-        return NVME_INVALID_FIELD | NVME_DNR;
-    }
+    if (attached) {
+        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;
+        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++) {
@@ -4278,7 +4281,7 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req)
             continue;
         }
 
-        if (!nvme_ns(ctrl, c->nsid)) {
+        if (attached && !nvme_ns(ctrl, c->nsid)) {
             continue;
         }
 
@@ -4493,7 +4496,9 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
     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);
+        return nvme_identify_ctrl_list(n, req, true);
+    case NVME_ID_CNS_CTRL_LIST:
+        return nvme_identify_ctrl_list(n, req, false);
     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/nvme/trace-events b/hw/nvme/trace-events
index ea33d0ccc3..7ba3714671 100644
--- a/hw/nvme/trace-events
+++ b/hw/nvme/trace-events
@@ -55,7 +55,7 @@ pci_nvme_identify(uint16_t cid, uint8_t cns, uint16_t ctrlid, uint8_t csi) "cid
 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_ctrl_list(uint8_t cns, uint16_t cntid) "cns 0x%"PRIx8" 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 0ff9ce17a9..188ab460df 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -980,6 +980,7 @@ enum NvmeIdCns {
     NVME_ID_CNS_NS_PRESENT_LIST       = 0x10,
     NVME_ID_CNS_NS_PRESENT            = 0x11,
     NVME_ID_CNS_NS_ATTACHED_CTRL_LIST = 0x12,
+    NVME_ID_CNS_CTRL_LIST             = 0x13,
     NVME_ID_CNS_CS_NS_PRESENT_LIST    = 0x1a,
     NVME_ID_CNS_CS_NS_PRESENT         = 0x1b,
     NVME_ID_CNS_IO_COMMAND_SET        = 0x1c,
-- 
2.17.1

[PATCH 2/3] hw/nvme/ctrl: fix endian conversion for nsid in ctrl list

[Gollu Appalanaidu]

In Identify Ctrl List of the CNS 0x12 and 0x13 no endian conversion
for the nsid field.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/nvme/ctrl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index d08a3350e2..813a72c655 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4255,6 +4255,7 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
                                         bool attached)
 {
     NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
+    uint32_t nsid = le32_to_cpu(c->nsid);
     uint16_t min_id = le16_to_cpu(c->ctrlid);
     uint16_t list[NVME_CONTROLLER_LIST_SIZE] = {};
     uint16_t *ids = &list[1];
@@ -4265,11 +4266,11 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
     trace_pci_nvme_identify_ctrl_list(c->cns, min_id);
 
     if (attached) {
-        if (c->nsid == NVME_NSID_BROADCAST) {
+        if (nsid == NVME_NSID_BROADCAST) {
             return NVME_INVALID_FIELD | NVME_DNR;
         }
 
-        ns = nvme_subsys_ns(n->subsys, c->nsid);
+        ns = nvme_subsys_ns(n->subsys, nsid);
         if (!ns) {
             return NVME_INVALID_FIELD | NVME_DNR;
         }
@@ -4281,7 +4282,7 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
             continue;
         }
 
-        if (attached && !nvme_ns(ctrl, c->nsid)) {
+        if (attached && !nvme_ns(ctrl, nsid)) {
             continue;
         }
 
-- 
2.17.1

[PATCH 3/3] hw/nvme/ctrl: documentation fix

[Gollu Appalanaidu]

In the documentation of the '-detached' param "be" and "not" has been
used side by side, fix that.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 813a72c655..a3df26d0ce 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -114,7 +114,7 @@
  *   This parameter is only valid together with the `subsys` parameter. If left
  *   at the default value (`false/off`), the namespace will be attached to all
  *   controllers in the NVMe subsystem at boot-up. If set to `true/on`, the
- *   namespace will be be available in the subsystem not not attached to any
+ *   namespace will be available in the subsystem not attached to any
  *   controllers.
  *
  * Setting `zoned` to true selects Zoned Command Set at the namespace.
-- 
2.17.1

Re: [PATCH 1/3] hw/nvme/ctrl: add controller list cns 0x13

[Klaus Jensen]

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

On May 17 15:37, Gollu Appalanaidu wrote:
>Add the controller identifiers list available in NVM Subsystem
>that may or may not be attached to namespaces.
>
>Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>---
> hw/nvme/ctrl.c       | 25 +++++++++++++++----------
> hw/nvme/trace-events |  2 +-
> include/block/nvme.h |  1 +
> 3 files changed, 17 insertions(+), 11 deletions(-)
>
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 2e7498a73e..d08a3350e2 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -4251,7 +4251,8 @@ 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)
>+static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
>+                                        bool attached)
> {
>     NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
>     uint16_t min_id = le16_to_cpu(c->ctrlid);
>@@ -4261,15 +4262,17 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req)
>     NvmeCtrl *ctrl;
>     int cntlid, nr_ids = 0;
>
>-    trace_pci_nvme_identify_ns_attached_list(min_id);
>+    trace_pci_nvme_identify_ctrl_list(c->cns, min_id);
>
>-    if (c->nsid == NVME_NSID_BROADCAST) {
>-        return NVME_INVALID_FIELD | NVME_DNR;
>-    }
>+    if (attached) {
>+        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;
>+        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++) {
>@@ -4278,7 +4281,7 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req)
>             continue;
>         }
>
>-        if (!nvme_ns(ctrl, c->nsid)) {
>+        if (attached && !nvme_ns(ctrl, c->nsid)) {
>             continue;
>         }
>
>@@ -4493,7 +4496,9 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeRequest *req)
>     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);
>+        return nvme_identify_ctrl_list(n, req, true);
>+    case NVME_ID_CNS_CTRL_LIST:
>+        return nvme_identify_ctrl_list(n, req, false);
>     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/nvme/trace-events b/hw/nvme/trace-events
>index ea33d0ccc3..7ba3714671 100644
>--- a/hw/nvme/trace-events
>+++ b/hw/nvme/trace-events
>@@ -55,7 +55,7 @@ pci_nvme_identify(uint16_t cid, uint8_t cns, uint16_t ctrlid, uint8_t csi) "cid
> 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_ctrl_list(uint8_t cns, uint16_t cntid) "cns 0x%"PRIx8" cntid=%"PRIu16""

Inconsistency here. Please use "field value", not "field=value" for 
cntid.

> 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 0ff9ce17a9..188ab460df 100644
>--- a/include/block/nvme.h
>+++ b/include/block/nvme.h
>@@ -980,6 +980,7 @@ enum NvmeIdCns {
>     NVME_ID_CNS_NS_PRESENT_LIST       = 0x10,
>     NVME_ID_CNS_NS_PRESENT            = 0x11,
>     NVME_ID_CNS_NS_ATTACHED_CTRL_LIST = 0x12,
>+    NVME_ID_CNS_CTRL_LIST             = 0x13,
>     NVME_ID_CNS_CS_NS_PRESENT_LIST    = 0x1a,
>     NVME_ID_CNS_CS_NS_PRESENT         = 0x1b,
>     NVME_ID_CNS_IO_COMMAND_SET        = 0x1c,
>-- 
>2.17.1
>

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

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

Re: [PATCH 2/3] hw/nvme/ctrl: fix endian conversion for nsid in ctrl list

[Klaus Jensen]

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

On May 17 15:37, Gollu Appalanaidu wrote:
>In Identify Ctrl List of the CNS 0x12 and 0x13 no endian conversion
>for the nsid field.
>
>Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>---
> hw/nvme/ctrl.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index d08a3350e2..813a72c655 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -4255,6 +4255,7 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
>                                         bool attached)
> {
>     NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
>+    uint32_t nsid = le32_to_cpu(c->nsid);
>     uint16_t min_id = le16_to_cpu(c->ctrlid);
>     uint16_t list[NVME_CONTROLLER_LIST_SIZE] = {};
>     uint16_t *ids = &list[1];
>@@ -4265,11 +4266,11 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
>     trace_pci_nvme_identify_ctrl_list(c->cns, min_id);
>
>     if (attached) {
>-        if (c->nsid == NVME_NSID_BROADCAST) {
>+        if (nsid == NVME_NSID_BROADCAST) {
>             return NVME_INVALID_FIELD | NVME_DNR;
>         }
>
>-        ns = nvme_subsys_ns(n->subsys, c->nsid);
>+        ns = nvme_subsys_ns(n->subsys, nsid);
>         if (!ns) {
>             return NVME_INVALID_FIELD | NVME_DNR;
>         }
>@@ -4281,7 +4282,7 @@ static uint16_t nvme_identify_ctrl_list(NvmeCtrl *n, NvmeRequest *req,
>             continue;
>         }
>
>-        if (attached && !nvme_ns(ctrl, c->nsid)) {
>+        if (attached && !nvme_ns(ctrl, nsid)) {
>             continue;
>         }
>
>-- 
>2.17.1
>

I know that the endianness conversion was missing before your patch, but 
please squash this up into patch 1.

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