Re: [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable

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

On 10/30/19 10:52, Ray Ni wrote:
> MpInitLib sets X2ApicEnable in two places.
> 1. CollectProcessorCount()
>    This function is called when MpInitLibInitialize() hasn't been
>    called before.
>    It sets X2ApicEnable and later in the same function it configures
>    all CPUs to operate in X2 APIC mode.
> 2. MpInitLibInitialize()
>    The X2ApicEnable setting happens when this function is called in
>    second time. But after that setting, no code consumes that flag.
> 
> With the above analysis and with the purpose of simplifying the code,
> the X2ApicEnable in #1 is changed to local variable and the #2 can be
> changed to remove the setting of X2ApicEnable.
> 
> 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 | 14 ++++++--------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  1 -
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 8f62a8d965..49be5d5385 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -459,12 +459,12 @@ CollectProcessorCount (
>  {
>    UINTN                  Index;
>    CPU_INFO_IN_HOB        *CpuInfoInHob;
> +  BOOLEAN                X2Apic;
>  
>    //
>    // Send 1st broadcast IPI to APs to wakeup APs
>    //
> -  CpuMpData->InitFlag     = ApInitConfig;
> -  CpuMpData->X2ApicEnable = FALSE;
> +  CpuMpData->InitFlag = ApInitConfig;
>    WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
>    CpuMpData->InitFlag = ApInitDone;
>    ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> @@ -481,22 +481,23 @@ CollectProcessorCount (
>    //  1. Number of CPU is greater than 255; or
>    //  2. There are any logical processors reporting an Initial APIC ID of 255 or greater.
>    //
> +  X2Apic = FALSE;
>    if (CpuMpData->CpuCount > 255) {
>      //
>      // If there are more than 255 processor found, force to enable X2APIC
>      //
> -    CpuMpData->X2ApicEnable = TRUE;
> +    X2Apic = 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;
> +        X2Apic = TRUE;
>          break;
>        }
>      }
>    }
>  
> -  if (CpuMpData->X2ApicEnable) {
> +  if (X2Apic) {
>      DEBUG ((DEBUG_INFO, "Force x2APIC mode!\n"));
>      //
>      // Wakeup all APs to enable x2APIC mode
> @@ -1780,9 +1781,6 @@ MpInitLibInitialize (
>      CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
>      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>        InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock);
> -      if (CpuInfoInHob[Index].InitialApicId >= 255 || Index > 254) {
> -        CpuMpData->X2ApicEnable = TRUE;
> -      }
>        CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE;
>        CpuMpData->CpuData[Index].ApFunction = 0;
>        CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 107872b367..8fa07b12c5 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -227,7 +227,6 @@ struct _CPU_MP_DATA {
>    UINTN                          **FailedCpuList;
>  
>    AP_INIT_STATE                  InitFlag;
> -  BOOLEAN                        X2ApicEnable;
>    BOOLEAN                        SwitchBspFlag;
>    UINTN                          NewBspNumber;
>    CPU_EXCHANGE_ROLE_INFO         BSPInfo;
> 

Assuming the previous patch does the right thing in the series (and I
think that's plausible), this patch looks correct as well.

For a second I got concerned as to whether the field removal from
CPU_MP_DATA would require updates to the offset constants in the
assembly code. However, assembly code seems to know offsets into
MP_CPU_EXCHANGE_INFO only, and doesn't care about CPU_MP_DATA. So the
patch is OK I think.

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

Thanks
Laszlo