Re: [PATCH v5 15/26] nvme: bump supported specification to 1.3

[Maxim Levitsky <mlevitsk@redhat.com>]

On Mon, 2020-03-16 at 00:50 -0700, Klaus Birkelund Jensen wrote:
> On Feb 12 12:35, Maxim Levitsky wrote:
> > On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > > Add new fields to the Identify Controller and Identify Namespace data
> > > structures accoding to NVM Express 1.3d.
> > > 
> > > NVM Express 1.3d requires the following additional features:
> > >   - addition of the Namespace Identification Descriptor List (CNS 03h)
> > >     for the Identify command
> > >   - support for returning Command Sequence Error if a Set Features
> > >     command is submitted for the Number of Queues feature after any I/O
> > >     queues have been created.
> > >   - The addition of the Log Specific Field (LSP) in the Get Log Page
> > >     command.
> > > 
> > > Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
> > > ---
> > >  hw/block/nvme.c       | 57 ++++++++++++++++++++++++++++++++++++++++---
> > >  hw/block/nvme.h       |  1 +
> > >  hw/block/trace-events |  3 ++-
> > >  include/block/nvme.h  | 20 ++++++++++-----
> > >  4 files changed, 71 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 900732bb2f38..4acfc85b56a2 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -9,7 +9,7 @@
> > >   */
> > >  
> > >  /**
> > > - * Reference Specification: NVM Express 1.2.1
> > > + * Reference Specification: NVM Express 1.3d
> > >   *
> > >   *   https://nvmexpress.org/resources/specifications/
> > >   */
> > > @@ -43,7 +43,7 @@
> > >  #include "trace.h"
> > >  #include "nvme.h"
> > >  
> > > -#define NVME_SPEC_VER 0x00010201
> > > +#define NVME_SPEC_VER 0x00010300
> > >  #define NVME_MAX_QS PCI_MSIX_FLAGS_QSIZE
> > >  #define NVME_TEMPERATURE 0x143
> > >  #define NVME_TEMPERATURE_WARNING 0x157
> > > @@ -735,6 +735,7 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > >      uint32_t dw12 = le32_to_cpu(cmd->cdw12);
> > >      uint32_t dw13 = le32_to_cpu(cmd->cdw13);
> > >      uint8_t  lid = dw10 & 0xff;
> > > +    uint8_t  lsp = (dw10 >> 8) & 0xf;
> > >      uint8_t  rae = (dw10 >> 15) & 0x1;
> > >      uint32_t numdl, numdu;
> > >      uint64_t off, lpol, lpou;
> > > @@ -752,7 +753,7 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > >          return NVME_INVALID_FIELD | NVME_DNR;
> > >      }
> > >  
> > > -    trace_nvme_dev_get_log(nvme_cid(req), lid, rae, len, off);
> > > +    trace_nvme_dev_get_log(nvme_cid(req), lid, lsp, rae, len, off);
> > >  
> > >      switch (lid) {
> > >      case NVME_LOG_ERROR_INFO:
> > > @@ -863,6 +864,8 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
> > >      cq = g_malloc0(sizeof(*cq));
> > >      nvme_init_cq(cq, n, prp1, cqid, vector, qsize + 1,
> > >          NVME_CQ_FLAGS_IEN(qflags));
> > 
> > Code alignment on that '('
> > > +
> > > +    n->qs_created = true;
> > 
> > Should be done also at nvme_create_sq
> 
> No, because you can't create a SQ without a matching CQ:
True, I missed that.

> 
>     if (unlikely(!cqid || nvme_check_cqid(n, cqid))) {
>         trace_nvme_dev_err_invalid_create_sq_cqid(cqid);
>         return NVME_INVALID_CQID | NVME_DNR;
>     }
> 
> 
> So if there is a matching cq, then qs_created = true.
> 
> > >      return NVME_SUCCESS;
> > >  }
> > >  
> > > @@ -924,6 +927,47 @@ static uint16_t nvme_identify_ns_list(NvmeCtrl *n, NvmeIdentify *c)
> > >      return ret;
> > >  }
> > >  
> > > +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeCmd *c)
> > > +{
> > > +    static const int len = 4096;
> > 
> > The spec caps the Identify payload size to 4K,
> > thus this should go to nvme.h
> 
> Done.
> 
> > > +
> > > +    struct ns_descr {
> > > +        uint8_t nidt;
> > > +        uint8_t nidl;
> > > +        uint8_t rsvd2[2];
> > > +        uint8_t nid[16];
> > > +    };
> > 
> > This is also part of the spec, thus should
> > move to nvme.h
> > 
> 
> Done - and cleaned up.
Perfect, thanks!
> 
> > > +
> > > +    uint32_t nsid = le32_to_cpu(c->nsid);
> > > +    uint64_t prp1 = le64_to_cpu(c->prp1);
> > > +    uint64_t prp2 = le64_to_cpu(c->prp2);
> > > +
> > > +    struct ns_descr *list;
> > > +    uint16_t ret;
> > > +
> > > +    trace_nvme_dev_identify_ns_descr_list(nsid);
> > > +
> > > +    if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> > > +        trace_nvme_dev_err_invalid_ns(nsid, n->num_namespaces);
> > > +        return NVME_INVALID_NSID | NVME_DNR;
> > > +    }
> > > +
> > > +    /*
> > > +     * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data
> > > +     * structure, a Namespace UUID (nidt = 0x3) must be reported in the
> > > +     * Namespace Identification Descriptor. Add a very basic Namespace UUID
> > > +     * here.
> > 
> > Some per namespace uuid qemu property will be very nice to have to have a uuid that
> > is at least somewhat unique.
> > Linux kernel I think might complain if it detects namespaces with duplicate uuids.
> 
> It will be "unique" per controller (because it's just the namespace id).
> The spec also says that it should be fixed for the lifetime of the
> namespace, but I'm not sure how to ensure that without keeping that
> state on disk somehow. I have a solution for this in a later series, but
> for now, I think this is ok.
> 
> But since we actually support multiple controllers, there certainly is
> an issue here. Maybe we can blend in some PCI id or something to make it
> unique across controllers.
IMHO, a qemu device property nicely shifts blame for this to an external management
program (e.g libvirt) which can store this in its XML file indeed for the
lifetime of the namespace

> 
> > 
> > > +     */
> > > +    list = g_malloc0(len);
> > > +    list->nidt = 0x3;
> > > +    list->nidl = 0x10;
> > 
> > Those should also be #defined in nvme.h
> 
> Fixed.
> 
> > > +    *(uint32_t *) &list->nid[12] = cpu_to_be32(nsid);
> > > +
> > > +    ret = nvme_dma_read_prp(n, (uint8_t *) list, len, prp1, prp2);
> > > +    g_free(list);
> > > +    return ret;
> > > +}
> > > +
> > >  static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> > >  {
> > >      NvmeIdentify *c = (NvmeIdentify *)cmd;
> > > @@ -935,6 +979,8 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> > >          return nvme_identify_ctrl(n, c);
> > >      case 0x02:
> > >          return nvme_identify_ns_list(n, c);
> > > +    case 0x03:
> > 
> > The CNS values should be defined in nvme.h.
> 
> Fixed.
> 
> > > +        return nvme_identify_ns_descr_list(n, cmd);
> > >      default:
> > >          trace_nvme_dev_err_invalid_identify_cns(le32_to_cpu(c->cns));
> > >          return NVME_INVALID_FIELD | NVME_DNR;
> > > @@ -1133,6 +1179,10 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > >          blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
> > >          break;
> > >      case NVME_NUMBER_OF_QUEUES:
> > > +        if (n->qs_created) {
> > > +            return NVME_CMD_SEQ_ERROR | NVME_DNR;
> > > +        }
> > > +
> > >          if ((dw11 & 0xffff) == 0xffff || ((dw11 >> 16) & 0xffff) == 0xffff) {
> > >              return NVME_INVALID_FIELD | NVME_DNR;
> > >          }
> > > @@ -1267,6 +1317,7 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
> > >  
> > >      n->aer_queued = 0;
> > >      n->outstanding_aers = 0;
> > > +    n->qs_created = false;
> > >  
> > >      blk_flush(n->conf.blk);
> > >      n->bar.cc = 0;
> > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > > index 1e715ab1d75c..7ced5fd485a9 100644
> > > --- a/hw/block/nvme.h
> > > +++ b/hw/block/nvme.h
> > > @@ -97,6 +97,7 @@ typedef struct NvmeCtrl {
> > >      BlockConf    conf;
> > >      NvmeParams   params;
> > >  
> > > +    bool        qs_created;
> > >      uint32_t    page_size;
> > >      uint16_t    page_bits;
> > >      uint16_t    max_prp_ents;
> > > diff --git a/hw/block/trace-events b/hw/block/trace-events
> > > index f982ec1a3221..9e5a4548bde0 100644
> > > --- a/hw/block/trace-events
> > > +++ b/hw/block/trace-events
> > > @@ -41,6 +41,7 @@ nvme_dev_del_cq(uint16_t cqid) "deleted completion queue, sqid=%"PRIu16""
> > >  nvme_dev_identify_ctrl(void) "identify controller"
> > >  nvme_dev_identify_ns(uint32_t ns) "nsid %"PRIu32""
> > >  nvme_dev_identify_ns_list(uint32_t ns) "nsid %"PRIu32""
> > > +nvme_dev_identify_ns_descr_list(uint32_t ns) "nsid %"PRIu32""
> > >  nvme_dev_getfeat(uint16_t cid, uint32_t fid) "cid %"PRIu16" fid 0x%"PRIx32""
> > >  nvme_dev_setfeat(uint16_t cid, uint32_t fid, uint32_t val) "cid %"PRIu16" fid 0x%"PRIx32" val 0x%"PRIx32""
> > >  nvme_dev_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
> > > @@ -48,7 +49,7 @@ nvme_dev_getfeat_numq(int result) "get feature number of queues, result=%d"
> > >  nvme_dev_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> > >  nvme_dev_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
> > >  nvme_dev_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
> > > -nvme_dev_get_log(uint16_t cid, uint8_t lid, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
> > > +nvme_dev_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
> > >  nvme_dev_process_aers(int queued) "queued %d"
> > >  nvme_dev_aer(uint16_t cid) "cid %"PRIu16""
> > >  nvme_dev_aer_aerl_exceeded(void) "aerl exceeded"
> > > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > > index 09419ed499d0..31eb9397d8c6 100644
> > > --- a/include/block/nvme.h
> > > +++ b/include/block/nvme.h
> > > @@ -550,7 +550,9 @@ typedef struct NvmeIdCtrl {
> > >      uint32_t    rtd3e;
> > >      uint32_t    oaes;
> > >      uint32_t    ctratt;
> > > -    uint8_t     rsvd100[156];
> > > +    uint8_t     rsvd100[12];
> > > +    uint8_t     fguid[16];
> > > +    uint8_t     rsvd128[128];
> > 
> > looks OK
> > >      uint16_t    oacs;
> > >      uint8_t     acl;
> > >      uint8_t     aerl;
> > > @@ -568,9 +570,15 @@ typedef struct NvmeIdCtrl {
> > >      uint8_t     tnvmcap[16];
> > >      uint8_t     unvmcap[16];
> > >      uint32_t    rpmbs;
> > > -    uint8_t     rsvd316[4];
> > > +    uint16_t    edstt;
> > > +    uint8_t     dsto;
> > > +    uint8_t     fwug;
> > 
> > looks OK
> > >      uint16_t    kas;
> > > -    uint8_t     rsvd322[190];
> > > +    uint16_t    hctma;
> > > +    uint16_t    mntmt;
> > > +    uint16_t    mxtmt;
> > > +    uint32_t    sanicap;
> > > +    uint8_t     rsvd332[180];
> > 
> > looks OK
> > >      uint8_t     sqes;
> > >      uint8_t     cqes;
> > >      uint16_t    maxcmd;
> > > @@ -691,19 +699,19 @@ typedef struct NvmeIdNs {
> > >      uint8_t     rescap;
> > >      uint8_t     fpi;
> > >      uint8_t     dlfeat;
> > > -    uint8_t     rsvd33;
> > >      uint16_t    nawun;
> > >      uint16_t    nawupf;
> > > +    uint16_t    nacwu;
> > 
> > Aha! Here you 'fix' the bug you had in patch 4.

I thought for a moment that you didn't fix this, but
after looking at V6, it is fixed.
I didn't get any feedback for patch 4, so I double checked this.

> > >      uint16_t    nabsn;
> > >      uint16_t    nabo;
> > >      uint16_t    nabspf;
> > > -    uint8_t     rsvd46[2];
> > > +    uint16_t    noiob;
> > >      uint8_t     nvmcap[16];
> > >      uint8_t     rsvd64[40];
> > >      uint8_t     nguid[16];
> > >      uint64_t    eui64;
> > >      NvmeLBAF    lbaf[16];
> > > -    uint8_t     res192[192];
> > > +    uint8_t     rsvd192[192];
> > 
> > And even do what I suggested with that field :-)
> > Please squash the changes.
> > >      uint8_t     vs[3712];
> > >  } NvmeIdNs;
> > >  
> > 
> > So I suggest you squash this set of changes with patch 4.
> > I also suggest you to split the other changes in this patch, 1 per feature added.
> > The tracing change can also be squashed with the other tracing patch you submitted.
> > 
> > In summary I would suggest you to have:
> > 
> > 1. patch that only adds all the fields from the 1.3d spec, and overall updates nvme.h
> > to be up to 1.3d spec
> > 
> > 2. patches that do refactoring, add more tracing (also form of refactoring, since tracing
> > isn't a functional thing)
> > 
> > 3. set of patches that implement all the 1.3d features.
> > 
> > 4. patch that only bumps the supported version right to 1.3d
> > 
> 
> Did this! :)

Thank you!

Best regards,
	Maxim Levitsky

>