[edk2-devel] [Patch V4] UefiCpuPkg/MpInitLib: Enable execute disable bit.

[edk2-devel] [Patch V4] 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>
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 (#111121): https://edk2.groups.io/g/devel/message/111121
Mute This Topic: https://groups.io/mt/102556608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

> 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);
> +  }

It helps reviewers if you document changes from one version to the next.
This code block was moved (compared to v3).  Why?

take care,
  Gerd



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

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

[Yuanhao Xie]

Hi Gerd,

In v4 , InterlockedIncrement called is after RestoreVolatileRegisters to ensure that "finished" reporting from the APs is as
later as possible.

Here is V3：
   GetProcessorNumber (CpuMpData, &ProcessorNumber);
-  RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
   InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount);
+
+  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);

Regards,
Yuanhao

-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com> 
Sent: Monday, November 13, 2023 6:54 PM
To: Xie, Yuanhao <yuanhao.xie@intel.com>
Cc: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: Re: [Patch V4] UefiCpuPkg/MpInitLib: Enable execute disable bit.

> 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);  }

It helps reviewers if you document changes from one version to the next.
This code block was moved (compared to v3).  Why?

take care,
  Gerd



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

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

[Gerd Hoffmann]

On Mon, Nov 13, 2023 at 01:59:48PM +0800, xieyuanh wrote:
> 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>
> 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>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>



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

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

[Laszlo Ersek]

On 11/13/23 06:59, Yuanhao Xie wrote:
> 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>
> 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(-)

Good problem description!

>
> 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;

Functionally this patch seems fine.

(I cannot test it very usefully, because this code path is not active in
OVMF.)

However, there's one thing I think is less than ideal: after this patch,
we'll have

  MP_CPU_EXCHANGE_INFO . EnableExecuteDisable

but also

  MP_CPU_EXCHANGE_INFO . CpuMpData -> EnableExecuteDisableForSwitchContext

Furthermore, we'll have two invocations of IsBspExecuteDisableEnabled():

- once for the original (HLT loop + INIT-SIPI-SIPI) method, in
  WakeUpAP() -> FillExchangeInfoData(),

- and another time for the new method, in MpInitLibInitialize().

I feel that we should centralize this to one spot.

Note that in commit 629c1dacc9bd ("UefiCpuPkg: ApWakeupFunction directly
use CpuMpData.", 2023-07-11), you changed the prototype of
ApWakeupFunction(), such that it would take CpuMpData directly, rather
than MP_CPU_EXCHANGE_INFO. This was done so that in the next commit
(964a4f032dcd, "UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI
sequence.", 2023-07-11), you could invoke ApWakeupFunction() on *both*
paths, old and new:

- old (INIT-SIPI-SIPI): from the assembly language startup code,

- new: from DxeApEntryPoint().

Therefore, it seems that the *old* field
"MP_CPU_EXCHANGE_INFO.EnableExecuteDisable" is now superfluous. You have
effectively pushed it down to "CpuMpData", so that it's available in
DxeApEntryPoint().

But CpuMpData is similarly available in the assembly language startup
code (that's why you could implement commit 629c1dacc9bd).

So I think this patch is good, but it should be followed with a further
patch: can you please rebase the *old* startup code to the new CpuMpData
field "EnableExecuteDisableForSwitchContext", and then eliminate
"MP_CPU_EXCHANGE_INFO.EnableExecuteDisable"?

Where we do

     mov         edi, esi
     add         edi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable)
     cmp         byte [edi], 0
     jz          SkipEnableExecuteDisable

on IA32, and

     mov        esi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable)
     cmp        byte [ebx + esi], 0
     jz         SkipEnableExecuteDisableBit

on X64, can we dereference the CpuMpData field (pointer) instead, and
grab the new field "EnableExecuteDisableForSwitchContext"?

Effectively MP_CPU_EXCHANGE_INFO seems to contain information that is
needed *only* by the INIT-SIPI-SIPI startup path, and CpuMpData holds
information that is needed by both startup paths. Given that we now have
XD status in the latter, we should drop it from the former.

(Of course, once we start using "EnableExecuteDisableForSwitchContext"
on both startup paths, then the *name* will be wrong -- it will no
longer be for "SwitchContext" only!)

... Yet further, this seems to indicate that calling
IsBspExecuteDisableEnabled() upon every invocation of WakeUpAP() (via
FillExchangeInfoData()) is superfluous, on the "old" startup path. We
should call IsBspExecuteDisableEnabled() only once, namely in
MpInitLibInitialize(), *regardless* of "WaitLoopExecutionMode", for
filling in the new field. Is that right?

Thanks,
Laszlo



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

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

[Ni, Ray]

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

Laszlo,
I agree that the BSP's XD status is saved in both CpuMpData and ExchangeInfo.
But, thinking from another perspective, ExchangeInfo is "only" used by the assembly
code. That's why the BSP code needs to save the XD status in CpuMpData and ExchangeInfo.

If we remove the XD status field in ExchangeInfo, then the assembly code has to understand
the structure layout of CpuMpData, which is what I prefer to avoid.

If you compare all fields in ExchangeInfo and CpuMpData, following fields are already duplicated:
* CpuMpData.CpuInfoHob <-> MpExchangeInfo.CpuInfo
* InitFlag
* SevEsIsEnabled
* SevSnpIsEnabled
* GhcbBase


So, I prefer to keep the current change proposed in Yuanhao's patch.

Thanks,
Ray

________________________________
From: Laszlo Ersek <lersek@redhat.com>
Sent: Tuesday, November 14, 2023 2:02 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Xie, Yuanhao <yuanhao.xie@intel.com>
Cc: 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: Re: [edk2-devel] [Patch V4] UefiCpuPkg/MpInitLib: Enable execute disable bit.

On 11/13/23 06:59, Yuanhao Xie wrote:
> 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>
> 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(-)

Good problem description!

>
> 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;

Functionally this patch seems fine.

(I cannot test it very usefully, because this code path is not active in
OVMF.)

However, there's one thing I think is less than ideal: after this patch,
we'll have

  MP_CPU_EXCHANGE_INFO . EnableExecuteDisable

but also

  MP_CPU_EXCHANGE_INFO . CpuMpData -> EnableExecuteDisableForSwitchContext

Furthermore, we'll have two invocations of IsBspExecuteDisableEnabled():

- once for the original (HLT loop + INIT-SIPI-SIPI) method, in
  WakeUpAP() -> FillExchangeInfoData(),

- and another time for the new method, in MpInitLibInitialize().

I feel that we should centralize this to one spot.

Note that in commit 629c1dacc9bd ("UefiCpuPkg: ApWakeupFunction directly
use CpuMpData.", 2023-07-11), you changed the prototype of
ApWakeupFunction(), such that it would take CpuMpData directly, rather
than MP_CPU_EXCHANGE_INFO. This was done so that in the next commit
(964a4f032dcd, "UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI
sequence.", 2023-07-11), you could invoke ApWakeupFunction() on *both*
paths, old and new:

- old (INIT-SIPI-SIPI): from the assembly language startup code,

- new: from DxeApEntryPoint().

Therefore, it seems that the *old* field
"MP_CPU_EXCHANGE_INFO.EnableExecuteDisable" is now superfluous. You have
effectively pushed it down to "CpuMpData", so that it's available in
DxeApEntryPoint().

But CpuMpData is similarly available in the assembly language startup
code (that's why you could implement commit 629c1dacc9bd).

So I think this patch is good, but it should be followed with a further
patch: can you please rebase the *old* startup code to the new CpuMpData
field "EnableExecuteDisableForSwitchContext", and then eliminate
"MP_CPU_EXCHANGE_INFO.EnableExecuteDisable"?

Where we do

     mov         edi, esi
     add         edi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable)
     cmp         byte [edi], 0
     jz          SkipEnableExecuteDisable

on IA32, and

     mov        esi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable)
     cmp        byte [ebx + esi], 0
     jz         SkipEnableExecuteDisableBit

on X64, can we dereference the CpuMpData field (pointer) instead, and
grab the new field "EnableExecuteDisableForSwitchContext"?

Effectively MP_CPU_EXCHANGE_INFO seems to contain information that is
needed *only* by the INIT-SIPI-SIPI startup path, and CpuMpData holds
information that is needed by both startup paths. Given that we now have
XD status in the latter, we should drop it from the former.

(Of course, once we start using "EnableExecuteDisableForSwitchContext"
on both startup paths, then the *name* will be wrong -- it will no
longer be for "SwitchContext" only!)

... Yet further, this seems to indicate that calling
IsBspExecuteDisableEnabled() upon every invocation of WakeUpAP() (via
FillExchangeInfoData()) is superfluous, on the "old" startup path. We
should call IsBspExecuteDisableEnabled() only once, namely in
MpInitLibInitialize(), *regardless* of "WaitLoopExecutionMode", for
filling in the new field. Is that right?

Thanks,
Laszlo



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



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

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

[Laszlo Ersek]

On 11/17/23 08:01, Ni, Ray wrote:
> Laszlo,
> I agree that the BSP's XD status is saved in both CpuMpData and
> ExchangeInfo.
> But, thinking from another perspective, ExchangeInfo is "only" used by
> the assembly
> code. That's why the BSP code needs to save the XD status in CpuMpData
> and ExchangeInfo.
> 
> If we remove the XD status field in ExchangeInfo, then the assembly code
> has to understand
> the structure layout of CpuMpData, which is what I prefer to avoid.
> 
> If you compare all fields in ExchangeInfo and CpuMpData, following
> fields are already duplicated:
> * CpuMpData.CpuInfoHob <-> MpExchangeInfo.CpuInfo
> * InitFlag
> * SevEsIsEnabled
> * SevSnpIsEnabled
> * GhcbBase
> 
> 
> So, I prefer to keep the current change proposed in Yuanhao's patch.

Very good explanation, thank you.

Can we perhaps document, in an additional patch:

- in "UefiCpuPkg/Library/MpInitLib/MpEqu.inc", that the assembly
routines are not supposed to access the internals of CPU_MP_DATA,

- the same statement above "struct _CPU_MP_DATA" in
"UefiCpuPkg/Library/MpInitLib/MpLib.h"?

A number of structures in "UefiCpuPkg/Library/MpInitLib/MpLib.h"
document whether they are to be used by assembly code vs. C code (vs.
both), but CPU_MP_DATA doesn't seem to have such comments.

For the current patch:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo



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

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

[Yuanhao Xie]

Hi Ray and Laszlo,

Thanks a lot for the feedbacks.

Please have a review on the patch v6 which:

-Kept execute disable bit both in MpExchangeInfo and CpuMpData.
-Added another patch in which I updated the comments of CpuMpData.

Regards
Yuanhao Xie

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Friday, November 17, 2023 4:38 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Xie, Yuanhao <yuanhao.xie@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [edk2-devel] [Patch V4] UefiCpuPkg/MpInitLib: Enable execute disable bit.

On 11/17/23 08:01, Ni, Ray wrote:
> Laszlo,
> I agree that the BSP's XD status is saved in both CpuMpData and 
> ExchangeInfo.
> But, thinking from another perspective, ExchangeInfo is "only" used by 
> the assembly code. That's why the BSP code needs to save the XD status 
> in CpuMpData and ExchangeInfo.
> 
> If we remove the XD status field in ExchangeInfo, then the assembly 
> code has to understand the structure layout of CpuMpData, which is 
> what I prefer to avoid.
> 
> If you compare all fields in ExchangeInfo and CpuMpData, following 
> fields are already duplicated:
> * CpuMpData.CpuInfoHob <-> MpExchangeInfo.CpuInfo
> * InitFlag
> * SevEsIsEnabled
> * SevSnpIsEnabled
> * GhcbBase
> 
> 
> So, I prefer to keep the current change proposed in Yuanhao's patch.

Very good explanation, thank you.

Can we perhaps document, in an additional patch:

- in "UefiCpuPkg/Library/MpInitLib/MpEqu.inc", that the assembly routines are not supposed to access the internals of CPU_MP_DATA,

- the same statement above "struct _CPU_MP_DATA" in "UefiCpuPkg/Library/MpInitLib/MpLib.h"?

A number of structures in "UefiCpuPkg/Library/MpInitLib/MpLib.h"
document whether they are to be used by assembly code vs. C code (vs.
both), but CPU_MP_DATA doesn't seem to have such comments.

For the current patch:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo



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