On 2021-02-03 12:55 p.m., Laszlo Ersek wrote:
On 02/03/21 07:13, Ankur Arora wrote:
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:
(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 //
This is probably through SMRR manipulation.
The "UefiCpuPkg/Library/SmmCpuFeaturesLib" instance contains SMRR support.
The "OvmfPkg/Library/SmmCpuFeaturesLib" instance contains no SMRR
support. (Just search both source files for "SMRR".)
Oh, now I see what SMRR does. Thanks that helps make sense of
what's going on here.
Ankur
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?
Yes, see for example the "Elect BSP" section in SmiRendezvous().
Otherwise I'm not sure why for instance, AllCpusInSync would be
a pointer.
TBH I can't explain that; I'm not too familiar with those parts...
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.
Right -- I don't recall any examples of AsmCpuid() being used like that
in edk2.
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71128): https://edk2.groups.io/g/devel/message/71128
Mute This Topic: https://groups.io/mt/80199926/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
