* [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF
@ 2020-08-18 8:24 Marcello Sylvester Bauer
2020-08-18 8:24 ` [PATCH v5 1/3] UefiPayloadPkg: Store the size of the MMCONF window Marcello Sylvester Bauer
` (3 more replies)
0 siblings, 4 replies; 27+ messages in thread
From: Marcello Sylvester Bauer @ 2020-08-18 8:24 UTC (permalink / raw)
To: devel
Support arbitrary platforms with different or even no MMCONF space.
Fixes crash on platforms not exposing 256 buses.
Tested on:
* AMD Stoney Ridge
Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg-MMCONF
PR: https://github.com/tianocore/edk2/pull/885
v5:
* MdePkg
- support variable size MMCONF in all PciExpressLibs
- use (UINTX)-1 as return values for invalid Pci addresses
v4:
* MdePkg: undo default PcdPciExpressBaseSize off by one
v3:
* split patch 2 by package
* MdePkg/PciExpress:
- PciExpressXX add return value specification
- Undo remove of ASSERT()
- PcdPciExpressBaseSize() correct function header
- correct return value types
v2:
* rebased with regards to commit 3900a63e3a1b9ba9a4105bedead7b986188cec2c
* add MdePkg Maintainer
Marcello Sylvester Bauer (2):
MdePkg: PciExpressLib support variable size MMCONF
UefiPayloadPkg: Support variable size MMCONF space
Patrick Rudolph (1):
UefiPayloadPkg: Store the size of the MMCONF window
MdePkg/MdePkg.dec | 4 +
UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 +
MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf | 6 +-
MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf | 1 +
MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf | 1 +
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 1 +
MdePkg/Include/Library/PciExpressLib.h | 5 +-
UefiPayloadPkg/Include/Guid/AcpiBoardInfoGuid.h | 1 +
MdePkg/Library/BasePciExpressLib/PciExpressLib.c | 216 ++++++++++++++---
MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c | 247 ++++++++++++++++----
MdePkg/Library/SmmPciExpressLib/PciExpressLib.c | 218 ++++++++++++++---
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 +-
UefiPayloadPkg/BlSupportPei/BlSupportPei.c | 3 +
13 files changed, 593 insertions(+), 115 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v5 1/3] UefiPayloadPkg: Store the size of the MMCONF window 2020-08-18 8:24 [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF Marcello Sylvester Bauer @ 2020-08-18 8:24 ` Marcello Sylvester Bauer 2020-08-18 8:24 ` [PATCH v5 2/3] MdePkg: PciExpressLib support variable size MMCONF Marcello Sylvester Bauer ` (2 subsequent siblings) 3 siblings, 0 replies; 27+ messages in thread From: Marcello Sylvester Bauer @ 2020-08-18 8:24 UTC (permalink / raw) To: devel; +Cc: Patrick Rudolph, Christian Walter, Maurice Ma, Guo Dong, Benjamin You From: Patrick Rudolph <patrick.rudolph@9elements.com> Store the real size of the Pcie Memory Mapped Address Space. This change is necessary to support variable size of MMCONF spaces. Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> Cc: Patrick Rudolph <patrick.rudolph@9elements.com> Cc: Christian Walter <christian.walter@9elements.com> Cc: Maurice Ma <maurice.ma@intel.com> Cc: Guo Dong <guo.dong@intel.com> Cc: Benjamin You <benjamin.you@intel.com> --- UefiPayloadPkg/Include/Guid/AcpiBoardInfoGuid.h | 1 + UefiPayloadPkg/BlSupportPei/BlSupportPei.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/UefiPayloadPkg/Include/Guid/AcpiBoardInfoGuid.h b/UefiPayloadPkg/Include/Guid/AcpiBoardInfoGuid.h index fe783fe5e14c..043b748ae4a9 100644 --- a/UefiPayloadPkg/Include/Guid/AcpiBoardInfoGuid.h +++ b/UefiPayloadPkg/Include/Guid/AcpiBoardInfoGuid.h @@ -24,6 +24,7 @@ typedef struct { UINT64 PmTimerRegBase; UINT64 ResetRegAddress; UINT64 PcieBaseAddress; + UINT64 PcieBaseSize; } ACPI_BOARD_INFO; #endif diff --git a/UefiPayloadPkg/BlSupportPei/BlSupportPei.c b/UefiPayloadPkg/BlSupportPei/BlSupportPei.c index 22972453117a..a7e99f9ec6de 100644 --- a/UefiPayloadPkg/BlSupportPei/BlSupportPei.c +++ b/UefiPayloadPkg/BlSupportPei/BlSupportPei.c @@ -240,8 +240,10 @@ Done: if (MmCfgHdr != NULL) { MmCfgBase = (EFI_ACPI_MEMORY_MAPPED_ENHANCED_CONFIGURATION_SPACE_BASE_ADDRESS_ALLOCATION_STRUCTURE *)((UINT8*) MmCfgHdr + sizeof (*MmCfgHdr)); AcpiBoardInfo->PcieBaseAddress = MmCfgBase->BaseAddress; + AcpiBoardInfo->PcieBaseSize = (MmCfgBase->EndBusNumber + 1 - MmCfgBase->StartBusNumber) * 4096 * 32 * 8; } else { AcpiBoardInfo->PcieBaseAddress = 0; + AcpiBoardInfo->PcieBaseSize = 0; } DEBUG ((DEBUG_INFO, "PmCtrl Reg 0x%lx\n", AcpiBoardInfo->PmCtrlRegBase)); DEBUG ((DEBUG_INFO, "PmTimer Reg 0x%lx\n", AcpiBoardInfo->PmTimerRegBase)); @@ -250,6 +252,7 @@ Done: DEBUG ((DEBUG_INFO, "PmEvt Reg 0x%lx\n", AcpiBoardInfo->PmEvtBase)); DEBUG ((DEBUG_INFO, "PmGpeEn Reg 0x%lx\n", AcpiBoardInfo->PmGpeEnBase)); DEBUG ((DEBUG_INFO, "PcieBaseAddr 0x%lx\n", AcpiBoardInfo->PcieBaseAddress)); + DEBUG ((DEBUG_INFO, "PcieBaseSize 0x%lx\n", AcpiBoardInfo->PcieBaseSize)); // // Verify values for proper operation -- 2.28.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v5 2/3] MdePkg: PciExpressLib support variable size MMCONF 2020-08-18 8:24 [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF Marcello Sylvester Bauer 2020-08-18 8:24 ` [PATCH v5 1/3] UefiPayloadPkg: Store the size of the MMCONF window Marcello Sylvester Bauer @ 2020-08-18 8:24 ` Marcello Sylvester Bauer 2020-08-20 10:55 ` Liming Gao 2020-08-18 8:24 ` [PATCH v5 3/3] UefiPayloadPkg: Support variable size MMCONF space Marcello Sylvester Bauer 2020-09-16 8:56 ` more development process failure [was: UefiPayloadPkg: Runtime MMCONF] Laszlo Ersek 3 siblings, 1 reply; 27+ messages in thread From: Marcello Sylvester Bauer @ 2020-08-18 8:24 UTC (permalink / raw) To: devel; +Cc: Patrick Rudolph, Christian Walter, Michael D Kinney, Liming Gao Add support for arbitrary sized MMCONF by introducing a new PCD. Add a return value to point out invalid PCI addresses. Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> Cc: Patrick Rudolph <patrick.rudolph@9elements.com> Cc: Christian Walter <christian.walter@9elements.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <liming.gao@intel.com> MdePkg: Support variable size MMCONF * ASSERT if pci address is out of bound * Add return value for invalid address Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> MdePkg: remove out of bound assertion Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> --- MdePkg/MdePkg.dec | 4 + MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf | 6 +- MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf | 1 + MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf | 1 + MdePkg/Include/Library/PciExpressLib.h | 5 +- MdePkg/Library/BasePciExpressLib/PciExpressLib.c | 216 ++++++++++++++--- MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c | 247 ++++++++++++++++---- MdePkg/Library/SmmPciExpressLib/PciExpressLib.c | 218 ++++++++++++++--- 8 files changed, 584 insertions(+), 114 deletions(-) diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index 73f6c2407357..812be75fb3b2 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|0x10000000|UINT64|0x0000000f + ## Default current ISO 639-2 language: English & French. # @Prompt Default Value of LangCodes Variable. gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes|"engfraengfra"|VOID*|0x0000001c 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 ## CONSUMES diff --git a/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf b/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf index 8d2ba1d18735..26a59bda1948 100644 --- a/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf +++ b/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf @@ -47,3 +47,4 @@ [LibraryClasses] [Pcd] gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress ## CONSUMES + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize ## CONSUMES diff --git a/MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf b/MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf index 729f6a3083ba..78cab6352fac 100644 --- a/MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf +++ b/MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf @@ -35,3 +35,4 @@ [LibraryClasses] [Pcd] gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress ## CONSUMES + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize ## CONSUMES diff --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-Patent diff --git a/MdePkg/Library/BasePciExpressLib/PciExpressLib.c b/MdePkg/Library/BasePciExpressLib/PciExpressLib.c index 99a166c3609b..910dd75bb48c 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. @@ -79,6 +80,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 size of PCI Express. + +**/ +STATIC +UINTN +PcdPciExpressBaseSize ( + VOID + ) +{ + return (UINTN) PcdGet64 (PcdPciExpressBaseSize); +} + /** Reads an 8-bit PCI configuration register. @@ -91,7 +110,8 @@ GetPciExpressBaseAddress ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT8 @@ -101,6 +121,9 @@ PciExpressRead8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioRead8 ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -117,7 +140,8 @@ PciExpressRead8 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT8 @@ -128,6 +152,9 @@ PciExpressWrite8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioWrite8 ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -148,7 +175,8 @@ PciExpressWrite8 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT8 @@ -159,6 +187,9 @@ PciExpressOr8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioOr8 ((UINTN) GetPciExpressBaseAddress () + Address, OrData); } @@ -179,7 +210,8 @@ PciExpressOr8 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -190,6 +222,9 @@ PciExpressAnd8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioAnd8 ((UINTN) GetPciExpressBaseAddress () + Address, AndData); } @@ -212,7 +247,8 @@ PciExpressAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -224,6 +260,9 @@ PciExpressAndThenOr8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioAndThenOr8 ( (UINTN) GetPciExpressBaseAddress () + Address, AndData, @@ -249,7 +288,9 @@ PciExpressAndThenOr8 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..7. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration + register. **/ UINT8 @@ -261,6 +302,9 @@ PciExpressBitFieldRead8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldRead8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -289,7 +333,8 @@ PciExpressBitFieldRead8 ( Range 0..7. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -302,6 +347,9 @@ PciExpressBitFieldWrite8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldWrite8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -334,7 +382,8 @@ PciExpressBitFieldWrite8 ( Range 0..7. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -347,6 +396,9 @@ PciExpressBitFieldOr8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldOr8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -379,7 +431,8 @@ PciExpressBitFieldOr8 ( Range 0..7. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -392,6 +445,9 @@ PciExpressBitFieldAnd8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldAnd8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -428,7 +484,8 @@ PciExpressBitFieldAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -442,6 +499,9 @@ PciExpressBitFieldAndThenOr8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldAndThenOr8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -464,7 +524,8 @@ PciExpressBitFieldAndThenOr8 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT16 @@ -474,6 +535,9 @@ PciExpressRead16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioRead16 ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -491,7 +555,8 @@ PciExpressRead16 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT16 @@ -502,6 +567,9 @@ PciExpressWrite16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioWrite16 ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -523,7 +591,8 @@ PciExpressWrite16 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -534,6 +603,9 @@ PciExpressOr16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioOr16 ((UINTN) GetPciExpressBaseAddress () + Address, OrData); } @@ -555,7 +627,8 @@ PciExpressOr16 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -566,6 +639,9 @@ PciExpressAnd16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioAnd16 ((UINTN) GetPciExpressBaseAddress () + Address, AndData); } @@ -589,7 +665,8 @@ PciExpressAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -601,6 +678,9 @@ PciExpressAndThenOr16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioAndThenOr16 ( (UINTN) GetPciExpressBaseAddress () + Address, AndData, @@ -627,7 +707,9 @@ PciExpressAndThenOr16 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..15. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration + register. **/ UINT16 @@ -639,6 +721,9 @@ PciExpressBitFieldRead16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldRead16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -668,7 +753,8 @@ PciExpressBitFieldRead16 ( Range 0..15. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -681,6 +767,9 @@ PciExpressBitFieldWrite16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldWrite16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -714,7 +803,8 @@ PciExpressBitFieldWrite16 ( Range 0..15. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -727,6 +817,9 @@ PciExpressBitFieldOr16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldOr16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -760,7 +853,8 @@ PciExpressBitFieldOr16 ( Range 0..15. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -773,6 +867,9 @@ PciExpressBitFieldAnd16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldAnd16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -810,7 +907,8 @@ PciExpressBitFieldAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -824,6 +922,9 @@ PciExpressBitFieldAndThenOr16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldAndThenOr16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -846,7 +947,8 @@ PciExpressBitFieldAndThenOr16 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT32 @@ -856,6 +958,9 @@ PciExpressRead32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioRead32 ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -873,7 +978,8 @@ PciExpressRead32 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT32 @@ -884,6 +990,9 @@ PciExpressWrite32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioWrite32 ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -905,7 +1014,8 @@ PciExpressWrite32 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -916,6 +1026,9 @@ PciExpressOr32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioOr32 ((UINTN) GetPciExpressBaseAddress () + Address, OrData); } @@ -937,7 +1050,8 @@ PciExpressOr32 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -948,6 +1062,9 @@ PciExpressAnd32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioAnd32 ((UINTN) GetPciExpressBaseAddress () + Address, AndData); } @@ -971,7 +1088,8 @@ PciExpressAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -983,6 +1101,9 @@ PciExpressAndThenOr32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioAndThenOr32 ( (UINTN) GetPciExpressBaseAddress () + Address, AndData, @@ -1009,7 +1130,9 @@ PciExpressAndThenOr32 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..31. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI + configuration register. **/ UINT32 @@ -1021,6 +1144,9 @@ PciExpressBitFieldRead32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldRead32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1050,7 +1176,8 @@ PciExpressBitFieldRead32 ( Range 0..31. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1063,6 +1190,9 @@ PciExpressBitFieldWrite32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldWrite32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1096,7 +1226,8 @@ PciExpressBitFieldWrite32 ( Range 0..31. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1109,6 +1240,9 @@ PciExpressBitFieldOr32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldOr32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1142,7 +1276,8 @@ PciExpressBitFieldOr32 ( Range 0..31. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1155,6 +1290,9 @@ PciExpressBitFieldAnd32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldAnd32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1192,7 +1330,8 @@ PciExpressBitFieldAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1206,6 +1345,9 @@ PciExpressBitFieldAndThenOr32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldAndThenOr32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1235,7 +1377,8 @@ PciExpressBitFieldAndThenOr32 ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer receiving the data read. - @return Size read data from StartAddress. + @retval (UINTN)-1 Invalid PCI address. + @retval other Size read data from StartAddress. **/ UINTN @@ -1249,6 +1392,9 @@ PciExpressReadBuffer ( UINTN ReturnValue; ASSERT_INVALID_PCI_ADDRESS (StartAddress); + if (StartAddress >= PcdPciExpressBaseSize()) { + return (UINTN) -1; + } ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); if (Size == 0) { @@ -1335,7 +1481,8 @@ PciExpressReadBuffer ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer containing the data to write. - @return Size written to StartAddress. + @retval (UINTN)-1 Invalid PCI address. + @retval other Size written to StartAddress. **/ UINTN @@ -1349,6 +1496,9 @@ PciExpressWriteBuffer ( UINTN ReturnValue; ASSERT_INVALID_PCI_ADDRESS (StartAddress); + if (StartAddress >= PcdPciExpressBaseSize()) { + return (UINTN) -1; + } ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); if (Size == 0) { diff --git a/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c b/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c index b8995435109f..cb80725c5fa6 100644 --- a/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c +++ b/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c @@ -25,6 +25,16 @@ #include <Library/DxeServicesTableLib.h> #include <Library/UefiRuntimeLib.h> +/** + Assert the validity of a PCI address. A valid PCI address should contain 1's + only in the low 28 bits. + + @param A The address to validate. + +**/ +#define ASSERT_INVALID_PCI_ADDRESS(A) \ + ASSERT (((A) & ~0xfffffff) == 0) + /// /// Define table for mapping PCI Express MMIO physical addresses to virtual addresses at OS runtime /// @@ -39,9 +49,10 @@ typedef struct { EFI_EVENT mDxeRuntimePciExpressLibVirtualNotifyEvent = NULL; /// -/// Module global that contains the base physical address of the PCI Express MMIO range. +/// Module global that contains the base physical address and size of the PCI Express MMIO range. /// UINTN mDxeRuntimePciExpressLibPciExpressBaseAddress = 0; +UINTN mDxeRuntimePciExpressLibPciExpressBaseSize = 0; /// /// The number of PCI devices that have been registered for runtime access. @@ -120,6 +131,7 @@ DxeRuntimePciExpressLibConstructor ( // Cache the physical address of the PCI Express MMIO range into a module global variable // mDxeRuntimePciExpressLibPciExpressBaseAddress = (UINTN) PcdGet64 (PcdPciExpressBaseAddress); + mDxeRuntimePciExpressLibPciExpressBaseSize = (UINTN) PcdGet64 (PcdPciExpressBaseSize); // // Register SetVirtualAddressMap () notify function @@ -179,8 +191,12 @@ DxeRuntimePciExpressLibDestructor ( This internal functions retrieves PCI Express Base Address via a PCD entry PcdPciExpressBaseAddress. - @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The base address of PCI Express. + If Address > 0x0FFFFFFF, then ASSERT(). + + @param Address The address that encodes the PCI Bus, Device, Function and Register. + + @retval (UINTN)-1 Invalid PCI address. + @retval other The base address of PCI Express. **/ UINTN @@ -193,7 +209,14 @@ GetPciExpressAddress ( // // Make sure Address is valid // - ASSERT (((Address) & ~0xfffffff) == 0); + ASSERT_INVALID_PCI_ADDRESS (Address); + + // + // Make sure the Address is in MMCONF address space + // + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } // // Convert Address to a physical address in the MMIO PCI Express range @@ -236,7 +259,6 @@ GetPciExpressAddress ( // // No match was found. This is a critical error at OS runtime, so ASSERT() and force a breakpoint. // - ASSERT (FALSE); CpuBreakpoint(); // @@ -288,7 +310,14 @@ PciExpressRegisterForRuntimeAccess ( // // Make sure Address is valid // - ASSERT (((Address) & ~0xfffffff) == 0); + ASSERT_INVALID_PCI_ADDRESS (Address); + + // + // Make sure the Address is in MMCONF address space + // + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return RETURN_UNSUPPORTED; + } // // Convert Address to a physical address in the MMIO PCI Express range @@ -354,8 +383,8 @@ PciExpressRegisterForRuntimeAccess ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT8 @@ -364,6 +393,10 @@ PciExpressRead8 ( IN UINTN Address ) { + ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioRead8 (GetPciExpressAddress (Address)); } @@ -380,7 +413,8 @@ PciExpressRead8 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT8 @@ -390,6 +424,9 @@ PciExpressWrite8 ( IN UINT8 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioWrite8 (GetPciExpressAddress (Address), Value); } @@ -410,7 +447,8 @@ PciExpressWrite8 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -420,6 +458,9 @@ PciExpressOr8 ( IN UINT8 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioOr8 (GetPciExpressAddress (Address), OrData); } @@ -440,7 +481,8 @@ PciExpressOr8 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -450,6 +492,9 @@ PciExpressAnd8 ( IN UINT8 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioAnd8 (GetPciExpressAddress (Address), AndData); } @@ -472,7 +517,8 @@ PciExpressAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -483,6 +529,9 @@ PciExpressAndThenOr8 ( IN UINT8 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioAndThenOr8 ( GetPciExpressAddress (Address), AndData, @@ -508,7 +557,8 @@ PciExpressAndThenOr8 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..7. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT8 @@ -519,6 +569,9 @@ PciExpressBitFieldRead8 ( IN UINTN EndBit ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldRead8 ( GetPciExpressAddress (Address), StartBit, @@ -547,7 +600,8 @@ PciExpressBitFieldRead8 ( Range 0..7. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -559,6 +613,9 @@ PciExpressBitFieldWrite8 ( IN UINT8 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldWrite8 ( GetPciExpressAddress (Address), StartBit, @@ -591,7 +648,8 @@ PciExpressBitFieldWrite8 ( Range 0..7. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -603,6 +661,9 @@ PciExpressBitFieldOr8 ( IN UINT8 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldOr8 ( GetPciExpressAddress (Address), StartBit, @@ -635,7 +696,8 @@ PciExpressBitFieldOr8 ( Range 0..7. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -647,6 +709,9 @@ PciExpressBitFieldAnd8 ( IN UINT8 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldAnd8 ( GetPciExpressAddress (Address), StartBit, @@ -683,7 +748,8 @@ PciExpressBitFieldAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -696,6 +762,9 @@ PciExpressBitFieldAndThenOr8 ( IN UINT8 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldAndThenOr8 ( GetPciExpressAddress (Address), StartBit, @@ -718,7 +787,8 @@ PciExpressBitFieldAndThenOr8 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT16 @@ -727,6 +797,9 @@ PciExpressRead16 ( IN UINTN Address ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioRead16 (GetPciExpressAddress (Address)); } @@ -744,7 +817,8 @@ PciExpressRead16 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT16 @@ -754,6 +828,9 @@ PciExpressWrite16 ( IN UINT16 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioWrite16 (GetPciExpressAddress (Address), Value); } @@ -775,7 +852,8 @@ PciExpressWrite16 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -785,6 +863,9 @@ PciExpressOr16 ( IN UINT16 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioOr16 (GetPciExpressAddress (Address), OrData); } @@ -806,7 +887,8 @@ PciExpressOr16 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -816,6 +898,9 @@ PciExpressAnd16 ( IN UINT16 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioAnd16 (GetPciExpressAddress (Address), AndData); } @@ -839,7 +924,8 @@ PciExpressAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -850,6 +936,9 @@ PciExpressAndThenOr16 ( IN UINT16 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioAndThenOr16 ( GetPciExpressAddress (Address), AndData, @@ -876,7 +965,8 @@ PciExpressAndThenOr16 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..15. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT16 @@ -887,6 +977,9 @@ PciExpressBitFieldRead16 ( IN UINTN EndBit ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldRead16 ( GetPciExpressAddress (Address), StartBit, @@ -916,7 +1009,8 @@ PciExpressBitFieldRead16 ( Range 0..15. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -928,6 +1022,9 @@ PciExpressBitFieldWrite16 ( IN UINT16 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldWrite16 ( GetPciExpressAddress (Address), StartBit, @@ -961,7 +1058,8 @@ PciExpressBitFieldWrite16 ( Range 0..15. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -973,6 +1071,9 @@ PciExpressBitFieldOr16 ( IN UINT16 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldOr16 ( GetPciExpressAddress (Address), StartBit, @@ -1006,7 +1107,8 @@ PciExpressBitFieldOr16 ( Range 0..15. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -1018,6 +1120,9 @@ PciExpressBitFieldAnd16 ( IN UINT16 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldAnd16 ( GetPciExpressAddress (Address), StartBit, @@ -1055,7 +1160,8 @@ PciExpressBitFieldAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -1068,6 +1174,9 @@ PciExpressBitFieldAndThenOr16 ( IN UINT16 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldAndThenOr16 ( GetPciExpressAddress (Address), StartBit, @@ -1090,7 +1199,8 @@ PciExpressBitFieldAndThenOr16 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT32 @@ -1099,6 +1209,9 @@ PciExpressRead32 ( IN UINTN Address ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioRead32 (GetPciExpressAddress (Address)); } @@ -1116,7 +1229,8 @@ PciExpressRead32 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT32 @@ -1126,6 +1240,9 @@ PciExpressWrite32 ( IN UINT32 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioWrite32 (GetPciExpressAddress (Address), Value); } @@ -1147,7 +1264,8 @@ PciExpressWrite32 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1157,6 +1275,9 @@ PciExpressOr32 ( IN UINT32 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioOr32 (GetPciExpressAddress (Address), OrData); } @@ -1178,7 +1299,8 @@ PciExpressOr32 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1188,6 +1310,9 @@ PciExpressAnd32 ( IN UINT32 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioAnd32 (GetPciExpressAddress (Address), AndData); } @@ -1211,7 +1336,8 @@ PciExpressAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1222,6 +1348,9 @@ PciExpressAndThenOr32 ( IN UINT32 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioAndThenOr32 ( GetPciExpressAddress (Address), AndData, @@ -1248,7 +1377,8 @@ PciExpressAndThenOr32 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..31. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT32 @@ -1259,6 +1389,9 @@ PciExpressBitFieldRead32 ( IN UINTN EndBit ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldRead32 ( GetPciExpressAddress (Address), StartBit, @@ -1288,7 +1421,8 @@ PciExpressBitFieldRead32 ( Range 0..31. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1300,6 +1434,9 @@ PciExpressBitFieldWrite32 ( IN UINT32 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldWrite32 ( GetPciExpressAddress (Address), StartBit, @@ -1333,7 +1470,8 @@ PciExpressBitFieldWrite32 ( Range 0..31. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1345,6 +1483,9 @@ PciExpressBitFieldOr32 ( IN UINT32 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldOr32 ( GetPciExpressAddress (Address), StartBit, @@ -1378,7 +1519,8 @@ PciExpressBitFieldOr32 ( Range 0..31. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1390,6 +1532,9 @@ PciExpressBitFieldAnd32 ( IN UINT32 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldAnd32 ( GetPciExpressAddress (Address), StartBit, @@ -1427,7 +1572,8 @@ PciExpressBitFieldAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1440,6 +1586,9 @@ PciExpressBitFieldAndThenOr32 ( IN UINT32 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldAndThenOr32 ( GetPciExpressAddress (Address), StartBit, @@ -1469,7 +1618,8 @@ PciExpressBitFieldAndThenOr32 ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer receiving the data read. - @return Size read data from StartAddress. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other Size read data from StartAddress. **/ UINTN @@ -1485,9 +1635,16 @@ PciExpressReadBuffer ( // // Make sure Address is valid // - ASSERT (((StartAddress) & ~0xfffffff) == 0); + ASSERT_INVALID_PCI_ADDRESS (StartAddress); ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); + // + // Make sure the Address is in MMCONF address space + // + if (StartAddress >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } + if (Size == 0) { return Size; } @@ -1572,7 +1729,8 @@ PciExpressReadBuffer ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer containing the data to write. - @return Size written to StartAddress. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other Size written to StartAddress. **/ UINTN @@ -1588,9 +1746,16 @@ PciExpressWriteBuffer ( // // Make sure Address is valid // - ASSERT (((StartAddress) & ~0xfffffff) == 0); + ASSERT_INVALID_PCI_ADDRESS (StartAddress); ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); + // + // Make sure the Address is in MMCONF address space + // + if (StartAddress >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } + if (Size == 0) { return 0; } diff --git a/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c b/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c index 35b9f775a80b..97bd32c8d201 100644 --- a/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c +++ b/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c @@ -20,9 +20,10 @@ #include <Library/PcdLib.h> /// -/// Module global that contains the base physical address of the PCI Express MMIO range. +/// Module global that contains the base physical address and size of the PCI Express MMIO range. /// UINTN mSmmPciExpressLibPciExpressBaseAddress = 0; +UINTN mSmmPciExpressLibPciExpressBaseSize = 0; /** The constructor function caches the PCI Express Base Address @@ -40,9 +41,10 @@ SmmPciExpressLibConstructor ( ) { // - // Cache the physical address of the PCI Express MMIO range into a module global variable + // Cache the physical address and size of the PCI Express MMIO range into a module global variable // mSmmPciExpressLibPciExpressBaseAddress = (UINTN) PcdGet64 (PcdPciExpressBaseAddress); + mSmmPciExpressLibPciExpressBaseSize = (UINTN) PcdGet64 (PcdPciExpressBaseSize); return EFI_SUCCESS; } @@ -97,8 +99,12 @@ PciExpressRegisterForRuntimeAccess ( mSmmPciExpressLibPciExpressBaseAddress is initialized in the library constructor from PCD entry PcdPciExpressBaseAddress. + If Address > 0x0FFFFFFF, then ASSERT(). + @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return MMIO address corresponding to Address. + + @retval (UINTN)-1 Invalid PCI address. + @retval other MMIO address corresponding to Address. **/ UINTN @@ -110,6 +116,12 @@ GetPciExpressAddress ( // Make sure Address is valid // ASSERT_INVALID_PCI_ADDRESS (Address); + // + // Make sure the Address is in MMCONF address space + // + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } return mSmmPciExpressLibPciExpressBaseAddress + Address; } @@ -125,7 +137,8 @@ GetPciExpressAddress ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT8 @@ -134,6 +147,9 @@ PciExpressRead8 ( IN UINTN Address ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioRead8 (GetPciExpressAddress (Address)); } @@ -150,7 +166,8 @@ PciExpressRead8 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT8 @@ -160,6 +177,9 @@ PciExpressWrite8 ( IN UINT8 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioWrite8 (GetPciExpressAddress (Address), Value); } @@ -180,7 +200,8 @@ PciExpressWrite8 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -190,6 +211,9 @@ PciExpressOr8 ( IN UINT8 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioOr8 (GetPciExpressAddress (Address), OrData); } @@ -210,7 +234,8 @@ PciExpressOr8 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -220,6 +245,9 @@ PciExpressAnd8 ( IN UINT8 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioAnd8 (GetPciExpressAddress (Address), AndData); } @@ -242,7 +270,8 @@ PciExpressAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -253,6 +282,9 @@ PciExpressAndThenOr8 ( IN UINT8 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioAndThenOr8 ( GetPciExpressAddress (Address), AndData, @@ -278,7 +310,8 @@ PciExpressAndThenOr8 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..7. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT8 @@ -289,6 +322,9 @@ PciExpressBitFieldRead8 ( IN UINTN EndBit ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldRead8 ( GetPciExpressAddress (Address), StartBit, @@ -317,7 +353,8 @@ PciExpressBitFieldRead8 ( Range 0..7. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -329,6 +366,9 @@ PciExpressBitFieldWrite8 ( IN UINT8 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldWrite8 ( GetPciExpressAddress (Address), StartBit, @@ -361,7 +401,8 @@ PciExpressBitFieldWrite8 ( Range 0..7. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -373,6 +414,9 @@ PciExpressBitFieldOr8 ( IN UINT8 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldOr8 ( GetPciExpressAddress (Address), StartBit, @@ -405,7 +449,8 @@ PciExpressBitFieldOr8 ( Range 0..7. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -417,6 +462,9 @@ PciExpressBitFieldAnd8 ( IN UINT8 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldAnd8 ( GetPciExpressAddress (Address), StartBit, @@ -453,7 +501,8 @@ PciExpressBitFieldAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -466,6 +515,9 @@ PciExpressBitFieldAndThenOr8 ( IN UINT8 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldAndThenOr8 ( GetPciExpressAddress (Address), StartBit, @@ -488,7 +540,8 @@ PciExpressBitFieldAndThenOr8 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT16 @@ -497,6 +550,9 @@ PciExpressRead16 ( IN UINTN Address ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioRead16 (GetPciExpressAddress (Address)); } @@ -514,7 +570,8 @@ PciExpressRead16 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT16 @@ -524,6 +581,9 @@ PciExpressWrite16 ( IN UINT16 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioWrite16 (GetPciExpressAddress (Address), Value); } @@ -545,7 +605,8 @@ PciExpressWrite16 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -555,6 +616,9 @@ PciExpressOr16 ( IN UINT16 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioOr16 (GetPciExpressAddress (Address), OrData); } @@ -576,7 +640,8 @@ PciExpressOr16 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -586,6 +651,9 @@ PciExpressAnd16 ( IN UINT16 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioAnd16 (GetPciExpressAddress (Address), AndData); } @@ -609,7 +677,8 @@ PciExpressAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -620,6 +689,9 @@ PciExpressAndThenOr16 ( IN UINT16 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioAndThenOr16 ( GetPciExpressAddress (Address), AndData, @@ -646,7 +718,8 @@ PciExpressAndThenOr16 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..15. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT16 @@ -657,6 +730,9 @@ PciExpressBitFieldRead16 ( IN UINTN EndBit ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldRead16 ( GetPciExpressAddress (Address), StartBit, @@ -686,7 +762,8 @@ PciExpressBitFieldRead16 ( Range 0..15. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -698,6 +775,9 @@ PciExpressBitFieldWrite16 ( IN UINT16 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldWrite16 ( GetPciExpressAddress (Address), StartBit, @@ -731,7 +811,8 @@ PciExpressBitFieldWrite16 ( Range 0..15. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -743,6 +824,9 @@ PciExpressBitFieldOr16 ( IN UINT16 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldOr16 ( GetPciExpressAddress (Address), StartBit, @@ -776,7 +860,8 @@ PciExpressBitFieldOr16 ( Range 0..15. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -788,6 +873,9 @@ PciExpressBitFieldAnd16 ( IN UINT16 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldAnd16 ( GetPciExpressAddress (Address), StartBit, @@ -825,7 +913,8 @@ PciExpressBitFieldAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -838,6 +927,9 @@ PciExpressBitFieldAndThenOr16 ( IN UINT16 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldAndThenOr16 ( GetPciExpressAddress (Address), StartBit, @@ -860,7 +952,8 @@ PciExpressBitFieldAndThenOr16 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT32 @@ -869,6 +962,9 @@ PciExpressRead32 ( IN UINTN Address ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioRead32 (GetPciExpressAddress (Address)); } @@ -886,7 +982,8 @@ PciExpressRead32 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT32 @@ -896,6 +993,9 @@ PciExpressWrite32 ( IN UINT32 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioWrite32 (GetPciExpressAddress (Address), Value); } @@ -917,7 +1017,8 @@ PciExpressWrite32 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -927,6 +1028,9 @@ PciExpressOr32 ( IN UINT32 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioOr32 (GetPciExpressAddress (Address), OrData); } @@ -948,7 +1052,8 @@ PciExpressOr32 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -958,6 +1063,9 @@ PciExpressAnd32 ( IN UINT32 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioAnd32 (GetPciExpressAddress (Address), AndData); } @@ -981,7 +1089,8 @@ PciExpressAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -992,6 +1101,9 @@ PciExpressAndThenOr32 ( IN UINT32 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioAndThenOr32 ( GetPciExpressAddress (Address), AndData, @@ -1018,7 +1130,8 @@ PciExpressAndThenOr32 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..31. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT32 @@ -1029,6 +1142,9 @@ PciExpressBitFieldRead32 ( IN UINTN EndBit ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldRead32 ( GetPciExpressAddress (Address), StartBit, @@ -1058,7 +1174,8 @@ PciExpressBitFieldRead32 ( Range 0..31. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1070,6 +1187,9 @@ PciExpressBitFieldWrite32 ( IN UINT32 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldWrite32 ( GetPciExpressAddress (Address), StartBit, @@ -1103,7 +1223,8 @@ PciExpressBitFieldWrite32 ( Range 0..31. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1115,6 +1236,9 @@ PciExpressBitFieldOr32 ( IN UINT32 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldOr32 ( GetPciExpressAddress (Address), StartBit, @@ -1148,7 +1272,8 @@ PciExpressBitFieldOr32 ( Range 0..31. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1160,6 +1285,9 @@ PciExpressBitFieldAnd32 ( IN UINT32 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldAnd32 ( GetPciExpressAddress (Address), StartBit, @@ -1197,7 +1325,8 @@ PciExpressBitFieldAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1210,6 +1339,9 @@ PciExpressBitFieldAndThenOr32 ( IN UINT32 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldAndThenOr32 ( GetPciExpressAddress (Address), StartBit, @@ -1239,7 +1371,8 @@ PciExpressBitFieldAndThenOr32 ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer receiving the data read. - @return Size read data from StartAddress. + @retval (UINTN)-1 Invalid PCI address. + @retval other Size read data from StartAddress. **/ UINTN @@ -1258,6 +1391,13 @@ PciExpressReadBuffer ( ASSERT_INVALID_PCI_ADDRESS (StartAddress); ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); + // + // Make sure the Address is in MMCONF address space + // + if (StartAddress >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } + if (Size == 0) { return Size; } @@ -1342,7 +1482,8 @@ PciExpressReadBuffer ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer containing the data to write. - @return Size written to StartAddress. + @retval (UINTN)-1 Invalid PCI address. + @retval other Size written to StartAddress. **/ UINTN @@ -1361,6 +1502,13 @@ PciExpressWriteBuffer ( ASSERT_INVALID_PCI_ADDRESS (StartAddress); ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); + // + // Make sure the Address is in MMCONF address space + // + if (StartAddress >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } + if (Size == 0) { return 0; -- 2.28.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v5 2/3] MdePkg: PciExpressLib support variable size MMCONF 2020-08-18 8:24 ` [PATCH v5 2/3] MdePkg: PciExpressLib support variable size MMCONF Marcello Sylvester Bauer @ 2020-08-20 10:55 ` Liming Gao 0 siblings, 0 replies; 27+ messages in thread From: Liming Gao @ 2020-08-20 10:55 UTC (permalink / raw) To: Marcello Sylvester Bauer, devel@edk2.groups.io Cc: Patrick Rudolph, Christian Walter, Kinney, Michael D Thanks for your updates. This version is good to me. Reviewed-by: Liming Gao <liming.gao@intel.com> -----Original Message----- From: Marcello Sylvester Bauer <marcello.bauer@9elements.com> Sent: 2020年8月18日 16:24 To: devel@edk2.groups.io Cc: Patrick Rudolph <patrick.rudolph@9elements.com>; Christian Walter <christian.walter@9elements.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com> Subject: [PATCH v5 2/3] MdePkg: PciExpressLib support variable size MMCONF Add support for arbitrary sized MMCONF by introducing a new PCD. Add a return value to point out invalid PCI addresses. Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> Cc: Patrick Rudolph <patrick.rudolph@9elements.com> Cc: Christian Walter <christian.walter@9elements.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <liming.gao@intel.com> MdePkg: Support variable size MMCONF * ASSERT if pci address is out of bound * Add return value for invalid address Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> MdePkg: remove out of bound assertion Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> --- MdePkg/MdePkg.dec | 4 + MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf | 6 +- MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf | 1 + MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf | 1 + MdePkg/Include/Library/PciExpressLib.h | 5 +- MdePkg/Library/BasePciExpressLib/PciExpressLib.c | 216 ++++++++++++++--- MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c | 247 ++++++++++++++++---- MdePkg/Library/SmmPciExpressLib/PciExpressLib.c | 218 ++++++++++++++--- 8 files changed, 584 insertions(+), 114 deletions(-) diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index 73f6c2407357..812be75fb3b2 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|0x10000000|UINT64|0x0000000f + ## Default current ISO 639-2 language: English & French. # @Prompt Default Value of LangCodes Variable. gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes|"engfraengfra"|VOID*|0x0000001c 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 ## CONSUMES diff --git a/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf b/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf index 8d2ba1d18735..26a59bda1948 100644 --- a/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf +++ b/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf @@ -47,3 +47,4 @@ [LibraryClasses] [Pcd] gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress ## CONSUMES + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize ## CONSUMES diff --git a/MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf b/MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf index 729f6a3083ba..78cab6352fac 100644 --- a/MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf +++ b/MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf @@ -35,3 +35,4 @@ [LibraryClasses] [Pcd] gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress ## CONSUMES + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize ## CONSUMES diff --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-Patent diff --git a/MdePkg/Library/BasePciExpressLib/PciExpressLib.c b/MdePkg/Library/BasePciExpressLib/PciExpressLib.c index 99a166c3609b..910dd75bb48c 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. @@ -79,6 +80,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 size of PCI Express. + +**/ +STATIC +UINTN +PcdPciExpressBaseSize ( + VOID + ) +{ + return (UINTN) PcdGet64 (PcdPciExpressBaseSize); +} + /** Reads an 8-bit PCI configuration register. @@ -91,7 +110,8 @@ GetPciExpressBaseAddress ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT8 @@ -101,6 +121,9 @@ PciExpressRead8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioRead8 ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -117,7 +140,8 @@ PciExpressRead8 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT8 @@ -128,6 +152,9 @@ PciExpressWrite8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioWrite8 ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -148,7 +175,8 @@ PciExpressWrite8 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT8 @@ -159,6 +187,9 @@ PciExpressOr8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioOr8 ((UINTN) GetPciExpressBaseAddress () + Address, OrData); } @@ -179,7 +210,8 @@ PciExpressOr8 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -190,6 +222,9 @@ PciExpressAnd8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioAnd8 ((UINTN) GetPciExpressBaseAddress () + Address, AndData); } @@ -212,7 +247,8 @@ PciExpressAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -224,6 +260,9 @@ PciExpressAndThenOr8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioAndThenOr8 ( (UINTN) GetPciExpressBaseAddress () + Address, AndData, @@ -249,7 +288,9 @@ PciExpressAndThenOr8 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..7. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration + register. **/ UINT8 @@ -261,6 +302,9 @@ PciExpressBitFieldRead8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldRead8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -289,7 +333,8 @@ PciExpressBitFieldRead8 ( Range 0..7. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -302,6 +347,9 @@ PciExpressBitFieldWrite8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldWrite8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -334,7 +382,8 @@ PciExpressBitFieldWrite8 ( Range 0..7. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -347,6 +396,9 @@ PciExpressBitFieldOr8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldOr8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -379,7 +431,8 @@ PciExpressBitFieldOr8 ( Range 0..7. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -392,6 +445,9 @@ PciExpressBitFieldAnd8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldAnd8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -428,7 +484,8 @@ PciExpressBitFieldAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -442,6 +499,9 @@ PciExpressBitFieldAndThenOr8 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT8) -1; + } return MmioBitFieldAndThenOr8 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -464,7 +524,8 @@ PciExpressBitFieldAndThenOr8 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT16 @@ -474,6 +535,9 @@ PciExpressRead16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioRead16 ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -491,7 +555,8 @@ PciExpressRead16 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT16 @@ -502,6 +567,9 @@ PciExpressWrite16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioWrite16 ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -523,7 +591,8 @@ PciExpressWrite16 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -534,6 +603,9 @@ PciExpressOr16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioOr16 ((UINTN) GetPciExpressBaseAddress () + Address, OrData); } @@ -555,7 +627,8 @@ PciExpressOr16 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -566,6 +639,9 @@ PciExpressAnd16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioAnd16 ((UINTN) GetPciExpressBaseAddress () + Address, AndData); } @@ -589,7 +665,8 @@ PciExpressAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -601,6 +678,9 @@ PciExpressAndThenOr16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioAndThenOr16 ( (UINTN) GetPciExpressBaseAddress () + Address, AndData, @@ -627,7 +707,9 @@ PciExpressAndThenOr16 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..15. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration + register. **/ UINT16 @@ -639,6 +721,9 @@ PciExpressBitFieldRead16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldRead16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -668,7 +753,8 @@ PciExpressBitFieldRead16 ( Range 0..15. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -681,6 +767,9 @@ PciExpressBitFieldWrite16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldWrite16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -714,7 +803,8 @@ PciExpressBitFieldWrite16 ( Range 0..15. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -727,6 +817,9 @@ PciExpressBitFieldOr16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldOr16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -760,7 +853,8 @@ PciExpressBitFieldOr16 ( Range 0..15. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -773,6 +867,9 @@ PciExpressBitFieldAnd16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldAnd16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -810,7 +907,8 @@ PciExpressBitFieldAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -824,6 +922,9 @@ PciExpressBitFieldAndThenOr16 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT16) -1; + } return MmioBitFieldAndThenOr16 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -846,7 +947,8 @@ PciExpressBitFieldAndThenOr16 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT32 @@ -856,6 +958,9 @@ PciExpressRead32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioRead32 ((UINTN) GetPciExpressBaseAddress () + Address); } @@ -873,7 +978,8 @@ PciExpressRead32 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT32 @@ -884,6 +990,9 @@ PciExpressWrite32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioWrite32 ((UINTN) GetPciExpressBaseAddress () + Address, Value); } @@ -905,7 +1014,8 @@ PciExpressWrite32 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -916,6 +1026,9 @@ PciExpressOr32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioOr32 ((UINTN) GetPciExpressBaseAddress () + Address, OrData); } @@ -937,7 +1050,8 @@ PciExpressOr32 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -948,6 +1062,9 @@ PciExpressAnd32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioAnd32 ((UINTN) GetPciExpressBaseAddress () + Address, AndData); } @@ -971,7 +1088,8 @@ PciExpressAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -983,6 +1101,9 @@ PciExpressAndThenOr32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioAndThenOr32 ( (UINTN) GetPciExpressBaseAddress () + Address, AndData, @@ -1009,7 +1130,9 @@ PciExpressAndThenOr32 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..31. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI + configuration register. **/ UINT32 @@ -1021,6 +1144,9 @@ PciExpressBitFieldRead32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldRead32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1050,7 +1176,8 @@ PciExpressBitFieldRead32 ( Range 0..31. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1063,6 +1190,9 @@ PciExpressBitFieldWrite32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldWrite32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1096,7 +1226,8 @@ PciExpressBitFieldWrite32 ( Range 0..31. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1109,6 +1240,9 @@ PciExpressBitFieldOr32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldOr32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1142,7 +1276,8 @@ PciExpressBitFieldOr32 ( Range 0..31. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1155,6 +1290,9 @@ PciExpressBitFieldAnd32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldAnd32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1192,7 +1330,8 @@ PciExpressBitFieldAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1206,6 +1345,9 @@ PciExpressBitFieldAndThenOr32 ( ) { ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= PcdPciExpressBaseSize()) { + return (UINT32) -1; + } return MmioBitFieldAndThenOr32 ( (UINTN) GetPciExpressBaseAddress () + Address, StartBit, @@ -1235,7 +1377,8 @@ PciExpressBitFieldAndThenOr32 ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer receiving the data read. - @return Size read data from StartAddress. + @retval (UINTN)-1 Invalid PCI address. + @retval other Size read data from StartAddress. **/ UINTN @@ -1249,6 +1392,9 @@ PciExpressReadBuffer ( UINTN ReturnValue; ASSERT_INVALID_PCI_ADDRESS (StartAddress); + if (StartAddress >= PcdPciExpressBaseSize()) { + return (UINTN) -1; + } ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); if (Size == 0) { @@ -1335,7 +1481,8 @@ PciExpressReadBuffer ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer containing the data to write. - @return Size written to StartAddress. + @retval (UINTN)-1 Invalid PCI address. + @retval other Size written to StartAddress. **/ UINTN @@ -1349,6 +1496,9 @@ PciExpressWriteBuffer ( UINTN ReturnValue; ASSERT_INVALID_PCI_ADDRESS (StartAddress); + if (StartAddress >= PcdPciExpressBaseSize()) { + return (UINTN) -1; + } ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); if (Size == 0) { diff --git a/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c b/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c index b8995435109f..cb80725c5fa6 100644 --- a/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c +++ b/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c @@ -25,6 +25,16 @@ #include <Library/DxeServicesTableLib.h> #include <Library/UefiRuntimeLib.h> +/** + Assert the validity of a PCI address. A valid PCI address should contain 1's + only in the low 28 bits. + + @param A The address to validate. + +**/ +#define ASSERT_INVALID_PCI_ADDRESS(A) \ + ASSERT (((A) & ~0xfffffff) == 0) + /// /// Define table for mapping PCI Express MMIO physical addresses to virtual addresses at OS runtime /// @@ -39,9 +49,10 @@ typedef struct { EFI_EVENT mDxeRuntimePciExpressLibVirtualNotifyEvent = NULL; /// -/// Module global that contains the base physical address of the PCI Express MMIO range. +/// Module global that contains the base physical address and size of the PCI Express MMIO range. /// UINTN mDxeRuntimePciExpressLibPciExpressBaseAddress = 0; +UINTN mDxeRuntimePciExpressLibPciExpressBaseSize = 0; /// /// The number of PCI devices that have been registered for runtime access. @@ -120,6 +131,7 @@ DxeRuntimePciExpressLibConstructor ( // Cache the physical address of the PCI Express MMIO range into a module global variable // mDxeRuntimePciExpressLibPciExpressBaseAddress = (UINTN) PcdGet64 (PcdPciExpressBaseAddress); + mDxeRuntimePciExpressLibPciExpressBaseSize = (UINTN) PcdGet64 (PcdPciExpressBaseSize); // // Register SetVirtualAddressMap () notify function @@ -179,8 +191,12 @@ DxeRuntimePciExpressLibDestructor ( This internal functions retrieves PCI Express Base Address via a PCD entry PcdPciExpressBaseAddress. - @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The base address of PCI Express. + If Address > 0x0FFFFFFF, then ASSERT(). + + @param Address The address that encodes the PCI Bus, Device, Function and Register. + + @retval (UINTN)-1 Invalid PCI address. + @retval other The base address of PCI Express. **/ UINTN @@ -193,7 +209,14 @@ GetPciExpressAddress ( // // Make sure Address is valid // - ASSERT (((Address) & ~0xfffffff) == 0); + ASSERT_INVALID_PCI_ADDRESS (Address); + + // + // Make sure the Address is in MMCONF address space + // + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } // // Convert Address to a physical address in the MMIO PCI Express range @@ -236,7 +259,6 @@ GetPciExpressAddress ( // // No match was found. This is a critical error at OS runtime, so ASSERT() and force a breakpoint. // - ASSERT (FALSE); CpuBreakpoint(); // @@ -288,7 +310,14 @@ PciExpressRegisterForRuntimeAccess ( // // Make sure Address is valid // - ASSERT (((Address) & ~0xfffffff) == 0); + ASSERT_INVALID_PCI_ADDRESS (Address); + + // + // Make sure the Address is in MMCONF address space + // + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return RETURN_UNSUPPORTED; + } // // Convert Address to a physical address in the MMIO PCI Express range @@ -354,8 +383,8 @@ PciExpressRegisterForRuntimeAccess ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT8 @@ -364,6 +393,10 @@ PciExpressRead8 ( IN UINTN Address ) { + ASSERT_INVALID_PCI_ADDRESS (Address); + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioRead8 (GetPciExpressAddress (Address)); } @@ -380,7 +413,8 @@ PciExpressRead8 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT8 @@ -390,6 +424,9 @@ PciExpressWrite8 ( IN UINT8 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioWrite8 (GetPciExpressAddress (Address), Value); } @@ -410,7 +447,8 @@ PciExpressWrite8 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -420,6 +458,9 @@ PciExpressOr8 ( IN UINT8 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioOr8 (GetPciExpressAddress (Address), OrData); } @@ -440,7 +481,8 @@ PciExpressOr8 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -450,6 +492,9 @@ PciExpressAnd8 ( IN UINT8 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioAnd8 (GetPciExpressAddress (Address), AndData); } @@ -472,7 +517,8 @@ PciExpressAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -483,6 +529,9 @@ PciExpressAndThenOr8 ( IN UINT8 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioAndThenOr8 ( GetPciExpressAddress (Address), AndData, @@ -508,7 +557,8 @@ PciExpressAndThenOr8 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..7. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT8 @@ -519,6 +569,9 @@ PciExpressBitFieldRead8 ( IN UINTN EndBit ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldRead8 ( GetPciExpressAddress (Address), StartBit, @@ -547,7 +600,8 @@ PciExpressBitFieldRead8 ( Range 0..7. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -559,6 +613,9 @@ PciExpressBitFieldWrite8 ( IN UINT8 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldWrite8 ( GetPciExpressAddress (Address), StartBit, @@ -591,7 +648,8 @@ PciExpressBitFieldWrite8 ( Range 0..7. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -603,6 +661,9 @@ PciExpressBitFieldOr8 ( IN UINT8 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldOr8 ( GetPciExpressAddress (Address), StartBit, @@ -635,7 +696,8 @@ PciExpressBitFieldOr8 ( Range 0..7. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -647,6 +709,9 @@ PciExpressBitFieldAnd8 ( IN UINT8 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldAnd8 ( GetPciExpressAddress (Address), StartBit, @@ -683,7 +748,8 @@ PciExpressBitFieldAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -696,6 +762,9 @@ PciExpressBitFieldAndThenOr8 ( IN UINT8 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldAndThenOr8 ( GetPciExpressAddress (Address), StartBit, @@ -718,7 +787,8 @@ PciExpressBitFieldAndThenOr8 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT16 @@ -727,6 +797,9 @@ PciExpressRead16 ( IN UINTN Address ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioRead16 (GetPciExpressAddress (Address)); } @@ -744,7 +817,8 @@ PciExpressRead16 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT16 @@ -754,6 +828,9 @@ PciExpressWrite16 ( IN UINT16 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioWrite16 (GetPciExpressAddress (Address), Value); } @@ -775,7 +852,8 @@ PciExpressWrite16 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -785,6 +863,9 @@ PciExpressOr16 ( IN UINT16 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioOr16 (GetPciExpressAddress (Address), OrData); } @@ -806,7 +887,8 @@ PciExpressOr16 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -816,6 +898,9 @@ PciExpressAnd16 ( IN UINT16 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioAnd16 (GetPciExpressAddress (Address), AndData); } @@ -839,7 +924,8 @@ PciExpressAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -850,6 +936,9 @@ PciExpressAndThenOr16 ( IN UINT16 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioAndThenOr16 ( GetPciExpressAddress (Address), AndData, @@ -876,7 +965,8 @@ PciExpressAndThenOr16 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..15. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT16 @@ -887,6 +977,9 @@ PciExpressBitFieldRead16 ( IN UINTN EndBit ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldRead16 ( GetPciExpressAddress (Address), StartBit, @@ -916,7 +1009,8 @@ PciExpressBitFieldRead16 ( Range 0..15. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -928,6 +1022,9 @@ PciExpressBitFieldWrite16 ( IN UINT16 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldWrite16 ( GetPciExpressAddress (Address), StartBit, @@ -961,7 +1058,8 @@ PciExpressBitFieldWrite16 ( Range 0..15. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -973,6 +1071,9 @@ PciExpressBitFieldOr16 ( IN UINT16 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldOr16 ( GetPciExpressAddress (Address), StartBit, @@ -1006,7 +1107,8 @@ PciExpressBitFieldOr16 ( Range 0..15. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -1018,6 +1120,9 @@ PciExpressBitFieldAnd16 ( IN UINT16 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldAnd16 ( GetPciExpressAddress (Address), StartBit, @@ -1055,7 +1160,8 @@ PciExpressBitFieldAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -1068,6 +1174,9 @@ PciExpressBitFieldAndThenOr16 ( IN UINT16 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldAndThenOr16 ( GetPciExpressAddress (Address), StartBit, @@ -1090,7 +1199,8 @@ PciExpressBitFieldAndThenOr16 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT32 @@ -1099,6 +1209,9 @@ PciExpressRead32 ( IN UINTN Address ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioRead32 (GetPciExpressAddress (Address)); } @@ -1116,7 +1229,8 @@ PciExpressRead32 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT32 @@ -1126,6 +1240,9 @@ PciExpressWrite32 ( IN UINT32 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioWrite32 (GetPciExpressAddress (Address), Value); } @@ -1147,7 +1264,8 @@ PciExpressWrite32 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1157,6 +1275,9 @@ PciExpressOr32 ( IN UINT32 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioOr32 (GetPciExpressAddress (Address), OrData); } @@ -1178,7 +1299,8 @@ PciExpressOr32 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1188,6 +1310,9 @@ PciExpressAnd32 ( IN UINT32 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioAnd32 (GetPciExpressAddress (Address), AndData); } @@ -1211,7 +1336,8 @@ PciExpressAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1222,6 +1348,9 @@ PciExpressAndThenOr32 ( IN UINT32 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioAndThenOr32 ( GetPciExpressAddress (Address), AndData, @@ -1248,7 +1377,8 @@ PciExpressAndThenOr32 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..31. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT32 @@ -1259,6 +1389,9 @@ PciExpressBitFieldRead32 ( IN UINTN EndBit ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldRead32 ( GetPciExpressAddress (Address), StartBit, @@ -1288,7 +1421,8 @@ PciExpressBitFieldRead32 ( Range 0..31. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1300,6 +1434,9 @@ PciExpressBitFieldWrite32 ( IN UINT32 Value ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldWrite32 ( GetPciExpressAddress (Address), StartBit, @@ -1333,7 +1470,8 @@ PciExpressBitFieldWrite32 ( Range 0..31. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1345,6 +1483,9 @@ PciExpressBitFieldOr32 ( IN UINT32 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldOr32 ( GetPciExpressAddress (Address), StartBit, @@ -1378,7 +1519,8 @@ PciExpressBitFieldOr32 ( Range 0..31. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1390,6 +1532,9 @@ PciExpressBitFieldAnd32 ( IN UINT32 AndData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldAnd32 ( GetPciExpressAddress (Address), StartBit, @@ -1427,7 +1572,8 @@ PciExpressBitFieldAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1440,6 +1586,9 @@ PciExpressBitFieldAndThenOr32 ( IN UINT32 OrData ) { + if (Address >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldAndThenOr32 ( GetPciExpressAddress (Address), StartBit, @@ -1469,7 +1618,8 @@ PciExpressBitFieldAndThenOr32 ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer receiving the data read. - @return Size read data from StartAddress. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other Size read data from StartAddress. **/ UINTN @@ -1485,9 +1635,16 @@ PciExpressReadBuffer ( // // Make sure Address is valid // - ASSERT (((StartAddress) & ~0xfffffff) == 0); + ASSERT_INVALID_PCI_ADDRESS (StartAddress); ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); + // + // Make sure the Address is in MMCONF address space + // + if (StartAddress >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } + if (Size == 0) { return Size; } @@ -1572,7 +1729,8 @@ PciExpressReadBuffer ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer containing the data to write. - @return Size written to StartAddress. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other Size written to StartAddress. **/ UINTN @@ -1588,9 +1746,16 @@ PciExpressWriteBuffer ( // // Make sure Address is valid // - ASSERT (((StartAddress) & ~0xfffffff) == 0); + ASSERT_INVALID_PCI_ADDRESS (StartAddress); ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); + // + // Make sure the Address is in MMCONF address space + // + if (StartAddress >= mDxeRuntimePciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } + if (Size == 0) { return 0; } diff --git a/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c b/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c index 35b9f775a80b..97bd32c8d201 100644 --- a/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c +++ b/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c @@ -20,9 +20,10 @@ #include <Library/PcdLib.h> /// -/// Module global that contains the base physical address of the PCI Express MMIO range. +/// Module global that contains the base physical address and size of the PCI Express MMIO range. /// UINTN mSmmPciExpressLibPciExpressBaseAddress = 0; +UINTN mSmmPciExpressLibPciExpressBaseSize = 0; /** The constructor function caches the PCI Express Base Address @@ -40,9 +41,10 @@ SmmPciExpressLibConstructor ( ) { // - // Cache the physical address of the PCI Express MMIO range into a module global variable + // Cache the physical address and size of the PCI Express MMIO range into a module global variable // mSmmPciExpressLibPciExpressBaseAddress = (UINTN) PcdGet64 (PcdPciExpressBaseAddress); + mSmmPciExpressLibPciExpressBaseSize = (UINTN) PcdGet64 (PcdPciExpressBaseSize); return EFI_SUCCESS; } @@ -97,8 +99,12 @@ PciExpressRegisterForRuntimeAccess ( mSmmPciExpressLibPciExpressBaseAddress is initialized in the library constructor from PCD entry PcdPciExpressBaseAddress. + If Address > 0x0FFFFFFF, then ASSERT(). + @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return MMIO address corresponding to Address. + + @retval (UINTN)-1 Invalid PCI address. + @retval other MMIO address corresponding to Address. **/ UINTN @@ -110,6 +116,12 @@ GetPciExpressAddress ( // Make sure Address is valid // ASSERT_INVALID_PCI_ADDRESS (Address); + // + // Make sure the Address is in MMCONF address space + // + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } return mSmmPciExpressLibPciExpressBaseAddress + Address; } @@ -125,7 +137,8 @@ GetPciExpressAddress ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT8 @@ -134,6 +147,9 @@ PciExpressRead8 ( IN UINTN Address ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioRead8 (GetPciExpressAddress (Address)); } @@ -150,7 +166,8 @@ PciExpressRead8 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT8 @@ -160,6 +177,9 @@ PciExpressWrite8 ( IN UINT8 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioWrite8 (GetPciExpressAddress (Address), Value); } @@ -180,7 +200,8 @@ PciExpressWrite8 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -190,6 +211,9 @@ PciExpressOr8 ( IN UINT8 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioOr8 (GetPciExpressAddress (Address), OrData); } @@ -210,7 +234,8 @@ PciExpressOr8 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -220,6 +245,9 @@ PciExpressAnd8 ( IN UINT8 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioAnd8 (GetPciExpressAddress (Address), AndData); } @@ -242,7 +270,8 @@ PciExpressAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -253,6 +282,9 @@ PciExpressAndThenOr8 ( IN UINT8 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioAndThenOr8 ( GetPciExpressAddress (Address), AndData, @@ -278,7 +310,8 @@ PciExpressAndThenOr8 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..7. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT8 @@ -289,6 +322,9 @@ PciExpressBitFieldRead8 ( IN UINTN EndBit ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldRead8 ( GetPciExpressAddress (Address), StartBit, @@ -317,7 +353,8 @@ PciExpressBitFieldRead8 ( Range 0..7. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -329,6 +366,9 @@ PciExpressBitFieldWrite8 ( IN UINT8 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldWrite8 ( GetPciExpressAddress (Address), StartBit, @@ -361,7 +401,8 @@ PciExpressBitFieldWrite8 ( Range 0..7. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -373,6 +414,9 @@ PciExpressBitFieldOr8 ( IN UINT8 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldOr8 ( GetPciExpressAddress (Address), StartBit, @@ -405,7 +449,8 @@ PciExpressBitFieldOr8 ( Range 0..7. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -417,6 +462,9 @@ PciExpressBitFieldAnd8 ( IN UINT8 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldAnd8 ( GetPciExpressAddress (Address), StartBit, @@ -453,7 +501,8 @@ PciExpressBitFieldAnd8 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT8 @@ -466,6 +515,9 @@ PciExpressBitFieldAndThenOr8 ( IN UINT8 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT8) -1; + } return MmioBitFieldAndThenOr8 ( GetPciExpressAddress (Address), StartBit, @@ -488,7 +540,8 @@ PciExpressBitFieldAndThenOr8 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT16 @@ -497,6 +550,9 @@ PciExpressRead16 ( IN UINTN Address ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioRead16 (GetPciExpressAddress (Address)); } @@ -514,7 +570,8 @@ PciExpressRead16 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT16 @@ -524,6 +581,9 @@ PciExpressWrite16 ( IN UINT16 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioWrite16 (GetPciExpressAddress (Address), Value); } @@ -545,7 +605,8 @@ PciExpressWrite16 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -555,6 +616,9 @@ PciExpressOr16 ( IN UINT16 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioOr16 (GetPciExpressAddress (Address), OrData); } @@ -576,7 +640,8 @@ PciExpressOr16 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -586,6 +651,9 @@ PciExpressAnd16 ( IN UINT16 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioAnd16 (GetPciExpressAddress (Address), AndData); } @@ -609,7 +677,8 @@ PciExpressAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -620,6 +689,9 @@ PciExpressAndThenOr16 ( IN UINT16 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioAndThenOr16 ( GetPciExpressAddress (Address), AndData, @@ -646,7 +718,8 @@ PciExpressAndThenOr16 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..15. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT16 @@ -657,6 +730,9 @@ PciExpressBitFieldRead16 ( IN UINTN EndBit ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldRead16 ( GetPciExpressAddress (Address), StartBit, @@ -686,7 +762,8 @@ PciExpressBitFieldRead16 ( Range 0..15. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -698,6 +775,9 @@ PciExpressBitFieldWrite16 ( IN UINT16 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldWrite16 ( GetPciExpressAddress (Address), StartBit, @@ -731,7 +811,8 @@ PciExpressBitFieldWrite16 ( Range 0..15. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -743,6 +824,9 @@ PciExpressBitFieldOr16 ( IN UINT16 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldOr16 ( GetPciExpressAddress (Address), StartBit, @@ -776,7 +860,8 @@ PciExpressBitFieldOr16 ( Range 0..15. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -788,6 +873,9 @@ PciExpressBitFieldAnd16 ( IN UINT16 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldAnd16 ( GetPciExpressAddress (Address), StartBit, @@ -825,7 +913,8 @@ PciExpressBitFieldAnd16 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT16 @@ -838,6 +927,9 @@ PciExpressBitFieldAndThenOr16 ( IN UINT16 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT16) -1; + } return MmioBitFieldAndThenOr16 ( GetPciExpressAddress (Address), StartBit, @@ -860,7 +952,8 @@ PciExpressBitFieldAndThenOr16 ( @param Address The address that encodes the PCI Bus, Device, Function and Register. - @return The read value from the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The read value from the PCI configuration register. **/ UINT32 @@ -869,6 +962,9 @@ PciExpressRead32 ( IN UINTN Address ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioRead32 (GetPciExpressAddress (Address)); } @@ -886,7 +982,8 @@ PciExpressRead32 ( Register. @param Value The value to write. - @return The value written to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written to the PCI configuration register. **/ UINT32 @@ -896,6 +993,9 @@ PciExpressWrite32 ( IN UINT32 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioWrite32 (GetPciExpressAddress (Address), Value); } @@ -917,7 +1017,8 @@ PciExpressWrite32 ( Register. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -927,6 +1028,9 @@ PciExpressOr32 ( IN UINT32 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioOr32 (GetPciExpressAddress (Address), OrData); } @@ -948,7 +1052,8 @@ PciExpressOr32 ( Register. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -958,6 +1063,9 @@ PciExpressAnd32 ( IN UINT32 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioAnd32 (GetPciExpressAddress (Address), AndData); } @@ -981,7 +1089,8 @@ PciExpressAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -992,6 +1101,9 @@ PciExpressAndThenOr32 ( IN UINT32 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioAndThenOr32 ( GetPciExpressAddress (Address), AndData, @@ -1018,7 +1130,8 @@ PciExpressAndThenOr32 ( @param EndBit The ordinal of the most significant bit in the bit field. Range 0..31. - @return The value of the bit field read from the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value of the bit field read from the PCI configuration register. **/ UINT32 @@ -1029,6 +1142,9 @@ PciExpressBitFieldRead32 ( IN UINTN EndBit ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldRead32 ( GetPciExpressAddress (Address), StartBit, @@ -1058,7 +1174,8 @@ PciExpressBitFieldRead32 ( Range 0..31. @param Value The new value of the bit field. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1070,6 +1187,9 @@ PciExpressBitFieldWrite32 ( IN UINT32 Value ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldWrite32 ( GetPciExpressAddress (Address), StartBit, @@ -1103,7 +1223,8 @@ PciExpressBitFieldWrite32 ( Range 0..31. @param OrData The value to OR with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1115,6 +1236,9 @@ PciExpressBitFieldOr32 ( IN UINT32 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldOr32 ( GetPciExpressAddress (Address), StartBit, @@ -1148,7 +1272,8 @@ PciExpressBitFieldOr32 ( Range 0..31. @param AndData The value to AND with the PCI configuration register. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1160,6 +1285,9 @@ PciExpressBitFieldAnd32 ( IN UINT32 AndData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldAnd32 ( GetPciExpressAddress (Address), StartBit, @@ -1197,7 +1325,8 @@ PciExpressBitFieldAnd32 ( @param AndData The value to AND with the PCI configuration register. @param OrData The value to OR with the result of the AND operation. - @return The value written back to the PCI configuration register. + @retval 0xFFFFFFFF Invalid PCI address. + @retval other The value written back to the PCI configuration register. **/ UINT32 @@ -1210,6 +1339,9 @@ PciExpressBitFieldAndThenOr32 ( IN UINT32 OrData ) { + if (Address >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINT32) -1; + } return MmioBitFieldAndThenOr32 ( GetPciExpressAddress (Address), StartBit, @@ -1239,7 +1371,8 @@ PciExpressBitFieldAndThenOr32 ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer receiving the data read. - @return Size read data from StartAddress. + @retval (UINTN)-1 Invalid PCI address. + @retval other Size read data from StartAddress. **/ UINTN @@ -1258,6 +1391,13 @@ PciExpressReadBuffer ( ASSERT_INVALID_PCI_ADDRESS (StartAddress); ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); + // + // Make sure the Address is in MMCONF address space + // + if (StartAddress >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } + if (Size == 0) { return Size; } @@ -1342,7 +1482,8 @@ PciExpressReadBuffer ( @param Size The size in bytes of the transfer. @param Buffer The pointer to a buffer containing the data to write. - @return Size written to StartAddress. + @retval (UINTN)-1 Invalid PCI address. + @retval other Size written to StartAddress. **/ UINTN @@ -1361,6 +1502,13 @@ PciExpressWriteBuffer ( ASSERT_INVALID_PCI_ADDRESS (StartAddress); ASSERT (((StartAddress & 0xFFF) + Size) <= 0x1000); + // + // Make sure the Address is in MMCONF address space + // + if (StartAddress >= mSmmPciExpressLibPciExpressBaseSize) { + return (UINTN) -1; + } + if (Size == 0) { return 0; -- 2.28.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v5 3/3] UefiPayloadPkg: Support variable size MMCONF space 2020-08-18 8:24 [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF Marcello Sylvester Bauer 2020-08-18 8:24 ` [PATCH v5 1/3] UefiPayloadPkg: Store the size of the MMCONF window Marcello Sylvester Bauer 2020-08-18 8:24 ` [PATCH v5 2/3] MdePkg: PciExpressLib support variable size MMCONF Marcello Sylvester Bauer @ 2020-08-18 8:24 ` Marcello Sylvester Bauer 2020-09-08 22:35 ` [edk2-devel] " Guo Dong 2020-09-16 8:56 ` more development process failure [was: UefiPayloadPkg: Runtime MMCONF] Laszlo Ersek 3 siblings, 1 reply; 27+ messages in thread From: Marcello Sylvester Bauer @ 2020-08-18 8:24 UTC (permalink / raw) To: devel Cc: Patrick Rudolph, Christian Walter, Maurice Ma, Nate DeSimone, Benjamin You The default size is still 256MiB, but will be overwritten by UefiPayloadPkg with the real MMCONF size. e.g.: On embedded AMD platforms the MMCONF window size is usually only 64MiB. Fixes crash on platforms not exposing 256 buses. Tested on: * AMD Stoney Ridge Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> Cc: Patrick Rudolph <patrick.rudolph@9elements.com> Cc: Christian Walter <christian.walter@9elements.com> Cc: Maurice Ma <maurice.ma@intel.com> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> Cc: Benjamin You <benjamin.you@intel.com> --- UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 + UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 1 + UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc index 942bc9076634..5898a474f9e9 100644 --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc @@ -365,6 +365,7 @@ [PcdsDynamicDefault] gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31 gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100 gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0 + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize|0 ################################################################################ # diff --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] TRUE 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.28.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v5 3/3] UefiPayloadPkg: Support variable size MMCONF space 2020-08-18 8:24 ` [PATCH v5 3/3] UefiPayloadPkg: Support variable size MMCONF space Marcello Sylvester Bauer @ 2020-09-08 22:35 ` Guo Dong 0 siblings, 0 replies; 27+ messages in thread From: Guo Dong @ 2020-09-08 22:35 UTC (permalink / raw) To: devel@edk2.groups.io, marcello.bauer@9elements.com Cc: Patrick Rudolph, Christian Walter, Ma, Maurice, Desimone, Nathaniel L, You, Benjamin Reviewed-by: Guo Dong <guo.dong@intel.com> > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marcello > Sylvester Bauer > Sent: Tuesday, August 18, 2020 1:24 AM > To: devel@edk2.groups.io > Cc: Patrick Rudolph <patrick.rudolph@9elements.com>; Christian Walter > <christian.walter@9elements.com>; Ma, Maurice <maurice.ma@intel.com>; > Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; You, Benjamin > <benjamin.you@intel.com> > Subject: [edk2-devel] [PATCH v5 3/3] UefiPayloadPkg: Support variable size > MMCONF space > > The default size is still 256MiB, but will be overwritten by > UefiPayloadPkg with the real MMCONF size. > > e.g.: On embedded AMD platforms the MMCONF window size is usually > only 64MiB. > > Fixes crash on platforms not exposing 256 buses. > Tested on: > * AMD Stoney Ridge > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Marcello Sylvester Bauer <marcello.bauer@9elements.com> > Cc: Patrick Rudolph <patrick.rudolph@9elements.com> > Cc: Christian Walter <christian.walter@9elements.com> > Cc: Maurice Ma <maurice.ma@intel.com> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > Cc: Benjamin You <benjamin.you@intel.com> > --- > UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 1 + > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 1 + > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 +++- > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc > b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc > index 942bc9076634..5898a474f9e9 100644 > --- a/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc > +++ b/UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc > @@ -365,6 +365,7 @@ [PcdsDynamicDefault] > gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow|31 > > gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100 > > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0 > > + gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize|0 > > > > > ################################################################ > ################ > > # > > diff --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] > > TRUE > > 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.28.0 > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > > View/Reply Online (#64366): https://edk2.groups.io/g/devel/message/64366 > Mute This Topic: https://groups.io/mt/76261187/1781375 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [guo.dong@intel.com] > -=-=-=-=-=-= ^ permalink raw reply [flat|nested] 27+ messages in thread
* more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-08-18 8:24 [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF Marcello Sylvester Bauer ` (2 preceding siblings ...) 2020-08-18 8:24 ` [PATCH v5 3/3] UefiPayloadPkg: Support variable size MMCONF space Marcello Sylvester Bauer @ 2020-09-16 8:56 ` Laszlo Ersek 2020-09-16 17:30 ` [edk2-devel] " Guo Dong 2020-09-21 9:57 ` Marcello Sylvester Bauer 3 siblings, 2 replies; 27+ messages in thread From: Laszlo Ersek @ 2020-09-16 8:56 UTC (permalink / raw) To: Dong Guo Cc: devel, marcello.bauer, Michael Kinney, Leif Lindholm (Nuvia address), Mark Doran, Andrew Fish, Soumya Guptha Guo, On 08/18/20 10:24, Marcello Sylvester Bauer wrote: > Support arbitrary platforms with different or even no MMCONF space. > Fixes crash on platforms not exposing 256 buses. > > Tested on: > * AMD Stoney Ridge > > Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg-MMCONF > PR: https://github.com/tianocore/edk2/pull/885 > > v5: > * MdePkg > - support variable size MMCONF in all PciExpressLibs > - use (UINTX)-1 as return values for invalid Pci addresses Okay, so we got more of the same development process violations here, as I've just reported at <https://edk2.groups.io/g/devel/message/65313>. See this new pull request: https://github.com/tianocore/edk2/pull/932/ "No description provided." You should be embarrassed. Laszlo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-16 8:56 ` more development process failure [was: UefiPayloadPkg: Runtime MMCONF] Laszlo Ersek @ 2020-09-16 17:30 ` Guo Dong 2020-09-16 18:14 ` Laszlo Ersek 2020-09-21 9:57 ` Marcello Sylvester Bauer 1 sibling, 1 reply; 27+ messages in thread From: Guo Dong @ 2020-09-16 17:30 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: marcello.bauer@9elements.com, Kinney, Michael D, Leif Lindholm (Nuvia address), Doran, Mark, Andrew Fish, Guptha, Soumya K Hi Laszlo, The patchset includes 3 patches, and all of them had been reviewed by package owners. The patch submitter has a pull request https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest master, and merged it by adding reviewed-by found from emails. I also make sure it passed all the checks before I put "push" button there. then retrigger a new build with "push" button. I am not sure what is missing. If there is any other requirements, should they be captured during code review or tool check? Thanks, Guo > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > Ersek > Sent: Wednesday, September 16, 2020 1:57 AM > To: Dong, Guo <guo.dong@intel.com> > Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney, Michael D > <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish > <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > Subject: [edk2-devel] more development process failure [was: UefiPayloadPkg: > Runtime MMCONF] > > Guo, > > On 08/18/20 10:24, Marcello Sylvester Bauer wrote: > > Support arbitrary platforms with different or even no MMCONF space. > > Fixes crash on platforms not exposing 256 buses. > > > > Tested on: > > * AMD Stoney Ridge > > > > Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg- > MMCONF > > PR: https://github.com/tianocore/edk2/pull/885 > > > > v5: > > * MdePkg > > - support variable size MMCONF in all PciExpressLibs > > - use (UINTX)-1 as return values for invalid Pci addresses > > Okay, so we got more of the same development process violations here, as > I've just reported at <https://edk2.groups.io/g/devel/message/65313>. > > See this new pull request: > > https://github.com/tianocore/edk2/pull/932/ > > "No description provided." > > You should be embarrassed. > > Laszlo > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-16 17:30 ` [edk2-devel] " Guo Dong @ 2020-09-16 18:14 ` Laszlo Ersek 2020-09-16 21:51 ` Guo Dong 2020-09-17 1:49 ` 回复: " gaoliming 0 siblings, 2 replies; 27+ messages in thread From: Laszlo Ersek @ 2020-09-16 18:14 UTC (permalink / raw) To: Dong, Guo, devel@edk2.groups.io Cc: marcello.bauer@9elements.com, Kinney, Michael D, Leif Lindholm (Nuvia address), Doran, Mark, Andrew Fish, Guptha, Soumya K On 09/16/20 19:30, Dong, Guo wrote: > > Hi Laszlo, > > The patchset includes 3 patches, and all of them had been reviewed by package owners. > The patch submitter has a pull request https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest master, and merged it by adding reviewed-by found from emails. > I also make sure it passed all the checks before I put "push" button there. then retrigger a new build with "push" button. > > I am not sure what is missing. If there is any other requirements, should they be captured during code review or tool check? - The description field of <https://github.com/tianocore/edk2/pull/932/> is empty. It's difficult to tell where the patches come from -- where they were posted and reviewed. A copy of the cover letter should have been included here, plus preferably a link to the v5 mailing list thread (the one that got merged in the end). - It was not confirmed in the v5 mailing list thread that the series had been merged. The confirmation should have included at least one of: (a) the github PR link, (b) the git commit range. (Preferably: both.) It's not the eventual git commits that I'm complaining about, but the lack of communication with the community, and the lack of record for posterity. Myself, I used to consider github PRs a means merely for replacing our earlier direct "git push" commands -- with a CI build + mergify. So, as a maintainer, I would myself queue up several patch sets in a single "batch" PR, add some links to BZs and the mailing list, and let it fly. But then Mike told me this was really wrong, and we should clearly associate any given PR with a specific patch set on the list. This meant an *immense* workload increase for me, in particular because I tend to merge patch sets for *other* people and subsystems too (after they pass review), that is, for such subsystems that I do not co-maintain. In particular during the feature freeze periods. So what really rubs me the wrong way is that, if I am expected to keep all of this meta-data nice and tidy, why aren't some other maintainers? It's a double standard. I can live with either *all of us* ignoring PR tidiness, or *all of us* doing our best to keep everything nicely cross-referenced. But right now I spend significant time and effort on keeping communication and records complete and clean in *all three of* bugzilla, github, and mailing list, whereas a good subset of the maintainers couldn't care less in *either* of those communication channels. For your reference, here's a random PR I submitted and merged for others: https://github.com/tianocore/edk2/pull/904 Observe in PR#904: - title carries cover letter subject - description carries cover letter body - description has a pointer to the BZ, and a link to the cover letter in the mailing list archive (two links in fact, in different archives) And then here's my report back on the list: https://edk2.groups.io/g/devel/message/64644 And my BZ comment to the same effect (also closing the BZ as RESOLVED|FIXED): https://bugzilla.tianocore.org/show_bug.cgi?id=2376#c9 https://edk2.groups.io/g/bugs/message/12777 I don't insist on the particular information content of github PRs, as -- at this stage -- they really are not more than just a way to set off CI, before pushing/merging a series. What I do insist on is that all of us maintainers (people with permission to set the "push" label) be subject to the same expectations when it comes to creating pull requests. (Please note also that I absolutely don't need a BZ for every contribution. My request is only that *if* there is a BZ, then handle it thoroughly.) Laszlo > > Thanks, > Guo > >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo >> Ersek >> Sent: Wednesday, September 16, 2020 1:57 AM >> To: Dong, Guo <guo.dong@intel.com> >> Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney, Michael D >> <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) >> <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish >> <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> >> Subject: [edk2-devel] more development process failure [was: UefiPayloadPkg: >> Runtime MMCONF] >> >> Guo, >> >> On 08/18/20 10:24, Marcello Sylvester Bauer wrote: >>> Support arbitrary platforms with different or even no MMCONF space. >>> Fixes crash on platforms not exposing 256 buses. >>> >>> Tested on: >>> * AMD Stoney Ridge >>> >>> Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg- >> MMCONF >>> PR: https://github.com/tianocore/edk2/pull/885 >>> >>> v5: >>> * MdePkg >>> - support variable size MMCONF in all PciExpressLibs >>> - use (UINTX)-1 as return values for invalid Pci addresses >> >> Okay, so we got more of the same development process violations here, as >> I've just reported at <https://edk2.groups.io/g/devel/message/65313>. >> >> See this new pull request: >> >> https://github.com/tianocore/edk2/pull/932/ >> >> "No description provided." >> >> You should be embarrassed. >> >> Laszlo >> >> >> >> >> > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-16 18:14 ` Laszlo Ersek @ 2020-09-16 21:51 ` Guo Dong 2020-09-17 5:59 ` Laszlo Ersek 2020-09-17 1:49 ` 回复: " gaoliming 1 sibling, 1 reply; 27+ messages in thread From: Guo Dong @ 2020-09-16 21:51 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: marcello.bauer@9elements.com, Kinney, Michael D, Leif Lindholm (Nuvia address), Doran, Mark, Andrew Fish, Guptha, Soumya K Thanks Laszlo for all these information. It looks it is not well communicated on the merge requirements. This is the first time for me to merge a patch set and I had thought it is same with merging a single patch (no cover letter). As UefiPayloadPkg maintainer, most time I worked on bootloaders. It would be great if we could have a single page to documents the rule and steps for maintainers. And it would be great for maintainers if EDK2 could move to github code review, and merge from the pull request directly once it passed the review. We don't need do any extra things for the patch merge. Thanks, Guo > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > Ersek > Sent: Wednesday, September 16, 2020 11:14 AM > To: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io > Cc: marcello.bauer@9elements.com; Kinney, Michael D > <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish > <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > Subject: Re: [edk2-devel] more development process failure [was: > UefiPayloadPkg: Runtime MMCONF] > > On 09/16/20 19:30, Dong, Guo wrote: > > > > Hi Laszlo, > > > > The patchset includes 3 patches, and all of them had been reviewed by > package owners. > > The patch submitter has a pull request > https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest > master, and merged it by adding reviewed-by found from emails. > > I also make sure it passed all the checks before I put "push" button there. then > retrigger a new build with "push" button. > > > > I am not sure what is missing. If there is any other requirements, should they > be captured during code review or tool check? > > - The description field of <https://github.com/tianocore/edk2/pull/932/> > is empty. It's difficult to tell where the patches come from -- where > they were posted and reviewed. A copy of the cover letter should have > been included here, plus preferably a link to the v5 mailing list thread > (the one that got merged in the end). > > - It was not confirmed in the v5 mailing list thread that the series had > been merged. The confirmation should have included at least one of: (a) > the github PR link, (b) the git commit range. (Preferably: both.) > > It's not the eventual git commits that I'm complaining about, but the > lack of communication with the community, and the lack of record for > posterity. > > Myself, I used to consider github PRs a means merely for replacing our > earlier direct "git push" commands -- with a CI build + mergify. So, as > a maintainer, I would myself queue up several patch sets in a single > "batch" PR, add some links to BZs and the mailing list, and let it fly. > But then Mike told me this was really wrong, and we should clearly > associate any given PR with a specific patch set on the list. > > This meant an *immense* workload increase for me, in particular because > I tend to merge patch sets for *other* people and subsystems too (after > they pass review), that is, for such subsystems that I do not > co-maintain. In particular during the feature freeze periods. > > So what really rubs me the wrong way is that, if I am expected to keep > all of this meta-data nice and tidy, why aren't some other maintainers? > It's a double standard. > > I can live with either *all of us* ignoring PR tidiness, or *all of us* > doing our best to keep everything nicely cross-referenced. > > But right now I spend significant time and effort on keeping > communication and records complete and clean in *all three of* bugzilla, > github, and mailing list, whereas a good subset of the maintainers > couldn't care less in *either* of those communication channels. > > For your reference, here's a random PR I submitted and merged for others: > > https://github.com/tianocore/edk2/pull/904 > > Observe in PR#904: > > - title carries cover letter subject > - description carries cover letter body > - description has a pointer to the BZ, and a link to the cover letter in > the mailing list archive (two links in fact, in different archives) > > And then here's my report back on the list: > > https://edk2.groups.io/g/devel/message/64644 > > And my BZ comment to the same effect (also closing the BZ as > RESOLVED|FIXED): > > https://bugzilla.tianocore.org/show_bug.cgi?id=2376#c9 > https://edk2.groups.io/g/bugs/message/12777 > > > I don't insist on the particular information content of github PRs, as > -- at this stage -- they really are not more than just a way to set off > CI, before pushing/merging a series. > > What I do insist on is that all of us maintainers (people with > permission to set the "push" label) be subject to the same expectations > when it comes to creating pull requests. > > (Please note also that I absolutely don't need a BZ for every > contribution. My request is only that *if* there is a BZ, then handle it > thoroughly.) > > Laszlo > > > > > > Thanks, > > Guo > > > >> -----Original Message----- > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > >> Ersek > >> Sent: Wednesday, September 16, 2020 1:57 AM > >> To: Dong, Guo <guo.dong@intel.com> > >> Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney, > Michael D > >> <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > >> <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish > >> <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > >> Subject: [edk2-devel] more development process failure [was: > UefiPayloadPkg: > >> Runtime MMCONF] > >> > >> Guo, > >> > >> On 08/18/20 10:24, Marcello Sylvester Bauer wrote: > >>> Support arbitrary platforms with different or even no MMCONF space. > >>> Fixes crash on platforms not exposing 256 buses. > >>> > >>> Tested on: > >>> * AMD Stoney Ridge > >>> > >>> Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg- > >> MMCONF > >>> PR: https://github.com/tianocore/edk2/pull/885 > >>> > >>> v5: > >>> * MdePkg > >>> - support variable size MMCONF in all PciExpressLibs > >>> - use (UINTX)-1 as return values for invalid Pci addresses > >> > >> Okay, so we got more of the same development process violations here, as > >> I've just reported at <https://edk2.groups.io/g/devel/message/65313>. > >> > >> See this new pull request: > >> > >> https://github.com/tianocore/edk2/pull/932/ > >> > >> "No description provided." > >> > >> You should be embarrassed. > >> > >> Laszlo > >> > >> > >> > >> > >> > > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-16 21:51 ` Guo Dong @ 2020-09-17 5:59 ` Laszlo Ersek 0 siblings, 0 replies; 27+ messages in thread From: Laszlo Ersek @ 2020-09-17 5:59 UTC (permalink / raw) To: Dong, Guo, devel@edk2.groups.io Cc: marcello.bauer@9elements.com, Kinney, Michael D, Leif Lindholm (Nuvia address), Doran, Mark, Andrew Fish, Guptha, Soumya K On 09/16/20 23:51, Dong, Guo wrote: > > Thanks Laszlo for all these information. > It looks it is not well communicated on the merge requirements. > This is the first time for me to merge a patch set and I had thought > it is same with merging a single patch (no cover letter). > As UefiPayloadPkg maintainer, most time I worked on bootloaders. > It would be great if we could have a single page to documents the > rule and steps for maintainers. > > And it would be great for maintainers if EDK2 could move to github > code review, and merge from the pull request directly once it passed > the review. We don't need do any extra things for the patch merge. Moving to github entirely is indeed the end-goal. I don't know the schedule however. Note that moving the review activities to github will not per se solve these problems. People will still have to compose good PR descriptions, and they will still have to update and cross-reference BZ tickets. (We are not going to abandon BZ for github.com's issue tracker -- we used the github.com issue tracker in the past for edk2, and it proved lacking.) Thanks, Laszlo > > Thanks, > Guo > >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo >> Ersek >> Sent: Wednesday, September 16, 2020 11:14 AM >> To: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io >> Cc: marcello.bauer@9elements.com; Kinney, Michael D >> <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) >> <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish >> <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> >> Subject: Re: [edk2-devel] more development process failure [was: >> UefiPayloadPkg: Runtime MMCONF] >> >> On 09/16/20 19:30, Dong, Guo wrote: >>> >>> Hi Laszlo, >>> >>> The patchset includes 3 patches, and all of them had been reviewed by >> package owners. >>> The patch submitter has a pull request >> https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest >> master, and merged it by adding reviewed-by found from emails. >>> I also make sure it passed all the checks before I put "push" button there. then >> retrigger a new build with "push" button. >>> >>> I am not sure what is missing. If there is any other requirements, should they >> be captured during code review or tool check? >> >> - The description field of <https://github.com/tianocore/edk2/pull/932/> >> is empty. It's difficult to tell where the patches come from -- where >> they were posted and reviewed. A copy of the cover letter should have >> been included here, plus preferably a link to the v5 mailing list thread >> (the one that got merged in the end). >> > >> - It was not confirmed in the v5 mailing list thread that the series had >> been merged. The confirmation should have included at least one of: (a) >> the github PR link, (b) the git commit range. (Preferably: both.) >> >> It's not the eventual git commits that I'm complaining about, but the >> lack of communication with the community, and the lack of record for >> posterity. >> >> Myself, I used to consider github PRs a means merely for replacing our >> earlier direct "git push" commands -- with a CI build + mergify. So, as >> a maintainer, I would myself queue up several patch sets in a single >> "batch" PR, add some links to BZs and the mailing list, and let it fly. >> But then Mike told me this was really wrong, and we should clearly >> associate any given PR with a specific patch set on the list. >> >> This meant an *immense* workload increase for me, in particular because >> I tend to merge patch sets for *other* people and subsystems too (after >> they pass review), that is, for such subsystems that I do not >> co-maintain. In particular during the feature freeze periods. >> >> So what really rubs me the wrong way is that, if I am expected to keep >> all of this meta-data nice and tidy, why aren't some other maintainers? >> It's a double standard. >> >> I can live with either *all of us* ignoring PR tidiness, or *all of us* >> doing our best to keep everything nicely cross-referenced. >> >> But right now I spend significant time and effort on keeping >> communication and records complete and clean in *all three of* bugzilla, >> github, and mailing list, whereas a good subset of the maintainers >> couldn't care less in *either* of those communication channels. >> >> For your reference, here's a random PR I submitted and merged for others: >> >> https://github.com/tianocore/edk2/pull/904 >> >> Observe in PR#904: >> >> - title carries cover letter subject >> - description carries cover letter body >> - description has a pointer to the BZ, and a link to the cover letter in >> the mailing list archive (two links in fact, in different archives) >> >> And then here's my report back on the list: >> >> https://edk2.groups.io/g/devel/message/64644 >> >> And my BZ comment to the same effect (also closing the BZ as >> RESOLVED|FIXED): >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=2376#c9 >> https://edk2.groups.io/g/bugs/message/12777 >> >> >> I don't insist on the particular information content of github PRs, as >> -- at this stage -- they really are not more than just a way to set off >> CI, before pushing/merging a series. >> >> What I do insist on is that all of us maintainers (people with >> permission to set the "push" label) be subject to the same expectations >> when it comes to creating pull requests. >> >> (Please note also that I absolutely don't need a BZ for every >> contribution. My request is only that *if* there is a BZ, then handle it >> thoroughly.) >> >> Laszlo >> >> >>> >>> Thanks, >>> Guo >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo >>>> Ersek >>>> Sent: Wednesday, September 16, 2020 1:57 AM >>>> To: Dong, Guo <guo.dong@intel.com> >>>> Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney, >> Michael D >>>> <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) >>>> <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish >>>> <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> >>>> Subject: [edk2-devel] more development process failure [was: >> UefiPayloadPkg: >>>> Runtime MMCONF] >>>> >>>> Guo, >>>> >>>> On 08/18/20 10:24, Marcello Sylvester Bauer wrote: >>>>> Support arbitrary platforms with different or even no MMCONF space. >>>>> Fixes crash on platforms not exposing 256 buses. >>>>> >>>>> Tested on: >>>>> * AMD Stoney Ridge >>>>> >>>>> Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg- >>>> MMCONF >>>>> PR: https://github.com/tianocore/edk2/pull/885 >>>>> >>>>> v5: >>>>> * MdePkg >>>>> - support variable size MMCONF in all PciExpressLibs >>>>> - use (UINTX)-1 as return values for invalid Pci addresses >>>> >>>> Okay, so we got more of the same development process violations here, as >>>> I've just reported at <https://edk2.groups.io/g/devel/message/65313>. >>>> >>>> See this new pull request: >>>> >>>> https://github.com/tianocore/edk2/pull/932/ >>>> >>>> "No description provided." >>>> >>>> You should be embarrassed. >>>> >>>> Laszlo >>>> >>>> >>>> >>>> >>>> >>> >> >> >> >> >> > ^ permalink raw reply [flat|nested] 27+ messages in thread
* 回复: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-16 18:14 ` Laszlo Ersek 2020-09-16 21:51 ` Guo Dong @ 2020-09-17 1:49 ` gaoliming 2020-09-17 2:31 ` Yao, Jiewen 2020-09-17 5:56 ` 回复: " Laszlo Ersek 1 sibling, 2 replies; 27+ messages in thread From: gaoliming @ 2020-09-17 1:49 UTC (permalink / raw) To: devel, lersek, 'Dong, Guo' Cc: marcello.bauer, 'Kinney, Michael D', 'Leif Lindholm (Nuvia address)', 'Doran, Mark', 'Andrew Fish', 'Guptha, Soumya K' Guo: On pull request, https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project section 7 gives the requirement. If <new-integration-branch> is a patch series, then copy the patch #0 summary into the pull request description. Laszlo: I think we can enhance wiki page https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project to add another step to reply the patch mail with the merged pull request or commit after PR is merged. Thanks Liming > -----邮件原件----- > 发件人: bounce+27952+65339+4905953+8761045@groups.io > <bounce+27952+65339+4905953+8761045@groups.io> 代表 Laszlo Ersek > 发送时间: 2020年9月17日 2:14 > 收件人: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io > 抄送: marcello.bauer@9elements.com; Kinney, Michael D > <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish > <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > 主题: Re: [edk2-devel] more development process failure [was: > UefiPayloadPkg: Runtime MMCONF] > > On 09/16/20 19:30, Dong, Guo wrote: > > > > Hi Laszlo, > > > > The patchset includes 3 patches, and all of them had been reviewed by > package owners. > > The patch submitter has a pull request > https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest > master, and merged it by adding reviewed-by found from emails. > > I also make sure it passed all the checks before I put "push" button there. > then retrigger a new build with "push" button. > > > > I am not sure what is missing. If there is any other requirements, should > they be captured during code review or tool check? > > - The description field of <https://github.com/tianocore/edk2/pull/932/> > is empty. It's difficult to tell where the patches come from -- where > they were posted and reviewed. A copy of the cover letter should have > been included here, plus preferably a link to the v5 mailing list thread > (the one that got merged in the end). > > - It was not confirmed in the v5 mailing list thread that the series had > been merged. The confirmation should have included at least one of: (a) > the github PR link, (b) the git commit range. (Preferably: both.) > > It's not the eventual git commits that I'm complaining about, but the > lack of communication with the community, and the lack of record for > posterity. > > Myself, I used to consider github PRs a means merely for replacing our > earlier direct "git push" commands -- with a CI build + mergify. So, as > a maintainer, I would myself queue up several patch sets in a single > "batch" PR, add some links to BZs and the mailing list, and let it fly. > But then Mike told me this was really wrong, and we should clearly > associate any given PR with a specific patch set on the list. > > This meant an *immense* workload increase for me, in particular because > I tend to merge patch sets for *other* people and subsystems too (after > they pass review), that is, for such subsystems that I do not > co-maintain. In particular during the feature freeze periods. > > So what really rubs me the wrong way is that, if I am expected to keep > all of this meta-data nice and tidy, why aren't some other maintainers? > It's a double standard. > > I can live with either *all of us* ignoring PR tidiness, or *all of us* > doing our best to keep everything nicely cross-referenced. > > But right now I spend significant time and effort on keeping > communication and records complete and clean in *all three of* bugzilla, > github, and mailing list, whereas a good subset of the maintainers > couldn't care less in *either* of those communication channels. > > For your reference, here's a random PR I submitted and merged for others: > > https://github.com/tianocore/edk2/pull/904 > > Observe in PR#904: > > - title carries cover letter subject > - description carries cover letter body > - description has a pointer to the BZ, and a link to the cover letter in > the mailing list archive (two links in fact, in different archives) > > And then here's my report back on the list: > > https://edk2.groups.io/g/devel/message/64644 > > And my BZ comment to the same effect (also closing the BZ as > RESOLVED|FIXED): > > https://bugzilla.tianocore.org/show_bug.cgi?id=2376#c9 > https://edk2.groups.io/g/bugs/message/12777 > > > I don't insist on the particular information content of github PRs, as > -- at this stage -- they really are not more than just a way to set off > CI, before pushing/merging a series. > > What I do insist on is that all of us maintainers (people with > permission to set the "push" label) be subject to the same expectations > when it comes to creating pull requests. > > (Please note also that I absolutely don't need a BZ for every > contribution. My request is only that *if* there is a BZ, then handle it > thoroughly.) > > Laszlo > > > > > > Thanks, > > Guo > > > >> -----Original Message----- > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > >> Ersek > >> Sent: Wednesday, September 16, 2020 1:57 AM > >> To: Dong, Guo <guo.dong@intel.com> > >> Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney, > Michael D > >> <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > >> <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish > >> <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > >> Subject: [edk2-devel] more development process failure [was: > UefiPayloadPkg: > >> Runtime MMCONF] > >> > >> Guo, > >> > >> On 08/18/20 10:24, Marcello Sylvester Bauer wrote: > >>> Support arbitrary platforms with different or even no MMCONF space. > >>> Fixes crash on platforms not exposing 256 buses. > >>> > >>> Tested on: > >>> * AMD Stoney Ridge > >>> > >>> Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg- > >> MMCONF > >>> PR: https://github.com/tianocore/edk2/pull/885 > >>> > >>> v5: > >>> * MdePkg > >>> - support variable size MMCONF in all PciExpressLibs > >>> - use (UINTX)-1 as return values for invalid Pci addresses > >> > >> Okay, so we got more of the same development process violations here, as > >> I've just reported at <https://edk2.groups.io/g/devel/message/65313>. > >> > >> See this new pull request: > >> > >> https://github.com/tianocore/edk2/pull/932/ > >> > >> "No description provided." > >> > >> You should be embarrassed. > >> > >> Laszlo > >> > >> > >> > >> > >> > > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-17 1:49 ` 回复: " gaoliming @ 2020-09-17 2:31 ` Yao, Jiewen 2020-09-17 6:31 ` Laszlo Ersek 2020-09-17 5:56 ` 回复: " Laszlo Ersek 1 sibling, 1 reply; 27+ messages in thread From: Yao, Jiewen @ 2020-09-17 2:31 UTC (permalink / raw) To: devel@edk2.groups.io, gaoliming@byosoft.com.cn, lersek@redhat.com, Dong, Guo Cc: marcello.bauer@9elements.com, Kinney, Michael D, 'Leif Lindholm (Nuvia address)', Doran, Mark, 'Andrew Fish', Guptha, Soumya K Hi Laszlo, Liming, and Mike I appreciate your effort to setup rule and document this *complex* EDK II Development Process. I am thinking if we can have a way (a tool) to mandate these process and check if there is any violation. If people makes mistake, he/she knows he/she is making mistake and can correct immediately, instead of letting mistake happens and getting blame later. In such way, we can prevent issue from happening. We have old maintainer leaving, new maintainers joining. That is the reality. We can have training for everyone. But we are still human. There are many bugs need to be fixed in the code. How can we expect a perfect process that everyone follows strictly without any violation? If we only have few violation, it is OK to stay with it. But if we continuously have violation, we need retrospect to ask, *why*? Why there is such a process to cause so many violation? And can we do better? A simpler process? A better tool? I also feel sorry that Laszlo need check by his eye on every PR and catch the violation for us. And I also feel sorry to blame some people who is contributing his/her time to help to maintain the code, review the code, check in the code. We both feel frustrated. We are all coming her to enable new features or fix bugs to make EDKII better. I would like to ask: Is that technically possible to enhance the CI to catch that earlier, as Laszlo point out: 1) Add patch 0 to PR - can we let CI reject empty description PR? 2) Send email - can we let CI send email automatically? Or remind us to send email? 3) update Bugzilla - can we let CI update Bugzilla automatically? Or remind us to update bugzilla? 4) Unicode char - can we add check in patchchecker, to reject predefined format violation? I know the new tool/CI cannot be built in one day. And we do improvement step by step. Thank you Yao Jiewen > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming > Sent: Thursday, September 17, 2020 9:49 AM > To: devel@edk2.groups.io; lersek@redhat.com; Dong, Guo > <guo.dong@intel.com> > Cc: marcello.bauer@9elements.com; Kinney, Michael D > <michael.d.kinney@intel.com>; 'Leif Lindholm (Nuvia address)' > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; 'Andrew Fish' > <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > Subject: 回复: [edk2-devel] more development process failure [was: > UefiPayloadPkg: Runtime MMCONF] > > Guo: > On pull request, https://github.com/tianocore/tianocore.github.io/wiki/EDK-II- > Development-Process#the-maintainer-process-for-the-edk-ii-project section 7 > gives the requirement. > If <new-integration-branch> is a patch series, then copy the patch #0 summary > into the pull request description. > > Laszlo: > I think we can enhance wiki page > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development- > Process#the-maintainer-process-for-the-edk-ii-project to add another step to > reply the patch mail with the merged pull request or commit after PR is merged. > > Thanks > Liming > > -----邮件原件----- > > 发件人: bounce+27952+65339+4905953+8761045@groups.io > > <bounce+27952+65339+4905953+8761045@groups.io> 代表 Laszlo Ersek > > 发送时间: 2020年9月17日 2:14 > > 收件人: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io > > 抄送: marcello.bauer@9elements.com; Kinney, Michael D > > <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish > > <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > > 主题: Re: [edk2-devel] more development process failure [was: > > UefiPayloadPkg: Runtime MMCONF] > > > > On 09/16/20 19:30, Dong, Guo wrote: > > > > > > Hi Laszlo, > > > > > > The patchset includes 3 patches, and all of them had been reviewed by > > package owners. > > > The patch submitter has a pull request > > https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest > > master, and merged it by adding reviewed-by found from emails. > > > I also make sure it passed all the checks before I put "push" button there. > > then retrigger a new build with "push" button. > > > > > > I am not sure what is missing. If there is any other requirements, should > > they be captured during code review or tool check? > > > > - The description field of <https://github.com/tianocore/edk2/pull/932/> > > is empty. It's difficult to tell where the patches come from -- where > > they were posted and reviewed. A copy of the cover letter should have > > been included here, plus preferably a link to the v5 mailing list thread > > (the one that got merged in the end). > > > > - It was not confirmed in the v5 mailing list thread that the series had > > been merged. The confirmation should have included at least one of: (a) > > the github PR link, (b) the git commit range. (Preferably: both.) > > > > It's not the eventual git commits that I'm complaining about, but the > > lack of communication with the community, and the lack of record for > > posterity. > > > > Myself, I used to consider github PRs a means merely for replacing our > > earlier direct "git push" commands -- with a CI build + mergify. So, as > > a maintainer, I would myself queue up several patch sets in a single > > "batch" PR, add some links to BZs and the mailing list, and let it fly. > > But then Mike told me this was really wrong, and we should clearly > > associate any given PR with a specific patch set on the list. > > > > This meant an *immense* workload increase for me, in particular because > > I tend to merge patch sets for *other* people and subsystems too (after > > they pass review), that is, for such subsystems that I do not > > co-maintain. In particular during the feature freeze periods. > > > > So what really rubs me the wrong way is that, if I am expected to keep > > all of this meta-data nice and tidy, why aren't some other maintainers? > > It's a double standard. > > > > I can live with either *all of us* ignoring PR tidiness, or *all of us* > > doing our best to keep everything nicely cross-referenced. > > > > But right now I spend significant time and effort on keeping > > communication and records complete and clean in *all three of* bugzilla, > > github, and mailing list, whereas a good subset of the maintainers > > couldn't care less in *either* of those communication channels. > > > > For your reference, here's a random PR I submitted and merged for others: > > > > https://github.com/tianocore/edk2/pull/904 > > > > Observe in PR#904: > > > > - title carries cover letter subject > > - description carries cover letter body > > - description has a pointer to the BZ, and a link to the cover letter in > > the mailing list archive (two links in fact, in different archives) > > > > And then here's my report back on the list: > > > > https://edk2.groups.io/g/devel/message/64644 > > > > And my BZ comment to the same effect (also closing the BZ as > > RESOLVED|FIXED): > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=2376#c9 > > https://edk2.groups.io/g/bugs/message/12777 > > > > > > I don't insist on the particular information content of github PRs, as > > -- at this stage -- they really are not more than just a way to set off > > CI, before pushing/merging a series. > > > > What I do insist on is that all of us maintainers (people with > > permission to set the "push" label) be subject to the same expectations > > when it comes to creating pull requests. > > > > (Please note also that I absolutely don't need a BZ for every > > contribution. My request is only that *if* there is a BZ, then handle it > > thoroughly.) > > > > Laszlo > > > > > > > > > > Thanks, > > > Guo > > > > > >> -----Original Message----- > > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > > >> Ersek > > >> Sent: Wednesday, September 16, 2020 1:57 AM > > >> To: Dong, Guo <guo.dong@intel.com> > > >> Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney, > > Michael D > > >> <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > > >> <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish > > >> <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > > >> Subject: [edk2-devel] more development process failure [was: > > UefiPayloadPkg: > > >> Runtime MMCONF] > > >> > > >> Guo, > > >> > > >> On 08/18/20 10:24, Marcello Sylvester Bauer wrote: > > >>> Support arbitrary platforms with different or even no MMCONF space. > > >>> Fixes crash on platforms not exposing 256 buses. > > >>> > > >>> Tested on: > > >>> * AMD Stoney Ridge > > >>> > > >>> Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg- > > >> MMCONF > > >>> PR: https://github.com/tianocore/edk2/pull/885 > > >>> > > >>> v5: > > >>> * MdePkg > > >>> - support variable size MMCONF in all PciExpressLibs > > >>> - use (UINTX)-1 as return values for invalid Pci addresses > > >> > > >> Okay, so we got more of the same development process violations here, as > > >> I've just reported at <https://edk2.groups.io/g/devel/message/65313>. > > >> > > >> See this new pull request: > > >> > > >> https://github.com/tianocore/edk2/pull/932/ > > >> > > >> "No description provided." > > >> > > >> You should be embarrassed. > > >> > > >> Laszlo > > >> > > >> > > >> > > >> > > >> > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-17 2:31 ` Yao, Jiewen @ 2020-09-17 6:31 ` Laszlo Ersek 2020-09-17 7:31 ` Yao, Jiewen 2020-09-18 4:39 ` Ni, Ray 0 siblings, 2 replies; 27+ messages in thread From: Laszlo Ersek @ 2020-09-17 6:31 UTC (permalink / raw) To: Yao, Jiewen, devel@edk2.groups.io, gaoliming@byosoft.com.cn, Dong, Guo Cc: marcello.bauer@9elements.com, Kinney, Michael D, 'Leif Lindholm (Nuvia address)', Doran, Mark, 'Andrew Fish', Guptha, Soumya K Jiewen, On 09/17/20 04:31, Yao, Jiewen wrote: > Hi Laszlo, Liming, and Mike > > I appreciate your effort to setup rule and document this *complex* EDK II Development Process. > > I am thinking if we can have a way (a tool) to mandate these process and check if there is any violation. If people makes mistake, he/she knows he/she is making mistake and can correct immediately, instead of letting mistake happens and getting blame later. In such way, we can prevent issue from happening. > > We have old maintainer leaving, new maintainers joining. That is the reality. We can have training for everyone. But we are still human. There are many bugs need to be fixed in the code. How can we expect a perfect process that everyone follows strictly without any violation? > > If we only have few violation, it is OK to stay with it. > But if we continuously have violation, we need retrospect to ask, *why*? Why there is such a process to cause so many violation? > And can we do better? A simpler process? A better tool? while I agree that the current process is not really simple, I'd like to point out some things: - The current complexity exists because we are in a transition period, and so we get to deal with both the workflow we're leaving (= the mailing list based review) and the system we're adopting (= github). This should not last forever. I don't know the exact schedule though. - I think that lack of attention to detail (on the human side) takes a relatively large chunk of the blame. The process at the moment is not simple, but it's exercised every day, every week by some people, so if somebody *wants*, they can get it right by following examples. Look at recent patch series threads that have been merged, recent BZs that have been closed, recent PRs that have been opened and merged. It's a fallacy that adopting a 100% github.com-native patch review workflow will solve all of these issues. There is no replacement for human discipline and attention to detail. In the current process, I *regularly* find pull requests (personal builds or maintainer push attempts) on github.com that fail CI (or merging due to conflicts) and then the submitter never bothers to close or refresh them. I have cleaned up (closed) a *multitude* of such PRs. > I also feel sorry that Laszlo need check by his eye on every PR and catch the violation for us. And I also feel sorry to blame some people who is contributing his/her time to help to maintain the code, review the code, check in the code. > We both feel frustrated. We are all coming her to enable new features or fix bugs to make EDKII better. > > I would like to ask: Is that technically possible to enhance the CI to catch that earlier, as Laszlo point out: > 1) Add patch 0 to PR - can we let CI reject empty description PR? It won't help. See the following bug report: https://bugzilla.tianocore.org/show_bug.cgi?id=2963#c0 While it is technically not empty (the string in comment#0 has nonzero length), it's practically *devoid of information*. People that are annoyed that they are required to write sensible summaries for patch sets and bug reports, will do anything and everything to wiggle out of that requirement. They will create single-sentence PR descriptions, which will allow them to pass the CI check. And the community will be *worse off*, because we will have complicated our CI logic, but the resultant historical records will be just as useless. > 2) Send email - can we let CI send email automatically? Or remind us to send email? github.com *already* sends an email notification when a PR undergoes a state change; that is, when it is merged, or else CI fails. The email is *already* there, people just have to *act* upon it -- run a local "git pull" again, see what the new commit range is, and send a response to the original thread. > 3) update Bugzilla - can we let CI update Bugzilla automatically? Or remind us to update bugzilla? Automatically closing tickets is not implemented between github.com and Bugzilla. It is implemented within github.com (merging a PR can auto-close issue tracker tickets, if you format the commit message corectly). However, auto-closing is *wrong*. It occurs that multiple patch series relate to a single ticket. In such cases, it's possible that 10+ patches are merged for a single ticket, and the ticket should *still* not be closed, because more patches (for the same ticket) are necessary. Only a human can tell when the fix or the feature is considered complete (according to their knowledge at that point in time). > 4) Unicode char - can we add check in patchchecker, to reject predefined format violation? There are many-many classes of unicode code points. It's not easy to express "accept U+003A for punctuation, but do not accept U+FF1A". It's easy to express "accept 7-bit ASCII only", but I think some people might take issue with that, because then their names could not be placed in subject lines in native form. > > I know the new tool/CI cannot be built in one day. And we do improvement step by step. The *real* problem is with the attitude. If a developer cares *only* until their patches are merged, then no tooling will *ever* fix this issue. People need to start caring about the afterlife of their work. When you throw a party, or join one, you stay around for the cleanup afterwards, don't you? When you call a contractor to fix something in or around the house, do you expect them to clean up when they're done, or are you happy cleaning after them? The exact same bad attitude is the reason that - we have botched error paths in programming languages like C, - we have programming languages and libraries that attempt (and *fail*) to clean up resources on errors, "on behalf" of the programmer -- I'm referring to exceptions and destructors in C++, for example. Both of these are symptoms that people *refuse* to deal with the "boring" aspects of the job. Just accept that the party isn't finished until the house and the garden are tidied up, and the furniture is restored to original order. Laszlo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-17 6:31 ` Laszlo Ersek @ 2020-09-17 7:31 ` Yao, Jiewen 2020-09-17 10:26 ` Laszlo Ersek 2020-09-18 4:39 ` Ni, Ray 1 sibling, 1 reply; 27+ messages in thread From: Yao, Jiewen @ 2020-09-17 7:31 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, gaoliming@byosoft.com.cn, Dong, Guo Cc: marcello.bauer@9elements.com, Kinney, Michael D, 'Leif Lindholm (Nuvia address)', Doran, Mark, 'Andrew Fish', Guptha, Soumya K Laszlo I like your description to compare the process with the programming language and software design. We have to clean up the resource. Please allow me to point out, this is the exactly the issue we are having today. 1) Take buffer overflow as an example. It has been there for 30 years. We have enough books and papers talking about it. Today, people are still making mistakes. Why? Because C language is not type safe, there is NO enforcement. That is why many people like other type safe language. If you are trying to make mistake, the compiler will tell you that you are making mistake. 2) Take resource leak as an example. The programming language invented garbage collection. The operating system auto cleaned up application resource after execution. 3) People has wrong check in which may break the system. What is why the world invented smoke test and continuous integration. *Those are where the inventions come*, to treat human as human being, to prevent people making mistake or teach them automatically. :-) I agree with the "mythical man-month" that there is no silver bulletin. I tend to agree with you on the attitude. I am trying to figure if we can do better to help other people (maintainer or contributor). If we really really can do nothing, that is OK. I am not sure if is a best way to resolve the problem to just complain in the email. I think you can understand my point. I will stop here. Thank you Yao Jiewen > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Thursday, September 17, 2020 2:32 PM > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; > gaoliming@byosoft.com.cn; Dong, Guo <guo.dong@intel.com> > Cc: marcello.bauer@9elements.com; Kinney, Michael D > <michael.d.kinney@intel.com>; 'Leif Lindholm (Nuvia address)' > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; 'Andrew Fish' > <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com> > Subject: Re: [edk2-devel] more development process failure [was: > UefiPayloadPkg: Runtime MMCONF] > > Jiewen, > > On 09/17/20 04:31, Yao, Jiewen wrote: > > Hi Laszlo, Liming, and Mike > > > > I appreciate your effort to setup rule and document this *complex* EDK II > Development Process. > > > > I am thinking if we can have a way (a tool) to mandate these process and > check if there is any violation. If people makes mistake, he/she knows he/she is > making mistake and can correct immediately, instead of letting mistake happens > and getting blame later. In such way, we can prevent issue from happening. > > > > We have old maintainer leaving, new maintainers joining. That is the reality. > We can have training for everyone. But we are still human. There are many bugs > need to be fixed in the code. How can we expect a perfect process that > everyone follows strictly without any violation? > > > > If we only have few violation, it is OK to stay with it. > > But if we continuously have violation, we need retrospect to ask, *why*? Why > there is such a process to cause so many violation? > > And can we do better? A simpler process? A better tool? > > while I agree that the current process is not really simple, I'd like to > point out some things: > > - The current complexity exists because we are in a transition period, > and so we get to deal with both the workflow we're leaving (= the > mailing list based review) and the system we're adopting (= github). > This should not last forever. I don't know the exact schedule though. > > - I think that lack of attention to detail (on the human side) takes a > relatively large chunk of the blame. The process at the moment is not > simple, but it's exercised every day, every week by some people, so if > somebody *wants*, they can get it right by following examples. Look at > recent patch series threads that have been merged, recent BZs that have > been closed, recent PRs that have been opened and merged. > > It's a fallacy that adopting a 100% github.com-native patch review > workflow will solve all of these issues. There is no replacement for > human discipline and attention to detail. In the current process, I > *regularly* find pull requests (personal builds or maintainer push > attempts) on github.com that fail CI (or merging due to conflicts) and > then the submitter never bothers to close or refresh them. I have > cleaned up (closed) a *multitude* of such PRs. > > > I also feel sorry that Laszlo need check by his eye on every PR and catch the > violation for us. And I also feel sorry to blame some people who is contributing > his/her time to help to maintain the code, review the code, check in the code. > > We both feel frustrated. We are all coming her to enable new features or fix > bugs to make EDKII better. > > > > I would like to ask: Is that technically possible to enhance the CI to catch that > earlier, as Laszlo point out: > > 1) Add patch 0 to PR - can we let CI reject empty description PR? > > It won't help. > > See the following bug report: > > https://bugzilla.tianocore.org/show_bug.cgi?id=2963#c0 > > While it is technically not empty (the string in comment#0 has nonzero > length), it's practically *devoid of information*. > > People that are annoyed that they are required to write sensible > summaries for patch sets and bug reports, will do anything and > everything to wiggle out of that requirement. They will create > single-sentence PR descriptions, which will allow them to pass the CI > check. And the community will be *worse off*, because we will have > complicated our CI logic, but the resultant historical records will be > just as useless. > > > 2) Send email - can we let CI send email automatically? Or remind us to send > email? > > github.com *already* sends an email notification when a PR undergoes a > state change; that is, when it is merged, or else CI fails. The email is > *already* there, people just have to *act* upon it -- run a local "git > pull" again, see what the new commit range is, and send a response to > the original thread. > > > 3) update Bugzilla - can we let CI update Bugzilla automatically? Or remind us > to update bugzilla? > > Automatically closing tickets is not implemented between github.com and > Bugzilla. It is implemented within github.com (merging a PR can > auto-close issue tracker tickets, if you format the commit message > corectly). > > However, auto-closing is *wrong*. It occurs that multiple patch series > relate to a single ticket. In such cases, it's possible that 10+ patches > are merged for a single ticket, and the ticket should *still* not be > closed, because more patches (for the same ticket) are necessary. Only a > human can tell when the fix or the feature is considered complete > (according to their knowledge at that point in time). > > > 4) Unicode char - can we add check in patchchecker, to reject predefined > format violation? > > There are many-many classes of unicode code points. It's not easy to > express "accept U+003A for punctuation, but do not accept U+FF1A". > > It's easy to express "accept 7-bit ASCII only", but I think some people > might take issue with that, because then their names could not be placed > in subject lines in native form. > > > > > I know the new tool/CI cannot be built in one day. And we do improvement > step by step. > > The *real* problem is with the attitude. If a developer cares *only* > until their patches are merged, then no tooling will *ever* fix this > issue. People need to start caring about the afterlife of their work. > When you throw a party, or join one, you stay around for the cleanup > afterwards, don't you? > > When you call a contractor to fix something in or around the house, do > you expect them to clean up when they're done, or are you happy cleaning > after them? > > > The exact same bad attitude is the reason that > > - we have botched error paths in programming languages like C, > > - we have programming languages and libraries that attempt (and *fail*) > to clean up resources on errors, "on behalf" of the programmer -- I'm > referring to exceptions and destructors in C++, for example. > > Both of these are symptoms that people *refuse* to deal with the > "boring" aspects of the job. > > > Just accept that the party isn't finished until the house and the garden > are tidied up, and the furniture is restored to original order. > > Laszlo > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-17 7:31 ` Yao, Jiewen @ 2020-09-17 10:26 ` Laszlo Ersek 0 siblings, 0 replies; 27+ messages in thread From: Laszlo Ersek @ 2020-09-17 10:26 UTC (permalink / raw) To: Yao, Jiewen, devel@edk2.groups.io, gaoliming@byosoft.com.cn, Dong, Guo Cc: marcello.bauer@9elements.com, Kinney, Michael D, 'Leif Lindholm (Nuvia address)', Doran, Mark, 'Andrew Fish', Guptha, Soumya K On 09/17/20 09:31, Yao, Jiewen wrote: > Laszlo > I like your description to compare the process with the programming language and software design. We have to clean up the resource. > > Please allow me to point out, this is the exactly the issue we are having today. > > 1) Take buffer overflow as an example. It has been there for 30 years. We have enough books and papers talking about it. Today, people are still making mistakes. Why? Because C language is not type safe, there is NO enforcement. > That is why many people like other type safe language. If you are trying to make mistake, the compiler will tell you that you are making mistake. This will sound more convincing once we have Rust (or something similar) in a mainstream OS kernel or mainstream firmware. > 2) Take resource leak as an example. The programming language invented garbage collection. The operating system auto cleaned up application resource after execution. Same comment as above. I think garbage collection is frequently considered too opaque for low-level applications (unpredictable performance and RAM penalties etc). > 3) People has wrong check in which may break the system. What is why the world invented smoke test and continuous integration. Yes, I agree. > *Those are where the inventions come*, to treat human as human being, to prevent people making mistake or teach them automatically. :-) Thanks, I'm grateful that you use the word "teach" here. I'll reference it below. > I agree with the "mythical man-month" that there is no silver bulletin. > I tend to agree with you on the attitude. > I am trying to figure if we can do better to help other people (maintainer or contributor). If we really really can do nothing, that is OK. > I am not sure if is a best way to resolve the problem to just complain in the email. "Just complaining in email" is in fact my attempt to "teach" people. Not automatically, but by pointing out examples that I consider good. Automatisms are already in place for broadcasting good practices. Bugzilla actions and github actions are propagated via email. People just need to be receptive and look at the list traffic. I've contributed to Wiki articles. But asking for more documentation and more automatisms is just too convenient an excuse. People can and *should* learn by example, especially if they're seasoned in a project (not newcomers). Asking for more documentation and more automatisms puts the burden on people *different* from those that need to improve their discipline. It basically means, "I refuse to improve my behavior until *you* find the time to implement the tools and documentations for me to improve my behavior". Similarly, "I refuse to handle errors until you give me exceptions and destructors". I don't think it's fair to *demand* inventions. Because, I perceive the goals that I'm advocating for as widely-held values. If we accept them as such, then the burden should be on people that break those values, not on the advocates. (If, on the other hand, we do not have a consensus that these values are universal, that's OK as well: then I can start ignoring the information content in the PRs that *I* submit, and save myself significant amounts of time. See again: double standards.) > I think you can understand my point. I will stop here. Yes, I understand. My general response is that inventions are nice and they should be utilized if someone comes forward and offers them. On the other hand, demanding inventions (tooling, documentation etc), i.e., demanding that the *other* party spend the effort first, as a condition for changing our attitude, is not fair. Laszlo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-17 6:31 ` Laszlo Ersek 2020-09-17 7:31 ` Yao, Jiewen @ 2020-09-18 4:39 ` Ni, Ray 2020-09-18 7:37 ` Andrew Fish 1 sibling, 1 reply; 27+ messages in thread From: Ni, Ray @ 2020-09-18 4:39 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Yao, Jiewen, gaoliming@byosoft.com.cn, Dong, Guo Cc: marcello.bauer@9elements.com, Kinney, Michael D, 'Leif Lindholm (Nuvia address)', Doran, Mark, 'Andrew Fish', Guptha, Soumya K Laszlo, I support your idea of having a meaningful description for BZ, for commit message, for code comments. Thinking from 1 or 2 years from now, the simple message we created may help nothing to remind me or others why the changes were made. We cannot reply on people's memories and even the people that have the memories may left the community. So, documentation is necessary. But I remember that our development process document in WIKI doesn't require anything on the commit message perspective. IMO, at least the commit message should contain: * current status or reason(fail, lack of a feature, bad coding style) * impact of the change or result (fail to pass, feature enabled, coding style improved) Can someone help to emphasize the requirement in the WIKI? Thanks, Ray ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-18 4:39 ` Ni, Ray @ 2020-09-18 7:37 ` Andrew Fish 2020-09-26 0:34 ` Guo Dong 0 siblings, 1 reply; 27+ messages in thread From: Andrew Fish @ 2020-09-18 7:37 UTC (permalink / raw) To: edk2-devel-groups-io, Ni, Ray Cc: lersek@redhat.com, Yao, Jiewen, gaoliming@byosoft.com.cn, Dong, Guo, marcello.bauer@9elements.com, Mike Kinney, Leif Lindholm (Nuvia address), Mark Doran, Guptha, Soumya K Ray, Here is my take and I think this is what Laszlo’s comments don’t make clear. I agree with you that the rote process steps should be very clear, but I think what Laszlo is pointing out that best practices and current thinking evolve over time and are hard to capture in words. So I think our challenge is to step back and write process documentation that make it easy to get started., but also make it easy to get the feedback of what the community think is best practices. I kind of feel right now that our process documents are biased towards people who work on on the edk2 day to day, and that makes it hard to bring on new members. I think the path forward is making it easy for the developers to take the right steps to get started so the community can give them feed back. This feedback is ultimately the current thinking of the community, but we also need think about how to make it easy to get started. So how do we document the process steps, that make it easy for the community to offer comments on the changes? One of the ways we solved this kind of problem at Apple, is we make the person we just hired the owner of the getting started guide. I think the goal is how do we make it easy for a new contributor to get he value of the feedback and knowledge of the community of experts. But I think the solution is not to document everything we do, but to document the path of least resistance to join the community. And that is going to require people not in the community reviewing the documentation. We are too biased and can not do it by our selves. Thanks, Andrew Fish > On Sep 17, 2020, at 9:39 PM, Ni, Ray <ray.ni@intel.com> wrote: > > Laszlo, > I support your idea of having a meaningful description for BZ, for commit message, for code comments. > > Thinking from 1 or 2 years from now, the simple message we created may help nothing to remind me or others why the changes were made. > > We cannot reply on people's memories and even the people that have the memories may left the community. > > So, documentation is necessary. > > But I remember that our development process document in WIKI doesn't require anything on the commit message perspective. > > IMO, at least the commit message should contain: > * current status or reason(fail, lack of a feature, bad coding style) > * impact of the change or result (fail to pass, feature enabled, coding style improved) > > Can someone help to emphasize the requirement in the WIKI? > > Thanks, > Ray > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-18 7:37 ` Andrew Fish @ 2020-09-26 0:34 ` Guo Dong 2020-09-27 1:44 ` 回复: " gaoliming 2020-09-28 17:55 ` Laszlo Ersek 0 siblings, 2 replies; 27+ messages in thread From: Guo Dong @ 2020-09-26 0:34 UTC (permalink / raw) To: Andrew Fish, edk2-devel-groups-io, Ni, Ray Cc: lersek@redhat.com, Yao, Jiewen, gaoliming@byosoft.com.cn, marcello.bauer@9elements.com, Kinney, Michael D, Leif Lindholm (Nuvia address), Doran, Mark, Guptha, Soumya K Sorry to have a long email thread since my merge and thanks all for the comments. In general, I still feel current process is a little complicated for the maintainers who don't daily work on EDK2 like me. I have less than %5 of time spent on open source EDK2 UefiPayloadPkg since I focus on bootloaders. It would be great if I could spend the time mainly on code review instead of the process as of now. Even after I read https://github.com/tianocore/tianocore.github.io/wiki/EDK-II- Development-Process#the-maintainer-process-for-the-edk-ii-project as Liming pointed out, Some info is still not clear for me. E.g. what's the purpose for putting cover letter to patch set pull request (it looks we could not trace to this PR from code)? is it mandatory or optional? What if there is no cover letter in the patch set in patch #0 summary? For the patch I merged, I am still not very sure what info I should put there. I don't know why Laszlo mentioned BZ for my merge since there is no BZ mentioned in the patchset. And I also don't know why Laszlo mentioned to send email after the patch is merged since I don't find this requirement in the development process. I don't think it is doable to ask all the maintainers to monitor EDK2 mail list on how others are doing since there are so many emails every day, especially there is no any patch for UefiPayloadPkg for several months. I hope we could simplify the process and have a clear steps in the process soon. So that the maintainers could focus on the actual code review. Thanks, Guo ^ permalink raw reply [flat|nested] 27+ messages in thread
* 回复: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-26 0:34 ` Guo Dong @ 2020-09-27 1:44 ` gaoliming 2020-09-27 17:29 ` Guo Dong 2020-09-28 17:55 ` Laszlo Ersek 1 sibling, 1 reply; 27+ messages in thread From: gaoliming @ 2020-09-27 1:44 UTC (permalink / raw) To: 'Dong, Guo', 'Andrew Fish', 'edk2-devel-groups-io', 'Ni, Ray' Cc: lersek, 'Yao, Jiewen', marcello.bauer, 'Kinney, Michael D', 'Leif Lindholm (Nuvia address)', 'Doran, Mark', 'Guptha, Soumya K' Guo: I add my comments. Thanks Liming > -----邮件原件----- > 发件人: Dong, Guo <guo.dong@intel.com> > 发送时间: 2020年9月26日 8:35 > 收件人: Andrew Fish <afish@apple.com>; edk2-devel-groups-io > <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com> > 抄送: lersek@redhat.com; Yao, Jiewen <jiewen.yao@intel.com>; > gaoliming@byosoft.com.cn; marcello.bauer@9elements.com; Kinney, > Michael D <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Guptha, > Soumya K <soumya.k.guptha@intel.com> > 主题: RE: [edk2-devel] more development process failure [was: > UefiPayloadPkg: Runtime MMCONF] > > > Sorry to have a long email thread since my merge and thanks all for the > comments. > In general, I still feel current process is a little complicated for the maintainers > who don't > daily work on EDK2 like me. I have less than %5 of time spent on open > source EDK2 > UefiPayloadPkg since I focus on bootloaders. It would be great if I could > spend the time > mainly on code review instead of the process as of now. [Liming] I understand your case. > > Even after I read > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II- > Development-Process#the-maintainer-process-for-the-edk-ii-project as > Liming pointed out, > Some info is still not clear for me. E.g. what's the purpose for putting cover > letter to patch > set pull request (it looks we could not trace to this PR from code)? is it > mandatory or optional? > What if there is no cover letter in the patch set in patch #0 summary? For the > patch I merged, > I am still not very sure what info I should put there. [Liming] The purpose is to relate pull request and BZ. The people can review pull request to get all information. If the patch set includes more than one patch, it will need cover letter. If the patch set includes only one patch, its patch title and description will auto be inserted into the created pull request. No cover letter is required. > > I don't know why Laszlo mentioned BZ for my merge since there is no BZ > mentioned in the patchset. > And I also don't know why Laszlo mentioned to send email after the patch is > merged since I don't find this > requirement in the development process. I don't think it is doable to ask all > the maintainers to monitor EDK2 > mail list on how others are doing since there are so many emails every day, > especially there is no any patch > for UefiPayloadPkg for several months. [Liming] The maintainer sends the email with the merged commit message to the mail list so that the patch contributor knows that his patch has been merged. > > I hope we could simplify the process and have a clear steps in the process > soon. So that the maintainers could > focus on the actual code review. [Liming] I would suggest to highlight the role of maintainer and reviewer. The reviewer should take same roles to the maintainer except for the patch merge. So, you prefer you take reviewer role only. Thanks Liming > > Thanks, > Guo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-27 1:44 ` 回复: " gaoliming @ 2020-09-27 17:29 ` Guo Dong 0 siblings, 0 replies; 27+ messages in thread From: Guo Dong @ 2020-09-27 17:29 UTC (permalink / raw) To: devel@edk2.groups.io, gaoliming@byosoft.com.cn, 'Andrew Fish', Ni, Ray Cc: lersek@redhat.com, Yao, Jiewen, marcello.bauer@9elements.com, Kinney, Michael D, 'Leif Lindholm (Nuvia address)', Doran, Mark, Guptha, Soumya K Thanks Liming for the comments. Added my replies. /Guo > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming > Sent: Saturday, September 26, 2020 6:45 PM > To: Dong, Guo <guo.dong@intel.com>; 'Andrew Fish' <afish@apple.com>; > 'edk2-devel-groups-io' <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com> > Cc: lersek@redhat.com; Yao, Jiewen <jiewen.yao@intel.com>; > marcello.bauer@9elements.com; Kinney, Michael D > <michael.d.kinney@intel.com>; 'Leif Lindholm (Nuvia address)' > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Guptha, Soumya > K <soumya.k.guptha@intel.com> > Subject: 回复: [edk2-devel] more development process failure [was: > UefiPayloadPkg: Runtime MMCONF] > > Guo: > I add my comments. > > Thanks > Liming > > -----邮件原件----- > > 发件人: Dong, Guo <guo.dong@intel.com> > > 发送时间: 2020年9月26日 8:35 > > 收件人: Andrew Fish <afish@apple.com>; edk2-devel-groups-io > > <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com> > > 抄送: lersek@redhat.com; Yao, Jiewen <jiewen.yao@intel.com>; > > gaoliming@byosoft.com.cn; marcello.bauer@9elements.com; Kinney, > > Michael D <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address) > > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Guptha, > > Soumya K <soumya.k.guptha@intel.com> > > 主题: RE: [edk2-devel] more development process failure [was: > > UefiPayloadPkg: Runtime MMCONF] > > > > > > Sorry to have a long email thread since my merge and thanks all for the > > comments. > > In general, I still feel current process is a little complicated for the > maintainers > > who don't > > daily work on EDK2 like me. I have less than %5 of time spent on open > > source EDK2 > > UefiPayloadPkg since I focus on bootloaders. It would be great if I could > > spend the time > > mainly on code review instead of the process as of now. > > [Liming] I understand your case. > > > > > Even after I read > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II- > > Development-Process#the-maintainer-process-for-the-edk-ii-project as > > Liming pointed out, > > Some info is still not clear for me. E.g. what's the purpose for putting cover > > letter to patch > > set pull request (it looks we could not trace to this PR from code)? is it > > mandatory or optional? > > What if there is no cover letter in the patch set in patch #0 summary? For the > > patch I merged, > > I am still not very sure what info I should put there. > > [Liming] The purpose is to relate pull request and BZ. The people can review > pull request > to get all information. If the patch set includes more than one patch, it will > need cover letter. > If the patch set includes only one patch, its patch title and description will auto > be inserted into > the created pull request. No cover letter is required. > It makes sense to add descriptions in pull request if there is BZ as the process required. But the patch I merged doesn’t have a related BZ. There is no point to add a description in pull request based on this purpose. > > > > I don't know why Laszlo mentioned BZ for my merge since there is no BZ > > mentioned in the patchset. > > And I also don't know why Laszlo mentioned to send email after the patch is > > merged since I don't find this > > requirement in the development process. I don't think it is doable to ask all > > the maintainers to monitor EDK2 > > mail list on how others are doing since there are so many emails every day, > > especially there is no any patch > > for UefiPayloadPkg for several months. > > [Liming] The maintainer sends the email with the merged commit message to > the > mail list so that the patch contributor knows that his patch has been merged. > Understand your point. But from the process page you shared, there is no such rule there. and it also mentioned "Email notifications for pull requests, pushes, and check status results are enabled by watching the EDK II repository". So it looks the process prefer user to "watch the EDKII repository" if they want to know if a patch is merged or not. > > > > I hope we could simplify the process and have a clear steps in the process > > soon. So that the maintainers could > > focus on the actual code review. > > [Liming] I would suggest to highlight the role of maintainer and reviewer. > The reviewer should take same roles to the maintainer except for the patch > merge. > So, you prefer you take reviewer role only. > I just express my expectation in the process in future to save effort for maintainers. I could help to merge the simple patches for UefiPayloadPkg if I have time. > Thanks > Liming > > > > Thanks, > > Guo > > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-26 0:34 ` Guo Dong 2020-09-27 1:44 ` 回复: " gaoliming @ 2020-09-28 17:55 ` Laszlo Ersek 2020-09-29 4:13 ` Guo Dong 1 sibling, 1 reply; 27+ messages in thread From: Laszlo Ersek @ 2020-09-28 17:55 UTC (permalink / raw) To: Dong, Guo, Andrew Fish, edk2-devel-groups-io, Ni, Ray Cc: Yao, Jiewen, gaoliming@byosoft.com.cn, marcello.bauer@9elements.com, Kinney, Michael D, Leif Lindholm (Nuvia address), Doran, Mark, Guptha, Soumya K On 09/26/20 02:34, Dong, Guo wrote: > > Sorry to have a long email thread since my merge and thanks all for > the comments. > In general, I still feel current process is a little complicated for > the maintainers who don't daily work on EDK2 like me. I have less > than %5 of time spent on open source EDK2 UefiPayloadPkg since I focus > on bootloaders. It would be great if I could spend the time mainly on > code review instead of the process as of now. I think this is a 100% reasonable request; it would mean that your "M" role should be replaced with an "R" role under "UefiPayloadPkg", and then your co-maintainers (Maurice and Benjamin) would be responsible for pushing the patches that you review. > > Even after I read > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project > as Liming pointed out, Some info is still not clear for me. E.g. > what's the purpose for putting cover letter to patch set pull request > (it looks we could not trace to this PR from code)? is it mandatory or > optional? There are two questions to consider here, actually. I do not insist that the PRs have sensible descriptions, at this stage. However, if *some* maintainers are expected to populate the PRs with sensible descriptions, then *all* of them should. So the reason I'm asking you to add sensible descriptions to PRs, at this stage, is not because I'm 100% committed to duplicating information there. Instead, the reason is that Mike has asked me to do so, and therefore you (and everyone else) should do so as well. Alternatively (a perfectly valid alternative), we should remove this PR description requirement for everybody. That works too. > What if there is no cover letter in the patch set in patch #0 summary? That's generally (not always though!) a bad sign in itself. Either the cover letter of the patch set, or the bugzilla report, should contain a good, relatively high-level description of the issue (or feature), and the changes implemented to address it. At this point, *that* description should be copied into the PR. If *neither* the BZ ticket *nor* the patch series cover letter contains this kind of summary / overview, then *that* is a big problem, and should be remedied. > For the patch I merged, > I am still not very sure what info I should put there. The cover letter [edk2-devel] [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF seems to say that the patch set adds support for "arbitrary platforms with different or even no MMCONF space" to UefiPayloadPkg. Additionally, it fixes a crash on platforms not exposing 256 buses. That's the info. > > I don't know why Laszlo mentioned BZ for my merge since there is no BZ > mentioned in the patchset. I finished with the following paragraph: "(Please note also that I absolutely don't need a BZ for every contribution. My request is only that *if* there is a BZ, then handle it thoroughly.)" I discussed BZs in general. > And I also don't know why Laszlo mentioned to send email after the > patch is merged since I don't find this requirement in the development > process. How else is a contributor supposed learn of their patch series being merged? Are they supposed to pull the master twice daily, and hope that their patches show up eventually? I mean, the patches you merge originate from the list. Where else is the best place to report back to the submitter (and to the rest of the community) than under the original patch thread? > I don't think it is doable to ask all the maintainers to monitor EDK2 > mail list on how others are doing since there are so many emails every > day, especially there is no any patch for UefiPayloadPkg for several > months. I strongly disagree. If you are listed as a maintainer, that implies you *care* what happens in the community. If you don't (or cannot) care about workflow, then the "M" role is not a good fit for you. > I hope we could simplify the process and have a clear steps in the > process soon. So that the maintainers could focus on the actual code > review. Please see "Maintainers.txt": M: Package Maintainer: Cc address for patches and questions. Responsible for reviewing and pushing package changes to source control. If you are a Maintainer, that means you are responsible for pushing changes, and for parts of the workflow that come with that. It's a service to the community. If you don't care about that, then the "R" role is more appropriate: R: Package Reviewer: Cc address for patches and questions. Reviewers help maintainers review code, but don't have push access. A designated Package Reviewer is reasonably familiar with the Package (or some modules thereof), and/or provides testing or regression testing for the Package (or some modules thereof), in certain platforms and environments. Thanks Laszlo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-28 17:55 ` Laszlo Ersek @ 2020-09-29 4:13 ` Guo Dong 2020-09-29 11:59 ` Laszlo Ersek 0 siblings, 1 reply; 27+ messages in thread From: Guo Dong @ 2020-09-29 4:13 UTC (permalink / raw) To: Laszlo Ersek, Andrew Fish, edk2-devel-groups-io, Ni, Ray Cc: Yao, Jiewen, gaoliming@byosoft.com.cn, marcello.bauer@9elements.com, Kinney, Michael D, Leif Lindholm (Nuvia address), Doran, Mark, Guptha, Soumya K > On 09/26/20 02:34, Dong, Guo wrote: > > > > Sorry to have a long email thread since my merge and thanks all for > > the comments. > > In general, I still feel current process is a little complicated for > > the maintainers who don't daily work on EDK2 like me. I have less > > than %5 of time spent on open source EDK2 UefiPayloadPkg since I focus > > on bootloaders. It would be great if I could spend the time mainly on > > code review instead of the process as of now. > > I think this is a 100% reasonable request; it would mean that your "M" > role should be replaced with an "R" role under "UefiPayloadPkg", and > then your co-maintainers (Maurice and Benjamin) would be responsible for > pushing the patches that you review. > > > > > Even after I read > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development- > Process#the-maintainer-process-for-the-edk-ii-project > > as Liming pointed out, Some info is still not clear for me. E.g. > > what's the purpose for putting cover letter to patch set pull request > > (it looks we could not trace to this PR from code)? is it mandatory or > > optional? > > There are two questions to consider here, actually. > > I do not insist that the PRs have sensible descriptions, at this stage. > > However, if *some* maintainers are expected to populate the PRs with > sensible descriptions, then *all* of them should. > > So the reason I'm asking you to add sensible descriptions to PRs, at > this stage, is not because I'm 100% committed to duplicating information > there. Instead, the reason is that Mike has asked me to do so, and > therefore you (and everyone else) should do so as well. > From the EDK II Development Process Mike edited, there is no such requirement to add description for the patches that have no BZ. I think you should ask Mike to update the process so that every Maintainer could follow the same steps if Mike ever asked you to do it. It doesn't make sense to blame others using this kind of "hidden rule" if it is "really" required. > Alternatively (a perfectly valid alternative), we should remove this PR > description requirement for everybody. That works too. > > > What if there is no cover letter in the patch set in patch #0 summary? > > That's generally (not always though!) a bad sign in itself. Either the > cover letter of the patch set, or the bugzilla report, should contain a > good, relatively high-level description of the issue (or feature), and > the changes implemented to address it. At this point, *that* description > should be copied into the PR. > > If *neither* the BZ ticket *nor* the patch series cover letter contains > this kind of summary / overview, then *that* is a big problem, and > should be remedied. > > > For the patch I merged, > > I am still not very sure what info I should put there. > > The cover letter > > [edk2-devel] [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF > > seems to say that the patch set adds support for "arbitrary platforms > with different or even no MMCONF space" to UefiPayloadPkg. Additionally, > it fixes a crash on platforms not exposing 256 buses. > > That's the info. > > > > > I don't know why Laszlo mentioned BZ for my merge since there is no BZ > > mentioned in the patchset. > > I finished with the following paragraph: > > "(Please note also that I absolutely don't need a BZ for every > contribution. My request is only that *if* there is a BZ, then handle it > thoroughly.)" > > I discussed BZs in general. > > > And I also don't know why Laszlo mentioned to send email after the > > patch is merged since I don't find this requirement in the development > > process. > > How else is a contributor supposed learn of their patch series being > merged? > > Are they supposed to pull the master twice daily, and hope that their > patches show up eventually? > > I mean, the patches you merge originate from the list. Where else is the > best place to report back to the submitter (and to the rest of the > community) than under the original patch thread? > As I replied to Liming, the EDK II Development Process mentioned this " Email notifications for pull requests, pushes, and check status results are enabled by watching the EDK II repository (https://github.com/ tianocore/edk2). " So that you could get email notification if these status if you want. Again, you had better ask Mike to add this step in the process if you think it is required. > > I don't think it is doable to ask all the maintainers to monitor EDK2 > > mail list on how others are doing since there are so many emails every > > day, especially there is no any patch for UefiPayloadPkg for several > > months. > > I strongly disagree. If you are listed as a maintainer, that implies you > *care* what happens in the community. If you don't (or cannot) care > about workflow, then the "M" role is not a good fit for you. > I don't think we have requirements to ask maintainers to read all EDK2 emails or know all the things in the community from EDK II Development Process or from Maintainers.txt. Not sure where you get it. From my view point, the package "M" only need *care* the package they maintained following the "M" role defined by the EDK2 documents. It is good to care other packages if having time, but there is no such requirement. > > I hope we could simplify the process and have a clear steps in the > > process soon. So that the maintainers could focus on the actual code > > review. > > Please see "Maintainers.txt": > > M: Package Maintainer: Cc address for patches and questions. Responsible > for reviewing and pushing package changes to source control. > > If you are a Maintainer, that means you are responsible for pushing > changes, and for parts of the workflow that come with that. It's a > service to the community. If you don't care about that, then the "R" > role is more appropriate: > > R: Package Reviewer: Cc address for patches and questions. Reviewers help > maintainers review code, but don't have push access. A designated Package > Reviewer is reasonably familiar with the Package (or some modules > thereof), and/or provides testing or regression testing for the Package > (or some modules thereof), in certain platforms and environments. > That's what I am doing now to review and merge the patch following these edk2 defined requirements. > Thanks > Laszlo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-29 4:13 ` Guo Dong @ 2020-09-29 11:59 ` Laszlo Ersek 0 siblings, 0 replies; 27+ messages in thread From: Laszlo Ersek @ 2020-09-29 11:59 UTC (permalink / raw) To: devel, guo.dong, Andrew Fish, Ni, Ray, Kinney, Michael D Cc: Yao, Jiewen, gaoliming@byosoft.com.cn, marcello.bauer@9elements.com, Leif Lindholm (Nuvia address), Doran, Mark, Guptha, Soumya K On 09/29/20 06:13, Guo Dong wrote: > >> On 09/26/20 02:34, Dong, Guo wrote: >>> >>> Sorry to have a long email thread since my merge and thanks all for >>> the comments. >>> In general, I still feel current process is a little complicated for >>> the maintainers who don't daily work on EDK2 like me. I have less >>> than %5 of time spent on open source EDK2 UefiPayloadPkg since I focus >>> on bootloaders. It would be great if I could spend the time mainly on >>> code review instead of the process as of now. >> >> I think this is a 100% reasonable request; it would mean that your "M" >> role should be replaced with an "R" role under "UefiPayloadPkg", and >> then your co-maintainers (Maurice and Benjamin) would be responsible for >> pushing the patches that you review. >> >>> >>> Even after I read >>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development- >> Process#the-maintainer-process-for-the-edk-ii-project >>> as Liming pointed out, Some info is still not clear for me. E.g. >>> what's the purpose for putting cover letter to patch set pull request >>> (it looks we could not trace to this PR from code)? is it mandatory or >>> optional? >> >> There are two questions to consider here, actually. >> >> I do not insist that the PRs have sensible descriptions, at this stage. >> >> However, if *some* maintainers are expected to populate the PRs with >> sensible descriptions, then *all* of them should. >> >> So the reason I'm asking you to add sensible descriptions to PRs, at >> this stage, is not because I'm 100% committed to duplicating information >> there. Instead, the reason is that Mike has asked me to do so, and >> therefore you (and everyone else) should do so as well. >> > From the EDK II Development Process Mike edited, there is no such > requirement to add description for the patches that have no BZ. > I think you should ask Mike to update the process so that every > Maintainer could follow the same steps if Mike ever asked you to > do it. It doesn't make sense to blame others using this kind of > "hidden rule" if it is "really" required. Fine. Mike: can you please include the PR subject and description requirements to the "EDK II Development Process" article in the Wiki? Thank you. >> Alternatively (a perfectly valid alternative), we should remove this PR >> description requirement for everybody. That works too. >> >>> What if there is no cover letter in the patch set in patch #0 summary? >> >> That's generally (not always though!) a bad sign in itself. Either the >> cover letter of the patch set, or the bugzilla report, should contain a >> good, relatively high-level description of the issue (or feature), and >> the changes implemented to address it. At this point, *that* description >> should be copied into the PR. >> >> If *neither* the BZ ticket *nor* the patch series cover letter contains >> this kind of summary / overview, then *that* is a big problem, and >> should be remedied. >> >>> For the patch I merged, >>> I am still not very sure what info I should put there. >> >> The cover letter >> >> [edk2-devel] [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF >> >> seems to say that the patch set adds support for "arbitrary platforms >> with different or even no MMCONF space" to UefiPayloadPkg. Additionally, >> it fixes a crash on platforms not exposing 256 buses. >> >> That's the info. >> >>> >>> I don't know why Laszlo mentioned BZ for my merge since there is no BZ >>> mentioned in the patchset. >> >> I finished with the following paragraph: >> >> "(Please note also that I absolutely don't need a BZ for every >> contribution. My request is only that *if* there is a BZ, then handle it >> thoroughly.)" >> >> I discussed BZs in general. >> >>> And I also don't know why Laszlo mentioned to send email after the >>> patch is merged since I don't find this requirement in the development >>> process. >> >> How else is a contributor supposed learn of their patch series being >> merged? >> >> Are they supposed to pull the master twice daily, and hope that their >> patches show up eventually? >> >> I mean, the patches you merge originate from the list. Where else is the >> best place to report back to the submitter (and to the rest of the >> community) than under the original patch thread? >> > As I replied to Liming, the EDK II Development Process mentioned this " > Email notifications for pull requests, pushes, and check status results > are enabled by watching the EDK II repository (https://github.com/ > tianocore/edk2). " So that you could get email notification if these > status if you want. Counter-arguments: (1) The email that github itself generates about having merged a PR is pure trash. For example, it does not mention the new commit range that is the result of the merge. Consequently, it does not help people when they try to figure out later what patches they need to backport or evaluate for their downstream products. (2) The email that github sends is not threaded together with the original posting / patch discussion on the list. There is no connection between them. This is why it would be so important to copy sensible information (such as mailing list archive URLs) into the PR description, so that we have some sort of connection between the PR and the patch series thread. > Again, you had better ask Mike to add this step > in the process if you think it is required. OK. Mike, can you please document in the same Wiki article that maintainers should follow up with a final message in the patch series thread, once the PR is merged? Thanks. > >>> I don't think it is doable to ask all the maintainers to monitor EDK2 >>> mail list on how others are doing since there are so many emails every >>> day, especially there is no any patch for UefiPayloadPkg for several >>> months. >> >> I strongly disagree. If you are listed as a maintainer, that implies you >> *care* what happens in the community. If you don't (or cannot) care >> about workflow, then the "M" role is not a good fit for you. >> > I don't think we have requirements to ask maintainers to read all EDK2 > emails Agreed. > or know all the things in the community from EDK II > Development Process or from Maintainers.txt. I disagree. As a maintainer (a person with push access to the master branch, where your acts will affect *every* edk2 consumer), you are responsible for keeping an eye on workflow-related discussions. > Not sure where you get it. It's obvious. > From my view point, the package "M" only need *care* the package > they maintained following the "M" role defined by the EDK2 documents. The master branch (the git history) of the project is a shared resource. If UefiPayloadPkg were maintained in a different repository, you could follow whatever workflow you liked. "pushing package changes to source control" in Maintainers.txt means that a maintainer doesn't live in a bubble. You as a maintainer modify the master branch and thereby impact every edk2 consumer. And it's not merely a "functional impact"; it means things like "how readable and informative is this commit message to others -- including those that are not (yet) UefiPayloadPkg stake-holders". > It is good to care other packages if having time, but there is no such > requirement. The point is not to watch other packages; the point is to watch the workflow-related discussions. > >>> I hope we could simplify the process and have a clear steps in the >>> process soon. So that the maintainers could focus on the actual code >>> review. >> >> Please see "Maintainers.txt": >> >> M: Package Maintainer: Cc address for patches and questions. Responsible >> for reviewing and pushing package changes to source control. >> >> If you are a Maintainer, that means you are responsible for pushing >> changes, and for parts of the workflow that come with that. It's a >> service to the community. If you don't care about that, then the "R" >> role is more appropriate: >> >> R: Package Reviewer: Cc address for patches and questions. Reviewers help >> maintainers review code, but don't have push access. A designated Package >> Reviewer is reasonably familiar with the Package (or some modules >> thereof), and/or provides testing or regression testing for the Package >> (or some modules thereof), in certain platforms and environments. >> > That's what I am doing now to review and merge the patch following these edk2 > defined requirements. I don't understand. You are responding to the "R" role description with "that's what I'm doing", and then you say "review and merge". That doesn't add up. Merging does *not* belong to the "R" role. Merging belongs to the "M" role, but the "M" role requires you to pay more attention to the list than you seem to prefer to. Laszlo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: 回复: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-17 1:49 ` 回复: " gaoliming 2020-09-17 2:31 ` Yao, Jiewen @ 2020-09-17 5:56 ` Laszlo Ersek 1 sibling, 0 replies; 27+ messages in thread From: Laszlo Ersek @ 2020-09-17 5:56 UTC (permalink / raw) To: gaoliming, devel, 'Dong, Guo' Cc: marcello.bauer, 'Kinney, Michael D', 'Leif Lindholm (Nuvia address)', 'Doran, Mark', 'Andrew Fish', 'Guptha, Soumya K' Liming, On 09/17/20 03:49, gaoliming wrote: > Guo: > On pull request, https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project section 7 gives the requirement. > If <new-integration-branch> is a patch series, then copy the patch #0 summary into the pull request description. > > Laszlo: > I think we can enhance wiki page https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project to add another step to reply the patch mail with the merged pull request or commit after PR is merged. this document is usually edited by Mike, so I'd prefer Mike to add such a step. But... I've given this some more thought. I don't feel that the argument "it's not documented" is a completely fair excuse, from long-time maintainers. It's not like we invent a new workflow element every two weeks. I think it's fair to expect that maintainers learn new things by doing it and by watching others do it. No particular maintainer lives and works in a bubble. If they merge a series, close a BZ, report back on the list, populate a PR with useful information -- all those steps are *visible* to everyone else. And other long-term maintainers could simply start copying good practices they see. I'm not talking about closely following other maintainers' work, just acknowledging / not completely ignoring their activitities. Laszlo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-16 8:56 ` more development process failure [was: UefiPayloadPkg: Runtime MMCONF] Laszlo Ersek 2020-09-16 17:30 ` [edk2-devel] " Guo Dong @ 2020-09-21 9:57 ` Marcello Sylvester Bauer 2020-09-22 6:45 ` Laszlo Ersek 1 sibling, 1 reply; 27+ messages in thread From: Marcello Sylvester Bauer @ 2020-09-21 9:57 UTC (permalink / raw) To: devel, lersek Cc: Yao, Jiewen, gaoliming@byosoft.com.cn, Dong, Guo, Mike Kinney, Leif Lindholm (Nuvia address), Mark Doran, Guptha, Soumya K [-- Attachment #1: Type: text/plain, Size: 1984 bytes --] Dear Laszlo, I'm really sorry for violating the development process. Reading the email thread that follows made me realize that I have to work on my submission work follow before creating more patch series. I'm looking forward to seeing this process moving completely to GitHub. No empty description of mine will appear in the future. PS: We are currently working on a CI integration for testing UefiPayloadPkg functionality on real hardware. It is still WIP but this could be part of the testing process in future. thanks, Marcello On Wed, Sep 16, 2020 at 10:57 AM Laszlo Ersek <lersek@redhat.com> wrote: > Guo, > > On 08/18/20 10:24, Marcello Sylvester Bauer wrote: > > Support arbitrary platforms with different or even no MMCONF space. > > Fixes crash on platforms not exposing 256 buses. > > > > Tested on: > > * AMD Stoney Ridge > > > > Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg-MMCONF > > PR: https://github.com/tianocore/edk2/pull/885 > > > > v5: > > * MdePkg > > - support variable size MMCONF in all PciExpressLibs > > - use (UINTX)-1 as return values for invalid Pci addresses > > Okay, so we got more of the same development process violations here, as > I've just reported at <https://edk2.groups.io/g/devel/message/65313>. > > See this new pull request: > > https://github.com/tianocore/edk2/pull/932/ > > "No description provided." > > You should be embarrassed. > > Laszlo > > > > > > > -- *[Marcello Sylvester Bauer]* 9elements Agency GmbH, Kortumstraße 19-21, 44787 Bochum, Germany Email: [DEINE EMAIL ADDRESSE] <https://static.9elements.com/email_signatur.html> 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 <https://9elements.com/privacy> [-- Attachment #2: Type: text/html, Size: 4785 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] 2020-09-21 9:57 ` Marcello Sylvester Bauer @ 2020-09-22 6:45 ` Laszlo Ersek 0 siblings, 0 replies; 27+ messages in thread From: Laszlo Ersek @ 2020-09-22 6:45 UTC (permalink / raw) To: Marcello Sylvester Bauer, devel Cc: Yao, Jiewen, gaoliming@byosoft.com.cn, Dong, Guo, Mike Kinney, Leif Lindholm (Nuvia address), Mark Doran, Guptha, Soumya K Hi Marcello, On 09/21/20 11:57, Marcello Sylvester Bauer wrote: > Dear Laszlo, > > I'm really sorry for violating the development process. my criticism was targeted solely at edk2 maintainers, not at contributors. As far as I'm aware, you did nothing wrong. I kept you CC'd because it's bad netiquette to drop thread participants mid-thread. I'm sorry for confusing you. Thanks Laszlo ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-09-29 12:00 UTC | newest] Thread overview: 27+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-18 8:24 [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF Marcello Sylvester Bauer 2020-08-18 8:24 ` [PATCH v5 1/3] UefiPayloadPkg: Store the size of the MMCONF window Marcello Sylvester Bauer 2020-08-18 8:24 ` [PATCH v5 2/3] MdePkg: PciExpressLib support variable size MMCONF Marcello Sylvester Bauer 2020-08-20 10:55 ` Liming Gao 2020-08-18 8:24 ` [PATCH v5 3/3] UefiPayloadPkg: Support variable size MMCONF space Marcello Sylvester Bauer 2020-09-08 22:35 ` [edk2-devel] " Guo Dong 2020-09-16 8:56 ` more development process failure [was: UefiPayloadPkg: Runtime MMCONF] Laszlo Ersek 2020-09-16 17:30 ` [edk2-devel] " Guo Dong 2020-09-16 18:14 ` Laszlo Ersek 2020-09-16 21:51 ` Guo Dong 2020-09-17 5:59 ` Laszlo Ersek 2020-09-17 1:49 ` 回复: " gaoliming 2020-09-17 2:31 ` Yao, Jiewen 2020-09-17 6:31 ` Laszlo Ersek 2020-09-17 7:31 ` Yao, Jiewen 2020-09-17 10:26 ` Laszlo Ersek 2020-09-18 4:39 ` Ni, Ray 2020-09-18 7:37 ` Andrew Fish 2020-09-26 0:34 ` Guo Dong 2020-09-27 1:44 ` 回复: " gaoliming 2020-09-27 17:29 ` Guo Dong 2020-09-28 17:55 ` Laszlo Ersek 2020-09-29 4:13 ` Guo Dong 2020-09-29 11:59 ` Laszlo Ersek 2020-09-17 5:56 ` 回复: " Laszlo Ersek 2020-09-21 9:57 ` Marcello Sylvester Bauer 2020-09-22 6:45 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox