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

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

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