From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 B7C0781D97 for ; Wed, 30 Nov 2016 18:53:53 -0800 (PST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga104.fm.intel.com with ESMTP; 30 Nov 2016 18:53:53 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,723,1473145200"; d="scan'208";a="1092931079" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 30 Nov 2016 18:53:53 -0800 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 30 Nov 2016 18:53:53 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 30 Nov 2016 18:53:52 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.239]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.96]) with mapi id 14.03.0248.002; Thu, 1 Dec 2016 10:53:51 +0800 From: "Tian, Feng" To: Laszlo Ersek , "Fan, Jeff" , "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: AQHSShV/5etlnCKWHEOoMRg7NdJHR6Dv6bkAgABBjACAAIdFgIABtYiA Date: Thu, 1 Dec 2016 02:53:49 +0000 Message-ID: <7F1BAD85ADEA444D97065A60D2E97EE566E56AC6@SHSMSX101.ccr.corp.intel.com> References: <20161129075130.15192-1-jeff.fan@intel.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2EB3A8@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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: Thu, 01 Dec 2016 02:53:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Feng Tian Thanks Feng -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 Sent: Wednesday, November 30, 2016 4:48 PM To: Fan, Jeff ; edk2-devel@ml01.01.org Cc: Kinney, Michael D ; Tian, Feng ; Yao, Jiewen Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphore= s on S3 boot path On 11/30/16 01:43, Fan, Jeff wrote: > Laszlo, >=20 > Thanks your comments. I added my comments as below in [Jeff] Thanks for your answers. Acked-by: Laszlo Ersek Tested-by: Laszlo Ersek Cheers Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > 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=20 > semaphores on S3 boot path >=20 > 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. >> >> We have one related fix at e78a2a49ee6b0c0d7c6997c87ace31d7761cf636.=20 >> But that is not completed. >> >> This fix is to clear Busy/Run/Present semaphores in InitializeMpSyncData= (). >> >> 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(+) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> 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 + mSemapho= reSize * 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 S= emaphoreGlobal are not cleared: PFLock, CodeAccessCheckLock, MemoryMappedLo= ck. Is that okay? >=20 > The values pointed-to by the following fields of SemaphoreMsr are not cle= ared either: Msr, AvailableCounter. Is that okay? >=20 > [Jeff] We need to clear the data in SMM_CPU_DATA_BLOCK/SMM_DISPATCHER_MP_= SYNC_DATA and semaphores pointed by the field in those 2 structures. Howev= er, the other spinlock located in SemaphoreBlock needn't to be cleared. >=20 > Can we imitate e78a2a49ee6b0c0d7c6997c87ace31d7761cf636 here; namely,=20 > can we save "SemaphoreBlock" and "TotalSize" from > InitializeSmmCpuSemaphores() in global variables (in SMRAM), and then jus= t do another ZeroMem() here? That would cover the currently listed objects = (*Counter, *InsideSmm, *AllCpusInSync), and everything else too, in a futur= e-proof way. >=20 > [Jeff] This issue is that ZeroMem only clear all the fields in structure = and needn't o clear the buffer pointed by these fields. >=20 > In fact, I wonder if the ZeroMem() could be moved into > InitializeMpSyncData() from InitializeSmmCpuSemaphores(). >=20 > [Jeff] If we cleared all semaphores(including Spinlock), we need to re-i= nitialize them again. I do not think there is some reasonable usage case to= let spinlock keep the garbage value. >=20 > Of course, if some pointed-to objects must not be cleared, then the > ZeroMem() is not appropriate. >=20 > [Jeff] Agree. >=20 > Thanks! > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >=20