[PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow

[PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow

[Ni, Ray]

Ray Ni (4):
  UefiCpuPkg/MpInitLib: Separate X2APIC enabling to subfunction
  UefiCpuPkg/MpInitLib: Sync BSP's APIC mode to APs in InitConfig path
  UefiCpuPkg/MpInitLib: Skip X2APIC enabling when BSP in X2APIC already
  UefiCpuPkg/CpuFeatures: Deprecate CPU_FEATURE_X2APIC

 .../Include/Library/RegisterCpuFeaturesLib.h  |   2 +-
 .../CpuCommonFeaturesLib/CpuCommonFeatures.h  |  69 +--------
 .../CpuCommonFeaturesLib.c                    |  14 +-
 .../CpuCommonFeaturesLib.inf                  |   1 -
 .../Library/CpuCommonFeaturesLib/X2Apic.c     | 138 ------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.c          |  78 +++++++---
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |  11 ++
 7 files changed, 69 insertions(+), 244 deletions(-)
 delete mode 100644 UefiCpuPkg/Library/CpuCommonFeaturesLib/X2Apic.c

-- 
2.39.1.windows.1

[PATCH 1/4] UefiCpuPkg/MpInitLib: Separate X2APIC enabling to subfunction

[Ni, Ray]

It's very confusing that auto X2 APIC enabling and APIC ID sorting
are all performed inside CollectProcessorCount().

The change is to separate the X2 APIC enabling to AutoEnableX2Apic()
and call that from MpInitLibInitialize().
SortApicId() is called from MpInitLibInitialize() as well.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 65 ++++++++++++++++++----------
 1 file changed, 42 insertions(+), 23 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index f1f2840714..bf80455965 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -495,33 +495,20 @@ GetProcessorNumber (
 }
 
 /**
-  This function will get CPU count in the system.
+  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.
 
   @param[in] CpuMpData        Pointer to PEI CPU MP Data
-
-  @return  CPU count detected
 **/
-UINTN
-CollectProcessorCount (
+VOID
+AutoEnableX2Apic (
   IN CPU_MP_DATA  *CpuMpData
   )
 {
+  BOOLEAN          X2Apic;
   UINTN            Index;
   CPU_INFO_IN_HOB  *CpuInfoInHob;
-  BOOLEAN          X2Apic;
-
-  //
-  // Send 1st broadcast IPI to APs to wakeup APs
-  //
-  CpuMpData->InitFlag = ApInitConfig;
-  WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
-  CpuMpData->InitFlag = ApInitDone;
-  //
-  // When InitFlag == ApInitConfig, WakeUpAP () guarantees all APs are checked in.
-  // FinishedCount is the number of check-in APs.
-  //
-  CpuMpData->CpuCount = CpuMpData->FinishedCount + 1;
-  ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
 
   //
   // Enable x2APIC mode if
@@ -570,12 +557,32 @@ CollectProcessorCount (
   }
 
   DEBUG ((DEBUG_INFO, "APIC MODE is %d\n", GetApicMode ()));
+}
+
+/**
+  This function will get CPU count in the system.
+
+  @param[in] CpuMpData        Pointer to PEI CPU MP Data
+
+  @return  CPU count detected
+**/
+UINTN
+CollectProcessorCount (
+  IN CPU_MP_DATA  *CpuMpData
+  )
+{
   //
-  // Sort BSP/Aps by CPU APIC ID in ascending order
+  // Send 1st broadcast IPI to APs to wakeup APs
   //
-  SortApicId (CpuMpData);
-
-  DEBUG ((DEBUG_INFO, "MpInitLib: Find %d processors in system.\n", CpuMpData->CpuCount));
+  CpuMpData->InitFlag = ApInitConfig;
+  WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
+  CpuMpData->InitFlag = ApInitDone;
+  //
+  // When InitFlag == ApInitConfig, WakeUpAP () guarantees all APs are checked in.
+  // FinishedCount is the number of check-in APs.
+  //
+  CpuMpData->CpuCount = CpuMpData->FinishedCount + 1;
+  ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
 
   return CpuMpData->CpuCount;
 }
@@ -1976,6 +1983,18 @@ MpInitLibInitialize (
       // Wakeup all APs and calculate the processor count in system
       //
       CollectProcessorCount (CpuMpData);
+
+      //
+      // Enable X2APIC if needed.
+      //
+      AutoEnableX2Apic (CpuMpData);
+
+      //
+      // Sort BSP/Aps by CPU APIC ID in ascending order
+      //
+      SortApicId (CpuMpData);
+
+      DEBUG ((DEBUG_INFO, "MpInitLib: Find %d processors in system.\n", CpuMpData->CpuCount));
     }
   } else {
     //
-- 
2.39.1.windows.1

[PATCH 2/4] UefiCpuPkg/MpInitLib: Sync BSP's APIC mode to APs in InitConfig path

[Ni, Ray]

The change saves the BSP's initial APIC mode and syncs to all APs
in first time wakeup. It allows certain platforms to switch to X2 APIC
as early as possible and also independent on CpuFeaturePei/Dxe.
The platform should switch BSP to X2 APIC mode first before the
CpuMpPeim runs.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 +++++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.h | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index bf80455965..2372475a04 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -694,6 +694,12 @@ ApWakeupFunction (
       ApStackData  = (AP_STACK_DATA *)((UINTN)ApTopOfStack - sizeof (AP_STACK_DATA));
       BistData     = (UINT32)ApStackData->Bist;
 
+      //
+      // Synchronize APIC mode with BSP in the first time AP wakeup ONLY.
+      //
+      SetApicMode (CpuMpData->InitialBspApicMode);
+      CurrentApicMode = CpuMpData->InitialBspApicMode;
+
       //
       // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
       //   to initialize AP in InitConfig path.
@@ -1977,6 +1983,11 @@ MpInitLibInitialize (
   //
   ProgramVirtualWireMode ();
 
+  //
+  // Save APIC mode for AP to sync
+  //
+  CpuMpData->InitialBspApicMode = GetApicMode ();
+
   if (OldCpuMpData == NULL) {
     if (MaxLogicalProcessorNumber > 1) {
       //
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b694c7b40f..1ede253334 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -243,6 +243,17 @@ struct _CPU_MP_DATA {
   //
   SPIN_LOCK                        MpLock;
   UINTN                            Buffer;
+  //
+  // InitialBspApicMode stores the initial BSP APIC mode.
+  // It is used to synchroneize the BSP APIC mode with APs
+  // in the first time APs wake up.
+  // Its value doesn't reflect the current APIC mode since there are
+  // two cases the APIC mode is changed:
+  // 1. MpLib explicitly switches to X2 APIC mode because number of threads is greater than 255,
+  //    or there are any logical processors reporting an initial APIC ID of 255 or greater.
+  // 2. Some code switches to X2 APIC mode in all threads through MP services PPI/Protocol.
+  //
+  UINTN                            InitialBspApicMode;
   UINTN                            CpuApStackSize;
   MP_ASSEMBLY_ADDRESS_MAP          AddressMap;
   UINTN                            WakeupBuffer;
-- 
2.39.1.windows.1

[PATCH 3/4] UefiCpuPkg/MpInitLib: Skip X2APIC enabling when BSP in X2APIC already

[Ni, Ray]

The BSP's APIC mode is synced to all APs in CollectProcessorCount().
So, it's safe to skip the X2 APIC enabling in AutoEnableX2Apic() which
runs later when BSP's APIC mode is X2 APIC already.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 2372475a04..5a8ed027be 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1998,7 +1998,9 @@ MpInitLibInitialize (
       //
       // Enable X2APIC if needed.
       //
-      AutoEnableX2Apic (CpuMpData);
+      if (CpuMpData->InitialBspApicMode == LOCAL_APIC_MODE_XAPIC) {
+        AutoEnableX2Apic (CpuMpData);
+      }
 
       //
       // Sort BSP/Aps by CPU APIC ID in ascending order
-- 
2.39.1.windows.1

[PATCH 4/4] UefiCpuPkg/CpuFeatures: Deprecate CPU_FEATURE_X2APIC

[Ni, Ray]

Since MpLib supports to sync BSP's initial APIC mode to APs,
platform can set BSP to X2 APIC mode before MpLib runs and
expect MpLib syncs the X2 APIC mode to all APs.

With such capability in MpLib, CpuCommonFeaturesLib's
X2 APIC enable logic is dropped for simplificity. Such code
removal also removes the confusion regarding how platform
controls the X2 APIC enabling.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 .../Include/Library/RegisterCpuFeaturesLib.h  |   2 +-
 .../CpuCommonFeaturesLib/CpuCommonFeatures.h  |  69 +--------
 .../CpuCommonFeaturesLib.c                    |  14 +-
 .../CpuCommonFeaturesLib.inf                  |   1 -
 .../Library/CpuCommonFeaturesLib/X2Apic.c     | 138 ------------------
 5 files changed, 3 insertions(+), 221 deletions(-)
 delete mode 100644 UefiCpuPkg/Library/CpuCommonFeaturesLib/X2Apic.c

diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
index 36459afc5e..ac5419141e 100644
--- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
+++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
@@ -44,7 +44,7 @@
 #define CPU_FEATURE_C_STATE                        21
 #define CPU_FEATURE_TM                             22
 #define CPU_FEATURE_TM2                            23
-#define CPU_FEATURE_X2APIC                         24
+#define CPU_FEATURE_X2APIC                         24  ///< deprecated, do not use it to enable X2 APIC.
 #define CPU_FEATURE_RESERVED_25                    25
 #define CPU_FEATURE_RESERVED_26                    26
 #define CPU_FEATURE_RESERVED_27                    27
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h
index 5434a45f6a..22ecab82e8 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h
@@ -1,7 +1,7 @@
 /** @file
   CPU Common features library header file.
 
-  Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017 - 2013, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -781,73 +781,6 @@ C1eInitialize (
   IN BOOLEAN                           State
   );
 
-/**
-  Prepares for the data used by CPU feature detection and initialization.
-
-  @param[in]  NumberOfProcessors  The number of CPUs in the platform.
-
-  @return  Pointer to a buffer of CPU related configuration data.
-
-  @note This service could be called by BSP only.
-**/
-VOID *
-EFIAPI
-X2ApicGetConfigData (
-  IN UINTN  NumberOfProcessors
-  );
-
-/**
-  Detects if X2Apci feature supported on current processor.
-
-  Detect if X2Apci has been already enabled.
-
-  @param[in]  ProcessorNumber  The index of the CPU executing this function.
-  @param[in]  CpuInfo          A pointer to the REGISTER_CPU_FEATURE_INFORMATION
-                               structure for the CPU executing this function.
-  @param[in]  ConfigData       A pointer to the configuration buffer returned
-                               by CPU_FEATURE_GET_CONFIG_DATA.  NULL if
-                               CPU_FEATURE_GET_CONFIG_DATA was not provided in
-                               RegisterCpuFeature().
-
-  @retval TRUE     X2Apci feature is supported.
-  @retval FALSE    X2Apci feature is not supported.
-
-  @note This service could be called by BSP/APs.
-**/
-BOOLEAN
-EFIAPI
-X2ApicSupport (
-  IN UINTN                             ProcessorNumber,
-  IN REGISTER_CPU_FEATURE_INFORMATION  *CpuInfo,
-  IN VOID                              *ConfigData  OPTIONAL
-  );
-
-/**
-  Initializes X2Apci feature to specific state.
-
-  @param[in]  ProcessorNumber  The index of the CPU executing this function.
-  @param[in]  CpuInfo          A pointer to the REGISTER_CPU_FEATURE_INFORMATION
-                               structure for the CPU executing this function.
-  @param[in]  ConfigData       A pointer to the configuration buffer returned
-                               by CPU_FEATURE_GET_CONFIG_DATA.  NULL if
-                               CPU_FEATURE_GET_CONFIG_DATA was not provided in
-                               RegisterCpuFeature().
-  @param[in]  State            If TRUE, then the X2Apci feature must be enabled.
-                               If FALSE, then the X2Apci feature must be disabled.
-
-  @retval RETURN_SUCCESS       X2Apci feature is initialized.
-
-  @note This service could be called by BSP only.
-**/
-RETURN_STATUS
-EFIAPI
-X2ApicInitialize (
-  IN UINTN                             ProcessorNumber,
-  IN REGISTER_CPU_FEATURE_INFORMATION  *CpuInfo,
-  IN VOID                              *ConfigData   OPTIONAL,
-  IN BOOLEAN                           State
-  );
-
 /**
   Prepares for the data used by CPU feature detection and initialization.
 
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
index 7f0e1004b9..cc8807c19d 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
@@ -2,7 +2,7 @@
   This library registers CPU features defined in Intel(R) 64 and IA-32
   Architectures Software Developer's Manual.
 
-  Copyright (c) 2017 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017 - 2023, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -192,18 +192,6 @@ CpuCommonFeaturesLibConstructor (
     ASSERT_EFI_ERROR (Status);
   }
 
-  if (IsCpuFeatureSupported (CPU_FEATURE_X2APIC)) {
-    Status = RegisterCpuFeature (
-               "X2Apic",
-               X2ApicGetConfigData,
-               X2ApicSupport,
-               X2ApicInitialize,
-               CPU_FEATURE_X2APIC,
-               CPU_FEATURE_END
-               );
-    ASSERT_EFI_ERROR (Status);
-  }
-
   if (IsCpuFeatureSupported (CPU_FEATURE_PPIN)) {
     Status = RegisterCpuFeature (
                "PPIN",
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
index 1b823155b1..1a95e745b5 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
@@ -40,7 +40,6 @@
   MachineCheck.c
   MonitorMwait.c
   PendingBreak.c
-  X2Apic.c
   Ppin.c
   ProcTrace.c
 
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/X2Apic.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/X2Apic.c
deleted file mode 100644
index 220f307e5a..0000000000
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/X2Apic.c
+++ /dev/null
@@ -1,138 +0,0 @@
-/** @file
-  X2Apic feature.
-
-  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "CpuCommonFeatures.h"
-
-/**
-  Prepares for the data used by CPU feature detection and initialization.
-
-  @param[in]  NumberOfProcessors  The number of CPUs in the platform.
-
-  @return  Pointer to a buffer of CPU related configuration data.
-
-  @note This service could be called by BSP only.
-**/
-VOID *
-EFIAPI
-X2ApicGetConfigData (
-  IN UINTN  NumberOfProcessors
-  )
-{
-  BOOLEAN  *ConfigData;
-
-  ConfigData = AllocateZeroPool (sizeof (BOOLEAN) * NumberOfProcessors);
-  ASSERT (ConfigData != NULL);
-  return ConfigData;
-}
-
-/**
-  Detects if X2Apci feature supported on current processor.
-
-  Detect if X2Apci has been already enabled.
-
-  @param[in]  ProcessorNumber  The index of the CPU executing this function.
-  @param[in]  CpuInfo          A pointer to the REGISTER_CPU_FEATURE_INFORMATION
-                               structure for the CPU executing this function.
-  @param[in]  ConfigData       A pointer to the configuration buffer returned
-                               by CPU_FEATURE_GET_CONFIG_DATA.  NULL if
-                               CPU_FEATURE_GET_CONFIG_DATA was not provided in
-                               RegisterCpuFeature().
-
-  @retval TRUE     X2Apci feature is supported.
-  @retval FALSE    X2Apci feature is not supported.
-
-  @note This service could be called by BSP/APs.
-**/
-BOOLEAN
-EFIAPI
-X2ApicSupport (
-  IN UINTN                             ProcessorNumber,
-  IN REGISTER_CPU_FEATURE_INFORMATION  *CpuInfo,
-  IN VOID                              *ConfigData  OPTIONAL
-  )
-{
-  BOOLEAN  *X2ApicEnabled;
-
-  ASSERT (ConfigData != NULL);
-  X2ApicEnabled = (BOOLEAN *)ConfigData;
-  //
-  // *ConfigData indicates if X2APIC enabled on current processor
-  //
-  X2ApicEnabled[ProcessorNumber] = (GetApicMode () == LOCAL_APIC_MODE_X2APIC) ? TRUE : FALSE;
-
-  return (CpuInfo->CpuIdVersionInfoEcx.Bits.x2APIC == 1);
-}
-
-/**
-  Initializes X2Apci feature to specific state.
-
-  @param[in]  ProcessorNumber  The index of the CPU executing this function.
-  @param[in]  CpuInfo          A pointer to the REGISTER_CPU_FEATURE_INFORMATION
-                               structure for the CPU executing this function.
-  @param[in]  ConfigData       A pointer to the configuration buffer returned
-                               by CPU_FEATURE_GET_CONFIG_DATA.  NULL if
-                               CPU_FEATURE_GET_CONFIG_DATA was not provided in
-                               RegisterCpuFeature().
-  @param[in]  State            If TRUE, then the X2Apci feature must be enabled.
-                               If FALSE, then the X2Apci feature must be disabled.
-
-  @retval RETURN_SUCCESS       X2Apci feature is initialized.
-
-  @note This service could be called by BSP only.
-**/
-RETURN_STATUS
-EFIAPI
-X2ApicInitialize (
-  IN UINTN                             ProcessorNumber,
-  IN REGISTER_CPU_FEATURE_INFORMATION  *CpuInfo,
-  IN VOID                              *ConfigData   OPTIONAL,
-  IN BOOLEAN                           State
-  )
-{
-  BOOLEAN  *X2ApicEnabled;
-
-  //
-  // The scope of the MSR_IA32_APIC_BASE is core for below processor type, only program
-  // MSR_IA32_APIC_BASE for thread 0 in each core.
-  //
-  if (IS_SILVERMONT_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo->DisplayModel)) {
-    if (CpuInfo->ProcessorInfo.Location.Thread != 0) {
-      return RETURN_SUCCESS;
-    }
-  }
-
-  ASSERT (ConfigData != NULL);
-  X2ApicEnabled = (BOOLEAN *)ConfigData;
-  if (X2ApicEnabled[ProcessorNumber]) {
-    PRE_SMM_CPU_REGISTER_TABLE_WRITE_FIELD (
-      ProcessorNumber,
-      Msr,
-      MSR_IA32_APIC_BASE,
-      MSR_IA32_APIC_BASE_REGISTER,
-      Bits.EXTD,
-      1
-      );
-  } else {
-    //
-    // Enable X2APIC mode only if X2APIC is not enabled,
-    // Needn't to disabe X2APIC mode again if X2APIC is not enabled
-    //
-    if (State) {
-      CPU_REGISTER_TABLE_WRITE_FIELD (
-        ProcessorNumber,
-        Msr,
-        MSR_IA32_APIC_BASE,
-        MSR_IA32_APIC_BASE_REGISTER,
-        Bits.EXTD,
-        1
-        );
-    }
-  }
-
-  return RETURN_SUCCESS;
-}
-- 
2.39.1.windows.1

Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow

[Gerd Hoffmann]

On Fri, Jul 07, 2023 at 01:28:57PM +0800, Ni, Ray wrote:

[ empty cover letter ]

Summary of the patch series changes would be nice.

If I read things correctly this series will:

  (a) allow platforms use x2apic mode by simply switching the BSP into
      x2apic mode early enough (for example in PlatformPei).
  (b) avoids waking up all APs a second time to set apic mode.

Is that correct?

Do you still plan to add a PCD to enable x2apic mode, or is that plan
obsoleted by (a) ?

take care,
  Gerd

Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow

[Ni, Ray]

Gerd,
No. I don't plan to add PCD.
I thought that initially but in the end I figured out:
* PCD is a way to let platform configure the common logic behavior.
* Why not treat "X2 APIC status in BSP" as a "hardware" PCD?

With above thoughts, I prefer (a).
Platform can choose whether to define a "platform-level" PCD to control
platform behavior regarding whether to enable X2 APIC in BSP before CPU MP.

Comments?

Thanks,
Ray


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Friday, July 7, 2023 4:56 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in MP init
> flow
> 
> On Fri, Jul 07, 2023 at 01:28:57PM +0800, Ni, Ray wrote:
> 
> [ empty cover letter ]
> 
> Summary of the patch series changes would be nice.
> 
> If I read things correctly this series will:
> 
>   (a) allow platforms use x2apic mode by simply switching the BSP into
>       x2apic mode early enough (for example in PlatformPei).
>   (b) avoids waking up all APs a second time to set apic mode.
> 
> Is that correct?
> 
> Do you still plan to add a PCD to enable x2apic mode, or is that plan
> obsoleted by (a) ?
> 
> take care,
>   Gerd
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow

[Gerd Hoffmann]

On Fri, Jul 07, 2023 at 09:25:39AM +0000, Ni, Ray wrote:
> Gerd,
> No. I don't plan to add PCD.
> I thought that initially but in the end I figured out:
> * PCD is a way to let platform configure the common logic behavior.
> * Why not treat "X2 APIC status in BSP" as a "hardware" PCD?

A PCD has the advantage that most configuration knobs use that,
so people are used to use PCDs to configure their platform builds.

Using hardware status as pseudo PCD technically works too, and
it isn't the first case in edk2 either, 5-level paging works the
same way for example.

ovmf test patch (enabling x2mode unconditionally) below.

Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

------------------------------ cut here -----------------------------

>From 46a2d9128c42f62547f76afbdb8eef9cf5d0a8a1 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 7 Jul 2023 12:44:21 +0200
Subject: [PATCH 1/1] OvmfPkg/PlatformPei: enable x2apic mode if supported

Switch the BSP local apic into x2apic mode if supported by the CPU.
CpuMpPei will switch the APs into x2apic mode, see commit FIXME.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
 OvmfPkg/PlatformPei/Platform.c      | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 3934aeed9514..b670e1ba6745 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
   DebugLib
   HobLib
   IoLib
+  LocalApicLib
   PciLib
   ResourcePublicationLib
   PeiServicesLib
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index f5dc41c3a8c4..fd34fceafa31 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -21,6 +21,7 @@
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
 #include <Library/IoLib.h>
+#include <Library/LocalApicLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/PcdLib.h>
 #include <Library/PciLib.h>
@@ -271,6 +272,29 @@ MaxCpuCountInitialization (
   ASSERT_RETURN_ERROR (PcdStatus);
 }
 
+STATIC
+VOID
+PlatformApicInit (
+  IN EFI_HOB_PLATFORM_INFO  *PlatformInfoHob
+  )
+{
+  UINT32   RegEax, RegEbx, RegEcx, RegEdx;
+
+  AsmCpuid (1, &RegEax, &RegEbx, &RegEcx, &RegEdx);
+  if (!(RegEcx & (1 << 21))) {
+    DEBUG ((DEBUG_INFO, "%a: x2apic mode not supported\n", __func__));
+    return;
+  }
+
+  SetApicMode(LOCAL_APIC_MODE_X2APIC);
+  if (!(GetApicMode() == LOCAL_APIC_MODE_X2APIC)) {
+    DEBUG ((DEBUG_WARN, "%a: enabling x2apic mode failed\n", __func__));
+    return;
+  }
+
+  DEBUG ((DEBUG_INFO, "%a: x2apic mode enabled\n", __func__));
+}
+
 /**
  * @brief Builds PlatformInfo Hob
  */
@@ -368,5 +392,7 @@ InitializePlatform (
   IntelTdxInitialize ();
   InstallFeatureControlCallback (PlatformInfoHob);
 
+  PlatformApicInit(PlatformInfoHob);
+
   return EFI_SUCCESS;
 }
-- 
2.41.0

Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow

[Aaron Young via groups.io]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

Hello, is this issue/patch still being worked/considered? If so, is there an ETA?

Without this fix, one cannot hotplug beyond 255 vcpus with OVMF and we need this capability.

NOTE: Gerd's original fix ( https://edk2.groups.io/g/devel/message/100801 ), does allow hotplug beyond 255 vcpus. So, that fix seems viable. Should it be re-evaluated?

We would gladly test a fix if that would be helpful.

Thanks in advance!

-Aaron Young ( aaron.young@oracle.com )


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110991): https://edk2.groups.io/g/devel/message/110991
Mute This Topic: https://groups.io/mt/100000874/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 1494 bytes --]

Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

+Ray

Unless I missed it, I do not see review of the patch series Ray sent back in July.

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Aaron Young via groups.io
Sent: Thursday, November 9, 2023 8:29 AM
To: Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow




 Hello, is this issue/patch still being worked/considered? If so, is there an ETA?

 Without this fix, one cannot hotplug beyond 255 vcpus with OVMF and we need this capability.

 NOTE: Gerd's original fix (https://edk2.groups.io/g/devel/message/100801), does allow hotplug beyond 255 vcpus. So, that fix seems viable. Should it be re-evaluated?

 We would gladly test a fix if that would be helpful.

 Thanks in advance!

 -Aaron Young (aaron.young@oracle.com<mailto:aaron.young@oracle.com>)



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110995): https://edk2.groups.io/g/devel/message/110995
Mute This Topic: https://groups.io/mt/100000874/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 4223 bytes --]

Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in MP init flow

[Laszlo Ersek]

On 11/9/23 19:08, Michael D Kinney wrote:
> +Ray
> 
>  
> 
> Unless I missed it, I do not see review of the patch series Ray sent
> back in July.

Right, please repost.

Laszlo

> 
>  
> 
> Mike
> 
>  
> 
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of *Aaron
> Young via groups.io
> *Sent:* Thursday, November 9, 2023 8:29 AM
> *To:* Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io
> *Subject:* Re: [edk2-devel] [PATCH 0/4] Sync BSP's APIC mode to APs in
> MP init flow
> 
>  
> 
>  
> 
>  Hello, is this issue/patch still being worked/considered? If so, is
> there an ETA?
> 
>  Without this fix, one cannot hotplug beyond 255 vcpus with OVMF and we
> need this capability.
> 
>  NOTE: Gerd's original fix
> (https://edk2.groups.io/g/devel/message/100801
> <https://edk2.groups.io/g/devel/message/100801>), does allow hotplug
> beyond 255 vcpus. So, that fix seems viable. Should it be re-evaluated?
> 
>  We would gladly test a fix if that would be helpful.
> 
>  Thanks in advance!
> 
>  -Aaron Young (aaron.young@oracle.com <mailto:aaron.young@oracle.com>)
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111150): https://edk2.groups.io/g/devel/message/111150
Mute This Topic: https://groups.io/mt/100000874/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-