| Message ID | 20210122170157.246374-1-groug@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: Adjust firmware path of PCI devices | expand |
On 1/22/21 2:01 PM, Greg Kurz wrote: > It is currently not possible to perform a strict boot from USB storage: > > $ qemu-system-ppc64 -accel kvm -nodefaults -nographic -serial stdio \ > -boot strict=on \ > -device qemu-xhci \ > -device usb-storage,drive=disk,bootindex=0 \ > -blockdev driver=file,node-name=disk,filename=fedora-ppc64le.qcow2 > > > SLOF ********************************************************************** > QEMU Starting > Build Date = Jul 17 2020 11:15:24 > FW Version = git-e18ddad8516ff2cf > Press "s" to enter Open Firmware. > > Populating /vdevice methods > Populating /vdevice/vty@71000000 > Populating /vdevice/nvram@71000001 > Populating /pci@800000020000000 > 00 0000 (D) : 1b36 000d serial bus [ usb-xhci ] > No NVRAM common partition, re-initializing... > Scanning USB > XHCI: Initializing > USB Storage > SCSI: Looking for devices > 101000000000000 DISK : "QEMU QEMU HARDDISK 2.5+" > Using default console: /vdevice/vty@71000000 > > Welcome to Open Firmware > > Copyright (c) 2004, 2017 IBM Corporation All rights reserved. > This program and the accompanying materials are made available > under the terms of the BSD License available at > http://www.opensource.org/licenses/bsd-license.php > > > Trying to load: from: /pci@800000020000000/usb@0/storage@1/disk@101000000000000 ... > E3405: No such device > > E3407: Load failed > > Type 'boot' and press return to continue booting the system. > Type 'reset-all' and press return to reboot the system. > > > Ready! > 0 > > > The device tree handed over by QEMU to SLOF indeed contains: > > qemu,boot-list = > "/pci@800000020000000/usb@0/storage@1/disk@101000000000000 HALT"; > > but the device node is named usb-xhci@0, not usb@0. > > This happens because the firmware names of PCI devices returned > by get_boot_devices_list() come from pcibus_get_fw_dev_path(), > while the sPAPR PHB code uses a different naming scheme for > device nodes. This inconsistency has always been there but it was > hidden for a long time because SLOF used to rename USB device > nodes, until this commit, merged in QEMU 4.2.0 : > > commit 85164ad4ed9960cac842fa4cc067c6b6699b0994 > Author: Alexey Kardashevskiy <aik@ozlabs.ru> > Date: Wed Sep 11 16:24:32 2019 +1000 > > pseries: Update SLOF firmware image > > This fixes USB host bus adapter name in the device tree to match QEMU's > one. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > Fortunately, sPAPR implements the firmware path provider interface. > This provides a way to override the default firmware paths. > > Just factor out the sPAPR PHB naming logic from spapr_dt_pci_device() > to a helper, and use it in the sPAPR firmware path provider hook. > > Fixes: 85164ad4ed99 ("pseries: Update SLOF firmware image") > Signed-off-by: Greg Kurz <groug@kaod.org> > --- Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> > include/hw/pci-host/spapr.h | 2 ++ > hw/ppc/spapr.c | 5 +++++ > hw/ppc/spapr_pci.c | 33 ++++++++++++++++++--------------- > 3 files changed, 25 insertions(+), 15 deletions(-) > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > index bd014823a933..5b03a7b0eb3f 100644 > --- a/include/hw/pci-host/spapr.h > +++ b/include/hw/pci-host/spapr.h > @@ -210,4 +210,6 @@ static inline unsigned spapr_phb_windows_supported(SpaprPhbState *sphb) > return sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1; > } > > +char *spapr_pci_fw_dev_name(PCIDevice *dev); > + > #endif /* PCI_HOST_SPAPR_H */ > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 6ab27ea269d5..632502c2ecf8 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3048,6 +3048,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, > SCSIDevice *d = CAST(SCSIDevice, dev, TYPE_SCSI_DEVICE); > SpaprPhbState *phb = CAST(SpaprPhbState, dev, TYPE_SPAPR_PCI_HOST_BRIDGE); > VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, TYPE_VHOST_SCSI_COMMON); > + PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE); > > if (d) { > void *spapr = CAST(void, bus->parent, "spapr-vscsi"); > @@ -3121,6 +3122,10 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, > return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn)); > } > > + if (pcidev) { > + return spapr_pci_fw_dev_name(pcidev); > + } > + > return NULL; > } > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 76d7c91e9c64..da6eb58724c8 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1334,15 +1334,29 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus, > return offset; > } > > +char *spapr_pci_fw_dev_name(PCIDevice *dev) > +{ > + const gchar *basename; > + int slot = PCI_SLOT(dev->devfn); > + int func = PCI_FUNC(dev->devfn); > + uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3); > + > + basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, > + ccode & 0xff); > + > + if (func != 0) { > + return g_strdup_printf("%s@%x,%x", basename, slot, func); > + } else { > + return g_strdup_printf("%s@%x", basename, slot); > + } > +} > + > /* create OF node for pci device and required OF DT properties */ > static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev, > void *fdt, int parent_offset) > { > int offset; > - const gchar *basename; > - gchar *nodename; > - int slot = PCI_SLOT(dev->devfn); > - int func = PCI_FUNC(dev->devfn); > + g_autofree gchar *nodename = spapr_pci_fw_dev_name(dev); > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > ResourceProps rp; > SpaprDrc *drc = drc_from_dev(sphb, dev); > @@ -1359,19 +1373,8 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev, > uint32_t pci_status = pci_default_read_config(dev, PCI_STATUS, 2); > gchar *loc_code; > > - basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, > - ccode & 0xff); > - > - if (func != 0) { > - nodename = g_strdup_printf("%s@%x,%x", basename, slot, func); > - } else { > - nodename = g_strdup_printf("%s@%x", basename, slot); > - } > - > _FDT(offset = fdt_add_subnode(fdt, parent_offset, nodename)); > > - g_free(nodename); > - > /* in accordance with PAPR+ v2.7 13.6.3, Table 181 */ > _FDT(fdt_setprop_cell(fdt, offset, "vendor-id", vendor_id)); > _FDT(fdt_setprop_cell(fdt, offset, "device-id", device_id)); >
On 23/01/2021 04:01, Greg Kurz wrote: > It is currently not possible to perform a strict boot from USB storage: > > $ qemu-system-ppc64 -accel kvm -nodefaults -nographic -serial stdio \ > -boot strict=on \ > -device qemu-xhci \ > -device usb-storage,drive=disk,bootindex=0 \ > -blockdev driver=file,node-name=disk,filename=fedora-ppc64le.qcow2 > > > SLOF ********************************************************************** > QEMU Starting > Build Date = Jul 17 2020 11:15:24 > FW Version = git-e18ddad8516ff2cf > Press "s" to enter Open Firmware. > > Populating /vdevice methods > Populating /vdevice/vty@71000000 > Populating /vdevice/nvram@71000001 > Populating /pci@800000020000000 > 00 0000 (D) : 1b36 000d serial bus [ usb-xhci ] > No NVRAM common partition, re-initializing... > Scanning USB > XHCI: Initializing > USB Storage > SCSI: Looking for devices > 101000000000000 DISK : "QEMU QEMU HARDDISK 2.5+" > Using default console: /vdevice/vty@71000000 > > Welcome to Open Firmware > > Copyright (c) 2004, 2017 IBM Corporation All rights reserved. > This program and the accompanying materials are made available > under the terms of the BSD License available at > http://www.opensource.org/licenses/bsd-license.php > > > Trying to load: from: /pci@800000020000000/usb@0/storage@1/disk@101000000000000 ... > E3405: No such device > > E3407: Load failed > > Type 'boot' and press return to continue booting the system. > Type 'reset-all' and press return to reboot the system. > > > Ready! > 0 > > > The device tree handed over by QEMU to SLOF indeed contains: > > qemu,boot-list = > "/pci@800000020000000/usb@0/storage@1/disk@101000000000000 HALT"; > > but the device node is named usb-xhci@0, not usb@0. I'd expect it to be a return of qdev_fw_name() so in this case something like "nec-usb-xhci" (which would still be broken) but seeing a plain "usb" is a bit strange. While your patch works, I wonder if we should assign fw_name to all pci nodes to avoid similar problems in the future? Thanks, > > This happens because the firmware names of PCI devices returned > by get_boot_devices_list() come from pcibus_get_fw_dev_path(), > while the sPAPR PHB code uses a different naming scheme for > device nodes. This inconsistency has always been there but it was > hidden for a long time because SLOF used to rename USB device > nodes, until this commit, merged in QEMU 4.2.0 : > > commit 85164ad4ed9960cac842fa4cc067c6b6699b0994 > Author: Alexey Kardashevskiy <aik@ozlabs.ru> > Date: Wed Sep 11 16:24:32 2019 +1000 > > pseries: Update SLOF firmware image > > This fixes USB host bus adapter name in the device tree to match QEMU's > one. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > Fortunately, sPAPR implements the firmware path provider interface. > This provides a way to override the default firmware paths. > > Just factor out the sPAPR PHB naming logic from spapr_dt_pci_device() > to a helper, and use it in the sPAPR firmware path provider hook. > > Fixes: 85164ad4ed99 ("pseries: Update SLOF firmware image") > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > include/hw/pci-host/spapr.h | 2 ++ > hw/ppc/spapr.c | 5 +++++ > hw/ppc/spapr_pci.c | 33 ++++++++++++++++++--------------- > 3 files changed, 25 insertions(+), 15 deletions(-) > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > index bd014823a933..5b03a7b0eb3f 100644 > --- a/include/hw/pci-host/spapr.h > +++ b/include/hw/pci-host/spapr.h > @@ -210,4 +210,6 @@ static inline unsigned spapr_phb_windows_supported(SpaprPhbState *sphb) > return sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1; > } > > +char *spapr_pci_fw_dev_name(PCIDevice *dev); > + > #endif /* PCI_HOST_SPAPR_H */ > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 6ab27ea269d5..632502c2ecf8 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3048,6 +3048,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, > SCSIDevice *d = CAST(SCSIDevice, dev, TYPE_SCSI_DEVICE); > SpaprPhbState *phb = CAST(SpaprPhbState, dev, TYPE_SPAPR_PCI_HOST_BRIDGE); > VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, TYPE_VHOST_SCSI_COMMON); > + PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE); > > if (d) { > void *spapr = CAST(void, bus->parent, "spapr-vscsi"); > @@ -3121,6 +3122,10 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, > return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn)); > } > > + if (pcidev) { > + return spapr_pci_fw_dev_name(pcidev); > + } > + > return NULL; > } > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 76d7c91e9c64..da6eb58724c8 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1334,15 +1334,29 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus, > return offset; > } > > +char *spapr_pci_fw_dev_name(PCIDevice *dev) > +{ > + const gchar *basename; > + int slot = PCI_SLOT(dev->devfn); > + int func = PCI_FUNC(dev->devfn); > + uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3); > + > + basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, > + ccode & 0xff); > + > + if (func != 0) { > + return g_strdup_printf("%s@%x,%x", basename, slot, func); > + } else { > + return g_strdup_printf("%s@%x", basename, slot); > + } > +} > + > /* create OF node for pci device and required OF DT properties */ > static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev, > void *fdt, int parent_offset) > { > int offset; > - const gchar *basename; > - gchar *nodename; > - int slot = PCI_SLOT(dev->devfn); > - int func = PCI_FUNC(dev->devfn); > + g_autofree gchar *nodename = spapr_pci_fw_dev_name(dev); > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > ResourceProps rp; > SpaprDrc *drc = drc_from_dev(sphb, dev); > @@ -1359,19 +1373,8 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev, > uint32_t pci_status = pci_default_read_config(dev, PCI_STATUS, 2); > gchar *loc_code; > > - basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, > - ccode & 0xff); > - > - if (func != 0) { > - nodename = g_strdup_printf("%s@%x,%x", basename, slot, func); > - } else { > - nodename = g_strdup_printf("%s@%x", basename, slot); > - } > - > _FDT(offset = fdt_add_subnode(fdt, parent_offset, nodename)); > > - g_free(nodename); > - > /* in accordance with PAPR+ v2.7 13.6.3, Table 181 */ > _FDT(fdt_setprop_cell(fdt, offset, "vendor-id", vendor_id)); > _FDT(fdt_setprop_cell(fdt, offset, "device-id", device_id)); >
On Fri, Jan 22, 2021 at 06:01:57PM +0100, Greg Kurz wrote: > It is currently not possible to perform a strict boot from USB storage: > > $ qemu-system-ppc64 -accel kvm -nodefaults -nographic -serial stdio \ > -boot strict=on \ > -device qemu-xhci \ > -device usb-storage,drive=disk,bootindex=0 \ > -blockdev driver=file,node-name=disk,filename=fedora-ppc64le.qcow2 > > > SLOF ********************************************************************** > QEMU Starting > Build Date = Jul 17 2020 11:15:24 > FW Version = git-e18ddad8516ff2cf > Press "s" to enter Open Firmware. > > Populating /vdevice methods > Populating /vdevice/vty@71000000 > Populating /vdevice/nvram@71000001 > Populating /pci@800000020000000 > 00 0000 (D) : 1b36 000d serial bus [ usb-xhci ] > No NVRAM common partition, re-initializing... > Scanning USB > XHCI: Initializing > USB Storage > SCSI: Looking for devices > 101000000000000 DISK : "QEMU QEMU HARDDISK 2.5+" > Using default console: /vdevice/vty@71000000 > > Welcome to Open Firmware > > Copyright (c) 2004, 2017 IBM Corporation All rights reserved. > This program and the accompanying materials are made available > under the terms of the BSD License available at > http://www.opensource.org/licenses/bsd-license.php > > > Trying to load: from: /pci@800000020000000/usb@0/storage@1/disk@101000000000000 ... > E3405: No such device > > E3407: Load failed > > Type 'boot' and press return to continue booting the system. > Type 'reset-all' and press return to reboot the system. > > > Ready! > 0 > > > The device tree handed over by QEMU to SLOF indeed contains: > > qemu,boot-list = > "/pci@800000020000000/usb@0/storage@1/disk@101000000000000 HALT"; > > but the device node is named usb-xhci@0, not usb@0. > > This happens because the firmware names of PCI devices returned > by get_boot_devices_list() come from pcibus_get_fw_dev_path(), > while the sPAPR PHB code uses a different naming scheme for > device nodes. This inconsistency has always been there but it was > hidden for a long time because SLOF used to rename USB device > nodes, until this commit, merged in QEMU 4.2.0 : > > commit 85164ad4ed9960cac842fa4cc067c6b6699b0994 > Author: Alexey Kardashevskiy <aik@ozlabs.ru> > Date: Wed Sep 11 16:24:32 2019 +1000 > > pseries: Update SLOF firmware image > > This fixes USB host bus adapter name in the device tree to match QEMU's > one. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > Fortunately, sPAPR implements the firmware path provider interface. > This provides a way to override the default firmware paths. > > Just factor out the sPAPR PHB naming logic from spapr_dt_pci_device() > to a helper, and use it in the sPAPR firmware path provider hook. > > Fixes: 85164ad4ed99 ("pseries: Update SLOF firmware image") > Signed-off-by: Greg Kurz <groug@kaod.org> Nice work. Applied to ppc-for-6.0.
On Sat, 23 Jan 2021 13:36:34 +1100 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > On 23/01/2021 04:01, Greg Kurz wrote: > > It is currently not possible to perform a strict boot from USB storage: > > > > $ qemu-system-ppc64 -accel kvm -nodefaults -nographic -serial stdio \ > > -boot strict=on \ > > -device qemu-xhci \ > > -device usb-storage,drive=disk,bootindex=0 \ > > -blockdev driver=file,node-name=disk,filename=fedora-ppc64le.qcow2 > > > > > > SLOF ********************************************************************** > > QEMU Starting > > Build Date = Jul 17 2020 11:15:24 > > FW Version = git-e18ddad8516ff2cf > > Press "s" to enter Open Firmware. > > > > Populating /vdevice methods > > Populating /vdevice/vty@71000000 > > Populating /vdevice/nvram@71000001 > > Populating /pci@800000020000000 > > 00 0000 (D) : 1b36 000d serial bus [ usb-xhci ] > > No NVRAM common partition, re-initializing... > > Scanning USB > > XHCI: Initializing > > USB Storage > > SCSI: Looking for devices > > 101000000000000 DISK : "QEMU QEMU HARDDISK 2.5+" > > Using default console: /vdevice/vty@71000000 > > > > Welcome to Open Firmware > > > > Copyright (c) 2004, 2017 IBM Corporation All rights reserved. > > This program and the accompanying materials are made available > > under the terms of the BSD License available at > > http://www.opensource.org/licenses/bsd-license.php > > > > > > Trying to load: from: /pci@800000020000000/usb@0/storage@1/disk@101000000000000 ... > > E3405: No such device > > > > E3407: Load failed > > > > Type 'boot' and press return to continue booting the system. > > Type 'reset-all' and press return to reboot the system. > > > > > > Ready! > > 0 > > > > > The device tree handed over by QEMU to SLOF indeed contains: > > > > qemu,boot-list = > > "/pci@800000020000000/usb@0/storage@1/disk@101000000000000 HALT"; > > > > but the device node is named usb-xhci@0, not usb@0. > > > I'd expect it to be a return of qdev_fw_name() so in this case something > like "nec-usb-xhci" (which would still be broken) but seeing a plain > "usb" is a bit strange. > The logic under get_boot_devices_list() is a bit hard to follow because of the multiple indirections, but AFAICT it doesn't seem to rely on qdev_fw_name() to get the names. None of the XHCI devices seem to be setting DeviceClass::fw_name anyway: $ git grep fw_name hw/usb/ hw/usb/bus.c: qdev_fw_name(qdev), nr); hw/usb/dev-hub.c: dc->fw_name = "hub"; hw/usb/dev-mtp.c: dc->fw_name = "mtp"; hw/usb/dev-network.c: dc->fw_name = "network"; hw/usb/dev-storage.c: dc->fw_name = "storage"; hw/usb/dev-uas.c: dc->fw_name = "storage"; The plain "usb" naming comes from PCI, which has its own naming logic for PCI devices (which qemu-xhci happens to be) : #0 0x0000000100319474 in pci_dev_fw_name (len=33, buf=0x7fffffffd4c8 "\020", dev=0x7fffc3320010) at ../../hw/pci/pci.c:2533 #1 0x0000000100319474 in pcibus_get_fw_dev_path (dev=0x7fffc3320010) at ../../hw/pci/pci.c:2550 #2 0x000000010053118c in bus_get_fw_dev_path (dev=0x7fffc3320010, bus=<optimized out>) at ../../hw/core/qdev-fw.c:38 #3 0x000000010053118c in qdev_get_fw_dev_path_helper (dev=0x7fffc3320010, p=0x7fffffffd728 "/pci@800000020000000/", size=128) at ../../hw/core/qdev-fw.c:72 #4 0x0000000100531064 in qdev_get_fw_dev_path_helper (dev=0x101c864a0, p=0x7fffffffd728 "/pci@800000020000000/", size=128) at ../../hw/core/qdev-fw.c:69 #5 0x0000000100531064 in qdev_get_fw_dev_path_helper (dev=0x1019f3560, p=0x7fffffffd728 "/pci@800000020000000/", size=128) at ../../hw/core/qdev-fw.c:69 #6 0x00000001005312f0 in qdev_get_fw_dev_path (dev=<optimized out>) at ../../hw/core/qdev-fw.c:91 #7 0x0000000100588a68 in get_boot_device_path (dev=<optimized out>, ignore_suffixes=<optimized out>, ignore_suffixes@entry=true, suffix=<optimized out>) at ../../softmmu/bootdevice.c:211 #8 0x0000000100589540 in get_boot_devices_list (size=0x7fffffffd990) at ../../softmmu/bootdevice.c:257 #9 0x0000000100606764 in spapr_dt_chosen (reset=true, fdt=0x7fffc26f0010, spapr=0x10149aef0) at ../../hw/ppc/spapr.c:1019 > While your patch works, I wonder if we should assign fw_name to all pci > nodes to avoid similar problems in the future? Thanks, > Not sure to understand "assign fw_name to all pci nodes" ... > > > > > > > This happens because the firmware names of PCI devices returned > > by get_boot_devices_list() come from pcibus_get_fw_dev_path(), > > while the sPAPR PHB code uses a different naming scheme for > > device nodes. This inconsistency has always been there but it was > > hidden for a long time because SLOF used to rename USB device > > nodes, until this commit, merged in QEMU 4.2.0 : > > > > commit 85164ad4ed9960cac842fa4cc067c6b6699b0994 > > Author: Alexey Kardashevskiy <aik@ozlabs.ru> > > Date: Wed Sep 11 16:24:32 2019 +1000 > > > > pseries: Update SLOF firmware image > > > > This fixes USB host bus adapter name in the device tree to match QEMU's > > one. > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > Fortunately, sPAPR implements the firmware path provider interface. > > This provides a way to override the default firmware paths. > > > > Just factor out the sPAPR PHB naming logic from spapr_dt_pci_device() > > to a helper, and use it in the sPAPR firmware path provider hook. > > > > Fixes: 85164ad4ed99 ("pseries: Update SLOF firmware image") > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > include/hw/pci-host/spapr.h | 2 ++ > > hw/ppc/spapr.c | 5 +++++ > > hw/ppc/spapr_pci.c | 33 ++++++++++++++++++--------------- > > 3 files changed, 25 insertions(+), 15 deletions(-) > > > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > > index bd014823a933..5b03a7b0eb3f 100644 > > --- a/include/hw/pci-host/spapr.h > > +++ b/include/hw/pci-host/spapr.h > > @@ -210,4 +210,6 @@ static inline unsigned spapr_phb_windows_supported(SpaprPhbState *sphb) > > return sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1; > > } > > > > +char *spapr_pci_fw_dev_name(PCIDevice *dev); > > + > > #endif /* PCI_HOST_SPAPR_H */ > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 6ab27ea269d5..632502c2ecf8 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -3048,6 +3048,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, > > SCSIDevice *d = CAST(SCSIDevice, dev, TYPE_SCSI_DEVICE); > > SpaprPhbState *phb = CAST(SpaprPhbState, dev, TYPE_SPAPR_PCI_HOST_BRIDGE); > > VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, TYPE_VHOST_SCSI_COMMON); > > + PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE); > > > > if (d) { > > void *spapr = CAST(void, bus->parent, "spapr-vscsi"); > > @@ -3121,6 +3122,10 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, > > return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn)); > > } > > > > + if (pcidev) { > > + return spapr_pci_fw_dev_name(pcidev); > > + } > > + > > return NULL; > > } > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 76d7c91e9c64..da6eb58724c8 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1334,15 +1334,29 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus, > > return offset; > > } > > > > +char *spapr_pci_fw_dev_name(PCIDevice *dev) > > +{ > > + const gchar *basename; > > + int slot = PCI_SLOT(dev->devfn); > > + int func = PCI_FUNC(dev->devfn); > > + uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3); > > + > > + basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, > > + ccode & 0xff); > > + > > + if (func != 0) { > > + return g_strdup_printf("%s@%x,%x", basename, slot, func); > > + } else { > > + return g_strdup_printf("%s@%x", basename, slot); > > + } > > +} > > + > > /* create OF node for pci device and required OF DT properties */ > > static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev, > > void *fdt, int parent_offset) > > { > > int offset; > > - const gchar *basename; > > - gchar *nodename; > > - int slot = PCI_SLOT(dev->devfn); > > - int func = PCI_FUNC(dev->devfn); > > + g_autofree gchar *nodename = spapr_pci_fw_dev_name(dev); > > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > > ResourceProps rp; > > SpaprDrc *drc = drc_from_dev(sphb, dev); > > @@ -1359,19 +1373,8 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev, > > uint32_t pci_status = pci_default_read_config(dev, PCI_STATUS, 2); > > gchar *loc_code; > > > > - basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, > > - ccode & 0xff); > > - > > - if (func != 0) { > > - nodename = g_strdup_printf("%s@%x,%x", basename, slot, func); > > - } else { > > - nodename = g_strdup_printf("%s@%x", basename, slot); > > - } > > - > > _FDT(offset = fdt_add_subnode(fdt, parent_offset, nodename)); > > > > - g_free(nodename); > > - > > /* in accordance with PAPR+ v2.7 13.6.3, Table 181 */ > > _FDT(fdt_setprop_cell(fdt, offset, "vendor-id", vendor_id)); > > _FDT(fdt_setprop_cell(fdt, offset, "device-id", device_id)); > > >
On 25/01/2021 21:23, Greg Kurz wrote: > On Sat, 23 Jan 2021 13:36:34 +1100 > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > >> >> >> On 23/01/2021 04:01, Greg Kurz wrote: >>> It is currently not possible to perform a strict boot from USB storage: >>> >>> $ qemu-system-ppc64 -accel kvm -nodefaults -nographic -serial stdio \ >>> -boot strict=on \ >>> -device qemu-xhci \ >>> -device usb-storage,drive=disk,bootindex=0 \ >>> -blockdev driver=file,node-name=disk,filename=fedora-ppc64le.qcow2 >>> >>> >>> SLOF ********************************************************************** >>> QEMU Starting >>> Build Date = Jul 17 2020 11:15:24 >>> FW Version = git-e18ddad8516ff2cf >>> Press "s" to enter Open Firmware. >>> >>> Populating /vdevice methods >>> Populating /vdevice/vty@71000000 >>> Populating /vdevice/nvram@71000001 >>> Populating /pci@800000020000000 >>> 00 0000 (D) : 1b36 000d serial bus [ usb-xhci ] >>> No NVRAM common partition, re-initializing... >>> Scanning USB >>> XHCI: Initializing >>> USB Storage >>> SCSI: Looking for devices >>> 101000000000000 DISK : "QEMU QEMU HARDDISK 2.5+" >>> Using default console: /vdevice/vty@71000000 >>> >>> Welcome to Open Firmware >>> >>> Copyright (c) 2004, 2017 IBM Corporation All rights reserved. >>> This program and the accompanying materials are made available >>> under the terms of the BSD License available at >>> http://www.opensource.org/licenses/bsd-license.php >>> >>> >>> Trying to load: from: /pci@800000020000000/usb@0/storage@1/disk@101000000000000 ... >>> E3405: No such device >>> >>> E3407: Load failed >>> >>> Type 'boot' and press return to continue booting the system. >>> Type 'reset-all' and press return to reboot the system. >>> >>> >>> Ready! >>> 0 > >>> >>> The device tree handed over by QEMU to SLOF indeed contains: >>> >>> qemu,boot-list = >>> "/pci@800000020000000/usb@0/storage@1/disk@101000000000000 HALT"; >>> >>> but the device node is named usb-xhci@0, not usb@0. >> >> >> I'd expect it to be a return of qdev_fw_name() so in this case something >> like "nec-usb-xhci" (which would still be broken) but seeing a plain >> "usb" is a bit strange. >> > > The logic under get_boot_devices_list() is a bit hard to follow > because of the multiple indirections, but AFAICT it doesn't seem > to rely on qdev_fw_name() to get the names. > > None of the XHCI devices seem to be setting DeviceClass::fw_name anyway: > > $ git grep fw_name hw/usb/ > hw/usb/bus.c: qdev_fw_name(qdev), nr); > hw/usb/dev-hub.c: dc->fw_name = "hub"; > hw/usb/dev-mtp.c: dc->fw_name = "mtp"; > hw/usb/dev-network.c: dc->fw_name = "network"; > hw/usb/dev-storage.c: dc->fw_name = "storage"; > hw/usb/dev-uas.c: dc->fw_name = "storage"; > > The plain "usb" naming comes from PCI, which has its own naming > logic for PCI devices (which qemu-xhci happens to be) : Right, this was the confusing bit for me. I thought that by just setting dc->fw_name to what we put in the DT should be enough but it is not. > > #0 0x0000000100319474 in pci_dev_fw_name (len=33, buf=0x7fffffffd4c8 "\020", dev=0x7fffc3320010) at ../../hw/pci/pci.c:2533 > #1 0x0000000100319474 in pcibus_get_fw_dev_path (dev=0x7fffc3320010) at ../../hw/pci/pci.c:2550 > #2 0x000000010053118c in bus_get_fw_dev_path (dev=0x7fffc3320010, bus=<optimized out>) at ../../hw/core/qdev-fw.c:38 > #3 0x000000010053118c in qdev_get_fw_dev_path_helper (dev=0x7fffc3320010, p=0x7fffffffd728 "/pci@800000020000000/", size=128) at ../../hw/core/qdev-fw.c:72 > #4 0x0000000100531064 in qdev_get_fw_dev_path_helper (dev=0x101c864a0, p=0x7fffffffd728 "/pci@800000020000000/", size=128) at ../../hw/core/qdev-fw.c:69 > #5 0x0000000100531064 in qdev_get_fw_dev_path_helper (dev=0x1019f3560, p=0x7fffffffd728 "/pci@800000020000000/", size=128) at ../../hw/core/qdev-fw.c:69 > #6 0x00000001005312f0 in qdev_get_fw_dev_path (dev=<optimized out>) at ../../hw/core/qdev-fw.c:91 > #7 0x0000000100588a68 in get_boot_device_path (dev=<optimized out>, ignore_suffixes=<optimized out>, ignore_suffixes@entry=true, suffix=<optimized out>) at ../../softmmu/bootdevice.c:211 > #8 0x0000000100589540 in get_boot_devices_list (size=0x7fffffffd990) at ../../softmmu/bootdevice.c:257 > #9 0x0000000100606764 in spapr_dt_chosen (reset=true, fdt=0x7fffc26f0010, spapr=0x10149aef0) at ../../hw/ppc/spapr.c:1019 > > >> While your patch works, I wonder if we should assign fw_name to all pci >> nodes to avoid similar problems in the future? Thanks, >> > > Not sure to understand "assign fw_name to all pci nodes" ... Basically this: ===== diff --git a/hw/pci/pci.c b/hw/pci/pci.c index de0fae10ab9c..8a286419aaf8 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2508,7 +2508,12 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len) const char *name = NULL; const pci_class_desc *desc = pci_class_descriptions; int class = pci_get_word(d->config + PCI_CLASS_DEVICE); + DeviceClass *dc = DEVICE_GET_CLASS(dev); + if (dc->fw_name) { + pstrcpy(buf, len, dc->fw_name); + return buf; + } while (desc->desc && (class & ~desc->fw_ign_bits) != (desc->class & ~desc->fw_ign_bits)) { diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 0a418f1e6711..057dd384ada7 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1467,8 +1467,14 @@ int spapr_pci_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, HotplugHandler *plug_handler = qdev_get_hotplug_handler(drc->dev); SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(plug_handler); PCIDevice *pdev = PCI_DEVICE(drc->dev); + DeviceState *dev = DEVICE(pdev); + uint32_t ccode = pci_default_read_config(pdev, PCI_CLASS_PROG, 3); + DeviceClass *dc = DEVICE_GET_CLASS(dev); *fdt_start_offset = spapr_dt_pci_device(sphb, pdev, fdt, 0); + + dc->fw_name = g_strdup(dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, + ccode & 0xff)); return 0; } ===== Note this is just to demonstrate the idea (this works though) :) (changing the device class is way too late, would need to dig QOM a bit, also need to do the same for hotplugged devices). But the point is that pci_dev_fw_name() (has "_fw_name"!) should not ignore dc->fw_name. Thanks,
On 27/01/2021 12:28, Alexey Kardashevskiy wrote: > > > On 25/01/2021 21:23, Greg Kurz wrote: >> On Sat, 23 Jan 2021 13:36:34 +1100 >> Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> >>> >>> >>> On 23/01/2021 04:01, Greg Kurz wrote: >>>> It is currently not possible to perform a strict boot from USB storage: >>>> >>>> $ qemu-system-ppc64 -accel kvm -nodefaults -nographic -serial stdio \ >>>> -boot strict=on \ >>>> -device qemu-xhci \ >>>> -device usb-storage,drive=disk,bootindex=0 \ >>>> -blockdev driver=file,node-name=disk,filename=fedora-ppc64le.qcow2 >>>> >>>> >>>> SLOF >>>> ********************************************************************** >>>> QEMU Starting >>>> Build Date = Jul 17 2020 11:15:24 >>>> FW Version = git-e18ddad8516ff2cf >>>> Press "s" to enter Open Firmware. >>>> >>>> Populating /vdevice methods >>>> Populating /vdevice/vty@71000000 >>>> Populating /vdevice/nvram@71000001 >>>> Populating /pci@800000020000000 >>>> 00 0000 (D) : 1b36 000d serial bus [ >>>> usb-xhci ] >>>> No NVRAM common partition, re-initializing... >>>> Scanning USB >>>> XHCI: Initializing >>>> USB Storage >>>> SCSI: Looking for devices >>>> 101000000000000 DISK : "QEMU QEMU HARDDISK 2.5+" >>>> Using default console: /vdevice/vty@71000000 >>>> >>>> Welcome to Open Firmware >>>> >>>> Copyright (c) 2004, 2017 IBM Corporation All rights reserved. >>>> This program and the accompanying materials are made available >>>> under the terms of the BSD License available at >>>> http://www.opensource.org/licenses/bsd-license.php >>>> >>>> >>>> Trying to load: from: >>>> /pci@800000020000000/usb@0/storage@1/disk@101000000000000 ... >>>> E3405: No such device >>>> >>>> E3407: Load failed >>>> >>>> Type 'boot' and press return to continue booting the system. >>>> Type 'reset-all' and press return to reboot the system. >>>> >>>> >>>> Ready! >>>> 0 > >>>> >>>> The device tree handed over by QEMU to SLOF indeed contains: >>>> >>>> qemu,boot-list = >>>> "/pci@800000020000000/usb@0/storage@1/disk@101000000000000 HALT"; >>>> >>>> but the device node is named usb-xhci@0, not usb@0. >>> >>> >>> I'd expect it to be a return of qdev_fw_name() so in this case something >>> like "nec-usb-xhci" (which would still be broken) but seeing a plain >>> "usb" is a bit strange. >>> >> >> The logic under get_boot_devices_list() is a bit hard to follow >> because of the multiple indirections, but AFAICT it doesn't seem >> to rely on qdev_fw_name() to get the names. >> >> None of the XHCI devices seem to be setting DeviceClass::fw_name anyway: >> >> $ git grep fw_name hw/usb/ >> hw/usb/bus.c: qdev_fw_name(qdev), nr); >> hw/usb/dev-hub.c: dc->fw_name = "hub"; >> hw/usb/dev-mtp.c: dc->fw_name = "mtp"; >> hw/usb/dev-network.c: dc->fw_name = "network"; >> hw/usb/dev-storage.c: dc->fw_name = "storage"; >> hw/usb/dev-uas.c: dc->fw_name = "storage"; >> >> The plain "usb" naming comes from PCI, which has its own naming >> logic for PCI devices (which qemu-xhci happens to be) : > > > Right, this was the confusing bit for me. I thought that by just setting > dc->fw_name to what we put in the DT should be enough but it is not. > > >> >> #0 0x0000000100319474 in pci_dev_fw_name (len=33, buf=0x7fffffffd4c8 >> "\020", dev=0x7fffc3320010) at ../../hw/pci/pci.c:2533 >> #1 0x0000000100319474 in pcibus_get_fw_dev_path (dev=0x7fffc3320010) >> at ../../hw/pci/pci.c:2550 >> #2 0x000000010053118c in bus_get_fw_dev_path (dev=0x7fffc3320010, >> bus=<optimized out>) at ../../hw/core/qdev-fw.c:38 >> #3 0x000000010053118c in qdev_get_fw_dev_path_helper >> (dev=0x7fffc3320010, p=0x7fffffffd728 "/pci@800000020000000/", >> size=128) at ../../hw/core/qdev-fw.c:72 >> #4 0x0000000100531064 in qdev_get_fw_dev_path_helper >> (dev=0x101c864a0, p=0x7fffffffd728 "/pci@800000020000000/", size=128) >> at ../../hw/core/qdev-fw.c:69 >> #5 0x0000000100531064 in qdev_get_fw_dev_path_helper >> (dev=0x1019f3560, p=0x7fffffffd728 "/pci@800000020000000/", size=128) >> at ../../hw/core/qdev-fw.c:69 >> #6 0x00000001005312f0 in qdev_get_fw_dev_path (dev=<optimized out>) >> at ../../hw/core/qdev-fw.c:91 >> #7 0x0000000100588a68 in get_boot_device_path (dev=<optimized out>, >> ignore_suffixes=<optimized out>, ignore_suffixes@entry=true, >> suffix=<optimized out>) at ../../softmmu/bootdevice.c:211 >> #8 0x0000000100589540 in get_boot_devices_list (size=0x7fffffffd990) >> at ../../softmmu/bootdevice.c:257 >> #9 0x0000000100606764 in spapr_dt_chosen (reset=true, >> fdt=0x7fffc26f0010, spapr=0x10149aef0) at ../../hw/ppc/spapr.c:1019 >> >> >>> While your patch works, I wonder if we should assign fw_name to all pci >>> nodes to avoid similar problems in the future? Thanks, >>> >> >> Not sure to understand "assign fw_name to all pci nodes" ... > > > Basically this: > > ===== > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index de0fae10ab9c..8a286419aaf8 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2508,7 +2508,12 @@ static char *pci_dev_fw_name(DeviceState *dev, > char *buf, int len) > const char *name = NULL; > const pci_class_desc *desc = pci_class_descriptions; > int class = pci_get_word(d->config + PCI_CLASS_DEVICE); > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > > + if (dc->fw_name) { > + pstrcpy(buf, len, dc->fw_name); > + return buf; > + } > while (desc->desc && > (class & ~desc->fw_ign_bits) != > (desc->class & ~desc->fw_ign_bits)) { > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 0a418f1e6711..057dd384ada7 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1467,8 +1467,14 @@ int spapr_pci_dt_populate(SpaprDrc *drc, > SpaprMachineState *spapr, > HotplugHandler *plug_handler = qdev_get_hotplug_handler(drc->dev); > SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(plug_handler); > PCIDevice *pdev = PCI_DEVICE(drc->dev); > + DeviceState *dev = DEVICE(pdev); > + uint32_t ccode = pci_default_read_config(pdev, PCI_CLASS_PROG, 3); > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > > *fdt_start_offset = spapr_dt_pci_device(sphb, pdev, fdt, 0); > + > + dc->fw_name = g_strdup(dt_name_from_class((ccode >> 16) & 0xff, > (ccode >> 8) & 0xff, > + ccode & 0xff)); > return 0; > } > ===== > > Note this is just to demonstrate the idea (this works though) :) > (changing the device class is way too late, would need to dig QOM a bit, > also need to do the same for hotplugged devices). > > But the point is that pci_dev_fw_name() (has "_fw_name"!) should not > ignore dc->fw_name. Thanks, Ping? Too stupid proposal? :)
On Wed, 10 Feb 2021 17:40:43 +1100 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > On 27/01/2021 12:28, Alexey Kardashevskiy wrote: > > > > > > On 25/01/2021 21:23, Greg Kurz wrote: > >> On Sat, 23 Jan 2021 13:36:34 +1100 > >> Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > >> > >>> > >>> > >>> On 23/01/2021 04:01, Greg Kurz wrote: > >>>> It is currently not possible to perform a strict boot from USB storage: > >>>> > >>>> $ qemu-system-ppc64 -accel kvm -nodefaults -nographic -serial stdio \ > >>>> -boot strict=on \ > >>>> -device qemu-xhci \ > >>>> -device usb-storage,drive=disk,bootindex=0 \ > >>>> -blockdev driver=file,node-name=disk,filename=fedora-ppc64le.qcow2 > >>>> > >>>> > >>>> SLOF > >>>> ********************************************************************** > >>>> QEMU Starting > >>>> Build Date = Jul 17 2020 11:15:24 > >>>> FW Version = git-e18ddad8516ff2cf > >>>> Press "s" to enter Open Firmware. > >>>> > >>>> Populating /vdevice methods > >>>> Populating /vdevice/vty@71000000 > >>>> Populating /vdevice/nvram@71000001 > >>>> Populating /pci@800000020000000 > >>>> 00 0000 (D) : 1b36 000d serial bus [ > >>>> usb-xhci ] > >>>> No NVRAM common partition, re-initializing... > >>>> Scanning USB > >>>> XHCI: Initializing > >>>> USB Storage > >>>> SCSI: Looking for devices > >>>> 101000000000000 DISK : "QEMU QEMU HARDDISK 2.5+" > >>>> Using default console: /vdevice/vty@71000000 > >>>> > >>>> Welcome to Open Firmware > >>>> > >>>> Copyright (c) 2004, 2017 IBM Corporation All rights reserved. > >>>> This program and the accompanying materials are made available > >>>> under the terms of the BSD License available at > >>>> http://www.opensource.org/licenses/bsd-license.php > >>>> > >>>> > >>>> Trying to load: from: > >>>> /pci@800000020000000/usb@0/storage@1/disk@101000000000000 ... > >>>> E3405: No such device > >>>> > >>>> E3407: Load failed > >>>> > >>>> Type 'boot' and press return to continue booting the system. > >>>> Type 'reset-all' and press return to reboot the system. > >>>> > >>>> > >>>> Ready! > >>>> 0 > > >>>> > >>>> The device tree handed over by QEMU to SLOF indeed contains: > >>>> > >>>> qemu,boot-list = > >>>> "/pci@800000020000000/usb@0/storage@1/disk@101000000000000 HALT"; > >>>> > >>>> but the device node is named usb-xhci@0, not usb@0. > >>> > >>> > >>> I'd expect it to be a return of qdev_fw_name() so in this case something > >>> like "nec-usb-xhci" (which would still be broken) but seeing a plain > >>> "usb" is a bit strange. > >>> > >> > >> The logic under get_boot_devices_list() is a bit hard to follow > >> because of the multiple indirections, but AFAICT it doesn't seem > >> to rely on qdev_fw_name() to get the names. > >> > >> None of the XHCI devices seem to be setting DeviceClass::fw_name anyway: > >> > >> $ git grep fw_name hw/usb/ > >> hw/usb/bus.c: qdev_fw_name(qdev), nr); > >> hw/usb/dev-hub.c: dc->fw_name = "hub"; > >> hw/usb/dev-mtp.c: dc->fw_name = "mtp"; > >> hw/usb/dev-network.c: dc->fw_name = "network"; > >> hw/usb/dev-storage.c: dc->fw_name = "storage"; > >> hw/usb/dev-uas.c: dc->fw_name = "storage"; > >> > >> The plain "usb" naming comes from PCI, which has its own naming > >> logic for PCI devices (which qemu-xhci happens to be) : > > > > > > Right, this was the confusing bit for me. I thought that by just setting > > dc->fw_name to what we put in the DT should be enough but it is not. > > > > > >> > >> #0 0x0000000100319474 in pci_dev_fw_name (len=33, buf=0x7fffffffd4c8 > >> "\020", dev=0x7fffc3320010) at ../../hw/pci/pci.c:2533 > >> #1 0x0000000100319474 in pcibus_get_fw_dev_path (dev=0x7fffc3320010) > >> at ../../hw/pci/pci.c:2550 > >> #2 0x000000010053118c in bus_get_fw_dev_path (dev=0x7fffc3320010, > >> bus=<optimized out>) at ../../hw/core/qdev-fw.c:38 > >> #3 0x000000010053118c in qdev_get_fw_dev_path_helper > >> (dev=0x7fffc3320010, p=0x7fffffffd728 "/pci@800000020000000/", > >> size=128) at ../../hw/core/qdev-fw.c:72 > >> #4 0x0000000100531064 in qdev_get_fw_dev_path_helper > >> (dev=0x101c864a0, p=0x7fffffffd728 "/pci@800000020000000/", size=128) > >> at ../../hw/core/qdev-fw.c:69 > >> #5 0x0000000100531064 in qdev_get_fw_dev_path_helper > >> (dev=0x1019f3560, p=0x7fffffffd728 "/pci@800000020000000/", size=128) > >> at ../../hw/core/qdev-fw.c:69 > >> #6 0x00000001005312f0 in qdev_get_fw_dev_path (dev=<optimized out>) > >> at ../../hw/core/qdev-fw.c:91 > >> #7 0x0000000100588a68 in get_boot_device_path (dev=<optimized out>, > >> ignore_suffixes=<optimized out>, ignore_suffixes@entry=true, > >> suffix=<optimized out>) at ../../softmmu/bootdevice.c:211 > >> #8 0x0000000100589540 in get_boot_devices_list (size=0x7fffffffd990) > >> at ../../softmmu/bootdevice.c:257 > >> #9 0x0000000100606764 in spapr_dt_chosen (reset=true, > >> fdt=0x7fffc26f0010, spapr=0x10149aef0) at ../../hw/ppc/spapr.c:1019 > >> > >> > >>> While your patch works, I wonder if we should assign fw_name to all pci > >>> nodes to avoid similar problems in the future? Thanks, > >>> > >> > >> Not sure to understand "assign fw_name to all pci nodes" ... > > > > > > Basically this: > > > > ===== > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index de0fae10ab9c..8a286419aaf8 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -2508,7 +2508,12 @@ static char *pci_dev_fw_name(DeviceState *dev, > > char *buf, int len) > > const char *name = NULL; > > const pci_class_desc *desc = pci_class_descriptions; > > int class = pci_get_word(d->config + PCI_CLASS_DEVICE); > > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > > > > + if (dc->fw_name) { > > + pstrcpy(buf, len, dc->fw_name); > > + return buf; > > + } > > while (desc->desc && > > (class & ~desc->fw_ign_bits) != > > (desc->class & ~desc->fw_ign_bits)) { > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 0a418f1e6711..057dd384ada7 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1467,8 +1467,14 @@ int spapr_pci_dt_populate(SpaprDrc *drc, > > SpaprMachineState *spapr, > > HotplugHandler *plug_handler = qdev_get_hotplug_handler(drc->dev); > > SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(plug_handler); > > PCIDevice *pdev = PCI_DEVICE(drc->dev); > > + DeviceState *dev = DEVICE(pdev); > > + uint32_t ccode = pci_default_read_config(pdev, PCI_CLASS_PROG, 3); > > + DeviceClass *dc = DEVICE_GET_CLASS(dev); > > > > *fdt_start_offset = spapr_dt_pci_device(sphb, pdev, fdt, 0); > > + > > + dc->fw_name = g_strdup(dt_name_from_class((ccode >> 16) & 0xff, > > (ccode >> 8) & 0xff, > > + ccode & 0xff)); > > return 0; > > } > > ===== > > > > Note this is just to demonstrate the idea (this works though) :) > > (changing the device class is way too late, would need to dig QOM a bit, > > also need to do the same for hotplugged devices). > > > > But the point is that pci_dev_fw_name() (has "_fw_name"!) should not > > ignore dc->fw_name. Thanks, > > Ping? Too stupid proposal? :) > Post a patch ? :) > >
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index bd014823a933..5b03a7b0eb3f 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -210,4 +210,6 @@ static inline unsigned spapr_phb_windows_supported(SpaprPhbState *sphb) return sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1; } +char *spapr_pci_fw_dev_name(PCIDevice *dev); + #endif /* PCI_HOST_SPAPR_H */ diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 6ab27ea269d5..632502c2ecf8 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3048,6 +3048,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, SCSIDevice *d = CAST(SCSIDevice, dev, TYPE_SCSI_DEVICE); SpaprPhbState *phb = CAST(SpaprPhbState, dev, TYPE_SPAPR_PCI_HOST_BRIDGE); VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, TYPE_VHOST_SCSI_COMMON); + PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE); if (d) { void *spapr = CAST(void, bus->parent, "spapr-vscsi"); @@ -3121,6 +3122,10 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn)); } + if (pcidev) { + return spapr_pci_fw_dev_name(pcidev); + } + return NULL; } diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 76d7c91e9c64..da6eb58724c8 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1334,15 +1334,29 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus, return offset; } +char *spapr_pci_fw_dev_name(PCIDevice *dev) +{ + const gchar *basename; + int slot = PCI_SLOT(dev->devfn); + int func = PCI_FUNC(dev->devfn); + uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3); + + basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, + ccode & 0xff); + + if (func != 0) { + return g_strdup_printf("%s@%x,%x", basename, slot, func); + } else { + return g_strdup_printf("%s@%x", basename, slot); + } +} + /* create OF node for pci device and required OF DT properties */ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev, void *fdt, int parent_offset) { int offset; - const gchar *basename; - gchar *nodename; - int slot = PCI_SLOT(dev->devfn); - int func = PCI_FUNC(dev->devfn); + g_autofree gchar *nodename = spapr_pci_fw_dev_name(dev); PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); ResourceProps rp; SpaprDrc *drc = drc_from_dev(sphb, dev); @@ -1359,19 +1373,8 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev, uint32_t pci_status = pci_default_read_config(dev, PCI_STATUS, 2); gchar *loc_code; - basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff, - ccode & 0xff); - - if (func != 0) { - nodename = g_strdup_printf("%s@%x,%x", basename, slot, func); - } else { - nodename = g_strdup_printf("%s@%x", basename, slot); - } - _FDT(offset = fdt_add_subnode(fdt, parent_offset, nodename)); - g_free(nodename); - /* in accordance with PAPR+ v2.7 13.6.3, Table 181 */ _FDT(fdt_setprop_cell(fdt, offset, "vendor-id", vendor_id)); _FDT(fdt_setprop_cell(fdt, offset, "device-id", device_id));
It is currently not possible to perform a strict boot from USB storage: $ qemu-system-ppc64 -accel kvm -nodefaults -nographic -serial stdio \ -boot strict=on \ -device qemu-xhci \ -device usb-storage,drive=disk,bootindex=0 \ -blockdev driver=file,node-name=disk,filename=fedora-ppc64le.qcow2 SLOF ********************************************************************** QEMU Starting Build Date = Jul 17 2020 11:15:24 FW Version = git-e18ddad8516ff2cf Press "s" to enter Open Firmware. Populating /vdevice methods Populating /vdevice/vty@71000000 Populating /vdevice/nvram@71000001 Populating /pci@800000020000000 00 0000 (D) : 1b36 000d serial bus [ usb-xhci ] No NVRAM common partition, re-initializing... Scanning USB XHCI: Initializing USB Storage SCSI: Looking for devices 101000000000000 DISK : "QEMU QEMU HARDDISK 2.5+" Using default console: /vdevice/vty@71000000 Welcome to Open Firmware Copyright (c) 2004, 2017 IBM Corporation All rights reserved. This program and the accompanying materials are made available under the terms of the BSD License available at http://www.opensource.org/licenses/bsd-license.php Trying to load: from: /pci@800000020000000/usb@0/storage@1/disk@101000000000000 ... E3405: No such device E3407: Load failed Type 'boot' and press return to continue booting the system. Type 'reset-all' and press return to reboot the system. Ready! 0 > The device tree handed over by QEMU to SLOF indeed contains: qemu,boot-list = "/pci@800000020000000/usb@0/storage@1/disk@101000000000000 HALT"; but the device node is named usb-xhci@0, not usb@0. This happens because the firmware names of PCI devices returned by get_boot_devices_list() come from pcibus_get_fw_dev_path(), while the sPAPR PHB code uses a different naming scheme for device nodes. This inconsistency has always been there but it was hidden for a long time because SLOF used to rename USB device nodes, until this commit, merged in QEMU 4.2.0 : commit 85164ad4ed9960cac842fa4cc067c6b6699b0994 Author: Alexey Kardashevskiy <aik@ozlabs.ru> Date: Wed Sep 11 16:24:32 2019 +1000 pseries: Update SLOF firmware image This fixes USB host bus adapter name in the device tree to match QEMU's one. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Fortunately, sPAPR implements the firmware path provider interface. This provides a way to override the default firmware paths. Just factor out the sPAPR PHB naming logic from spapr_dt_pci_device() to a helper, and use it in the sPAPR firmware path provider hook. Fixes: 85164ad4ed99 ("pseries: Update SLOF firmware image") Signed-off-by: Greg Kurz <groug@kaod.org> --- include/hw/pci-host/spapr.h | 2 ++ hw/ppc/spapr.c | 5 +++++ hw/ppc/spapr_pci.c | 33 ++++++++++++++++++--------------- 3 files changed, 25 insertions(+), 15 deletions(-)