BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2845
Platform shall enable X2APIC by default to meet requirements for interrupt steering policy on Windows OS.
Change-Id: Ia9e24bce79c91762c560fa3de6260716939f0b1b
Signed-off-by: Ji-yunX Lu <ji-yunx.lu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index ab7a8ed663..70bc5da195 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -488,8 +488,8 @@ CollectProcessorCount (
)
{
UINTN Index;
- CPU_INFO_IN_HOB *CpuInfoInHob;
BOOLEAN X2Apic;
+ CPUID_VERSION_INFO_ECX VersionInfoEcx;
//
// Send 1st broadcast IPI to APs to wakeup APs
@@ -505,26 +505,13 @@ 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.
- //
X2Apic = FALSE;
- if (CpuMpData->CpuCount > 255) {
+ AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, &VersionInfoEcx.Uint32, NULL);
+ if (VersionInfoEcx.Bits.x2APIC == 1) {
//
- // If there are more than 255 processor found, force to enable X2APIC
+ // Enable x2APIC mode if capable
//
X2Apic = TRUE;
- } else {
- CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
- for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
- if (CpuInfoInHob[Index].InitialApicId >= 0xFF) {
- X2Apic = TRUE;
- break;
- }
- }
}
if (X2Apic) {
--
2.26.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62342): https://edk2.groups.io/g/devel/message/62342
Mute This Topic: https://groups.io/mt/75413450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 07/10/20 05:31, Ji-yunX Lu wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2845 > Platform shall enable X2APIC by default to meet requirements for interrupt steering policy on Windows OS. > > Change-Id: Ia9e24bce79c91762c560fa3de6260716939f0b1b > Signed-off-by: Ji-yunX Lu <ji-yunx.lu@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 21 ++++----------------- > 1 file changed, 4 insertions(+), 17 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index ab7a8ed663..70bc5da195 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -488,8 +488,8 @@ CollectProcessorCount ( > ) > { > UINTN Index; > - CPU_INFO_IN_HOB *CpuInfoInHob; > BOOLEAN X2Apic; > + CPUID_VERSION_INFO_ECX VersionInfoEcx; > > // > // Send 1st broadcast IPI to APs to wakeup APs > @@ -505,26 +505,13 @@ 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. > - // > X2Apic = FALSE; > - if (CpuMpData->CpuCount > 255) { > + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, &VersionInfoEcx.Uint32, NULL); > + if (VersionInfoEcx.Bits.x2APIC == 1) { > // > - // If there are more than 255 processor found, force to enable X2APIC > + // Enable x2APIC mode if capable > // > X2Apic = TRUE; > - } else { > - CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; > - for (Index = 0; Index < CpuMpData->CpuCount; Index++) { > - if (CpuInfoInHob[Index].InitialApicId >= 0xFF) { > - X2Apic = TRUE; > - break; > - } > - } > } > > if (X2Apic) { > (1) I think this would break platforms that resolve the LocalApicLib class to the "BaseXApicLib.inf" instance. Based on the message of my earlier commit decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic support", 2015-11-30), it seems like the BaseXApicLib instance notices and trips an assert when the LAPIC is in X2APIC mode, at the next time a LocalApicLib API is used. The BaseXApicLib instance contains many ASSERTs like this: ASSERT (GetApicMode () == LOCAL_APIC_MODE_XAPIC); and the GetApicMode() function itself has the following ASSERT: ASSERT (ApicBaseMsr.Bits.EXTD == 0); So, the change proposed in this patch needs to be gated by a boolean or Feature PCD, and the PCD should default to FALSE. If the platform uses the BaseXApicX2ApicLib instance, then it can set the PCD to TRUE. In turn, for such platform DSCs that already use BaseXApicX2ApicLib exclusively, in edk2 and in edk2-platforms, please post patches that set the PCD to TRUE. This includes the OvmfPkg platform DSC files, for example. (2) Please drop the "Change-Id" tag from the commit message. (3) I think this patch would not pass PatchCheck.py; the commit message is not wrapped correctly. The first line is 105 characters long. (4) Please elaborate on the "requirements for interrupt steering policy on Windows OS", in the commit message. I don't understand what that means. If you have a link to MSDN or similar, please include that. (5) Please take better care of the associated BZ <https://bugzilla.tianocore.org/show_bug.cgi?id=2845>. Given that you posted a patch, (a) the BZ should have status IN_PROGRESS, (b) you should be set as the assignee, and (c) a BZ comment should link your patch from the mailing list archive. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#62328): https://edk2.groups.io/g/devel/message/62328 Mute This Topic: https://groups.io/mt/75413450/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 07/10/20 07:38, Laszlo Ersek wrote: > On 07/10/20 05:31, Ji-yunX Lu wrote: >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2845 >> Platform shall enable X2APIC by default to meet requirements for interrupt steering policy on Windows OS. >> >> Change-Id: Ia9e24bce79c91762c560fa3de6260716939f0b1b >> Signed-off-by: Ji-yunX Lu <ji-yunx.lu@intel.com> >> Cc: Eric Dong <eric.dong@intel.com> >> Cc: Ray Ni <ray.ni@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Rahul Kumar <rahul1.kumar@intel.com> >> --- >> UefiCpuPkg/Library/MpInitLib/MpLib.c | 21 ++++----------------- >> 1 file changed, 4 insertions(+), 17 deletions(-) >> >> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> index ab7a8ed663..70bc5da195 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> @@ -488,8 +488,8 @@ CollectProcessorCount ( >> ) >> { >> UINTN Index; >> - CPU_INFO_IN_HOB *CpuInfoInHob; >> BOOLEAN X2Apic; >> + CPUID_VERSION_INFO_ECX VersionInfoEcx; >> >> // >> // Send 1st broadcast IPI to APs to wakeup APs >> @@ -505,26 +505,13 @@ 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. >> - // >> X2Apic = FALSE; >> - if (CpuMpData->CpuCount > 255) { >> + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, &VersionInfoEcx.Uint32, NULL); >> + if (VersionInfoEcx.Bits.x2APIC == 1) { >> // >> - // If there are more than 255 processor found, force to enable X2APIC >> + // Enable x2APIC mode if capable >> // >> X2Apic = TRUE; >> - } else { >> - CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; >> - for (Index = 0; Index < CpuMpData->CpuCount; Index++) { >> - if (CpuInfoInHob[Index].InitialApicId >= 0xFF) { >> - X2Apic = TRUE; >> - break; >> - } >> - } >> } >> >> if (X2Apic) { >> > > (1) I think this would break platforms that resolve the LocalApicLib > class to the "BaseXApicLib.inf" instance. > > Based on the message of my earlier commit decb365b0016 ("OvmfPkg: select > LocalApicLib instance with x2apic support", 2015-11-30), it seems like > the BaseXApicLib instance notices and trips an assert when the LAPIC is > in X2APIC mode, at the next time a LocalApicLib API is used. > > The BaseXApicLib instance contains many ASSERTs like this: > > ASSERT (GetApicMode () == LOCAL_APIC_MODE_XAPIC); > > and the GetApicMode() function itself has the following ASSERT: > > ASSERT (ApicBaseMsr.Bits.EXTD == 0); > > So, the change proposed in this patch needs to be gated by a boolean or > Feature PCD, and the PCD should default to FALSE. If the platform uses > the BaseXApicX2ApicLib instance, then it can set the PCD to TRUE. > > In turn, for such platform DSCs that already use BaseXApicX2ApicLib > exclusively, in edk2 and in edk2-platforms, please post patches that set > the PCD to TRUE. This includes the OvmfPkg platform DSC files, for example. If a PCD is considered overkill for this, then a new API could be declared in LocalApicLib. UINTN EFIAPI GetMaxApicMode ( VOID ); In BaseXApicLib, the function would return constant LOCAL_APIC_MODE_XAPIC. In BaseXApicX2ApicLib, the function would call the AsmCpuid() seen above in the patch, and return LOCAL_APIC_MODE_XAPIC or LOCAL_APIC_MODE_X2APIC, dependent on "VersionInfoEcx.Bits.x2APIC". And then this patch would call GetMaxApicMode(), rather than check CPUID. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#62344): https://edk2.groups.io/g/devel/message/62344 Mute This Topic: https://groups.io/mt/75413450/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.