* [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish
@ 2016-11-17 5:34 Michael Kinney
2016-11-17 10:21 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Michael Kinney @ 2016-11-17 5:34 UTC (permalink / raw)
To: edk2-devel; +Cc: Liming Gao, Laszlo Ersek, Andrew Fish, Jeff Fan
Add volatile qualifier to mNumberToFinish to prevent compiler
optimization. Also update TransferApToSafeState() to pass in
UINTN values and treat the mNumberToFinish as an address value
that is passed to the assembly code.
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 | 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(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 3fb6864..4531298 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;
@@ -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 &= ~(CPU_STACK_ALIGNMENT - 1);
CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, sizeof (mApHltLoopCodeTemplate));
- TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, &mNumberToFinish);
+ TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID *)&mNumberToFinish);
}
/**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
index 9760373..d57eb33 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
@@ -98,23 +98,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 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] 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 88d9c85..38dd9fa 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -828,16 +828,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 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] 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 6844c3f..d45fed2 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
@@ -103,24 +103,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 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] 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish
2016-11-17 5:34 [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish Michael Kinney
@ 2016-11-17 10:21 ` Laszlo Ersek
2016-11-17 16:06 ` Kinney, Michael D
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2016-11-17 10:21 UTC (permalink / raw)
To: Michael Kinney, edk2-devel; +Cc: Andrew Fish, Liming Gao, Jeff Fan
On 11/17/16 06:34, Michael Kinney wrote:
> Add volatile qualifier to mNumberToFinish to prevent compiler
> optimization. Also update TransferApToSafeState() to pass in
> UINTN values and treat the mNumberToFinish as an address value
> that is passed to the assembly code.
Is it possible to split these actions into two patches? One for
mNumberToFinish, the other patch for UINTN values.
> 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 | 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(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 3fb6864..4531298 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;
> @@ -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);
This change looks good.
> + TopOfStack &= ~(CPU_STACK_ALIGNMENT - 1);
Please don't drop the UINT32 cast from before the bit-neg altogether,
instead please turn it into a UINTN cast.
The reason is that CPU_STACK_ALIGNMENT has type "int", so the bit-neg
flips the sign bit too. The resultant value is a negative integer.
Due to the two's complement representation, the negative value is
actually correct, and when it is converted to UINTN, for the sake of the
"&" operation that's inherent in "&=", the behavior is correct. But this
silently relies on two's complement, which in my personal opinion is a
bad thing. I consider the changing of sign bits with direct bit
operations a sin :) -- unless mangling the sign bit is our express
purpose -- so I request that we please convert the int value first to
the unsigned type with correct width (here, UINTN), and then negate the
bits.
> CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, sizeof (mApHltLoopCodeTemplate));
> - TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, &mNumberToFinish);
> + TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID *)&mNumberToFinish);
> }
>
> /**
The (VOID*) cast is unnecessary here (in the last arg), any pointer can
be cast to UINTN directly.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> index 9760373..d57eb33 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> @@ -98,23 +98,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 32-bit address of the safe hlt-loop function.
Should we replace "32-bit" with "natural width"?
> + @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
Looks okay.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 88d9c85..38dd9fa 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -828,16 +828,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 32-bit address of the safe hlt-loop function.
If we update the comment, we should do it here too.
> + @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 6844c3f..d45fed2 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> @@ -103,24 +103,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.
and here
> - @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 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] 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
>
Looks good to me.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish
2016-11-17 10:21 ` Laszlo Ersek
@ 2016-11-17 16:06 ` Kinney, Michael D
2016-11-17 20:59 ` Kinney, Michael D
0 siblings, 1 reply; 4+ messages in thread
From: Kinney, Michael D @ 2016-11-17 16:06 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@ml01.01.org, Kinney, Michael D
Cc: Andrew Fish, Gao, Liming, Fan, Jeff
Laszlo,
Yes. I can split. I will send V2 with your feedback.
For the following line, the cast to (VOID *) before cast to (UINTN) was
required to pass VS build. Since mNumberToFinish is a volatile pointer,
it needs to be cast to a non-volatile pointer before the pointer can be
converted to an unsigned integer value.
TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID *)&mNumberToFinish);
Mike
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 17, 2016 2:22 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@ml01.01.org
> Cc: Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff
> <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to
> mNumberToFinish
>
> On 11/17/16 06:34, Michael Kinney wrote:
> > Add volatile qualifier to mNumberToFinish to prevent compiler
> > optimization. Also update TransferApToSafeState() to pass in
> > UINTN values and treat the mNumberToFinish as an address value
> > that is passed to the assembly code.
>
> Is it possible to split these actions into two patches? One for
> mNumberToFinish, the other patch for UINTN values.
>
> > 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 | 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(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > index 3fb6864..4531298 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;
> > @@ -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);
>
> This change looks good.
>
> > + TopOfStack &= ~(CPU_STACK_ALIGNMENT - 1);
>
> Please don't drop the UINT32 cast from before the bit-neg altogether,
> instead please turn it into a UINTN cast.
>
> The reason is that CPU_STACK_ALIGNMENT has type "int", so the bit-neg
> flips the sign bit too. The resultant value is a negative integer.
>
> Due to the two's complement representation, the negative value is
> actually correct, and when it is converted to UINTN, for the sake of the
> "&" operation that's inherent in "&=", the behavior is correct. But this
> silently relies on two's complement, which in my personal opinion is a
> bad thing. I consider the changing of sign bits with direct bit
> operations a sin :) -- unless mangling the sign bit is our express
> purpose -- so I request that we please convert the int value first to
> the unsigned type with correct width (here, UINTN), and then negate the
> bits.
>
> > CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, sizeof
> (mApHltLoopCodeTemplate));
> > - TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack,
> &mNumberToFinish);
> > + TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID
> *)&mNumberToFinish);
> > }
> >
> > /**
>
> The (VOID*) cast is unnecessary here (in the last arg), any pointer can
> be cast to UINTN directly.
>
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> > index 9760373..d57eb33 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> > @@ -98,23 +98,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 32-bit address of the safe hlt-loop
> function.
>
> Should we replace "32-bit" with "natural width"?
>
> > + @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
>
> Looks okay.
>
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 88d9c85..38dd9fa 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -828,16 +828,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 32-bit address of the safe hlt-loop
> function.
>
> If we update the comment, we should do it here too.
>
> > + @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 6844c3f..d45fed2 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> > @@ -103,24 +103,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.
>
> and here
>
> > - @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 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] 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
> >
>
> Looks good to me.
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish
2016-11-17 16:06 ` Kinney, Michael D
@ 2016-11-17 20:59 ` Kinney, Michael D
0 siblings, 0 replies; 4+ messages in thread
From: Kinney, Michael D @ 2016-11-17 20:59 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@ml01.01.org, Kinney, Michael D
Cc: Andrew Fish, Gao, Liming, Fan, Jeff
Laszlo,
I redid some tests using the following as you suggested, and it works.
TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)&mNumberToFinish);
So I will use this style in V2.
Mike
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, November 17, 2016 8:06 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@ml01.01.org; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff
> <jeff.fan@intel.com>
> Subject: RE: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to
> mNumberToFinish
>
> Laszlo,
>
> Yes. I can split. I will send V2 with your feedback.
>
> For the following line, the cast to (VOID *) before cast to (UINTN) was
> required to pass VS build. Since mNumberToFinish is a volatile pointer,
> it needs to be cast to a non-volatile pointer before the pointer can be
> converted to an unsigned integer value.
>
> TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID
> *)&mNumberToFinish);
>
> Mike
>
>
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Thursday, November 17, 2016 2:22 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@ml01.01.org
> > Cc: Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff
> > <jeff.fan@intel.com>
> > Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to
> > mNumberToFinish
> >
> > On 11/17/16 06:34, Michael Kinney wrote:
> > > Add volatile qualifier to mNumberToFinish to prevent compiler
> > > optimization. Also update TransferApToSafeState() to pass in
> > > UINTN values and treat the mNumberToFinish as an address value
> > > that is passed to the assembly code.
> >
> > Is it possible to split these actions into two patches? One for
> > mNumberToFinish, the other patch for UINTN values.
> >
> > > 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 | 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(-)
> > >
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > > index 3fb6864..4531298 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;
> > > @@ -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);
> >
> > This change looks good.
> >
> > > + TopOfStack &= ~(CPU_STACK_ALIGNMENT - 1);
> >
> > Please don't drop the UINT32 cast from before the bit-neg altogether,
> > instead please turn it into a UINTN cast.
> >
> > The reason is that CPU_STACK_ALIGNMENT has type "int", so the bit-neg
> > flips the sign bit too. The resultant value is a negative integer.
> >
> > Due to the two's complement representation, the negative value is
> > actually correct, and when it is converted to UINTN, for the sake of the
> > "&" operation that's inherent in "&=", the behavior is correct. But this
> > silently relies on two's complement, which in my personal opinion is a
> > bad thing. I consider the changing of sign bits with direct bit
> > operations a sin :) -- unless mangling the sign bit is our express
> > purpose -- so I request that we please convert the int value first to
> > the unsigned type with correct width (here, UINTN), and then negate the
> > bits.
> >
> > > CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, sizeof
> > (mApHltLoopCodeTemplate));
> > > - TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack,
> > &mNumberToFinish);
> > > + TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID
> > *)&mNumberToFinish);
> > > }
> > >
> > > /**
> >
> > The (VOID*) cast is unnecessary here (in the last arg), any pointer can
> > be cast to UINTN directly.
> >
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> > > index 9760373..d57eb33 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> > > @@ -98,23 +98,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 32-bit address of the safe hlt-loop
> > function.
> >
> > Should we replace "32-bit" with "natural width"?
> >
> > > + @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
> >
> > Looks okay.
> >
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > > index 88d9c85..38dd9fa 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > > @@ -828,16 +828,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 32-bit address of the safe hlt-loop
> > function.
> >
> > If we update the comment, we should do it here too.
> >
> > > + @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 6844c3f..d45fed2 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> > > @@ -103,24 +103,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.
> >
> > and here
> >
> > > - @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 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] 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
> > >
> >
> > Looks good to me.
> >
> > Thanks
> > Laszlo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-17 20:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-17 5:34 [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish Michael Kinney
2016-11-17 10:21 ` Laszlo Ersek
2016-11-17 16:06 ` Kinney, Michael D
2016-11-17 20:59 ` Kinney, Michael D
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox