Re: [Qemu-devel] [RFC PATCH v2 0/4] Allow RedHat PCI bridges reserve more buses than necessary during init

[Laszlo Ersek]

On 07/26/17 08:48, Marcel Apfelbaum wrote:
> On 25/07/2017 18:46, Laszlo Ersek wrote:

[snip]

>> (2) Bus range reservation, and hotplugging bridges. What's the
>> motivation? Our recommendations in "docs/pcie.txt" suggest flat
>> hierarchies.
>>
> 
> It remains flat. You have one single PCIE-PCI bridge plugged
> into a PCIe Root Port, no deep nesting.
> 
> The reason is to be able to support legacy PCI devices without
> "committing" with a DMI-PCI bridge in advance. (Keep Q35 without)
> legacy hw.
> 
> The only way to support PCI devices in Q35 is to have them cold-plugged
> into the pcie.0 bus, which is good, but not enough for expanding the
> Q35 usability in order to make it eventually the default
> QEMU x86 machine (I know this is another discussion and I am in
> minority, at least for now).
> 
> The plan is:
> Start Q35 machine as usual, but one of the PCIe Root Ports includes
> hints for firmware needed t support legacy PCI devices. (IO Ports range,
> extra bus,...)
> 
> Once a pci device is needed you have 2 options:
> 1. Plug a PCIe-PCI bridge into a PCIe Root Port and the PCI device
>    in the bridge.
> 2. Hotplug a PCIe-PCI bridge into a PCIe Root Port and then hotplug
>    a PCI device into the bridge.

Thank you for the explanation, it makes the intent a lot clearer.

However, what does the hot-pluggability of the PCIe-PCI bridge buy us?
In other words, what does it buy us when we do not add the PCIe-PCI
bridge immediately at guest startup, as an integrated device?

Why is it a problem to "commit" in advance? I understand that we might
not like the DMI-PCI bridge (due to it being legacy), but what speaks
against cold-plugging the PCIe-PCI bridge either as an integrated device
in pcie.0 (assuming that is permitted), or cold-plugging the PCIe-PCI
bridge in a similarly cold-plugged PCIe root port?

I mean, in the cold-plugged case, you use up two bus numbers at the
most, one for the root port, and another for the PCIe-PCI bridge. In the
hot-plugged case, you have to start with the cold-plugged root port just
the same (so that you can communicate the bus number reservation *at
all*), and then reserve (= use up in advance) the bus number, the IO
space, and the MMIO space(s). I don't see the difference; hot-plugging
the PCIe-PCI bridge (= not committing in advance) doesn't seem to save
any resources.

I guess I would see a difference if we reserved more than one bus number
in the hotplug case, namely in order to support recursive hotplug under
the PCIe-PCI bridge. But, you confirmed that we intend to keep the flat
hierarchy (ie the exercise is only for enabling legacy PCI endpoints,
not for recursive hotplug).  The PCIe-PCI bridge isn't a device that
does anything at all on its own, so why not just coldplug it? Its
resources have to be reserved in advance anyway.

So, thus far I would say "just cold-plug the PCIe-PCI bridge at startup,
possibly even make it an integrated device, and then you don't need to
reserve bus numbers (and other apertures)".

Where am I wrong?

[snip]

>> (4) Whether the reservation size should be absolute or relative (raised
>> by Gerd). IIUC, Gerd suggests that the absolute aperture size should be
>> specified (as a minimum), not the increment / reservation for hotplug
>> purposes.
>>
>> The Platform Initialization Specification, v1.6, downloadable at
>> <http://www.uefi.org/specs>, writes the following under
>>
>>    EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()
>>
>> in whose implementation I will have to parse the values from the
>> capability structure, and return the appropriate representation to the
>> platform-independent PciBusDxe driver (i.e., the enumeration /
>> allocation agent):
>>
>>> The padding is returned in the form of ACPI (2.0 & 3.0) resource
>>> descriptors. The exact definition of each of the fields is the same as
>>> in the
>>> EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL.SubmitResources()
>>> function. See the section 10.8 for the definition of this function.
>>>
>>> The PCI bus driver is responsible for adding this resource request to
>>> the resource requests by the physical PCI devices. If Attributes is
>>> EfiPaddingPciBus, the padding takes effect at the PCI bus level. If
>>> Attributes is EfiPaddingPciRootBridge, the required padding takes
>>> effect at the root bridge level. For details, see the definition of
>>> EFI_HPC_PADDING_ATTRIBUTES in "Related Definitions" below.
>>
>> Emphasis on "*adding* this resource request to the resource requests by
>> the physical PCI devices".
>>
>> However... After checking some OVMF logs, it seems that in practice,
>> PciBusDxe does exactly what Gerd suggests.
>>
>> Currently OVMF returns a constant 2MB MMIO padding and a constant 512B
>> IO padding for all hotplug-capable bridges (including PCI Express root
>> ports), from GetResourcePadding(). For example, for the following QEMU
>> command line fragment:
>>
>>    -device
>> pxb-pcie,bus_nr=64,id=rootbr1,numa_node=1,bus=pcie.0,addr=0x3 \
>>    \
>>    -device ioh3420,id=rootbr1-port1,bus=rootbr1,addr=0x1,slot=3 \
>>    -device e1000e,bus=rootbr1-port1,netdev=net0 \
>>    \
>>    -device ioh3420,id=rootbr1-port2,bus=rootbr1,addr=0x2,slot=4 \
>>    -device e1000e,bus=rootbr1-port2,netdev=net1 \
>>
>> PciBusDxe produces the following log (extract):
>>
>>> PciBus: Resource Map for Root Bridge PciRoot(0x40)
>>> Type =   Io16; Base = 0x8000;   Length = 0x2000;        Alignment =
>>> 0xFFF
>>>     Base = 0x8000;       Length = 0x1000;        Alignment =
>>> 0xFFF;      Owner = PPB [40|02|00:**]
>>>     Base = 0x9000;       Length = 0x1000;        Alignment =
>>> 0xFFF;      Owner = PPB [40|01|00:**]
>>> Type =  Mem32; Base = 0x98600000;       Length = 0x400000;     
>>> Alignment = 0x1FFFFF
>>>     Base = 0x98600000;   Length = 0x200000;      Alignment =
>>> 0x1FFFFF;   Owner = PPB [40|02|00:**]
>>>     Base = 0x98800000;   Length = 0x200000;      Alignment =
>>> 0x1FFFFF;   Owner = PPB [40|01|00:**]
>>>
>>> PciBus: Resource Map for Bridge [40|01|00]
>>> Type =   Io16; Base = 0x9000;   Length = 0x1000;        Alignment =
>>> 0xFFF
>>>     Base = Padding;      Length = 0x200; Alignment = 0x1FF
>>>     Base = 0x9000;       Length = 0x20;  Alignment = 0x1F;      
>>> Owner = PCI [41|00|00:18]
>>> Type =  Mem32; Base = 0x98800000;       Length = 0x200000;     
>>> Alignment = 0x1FFFFF
>>>     Base = Padding;      Length = 0x200000;      Alignment = 0x1FFFFF
>>>     Base = 0x98800000;   Length = 0x20000;       Alignment =
>>> 0x1FFFF;    Owner = PCI [41|00|00:14]
>>>     Base = 0x98820000;   Length = 0x20000;       Alignment =
>>> 0x1FFFF;    Owner = PCI [41|00|00:10]
>>>     Base = 0x98840000;   Length = 0x4000;        Alignment =
>>> 0x3FFF;     Owner = PCI [41|00|00:1C]
>>>
>>> PciBus: Resource Map for Bridge [40|02|00]
>>> Type =   Io16; Base = 0x8000;   Length = 0x1000;        Alignment =
>>> 0xFFF
>>>     Base = Padding;      Length = 0x200; Alignment = 0x1FF
>>>     Base = 0x8000;       Length = 0x20;  Alignment = 0x1F;      
>>> Owner = PCI [42|00|00:18]
>>> Type =  Mem32; Base = 0x98600000;       Length = 0x200000;     
>>> Alignment = 0x1FFFFF
>>>     Base = Padding;      Length = 0x200000;      Alignment = 0x1FFFFF
>>>     Base = 0x98600000;   Length = 0x20000;       Alignment =
>>> 0x1FFFF;    Owner = PCI [42|00|00:14]
>>>     Base = 0x98620000;   Length = 0x20000;       Alignment =
>>> 0x1FFFF;    Owner = PCI [42|00|00:10]
>>>     Base = 0x98640000;   Length = 0x4000;        Alignment =
>>> 0x3FFF;     Owner = PCI [42|00|00:1C]
>>
>> This means that
>> - both ioh3420 root ports get a padding of 512B for IO and 2MB fo MMIO,
> 
> So the 512 IO will be reserved even if the IO handling is not enabled
> in the bridge? (I am asking because a similar issue I might have with
> other guest code.)

I apologize for being unclear about the "distribution of work" between
edk2's generic PciBusDxe driver and OVMF's platform-specific
PciHotPlugInitDxe driver (which provides the
EFI_PCI_HOT_PLUG_INIT_PROTOCOL, of which GetResourcePadding() is a
member function).

Basically, PciBusDxe is a platform-independent, universal driver, that
calls into "hooks", optionally provided by the platform, at specific
points in the enumeration / allocation.

EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() is such a platform
specific hook, and OVMF is "reasonably free" to provide PciBusDxe with
reservation hints from it, as OVMF sees fit, for any specific hotplug
controller. (Of course OVMF's freedom is limited by two factors: first
by the information that QEMU exposes to firmware, and second by the
"imperative" that in GetResourcePadding() we really shouldn't look at
any *other* PCI(e) controllers than the exact hotplug controller that
PciBusDxe is asking about.)

With that in mind:

*Currently*, OVMF advises PciBusDxe to "reserve 512B of IO space" for
*any* hotplug controller. (From the above log, we can see that on the
root bridge, this 512B are then rounded up to 4KB.)

*In the future*, OVMF should replace the constant 512B with the
following logic: consult the IO base/limit registers of the subject
hotplug controller, and if they are zero, then return "reserve no IO
space for this hotplug controller" to PciBusDxe. If the base/limit
registers are nonzero, OVMF would say "reserve 4KB IO space", or even
propagate the value from the capability.

[snip]

>> The PI spec says,
>>
>>> [...] For all the root HPCs and the nonroot HPCs, call
>>> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() to obtain the
>>> amount of overallocation and add that amount to the requests from the
>>> physical devices. Reprogram the bus numbers by taking into account the
>>> bus resource padding information. [...]
>>
>> However, according to my interpretation of the source code, PciBusDxe
>> does not consider bus number padding for non-root HPCs (which are "all"
>> HPCs on QEMU).
>>
> 
> Theoretically speaking, it is possible to change the  behavior, right?

Not just theoretically; in the past I have changed PciBusDxe -- it
wouldn't identify QEMU's hotplug controllers (root port, downstream port
etc) appropriately, and I managed to get some patches in. It's just that
the less we understand the current code and the more intrusive/extensive
the change is, the harder it is to sell the *idea*. PciBusDxe is
platform-independent and shipped on many a physical system too.

>> So, I agree that we can add a bus number range field to the capability,
>> but I'm fairly sure it won't work with edk2's PciBusDxe without major
>> surgery. (Given that we haven't ever considered hot-plugging entire
>> bridges until now, i.e., anything that would require bus number
>> reservation, I think we can live with such a limitation when using OVMF,
>> for an indefinite time to come.)
>>
> 
> Since the OVMF will still support cold-plug, I think is OK for now.
> Once the feature gets in SeaBIOS I will open a BZ for tracking.

Please do that (at that time); it will certainly need a dedicated
discussion on edk2-devel.

(Also, your statement about cold-plug being viable for at least a while
is consistent with my questions / implications near the top: what does
the hot-pluggability of the PCIe-PCI bridge buy us ultimately?)

Thanks!
Laszlo