Laszlo, I agree to separate another patch to change the prototype of TransferApToSafeState() to reduce one cast operation. I will create v2 for it.
I also agree updating prototype of InterlockedDecrement() is compatible updating. But there are other 5 APIs as blow: InterlockedIncrement() InterlockedCompareExchange16() InterlockedCompareExchange32() InterlockedCompareExchange64() InterlockedCompareExchangePointer() To be consistence, we may need to update them together. Liming & MIke, any comments on this updating. Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Tuesday, November 15, 2016 10:27 AM To: Fan, Jeff; edk2-de...@ml01.01.org Cc: Paolo Bonzini; Yao, Jiewen; Tian, Feng; Kinney, Michael D Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish On 11/15/16 03:18, Jeff Fan wrote: > The GCC 5.4 will optimize mNumberToFinish in EarlyInitializeCpu(). It > will cause > S3 resume failure. > > Adding *volatile* could make sure compiler does not so such optimization. > > Cc: Paolo Bonzini <pbonz...@redhat.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Feng Tian <feng.t...@intel.com> > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff....@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index 3fb6864..f13ff3e 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; > @@ -371,7 +371,7 @@ EarlyMPRendezvousProcedure ( > // > // Count down the number with lock mechanism. > // > - InterlockedDecrement (&mNumberToFinish); > + InterlockedDecrement ((UINT32 *) &mNumberToFinish); > } > > /** > @@ -406,7 +406,7 @@ MPRendezvousProcedure ( > TopOfStack = (UINT32) (UINTN) Stack + sizeof (Stack); > TopOfStack &= ~(UINT32) (CPU_STACK_ALIGNMENT - 1); > CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, > sizeof (mApHltLoopCodeTemplate)); > - TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, > &mNumberToFinish); > + TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, > + (UINT32 *) &mNumberToFinish); > } > > /** > I think I understand the idea, but the current solution requires you to cast away "volatile" in two places. That's not nice, normally it is undefined behavior. I recommend to extend this patch, with more patches: please change the prototype of TransferApToSafeState() so that it takes a pointer-to-volatile. I also suggest to change the prototype of InterlockedDecrement(). (You won't have to update all other call sites: it is fine to take/access a non-volatile object as a volatile, but not the other way around.) I agree this increases the scope of the patch quite a bit, so maybe others should chime in as well. Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
