[edk2-devel] [PATCH 0/5] Use MpService2Ppi to wakeup CPU in Smm CpuS3

[edk2-devel] [PATCH 0/5] Use MpService2Ppi to wakeup CPU in Smm CpuS3

[duntan]

This patch set is to prepare MpService2Ppi in S3Resume when PEI and
SMM env run in the same execution mode, and use MpService2Ppi to
wakeup Cpu to do CPU initialization in Smm CpuS3 boot flow if
MpService2Ppi is not 0 in mSmmS3ResumeState.

Dun Tan (5):
  MdeModulePkg: add MpService2Ppi field in SMM_S3_RESUME_STATE
  UefiCpuPkg/S3Resume2Pei: prepare MpService2Ppi in S3Resume
  UefiCpuPkg/S3Resume2Pei: assert for invalid excution mode combo
  UefiCpuPkg/PiSmmCpuDxe: code refinement for CpuS3.c
  UefiCpuPkg/PiSmmCpuDxe: use MpService2Ppi to wakeup AP in s3

 MdeModulePkg/Include/Guid/AcpiS3Context.h               |   3 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                       | 175 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c       |  25 ++++++++++++++++++++++++-
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf |   3 ++-
 4 files changed, 141 insertions(+), 65 deletions(-)

-- 
2.31.1.windows.1



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

[edk2-devel] [PATCH 1/5] MdeModulePkg: add MpService2Ppi field in SMM_S3_RESUME_STATE

[duntan]

Add MpService2Ppi field in SMM_S3_RESUME_STATE of
AcpiS3Context.h. It will be used to wakeup AP to do the CPU
initialization during smm s3 boot flow in following patches.
With this field, we can avoid sending InitSipiSipi to wakeup
AP.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
---
 MdeModulePkg/Include/Guid/AcpiS3Context.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Include/Guid/AcpiS3Context.h b/MdeModulePkg/Include/Guid/AcpiS3Context.h
index 645496d191..b1c177e072 100644
--- a/MdeModulePkg/Include/Guid/AcpiS3Context.h
+++ b/MdeModulePkg/Include/Guid/AcpiS3Context.h
@@ -1,7 +1,7 @@
 /** @file
   Definitions for data structures used in S3 resume.
 
-Copyright (c) 2011 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2011 - 2023, Intel Corporation. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -20,6 +20,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 typedef struct {
   UINT64                  Signature;
   EFI_PHYSICAL_ADDRESS    SmmS3ResumeEntryPoint;
+  EFI_PHYSICAL_ADDRESS    MpService2Ppi;
   EFI_PHYSICAL_ADDRESS    SmmS3StackBase;
   UINT64                  SmmS3StackSize;
   UINT64                  SmmS3Cr0;
-- 
2.31.1.windows.1



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

Re: [edk2-devel] [PATCH 1/5] MdeModulePkg: add MpService2Ppi field in SMM_S3_RESUME_STATE

[Ni, Ray]

SmmS3ResumeState is a structure which contains two kinds of fields:
* that are initialized in previous boot. E.g.: Signature, Smm***, Smst
* that are initialized in S3 boot: e.g.: Return***

Your patch adds a new field that is initialized in S3 boot path.
Can you put the fields together with other Return*** fields as they are
a group of fields initialized in S3 boot path?

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, July 27, 2023 10:21 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>
> Subject: [PATCH 1/5] MdeModulePkg: add MpService2Ppi field in
> SMM_S3_RESUME_STATE
> 
> Add MpService2Ppi field in SMM_S3_RESUME_STATE of
> AcpiS3Context.h. It will be used to wakeup AP to do the CPU
> initialization during smm s3 boot flow in following patches.
> With this field, we can avoid sending InitSipiSipi to wakeup
> AP.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> ---
>  MdeModulePkg/Include/Guid/AcpiS3Context.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Include/Guid/AcpiS3Context.h
> b/MdeModulePkg/Include/Guid/AcpiS3Context.h
> index 645496d191..b1c177e072 100644
> --- a/MdeModulePkg/Include/Guid/AcpiS3Context.h
> +++ b/MdeModulePkg/Include/Guid/AcpiS3Context.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Definitions for data structures used in S3 resume.
> 
> -Copyright (c) 2011 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2011 - 2023, Intel Corporation. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -20,6 +20,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  typedef struct {
>    UINT64                  Signature;
>    EFI_PHYSICAL_ADDRESS    SmmS3ResumeEntryPoint;
> +  EFI_PHYSICAL_ADDRESS    MpService2Ppi;
>    EFI_PHYSICAL_ADDRESS    SmmS3StackBase;
>    UINT64                  SmmS3StackSize;
>    UINT64                  SmmS3Cr0;
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107336): https://edk2.groups.io/g/devel/message/107336
Mute This Topic: https://groups.io/mt/100383957/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 1/5] MdeModulePkg: add MpService2Ppi field in SMM_S3_RESUME_STATE

[duntan]

Thanks for the comments. I'll put the new fields together with other Return*** fields in next version patch.

Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Friday, July 28, 2023 2:26 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: RE: [PATCH 1/5] MdeModulePkg: add MpService2Ppi field in SMM_S3_RESUME_STATE

SmmS3ResumeState is a structure which contains two kinds of fields:
* that are initialized in previous boot. E.g.: Signature, Smm***, Smst
* that are initialized in S3 boot: e.g.: Return***

Your patch adds a new field that is initialized in S3 boot path.
Can you put the fields together with other Return*** fields as they are a group of fields initialized in S3 boot path?

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, July 27, 2023 10:21 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 
> Gao, Liming <gaoliming@byosoft.com.cn>
> Subject: [PATCH 1/5] MdeModulePkg: add MpService2Ppi field in 
> SMM_S3_RESUME_STATE
> 
> Add MpService2Ppi field in SMM_S3_RESUME_STATE of AcpiS3Context.h. It 
> will be used to wakeup AP to do the CPU initialization during smm s3 
> boot flow in following patches.
> With this field, we can avoid sending InitSipiSipi to wakeup AP.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> ---
>  MdeModulePkg/Include/Guid/AcpiS3Context.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Include/Guid/AcpiS3Context.h
> b/MdeModulePkg/Include/Guid/AcpiS3Context.h
> index 645496d191..b1c177e072 100644
> --- a/MdeModulePkg/Include/Guid/AcpiS3Context.h
> +++ b/MdeModulePkg/Include/Guid/AcpiS3Context.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Definitions for data structures used in S3 resume.
> 
> -Copyright (c) 2011 - 2018, Intel Corporation. All rights 
> reserved.<BR>
> +Copyright (c) 2011 - 2023, Intel Corporation. All rights 
> +reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -20,6 +20,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  
> typedef struct {
>    UINT64                  Signature;
>    EFI_PHYSICAL_ADDRESS    SmmS3ResumeEntryPoint;
> +  EFI_PHYSICAL_ADDRESS    MpService2Ppi;
>    EFI_PHYSICAL_ADDRESS    SmmS3StackBase;
>    UINT64                  SmmS3StackSize;
>    UINT64                  SmmS3Cr0;
> --
> 2.31.1.windows.1



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

[edk2-devel] [PATCH 2/5] UefiCpuPkg/S3Resume2Pei: prepare MpService2Ppi in S3Resume

[duntan]

Prepare MpService2Ppi in S3Resume when PEI and SMM env run
in the same execution mode. Then smm s3 code can use Mp
Service to wakeup AP instead of only sending InitSipiSipi.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c       | 18 +++++++++++++++++-
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf |  3 ++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index 9ea5f6f4e5..6574849939 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
@@ -4,7 +4,7 @@
   This module will execute the boot script saved during last boot and after that,
   control is passed to OS waking up handler.
 
-  Copyright (c) 2006 - 2022, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2006 - 2023, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -23,6 +23,7 @@
 #include <Ppi/PostBootScriptTable.h>
 #include <Ppi/EndOfPeiPhase.h>
 #include <Ppi/SmmCommunication.h>
+#include <Ppi/MpServices2.h>
 
 #include <Library/DebugLib.h>
 #include <Library/BaseLib.h>
@@ -988,6 +989,7 @@ S3RestoreConfig2 (
   BOOLEAN                        Build4GPageTableOnly;
   BOOLEAN                        InterruptStatus;
   IA32_CR0                       Cr0;
+  EDKII_PEI_MP_SERVICES2_PPI     *MpService2Ppi;
 
   TempAcpiS3Context                 = 0;
   TempEfiBootScriptExecutorVariable = 0;
@@ -1088,6 +1090,7 @@ S3RestoreConfig2 (
     SmmS3ResumeState->ReturnContext1     = (EFI_PHYSICAL_ADDRESS)(UINTN)AcpiS3Context;
     SmmS3ResumeState->ReturnContext2     = (EFI_PHYSICAL_ADDRESS)(UINTN)EfiBootScriptExecutorVariable;
     SmmS3ResumeState->ReturnStackPointer = (EFI_PHYSICAL_ADDRESS)STACK_ALIGN_DOWN (&Status);
+    SmmS3ResumeState->MpService2Ppi      = 0;
 
     DEBUG ((DEBUG_INFO, "SMM S3 Signature                = %x\n", SmmS3ResumeState->Signature));
     DEBUG ((DEBUG_INFO, "SMM S3 Stack Base               = %x\n", SmmS3ResumeState->SmmS3StackBase));
@@ -1109,6 +1112,19 @@ S3RestoreConfig2 (
     if (((SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_32) && (sizeof (UINTN) == sizeof (UINT32))) ||
         ((SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_64) && (sizeof (UINTN) == sizeof (UINT64))))
     {
+      //
+      // Get MP Services2 Ppi to pass it to Smm S3.
+      //
+      Status = PeiServicesLocatePpi (
+                 &gEdkiiPeiMpServices2PpiGuid,
+                 0,
+                 NULL,
+                 (VOID **)&MpService2Ppi
+                 );
+      ASSERT_EFI_ERROR (Status);
+      SmmS3ResumeState->MpService2Ppi = (EFI_PHYSICAL_ADDRESS)(UINTN)MpService2Ppi;
+      DEBUG ((DEBUG_INFO, "SMM S3 MpService2Ppi Point = %x\n", SmmS3ResumeState->MpService2Ppi));
+
       SwitchStack (
         (SWITCH_STACK_ENTRY_POINT)(UINTN)SmmS3ResumeState->SmmS3ResumeEntryPoint,
         (VOID *)AcpiS3Context,
diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
index aae984d138..9c9b6f3db3 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
@@ -5,7 +5,7 @@
 # This module will excute the boot script saved during last boot and after that,
 # control is passed to OS waking up handler.
 #
-# Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2010 - 2023, Intel Corporation. All rights reserved.<BR>
 # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -86,6 +86,7 @@
   gPeiPostScriptTablePpiGuid                    ## SOMETIMES_PRODUCES
   gEfiEndOfPeiSignalPpiGuid                     ## SOMETIMES_PRODUCES
   gEfiPeiSmmCommunicationPpiGuid                ## SOMETIMES_CONSUMES
+  gEdkiiPeiMpServices2PpiGuid                   ## SOMETIMES_CONSUMES
 
 [FeaturePcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode         ## CONSUMES
-- 
2.31.1.windows.1



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

Re: [edk2-devel] [PATCH 2/5] UefiCpuPkg/S3Resume2Pei: prepare MpService2Ppi in S3Resume

[Ni, Ray]

> +      DEBUG ((DEBUG_INFO, "SMM S3 MpService2Ppi Point = %x\n",
> SmmS3ResumeState->MpService2Ppi));
Can you please check PrintLib implementation that "%x" is suitable for a 64bit value?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107337): https://edk2.groups.io/g/devel/message/107337
Mute This Topic: https://groups.io/mt/100383960/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 2/5] UefiCpuPkg/S3Resume2Pei: prepare MpService2Ppi in S3Resume

[duntan]

It should be %lx. Will change the print format in next version patch.

Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Friday, July 28, 2023 2:28 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: RE: [PATCH 2/5] UefiCpuPkg/S3Resume2Pei: prepare MpService2Ppi in S3Resume

> +      DEBUG ((DEBUG_INFO, "SMM S3 MpService2Ppi Point = %x\n",
> SmmS3ResumeState->MpService2Ppi));
Can you please check PrintLib implementation that "%x" is suitable for a 64bit value?


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

[edk2-devel] [PATCH 3/5] UefiCpuPkg/S3Resume2Pei: assert for invalid excution mode combo

[duntan]

Add assert for invalid excution mode combination of 64bit PEI +
32bit DXE.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
index 6574849939..34aa901f93 100644
--- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
+++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
@@ -1106,6 +1106,13 @@ S3RestoreConfig2 (
     DEBUG ((DEBUG_INFO, "SMM S3 Return Stack Pointer     = %x\n", SmmS3ResumeState->ReturnStackPointer));
     DEBUG ((DEBUG_INFO, "SMM S3 Smst                     = %x\n", SmmS3ResumeState->Smst));
 
+    //
+    // 64bit PEI and 32bit DXE is not a supported combination.
+    //
+    if (SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_32) {
+      ASSERT (sizeof (UINTN) == sizeof (UINT32));
+    }
+
     //
     // Directly do the switch stack when PEI and SMM env run in the same execution mode.
     //
-- 
2.31.1.windows.1



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

Re: [edk2-devel] [PATCH 3/5] UefiCpuPkg/S3Resume2Pei: assert for invalid excution mode combo

[Ni, Ray]

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

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, July 27, 2023 10:21 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul
> R <rahul.r.kumar@intel.com>
> Subject: [PATCH 3/5] UefiCpuPkg/S3Resume2Pei: assert for invalid excution mode
> combo
> 
> Add assert for invalid excution mode combination of 64bit PEI +
> 32bit DXE.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> index 6574849939..34aa901f93 100644
> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
> @@ -1106,6 +1106,13 @@ S3RestoreConfig2 (
>      DEBUG ((DEBUG_INFO, "SMM S3 Return Stack Pointer     = %x\n",
> SmmS3ResumeState->ReturnStackPointer));
>      DEBUG ((DEBUG_INFO, "SMM S3 Smst                     = %x\n",
> SmmS3ResumeState->Smst));
> 
> +    //
> +    // 64bit PEI and 32bit DXE is not a supported combination.
> +    //
> +    if (SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_32) {
> +      ASSERT (sizeof (UINTN) == sizeof (UINT32));
> +    }
> +
>      //
>      // Directly do the switch stack when PEI and SMM env run in the same
> execution mode.
>      //
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107338): https://edk2.groups.io/g/devel/message/107338
Mute This Topic: https://groups.io/mt/100383961/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 4/5] UefiCpuPkg/PiSmmCpuDxe: code refinement for CpuS3.c

[duntan]

This commit is code logic refinement for CpuS3.c. It doesn't
change any code functionality.
In this commit, abstract the function originally executed by BSP
into a new InitializeBsp(). Also prepare the AP StartupVector and
send InitSipiSipi in SmmRestoreCpu() when mAcpiCpuData is valid.
Or only InitializeBsp will be executed by BSP. This can make the
code logic easier to understand.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------
 1 file changed, 63 insertions(+), 47 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 0f7ee0372d..d2e2135d08 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -627,6 +627,7 @@ PrepareApStartupVector (
   mExchangeInfo->BufferStart                         = (UINT32)StartupVector;
   mExchangeInfo->Cr3                                 = (UINT32)(AsmReadCr3 ());
   mExchangeInfo->InitializeFloatingPointUnitsAddress = (UINTN)InitializeFloatingPointUnits;
+  mExchangeInfo->ApFunction                          = (VOID *)(UINTN)InitializeAp;
 }
 
 /**
@@ -647,27 +648,6 @@ InitializeCpuBeforeRebase (
 
   ProgramVirtualWireMode ();
 
-  PrepareApStartupVector (mAcpiCpuData.StartupVector);
-
-  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
-    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
-  } else {
-    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
-  }
-
-  mNumberToFinish           = (UINT32)(mNumberOfCpus - 1);
-  mExchangeInfo->ApFunction = (VOID *)(UINTN)InitializeAp;
-
-  //
-  // Execute code for before SmmBaseReloc. Note: This flag is maintained across S3 boots.
-  //
-  mInitApsAfterSmmBaseReloc = FALSE;
-
-  //
-  // Send INIT IPI - SIPI to all APs
-  //
-  SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
-
   while (mNumberToFinish > 0) {
     CpuPause ();
   }
@@ -714,6 +694,54 @@ InitializeCpuAfterRebase (
   }
 }
 
+/**
+  Procedure for BSP to do the cpu initialization.
+
+**/
+VOID
+InitializeBsp (
+  VOID
+  )
+{
+  //
+  // Skip initialization if mAcpiCpuData is not valid
+  //
+  if (mAcpiCpuData.NumberOfCpus > 0) {
+    //
+    // First time microcode load and restore MTRRs
+    //
+    InitializeCpuBeforeRebase ();
+  }
+
+  DEBUG ((DEBUG_INFO, "SmmRestoreCpu: mSmmRelocated is %d\n", mSmmRelocated));
+
+  //
+  // Check whether Smm Relocation is done or not.
+  // If not, will do the SmmBases Relocation here!!!
+  //
+  if (!mSmmRelocated) {
+    //
+    // Restore SMBASE for BSP and all APs
+    //
+    SmmRelocateBases ();
+  } else {
+    //
+    // Issue SMI IPI (All Excluding  Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+    //
+    ExecuteFirstSmiInit ();
+  }
+
+  //
+  // Skip initialization if mAcpiCpuData is not valid
+  //
+  if (mAcpiCpuData.NumberOfCpus > 0) {
+    //
+    // Restore MSRs for BSP and all APs
+    //
+    InitializeCpuAfterRebase ();
+  }
+}
+
 /**
   Restore SMM Configuration in S3 boot path.
 
@@ -814,43 +842,31 @@ SmmRestoreCpu (
   }
 
   //
-  // Skip initialization if mAcpiCpuData is not valid
+  // Skip AP initialization if mAcpiCpuData is not valid
   //
   if (mAcpiCpuData.NumberOfCpus > 0) {
-    //
-    // First time microcode load and restore MTRRs
-    //
-    InitializeCpuBeforeRebase ();
-  }
+    if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+      ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
+    } else {
+      ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
+    }
 
-  DEBUG ((DEBUG_INFO, "SmmRestoreCpu: mSmmRelocated is %d\n", mSmmRelocated));
+    mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
 
-  //
-  // Check whether Smm Relocation is done or not.
-  // If not, will do the SmmBases Relocation here!!!
-  //
-  if (!mSmmRelocated) {
-    //
-    // Restore SMBASE for BSP and all APs
-    //
-    SmmRelocateBases ();
-  } else {
     //
-    // Issue SMI IPI (All Excluding  Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+    // Execute code for before SmmBaseReloc. Note: This flag is maintained across S3 boots.
     //
-    ExecuteFirstSmiInit ();
-  }
+    mInitApsAfterSmmBaseReloc = FALSE;
 
-  //
-  // Skip initialization if mAcpiCpuData is not valid
-  //
-  if (mAcpiCpuData.NumberOfCpus > 0) {
+    PrepareApStartupVector (mAcpiCpuData.StartupVector);
     //
-    // Restore MSRs for BSP and all APs
+    // Send INIT IPI - SIPI to all APs
     //
-    InitializeCpuAfterRebase ();
+    SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
   }
 
+  InitializeBsp ();
+
   //
   // Set a flag to restore SMM configuration in S3 path.
   //
-- 
2.31.1.windows.1



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

Re: [edk2-devel] [PATCH 4/5] UefiCpuPkg/PiSmmCpuDxe: code refinement for CpuS3.c

[Ni, Ray]

1. mInitApsAfterSmmBaseReloc assignment is done in different places. It's set to FALSE before BSP/AP initialization, and it's set to TRUE in InitializeCpuAfterRebase().
    Do you think it's better to only assign it to FALSE/TRUE in InitializeBsp()?

2. ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus); is done in multiple places.
    Do you think the assertion in InitializeCpuAfterRebase() can be removed?

3. Do you think if InitializeAp and InitializeBsp could be implemented as a single function because I found that they are doing almost the same thing?


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Thursday, July 27, 2023 10:21 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul
> R <rahul.r.kumar@intel.com>
> Subject: [edk2-devel] [PATCH 4/5] UefiCpuPkg/PiSmmCpuDxe: code refinement
> for CpuS3.c
> 
> This commit is code logic refinement for CpuS3.c. It doesn't
> change any code functionality.
> In this commit, abstract the function originally executed by BSP
> into a new InitializeBsp(). Also prepare the AP StartupVector and
> send InitSipiSipi in SmmRestoreCpu() when mAcpiCpuData is valid.
> Or only InitializeBsp will be executed by BSP. This can make the
> code logic easier to understand.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 110
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> --------------------------------------------
>  1 file changed, 63 insertions(+), 47 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 0f7ee0372d..d2e2135d08 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -627,6 +627,7 @@ PrepareApStartupVector (
>    mExchangeInfo->BufferStart                         = (UINT32)StartupVector;
>    mExchangeInfo->Cr3                                 = (UINT32)(AsmReadCr3 ());
>    mExchangeInfo->InitializeFloatingPointUnitsAddress =
> (UINTN)InitializeFloatingPointUnits;
> +  mExchangeInfo->ApFunction                          = (VOID *)(UINTN)InitializeAp;
>  }
> 
>  /**
> @@ -647,27 +648,6 @@ InitializeCpuBeforeRebase (
> 
>    ProgramVirtualWireMode ();
> 
> -  PrepareApStartupVector (mAcpiCpuData.StartupVector);
> -
> -  if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
> -    ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
> -  } else {
> -    ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
> -  }
> -
> -  mNumberToFinish           = (UINT32)(mNumberOfCpus - 1);
> -  mExchangeInfo->ApFunction = (VOID *)(UINTN)InitializeAp;
> -
> -  //
> -  // Execute code for before SmmBaseReloc. Note: This flag is maintained across
> S3 boots.
> -  //
> -  mInitApsAfterSmmBaseReloc = FALSE;
> -
> -  //
> -  // Send INIT IPI - SIPI to all APs
> -  //
> -  SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
> -
>    while (mNumberToFinish > 0) {
>      CpuPause ();
>    }
> @@ -714,6 +694,54 @@ InitializeCpuAfterRebase (
>    }
>  }
> 
> +/**
> +  Procedure for BSP to do the cpu initialization.
> +
> +**/
> +VOID
> +InitializeBsp (
> +  VOID
> +  )
> +{
> +  //
> +  // Skip initialization if mAcpiCpuData is not valid
> +  //
> +  if (mAcpiCpuData.NumberOfCpus > 0) {
> +    //
> +    // First time microcode load and restore MTRRs
> +    //
> +    InitializeCpuBeforeRebase ();
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "SmmRestoreCpu: mSmmRelocated is %d\n",
> mSmmRelocated));
> +
> +  //
> +  // Check whether Smm Relocation is done or not.
> +  // If not, will do the SmmBases Relocation here!!!
> +  //
> +  if (!mSmmRelocated) {
> +    //
> +    // Restore SMBASE for BSP and all APs
> +    //
> +    SmmRelocateBases ();
> +  } else {
> +    //
> +    // Issue SMI IPI (All Excluding  Self SMM IPI + BSP SMM IPI) to execute first
> SMI init.
> +    //
> +    ExecuteFirstSmiInit ();
> +  }
> +
> +  //
> +  // Skip initialization if mAcpiCpuData is not valid
> +  //
> +  if (mAcpiCpuData.NumberOfCpus > 0) {
> +    //
> +    // Restore MSRs for BSP and all APs
> +    //
> +    InitializeCpuAfterRebase ();
> +  }
> +}
> +
>  /**
>    Restore SMM Configuration in S3 boot path.
> 
> @@ -814,43 +842,31 @@ SmmRestoreCpu (
>    }
> 
>    //
> -  // Skip initialization if mAcpiCpuData is not valid
> +  // Skip AP initialization if mAcpiCpuData is not valid
>    //
>    if (mAcpiCpuData.NumberOfCpus > 0) {
> -    //
> -    // First time microcode load and restore MTRRs
> -    //
> -    InitializeCpuBeforeRebase ();
> -  }
> +    if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
> +      ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
> +    } else {
> +      ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
> +    }
> 
> -  DEBUG ((DEBUG_INFO, "SmmRestoreCpu: mSmmRelocated is %d\n",
> mSmmRelocated));
> +    mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
> 
> -  //
> -  // Check whether Smm Relocation is done or not.
> -  // If not, will do the SmmBases Relocation here!!!
> -  //
> -  if (!mSmmRelocated) {
> -    //
> -    // Restore SMBASE for BSP and all APs
> -    //
> -    SmmRelocateBases ();
> -  } else {
>      //
> -    // Issue SMI IPI (All Excluding  Self SMM IPI + BSP SMM IPI) to execute first SMI
> init.
> +    // Execute code for before SmmBaseReloc. Note: This flag is maintained
> across S3 boots.
>      //
> -    ExecuteFirstSmiInit ();
> -  }
> +    mInitApsAfterSmmBaseReloc = FALSE;
> 
> -  //
> -  // Skip initialization if mAcpiCpuData is not valid
> -  //
> -  if (mAcpiCpuData.NumberOfCpus > 0) {
> +    PrepareApStartupVector (mAcpiCpuData.StartupVector);
>      //
> -    // Restore MSRs for BSP and all APs
> +    // Send INIT IPI - SIPI to all APs
>      //
> -    InitializeCpuAfterRebase ();
> +    SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
>    }
> 
> +  InitializeBsp ();
> +
>    //
>    // Set a flag to restore SMM configuration in S3 path.
>    //
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107373): https://edk2.groups.io/g/devel/message/107373
Mute This Topic: https://groups.io/mt/100383962/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 5/5] UefiCpuPkg/PiSmmCpuDxe: use MpService2Ppi to wakeup AP in s3

[duntan]

Use MpService2Ppi to wakeup AP in s3 boot flow during initializing
CPU. If mSmmS3ResumeState->MpService2Ppi is not 0, then BSP will
use MpService2Ppi->StartupAllCPUs to do CPU initialization for both
BSP and AP instead of only sending InitSipiSipi for AP.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 55 insertions(+), 20 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index d2e2135d08..8b0e4afc59 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -7,6 +7,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
 #include "PiSmmCpuDxeSmm.h"
+#include <PiPei.h>
+#include <Ppi/MpServices2.h>
 
 #pragma pack(1)
 typedef struct {
@@ -571,12 +573,17 @@ InitializeAp (
   SetRegister (FALSE);
 
   //
-  // Place AP into the safe code, count down the number with lock mechanism in the safe code.
+  // When using MpService2Ppi to wakeup AP, BSP will fail to return from MpService2Ppi if AP hang there.
   //
-  TopOfStack  = (UINTN)Stack + sizeof (Stack);
-  TopOfStack &= ~(UINTN)(CPU_STACK_ALIGNMENT - 1);
-  CopyMem ((VOID *)(UINTN)mApHltLoopCode, mApHltLoopCodeTemplate, sizeof (mApHltLoopCodeTemplate));
-  TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)&mNumberToFinish);
+  if (mSmmS3ResumeState->MpService2Ppi == 0) {
+    //
+    // Place AP into the safe code, count down the number with lock mechanism in the safe code.
+    //
+    TopOfStack  = (UINTN)Stack + sizeof (Stack);
+    TopOfStack &= ~(UINTN)(CPU_STACK_ALIGNMENT - 1);
+    CopyMem ((VOID *)(UINTN)mApHltLoopCode, mApHltLoopCodeTemplate, sizeof (mApHltLoopCodeTemplate));
+    TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)&mNumberToFinish);
+  }
 }
 
 /**
@@ -634,7 +641,7 @@ PrepareApStartupVector (
   The function is invoked before SMBASE relocation in S3 path to restores CPU status.
 
   The function is invoked before SMBASE relocation in S3 path. It does first time microcode load
-  and restores MTRRs for both BSP and APs.
+  and restores MTRRs for BSP.
 
 **/
 VOID
@@ -689,8 +696,10 @@ InitializeCpuAfterRebase (
   //
   SetRegister (FALSE);
 
-  while (mNumberToFinish > 0) {
-    CpuPause ();
+  if (mSmmS3ResumeState->MpService2Ppi == 0) {
+    while (mNumberToFinish > 0) {
+      CpuPause ();
+    }
   }
 }
 
@@ -742,6 +751,24 @@ InitializeBsp (
   }
 }
 
+/**
+  Cpu initialization procedure.
+
+  @param[in,out] Buffer  The pointer to private data buffer.
+**/
+VOID
+EFIAPI
+InitializeCpuProcedure (
+  IN OUT VOID  *Buffer
+  )
+{
+  if (mBspApicId == GetApicId ()) {
+    InitializeBsp ();
+  } else {
+    InitializeAp ();
+  }
+}
+
 /**
   Restore SMM Configuration in S3 boot path.
 
@@ -790,11 +817,12 @@ SmmRestoreCpu (
   VOID
   )
 {
-  SMM_S3_RESUME_STATE       *SmmS3ResumeState;
-  IA32_DESCRIPTOR           Ia32Idtr;
-  IA32_DESCRIPTOR           X64Idtr;
-  IA32_IDT_GATE_DESCRIPTOR  IdtEntryTable[EXCEPTION_VECTOR_NUMBER];
-  EFI_STATUS                Status;
+  SMM_S3_RESUME_STATE         *SmmS3ResumeState;
+  IA32_DESCRIPTOR             Ia32Idtr;
+  IA32_DESCRIPTOR             X64Idtr;
+  IA32_IDT_GATE_DESCRIPTOR    IdtEntryTable[EXCEPTION_VECTOR_NUMBER];
+  EFI_STATUS                  Status;
+  EDKII_PEI_MP_SERVICES2_PPI  *Mp2ServicePpi;
 
   DEBUG ((DEBUG_INFO, "SmmRestoreCpu()\n"));
 
@@ -858,15 +886,22 @@ SmmRestoreCpu (
     //
     mInitApsAfterSmmBaseReloc = FALSE;
 
-    PrepareApStartupVector (mAcpiCpuData.StartupVector);
-    //
-    // Send INIT IPI - SIPI to all APs
-    //
-    SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
+    if (mSmmS3ResumeState->MpService2Ppi != 0) {
+      mBspApicId    = GetApicId ();
+      Mp2ServicePpi = (EDKII_PEI_MP_SERVICES2_PPI *)(UINTN)mSmmS3ResumeState->MpService2Ppi;
+      Mp2ServicePpi->StartupAllCPUs (Mp2ServicePpi, (EFI_AP_PROCEDURE)InitializeCpuProcedure, 0, NULL);
+    } else {
+      PrepareApStartupVector (mAcpiCpuData.StartupVector);
+      //
+      // Send INIT IPI - SIPI to all APs
+      //
+      SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
+      InitializeBsp ();
+    }
+  } else {
+    InitializeBsp ();
   }
 
-  InitializeBsp ();
-
   //
   // Set a flag to restore SMM configuration in S3 path.
   //
-- 
2.31.1.windows.1



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