2:53 p.m.
From: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 01/11] hw/nvme: fix narrowing conversion
From: Dmitry Tikhov <ddtikhov(a)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(a)gmail.com>
Reviewed-by: Klaus Jensen <k.jensen(a)samsung.com>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 02/11] hw/nvme: add missing return statement
From: Dmitry Tikhov <d.tihov(a)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(a)yadro.com>
Reviewed-by: Klaus Jensen <k.jensen(a)samsung.com>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 03/11] hw/nvme: fix copy cmd for pi enabled namespaces
From: Dmitry Tikhov <d.tihov(a)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(a)yadro.com>
Reviewed-by: Klaus Jensen <k.jensen(a)samsung.com>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 04/11] hw/nvme: fix smart aen
From: Klaus Jensen <k.jensen(a)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(a)bytedance.com>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 05/11] hw/nvme: enforce common serial per subsystem
From: Klaus Jensen <k.jensen(a)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(a)lst.de>
Reviewed-by: Keith Busch <kbusch(a)kernel.org>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 06/11] hw/nvme: do not auto-generate eui64
From: Klaus Jensen <k.jensen(a)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(a)redhat.com
Reviewed-by: Christoph Hellwig <hch(a)lst.de>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 07/11] hw/nvme: do not auto-generate uuid
From: Klaus Jensen <k.jensen(a)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(a)kernel.org>
Reviewed-by: Christoph Hellwig <hch(a)lst.de>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 08/11] hw/nvme: do not report null uuid
From: Klaus Jensen <k.jensen(a)samsung.com>
Do not report the "null uuid" (all zeros) in the namespace
identification descriptors.
Reported-by: Luis Chamberlain <mcgrof(a)kernel.org>
Reported-by: Christoph Hellwig <hch(a)lst.de>
Reviewed-by: Christoph Hellwig <hch(a)lst.de>
Reviewed-by: Keith Busch <kbusch(a)kernel.org>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 09/11] hw/nvme: bump firmware revision
From: Klaus Jensen <k.jensen(a)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(a)kernel.org>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 10/11] hw/nvme: deprecate the use-intel-id compatibility parameter
From: Klaus Jensen <k.jensen(a)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(a)amsat.org>
Signed-off-by: Klaus Jensen <k.jensen(a)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
2:53 p.m.
New subject: [PULL 11/11] hw/nvme: add new command abort case
From: Dmitry Tikhov <d.tihov(a)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(a)yadro.com>
Signed-off-by: Klaus Jensen <k.jensen(a)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
903
days inactive
903
days old
11 comments
1 participants
participants (1)
-
Klaus Jensen