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
