[PATCH 0/2] Set X2ApicEnable flag from BSP

[PATCH 0/2] Set X2ApicEnable flag from BSP

[Ni, Ray]

Ray Ni (2):
  UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP
  UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 37 ++++++++++++++++------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h |  1 -
 2 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.21.0.windows.1

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

[Ni, Ray]

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);
-- 
2.21.0.windows.1

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

[Ni, Ray]

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;
-- 
2.21.0.windows.1

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

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

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray
Sent: Wednesday, October 30, 2019 5:53 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP

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);
--
2.21.0.windows.1

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

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray
Sent: Wednesday, October 30, 2019 5:53 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable

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;
--
2.21.0.windows.1