Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish

[Laszlo Ersek <lersek@redhat.com>]

On 11/15/16 04:02, Fan, Jeff wrote:
> Laszlo,
> 
> I agree to separate another patch to change the prototype of TransferApToSafeState() to reduce one cast operation. I will create v2 for it.
> 
> I also agree updating prototype of InterlockedDecrement() is compatible updating. But there are other 5 APIs as blow:
> InterlockedIncrement()
> InterlockedCompareExchange16()
> InterlockedCompareExchange32()
> InterlockedCompareExchange64()
> InterlockedCompareExchangePointer()
> 
> To be consistence, we may need to update them together. 

I agree. I didn't know the full list of Interlocked*() APIs off-hand,
but I expected there would be several such functions, and for
consistency they should indeed be updated together. I'm not sure if
everyone agrees with this, hence discussion is welcome.

> 
> Liming & MIke, any comments on this updating.

Yes, please comment!

Thanks
Laszlo

> 
> Thanks!
> Jeff
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Tuesday, November 15, 2016 10:27 AM
> To: Fan, Jeff; edk2-devel@ml01.01.org
> Cc: Paolo Bonzini; Yao, Jiewen; Tian, Feng; Kinney, Michael D
> Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Add volatile for mNumberToFinish
> 
> On 11/15/16 03:18, Jeff Fan wrote:
>> The GCC 5.4 will optimize mNumberToFinish in EarlyInitializeCpu(). It 
>> will cause
>> S3 resume failure.
>>
>> Adding *volatile* could make sure compiler does not so such optimization.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Feng Tian <feng.tian@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/CpuS3.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> index 3fb6864..f13ff3e 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>> @@ -55,7 +55,7 @@ AsmGetAddressMap (
>>  #define LEGACY_REGION_BASE    (0xA0000 - LEGACY_REGION_SIZE)
>>  
>>  ACPI_CPU_DATA                mAcpiCpuData;
>> -UINT32                       mNumberToFinish;
>> +volatile UINT32              mNumberToFinish;
>>  MP_CPU_EXCHANGE_INFO         *mExchangeInfo;
>>  BOOLEAN                      mRestoreSmmConfigurationInS3 = FALSE;
>>  VOID                         *mGdtForAp = NULL;
>> @@ -371,7 +371,7 @@ EarlyMPRendezvousProcedure (
>>    //
>>    // Count down the number with lock mechanism.
>>    //
>> -  InterlockedDecrement (&mNumberToFinish);
>> +  InterlockedDecrement ((UINT32 *) &mNumberToFinish);
>>  }
>>  
>>  /**
>> @@ -406,7 +406,7 @@ MPRendezvousProcedure (
>>    TopOfStack  = (UINT32) (UINTN) Stack + sizeof (Stack);
>>    TopOfStack &= ~(UINT32) (CPU_STACK_ALIGNMENT - 1);
>>    CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, 
>> sizeof (mApHltLoopCodeTemplate));
>> -  TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, 
>> &mNumberToFinish);
>> +  TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, 
>> + (UINT32 *) &mNumberToFinish);
>>  }
>>  
>>  /**
>>
> 
> I think I understand the idea, but the current solution requires you to cast away "volatile" in two places. That's not nice, normally it is undefined behavior.
> 
> I recommend to extend this patch, with more patches: please change the prototype of TransferApToSafeState() so that it takes a pointer-to-volatile.
> 
> I also suggest to change the prototype of InterlockedDecrement(). (You won't have to update all other call sites: it is fine to take/access a non-volatile object as a volatile, but not the other way around.)
> 
> I agree this increases the scope of the patch quite a bit, so maybe others should chime in as well.
> 
> Thanks!
> Laszlo
>