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 C3CEF81F1B for ; Thu, 17 Nov 2016 02:21:30 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 7B00831B320; Thu, 17 Nov 2016 10:21:35 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-6.phx2.redhat.com [10.3.116.6]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAHALXLk011552; Thu, 17 Nov 2016 05:21:34 -0500 To: Michael Kinney , edk2-devel@ml01.01.org References: <1479360884-16708-1-git-send-email-michael.d.kinney@intel.com> Cc: Andrew Fish , Liming Gao , Jeff Fan From: Laszlo Ersek Message-ID: <7c720463-787b-e8ea-8809-7a0f18da2141@redhat.com> Date: Thu, 17 Nov 2016 11:21:32 +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: <1479360884-16708-1-git-send-email-michael.d.kinney@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 17 Nov 2016 10:21:35 +0000 (UTC) Subject: Re: [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to 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: Thu, 17 Nov 2016 10:21:30 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/17/16 06:34, Michael Kinney wrote: > Add volatile qualifier to mNumberToFinish to prevent compiler > optimization. Also update TransferApToSafeState() to pass in > UINTN values and treat the mNumberToFinish as an address value > that is passed to the assembly code. Is it possible to split these actions into two patches? One for mNumberToFinish, the other patch for UINTN values. > Cc: Liming Gao > Cc: Laszlo Ersek > Cc: Andrew Fish > Cc: Jeff Fan > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Michael Kinney > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 10 +++++----- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 18 +++++++++--------- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 12 ++++++------ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 18 +++++++++--------- > 4 files changed, 29 insertions(+), 29 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index 3fb6864..4531298 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; > @@ -385,7 +385,7 @@ MPRendezvousProcedure ( > CPU_REGISTER_TABLE *RegisterTableList; > UINT32 InitApicId; > UINTN Index; > - UINT32 TopOfStack; > + UINTN TopOfStack; > UINT8 Stack[128]; > > ProgramVirtualWireMode (); > @@ -403,10 +403,10 @@ MPRendezvousProcedure ( > // > // Place AP into the safe code, count down the number with lock mechanism in the safe code. > // > - TopOfStack = (UINT32) (UINTN) Stack + sizeof (Stack); > - TopOfStack &= ~(UINT32) (CPU_STACK_ALIGNMENT - 1); > + TopOfStack = (UINTN) Stack + sizeof (Stack); This change looks good. > + TopOfStack &= ~(CPU_STACK_ALIGNMENT - 1); Please don't drop the UINT32 cast from before the bit-neg altogether, instead please turn it into a UINTN cast. The reason is that CPU_STACK_ALIGNMENT has type "int", so the bit-neg flips the sign bit too. The resultant value is a negative integer. Due to the two's complement representation, the negative value is actually correct, and when it is converted to UINTN, for the sake of the "&" operation that's inherent in "&=", the behavior is correct. But this silently relies on two's complement, which in my personal opinion is a bad thing. I consider the changing of sign bits with direct bit operations a sin :) -- unless mangling the sign bit is our express purpose -- so I request that we please convert the int value first to the unsigned type with correct width (here, UINTN), and then negate the bits. > CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, sizeof (mApHltLoopCodeTemplate)); > - TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, &mNumberToFinish); > + TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID *)&mNumberToFinish); > } > > /** The (VOID*) cast is unnecessary here (in the last arg), any pointer can be cast to UINTN directly. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > index 9760373..d57eb33 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > @@ -98,23 +98,23 @@ InitGdt ( > /** > Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch. > > - @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function. > - @param[in] TopOfStack A pointer to the new stack to use for the ApHltLoopCode. > - @param[in] NumberToFinish Semaphore of APs finish count. > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function. Should we replace "32-bit" with "natural width"? > + @param[in] TopOfStack A pointer to the new stack to use for the ApHltLoopCode. > + @param[in] NumberToFinishAddress Address of Semaphore of APs finish count. > > **/ > VOID > TransferApToSafeState ( > - IN UINT32 ApHltLoopCode, > - IN UINT32 TopOfStack, > - IN UINT32 *NumberToFinish > + IN UINTN ApHltLoopCode, > + IN UINTN TopOfStack, > + IN UINTN NumberToFinishAddress > ) > { > SwitchStack ( > - (SWITCH_STACK_ENTRY_POINT) (UINTN) ApHltLoopCode, > - NumberToFinish, > + (SWITCH_STACK_ENTRY_POINT)ApHltLoopCode, > + (VOID *)NumberToFinishAddress, > NULL, > - (VOID *) (UINTN) TopOfStack > + (VOID *)TopOfStack > ); > // > // It should never reach here Looks okay. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 88d9c85..38dd9fa 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -828,16 +828,16 @@ GetAcpiS3EnableFlag ( > /** > Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch. > > - @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function. > - @param[in] TopOfStack A pointer to the new stack to use for the ApHltLoopCode. > - @param[in] NumberToFinish Semaphore of APs finish count. > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function. If we update the comment, we should do it here too. > + @param[in] TopOfStack A pointer to the new stack to use for the ApHltLoopCode. > + @param[in] NumberToFinishAddress Address of Semaphore of APs finish count. > > **/ > VOID > TransferApToSafeState ( > - IN UINT32 ApHltLoopCode, > - IN UINT32 TopOfStack, > - IN UINT32 *NumberToFinish > + IN UINTN ApHltLoopCode, > + IN UINTN TopOfStack, > + IN UINTN NumberToFinishAddress > ); > > #endif > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > index 6844c3f..d45fed2 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -103,24 +103,24 @@ GetProtectedModeCS ( > /** > Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch. > > - @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function. and here > - @param[in] TopOfStack A pointer to the new stack to use for the ApHltLoopCode. > - @param[in] NumberToFinish Semaphore of APs finish count. > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function. > + @param[in] TopOfStack A pointer to the new stack to use for the ApHltLoopCode. > + @param[in] NumberToFinishAddress Address of Semaphore of APs finish count. > > **/ > VOID > TransferApToSafeState ( > - IN UINT32 ApHltLoopCode, > - IN UINT32 TopOfStack, > - IN UINT32 *NumberToFinish > + IN UINTN ApHltLoopCode, > + IN UINTN TopOfStack, > + IN UINTN NumberToFinishAddress > ) > { > AsmDisablePaging64 ( > GetProtectedModeCS (), > - (UINT32) (UINTN) ApHltLoopCode, > - (UINT32) (UINTN) NumberToFinish, > + (UINT32)ApHltLoopCode, > + (UINT32)NumberToFinishAddress, > 0, > - TopOfStack > + (UINT32)TopOfStack > ); > // > // It should never reach here > Looks good to me. Thanks Laszlo