From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 CB61C81EEA for ; Thu, 17 Nov 2016 08:07:05 -0800 (PST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 17 Nov 2016 08:06:33 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,506,1473145200"; d="scan'208";a="1060725618" Received: from orsmsx108.amr.corp.intel.com ([10.22.240.6]) by orsmga001.jf.intel.com with ESMTP; 17 Nov 2016 08:06:10 -0800 Received: from orsmsx115.amr.corp.intel.com (10.22.240.11) by ORSMSX108.amr.corp.intel.com (10.22.240.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 17 Nov 2016 08:06:09 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.63]) by ORSMSX115.amr.corp.intel.com ([10.22.240.11]) with mapi id 14.03.0248.002; Thu, 17 Nov 2016 08:06:09 -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///ZBKA= Date: Thu, 17 Nov 2016 16:06:08 +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: <7c720463-787b-e8ea-8809-7a0f18da2141@redhat.com> 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 16:07:06 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Yes. I can split. I will send V2 with your feedback. 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=20 converted to an unsigned integer value. TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VOID *)&m= NumberToFinish); Mike > -----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.or= g > Cc: Andrew Fish ; Gao, Liming ; Fa= n, Jeff > > Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile to > mNumberToFinish >=20 > 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. >=20 > Is it possible to split these actions into two patches? One for > mNumberToFinish, the other patch for UINTN values. >=20 > > 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/PiSmmCpuDxe= Smm/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 mech= anism in the > safe code. > > // > > - TopOfStack =3D (UINT32) (UINTN) Stack + sizeof (Stack); > > - TopOfStack &=3D ~(UINT32) (CPU_STACK_ALIGNMENT - 1); > > + TopOfStack =3D (UINTN) Stack + sizeof (Stack); >=20 > This change looks good. >=20 > > + TopOfStack &=3D ~(CPU_STACK_ALIGNMENT - 1); >=20 > Please don't drop the UINT32 cast from before the bit-neg altogether, > instead please turn it into a UINTN cast. >=20 > 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. >=20 > 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 "&=3D", the behavior is correct. But thi= s > 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. >=20 > > CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, si= zeof > (mApHltLoopCodeTemplate)); > > - TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, > &mNumberToFinish); > > + TransferApToSafeState ((UINTN)mApHltLoopCode, TopOfStack, (UINTN)(VO= ID > *)&mNumberToFinish); > > } > > > > /** >=20 > The (VOID*) cast is unnecessary here (in the last arg), any pointer can > be cast to UINTN directly. >=20 > > 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 th= e > ApHltLoopCode. > > - @param[in] NumberToFinish Semaphore of APs finish count. > > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt= -loop > function. >=20 > Should we replace "32-bit" with "natural width"? >=20 > > + @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 >=20 > Looks okay. >=20 > > 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 th= e > ApHltLoopCode. > > - @param[in] NumberToFinish Semaphore of APs finish count. > > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt= -loop > function. >=20 > If we update the comment, we should do it here too. >=20 > > + @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. >=20 > and here >=20 > > - @param[in] TopOfStack A pointer to the new stack to use for th= e > 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 > > >=20 > Looks good to me. >=20 > Thanks > Laszlo