* [PATCH 1/1] hw/nvme: namespace parameter for EUI64
@ 2021-06-09 11:46 Heinrich Schuchardt
2021-06-09 12:14 ` Klaus Jensen
2021-06-09 12:26 ` Heinrich Schuchardt
0 siblings, 2 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-06-09 11:46 UTC (permalink / raw
To: qemu-devel
Cc: Keith Busch, Klaus Jensen, Philippe Mathieu-Daudé,
Heinrich Schuchardt, Peter Maydell
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 11:46 [PATCH 1/1] hw/nvme: namespace parameter for EUI64 Heinrich Schuchardt
@ 2021-06-09 12:14 ` Klaus Jensen
2021-06-09 12:21 ` Heinrich Schuchardt
2021-06-09 12:26 ` Heinrich Schuchardt
1 sibling, 1 reply; 12+ messages in thread
From: Klaus Jensen @ 2021-06-09 12:14 UTC (permalink / raw
To: Heinrich Schuchardt
Cc: Keith Busch, Klaus Jensen, Philippe Mathieu-Daudé,
qemu-devel, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 5492 bytes --]
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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 12:14 ` Klaus Jensen
@ 2021-06-09 12:21 ` Heinrich Schuchardt
2021-06-09 12:33 ` Klaus Jensen
0 siblings, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-06-09 12:21 UTC (permalink / raw
To: Klaus Jensen
Cc: Keith Busch, Klaus Jensen, Philippe Mathieu-Daudé,
qemu-devel, Peter Maydell
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.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 11:46 [PATCH 1/1] hw/nvme: namespace parameter for EUI64 Heinrich Schuchardt
2021-06-09 12:14 ` Klaus Jensen
@ 2021-06-09 12:26 ` Heinrich Schuchardt
1 sibling, 0 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-06-09 12:26 UTC (permalink / raw
To: qemu-devel, qemu-block
Cc: Keith Busch, Klaus Jensen, Philippe Mathieu-Daudé,
Peter Maydell
+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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 12:21 ` Heinrich Schuchardt
@ 2021-06-09 12:33 ` Klaus Jensen
2021-06-09 14:32 ` Heinrich Schuchardt
2021-06-09 14:39 ` Daniel P. Berrangé
0 siblings, 2 replies; 12+ messages in thread
From: Klaus Jensen @ 2021-06-09 12:33 UTC (permalink / raw
To: Heinrich Schuchardt
Cc: Keith Busch, Klaus Jensen, Philippe Mathieu-Daudé,
qemu-devel, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 6786 bytes --]
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'.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 12:33 ` Klaus Jensen
@ 2021-06-09 14:32 ` Heinrich Schuchardt
2021-06-09 14:39 ` Daniel P. Berrangé
1 sibling, 0 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-06-09 14:32 UTC (permalink / raw
To: Klaus Jensen
Cc: Keith Busch, Klaus Jensen, Philippe Mathieu-Daudé,
qemu-devel, Peter Maydell
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 12:33 ` Klaus Jensen
2021-06-09 14:32 ` Heinrich Schuchardt
@ 2021-06-09 14:39 ` Daniel P. Berrangé
2021-06-09 18:13 ` Heinrich Schuchardt
1 sibling, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2021-06-09 14:39 UTC (permalink / raw
To: Klaus Jensen
Cc: Peter Maydell, Heinrich Schuchardt, qemu-devel, Klaus Jensen,
Keith Busch, Philippe Mathieu-Daudé
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.
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 14:39 ` Daniel P. Berrangé
@ 2021-06-09 18:13 ` Heinrich Schuchardt
2021-06-09 19:57 ` Klaus Jensen
0 siblings, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-06-09 18:13 UTC (permalink / raw
To: Daniel P. Berrangé, Klaus Jensen
Cc: Keith Busch, Klaus Jensen, Philippe Mathieu-Daudé,
qemu-devel, Peter Maydell
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 18:13 ` Heinrich Schuchardt
@ 2021-06-09 19:57 ` Klaus Jensen
2021-06-09 20:15 ` Heinrich Schuchardt
0 siblings, 1 reply; 12+ messages in thread
From: Klaus Jensen @ 2021-06-09 19:57 UTC (permalink / raw
To: Heinrich Schuchardt
Cc: Peter Maydell, Daniel P. Berrangé, Klaus Jensen, qemu-devel,
Keith Busch, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 9177 bytes --]
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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 19:57 ` Klaus Jensen
@ 2021-06-09 20:15 ` Heinrich Schuchardt
2021-06-10 5:31 ` Klaus Jensen
0 siblings, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2021-06-09 20:15 UTC (permalink / raw
To: Klaus Jensen
Cc: Peter Maydell, Daniel P. Berrangé, Klaus Jensen, qemu-devel,
Keith Busch, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-09 20:15 ` Heinrich Schuchardt
@ 2021-06-10 5:31 ` Klaus Jensen
2021-06-10 8:28 ` Daniel P. Berrangé
0 siblings, 1 reply; 12+ messages in thread
From: Klaus Jensen @ 2021-06-10 5:31 UTC (permalink / raw
To: Heinrich Schuchardt
Cc: Peter Maydell, Daniel P. Berrangé, Klaus Jensen, qemu-devel,
Keith Busch, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 12160 bytes --]
On Jun 9 22:15, Heinrich Schuchardt wrote:
>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.
I don't think that is obscure. This is exactly why machine types are
versioned. It is documented as a feature to ensure working live
migration between versions, but it is definitely also useful for just
making sure that no behavior changes between qemu upgrades.
We have used this feature in the past to change the PCI Vendor/Device ID
of the device.
>
>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.
>
From an NVMe perspective we are not changing it. We are going from "not
supported" to "supported". But I acknowledge that there are systems that
rely on EUI64 being zero - I just don't see why that should refrain us
from adding EUI64 and NGUID by default in future versions when we can
ensure compatibility with the versioned machine type (i.e. virt-6.0).
>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?
>
These are all good points.
Considering that, I understand the need for an eui64 parameter (and
maybe one for nguid as well) to explicitly control the field values.
But in the absence of these parameters, it would be nice to provide a
sensible default in future versions, e.g.:
-device nvme-ns,uuid=00000000-0000-0000-0000-deadbeefd0de
ieee oui : 0x525400
uuid : 0x00000000000000000000deadbeefd0de
eui64 : 0x525400adbeefd0de
nguid : 0x0000deadbeefd0de525400adbeefd0de
If the user explicitly requires the pre-6.1 behavior of a zero EUI64 or
NGUID, the behavior may be explicitly reverted using compat properties,
(e.g. 'support-eui64=no' and 'support-nguid=no') or implicitly by using
a 6.0 machine type.
Anyway, I'm not sure we are getting any closer to agreeing on that, so
for now I have no problems with accepting your patch as-is. I can follow
up with a patch for my suggestion and we can take it from there.
Acked-by: Klaus Jensen <k.jensen@samsung.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] hw/nvme: namespace parameter for EUI64
2021-06-10 5:31 ` Klaus Jensen
@ 2021-06-10 8:28 ` Daniel P. Berrangé
0 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2021-06-10 8:28 UTC (permalink / raw
To: Klaus Jensen
Cc: Peter Maydell, Heinrich Schuchardt, qemu-devel, Klaus Jensen,
Keith Busch, Philippe Mathieu-Daudé
On Thu, Jun 10, 2021 at 07:31:32AM +0200, Klaus Jensen wrote:
> On Jun 9 22:15, Heinrich Schuchardt wrote:
> > 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:
> > > > > > > > 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.
>
> I don't think that is obscure. This is exactly why machine types are
> versioned. It is documented as a feature to ensure working live migration
> between versions, but it is definitely also useful for just making sure that
> no behavior changes between qemu upgrades.
>
> We have used this feature in the past to change the PCI Vendor/Device ID of
> the device.
>
> >
> > 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.
> >
>
> From an NVMe perspective we are not changing it. We are going from "not
> supported" to "supported". But I acknowledge that there are systems that
> rely on EUI64 being zero - I just don't see why that should refrain us from
> adding EUI64 and NGUID by default in future versions when we can ensure
> compatibility with the versioned machine type (i.e. virt-6.0).
Yes, the whole point of versioned machine types is that they let us fix
bugs and add features to device implementations, while maintaining back
compat. So going from no-EUI64 to EUI64 by default in a new machine
type version is exactly the kind of thing that is intended to happen.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-06-10 8:31 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-09 11:46 [PATCH 1/1] hw/nvme: namespace parameter for EUI64 Heinrich Schuchardt
2021-06-09 12:14 ` Klaus Jensen
2021-06-09 12:21 ` Heinrich Schuchardt
2021-06-09 12:33 ` Klaus Jensen
2021-06-09 14:32 ` Heinrich Schuchardt
2021-06-09 14:39 ` Daniel P. Berrangé
2021-06-09 18:13 ` Heinrich Schuchardt
2021-06-09 19:57 ` Klaus Jensen
2021-06-09 20:15 ` Heinrich Schuchardt
2021-06-10 5:31 ` Klaus Jensen
2021-06-10 8:28 ` Daniel P. Berrangé
2021-06-09 12:26 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.