Re: [edk2-devel] [PATCH v2 2/7] MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 6 Jun 2023 at 20:48, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> On 6/2/2023 11:17 AM, Ard Biesheuvel wrote:
> > The Risc-V and LoongArch specific versions of the DXE core handoff code
> > in DxeIpl are essentially copies of the EBC version (modulo the
> > copyright in the header and some debug prints in the code).
> >
> > In preparation for introducing a generic PPI based method to implement
> > the non-executable stack, let's merge these versions, so we only need to
> > add this logic once.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} |  2 +-
> >   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                          | 10 +--
> >   MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c           | 63 ----------------
> >   MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c               | 75 --------------------
> >   4 files changed, 3 insertions(+), 147 deletions(-)
> >
...
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
> > deleted file mode 100644
> > index 95d3af19ea4c9f00..0000000000000000
> > --- a/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
> > +++ /dev/null
> > @@ -1,63 +0,0 @@
> > -/** @file
> >
> > -  LoongArch specifc functionality for DxeLoad.
> >
> > -
> >
> > -  Copyright (c) 2022, Loongson Technology Corporation Limited. All rights reserved.<BR>
> >
> > -
> >
> > -  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > -
> >
> > -**/
> >
> > -
> >
> > -#include "DxeIpl.h"
> >
> > -
> >
> > -/**
> >
> > -   Transfers control to DxeCore.
> >
> > -
> >
> > -   This function performs a CPU architecture specific operations to execute
> >
> > -   the entry point of DxeCore with the parameters of HobList.
> >
> > -   It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.
> >
> > -
> >
> > -   @param[in] DxeCoreEntryPoint         The entry point of DxeCore.
> >
> > -   @param[in] HobList                   The start of HobList passed to DxeCore.
> >
> > -
> >
> > -**/
> >
> > -VOID
> >
> > -HandOffToDxeCore (
> >
> > -  IN EFI_PHYSICAL_ADDRESS  DxeCoreEntryPoint,
> >
> > -  IN EFI_PEI_HOB_POINTERS  HobList
> >
> > -  )
> >
> > -{
> >
> > -  VOID        *BaseOfStack;
> >
> > -  VOID        *TopOfStack;
> >
> > -  EFI_STATUS  Status;
> >
> > -
> >
> > -  //
> >
> > -  // Allocate 128KB for the Stack
> >
> > -  //
> >
> > -  BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
> >
> > -  ASSERT (BaseOfStack != NULL);
> >
> > -
>
> I know this code path is critical to continue boot, but dereferencing a
> null pointer if asserts are not active is not a great alternative.
>
> Perhaps a status error code could be reported to allow platforms to
> potentially hook onto? Kind of like what is done if the DXE IPL PPI
> cannot be found
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c#LL518-L522.
>

I understand your point, but you are responding to a patch that
removes these lines.

All architectures currently implement basically the same logic here,
with the exception of IA32, which does

Status = PeiServicesAllocatePages (EfiBootServicesData,
EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack);
ASSERT_EFI_ERROR (Status);

and so the only difference is that it dereferences a bogus pointer
instead of a NULL pointer on failure. (RISC-V doesn't ASSERT() so it
will happily carry on even in DEBUG mode)

So I propose we log this as a todo item for after we've unified all
these implementations across architectures.