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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106714): https://edk2.groups.io/g/devel/message/106714
Mute This Topic: https://groups.io/mt/100000874/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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



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

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
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106726): https://edk2.groups.io/g/devel/message/106726
Mute This Topic: https://groups.io/mt/100000874/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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



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

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

+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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-