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

[Gollu Appalanaidu]

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;
}