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

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

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