[PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[PATCH v2] 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.Base and IDT.Base to not above
32 bits restriction.

Change-Id: I231180f45d9f542641082c57d001e38e3f6759d5
Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---

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       |  9 +++
 UefiCpuPkg/Library/MpInitLib/MpLib.c          | 76 +++++++++++++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.h          | 12 +++
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  1 +
 UefiCpuPkg/UefiCpuPkg.dec                     |  4 +
 6 files changed, 103 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 1771575c69..20851f251a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -76,3 +76,4 @@
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                       ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck               ## CONSUMES
\ No newline at end of file
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 2c00d72dde..f598372c4d 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -394,6 +394,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 ();
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 07426274f6..69a0372df7 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1909,6 +1909,54 @@ CheckAllAPs (
   return EFI_NOT_READY;
 }
 
+/**
+  Check whether CR3/GDT/IDT value valid for AP.
+
+  @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 ((IA32_DESCRIPTOR *) &Gdtr);
+  if ((Gdtr.Base >= BASE_4GB) || (Gdtr.Base + Gdtr.Limit >= BASE_4GB)) {
+    return FALSE;
+  }
+
+  AsmReadIdtr ((IA32_DESCRIPTOR *) &Idtr);
+  if ((Idtr.Base >= BASE_4GB) || (Idtr.Base + Idtr.Limit >= BASE_4GB)) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
 /**
   MP Initialize Library initialization.
 
@@ -2318,6 +2366,13 @@ SwitchBSPWorker (
     return EFI_NOT_READY;
   }
 
+  //
+  // Check whether CR3/GDT/IDT valid for AP.
+  //
+  if (!ValidCR3GdtIdtCheck()) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   CpuMpData->BSPInfo.State = CPU_SWITCH_STATE_IDLE;
   CpuMpData->APInfo.State  = CPU_SWITCH_STATE_IDLE;
   CpuMpData->SwitchBspFlag = TRUE;
@@ -2420,6 +2475,13 @@ EnableDisableApWorker (
     return EFI_NOT_FOUND;
   }
 
+  //
+  // Check whether CR3/GDT/IDT valid for AP.
+  //
+  if (!ValidCR3GdtIdtCheck()) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   if (!EnableAP) {
     SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateDisabled);
   } else {
@@ -2607,6 +2669,13 @@ StartupAllCPUsWorker (
     return EFI_DEVICE_ERROR;
   }
 
+  //
+  // Check whether CR3/GDT/IDT valid for AP.
+  //
+  if (!ValidCR3GdtIdtCheck()) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   //
   // Update AP state
   //
@@ -2772,6 +2841,13 @@ StartupThisAPWorker (
     return EFI_INVALID_PARAMETER;
   }
 
+  //
+  // Check whether CR3/GDT/IDT valid for AP.
+  //
+  if (!ValidCR3GdtIdtCheck()) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   //
   // Update AP state
   //
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 02652eaae1..4ac5cb51e3 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -740,5 +740,17 @@ PlatformShadowMicrocode (
   IN OUT CPU_MP_DATA             *CpuMpData
   );
 
+/**
+  Check whether CR3/GDT/IDT value valid for AP.
+
+  @retval  TRUE          Pass the check.
+  @retval  FALSE         Fail the check.
+
+**/
+BOOLEAN
+ValidCR3GdtIdtCheck (
+  VOID
+  );
+
 #endif
 
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
index 34abf25d43..0ca86fcdaa 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
@@ -65,6 +65,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                      ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                   ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                       ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck           ## CONSUMES
 
 [Ppis]
   gEdkiiPeiShadowMicrocodePpiGuid        ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index d83c084467..467ec5e001 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -187,6 +187,10 @@
   # @Prompt Configure stack size for Application Processor (AP)
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize|0x8000|UINT32|0x00000003
 
+  ## This value specifies whether need to check the CR3/GDT/IDT value for AP.
+  # @Prompt Whether need to check the CR3/GDT/IDT value for AP
+  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEAN|0x30000044
+
   ## Specifies stack size in the temporary RAM. 0 means half of TemporaryRamSize.
   # @Prompt Stack size in the temporary RAM.
   gUefiCpuPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0|UINT32|0x10001003
-- 
2.23.0.windows.1

Re: [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/03/20 17:11, 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.Base and IDT.Base to not above
> 32 bits restriction.
> 
> Change-Id: I231180f45d9f542641082c57d001e38e3f6759d5

(1) Please drop the Change-Id line.


(2) We should document that the motivation for this patch is a special
UEFI shell application that changes the GDT / CR3 to above 4GB.
Currently, neither the bugzilla, nor the commit message, nor the PCD
documentation in the DEC file explains this. So one is left wondering if
and why they should change the PCD to TRUE on their platform.

Please append the following two paragraphs to the commit message:

"""
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>
> ---
> 
> 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       |  9 +++
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 76 +++++++++++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 12 +++
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  1 +
>  UefiCpuPkg/UefiCpuPkg.dec                     |  4 +
>  6 files changed, 103 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 1771575c69..20851f251a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -76,3 +76,4 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                       ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck               ## CONSUMES
> \ No newline at end of file

(3) Can you insert this new line just after
"gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase" please? In order to
keep the "gUefiCpuPkgTokenSpaceGuid" PCDs grouped together.


> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 2c00d72dde..f598372c4d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -394,6 +394,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()) {

(4) Missing space character after "ValidCR3GdtIdtCheck".

(Applies to all the other call sites as well.)


> +    CpuDeadLoop ();
> +  }
> +
>    CpuMpData = GetCpuMpData ();
>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>    CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 07426274f6..69a0372df7 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1909,6 +1909,54 @@ CheckAllAPs (
>    return EFI_NOT_READY;
>  }
>  
> +/**
> +  Check whether CR3/GDT/IDT value valid for AP.
> +
> +  @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 ((IA32_DESCRIPTOR *) &Gdtr);

(5) The cast is superfluous, please remove it. According to <BaseLib.h>,
AsmReadGdtr() takes the following parameter:

  OUT     IA32_DESCRIPTOR           *Gdtr


> +  if ((Gdtr.Base >= BASE_4GB) || (Gdtr.Base + Gdtr.Limit >= BASE_4GB)) {
> +    return FALSE;
> +  }

I agree that both checks (even the second one) should use ">=", given
that Limit is an inclusive boundary.

My understanding is that we don't have to worry about any UINTN overflow
in the addition here, as Base and Limit come from the "live" GDTR.

(6) So we could even eliminate the first expression, and only do:

  if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
    return FALSE;
  }

Because, given that we do not expect any overflows here, if this (2nd)
condition is FALSE, then the 1st condition must be FALSE too.

I don't insist on this simplification. If you also like the
simplification, then please
- update the commit message
- update the code comment above

because we will no longer check Base in itself.


> +
> +  AsmReadIdtr ((IA32_DESCRIPTOR *) &Idtr);

(7) The cast is superfluous.


> +  if ((Idtr.Base >= BASE_4GB) || (Idtr.Base + Idtr.Limit >= BASE_4GB)) {
> +    return FALSE;
> +  }

(8) Same comment as (6) -- please consider removing the 1st condition as
a simplification (if you disagree, that's OK too).


> +
> +  return TRUE;
> +}
> +
>  /**
>    MP Initialize Library initialization.
>  
> @@ -2318,6 +2366,13 @@ SwitchBSPWorker (
>      return EFI_NOT_READY;
>    }
>  
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    CpuMpData->BSPInfo.State = CPU_SWITCH_STATE_IDLE;
>    CpuMpData->APInfo.State  = CPU_SWITCH_STATE_IDLE;
>    CpuMpData->SwitchBspFlag = TRUE;

(9) I think we should return EFI_UNSUPPORTED, not EFI_INVALID_PARAMETER.


(10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".

Instead, the calls should be made in the DXE instance of the library
("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
functions:

- MpInitLibStartupAllAPs
- MpInitLibStartupThisAP
- MpInitLibSwitchBSP
- MpInitLibEnableDisableAP

Here's why:

(a) The symptom does not affect the PEI phase -- by the time the UEFI
application is executed, the PEI phase has ended; there's no need to
modify the PEI instance of the library.

(b) The currently proposed failure exits are too late. For example, in
the SwitchBSPWorker() function, by the time we exit, we have called
DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
DisableLvtInterrupts() -- and the error path does not restore the
original environment.

(c) According to the PI spec (v1.7), the StartupAllAPs(),
StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
first action in the above-listed DxeMpLib functions.

(Assuming the protocol members are called from an AP, and consequently
we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
*caller's* fault, per spec!)


> @@ -2420,6 +2475,13 @@ EnableDisableApWorker (
>      return EFI_NOT_FOUND;
>    }
>  
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    if (!EnableAP) {
>      SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateDisabled);
>    } else {
> @@ -2607,6 +2669,13 @@ StartupAllCPUsWorker (
>      return EFI_DEVICE_ERROR;
>    }
>  
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    //
>    // Update AP state
>    //
> @@ -2772,6 +2841,13 @@ StartupThisAPWorker (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    //
>    // Update AP state
>    //
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 02652eaae1..4ac5cb51e3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -740,5 +740,17 @@ PlatformShadowMicrocode (
>    IN OUT CPU_MP_DATA             *CpuMpData
>    );
>  
> +/**
> +  Check whether CR3/GDT/IDT value valid for AP.
> +
> +  @retval  TRUE          Pass the check.
> +  @retval  FALSE         Fail the check.
> +
> +**/
> +BOOLEAN
> +ValidCR3GdtIdtCheck (

(11) In the function name, please write "Cr3", not "CR3". (See also:
AsmReadCr3().)


> +  VOID
> +  );
> +
>  #endif
>  
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 34abf25d43..0ca86fcdaa 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -65,6 +65,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                      ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                   ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                       ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck           ## CONSUMES
>  
>  [Ppis]
>    gEdkiiPeiShadowMicrocodePpiGuid        ## SOMETIMES_CONSUMES

(12) Superficially, comment (3) applies -- please keep the PCDs in the
UefiCpuPkg token space grouped together.

However, per my point (10), the PEI instance of the library should not
be modified at all; so please drop this hunk.


> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index d83c084467..467ec5e001 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -187,6 +187,10 @@
>    # @Prompt Configure stack size for Application Processor (AP)
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize|0x8000|UINT32|0x00000003
>  
> +  ## This value specifies whether need to check the CR3/GDT/IDT value for AP.
> +  # @Prompt Whether need to check the CR3/GDT/IDT value for AP
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEAN|0x30000044
> +
>    ## Specifies stack size in the temporary RAM. 0 means half of TemporaryRamSize.
>    # @Prompt Stack size in the temporary RAM.
>    gUefiCpuPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0|UINT32|0x10001003
> 

OK, so this is [PcdsFixedAtBuild, PcdsPatchableInModule], not a
FeaturePCD. I proposed FeaturePCD because optimizing compilers are
supposed to eliminate it... But, I can see that BaseTools generates the
same kind of code for Fixed BOOLEAN PCDs as well. OK!

(13) I think you should modify the UNI file in accordance with the DEC file.

Thanks!
Laszlo

Re: [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/03/20 21:00, Laszlo Ersek wrote:

> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
> 
> Instead, the calls should be made in the DXE instance of the library
> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
> functions:
> 
> - MpInitLibStartupAllAPs
> - MpInitLibStartupThisAP
> - MpInitLibSwitchBSP
> - MpInitLibEnableDisableAP
> 
> Here's why:
> 
> (a) The symptom does not affect the PEI phase -- by the time the UEFI
> application is executed, the PEI phase has ended; there's no need to
> modify the PEI instance of the library.
> 
> (b) The currently proposed failure exits are too late. For example, in
> the SwitchBSPWorker() function, by the time we exit, we have called
> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
> DisableLvtInterrupts() -- and the error path does not restore the
> original environment.
> 
> (c) According to the PI spec (v1.7), the StartupAllAPs(),
> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
> first action in the above-listed DxeMpLib functions.
> 
> (Assuming the protocol members are called from an AP, and consequently
> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
> *caller's* fault, per spec!)

This means we can move the ValidCr3GdtIdtCheck() function to
"DxeMpLib.c", and it is not necessary to modify "MpLib.h".

Thanks
Laszlo

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

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 2398 bytes --]

Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.

We should not assume PEI runs at 32 bit mode.

________________________________
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek <lersek@redhat.com>
发送时间: Friday, September 4, 2020 3:55:47 AM
收件人: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>
抄送: Ni, Ray <ray.ni@intel.com>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/03/20 21:00, Laszlo Ersek wrote:

> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>
> Instead, the calls should be made in the DXE instance of the library
> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
> functions:
>
> - MpInitLibStartupAllAPs
> - MpInitLibStartupThisAP
> - MpInitLibSwitchBSP
> - MpInitLibEnableDisableAP
>
> Here's why:
>
> (a) The symptom does not affect the PEI phase -- by the time the UEFI
> application is executed, the PEI phase has ended; there's no need to
> modify the PEI instance of the library.
>
> (b) The currently proposed failure exits are too late. For example, in
> the SwitchBSPWorker() function, by the time we exit, we have called
> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
> DisableLvtInterrupts() -- and the error path does not restore the
> original environment.
>
> (c) According to the PI spec (v1.7), the StartupAllAPs(),
> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
> first action in the above-listed DxeMpLib functions.
>
> (Assuming the protocol members are called from an AP, and consequently
> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
> *caller's* fault, per spec!)

This means we can move the ValidCr3GdtIdtCheck() function to
"DxeMpLib.c", and it is not necessary to modify "MpLib.h".

Thanks
Laszlo





[-- Attachment #2: Type: text/html, Size: 3519 bytes --]

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

[Dong, Eric]

[-- Attachment #1: Type: text/plain, Size: 3095 bytes --]

I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.

Thanks,
Eric

From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, September 4, 2020 9:34 AM
To: devel@edk2.groups.io; lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
Cc: Lou, Yun <yun.lou@intel.com>
Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.

We should not assume PEI runs at 32 bit mode.

________________________________
发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
发送时间: Friday, September 4, 2020 3:55:47 AM
收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/03/20 21:00, Laszlo Ersek wrote:

> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>
> Instead, the calls should be made in the DXE instance of the library
> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
> functions:
>
> - MpInitLibStartupAllAPs
> - MpInitLibStartupThisAP
> - MpInitLibSwitchBSP
> - MpInitLibEnableDisableAP
>
> Here's why:
>
> (a) The symptom does not affect the PEI phase -- by the time the UEFI
> application is executed, the PEI phase has ended; there's no need to
> modify the PEI instance of the library.
>
> (b) The currently proposed failure exits are too late. For example, in
> the SwitchBSPWorker() function, by the time we exit, we have called
> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
> DisableLvtInterrupts() -- and the error path does not restore the
> original environment.
>
> (c) According to the PI spec (v1.7), the StartupAllAPs(),
> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
> first action in the above-listed DxeMpLib functions.
>
> (Assuming the protocol members are called from an AP, and consequently
> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
> *caller's* fault, per spec!)

This means we can move the ValidCr3GdtIdtCheck() function to
"DxeMpLib.c", and it is not necessary to modify "MpLib.h".

Thanks
Laszlo




[-- Attachment #2: Type: text/html, Size: 8498 bytes --]

回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[vanjeff_919]

[-- Attachment #1.1: Type: text/plain, Size: 3571 bytes --]

Laszlo & Eric,

Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

Jeff

发件人: Dong, Eric<mailto:eric.dong@intel.com>
发送时间: 2020年9月4日 10:01
收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.

Thanks,
Eric

From: Ni, Ray <ray.ni@intel.com>
Sent: Friday, September 4, 2020 9:34 AM
To: devel@edk2.groups.io; lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
Cc: Lou, Yun <yun.lou@intel.com>
Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.

We should not assume PEI runs at 32 bit mode.


发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
发送时间: Friday, September 4, 2020 3:55:47 AM
收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/03/20 21:00, Laszlo Ersek wrote:

> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>
> Instead, the calls should be made in the DXE instance of the library
> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
> functions:
>
> - MpInitLibStartupAllAPs
> - MpInitLibStartupThisAP
> - MpInitLibSwitchBSP
> - MpInitLibEnableDisableAP
>
> Here's why:
>
> (a) The symptom does not affect the PEI phase -- by the time the UEFI
> application is executed, the PEI phase has ended; there's no need to
> modify the PEI instance of the library.
>
> (b) The currently proposed failure exits are too late. For example, in
> the SwitchBSPWorker() function, by the time we exit, we have called
> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
> DisableLvtInterrupts() -- and the error path does not restore the
> original environment.
>
> (c) According to the PI spec (v1.7), the StartupAllAPs(),
> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
> first action in the above-listed DxeMpLib functions.
>
> (Assuming the protocol members are called from an AP, and consequently
> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
> *caller's* fault, per spec!)

This means we can move the ValidCr3GdtIdtCheck() function to
"DxeMpLib.c", and it is not necessary to modify "MpLib.h".

Thanks
Laszlo




[-- Attachment #1.2: Type: text/html, Size: 11940 bytes --]

[-- Attachment #2: 89184FA29A184BD3AE9BDABEFEE0BA6A.png --]
[-- Type: image/png, Size: 144 bytes --]

[-- Attachment #3: 435BFC14D84B499EABDAF41539376076.png --]
[-- Type: image/png, Size: 132 bytes --]

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

[Dong, Eric]

[-- Attachment #1.1: Type: text/plain, Size: 4156 bytes --]

Agree, current is too much PCDs for the driver.

Laszlo, what’s your comments?

Thanks,
Eric
From: Fan Jeff <vanjeff_919@hotmail.com>
Sent: Friday, September 4, 2020 10:19 AM
To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
Cc: Lou, Yun <yun.lou@intel.com>
Subject: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Laszlo & Eric,

Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

Jeff

发件人: Dong, Eric<mailto:eric.dong@intel.com>
发送时间: 2020年9月4日 10:01
收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.

Thanks,
Eric

From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Sent: Friday, September 4, 2020 9:34 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.

We should not assume PEI runs at 32 bit mode.


发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
发送时间: Friday, September 4, 2020 3:55:47 AM
收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/03/20 21:00, Laszlo Ersek wrote:

> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>
> Instead, the calls should be made in the DXE instance of the library
> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
> functions:
>
> - MpInitLibStartupAllAPs
> - MpInitLibStartupThisAP
> - MpInitLibSwitchBSP
> - MpInitLibEnableDisableAP
>
> Here's why:
>
> (a) The symptom does not affect the PEI phase -- by the time the UEFI
> application is executed, the PEI phase has ended; there's no need to
> modify the PEI instance of the library.
>
> (b) The currently proposed failure exits are too late. For example, in
> the SwitchBSPWorker() function, by the time we exit, we have called
> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
> DisableLvtInterrupts() -- and the error path does not restore the
> original environment.
>
> (c) According to the PI spec (v1.7), the StartupAllAPs(),
> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
> first action in the above-listed DxeMpLib functions.
>
> (Assuming the protocol members are called from an AP, and consequently
> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
> *caller's* fault, per spec!)

This means we can move the ValidCr3GdtIdtCheck() function to
"DxeMpLib.c", and it is not necessary to modify "MpLib.h".

Thanks
Laszlo



[-- Attachment #1.2: Type: text/html, Size: 13568 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 144 bytes --]

[-- Attachment #3: image002.png --]
[-- Type: image/png, Size: 132 bytes --]

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

[Yao, Jiewen]

[-- Attachment #1.1: Type: text/plain, Size: 4975 bytes --]

Hey
I would like to ask the original requirement.

Why there is a test case to test such configuration, to move the page table or GDT or IDT? I don’t understand.

Is that complexity really needed ?

If we treat it as an invalid test case, then we don’t need do anything here, right ?




From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong, Eric
Sent: Friday, September 4, 2020 10:27 AM
To: Fan Jeff <vanjeff_919@hotmail.com>; devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
Cc: Lou, Yun <yun.lou@intel.com>
Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Agree, current is too much PCDs for the driver.

Laszlo, what’s your comments?

Thanks,
Eric
From: Fan Jeff <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>>
Sent: Friday, September 4, 2020 10:19 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; lersek@redhat.com<mailto:lersek@redhat.com>
Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
Subject: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Laszlo & Eric,

Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

Jeff

发件人: Dong, Eric<mailto:eric.dong@intel.com>
发送时间: 2020年9月4日 10:01
收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.

Thanks,
Eric

From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Sent: Friday, September 4, 2020 9:34 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.

We should not assume PEI runs at 32 bit mode.


发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
发送时间: Friday, September 4, 2020 3:55:47 AM
收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/03/20 21:00, Laszlo Ersek wrote:

> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>
> Instead, the calls should be made in the DXE instance of the library
> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
> functions:
>
> - MpInitLibStartupAllAPs
> - MpInitLibStartupThisAP
> - MpInitLibSwitchBSP
> - MpInitLibEnableDisableAP
>
> Here's why:
>
> (a) The symptom does not affect the PEI phase -- by the time the UEFI
> application is executed, the PEI phase has ended; there's no need to
> modify the PEI instance of the library.
>
> (b) The currently proposed failure exits are too late. For example, in
> the SwitchBSPWorker() function, by the time we exit, we have called
> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
> DisableLvtInterrupts() -- and the error path does not restore the
> original environment.
>
> (c) According to the PI spec (v1.7), the StartupAllAPs(),
> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
> first action in the above-listed DxeMpLib functions.
>
> (Assuming the protocol members are called from an AP, and consequently
> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
> *caller's* fault, per spec!)

This means we can move the ValidCr3GdtIdtCheck() function to
"DxeMpLib.c", and it is not necessary to modify "MpLib.h".

Thanks
Laszlo



[-- Attachment #1.2: Type: text/html, Size: 16406 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 144 bytes --]

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

[Laszlo Ersek]

On 09/04/20 05:09, Yao, Jiewen wrote:
> Hey
> I would like to ask the original requirement.
> 
> Why there is a test case to test such configuration, to move the page table or GDT or IDT? I don’t understand.

+1

> Is that complexity really needed ?
> 
> If we treat it as an invalid test case, then we don’t need do anything here, right ?

Exactly.

My understanding is just that Eric really needs this application to
*not* hang (if it fails gracefully, that seems to be OK). I'm happy to
accommodate that, but:

- the MpInitLib code should suffer as little as possible complications
as a result,
- other platforms should not take a hit *at all*.

Laszlo

> 
> 
> 
> 
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong, Eric
> Sent: Friday, September 4, 2020 10:27 AM
> To: Fan Jeff <vanjeff_919@hotmail.com>; devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
> Cc: Lou, Yun <yun.lou@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> Agree, current is too much PCDs for the driver.
> 
> Laszlo, what’s your comments?
> 
> Thanks,
> Eric
> From: Fan Jeff <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>>
> Sent: Friday, September 4, 2020 10:19 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; lersek@redhat.com<mailto:lersek@redhat.com>
> Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
> Subject: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> Laszlo & Eric,
> 
> Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.
> 
> Jeff
> 
> 发件人: Dong, Eric<mailto:eric.dong@intel.com>
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Lou, Yun<mailto:yun.lou@intel.com>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.
> 
> Thanks,
> Eric
> 
> From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Sent: Friday, September 4, 2020 9:34 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
> We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.
> 
> We should not assume PEI runs at 32 bit mode.
> 
> 
> 发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
> 抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> On 09/03/20 21:00, Laszlo Ersek wrote:
> 
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called
>> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
>> DisableLvtInterrupts() -- and the error path does not restore the
>> original environment.
>>
>> (c) According to the PI spec (v1.7), the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
>> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
>> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
>> first action in the above-listed DxeMpLib functions.
>>
>> (Assuming the protocol members are called from an AP, and consequently
>> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
>> *caller's* fault, per spec!)
> 
> This means we can move the ValidCr3GdtIdtCheck() function to
> "DxeMpLib.c", and it is not necessary to modify "MpLib.h".
> 
> Thanks
> Laszlo
> 
> 
> 

Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
> 
> Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Now, *if* FillExchangeInfoData() is currently *wrong* to have these
32-bit assumptions, because edk2 modules themselves can break those
assumptions (without the custom UEFI application), then we have a more
serious problem. But for that problem, just "checking and rejecting" is
not a sufficient solution, regardless of how and where we check and reject.

Thanks
Laszlo




> 
> Jeff
> 
> 发件人: Dong, Eric<mailto:eric.dong@intel.com>
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Lou, Yun<mailto:yun.lou@intel.com>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.
> 
> Thanks,
> Eric
> 
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, September 4, 2020 9:34 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
> Cc: Lou, Yun <yun.lou@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
> We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.
> 
> We should not assume PEI runs at 32 bit mode.
> 
> 
> 发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
> 抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> On 09/03/20 21:00, Laszlo Ersek wrote:
> 
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called
>> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
>> DisableLvtInterrupts() -- and the error path does not restore the
>> original environment.
>>
>> (c) According to the PI spec (v1.7), the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
>> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
>> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
>> first action in the above-listed DxeMpLib functions.
>>
>> (Assuming the protocol members are called from an AP, and consequently
>> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
>> *caller's* fault, per spec!)
> 
> This means we can move the ValidCr3GdtIdtCheck() function to
> "DxeMpLib.c", and it is not necessary to modify "MpLib.h".
> 
> Thanks
> Laszlo
> 
> 
> 

回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Fan Jeff]

[-- Attachment #1: Type: text/plain, Size: 6121 bytes --]

Laszlo,

Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is that we CANNOT assume original values are still valid during POST phase.

On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one function. Validating them are necessary.

For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.

Jeff

发件人: Laszlo Ersek<mailto:lersek@redhat.com>
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff<mailto:vanjeff_919@hotmail.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; eric.dong@intel.com<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
>
> Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Now, *if* FillExchangeInfoData() is currently *wrong* to have these
32-bit assumptions, because edk2 modules themselves can break those
assumptions (without the custom UEFI application), then we have a more
serious problem. But for that problem, just "checking and rejecting" is
not a sufficient solution, regardless of how and where we check and reject.

Thanks
Laszlo




>
> Jeff
>
> 发件人: Dong, Eric<mailto:eric.dong@intel.com>
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Lou, Yun<mailto:yun.lou@intel.com>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.
>
> Thanks,
> Eric
>
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, September 4, 2020 9:34 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
> Cc: Lou, Yun <yun.lou@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
> We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.
>
> We should not assume PEI runs at 32 bit mode.
>
>
> 发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
> 抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> On 09/03/20 21:00, Laszlo Ersek wrote:
>
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called
>> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
>> DisableLvtInterrupts() -- and the error path does not restore the
>> original environment.
>>
>> (c) According to the PI spec (v1.7), the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
>> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
>> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
>> first action in the above-listed DxeMpLib functions.
>>
>> (Assuming the protocol members are called from an AP, and consequently
>> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
>> *caller's* fault, per spec!)
>
> This means we can move the ValidCr3GdtIdtCheck() function to
> "DxeMpLib.c", and it is not necessary to modify "MpLib.h".
>
> Thanks
> Laszlo
>
> 
>


[-- Attachment #2: Type: text/html, Size: 11264 bytes --]

Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Yao, Jiewen]

[-- Attachment #1: Type: text/plain, Size: 8707 bytes --]

When we say “validate”, we need get clear on what is the contract between caller and callee, and what is the expectation of the validation.

For example, in this case, the validation is limited to 4G or not 4G. Why?
This function does not verify following, why?

  1.  if the GDT table is valid.
  2.  if the IDT table is valid
  3.  if the exception handler is valid
  4.  if the timer handler still works
  5.  if the page table is valid
  6.  if the page table is 1:1 mapping
  7.  if the page table still enforce the expected protection, such as RO, NX
  8.  ……

If an application creates a crazy state, CpuDxe may pass the validation with below 4G page table, but still fail to wake up APs.

If it is the app’s responsibility to ensure the system in good state, the validation is unnecessary.
If it is the CpuDxe’s responsibility to ensure the system in good state, the validation is insufficient.

I think we should wait. I am working with Eric to collect the requirement on why test case is designed in this way.

DebugAgentDxe is a good case. It is for debug purpose.
I believe there must be contract between CPU driver and DebugAgentDxe that which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
It is DebugAgentDxe’s responsibility to ensure the new system state is correct and compatible with the CPU driver.
With the contract, I don’t think CPU driver need validate the system state changed by DebugAgentDxe.
If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.




From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 3:32 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: Lou, Yun <yun.lou@intel.com>
Subject: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Laszlo,

Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is that we CANNOT assume original values are still valid during POST phase.

On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one function. Validating them are necessary.

For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.

Jeff

发件人: Laszlo Ersek<mailto:lersek@redhat.com>
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff<mailto:vanjeff_919@hotmail.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; eric.dong@intel.com<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
>
> Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Now, *if* FillExchangeInfoData() is currently *wrong* to have these
32-bit assumptions, because edk2 modules themselves can break those
assumptions (without the custom UEFI application), then we have a more
serious problem. But for that problem, just "checking and rejecting" is
not a sufficient solution, regardless of how and where we check and reject.

Thanks
Laszlo




>
> Jeff
>
> 发件人: Dong, Eric<mailto:eric.dong@intel.com>
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Lou, Yun<mailto:yun.lou@intel.com>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.
>
> Thanks,
> Eric
>
> From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Sent: Friday, September 4, 2020 9:34 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
> We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.
>
> We should not assume PEI runs at 32 bit mode.
>
>
> 发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>> <devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com<mailto:eric.dong@intel.com%3cmailto:eric.dong@intel.com>>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>> <devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>>
> 抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com<mailto:ray.ni@intel.com%3cmailto:ray.ni@intel.com>>>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> On 09/03/20 21:00, Laszlo Ersek wrote:
>
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called
>> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
>> DisableLvtInterrupts() -- and the error path does not restore the
>> original environment.
>>
>> (c) According to the PI spec (v1.7), the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
>> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
>> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
>> first action in the above-listed DxeMpLib functions.
>>
>> (Assuming the protocol members are called from an AP, and consequently
>> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
>> *caller's* fault, per spec!)
>
> This means we can move the ValidCr3GdtIdtCheck() function to
> "DxeMpLib.c", and it is not necessary to modify "MpLib.h".
>
> Thanks
> Laszlo
> >



[-- Attachment #2: Type: text/html, Size: 20568 bytes --]

Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 10:06, Yao, Jiewen wrote:
> When we say “validate”, we need get clear on what is the contract between caller and callee, and what is the expectation of the validation.
> 
> For example, in this case, the validation is limited to 4G or not 4G. Why?
> This function does not verify following, why?
> 
>   1.  if the GDT table is valid.
>   2.  if the IDT table is valid
>   3.  if the exception handler is valid
>   4.  if the timer handler still works
>   5.  if the page table is valid
>   6.  if the page table is 1:1 mapping
>   7.  if the page table still enforce the expected protection, such as RO, NX
>   8.  ……

Yes, very good point; it has crossed my mind before too. The currently
proposed checks verify the CR3 (but not the end of the root page
directory). They also don't try to walk the whole forest of page tables
and check every entry against 4GB (or, as you say, for 1:1 mapping). The
check covers the GDT and the IDT, but not the GDT and IDT entries
(segment granularity? direction of growth?)

I'm OK with the proposed rudimentary checks because in my mind they are
supposed to catch only one idiosyncratic UEFI application.

> If an application creates a crazy state, CpuDxe may pass the validation with below 4G page table, but still fail to wake up APs.

Yes, absolutely.

> 
> If it is the app’s responsibility to ensure the system in good state, the validation is unnecessary.
> If it is the CpuDxe’s responsibility to ensure the system in good state, the validation is insufficient.

Agreed on both counts.

I'm just under the impression that Eric has some internal use case that
doesn't let him fix the application -- or maybe there's no time left for
them for fixing the application.

> 
> I think we should wait. I am working with Eric to collect the requirement on why test case is designed in this way.

Sounds good, thanks!

> 
> DebugAgentDxe is a good case. It is for debug purpose.
> I believe there must be contract between CPU driver and DebugAgentDxe that which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
> It is DebugAgentDxe’s responsibility to ensure the new system state is correct and compatible with the CPU driver.

Agreed 100%

> With the contract, I don’t think CPU driver need validate the system state changed by DebugAgentDxe.
> If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.

Agreed again.

Thanks
Laszlo

Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Yao, Jiewen]

Thank you Laszlo.

Eric and I communicated with internal team. They understand that CPU is the only owner of the CPU state (CR3/DGT/IDT) and MP_SERVICE_PROTOCOL. Then we agree that the original test is invalid.

As conclusion, we can withdraw this patch and close the Bugzilla with "not a defect". 😊

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Friday, September 4, 2020 4:37 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> vanjeff_919@hotmail.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: Lou, Yun <yun.lou@intel.com>
> Subject: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for
> CR3/GDT/IDT.
> 
> On 09/04/20 10:06, Yao, Jiewen wrote:
> > When we say “validate”, we need get clear on what is the contract between
> caller and callee, and what is the expectation of the validation.
> >
> > For example, in this case, the validation is limited to 4G or not 4G. Why?
> > This function does not verify following, why?
> >
> >   1.  if the GDT table is valid.
> >   2.  if the IDT table is valid
> >   3.  if the exception handler is valid
> >   4.  if the timer handler still works
> >   5.  if the page table is valid
> >   6.  if the page table is 1:1 mapping
> >   7.  if the page table still enforce the expected protection, such as RO, NX
> >   8.  ……
> 
> Yes, very good point; it has crossed my mind before too. The currently
> proposed checks verify the CR3 (but not the end of the root page
> directory). They also don't try to walk the whole forest of page tables
> and check every entry against 4GB (or, as you say, for 1:1 mapping). The
> check covers the GDT and the IDT, but not the GDT and IDT entries
> (segment granularity? direction of growth?)
> 
> I'm OK with the proposed rudimentary checks because in my mind they are
> supposed to catch only one idiosyncratic UEFI application.
> 
> > If an application creates a crazy state, CpuDxe may pass the validation with
> below 4G page table, but still fail to wake up APs.
> 
> Yes, absolutely.
> 
> >
> > If it is the app’s responsibility to ensure the system in good state, the
> validation is unnecessary.
> > If it is the CpuDxe’s responsibility to ensure the system in good state, the
> validation is insufficient.
> 
> Agreed on both counts.
> 
> I'm just under the impression that Eric has some internal use case that
> doesn't let him fix the application -- or maybe there's no time left for
> them for fixing the application.
> 
> >
> > I think we should wait. I am working with Eric to collect the requirement on
> why test case is designed in this way.
> 
> Sounds good, thanks!
> 
> >
> > DebugAgentDxe is a good case. It is for debug purpose.
> > I believe there must be contract between CPU driver and DebugAgentDxe that
> which status DebugAgentDxe may modify and which state DebugAgentDxe may
> not.
> > It is DebugAgentDxe’s responsibility to ensure the new system state is correct
> and compatible with the CPU driver.
> 
> Agreed 100%
> 
> > With the contract, I don’t think CPU driver need validate the system state
> changed by DebugAgentDxe.
> > If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to
> run.
> 
> Agreed again.
> 
> Thanks
> Laszlo
> 
> 
> 

Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Dong, Eric]

[-- Attachment #1: Type: text/plain, Size: 4076 bytes --]

yes, I will close that Bugzilla. Thanks all for your feedback.

Thanks,
Eric

From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Saturday, September 5, 2020 8:31 PM
To: devel@edk2.groups.io; lersek@redhat.com; vanjeff_919@hotmail.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: Lou, Yun <yun.lou@intel.com>
Subject: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Thank you Laszlo.

Eric and I communicated with internal team. They understand that CPU is the only owner of the CPU state (CR3/DGT/IDT) and MP_SERVICE_PROTOCOL. Then we agree that the original test is invalid.

As conclusion, we can withdraw this patch and close the Bugzilla with "not a defect". 😊

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo Ersek
> Sent: Friday, September 4, 2020 4:37 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>;
> vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray
> <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
> Subject: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for
> CR3/GDT/IDT.
>
> On 09/04/20 10:06, Yao, Jiewen wrote:
> > When we say “validate”, we need get clear on what is the contract between
> caller and callee, and what is the expectation of the validation.
> >
> > For example, in this case, the validation is limited to 4G or not 4G. Why?
> > This function does not verify following, why?
> >
> >   1.  if the GDT table is valid.
> >   2.  if the IDT table is valid
> >   3.  if the exception handler is valid
> >   4.  if the timer handler still works
> >   5.  if the page table is valid
> >   6.  if the page table is 1:1 mapping
> >   7.  if the page table still enforce the expected protection, such as RO, NX
> >   8.  ……
>
> Yes, very good point; it has crossed my mind before too. The currently
> proposed checks verify the CR3 (but not the end of the root page
> directory). They also don't try to walk the whole forest of page tables
> and check every entry against 4GB (or, as you say, for 1:1 mapping). The
> check covers the GDT and the IDT, but not the GDT and IDT entries
> (segment granularity? direction of growth?)
>
> I'm OK with the proposed rudimentary checks because in my mind they are
> supposed to catch only one idiosyncratic UEFI application.
>
> > If an application creates a crazy state, CpuDxe may pass the validation with
> below 4G page table, but still fail to wake up APs.
>
> Yes, absolutely.
>
> >
> > If it is the app’s responsibility to ensure the system in good state, the
> validation is unnecessary.
> > If it is the CpuDxe’s responsibility to ensure the system in good state, the
> validation is insufficient.
>
> Agreed on both counts.
>
> I'm just under the impression that Eric has some internal use case that
> doesn't let him fix the application -- or maybe there's no time left for
> them for fixing the application.
>
> >
> > I think we should wait. I am working with Eric to collect the requirement on
> why test case is designed in this way.
>
> Sounds good, thanks!
>
> >
> > DebugAgentDxe is a good case. It is for debug purpose.
> > I believe there must be contract between CPU driver and DebugAgentDxe that
> which status DebugAgentDxe may modify and which state DebugAgentDxe may
> not.
> > It is DebugAgentDxe’s responsibility to ensure the new system state is correct
> and compatible with the CPU driver.
>
> Agreed 100%
>
> > With the contract, I don’t think CPU driver need validate the system state
> changed by DebugAgentDxe.
> > If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to
> run.
>
> Agreed again.
>
> Thanks
> Laszlo
>
>
> 

[-- Attachment #2: Type: text/html, Size: 13307 bytes --]

Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/05/20 15:50, Dong, Eric wrote:
> yes, I will close that Bugzilla. Thanks all for your feedback.
> 
> Thanks,
> Eric
> 
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Saturday, September 5, 2020 8:31 PM
> To: devel@edk2.groups.io; lersek@redhat.com; vanjeff_919@hotmail.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Lou, Yun <yun.lou@intel.com>
> Subject: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> Thank you Laszlo.
> 
> Eric and I communicated with internal team. They understand that CPU is the only owner of the CPU state (CR3/DGT/IDT) and MP_SERVICE_PROTOCOL. Then we agree that the original test is invalid.
> 
> As conclusion, we can withdraw this patch and close the Bugzilla with "not a defect". 😊
> 
> Thank you
> Yao Jiewen

Thank you both very much for solving this internally!
Laszlo

回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Fan Jeff]

[-- Attachment #1.1: Type: text/plain, Size: 9715 bytes --]

Jiewen,

I don’t think AP need to validate everyting. BSP should make sure CR3/GDT/IDT corrently firstly.  Othersize, everything does not make sense.

For CR3 case, if we assume CR3 above 4GB space is legal for BSP (If there are any limitation on it, please corrent me) but the current CPU AP wakeup method does not allow CR3 above 4GB space, this maybe the CPU driver’s bug or implementation limitation.

The key is that CR3 above 4GB is legal or not.
If we think this is bug, we need to enhance CPU driver to fix it.
If we think this is one limiation, we need to validate the CR3 value and tell the caller.

Jeff

发件人: Yao, Jiewen<mailto:jiewen.yao@intel.com>
发送时间: 2020年9月4日 16:07
收件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>; Laszlo Ersek<mailto:lersek@redhat.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

When we say “validate”, we need get clear on what is the contract between caller and callee, and what is the expectation of the validation.

For example, in this case, the validation is limited to 4G or not 4G. Why?
This function does not verify following, why?

  1.  if the GDT table is valid.
  2.  if the IDT table is valid
  3.  if the exception handler is valid
  4.  if the timer handler still works
  5.  if the page table is valid
  6.  if the page table is 1:1 mapping
  7.  if the page table still enforce the expected protection, such as RO, NX
  8.  ……

If an application creates a crazy state, CpuDxe may pass the validation with below 4G page table, but still fail to wake up APs.

If it is the app’s responsibility to ensure the system in good state, the validation is unnecessary.
If it is the CpuDxe’s responsibility to ensure the system in good state, the validation is insufficient.

I think we should wait. I am working with Eric to collect the requirement on why test case is designed in this way.

DebugAgentDxe is a good case. It is for debug purpose.
I believe there must be contract between CPU driver and DebugAgentDxe that which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
It is DebugAgentDxe’s responsibility to ensure the new system state is correct and compatible with the CPU driver.
With the contract, I don’t think CPU driver need validate the system state changed by DebugAgentDxe.
If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.




From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 3:32 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: Lou, Yun <yun.lou@intel.com>
Subject: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Laszlo,

Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is that we CANNOT assume original values are still valid during POST phase.

On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one function. Validating them are necessary.

For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.

Jeff

发件人: Laszlo Ersek<mailto:lersek@redhat.com>
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff<mailto:vanjeff_919@hotmail.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; eric.dong@intel.com<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
>
> Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Now, *if* FillExchangeInfoData() is currently *wrong* to have these
32-bit assumptions, because edk2 modules themselves can break those
assumptions (without the custom UEFI application), then we have a more
serious problem. But for that problem, just "checking and rejecting" is
not a sufficient solution, regardless of how and where we check and reject.

Thanks
Laszlo




>
> Jeff
>
> 发件人: Dong, Eric<mailto:eric.dong@intel.com>
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Lou, Yun<mailto:yun.lou@intel.com>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.
>
> Thanks,
> Eric
>
> From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Sent: Friday, September 4, 2020 9:34 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
> We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.
>
> We should not assume PEI runs at 32 bit mode.
>
>
> 发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>> <devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com<mailto:eric.dong@intel.com%3cmailto:eric.dong@intel.com>>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>> <devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>>
> 抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com<mailto:ray.ni@intel.com%3cmailto:ray.ni@intel.com>>>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> On 09/03/20 21:00, Laszlo Ersek wrote:
>
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called
>> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
>> DisableLvtInterrupts() -- and the error path does not restore the
>> original environment.
>>
>> (c) According to the PI spec (v1.7), the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
>> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
>> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
>> first action in the above-listed DxeMpLib functions.
>>
>> (Assuming the protocol members are called from an AP, and consequently
>> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
>> *caller's* fault, per spec!)
>
> This means we can move the ValidCr3GdtIdtCheck() function to
> "DxeMpLib.c", and it is not necessary to modify "MpLib.h".
>
> Thanks
> Laszlo
> >




[-- Attachment #1.2: Type: text/html, Size: 24085 bytes --]

[-- Attachment #2: 154E6E9E578E47869D1F0A70DF84BCF0.png --]
[-- Type: image/png, Size: 144 bytes --]

Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 09:32, Fan Jeff wrote:
> Laszlo,
>
> Why sync the BSP's CR3/GDT/IDT values for AP when AP wakes up instead
> of using old cached BSP's CR3/GDT/IDT values when CPU driver
> initiallly dispatched is that we CANNOT assume original values are
> still valid during POST phase.
>
> On this senario, BSP's CR3/GDT/IDT are just like input parameters for
> one function. Validating them are necessary.
>
> For example, DebugAgentDxe driver may be loaded in UEFI shell to setup
> source level debugging enviroment of EDKII debugger tools.  It may
> setup new IDT space.

Then DebugAgentDxe should be changed to allocate the new IDT in the
32-bit address space.

I don't think it's acceptable that loading DebugAgentDxe from the UEFI
shell may or may not render the MP services protocol unusable. What if
the programmer wants to debug something related to MP services? "I've
loaded DebugAgentDxe, but now I cannot start the payload I want to
debug."

As far as I can see,
"SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c"
uses the static variable "mIdtEntryTable" as IDT. I think it should be
possible to replace "mIdtEntryTable" with a dynamically allocated
object. The allocation should be restricted to 32-bit. It should be
possible to perform the allocation very early (perhaps in the library
constructor).

Again, I'm OK with adding ASSERT()s to FillExchangeInfoData(). If IDT /
GDT / CR3 are out of 32-bit address space, then that's a programming
error, or a platform misconfiguration. It's not something we should try
to dynamically recover from.

The MP services protocol implementation may not expect that the
CR3/GDT/IDT remain unchanged on the BSP during the DXE/BDS phases, but
it does expect them to remain under 4GB.

- A UEFI driver or app must not break this assumption at all, because a
  UEFI driver or app has no business messing with these artifacts.

- Whereas a DXE driver (such as DebugAgentDxe), exactly because it is
  part of the platform firmware, is expected to collaborate with CpuDxe
  / MpInitLib, and to satisfy the invariants.

Thanks,
Laszlo

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

[Laszlo Ersek]

On 09/04/20 03:34, Ni, Ray wrote:
> Why do we need a new PCD to control such check? Under what circumstance the PCD is false?

Under *all* circumstances, except when the platform wants to be
compatible with the UEFI application that manually moves the GDT or IDT
or CR3 above 4GB.

To repeat my earlier point: I consider the actions of this UEFI
application *invalid*. The UEFI spec does not authorize UEFI
applications to mess with low level concepts such as IDT / GDT / CR3.
All that stuff belongs to platform / DXE drivers. So if a UEFI
application messes with them anyway, breakage is *expected*.

In other words, I consider this patch to be adding compatibility with an
invalid UEFI application. That's fine (I assume Eric has a good reason
for asking for this compatibility -- must be an important or high
profile application), but we should *not* pessimize other platforms.
This is the reason for the Feature PCD -- on platforms that do not care
about compatibility with this application, the compiler should be able
to optimize away all this IDTR / GDTR / CR3 checking.

> We may need to move such check out of MpLib.c.

Yes, I agree. Minimally, it should go into the DXE instance of
MpInitLib. Another option is to move it out to CpuDxe.

> Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.

Yes, that's a further possible refinement. Restrict the logic to

  FeaturePCD && DXE && X64

> We should not assume PEI runs at 32 bit mode.

The PEI phase is irrelevant here -- by the time the UEFI application
runs, PEI is gone. So indeed PEI components / lib instances should not
be changed in any way.

(Note: as I said earlier, if someone can show that edk2 *itself* has
this problem, that is, GDT / IDT / CR can be set above 4GB *without*
using this particular UEFI shell application, then we have a more
serious problem. But in that case, it's not MpInitLib or the MP services
protocol that we need to fix -- instead, we need to fix the
*allocations* / placements themselves, so that they be under 4GB.)

Thanks
Laszlo

> 
> ________________________________
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek <lersek@redhat.com>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>
> 抄送: Ni, Ray <ray.ni@intel.com>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
> 
> On 09/03/20 21:00, Laszlo Ersek wrote:
> 
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called
>> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
>> DisableLvtInterrupts() -- and the error path does not restore the
>> original environment.
>>
>> (c) According to the PI spec (v1.7), the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
>> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
>> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
>> first action in the above-listed DxeMpLib functions.
>>
>> (Assuming the protocol members are called from an AP, and consequently
>> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
>> *caller's* fault, per spec!)
> 
> This means we can move the ValidCr3GdtIdtCheck() function to
> "DxeMpLib.c", and it is not necessary to modify "MpLib.h".
> 
> Thanks
> Laszlo
> 
> 
> 
> 

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

[Dong, Eric]

[-- Attachment #1: Type: text/plain, Size: 13407 bytes --]

Hi Laszlo,

Very appreciate your good and detail comments, you are very good coach!

I follow all your comments in this mail and updated V3 patch, please check it and provide feedback.

Thanks,
Eric
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Friday, September 4, 2020 3:00 AM
To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/03/20 17:11, 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.Base and IDT.Base to not above
> 32 bits restriction.
>
> Change-Id: I231180f45d9f542641082c57d001e38e3f6759d5

(1) Please drop the Change-Id line.


(2) We should document that the motivation for this patch is a special
UEFI shell application that changes the GDT / CR3 to above 4GB.
Currently, neither the bugzilla, nor the commit message, nor the PCD
documentation in the DEC file explains this. So one is left wondering if
and why they should change the PCD to TRUE on their platform.

Please append the following two paragraphs to the commit message:

"""
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<mailto:eric.dong@intel.com>>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> ---
>
> 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       |  9 +++
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 76 +++++++++++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 12 +++
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  1 +
>  UefiCpuPkg/UefiCpuPkg.dec                     |  4 +
>  6 files changed, 103 insertions(+)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 1771575c69..20851f251a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -76,3 +76,4 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                       ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck               ## CONSUMES
> \ No newline at end of file

(3) Can you insert this new line just after
"gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase" please? In order to
keep the "gUefiCpuPkgTokenSpaceGuid" PCDs grouped together.


> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 2c00d72dde..f598372c4d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -394,6 +394,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()) {

(4) Missing space character after "ValidCR3GdtIdtCheck".

(Applies to all the other call sites as well.)


> +    CpuDeadLoop ();
> +  }
> +
>    CpuMpData = GetCpuMpData ();
>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>    CpuMpData->Pm16CodeSegment = GetProtectedMode16CS ();
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 07426274f6..69a0372df7 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1909,6 +1909,54 @@ CheckAllAPs (
>    return EFI_NOT_READY;
>  }
>
> +/**
> +  Check whether CR3/GDT/IDT value valid for AP.
> +
> +  @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 ((IA32_DESCRIPTOR *) &Gdtr);

(5) The cast is superfluous, please remove it. According to <BaseLib.h>,
AsmReadGdtr() takes the following parameter:

  OUT     IA32_DESCRIPTOR           *Gdtr


> +  if ((Gdtr.Base >= BASE_4GB) || (Gdtr.Base + Gdtr.Limit >= BASE_4GB)) {
> +    return FALSE;
> +  }

I agree that both checks (even the second one) should use ">=", given
that Limit is an inclusive boundary.

My understanding is that we don't have to worry about any UINTN overflow
in the addition here, as Base and Limit come from the "live" GDTR.

(6) So we could even eliminate the first expression, and only do:

  if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
    return FALSE;
  }

Because, given that we do not expect any overflows here, if this (2nd)
condition is FALSE, then the 1st condition must be FALSE too.

I don't insist on this simplification. If you also like the
simplification, then please
- update the commit message
- update the code comment above

because we will no longer check Base in itself.


> +
> +  AsmReadIdtr ((IA32_DESCRIPTOR *) &Idtr);

(7) The cast is superfluous.


> +  if ((Idtr.Base >= BASE_4GB) || (Idtr.Base + Idtr.Limit >= BASE_4GB)) {
> +    return FALSE;
> +  }

(8) Same comment as (6) -- please consider removing the 1st condition as
a simplification (if you disagree, that's OK too).


> +
> +  return TRUE;
> +}
> +
>  /**
>    MP Initialize Library initialization.
>
> @@ -2318,6 +2366,13 @@ SwitchBSPWorker (
>      return EFI_NOT_READY;
>    }
>
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    CpuMpData->BSPInfo.State = CPU_SWITCH_STATE_IDLE;
>    CpuMpData->APInfo.State  = CPU_SWITCH_STATE_IDLE;
>    CpuMpData->SwitchBspFlag = TRUE;

(9) I think we should return EFI_UNSUPPORTED, not EFI_INVALID_PARAMETER.


(10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".

Instead, the calls should be made in the DXE instance of the library
("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
functions:

- MpInitLibStartupAllAPs
- MpInitLibStartupThisAP
- MpInitLibSwitchBSP
- MpInitLibEnableDisableAP

Here's why:

(a) The symptom does not affect the PEI phase -- by the time the UEFI
application is executed, the PEI phase has ended; there's no need to
modify the PEI instance of the library.

(b) The currently proposed failure exits are too late. For example, in
the SwitchBSPWorker() function, by the time we exit, we have called
DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
DisableLvtInterrupts() -- and the error path does not restore the
original environment.

(c) According to the PI spec (v1.7), the StartupAllAPs(),
StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
first action in the above-listed DxeMpLib functions.

(Assuming the protocol members are called from an AP, and consequently
we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
*caller's* fault, per spec!)


> @@ -2420,6 +2475,13 @@ EnableDisableApWorker (
>      return EFI_NOT_FOUND;
>    }
>
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    if (!EnableAP) {
>      SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateDisabled);
>    } else {
> @@ -2607,6 +2669,13 @@ StartupAllCPUsWorker (
>      return EFI_DEVICE_ERROR;
>    }
>
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    //
>    // Update AP state
>    //
> @@ -2772,6 +2841,13 @@ StartupThisAPWorker (
>      return EFI_INVALID_PARAMETER;
>    }
>
> +  //
> +  // Check whether CR3/GDT/IDT valid for AP.
> +  //
> +  if (!ValidCR3GdtIdtCheck()) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    //
>    // Update AP state
>    //
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 02652eaae1..4ac5cb51e3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -740,5 +740,17 @@ PlatformShadowMicrocode (
>    IN OUT CPU_MP_DATA             *CpuMpData
>    );
>
> +/**
> +  Check whether CR3/GDT/IDT value valid for AP.
> +
> +  @retval  TRUE          Pass the check.
> +  @retval  FALSE         Fail the check.
> +
> +**/
> +BOOLEAN
> +ValidCR3GdtIdtCheck (

(11) In the function name, please write "Cr3", not "CR3". (See also:
AsmReadCr3().)


> +  VOID
> +  );
> +
>  #endif
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 34abf25d43..0ca86fcdaa 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -65,6 +65,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                      ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                   ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                       ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck           ## CONSUMES
>
>  [Ppis]
>    gEdkiiPeiShadowMicrocodePpiGuid        ## SOMETIMES_CONSUMES

(12) Superficially, comment (3) applies -- please keep the PCDs in the
UefiCpuPkg token space grouped together.

However, per my point (10), the PEI instance of the library should not
be modified at all; so please drop this hunk.


> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index d83c084467..467ec5e001 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -187,6 +187,10 @@
>    # @Prompt Configure stack size for Application Processor (AP)
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize|0x8000|UINT32|0x00000003
>
> +  ## This value specifies whether need to check the CR3/GDT/IDT value for AP.
> +  # @Prompt Whether need to check the CR3/GDT/IDT value for AP
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck|FALSE|BOOLEAN|0x30000044
> +
>    ## Specifies stack size in the temporary RAM. 0 means half of TemporaryRamSize.
>    # @Prompt Stack size in the temporary RAM.
>    gUefiCpuPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize|0|UINT32|0x10001003
>

OK, so this is [PcdsFixedAtBuild, PcdsPatchableInModule], not a
FeaturePCD. I proposed FeaturePCD because optimizing compilers are
supposed to eliminate it... But, I can see that BaseTools generates the
same kind of code for Fixed BOOLEAN PCDs as well. OK!

(13) I think you should modify the UNI file in accordance with the DEC file.

Thanks!
Laszlo




[-- Attachment #2: Type: text/html, Size: 39026 bytes --]

Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Fan Jeff]

[-- Attachment #1: Type: text/html, Size: 23962 bytes --]

Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Yao, Jiewen]

[-- Attachment #1: Type: text/plain, Size: 12161 bytes --]

Jeff
I think the CR3 should be managed by the CPU driver. And it should NOT be changed by other module during post phase without a clear contract.

Since CPU driver managed the value (or DebugAgent managing it under contract), it should *always* be valid. I don’t see the value to check the value managed by the module itself, and return EFI_UNSUPPORTED.

If CPU driver sets CR3 above 4G and sync it to AP, then it is a bug.
We should fix the CPU driver or MP Library to *make it always work*.

There are multiple options. For example, the CPU driver can always set a dedicate CR3/GDT/IDT for AP (below 4G). Then no matter BSP state is changed, we can always wake up AP correctly with a pre-defined state. Syncing BSP state to AP just an implementation choice, it is not absolute necessary.

In a system boot, the ability to wake up AP is very important. Especially when the BIOS transfers the state to OS, the BSP need wake up AP to put AP in a good state, such as protected mode with paging disabled. Without doing that, we may get random system crash during boot.

If we really really want to do some check to ensure AP state is good, I would rather to prepare a dedicated known good state for AP and always use this known good state for AP wakeup.
Giving up and returning UNSUPPORTED is definitely NOT my preference.


Thank you
Yao Jiewen







From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 7:17 PM
To: vanjeff_919@hotmail.com; Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; lersek@redhat.com; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: Lou, Yun <yun.lou@intel.com>
Subject: Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Jiewen,

Maybe my word "valid during post phase" confused you. My point is that those values maybe changed during post phase.

So, if their spaces above 4GB are not legal, i agree not to do anything. If legal, then we need to something.

Jeff



在 Fan Jeff <vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>>，2020年9月4日 下午4:44写道：
Jiewen,

I don’t think AP need to validate everyting. BSP should make sure CR3/GDT/IDT corrently firstly.  Othersize, everything does not make sense.

For CR3 case, if we assume CR3 above 4GB space is legal for BSP (If there are any limitation on it, please corrent me) but the current CPU AP wakeup method does not allow CR3 above 4GB space, this maybe the CPU driver’s bug or implementation limitation.

The key is that CR3 above 4GB is legal or not.
If we think this is bug, we need to enhance CPU driver to fix it.
If we think this is one limiation, we need to validate the CR3 value and tell the caller.

Jeff

发件人: Yao, Jiewen<mailto:jiewen.yao@intel.com>
发送时间: 2020年9月4日 16:07
收件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vanjeff_919@hotmail.com<mailto:vanjeff_919@hotmail.com>; Laszlo Ersek<mailto:lersek@redhat.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

When we say “validate”, we need get clear on what is the contract between caller and callee, and what is the expectation of the validation.

For example, in this case, the validation is limited to 4G or not 4G. Why?
This function does not verify following, why?

  1.  if the GDT table is valid.
  2.  if the IDT table is valid
  3.  if the exception handler is valid
  4.  if the timer handler still works
  5.  if the page table is valid
  6.  if the page table is 1:1 mapping
  7.  if the page table still enforce the expected protection, such as RO, NX
  8.  ……

If an application creates a crazy state, CpuDxe may pass the validation with below 4G page table, but still fail to wake up APs.

If it is the app’s responsibility to ensure the system in good state, the validation is unnecessary.
If it is the CpuDxe’s responsibility to ensure the system in good state, the validation is insufficient.

I think we should wait. I am working with Eric to collect the requirement on why test case is designed in this way.

DebugAgentDxe is a good case. It is for debug purpose.
I believe there must be contract between CPU driver and DebugAgentDxe that which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
It is DebugAgentDxe’s responsibility to ensure the new system state is correct and compatible with the CPU driver.
With the contract, I don’t think CPU driver need validate the system state changed by DebugAgentDxe.
If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.




From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 3:32 PM
To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
Subject: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

Laszlo,

Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is that we CANNOT assume original values are still valid during POST phase.

On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one function. Validating them are necessary.

For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.

Jeff

发件人: Laszlo Ersek<mailto:lersek@redhat.com>
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff<mailto:vanjeff_919@hotmail.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; eric.dong@intel.com<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>
抄送: Lou, Yun<mailto:yun.lou@intel.com>
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
>
> Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Now, *if* FillExchangeInfoData() is currently *wrong* to have these
32-bit assumptions, because edk2 modules themselves can break those
assumptions (without the custom UEFI application), then we have a more
serious problem. But for that problem, just "checking and rejecting" is
not a sufficient solution, regardless of how and where we check and reject.

Thanks
Laszlo




>
> Jeff
>
> 发件人: Dong, Eric<mailto:eric.dong@intel.com>
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray<mailto:ray.ni@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>; lersek@redhat.com<mailto:lersek@redhat.com>
> 抄送: Lou, Yun<mailto:yun.lou@intel.com>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> I guess Laszlo think this check is not always needed, just used for this special shell application case. He wants to use default FALSE to always ignore this check and make code logic clear.
>
> Thanks,
> Eric
>
> From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Sent: Friday, September 4, 2020 9:34 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; lersek@redhat.com<mailto:lersek@redhat.com>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Lou, Yun <yun.lou@intel.com<mailto:yun.lou@intel.com>>
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> Why do we need a new PCD to control such check? Under what circumstance the PCD is false?
> We may need to move such check out of MpLib.c. Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes.
>
> We should not assume PEI runs at 32 bit mode.
>
>
> 发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>> <devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>> 代表 Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com<mailto:eric.dong@intel.com%3cmailto:eric.dong@intel.com>>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>> <devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>>>
> 抄送: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com<mailto:ray.ni@intel.com%3cmailto:ray.ni@intel.com>>>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.
>
> On 09/03/20 21:00, Laszlo Ersek wrote:
>
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called
>> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and
>> DisableLvtInterrupts() -- and the error path does not restore the
>> original environment.
>>
>> (c) According to the PI spec (v1.7), the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of
>> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP.
>> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very
>> first action in the above-listed DxeMpLib functions.
>>
>> (Assuming the protocol members are called from an AP, and consequently
>> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the
>> *caller's* fault, per spec!)
>
> This means we can move the ValidCr3GdtIdtCheck() function to
> "DxeMpLib.c", and it is not necessary to modify "MpLib.h".
>
> Thanks
> Laszlo
> >




[-- Attachment #2: Type: text/html, Size: 30471 bytes --]

Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 13:48, Yao, Jiewen wrote:
> Jeff
> I think the CR3 should be managed by the CPU driver. And it should NOT be changed by other module during post phase without a clear contract.
> 
> Since CPU driver managed the value (or DebugAgent managing it under contract), it should *always* be valid. I don’t see the value to check the value managed by the module itself, and return EFI_UNSUPPORTED.
> 
> If CPU driver sets CR3 above 4G and sync it to AP, then it is a bug.
> We should fix the CPU driver or MP Library to *make it always work*.
> 
> There are multiple options. For example, the CPU driver can always set a dedicate CR3/GDT/IDT for AP (below 4G). Then no matter BSP state is changed, we can always wake up AP correctly with a pre-defined state. Syncing BSP state to AP just an implementation choice, it is not absolute necessary.
> 
> In a system boot, the ability to wake up AP is very important. Especially when the BIOS transfers the state to OS, the BSP need wake up AP to put AP in a good state, such as protected mode with paging disabled. Without doing that, we may get random system crash during boot.
> 
> If we really really want to do some check to ensure AP state is good, I would rather to prepare a dedicated known good state for AP and always use this known good state for AP wakeup.
> Giving up and returning UNSUPPORTED is definitely NOT my preference.

Should we audit the various AllocatePageTableMemory() function
implementations in edk2?

Or is there more stuff we need to look at?

Thanks
Laszlo