Re: [edk2-devel] [PATCH v8 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler

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

Adding Paolo, one comment below:

On 02/22/21 08:19, Ankur Arora wrote:
> Call the CPU hot-eject handler if one is installed. The condition for
> installation is (PcdCpuMaxLogicalProcessorNumber > 1), and there's
> a hot-unplug request.
> 
> The handler executes in context of SmmCpuFeaturesRendezvousExit(),
> which is called at the tail end of SmiRendezvous() after the BSP has
> given the signal to exit via the "AllCpusInSync" loop.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
> 
> Notes:
>     Address the following review comments from v6, patch-6:
>      (19a) Move the call to the ejection handler to a separate patch.
>      (19b) Describe the calling context of SmmCpuFeaturesRendezvousExit().
>      (20) Add comment describing the state when the Handler is not armed.
> 
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index adbfc90ad46e..99988285b6a2 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -467,6 +467,21 @@ SmmCpuFeaturesRendezvousExit (
>    IN UINTN  CpuIndex
>    )
>  {
> +  //
> +  // We only call the Handler if CPU hot-eject is enabled
> +  // (PcdCpuMaxLogicalProcessorNumber > 1), and hot-eject is needed
> +  // in this SMI exit (otherwise mCpuHotEjectData->Handler is not armed.)
> +  //
> +
> +  if (mCpuHotEjectData != NULL) {
> +    CPU_HOT_EJECT_HANDLER Handler;
> +
> +    Handler = mCpuHotEjectData->Handler;

This patch looks otherwise OK to me, but:

In patch v8 08/10, we have a ReleaseMemoryFence(). (For now, it is only
expressed as a MemoryFence() call; we'll make that more precise later.)

(1) I think that should be paired with an AcquireMemoryFence() call,
just before loading "mCpuHotEjectData->Handler" above -- for now, also
expressed as a MemoryFence() call only.

BTW the first article in Paolo's series has been published:

  https://lwn.net/Articles/844224/

so in terms of that, we have something similar to this diagram:

    thread 1                              thread 2
    --------------------------------      ------------------------
    a.x = 1;
    smp_wmb();
    WRITE_ONCE(message, &a);              datum = READ_ONCE(message);
                                          smp_rmb();
                                          if (datum != NULL)
                                            printk("%x\n", datum->x);

In patch 8, UnplugCpus() does the first two lines of the "thread 1"
(BSP) actions, and the third line is covered by the final "AllCpusInSync
= FALSE" assignment in BSPHandler() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c].

Regarding the thread#2 (AP) actions, line#1 is covered by the
"AllCpusInSync loop" near the end of SmiRendezvous(). Lines 3+ are
covered by our SmmCpuFeaturesRendezvousExit() implementation here. But
line#2 (the AcquireMemoryFence()) is missing.

... I'll suspend the review at this point for today; let's see whether
we agree on the comments I've made so far. I hope to continue the review
tomorrow.

Thanks!
Laszlo

> +
> +    if (Handler != NULL) {
> +      Handler (CpuIndex);
> +    }
> +  }
>  }
>  
>  /**
>