Liming, I think so. Some internal functions in MdePkg\Library\BaseSynchronizationLib is just using volatile for input parameter. For example,
UINT32 EFIAPI InternalSyncIncrement ( IN volatile UINT32 *Value ); Thanks! Jeff From: Gao, Liming Sent: Tuesday, November 15, 2016 6:49 PM To: Fan, Jeff; Laszlo Ersek; edk2-de...@ml01.01.org Cc: Paolo Bonzini; Yao, Jiewen; Tian, Feng; Kinney, Michael D Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish Jeff: From those API design point, does they expect the input parameter is volatile? If yes, I agree this change. Thanks Liming From: Fan, Jeff Sent: Tuesday, November 15, 2016 11:03 AM To: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>>; edk2-de...@ml01.01.org<mailto:edk2-de...@ml01.01.org> Cc: Paolo Bonzini <pbonz...@redhat.com<mailto:pbonz...@redhat.com>>; Yao, Jiewen <jiewen....@intel.com<mailto:jiewen....@intel.com>>; Tian, Feng <feng.t...@intel.com<mailto:feng.t...@intel.com>>; Kinney, Michael D <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Gao, Liming <liming....@intel.com<mailto:liming....@intel.com>> Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish 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<mailto: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 > Cc: Laszlo Ersek > Cc: Jiewen Yao > Cc: Feng Tian > Cc: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan > --- > 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
