[edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
Jiahui Cen via groups.io
cenjiahui=huawei.com at groups.io
Thu Jan 14 12:44:34 UTC 2021
Hi Laszlo,
On 2021/1/14 18:46, Laszlo Ersek wrote:
> On 01/12/21 10:45, Jiahui Cen via groups.io wrote:
>> Extend parameter list of PciHostBridgeUtilityGetRootBridges() with
>> DmaAbove4G, NoExtendedConfigSpace, BusMin and BusMax to support for
>> ArmVirtPkg.
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059
>>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Signed-off-by: Jiahui Cen <cenjiahui at huawei.com>
>> Signed-off-by: Yubo Miao <miaoyubo at huawei.com>
>> ---
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 3 --
>> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 30 +++++++++----
>> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 7 +++
>> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 47 ++++++++++++--------
>> 4 files changed, 57 insertions(+), 30 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> index 4cdf367ffee2..83a734c1725e 100644
>> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
>> @@ -41,6 +41,3 @@ [LibraryClasses]
>> MemoryAllocationLib
>> PciLib
>> QemuFwCfgLib
>> -
>> -[Pcd]
>> - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> index 29ea19f2d7e5..ed2b386001e1 100644
>> --- a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
>> @@ -100,23 +100,31 @@ PciHostBridgeUtilityUninitRootBridge (
>> /**
>> Utility function to return all the root bridge instances in an array.
>>
>> - @param[out] Count The number of root bridge instances.
>> + @param[out] Count The number of root bridge instances.
>>
>> - @param[in] Attributes Initial attributes.
>> + @param[in] Attributes Initial attributes.
>>
>> - @param[in] AllocAttributes Allocation attributes.
>> + @param[in] AllocAttributes Allocation attributes.
>>
>> - @param[in] Io IO aperture.
>> + @param[in] DmaAbove4G DMA above 4GB memory.
>>
>> - @param[in] Mem MMIO aperture.
>> + @param[in] NoExtendedConfigSpace No Extended Config Space.
>>
>> - @param[in] MemAbove4G MMIO aperture above 4G.
>> + @param[in] BusMin Minimum Bus number
>>
>> - @param[in] PMem Prefetchable MMIO aperture.
>> + @param[in] BusMax Maximum Bus number
>>
>> - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
>> + @param[in] Io IO aperture.
>>
>> - @return All the root bridge instances in an array.
>> + @param[in] Mem MMIO aperture.
>> +
>> + @param[in] MemAbove4G MMIO aperture above 4G.
>> +
>> + @param[in] PMem Prefetchable MMIO aperture.
>> +
>> + @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
>> +
>> + @return All the root bridge instances in an array.
>> **/
>> PCI_ROOT_BRIDGE *
>> EFIAPI
>> @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges (
>> UINTN *Count,
>> UINT64 Attributes,
>> UINT64 AllocationAttributes,
>> + BOOLEAN DmaAbove4G,
>> + BOOLEAN NoExtendedConfigSpace,
>> + UINT32 BusMin,
>> + UINT32 BusMax,
>> PCI_ROOT_BRIDGE_APERTURE *Io,
>> PCI_ROOT_BRIDGE_APERTURE *Mem,
>> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
>
> (1) You forgot to annotate the new params with IN. (Also update the C
> file please, in sync.)
>
Will add these annotation and also for other patches.
> (2) The BusMin / BusMax addition must absolutely be a separate patch, so
> that we can discuss and review it separately. It's not a simple data
> propagation change -- it generalizes the function internally.
>
> (3) BusMax should be documented as an inclusive maximum (but see more on
> this below).
>
Right, will fix.
> (4) I don't understand where the UINT32 type for BusMin / BusMax comes
> from. PciHostBridgeUtilityInitRootBridge() takes UINT8 bus numbers
> (which makes sense). And the scanning uses UINTN values, see e.g.
> RootBridgeNumber, which also makes sense (for convenience). UINT32
> matches neither. It's not necessarily wrong, but confusing.
>
> ... if you chose UINT32 because ProcessPciHost()
> [ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c] outputs
> BusMin and BusMax as UINT32, then please use UINTN instead.
>
I chose UINT32 because ProcessPciHost() outputs as UINT32 and
PciHostBridgeGetRootBridges() also accepts as UINT32 (in
ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c),
and also UINT32 covers UINT8 which comes from OvmfPkg.
Using UINT8 does not fit value passed from ArmVirtPkg, so
as you suggest, UINTN seems a better choice.
>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> index 0a1d94a29bf3..28ad32752cab 100644
>> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>> @@ -9,6 +9,9 @@
>> **/
>> #include <PiDxe.h>
>>
>> +#include <IndustryStandard/Pci.h>
>> +#include <IndustryStandard/Q35MchIch9.h>
>> +
>> #include <Protocol/PciHostBridgeResourceAllocation.h>
>> #include <Protocol/PciRootBridgeIo.h>
>>
>> @@ -81,6 +84,10 @@ PciHostBridgeGetRootBridges (
>> Count,
>> Attributes,
>> AllocationAttributes,
>> + FALSE,
>> + PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
>> + 0,
>> + PCI_MAX_BUS,
>> &Io,
>> &Mem,
>> &MemAbove4G,
>> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> index 629ebcd7a5be..fd2f54a139e2 100644
>> --- a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
>> @@ -12,7 +12,6 @@
>>
>> #include <IndustryStandard/Acpi10.h>
>> #include <IndustryStandard/Pci.h>
>> -#include <IndustryStandard/Q35MchIch9.h>
>> #include <Library/BaseMemoryLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/DevicePathLib.h>
>> @@ -189,23 +188,31 @@ PciHostBridgeUtilityUninitRootBridge (
>> /**
>> Utility function to return all the root bridge instances in an array.
>>
>> - @param[out] Count The number of root bridge instances.
>> + @param[out] Count The number of root bridge instances.
>>
>> - @param[in] Attributes Initial attributes.
>> + @param[in] Attributes Initial attributes.
>>
>> - @param[in] AllocAttributes Allocation attributes.
>> + @param[in] AllocAttributes Allocation attributes.
>>
>> - @param[in] Io IO aperture.
>> + @param[in] DmaAbove4G DMA above 4GB memory.
>>
>> - @param[in] Mem MMIO aperture.
>> + @param[in] NoExtendedConfigSpace No Extended Config Space.
>>
>> - @param[in] MemAbove4G MMIO aperture above 4G.
>> + @param[in] BusMin Minimum Bus number
>>
>> - @param[in] PMem Prefetchable MMIO aperture.
>> + @param[in] BusMax Maximum Bus number
>>
>> - @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
>> + @param[in] Io IO aperture.
>>
>> - @return All the root bridge instances in an array.
>> + @param[in] Mem MMIO aperture.
>> +
>> + @param[in] MemAbove4G MMIO aperture above 4G.
>> +
>> + @param[in] PMem Prefetchable MMIO aperture.
>> +
>> + @param[in] PMemAbove4G Prefetchable MMIO aperture above 4G.
>> +
>> + @return All the root bridge instances in an array.
>> **/
>> PCI_ROOT_BRIDGE *
>> EFIAPI
>> @@ -213,6 +220,10 @@ PciHostBridgeUtilityGetRootBridges (
>> UINTN *Count,
>> UINT64 Attributes,
>> UINT64 AllocationAttributes,
>> + BOOLEAN DmaAbove4G,
>> + BOOLEAN NoExtendedConfigSpace,
>> + UINT32 BusMin,
>> + UINT32 BusMax,
>> PCI_ROOT_BRIDGE_APERTURE *Io,
>> PCI_ROOT_BRIDGE_APERTURE *Mem,
>> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G,
>> @@ -240,7 +251,7 @@ PciHostBridgeUtilityGetRootBridges (
>> QemuFwCfgSelectItem (FwCfgItem);
>> QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges);
>>
>> - if (ExtraRootBridges > PCI_MAX_BUS) {
>> + if (ExtraRootBridges > BusMax - BusMin) {
>> DEBUG ((DEBUG_ERROR, "%a: invalid count of extra root buses (%Lu) "
>> "reported by QEMU\n", __FUNCTION__, ExtraRootBridges));
>> return NULL;
>
> (5) The code change looks valid, but please add a comment here (in the
> patch dedicated to the bus numbers, that is). Because BusMax is
> inclusive, the max bus count is (BusMax - BusMin + 1). From that, the
> "main" root bus is is a given, so the max count for the "extra" root
> bridges is one less, i.e. (BusMax - BusMin). If the QEMU hint exceeds
> that, we have invalid behavior.
>
> (6) In the patch that will deal with the bus numbers exlusively, please
> add a sanity check near the top of the function:
>
> BusMin > BusMax || BusMax > PCI_MAX_BUS
>
> If this condition evaluates to TRUE, the function should set (*Count) to
> 0, and return NULL, at once.
>
Will add them.
>
>> @@ -262,15 +273,15 @@ PciHostBridgeUtilityGetRootBridges (
>> //
>> // The "main" root bus is always there.
>> //
>> - LastRootBridgeNumber = 0;
>> + LastRootBridgeNumber = BusMin;
>>
>> //
>> // Scan all other root buses. If function 0 of any device on a bus returns a
>> // VendorId register value different from all-bits-one, then that bus is
>> // alive.
>> //
>> - for (RootBridgeNumber = 1;
>> - RootBridgeNumber <= PCI_MAX_BUS && Initialized < ExtraRootBridges;
>> + for (RootBridgeNumber = BusMin + 1;
>> + RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges;
>> ++RootBridgeNumber) {
>> UINTN Device;
>>
>> @@ -290,8 +301,8 @@ PciHostBridgeUtilityGetRootBridges (
>> Attributes,
>> Attributes,
>> AllocationAttributes,
>> - FALSE,
>> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
>> + DmaAbove4G,
>> + NoExtendedConfigSpace,
>> (UINT8) LastRootBridgeNumber,
>> (UINT8) (RootBridgeNumber - 1),
>> Io,
>> @@ -317,8 +328,8 @@ PciHostBridgeUtilityGetRootBridges (
>> Attributes,
>> Attributes,
>> AllocationAttributes,
>> - FALSE,
>> - PcdGet16 (PcdOvmfHostBridgePciDevId) != INTEL_Q35_MCH_DEVICE_ID,
>> + DmaAbove4G,
>> + NoExtendedConfigSpace,
>> (UINT8) LastRootBridgeNumber,
>> PCI_MAX_BUS,
>> Io,
>>
>
> (7) You missed replacing PCI_MAX_BUS with BusMax here. (But it belongs
> in the separate patch that will deal with the bus numbers, and only with
> the bus numbers.)
>
> ... Which in turn makes me ask you to please test your changes more
> carefully. I believe this bug here is actually shown in the firmware
> debug log. Namely, the "virt" machine type only supports buses 0x0..0xf,
> inclusive (if I remember correctly), because its MMCONFIG space is quite
> limited.
>
I tested on QEMU for x86_64 and aarch64, and it actually worked well so that
I missed this issue. IIUC, for aarch64, the virt machine supports bus range
[0x0, 0xff] by default, and the MMCONFIG space size is 256MB.
> Now, assume the common case, with the "virt" machine type, where you
> don't add any pxb devices to the QEMU cmdline -- just go with the main
> bus (bus#0). With this bug, the "max sub bus number" on bus#0 goes from
> 0xf to 0xff. I'm fairly sure that change is visible in the messages that
> are logged by PciHostBridgeDxe.
>
> You're supposed to regression test the previous use case with the
> patched code -- there should be no change in behavior. Comparing the
> before-after logs is one of the checks someone should do for this.
>
>
> ... I've found the bus number stuff in this patch so distracting that
> I'm actually not capable of reviewing the patch wrt. its "original"
> purpose, namely the exposure of the DmaAbove4G and NoExtendedConfigSpace
> parameters. I'll do that in v6, once you have split this patch in two.
>
Thanks for the review. Will split it.
Thanks,
Jiahui
> Laszlo
>
> .
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70283): https://edk2.groups.io/g/devel/message/70283
Mute This Topic: https://groups.io/mt/79619797/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list