[Patch v2 0/2] Fix ACPI S3 resume failure with GCC 5.4

[Patch v2 0/2] Fix ACPI S3 resume failure with GCC 5.4

[Michael Kinney]

Add volatile qualifier to mNumberToFinish to prevent GCC 5.4
compiler from optimizing away required logic in ACPI S3 resume.

Update TransferApToSafeState() use UINTN params to reduce the
number of type casts required in these calls and change
the NumberToFinish parameter from UINT32* to UINTN
NumberToFinishAddress to resolve compiler warnings from
a volatile pointer to a non-volatile pointer conversion.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>

Michael Kinney (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: TransferApToSafeState() use UINTN params
  UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish

 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 10 +++++-----
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 18 +++++++++---------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    | 12 ++++++------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  | 18 +++++++++---------
 4 files changed, 29 insertions(+), 29 deletions(-)

-- 
2.6.3.windows.1

[Patch v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: TransferApToSafeState() use UINTN params

[Michael Kinney]

Update TransferApToSafeState() use UINTN params to reduce the
number of type casts required in these calls.  Also change
the NumberToFinish parameter from UINT32* to UINTN
NumberToFinishAddress to resolve issues with conversion from
a volatile pointer to a non-volatile pointer.  The assembly
code that receives the NumberToFinishAddress value must treat
that memory location as a volatile to track the number of APs.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             |  8 ++++----
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 18 +++++++++---------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    | 12 ++++++------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  | 18 +++++++++---------
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 3fb6864..4af86b7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -385,7 +385,7 @@ MPRendezvousProcedure (
   CPU_REGISTER_TABLE         *RegisterTableList;
   UINT32                     InitApicId;
   UINTN                      Index;
-  UINT32                     TopOfStack;
+  UINTN                      TopOfStack;
   UINT8                      Stack[128];
 
   ProgramVirtualWireMode ();
@@ -403,10 +403,10 @@ MPRendezvousProcedure (
   //
   // Place AP into the safe code, count down the number with lock mechanism in the safe code.
   //
-  TopOfStack  = (UINT32) (UINTN) Stack + sizeof (Stack);
-  TopOfStack &= ~(UINT32) (CPU_STACK_ALIGNMENT - 1);
+  TopOfStack  = (UINTN) Stack + sizeof (Stack);
+  TopOfStack &= ~(UINTN) (CPU_STACK_ALIGNMENT - 1);
   CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, sizeof (mApHltLoopCodeTemplate));
-  TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, &mNumberToFinish);
+  TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)&mNumberToFinish);
 }
 
 /**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
index 4281698..f4db6c8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
@@ -129,23 +129,23 @@ InitGdt (
 /**
   Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch.
 
-  @param[in] ApHltLoopCode    The 32-bit address of the safe hlt-loop function.
-  @param[in] TopOfStack       A pointer to the new stack to use for the ApHltLoopCode.
-  @param[in] NumberToFinish   Semaphore of APs finish count.
+  @param[in] ApHltLoopCode          The address of the safe hlt-loop function.
+  @param[in] TopOfStack             A pointer to the new stack to use for the ApHltLoopCode.
+  @param[in] NumberToFinishAddress  Address of Semaphore of APs finish count.
 
 **/
 VOID
 TransferApToSafeState (
-  IN UINT32             ApHltLoopCode,
-  IN UINT32             TopOfStack,
-  IN UINT32             *NumberToFinish
+  IN UINTN  ApHltLoopCode,
+  IN UINTN  TopOfStack,
+  IN UINTN  NumberToFinishAddress
   )
 {
   SwitchStack (
-    (SWITCH_STACK_ENTRY_POINT) (UINTN) ApHltLoopCode,
-    NumberToFinish,
+    (SWITCH_STACK_ENTRY_POINT)ApHltLoopCode,
+    (VOID *)NumberToFinishAddress,
     NULL,
-    (VOID *) (UINTN) TopOfStack
+    (VOID *)TopOfStack
     );
   //
   // It should never reach here
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 907526e..abe5cc6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -974,16 +974,16 @@ GetAcpiS3EnableFlag (
 /**
   Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch.
 
-  @param[in] ApHltLoopCode    The 32-bit address of the safe hlt-loop function.
-  @param[in] TopOfStack       A pointer to the new stack to use for the ApHltLoopCode.
-  @param[in] NumberToFinish   Semaphore of APs finish count.
+  @param[in] ApHltLoopCode          The address of the safe hlt-loop function.
+  @param[in] TopOfStack             A pointer to the new stack to use for the ApHltLoopCode.
+  @param[in] NumberToFinishAddress  Address of Semaphore of APs finish count.
 
 **/
 VOID
 TransferApToSafeState (
-  IN UINT32             ApHltLoopCode,
-  IN UINT32             TopOfStack,
-  IN UINT32             *NumberToFinish
+  IN UINTN  ApHltLoopCode,
+  IN UINTN  TopOfStack,
+  IN UINTN  NumberToFinishAddress
   );
 
 #endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
index d6d6a1a..9fc00c1 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
@@ -129,24 +129,24 @@ GetProtectedModeCS (
 /**
   Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch.
 
-  @param[in] ApHltLoopCode    The 32-bit address of the safe hlt-loop function.
-  @param[in] TopOfStack       A pointer to the new stack to use for the ApHltLoopCode.
-  @param[in] NumberToFinish   Semaphore of APs finish count.
+  @param[in] ApHltLoopCode          The address of the safe hlt-loop function.
+  @param[in] TopOfStack             A pointer to the new stack to use for the ApHltLoopCode.
+  @param[in] NumberToFinishAddress  Address of Semaphore of APs finish count.
 
 **/
 VOID
 TransferApToSafeState (
-  IN UINT32             ApHltLoopCode,
-  IN UINT32             TopOfStack,
-  IN UINT32             *NumberToFinish
+  IN UINTN  ApHltLoopCode,
+  IN UINTN  TopOfStack,
+  IN UINTN  NumberToFinishAddress
   )
 {
   AsmDisablePaging64 (
     GetProtectedModeCS (),
-    (UINT32) (UINTN) ApHltLoopCode,
-    (UINT32) (UINTN) NumberToFinish,
+    (UINT32)ApHltLoopCode,
+    (UINT32)NumberToFinishAddress,
     0,
-    TopOfStack
+    (UINT32)TopOfStack
     );
   //
   // It should never reach here
-- 
2.6.3.windows.1

[Patch v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish

[Michael Kinney]

Add volatile qualifier to mNumberToFinish to prevent GCC 5.4
compiler from optimizing away required logic in ACPI S3 resume.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 4af86b7..532b7c4 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -55,7 +55,7 @@ AsmGetAddressMap (
 #define LEGACY_REGION_BASE    (0xA0000 - LEGACY_REGION_SIZE)
 
 ACPI_CPU_DATA                mAcpiCpuData;
-UINT32                       mNumberToFinish;
+volatile UINT32              mNumberToFinish;
 MP_CPU_EXCHANGE_INFO         *mExchangeInfo;
 BOOLEAN                      mRestoreSmmConfigurationInS3 = FALSE;
 VOID                         *mGdtForAp = NULL;
-- 
2.6.3.windows.1

Re: [Patch v2 0/2] Fix ACPI S3 resume failure with GCC 5.4

[Laszlo Ersek]

On 11/17/16 22:19, Michael Kinney wrote:
> Add volatile qualifier to mNumberToFinish to prevent GCC 5.4
> compiler from optimizing away required logic in ACPI S3 resume.
> 
> Update TransferApToSafeState() use UINTN params to reduce the
> number of type casts required in these calls and change
> the NumberToFinish parameter from UINT32* to UINTN
> NumberToFinishAddress to resolve compiler warnings from
> a volatile pointer to a non-volatile pointer conversion.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
> 
> Michael Kinney (2):
>   UefiCpuPkg/PiSmmCpuDxeSmm: TransferApToSafeState() use UINTN params
>   UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 10 +++++-----
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 18 +++++++++---------
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    | 12 ++++++------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  | 18 +++++++++---------
>  4 files changed, 29 insertions(+), 29 deletions(-)
> 

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

Re: [Patch v2 0/2] Fix ACPI S3 resume failure with GCC 5.4

[Fan, Jeff]

Reviewed-by: Jeff Fan <jeff.fan@intel.com>

Thanks!

-----Original Message-----
From: Kinney, Michael D 
Sent: Friday, November 18, 2016 5:19 AM
To: edk2-devel@lists.01.org
Cc: Gao, Liming; Laszlo Ersek; Andrew Fish; Fan, Jeff
Subject: [Patch v2 0/2] Fix ACPI S3 resume failure with GCC 5.4

Add volatile qualifier to mNumberToFinish to prevent GCC 5.4 compiler from optimizing away required logic in ACPI S3 resume.

Update TransferApToSafeState() use UINTN params to reduce the number of type casts required in these calls and change the NumberToFinish parameter from UINT32* to UINTN NumberToFinishAddress to resolve compiler warnings from a volatile pointer to a non-volatile pointer conversion.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>

Michael Kinney (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: TransferApToSafeState() use UINTN params
  UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish

 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 10 +++++-----
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 18 +++++++++---------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    | 12 ++++++------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  | 18 +++++++++---------
 4 files changed, 29 insertions(+), 29 deletions(-)

--
2.6.3.windows.1