* [PATCH] UefiCpuPkg/DxeMpLib: Remove sizeof() that is not necessary @ 2016-11-15 2:23 Jeff Fan 2016-11-15 2:23 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish Jeff Fan 2016-11-15 2:23 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Free SmramRanges to save SMM space Jeff Fan 0 siblings, 2 replies; 10+ messages in thread From: Jeff Fan @ 2016-11-15 2:23 UTC (permalink / raw) To: edk2-devel; +Cc: Laszlo Ersek, Feng Tian, Michael D Kinney Reported at https://lists.01.org/pipermail/edk2-devel/2016-November/004685.html Reported-by: Laszlo Ersek <lersek@redhat.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Feng Tian <feng.tian@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Fan <jeff.fan@intel.com> --- UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c index 7f3900b..dcc9134 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c @@ -311,7 +311,7 @@ InitMpGlobalData ( Status = gBS->AllocatePages ( AllocateMaxAddress, EfiReservedMemoryType, - EFI_SIZE_TO_PAGES (sizeof (CpuMpData->AddressMap.RelocateApLoopFuncSize)), + EFI_SIZE_TO_PAGES (CpuMpData->AddressMap.RelocateApLoopFuncSize), &Address ); ASSERT_EFI_ERROR (Status); -- 2.9.3.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish 2016-11-15 2:23 [PATCH] UefiCpuPkg/DxeMpLib: Remove sizeof() that is not necessary Jeff Fan @ 2016-11-15 2:23 ` Jeff Fan 2016-11-15 2:23 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Free SmramRanges to save SMM space Jeff Fan 1 sibling, 0 replies; 10+ messages in thread From: Jeff Fan @ 2016-11-15 2:23 UTC (permalink / raw) To: edk2-devel Cc: Paolo Bonzini, Laszlo Ersek, Jiewen Yao, Feng Tian, Michael D Kinney 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 <pbonzini@redhat.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Feng Tian <feng.tian@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Fan <jeff.fan@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); } /** -- 2.9.3.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Free SmramRanges to save SMM space 2016-11-15 2:23 [PATCH] UefiCpuPkg/DxeMpLib: Remove sizeof() that is not necessary Jeff Fan 2016-11-15 2:23 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish Jeff Fan @ 2016-11-15 2:23 ` Jeff Fan 2016-11-15 7:33 ` Zeng, Star 1 sibling, 1 reply; 10+ messages in thread From: Jeff Fan @ 2016-11-15 2:23 UTC (permalink / raw) To: edk2-devel; +Cc: Zeng Star, Feng Tian, Michael D Kinney Cc: Zeng Star <star.zeng@intel.com> Cc: Feng Tian <feng.tian@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Fan <jeff.fan@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c index 852b5c7..5f13446 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c @@ -1004,6 +1004,7 @@ FindSmramInfo ( } } while (Found); + FreePool (SmramRanges); DEBUG ((EFI_D_INFO, "SMRR Base: 0x%x, SMRR Size: 0x%x\n", *SmrrBase, *SmrrSize)); } -- 2.9.3.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Free SmramRanges to save SMM space 2016-11-15 2:23 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Free SmramRanges to save SMM space Jeff Fan @ 2016-11-15 7:33 ` Zeng, Star 0 siblings, 0 replies; 10+ messages in thread From: Zeng, Star @ 2016-11-15 7:33 UTC (permalink / raw) To: Fan, Jeff, edk2-devel@lists.01.org; +Cc: Tian, Feng, Kinney, Michael D Reviewed-by: Star Zeng <star.zeng@intel.com> -----Original Message----- From: Fan, Jeff Sent: Tuesday, November 15, 2016 10:24 AM To: edk2-devel@lists.01.org Cc: Zeng, Star <star.zeng@intel.com>; Tian, Feng <feng.tian@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Free SmramRanges to save SMM space Cc: Zeng Star <star.zeng@intel.com> Cc: Feng Tian <feng.tian@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Fan <jeff.fan@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c index 852b5c7..5f13446 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c @@ -1004,6 +1004,7 @@ FindSmramInfo ( } } while (Found); + FreePool (SmramRanges); DEBUG ((EFI_D_INFO, "SMRR Base: 0x%x, SMRR Size: 0x%x\n", *SmrrBase, *SmrrSize)); } -- 2.9.3.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] UefiCpuPkg/DxeMpLib: Remove sizeof() that is not necessary @ 2016-11-15 2:18 Jeff Fan 2016-11-15 2:18 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish Jeff Fan 0 siblings, 1 reply; 10+ messages in thread From: Jeff Fan @ 2016-11-15 2:18 UTC (permalink / raw) To: edk2-devel; +Cc: Laszlo Ersek, Feng Tian, Michael D Kinney Reported at https://lists.01.org/pipermail/edk2-devel/2016-November/004685.html Reported-by: Laszlo Ersek <lersek@redhat.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Feng Tian <feng.tian@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Fan <jeff.fan@intel.com> --- UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c index 7f3900b..dcc9134 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c @@ -311,7 +311,7 @@ InitMpGlobalData ( Status = gBS->AllocatePages ( AllocateMaxAddress, EfiReservedMemoryType, - EFI_SIZE_TO_PAGES (sizeof (CpuMpData->AddressMap.RelocateApLoopFuncSize)), + EFI_SIZE_TO_PAGES (CpuMpData->AddressMap.RelocateApLoopFuncSize), &Address ); ASSERT_EFI_ERROR (Status); -- 2.9.3.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish 2016-11-15 2:18 [PATCH] UefiCpuPkg/DxeMpLib: Remove sizeof() that is not necessary Jeff Fan @ 2016-11-15 2:18 ` Jeff Fan 2016-11-15 2:27 ` Laszlo Ersek 0 siblings, 1 reply; 10+ messages in thread From: Jeff Fan @ 2016-11-15 2:18 UTC (permalink / raw) To: edk2-devel Cc: Paolo Bonzini, Laszlo Ersek, Jiewen Yao, Feng Tian, Michael D Kinney 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 <pbonzini@redhat.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Feng Tian <feng.tian@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Fan <jeff.fan@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); } /** -- 2.9.3.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish 2016-11-15 2:18 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish Jeff Fan @ 2016-11-15 2:27 ` Laszlo Ersek 2016-11-15 3:02 ` Fan, Jeff 0 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2016-11-15 2:27 UTC (permalink / raw) To: Jeff Fan, edk2-devel Cc: Paolo Bonzini, Jiewen Yao, Feng Tian, Michael D Kinney 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 <pbonzini@redhat.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Feng Tian <feng.tian@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff.fan@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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish 2016-11-15 2:27 ` Laszlo Ersek @ 2016-11-15 3:02 ` Fan, Jeff 2016-11-15 10:49 ` Gao, Liming 2016-11-15 15:24 ` Laszlo Ersek 0 siblings, 2 replies; 10+ messages in thread From: Fan, Jeff @ 2016-11-15 3:02 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@ml01.01.org Cc: Paolo Bonzini, Yao, Jiewen, Tian, Feng, Kinney, Michael D, Gao, Liming 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:lersek@redhat.com] Sent: Tuesday, November 15, 2016 10:27 AM To: Fan, Jeff; edk2-devel@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 <pbonzini@redhat.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Feng Tian <feng.tian@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff.fan@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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish 2016-11-15 3:02 ` Fan, Jeff @ 2016-11-15 10:49 ` Gao, Liming 2016-11-15 13:35 ` Fan, Jeff 2016-11-15 15:24 ` Laszlo Ersek 1 sibling, 1 reply; 10+ messages in thread From: Gao, Liming @ 2016-11-15 10:49 UTC (permalink / raw) To: Fan, Jeff, Laszlo Ersek, edk2-devel@ml01.01.org Cc: Paolo Bonzini, Yao, Jiewen, Tian, Feng, Kinney, Michael D 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 <lersek@redhat.com>; edk2-devel@ml01.01.org Cc: Paolo Bonzini <pbonzini@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tian, Feng <feng.tian@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@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:lersek@redhat.com] Sent: Tuesday, November 15, 2016 10:27 AM To: Fan, Jeff; edk2-devel@ml01.01.org<mailto:edk2-devel@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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish 2016-11-15 10:49 ` Gao, Liming @ 2016-11-15 13:35 ` Fan, Jeff 0 siblings, 0 replies; 10+ messages in thread From: Fan, Jeff @ 2016-11-15 13:35 UTC (permalink / raw) To: Gao, Liming, Laszlo Ersek, edk2-devel@ml01.01.org Cc: Paolo Bonzini, Yao, Jiewen, Tian, Feng, Kinney, Michael D 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-devel@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 <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org> Cc: Paolo Bonzini <pbonzini@redhat.com<mailto:pbonzini@redhat.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@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:lersek@redhat.com] Sent: Tuesday, November 15, 2016 10:27 AM To: Fan, Jeff; edk2-devel@ml01.01.org<mailto:edk2-devel@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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish 2016-11-15 3:02 ` Fan, Jeff 2016-11-15 10:49 ` Gao, Liming @ 2016-11-15 15:24 ` Laszlo Ersek 1 sibling, 0 replies; 10+ messages in thread From: Laszlo Ersek @ 2016-11-15 15:24 UTC (permalink / raw) To: Fan, Jeff, edk2-devel@ml01.01.org Cc: Paolo Bonzini, Yao, Jiewen, Tian, Feng, Kinney, Michael D, Gao, Liming 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:lersek@redhat.com] > Sent: Tuesday, November 15, 2016 10:27 AM > To: Fan, Jeff; edk2-devel@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 <pbonzini@redhat.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Feng Tian <feng.tian@intel.com> >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jeff Fan <jeff.fan@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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-11-15 15:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-15 2:23 [PATCH] UefiCpuPkg/DxeMpLib: Remove sizeof() that is not necessary Jeff Fan 2016-11-15 2:23 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish Jeff Fan 2016-11-15 2:23 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Free SmramRanges to save SMM space Jeff Fan 2016-11-15 7:33 ` Zeng, Star -- strict thread matches above, loose matches on Subject: below -- 2016-11-15 2:18 [PATCH] UefiCpuPkg/DxeMpLib: Remove sizeof() that is not necessary Jeff Fan 2016-11-15 2:18 ` [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish Jeff Fan 2016-11-15 2:27 ` Laszlo Ersek 2016-11-15 3:02 ` Fan, Jeff 2016-11-15 10:49 ` Gao, Liming 2016-11-15 13:35 ` Fan, Jeff 2016-11-15 15:24 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox