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

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

+cc qemu-block@nongnu.org

On 6/9/21 1:46 PM, 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
>