On 10/9/23 02:07, Taylor Beebe wrote:
> HOB memory should not be written to in DXE phase. This patch
> copies the PCD database from PEI into a new buffer so updates
> to dynamic PCDs don't write to HOB memory.
>
> Signed-off-by: Taylor Beebe <taylor.d.be...@gmail.com>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>
> Cc: Dandan Bi <dandan...@intel.com>
> ---
> MdeModulePkg/Universal/PCD/Dxe/Service.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Service.c
> b/MdeModulePkg/Universal/PCD/Dxe/Service.c
> index 1ae06a639c43..0feb11142545 100644
> --- a/MdeModulePkg/Universal/PCD/Dxe/Service.c
> +++ b/MdeModulePkg/Universal/PCD/Dxe/Service.c
> @@ -885,15 +885,17 @@ BuildPcdDxeDataBase (
> // be NULL. If it is NULL, we just copy over the DXE Default
> // Value to PCD Database.
> //
> - PeiDatabase = (PEI_PCD_DATABASE *)GET_GUID_HOB_DATA (GuidHob);
> + PeiDatabase = AllocateCopyPool ((UINTN)GET_GUID_HOB_DATA_SIZE (GuidHob),
> GET_GUID_HOB_DATA (GuidHob));
> + ASSERT (PeiDatabase != NULL);
>
> //
> // Get next one that stores full PEI data
> //
> GuidHob = GetNextGuidHob (&gPcdDataBaseHobGuid, GET_NEXT_HOB (GuidHob));
> if (GuidHob != NULL) {
> - mPeiPcdDbBinary = (PEI_PCD_DATABASE *)GET_GUID_HOB_DATA (GuidHob);
> mPeiPcdDbSize = (UINTN)GET_GUID_HOB_DATA_SIZE (GuidHob);
> + mPeiPcdDbBinary = (PEI_PCD_DATABASE *)AllocateCopyPool (mPeiPcdDbSize,
> GET_GUID_HOB_DATA (GuidHob));
> + ASSERT (mPeiPcdDbBinary != NULL);
> }
>
> //
I'm only skimming the MdeModulePkg patches to get a feel for them, but
this ASSERT() catches my eye. Using ASSERT()s for error checking is an
ani-pattern. I realize that BuildPcdDxeDataBase() cannot return a value,
and also that it already contains two instances of this anti-pattern:
mPcdDatabase.PeiDb = AllocateZeroPool (sizeof (PEI_PCD_DATABASE));
ASSERT (mPcdDatabase.PeiDb != NULL);
mCallbackFnTable = AllocateZeroPool (mPcdTotalTokenCount * sizeof
(LIST_ENTRY));
ASSERT (mCallbackFnTable != NULL);
but at least please don't add more of it. The right way to deal with
this is:
mPeiPcdDbBinary = AllocateCopyPool (...);
if (mPeiPcdDbBinary == NULL) {
DEBUG ((DEBUG_ERROR, "%a: ...\n", __func__));
ASSERT (FALSE);
CpuDeadLoop ();
}
This way:
- you'll get an informative error message in DEBUG/NOOPT builds,
- you'll get a source file name and line number,
- via ASSERT -> DebugAssert -> PcdDebugPropertyMask, you'll see either
CpuBreakpoint() or CpuDeadLoop() (or nothing) called, dependent on
platform choice,
- regardless of platform choice and build target (DEBUG/NOOPT/RELEASE),
execution will not progress past the explicit CpuDeadLoop().
(This approach was originally suggested by Mike Kinney to me.)
Plain ASSERT() for error checking is a bad pattern in the edk2 core;
please avoid it if you can.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109435): https://edk2.groups.io/g/devel/message/109435
Mute This Topic: https://groups.io/mt/101843346/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
