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.go...@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.
signature.asc
Description: PGP signature
