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

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

[Ji-yunX Lu]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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