From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 29 Aug 2019 05:37:02 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C9666C0568FD; Thu, 29 Aug 2019 12:37:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-174.ams2.redhat.com [10.36.117.174]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0130660C44; Thu, 29 Aug 2019 12:37:00 +0000 (UTC) Subject: Re: [Patch] UefiCpuPkg/SecCore: get AllSecPpiList after SecPlatformMain. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20190828065020.2992-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 29 Aug 2019 14:37:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190828065020.2992-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 29 Aug 2019 12:37:01 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Eric, On 08/28/19 08:50, Eric Dong wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2136 > > SecPlatformMain is a platform hook function which let platform does > some update. Some platform may adjust SecCoreData->PeiTemporaryRamBase > which caused former saved AllSecPpiList variable invalid. > > This patch update the logic to get AllSecPpiList after SecPlatformMain. > > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Eric Dong > --- > UefiCpuPkg/SecCore/SecMain.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/UefiCpuPkg/SecCore/SecMain.c b/UefiCpuPkg/SecCore/SecMain.c > index f914446257..66c952b897 100644 > --- a/UefiCpuPkg/SecCore/SecMain.c > +++ b/UefiCpuPkg/SecCore/SecMain.c > @@ -228,7 +228,6 @@ SecStartupPhase2( > > PeiCoreEntryPoint = NULL; > SecCoreData = (EFI_SEC_PEI_HAND_OFF *) Context; > - AllSecPpiList = (EFI_PEI_PPI_DESCRIPTOR *) SecCoreData->PeiTemporaryRamBase; > > // > // Perform platform specific initialization before entering PeiCore. > @@ -282,6 +281,8 @@ SecStartupPhase2( > } > > if (PpiList != NULL) { > + AllSecPpiList = (EFI_PEI_PPI_DESCRIPTOR *) SecCoreData->PeiTemporaryRamBase; > + > // > // Remove the terminal flag from the terminal PPI > // > Based on the SecPlatformMain() documentation in "UefiCpuPkg/Include/Library/PlatformSecLib.h", it seems valid for a platform to change "SecCoreData->PeiTemporaryRamBase". Therefore, in SecStartupPhase2(), it appears justified to assign "AllSecPpiList" only after calling SecPlatformMain(). This patch does something else too: it makes the assignment to "AllSecPpiList" conditional. I agree that is justified, as well. Namely: [*] If SecPlatformMain() returns no platform-specific PPI list, then there is nothing to merge, so we don't need "AllSecPpiList" at all. I think however that this change should be documented explicitly. When pushing, please add the sentence [*] to the commit message. With that: Reviewed-by: Laszlo Ersek Thanks Laszlo