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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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

-- 
Alexey