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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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