MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
PciScanBus() assumes the GetResourcePadding() puts BUS descriptor
in the very beginning, if it's not, the Descriptors will be updated
to point to middle of the pool buffer, which can cause
FreePool(Descriptors) hang in DEBUG image.
No functionality impact to RELEASE image.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index e1d62e8c21..8b076e8791 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -1,7 +1,7 @@
/** @file
Internal library implementation for PCI Bus module.
-Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
@@ -986,6 +986,7 @@ PciScanBus (
UINT64 PciAddress;
EFI_HPC_PADDING_ATTRIBUTES Attributes;
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptors;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *NextDescriptors;
UINT16 BusRange;
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo;
BOOLEAN BusPadding;
@@ -1143,8 +1144,9 @@ PciScanBus (
}
BusRange = 0;
+ NextDescriptors = Descriptors;
Status = PciGetBusRange (
- &Descriptors,
+ &NextDescriptors,
NULL,
NULL,
&BusRange
--
2.12.2.windows.2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Ni, Ruiyu
Sent: Thursday, July 27, 2017 11:06 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: [PATCH] MdeModulePkg/PciBus: Avoid hang when BUS pad resource is not in top
PciScanBus() assumes the GetResourcePadding() puts BUS descriptor in the very beginning, if it's not, the Descriptors will be updated to point to middle of the pool buffer, which can cause
FreePool(Descriptors) hang in DEBUG image.
No functionality impact to RELEASE image.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index e1d62e8c21..8b076e8791 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -1,7 +1,7 @@
/** @file
Internal library implementation for PCI Bus module.
-Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR> This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License @@ -986,6 +986,7 @@ PciScanBus (
UINT64 PciAddress;
EFI_HPC_PADDING_ATTRIBUTES Attributes;
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptors;
+ EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *NextDescriptors;
UINT16 BusRange;
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo;
BOOLEAN BusPadding;
@@ -1143,8 +1144,9 @@ PciScanBus (
}
BusRange = 0;
+ NextDescriptors = Descriptors;
Status = PciGetBusRange (
- &Descriptors,
+ &NextDescriptors,
NULL,
NULL,
&BusRange
--
2.12.2.windows.2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hello Ray,
please excuse me for hijacking this thread a little bit, but I'm very
happy that you happen to be looking at this exact part of the code,
which is also what I've been discussing with Marcel and Aleksandr (CC'd)
due to a different reason.
(The relevant sub-thread is at
Allow RedHat PCI bridges reserve more buses than necessary during init
http://mid.mail-archive.com/d0bd04f4-1ac4-0e3a-885f-2ceb6180f69a@redhat.com
but it's very long, so I'm not asking you to read that -- instead,
please let me ask you our question independently and in a self-contained
form.)
In "OvmfPkg/PciHotPlugInitDxe/", we plan to return some BUS resource
paddings, from the EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()
function.
QEMU has no "root" hotplug controllers, only "non-root" hotplug
controllers (using the terminology from "12.6 PCI Hot Plug PCI
Initialization Protocol" in the PI-1.6 spec). Therefore OVMF's
EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetRootHpcList() returns an empty array.
The BUS resource padding that we'd like to return from
GetResourcePadding() would be for "non-root" hotplug controllers.
According to the PI spec, this is a valid thing to do:
* From "12.5 Sample Implementation for a Platform Containing PCI
Hot Plug* Slots":
> [...] 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. [...]
* From "12.6 PCI Hot Plug PCI Initialization Protocol,
EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()":
> [...] This member function is called for all the root HPCs and nonroot
> HPCs that are detected by the PCI bus enumerator. [...] The PCI bus
> driver is responsible for adding this resource request to the resource
> requests by the physical PCI devices. [...]
I searched PciBusDxe for GetResourcePadding() call sites, and there are
two:
(1) In "MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c", function
GetResourcePaddingForHpb(). This call site seems to work as follows,
for our purposes:
GetResourcePaddingPpb() [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c]
GetResourcePaddingForHpb() [MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c]
gPciHotPlugInit->GetResourcePadding()
//
// and then save the resource descriptors, including the bus
// padding, in PciIoDevice->ResourcePaddingDescriptors
//
ResourcePaddingPolicy() [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c]
ApplyResourcePadding() [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c]
//
// translate IO and MMIO resources from
// PciDev->ResourcePaddingDescriptors to internal padding
// representation
//
This call site and processing seem to be active for "non-root"
hotplug controllers, but they do not consider bus numbers, only IO
and MMIO.
(2) In the PciScanBus() function, file
"MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c", we have another
GetResourcePadding() call -- your present patch is updating this
code part.
From the resource padding list returned by this GetResourcePadding()
invocation, PciBusDxe cares only about the bus numbers. I think
that's probably fine. However, in this location,
GetResourcePadding() is called *only* for "root" hotplug
controllers, of which QEMU has none.
The end result is that bus number paddings are not considered at all
for "non-root" hotplug controllers:
- site (1) is active for "non-root" HPCs, but it ignores bus number
paddings,
- site (2) processes bus number paddings, but it is inactive for
"non-root" HPCs.
This doesn't seem to match what the PI spec says. Is this behavior
intentional in PciBusDxe? If so, what is the reason for it?
Thank you!
Laszlo
On 07/27/17 05:05, Ruiyu Ni wrote:
> PciScanBus() assumes the GetResourcePadding() puts BUS descriptor
> in the very beginning, if it's not, the Descriptors will be updated
> to point to middle of the pool buffer, which can cause
> FreePool(Descriptors) hang in DEBUG image.
> No functionality impact to RELEASE image.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index e1d62e8c21..8b076e8791 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -1,7 +1,7 @@
> /** @file
> Internal library implementation for PCI Bus module.
>
> -Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD License
> @@ -986,6 +986,7 @@ PciScanBus (
> UINT64 PciAddress;
> EFI_HPC_PADDING_ATTRIBUTES Attributes;
> EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptors;
> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *NextDescriptors;
> UINT16 BusRange;
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo;
> BOOLEAN BusPadding;
> @@ -1143,8 +1144,9 @@ PciScanBus (
> }
>
> BusRange = 0;
> + NextDescriptors = Descriptors;
> Status = PciGetBusRange (
> - &Descriptors,
> + &NextDescriptors,
> NULL,
> NULL,
> &BusRange
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo, Thanks for raising this questions. Please see my embedded reply in below Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Thursday, July 27, 2017 7:23 PM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > Cc: Marcel Apfelbaum <marcel@redhat.com>; Aleksandr Bezzubikov > <zuban32s@gmail.com>; Zeng, Star <star.zeng@intel.com> > Subject: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid > hang when BUS pad resource is not in top] > > Hello Ray, > > please excuse me for hijacking this thread a little bit, but I'm very happy that > you happen to be looking at this exact part of the code, which is also what > I've been discussing with Marcel and Aleksandr (CC'd) due to a different > reason. > > (The relevant sub-thread is at > > Allow RedHat PCI bridges reserve more buses than necessary during init > > http://mid.mail-archive.com/d0bd04f4-1ac4-0e3a-885f- > 2ceb6180f69a@redhat.com > > but it's very long, so I'm not asking you to read that -- instead, please let me > ask you our question independently and in a self-contained > form.) > > In "OvmfPkg/PciHotPlugInitDxe/", we plan to return some BUS resource > paddings, from the > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() > function. > > QEMU has no "root" hotplug controllers, only "non-root" hotplug controllers > (using the terminology from "12.6 PCI Hot Plug PCI Initialization Protocol" in > the PI-1.6 spec). Therefore OVMF's > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetRootHpcList() returns an empty > array. > The BUS resource padding that we'd like to return from > GetResourcePadding() would be for "non-root" hotplug controllers. > > According to the PI spec, this is a valid thing to do: > > * From "12.5 Sample Implementation for a Platform Containing PCI > Hot Plug* Slots": > > > [...] 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. [...] > > * From "12.6 PCI Hot Plug PCI Initialization Protocol, > EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()": > > > [...] This member function is called for all the root HPCs and nonroot > > HPCs that are detected by the PCI bus enumerator. [...] The PCI bus > > driver is responsible for adding this resource request to the resource > > requests by the physical PCI devices. [...] > > I searched PciBusDxe for GetResourcePadding() call sites, and there are > two: > > (1) In "MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c", function > GetResourcePaddingForHpb(). This call site seems to work as follows, > for our purposes: > > GetResourcePaddingPpb() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > GetResourcePaddingForHpb() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c] > gPciHotPlugInit->GetResourcePadding() > // > // and then save the resource descriptors, including the bus > // padding, in PciIoDevice->ResourcePaddingDescriptors > // > > ResourcePaddingPolicy() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > ApplyResourcePadding() > [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] > // > // translate IO and MMIO resources from > // PciDev->ResourcePaddingDescriptors to internal padding > // representation > // > > This call site and processing seem to be active for "non-root" > hotplug controllers, but they do not consider bus numbers, only IO > and MMIO. [Ray] It's in bus allocation phase. So the PciBus driver only cares about the bus padding. PciBus driver firstly allocates the bus resource and programs the PCI-PCI bridges to set the bus numbers. After that, PciBus allocates the IO/MMIO resources and program the BARs in PCI devices & PCI-PCI bridges. So it's good here. > > (2) In the PciScanBus() function, file > "MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c", we have another > GetResourcePadding() call -- your present patch is updating this > code part. > > From the resource padding list returned by this GetResourcePadding() > invocation, PciBusDxe cares only about the bus numbers. I think > that's probably fine. However, in this location, > GetResourcePadding() is called *only* for "root" hotplug > controllers, of which QEMU has none. > > The end result is that bus number paddings are not considered at all for > "non-root" hotplug controllers: [Ray] Bus number padding should be considered for "non-root" HPCs. So I agree it's a bug/gap I need to fix. I noticed that already before your question. And I do have a plan to fix it though the priority is not very high. > - site (1) is active for "non-root" HPCs, but it ignores bus number > paddings, > - site (2) processes bus number paddings, but it is inactive for > "non-root" HPCs. > > This doesn't seem to match what the PI spec says. Is this behavior intentional > in PciBusDxe? If so, what is the reason for it? [Ray] In summary, site(1) is good. Site(2) contains a bug/gap. Would you please submit a Bugzilla to record this issue? But can you just return the non-root HPCs as root for a workaround? What prevent you from reporting the HPC as non-root? My opinion is the differences between non-root and root HPCs are: 1. Explicitly initialization is needed for root HPCs 2. Root HPCs may not be detected as hot-pluggable by PciBus using standard ways If your platform doesn't report any root HPCs, why not just report non-root as root ones? > > Thank you! > Laszlo > > On 07/27/17 05:05, Ruiyu Ni wrote: > > PciScanBus() assumes the GetResourcePadding() puts BUS descriptor in > > the very beginning, if it's not, the Descriptors will be updated to > > point to middle of the pool buffer, which can cause > > FreePool(Descriptors) hang in DEBUG image. > > No functionality impact to RELEASE image. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > index e1d62e8c21..8b076e8791 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > @@ -1,7 +1,7 @@ > > /** @file > > Internal library implementation for PCI Bus module. > > > > -Copyright (c) 2006 - 2016, Intel Corporation. All rights > > reserved.<BR> > > +Copyright (c) 2006 - 2017, Intel Corporation. All rights > > +reserved.<BR> > > (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR> > > This program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License @@ -986,6 > > +986,7 @@ PciScanBus ( > > UINT64 PciAddress; > > EFI_HPC_PADDING_ATTRIBUTES Attributes; > > EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptors; > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *NextDescriptors; > > UINT16 BusRange; > > EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo; > > BOOLEAN BusPadding; > > @@ -1143,8 +1144,9 @@ PciScanBus ( > > } > > > > BusRange = 0; > > + NextDescriptors = Descriptors; > > Status = PciGetBusRange ( > > - &Descriptors, > > + &NextDescriptors, > > NULL, > > NULL, > > &BusRange > > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 07/28/17 12:19, Ni, Ruiyu wrote: > Laszlo, > Thanks for raising this questions. > Please see my embedded reply in below > > Thanks/Ray > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Thursday, July 27, 2017 7:23 PM >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> Cc: Marcel Apfelbaum <marcel@redhat.com>; Aleksandr Bezzubikov >> <zuban32s@gmail.com>; Zeng, Star <star.zeng@intel.com> >> Subject: [edk2] bus number padding [was: MdeModulePkg/PciBus: Avoid >> hang when BUS pad resource is not in top] >> >> Hello Ray, >> >> please excuse me for hijacking this thread a little bit, but I'm very happy that >> you happen to be looking at this exact part of the code, which is also what >> I've been discussing with Marcel and Aleksandr (CC'd) due to a different >> reason. >> >> (The relevant sub-thread is at >> >> Allow RedHat PCI bridges reserve more buses than necessary during init >> >> http://mid.mail-archive.com/d0bd04f4-1ac4-0e3a-885f- >> 2ceb6180f69a@redhat.com >> >> but it's very long, so I'm not asking you to read that -- instead, please let me >> ask you our question independently and in a self-contained >> form.) >> >> In "OvmfPkg/PciHotPlugInitDxe/", we plan to return some BUS resource >> paddings, from the >> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding() >> function. >> >> QEMU has no "root" hotplug controllers, only "non-root" hotplug controllers >> (using the terminology from "12.6 PCI Hot Plug PCI Initialization Protocol" in >> the PI-1.6 spec). Therefore OVMF's >> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetRootHpcList() returns an empty >> array. >> The BUS resource padding that we'd like to return from >> GetResourcePadding() would be for "non-root" hotplug controllers. >> >> According to the PI spec, this is a valid thing to do: >> >> * From "12.5 Sample Implementation for a Platform Containing PCI >> Hot Plug* Slots": >> >>> [...] 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. [...] >> >> * From "12.6 PCI Hot Plug PCI Initialization Protocol, >> EFI_PCI_HOT_PLUG_INIT_PROTOCOL.GetResourcePadding()": >> >>> [...] This member function is called for all the root HPCs and nonroot >>> HPCs that are detected by the PCI bus enumerator. [...] The PCI bus >>> driver is responsible for adding this resource request to the resource >>> requests by the physical PCI devices. [...] >> >> I searched PciBusDxe for GetResourcePadding() call sites, and there are >> two: >> >> (1) In "MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c", function >> GetResourcePaddingForHpb(). This call site seems to work as follows, >> for our purposes: >> >> GetResourcePaddingPpb() >> [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] >> GetResourcePaddingForHpb() >> [MdeModulePkg/Bus/Pci/PciBusDxe/PciHotPlugSupport.c] >> gPciHotPlugInit->GetResourcePadding() >> // >> // and then save the resource descriptors, including the bus >> // padding, in PciIoDevice->ResourcePaddingDescriptors >> // >> >> ResourcePaddingPolicy() >> [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] >> ApplyResourcePadding() >> [MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c] >> // >> // translate IO and MMIO resources from >> // PciDev->ResourcePaddingDescriptors to internal padding >> // representation >> // >> >> This call site and processing seem to be active for "non-root" >> hotplug controllers, but they do not consider bus numbers, only IO >> and MMIO. > [Ray] It's in bus allocation phase. So the PciBus driver only cares about the > bus padding. > PciBus driver firstly allocates the bus resource and programs the PCI-PCI bridges > to set the bus numbers. > After that, PciBus allocates the IO/MMIO resources and program the BARs in > PCI devices & PCI-PCI bridges. > So it's good here. >> >> (2) In the PciScanBus() function, file >> "MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c", we have another >> GetResourcePadding() call -- your present patch is updating this >> code part. >> >> From the resource padding list returned by this GetResourcePadding() >> invocation, PciBusDxe cares only about the bus numbers. I think >> that's probably fine. However, in this location, >> GetResourcePadding() is called *only* for "root" hotplug >> controllers, of which QEMU has none. >> >> The end result is that bus number paddings are not considered at all for >> "non-root" hotplug controllers: > [Ray] Bus number padding should be considered for "non-root" HPCs. > So I agree it's a bug/gap I need to fix. I noticed that already before your question. > And I do have a plan to fix it though the priority is not very high. Awesome, thank you! It is not very urgent for us. Do you agree that I file a TianoCore BZ about this? > >> - site (1) is active for "non-root" HPCs, but it ignores bus number >> paddings, >> - site (2) processes bus number paddings, but it is inactive for >> "non-root" HPCs. >> >> This doesn't seem to match what the PI spec says. Is this behavior intentional >> in PciBusDxe? If so, what is the reason for it? > [Ray] In summary, site(1) is good. Site(2) contains a bug/gap. > Would you please submit a Bugzilla to record this issue? Thank you for answering my question :) I've now filed the following TianoCore BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=656 > But can you just return the non-root HPCs as root for a workaround? > What prevent you from reporting the HPC as non-root? Yes, I've thought of this as well. The problem is that the "a-priori" knowledge of the root HPCs, that the PI spec mentions for GetRootHpcList(), is actually *dynamic* knowledge in QEMU's case. In "OvmfPkg/PciHotPlugInitDxe", I could only collect the affected bridges by doing a full (recursive) PCI hierarchy enumeration, and look into the config space of each "candidate" controller. However, PciBusDxe already implements the full enumeration, so I don't think PciHotPlugInitDxe should it do internally. (In particular in a platform hook that could be called from a "sensitive" location in PciBusDxe.) > My opinion is the differences between non-root and root HPCs are: > 1. Explicitly initialization is needed for root HPCs I agree that this is one difference. Such initialization is not needed in QEMU's case however. > 2. Root HPCs may not be detected as hot-pluggable by PciBus using standard ways I agree that this is the other difference. All of QEMU's hotplug controllers can be enumerated in standard ways however. > If your platform doesn't report any root HPCs, why not just report non-root as root ones? (See above:) because the list of non-root HPCs is not known to OVMF in any static way, it is dynamic (boot time) information. And, QEMU does not expose this information to OVMF in any dedicated, easy-to-consume way. The only way to find out about the non-root HPCs is to actually traverse the PCI hierarchy. Given that PciBusDxe is in the middle of doing exactly that, I think that OvmfPkg/PciHotPlugInitDxe should not perform an "internal" enumeration in GetRootHpcList(). I don't think that OvmfPkg should have a private implementation of this PCI traversal, when the "main" traversal is already done in PciBusDxe. (And, likely much better than what I could write.) When you fix TianoCore BZ#656, that will be perfect for us. :) Thank you very much! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.