[PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.

[PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.

[Dong, Eric]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2954

AP needs to run from real mode to 32bit mode to LONG mode. Page table
(pointed by Cr3) and GDT are necessary to set up to correct value when
CPU execution mode is switched to LONG mode.
AP uses the same location page table (Cr3) and GDT as what BSP uses.
But when the page table or GDT is above 4GB, it's impossible for CPU
to use because GDTR.base and Cr3 are 32bits before switching to LONG
mode.

This patch adds check for the Cr3, GDT and IDT to not above 4G limitation.

The check is avoided -- assumed successful -- if the new
PcdEnableCpuApCr3GdtIdtCheck is FALSE (which is the default). The reason
is that the 32-bit requirement is always ensured by edk2 itself; the
requirement is only possibly invalidated by a particular UEFI shell
application that manually moves the GDT/IDT/CR3 above 4GB. Platforms
that don't intend to be compatible with such UEFI applications need not
set the PCD to TRUE.

If the PCD is TRUE and the check fails, then the StartupAllAPs(),
StartupThisAP(), SwitchBSP() and EnableDisableAP() MP service APIs are
rejected at once. Reporting an error immediately is more graceful than
hanging when the APs attempt to switch to long mode.

Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
V3:
only add check for the DxeMplib, no need for PeiMpLib.

V2:
Change the check point. Just in the different caller to make the logic
clear. V1 patch add check just before the use of the code. It make the
logic complicated.

 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 97 +++++++++++++++++++
 UefiCpuPkg/UefiCpuPkg.dec                     |  4 +
 UefiCpuPkg/UefiCpuPkg.uni                     |  2 +
 4 files changed, 104 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 1771575c69..7792df516e 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -74,5 +74,6 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds  ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                          ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                       ## SOMETIMES_CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck               ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ## CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 2c00d72dde..332b4447bb 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -29,6 +29,66 @@ VOID             *mReservedApLoopFunc = NULL;
 UINTN            mReservedTopOfApStack;
 volatile UINT32  mNumberToFinish = 0;
 
+/**
+  Check whether Cr3/GDT/IDT value valid for the APs.
+
+  @retval  TRUE          Pass the check.
+  @retval  FALSE         Fail the check.
+
+**/
+BOOLEAN
+ValidCr3GdtIdtCheck (
+  VOID
+  )
+{
+  IA32_DESCRIPTOR   Gdtr;
+  IA32_DESCRIPTOR   Idtr;
+
+  if (!PcdGetBool (PcdEnableCpuApCr3GdtIdtCheck)) {
+    return TRUE;
+  }
+
+  //
+  // AP needs to run from real mode to 32bit mode to LONG mode. Page table
+  // (pointed by Cr3) and GDT are necessary to set up to correct value when
+  // CPU execution mode is switched to LONG mode. IDT also necessary if the
+  // exception happened.
+  // AP uses the same location page table (Cr3) and GDT/IDT as what BSP uses.
+  // But when the page table or GDT is above 4GB, it's impossible for CPU
+  // to use because GDTR.base and Cr3 are 32bits before switching to LONG
+  // mode.
+  // Here add check for the Cr3, GDT.Base and range, IDT.Base and range are
+  // not above 32 bits limitation.
+  //
+  if (AsmReadCr3 () >= BASE_4GB) {
+    return FALSE;
+  }
+
+  AsmReadGdtr (&Gdtr);
+  //
+  // Here code needs to check both Gdtr.Base and Gdtr.Base + Gdtr.Limit
+  // below BASE_4GB, but Gdtr.Base + Gdtr.Limit below BASE_4GB also means
+  // Gdtr.Base below BASE_4GB. so here just add Gdtr.Base + Gdtr.Limit
+  // check.
+  //
+  if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
+    return FALSE;
+  }
+
+  AsmReadIdtr (&Idtr);
+  //
+  // Here code needs to check both Idtr.Base and Idtr.Base + Idtr.Limit
+  // below BASE_4GB, but Idtr.Base + Idtr.Limit below BASE_4GB also means
+  // Idtr.Base below BASE_4GB. so here just add Idtr.Base + Idtr.Limit
+  // check.
+  //
+  if (Idtr.Base + Idtr.Limit >= BASE_4GB) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
 /**
   Enable Debug Agent to support source debugging on AP function.
 
@@ -394,6 +454,15 @@ MpInitChangeApLoopCallback (
 {
   CPU_MP_DATA               *CpuMpData;
 
+  //
+  // Check the Cr3/GDT/IDT before waking up AP.
+  // If the check return fail, it will block later
+  // OS boot, so halt the system here.
+  //
+  if (!ValidCr3GdtIdtCheck ()) {
+    CpuDeadLoop ();
+  }
+
   CpuMpData = GetCpuMpData ();
   CpuMpData->PmCodeSegment = GetProtectedModeCS ();
   CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
@@ -676,6 +745,13 @@ MpInitLibStartupAllAPs (
 {
   EFI_STATUS              Status;
 
+  //
+  // Check whether Cr3/GDT/IDT valid for AP.
+  //
+  if (!ValidCr3GdtIdtCheck()) {
+    return EFI_UNSUPPORTED;
+  }
+
   //
   // Temporarily stop checkAllApsStatus for avoid resource dead-lock.
   //
@@ -783,6 +859,13 @@ MpInitLibStartupThisAP (
 {
   EFI_STATUS              Status;
 
+  //
+  // Check whether Cr3/GDT/IDT valid for AP.
+  //
+  if (!ValidCr3GdtIdtCheck()) {
+    return EFI_UNSUPPORTED;
+  }
+
   //
   // temporarily stop checkAllApsStatus for avoid resource dead-lock.
   //
@@ -839,6 +922,13 @@ MpInitLibSwitchBSP (
   EFI_TIMER_ARCH_PROTOCOL      *Timer;
   UINT64                       TimerPeriod;
 
+  //
+  // Check whether Cr3/GDT/IDT valid for AP.
+  //
+  if (!ValidCr3GdtIdtCheck()) {
+    return EFI_UNSUPPORTED;
+  }
+
   TimerPeriod = 0;
   //
   // Locate Timer Arch Protocol
@@ -912,6 +1002,13 @@ MpInitLibEnableDisableAP (
   EFI_STATUS     Status;
   BOOLEAN        TempStopCheckState;
 
+  //
+  // Check whether Cr3/GDT/IDT valid for AP.
+  //
+  if (!ValidCr3GdtIdtCheck()) {
+    return EFI_UNSUPPORTED;
+  }
+
   TempStopCheckState = FALSE;
   //
   // temporarily stop checkAllAPsStatus for initialize parameters.
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index d83c084467..08ec36e76c 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -148,6 +148,10 @@
   # @Prompt Lock SMM Feature Control MSR.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOLEAN|0x3213210B
 
+  ## This value specifies whether need to check the Cr3/GDT/IDT value for the APs.
+  # @Prompt Enable Cr3/GDT/IDT value check for the APs.
+  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEAN|0x30000044
+
 [PcdsFixedAtBuild]
   ## List of exception vectors which need switching stack.
   #  This PCD will only take into effect if PcdCpuStackGuard is enabled.
diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index 219c1963bf..3461a83003 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -96,6 +96,8 @@
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_PROMPT  #language en-US "Lock SMM Feature Control MSR"
 
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdEnableCpuApCr3GdtIdtCheck_PROMPT  #language en-US "Enable Cr3/GDT/IDT value check for the APs"
+
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HELP  #language en-US "Lock SMM Feature Control MSR?<BR><BR>\n"
                                                                                            "TRUE  - locked.<BR>\n"
                                                                                            "FALSE - unlocked.<BR>"
-- 
2.23.0.windows.1

Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.

[Yao, Jiewen]

I provided the feedback in https://bugzilla.tianocore.org/show_bug.cgi?id=2954.

I suggest we justify the requirement at first.

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong, Eric
> Sent: Friday, September 4, 2020 9:52 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for
> Cr3/GDT/IDT.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2954
> 
> AP needs to run from real mode to 32bit mode to LONG mode. Page table
> (pointed by Cr3) and GDT are necessary to set up to correct value when
> CPU execution mode is switched to LONG mode.
> AP uses the same location page table (Cr3) and GDT as what BSP uses.
> But when the page table or GDT is above 4GB, it's impossible for CPU
> to use because GDTR.base and Cr3 are 32bits before switching to LONG
> mode.
> 
> This patch adds check for the Cr3, GDT and IDT to not above 4G limitation.
> 
> The check is avoided -- assumed successful -- if the new
> PcdEnableCpuApCr3GdtIdtCheck is FALSE (which is the default). The reason
> is that the 32-bit requirement is always ensured by edk2 itself; the
> requirement is only possibly invalidated by a particular UEFI shell
> application that manually moves the GDT/IDT/CR3 above 4GB. Platforms
> that don't intend to be compatible with such UEFI applications need not
> set the PCD to TRUE.
> 
> If the PCD is TRUE and the check fails, then the StartupAllAPs(),
> StartupThisAP(), SwitchBSP() and EnableDisableAP() MP service APIs are
> rejected at once. Reporting an error immediately is more graceful than
> hanging when the APs attempt to switch to long mode.
> 
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> V3:
> 
> only add check for the DxeMplib, no need for PeiMpLib.
> 
> 
> 
> V2:
> 
> Change the check point. Just in the different caller to make the logic
> 
> clear. V1 patch add check just before the use of the code. It make the
> 
> logic complicated.
> 
> 
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 97 +++++++++++++++++++
>  UefiCpuPkg/UefiCpuPkg.dec                     |  4 +
>  UefiCpuPkg/UefiCpuPkg.uni                     |  2 +
>  4 files changed, 104 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 1771575c69..7792df516e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -74,5 +74,6 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds
> ## CONSUMES
> 
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                          ## CONSUMES
> 
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                       ##
> SOMETIMES_CONSUMES
> 
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck               ##
> CONSUMES
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ##
> CONSUMES
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ##
> CONSUMES
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 2c00d72dde..332b4447bb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -29,6 +29,66 @@ VOID             *mReservedApLoopFunc = NULL;
>  UINTN            mReservedTopOfApStack;
> 
>  volatile UINT32  mNumberToFinish = 0;
> 
> 
> 
> +/**
> 
> +  Check whether Cr3/GDT/IDT value valid for the APs.
> 
> +
> 
> +  @retval  TRUE          Pass the check.
> 
> +  @retval  FALSE         Fail the check.
> 
> +
> 
> +**/
> 
> +BOOLEAN
> 
> +ValidCr3GdtIdtCheck (
> 
> +  VOID
> 
> +  )
> 
> +{
> 
> +  IA32_DESCRIPTOR   Gdtr;
> 
> +  IA32_DESCRIPTOR   Idtr;
> 
> +
> 
> +  if (!PcdGetBool (PcdEnableCpuApCr3GdtIdtCheck)) {
> 
> +    return TRUE;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // AP needs to run from real mode to 32bit mode to LONG mode. Page table
> 
> +  // (pointed by Cr3) and GDT are necessary to set up to correct value when
> 
> +  // CPU execution mode is switched to LONG mode. IDT also necessary if the
> 
> +  // exception happened.
> 
> +  // AP uses the same location page table (Cr3) and GDT/IDT as what BSP uses.
> 
> +  // But when the page table or GDT is above 4GB, it's impossible for CPU
> 
> +  // to use because GDTR.base and Cr3 are 32bits before switching to LONG
> 
> +  // mode.
> 
> +  // Here add check for the Cr3, GDT.Base and range, IDT.Base and range are
> 
> +  // not above 32 bits limitation.
> 
> +  //
> 
> +  if (AsmReadCr3 () >= BASE_4GB) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  AsmReadGdtr (&Gdtr);
> 
> +  //
> 
> +  // Here code needs to check both Gdtr.Base and Gdtr.Base + Gdtr.Limit
> 
> +  // below BASE_4GB, but Gdtr.Base + Gdtr.Limit below BASE_4GB also means
> 
> +  // Gdtr.Base below BASE_4GB. so here just add Gdtr.Base + Gdtr.Limit
> 
> +  // check.
> 
> +  //
> 
> +  if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  AsmReadIdtr (&Idtr);
> 
> +  //
> 
> +  // Here code needs to check both Idtr.Base and Idtr.Base + Idtr.Limit
> 
> +  // below BASE_4GB, but Idtr.Base + Idtr.Limit below BASE_4GB also means
> 
> +  // Idtr.Base below BASE_4GB. so here just add Idtr.Base + Idtr.Limit
> 
> +  // check.
> 
> +  //
> 
> +  if (Idtr.Base + Idtr.Limit >= BASE_4GB) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  return TRUE;
> 
> +}
> 
> +
> 
>  /**
> 
>    Enable Debug Agent to support source debugging on AP function.
> 
> 
> 
> @@ -394,6 +454,15 @@ MpInitChangeApLoopCallback (
>  {
> 
>    CPU_MP_DATA               *CpuMpData;
> 
> 
> 
> +  //
> 
> +  // Check the Cr3/GDT/IDT before waking up AP.
> 
> +  // If the check return fail, it will block later
> 
> +  // OS boot, so halt the system here.
> 
> +  //
> 
> +  if (!ValidCr3GdtIdtCheck ()) {
> 
> +    CpuDeadLoop ();
> 
> +  }
> 
> +
> 
>    CpuMpData = GetCpuMpData ();
> 
>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
> 
>    CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
> 
> @@ -676,6 +745,13 @@ MpInitLibStartupAllAPs (
>  {
> 
>    EFI_STATUS              Status;
> 
> 
> 
> +  //
> 
> +  // Check whether Cr3/GDT/IDT valid for AP.
> 
> +  //
> 
> +  if (!ValidCr3GdtIdtCheck()) {
> 
> +    return EFI_UNSUPPORTED;
> 
> +  }
> 
> +
> 
>    //
> 
>    // Temporarily stop checkAllApsStatus for avoid resource dead-lock.
> 
>    //
> 
> @@ -783,6 +859,13 @@ MpInitLibStartupThisAP (
>  {
> 
>    EFI_STATUS              Status;
> 
> 
> 
> +  //
> 
> +  // Check whether Cr3/GDT/IDT valid for AP.
> 
> +  //
> 
> +  if (!ValidCr3GdtIdtCheck()) {
> 
> +    return EFI_UNSUPPORTED;
> 
> +  }
> 
> +
> 
>    //
> 
>    // temporarily stop checkAllApsStatus for avoid resource dead-lock.
> 
>    //
> 
> @@ -839,6 +922,13 @@ MpInitLibSwitchBSP (
>    EFI_TIMER_ARCH_PROTOCOL      *Timer;
> 
>    UINT64                       TimerPeriod;
> 
> 
> 
> +  //
> 
> +  // Check whether Cr3/GDT/IDT valid for AP.
> 
> +  //
> 
> +  if (!ValidCr3GdtIdtCheck()) {
> 
> +    return EFI_UNSUPPORTED;
> 
> +  }
> 
> +
> 
>    TimerPeriod = 0;
> 
>    //
> 
>    // Locate Timer Arch Protocol
> 
> @@ -912,6 +1002,13 @@ MpInitLibEnableDisableAP (
>    EFI_STATUS     Status;
> 
>    BOOLEAN        TempStopCheckState;
> 
> 
> 
> +  //
> 
> +  // Check whether Cr3/GDT/IDT valid for AP.
> 
> +  //
> 
> +  if (!ValidCr3GdtIdtCheck()) {
> 
> +    return EFI_UNSUPPORTED;
> 
> +  }
> 
> +
> 
>    TempStopCheckState = FALSE;
> 
>    //
> 
>    // temporarily stop checkAllAPsStatus for initialize parameters.
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index d83c084467..08ec36e76c 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -148,6 +148,10 @@
>    # @Prompt Lock SMM Feature Control MSR.
> 
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOL
> EAN|0x3213210B
> 
> 
> 
> +  ## This value specifies whether need to check the Cr3/GDT/IDT value for the
> APs.
> 
> +  # @Prompt Enable Cr3/GDT/IDT value check for the APs.
> 
> +
> gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEA
> N|0x30000044
> 
> +
> 
>  [PcdsFixedAtBuild]
> 
>    ## List of exception vectors which need switching stack.
> 
>    #  This PCD will only take into effect if PcdCpuStackGuard is enabled.
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index 219c1963bf..3461a83003 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -96,6 +96,8 @@
> 
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_PROMP
> T  #language en-US "Lock SMM Feature Control MSR"
> 
> 
> 
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdEnableCpuApCr3GdtIdtCheck_PROMPT
> #language en-US "Enable Cr3/GDT/IDT value check for the APs"
> 
> +
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HELP
> #language en-US "Lock SMM Feature Control MSR?<BR><BR>\n"
> 
>                                                                                             "TRUE  - locked.<BR>\n"
> 
>                                                                                             "FALSE - unlocked.<BR>"
> 
> --
> 2.23.0.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#65019): https://edk2.groups.io/g/devel/message/65019
> Mute This Topic: https://groups.io/mt/76621468/1772286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jiewen.yao@intel.com]
> -=-=-=-=-=-=

Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.

[Laszlo Ersek]

Hello Jiewen,

On 09/04/20 05:48, Yao, Jiewen wrote:
> I provided the feedback in https://bugzilla.tianocore.org/show_bug.cgi?id=2954.
>
> I suggest we justify the requirement at first.

in comment <https://bugzilla.tianocore.org/show_bug.cgi?id=2954#c2>, you
write,

> I don't treat it as a valid test case, to move page table, GDT, IDT in
> UEFI environment.
>
> The CPU context should be managed by CPU driver, and only CPU driver.
> Other driver or application should only change the system state via
> UEFI/PI spec defined API. Otherwise, the system may run into an
> unknown state.
>
> IMHO, I dont think it is a valid case.

and I couldn't agree more with you -- I'm also convinced that what the
particular UEFI application does is invalid.

However, it seems like Eric really needs this change. I don't know his
business case (maybe he's not at liberty to share it), but when he
implied it was really important, I trusted him. So I figured we can
accommodate this use case (even if we don't exactly know all the
details, and even if we know for a fact that what the UEFI application
does is *invalid*, per spec) -- but then the impact, on both
performance, and on code complexity, should be minimal, for platforms
that do not care. (And those platforms have the *right* not to care.)

So I'm going to review this patch now *assuming* that the edk2 community
is OK to accept the use case in the first place. I'm not *encouraging*
the use case or whatever -- the use case is clearly wrong. But Eric
seems to need it: maybe some of the circumstances are out of his
control, and he can't just say "don't run this application", or "change
the application". Perhaps it's a temporary fix in edk2 (Eric mentioned
release time pressure, IIRC). I can fully appreciate that, and I'm OK to
tolerate temporary fixes -- it's just that temporary fixes sometimes
become permanent (increasing technical debt), and for that reason, I'd
prefer this hack to be as clean and non-intrusive as possible.

Thanks,
Laszlo


>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong, Eric
>> Sent: Friday, September 4, 2020 9:52 AM
>> To: devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Subject: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for
>> Cr3/GDT/IDT.
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2954
>>
>> AP needs to run from real mode to 32bit mode to LONG mode. Page table
>> (pointed by Cr3) and GDT are necessary to set up to correct value when
>> CPU execution mode is switched to LONG mode.
>> AP uses the same location page table (Cr3) and GDT as what BSP uses.
>> But when the page table or GDT is above 4GB, it's impossible for CPU
>> to use because GDTR.base and Cr3 are 32bits before switching to LONG
>> mode.
>>
>> This patch adds check for the Cr3, GDT and IDT to not above 4G limitation.
>>
>> The check is avoided -- assumed successful -- if the new
>> PcdEnableCpuApCr3GdtIdtCheck is FALSE (which is the default). The reason
>> is that the 32-bit requirement is always ensured by edk2 itself; the
>> requirement is only possibly invalidated by a particular UEFI shell
>> application that manually moves the GDT/IDT/CR3 above 4GB. Platforms
>> that don't intend to be compatible with such UEFI applications need not
>> set the PCD to TRUE.
>>
>> If the PCD is TRUE and the check fails, then the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP() and EnableDisableAP() MP service APIs are
>> rejected at once. Reporting an error immediately is more graceful than
>> hanging when the APs attempt to switch to long mode.
>>
>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> V3:
>>
>> only add check for the DxeMplib, no need for PeiMpLib.
>>
>>
>>
>> V2:
>>
>> Change the check point. Just in the different caller to make the logic
>>
>> clear. V1 patch add check just before the use of the code. It make the
>>
>> logic complicated.
>>
>>
>>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 97 +++++++++++++++++++
>>  UefiCpuPkg/UefiCpuPkg.dec                     |  4 +
>>  UefiCpuPkg/UefiCpuPkg.uni                     |  2 +
>>  4 files changed, 104 insertions(+)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> index 1771575c69..7792df516e 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> @@ -74,5 +74,6 @@
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds
>> ## CONSUMES
>>
>>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                          ## CONSUMES
>>
>>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                       ##
>> SOMETIMES_CONSUMES
>>
>> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck               ##
>> CONSUMES
>>
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ##
>> CONSUMES
>>
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ##
>> CONSUMES
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> index 2c00d72dde..332b4447bb 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>> @@ -29,6 +29,66 @@ VOID             *mReservedApLoopFunc = NULL;
>>  UINTN            mReservedTopOfApStack;
>>
>>  volatile UINT32  mNumberToFinish = 0;
>>
>>
>>
>> +/**
>>
>> +  Check whether Cr3/GDT/IDT value valid for the APs.
>>
>> +
>>
>> +  @retval  TRUE          Pass the check.
>>
>> +  @retval  FALSE         Fail the check.
>>
>> +
>>
>> +**/
>>
>> +BOOLEAN
>>
>> +ValidCr3GdtIdtCheck (
>>
>> +  VOID
>>
>> +  )
>>
>> +{
>>
>> +  IA32_DESCRIPTOR   Gdtr;
>>
>> +  IA32_DESCRIPTOR   Idtr;
>>
>> +
>>
>> +  if (!PcdGetBool (PcdEnableCpuApCr3GdtIdtCheck)) {
>>
>> +    return TRUE;
>>
>> +  }
>>
>> +
>>
>> +  //
>>
>> +  // AP needs to run from real mode to 32bit mode to LONG mode. Page table
>>
>> +  // (pointed by Cr3) and GDT are necessary to set up to correct value when
>>
>> +  // CPU execution mode is switched to LONG mode. IDT also necessary if the
>>
>> +  // exception happened.
>>
>> +  // AP uses the same location page table (Cr3) and GDT/IDT as what BSP uses.
>>
>> +  // But when the page table or GDT is above 4GB, it's impossible for CPU
>>
>> +  // to use because GDTR.base and Cr3 are 32bits before switching to LONG
>>
>> +  // mode.
>>
>> +  // Here add check for the Cr3, GDT.Base and range, IDT.Base and range are
>>
>> +  // not above 32 bits limitation.
>>
>> +  //
>>
>> +  if (AsmReadCr3 () >= BASE_4GB) {
>>
>> +    return FALSE;
>>
>> +  }
>>
>> +
>>
>> +  AsmReadGdtr (&Gdtr);
>>
>> +  //
>>
>> +  // Here code needs to check both Gdtr.Base and Gdtr.Base + Gdtr.Limit
>>
>> +  // below BASE_4GB, but Gdtr.Base + Gdtr.Limit below BASE_4GB also means
>>
>> +  // Gdtr.Base below BASE_4GB. so here just add Gdtr.Base + Gdtr.Limit
>>
>> +  // check.
>>
>> +  //
>>
>> +  if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
>>
>> +    return FALSE;
>>
>> +  }
>>
>> +
>>
>> +  AsmReadIdtr (&Idtr);
>>
>> +  //
>>
>> +  // Here code needs to check both Idtr.Base and Idtr.Base + Idtr.Limit
>>
>> +  // below BASE_4GB, but Idtr.Base + Idtr.Limit below BASE_4GB also means
>>
>> +  // Idtr.Base below BASE_4GB. so here just add Idtr.Base + Idtr.Limit
>>
>> +  // check.
>>
>> +  //
>>
>> +  if (Idtr.Base + Idtr.Limit >= BASE_4GB) {
>>
>> +    return FALSE;
>>
>> +  }
>>
>> +
>>
>> +  return TRUE;
>>
>> +}
>>
>> +
>>
>>  /**
>>
>>    Enable Debug Agent to support source debugging on AP function.
>>
>>
>>
>> @@ -394,6 +454,15 @@ MpInitChangeApLoopCallback (
>>  {
>>
>>    CPU_MP_DATA               *CpuMpData;
>>
>>
>>
>> +  //
>>
>> +  // Check the Cr3/GDT/IDT before waking up AP.
>>
>> +  // If the check return fail, it will block later
>>
>> +  // OS boot, so halt the system here.
>>
>> +  //
>>
>> +  if (!ValidCr3GdtIdtCheck ()) {
>>
>> +    CpuDeadLoop ();
>>
>> +  }
>>
>> +
>>
>>    CpuMpData = GetCpuMpData ();
>>
>>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>>
>>    CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
>>
>> @@ -676,6 +745,13 @@ MpInitLibStartupAllAPs (
>>  {
>>
>>    EFI_STATUS              Status;
>>
>>
>>
>> +  //
>>
>> +  // Check whether Cr3/GDT/IDT valid for AP.
>>
>> +  //
>>
>> +  if (!ValidCr3GdtIdtCheck()) {
>>
>> +    return EFI_UNSUPPORTED;
>>
>> +  }
>>
>> +
>>
>>    //
>>
>>    // Temporarily stop checkAllApsStatus for avoid resource dead-lock.
>>
>>    //
>>
>> @@ -783,6 +859,13 @@ MpInitLibStartupThisAP (
>>  {
>>
>>    EFI_STATUS              Status;
>>
>>
>>
>> +  //
>>
>> +  // Check whether Cr3/GDT/IDT valid for AP.
>>
>> +  //
>>
>> +  if (!ValidCr3GdtIdtCheck()) {
>>
>> +    return EFI_UNSUPPORTED;
>>
>> +  }
>>
>> +
>>
>>    //
>>
>>    // temporarily stop checkAllApsStatus for avoid resource dead-lock.
>>
>>    //
>>
>> @@ -839,6 +922,13 @@ MpInitLibSwitchBSP (
>>    EFI_TIMER_ARCH_PROTOCOL      *Timer;
>>
>>    UINT64                       TimerPeriod;
>>
>>
>>
>> +  //
>>
>> +  // Check whether Cr3/GDT/IDT valid for AP.
>>
>> +  //
>>
>> +  if (!ValidCr3GdtIdtCheck()) {
>>
>> +    return EFI_UNSUPPORTED;
>>
>> +  }
>>
>> +
>>
>>    TimerPeriod = 0;
>>
>>    //
>>
>>    // Locate Timer Arch Protocol
>>
>> @@ -912,6 +1002,13 @@ MpInitLibEnableDisableAP (
>>    EFI_STATUS     Status;
>>
>>    BOOLEAN        TempStopCheckState;
>>
>>
>>
>> +  //
>>
>> +  // Check whether Cr3/GDT/IDT valid for AP.
>>
>> +  //
>>
>> +  if (!ValidCr3GdtIdtCheck()) {
>>
>> +    return EFI_UNSUPPORTED;
>>
>> +  }
>>
>> +
>>
>>    TempStopCheckState = FALSE;
>>
>>    //
>>
>>    // temporarily stop checkAllAPsStatus for initialize parameters.
>>
>> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
>> index d83c084467..08ec36e76c 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.dec
>> +++ b/UefiCpuPkg/UefiCpuPkg.dec
>> @@ -148,6 +148,10 @@
>>    # @Prompt Lock SMM Feature Control MSR.
>>
>>
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOL
>> EAN|0x3213210B
>>
>>
>>
>> +  ## This value specifies whether need to check the Cr3/GDT/IDT value for the
>> APs.
>>
>> +  # @Prompt Enable Cr3/GDT/IDT value check for the APs.
>>
>> +
>> gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEA
>> N|0x30000044
>>
>> +
>>
>>  [PcdsFixedAtBuild]
>>
>>    ## List of exception vectors which need switching stack.
>>
>>    #  This PCD will only take into effect if PcdCpuStackGuard is enabled.
>>
>> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
>> index 219c1963bf..3461a83003 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.uni
>> +++ b/UefiCpuPkg/UefiCpuPkg.uni
>> @@ -96,6 +96,8 @@
>>
>>
>>  #string
>> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_PROMP
>> T  #language en-US "Lock SMM Feature Control MSR"
>>
>>
>>
>> +#string
>> STR_gUefiCpuPkgTokenSpaceGuid_PcdEnableCpuApCr3GdtIdtCheck_PROMPT
>> #language en-US "Enable Cr3/GDT/IDT value check for the APs"
>>
>> +
>>
>>  #string
>> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HELP
>> #language en-US "Lock SMM Feature Control MSR?<BR><BR>\n"
>>
>>                                                                                             "TRUE  - locked.<BR>\n"
>>
>>                                                                                             "FALSE - unlocked.<BR>"
>>
>> --
>> 2.23.0.windows.1
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#65019): https://edk2.groups.io/g/devel/message/65019
>> Mute This Topic: https://groups.io/mt/76621468/1772286
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jiewen.yao@intel.com]
>> -=-=-=-=-=-=
>

Re: [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 03:51, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2954
> 
> AP needs to run from real mode to 32bit mode to LONG mode. Page table
> (pointed by Cr3) and GDT are necessary to set up to correct value when
> CPU execution mode is switched to LONG mode.
> AP uses the same location page table (Cr3) and GDT as what BSP uses.
> But when the page table or GDT is above 4GB, it's impossible for CPU
> to use because GDTR.base and Cr3 are 32bits before switching to LONG
> mode.
> 
> This patch adds check for the Cr3, GDT and IDT to not above 4G limitation.
> 
> The check is avoided -- assumed successful -- if the new
> PcdEnableCpuApCr3GdtIdtCheck is FALSE (which is the default). The reason
> is that the 32-bit requirement is always ensured by edk2 itself; the
> requirement is only possibly invalidated by a particular UEFI shell
> application that manually moves the GDT/IDT/CR3 above 4GB. Platforms
> that don't intend to be compatible with such UEFI applications need not
> set the PCD to TRUE.
> 
> If the PCD is TRUE and the check fails, then the StartupAllAPs(),
> StartupThisAP(), SwitchBSP() and EnableDisableAP() MP service APIs are
> rejected at once. Reporting an error immediately is more graceful than
> hanging when the APs attempt to switch to long mode.
> 
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> V3:
> only add check for the DxeMplib, no need for PeiMpLib.
> 
> V2:
> Change the check point. Just in the different caller to make the logic
> clear. V1 patch add check just before the use of the code. It make the
> logic complicated.
> 
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 97 +++++++++++++++++++
>  UefiCpuPkg/UefiCpuPkg.dec                     |  4 +
>  UefiCpuPkg/UefiCpuPkg.uni                     |  2 +
>  4 files changed, 104 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 1771575c69..7792df516e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -74,5 +74,6 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds  ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                          ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                       ## SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck               ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ## CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 2c00d72dde..332b4447bb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -29,6 +29,66 @@ VOID             *mReservedApLoopFunc = NULL;
>  UINTN            mReservedTopOfApStack;
>  volatile UINT32  mNumberToFinish = 0;
>  
> +/**
> +  Check whether Cr3/GDT/IDT value valid for the APs.
> +
> +  @retval  TRUE          Pass the check.
> +  @retval  FALSE         Fail the check.
> +
> +**/
> +BOOLEAN
> +ValidCr3GdtIdtCheck (
> +  VOID
> +  )
> +{
> +  IA32_DESCRIPTOR   Gdtr;
> +  IA32_DESCRIPTOR   Idtr;
> +
> +  if (!PcdGetBool (PcdEnableCpuApCr3GdtIdtCheck)) {
> +    return TRUE;
> +  }
> +
> +  //
> +  // AP needs to run from real mode to 32bit mode to LONG mode. Page table
> +  // (pointed by Cr3) and GDT are necessary to set up to correct value when
> +  // CPU execution mode is switched to LONG mode. IDT also necessary if the
> +  // exception happened.
> +  // AP uses the same location page table (Cr3) and GDT/IDT as what BSP uses.
> +  // But when the page table or GDT is above 4GB, it's impossible for CPU
> +  // to use because GDTR.base and Cr3 are 32bits before switching to LONG
> +  // mode.
> +  // Here add check for the Cr3, GDT.Base and range, IDT.Base and range are
> +  // not above 32 bits limitation.
> +  //
> +  if (AsmReadCr3 () >= BASE_4GB) {
> +    return FALSE;
> +  }
> +
> +  AsmReadGdtr (&Gdtr);
> +  //
> +  // Here code needs to check both Gdtr.Base and Gdtr.Base + Gdtr.Limit
> +  // below BASE_4GB, but Gdtr.Base + Gdtr.Limit below BASE_4GB also means
> +  // Gdtr.Base below BASE_4GB. so here just add Gdtr.Base + Gdtr.Limit
> +  // check.
> +  //
> +  if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
> +    return FALSE;
> +  }
> +
> +  AsmReadIdtr (&Idtr);
> +  //
> +  // Here code needs to check both Idtr.Base and Idtr.Base + Idtr.Limit
> +  // below BASE_4GB, but Idtr.Base + Idtr.Limit below BASE_4GB also means
> +  // Idtr.Base below BASE_4GB. so here just add Idtr.Base + Idtr.Limit
> +  // check.
> +  //
> +  if (Idtr.Base + Idtr.Limit >= BASE_4GB) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
>  /**
>    Enable Debug Agent to support source debugging on AP function.
>  
> @@ -394,6 +454,15 @@ MpInitChangeApLoopCallback (
>  {
>    CPU_MP_DATA               *CpuMpData;
>  
> +  //
> +  // Check the Cr3/GDT/IDT before waking up AP.
> +  // If the check return fail, it will block later
> +  // OS boot, so halt the system here.
> +  //
> +  if (!ValidCr3GdtIdtCheck ()) {
> +    CpuDeadLoop ();
> +  }
> +
>    CpuMpData = GetCpuMpData ();
>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>    CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
> @@ -676,6 +745,13 @@ MpInitLibStartupAllAPs (
>  {
>    EFI_STATUS              Status;
>  
> +  //
> +  // Check whether Cr3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCr3GdtIdtCheck()) {

(1) missing space character

> +    return EFI_UNSUPPORTED;
> +  }
> +
>    //
>    // Temporarily stop checkAllApsStatus for avoid resource dead-lock.
>    //
> @@ -783,6 +859,13 @@ MpInitLibStartupThisAP (
>  {
>    EFI_STATUS              Status;
>  
> +  //
> +  // Check whether Cr3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCr3GdtIdtCheck()) {

(2) missing space character

> +    return EFI_UNSUPPORTED;
> +  }
> +
>    //
>    // temporarily stop checkAllApsStatus for avoid resource dead-lock.
>    //
> @@ -839,6 +922,13 @@ MpInitLibSwitchBSP (
>    EFI_TIMER_ARCH_PROTOCOL      *Timer;
>    UINT64                       TimerPeriod;
>  
> +  //
> +  // Check whether Cr3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCr3GdtIdtCheck()) {

(3) missing space character

> +    return EFI_UNSUPPORTED;
> +  }
> +
>    TimerPeriod = 0;
>    //
>    // Locate Timer Arch Protocol
> @@ -912,6 +1002,13 @@ MpInitLibEnableDisableAP (
>    EFI_STATUS     Status;
>    BOOLEAN        TempStopCheckState;
>  
> +  //
> +  // Check whether Cr3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCr3GdtIdtCheck()) {

(4) missing space character

> +    return EFI_UNSUPPORTED;
> +  }
> +
>    TempStopCheckState = FALSE;
>    //
>    // temporarily stop checkAllAPsStatus for initialize parameters.
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index d83c084467..08ec36e76c 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -148,6 +148,10 @@
>    # @Prompt Lock SMM Feature Control MSR.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOLEAN|0x3213210B
>  
> +  ## This value specifies whether need to check the Cr3/GDT/IDT value for the APs.
> +  # @Prompt Enable Cr3/GDT/IDT value check for the APs.
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEAN|0x30000044
> +
>  [PcdsFixedAtBuild]
>    ## List of exception vectors which need switching stack.
>    #  This PCD will only take into effect if PcdCpuStackGuard is enabled.
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index 219c1963bf..3461a83003 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -96,6 +96,8 @@
>  
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_PROMPT  #language en-US "Lock SMM Feature Control MSR"
>  
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdEnableCpuApCr3GdtIdtCheck_PROMPT  #language en-US "Enable Cr3/GDT/IDT value check for the APs"
> +
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmFeatureControlMsrLock_HELP  #language en-US "Lock SMM Feature Control MSR?<BR><BR>\n"
>                                                                                             "TRUE  - locked.<BR>\n"
>                                                                                             "FALSE - unlocked.<BR>"
> 

(5) For consistency with the other entries in the UNI file, I think:

(5a) "PcdEnableCpuApCr3GdtIdtCheck_PROMPT" should come after
"PcdCpuSmmFeatureControlMsrLock_HELP", and

(5b) we should add a HELP item as well, for the new entry.

(Personally I don't care for UNI files *at all*, but I think it's best
to preserve consistency with the existent contents of this file.)

Thanks!
Laszlo