Re: [edk2-devel] [PATCH v5 05/28] MdeModulePkg: Copy PEI PCD Database Into New Buffer

["Laszlo Ersek" <lersek@redhat.com>]

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.beebe@gmail.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Dandan Bi <dandan.bi@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-