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 F39FF81E63 for ; Mon, 14 Nov 2016 18:27:26 -0800 (PST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (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 4477FC001C0A; Tue, 15 Nov 2016 02:27:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-50.phx2.redhat.com [10.3.116.50]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAF2RTiQ025523; Mon, 14 Nov 2016 21:27:29 -0500 To: Jeff Fan , edk2-devel@ml01.01.org References: <20161115021839.3984-1-jeff.fan@intel.com> <20161115021839.3984-2-jeff.fan@intel.com> Cc: Paolo Bonzini , Jiewen Yao , Feng Tian , Michael D Kinney From: Laszlo Ersek Message-ID: Date: Tue, 15 Nov 2016 03:27:28 +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: <20161115021839.3984-2-jeff.fan@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 15 Nov 2016 02:27:31 +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 02:27:27 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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