Re: [edk2: PATCH] UefiCpuPkg: To enable X2Apic by default

["Laszlo Ersek" <lersek@redhat.com>]

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