From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mx.groups.io with SMTP id smtpd.web12.817.1610628288917925756 for ; Thu, 14 Jan 2021 04:44:49 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.190, mailfrom: cenjiahui@huawei.com) Received: from DGGEMS409-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4DGkWf4L6mz15sgk; Thu, 14 Jan 2021 20:43:42 +0800 (CST) Received: from [10.174.184.155] (10.174.184.155) by DGGEMS409-HUB.china.huawei.com (10.3.19.209) with Microsoft SMTP Server id 14.3.498.0; Thu, 14 Jan 2021 20:44:35 +0800 Subject: Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges To: Laszlo Ersek , CC: Jordan Justen , Ard Biesheuvel , Rebecca Cran , Peter Grehan , Anthony Perard , "Julien Grall" , Leif Lindholm , Sami Mujawar , , , "Yubo Miao" References: <20210112094549.10238-1-cenjiahui@huawei.com> <20210112094549.10238-8-cenjiahui@huawei.com> <28c4d432-f281-91dc-1b93-faf67297f181@redhat.com> From: "Jiahui Cen" Message-ID: Date: Thu, 14 Jan 2021 20:44:34 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <28c4d432-f281-91dc-1b93-faf67297f181@redhat.com> X-Originating-IP: [10.174.184.155] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Signed-off-by: Jiahui Cen >> Signed-off-by: Yubo Miao >> --- >> 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 >> >> +#include >> +#include >> + >> #include >> #include >> >> @@ -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 >> #include >> -#include >> #include >> #include >> #include >> @@ -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 > > . >