[edk2-devel] [PATCH v2 2/2] MdePkg: Add support for variable size MMCONF space
Liming Gao
liming.gao at intel.com
Tue Jul 21 00:49:39 UTC 2020
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 <maurice.ma at intel.com>
Sent: 2020年7月20日 23:26
To: Marcello Sylvester Bauer <marcello.bauer at 9elements.com>; devel at edk2.groups.io
Cc: Patrick Rudolph <patrick.rudolph at 9elements.com>; Christian Walter <christian.walter at 9elements.com>; Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>; Zeng, Star <star.zeng at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>; Gao, Liming <liming.gao at 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 <marcello.bauer at 9elements.com>
> Sent: Thursday, July 16, 2020 4:48
> To: devel at edk2.groups.io
> Cc: Patrick Rudolph <patrick.rudolph at 9elements.com>; Christian Walter
> <christian.walter at 9elements.com>; Ma, Maurice <maurice.ma at intel.com>;
> Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>; Zeng, Star
> <star.zeng at intel.com>; Kinney, Michael D <michael.d.kinney at intel.com>;
> Gao, Liming <liming.gao at intel.com>
> Subject: [PATCH v2 2/2] MdePkg: Add support for variable size MMCONF
> space
>
> From: Patrick Rudolph <patrick.rudolph at 9elements.com>
>
> 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 <patrick.rudolph at 9elements.com>
> Signed-off-by: Marcello Sylvester Bauer <marcello.bauer at 9elements.com>
> Cc: Patrick Rudolph <patrick.rudolph at 9elements.com>
> Cc: Christian Walter <christian.walter at 9elements.com>
> Cc: Maurice Ma <maurice.ma at intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
> Cc: Star Zeng <star.zeng at intel.com>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Liming Gao <liming.gao at intel.com>
> ---
> 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.<BR>@@ -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.<BR>
> 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62923): https://edk2.groups.io/g/devel/message/62923
Mute This Topic: https://groups.io/mt/75539546/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