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

["Laszlo Ersek" <lersek@redhat.com>]

On 02/02/21 15:00, Laszlo Ersek wrote:

> ... 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).

To summarize, this is what I would ask for:

- make CPU_HOT_EJECT_DATA volatile

- make (*CPU_HOT_EJECT_DATA.ApicIdMap) volatile

- after storing something to CPU_HOT_EJECT_DATA or
CPU_HOT_EJECT_DATA.ApicIdMap on the BSP, execute a MemoryFence()

- before fetching something from CPU_HOT_EJECT_DATA or
CPU_HOT_EJECT_DATA.ApicIdMap on an AP, execute a MemoryFence()


Except: MemoryFence() isn't a *memory fence* in fact.

See "MdePkg/Library/BaseLib/X64/GccInline.c".

It's just a compiler barrier, which may not add anything beyond what
we'd already have from "volatile".

Case in point: PiSmmCpuDxeSmm performs heavy multi-processing, but does
not contain a single invocation of MemoryFence(). It uses volatile
objects, and a handful of InterlockedCompareExchangeXx() calls, for
implementing semaphores. (NB: there is no 8-bit variant of
InterlockedCompareExchange(), as "volatile UINT8" is considered atomic
in itself, and a suitable basis for a sempahore too.) And given the
synchronization from those semaphores, PiSmmCpuDpxeSmm trusts that
updates to the *other* volatile objects are both atomic and visible.

I'm pretty sure this only works because x86 is in-order. There are
instruction stream barriers in place, and compiler barriers too, but no
actual memory barriers.

Now the question is whether we have managed to *sufficiently* imitate
these patterns from PiSmmCpuDxeSmm, in this patch set.

Making stuff volatile, and relying on the existent sync-up point, might
suffice.

Thanks
Laszlo