qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH v5 05/14] hw/block/nvme: Add support for Namespace Types


From: Dmitry Fomichev
Subject: RE: [PATCH v5 05/14] hw/block/nvme: Add support for Namespace Types
Date: Thu, 1 Oct 2020 22:30:55 +0000

> -----Original Message-----
> From: Klaus Jensen <its@irrelevant.dk>
> Sent: Thursday, October 1, 2020 6:15 PM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Cc: Keith Busch <kbusch@kernel.org>; Klaus Jensen
> <k.jensen@samsung.com>; Kevin Wolf <kwolf@redhat.com>; Philippe
> Mathieu-Daudé <philmd@redhat.com>; Maxim Levitsky
> <mlevitsk@redhat.com>; Fam Zheng <fam@euphon.net>; Niklas Cassel
> <Niklas.Cassel@wdc.com>; Damien Le Moal <Damien.LeMoal@wdc.com>;
> qemu-block@nongnu.org; qemu-devel@nongnu.org; Alistair Francis
> <Alistair.Francis@wdc.com>; Matias Bjorling <Matias.Bjorling@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.cassel@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.cassel@wdc.com>
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@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);
> > +}
> > +


reply via email to

[Prev in Thread] Current Thread [Next in Thread]