Re: [edk2-devel] [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events

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

On 03/12/21 07:26, Ankur Arora wrote:
> Process fw_remove events in QemuCpuhpCollectApicIds(), and collect APIC IDs
> and QEMU CPU Selectors for CPUs being hot-unplugged.
> 
> In addition, we now ignore CPUs which only have remove set. These
> CPUs haven't been processed by OSPM yet.
> 
> This is based on the QEMU hot-unplug protocol documented here:
>   https://lore.kernel.org/qemu-devel/20201204170939.1815522-3-imammedo@redhat.com/
> 
> 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:
>     Addresses the following comments from v8:
>       (1) Fix commit message to mention that we collect cpu-selectors as well.
>       (2,3,6) s/UnplugSelector/UnplugSelectors/ in CpuHotplug.c, QemuCpuhp.c
>       (4) Fix comment above the declaration of the now renamed mToUnplugSelector.
>       (5) Fix spacing around "||".
>       (7) Fix QemuCpuCollectApicIds() comments to line up descriptions for
>           ToUnplugSelectors and other params.
>       (8) s/ExtendSel/ExtendSels/.
>       (9) Add the (ExtendSels => ExtendIds) assert.
>       (10) Fix the missing CurrentSelector++ bug.
> 
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                 |   1 +
>  OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |   1 +
>  OvmfPkg/CpuHotplugSmm/CpuHotplug.c                |  29 +++++--
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                 | 101 +++++++++++++++-------
>  4 files changed, 93 insertions(+), 39 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> 
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> index 8adaa0ad91f0..3e2c2192e1c0 100644
> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> @@ -55,6 +55,7 @@ QemuCpuhpCollectApicIds (
>    OUT APIC_ID                      *PluggedApicIds,
>    OUT UINT32                       *PluggedCount,
>    OUT APIC_ID                      *ToUnplugApicIds,
> +  OUT UINT32                       *ToUnplugSelectors,
>    OUT UINT32                       *ToUnplugCount
>    );
>  
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> index a34a6d3fae61..2ec7a107a64d 100644
> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> @@ -34,6 +34,7 @@
>  #define QEMU_CPUHP_STAT_ENABLED                BIT0
>  #define QEMU_CPUHP_STAT_INSERT                 BIT1
>  #define QEMU_CPUHP_STAT_REMOVE                 BIT2
> +#define QEMU_CPUHP_STAT_FW_REMOVE              BIT4
>  
>  #define QEMU_CPUHP_RW_CMD_DATA               0x8
>  
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index bf68fcd42914..ee1497b93140 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -45,13 +45,16 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
>  // don't want to allocate SMRAM at OS runtime, and potentially fail (or
>  // fragment the SMRAM map).
>  //
> -// These arrays provide room for ("possible CPU count" minus one) APIC IDs
> -// each, as we don't expect every possible CPU to appear, or disappear, in a
> -// single MMI. The numbers of used (populated) elements in the arrays are
> +// The first array stores APIC IDs for hot-plug events, the second and the
> +// third store APIC IDs and QEMU CPU Selectors (both indexed similarly) for
> +// hot-unplug events. All of these provide room for "possible CPU count" minus
> +// one elements as we don't expect every possible CPU to appear, or disappear,
> +// in a single MMI. The numbers of used (populated) elements in the arrays are
>  // determined on every MMI separately.
>  //
>  STATIC APIC_ID *mPluggedApicIds;
>  STATIC APIC_ID *mToUnplugApicIds;
> +STATIC UINT32  *mToUnplugSelectors;
>  //
>  // Address of the non-SMRAM reserved memory page that contains the Post-SMM Pen
>  // for hot-added CPUs.
> @@ -289,6 +292,7 @@ CpuHotplugMmi (
>               mPluggedApicIds,
>               &PluggedCount,
>               mToUnplugApicIds,
> +             mToUnplugSelectors,
>               &ToUnplugCount
>               );
>    if (EFI_ERROR (Status)) {
> @@ -333,7 +337,9 @@ CpuHotplugEntry (
>    )
>  {
>    EFI_STATUS Status;
> +  UINTN      Len;
>    UINTN      Size;
> +  UINTN      SizeSel;
>  
>    //
>    // This module should only be included when SMM support is required.
> @@ -387,8 +393,9 @@ CpuHotplugEntry (
>    //
>    // Allocate the data structures that depend on the possible CPU count.
>    //
> -  if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Size)) ||
> -      RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Size, &Size))) {
> +  if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Len)) ||
> +      RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Len, &Size)) ||
> +      RETURN_ERROR (SafeUintnMult (sizeof (UINT32), Len, &SizeSel))) {
>      Status = EFI_ABORTED;
>      DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_PLUG_DATA\n", __FUNCTION__));
>      goto Fatal;
> @@ -405,6 +412,12 @@ CpuHotplugEntry (
>      DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));
>      goto ReleasePluggedApicIds;
>    }
> +  Status = gMmst->MmAllocatePool (EfiRuntimeServicesData, SizeSel,
> +                    (VOID **)&mToUnplugSelectors);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));
> +    goto ReleaseToUnplugApicIds;
> +  }
>  
>    //
>    // Allocate the Post-SMM Pen for hot-added CPUs.
> @@ -412,7 +425,7 @@ CpuHotplugEntry (
>    Status = SmbaseAllocatePostSmmPen (&mPostSmmPenAddress,
>               SystemTable->BootServices);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseToUnplugApicIds;
> +    goto ReleaseToUnplugSelectors;
>    }
>  
>    //
> @@ -472,6 +485,10 @@ ReleasePostSmmPen:
>    SmbaseReleasePostSmmPen (mPostSmmPenAddress, SystemTable->BootServices);
>    mPostSmmPenAddress = 0;
>  
> +ReleaseToUnplugSelectors:
> +  gMmst->MmFreePool (mToUnplugSelectors);
> +  mToUnplugSelectors = NULL;
> +
>  ReleaseToUnplugApicIds:
>    gMmst->MmFreePool (mToUnplugApicIds);
>    mToUnplugApicIds = NULL;
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> index 8d4a6693c8d6..8434dd446b96 100644
> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> @@ -145,27 +145,30 @@ QemuCpuhpWriteCommand (
>  
>    On error, the contents of the output parameters are undefined.
>  
> -  @param[in] MmCpuIo           The EFI_MM_CPU_IO_PROTOCOL instance for
> -                               accessing IO Ports.
> +  @param[in] MmCpuIo             The EFI_MM_CPU_IO_PROTOCOL instance for
> +                                 accessing IO Ports.
>  
> -  @param[in] PossibleCpuCount  The number of possible CPUs in the system. Must
> -                               be positive.
> +  @param[in] PossibleCpuCount    The number of possible CPUs in the system. Must
> +                                 be positive.
>  
> -  @param[in] ApicIdCount       The number of elements each one of the
> -                               PluggedApicIds and ToUnplugApicIds arrays can
> -                               accommodate. Must be positive.
> +  @param[in] ApicIdCount         The number of elements each one of the
> +                                 PluggedApicIds and ToUnplugApicIds arrays can
> +                                 accommodate. Must be positive.
>  
> -  @param[out] PluggedApicIds   The APIC IDs of the CPUs that have been
> -                               hot-plugged.
> +  @param[out] PluggedApicIds     The APIC IDs of the CPUs that have been
> +                                 hot-plugged.
>  
> -  @param[out] PluggedCount     The number of filled-in APIC IDs in
> -                               PluggedApicIds.
> +  @param[out] PluggedCount       The number of filled-in APIC IDs in
> +                                 PluggedApicIds.
>  
> -  @param[out] ToUnplugApicIds  The APIC IDs of the CPUs that are about to be
> -                               hot-unplugged.
> +  @param[out] ToUnplugApicIds    The APIC IDs of the CPUs that are about to be
> +                                 hot-unplugged.
>  
> -  @param[out] ToUnplugCount    The number of filled-in APIC IDs in
> -                               ToUnplugApicIds.
> +  @param[out] ToUnplugSelectors  The QEMU Selectors of the CPUs that are about
> +                                 to be hot-unplugged.
> +
> +  @param[out] ToUnplugCount      The number of filled-in APIC IDs in
> +                                 ToUnplugApicIds.
>  
>    @retval EFI_INVALID_PARAMETER  PossibleCpuCount is zero, or ApicIdCount is
>                                   zero.
> @@ -187,6 +190,7 @@ QemuCpuhpCollectApicIds (
>    OUT APIC_ID                      *PluggedApicIds,
>    OUT UINT32                       *PluggedCount,
>    OUT APIC_ID                      *ToUnplugApicIds,
> +  OUT UINT32                       *ToUnplugSelectors,
>    OUT UINT32                       *ToUnplugCount
>    )
>  {
> @@ -204,6 +208,7 @@ QemuCpuhpCollectApicIds (
>      UINT32  PendingSelector;
>      UINT8   CpuStatus;
>      APIC_ID *ExtendIds;
> +    UINT32  *ExtendSels;
>      UINT32  *ExtendCount;
>      APIC_ID NewApicId;
>  
> @@ -245,10 +250,10 @@ QemuCpuhpCollectApicIds (
>      if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) != 0) {
>        //
>        // The "insert" event guarantees the "enabled" status; plus it excludes
> -      // the "remove" event.
> +      // the "fw_remove" event.
>        //
>        if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0 ||
> -          (CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> +          (CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) {
>          DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: "
>            "inconsistent CPU status\n", __FUNCTION__, CurrentSelector,
>            CpuStatus));
> @@ -259,33 +264,63 @@ QemuCpuhpCollectApicIds (
>          CurrentSelector));
>  
>        ExtendIds   = PluggedApicIds;
> +      ExtendSels  = NULL;
>        ExtendCount = PluggedCount;
> -    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> -      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", __FUNCTION__,
> -        CurrentSelector));
> +    } else if ((CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) {
> +      //
> +      // "fw_remove" event guarantees "enabled".
> +      //
> +      if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0) {
> +        DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: "
> +          "inconsistent CPU status\n", __FUNCTION__, CurrentSelector,
> +          CpuStatus));
> +        return EFI_PROTOCOL_ERROR;
> +      }
> +
> +      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: fw_remove\n",
> +        __FUNCTION__, CurrentSelector));
>  
>        ExtendIds   = ToUnplugApicIds;
> +      ExtendSels  = ToUnplugSelectors;
>        ExtendCount = ToUnplugCount;
> +    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> +      //
> +      // Let the OSPM deal with the "remove" event.
> +      //
> +      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove (ignored)\n",
> +        __FUNCTION__, CurrentSelector));
> +
> +      ExtendIds   = NULL;
> +      ExtendSels  = NULL;
> +      ExtendCount = NULL;
>      } else {
>        DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: no event\n",
>          __FUNCTION__, CurrentSelector));
>        break;
>      }
>  
> -    //
> -    // Save the APIC ID of the CPU with the pending event, to the corresponding
> -    // APIC ID array.
> -    //
> -    if (*ExtendCount == ApicIdCount) {
> -      DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));
> -      return EFI_BUFFER_TOO_SMALL;
> -    }
> -    QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);
> -    NewApicId = QemuCpuhpReadCommandData (MmCpuIo);
> -    DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,
> -      NewApicId));
> -    ExtendIds[(*ExtendCount)++] = NewApicId;
> +    ASSERT ((ExtendIds == NULL) == (ExtendCount == NULL));
> +    ASSERT ((ExtendSels == NULL) || (ExtendIds != NULL));
>  
> +    if (ExtendIds != NULL) {
> +      //
> +      // Save the APIC ID of the CPU with the pending event, to the
> +      // corresponding APIC ID array.
> +      // For unplug events, also save the CurrentSelector.
> +      //
> +      if (*ExtendCount == ApicIdCount) {
> +        DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));
> +        return EFI_BUFFER_TOO_SMALL;
> +      }
> +      QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);
> +      NewApicId = QemuCpuhpReadCommandData (MmCpuIo);
> +      DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,
> +        NewApicId));
> +      if (ExtendSels != NULL) {
> +        ExtendSels[(*ExtendCount)] = CurrentSelector;
> +      }
> +      ExtendIds[(*ExtendCount)++] = NewApicId;
> +    }
>      //
>      // We've processed the CPU with (known) pending events, but we must never
>      // clear events. Therefore we need to advance past this CPU manually;
>