From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BCF612194D3B8 for ; Wed, 26 Dec 2018 14:16:08 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 37670DF872; Wed, 26 Dec 2018 22:16:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-136.rdu2.redhat.com [10.10.121.136]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9355B608DD; Wed, 26 Dec 2018 22:16:06 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20181224021236.12976-1-eric.dong@intel.com> <20181224021236.12976-2-eric.dong@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 26 Dec 2018 23:16:05 +0100 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: <20181224021236.12976-2-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 26 Dec 2018 22:16:08 +0000 (UTC) Subject: Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Dec 2018 22:16:09 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/24/18 03:12, Eric Dong wrote: > GetProcessorIndex function calls GetMpPpi to get the MP Ppi. > Ap will calls GetProcessorIndex function which final let AP > calls PeiService. > > This patch avoid GetProcessorIndex call PeiService. > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 2 +- > .../DxeRegisterCpuFeaturesLib.c | 6 ++++-- > .../PeiRegisterCpuFeaturesLib.c | 21 ++++++++++++++++----- > .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 6 +++++- > 4 files changed, 26 insertions(+), 9 deletions(-) There are a number of things I dislike about this patch: > > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > index 624ddee055..6dcc73765b 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > @@ -409,7 +409,7 @@ CollectProcessorData ( > CPU_FEATURES_DATA *CpuFeaturesData; > > CpuFeaturesData = (CPU_FEATURES_DATA *)Buffer; > - ProcessorNumber = GetProcessorIndex (); > + ProcessorNumber = GetProcessorIndex (CpuFeaturesData); > CpuInfo = &CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo; > // > // collect processor information > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > index 926698dc95..6f3e5bd2a8 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > @@ -66,11 +66,13 @@ GetMpProtocol ( > /** > Worker function to return processor index. > > + @param CpuFeaturesData Cpu Feature Data structure. > + > @return The processor index. > **/ > UINTN > GetProcessorIndex ( > - VOID > + IN CPU_FEATURES_DATA *CpuFeaturesData > ) > { > EFI_STATUS Status; > @@ -225,7 +227,7 @@ CpuFeaturesInitialize ( > > CpuFeaturesData = GetCpuFeaturesData (); > > - OldBspNumber = GetProcessorIndex(); > + OldBspNumber = GetProcessorIndex(CpuFeaturesData); > CpuFeaturesData->BspNumber = OldBspNumber; > > Status = gBS->CreateEvent ( (1) In other parts of the patch, we use the opportunity to correct the non-idiomatic parenthesizing style (that is, missing space). We miss that here. > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c > index 0bb3dee8b6..0bbcb50181 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c > @@ -96,20 +96,26 @@ GetMpPpi ( > /** > Worker function to return processor index. > > + @param CpuFeaturesData Cpu Feature Data structure. > + > @return The processor index. > **/ > UINTN > GetProcessorIndex ( > - VOID > + IN CPU_FEATURES_DATA *CpuFeaturesData > ) > { > EFI_STATUS Status; > - EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; > UINTN ProcessorIndex; > + EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; (2) This change looks unnecessary; it just grows the patch. > > - CpuMpPpi = GetMpPpi (); > + ASSERT (CpuFeaturesData->CpuMpPpi != NULL); > + if (CpuFeaturesData->CpuMpPpi == NULL) { > + return (UINTN) (-1); > + } > + CpuMpPpi = (EFI_PEI_MP_SERVICES_PPI *)CpuFeaturesData->CpuMpPpi; > > - Status = CpuMpPpi->WhoAmI(GetPeiServicesTablePointer (), CpuMpPpi, &ProcessorIndex); > + Status = CpuMpPpi->WhoAmI(NULL, CpuMpPpi, &ProcessorIndex); (3) This exploits internal knowledge about the PPI WhoAmI member implementation in CpuMpPei, namely that PeiWhoAmI() ignores "PeiServices". I think this minimally deserves a comment. > ASSERT_EFI_ERROR (Status); > return ProcessorIndex; > } > @@ -286,6 +292,9 @@ GetNumberOfProcessor ( > { > EFI_STATUS Status; > EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; > + CPU_FEATURES_DATA *CpuFeaturesData; > + > + CpuFeaturesData = GetCpuFeaturesData(); > > // > // Get MP Services Protocol > @@ -298,6 +307,8 @@ GetNumberOfProcessor ( > ); > ASSERT_EFI_ERROR (Status); > > + CpuFeaturesData->CpuMpPpi = CpuMpPpi; > + > // > // Get the number of CPUs > // (4) I think it's pretty ugly to cache CpuMpPpi in the GetNumberOfProcessor() function of the PEI instance of the library. The function is called from CpuFeaturesDetect() [RegisterCpuFeaturesLib.c] and GetAcpiCpuData() [CpuFeaturesInitialize.c]. Thus, caching CpuMpPpi right here doesn't make much sense to me, beyond "it happens early enough, and doing it multiple times doesn't hurt". I guess we can live with such a hack, but then a comment should be added. Anyway, OVMF does not use this library class, so I'm neutral on this patch, I just didn't want to ignore it altogether. I don't feel comfortable acking the patch, given the hacks in it, but I also don't intend to prevent it from going in. Please proceed with Ray's review. I'll probably skip reviewing v3 if there's going to be one. Thanks, Laszlo > @@ -329,7 +340,7 @@ CpuFeaturesInitialize ( > > CpuFeaturesData = GetCpuFeaturesData (); > > - OldBspNumber = GetProcessorIndex(); > + OldBspNumber = GetProcessorIndex (CpuFeaturesData); > CpuFeaturesData->BspNumber = OldBspNumber; > > // > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > index cf3da84837..19c3420511 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > @@ -85,6 +85,8 @@ typedef struct { > UINTN BspNumber; > > PROGRAM_CPU_REGISTER_FLAGS CpuFlags; > + > + VOID *CpuMpPpi; > } CPU_FEATURES_DATA; > > #define CPU_FEATURE_ENTRY_FROM_LINK(a) \ > @@ -108,11 +110,13 @@ GetCpuFeaturesData ( > /** > Worker function to return processor index. > > + @param CpuFeaturesData Cpu Feature Data structure. > + > @return The processor index. > **/ > UINTN > GetProcessorIndex ( > - VOID > + IN CPU_FEATURES_DATA *CpuFeaturesData > ); > > /** >