qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64


From: Heinrich Schuchardt
Subject: Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
Date: Wed, 9 Jun 2021 16:32:03 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0

On 6/9/21 2:33 PM, Klaus Jensen wrote:
On Jun  9 14:21, Heinrich Schuchardt wrote:
On 6/9/21 2:14 PM, Klaus Jensen wrote:
On Jun  9 13:46, Heinrich Schuchardt wrote:
The EUI64 field is the only identifier for NVMe namespaces in UEFI
device
paths. Add a new namespace property "eui64", that provides the user the
option to specify the EUI64.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
docs/system/nvme.rst |  4 +++
hw/nvme/ctrl.c       | 58 ++++++++++++++++++++++++++------------------
hw/nvme/ns.c         |  2 ++
hw/nvme/nvme.h       |  1 +
4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst
index f7f63d6bf6..a6042f942a 100644
--- a/docs/system/nvme.rst
+++ b/docs/system/nvme.rst
@@ -81,6 +81,10 @@ There are a number of parameters available:
  Set the UUID of the namespace. This will be reported as a "Namespace
UUID"
  descriptor in the Namespace Identification Descriptor List.

+``eui64``
+  Set the EUI64 of the namespace. This will be reported as a "IEEE
Extended
+  Unique Identifier" descriptor in the Namespace Identification
Descriptor List.
+
``bus``
  If there are more ``nvme`` devices defined, this parameter may be
used to
  attach the namespace to a specific ``nvme`` device (identified by an
``id``
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0bcaf7192f..21f2d6843b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4426,19 +4426,19 @@ static uint16_t
nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
    NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
    uint32_t nsid = le32_to_cpu(c->nsid);
    uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
-
-    struct data {
-        struct {
-            NvmeIdNsDescr hdr;
-            uint8_t v[NVME_NIDL_UUID];
-        } uuid;
-        struct {
-            NvmeIdNsDescr hdr;
-            uint8_t v;
-        } csi;
-    };
-
-    struct data *ns_descrs = (struct data *)list;
+    uint8_t *pos = list;
+    struct {
+        NvmeIdNsDescr hdr;
+        uint8_t v[NVME_NIDL_UUID];
+    } QEMU_PACKED uuid;
+    struct {
+        NvmeIdNsDescr hdr;
+        uint64_t v;
+    } QEMU_PACKED eui64;
+    struct {
+        NvmeIdNsDescr hdr;
+        uint8_t v;
+    } QEMU_PACKED csi;

    trace_pci_nvme_identify_ns_descr_list(nsid);

@@ -4452,17 +4452,29 @@ static uint16_t
nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req)
    }

    /*
-     * Because the NGUID and EUI64 fields are 0 in the Identify
Namespace data
-     * structure, a Namespace UUID (nidt = 3h) must be reported in the
-     * Namespace Identification Descriptor. Add the namespace UUID
here.
+     * If the EUI64 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.
     */
-    ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
-    ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID;
-    memcpy(&ns_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID);
-
-    ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI;
-    ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI;
-    ns_descrs->csi.v = ns->csi;
+    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;
+        eui64.hdr.nidl = NVME_NIDL_EUI64;
+        eui64.v = cpu_to_be64(ns->params.eui64);
+        memcpy(pos, &eui64, sizeof(eui64));
+        pos += sizeof(eui64);
+    }
+
+    csi.hdr.nidt = NVME_NIDT_CSI;
+    csi.hdr.nidl = NVME_NIDL_CSI;
+    csi.v = ns->csi;
+    memcpy(pos, &csi, sizeof(csi));
+    pos += sizeof(csi);

    return nvme_c2h(n, list, sizeof(list), req);
}
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 992e5a13f5..ddf395d60e 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -77,6 +77,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error
**errp)
    id_ns->mssrl = cpu_to_le16(ns->params.mssrl);
    id_ns->mcl = cpu_to_le32(ns->params.mcl);
    id_ns->msrc = ns->params.msrc;
+    id_ns->eui64 = cpu_to_be64(ns->params.eui64);

    ds = 31 - clz32(ns->blkconf.logical_block_size);
    ms = ns->params.ms;
@@ -518,6 +519,7 @@ static Property nvme_ns_props[] = {
    DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false),
    DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
    DEFINE_PROP_UUID("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),
    DEFINE_PROP_UINT8("pi", NvmeNamespace, params.pi, 0),
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 81a35cda14..abe7fab21c 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -83,6 +83,7 @@ typedef struct NvmeNamespaceParams {
    bool     shared;
    uint32_t nsid;
    QemuUUID uuid;
+    uint64_t eui64;

    uint16_t ms;
    uint8_t  mset;
--
2.30.2



Would it make sense to provide a sensible default for EUI64 such that it
is always there? That is, using the same IEEE OUI as we already report
in the IEEE field and add an extension identifier by grabbing 5 bytes
from the UUID?

According to the NVMe 1.4 specification it is allowable to have a NVMe
device that does not support EUI64. As the EUI64 is used to define boot
options in UEFI using a non-zero default may break existing
installations.


Right. We dont wanna do that.

My knowledge of UEFI is very limited, but, since a value of '0' for
EUI64 means "not supported", isn't it wrong for UEFI implementations to
have used it as a valid identifier? Prior to this patch, if there were
two namespaces they would both have an EUI64 of '0'.

All NVMe devices I have bought had an EUI64. I assume missing EUI64
support is more or less a QEMU only issue. I don't expect that the UEFI
specification will be changed to work around it.

According to a 2016 bug report this missing EUI64 support is an issue in
Windows too:

[Qemu-devel] [Bug 1576347] Re: Only one NVMe device is usable in Windows
https://lists.gnu.org/archive/html/qemu-devel/2016-12/msg01486.html

Only one NVMe device is usable in Windows (10) guest
https://bugs.launchpad.net/qemu/+bug/1576347

Best regards

Heinrich



reply via email to

[Prev in Thread] Current Thread [Next in Thread]