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 <[email protected]>
> Cc: Laszlo Ersek <[email protected]>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <[email protected]>
> ---
> .../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
> );
>
> /**
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
