MdeModulePkg/PciBusDxe: Pcd to ignore I/O Space BARs.

[josephoresko]

Description

PCIe enumeration fails if an endpoint requests I/O space BARs if the platform does not support I/O space. While I/O space is traditionally supported in x86_64, platforms using an Arm architecture may or may not support I/O space. In the case where I/O space is not supported, the platform needs a mechanism to decide whether to gracefully ignore I/O space BARs instead of PciBus bailing out during enumeration.

This change adds PcdPciIgnoreIoSpaceBars to allow the platform to specify that I/O space BARs should be ignored. Endpoints will not receive I/O resources, but they will continue to receive Memory BAR resources.

• Breaking change?
• Impacts security?
• Includes tests?

How This Was Tested

ARM64 platform boot with a mix of PCIe endpoints that request and do not request I/O Space BARs

Integration Instructions

N/A

[josephoresko]

PR submitted prematurely; working on updating PR description

[leiflindholm]

PR submitted prematurely; working on updating PR description

Looking good now.

[leiflindholm]

@makubacki @Javagedes This is an RFC for an alternative approach to solving the same problem as #5853 and #5916 intended to.

[Javagedes]

I am fine with this design over mine.

[makubacki]

A PCD approach is fine with me.

[ardbiesheuvel]

This looks dodgy to me: you are using the PCD to force the else branch to be taken, and so you are relying on the switch statement there to treat the 0x1 value as BarTypeUnknown. This might work for your use case, but it is not very clean.

I'd prefer it if, rather than ignoring I/O BARs altogether, we could introduce a PCD that defines the behavior when the system runs out of I/O resources while allocating I/O BARs.

I/O BARs are generally only used on PCIe legacy endpoints, and on SATA and VGA devices in particular which implement IDE resp. VGA legacy compatibility on PCs. But there might be valid use cases too for I/O BARs, and those should be supported on non-x86 systems that happen to provide an port I/O resource window, and preferably without having to remember setting a PCD to the appropriate value.

So whenever an I/O BAR allocation fails, we should ignore this for VGA and SATA class endpoints unless the arch is x86. If we could wire this up to a PCD whose default is OFF for x86 and ON for everything, x86 can opt into this behavior on platforms that lack port I/O, but other than that, the default behavior would be sane across all architectures.

[josephoresko]

Thanks for the feedback. First, I've amended the existing commit with what I hope is a more robust PCD check (rather than relying on a later portion of the code to set BarTypeUnknown, as you have pointed out). This can serve as a baseline to compare additional approaches against later.

Regarding defining the behavior when I/O Space resource are exhausted, I view that as separate from defining the behavior when I/O Space is not supported. Also, there already appears to be a policy in place for that case. PciHostBridgeAdjustAllocation appears to handle the out of resource case by failing individual devices, but if the Host Bridge doesn't support the requested resource then EFI_ABORTED is returned rather than failing a particular device, which causes enumeration to bail out for all root bridges.

If the platform doesn't support IO Space, this should be known in PciHostBridgeLib, where Io.Base will be greater than Io.Limit. That's the case where I envision using PcdPciIgnoreIoSpaceBars. It may be a better fit to move the decision and logic for ignoring I/O Space requests out of the generic PciBusDxe and into PciHostBridgeDxe, where the policy could be either governed by PcdPciIgnoreIoSpaceBars (or perhaps configured directly via PciHostBridgeLib, e.g. through a resource allocation flag). What are your thoughts?

[ardbiesheuvel]

Fair point re exhausted vs unsupported. But if the latter case is as easily detected as you suggest, why do we need the PCD in the first place? If no I/O resource window has been provided in the first place, we should simply leave the I/O BARs unassigned. I'd argue that most UEFI drivers won't even notice, as they don't use the legacy port I/O anyway.

What I would like to avoid is a PCD with a default value on ARM that will result in systems that do support port I/O being silently broken, in a way that will only be noticed when someone plus in, e.g., a serial UART card (@leiflindholm kindly gifted me one to take to UEFI plugfests years ago).

IOW, the behavior of failing an individual based on insufficient port I/O resources should be predicated on whether or not any I/O resources exist on the system to begin with.

[leiflindholm]

What I would like to avoid is a PCD with a default value on ARM that will result in systems that do support port I/O being silently broken, in a way that will only be noticed when someone plus in, e.g., a serial UART card (@leiflindholm kindly gifted me one to take to UEFI plugfests years ago).

I agree with those design goals.
But I'd like to point out that the implementation in this PR defaults to not ignoring I/O BARs, on any architecture.

[ardbiesheuvel]

Sure. But the PCD is a footgun, as setting it on a platform that does support port I/O will silently break it. And note that a platform could implement more than one host bridge, in which case setting the PCD fixes the issue on one host bridge while breaking the other.

[josephoresko]

Thanks for the feedback. I am all for removing the PCD as long as we can agree it is acceptable to ignore I/O Space BARs when PciHostBridgeLib sets Io.Base greater than Io.Limit, i.e. the host bridge doesn't support I/O Space resources. Are there any objections to this?

[lersek]

(Admittedly drive-by comment.)

My understanding is that a PCI Express endpoint is supposed to work properly even if it's IO regions cannot be assigned.

I vaguely recall that PciBusDxe parses the express capability early on, and remembers whether the device is express or not. (... See CreatePciIoDevice() in PciEnumeratorSupport.c, the assignments to PciIoDevice->IsPciExp.) Thus, the idea would be to consult this flag when trying and failing to allocate IO space for the device, and to ignore the error when the device is express.

Unfortunately, the data flow in practice doesn't seem to permit this. Note step 8 at https://uefi.org/specs/PI/1.9/V5_PCI_HostBridge.html#sample-implementation (which PciBusDxe follows, to my understanding):

[...] Scan all the devices in the specified bus range and on the specified segment. [...] If it is an ordinary device, collect the resource request and add up all of these requests in multiple pools (e.g., I/O, 32-bit prefetchable memory). [...]

The problem (AIUI) is that resource requests from multiple devices are pooled -- and the specific request provenance is lost -- before the actual allocation is attempted. PciHostBridgeResourceAllocator() is what appears to perform this pooling (first into AcpiConfig, then translated into the various XxxResStatus variables). PciHostBridgeAdjustAllocation() gets no individual device information, only the lump sum for each resource type. If the root bridge doesn't support a particular resource type, that's where we bail with EFI_ABORTED. If the resource type is supported but there isn't enough of it, then we arbitrarily pick a victim device -- it happens to be the hungriest device, found with GetMaxResourceConsumerDevice() --, and reject it.

This approach seems a bit backwards for what's needed here. Upon finding that the root bridge doesn't support IO space, we'd have to iterate over all the PCI express devices under that root bridge, and hide their IO bars now. Then let the loop in the caller, namely PciHostBridgeResourceAllocator(), run for another iteration. This corresponds to step 12 under the above link, I believe.

It looks complicated. Could we inform the PciHostBridgeResourceAllocator() -> CreateResourceMap() -> GetResourceFromDevice() call chain as to whether the root bridge exposes an IO window? If it doesn't, then GetResourceFromDevice() could hide (= not report) the IO bars of each PCI Express device.

Or perhaps we can decide about hiding IO bars as early as in PciParseBar(), after all: can we pass the same root bridge IO aperture availability along PciHostBridgeEnumerator() -> PciPciDeviceInfoCollector() -> PciSearchDevice() -> GatherDeviceInfo() -> PciParseBar()?

[ardbiesheuvel]

Hi Laszlo,

Thanks for the insight.

My understanding is that a PCI Express endpoint is supposed to work properly even if it's IO regions cannot be assigned.

You mean legacy endpoint, right? IIRC, those are the only ones that are permitted to have I/O BARs in the first place, and so your statement implies that all PCIe endpoints are supposed to work properly under this condition.

Or perhaps we can decide about hiding IO bars as early as in PciParseBar(), after all: can we pass the same root bridge IO aperture availability along PciHostBridgeEnumerator() -> PciPciDeviceInfoCollector() -> PciSearchDevice() -> GatherDeviceInfo() -> PciParseBar()?

This is what the patch already does, as far as I can tell. But instead of the PCD, I'd like the condition to be tested to be whether on this particular host bridge, there is no I/O resource window to begin with.

[lersek]

Hi Ard,

so your statement implies that all PCIe endpoints are supposed to work properly under this condition

Yes! That's what I meant.

This is what the patch already does, as far as I can tell.

Not exactly; what I tried to say was that we should somehow fetch the IO aperture's availability in PciHostBridgeEnumerator() for each root bridge in turn, and pass that info down to PciParseBar(). The patch (its current version) does not propagate the info downwards like that; it consumes the PCD at the lowest level.

I'd like the condition to be tested to be whether on this particular host bridge, there is no I/O resource window to begin with.

Absolutely.

(Please excuse me if I'm rusty in the argument below.)

The problem is that this information is maintained inside the EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL implementation, and PciBusDxe can communicate with that protocol via member functions that are (apparently) proving "too rigid" now.

MdeModulePkg/Bus/Pci/PciHostBridgeDxe consumes the platform's PciHostBridgeLib instance (which is where the platform exposes whether each of its root bridges provides an IO aperture or not), and the only way PciHostBridgeDxe makes that information available (very indirectly) to PciBusDxe is via EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL. This restriction on the data flow / data representation is our problem. We want PciBusDxe to have more direct insight into the IO aperture (if any) of each root bridge.

... Well, how about the following?

1. PciHostBridgeEnumerator() already loops over all the root bridges using EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL.GetNextRootBridge(). This loop is performed multiple times in fact; for now we care about the one loop that calls PciPciDeviceInfoCollector() in its body.
2. In said loop body, we could open EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL on the handle output by EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL.GetNextRootBridge().
3. Call EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Configuration(). This is supposed to output the set of ACPI resource descriptors corresponding to the current configuration of the root bridge.
4. Parse those ACPI resource descriptors to see if the bridge supports an IO aperture.
5. Pass the result (yes/no) down via PciPciDeviceInfoCollector() -> PciSearchDevice() -> GatherDeviceInfo() -> PciParseBar().

[josephoresko]

Thanks Laszlo for the input. Let me spend some time exploring this further and tuning the patch.

[github-actions]

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

[github-actions]

This pull request has been automatically been closed because it did not have any activity in 60 days and no follow up within 7 days after being marked stale. Thank you for your contributions.

[tvidyasagar]

@josephoresko First of all, thanks for your patch and I verified it to be working as expected on one of ARM64 platforms.
What is the next step for this patch to get merged? Are you planning to revive this patch and send it for review again?

[josephoresko]

@tvidyasagar, in terms of next steps, I want to explore some of the suggestions from @lersek and compare them against what I've proposed here. This will take some time.

[github-actions]

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

[github-actions]

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

[github-actions]

This pull request has been automatically been closed because it did not have any activity in 60 days and no follow up within 7 days after being marked stale. Thank you for your contributions.

[mergify]

PR can not be merged due to conflict. Please rebase and resubmit

[michaelescue]

@lersek, @ardbiesheuvel,

Thank you for your insights and suggestions. We did a lot of prototyping with your suggestions and were unable to come up with a reasonable solution which maintained the original intent of this PR. Ignoring the I/O BARs at the bar parsing level won't work without significant architectural changes to PciHostBridgeDxe and produced protocols.

Call EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Configuration(). This is supposed to output the set of ACPI resource descriptors corresponding to the current configuration of the root bridge.

The primary conflict with using EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Configuration() to detect I/O space support is when allocating resources dynamically (PCI_ROOT_BRIDGE.ResourceAssigned == FALSE). ACPI address space descriptors are only returned if PCI_RES_NODE.Status == ResAllocated, but when allocating dynamically, all resource nodes are initially ResNone until the EfiPciHostBridgeAllocateResources phase where resources are allocated (post BAR parsing). So when PciParseBar invokes this function, it will appear as if I/O is never supported regardless of its configuration

A lot of thought went into how to work around this, but ultimately this is a resource allocation conflict problem and not a BAR parsing problem. Since PciHostBridgeDxe is concerned with resource allocation and encapsulates the PCI_ROOT_BRIDGE_INSTANCE structures, I feel the following proposed solution seemed appropriate; allow the PciBusDxe to describe the I/O space requirements and then conditionally reject submitting them in the PciHostBridgeDxe scope.

This PR #11776 has been created to suggest this alternative approach. Your feedback and discussion would be appreciated. Thanks!

Michael

[josephoresko]

Thanks @michaelescue for the alternative approach and for prototyping the suggestions from @lersek and @ardbiesheuvel.