> -----Original Message-----
> From: Klaus Jensen <i...@irrelevant.dk>
> Sent: Thursday, October 1, 2020 6:15 PM
> To: Dmitry Fomichev <dmitry.fomic...@wdc.com>
> Cc: Keith Busch <kbu...@kernel.org>; Klaus Jensen
> <k.jen...@samsung.com>; Kevin Wolf <kw...@redhat.com>; Philippe
> Mathieu-Daudé <phi...@redhat.com>; Maxim Levitsky
> <mlevi...@redhat.com>; Fam Zheng <f...@euphon.net>; Niklas Cassel
> <niklas.cas...@wdc.com>; Damien Le Moal <damien.lem...@wdc.com>;
> qemu-bl...@nongnu.org; qemu-devel@nongnu.org; Alistair Francis
> <alistair.fran...@wdc.com>; Matias Bjorling <matias.bjorl...@wdc.com>
> Subject: Re: [PATCH v5 05/14] hw/block/nvme: Add support for Namespace
> Types
>
> On Sep 28 11:35, Dmitry Fomichev wrote:
> > From: Niklas Cassel <niklas.cas...@wdc.com>
> >
> > Namespace Types introduce a new command set, "I/O Command Sets",
> > that allows the host to retrieve the command sets associated with
> > a namespace. Introduce support for the command set and enable
> > detection for the NVM Command Set.
> >
> > The new workflows for identify commands rely heavily on zero-filled
> > identify structs. E.g., certain CNS commands are defined to return
> > a zero-filled identify struct when an inactive namespace NSID
> > is supplied.
> >
> > Add a helper function in order to avoid code duplication when
> > reporting zero-filled identify structures.
> >
> > Signed-off-by: Niklas Cassel <niklas.cas...@wdc.com>
> > Signed-off-by: Dmitry Fomichev <dmitry.fomic...@wdc.com>
> > ---
> > hw/block/nvme-ns.c | 3 +
> > hw/block/nvme.c | 210 +++++++++++++++++++++++++++++++++++++-
> -------
> > 2 files changed, 175 insertions(+), 38 deletions(-)
> >
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index bbd7879492..31b7f986c3 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
>
> The following looks like a rebase gone wrong.
>
> There are some redundant checks and wrong return values.
>
> > static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest
> *req)
> > {
> > NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> > + NvmeNamespace *ns;
> > uint32_t nsid = le32_to_cpu(c->nsid);
> > - uint8_t list[NVME_IDENTIFY_DATA_SIZE];
> > -
> > - struct data {
> > - struct {
> > - NvmeIdNsDescr hdr;
> > - uint8_t v[16];
> > - } uuid;
> > - };
> > -
> > - struct data *ns_descrs = (struct data *)list;
> > + NvmeIdNsDescr *desc;
> > + uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> > + static const int data_len = sizeof(list);
> > + void *list_ptr = list;
>
> Oh maaan, please do not replace my nicely cleaned up code with pointer
> arithmetics :(
>
> >
> > trace_pci_nvme_identify_ns_descr_list(nsid);
> >
> > - if (!nvme_nsid_valid(n, nsid) || nsid == NVME_NSID_BROADCAST) {
> > - return NVME_INVALID_NSID | NVME_DNR;
> > - }
> > -
>
> This removal looks wrong.
>
> > if (unlikely(!nvme_ns(n, nsid))) {
> > return NVME_INVALID_FIELD | NVME_DNR;
> > }
> >
> > - memset(list, 0x0, sizeof(list));
> > + ns = nvme_ns(n, nsid);
> > + if (unlikely(!ns)) {
> > + return nvme_rpt_empty_id_struct(n, req);
> > + }
> >
>
> And this doesnt look like it belongs (its checked just a few lines
> before, and it returns an error status as it should).
>
This and above looks like a merge error, I am correcting this along
with moving UUID calculation to a separate commit.
> > /*
> > * Because the NGUID and EUI64 fields are 0 in the Identify Namespace
> data
> > @@ -1597,12 +1667,31 @@ static uint16_t
> nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
> > * Namespace Identification Descriptor. Add a very basic Namespace
> UUID
> > * here.
> > */
> > - ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
> > - ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
> > - stl_be_p(&ns_descrs->uuid.v, nsid);
> > + desc = list_ptr;
> > + desc->nidt = NVME_NIDT_UUID;
> > + desc->nidl = NVME_NIDL_UUID;
> > + list_ptr += sizeof(*desc);
> > + memcpy(list_ptr, ns->params.uuid.data, NVME_NIDL_UUID);
> > + list_ptr += NVME_NIDL_UUID;
> >
> > - return nvme_dma(n, list, NVME_IDENTIFY_DATA_SIZE,
> > - DMA_DIRECTION_FROM_DEVICE, req);
> > + desc = list_ptr;
> > + desc->nidt = NVME_NIDT_CSI;
> > + desc->nidl = NVME_NIDL_CSI;
> > + list_ptr += sizeof(*desc);
> > + *(uint8_t *)list_ptr = NVME_CSI_NVM;
> > +
> > + return nvme_dma(n, list, data_len, DMA_DIRECTION_FROM_DEVICE,
> req);
> > +}
> > +
