io range is not mandatory according to pcie spec,
so allow bridge configurations without io address
space assigned.

Signed-off-by: Gerd Hoffmann <kraxel@...>
Reviewed-by: Ard Biesheuvel <ardb@...>
---
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index b20bcd310ad5..712662707931 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1085,6 +1085,9 @@ NotifyPhase (
RootBridge->ResAllocNode[Index].Base = BaseAddress;
RootBridge->ResAllocNode[Index].Status = ResAllocated;
DEBUG ((DEBUG_INFO, "Success\n"));
+ } else if ((Index == TypeIo) && (RootBridge->Io.Base == MAX_UINT64)) {
+ /* optional on PCIe */
+ DEBUG ((DEBUG_INFO, "PCI Root Bridge does not provide IO Resources.\n"));
} else {
ReturnStatus = EFI_OUT_OF_RESOURCES;
DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
--
2.35.1

On Fri, 22 Apr 2022 at 09:37, Gerd Hoffmann <kraxel@...> wrote:

io range is not mandatory according to pcie spec,
so allow bridge configurations without io address
space assigned.

Signed-off-by: Gerd Hoffmann <kraxel@...>
Reviewed-by: Ard Biesheuvel <ardb@...>

Could one of the MdeModulePkg maintainers please get this reviewed? Thanks.

---
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index b20bcd310ad5..712662707931 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1085,6 +1085,9 @@ NotifyPhase (
RootBridge->ResAllocNode[Index].Base = BaseAddress;
RootBridge->ResAllocNode[Index].Status = ResAllocated;
DEBUG ((DEBUG_INFO, "Success\n"));
+ } else if ((Index == TypeIo) && (RootBridge->Io.Base == MAX_UINT64)) {
+ /* optional on PCIe */
+ DEBUG ((DEBUG_INFO, "PCI Root Bridge does not provide IO Resources.\n"));
} else {
ReturnStatus = EFI_OUT_OF_RESOURCES;
DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
--
2.35.1

Ard,
can you explain more?

Your code changes the PciHostBridge driver to ignore the failure of IO allocation.
If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation to override the IO request from the devices?

Thanks,
Ray

I thought the patch is from Ard but it was from Gerd.
So, the question is for Gerd.:)

I am a bit nervous on this change because it's a behavior change and may cause
certain devices malfunction and it's a silent failure.

Thanks,
Ray

On Wed, Apr 27, 2022 at 03:08:50AM +0000, Ni, Ray wrote:

Ard,
can you explain more?

Your code changes the PciHostBridge driver to ignore the failure of IO allocation.
If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation to override the IO request from the devices?

Hmm, it's a problem indeed, device initialization fails in case an
io bar is present even if the bar is not required to drive the device.

Suggestions how to deal with this best? ovmf has it's own
IncompatiblePciDevice Protocol implementation, so I could
handle it there because only OvmfPkg/Microvm needs this.

Or should the MdeModulePkg version be updated too?

thanks,
Gerd

On Fri, 29 Apr 2022 at 08:50, Gerd Hoffmann <kraxel@...> wrote:

On Wed, Apr 27, 2022 at 03:08:50AM +0000, Ni, Ray wrote:

Ard,
can you explain more?

Your code changes the PciHostBridge driver to ignore the failure of IO allocation.
If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation to override the IO request from the devices?

Hmm, it's a problem indeed, device initialization fails in case an
io bar is present even if the bar is not required to drive the device.

I'd say the risk for regressions is rather low, though, given that it
only affects configurations that would fail PCI resource allocation
today. Or am I missing something?

In any case, the PCIe spec is clear about this: I/O space is optional,
and we need to incorporate this into the generic code at *some* point.
It makes no sense for every individual platform to keep adding these
hacks.

Suggestions how to deal with this best? ovmf has it's own
IncompatiblePciDevice Protocol implementation, so I could
handle it there because only OvmfPkg/Microvm needs this.

Or should the MdeModulePkg version be updated too?

I'd say we do both, to avoid stalling your series forever :-)

Hi,

I'd say the risk for regressions is rather low, though, given that it
only affects configurations that would fail PCI resource allocation
today. Or am I missing something?

In any case, the PCIe spec is clear about this: I/O space is optional,
and we need to incorporate this into the generic code at *some* point.
It makes no sense for every individual platform to keep adding these
hacks.

Do you know how Linux handles this?
Can Linux allocate resource for PCI(E) devices? How does it deal with the IO type?

Yes. Details depend a bit on the specific configuration, but in general
linux will try assign io address space to pcie root ports and devices
plugged into those ports. A failure is not considered fatal though.

A more common case than the pci root bridge not supporting io address
space at all is having more than 16 pcie root ports. Given io bride
windows are 1k in size and we have 16k total there is simply not enough
io address space in that case, so some of the root ports stay without
io and linux is fine with that.

Why changing the MdeModulePkg's IncompatiblePciDevice driver can avoid
stalling the patch series?
I feel it's enough to just change the OvmfPkg version.

It's not much of a problem for ovmf even without such an update,
typically the devices used with microvm don't have io bars in the first
place.

Also note that without this series pcie devices are not supported at all
on microvm, so not supporting all devices initially wouldn't be a
regression.

I'll look into it in any case.

take care,
Gerd

Hi,

I'd say the risk for regressions is rather low, though, given that it
only affects configurations that would fail PCI resource allocation
today. Or am I missing something?

In any case, the PCIe spec is clear about this: I/O space is optional,
and we need to incorporate this into the generic code at *some* point.
It makes no sense for every individual platform to keep adding these
hacks.

Do you know how Linux handles this?
Can Linux allocate resource for PCI(E) devices? How does it deal with the IO type?

Yes. Details depend a bit on the specific configuration, but in general
linux will try assign io address space to pcie root ports and devices
plugged into those ports. A failure is not considered fatal though.

An error message and continue?

A more common case than the pci root bridge not supporting io address
space at all is having more than 16 pcie root ports. Given io bride
windows are 1k in size and we have 16k total there is simply not enough
io address space in that case, so some of the root ports stay without
io and linux is fine with that.

Does it have some certain policy that IO resource for first root bridge should
be satisfied?

Why changing the MdeModulePkg's IncompatiblePciDevice driver can avoid
stalling the patch series?
I feel it's enough to just change the OvmfPkg version.

It's not much of a problem for ovmf even without such an update,
typically the devices used with microvm don't have io bars in the first
place.

Also note that without this series pcie devices are not supported at all
on microvm, so not supporting all devices initially wouldn't be a
regression.

I'll look into it in any case.

The safest way is to change OVMF now.
Add @Nong, Foster and @Albecki, Mateusz for comments.

take care,
Gerd

Hi,

Can Linux allocate resource for PCI(E) devices? How does it deal with the IO type?

Yes. Details depend a bit on the specific configuration, but in general
linux will try assign io address space to pcie root ports and devices
plugged into those ports. A failure is not considered fatal though.

An error message and continue?

Not even an error message. In case the pci core code assigns a io
window to the pci root port it will log a message saying so. In case
it doesn't it stays silent.

A more common case than the pci root bridge not supporting io address
space at all is having more than 16 pcie root ports. Given io bride
windows are 1k in size and we have 16k total there is simply not enough
io address space in that case, so some of the root ports stay without
io and linux is fine with that.

Does it have some certain policy that IO resource for first root bridge should
be satisfied?

I don't know for sure. From the boot logs it looks like the kernel
simply assigns resources in pci scan order, and when it runs out of
resources it stops assigning.

take care,
Gerd

Hi,

If IO requirement of certain PCI(E) devices can be ignored, can you change the IncompatiblePciDevice protocol implementation to override the IO request from the devices?

Hmm, how can the IncompatiblePciDevice protocol specify that IO bars
should be ignored?

Seems I can override the size using
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR.AddrLen. I can't set the length to
zero though because setting AddrLen to 0 means "no overide".

Is there another way to have edk2 ignore an PCI bar? The spec isn't
verbose here (looking at PI spec 1.7a, table 5-20).

thanks,
Gerd

@Ni, Ray

I think EDK2 needs to provide a way for root port to operate without IO space assigned in a platform-independent way. I can think of the following cases when root port didn't get IO space:

1. We have run out of IO space but it's fine since the device under the root port doesn't use IO or has only non-critical functionalities under IO
2. We have run out of IO space and it's really not fine since device needs IO
3. We are running on a CPU which doesn't support IO

For 1. the question is whether the device driver in EDK2 understands that IO bar for that device is optional and will bother to check if it has been assigned and either fail gracefully or continue operation in limited capacity. For 2. the question is whether the driver will fail gracefully. 3 is for completeness at this point I think since the only other architecture that uses EDK2 is ARM which has to deal with it in some way right now which I think maps IO region into MMIO so in a way it supports IO.

I've checked the device driver behavior in EDK2 for devices which use IO bar here is the rundown:
1. IDE - Doesn't check if IO has been assigned, not giving IO results in undefined behavior
2. SerialIo -> Doesn't check, will assert the system when IO is not assigned (although the logic there is really strange as it can use 3 different access methods)
3. UHCI -> Checks but too late, will most likely result in undefined behavior

Even with those bad device drivers I would agree that taking this change presents low risk given that those devices are pretty old and should be mostly unused on new systems(SerialIo being an exception but that one is usually an RCIEP). That said I think we are missing a larger issue here - why are we running out of IO when we have 16 root ports? Surely we don't have a device with IO requirement behind each of those root ports so is the BIOS blindly assigning IO to root ports which have no requirement? I see on my system that when we don't have IO requirement behind the root port BIOS sets IOBASE to 0xF0 and IOLIMIT to 0x0 which means no IO decode will be performed.

Thanks,
Mateusz

On Mon, May 23, 2022 at 04:48:05AM -0700, Albecki, Mateusz wrote:

@Ni, Ray

I think EDK2 needs to provide a way for root port to operate without IO space assigned in a platform-independent way. I can think of the following cases when root port didn't get IO space:

1. We have run out of IO space but it's fine since the device under the root port doesn't use IO or has only non-critical functionalities under IO
2. We have run out of IO space and it's really not fine since device needs IO
3. We are running on a CPU which doesn't support IO

For 1. the question is whether the device driver in EDK2 understands that IO bar for that device is optional and will bother to check if it has been assigned and either fail gracefully or continue operation in limited capacity. For 2. the question is whether the driver will fail gracefully. 3 is for completeness at this point I think since the only other architecture that uses EDK2 is ARM which has to deal with it in some way right now which I think maps IO region into MMIO so in a way it supports IO.

Well, the case I'm trying to handle here is qemu microvm. It's x86, but
io address space support for pcie devices is not wired up. So the pcie
host bridge doesn't support io, which is rather close to case (3).

I've checked the device driver behavior in EDK2 for devices which use IO bar here is the rundown:
1. IDE - Doesn't check if IO has been assigned, not giving IO results in undefined behavior
2. SerialIo -> Doesn't check, will assert the system when IO is not assigned (although the logic there is really strange as it can use 3 different access methods)
3. UHCI -> Checks but too late, will most likely result in undefined behavior

Current edk2 behavior is that the initialization of the pcie host bridge
fails in case no io space is present (and all devices connected to it are
not initialized either of course).

With this patch applied pcie host bridge initialization works. PCIe
devices without io bars are enumerated and initialized sucessfully.
PCIe devices with io bar fail to initialize. That isn't much of a
problem tough as a qemu microvm typically has no pcie devices with io
bars.

Even with those bad device drivers I would agree that taking this
change presents low risk given that those devices are pretty old and
should be mostly unused on new systems(SerialIo being an exception but
that one is usually an RCIEP).

Also note that for pcie root bridges which do support io address space
this patch changes nothing.

That said I think we are missing a larger issue here - why are we
running out of IO when we have 16 root ports?

I don't think so. I see the *linux kernel* hand out io address space to
pcie root ports (until it runs out). edk2 doesn't.

take care,
Gerd

On Mon, May 23, 2022 at 11:24 PM, Gerd Hoffmann wrote:

On Mon, May 23, 2022 at 04:48:05AM -0700, Albecki, Mateusz wrote:

@Ni, Ray

I think EDK2 needs to provide a way for root port to operate without IO space assigned in a platform-independent way. I can think of the following cases when root port didn't get IO space:

1. We have run out of IO space but it's fine since the device under the root port doesn't use IO or has only non-critical functionalities under IO
2. We have run out of IO space and it's really not fine since device needs IO
3. We are running on a CPU which doesn't support IO

For 1. the question is whether the device driver in EDK2 understands that IO bar for that device is optional and will bother to check if it has been assigned and either fail gracefully or continue operation in limited capacity. For 2. the question is whether the driver will fail gracefully. 3 is for completeness at this point I think since the only other architecture that uses EDK2 is ARM which has to deal with it in some way right now which I think maps IO region into MMIO so in a way it supports IO.

Well, the case I'm trying to handle here is qemu microvm. It's x86, but
io address space support for pcie devices is not wired up. So the pcie
host bridge doesn't support io, which is rather close to case (3).

I've checked the device driver behavior in EDK2 for devices which use IO bar here is the rundown:
1. IDE - Doesn't check if IO has been assigned, not giving IO results in undefined behavior
2. SerialIo -> Doesn't check, will assert the system when IO is not assigned (although the logic there is really strange as it can use 3 different access methods)
3. UHCI -> Checks but too late, will most likely result in undefined behavior

Current edk2 behavior is that the initialization of the pcie host bridge
fails in case no io space is present (and all devices connected to it are
not initialized either of course).

With this patch applied pcie host bridge initialization works. PCIe
devices without io bars are enumerated and initialized sucessfully.
PCIe devices with io bar fail to initialize. That isn't much of a
problem tough as a qemu microvm typically has no pcie devices with io
bars.

You mention that devices with IO bar fail to initialize but that is contrary to what I would expect from code review. I've run an experiment with your change in which I am telling
EDK2 that no IO space is available on the system by not updating the IO range in PciHostBridgeLib. Sure enough Devices that need an IO are still enumerated, device path and PciIo are installed
and in general everything works as it used to. If I had an UHCI controller on that system UHCI driver would be loaded and it could potentially result in some strange behavior since that driver isn't smart enough to check
if IO space has been allocated for the device.

To make things worse I see that if we return success there EDK2 will actually go ahead and start assigning trash addresses to the device and enable IO space decoding in case of the PCI root port which means that device will try to decode
invalid IO ranges. Not an issue for a system without an IO but for a system in which we have run out of the IO and we have entered this code branch this new behavior is potentially more dangerous then simply not enumerating the device.

Logs:

Even with those bad device drivers I would agree that taking this
change presents low risk given that those devices are pretty old and
should be mostly unused on new systems(SerialIo being an exception but
that one is usually an RCIEP).

Also note that for pcie root bridges which do support io address space
this patch changes nothing.

It seems to me like it does. Specifically the error scenario where the system has run out of IO space will not be handled properly I think.

That said I think we are missing a larger issue here - why are we
running out of IO when we have 16 root ports?

I don't think so. I see the *linux kernel* hand out io address space to
pcie root ports (until it runs out). edk2 doesn't.

Ok I misunderstood previous mails

take care,
Gerd

I think to really handle it we would have to have a more involved change. Specifically in the PciHostBridge.c:NoitfyPhase function we need to have a way to tell the PciBus driver which resources specifically failed to allocate and treat this as a condition we need to handle instead of panicking and asserting the system or dropping the entire host bridge. When PciBus driver sees that IO failed to allocate it would skip IO bars and would not allow to set the IO space enable bit in the root bridge. We also would need to change the logic in PciResourceSupport.c:ProgramBar because that function is very optimistic and assumes that if we were able to program one BAR then surely all resources for the device are allocated:


Thanks,
Mateusz

Hi,

To make things worse I see that if we return success there EDK2 will
actually go ahead and start assigning trash addresses to the device
and enable IO space decoding in case of the PCI root port which means
that device will try to decode invalid IO ranges.

Logs:
PciBus: Resource Map for Root Bridge PciRoot(0x0)
Type =   Io16; Base = 0xFFFFFFFFFFFFFFFF; Length = 0x1000; Alignment = 0xFFF

That is wrong indeed.

I think *this* should work ...

+ } else if ((Index == TypeIo) &&
+ (RootBridge->Io.Base == MAX_UINT64) &&
+ (RootBridge->ResAllocNode[Index].Length == 0)) {
+ /* I/O is optional on PCIe */
+ DEBUG ((DEBUG_INFO, "Success (PCIe NoIO)\n"));

... i.e. return success only in case there are no allocation requests
for IO ranges.

Also note that for pcie root bridges which do support io address space
this patch changes nothing.

It seems to me like it does.

It doesn't. When io address space is present the "RootBridge->Io.Base
== MAX_UINT64" check will never be true.

But the "no io address space" case was wrong indeed.

I think to really handle it we would have to have a more involved
change.

If we want support PCIe devices with I/O bars behind a PCIe host bridge
without I/O window, then yes. My main focus is supporting PCIe devices
without I/O bars though. This doesn't work currently because the code
considers a pcie host bridge without I/O window a hard failure even in
case there are no I/O allocation requests. For fixing that the five
lines above should be enough I think.

thanks & take care,
Gerd

This doesn't work currently because the code
considers a pcie host bridge without I/O window a hard failure even in
case there are no I/O allocation requests. For fixing that the five
lines above should be enough I think.

That's a real bug we should fix. I agree!!