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

[PATCH 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/

Klaus Jensen (5):
  hw/nvme: enforce common serial per subsystem
  hw/nvme: always set eui64
  hw/nvme: do not report null uuid
  hw/nvme: do not auto-generate uuid
  hw/nvme: bump firmware revision

 docs/system/devices/nvme.rst |  6 ++++--
 hw/nvme/ctrl.c               | 21 ++++++++++-----------
 hw/nvme/ns.c                 | 14 +++++++++-----
 hw/nvme/nvme.h               |  4 ++++
 hw/nvme/subsys.c             |  7 +++++++
 5 files changed, 34 insertions(+), 18 deletions(-)

-- 
2.35.1

[PATCH 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.

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 2/5] hw/nvme: always set eui64

[Klaus Jensen]

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

Unconditionally set an EUI64 for namespaces. The nvme-ns device defaults
to auto-generating a persistent EUI64 if not specified, but for single
namespace setups (-device nvme,drive=...), this does not happen.

Since the EUI64 has previously been zeroed it is not considered valid,
so it should be safe to add this now.

The generated EUI64 is of the form 52:54:00:<namespace counter>. Note,
this is NOT the namespace identifier since that is not unique across
subsystems; it is a global namespace counter. This has the effect that
the value of this auto-generated EUI64 is dependent on the order with
which the namespaces are created. If a more flexible setup is required,
the eui64 namespace parameter should be explicitly set. Update the
documentation to make this clear.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 docs/system/devices/nvme.rst |  6 ++++--
 hw/nvme/ctrl.c               |  2 ++
 hw/nvme/ns.c                 | 12 ++++++++----
 hw/nvme/nvme.h               |  3 +++
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst
index b5acb2a9c19d..f9b8c7f3eeb4 100644
--- a/docs/system/devices/nvme.rst
+++ b/docs/system/devices/nvme.rst
@@ -84,8 +84,10 @@ There are a number of parameters available:
 ``eui64``
   Set the EUI-64 of the namespace. This will be reported as a "IEEE Extended
   Unique Identifier" descriptor in the Namespace Identification Descriptor List.
-  Since machine type 6.1 a non-zero default value is used if the parameter
-  is not provided. For earlier machine types the field defaults to 0.
+  Since machine type 6.1, an EUI-64 is auto-generated. However, note that this
+  auto-generated value is dependent on the order with which the namespaces are
+  created. If you intend on adding/removing namespaces on your VM, set this
+  parameter explicitly. For earlier machine types the field defaults to 0.
 
 ``bus``
   If there are more ``nvme`` devices defined, this parameter may be used to
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 03760ddeae8c..17cf9862ab89 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6862,6 +6862,8 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         ns = &n->namespace;
         ns->params.nsid = 1;
 
+        nvme_ns_set_eui64(ns);
+
         if (nvme_ns_setup(ns, errp)) {
             return;
         }
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 324f53ea0cd1..5685221f47c6 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -25,6 +25,8 @@
 #define MIN_DISCARD_GRANULARITY (4 * KiB)
 #define NVME_DEFAULT_ZONE_SIZE   (128 * MiB)
 
+uint64_t nvme_ns_count;
+
 void nvme_ns_init_format(NvmeNamespace *ns)
 {
     NvmeIdNs *id_ns = &ns->id_ns;
@@ -54,9 +56,13 @@ void nvme_ns_init_format(NvmeNamespace *ns)
     id_ns->npda = id_ns->npdg = npdg - 1;
 }
 
+void nvme_ns_set_eui64(NvmeNamespace *ns)
+{
+    ns->params.eui64 = NVME_EUI64_DEFAULT | ++nvme_ns_count;
+}
+
 static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 {
-    static uint64_t ns_count;
     NvmeIdNs *id_ns = &ns->id_ns;
     NvmeIdNsNvm *id_ns_nvm = &ns->id_ns_nvm;
     uint8_t ds;
@@ -75,10 +81,8 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
         id_ns->nmic |= NVME_NMIC_NS_SHARED;
     }
 
-    /* Substitute a missing EUI-64 by an autogenerated one */
-    ++ns_count;
     if (!ns->params.eui64 && ns->params.eui64_default) {
-        ns->params.eui64 = ns_count + NVME_EUI64_DEFAULT;
+        nvme_ns_set_eui64(ns);
     }
 
     /* simple copy */
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 7f2e8f1b6491..3922cf531f6d 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -33,6 +33,8 @@ QEMU_BUILD_BUG_ON(NVME_MAX_NAMESPACES > NVME_NSID_BROADCAST - 1);
 typedef struct NvmeCtrl NvmeCtrl;
 typedef struct NvmeNamespace NvmeNamespace;
 
+extern uint64_t nvme_ns_count;
+
 #define TYPE_NVME_BUS "nvme-bus"
 OBJECT_DECLARE_SIMPLE_TYPE(NvmeBus, NVME_BUS)
 
@@ -511,6 +513,7 @@ static inline uint16_t nvme_cid(NvmeRequest *req)
     return le16_to_cpu(req->cqe.cid);
 }
 
+void nvme_ns_set_eui64(NvmeNamespace *ns);
 void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns);
 uint16_t nvme_bounce_data(NvmeCtrl *n, void *ptr, uint32_t len,
                           NvmeTxDirection dir, NvmeRequest *req);
-- 
2.35.1

[PATCH 3/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>
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 17cf9862ab89..2b7828ac6952 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4950,16 +4950,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 4/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.

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 5685221f47c6..960c5124b811 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -617,7 +617,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

[PATCH 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.

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 2b7828ac6952..dc298aacc422 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6708,7 +6708,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 0/5] hw/nvme: fix namespace identifiers

[Keith Busch]

On Tue, Apr 19, 2022 at 02:10:34PM +0200, 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.

Series looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

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

[Christoph Hellwig]

On Tue, Apr 19, 2022 at 02:10:35PM +0200, Klaus Jensen wrote:
> 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.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

Looks good:

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

Re: [PATCH 2/5] hw/nvme: always set eui64

[Christoph Hellwig]

On Tue, Apr 19, 2022 at 02:10:36PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Unconditionally set an EUI64 for namespaces. The nvme-ns device defaults
> to auto-generating a persistent EUI64 if not specified, but for single
> namespace setups (-device nvme,drive=...), this does not happen.
> 
> Since the EUI64 has previously been zeroed it is not considered valid,
> so it should be safe to add this now.
> 
> The generated EUI64 is of the form 52:54:00:<namespace counter>. Note,
> this is NOT the namespace identifier since that is not unique across
> subsystems; it is a global namespace counter. This has the effect that
> the value of this auto-generated EUI64 is dependent on the order with
> which the namespaces are created. If a more flexible setup is required,
> the eui64 namespace parameter should be explicitly set. Update the
> documentation to make this clear.

How is this actually globally unique given that it uses a start value
that is incremented for each created namespace?  Also EUI64 values are
based on a OUI, while NVME_EUI64_DEFAULT seems to have the OUI values
cleared to all zero as far as I can tell.

I would strongly advise againt autogenerating eui64 values.  They are
small and have little entropy, and require at least three bytes (for
new allocations more) to be set to a IEEE assigned OUI.

Re: [PATCH 3/5] hw/nvme: do not report null uuid

[Christoph Hellwig]

Looks good:

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

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

[Christoph Hellwig]

On Tue, Apr 19, 2022 at 02:10:38PM +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.

So unlike the EUI, UUIDs are designed to be autogenerated even if the
current algorithm is completely broken.  We'd just need to persist them.
Note that NVMe at least in theory requires providing at least on of
the unique identifiers, and the UUID is the only one designed to be
autogenerated in a distributed fashion.

Re: [PATCH 2/5] hw/nvme: always set eui64

[Klaus Jensen]

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

On Apr 20 07:30, Christoph Hellwig wrote:
> On Tue, Apr 19, 2022 at 02:10:36PM +0200, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Unconditionally set an EUI64 for namespaces. The nvme-ns device defaults
> > to auto-generating a persistent EUI64 if not specified, but for single
> > namespace setups (-device nvme,drive=...), this does not happen.
> > 
> > Since the EUI64 has previously been zeroed it is not considered valid,
> > so it should be safe to add this now.
> > 
> > The generated EUI64 is of the form 52:54:00:<namespace counter>. Note,
> > this is NOT the namespace identifier since that is not unique across
> > subsystems; it is a global namespace counter. This has the effect that
> > the value of this auto-generated EUI64 is dependent on the order with
> > which the namespaces are created. If a more flexible setup is required,
> > the eui64 namespace parameter should be explicitly set. Update the
> > documentation to make this clear.
> 
> How is this actually globally unique given that it uses a start value
> that is incremented for each created namespace?

I think it is as good as we can do when we cannot store the EUI64
persistently anywhere. The EUI64s will be unique to a single QEMU
instance. If someone wants to simulate a fabrics setup or something like
that, then, as per the documentation, set the EUI explicitly.

> Also EUI64 values are based on a OUI, while NVME_EUI64_DEFAULT seems
> to have the OUI values cleared to all zero as far as I can tell.
> 

It really should be a u8 array, yes, but won't the integer approach
work? The "template" is byte swapped to big endian, or am I off here?

> I would strongly advise againt autogenerating eui64 values.  They are
> small and have little entropy, and require at least three bytes (for
> new allocations more) to be set to a IEEE assigned OUI.

52:54:00 is a "private" OUI if I am not mistaken (something about some
bit being 1 or 0, cant remember the specifics) and is what QEMU uses
when an IEEE OUI is needed (MAC-addresses etc.). This is also what the
device uses in the IEEE field.

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

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

[Klaus Jensen]

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

On Apr 20 07:33, Christoph Hellwig wrote:
> On Tue, Apr 19, 2022 at 02:10:38PM +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.
> 
> So unlike the EUI, UUIDs are designed to be autogenerated even if the
> current algorithm is completely broken.  We'd just need to persist them.
> Note that NVMe at least in theory requires providing at least on of
> the unique identifiers, and the UUID is the only one designed to be
> autogenerated in a distributed fashion.

I understand, but it boils down to the fact that we do not have a
general method of storing "metadata" like this persistently.

But maybe it is time that we come up with something to do this.

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

Re: [PATCH 2/5] hw/nvme: always set eui64

[Klaus Jensen]

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

On Apr 20 07:48, Klaus Jensen wrote:
> On Apr 20 07:30, Christoph Hellwig wrote:
> > Also EUI64 values are based on a OUI, while NVME_EUI64_DEFAULT seems
> > to have the OUI values cleared to all zero as far as I can tell.
> > 
> 
> It really should be a u8 array, yes, but won't the integer approach
> work? The "template" is byte swapped to big endian, or am I off here?
> 

Nevermind. I see what you mean, I'll fix it up.

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

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

[Christoph Hellwig]

On Wed, Apr 20, 2022 at 07:51:32AM +0200, Klaus Jensen wrote:
> > So unlike the EUI, UUIDs are designed to be autogenerated even if the
> > current algorithm is completely broken.  We'd just need to persist them.
> > Note that NVMe at least in theory requires providing at least on of
> > the unique identifiers, and the UUID is the only one designed to be
> > autogenerated in a distributed fashion.
> 
> I understand, but it boils down to the fact that we do not have a
> general method of storing "metadata" like this persistently.
> 
> But maybe it is time that we come up with something to do this.

If we can't make the persistent uniqueue identifiers persistent and
unique, we should not provide them.  While NVMe does require a
namespace to report at least one of the three identifies, the failure
mode for now having one is much more graceful than providing one that
is not unique or not persistent.

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

[Klaus Jensen]

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

On Apr 20 08:53, Christoph Hellwig wrote:
> On Wed, Apr 20, 2022 at 07:51:32AM +0200, Klaus Jensen wrote:
> > > So unlike the EUI, UUIDs are designed to be autogenerated even if the
> > > current algorithm is completely broken.  We'd just need to persist them.
> > > Note that NVMe at least in theory requires providing at least on of
> > > the unique identifiers, and the UUID is the only one designed to be
> > > autogenerated in a distributed fashion.
> > 
> > I understand, but it boils down to the fact that we do not have a
> > general method of storing "metadata" like this persistently.
> > 
> > But maybe it is time that we come up with something to do this.
> 
> If we can't make the persistent uniqueue identifiers persistent and
> unique, we should not provide them.  While NVMe does require a
> namespace to report at least one of the three identifies, the failure
> mode for now having one is much more graceful than providing one that
> is not unique or not persistent.

Alright. I think we can do that. We can revert the eui64 defaulting as
well.

Thanks for your reviews/comments Christoph.

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