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

[Klaus Jensen <its@irrelevant.dk>]

[-- 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 --]