Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

["Paolo Bonzini" <pbonzini@redhat.com>]

On 23/02/21 18:06, Laszlo Ersek wrote:
> On 02/23/21 08:45, Paolo Bonzini wrote:
>> On 22/02/21 15:53, Laszlo Ersek wrote:
>>>> +
>>>> +  if (mCpuHotEjectData != NULL) {
>>>> +    CPU_HOT_EJECT_HANDLER Handler;
>>>> +
>>>> +    Handler = mCpuHotEjectData->Handler;
>>> This patch looks otherwise OK to me, but:
>>>
>>> In patch v8 08/10, we have a ReleaseMemoryFence(). (For now, it is only
>>> expressed as a MemoryFence() call; we'll make that more precise later.)
>>>
>>> (1) I think that should be paired with an AcquireMemoryFence() call,
>>> just before loading "mCpuHotEjectData->Handler" above -- for now, also
>>> expressed as a MemoryFence() call only.
>>
>> In Linux terms, there is a control dependency here.  However, it should
>> at least be a separate statement to load mCpuHotEjectData (which from my
>> EDK2 reminiscences should be a global) into a local variable.  So
>>
>>    EjectData = mCPUHotEjectData;
>>    // Optional AcquireMemoryFence here
>>    if (EjectData != NULL) {
>>      CPU_HOT_EJECT_HANDLER Handler;
>>
>>      Handler = EjectData->Handler;
>>      if (Handler != NULL) {
>>        Handler (CpuIndex);
>>      }
>>    }
> 
> Yes, "mCPUHotEjectData" is a global.
> 
> "mCpuHotEjectData" itself is set up on the BSP (from the entry point
> function of the PiSmmCpuSmmDxe driver), before any other APs have a
> chance to execute any SMM-related code at all. Furthermore, once set up,
> mCpuHotEjectData never changes -- it remains set to a particular
> non-NULL value forever, or it remains NULL forever. (The latter case
> applies when the possible CPU count is 1; IOW, then there is no AP at all.)

Ok, that's what I was missing.  However, your code below has *two* loads 
of mCpuHotEjectData and the fence would have to go after the second 
(between the load of mCpuHotEjectData and the load of the Handler 
field).  Therefore I would still use a local variable even if you decide 
to put the fence inside the "if", which I agree is the clearest.

Paolo

> Because of that, I thought that the first comparison (mCpuHotEjectData
> != NULL) would not need any fence -- I thought it was similar to a
> userspace program that (a) set a global variable in the "main" thread,
> before calling pthread_create(), (b) treated the global variable as a
> constant, ever after (meaning all threads).
> 
> However, mCpuHotEjectData->Handler is changed regularly (modified by the
> BSP, and read "later" by all processors). That's why I thought the
> acquire fence was needed in the following location:
> 
>    if (mCpuHotEjectData != NULL) {
>      CPU_HOT_EJECT_HANDLER Handler;
> 
>      //
>      // HERE -- AcquireMemoryFence()
>      //
>      Handler = mCpuHotEjectData->Handler;
>      if (Handler != NULL) {
>        Handler (CpuIndex);
>      }
>    }
> 
> Thanks!
> Laszlo
>