Re: [PATCH 1/2] UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP

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

On 10/30/19 10:52, Ray Ni wrote:
> Today's logic sets X2ApicEnable flag in each AP's initialization
> path when InitFlag == ApInitConfig.
> Since all CPUs update the same global data, a spin-lock is used
> to avoid modifications from multiple CPUs happen at the same time.
> The spin-lock causes two problems:
> 1. Potential performance downgrade.
> 2. Undefined behavior when improper timer lib is used.
>    For example we saw certain platforms used AcpiTimerLib from
>    PcAtChipsetPkg and that library depends on retrieving PeiServices
>    from idtr. But in fact AP's (idtr - 4) doesn't point to
>    PeiServices.
> 
> The patch simplifies the code to let BSP set the X2ApicEnable flag so
> the spin-lock acquisition from AP is not needed any more.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 622b70ca3c..8f62a8d965 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -458,6 +458,7 @@ CollectProcessorCount (
>    )
>  {
>    UINTN                  Index;
> +  CPU_INFO_IN_HOB        *CpuInfoInHob;
>  
>    //
>    // Send 1st broadcast IPI to APs to wakeup APs
> @@ -474,12 +475,27 @@ CollectProcessorCount (
>      CpuPause ();
>    }
>  
> +  
> +  //
> +  // Enable x2APIC mode if
> +  //  1. Number of CPU is greater than 255; or
> +  //  2. There are any logical processors reporting an Initial APIC ID of 255 or greater.
> +  //
>    if (CpuMpData->CpuCount > 255) {
>      //
>      // If there are more than 255 processor found, force to enable X2APIC
>      //
>      CpuMpData->X2ApicEnable = TRUE;
> +  } else {
> +    CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
> +    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> +      if (CpuInfoInHob[Index].InitialApicId >= 0xFF) {
> +        CpuMpData->X2ApicEnable = TRUE;
> +        break;
> +      }
> +    }
>    }
> +
>    if (CpuMpData->X2ApicEnable) {
>      DEBUG ((DEBUG_INFO, "Force x2APIC mode!\n"));
>      //
> @@ -541,15 +557,6 @@ InitializeApData (
>  
>    CpuMpData->CpuData[ProcessorNumber].Waiting    = FALSE;
>    CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE : FALSE;
> -  if (CpuInfoInHob[ProcessorNumber].InitialApicId >= 0xFF) {
> -    //
> -    // Set x2APIC mode if there are any logical processor reporting
> -    // an Initial APIC ID of 255 or greater.
> -    //
> -    AcquireSpinLock(&CpuMpData->MpLock);
> -    CpuMpData->X2ApicEnable = TRUE;
> -    ReleaseSpinLock(&CpuMpData->MpLock);
> -  }
>  
>    InitializeSpinLock(&CpuMpData->CpuData[ProcessorNumber].ApLock);
>    SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateIdle);
> 

I would have suggested "InterlockedCompareExchange8" as an alternative,
but (a) maybe there's really no reason for updating that field from APs,
(b) there doesn't seem to be a single byte version of that function.

So, the patch looks plausible. Got no time to test it now, alas.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo