Re: [Qemu-devel] [PATCH V2] pci: fixes to allow booting from extra root pci buses.

[Laszlo Ersek <lersek@redhat.com>]

On 06/12/15 15:03, Kevin O'Connor wrote
> On Fri, Jun 12, 2015 at 11:25:50AM +0200, Laszlo Ersek wrote:
>> On 06/11/15 21:24, Kevin O'Connor wrote:
>>> On Thu, Jun 11, 2015 at 08:34:56PM +0200, Laszlo Ersek wrote:
>>>> On 06/11/15 19:46, Marcel Apfelbaum wrote:
>>>>> On 06/11/2015 07:54 PM, Kevin O'Connor wrote:
>>>>>> On real machines, the firmware assigns the 4 - it's not a
>>>>>> physical address; it's a logical address (like all bus numbers in
>>>>>> PCI).  The firmware might assign a totally different number on
>>>>>> the next boot.
>>>>> Now I am confused. Don't get me wrong, I am not an expert on fw, I
>>>>> hardly try to understand it.
>>>>>
>>>>> I looked up a real hardware machine and it seemed to me that the
>>>>> extra pci root numbers are provided in the ACPI tables, meaning by
>>>>> the vendor, not the fw. In this case QEMU is the vendor, i440fx is
>>>>> the machine, right?
>>>>>
>>>>> I am not aware that Seabios/OVMF are deciding the bus numbers for
>>>>> the *PCI roots*.
>>>>> They are doing it for the pci-2-pci bridges of course.
>>>>> I saw that Seabios is trying to "guess" the root-buses by going
>>>>> over all the 0-0xff range and probing all the slots, looking for
>>>>> devices. So it expects the hw to be hardwired regarding PCI root
>>>>> buses.
>>>>
>>>> This is exactly how I understood it.
>>>>
>>>> We're not interested in placing such bus numbers in device paths
>>>> that are assigned during PCI enumeration. (Like subordinate bus
>>>> numbers.) We're talking about the root bus numbers.
>>>>
>>>> OVMF implements the same kind of probing that SeaBIOS does (based
>>>> on natural language description from Michael and Marcel, not on the
>>>> actual code). Devices on the root buses respond without any prior
>>>> bus number assignments.
>>>
>>> Alas, that is not correct.  Coreboot supports several AMD boards
>>> that have multiple southbridge chips which provide independent PCI
>>> root buses.  These chips have to be configured and assigned a bus
>>> number prior to use (which coreboot does).
>>
>> Thanks.
>>
>> Assuming such a physical hardware configuration, and that Coreboot
>> configures the root buses before the SeaBIOS payload is launched: how
>> does Coreboot identify a device, on a nonzero root bus, for SeaBIOS
>> to boot from? Is that possible at all, or is the user expected to
>> configure / select that in SeaBIOS exclusively?
>
> Coreboot does not provide information on what to boot.  It's task is
> low level hardware initialization.  It's the job of SeaBIOS to boot
> the OS (and determine which media, etc to boot from).  SeaBIOS gets
> boot preference information from a static configuration file
> (bootorder) stored in flash (cbfs).
>
>> Assuming there is no such feature between Coreboot and SeaBIOS (ie.
>> one that would parallel our QEMU use case on physical hardware), what
>> solution would you find acceptable for the case when QEMU basically
>> promises "I know where you'll find those root buses, and the
>> bootorder fw_cfg file will match them"?
>
> We currently go to great lengths to avoid logical identifiers in
> bootorder and I'm confused why we would wish to add them now.  Bus
> number is not currently used anywhere in bootorder because (in the
> general case) it's an arbitrary identifier that's not stable between
> boots and (in the general case) may not be stable even within a boot.
>
> I understand that in this specific case (extra root buses on QEMU) it
> is stable within a boot, but it seems strange that we'd want to define
> the interface knowing it's a poor choice in the general case.
>
> As for what I would suggest - well, SeaBIOS has already supported
> multiple root buses for years and already has a mechanism for
> deterministically specifying a device on an extra root bus.  (By
> specifying the N'th extra root bus instead of specifying the logical
> id given to that bus).  This is by no means a perfect solution and
> it's certainly open to change - but the current proposed patches
> appear to be regressions to me.
>
>> Could we simply make this patch conditional on runningOnQEMU()?
>
> It's possible.  I'd certainly prefer to avoid adding special cases if
> possible.

Okay. Let's compare the two options we appear to have:

(1) A patch like this for SeaBIOS:

> diff --git a/src/boot.c b/src/boot.c
> index ec59c37..c7fd091 100644
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -114,7 +114,8 @@ build_pci_path(char *buf, int max, const char *devname, struct pci_device *pci)
>      } else {
>          if (pci->rootbus)
>              p += snprintf(p, max, "/pci-root@%x", pci->rootbus);
> -        p += snprintf(p, buf+max-p, "%s", FW_PCI_DOMAIN);
> +        if (!runningOnQEMU() || !pci->rootbus)
> +            p += snprintf(p, buf+max-p, "%s", FW_PCI_DOMAIN);
>      }
>
>      int dev = pci_bdf_to_dev(pci->bdf), fn = pci_bdf_to_fn(pci->bdf);
> diff --git a/src/hw/pci.c b/src/hw/pci.c
> index 0379b55..169a040 100644
> --- a/src/hw/pci.c
> +++ b/src/hw/pci.c
> @@ -13,6 +13,7 @@
>  #include "string.h" // memset
>  #include "util.h" // udelay
>  #include "x86.h" // outl
> +#include "fw/paravirt.h" // runningOnQEMU
>
>  void pci_config_writel(u16 bdf, u32 addr, u32 val)
>  {
> @@ -133,7 +134,7 @@ pci_probe_devices(void)
>                  if (bus != lastbus)
>                      rootbuses++;
>                  lastbus = bus;
> -                rootbus = rootbuses;
> +                rootbus = runningOnQEMU() ? bus : rootbuses;
>                  if (bus > MaxPCIBus)
>                      MaxPCIBus = bus;
>              } else {

(2) The QEMU command line and the effects the command line has on the
virtual hardware should not change. However, all of the following have
to be updated:

- the "explicit_ofw_unit_address" property assignments in
  pxb_dev_initfn() have to be done separately, after all PXBs have been
  seen, and sorted. This probably requires another "machine init done"
  notifier.

- the _UID assignments in build_ssdt() need to reflect the exact same
  values

- OVMF's root bridge driver needs to generate the same _UID values in
  the PciRoot() device path nodes

- OVMF's boot order library must consider the /pci-root@N/pci@i0cf8/...
  format, where the root bus is the N'th extra root bus (in hex
  notation).

Basically, we need to keep the bus_nr=N user interface, and the effects
it has on the virtual hardware, intact, but translate the numbers that
are exposed via fw_cfg *and* ACPI (because those must match!) from
"identifier" to "serial number after sorting by identifier"; in practice
replicating the detection traversal that SeaBIOS does.

Doesn't seem impossible (unless Marcel raises a design-level issue with
it), but I'll have to withdraw for a while and research it.

Thanks
Laszlo