[PATCH] spapr: Adjust firmware path of PCI devices

[PATCH] spapr: Adjust firmware path of PCI devices

[Greg Kurz]

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.

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

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

[Daniel Henrique Barboza]

On 1/22/21 2:01 PM, 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.
> 
> 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>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

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

[Alexey Kardashevskiy]

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

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

[Greg Kurz]

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

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

[Alexey Kardashevskiy]

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,



-- 
Alexey

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

[Alexey Kardashevskiy]

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? :)



-- 
Alexey

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

[Greg Kurz]

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 ? :)

> 
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3318 bytes --]

On Fri, Jan 22, 2021 at 06:01:57PM +0100, 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.
> 
> 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>

Nice work.  Applied to ppc-for-6.0.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]