From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 9B97E81DA1 for ; Thu, 17 Nov 2016 12:59:40 -0800 (PST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP; 17 Nov 2016 12:59:45 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,655,1473145200"; d="scan'208";a="787732675" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by FMSMGA003.fm.intel.com with ESMTP; 17 Nov 2016 12:59:45 -0800 Received: from orsmsx112.amr.corp.intel.com (10.22.240.13) by ORSMSX109.amr.corp.intel.com (10.22.240.7) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 17 Nov 2016 12:59:45 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.63]) by ORSMSX112.amr.corp.intel.com ([169.254.3.163]) with mapi id 14.03.0248.002; Thu, 17 Nov 2016 12:59:44 -0800 From: "Kinney, Michael D" To: Laszlo Ersek , "edk2-devel@ml01.01.org" , "Kinney, Michael D" CC: Andrew Fish , "Gao, Liming" , "Fan, Jeff" Thread-Topic: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to mNumberToFinish Thread-Index: AQHSQJRuq97HVCiGLEqUp9AlCv3y8aDdfa8A///ZBKCAAFLCwA== Date: Thu, 17 Nov 2016 20:59:44 +0000 Message-ID: References: <1479360884-16708-1-git-send-email-michael.d.kinney@intel.com> <7c720463-787b-e8ea-8809-7a0f18da2141@redhat.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMWE2NDVkZjktNWJmMS00NjU4LTljMjUtYmJmYTViOWFiYTU3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlplY0F2bkY4NERGcFdWNjdFVkhCRzBReCsrVHJYellRcERkRkVkOTRsdzQ9In0= x-originating-ip: [10.22.254.140] MIME-Version: 1.0 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 20:59:40 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, I redid some tests using the following as you suggested, and it works. TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)&mNumber= ToFinish); So I will use this style in V2. Mike > -----Original Message----- > From: Kinney, Michael D > Sent: Thursday, November 17, 2016 8:06 AM > To: Laszlo Ersek ; edk2-devel@ml01.01.org; Kinney, Mic= hael D > > Cc: Andrew Fish ; Gao, Liming ; Fa= n, Jeff > > Subject: RE: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to > mNumberToFinish >=20 > Laszlo, >=20 > Yes. I can split. I will send V2 with your feedback. >=20 > For the following line, the cast to (VOID *) before cast to (UINTN) was > required to pass VS build. Since mNumberToFinish is a volatile pointer, > it needs to be cast to a non-volatile pointer before the pointer can be > converted to an unsigned integer value. >=20 > TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID > *)&mNumberToFinish); >=20 > Mike >=20 >=20 > > -----Original Message----- > > From: Laszlo Ersek [mailto:lersek@redhat.com] > > Sent: Thursday, November 17, 2016 2:22 AM > > To: Kinney, Michael D ; edk2-devel@ml01.01.= org > > Cc: Andrew Fish ; Gao, Liming ; = Fan, Jeff > > > > Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to > > mNumberToFinish > > > > 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 =3D FALSE; > > > VOID *mGdtForAp =3D 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 me= chanism in > the > > safe code. > > > // > > > - TopOfStack =3D (UINT32) (UINTN) Stack + sizeof (Stack); > > > - TopOfStack &=3D ~(UINT32) (CPU_STACK_ALIGNMENT - 1); > > > + TopOfStack =3D (UINTN) Stack + sizeof (Stack); > > > > This change looks good. > > > > > + TopOfStack &=3D ~(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 th= e > > "&" operation that's inherent in "&=3D", the behavior is correct. But t= his > > 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 feature= s on S3 > patch. > > > > > > - @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loo= p 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 h= lt-loop > > function. > > > > Should we replace "32-bit" with "natural width"? > > > > > + @param[in] TopOfStack A pointer to the new stack to us= e for the > > ApHltLoopCode. > > > + @param[in] NumberToFinishAddress Address of Semaphore of APs fini= sh 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 feature= s on S3 > patch. > > > > > > - @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loo= p 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 h= lt-loop > > function. > > > > If we update the comment, we should do it here too. > > > > > + @param[in] TopOfStack A pointer to the new stack to us= e for the > > ApHltLoopCode. > > > + @param[in] NumberToFinishAddress Address of Semaphore of APs fini= sh 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 feature= s on S3 > patch. > > > > > > - @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loo= p 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 h= lt-loop > > function. > > > + @param[in] TopOfStack A pointer to the new stack to us= e for the > > ApHltLoopCode. > > > + @param[in] NumberToFinishAddress Address of Semaphore of APs fini= sh 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