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

[Gollu Appalanaidu <anaidu.gollu@samsung.com>]

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