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);
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.
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71031): https://edk2.groups.io/g/devel/message/71031
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]
-=-=-=-=-=-=-=-=-=-=-=-
