| Message ID | 20200204095208.269131-16-k.jensen@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvme: support NVMe v1.3d, SGLs and multiple namespaces | expand |
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 > 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 > + > + 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 > + > + 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. > + */ > + list = g_malloc0(len); > + list->nidt = 0x3; > + list->nidl = 0x10; Those should also be #defined in nvme.h > + *(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. > + 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. > 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 Best regards, Maxim Levitsky
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: 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. > > + > > + 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. > > > + */ > > + 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. > > 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! :)
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 >
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)); + + n->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; + + struct ns_descr { + uint8_t nidt; + uint8_t nidl; + uint8_t rsvd2[2]; + uint8_t nid[16]; + }; + + 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. + */ + list = g_malloc0(len); + list->nidt = 0x3; + list->nidl = 0x10; + *(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: + 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]; 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; uint16_t kas; - uint8_t rsvd322[190]; + uint16_t hctma; + uint16_t mntmt; + uint16_t mxtmt; + uint32_t sanicap; + uint8_t rsvd332[180]; 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; 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]; uint8_t vs[3712]; } NvmeIdNs;
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(-)