[PATCH v2 1/2] hw/nvme: fix endianess conversion and add controller list

[PATCH v2 1/2] hw/nvme: fix endianess conversion and add controller list

[Gollu Appalanaidu]

Add the controller identifiers list CNS 0x13, available list of ctrls
in NVM Subsystem that may or may not be attached to namespaces.

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>

-v2:
 Fix the review comments from Klaus and squashed 2nd commit into
 1st commit

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

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2e7498a73e..813a72c655 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4251,9 +4251,11 @@ 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;
+    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];
@@ -4261,15 +4263,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 (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, nsid);
+        if (!ns) {
+            return NVME_INVALID_FIELD | NVME_DNR;
+        }
     }
 
     for (cntlid = min_id; cntlid < ARRAY_SIZE(n->subsys->ctrls); cntlid++) {
@@ -4278,7 +4282,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, nsid)) {
             continue;
         }
 
@@ -4493,7 +4497,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..4976eb9bec 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 v2 2/2] hw/nvme: 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 v2 1/2] hw/nvme: fix endianess conversion and add controller list

[Klaus Jensen]

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

On Jun  1 20:32, Gollu Appalanaidu wrote:
>Add the controller identifiers list CNS 0x13, available list of ctrls
>in NVM Subsystem that may or may not be attached to namespaces.
>
>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>
>
>-v2:
> Fix the review comments from Klaus and squashed 2nd commit into
> 1st commit
>
>---
> hw/nvme/ctrl.c       | 26 ++++++++++++++++----------
> hw/nvme/trace-events |  2 +-
> include/block/nvme.h |  1 +
> 3 files changed, 18 insertions(+), 11 deletions(-)
>
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 2e7498a73e..813a72c655 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -4251,9 +4251,11 @@ 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;
>+    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];
>@@ -4261,15 +4263,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 (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, nsid);
>+        if (!ns) {
>+            return NVME_INVALID_FIELD | NVME_DNR;
>+        }
>     }
>
>     for (cntlid = min_id; cntlid < ARRAY_SIZE(n->subsys->ctrls); cntlid++) {

Assume that `attached` is false and `n->subsys` is NULL.

KABOOOOM :)

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

Re: [PATCH v2 2/2] hw/nvme: documentation fix

[Klaus Jensen]

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

On Jun  1 20:32, Gollu Appalanaidu wrote:
>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

namespace will be available in the subsystem *but* not attached to an

>  *   controllers.
>  *
>  * Setting `zoned` to true selects Zoned Command Set at the namespace.
>-- 
>2.17.1
>


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

Re: [PATCH v2 1/2] hw/nvme: fix endianess conversion and add controller list

[Gollu Appalanaidu]

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

On Wed, Jun 09, 2021 at 10:22:49PM +0200, Klaus Jensen wrote:
>On Jun  1 20:32, Gollu Appalanaidu wrote:
>>Add the controller identifiers list CNS 0x13, available list of ctrls
>>in NVM Subsystem that may or may not be attached to namespaces.
>>
>>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>
>>
>>-v2:
>>Fix the review comments from Klaus and squashed 2nd commit into
>>1st commit
>>
>>---
>>hw/nvme/ctrl.c       | 26 ++++++++++++++++----------
>>hw/nvme/trace-events |  2 +-
>>include/block/nvme.h |  1 +
>>3 files changed, 18 insertions(+), 11 deletions(-)
>>
>>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>>index 2e7498a73e..813a72c655 100644
>>--- a/hw/nvme/ctrl.c
>>+++ b/hw/nvme/ctrl.c
>>@@ -4251,9 +4251,11 @@ 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;
>>+    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];
>>@@ -4261,15 +4263,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 (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, nsid);
>>+        if (!ns) {
>>+            return NVME_INVALID_FIELD | NVME_DNR;
>>+        }
>>    }
>>
>>    for (cntlid = min_id; cntlid < ARRAY_SIZE(n->subsys->ctrls); cntlid++) {
>
>Assume that `attached` is false and `n->subsys` is NULL.
>
>KABOOOOM :)

This scenario has been tested but executed without any issue, since here
ARRAY_SIZE calculating size as per the "NVME_MAX_CONTROLLERS" defined.

These two CNS values shows affect when there exists a Subsystem. will add
check condition if there is no Subsystem will return invalid field in command.

if (!n->subsys) {
	return NVME_INVALID_FIELD | NVME_DNR;
}


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] hw/nvme: fix endianess conversion and add controller list

[Klaus Jensen]

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

On Jun 13 17:29, Gollu Appalanaidu wrote:
>On Wed, Jun 09, 2021 at 10:22:49PM +0200, Klaus Jensen wrote:
>>On Jun  1 20:32, Gollu Appalanaidu wrote:
>>>Add the controller identifiers list CNS 0x13, available list of ctrls
>>>in NVM Subsystem that may or may not be attached to namespaces.
>>>
>>>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>
>>>
>>>-v2:
>>>Fix the review comments from Klaus and squashed 2nd commit into
>>>1st commit
>>>
>>>---
>>>hw/nvme/ctrl.c       | 26 ++++++++++++++++----------
>>>hw/nvme/trace-events |  2 +-
>>>include/block/nvme.h |  1 +
>>>3 files changed, 18 insertions(+), 11 deletions(-)
>>>
>>>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>>>index 2e7498a73e..813a72c655 100644
>>>--- a/hw/nvme/ctrl.c
>>>+++ b/hw/nvme/ctrl.c
>>>@@ -4251,9 +4251,11 @@ 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;
>>>+    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];
>>>@@ -4261,15 +4263,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 (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, nsid);
>>>+        if (!ns) {
>>>+            return NVME_INVALID_FIELD | NVME_DNR;
>>>+        }
>>>   }
>>>
>>>   for (cntlid = min_id; cntlid < ARRAY_SIZE(n->subsys->ctrls); cntlid++) {
>>
>>Assume that `attached` is false and `n->subsys` is NULL.
>>
>>KABOOOOM :)
>
>This scenario has been tested but executed without any issue, since here
>ARRAY_SIZE calculating size as per the "NVME_MAX_CONTROLLERS" defined.
>

Yes, I realized this was compile-time, but still surprising behavior and 
not intended ;)

>These two CNS values shows affect when there exists a Subsystem. will add
>check condition if there is no Subsystem will return invalid field in command.
>
>if (!n->subsys) {
>	return NVME_INVALID_FIELD | NVME_DNR;
>}
>

Sounds good.

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