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.web11.1279.1686092832685563437 for ; Tue, 06 Jun 2023 16:07:12 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=OSz/Wcyj; 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 ABFD720BE499; Tue, 6 Jun 2023 16:07:10 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com ABFD720BE499 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1686092832; bh=kWkxAd4uhcRBvWWya5+ZSm7BJsmUqly6DpsSwz50p7c=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=OSz/WcyjK1+CBalWN+S6hau30BeKy/Dy/5U2MvnjftzDbSF6SVV6h/EMI7h8RjgIM zn/d0mbj2/lc7bOiEx1+mfC3PIRSeRlmEmy+A/TvsxMDgSz8PSSjL4ko9pd2368LDG cWAWftG5Ih+6lFUqH+HvSbO4UJPDpk2wFpTLXzRU= Message-ID: <73b9ba5a-dab4-3883-c645-8ba943562b96@linux.microsoft.com> Date: Tue, 6 Jun 2023 19:07:09 -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: Ard Biesheuvel Cc: devel@edk2.groups.io, 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: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 6/6/2023 6:17 PM, Ard Biesheuvel wrote: > On Tue, 6 Jun 2023 at 20:48, Michael Kubacki > 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 >>> --- >>> 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.
>>> >>> - >>> >>> - 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.