[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] spapr: Adjust firmware path of PCI devices
|
From: |
Greg Kurz |
|
Subject: |
Re: [PATCH] spapr: Adjust firmware path of PCI devices |
|
Date: |
Wed, 10 Feb 2021 09:08:12 +0100 |
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 ? :)
>
>