Re: [PATCH v2 3/3] UefiCpuPkg/CpuFeature: Introduce First to indicate 1st unit.

["Zeng, Star" <star.zeng@intel.com>]

Some feedback added.

> -----Original Message-----
> From: Ray Ni [mailto:niruiyu@users.noreply.github.com]
> Sent: Tuesday, November 26, 2019 2:16 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH v2 3/3] UefiCpuPkg/CpuFeature: Introduce First to indicate
> 1st unit.
> 
> From: Ray Ni <ray.ni@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1584
> 
> The flow of CPU feature initialization logic is:
> 1. BSP calls GetConfigDataFunc() for each thread/AP;
> 2. Each thread/AP calls SupportFunc() to detect its own capability;
> 3. BSP calls InitializeFunc() for each thread/AP.
> 
> There is a design gap in step #3. For a package scope feature that only
> requires one thread of each package does the initialization operation,
> what InitializeFunc() currently does is to do the initialization
> operation only CPU physical location Core# is 0.
> But in certain platform, Core#0 might be disabled in hardware level
> which results the certain package scope feature isn't initialized at
> all.

Need some patches to update individual InitializeFunc() for features.
These patches can be a separated patch series.

> 
> The patch adds a new field Fist to indicate the CPU's location in

"Firt" should be "First".

> its parent scope.
> First.Package is set for all APs/threads under first package;
> First.Core is set for all APs/threads under first core of each
> package;
> First.Thread is set for the AP/thread of each core.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  .../Include/Library/RegisterCpuFeaturesLib.h  | 36 +++++++++
>  .../CpuFeaturesInitialize.c                   | 74 +++++++++++++++++++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> index d075606cdb..7114c8ce89 100644
> --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> @@ -78,6 +78,37 @@
>  #define CPU_FEATURE_END                             MAX_UINT32
>  /// @}
> 
> +///
> +/// The bit field to indicate whether the processor is the first in its parent
> scope.
> +///
> +typedef struct {
> +  //
> +  // Set to 1 when current processor is the first thread in the core it resides
> in.
> +  //
> +  UINT32 Thread   : 1;
> +  //
> +  // Set to 1 when current processor is a thread of the first core in the
> module it resides in.
> +  //
> +  UINT32 Core     : 1;
> +  //
> +  // Set to 1 when current processor is a thread of the first module in the tile
> it resides in.
> +  //
> +  UINT32 Module   : 1;
> +  //
> +  // Set to 1 when current processor is a thread of the first tile in the die it
> resides in.
> +  //
> +  UINT32 Tile     : 1;
> +  //
> +  // Set to 1 when current processor is a thread of the first die in the package
> it resides in.
> +  //
> +  UINT32 Die      : 1;
> +  //
> +  // Set to 1 when current processor is a thread of the first package in the
> system.
> +  //
> +  UINT32 Package  : 1;
> +  UINT32 Reserved : 26;
> +} REGISTER_CPU_FEATURE_FIRST_PROCESSOR;
> +
>  ///
>  /// CPU Information passed into the SupportFunc and InitializeFunc of the
>  /// RegisterCpuFeature() library function.  This structure contains
> information
> @@ -88,6 +119,11 @@ typedef struct {
>    /// The package that the CPU resides
>    ///
>    EFI_PROCESSOR_INFORMATION            ProcessorInfo;
> +
> +  ///
> +  /// The bit flag indicating whether the CPU is the first
> Thread/Core/Module/Tile/Die/Package in its parent scope.
> +  ///
> +  REGISTER_CPU_FEATURE_FIRST_PROCESSOR First;
>    ///
>    /// The Display Family of the CPU computed from CPUID leaf
> CPUID_VERSION_INFO
>    ///
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index 0a4fcff033..23076fd453 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -105,6 +105,9 @@ CpuInitDataInitialize (
>    EFI_CPU_PHYSICAL_LOCATION            *Location;
>    BOOLEAN                              *CoresVisited;
>    UINTN                                Index;
> +  UINT32                               PackageIndex;
> +  UINT32                               CoreIndex;
> +  UINT32                               First;
>    ACPI_CPU_DATA                        *AcpiCpuData;
>    CPU_STATUS_INFORMATION               *CpuStatus;
>    UINT32                               *ValidCoreCountPerPackage;
> @@ -234,6 +237,77 @@ CpuInitDataInitialize (
>    ASSERT (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL);
>    CpuFeaturesData->CpuFlags.PackageSemaphoreCount = AllocateZeroPool
> (sizeof (UINT32) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount *
> CpuStatus->MaxThreadCount);
>    ASSERT (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL);
> +
> +  //
> +  // Initialize CpuFeaturesData->InitOrder[].CpuInfo.First
> +  //
> +
> +  //
> +  // Set First.Package for each thread belonging to the first package.
> +  //
> +  First = MAX_UINT32;
> +  for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> +    Location = &CpuFeaturesData-
> >InitOrder[ProcessorNumber].CpuInfo.ProcessorInfo.Location;
> +    First = MIN (Location->Package, First);
> +  }
> +  for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> +    Location = &CpuFeaturesData-
> >InitOrder[ProcessorNumber].CpuInfo.ProcessorInfo.Location;
> +    if (Location->Package == First) {
> +      CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo.First.Package =
> 1;
> +    }
> +  }
> +
> +  //
> +  // Set First.Die/Tile/Module for each thread assuming:
> +  //  single Die under each package, single Tile under each Die, single Module
> under each Tile

This assumption needs to be addressed in this or a separated patch series.

> +  //
> +  for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> +    CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo.First.Die = 1;
> +    CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo.First.Tile = 1;
> +    CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo.First.Module =
> 1;
> +  }
> +
> +  for (PackageIndex = 0; PackageIndex < CpuStatus->PackageCount;
> PackageIndex++) {
> +    //
> +    // Set First.Core for each thread in the first core of each package.
> +    //
> +    First = MAX_UINT32;
> +    for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> +      Location = &CpuFeaturesData-
> >InitOrder[ProcessorNumber].CpuInfo.ProcessorInfo.Location;
> +      if (Location->Package == PackageIndex) {

Here the code is assuming Location->Package starts from 0 and consecutive.

> +        First = MIN (Location->Core, First);
> +      }
> +    }
> +
> +    for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> +      Location = &CpuFeaturesData-
> >InitOrder[ProcessorNumber].CpuInfo.ProcessorInfo.Location;
> +      if (Location->Package == PackageIndex && Location->Core == First) {
> +        CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo.First.Core = 1;
> +      }
> +    }
> +  }
> +
> +  for (PackageIndex = 0; PackageIndex < CpuStatus->PackageCount;
> PackageIndex++) {
> +    for (CoreIndex = 0; CoreIndex < CpuStatus->MaxCoreCount;
> CoreIndex++) {
> +      //
> +      // Set First.Thread for the first thread of each core.
> +      //
> +      First = MAX_UINT32;
> +      for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> +        Location = &CpuFeaturesData-
> >InitOrder[ProcessorNumber].CpuInfo.ProcessorInfo.Location;
> +        if (Location->Package == PackageIndex && Location->Core ==
> CoreIndex) {

Here the code is assuming Location->Package and Location->Core start from 0 and consecutive.
We could not have this assumption, this patch is to resolve this assumption.


Thanks,
Star

> +          First = MIN (Location->Thread, First);
> +        }
> +      }
> +
> +      for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> +        Location = &CpuFeaturesData-
> >InitOrder[ProcessorNumber].CpuInfo.ProcessorInfo.Location;
> +        if (Location->Package == PackageIndex && Location->Core ==
> CoreIndex && Location->Thread == First) {
> +          CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo.First.Thread
> = 1;
> +        }
> +      }
> +    }
> +  }
>  }
> 
>  /**
> --
> 2.21.0.windows.1