QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Greg Kurz <groug@kaod.org>
To: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
	David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH] spapr: Adjust firmware path of PCI devices
Date: Mon, 25 Jan 2021 11:23:11 +0100
Message-ID: <20210125112311.254ba960@bahia.lan> (raw)
In-Reply-To: <4294d3b5-abf3-fd43-660b-d82caf791d4f@ozlabs.ru>

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



  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-22 17:01 Greg Kurz
2021-01-22 17:26 ` Daniel Henrique Barboza
2021-01-23  2:36 ` Alexey Kardashevskiy
2021-01-25 10:23   ` Greg Kurz [this message]
2021-01-27  1:28     ` Alexey Kardashevskiy
2021-02-10  6:40       ` Alexey Kardashevskiy
2021-02-10  8:08         ` Greg Kurz
2021-01-25  8:21 ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210125112311.254ba960@bahia.lan \
    --to=groug@kaod.org \
    --cc=aik@ozlabs.ru \
    --cc=david@gibson.dropbear.id.au \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git
	git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git