From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mx.groups.io with SMTP id smtpd.web09.4701.1609998268958258492 for ; Wed, 06 Jan 2021 21:44:29 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.191, mailfrom: cenjiahui@huawei.com) Received: from DGGEMS410-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4DBFWj3wV1zM0jm; Thu, 7 Jan 2021 13:43:13 +0800 (CST) Received: from [10.174.184.155] (10.174.184.155) by DGGEMS410-HUB.china.huawei.com (10.3.19.210) with Microsoft SMTP Server id 14.3.498.0; Thu, 7 Jan 2021 13:44:18 +0800 Subject: Re: [edk2-devel] [PATCH v3 1/5] OvmfPkg: Introduce PciHostBridgeUtilityLib class To: Laszlo Ersek , CC: Jordan Justen , Ard Biesheuvel , Rebecca Cran , Peter Grehan , Anthony Perard , "Julien Grall" , Leif Lindholm , Sami Mujawar , , , "Yubo Miao" References: <20201222095944.8686-1-cenjiahui@huawei.com> <20201222095944.8686-2-cenjiahui@huawei.com> <2c6617a0-db0b-3945-fd54-27d3467ef5b0@redhat.com> From: "Jiahui Cen" Message-ID: Date: Thu, 7 Jan 2021 13:44:16 +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: <2c6617a0-db0b-3945-fd54-27d3467ef5b0@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/6 16:51, Laszlo Ersek wrote: > On 12/22/20 10:59, Jiahui Cen via groups.io wrote: >> Introduce a new PciHostBridgeUtilityLib class to share duplicate code >> between OvmfPkg and ArmVirtPkg. >> >> Extract function PciHostBridgeUtilityResourceConflict from >> PciHostBridgeResourceConflict in OvmfPkg/PciHostBridgeLib. >> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059 >> >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Rebecca Cran >> Cc: Peter Grehan >> Cc: Anthony Perard >> Cc: Julien Grall >> Signed-off-by: Jiahui Cen >> Signed-off-by: Yubo Miao >> --- >> OvmfPkg/OvmfPkg.dec | 4 + >> OvmfPkg/Bhyve/BhyveX64.dsc | 1 + >> OvmfPkg/OvmfPkgIa32.dsc | 1 + >> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >> OvmfPkg/OvmfPkgX64.dsc | 1 + >> OvmfPkg/OvmfXen.dsc | 1 + >> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 + >> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 37 ++++++++++ >> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 37 ++++++++++ >> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 41 +---------- >> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 77 ++++++++++++++++++++ >> 11 files changed, 163 insertions(+), 39 deletions(-) > > (1) Since you posted v2, OvmfPkg gained a new platform DSC file. The > full set of DSC files in OvmfPkg is, at this point: > > OvmfPkg/AmdSev/AmdSevX64.dsc > OvmfPkg/Bhyve/BhyveX64.dsc > OvmfPkg/OvmfPkgIa32.dsc > OvmfPkg/OvmfPkgIa32X64.dsc > OvmfPkg/OvmfPkgX64.dsc > OvmfPkg/OvmfXen.dsc > > "OvmfPkg/AmdSev/AmdSevX64.dsc" consumes > "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf" as well; please > add the new PciHostBridgeUtilityLib resolution to that DSC file too, in v4. > > (You should check for any and all DSC files with such a dependency, > every time you rebase.) Thanks for your review. I'll fix it in v4, and will be careful about the dependencies in the future. Thanks, Jiahui > > With that update, I'm ready to grant an R-b for this patch. > > Thanks! > Laszlo > >> >> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec >> index 50d7b27d941c..e39097a253a1 100644 >> --- a/OvmfPkg/OvmfPkg.dec >> +++ b/OvmfPkg/OvmfPkg.dec >> @@ -49,6 +49,10 @@ [LibraryClasses] >> # access. >> PciCapPciSegmentLib|Include/Library/PciCapPciSegmentLib.h >> >> + ## @libraryclass Provide common utility functions to PciHostBridgeLib >> + # instances in ArmVirtPkg and OvmfPkg. >> + PciHostBridgeUtilityLib|Include/Library/PciHostBridgeUtilityLib.h >> + >> ## @libraryclass Register a status code handler for printing the Boot >> # Manager's LoadImage() and StartImage() preparations, and >> # return codes, to the UEFI console. >> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc >> index b93fe30ae4e0..f1fdd85d1911 100644 >> --- a/OvmfPkg/Bhyve/BhyveX64.dsc >> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc >> @@ -660,6 +660,7 @@ [Components] >> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf { >> >> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf >> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf >> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf >> } >> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf { >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >> index 26a013ec353e..6eef5e0cfa9c 100644 >> --- a/OvmfPkg/OvmfPkgIa32.dsc >> +++ b/OvmfPkg/OvmfPkgIa32.dsc >> @@ -745,6 +745,7 @@ [Components] >> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf { >> >> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf >> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf >> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf >> } >> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf { >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >> index 10579fe46c5b..4b2f48406543 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >> @@ -759,6 +759,7 @@ [Components.X64] >> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf { >> >> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf >> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf >> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf >> } >> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf { >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index c9235e48ad62..8577ccaa35af 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -755,6 +755,7 @@ [Components] >> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf { >> >> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf >> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf >> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf >> } >> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf { >> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc >> index 12b7a87ee877..fa35d122cf3e 100644 >> --- a/OvmfPkg/OvmfXen.dsc >> +++ b/OvmfPkg/OvmfXen.dsc >> @@ -550,6 +550,7 @@ [Components] >> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf { >> >> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf >> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf >> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf >> } >> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf { >> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf >> index 6ec9ec751478..4c56f3c90b3b 100644 >> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf >> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf >> @@ -39,6 +39,7 @@ [LibraryClasses] >> DebugLib >> DevicePathLib >> MemoryAllocationLib >> + PciHostBridgeUtilityLib >> PciLib >> QemuFwCfgLib >> >> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf >> new file mode 100644 >> index 000000000000..1ba8ec3e03c7 >> --- /dev/null >> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf >> @@ -0,0 +1,37 @@ >> +## @file >> +# Provide common utility functions to PciHostBridgeLib instances in >> +# ArmVirtPkg and OvmfPkg. >> +# >> +# Copyright (C) 2016, Red Hat, Inc. >> +# Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
>> +# Copyright (c) 2020, Huawei Corporation. All rights reserved.
>> +# >> +# SPDX-License-Identifier: BSD-2-Clause-Patent >> +# >> +# >> +## >> + >> +[Defines] >> + INF_VERSION = 1.29 >> + BASE_NAME = PciHostBridgeUtilityLib >> + FILE_GUID = e3aa5932-527a-42e7-86f5-81b144c7e5f1 >> + MODULE_TYPE = DXE_DRIVER >> + VERSION_STRING = 1.0 >> + LIBRARY_CLASS = PciHostBridgeUtilityLib >> + >> +# >> +# The following information is for reference only and not required by the build >> +# tools. >> +# >> +# VALID_ARCHITECTURES = IA32 X64 AARCH64 ARM >> +# >> + >> +[Sources] >> + PciHostBridgeUtilityLib.c >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + OvmfPkg/OvmfPkg.dec >> + >> +[LibraryClasses] >> + DebugLib >> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h >> new file mode 100644 >> index 000000000000..f932d412aa10 >> --- /dev/null >> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h >> @@ -0,0 +1,37 @@ >> +/** @file >> + Provide common utility functions to PciHostBridgeLib instances in >> + ArmVirtPkg and OvmfPkg. >> + >> + Copyright (C) 2016, Red Hat, Inc. >> + Copyright (c) 2016, Intel Corporation. All rights reserved.
>> + Copyright (c) 2020, Huawei Corporation. All rights reserved.
>> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> + >> +**/ >> + >> +#ifndef __PCI_HOST_BRIDGE_UTILITY_LIB_H__ >> +#define __PCI_HOST_BRIDGE_UTILITY_LIB_H__ >> + >> + >> +/** >> + Utility function to inform the platform that the resource conflict happens. >> + >> + @param Configuration Pointer to PCI I/O and PCI memory resource >> + descriptors. The Configuration contains the resources >> + for all the root bridges. The resource for each root >> + bridge is terminated with END descriptor and an >> + additional END is appended indicating the end of the >> + entire resources. The resource descriptor field >> + values follow the description in >> + EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL >> + .SubmitResources(). >> +**/ >> +VOID >> +EFIAPI >> +PciHostBridgeUtilityResourceConflict ( >> + VOID *Configuration >> + ); >> + >> + >> +#endif // __PCI_HOST_BRIDGE_UTILITY_LIB_H__ >> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c >> index e850f7d183ee..4a176347fd49 100644 >> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c >> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c >> @@ -20,6 +20,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include "PciHostBridge.h" >> @@ -33,12 +34,6 @@ typedef struct { >> #pragma pack () >> >> >> -GLOBAL_REMOVE_IF_UNREFERENCED >> -CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = { >> - L"Mem", L"I/O", L"Bus" >> -}; >> - >> - >> STATIC >> CONST >> OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH mRootBridgeDevicePathTemplate = { >> @@ -407,37 +402,5 @@ PciHostBridgeResourceConflict ( >> VOID *Configuration >> ) >> { >> - EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; >> - UINTN RootBridgeIndex; >> - DEBUG ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n")); >> - >> - RootBridgeIndex = 0; >> - Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration; >> - while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) { >> - DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++)); >> - for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) { >> - ASSERT (Descriptor->ResType < >> - ARRAY_SIZE (mPciHostBridgeLibAcpiAddressSpaceTypeStr) >> - ); >> - DEBUG ((DEBUG_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n", >> - mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType], >> - Descriptor->AddrLen, Descriptor->AddrRangeMax >> - )); >> - if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) { >> - DEBUG ((DEBUG_ERROR, " Granularity/SpecificFlag = %ld / %02x%s\n", >> - Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag, >> - ((Descriptor->SpecificFlag & >> - EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE >> - ) != 0) ? L" (Prefetchable)" : L"" >> - )); >> - } >> - } >> - // >> - // Skip the END descriptor for root bridge >> - // >> - ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR); >> - Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)( >> - (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1 >> - ); >> - } >> + PciHostBridgeUtilityResourceConflict (Configuration); >> } >> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c >> new file mode 100644 >> index 000000000000..bbaa5f830c98 >> --- /dev/null >> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c >> @@ -0,0 +1,77 @@ >> +/** @file >> + Provide common utility functions to PciHostBridgeLib instances in >> + ArmVirtPkg and OvmfPkg. >> + >> + Copyright (C) 2016, Red Hat, Inc. >> + Copyright (c) 2016, Intel Corporation. All rights reserved.
>> + Copyright (c) 2020, Huawei Corporation. All rights reserved.
>> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> + >> +**/ >> + >> +#include >> +#include >> +#include >> + >> + >> +GLOBAL_REMOVE_IF_UNREFERENCED >> +CHAR16 *mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[] = { >> + L"Mem", L"I/O", L"Bus" >> +}; >> + >> + >> +/** >> + Utility function to inform the platform that the resource conflict happens. >> + >> + @param Configuration Pointer to PCI I/O and PCI memory resource >> + descriptors. The Configuration contains the resources >> + for all the root bridges. The resource for each root >> + bridge is terminated with END descriptor and an >> + additional END is appended indicating the end of the >> + entire resources. The resource descriptor field >> + values follow the description in >> + EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL >> + .SubmitResources(). >> +**/ >> +VOID >> +EFIAPI >> +PciHostBridgeUtilityResourceConflict ( >> + VOID *Configuration >> + ) >> +{ >> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; >> + UINTN RootBridgeIndex; >> + DEBUG ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n")); >> + >> + RootBridgeIndex = 0; >> + Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration; >> + while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) { >> + DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++)); >> + for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) { >> + ASSERT (Descriptor->ResType < >> + ARRAY_SIZE (mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr) >> + ); >> + DEBUG ((DEBUG_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n", >> + mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr[Descriptor->ResType], >> + Descriptor->AddrLen, Descriptor->AddrRangeMax >> + )); >> + if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) { >> + DEBUG ((DEBUG_ERROR, " Granularity/SpecificFlag = %ld / %02x%s\n", >> + Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag, >> + ((Descriptor->SpecificFlag & >> + EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE >> + ) != 0) ? L" (Prefetchable)" : L"" >> + )); >> + } >> + } >> + // >> + // Skip the END descriptor for root bridge >> + // >> + ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR); >> + Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)( >> + (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1 >> + ); >> + } >> +} >> + >> > > . >