On Aug 19 10:04, Luigi Leonardi wrote: > The correct way to register an NVMe namespace in fw_boot_order is to use > the NVMe controller as the device pointer with the namespace ID appended > as a suffix, producing: > > /pci@i0cf8/pci1b36,0010@4/namespace@1,0 > > This is how the controller-level bootindex works. > Explicit nvme-ns devices instead register the nvme-ns device itself, > which is wrong: realize() reparents the namespace onto the NVMe subsystem > bus, disconnecting it from the controller in the bus hierarchy. > qdev_get_fw_dev_path() therefore produces a path without the controller's > address: > > /nvme-ns/namespace@1,0 > > Fix by re-registering in realize(), once the controller and final nsid > are known, using the controller as the device pointer with the namespace > suffix. The old entry (registered against the nvme-ns device) must be > removed first since add_boot_device_path() only removes entries matching > the same device pointer, and leaving both would cause a > duplicate-bootindex error. > > device_add_bootindex_property() cannot be called in realize() because > QEMU parses command-line properties before realize() runs, so the > bootindex property must exist by instance_init() time. The final nsid is > not yet known then either as it's assigned in realize(). > > The fix is restricted to non-shared namespaces: for shared namespaces > there is no single controller to register the boot path against. > > Also add the symmetric unrealize() cleanup so that hot-unplugging a > non-shared nvme-ns with a bootindex removes the fw_boot_order entry that > was registered against the controller in realize(). > > Reported-by: Gerd Hoffmann <[email protected]> > Signed-off-by: Luigi Leonardi <[email protected]> > --- > hw/nvme/ns.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c > index 7f0f9ac766..83786b4401 100644 > --- a/hw/nvme/ns.c > +++ b/hw/nvme/ns.c > @@ -727,6 +727,10 @@ static void nvme_ns_unrealize(DeviceState *dev) > nvme_ns_shutdown(ns); > nvme_ns_cleanup(ns); > > + if (!ns->params.shared && ns->bootindex >= 0) { > + del_boot_device_path(DEVICE(ns->ctrl), ns->bootindex_suffix); > + } > + > /* Symmetric with nvme_ns_realize() which sets subsys->namespaces[nsid]. > */ > if (subsys && nsid && subsys->namespaces[nsid] == ns) { > subsys->namespaces[nsid] = NULL; > @@ -891,6 +895,19 @@ static void nvme_ns_realize(DeviceState *dev, Error > **errp) > > if (!ns->params.shared) { > ns->ctrl = n; > + > + /* > + * Register the boot device path using the NVMe controller > + * so the OFW path includes the controller's PCI address: > + * /pci@i0cf8/pci1b36,0010@<slot>,0/namespace@<nsid>,0 > + */ > + if (ns->bootindex >= 0) { > + del_boot_device_path(dev, NULL); > + snprintf(ns->bootindex_suffix, sizeof(ns->bootindex_suffix), > + "/namespace@%" PRIu32 ",0", nsid); > + add_boot_device_path(ns->bootindex, DEVICE(n), > + ns->bootindex_suffix); > + } > } > } > > @@ -1117,10 +1134,8 @@ static void nvme_ns_instance_init(Object *obj) > { > NvmeNamespace *ns = NVME_NS(obj); > > - sprintf(ns->bootindex_suffix, "/namespace@%" PRIu32 ",0", > ns->params.nsid); > - > device_add_bootindex_property(obj, &ns->bootindex, "bootindex", > - ns->bootindex_suffix, DEVICE(obj)); > + NULL, DEVICE(obj)); > } > > static const TypeInfo nvme_ns_info = { > > --- > base-commit: 9696bf5dc5a5bf0b4a9d05b6cdfe5f13990f97aa > change-id: 20260818-fix_nvme-d8cfaa3368c9 > > Best regards, > -- > Luigi Leonardi <[email protected]> > >
LGTM. Thanks! Reviewed-by: Klaus Jensen <[email protected]>
signature.asc
Description: PGP signature
