[PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path

[PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path

[Jeff Fan]

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

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path

[Laszlo Ersek]

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

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path

[Fan, Jeff]

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

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path

[Laszlo Ersek]

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
> 

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear some semaphores on S3 boot path

[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
>