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

[Heinrich Schuchardt]

Am 9. Juni 2021 21:57:26 MESZ schrieb Klaus Jensen <its@irrelevant.dk>:
>On Jun  9 20:13, Heinrich Schuchardt wrote:
>>Am 9. Juni 2021 16:39:20 MESZ schrieb "Daniel P. Berrangé"
><berrange@redhat.com>:
>>>On Wed, Jun 09, 2021 at 02:33:08PM +0200, 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.
>>>
>>>Any change in defaults can (must) be tied to the machine type
>versions,
>>>so that existing installs are unchanged, but new installs using
>latest
>>>machine type get the new default.
>>
>>The road of least surprise is preferable. All machine should behave
>the 
>>same if there is no real necessity to deviate.
>>
>
>I'd rather not introduce a new user-facing knob for this when a very 
>sensible default can be derived from the uuid and the QEMU IEEE OUI. We
>
>already have the uuid parameter that allows users to ensure that the 
>namespace holds a persistent unique identifier. Adding another
>parameter 
>with the same purpose seems unnecessary. And since we are adding EUI64,
>
>we should probably also add NGUID while we are at it.
>
>So, instead of adding parameters for EUI64 and NGUID that the user must
>
>specify to get this more real-world behavior, I'd prefer to rely on a 
>couple of boolean compat properties, e.g. 'support-eui64' and 
>'support-nguid' that defaults to 'on', but is set to 'off' for pre-6.1 
>machines.
>
>I think this is a nice compromise between making sure that having 
>sensible EUI64 and NGUID values set is the new default while making
>sure 
>that we do not break existing setup.
>
>Would this be an acceptable compromise to you Heinrich?

EUI64 defined on some machine and not on others is totally obscure for users.

The virt machine is typically used and is pre-6.1. As pointed out above you 
should not change the EUI64 when QEMU is upgraded and invoked with the same 
parameter set.

EUI64 is what UEFI and Windows rely on. Why should a user specify a UUID if he 
needs a EUI64?

There is no collisionfree mapping of 16 byte UUIDs to 8 byte EUI64s. So if a 
user specifies different UUIDs he may still get conflicting EUI64s.

How should a user find out which UUID he has to specify to get the EUI64 he 
wants to set?

If you want to remove the uuid parameter, I don't care. But EUI64 is what we 
need.

Best regards

Heinrich