* [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path @ 2016-11-29 7:51 Jeff Fan 2016-11-29 20:48 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Jeff Fan @ 2016-11-29 7:51 UTC (permalink / raw) To: edk2-devel; +Cc: Laszlo Ersek, Feng Tian, Jiewen Yao, Michael D Kinney Some semaphores are not cleared on S3 boot path. For example, mSmmMpSyncData->CpuData[CpuIndex].Present. It may still keeps the value set at SMM runtime during S3 resume. It may causes BSP have the wrong judgement on SMM AP's present state. We have one related fix at e78a2a49ee6b0c0d7c6997c87ace31d7761cf636. But that is not completed. This fix is to clear Busy/Run/Present semaphores in InitializeMpSyncData(). Cc: Laszlo Ersek <lersek@redhat.com> Cc: Feng Tian <feng.tian@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Fan <jeff.fan@intel.com> --- 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 + mSemaphoreSize * CpuIndex); mSmmMpSyncData->CpuData[CpuIndex].Present = (BOOLEAN *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Present + mSemaphoreSize * CpuIndex); + *(mSmmMpSyncData->CpuData[CpuIndex].Busy) = 0; + *(mSmmMpSyncData->CpuData[CpuIndex].Run) = 0; + *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE; } } } -- 2.9.3.windows.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path 2016-11-29 7:51 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path Jeff Fan @ 2016-11-29 20:48 ` Laszlo Ersek 2016-11-30 0:43 ` Fan, Jeff 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2016-11-29 20:48 UTC (permalink / raw) To: Jeff Fan, edk2-devel; +Cc: Michael D Kinney, Jiewen Yao, Feng Tian 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 value set at > SMM runtime during S3 resume. It may causes BSP have the wrong judgement on SMM > AP's present state. > > We have one related fix at e78a2a49ee6b0c0d7c6997c87ace31d7761cf636. But that is > not completed. > > This fix is to clear Busy/Run/Present semaphores in InitializeMpSyncData(). > > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Feng Tian <feng.tian@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff.fan@intel.com> > --- > 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 + mSemaphoreSize * CpuIndex); > mSmmMpSyncData->CpuData[CpuIndex].Present = > (BOOLEAN *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Present + mSemaphoreSize * CpuIndex); > + *(mSmmMpSyncData->CpuData[CpuIndex].Busy) = 0; > + *(mSmmMpSyncData->CpuData[CpuIndex].Run) = 0; > + *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE; > } > } > } > Even after this patch, the values pointed-to by the following fields of SemaphoreGlobal are not cleared: PFLock, CodeAccessCheckLock, MemoryMappedLock. Is that okay? The values pointed-to by the following fields of SemaphoreMsr are not cleared either: Msr, AvailableCounter. Is that okay? Can we imitate e78a2a49ee6b0c0d7c6997c87ace31d7761cf636 here; namely, can we 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. In fact, I wonder if the ZeroMem() could be moved into InitializeMpSyncData() from InitializeSmmCpuSemaphores(). Of course, if some pointed-to objects must not be cleared, then the ZeroMem() is not appropriate. Thanks! Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path 2016-11-29 20:48 ` Laszlo Ersek @ 2016-11-30 0:43 ` Fan, Jeff 2016-11-30 8:47 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Fan, Jeff @ 2016-11-30 0:43 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@ml01.01.org Cc: Kinney, Michael D, Yao, Jiewen, Tian, Feng Laszlo, Thanks your comments. I added my comments as below in [Jeff] Thanks! Jeff -----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 semaphores 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 > mSmmMpSyncData->value set at > SMM runtime during S3 resume. It may causes BSP have the wrong > judgement on SMM AP's present state. > > We have one related fix at e78a2a49ee6b0c0d7c6997c87ace31d7761cf636. > But that is not completed. > > This fix is to clear Busy/Run/Present semaphores in InitializeMpSyncData(). > > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Feng Tian <feng.tian@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff.fan@intel.com> > --- > 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 + mSemaphoreSize * CpuIndex); > mSmmMpSyncData->CpuData[CpuIndex].Present = > (BOOLEAN *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Present + > mSemaphoreSize * CpuIndex); > + *(mSmmMpSyncData->CpuData[CpuIndex].Busy) = 0; > + *(mSmmMpSyncData->CpuData[CpuIndex].Run) = 0; > + *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE; > } > } > } > Even after this patch, the values pointed-to by the following fields of SemaphoreGlobal are not cleared: PFLock, CodeAccessCheckLock, MemoryMappedLock. Is that okay? The values pointed-to by the following fields of SemaphoreMsr are not cleared either: Msr, AvailableCounter. Is that okay? [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. However, the other spinlock located in SemaphoreBlock needn't to be cleared. Can we imitate e78a2a49ee6b0c0d7c6997c87ace31d7761cf636 here; namely, can we 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 and 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-initialize them again. I do not think there is some reasonable usage case to let 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path 2016-11-30 0:43 ` Fan, Jeff @ 2016-11-30 8:47 ` Laszlo Ersek 2016-12-01 2:53 ` Tian, Feng 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2016-11-30 8:47 UTC (permalink / raw) To: Fan, Jeff, edk2-devel@ml01.01.org Cc: Kinney, Michael D, Tian, Feng, Yao, Jiewen On 11/30/16 01:43, Fan, Jeff wrote: > Laszlo, > > Thanks your comments. I added my comments as below in [Jeff] Thanks for your answers. Acked-by: Laszlo Ersek <lersek@redhat.com> Tested-by: Laszlo Ersek <lersek@redhat.com> 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 semaphores 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 >> mSmmMpSyncData->value set at >> SMM runtime during S3 resume. It may causes BSP have the wrong >> judgement on SMM AP's present state. >> >> We have one related fix at e78a2a49ee6b0c0d7c6997c87ace31d7761cf636. >> But that is not completed. >> >> This fix is to clear Busy/Run/Present semaphores in InitializeMpSyncData(). >> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Feng Tian <feng.tian@intel.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jeff Fan <jeff.fan@intel.com> >> --- >> 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 + mSemaphoreSize * CpuIndex); >> mSmmMpSyncData->CpuData[CpuIndex].Present = >> (BOOLEAN *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Present + >> mSemaphoreSize * CpuIndex); >> + *(mSmmMpSyncData->CpuData[CpuIndex].Busy) = 0; >> + *(mSmmMpSyncData->CpuData[CpuIndex].Run) = 0; >> + *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE; >> } >> } >> } >> > > Even after this patch, the values pointed-to by the following fields of SemaphoreGlobal are not cleared: PFLock, CodeAccessCheckLock, MemoryMappedLock. Is that okay? > > The values pointed-to by the following fields of SemaphoreMsr are not cleared either: Msr, AvailableCounter. Is that okay? > > [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. However, the other spinlock located in SemaphoreBlock needn't to be cleared. > > Can we imitate e78a2a49ee6b0c0d7c6997c87ace31d7761cf636 here; namely, can we 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 and 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-initialize them again. I do not think there is some reasonable usage case to let 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 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path 2016-11-30 8:47 ` Laszlo Ersek @ 2016-12-01 2:53 ` Tian, Feng 0 siblings, 0 replies; 5+ messages in thread From: Tian, Feng @ 2016-12-01 2:53 UTC (permalink / raw) To: Laszlo Ersek, Fan, Jeff, edk2-devel@ml01.01.org Cc: Kinney, Michael D, Yao, Jiewen, Tian, Feng Reviewed-by: Feng Tian <feng.tian@intel.com> Thanks Feng -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Wednesday, November 30, 2016 4:48 PM To: Fan, Jeff <jeff.fan@intel.com>; edk2-devel@ml01.01.org Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path On 11/30/16 01:43, Fan, Jeff wrote: > Laszlo, > > Thanks your comments. I added my comments as below in [Jeff] Thanks for your answers. Acked-by: Laszlo Ersek <lersek@redhat.com> Tested-by: Laszlo Ersek <lersek@redhat.com> 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 > semaphores 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 >> mSmmMpSyncData->value set at >> SMM runtime during S3 resume. It may causes BSP have the wrong >> judgement on SMM AP's present state. >> >> We have one related fix at e78a2a49ee6b0c0d7c6997c87ace31d7761cf636. >> But that is not completed. >> >> This fix is to clear Busy/Run/Present semaphores in InitializeMpSyncData(). >> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Feng Tian <feng.tian@intel.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jeff Fan <jeff.fan@intel.com> >> --- >> 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 + mSemaphoreSize * CpuIndex); >> mSmmMpSyncData->CpuData[CpuIndex].Present = >> (BOOLEAN *)((UINTN)mSmmCpuSemaphores.SemaphoreCpu.Present + >> mSemaphoreSize * CpuIndex); >> + *(mSmmMpSyncData->CpuData[CpuIndex].Busy) = 0; >> + *(mSmmMpSyncData->CpuData[CpuIndex].Run) = 0; >> + *(mSmmMpSyncData->CpuData[CpuIndex].Present) = FALSE; >> } >> } >> } >> > > Even after this patch, the values pointed-to by the following fields of SemaphoreGlobal are not cleared: PFLock, CodeAccessCheckLock, MemoryMappedLock. Is that okay? > > The values pointed-to by the following fields of SemaphoreMsr are not cleared either: Msr, AvailableCounter. Is that okay? > > [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. However, the other spinlock located in SemaphoreBlock needn't to be cleared. > > Can we imitate e78a2a49ee6b0c0d7c6997c87ace31d7761cf636 here; namely, > can we 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 and 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-initialize them again. I do not think there is some reasonable usage case to let 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 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-01 2:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-29 7:51 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path Jeff Fan 2016-11-29 20:48 ` Laszlo Ersek 2016-11-30 0:43 ` Fan, Jeff 2016-11-30 8:47 ` Laszlo Ersek 2016-12-01 2:53 ` Tian, Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox