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

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

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

(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
EFI_PHYSICAL_ADDRESS (just a fancy name for UINT64), and use
InterlockedCompareExchange64() again.

Thanks
Laszlo