Hallo, the second patch will be divided accordingly. 1) act (done for v3) 2) the other libraries are not required for the patch series. I will adapt the commit message. 3) I will undo this change. 4) act (done for v3) 5) Indeed, my bad. On Tue, Jul 21, 2020 at 2:49 AM Liming Gao wrote: > Marcello: > Besides separate the patch for the different packages, I have some > comments on the change in PciExpressLib. > > 1) PciExpressxx() API description should be updated to include the invalid > return value 0xFF, 0xFFFF or 0xFFFFFFFF in PciExpressLib.h and > PciExpressLib library implementation. > 2) There are three PciExprssion library instancs BasePciExpressLib, > DxeRuntimePciExpressLib and SmmPciExpressLib. They are required to be > updated. > 3) For the change in MdePkg\Library\BasePciExpressLib, why remove ASSERT() > in PciExpressRegisterForRuntimeAccess() function? > 4) For the change in MdePkg\Library\BasePciExpressLib, > PcdPciExpressBaseSize() function header return value description is > incorrect. > 5) For the change in MdePkg\Library\BasePciExpressLib, > PciExpressAndThenOr32() function should return UINT32, but the code uses > return (UINT16) ~0;. There are similar issues in other function > implementation. Please check them. > > Thanks > Liming > -----Original Message----- > From: Ma, Maurice > Sent: 2020年7月20日 23:26 > To: Marcello Sylvester Bauer ; > devel@edk2.groups.io > Cc: Patrick Rudolph ; Christian Walter < > christian.walter@9elements.com>; Desimone, Nathaniel L < > nathaniel.l.desimone@intel.com>; Zeng, Star ; > Kinney, Michael D ; Gao, Liming < > liming.gao@intel.com> > Subject: RE: [PATCH v2 2/2] MdePkg: Add support for variable size MMCONF > space > > Hi Marcello, > > Since this patch mixes changes for both UefiPayloadPkg and MdePkg. I > think you might want to split this patch into two so that it can be > reviewed by the proper package maintainers. > > Thanks > Maurice > > -----Original Message----- > > From: Marcello Sylvester Bauer > > Sent: Thursday, July 16, 2020 4:48 > > To: devel@edk2.groups.io > > Cc: Patrick Rudolph ; Christian Walter > > ; Ma, Maurice ; > > Desimone, Nathaniel L ; Zeng, Star > > ; Kinney, Michael D ; > > Gao, Liming > > Subject: [PATCH v2 2/2] MdePkg: Add support for variable size MMCONF > > space > > > > From: Patrick Rudolph > > > > On embedded AMD platforms the MMCONF window is usually only 64MiB. > > > > Add support for arbitrary sized MMCONF by introducing a new PCD. > > The default size is still 256MiB, but will be overwritten by > > UefiPayloadPkg with the real MMCONF size. > > > > Fixes crash on platforms not exposing 256 buses. > > > > Tested on: > > * AMD Stoney Ridge > > > > Signed-off-by: Patrick Rudolph > > Signed-off-by: Marcello Sylvester Bauer > > Cc: Patrick Rudolph > > Cc: Christian Walter > > Cc: Maurice Ma > > Cc: Nate DeSimone > > Cc: Star Zeng > > Cc: Michael D Kinney > > Cc: Liming Gao > > --- > > MdePkg/MdePkg.dec | 4 + > > UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 + > > MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf | 6 +- > > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 1 + > > MdePkg/Include/Library/PciExpressLib.h | 5 +- > > MdePkg/Library/BasePciExpressLib/PciExpressLib.c | 118 > > +++++++++++++++++++- > > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 +- > > 7 files changed, 131 insertions(+), 8 deletions(-) > > > > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index > > 73f6c2407357..02e736a01126 100644 > > --- a/MdePkg/MdePkg.dec > > +++ b/MdePkg/MdePkg.dec > > @@ -2274,6 +2274,10 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, > > PcdsDynamic, PcdsDynamicEx] > > # @Prompt PCI Express Base Address. > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xE0000000|UINT64| > > 0x0000000a + ## This value is used to set the size of PCI express > > hierarchy. The default is 256 MB.+ # @Prompt PCI Express Base Size.+ > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize|0x0FFFFFFF|UINT64|0x00 > > 00000f+ ## Default current ISO 639-2 language: English & French. # > > @Prompt Default Value of LangCodes Variable. > > gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes|"engfraengfra" > > |VOID*|0x0000001cdiff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc > > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc > > index a768a8702c66..162cbf47a83f 100644 > > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc > > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc > > @@ -363,6 +363,7 @@ [PcdsDynamicDefault] > > gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31 > > gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100 > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0+ > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize|0 > > ############################################################# > > ################### #diff --git > > a/MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf > > b/MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf > > index a7edb74cde71..12734b022ac7 100644 > > --- a/MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf > > +++ b/MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf > > @@ -1,7 +1,7 @@ > > ## @file-# Instance of PCI Express Library using the 256 MB PCI > > Express MMIO window.+# Instance of PCI Express Library using the > > variable size PCI Express MMIO window. #-# PCI Express Library that > > uses the 256 MB PCI Express MMIO window to perform+# PCI Express > > Library that uses the variable size PCI Express MMIO window to perform > > # PCI Configuration cycles. Layers on top of an I/O Library instance. > > # # Copyright (c) 2007 - 2018, Intel Corporation. All rights > reserved.
@@ -38,4 +38,4 @@ [LibraryClasses] > > [Pcd] gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress ## > > CONSUMES-+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize ## > > CONSUMESdiff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf > > b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf > > index 1371d5eb7952..cebc81135565 100644 > > --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf > > +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf > > @@ -54,6 +54,7 @@ [Pcd] > > gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution > > gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress+ > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize [Depex] TRUEdiff --git > > a/MdePkg/Include/Library/PciExpressLib.h > > b/MdePkg/Include/Library/PciExpressLib.h > > index 826fdcf7db6c..d78193a0a352 100644 > > --- a/MdePkg/Include/Library/PciExpressLib.h > > +++ b/MdePkg/Include/Library/PciExpressLib.h > > @@ -2,8 +2,9 @@ > > Provides services to access PCI Configuration Space using the MMIO PCI > > Express window. This library is identical to the PCI Library, except > the access > > method for performing PCI- configuration cycles must be through the > > 256 MB PCI Express MMIO window whose base address- is defined by > > PcdPciExpressBaseAddress.+ configuration cycles must be through the > > PCI Express MMIO window whose base address+ is defined by > > PcdPciExpressBaseAddress and size defined by PcdPciExpressBaseSize.+ > > Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> > SPDX- > > License-Identifier: BSD-2-Clause-Patentdiff --git > > a/MdePkg/Library/BasePciExpressLib/PciExpressLib.c > > b/MdePkg/Library/BasePciExpressLib/PciExpressLib.c > > index 99a166c3609b..4d099f2c575e 100644 > > --- a/MdePkg/Library/BasePciExpressLib/PciExpressLib.c > > +++ b/MdePkg/Library/BasePciExpressLib/PciExpressLib.c > > @@ -22,7 +22,8 @@ > > /** Assert the validity of a PCI address. A valid PCI address should > contain 1's- > > only in the low 28 bits.+ only in the low 28 bits. > PcdPciExpressBaseSize limits the > > size to the real+ number of PCI busses in this segment. @param A > The address > > to validate. @@ -58,7 +59,6 @@ PciExpressRegisterForRuntimeAccess ( > > IN UINTN Address ) {- ASSERT_INVALID_PCI_ADDRESS (Address); > return > > RETURN_UNSUPPORTED; } @@ -79,6 +79,24 @@ GetPciExpressBaseAddress ( > > return (VOID*)(UINTN) PcdGet64 (PcdPciExpressBaseAddress); } +/**+ > > Gets the size of PCI Express.++ This internal functions retrieves PCI > > Express Base Size via a PCD entry+ PcdPciExpressBaseSize.++ @return > > The base address of PCI Express.++**/+STATIC+UINTN+PcdPciExpressBaseSize > (+ VOID+ )+{+ return > > (UINTN) PcdGet64 (PcdPciExpressBaseSize);+}+ /** Reads an 8-bit PCI > > configuration register. @@ -101,6 +119,9 @@ PciExpressRead8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return MmioRead8 > > ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -128,6 +149,9 @@ > > PciExpressWrite8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return MmioWrite8 > > ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -159,6 > > +183,9 @@ PciExpressOr8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return MmioOr8 > ((UINTN) > > GetPciExpressBaseAddress () + Address, OrData); } @@ -190,6 +217,9 @@ > > PciExpressAnd8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return MmioAnd8 > ((UINTN) > > GetPciExpressBaseAddress () + Address, AndData); } @@ -224,6 +254,9 @@ > > PciExpressAndThenOr8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return > MmioAndThenOr8 > > ( (UINTN) GetPciExpressBaseAddress () + Address, > AndData,@@ - > > 261,6 +294,9 @@ PciExpressBitFieldRead8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return > MmioBitFieldRead8 > > ( (UINTN) GetPciExpressBaseAddress () + Address, > StartBit,@@ - > > 302,6 +338,9 @@ PciExpressBitFieldWrite8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return > MmioBitFieldWrite8 > > ( (UINTN) GetPciExpressBaseAddress () + Address, > StartBit,@@ - > > 347,6 +386,9 @@ PciExpressBitFieldOr8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return > MmioBitFieldOr8 > > ( (UINTN) GetPciExpressBaseAddress () + Address, > StartBit,@@ - > > 392,6 +434,9 @@ PciExpressBitFieldAnd8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return > MmioBitFieldAnd8 > > ( (UINTN) GetPciExpressBaseAddress () + Address, > StartBit,@@ - > > 442,6 +487,9 @@ PciExpressBitFieldAndThenOr8 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT8) ~0;+ } return > > MmioBitFieldAndThenOr8 ( (UINTN) GetPciExpressBaseAddress () + > > Address, StartBit,@@ -474,6 +522,9 @@ PciExpressRead16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > MmioRead16 > > ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -502,6 +553,9 @@ > > PciExpressWrite16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > MmioWrite16 > > ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -534,6 > > +588,9 @@ PciExpressOr16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return MmioOr16 > > ((UINTN) GetPciExpressBaseAddress () + Address, OrData); } @@ -566,6 > > +623,9 @@ PciExpressAnd16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return MmioAnd16 > > ((UINTN) GetPciExpressBaseAddress () + Address, AndData); } @@ -601,6 > > +661,9 @@ PciExpressAndThenOr16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > > MmioAndThenOr16 ( (UINTN) GetPciExpressBaseAddress () + > Address, > > AndData,@@ -639,6 +702,9 @@ PciExpressBitFieldRead16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > > MmioBitFieldRead16 ( (UINTN) GetPciExpressBaseAddress () + > Address, > > StartBit,@@ -681,6 +747,9 @@ PciExpressBitFieldWrite16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > > MmioBitFieldWrite16 ( (UINTN) GetPciExpressBaseAddress () + > Address, > > StartBit,@@ -727,6 +796,9 @@ PciExpressBitFieldOr16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > MmioBitFieldOr16 > > ( (UINTN) GetPciExpressBaseAddress () + Address, > StartBit,@@ - > > 773,6 +845,9 @@ PciExpressBitFieldAnd16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > > MmioBitFieldAnd16 ( (UINTN) GetPciExpressBaseAddress () + > Address, > > StartBit,@@ -824,6 +899,9 @@ PciExpressBitFieldAndThenOr16 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > > MmioBitFieldAndThenOr16 ( (UINTN) GetPciExpressBaseAddress () > + > > Address, StartBit,@@ -856,6 +934,9 @@ PciExpressRead32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > MmioRead32 > > ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -884,6 +965,9 @@ > > PciExpressWrite32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > MmioWrite32 > > ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -916,6 > > +1000,9 @@ PciExpressOr32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return MmioOr32 > > ((UINTN) GetPciExpressBaseAddress () + Address, OrData); } @@ -948,6 > > +1035,9 @@ PciExpressAnd32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return MmioAnd32 > > ((UINTN) GetPciExpressBaseAddress () + Address, AndData); } @@ -983,6 > > +1073,9 @@ PciExpressAndThenOr32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > > MmioAndThenOr32 ( (UINTN) GetPciExpressBaseAddress () + > Address, > > AndData,@@ -1021,6 +1114,9 @@ PciExpressBitFieldRead32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > > MmioBitFieldRead32 ( (UINTN) GetPciExpressBaseAddress () + > Address, > > StartBit,@@ -1063,6 +1159,9 @@ PciExpressBitFieldWrite32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT16) ~0;+ } return > > MmioBitFieldWrite32 ( (UINTN) GetPciExpressBaseAddress () + > Address, > > StartBit,@@ -1109,6 +1208,9 @@ PciExpressBitFieldOr32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT32) ~0;+ } return > MmioBitFieldOr32 > > ( (UINTN) GetPciExpressBaseAddress () + Address, > StartBit,@@ - > > 1155,6 +1257,9 @@ PciExpressBitFieldAnd32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT32) ~0;+ } return > > MmioBitFieldAnd32 ( (UINTN) GetPciExpressBaseAddress () + > Address, > > StartBit,@@ -1206,6 +1311,9 @@ PciExpressBitFieldAndThenOr32 ( > > ) { ASSERT_INVALID_PCI_ADDRESS (Address);+ if (Address >= > > PcdPciExpressBaseSize()) {+ return (UINT32) ~0;+ } return > > MmioBitFieldAndThenOr32 ( (UINTN) GetPciExpressBaseAddress () > + > > Address, StartBit,@@ -1249,6 +1357,9 @@ PciExpressReadBuffer ( > > UINTN ReturnValue; ASSERT_INVALID_PCI_ADDRESS (StartAddress);+ > if > > (StartAddress >= PcdPciExpressBaseSize()) {+ return (UINTN) ~0;+ } > ASSERT > > (((StartAddress & 0xFFF) + Size) <= 0x1000); if (Size == 0) {@@ > -1349,6 > > +1460,9 @@ PciExpressWriteBuffer ( > > UINTN ReturnValue; > ASSERT_INVALID_PCI_ADDRESS > > (StartAddress);+ if (StartAddress >= PcdPciExpressBaseSize()) {+ > return > > (UINTN) ~0;+ } ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); > if (Size > > == 0) {diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c > > b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c > > index a3974dcc02f8..a746d0581ee3 100644 > > --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c > > +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c > > @@ -155,13 +155,15 @@ BlDxeEntryPoint ( > > } //- // Set PcdPciExpressBaseAddress by HOB info+ // Set > > PcdPciExpressBaseAddress and PcdPciExpressBaseSize by HOB info // > > GuidHob = GetFirstGuidHob (&gUefiAcpiBoardInfoGuid); if (GuidHob != > NULL) > > { AcpiBoardInfo = (ACPI_BOARD_INFO *)GET_GUID_HOB_DATA (GuidHob); > > Status = PcdSet64S (PcdPciExpressBaseAddress, AcpiBoardInfo- > > >PcieBaseAddress); ASSERT_EFI_ERROR (Status);+ Status = PcdSet64S > > (PcdPciExpressBaseSize, AcpiBoardInfo->PcieBaseSize);+ > ASSERT_EFI_ERROR > > (Status); } return EFI_SUCCESS;-- > > 2.27.0 > > > > > -- *[Marcello Sylvester Bauer]* 9elements Agency GmbH, Kortumstraße 19-21, 44787 Bochum, Germany Email: [DEINE EMAIL ADDRESSE] Phone: *+49 234 68 94 188 <+492346894188>* Mobile: *+49 1722847618 <+491722847618>* Sitz der Gesellschaft: Bochum Handelsregister: Amtsgericht Bochum, HRB 17519 Geschäftsführung: Sebastian Deutsch, Eray Basar Datenschutzhinweise nach Art. 13 DSGVO