| Message ID | 20220419121039.1259477-3-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | hw/nvme: fix namespace identifiers | expand |
On Tue, Apr 19, 2022 at 02:10:36PM +0200, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Unconditionally set an EUI64 for namespaces. The nvme-ns device defaults > to auto-generating a persistent EUI64 if not specified, but for single > namespace setups (-device nvme,drive=...), this does not happen. > > Since the EUI64 has previously been zeroed it is not considered valid, > so it should be safe to add this now. > > The generated EUI64 is of the form 52:54:00:<namespace counter>. Note, > this is NOT the namespace identifier since that is not unique across > subsystems; it is a global namespace counter. This has the effect that > the value of this auto-generated EUI64 is dependent on the order with > which the namespaces are created. If a more flexible setup is required, > the eui64 namespace parameter should be explicitly set. Update the > documentation to make this clear. How is this actually globally unique given that it uses a start value that is incremented for each created namespace? Also EUI64 values are based on a OUI, while NVME_EUI64_DEFAULT seems to have the OUI values cleared to all zero as far as I can tell. I would strongly advise againt autogenerating eui64 values. They are small and have little entropy, and require at least three bytes (for new allocations more) to be set to a IEEE assigned OUI.
On Apr 20 07:30, Christoph Hellwig wrote: > On Tue, Apr 19, 2022 at 02:10:36PM +0200, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > Unconditionally set an EUI64 for namespaces. The nvme-ns device defaults > > to auto-generating a persistent EUI64 if not specified, but for single > > namespace setups (-device nvme,drive=...), this does not happen. > > > > Since the EUI64 has previously been zeroed it is not considered valid, > > so it should be safe to add this now. > > > > The generated EUI64 is of the form 52:54:00:<namespace counter>. Note, > > this is NOT the namespace identifier since that is not unique across > > subsystems; it is a global namespace counter. This has the effect that > > the value of this auto-generated EUI64 is dependent on the order with > > which the namespaces are created. If a more flexible setup is required, > > the eui64 namespace parameter should be explicitly set. Update the > > documentation to make this clear. > > How is this actually globally unique given that it uses a start value > that is incremented for each created namespace? I think it is as good as we can do when we cannot store the EUI64 persistently anywhere. The EUI64s will be unique to a single QEMU instance. If someone wants to simulate a fabrics setup or something like that, then, as per the documentation, set the EUI explicitly. > Also EUI64 values are based on a OUI, while NVME_EUI64_DEFAULT seems > to have the OUI values cleared to all zero as far as I can tell. > It really should be a u8 array, yes, but won't the integer approach work? The "template" is byte swapped to big endian, or am I off here? > I would strongly advise againt autogenerating eui64 values. They are > small and have little entropy, and require at least three bytes (for > new allocations more) to be set to a IEEE assigned OUI. 52:54:00 is a "private" OUI if I am not mistaken (something about some bit being 1 or 0, cant remember the specifics) and is what QEMU uses when an IEEE OUI is needed (MAC-addresses etc.). This is also what the device uses in the IEEE field.
On Apr 20 07:48, Klaus Jensen wrote: > On Apr 20 07:30, Christoph Hellwig wrote: > > Also EUI64 values are based on a OUI, while NVME_EUI64_DEFAULT seems > > to have the OUI values cleared to all zero as far as I can tell. > > > > It really should be a u8 array, yes, but won't the integer approach > work? The "template" is byte swapped to big endian, or am I off here? > Nevermind. I see what you mean, I'll fix it up.
diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst index b5acb2a9c19d..f9b8c7f3eeb4 100644 --- a/docs/system/devices/nvme.rst +++ b/docs/system/devices/nvme.rst @@ -84,8 +84,10 @@ There are a number of parameters available: ``eui64`` Set the EUI-64 of the namespace. This will be reported as a "IEEE Extended Unique Identifier" descriptor in the Namespace Identification Descriptor List. - Since machine type 6.1 a non-zero default value is used if the parameter - is not provided. For earlier machine types the field defaults to 0. + Since machine type 6.1, an EUI-64 is auto-generated. However, note that this + auto-generated value is dependent on the order with which the namespaces are + created. If you intend on adding/removing namespaces on your VM, set this + parameter explicitly. For earlier machine types the field defaults to 0. ``bus`` If there are more ``nvme`` devices defined, this parameter may be used to diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 03760ddeae8c..17cf9862ab89 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -6862,6 +6862,8 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) ns = &n->namespace; ns->params.nsid = 1; + nvme_ns_set_eui64(ns); + if (nvme_ns_setup(ns, errp)) { return; } diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c index 324f53ea0cd1..5685221f47c6 100644 --- a/hw/nvme/ns.c +++ b/hw/nvme/ns.c @@ -25,6 +25,8 @@ #define MIN_DISCARD_GRANULARITY (4 * KiB) #define NVME_DEFAULT_ZONE_SIZE (128 * MiB) +uint64_t nvme_ns_count; + void nvme_ns_init_format(NvmeNamespace *ns) { NvmeIdNs *id_ns = &ns->id_ns; @@ -54,9 +56,13 @@ void nvme_ns_init_format(NvmeNamespace *ns) id_ns->npda = id_ns->npdg = npdg - 1; } +void nvme_ns_set_eui64(NvmeNamespace *ns) +{ + ns->params.eui64 = NVME_EUI64_DEFAULT | ++nvme_ns_count; +} + static int nvme_ns_init(NvmeNamespace *ns, Error **errp) { - static uint64_t ns_count; NvmeIdNs *id_ns = &ns->id_ns; NvmeIdNsNvm *id_ns_nvm = &ns->id_ns_nvm; uint8_t ds; @@ -75,10 +81,8 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp) id_ns->nmic |= NVME_NMIC_NS_SHARED; } - /* Substitute a missing EUI-64 by an autogenerated one */ - ++ns_count; if (!ns->params.eui64 && ns->params.eui64_default) { - ns->params.eui64 = ns_count + NVME_EUI64_DEFAULT; + nvme_ns_set_eui64(ns); } /* simple copy */ diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h index 7f2e8f1b6491..3922cf531f6d 100644 --- a/hw/nvme/nvme.h +++ b/hw/nvme/nvme.h @@ -33,6 +33,8 @@ QEMU_BUILD_BUG_ON(NVME_MAX_NAMESPACES > NVME_NSID_BROADCAST - 1); typedef struct NvmeCtrl NvmeCtrl; typedef struct NvmeNamespace NvmeNamespace; +extern uint64_t nvme_ns_count; + #define TYPE_NVME_BUS "nvme-bus" OBJECT_DECLARE_SIMPLE_TYPE(NvmeBus, NVME_BUS) @@ -511,6 +513,7 @@ static inline uint16_t nvme_cid(NvmeRequest *req) return le16_to_cpu(req->cqe.cid); } +void nvme_ns_set_eui64(NvmeNamespace *ns); void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns); uint16_t nvme_bounce_data(NvmeCtrl *n, void *ptr, uint32_t len, NvmeTxDirection dir, NvmeRequest *req);