[PULL 00/11] hw/nvme updates
From: Klaus Jensen <k.jensen@samsung.com> Hi Peter, The following changes since commit 70e975203f366f2f30daaeb714bb852562b7b72f: Merge tag 'pull-request-2022-06-03' of https://gitlab.com/thuth/qemu into staging (2022-06-03 06:43:38 -0700) are available in the Git repository at: git://git.infradead.org/qemu-nvme.git tags/nvme-next-pull-request for you to fetch changes up to d7fe639cabf778903f6cab23ff58c905c71375ec: hw/nvme: add new command abort case (2022-06-03 21:48:24 +0200) ---------------------------------------------------------------- hw/nvme updates ---------------------------------------------------------------- Dmitry Tikhov (4): hw/nvme: fix narrowing conversion hw/nvme: add missing return statement hw/nvme: fix copy cmd for pi enabled namespaces hw/nvme: add new command abort case Klaus Jensen (7): hw/nvme: fix smart aen hw/nvme: enforce common serial per subsystem hw/nvme: do not auto-generate eui64 hw/nvme: do not auto-generate uuid hw/nvme: do not report null uuid hw/nvme: bump firmware revision hw/nvme: deprecate the use-intel-id compatibility parameter docs/about/deprecated.rst | 15 +++++++++++++++ hw/core/machine.c | 1 + hw/nvme/ctrl.c | 26 ++++++++++++++------------ hw/nvme/dif.c | 5 +++++ hw/nvme/ns.c | 9 +++++---- hw/nvme/nvme.h | 1 + hw/nvme/subsys.c | 7 +++++++ 7 files changed, 48 insertions(+), 16 deletions(-) -- 2.36.1
From: Dmitry Tikhov <ddtikhov@gmail.com> Since nlbas is of type int, it does not work with large namespace size values, e.g., 9 TB size of file backing namespace and 8 byte metadata with 4096 bytes lbasz gives negative nlbas value, which is later promoted to negative int64_t type value and results in negative ns->moff which breaks namespace Signed-off-by: Dmitry Tikhov <ddtikhov@gmail.com> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/ns.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c index 324f53ea0cd1..af6504fad2d8 100644 --- a/hw/nvme/ns.c +++ b/hw/nvme/ns.c @@ -29,7 +29,8 @@ void nvme_ns_init_format(NvmeNamespace *ns) { NvmeIdNs *id_ns = &ns->id_ns; BlockDriverInfo bdi; - int npdg, nlbas, ret; + int npdg, ret; + int64_t nlbas; ns->lbaf = id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(id_ns->flbas)]; ns->lbasz = 1 << ns->lbaf.ds; @@ -42,7 +43,7 @@ void nvme_ns_init_format(NvmeNamespace *ns) id_ns->ncap = id_ns->nsze; id_ns->nuse = id_ns->ncap; - ns->moff = (int64_t)nlbas << ns->lbaf.ds; + ns->moff = nlbas << ns->lbaf.ds; npdg = ns->blkconf.discard_granularity / ns->lbasz; -- 2.36.1
From: Dmitry Tikhov <d.tihov@yadro.com> Since there is no return after nvme_dsm_cb invocation, metadata associated with non-zero block range is currently zeroed. Also this behaviour leads to segfault since we schedule iocb->bh two times. First when entering nvme_dsm_cb with iocb->idx == iocb->nr and second because of missing return on call stack unwinding by calling blk_aio_pwrite_zeroes and subsequent nvme_dsm_cb callback. Fixes: d7d1474fd85d ("hw/nvme: reimplement dsm to allow cancellation") Signed-off-by: Dmitry Tikhov <d.tihov@yadro.com> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/ctrl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 03760ddeae8c..74540a03d518 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -2372,6 +2372,7 @@ static void nvme_dsm_md_cb(void *opaque, int ret) } nvme_dsm_cb(iocb, 0); + return; } iocb->aiocb = blk_aio_pwrite_zeroes(ns->blkconf.blk, nvme_moff(ns, slba), -- 2.36.1
From: Dmitry Tikhov <d.tihov@yadro.com> Current implementation have problem in the read part of copy command. Because there is no metadata mangling before nvme_dif_check invocation, reftag error could be thrown for blocks of namespace that have not been previously written to. Signed-off-by: Dmitry Tikhov <d.tihov@yadro.com> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/ctrl.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 74540a03d518..08574c4dcbc8 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -2787,6 +2787,10 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret) size_t mlen = nvme_m2b(ns, nlb); uint8_t *mbounce = iocb->bounce + nvme_l2b(ns, nlb); + status = nvme_dif_mangle_mdata(ns, mbounce, mlen, slba); + if (status) { + goto invalid; + } status = nvme_dif_check(ns, iocb->bounce, len, mbounce, mlen, prinfor, slba, apptag, appmask, &reftag); if (status) { -- 2.36.1
From: Klaus Jensen <k.jensen@samsung.com> Pass the right constant to nvme_smart_event(). The NVME_AER* values hold the bit position in the SMART byte, not the shifted value that we expect it to be in nvme_smart_event(). Fixes: c62720f137df ("hw/block/nvme: trigger async event during injecting smart warning") Acked-by: zhenwei pi <pizhenwei@bytedance.com> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/ctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 08574c4dcbc8..a2f6069f7fe1 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -5325,7 +5325,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req) if ((n->temperature >= n->features.temp_thresh_hi) || (n->temperature <= n->features.temp_thresh_low)) { - nvme_smart_event(n, NVME_AER_INFO_SMART_TEMP_THRESH); + nvme_smart_event(n, NVME_SMART_TEMPERATURE); } break; -- 2.36.1
From: Klaus Jensen <k.jensen@samsung.com> The Identify Controller Serial Number (SN) is the serial number for the NVM subsystem and must be the same across all controller in the NVM subsystem. Enforce this. Reviewed-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/nvme.h | 1 + hw/nvme/subsys.c | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h index 677381932569..e41771604f59 100644 --- a/hw/nvme/nvme.h +++ b/hw/nvme/nvme.h @@ -48,6 +48,7 @@ typedef struct NvmeSubsystem { DeviceState parent_obj; NvmeBus bus; uint8_t subnqn[256]; + char *serial; NvmeCtrl *ctrls[NVME_MAX_CONTROLLERS]; NvmeNamespace *namespaces[NVME_MAX_NAMESPACES + 1]; diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c index fb58d639504e..691a90d20947 100644 --- a/hw/nvme/subsys.c +++ b/hw/nvme/subsys.c @@ -27,6 +27,13 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp) return -1; } + if (!subsys->serial) { + subsys->serial = g_strdup(n->params.serial); + } else if (strcmp(subsys->serial, n->params.serial)) { + error_setg(errp, "invalid controller serial"); + return -1; + } + subsys->ctrls[cntlid] = n; for (nsid = 1; nsid < ARRAY_SIZE(subsys->namespaces); nsid++) { -- 2.36.1
From: Klaus Jensen <k.jensen@samsung.com> We cannot provide auto-generated unique or persistent namespace identifiers (EUI64, NGUID, UUID) easily. Since 6.1, namespaces have been assigned a generated EUI64 of the form "52:54:00:<namespace counter>". This is will be unique within a QEMU instance, but not globally. Revert that this is assigned automatically and immediately deprecate the compatibility parameter. Users can opt-in to this with the `eui64-default=on` device parameter or set it explicitly with `eui64=UINT64`. Cc: libvir-list@redhat.com Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- docs/about/deprecated.rst | 7 +++++++ hw/core/machine.c | 1 + hw/nvme/ns.c | 2 +- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index e19bcba24240..47a8628b5601 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -296,6 +296,13 @@ contains native support for this feature and thus use of the option ROM approach is obsolete. The native SeaBIOS support can be activated by using ``-machine graphics=off``. +``-device nvme-ns,eui64-default=on|off`` (since 7.1) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +In QEMU versions 6.1, 6.2 and 7.0, the ``nvme-ns`` generates an EUI-64 +identifer that is not globally unique. If an EUI-64 identifer is required, the +user must set it explicitly using the ``nvme-ns`` device parameter ``eui64``. + Block device options '''''''''''''''''''' diff --git a/hw/core/machine.c b/hw/core/machine.c index bb0dc8f6a93d..c53548d0b138 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -43,6 +43,7 @@ GlobalProperty hw_compat_7_0[] = { { "arm-gicv3-common", "force-8-bit-prio", "on" }, + { "nvme-ns", "eui64-default", "on"}, }; const size_t hw_compat_7_0_len = G_N_ELEMENTS(hw_compat_7_0); diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c index af6504fad2d8..06a04131f192 100644 --- a/hw/nvme/ns.c +++ b/hw/nvme/ns.c @@ -641,7 +641,7 @@ static Property nvme_ns_props[] = { DEFINE_PROP_SIZE("zoned.zrwas", NvmeNamespace, params.zrwas, 0), DEFINE_PROP_SIZE("zoned.zrwafg", NvmeNamespace, params.zrwafg, -1), DEFINE_PROP_BOOL("eui64-default", NvmeNamespace, params.eui64_default, - true), + false), DEFINE_PROP_END_OF_LIST(), }; -- 2.36.1
From: Klaus Jensen <k.jensen@samsung.com> Do not default to generate an UUID for namespaces if it is not explicitly specified. This is a technically a breaking change in behavior. However, since the UUID changes on every VM launch, it is not spec compliant and is of little use since the UUID cannot be used reliably anyway and the behavior prior to this patch must be considered buggy. Reviewed-by: Keith Busch <kbusch@kernel.org> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/ns.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c index 06a04131f192..1b9c9d11567f 100644 --- a/hw/nvme/ns.c +++ b/hw/nvme/ns.c @@ -614,7 +614,7 @@ static Property nvme_ns_props[] = { DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false), DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, true), DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0), - DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid), + DEFINE_PROP_UUID_NODEFAULT("uuid", NvmeNamespace, params.uuid), DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0), DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0), DEFINE_PROP_UINT8("mset", NvmeNamespace, params.mset, 0), -- 2.36.1
From: Klaus Jensen <k.jensen@samsung.com> Do not report the "null uuid" (all zeros) in the namespace identification descriptors. Reported-by: Luis Chamberlain <mcgrof@kernel.org> Reported-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/ctrl.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index a2f6069f7fe1..909e357a7eb9 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -4955,16 +4955,13 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req) return NVME_INVALID_FIELD | NVME_DNR; } - /* - * If the EUI-64 field is 0 and the NGUID field is 0, the namespace must - * provide a valid Namespace UUID in the Namespace Identification Descriptor - * data structure. QEMU does not yet support setting NGUID. - */ - uuid.hdr.nidt = NVME_NIDT_UUID; - uuid.hdr.nidl = NVME_NIDL_UUID; - memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID); - memcpy(pos, &uuid, sizeof(uuid)); - pos += sizeof(uuid); + if (!qemu_uuid_is_null(&ns->params.uuid)) { + uuid.hdr.nidt = NVME_NIDT_UUID; + uuid.hdr.nidl = NVME_NIDL_UUID; + memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID); + memcpy(pos, &uuid, sizeof(uuid)); + pos += sizeof(uuid); + } if (ns->params.eui64) { eui64.hdr.nidt = NVME_NIDT_EUI64; -- 2.36.1
From: Klaus Jensen <k.jensen@samsung.com> The Linux kernel quirks the QEMU NVMe controller pretty heavily because of the namespace identifier mess. Since this is now fixed, bump the firmware revision number to allow the quirk to be disabled for this revision. As of now, bump the firmware revision number to be equal to the QEMU release version number. Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/ctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 909e357a7eb9..1e6e0fcad918 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -6713,7 +6713,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev) id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID)); id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID)); strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl", ' '); - strpadcpy((char *)id->fr, sizeof(id->fr), "1.0", ' '); + strpadcpy((char *)id->fr, sizeof(id->fr), QEMU_VERSION, ' '); strpadcpy((char *)id->sn, sizeof(id->sn), n->params.serial, ' '); id->cntlid = cpu_to_le16(n->cntlid); -- 2.36.1
From: Klaus Jensen <k.jensen@samsung.com> Since version 5.2 commit 6eb7a071292a ("hw/block/nvme: change controller pci id"), the emulated NVMe controller has defaulted to a non-Intel PCI identifier. Deprecate the compatibility parameter so we can get rid of it once and for all. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- docs/about/deprecated.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 47a8628b5601..aa2e32020707 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -303,6 +303,14 @@ In QEMU versions 6.1, 6.2 and 7.0, the ``nvme-ns`` generates an EUI-64 identifer that is not globally unique. If an EUI-64 identifer is required, the user must set it explicitly using the ``nvme-ns`` device parameter ``eui64``. +``-device nvme,use-intel-id=on|off`` (since 7.1) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The ``nvme`` device originally used a PCI Vendor/Device Identifier combination +from Intel that was not properly allocated. Since version 5.2, the controller +has used a properly allocated identifier. Deprecate the ``use-intel-id`` +machine compatibility parameter. + Block device options '''''''''''''''''''' -- 2.36.1
From: Dmitry Tikhov <d.tihov@yadro.com> NVMe command set specification for end-to-end data protection formatted namespace states: o If the Reference Tag Check bit of the PRCHK field is set to ‘1’ and the namespace is formatted for Type 3 protection, then the controller: ▪ should not compare the protection Information Reference Tag field to the computed reference tag; and ▪ may ignore the ILBRT and EILBRT fields. If a command is aborted as a result of the Reference Tag Check bit of the PRCHK field being set to ‘1’, then that command should be aborted with a status code of Invalid Protection Information, but may be aborted with a status code of Invalid Field in Command. Currently qemu compares reftag in the nvme_dif_prchk function whenever Reference Tag Check bit is set in the command. For type 3 namespaces however, caller of nvme_dif_prchk - nvme_dif_check does not increment reftag for each subsequent logical block. That way commands incorporating more than one logical block for type 3 formatted namespaces with reftag check bit set, always fail with End-to-end Reference Tag Check Error. Comply with spec by handling case of set Reference Tag Check bit in the type 3 formatted namespace. Fixes: 146f720c5563 ("hw/block/nvme: end-to-end data protection") Signed-off-by: Dmitry Tikhov <d.tihov@yadro.com> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/nvme/dif.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c index 62d885f83ea4..63c44c86ab55 100644 --- a/hw/nvme/dif.c +++ b/hw/nvme/dif.c @@ -26,6 +26,11 @@ uint16_t nvme_check_prinfo(NvmeNamespace *ns, uint8_t prinfo, uint64_t slba, return NVME_INVALID_PROT_INFO | NVME_DNR; } + if ((NVME_ID_NS_DPS_TYPE(ns->id_ns.dps) == NVME_ID_NS_DPS_TYPE_3) && + (prinfo & NVME_PRINFO_PRCHK_REF)) { + return NVME_INVALID_PROT_INFO; + } + return NVME_SUCCESS; } -- 2.36.1
participants (1)
-
Klaus Jensen