On Tue, 6 Jun 2023 at 20:48, Michael Kubacki
<mikub...@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 <a...@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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105822): https://edk2.groups.io/g/devel/message/105822
Mute This Topic: https://groups.io/mt/99288479/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
