[PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired

[PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired

[Jeff Fan]

SMM BSP's *busy* state should be acquired. We could use AcquireSpinLock()
instead of AcquireSpinLockOrFail().

Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index a1d16b4..e03f1e0 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -407,7 +407,7 @@ BSPHandler (
   //
   // The BUSY lock is initialized to Acquired state
   //
-  AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy);
+  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
 
   //
   // Perform the pre tasks
-- 
2.9.3.windows.2

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired

[Wu, Hao A]

Reviewed-by: Hao Wu <hao.a.wu@intel.com>


Best Regards,
Hao Wu


> -----Original Message-----
> From: Fan, Jeff
> Sent: Tuesday, April 18, 2017 10:16 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Tian, Feng; Kinney, Michael D
> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired
> 
> SMM BSP's *busy* state should be acquired. We could use AcquireSpinLock()
> instead of AcquireSpinLockOrFail().
> 
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index a1d16b4..e03f1e0 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -407,7 +407,7 @@ BSPHandler (
>    //
>    // The BUSY lock is initialized to Acquired state
>    //
> -  AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> +  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> 
>    //
>    // Perform the pre tasks
> --
> 2.9.3.windows.2

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired

[Laszlo Ersek]

Hi Jeff,

On 04/18/17 04:16, Jeff Fan wrote:
> SMM BSP's *busy* state should be acquired. We could use AcquireSpinLock()
> instead of AcquireSpinLockOrFail().
> 
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index a1d16b4..e03f1e0 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -407,7 +407,7 @@ BSPHandler (
>    //
>    // The BUSY lock is initialized to Acquired state
>    //
> -  AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> +  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>  
>    //
>    // Perform the pre tasks
> 

what symptoms did you experience without the fix?

Thanks
Laszlo

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired

[Fan, Jeff]

Laszlo,

There is no any real issue we encountered.

Some static code check tool reported AcquireSpinLockOrFai() return value was not been checked.
Then I found we may ignore some issue if AcquireSpinLockOrFai() return FALSE (even it will not be happened).

Using AcquireSpinLock() is due to the following code are using AcquireSpinLock() to check AP's BUSY state also.

Thanks!
Jeff

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Monday, April 24, 2017 7:41 PM
To: Fan, Jeff; edk2-devel@lists.01.org
Cc: Wu, Hao A; Kinney, Michael D; Tian, Feng
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired

Hi Jeff,

On 04/18/17 04:16, Jeff Fan wrote:
> SMM BSP's *busy* state should be acquired. We could use 
> AcquireSpinLock() instead of AcquireSpinLockOrFail().
> 
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index a1d16b4..e03f1e0 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -407,7 +407,7 @@ BSPHandler (
>    //
>    // The BUSY lock is initialized to Acquired state
>    //
> -  AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy);
> +  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>  
>    //
>    // Perform the pre tasks
> 

what symptoms did you experience without the fix?

Thanks
Laszlo

Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired

[Laszlo Ersek]

On 04/25/17 02:54, Fan, Jeff wrote:
> Laszlo,
> 
> There is no any real issue we encountered.
> 
> Some static code check tool reported AcquireSpinLockOrFai() return value was not been checked.
> Then I found we may ignore some issue if AcquireSpinLockOrFai() return FALSE (even it will not be happened).
> 
> Using AcquireSpinLock() is due to the following code are using AcquireSpinLock() to check AP's BUSY state also.

Thanks for the explanation!
Laszlo

> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Monday, April 24, 2017 7:41 PM
> To: Fan, Jeff; edk2-devel@lists.01.org
> Cc: Wu, Hao A; Kinney, Michael D; Tian, Feng
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Lock should be acquired
> 
> Hi Jeff,
> 
> On 04/18/17 04:16, Jeff Fan wrote:
>> SMM BSP's *busy* state should be acquired. We could use 
>> AcquireSpinLock() instead of AcquireSpinLockOrFail().
>>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Cc: Feng Tian <feng.tian@intel.com>
>> Cc: Michael 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 | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> index a1d16b4..e03f1e0 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
>> @@ -407,7 +407,7 @@ BSPHandler (
>>    //
>>    // The BUSY lock is initialized to Acquired state
>>    //
>> -  AcquireSpinLockOrFail (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>> +  AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy);
>>  
>>    //
>>    // Perform the pre tasks
>>
> 
> what symptoms did you experience without the fix?
> 
> Thanks
> Laszlo
>