From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.4921.1686077320495124687 for ; Tue, 06 Jun 2023 11:48:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=I9hhd0xK; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id AED3520BE48E; Tue, 6 Jun 2023 11:48:38 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com AED3520BE48E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1686077320; bh=2/H1cAuKg8aeNrx5Dtxix1trV9fAqFPykoUh2OrtVlc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=I9hhd0xKJNNfU04jKaZ971g+8VU4Q+TwHnzAWuGWMXmK7F4WPkMsKT+ppY0EloKAQ ct5yCXbr3SfJuXTqGq3CGU8eTKpPxMe4+bwJKki/bNPRmsii17vcrwH9jAekA47a+a M62Cxmvhu/6AMe3u4hQjqVAl+zb/fORbLKDm7VRI= Message-ID: Date: Tue, 6 Jun 2023 14:48:37 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.2 Subject: Re: [edk2-devel] [PATCH v2 2/7] MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code To: devel@edk2.groups.io, ardb@kernel.org Cc: Ray Ni , Jiewen Yao , Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , Dandan Bi , Dun Tan , Liming Gao , "Kinney, Michael D" , Leif Lindholm References: <20230602151739.3600820-1-ardb@kernel.org> <20230602151739.3600820-3-ardb@kernel.org> From: "Michael Kubacki" In-Reply-To: <20230602151739.3600820-3-ardb@kernel.org> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > --- > 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/Ebc/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c > similarity index 92% > rename from MdeModulePkg/Core/DxeIplPeim/Ebc/DxeLoadFunc.c > rename to MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c > index c1a16b602452218e..a0f85ebea56e6cba 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/Ebc/DxeLoadFunc.c > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c > @@ -1,5 +1,5 @@ > /** @file > > - EBC-specific functionality for DxeLoad. > > + Generic version of arch-specific functionality for DxeLoad. > > > > Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > index 052ea0ec1a6f2771..60c998be6c1bad01 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > @@ -45,17 +45,11 @@ [Sources.X64] > X64/VirtualMemory.c > > X64/DxeLoadFunc.c > > > > -[Sources.EBC] > > - Ebc/DxeLoadFunc.c > > - > > [Sources.ARM, Sources.AARCH64] > > Arm/DxeLoadFunc.c > > > > -[Sources.RISCV64] > > - RiscV64/DxeLoadFunc.c > > - > > -[Sources.LOONGARCH64] > > - LoongArch64/DxeLoadFunc.c > > +[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC] > > + DxeHandoff.c > > > > [Packages] > > MdePkg/MdePkg.dec > > 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.
> > - > > - 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. > > - // > > - // Compute the top of the stack we were allocated. Pre-allocate a UINTN > > - // for safety. > > - // > > - TopOfStack = (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SIZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT); > > - TopOfStack = ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT); > > - > > - // > > - // End of PEI phase signal > > - // > > - Status = PeiServicesInstallPpi (&gEndOfPeiSignalPpi); > > - ASSERT_EFI_ERROR (Status); > > - > > - // > > - // Update the contents of BSP stack HOB to reflect the real stack info passed to DxeCore. > > - // > > - UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE); > > - > > - SwitchStack ( > > - (SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint, > > - HobList.Raw, > > - NULL, > > - TopOfStack > > - ); > > -} > > diff --git a/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c > deleted file mode 100644 > index b3567d88f73467e7..0000000000000000 > --- a/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c > +++ /dev/null > @@ -1,75 +0,0 @@ > -/** @file > > - RISC-V specific functionality for DxeLoad. > > - > > - Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.
> > - > > - 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 DxeCoreEntryPoint The entry point of DxeCore. > > - @param 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)); > > - if (BaseOfStack == NULL) { > > - DEBUG ((DEBUG_ERROR, "%a: Can't allocate memory for stack.", __func__)); > > - ASSERT (FALSE); > > - } > > - > > - // > > - // Compute the top of the stack we were allocated. Pre-allocate a UINTN > > - // for safety. > > - // > > - TopOfStack = (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SIZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT); > > - TopOfStack = ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT); > > - > > - // > > - // End of PEI phase signal > > - // > > - Status = PeiServicesInstallPpi (&gEndOfPeiSignalPpi); > > - if (EFI_ERROR (Status)) { > > - DEBUG ((DEBUG_ERROR, "%a: Fail to signal End of PEI event.", __func__)); > > - ASSERT (FALSE); > > - } > > - > > - // > > - // Update the contents of BSP stack HOB to reflect the real stack info passed to DxeCore. > > - // > > - UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE); > > - > > - DEBUG ((DEBUG_INFO, "DXE Core new stack at %x, stack pointer at %x\n", BaseOfStack, TopOfStack)); > > - > > - // > > - // Transfer the control to the entry point of DxeCore. > > - // > > - SwitchStack ( > > - (SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint, > > - HobList.Raw, > > - NULL, > > - TopOfStack > > - ); > > -} >