[PATCH v2 0/5] hw/nvme: fix namespace identifiers

[PATCH v2 0/5] hw/nvme: fix namespace identifiers

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

The namespace identifiers reported by the controller is kind of a mess.
See [1,2].

This series should fix this for both the `-device nvme,drive=...` and
`-device nvme-ns,...` cases.

  [1]: https://lore.kernel.org/linux-nvme/20220224192845.1097602-1-hch@lst.de/
  [2]: https://lore.kernel.org/linux-nvme/20220413044905.376785-1-hch@lst.de/

Changes since v1:
 - Revert auto-generation of eui64 (Christoph)
   User should set it explicitly.

Klaus Jensen (5):
  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

 docs/about/deprecated.rst |  7 +++++++
 hw/core/machine.c         |  4 +++-
 hw/nvme/ctrl.c            | 19 ++++++++-----------
 hw/nvme/ns.c              |  4 ++--
 hw/nvme/nvme.h            |  1 +
 hw/nvme/subsys.c          |  7 +++++++
 6 files changed, 28 insertions(+), 14 deletions(-)

-- 
2.35.1

[PATCH v2 1/5] hw/nvme: enforce common serial per subsystem

[Klaus Jensen]

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 739c8b8f7962..7f2e8f1b6491 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.35.1

[PATCH v2 2/5] hw/nvme: do not auto-generate eui64

[Klaus Jensen]

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
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 docs/about/deprecated.rst | 7 +++++++
 hw/core/machine.c         | 4 +++-
 hw/nvme/ns.c              | 2 +-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 896e5a97abbd..c65faa5ab4ad 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -356,6 +356,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 cb9bbc844d24..1e2108d95f11 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -37,7 +37,9 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-pci.h"
 
-GlobalProperty hw_compat_7_0[] = {};
+GlobalProperty hw_compat_7_0[] = {
+    { "nvme-ns", "eui64-default", "on"},
+};
 const size_t hw_compat_7_0_len = G_N_ELEMENTS(hw_compat_7_0);
 
 GlobalProperty hw_compat_6_2[] = {
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.35.1

Re: [PATCH v2 2/5] hw/nvme: do not auto-generate eui64

[Christoph Hellwig]

On Fri, Apr 29, 2022 at 10:33:33AM +0200, Klaus Jensen wrote:
> 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`.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 3/5] hw/nvme: do not auto-generate uuid

[Klaus Jensen]

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>
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.35.1

Re: [PATCH v2 3/5] hw/nvme: do not auto-generate uuid

[Christoph Hellwig]

On Fri, Apr 29, 2022 at 10:33:34AM +0200, Klaus Jensen wrote:
> 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>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 4/5] hw/nvme: do not report null uuid

[Klaus Jensen]

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 08574c4dcbc8..5a727b6ec344 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.35.1

[PATCH v2 5/5] hw/nvme: bump firmware revision

[Klaus Jensen]

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 5a727b6ec344..650b606c6c24 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.35.1

Re: [PATCH v2 0/5] hw/nvme: fix namespace identifiers

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On Apr 29 10:33, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The namespace identifiers reported by the controller is kind of a mess.
> See [1,2].
> 
> This series should fix this for both the `-device nvme,drive=...` and
> `-device nvme-ns,...` cases.
> 
>   [1]: https://lore.kernel.org/linux-nvme/20220224192845.1097602-1-hch@lst.de/
>   [2]: https://lore.kernel.org/linux-nvme/20220413044905.376785-1-hch@lst.de/
> 
> Changes since v1:
>  - Revert auto-generation of eui64 (Christoph)
>    User should set it explicitly.
> 
> Klaus Jensen (5):
>   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
> 
>  docs/about/deprecated.rst |  7 +++++++
>  hw/core/machine.c         |  4 +++-
>  hw/nvme/ctrl.c            | 19 ++++++++-----------
>  hw/nvme/ns.c              |  4 ++--
>  hw/nvme/nvme.h            |  1 +
>  hw/nvme/subsys.c          |  7 +++++++
>  6 files changed, 28 insertions(+), 14 deletions(-)
> 
> -- 
> 2.35.1
> 

Thanks for the reviews! Applied to nvme-next.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]