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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

On 6/6/2023 6:17 PM, Ard Biesheuvel wrote:
> 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.

Sorry, I got this mixed up with another patch I was looking at on 
GitHub. That sounds good.