From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.331.1686089876711404917 for ; Tue, 06 Jun 2023 15:17:56 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=mNw7uIBu; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 2D0EE63821 for ; Tue, 6 Jun 2023 22:17:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8E1E5C433A0 for ; Tue, 6 Jun 2023 22:17:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1686089875; bh=DHD22++VPvKEtfdqx26JOsUkmiBPTzqf85fPThj83qI=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=mNw7uIBuJLTFp3uqisiCtttTGRfXtvF7C4D+VFsq5wfIpMKLUY0COtNgAY8anBI4t rNMCxmQsXiO4d0mDsO29dN/7PZXZ76lQVYYmpgNsP/MTVzNS76Acg+UfgRLt9AC6tN /u7t4w1JBmyk5+v4CNSJdwUSSd379W6UNX9OxXQfEDzAIQRrTzQJzevPU40U+l5uT/ 2wHiF19CIsdkCgqpglkMzpA8wYh8Wfenqhk26DOyfTo1a+l7D2hgf0hGKO1yW91sAL j6v2j+cNTVhldc2EcG5b0u4649N/caLAo+y8m+1VeWPvE/PuyPnotpA/A8ChU8quDe iPj8L68IS6CJQ== Received: by mail-lf1-f52.google.com with SMTP id 2adb3069b0e04-4f6148f9679so6066747e87.3 for ; Tue, 06 Jun 2023 15:17:55 -0700 (PDT) X-Gm-Message-State: AC+VfDwrF+THQErbs3m1RM3D8wxRNkWY083hL92nTG8sNwY45Lo3Pyjr AVJT/OXrRqmVNhiyTKeHVGn9DfsmRFao8O/orzU= X-Google-Smtp-Source: ACHHUZ5+X/zoZRguFAuzJZf8VyfB4o3oeMCEboIx2chsOQTs+zzrWO2Ahwlkd5vuNZT2Rf8Jk53FYxDdW1olTHF7Vd8= X-Received: by 2002:a2e:9b48:0:b0:2b1:a4e9:63b6 with SMTP id o8-20020a2e9b48000000b002b1a4e963b6mr1911244ljj.51.1686089873497; Tue, 06 Jun 2023 15:17:53 -0700 (PDT) MIME-Version: 1.0 References: <20230602151739.3600820-1-ardb@kernel.org> <20230602151739.3600820-3-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 7 Jun 2023 00:17:42 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v2 2/7] MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code To: Michael Kubacki 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 Content-Type: text/plain; charset="UTF-8" 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.