From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 68F6481E57 for ; Tue, 15 Nov 2016 07:24:25 -0800 (PST) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BC35CA738A; Tue, 15 Nov 2016 15:24:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-97.phx2.redhat.com [10.3.116.97]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAFFORK0025395; Tue, 15 Nov 2016 10:24:27 -0500 To: "Fan, Jeff" , "edk2-devel@ml01.01.org" References: <20161115021839.3984-1-jeff.fan@intel.com> <20161115021839.3984-2-jeff.fan@intel.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2DD5E1@shsmsx102.ccr.corp.intel.com> Cc: Paolo Bonzini , "Yao, Jiewen" , "Tian, Feng" , "Kinney, Michael D" , "Gao, Liming" From: Laszlo Ersek Message-ID: <8417e0cd-f28d-bb85-33a7-9e3da5d7df37@redhat.com> Date: Tue, 15 Nov 2016 16:24:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <542CF652F8836A4AB8DBFAAD40ED192A4A2DD5E1@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 15 Nov 2016 15:24:29 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Nov 2016 15:24:25 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 >> 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 >