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

[Greg Kurz <groug@kaod.org>]

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));
> > 
>