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 <[email protected]> >> Cc: Eric Dong <[email protected]> >> Cc: Ray Ni <[email protected]> >> Cc: Laszlo Ersek <[email protected]> >> Cc: Rahul Kumar <[email protected]> >> --- >> 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/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
