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.jen...@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 >