On 11/15/16 04:02, Fan, Jeff wrote: > 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.
I agree. I didn't know the full list of Interlocked*() APIs off-hand, but I expected there would be several such functions, and for consistency they should indeed be updated together. I'm not sure if everyone agrees with this, hence discussion is welcome. > > Liming & MIke, any comments on this updating. Yes, please comment! Thanks Laszlo > > 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
