[edk2-devel] [Patch V6 0/2] UefiCpuPkg/MpInitLib: Enable execute disable bit.

[edk2-devel] [Patch V6 0/2] UefiCpuPkg/MpInitLib: Enable execute disable bit.

[Yuanhao Xie]

This patch series synchronizes the No-Execute bit in the IA32_EFER
register for the APs before the RestoreVolatileRegisters operation.
It also updated the comments of _CPU_MP_DATA to delcared that
duplications in CpuMpData are present to avoid to be direct accessed
and comprehended in assembly code.

It also fixed the build error in V5.

Yuanhao Xie (1):
  UefiCpuPkg/MpInitLib: Enable execute disable bit.

xieyuanh (1):
  UefiCpuPkg/MpInitLib: Update the comments of _CPU_MP_DATA.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 +++++++++++---
 UefiCpuPkg/Library/MpInitLib/MpLib.h |  7 ++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

--
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111453): https://edk2.groups.io/g/devel/message/111453
Mute This Topic: https://groups.io/mt/102702382/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V6 1/2] UefiCpuPkg/MpInitLib: Enable execute disable bit.

[Yuanhao Xie]

From: Yuanhao Xie <yuanhao.xie@intel.com>

This patch synchronizes the No-Execute bit in the IA32_EFER
register for the APs before the RestoreVolatileRegisters operation.

The commit 964a4f0, titled "Eliminate the second INIT-SIPI-SIPI
sequence," replaces the second INIT-SIPI-SIPI sequence with the BSP
calling the SwitchApContext function to initiate a specialized start-up
signal, waking up APs in the DXE instead of using INIT-SIPI-SIPI.

Due to this change, the logic for "Enable execute disable bit" in
MpFuncs.nasm is no longer executed. However, to ensure the proper setup
of the page table, it is necessary to synchronize the IA32_EFER.NXE for
APs before executing RestoreVolatileRegisters .

Based on SDM:
If IA32_EFER.NXE is set to 1, it signifies execute-disable, meaning
instruction fetches are not allowed from the 4-KByte page controlled by
this entry. Conversely, if it is set to 0, it is reserved.

Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Cc: Laszlo Ersek lersek@redhat.com
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 +++++++++++---
 UefiCpuPkg/Library/MpInitLib/MpLib.h |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 9a6ec5db5c..f29e66a14f 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -910,9 +910,16 @@ DxeApEntryPoint (
   CPU_MP_DATA  *CpuMpData
   )
 {
-  UINTN  ProcessorNumber;
+  UINTN                   ProcessorNumber;
+  MSR_IA32_EFER_REGISTER  EferMsr;
 
   GetProcessorNumber (CpuMpData, &ProcessorNumber);
+  if (CpuMpData->EnableExecuteDisableForSwitchContext) {
+    EferMsr.Uint64   = AsmReadMsr64 (MSR_IA32_EFER);
+    EferMsr.Bits.NXE = 1;
+    AsmWriteMsr64 (MSR_IA32_EFER, EferMsr.Uint64);
+  }
+
   RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
   InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount);
   PlaceAPInMwaitLoopOrRunLoop (
@@ -2188,8 +2195,9 @@ MpInitLibInitialize (
     if (MpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
       ASSERT (CpuMpData->ApLoopMode != ApInHltLoop);
 
-      CpuMpData->FinishedCount = 0;
-      CpuMpData->InitFlag      = ApInitDone;
+      CpuMpData->FinishedCount                        = 0;
+      CpuMpData->InitFlag                             = ApInitDone;
+      CpuMpData->EnableExecuteDisableForSwitchContext = IsBspExecuteDisableEnabled ();
       SaveCpuMpData (CpuMpData);
       //
       // In scenarios where both the PEI and DXE phases run in the same
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 763db4963d..af296f6ac0 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -270,6 +270,7 @@ struct _CPU_MP_DATA {
   UINT64                           TotalTime;
   EFI_EVENT                        WaitEvent;
   UINTN                            **FailedCpuList;
+  BOOLEAN                          EnableExecuteDisableForSwitchContext;
 
   AP_INIT_STATE                    InitFlag;
   BOOLEAN                          SwitchBspFlag;
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111454): https://edk2.groups.io/g/devel/message/111454
Mute This Topic: https://groups.io/mt/102702384/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V6 1/2] UefiCpuPkg/MpInitLib: Enable execute disable bit.

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Xie, Yuanhao <yuanhao.xie@intel.com>
> Sent: Monday, November 20, 2023 12:55 PM
> To: devel@edk2.groups.io
> Cc: Xie, Yuanhao <yuanhao.xie@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>
> Subject: [Patch V6 1/2] UefiCpuPkg/MpInitLib: Enable execute disable bit.
> 
> From: Yuanhao Xie <yuanhao.xie@intel.com>
> 
> This patch synchronizes the No-Execute bit in the IA32_EFER
> register for the APs before the RestoreVolatileRegisters operation.
> 
> The commit 964a4f0, titled "Eliminate the second INIT-SIPI-SIPI
> sequence," replaces the second INIT-SIPI-SIPI sequence with the BSP
> calling the SwitchApContext function to initiate a specialized start-up
> signal, waking up APs in the DXE instead of using INIT-SIPI-SIPI.
> 
> Due to this change, the logic for "Enable execute disable bit" in
> MpFuncs.nasm is no longer executed. However, to ensure the proper setup
> of the page table, it is necessary to synchronize the IA32_EFER.NXE for
> APs before executing RestoreVolatileRegisters .
> 
> Based on SDM:
> If IA32_EFER.NXE is set to 1, it signifies execute-disable, meaning
> instruction fetches are not allowed from the 4-KByte page controlled by
> this entry. Conversely, if it is set to 0, it is reserved.
> 
> Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Cc: Laszlo Ersek lersek@redhat.com
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 +++++++++++---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  1 +
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 9a6ec5db5c..f29e66a14f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -910,9 +910,16 @@ DxeApEntryPoint (
>    CPU_MP_DATA  *CpuMpData
>    )
>  {
> -  UINTN  ProcessorNumber;
> +  UINTN                   ProcessorNumber;
> +  MSR_IA32_EFER_REGISTER  EferMsr;
> 
>    GetProcessorNumber (CpuMpData, &ProcessorNumber);
> +  if (CpuMpData->EnableExecuteDisableForSwitchContext) {
> +    EferMsr.Uint64   = AsmReadMsr64 (MSR_IA32_EFER);
> +    EferMsr.Bits.NXE = 1;
> +    AsmWriteMsr64 (MSR_IA32_EFER, EferMsr.Uint64);
> +  }
> +
>    RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> FALSE);
>    InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount);
>    PlaceAPInMwaitLoopOrRunLoop (
> @@ -2188,8 +2195,9 @@ MpInitLibInitialize (
>      if (MpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
>        ASSERT (CpuMpData->ApLoopMode != ApInHltLoop);
> 
> -      CpuMpData->FinishedCount = 0;
> -      CpuMpData->InitFlag      = ApInitDone;
> +      CpuMpData->FinishedCount                        = 0;
> +      CpuMpData->InitFlag                             =
> ApInitDone;
> +      CpuMpData->EnableExecuteDisableForSwitchContext =
> IsBspExecuteDisableEnabled ();
>        SaveCpuMpData (CpuMpData);
>        //
>        // In scenarios where both the PEI and DXE phases run in the same
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 763db4963d..af296f6ac0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -270,6 +270,7 @@ struct _CPU_MP_DATA {
>    UINT64                           TotalTime;
>    EFI_EVENT                        WaitEvent;
>    UINTN                            **FailedCpuList;
> +  BOOLEAN
> EnableExecuteDisableForSwitchContext;
> 
>    AP_INIT_STATE                    InitFlag;
>    BOOLEAN                          SwitchBspFlag;
> --
> 2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111495): https://edk2.groups.io/g/devel/message/111495
Mute This Topic: https://groups.io/mt/102702384/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V6 2/2] UefiCpuPkg/MpInitLib: Update the comments of _CPU_MP_DATA.

[Yuanhao Xie]

No functional changes in this patch.

Updated the comments of _CPU_MP_DATA to delcared that duplications in
CpuMpData are present to avoid to be direct accessed and comprehended
 in assembly code. CpuMpData: Intended for use in C code while
 ExchangeInfo are used in assembly code in this module.

Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
Cc: Laszlo Ersek lersek@redhat.com
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index af296f6ac0..924677d823 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -239,7 +239,11 @@ typedef struct {
 #pragma pack()
 
 //
-// CPU MP Data save in memory
+// CPU MP Data save in memory, and intended for use in C code.
+// There are some duplicated fields, such as XD status, between
+// CpuMpData and ExchangeInfo. These duplications in CpuMpData
+// are present to avoid to be direct accessed and comprehended
+// in assembly code.
 //
 struct _CPU_MP_DATA {
   UINT64                           CpuInfoInHob;
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111455): https://edk2.groups.io/g/devel/message/111455
Mute This Topic: https://groups.io/mt/102702385/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-