[edk2-devel] [PATCH v2 1/4] OvmfPkg: Extract functions form PciHostBridgeLib
Laszlo Ersek
lersek at redhat.com
Wed Nov 11 16:45:13 UTC 2020
On 11/09/20 14:05, Jiahui Cen wrote:
> From: Yubo Miao <miaoyubo at huawei.com>
>
> Introduce a new PciHostBridgeUtilityLib class to share duplicate code
> between OvmfPkg and ArmVirtPkg.
>
> Extract function PciHostBridgeResourceConflict from
> OvmfPkg/PciHostBridgeLib.
>
> 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: Yubo Miao <miaoyubo at huawei.com>
> Signed-off-by: Jiahui Cen <cenjiahui at huawei.com>
> ---
> 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 | 49 +++++++++++++
> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 33 +++++++++
> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 64 +----------------
> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 74 ++++++++++++++++++++
> 9 files changed, 162 insertions(+), 63 deletions(-)
(1) There is a typo in the subject: s/form/from/
(2) "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf" is consumed
in the following platform DSC file as well -- please update it too:
OvmfPkg/Bhyve/BhyveX64.dsc
(This will affect the list of necessary CC's on this patch.)
(3) In my v1 review at <https://edk2.groups.io/g/devel/message/57062>
(msgid <57fd0043-d63a-55b0-9c55-4ca079331885 at redhat.com>), I mentioned
"OvmfPkg/OvmfPkg.dec".
You forgot to extend that file, with the new library class. Please add
the following hunk to this patch:
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 118c968fda4e..b89252eeab2a 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.
(4) Issue (3) above would have been caught by CI, namely by the
"LibraryClassCheck" plugin:
https://github.com/tianocore/edk2/blob/master/.pytool/Readme.md#library-declaration-test---libraryclasscheck
In order to put your patches through CI before posting, I suggest
pushing them to a branch in your edk2 clone on github.com, and
submitting a pull request against the central repository. The
pull request will be auto-closed in the end, unconditionally, but it
will give you CI results.
(You can also run CI locally, but setting that up is laborious.)
Back to your patch:
On 11/09/20 14:05, Jiahui Cen wrote:
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 58d9f292f9ac..0c2bf0b13c34 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -738,6 +738,7 @@ [Components]
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 3551f9710a6c..baf36a4f7a54 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -752,6 +752,7 @@ [Components.X64]
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 7a8bdb8a8697..219b5f559b53 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -748,6 +748,7 @@ [Components]
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> <LibraryClasses>
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 34c9de19dfba..442c0730ef32 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -544,6 +544,7 @@ [Components]
> <LibraryClasses>
> PciHostBridgeLib|OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> NULL|OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf
> + PciHostBridgeUtilityLib|OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> }
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
> <LibraryClasses>
(5) Please keep the PciHostBridgeUtilityLib class resolution right next
to the PciHostBridgeLib one.
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> index 6ec9ec751478..7d01528c94f1 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -41,6 +41,7 @@ [LibraryClasses]
> MemoryAllocationLib
> PciLib
> QemuFwCfgLib
> + PciHostBridgeUtilityLib
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
(6) Please keep the entries under [LibraryClasses] alphabetically
sorted.
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> new file mode 100644
> index 000000000000..c88ab8e4155d
> --- /dev/null
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf
> @@ -0,0 +1,49 @@
> +## @file
> +# OVMF and Arm's instance of the PCI Host Bridge Utility Library.
(7) I suggest the following description:
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.<BR>
(8) It makes sense to duplicate the (C) notices from the original file
(namely "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf"), but
this is still a new file due to your contribution.
Therefore, please add a proper Huawei copyright notice.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
(9) The latest specified INF version is 1.29, according to
<https://edk2-docs.gitbook.io/edk-ii-inf-specification/#edk-ii-module-information-inf-file-specification>;
please write
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 EBC
> +#
> +
(10) EBC is gone, plus we'll want to plug this lib into
"ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf", which
itself names AARCH64 and ARM for VALID_ARCHITECTURES.
Therefore, please state "IA32 X64 AARCH64 ARM" here.
> +[Sources]
> + PciHostBridgeUtilityLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> + DebugLib
> + DevicePathLib
> + MemoryAllocationLib
> + PciLib
> + QemuFwCfgLib
(11) The list of required (consumed) library classes, along with the
library class header files included in "PciHostBridgeUtilityLib.c",
should be as minimal as possible (and alphabetically sorted).
Specifically, you only need to keep "DebugLib"; everything else should
be removed, as far as I can tell.
(We can extend the [LibraryClasses] section in later patches, if
necessary.)
> +
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
> + gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
(12) The [Pcd] section should be dropped altogether.
> diff --git a/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> new file mode 100644
> index 000000000000..d2622fd907e6
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h
> @@ -0,0 +1,33 @@
> +/** @file
> + PCI Host Bridge Library consumed by PciHostBridgeDxe driver returning
> + the platform specific information about the PCI Host Bridge.
(13) This file-top comment should say:
Provide common utility functions to PciHostBridgeLib instances in
ArmVirtPkg and OvmfPkg.
> +
> + Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
(14) It's fine to keep the (C) notices from the file that you are using
as template, but you should please add a Huawei (C) notice as well.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef __PCI_HOST_BRIDGE_UTILITY_LIB_H__
> +#define __PCI_HOST_BRIDGE_UTILITY_LIB_H__
> +
> +/**
> + Inform the platform that the resource conflict happens.
> +
> + @param HostBridgeHandle Handle of the Host Bridge.
> + @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
> +PciHostBridgeResourceConflict (
> + EFI_HANDLE HostBridgeHandle,
> + VOID *Configuration
> + );
> +
(15) Please rename this function to
"PciHostBridgeUtilityResourceConflict".
The idea is that no PciHostBridgeUtilityLib instance should directly
implement a PciHostBridgeLib API. PciHostBridgeUtilityLib can offer APIs
that are as "fat" as necessary, taking over as much work as possible,
but we should keep the namespaces isolated.
(16) In our helper library, the "HostBridgeHandle" parameter is not
used; please remove it from both the leading comment on the function,
and also from the function declaration.
> +#endif
(17) Please append a comment to this line:
// __PCI_HOST_BRIDGE_UTILITY_LIB_H__
> diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> index e850f7d183ee..1c8f465834f3 100644
> --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -22,6 +22,7 @@
> #include <Library/PciHostBridgeLib.h>
> #include <Library/PciLib.h>
> #include <Library/QemuFwCfgLib.h>
> +#include <Library/PciHostBridgeUtilityLib.h>
> #include "PciHostBridge.h"
>
>
(18) Please keep the #include list of the Library class headers
alphabetically sorted.
> @@ -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 = {
> @@ -384,60 +379,3 @@ PciHostBridgeFreeRootBridges (
>
> FreePool (Bridges);
> }
> -
> -
> -/**
> - Inform the platform that the resource conflict happens.
> -
> - @param HostBridgeHandle Handle of the Host Bridge.
> - @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
> -PciHostBridgeResourceConflict (
> - EFI_HANDLE HostBridgeHandle,
> - 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
> - );
> - }
> -}
(19) Please do not remove the entire definition of this function.
According to my point (15), only replace the body of this function with
the following function call:
PciHostBridgeUtilityResourceConflict (Configuration);
> diff --git a/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> new file mode 100644
> index 000000000000..7e9512dc08f1
> --- /dev/null
> +++ b/OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c
> @@ -0,0 +1,74 @@
> +/** @file
> + OVMF's instance of the PCI Host Bridge Library.
> +
> + Copyright (c) 2020, Huawei Corporation. All rights reserved.<BR>
(20) Please preserve the (C) notices from the original file (from where
the code is coming); namely
"OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c".
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#include <Library/DebugLib.h>
> +#include <Library/PciHostBridgeUtilityLib.h>
(21) Is this #include list really sufficient to compile the tree when
only this patch is applied?
I would think the following directive is needed, additionally:
#include <IndustryStandard/Acpi10.h>
(I suggest placing it above the Library #includes.)
> +
> +
> +GLOBAL_REMOVE_IF_UNREFERENCED
> +CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
> + L"Mem", L"I/O", L"Bus"
> +};
> +
> +
(22) Please rename this object to
"mPciHostBridgeUtilityLibAcpiAddressSpaceTypeStr" (you'll have to modify
the reference to it below as well).
> +/**
> + Inform the platform that the resource conflict happens.
> +
> + @param HostBridgeHandle Handle of the Host Bridge.
> + @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
> +PciHostBridgeResourceConflict (
(23) Please rename to "PciHostBridgeUtilityResourceConflict".
(24) Please drop "HostBridgeHandle", corresponding to point (16).
> + EFI_HANDLE HostBridgeHandle,
> + 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
> + );
> + }
> +}
> +
>
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67291): https://edk2.groups.io/g/devel/message/67291
Mute This Topic: https://groups.io/mt/78135579/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