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, 09 Jun 2021 20:13:40 +0200
User-agent: K-9 Mail for Android

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.

Best regards

Heinrich

>
>IOW, it would be possible to set a non-zero EUI if it only gets set
>for 6.1.0 machine types and later.
>
>Regards,
>Daniel




reply via email to

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