[PATCH 0/2] UefiPayloadPkg: Use PCIE base address set by boot loader

[PATCH 0/2] UefiPayloadPkg: Use PCIE base address set by boot loader

[Ni, Ray]

Ray Ni (2):
  MdePkg/PciSegmentInfoLib: Add missing EFIAPI to GetPciSegmentInfo()
  UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB

 MdePkg/Include/Library/PciSegmentInfoLib.h    |  1 +
 .../PciSegmentInfoLib.c                       |  1 +
 .../PciSegmentInfoLibAcpiBoardInfo.c          | 55 +++++++++++++++++++
 .../PciSegmentInfoLibAcpiBoardInfo.inf        | 36 ++++++++++++
 UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc      |  6 +-
 5 files changed, 97 insertions(+), 2 deletions(-)
 create mode 100644 UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.c
 create mode 100644 UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.inf

-- 
2.26.2.windows.1

[PATCH 1/2] MdePkg/PciSegmentInfoLib: Add missing EFIAPI to GetPciSegmentInfo()

[Ni, Ray]

All public APIs should have EFIAPI modifier. Somehow PciSegmentInfoLib
missed the EFIAPI modifier.
The patch updates the library header file and NULL instance in MdePkg
to add the missing EFIAPI.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Include/Library/PciSegmentInfoLib.h                   | 1 +
 MdePkg/Library/BasePciSegmentInfoLibNull/PciSegmentInfoLib.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/MdePkg/Include/Library/PciSegmentInfoLib.h b/MdePkg/Include/Library/PciSegmentInfoLib.h
index 320a223628..a59644af99 100644
--- a/MdePkg/Include/Library/PciSegmentInfoLib.h
+++ b/MdePkg/Include/Library/PciSegmentInfoLib.h
@@ -28,6 +28,7 @@ typedef struct {
   @retval A callee owned array holding the segment information.
 **/
 PCI_SEGMENT_INFO *
+EFIAPI
 GetPciSegmentInfo (
   UINTN  *Count
   );
diff --git a/MdePkg/Library/BasePciSegmentInfoLibNull/PciSegmentInfoLib.c b/MdePkg/Library/BasePciSegmentInfoLibNull/PciSegmentInfoLib.c
index f9019c4e64..11c09506ab 100644
--- a/MdePkg/Library/BasePciSegmentInfoLibNull/PciSegmentInfoLib.c
+++ b/MdePkg/Library/BasePciSegmentInfoLibNull/PciSegmentInfoLib.c
@@ -21,6 +21,7 @@
   @retval A callee owned array holding the segment information.
 **/
 PCI_SEGMENT_INFO *
+EFIAPI
 GetPciSegmentInfo (
   UINTN  *Count
   )
-- 
2.26.2.windows.1

[PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB

[Ni, Ray]

Today's UefiPayloadPkg always uses 0xE0000000 as the PCIE base address
and ignores the value set in AcpiBoardInfo HOB created by the boot
loader. This makes the payload binary cannot work in environment
where the PCIE base address set by boot loader doesn't equal to
0xE0000000.

The patch enhances UefiPayloadPkg so that the PCIE base address
set by boot loader in the AcpiBoardInfo HOB is used.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
---
 .../PciSegmentInfoLibAcpiBoardInfo.c          | 55 +++++++++++++++++++
 .../PciSegmentInfoLibAcpiBoardInfo.inf        | 36 ++++++++++++
 UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc      |  6 +-
 3 files changed, 95 insertions(+), 2 deletions(-)
 create mode 100644 UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.c
 create mode 100644 UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.inf

diff --git a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.c b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.c
new file mode 100644
index 0000000000..28ca4b5799
--- /dev/null
+++ b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.c
@@ -0,0 +1,55 @@
+/** @file
+  PCI Segment Information Library that returns one segment whose
+  segment base address is retrieved from AcpiBoardInfo HOB.
+
+  Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <PiDxe.h>
+#include <Guid/AcpiBoardInfoGuid.h>
+
+#include <Library/HobLib.h>
+#include <Library/PciSegmentInfoLib.h>
+#include <Library/DebugLib.h>
+
+STATIC PCI_SEGMENT_INFO mPciSegment0 = {
+  0,  // Segment number
+  0,  // To be fixed later
+  0,  // Start bus number
+  255 // End bus number
+};
+
+/**
+  Return an array of PCI_SEGMENT_INFO holding the segment information.
+
+  Note: The returned array/buffer is owned by callee.
+
+  @param  Count  Return the count of segments.
+
+  @retval A callee owned array holding the segment information.
+**/
+PCI_SEGMENT_INFO *
+GetPciSegmentInfo (
+  UINTN  *Count
+  )
+{
+  EFI_HOB_GUID_TYPE  *GuidHob;
+  ACPI_BOARD_INFO    *AcpiBoardInfo;
+
+  ASSERT (Count != NULL);
+
+  if (mPciSegment0.BaseAddress == 0) {
+    //
+    // Find the acpi board information guid hob
+    //
+    GuidHob = GetFirstGuidHob (&gUefiAcpiBoardInfoGuid);
+    ASSERT (GuidHob != NULL);
+
+    AcpiBoardInfo = (ACPI_BOARD_INFO *) GET_GUID_HOB_DATA (GuidHob);
+    mPciSegment0.BaseAddress = AcpiBoardInfo->PcieBaseAddress;
+  }
+  *Count = 1;
+  return &mPciSegment0;
+}
diff --git a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.inf b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.inf
new file mode 100644
index 0000000000..ec4dbaaa55
--- /dev/null
+++ b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.inf
@@ -0,0 +1,36 @@
+## @file
+#   PCI Segment Information Library that returns one segment whose
+#   segment base address is retrieved from AcpiBoardInfo HOB.
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = PciSegmentInfoLibAcpiBoardInfo
+  FILE_GUID                      = 0EA82AA2-6C36-4FD5-BC90-FFA3ECB5E0CE
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = PciSegmentInfoLib | DXE_DRIVER
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64 EBC
+#
+
+[Sources]
+  PciSegmentInfoLibAcpiBoardInfo.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  UefiPayloadPkg/UefiPayloadPkg.dec
+
+[LibraryClasses]
+  PcdLib
+  HobLib
+  DebugLib
diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
index 0736cd9954..e114039f82 100644
--- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
@@ -129,7 +129,8 @@ [LibraryClasses]
   PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
   PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
 !endif
-  PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
+  PciSegmentLib|MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegmentLibSegmentInfo.inf
+  PciSegmentInfoLib|UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoardInfo.inf
   PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
   PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeCoffGetEntryPointLib.inf
   CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
@@ -288,7 +289,6 @@ [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
 
-  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)
 
 !if $(SOURCE_DEBUG_ENABLE)
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
@@ -360,6 +360,8 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100
 
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)
+
 ################################################################################
 #
 # Components Section - list of all EDK II Modules needed by this Platform.
-- 
2.26.2.windows.1

Re: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB

[Ma, Maurice]

Hi, Ray,

Thank you very much for making this change.

Two minor comments here,  
- Should we add "EFIAPI" for function GetPciSegmentInfo() since it is standard library interface ? 
- For  ASSERT (Count != NULL),   can we have an error handling flow in the code to deal with the case ?

Thanks
Maurice

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, June 3, 2020 3:21
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>;
> You, Benjamin <benjamin.you@intel.com>
> Subject: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in
> AcpiBoardInfo HOB
> 
> Today's UefiPayloadPkg always uses 0xE0000000 as the PCIE base address and
> ignores the value set in AcpiBoardInfo HOB created by the boot loader. This
> makes the payload binary cannot work in environment where the PCIE base
> address set by boot loader doesn't equal to 0xE0000000.
> 
> The patch enhances UefiPayloadPkg so that the PCIE base address set by boot
> loader in the AcpiBoardInfo HOB is used.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> ---
>  .../PciSegmentInfoLibAcpiBoardInfo.c          | 55 +++++++++++++++++++
>  .../PciSegmentInfoLibAcpiBoardInfo.inf        | 36 ++++++++++++
>  UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc      |  6 +-
>  3 files changed, 95 insertions(+), 2 deletions(-)  create mode 100644
> UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAc
> piBoardInfo.c
>  create mode 100644
> UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAc
> piBoardInfo.inf
> 
> diff --git
> a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLib
> AcpiBoardInfo.c
> b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLib
> AcpiBoardInfo.c
> new file mode 100644
> index 0000000000..28ca4b5799
> --- /dev/null
> +++ b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentIn
> +++ foLibAcpiBoardInfo.c
> @@ -0,0 +1,55 @@
> +/** @file+  PCI Segment Information Library that returns one segment
> whose+  segment base address is retrieved from AcpiBoardInfo HOB.++
> Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+  SPDX-License-
> Identifier: BSD-2-Clause-Patent++**/++#include <PiDxe.h>+#include
> <Guid/AcpiBoardInfoGuid.h>++#include <Library/HobLib.h>+#include
> <Library/PciSegmentInfoLib.h>+#include <Library/DebugLib.h>++STATIC
> PCI_SEGMENT_INFO mPciSegment0 = {+  0,  // Segment number+  0,  // To be
> fixed later+  0,  // Start bus number+  255 // End bus number+};++/**+  Return
> an array of PCI_SEGMENT_INFO holding the segment information.++  Note: The
> returned array/buffer is owned by callee.++  @param  Count  Return the count
> of segments.++  @retval A callee owned array holding the segment
> information.+**/+PCI_SEGMENT_INFO *+GetPciSegmentInfo (+  UINTN
> *Count+  )+{+  EFI_HOB_GUID_TYPE  *GuidHob;+  ACPI_BOARD_INFO
> *AcpiBoardInfo;++  ASSERT (Count != NULL);++  if (mPciSegment0.BaseAddress
> == 0) {+    //+    // Find the acpi board information guid hob+    //+    GuidHob =
> GetFirstGuidHob (&gUefiAcpiBoardInfoGuid);+    ASSERT (GuidHob != NULL);++
> AcpiBoardInfo = (ACPI_BOARD_INFO *) GET_GUID_HOB_DATA (GuidHob);+
> mPciSegment0.BaseAddress = AcpiBoardInfo->PcieBaseAddress;+  }+  *Count =
> 1;+  return &mPciSegment0;+}diff --git
> a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLib
> AcpiBoardInfo.inf
> b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLib
> AcpiBoardInfo.inf
> new file mode 100644
> index 0000000000..ec4dbaaa55
> --- /dev/null
> +++ b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentIn
> +++ foLibAcpiBoardInfo.inf
> @@ -0,0 +1,36 @@
> +## @file+#   PCI Segment Information Library that returns one segment
> whose+#   segment base address is retrieved from AcpiBoardInfo HOB.+#+#
> Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+#+#  SPDX-
> License-Identifier: BSD-2-Clause-Patent+#+#+##++[Defines]+  INF_VERSION
> = 0x00010005+  BASE_NAME                      = PciSegmentInfoLibAcpiBoardInfo+
> FILE_GUID                      = 0EA82AA2-6C36-4FD5-BC90-FFA3ECB5E0CE+
> MODULE_TYPE                    = BASE+  VERSION_STRING                 = 1.0+
> LIBRARY_CLASS                  = PciSegmentInfoLib | DXE_DRIVER++#+# The
> following information is for reference only and not required by the build
> tools.+#+#  VALID_ARCHITECTURES           = IA32 X64 EBC+#++[Sources]+
> PciSegmentInfoLibAcpiBoardInfo.c++[Packages]+  MdePkg/MdePkg.dec+
> UefiPayloadPkg/UefiPayloadPkg.dec++[LibraryClasses]+  PcdLib+  HobLib+
> DebugLibdiff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> index 0736cd9954..e114039f82 100644
> --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> @@ -129,7 +129,8 @@ [LibraryClasses]
>    PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf !endif-
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.i
> nf+
> PciSegmentLib|MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegmentLi
> bSegmentInfo.inf+
> PciSegmentInfoLib|UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/Pc
> iSegmentInfoLibAcpiBoardInfo.inf
> PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeC
> offGetEntryPointLib.inf
> CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCache
> MaintenanceLib.inf@@ -288,7 +289,6 @@ [PcdsFixedAtBuild]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
> gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa,
> 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23,
> 0x31 } -
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)  !if
> $(SOURCE_DEBUG_ENABLE)
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2@
> @ -360,6 +360,8 @@ [PcdsDynamicDefault]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31
> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100 +
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)+
> ################################################################
> ################ # # Components Section - list of all EDK II Modules needed
> by this Platform.--
> 2.26.2.windows.1

Re: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB

[Guo Dong]

Hi Ray,

How about set PcdPciExpressBaseAddress to 0 instead of $(PCIE_BASE) by default?
We could even remove PCIE_BASE from DSC.

Thanks,
Guo

> -----Original Message-----
> From: Ma, Maurice <maurice.ma@intel.com>
> Sent: Wednesday, June 3, 2020 8:35 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> <benjamin.you@intel.com>
> Subject: RE: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored
> in AcpiBoardInfo HOB
> 
> Hi, Ray,
> 
> Thank you very much for making this change.
> 
> Two minor comments here,
> - Should we add "EFIAPI" for function GetPciSegmentInfo() since it is
> standard library interface ?
> - For  ASSERT (Count != NULL),   can we have an error handling flow in the
> code to deal with the case ?
> 
> Thanks
> Maurice
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Wednesday, June 3, 2020 3:21
> > To: devel@edk2.groups.io
> > Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > Subject: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored
> > in AcpiBoardInfo HOB
> >
> > Today's UefiPayloadPkg always uses 0xE0000000 as the PCIE base address
> > and ignores the value set in AcpiBoardInfo HOB created by the boot
> > loader. This makes the payload binary cannot work in environment where
> > the PCIE base address set by boot loader doesn't equal to 0xE0000000.
> >
> > The patch enhances UefiPayloadPkg so that the PCIE base address set by
> > boot loader in the AcpiBoardInfo HOB is used.
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Maurice Ma <maurice.ma@intel.com>
> > Cc: Guo Dong <guo.dong@intel.com>
> > Cc: Benjamin You <benjamin.you@intel.com>
> > ---
> >  .../PciSegmentInfoLibAcpiBoardInfo.c          | 55 +++++++++++++++++++
> >  .../PciSegmentInfoLibAcpiBoardInfo.inf        | 36 ++++++++++++
> >  UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc      |  6 +-
> >  3 files changed, 95 insertions(+), 2 deletions(-)  create mode 100644
> >
> UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLi
> > bAc
> > piBoardInfo.c
> >  create mode 100644
> >
> UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLi
> > bAc
> > piBoardInfo.inf
> >
> > diff --git
> >
> a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > Lib
> > AcpiBoardInfo.c
> >
> b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > Lib
> > AcpiBoardInfo.c
> > new file mode 100644
> > index 0000000000..28ca4b5799
> > --- /dev/null
> > +++
> b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegment
> > +++ In
> > +++ foLibAcpiBoardInfo.c
> > @@ -0,0 +1,55 @@
> > +/** @file+  PCI Segment Information Library that returns one segment
> > whose+  segment base address is retrieved from AcpiBoardInfo HOB.++
> > Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+
> > SPDX-License-
> > Identifier: BSD-2-Clause-Patent++**/++#include <PiDxe.h>+#include
> > <Guid/AcpiBoardInfoGuid.h>++#include <Library/HobLib.h>+#include
> > <Library/PciSegmentInfoLib.h>+#include <Library/DebugLib.h>++STATIC
> > PCI_SEGMENT_INFO mPciSegment0 = {+  0,  // Segment number+  0,  // To
> > be fixed later+  0,  // Start bus number+  255 // End bus
> > number+};++/**+  Return an array of PCI_SEGMENT_INFO holding the
> > segment information.++  Note: The returned array/buffer is owned by
> > callee.++  @param  Count  Return the count of segments.++  @retval A
> > callee owned array holding the segment
> > information.+**/+PCI_SEGMENT_INFO *+GetPciSegmentInfo (+  UINTN
> > *Count+  )+{+  EFI_HOB_GUID_TYPE  *GuidHob;+  ACPI_BOARD_INFO
> *AcpiBoardInfo;++  ASSERT (Count != NULL);++  if
> (mPciSegment0.BaseAddress
> > == 0) {+    //+    // Find the acpi board information guid hob+    //+    GuidHob
> =
> > GetFirstGuidHob (&gUefiAcpiBoardInfoGuid);+    ASSERT (GuidHob !=
> NULL);++
> > AcpiBoardInfo = (ACPI_BOARD_INFO *) GET_GUID_HOB_DATA
> (GuidHob);+
> > mPciSegment0.BaseAddress = AcpiBoardInfo->PcieBaseAddress;+  }+
> > *Count = 1;+  return &mPciSegment0;+}diff --git
> >
> a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > Lib
> > AcpiBoardInfo.inf
> >
> b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > Lib
> > AcpiBoardInfo.inf
> > new file mode 100644
> > index 0000000000..ec4dbaaa55
> > --- /dev/null
> > +++
> b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegment
> > +++ In
> > +++ foLibAcpiBoardInfo.inf
> > @@ -0,0 +1,36 @@
> > +## @file+#   PCI Segment Information Library that returns one segment
> > whose+#   segment base address is retrieved from AcpiBoardInfo
> HOB.+#+#
> > Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+#+#
> > SPDX-
> > License-Identifier: BSD-2-Clause-Patent+#+#+##++[Defines]+
> INF_VERSION
> > = 0x00010005+  BASE_NAME                      = PciSegmentInfoLibAcpiBoardInfo+
> > FILE_GUID                      = 0EA82AA2-6C36-4FD5-BC90-FFA3ECB5E0CE+
> > MODULE_TYPE                    = BASE+  VERSION_STRING                 = 1.0+
> > LIBRARY_CLASS                  = PciSegmentInfoLib | DXE_DRIVER++#+# The
> > following information is for reference only and not required by the build
> > tools.+#+#  VALID_ARCHITECTURES           = IA32 X64 EBC+#++[Sources]+
> > PciSegmentInfoLibAcpiBoardInfo.c++[Packages]+  MdePkg/MdePkg.dec+
> > UefiPayloadPkg/UefiPayloadPkg.dec++[LibraryClasses]+  PcdLib+  HobLib+
> > DebugLibdiff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > index 0736cd9954..e114039f82 100644
> > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > @@ -129,7 +129,8 @@ [LibraryClasses]
> >    PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> > PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> > PciExpressLib|!endif-
> >
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibP
> ci
> > PciSegmentLib|.i
> > nf+
> >
> PciSegmentLib|MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegme
> ntLi
> > bSegmentInfo.inf+
> >
> PciSegmentInfoLib|UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInf
> > PciSegmentInfoLib|o/Pc
> > iSegmentInfoLibAcpiBoardInfo.inf
> > PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> >
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Base
> P
> > PeCoffGetEntryPointLib|eC
> > offGetEntryPointLib.inf
> >
> CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCac
> he
> > MaintenanceLib.inf@@ -288,7 +289,6 @@ [PcdsFixedAtBuild]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
> > gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21,
> 0xaa,
> > 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4,
> > 0x66, 0x23,
> > 0x31 } -
> > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)  !if
> > $(SOURCE_DEBUG_ENABLE)
> >
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x
> 2@
> > @ -360,6 +360,8 @@ [PcdsDynamicDefault]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31
> > gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100 +
> > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)+
> >
> ##########################################################
> ######
> > ################ # # Components Section - list of all EDK II Modules
> > needed by this Platform.--
> > 2.26.2.windows.1

Re: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB

[Ni, Ray]

Maurice,
1. EFIAPI: sure. I will add the missing "EFIAPI".
2. ASSERT: Is below code to avoid dereferencing NULL pointer ok to you?

  ASSERT (Count != NULL);
+  if (Count == NULL) {
+    return NULL;
+  }

Thanks,
Ray

> -----Original Message-----
> From: Ma, Maurice <maurice.ma@intel.com>
> Sent: Wednesday, June 3, 2020 11:35 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> Subject: RE: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB
> 
> Hi, Ray,
> 
> Thank you very much for making this change.
> 
> Two minor comments here,
> - Should we add "EFIAPI" for function GetPciSegmentInfo() since it is standard library interface ?
> - For  ASSERT (Count != NULL),   can we have an error handling flow in the code to deal with the case ?
> 
> Thanks
> Maurice
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Wednesday, June 3, 2020 3:21
> > To: devel@edk2.groups.io
> > Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>;
> > You, Benjamin <benjamin.you@intel.com>
> > Subject: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in
> > AcpiBoardInfo HOB
> >
> > Today's UefiPayloadPkg always uses 0xE0000000 as the PCIE base address and
> > ignores the value set in AcpiBoardInfo HOB created by the boot loader. This
> > makes the payload binary cannot work in environment where the PCIE base
> > address set by boot loader doesn't equal to 0xE0000000.
> >
> > The patch enhances UefiPayloadPkg so that the PCIE base address set by boot
> > loader in the AcpiBoardInfo HOB is used.
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Maurice Ma <maurice.ma@intel.com>
> > Cc: Guo Dong <guo.dong@intel.com>
> > Cc: Benjamin You <benjamin.you@intel.com>
> > ---
> >  .../PciSegmentInfoLibAcpiBoardInfo.c          | 55 +++++++++++++++++++
> >  .../PciSegmentInfoLibAcpiBoardInfo.inf        | 36 ++++++++++++
> >  UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc      |  6 +-
> >  3 files changed, 95 insertions(+), 2 deletions(-)  create mode 100644
> > UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAc
> > piBoardInfo.c
> >  create mode 100644
> > UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAc
> > piBoardInfo.inf
> >
> > diff --git
> > a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLib
> > AcpiBoardInfo.c
> > b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLib
> > AcpiBoardInfo.c
> > new file mode 100644
> > index 0000000000..28ca4b5799
> > --- /dev/null
> > +++ b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentIn
> > +++ foLibAcpiBoardInfo.c
> > @@ -0,0 +1,55 @@
> > +/** @file+  PCI Segment Information Library that returns one segment
> > whose+  segment base address is retrieved from AcpiBoardInfo HOB.++
> > Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+  SPDX-License-
> > Identifier: BSD-2-Clause-Patent++**/++#include <PiDxe.h>+#include
> > <Guid/AcpiBoardInfoGuid.h>++#include <Library/HobLib.h>+#include
> > <Library/PciSegmentInfoLib.h>+#include <Library/DebugLib.h>++STATIC
> > PCI_SEGMENT_INFO mPciSegment0 = {+  0,  // Segment number+  0,  // To be
> > fixed later+  0,  // Start bus number+  255 // End bus number+};++/**+  Return
> > an array of PCI_SEGMENT_INFO holding the segment information.++  Note: The
> > returned array/buffer is owned by callee.++  @param  Count  Return the count
> > of segments.++  @retval A callee owned array holding the segment
> > information.+**/+PCI_SEGMENT_INFO *+GetPciSegmentInfo (+  UINTN
> > *Count+  )+{+  EFI_HOB_GUID_TYPE  *GuidHob;+  ACPI_BOARD_INFO
> > *AcpiBoardInfo;++  ASSERT (Count != NULL);++  if (mPciSegment0.BaseAddress
> > == 0) {+    //+    // Find the acpi board information guid hob+    //+    GuidHob =
> > GetFirstGuidHob (&gUefiAcpiBoardInfoGuid);+    ASSERT (GuidHob != NULL);++
> > AcpiBoardInfo = (ACPI_BOARD_INFO *) GET_GUID_HOB_DATA (GuidHob);+
> > mPciSegment0.BaseAddress = AcpiBoardInfo->PcieBaseAddress;+  }+  *Count =
> > 1;+  return &mPciSegment0;+}diff --git
> > a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLib
> > AcpiBoardInfo.inf
> > b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLib
> > AcpiBoardInfo.inf
> > new file mode 100644
> > index 0000000000..ec4dbaaa55
> > --- /dev/null
> > +++ b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentIn
> > +++ foLibAcpiBoardInfo.inf
> > @@ -0,0 +1,36 @@
> > +## @file+#   PCI Segment Information Library that returns one segment
> > whose+#   segment base address is retrieved from AcpiBoardInfo HOB.+#+#
> > Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+#+#  SPDX-
> > License-Identifier: BSD-2-Clause-Patent+#+#+##++[Defines]+  INF_VERSION
> > = 0x00010005+  BASE_NAME                      = PciSegmentInfoLibAcpiBoardInfo+
> > FILE_GUID                      = 0EA82AA2-6C36-4FD5-BC90-FFA3ECB5E0CE+
> > MODULE_TYPE                    = BASE+  VERSION_STRING                 = 1.0+
> > LIBRARY_CLASS                  = PciSegmentInfoLib | DXE_DRIVER++#+# The
> > following information is for reference only and not required by the build
> > tools.+#+#  VALID_ARCHITECTURES           = IA32 X64 EBC+#++[Sources]+
> > PciSegmentInfoLibAcpiBoardInfo.c++[Packages]+  MdePkg/MdePkg.dec+
> > UefiPayloadPkg/UefiPayloadPkg.dec++[LibraryClasses]+  PcdLib+  HobLib+
> > DebugLibdiff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > index 0736cd9954..e114039f82 100644
> > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > @@ -129,7 +129,8 @@ [LibraryClasses]
> >    PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> > PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf !endif-
> > PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.i
> > nf+
> > PciSegmentLib|MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegmentLi
> > bSegmentInfo.inf+
> > PciSegmentInfoLib|UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/Pc
> > iSegmentInfoLibAcpiBoardInfo.inf
> > PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> > PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BasePeC
> > offGetEntryPointLib.inf
> > CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCache
> > MaintenanceLib.inf@@ -288,7 +289,6 @@ [PcdsFixedAtBuild]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
> > gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa,
> > 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23,
> > 0x31 } -
> > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)  !if
> > $(SOURCE_DEBUG_ENABLE)
> > gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2@
> > @ -360,6 +360,8 @@ [PcdsDynamicDefault]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31
> > gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100 +
> > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)+
> > ################################################################
> > ################ # # Components Section - list of all EDK II Modules needed
> > by this Platform.--
> > 2.26.2.windows.1
> 

Re: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB

[Ni, Ray]

Guo,
Thanks for the comments. It's a good cleanup. I will remove PCIE_BASE from DSC.

> -----Original Message-----
> From: Dong, Guo <guo.dong@intel.com>
> Sent: Thursday, June 4, 2020 1:39 AM
> To: Ma, Maurice <maurice.ma@intel.com>; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: You, Benjamin <benjamin.you@intel.com>
> Subject: RE: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB
> 
> 
> Hi Ray,
> 
> How about set PcdPciExpressBaseAddress to 0 instead of $(PCIE_BASE) by default?
> We could even remove PCIE_BASE from DSC.
> 
> Thanks,
> Guo
> 
> > -----Original Message-----
> > From: Ma, Maurice <maurice.ma@intel.com>
> > Sent: Wednesday, June 3, 2020 8:35 AM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > <benjamin.you@intel.com>
> > Subject: RE: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored
> > in AcpiBoardInfo HOB
> >
> > Hi, Ray,
> >
> > Thank you very much for making this change.
> >
> > Two minor comments here,
> > - Should we add "EFIAPI" for function GetPciSegmentInfo() since it is
> > standard library interface ?
> > - For  ASSERT (Count != NULL),   can we have an error handling flow in the
> > code to deal with the case ?
> >
> > Thanks
> > Maurice
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Wednesday, June 3, 2020 3:21
> > > To: devel@edk2.groups.io
> > > Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > > Subject: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored
> > > in AcpiBoardInfo HOB
> > >
> > > Today's UefiPayloadPkg always uses 0xE0000000 as the PCIE base address
> > > and ignores the value set in AcpiBoardInfo HOB created by the boot
> > > loader. This makes the payload binary cannot work in environment where
> > > the PCIE base address set by boot loader doesn't equal to 0xE0000000.
> > >
> > > The patch enhances UefiPayloadPkg so that the PCIE base address set by
> > > boot loader in the AcpiBoardInfo HOB is used.
> > >
> > > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > > Cc: Maurice Ma <maurice.ma@intel.com>
> > > Cc: Guo Dong <guo.dong@intel.com>
> > > Cc: Benjamin You <benjamin.you@intel.com>
> > > ---
> > >  .../PciSegmentInfoLibAcpiBoardInfo.c          | 55 +++++++++++++++++++
> > >  .../PciSegmentInfoLibAcpiBoardInfo.inf        | 36 ++++++++++++
> > >  UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc      |  6 +-
> > >  3 files changed, 95 insertions(+), 2 deletions(-)  create mode 100644
> > >
> > UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLi
> > > bAc
> > > piBoardInfo.c
> > >  create mode 100644
> > >
> > UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLi
> > > bAc
> > > piBoardInfo.inf
> > >
> > > diff --git
> > >
> > a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > > Lib
> > > AcpiBoardInfo.c
> > >
> > b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > > Lib
> > > AcpiBoardInfo.c
> > > new file mode 100644
> > > index 0000000000..28ca4b5799
> > > --- /dev/null
> > > +++
> > b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegment
> > > +++ In
> > > +++ foLibAcpiBoardInfo.c
> > > @@ -0,0 +1,55 @@
> > > +/** @file+  PCI Segment Information Library that returns one segment
> > > whose+  segment base address is retrieved from AcpiBoardInfo HOB.++
> > > Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+
> > > SPDX-License-
> > > Identifier: BSD-2-Clause-Patent++**/++#include <PiDxe.h>+#include
> > > <Guid/AcpiBoardInfoGuid.h>++#include <Library/HobLib.h>+#include
> > > <Library/PciSegmentInfoLib.h>+#include <Library/DebugLib.h>++STATIC
> > > PCI_SEGMENT_INFO mPciSegment0 = {+  0,  // Segment number+  0,  // To
> > > be fixed later+  0,  // Start bus number+  255 // End bus
> > > number+};++/**+  Return an array of PCI_SEGMENT_INFO holding the
> > > segment information.++  Note: The returned array/buffer is owned by
> > > callee.++  @param  Count  Return the count of segments.++  @retval A
> > > callee owned array holding the segment
> > > information.+**/+PCI_SEGMENT_INFO *+GetPciSegmentInfo (+  UINTN
> > > *Count+  )+{+  EFI_HOB_GUID_TYPE  *GuidHob;+  ACPI_BOARD_INFO
> > *AcpiBoardInfo;++  ASSERT (Count != NULL);++  if
> > (mPciSegment0.BaseAddress
> > > == 0) {+    //+    // Find the acpi board information guid hob+    //+    GuidHob
> > =
> > > GetFirstGuidHob (&gUefiAcpiBoardInfoGuid);+    ASSERT (GuidHob !=
> > NULL);++
> > > AcpiBoardInfo = (ACPI_BOARD_INFO *) GET_GUID_HOB_DATA
> > (GuidHob);+
> > > mPciSegment0.BaseAddress = AcpiBoardInfo->PcieBaseAddress;+  }+
> > > *Count = 1;+  return &mPciSegment0;+}diff --git
> > >
> > a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > > Lib
> > > AcpiBoardInfo.inf
> > >
> > b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > > Lib
> > > AcpiBoardInfo.inf
> > > new file mode 100644
> > > index 0000000000..ec4dbaaa55
> > > --- /dev/null
> > > +++
> > b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegment
> > > +++ In
> > > +++ foLibAcpiBoardInfo.inf
> > > @@ -0,0 +1,36 @@
> > > +## @file+#   PCI Segment Information Library that returns one segment
> > > whose+#   segment base address is retrieved from AcpiBoardInfo
> > HOB.+#+#
> > > Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+#+#
> > > SPDX-
> > > License-Identifier: BSD-2-Clause-Patent+#+#+##++[Defines]+
> > INF_VERSION
> > > = 0x00010005+  BASE_NAME                      = PciSegmentInfoLibAcpiBoardInfo+
> > > FILE_GUID                      = 0EA82AA2-6C36-4FD5-BC90-FFA3ECB5E0CE+
> > > MODULE_TYPE                    = BASE+  VERSION_STRING                 = 1.0+
> > > LIBRARY_CLASS                  = PciSegmentInfoLib | DXE_DRIVER++#+# The
> > > following information is for reference only and not required by the build
> > > tools.+#+#  VALID_ARCHITECTURES           = IA32 X64 EBC+#++[Sources]+
> > > PciSegmentInfoLibAcpiBoardInfo.c++[Packages]+  MdePkg/MdePkg.dec+
> > > UefiPayloadPkg/UefiPayloadPkg.dec++[LibraryClasses]+  PcdLib+  HobLib+
> > > DebugLibdiff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > index 0736cd9954..e114039f82 100644
> > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > @@ -129,7 +129,8 @@ [LibraryClasses]
> > >    PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> > > PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> > > PciExpressLib|!endif-
> > >
> > PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibP
> > ci
> > > PciSegmentLib|.i
> > > nf+
> > >
> > PciSegmentLib|MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegme
> > ntLi
> > > bSegmentInfo.inf+
> > >
> > PciSegmentInfoLib|UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInf
> > > PciSegmentInfoLib|o/Pc
> > > iSegmentInfoLibAcpiBoardInfo.inf
> > > PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> > >
> > PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Base
> > P
> > > PeCoffGetEntryPointLib|eC
> > > offGetEntryPointLib.inf
> > >
> > CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCac
> > he
> > > MaintenanceLib.inf@@ -288,7 +289,6 @@ [PcdsFixedAtBuild]
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21,
> > 0xaa,
> > > 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4,
> > > 0x66, 0x23,
> > > 0x31 } -
> > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)  !if
> > > $(SOURCE_DEBUG_ENABLE)
> > >
> > gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x
> > 2@
> > > @ -360,6 +360,8 @@ [PcdsDynamicDefault]
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100 +
> > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)+
> > >
> > ##########################################################
> > ######
> > > ################ # # Components Section - list of all EDK II Modules
> > > needed by this Platform.--
> > > 2.26.2.windows.1
> 

Re: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo HOB

[Ma, Maurice]

Hi, Ray,

Yes,  that addressed my questions.   Thank you!

Thanks
Maurice
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, June 3, 2020 20:06
> To: Ma, Maurice <maurice.ma@intel.com>; devel@edk2.groups.io
> Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> <benjamin.you@intel.com>
> Subject: RE: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in
> AcpiBoardInfo HOB
> 
> Maurice,
> 1. EFIAPI: sure. I will add the missing "EFIAPI".
> 2. ASSERT: Is below code to avoid dereferencing NULL pointer ok to you?
> 
>   ASSERT (Count != NULL);
> +  if (Count == NULL) {
> +    return NULL;
> +  }
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Ma, Maurice <maurice.ma@intel.com>
> > Sent: Wednesday, June 3, 2020 11:35 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Dong, Guo <guo.dong@intel.com>; You, Benjamin
> > <benjamin.you@intel.com>
> > Subject: RE: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr
> > stored in AcpiBoardInfo HOB
> >
> > Hi, Ray,
> >
> > Thank you very much for making this change.
> >
> > Two minor comments here,
> > - Should we add "EFIAPI" for function GetPciSegmentInfo() since it is standard
> library interface ?
> > - For  ASSERT (Count != NULL),   can we have an error handling flow in the
> code to deal with the case ?
> >
> > Thanks
> > Maurice
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Wednesday, June 3, 2020 3:21
> > > To: devel@edk2.groups.io
> > > Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > > Subject: [PATCH 2/2] UefiPayloadPkg/Pci: Use the PCIE Base Addr
> > > stored in AcpiBoardInfo HOB
> > >
> > > Today's UefiPayloadPkg always uses 0xE0000000 as the PCIE base
> > > address and ignores the value set in AcpiBoardInfo HOB created by
> > > the boot loader. This makes the payload binary cannot work in
> > > environment where the PCIE base address set by boot loader doesn't equal
> to 0xE0000000.
> > >
> > > The patch enhances UefiPayloadPkg so that the PCIE base address set
> > > by boot loader in the AcpiBoardInfo HOB is used.
> > >
> > > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > > Cc: Maurice Ma <maurice.ma@intel.com>
> > > Cc: Guo Dong <guo.dong@intel.com>
> > > Cc: Benjamin You <benjamin.you@intel.com>
> > > ---
> > >  .../PciSegmentInfoLibAcpiBoardInfo.c          | 55 +++++++++++++++++++
> > >  .../PciSegmentInfoLibAcpiBoardInfo.inf        | 36 ++++++++++++
> > >  UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc      |  6 +-
> > >  3 files changed, 95 insertions(+), 2 deletions(-)  create mode
> > > 100644
> > > UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > > LibAc
> > > piBoardInfo.c
> > >  create mode 100644
> > > UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfo
> > > LibAc
> > > piBoardInfo.inf
> > >
> > > diff --git
> > > a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentIn
> > > foLib
> > > AcpiBoardInfo.c
> > > b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentIn
> > > foLib
> > > AcpiBoardInfo.c
> > > new file mode 100644
> > > index 0000000000..28ca4b5799
> > > --- /dev/null
> > > +++ b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegme
> > > +++ ntIn
> > > +++ foLibAcpiBoardInfo.c
> > > @@ -0,0 +1,55 @@
> > > +/** @file+  PCI Segment Information Library that returns one
> > > +segment
> > > whose+  segment base address is retrieved from AcpiBoardInfo HOB.++
> > > Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+
> > > SPDX-License-
> > > Identifier: BSD-2-Clause-Patent++**/++#include <PiDxe.h>+#include
> > > <Guid/AcpiBoardInfoGuid.h>++#include <Library/HobLib.h>+#include
> > > <Library/PciSegmentInfoLib.h>+#include <Library/DebugLib.h>++STATIC
> > > PCI_SEGMENT_INFO mPciSegment0 = {+  0,  // Segment number+  0,  //
> > > To be fixed later+  0,  // Start bus number+  255 // End bus
> > > number+};++/**+  Return an array of PCI_SEGMENT_INFO holding the
> > > segment information.++  Note: The returned array/buffer is owned by
> > > callee.++  @param  Count  Return the count of segments.++  @retval A
> > > callee owned array holding the segment
> > > information.+**/+PCI_SEGMENT_INFO *+GetPciSegmentInfo (+  UINTN
> > > *Count+  )+{+  EFI_HOB_GUID_TYPE  *GuidHob;+  ACPI_BOARD_INFO
> *AcpiBoardInfo;++  ASSERT (Count != NULL);++  if (mPciSegment0.BaseAddress
> > > == 0) {+    //+    // Find the acpi board information guid hob+    //+
> GuidHob =
> > > GetFirstGuidHob (&gUefiAcpiBoardInfoGuid);+    ASSERT (GuidHob !=
> NULL);++
> > > AcpiBoardInfo = (ACPI_BOARD_INFO *) GET_GUID_HOB_DATA (GuidHob);+
> > > mPciSegment0.BaseAddress = AcpiBoardInfo->PcieBaseAddress;+  }+
> > > *Count = 1;+  return &mPciSegment0;+}diff --git
> > > a/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentIn
> > > foLib
> > > AcpiBoardInfo.inf
> > > b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentIn
> > > foLib
> > > AcpiBoardInfo.inf
> > > new file mode 100644
> > > index 0000000000..ec4dbaaa55
> > > --- /dev/null
> > > +++ b/UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegme
> > > +++ ntIn
> > > +++ foLibAcpiBoardInfo.inf
> > > @@ -0,0 +1,36 @@
> > > +## @file+#   PCI Segment Information Library that returns one segment
> > > whose+#   segment base address is retrieved from AcpiBoardInfo HOB.+#+#
> > > Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>+#+#
> > > SPDX-
> > > License-Identifier: BSD-2-Clause-Patent+#+#+##++[Defines]+  INF_VERSION
> > > = 0x00010005+  BASE_NAME                      =
> PciSegmentInfoLibAcpiBoardInfo+
> > > FILE_GUID                      = 0EA82AA2-6C36-4FD5-BC90-FFA3ECB5E0CE+
> > > MODULE_TYPE                    = BASE+  VERSION_STRING                 = 1.0+
> > > LIBRARY_CLASS                  = PciSegmentInfoLib | DXE_DRIVER++#+# The
> > > following information is for reference only and not required by the build
> > > tools.+#+#  VALID_ARCHITECTURES           = IA32 X64 EBC+#++[Sources]+
> > > PciSegmentInfoLibAcpiBoardInfo.c++[Packages]+  MdePkg/MdePkg.dec+
> > > UefiPayloadPkg/UefiPayloadPkg.dec++[LibraryClasses]+  PcdLib+
> > > HobLib+ DebugLibdiff --git
> > > a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > index 0736cd9954..e114039f82 100644
> > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > @@ -129,7 +129,8 @@ [LibraryClasses]
> > >
> > > PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> > > PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> > > PciExpressLib|!endif-
> > >
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibP
> > > PciSegmentLib|ci.i
> > > nf+
> > >
> PciSegmentLib|MdePkg/Library/PciSegmentLibSegmentInfo/BasePciSegment
> > > PciSegmentLib|Li
> > > bSegmentInfo.inf+
> > > PciSegmentInfoLib|UefiPayloadPkg/Library/PciSegmentInfoLibAcpiBoardI
> > > PciSegmentInfoLib|nfo/Pc
> > > iSegmentInfoLibAcpiBoardInfo.inf
> > > PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
> > > PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Bas
> > > PeCoffGetEntryPointLib|ePeC
> > > offGetEntryPointLib.inf
> > >
> CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCache
> > > MaintenanceLib.inf@@ -288,7 +289,6 @@ [PcdsFixedAtBuild]
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21,
> 0xaa,
> > > 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4,
> > > 0x66, 0x23,
> > > 0x31 } -
> > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)  !if
> > > $(SOURCE_DEBUG_ENABLE)
> > >
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2@
> > > @ -360,6 +360,8 @@ [PcdsDynamicDefault]
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100 +
> > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|$(PCIE_BASE)+
> > >
> ################################################################
> > > ################ # # Components Section - list of all EDK II Modules
> > > needed by this Platform.--
> > > 2.26.2.windows.1
> >
>