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

[Laszlo Ersek]

On 06/12/15 20:40, Kevin O'Connor wrote:
> On Fri, Jun 12, 2015 at 05:45:04PM +0200, Laszlo Ersek wrote:
>> On 06/12/15 15:03, Kevin O'Connor wrote
>>> 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, "/address@hidden", 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 {
> 
> If we went down this path, I hope we could agree on the same prefix
> and thus limit the runningOnQEMU() to just the second part.  Was there
> a concern with "/address@hidden,%x/"?

I don't recall any specific concern, but if we want to present either
/address@hidden,%x/, or the pattern SeaBIOS currently expects, then in QEMU
the same stuff has to be poked at anyway.

> 
>> (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.
> 
> I admit the sorting of pxb objects just to reverse engineer what
> SeaBIOS expects would not be fun.

Actually it's kinda fun. :)

> Doesn't QEMU have to sort the buses
> anyway to know which secondary bus ranges are associated with each
> root bus?

I don't think so.

OVMF does the same probing (in the same order) as SeaBIOS for the root
buses, and the intervals between each pair are handed to edk2's PCI bus
driver (which is independent of the OVMF platform code). This latter
driver performs the assignments / allocations from the allowed interval.

>> - 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 
>> /address@hidden/address@hidden/...
>>   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.
> 
> Why does fw_cfg and ACPI have to match?

You will like the explanation for that. I'm about to test a new QEMU
series, and I'll CC you on the relevant patches. The last patch in the
series answers this question.

(In a nutshell: OVMF translates OFW devpath fragments to UEFI devpath
fragments, for boot option matching. The UEFI devpath fragments in
question start with a PciRoot() ACPI node, and the _UID value in that
initial node must match what QEMU hands down and OVMF uses in
translation. Finally, the UEFI spec requires that such a _UID value, in
the UEFI devpath, actually identify the valid ACPI object -- and that
ACPI object comes from QEMU's generator.)

If everything goes well, we might not need to change SeaBIOS at all.
(... Hope dies last :))

Thanks
Laszlo