Re: [PATCH v6 7/9] OvmfPkg/CpuHotplugSmm: add CpuEject()

["Ankur Arora" <ankur.a.arora@oracle.com>]

On 2021-02-02 6:00 a.m., Laszlo Ersek wrote:
> On 02/01/21 21:12, Ankur Arora wrote:
>> On 2021-02-01 11:08 a.m., Laszlo Ersek wrote:
>>> apologies, I've got more comments here:
>>>
>>> On 01/29/21 01:59, Ankur Arora wrote:
>>>
>>>>    /**
>>>> +  CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit(),
>>>> +  on each CPU at exit from SMM.
>>>> +
>>>> +  If, the executing CPU is not being ejected, nothing to be done.
>>>> +  If, the executing CPU is being ejected, wait in a CpuDeadLoop()
>>>> +  until ejected.
>>>> +
>>>> +  @param[in] ProcessorNum      Index of executing CPU.
>>>> +
>>>> +**/
>>>> +VOID
>>>> +EFIAPI
>>>> +CpuEject (
>>>> +  IN UINTN ProcessorNum
>>>> +  )
>>>> +{
>>>> +  //
>>>> +  // APIC ID is UINT32, but mCpuHotEjectData->ApicIdMap[] is UINT64
>>>> +  // so use UINT64 throughout.
>>>> +  //
>>>> +  UINT64 ApicId;
>>>> +
>>>> +  ApicId = mCpuHotEjectData->ApicIdMap[ProcessorNum];
>>>> +  if (ApicId == CPU_EJECT_INVALID) {
>>>> +    return;
>>>> +  }
>>>> +
>>>> +  //
>>>> +  // CPU(s) being unplugged get here from
>>>> SmmCpuFeaturesSmiRendezvousExit()
>>>> +  // after having been cleared to exit the SMI by the monarch and
>>>> thus have
>>>> +  // no SMM processing remaining.
>>>> +  //
>>>> +  // Given that we cannot allow them to escape to the guest, we pen
>>>> them
>>>> +  // here until the SMM monarch tells the HW to unplug them.
>>>> +  //
>>>> +  CpuDeadLoop ();
>>>> +}
>>>
>>> (15) There is no such function as SmmCpuFeaturesSmiRendezvousExit() --
>>> it's SmmCpuFeaturesRendezvousExit().
>>>
>>> (16) This function uses a data structure for communication between BSP
>>> and APs -- mCpuHotEjectData->ApicIdMap is modified in UnplugCpus() on
>>> the BSP, and checked above by the APs (too).
>>>
>>> What guarantees the visibility of mCpuHotEjectData->ApicIdMap?
>>
>> I was banking on SmiRendezvous() explicitly signalling that all
>> processing on the BSP was done before any AP will look at
>> mCpuHotEjectData in SmmCpuFeaturesRendezvousExit().
>>
>> 1716     //
>> 1717     // Wait for BSP's signal to exit SMI
>> 1718     //
>> 1719     while (*mSmmMpSyncData->AllCpusInSync) {
>> 1720       CpuPause ();
>> 1721     }
>> 1722   }
>> 1723
>> 1724 Exit:
>> 1725   SmmCpuFeaturesRendezvousExit (CpuIndex);
> 
> Right; it's a general pattern in edk2: volatile UINT8 (aka BOOLEAN)
> objects are considered atomic. (See
> SMM_DISPATCHER_MP_SYNC_DATA.AllCpusInSync -- it's a pointer to a
> volatile BOOLEAN.)
> 
> But our UINT64 values are neither volatile nor UINT8, and I got suddenly
> doubtful about "AllCpusInSync" working as a multiprocessor barrier.
> 
> (I could be unjustifiedly worried, as a bunch of other fields in
> SMM_DISPATCHER_MP_SYNC_DATA are volatile, wider than UINT8, and *not*
> accessed with InterlockedCompareExchageXx().)

Thanks for pointing me to this code. There's a curious comment in
about making this structure uncache-able in the declaration here
(though I couldn't figure out how that is done):

418 typedef struct {
419   //
420   // Pointer to an array. The array should be located immediately after this structure
421   // so that UC cache-ability can be set together.
422   //
423   SMM_CPU_DATA_BLOCK            *CpuData;
424   volatile UINT32               *Counter;
425   volatile UINT32               BspIndex;
426   volatile BOOLEAN              *InsideSmm;
427   volatile BOOLEAN              *AllCpusInSync;
428   volatile SMM_CPU_SYNC_MODE    EffectiveSyncMode;
429   volatile BOOLEAN              SwitchBsp;
430   volatile BOOLEAN              *CandidateBsp;
431   EFI_AP_PROCEDURE              StartupProcedure;
432   VOID                          *StartupProcArgs;
433 } SMM_DISPATCHER_MP_SYNC_DATA;

Also, is there an expectation that these fields (at least some of
them) switch over when a new leader is chosen?

Otherwise I'm not sure why for instance, AllCpusInSync would be
a pointer.

> 
> 
>>
>>>
>>> I think we might want to use InterlockedCompareExchange64() in both
>>> EjectCpu() and UnplugCpus() (and make "ApicIdMap" volatile, in
>>> addition). InterlockedCompareExchange64() can be used just for
>>> comparison as well, by passing ExchangeValue=CompareValue.
>>
>>
>> Speaking specifically about the ApicIdMap, I'm not sure I fully
>> agree (assuming my comment just above is correct.)
>>
>>
>> The only AP (reader) ApicIdMap deref is here:
>>
>> CpuEject():
>> 218   ApicId = mCpuHotEjectData->ApicIdMap[ProcessorNum];
>>
>> For the to-be-ejected-AP, this value can only move from
>>     valid-APIC-ID (=> wait in CpuDeadLoop()) -> CPU_EJECT_INVALID.
>>
>> Given that, by the time the worker does the write on line 254, this
>> AP is guaranteed to be dead already, I don't think there's any
>> scenario where the to-be-ejected-AP can see anything other than
>> a valid-APIC-ID.
> 
> The scenario I had in mind was different: what guarantees that the
> effect of
> 
>     375        mCpuHotEjectData->ApicIdMap[ProcessorNum] = (UINT64)RemoveApicId;
> 
> which is performed by the BSP in UnplugCpus(), is visible by the AP on
> line 218 (see your quote above)?
> 
> What if the AP gets to line 218 before the BSP's write on line 375
> *propagates* sufficiently?

I understand. That does make sense. And, as you said elsewhere, a real
memory fence would come in useful here.

We could use AsmCpuid() as a poor man's mfence, but that seems overkill
given that x86 at least guarantees store-order.


Ankur

> 
> There's no question that the BSP writes before the AP reads, but I'm
> uncertain if that suffices for the *effect* of the write to be visible
> to the AP. My concern is not whether the AP sees a partial vs. a settled
> update; my concern is if the AP could see an entirely *stale* value.
> 
> The consequence of that problem would be that an AP that the BSP were
> about to eject would return from CpuEject() to
> SmmCpuFeaturesRendezvousExit() to SmiRendezvous().
> 
> ... I guess that volatile-qualifying both CPU_HOT_EJECT_DATA, and the
> array pointed-to by CPU_HOT_EJECT_DATA.ApicIdMap, should suffice. In
> combination with the sync-up point that you quoted. This seems to match
> existing practice in PiSmmCpuDxeSmm -- there are no concurrent accesses,
> so atomicity is not a concern, and serializing the instruction streams
> coarsely, with the sync-up, in combination with volatile accesses,
> should presumably guarantee visibility (on x86 anyway).
> 
> Thanks
> Laszlo
> 
> 
>>
>> 241         QemuCpuhpWriteCpuSelector (mMmCpuIo, (APIC_ID) RemoveApicId);
>> 242         QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
>> 243
>> 244         //
>> 245         // Compiler barrier to ensure the next store isn't reordered
>> 246         //
>> 247         MemoryFence ();
>> 248
>> 249         //
>> 250         // Clear the eject status for CpuIndex to ensure that an
>> invalid
>> 251         // SMI later does not end up trying to eject it or a newly
>> 252         // hotplugged CpuIndex does not go into the dead loop.
>> 253         //
>> 254         mCpuHotEjectData->ApicIdMap[CpuIndex] = CPU_EJECT_INVALID;
>>    For APs that are not being ejected, they will always see
>> CPU_EJECT_INVALID
>> since the writer never changes that.
>>
>> The one scenario in which bad things could happen is if entries in the
>> ApicIdMap are unaligned (or if the compiler or cpu-arch tears aligned
>> writes).
>>
>>>
>>> (17) I think a similar observation applies to the "Handler" field too,
>>> as APs call it, while the BSP keeps flipping it between NULL and a real
>>> function address. We might have to turn that field into an
>>  From a real function address, to NULL is the problem part right?
>>
>> (Same argument as above for the transition in UnplugCpus() from
>> NULL -> function-address.)
>>
>>
>>> EFI_PHYSICAL_ADDRESS (just a fancy name for UINT64), and use
>>> InterlockedCompareExchange64() again.
>>
>> AFAICS, these are the problematic derefs:
>>
>> SmmCpuFeaturesRendezvousExit():
>>
>> 450   if (mCpuHotEjectData == NULL ||
>> 451       mCpuHotEjectData->Handler == NULL) {
>> 452     return;
>>
>> and problematic assignments:
>>
>> 266     //
>> 267     // We are done until the next hot-unplug; clear the handler.
>> 268     //
>> 269     mCpuHotEjectData->Handler = NULL;
>> 270     return;
>> 271   }
>>
>> Here as well, I've been banking on aligned writes such that the APs would
>> only see the before or after value not an intermediate value.
>>
>> Thanks
>> Ankur
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>>
>