[PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory


Gerd Hoffmann
 

io range is not mandatory according to pcie spec, so allow
pcie host bridge configurations without io window in case
there are no io reservations.

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

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index b20bcd310ad5..354be6dbb313 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1085,6 +1085,12 @@ 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) &&
+ (RootBridge->ResAllocNode[Index].Length == 0))
+ {
+ /* I/O is optional on PCIe */
+ DEBUG ((DEBUG_INFO, "Success (PCIe NoIO)\n"));
} else {
ReturnStatus = EFI_OUT_OF_RESOURCES;
DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
--
2.36.1


Ni, Ray
 

Gerd,
The fix should work. But I am curious why the if in blow is not hit.
https://github.com/TianoCore/edk2/blob/master/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c#L922

The RootBridge->ResAllocNode[TypeIo].Status is assigned as ResNone in:
https://github.com/TianoCore/edk2/blob/master/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c#L263

Or maybe your OVMF platform doesn't execute the above code path?

Then I guess maybe the Base/Limit of Aperture is not set properly so that below if is hit?
https://github.com/tianocore/edk2/blob/64706ef761273ba403f9cb3b7a986bfb804c0a87/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c#L249

Thanks,
Ray

-----Original Message-----
From: Gerd Hoffmann <kraxel@...>
Sent: Thursday, June 2, 2022 4:42 PM
To: devel@edk2.groups.io
Cc: Wu, Hao A <hao.a.wu@...>; Pawel Polawski <ppolawsk@...>; Ard Biesheuvel <ardb+tianocore@...>;
Albecki, Mateusz <mateusz.albecki@...>; Chang, Abner <abner.chang@...>; Ni, Ray <ray.ni@...>; Leif
Lindholm <quic_llindhol@...>; Yao, Jiewen <jiewen.yao@...>; Oliver Steffen <osteffen@...>; Gao,
Liming <gaoliming@...>; Gerd Hoffmann <kraxel@...>; Wang, Jian J <jian.j.wang@...>; Justen,
Jordan L <jordan.l.justen@...>; Ard Biesheuvel <ardb@...>
Subject: [PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory

io range is not mandatory according to pcie spec, so allow
pcie host bridge configurations without io window in case
there are no io reservations.

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

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index b20bcd310ad5..354be6dbb313 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1085,6 +1085,12 @@ 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) &&
+ (RootBridge->ResAllocNode[Index].Length == 0))
+ {
+ /* I/O is optional on PCIe */
+ DEBUG ((DEBUG_INFO, "Success (PCIe NoIO)\n"));
} else {
ReturnStatus = EFI_OUT_OF_RESOURCES;
DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
--
2.36.1


Ard Biesheuvel
 

On Thu, 2 Jun 2022 at 12:14, Ni, Ray <ray.ni@...> wrote:

Gerd,
The fix should work. But I am curious why the if in blow is not hit.
https://github.com/TianoCore/edk2/blob/master/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c#L922

The RootBridge->ResAllocNode[TypeIo].Status is assigned as ResNone in:
https://github.com/TianoCore/edk2/blob/master/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c#L263

Or maybe your OVMF platform doesn't execute the above code path?

Then I guess maybe the Base/Limit of Aperture is not set properly so that below if is hit?
https://github.com/tianocore/edk2/blob/64706ef761273ba403f9cb3b7a986bfb804c0a87/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c#L249
I did a quick test with both ArmVirtQemu and microvm (using this
series but omitting the MdeModulePkg), and I can confirm that not
having a I/O resource window at all seems to work fine if none of the
PCI devices have I/O BARs.

Gerd, do you remember why exactly this patch is needed? Is it related
to devices that have I/O BARs but don't actually require them to
function correctly?


Gerd Hoffmann
 

Hi,

I did a quick test with both ArmVirtQemu and microvm (using this
series but omitting the MdeModulePkg), and I can confirm that not
having a I/O resource window at all seems to work fine if none of the
PCI devices have I/O BARs.

Gerd, do you remember why exactly this patch is needed? Is it related
to devices that have I/O BARs but don't actually require them to
function correctly?
Well, the difference seem to be pcie root ports. When plugging my
virtio device into the root bus everything is fine:

PCI Bus First Scanning
PciBus: Discovered PCI @ [00|00|00]

PciBus: Discovered PCI @ [00|01|00]
BAR[1]: Type = Mem32; Alignment = 0xFFF; Length = 0x1000; Offset = 0x14
BAR[4]: Type = PMem64; Alignment = 0x3FFF; Length = 0x4000; Offset = 0x20
[ ... ]
PciHostBridge: NotifyPhase (AllocateResources)
RootBridge: PciRoot(0x0)
Mem64: Base/Length/Alignment = 6000000000/100000/FFFFF - Success
Mem: Base/Length/Alignment = C0000000/100000/FFFFF - Success
PciBus: HostBridge->NotifyPhase(AllocateResources) - Success

When plugging the virtio device into a pcie root port it doesn't work
and the log looks like this:

PCI Bus First Scanning
PciBus: Discovered PCI @ [00|00|00]

PciBus: Discovered PPB @ [00|08|00]
Padding: Type = Mem32; Alignment = 0x1FFFFF; Length = 0x200000
Padding: Type = Io; Alignment = 0x1FF; Length = 0x200
BAR[0]: Type = Mem32; Alignment = 0xFFF; Length = 0x1000; Offset = 0x10

PciBus: Discovered PCI @ [01|00|00]
BAR[1]: Type = Mem32; Alignment = 0xFFF; Length = 0x1000; Offset = 0x14
BAR[4]: Type = PMem64; Alignment = 0x3FFF; Length = 0x4000; Offset = 0x20
[ ... ]
PciHostBridge: NotifyPhase (AllocateResources)
RootBridge: PciRoot(0x0)
Mem: Base/Length/Alignment = C0000000/300000/1FFFFF - Success
Mem64: Base/Length/Alignment = 6000000000/100000/FFFFF - Success
I/O: Base/Length/Alignment = FFFFFFFFFFFFFFFF/1000/FFF - Out Of Resource!
[ ... ]
PciHostBridge: NotifyPhase (AllocateResources)
RootBridge: PciRoot(0x0)
Mem64: Base/Length/Alignment = 6000000000/100000/FFFFF - Success
Mem: Base/Length/Alignment = C0000000/200000/FFFFF - Success
I/O: Base/Length/Alignment = FFFFFFFFFFFFFFFF/0/FFF - Out Of Resource!

So, it's apparently the io window of the pcie root port which causes
edk2 try allocate io resources.

take care,
Gerd


Ard Biesheuvel
 

On Thu, 2 Jun 2022 at 15:15, Gerd Hoffmann <kraxel@...> wrote:

Hi,

I did a quick test with both ArmVirtQemu and microvm (using this
series but omitting the MdeModulePkg), and I can confirm that not
having a I/O resource window at all seems to work fine if none of the
PCI devices have I/O BARs.

Gerd, do you remember why exactly this patch is needed? Is it related
to devices that have I/O BARs but don't actually require them to
function correctly?
Well, the difference seem to be pcie root ports. When plugging my
virtio device into the root bus everything is fine:

PCI Bus First Scanning
PciBus: Discovered PCI @ [00|00|00]

PciBus: Discovered PCI @ [00|01|00]
BAR[1]: Type = Mem32; Alignment = 0xFFF; Length = 0x1000; Offset = 0x14
BAR[4]: Type = PMem64; Alignment = 0x3FFF; Length = 0x4000; Offset = 0x20
[ ... ]
PciHostBridge: NotifyPhase (AllocateResources)
RootBridge: PciRoot(0x0)
Mem64: Base/Length/Alignment = 6000000000/100000/FFFFF - Success
Mem: Base/Length/Alignment = C0000000/100000/FFFFF - Success
PciBus: HostBridge->NotifyPhase(AllocateResources) - Success

When plugging the virtio device into a pcie root port it doesn't work
and the log looks like this:

PCI Bus First Scanning
PciBus: Discovered PCI @ [00|00|00]

PciBus: Discovered PPB @ [00|08|00]
Padding: Type = Mem32; Alignment = 0x1FFFFF; Length = 0x200000
Padding: Type = Io; Alignment = 0x1FF; Length = 0x200
BAR[0]: Type = Mem32; Alignment = 0xFFF; Length = 0x1000; Offset = 0x10

PciBus: Discovered PCI @ [01|00|00]
BAR[1]: Type = Mem32; Alignment = 0xFFF; Length = 0x1000; Offset = 0x14
BAR[4]: Type = PMem64; Alignment = 0x3FFF; Length = 0x4000; Offset = 0x20
[ ... ]
PciHostBridge: NotifyPhase (AllocateResources)
RootBridge: PciRoot(0x0)
Mem: Base/Length/Alignment = C0000000/300000/1FFFFF - Success
Mem64: Base/Length/Alignment = 6000000000/100000/FFFFF - Success
I/O: Base/Length/Alignment = FFFFFFFFFFFFFFFF/1000/FFF - Out Of Resource!
[ ... ]
PciHostBridge: NotifyPhase (AllocateResources)
RootBridge: PciRoot(0x0)
Mem64: Base/Length/Alignment = 6000000000/100000/FFFFF - Success
Mem: Base/Length/Alignment = C0000000/200000/FFFFF - Success
I/O: Base/Length/Alignment = FFFFFFFFFFFFFFFF/0/FFF - Out Of Resource!

So, it's apparently the io window of the pcie root port which causes
edk2 try allocate io resources.
This seems to be related to the padding logic, i.e., we are trying to
preserve some extra I/O space for the root port in case we hotplug
something that might need it.

The hack below gets around this - we'll need something suitable check
here that avoids I/O padding when the root port has not I/O resource
window in the first place. Care to cook something up?



--- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
+++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
@@ -733,7 +733,7 @@ GetResourcePadding (
}
}

- if (DefaultIo) {
+ if (DefaultIo && FALSE) {
//
// Request defaults.
//


Gerd Hoffmann
 

Hi,

This seems to be related to the padding logic, i.e., we are trying to
preserve some extra I/O space for the root port in case we hotplug
something that might need it.
--- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
+++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
@@ -733,7 +733,7 @@ GetResourcePadding (
}
}

- if (DefaultIo) {
+ if (DefaultIo && FALSE) {
//
// Request defaults.
//
Oh, *there* it comes from. Given this is configurable already we can
fix that one in qemu with a microvm tweak:

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 4b3b1dd262f1..f01d972f5d28 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -757,6 +757,12 @@ static void microvm_class_init(ObjectClass *oc, void *data)
"Set off to disable adding virtio-mmio devices to the kernel cmdline");

machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
+
+ /*
+ * pcie host bridge (gpex) on microvm has no io address window,
+ * so reserving io space is not going to work. Turn it off.
+ */
+ object_register_sugar_prop("pcie-root-port", "io-reserve", "0", true);
}

static const TypeInfo microvm_machine_info = {

So, I think we can drop patch #1. Want me respin the series, or can you
simply drop the patch on merge?

thanks,
Gerd


Ard Biesheuvel
 

On Fri, 3 Jun 2022 at 10:29, Gerd Hoffmann <kraxel@...> wrote:

Hi,

This seems to be related to the padding logic, i.e., we are trying to
preserve some extra I/O space for the root port in case we hotplug
something that might need it.
--- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
+++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
@@ -733,7 +733,7 @@ GetResourcePadding (
}
}

- if (DefaultIo) {
+ if (DefaultIo && FALSE) {
//
// Request defaults.
//
Oh, *there* it comes from. Given this is configurable already we can
fix that one in qemu with a microvm tweak:

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 4b3b1dd262f1..f01d972f5d28 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -757,6 +757,12 @@ static void microvm_class_init(ObjectClass *oc, void *data)
"Set off to disable adding virtio-mmio devices to the kernel cmdline");

machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
+
+ /*
+ * pcie host bridge (gpex) on microvm has no io address window,
+ * so reserving io space is not going to work. Turn it off.
+ */
+ object_register_sugar_prop("pcie-root-port", "io-reserve", "0", true);
}

static const TypeInfo microvm_machine_info = {

So, I think we can drop patch #1. Want me respin the series, or can you
simply drop the patch on merge?
I've already queued it up.