Re: [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish

[Laszlo Ersek <lersek@redhat.com>]

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