From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 C92A481EBB for ; Tue, 29 Nov 2016 16:43:36 -0800 (PST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga102.fm.intel.com with ESMTP; 29 Nov 2016 16:43:36 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,719,1473145200"; d="scan'208";a="37009378" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga005.jf.intel.com with ESMTP; 29 Nov 2016 16:43:36 -0800 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 29 Nov 2016 16:43:35 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 29 Nov 2016 16:43:35 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.142]) with mapi id 14.03.0248.002; Wed, 30 Nov 2016 08:43:33 +0800 From: "Fan, Jeff" To: Laszlo Ersek , "edk2-devel@ml01.01.org" CC: "Kinney, Michael D" , "Yao, Jiewen" , "Tian, Feng" Thread-Topic: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path Thread-Index: AQHSShWGaAVhz7MZvUe7URsnzpZzy6Dv6bkAgADBH3A= Date: Wed, 30 Nov 2016 00:43:32 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2EB3A8@shsmsx102.ccr.corp.intel.com> References: <20161129075130.15192-1-jeff.fan@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjAzOTdhZGEtMmY4OC00MDE2LWFhNGEtMTdiNjRkMzUwMjk4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ino5SUtnTmJXOVpPZ1wvV2NFenJ0QWRcL2dWQUY4VXpSNm9Xa2o4elg3Rjcydz0ifQ== x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path 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: Wed, 30 Nov 2016 00:43:37 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Thanks your comments. I added my comments as below in [Jeff] Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 Sent: Wednesday, November 30, 2016 4:49 AM To: Fan, Jeff; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Yao, Jiewen; Tian, Feng Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphore= s on S3 boot path On 11/29/16 08:51, Jeff Fan wrote: > Some semaphores are not cleared on S3 boot path. For example, > mSmmMpSyncData->CpuData[CpuIndex].Present. It may still keeps the=20 > mSmmMpSyncData->value set at > SMM runtime during S3 resume. It may causes BSP have the wrong=20 > judgement on SMM AP's present state. >=20 > We have one related fix at e78a2a49ee6b0c0d7c6997c87ace31d7761cf636.=20 > But that is not completed. >=20 > This fix is to clear Busy/Run/Present semaphores in InitializeMpSyncData(= ). >=20 > Cc: Laszlo Ersek > Cc: Feng Tian > Cc: Jiewen Yao > Cc: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 3 +++ > 1 file changed, 3 insertions(+) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c=20 > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index cfbf59e..a873b68 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1357,6 +1357,9 @@ InitializeMpSyncData ( > (UINT32 *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Run + mSemaphor= eSize * CpuIndex); > mSmmMpSyncData->CpuData[CpuIndex].Present =3D > (BOOLEAN *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Present +=20 > mSemaphoreSize * CpuIndex); > + *(mSmmMpSyncData->CpuData[CpuIndex].Busy) =3D 0; > + *(mSmmMpSyncData->CpuData[CpuIndex].Run) =3D 0; > + *(mSmmMpSyncData->CpuData[CpuIndex].Present) =3D FALSE; > } > } > } >=20 Even after this patch, the values pointed-to by the following fields of Sem= aphoreGlobal are not cleared: PFLock, CodeAccessCheckLock, MemoryMappedLock= . Is that okay? The values pointed-to by the following fields of SemaphoreMsr are not clear= ed either: Msr, AvailableCounter. Is that okay? [Jeff] We need to clear the data in SMM_CPU_DATA_BLOCK/SMM_DISPATCHER_MP_SY= NC_DATA and semaphores pointed by the field in those 2 structures. However= , the other spinlock located in SemaphoreBlock needn't to be cleared. Can we imitate e78a2a49ee6b0c0d7c6997c87ace31d7761cf636 here; namely, can w= e save "SemaphoreBlock" and "TotalSize" from InitializeSmmCpuSemaphores() in global variables (in SMRAM), and then just = do another ZeroMem() here? That would cover the currently listed objects (*= Counter, *InsideSmm, *AllCpusInSync), and everything else too, in a future-= proof way. [Jeff] This issue is that ZeroMem only clear all the fields in structure an= d needn't o clear the buffer pointed by these fields. In fact, I wonder if the ZeroMem() could be moved into InitializeMpSyncData() from InitializeSmmCpuSemaphores(). [Jeff] If we cleared all semaphores(including Spinlock), we need to re-ini= tialize them again. I do not think there is some reasonable usage case to l= et spinlock keep the garbage value. Of course, if some pointed-to objects must not be cleared, then the ZeroMem() is not appropriate. [Jeff] Agree. Thanks! Laszlo