Re: [Patch V2 4/6] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core

["Laszlo Ersek" <lersek@redhat.com>]

Hi Mike,

On 04/30/19 03:30, Michael D Kinney wrote:
> Avoid access to MSR_IA32_APIC_BASE that may not be supported
> on single core CPUs.  If PcdCpuMaxLogicalProcessorNumber is 1,
> then there is only one CPU that must be the BSP.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 35dff91fd2..5488049c08 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    MP initialize support functions for PEI phase.
>  
> -  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -101,6 +101,19 @@ GetCpuMpData (
>    MSR_IA32_APIC_BASE_REGISTER  ApicBaseMsr;
>    IA32_DESCRIPTOR              Idtr;
>  
> +  //
> +  // If there is only 1 CPU, then it must be the BSP.  This avoids an access to
> +  // MSR_IA32_APIC_BASE that may not be supported on single core CPUs.
> +  //
> +  if (PcdGet32 (PcdCpuMaxLogicalProcessorNumber) == 1) {

this PcdGet32() call has the same issue as in v1. It is unsafe because:

- if the platform makes PcdCpuMaxLogicalProcessorNumber a dynamic or
dynamic-ex PCD, then the above call will result in a PPI member function
invocation,

- and said invocation may be executed on an AP, not necessarily on the BSP.

But, APs must not call PPI member functions, to my knowledge.

We can certainly use a PCD here, but both UefiCpuPkg.dec, and the code
added here, must prevent platforms from making that PCD dynamic or
dynamic-ex.

Thanks
Laszlo

> +    CpuMpData = GetCpuMpDataFromGuidedHob ();
> +    ASSERT (CpuMpData != NULL);
> +    return CpuMpData;
> +  }
> +
> +  //
> +  // Otherwise use MSR_IA32_APIC_BASE to determine if the CPU is BSP or AP.
> +  //
>    ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
>    if (ApicBaseMsr.Bits.BSP == 1) {
>      CpuMpData = GetCpuMpDataFromGuidedHob ();
>