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

[Laszlo Ersek <lersek@redhat.com>]

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 <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  .../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
>    );
>  
>  /**
>