QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Alexey Kardashevskiy <aik@ozlabs.ru>
To: Greg Kurz <groug@kaod.org>
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: Wed, 27 Jan 2021 12:28:57 +1100
Message-ID: <d647f198-4f47-692e-8a6e-8312044edfa8@ozlabs.ru> (raw)
In-Reply-To: <20210125112311.254ba960@bahia.lan>



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


  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
2021-01-27  1:28     ` Alexey Kardashevskiy [this message]
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=d647f198-4f47-692e-8a6e-8312044edfa8@ozlabs.ru \
    --to=aik@ozlabs.ru \
    --cc=david@gibson.dropbear.id.au \
    --cc=groug@kaod.org \
    --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