1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v7 0/6] OvmfPkg/Microvm/pcie: add pcie support
  4. View patch

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

Gerd Hoffmann posted 6 patches 3 years, 8 months ago
[edk2-devel] [PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
Posted by Gerd Hoffmann 3 years, 8 months ago 
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@redhat.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
---
 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90138): https://edk2.groups.io/g/devel/message/90138
Mute This Topic: https://groups.io/mt/91495635/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
Posted by Ni, Ray 3 years, 8 months ago 
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@redhat.com>
> Sent: Thursday, June 2, 2022 4:42 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Pawel Polawski <ppolawsk@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Albecki, Mateusz <mateusz.albecki@intel.com>; Chang, Abner <abner.chang@hpe.com>; Ni, Ray <ray.ni@intel.com>; Leif
> Lindholm <quic_llindhol@quicinc.com>; Yao, Jiewen <jiewen.yao@intel.com>; Oliver Steffen <osteffen@redhat.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ardb@kernel.org>
> 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@redhat.com>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90156): https://edk2.groups.io/g/devel/message/90156
Mute This Topic: https://groups.io/mt/91495635/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
Posted by Ard Biesheuvel 3 years, 8 months ago 
On Thu, 2 Jun 2022 at 12:14, Ni, Ray <ray.ni@intel.com> 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?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90158): https://edk2.groups.io/g/devel/message/90158
Mute This Topic: https://groups.io/mt/91495635/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
Posted by Gerd Hoffmann 3 years, 8 months ago 
  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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90159): https://edk2.groups.io/g/devel/message/90159
Mute This Topic: https://groups.io/mt/91495635/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
Posted by Ard Biesheuvel 3 years, 8 months ago 
On Thu, 2 Jun 2022 at 15:15, Gerd Hoffmann <kraxel@redhat.com> 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.
     //


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90161): https://edk2.groups.io/g/devel/message/90161
Mute This Topic: https://groups.io/mt/91495635/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
Posted by Gerd Hoffmann 3 years, 8 months ago 
  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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90173): https://edk2.groups.io/g/devel/message/90173
Mute This Topic: https://groups.io/mt/91495635/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
Posted by Ard Biesheuvel 3 years, 8 months ago 
On Fri, 3 Jun 2022 at 10:29, Gerd Hoffmann <kraxel@redhat.com> 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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90174): https://edk2.groups.io/g/devel/message/90174
Mute This Topic: https://groups.io/mt/91495635/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.