From: "Jiahui Cen" <cenjiahui@huawei.com>
To: Laszlo Ersek <lersek@redhat.com>, <devel@edk2.groups.io>
Cc: Jordan Justen <jordan.l.justen@intel.com>,
Ard Biesheuvel <ard.biesheuvel@arm.com>,
Rebecca Cran <rebecca@bsdio.com>,
Peter Grehan <grehan@freebsd.org>,
Anthony Perard <anthony.perard@citrix.com>,
"Julien Grall" <julien@xen.org>,
Leif Lindholm <leif@nuviainc.com>,
Sami Mujawar <sami.mujawar@arm.com>, <xieyingtai@huawei.com>,
<wu.wubin@huawei.com>, "Yubo Miao" <miaoyubo@huawei.com>
Subject: Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges
Date: Thu, 14 Jan 2021 20:44:34 +0800 [thread overview]
Message-ID: <b5a8c71e-da8b-4496-4cc1-f43fbd444951@huawei.com> (raw)
In-Reply-To: <28c4d432-f281-91dc-1b93-faf67297f181@redhat.com>
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@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> Signed-off-by: Yubo Miao <miaoyubo@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
>
> .
>
next prev parent reply other threads:[~2021-01-14 12:44 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-12 9:45 [PATCH v4 0/9] Add extra pci roots support for Arm Jiahui Cen
2021-01-12 9:45 ` [PATCH v4 1/9] OvmfPkg: Introduce PciHostBridgeUtilityLib class Jiahui Cen
2021-01-13 0:41 ` [edk2-devel] " Laszlo Ersek
2021-01-14 8:57 ` Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 2/9] ArmVirtPkg: Refactor with PciHostBridgeUtilityLib Jiahui Cen
2021-01-13 0:44 ` [edk2-devel] " Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge Jiahui Cen
2021-01-13 1:28 ` [edk2-devel] " Laszlo Ersek
2021-01-13 6:00 ` Jiahui Cen
2021-01-13 9:06 ` Laszlo Ersek
2021-01-14 8:51 ` Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 4/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of InitRootBridge Jiahui Cen
2021-01-13 1:51 ` [edk2-devel] " Laszlo Ersek
2021-01-13 6:01 ` Jiahui Cen
2021-01-12 9:45 ` [PATCH v4 5/9] ArmVirtPkg/FdtPciHostBridgeLib: Rebase to InitRootBridge() / UninitRootBridge() Jiahui Cen
2021-01-13 2:15 ` [edk2-devel] " Laszlo Ersek
2021-01-13 6:10 ` Jiahui Cen
2021-01-13 9:05 ` Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 6/9] OvmfPkg/PciHostBridgeLib: Extract GetRootBridges/FreeRootBridges Jiahui Cen
2021-01-14 9:12 ` [edk2-devel] " Laszlo Ersek
2021-01-12 9:45 ` [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges Jiahui Cen
2021-01-14 10:46 ` [edk2-devel] " Laszlo Ersek
2021-01-14 12:44 ` Jiahui Cen [this message]
2021-01-14 16:03 ` Laszlo Ersek
2021-01-15 7:25 ` Jiahui Cen
2021-01-15 7:59 ` Laszlo Ersek
2021-01-15 8:30 ` Jiahui Cen
2021-01-12 9:45 ` [PATCH v4 8/9] ArmVirtPkg/FdtPciHostBridgeLib: Refactor GetRootBridges() / FreeRootBridges() Jiahui Cen
2021-01-14 11:01 ` [edk2-devel] " Laszlo Ersek
2021-01-14 12:48 ` Jiahui Cen
2021-01-12 9:45 ` [PATCH v4 9/9] ArmVirtPkg/ArmVirtQemu: Add support for HotPlug Jiahui Cen
2021-01-14 11:04 ` [edk2-devel] " Laszlo Ersek
2021-01-14 11:53 ` [PATCH v4 0/9] Add extra pci roots support for Arm Laszlo Ersek
2021-01-14 12:51 ` Jiahui Cen
2021-01-18 17:26 ` [edk2-devel] " Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b5a8c71e-da8b-4496-4cc1-f43fbd444951@huawei.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox