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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

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