On Wed, 10 Feb 2021 17:40:43 +1100 Alexey Kardashevskiy <a...@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 <a...@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 ? :) > >
