[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] spapr: Adjust firmware path of PCI devices

From: Alexey Kardashevskiy
Subject: Re: [PATCH] spapr: Adjust firmware path of PCI devices
Date: Wed, 27 Jan 2021 12:28:57 +1100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:85.0) Gecko/20100101 Thunderbird/85.0

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

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.

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 
#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 
#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 
#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 
#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,


reply via email to

[Prev in Thread] Current Thread [Next in Thread]