Re: [edk2] [Patch 2/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService.

2019-01-07 Thread Dong, Eric 
Hi Laszlo,

Thanks for your detail check, sorry for this mistake. I will send V4 changes 
and only Cc Ray.

I based on do the minimal change rule to create v2 change, so some code may 
seems ugly.
I based on do the clean code rule to create V3 change, so the change may seems 
big.

Thanks,
Eric 

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, January 8, 2019 3:18 AM
> To: Ni, Ray ; Dong, Eric ; edk2-
> de...@lists.01.org
> Subject: Re: [Patch 2/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls
> PeiService.
> 
> On 01/07/19 09:24, Ni, Ray wrote:
> > 1.
> > How about defining a union for MpService.
> > typedef union {
> >   EFI_MP_SERVICES_PROTOCOL  *Protocol;
> >   EFI_PEI_MP_SERVICES_PPI *Ppi;
> > } MP_SERVICES;
> > So a new field as below can be added here:
> >
> > MP_SERVICES  MpService;
> >
> > GetMpService() can be also changed to return MP_SERVICES;
> >
> > 2. Please update the copyright year to 2019.
> 
> - I wasn't CC'd on the cover letter, and the patches themselves don't include
> any version numbers, so it's hard to tell what version this is.
> 
> - Upon examining my list folder, in addition to my inbox, the cover letter
> seems to be "[edk2] [Patch v2 0/2] Avoid AP calls PeiServices table." -- at
> .
> 
> That's incorrect; this is version 3 actually. V1 was at
> ,
> and the actual v2 was at
> .
> 
> - As I stated under v2, at
> ,
> I'll defer to Ray on v3.
> 
> Thanks,
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService.

2019-01-07 Thread Laszlo Ersek 
On 01/07/19 09:24, Ni, Ray wrote:
> 1.
> How about defining a union for MpService.
> typedef union {
>   EFI_MP_SERVICES_PROTOCOL  *Protocol;
>   EFI_PEI_MP_SERVICES_PPI *Ppi;
> } MP_SERVICES;
> So a new field as below can be added here:
> 
> MP_SERVICES  MpService;
> 
> GetMpService() can be also changed to return MP_SERVICES;
> 
> 2. Please update the copyright year to 2019.

- I wasn't CC'd on the cover letter, and the patches themselves don't
include any version numbers, so it's hard to tell what version this is.

- Upon examining my list folder, in addition to my inbox, the cover
letter seems to be "[edk2] [Patch v2 0/2] Avoid AP calls PeiServices
table." -- at
.

That's incorrect; this is version 3 actually. V1 was at
,
and the actual v2 was at
.

- As I stated under v2, at
,
I'll defer to Ray on v3.

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch 2/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService.

2019-01-06 Thread Eric Dong 
V2:
1. Initialize CpuFeaturesData->MpService in CpuInitDataInitialize
   and make this function been called at the begin of the
   initialization.
2. let all other functions use CpuFeaturesData->MpService install
   of locate the protocol itself.

V1:
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 | 22 +---
 .../DxeRegisterCpuFeaturesLib.c| 57 +++-
 .../PeiRegisterCpuFeaturesLib.c| 62 +-
 .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   | 16 +-
 4 files changed, 85 insertions(+), 72 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c 
b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index 624ddee055..5866e022f0 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -138,7 +138,7 @@ FillProcessorInfo (
 **/
 VOID
 CpuInitDataInitialize (
-  IN UINTN NumberOfCpus
+  VOID
   )
 {
   EFI_STATUS   Status;
@@ -157,12 +157,22 @@ CpuInitDataInitialize (
   ACPI_CPU_DATA*AcpiCpuData;
   CPU_STATUS_INFORMATION   *CpuStatus;
   UINT32   *ValidCoreCountPerPackage;
+  UINTNNumberOfCpus;
+  UINTNNumberOfEnabledProcessors;
 
   Core= 0;
   Package = 0;
   Thread  = 0;
 
   CpuFeaturesData = GetCpuFeaturesData ();
+
+  //
+  // Initialize CpuFeaturesData->MpService as early as possile, so later 
function can use it.
+  //
+  CpuFeaturesData->MpService = GetMpService ();
+
+  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+
   CpuFeaturesData->InitOrder = AllocateZeroPool (sizeof 
(CPU_FEATURES_INIT_ORDER) * NumberOfCpus);
   ASSERT (CpuFeaturesData->InitOrder != NULL);
 
@@ -409,7 +419,7 @@ CollectProcessorData (
   CPU_FEATURES_DATA*CpuFeaturesData;
 
   CpuFeaturesData = (CPU_FEATURES_DATA *)Buffer;
-  ProcessorNumber = GetProcessorIndex ();
+  ProcessorNumber = GetProcessorIndex (CpuFeaturesData);
   CpuInfo = &CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo;
   //
   // collect processor information
@@ -1105,15 +1115,11 @@ CpuFeaturesDetect (
   VOID
   )
 {
-  UINTN  NumberOfCpus;
-  UINTN  NumberOfEnabledProcessors;
   CPU_FEATURES_DATA  *CpuFeaturesData;
 
   CpuFeaturesData = GetCpuFeaturesData();
 
-  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
-
-  CpuInitDataInitialize (NumberOfCpus);
+  CpuInitDataInitialize ();
 
   //
   // Wakeup all APs for data collection.
@@ -1125,6 +1131,6 @@ CpuFeaturesDetect (
   //
   CollectProcessorData (CpuFeaturesData);
 
-  AnalysisProcessorFeatures (NumberOfCpus);
+  AnalysisProcessorFeatures (CpuFeaturesData->NumberOfCpus);
 }
 
diff --git 
a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c 
b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
index 926698dc95..2d2095e437 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
@@ -20,7 +20,6 @@
 #include "RegisterCpuFeatures.h"
 
 CPU_FEATURES_DATA  mCpuFeaturesData = {0};
-EFI_MP_SERVICES_PROTOCOL   *mCpuFeaturesMpServices = NULL;
 
 /**
   Worker function to get CPU_FEATURES_DATA pointer.
@@ -40,44 +39,44 @@ GetCpuFeaturesData (
 
   @return Pointer to EFI_MP_SERVICES_PROTOCOL.
 **/
-EFI_MP_SERVICES_PROTOCOL *
-GetMpProtocol (
+VOID *
+GetMpService (
   VOID
   )
 {
-  EFI_STATUS Status;
-
-  if (mCpuFeaturesMpServices == NULL) {
-//
-// Get MP Services Protocol
-//
-Status = gBS->LocateProtocol (
-  &gEfiMpServiceProtocolGuid,
-  NULL,
-  (VOID **)&mCpuFeaturesMpServices
-  );
-ASSERT_EFI_ERROR (Status);
-  }
+  EFI_STATUSStatus;
+  EFI_MP_SERVICES_PROTOCOL  *MpServices;
 
-  ASSERT (mCpuFeaturesMpServices != NULL);
-  return mCpuFeaturesMpServices;
+  //
+  // Get MP Services Protocol
+  //
+  Status = gBS->LocateProtocol (
+&gEfiMpServiceProtocolGuid,
+NULL,
+(VOID **)&MpServices
+);
+  ASSERT_EFI_ERROR (Status);
+
+  return MpServices;
 }
 
 /**
   Worker function to return processor index.
 
+  @param  CpuFeaturesDataCpu Feature Data structure.
+
   @return  The processor index.
 **/
 UINTN
 GetProcessorIndex