Re: [PATCH v2 09/16] OvmfPkg/CpuHotplugSmm: add function for collecting CPUs with events

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/26/20 11:11 PM, Laszlo Ersek wrote:
> Add a function that collects the APIC IDs of CPUs that have just been
> hot-plugged, or are about to be hot-unplugged.
> 
> Pending events are only located and never cleared; QEMU's AML needs the
> firmware to leave the status bits intact in the hotplug register block.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> Notes:
>      v2:
>      
>      - Pick up Ard's Acked-by, which is conditional on approval from Intel
>        reviewers on Cc. (I'd like to save Ard the churn of re-acking
>        unmodified patches.)
> 
>   OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |   2 +
>   OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf           |   1 +
>   OvmfPkg/CpuHotplugSmm/ApicId.h                    |  23 +++
>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                 |  20 ++-
>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                 | 171 +++++++++++++++++++-
>   5 files changed, 211 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> index 3d013633501b..a34a6d3fae61 100644
> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> @@ -13,32 +13,34 @@
>       The new ("modern") hotplug interface appeared in QEMU v2.7.0.
>   
>       The macros in this header file map to the minimal subset of the modern
>       interface that OVMF needs.
>   **/
>   
>   #ifndef QEMU_CPU_HOTPLUG_H_
>   #define QEMU_CPU_HOTPLUG_H_
>   
>   #include <Base.h>
>   
>   //
>   // Each register offset is:
>   // - relative to the board-dependent IO base address of the register block,
>   // - named QEMU_CPUHP_(R|W|RW)_*, according to the possible access modes of the
>   //   register,
>   // - followed by distinguished bitmasks or values in the register.
>   //
>   #define QEMU_CPUHP_R_CMD_DATA2               0x0
>   
>   #define QEMU_CPUHP_R_CPU_STAT                0x4
>   #define QEMU_CPUHP_STAT_ENABLED                BIT0
> +#define QEMU_CPUHP_STAT_INSERT                 BIT1
> +#define QEMU_CPUHP_STAT_REMOVE                 BIT2
>   
>   #define QEMU_CPUHP_RW_CMD_DATA               0x8
>   
>   #define QEMU_CPUHP_W_CPU_SEL                 0x0
>   
>   #define QEMU_CPUHP_W_CMD                     0x5
>   #define QEMU_CPUHP_CMD_GET_PENDING             0x0
>   #define QEMU_CPUHP_CMD_GET_ARCH_ID             0x3
>   
>   #endif // QEMU_CPU_HOTPLUG_H_
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> index ac4ca4c1f4f2..ab690a9e5e20 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> @@ -3,44 +3,45 @@
>   #
>   # Copyright (c) 2020, Red Hat, Inc.
>   #
>   # SPDX-License-Identifier: BSD-2-Clause-Patent
>   ##
>   
>   [Defines]
>     INF_VERSION                = 1.29
>     PI_SPECIFICATION_VERSION   = 0x00010046                            # PI-1.7.0
>     BASE_NAME                  = CpuHotplugSmm
>     FILE_GUID                  = 84EEA114-C6BE-4445-8F90-51D97863E363
>     MODULE_TYPE                = DXE_SMM_DRIVER
>     ENTRY_POINT                = CpuHotplugEntry
>   
>   #
>   # The following information is for reference only and not required by the build
>   # tools.
>   #
>   # VALID_ARCHITECTURES        = IA32 X64
>   #
>   
>   [Sources]
> +  ApicId.h
>     CpuHotplug.c
>     QemuCpuhp.c
>     QemuCpuhp.h
>   
>   [Packages]
>     MdePkg/MdePkg.dec
>     OvmfPkg/OvmfPkg.dec
>   
>   [LibraryClasses]
>     BaseLib
>     DebugLib
>     MmServicesTableLib
>     PcdLib
>     UefiDriverEntryPoint
>   
>   [Protocols]
>     gEfiMmCpuIoProtocolGuid                                           ## CONSUMES
>   
>   [Pcd]
>     gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase             ## CONSUMES
>   
>   [FeaturePcd]
> diff --git a/OvmfPkg/CpuHotplugSmm/ApicId.h b/OvmfPkg/CpuHotplugSmm/ApicId.h
> new file mode 100644
> index 000000000000..3c365148ed02
> --- /dev/null
> +++ b/OvmfPkg/CpuHotplugSmm/ApicId.h
> @@ -0,0 +1,23 @@
> +/** @file
> +  Type and macro definitions for representing and printing APIC IDs, compatibly
> +  with the LocalApicLib and PrintLib classes, respectively.
> +
> +  Copyright (c) 2020, Red Hat, Inc.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef APIC_ID_H_
> +#define APIC_ID_H_
> +
> +//
> +// The type that LocalApicLib represents an APIC ID with.
> +//
> +typedef UINT32 APIC_ID;
> +
> +//
> +// The PrintLib conversion specification for formatting an APIC_ID.
> +//
> +#define FMT_APIC_ID "0x%08x"
> +
> +#endif // APIC_ID_H_
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> index 82f88f0b73bb..8adaa0ad91f0 100644
> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> @@ -1,47 +1,61 @@
>   /** @file
> -  Simple wrapper functions that access QEMU's modern CPU hotplug register
> -  block.
> +  Simple wrapper functions and utility functions that access QEMU's modern CPU
> +  hotplug register block.
>   
> -  These functions thinly wrap some of the registers described in
> +  These functions manipulate some of the registers described in
>     "docs/specs/acpi_cpu_hotplug.txt" in the QEMU source. IO Ports are accessed
>     via EFI_MM_CPU_IO_PROTOCOL. If a protocol call fails, these functions don't
>     return.
>   
>     Copyright (c) 2020, Red Hat, Inc.
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   **/
>   
>   #ifndef QEMU_CPUHP_H_
>   #define QEMU_CPUHP_H_
>   
>   #include <Protocol/MmCpuIo.h>  // EFI_MM_CPU_IO_PROTOCOL
> +#include <Uefi/UefiBaseType.h> // EFI_STATUS
> +
> +#include "ApicId.h"            // APIC_ID
>   
>   UINT32
>   QemuCpuhpReadCommandData2 (
>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
>     );
>   
>   UINT8
>   QemuCpuhpReadCpuStatus (
>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
>     );
>   
>   UINT32
>   QemuCpuhpReadCommandData (
>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
>     );
>   
>   VOID
>   QemuCpuhpWriteCpuSelector (
>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
>     IN UINT32                       Selector
>     );
>   
>   VOID
>   QemuCpuhpWriteCommand (
>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
>     IN UINT8                        Command
>     );
>   
> +EFI_STATUS
> +QemuCpuhpCollectApicIds (
> +  IN  CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
> +  IN  UINT32                       PossibleCpuCount,
> +  IN  UINT32                       ApicIdCount,
> +  OUT APIC_ID                      *PluggedApicIds,
> +  OUT UINT32                       *PluggedCount,
> +  OUT APIC_ID                      *ToUnplugApicIds,
> +  OUT UINT32                       *ToUnplugCount
> +  );
> +
>   #endif // QEMU_CPUHP_H_
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> index 31e46f51934a..8d4a6693c8d6 100644
> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> @@ -1,27 +1,27 @@
>   /** @file
> -  Simple wrapper functions that access QEMU's modern CPU hotplug register
> -  block.
> +  Simple wrapper functions and utility functions that access QEMU's modern CPU
> +  hotplug register block.
>   
> -  These functions thinly wrap some of the registers described in
> +  These functions manipulate some of the registers described in
>     "docs/specs/acpi_cpu_hotplug.txt" in the QEMU source. IO Ports are accessed
>     via EFI_MM_CPU_IO_PROTOCOL. If a protocol call fails, these functions don't
>     return.
>   
>     Copyright (c) 2020, Red Hat, Inc.
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   **/
>   
>   #include <IndustryStandard/Q35MchIch9.h>     // ICH9_CPU_HOTPLUG_BASE
>   #include <IndustryStandard/QemuCpuHotplug.h> // QEMU_CPUHP_R_CMD_DATA2
>   #include <Library/BaseLib.h>                 // CpuDeadLoop()
>   #include <Library/DebugLib.h>                // DEBUG()
>   
>   #include "QemuCpuhp.h"
>   
>   UINT32
>   QemuCpuhpReadCommandData2 (
>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
>     )
>   {
>     UINT32     CommandData2;
> @@ -115,22 +115,187 @@ QemuCpuhpWriteCpuSelector (
>   
>   VOID
>   QemuCpuhpWriteCommand (
>     IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
>     IN UINT8                        Command
>     )
>   {
>     EFI_STATUS Status;
>   
>     Status = MmCpuIo->Io.Write (
>                            MmCpuIo,
>                            MM_IO_UINT8,
>                            ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD,
>                            1,
>                            &Command
>                            );
>     if (EFI_ERROR (Status)) {
>       DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
>       ASSERT (FALSE);
>       CpuDeadLoop ();
>     }
>   }
> +
> +/**
> +  Collect the APIC IDs of
> +  - the CPUs that have been hot-plugged,
> +  - the CPUs that are about to be hot-unplugged.
> +
> +  This function only scans for events -- it does not modify them -- in the
> +  hotplug registers.
> +
> +  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] PossibleCpuCount  The number of possible CPUs in the system. Must
> +                               be positive.

Maybe nitpicking: "positive (non zero)"?

> +
> +  @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] 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] ToUnplugCount    The number of filled-in APIC IDs in
> +                               ToUnplugApicIds.
> +
> +  @retval EFI_INVALID_PARAMETER  PossibleCpuCount is zero, or ApicIdCount is
> +                                 zero.
> +
> +  @retval EFI_PROTOCOL_ERROR     Invalid bitmap detected in the
> +                                 QEMU_CPUHP_R_CPU_STAT register.
> +
> +  @retval EFI_BUFFER_TOO_SMALL   There was an attempt to place more than
> +                                 ApicIdCount APIC IDs into one of the
> +                                 PluggedApicIds and ToUnplugApicIds arrays.
> +
> +  @retval EFI_SUCCESS            Output parameters have been set successfully.
> +**/
> +EFI_STATUS
> +QemuCpuhpCollectApicIds (
> +  IN  CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
> +  IN  UINT32                       PossibleCpuCount,
> +  IN  UINT32                       ApicIdCount,
> +  OUT APIC_ID                      *PluggedApicIds,
> +  OUT UINT32                       *PluggedCount,
> +  OUT APIC_ID                      *ToUnplugApicIds,
> +  OUT UINT32                       *ToUnplugCount
> +  )
> +{
> +  UINT32 CurrentSelector;
> +
> +  if (PossibleCpuCount == 0 || ApicIdCount == 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *PluggedCount = 0;
> +  *ToUnplugCount = 0;
> +
> +  CurrentSelector = 0;
> +  do {
> +    UINT32  PendingSelector;
> +    UINT8   CpuStatus;
> +    APIC_ID *ExtendIds;
> +    UINT32  *ExtendCount;
> +    APIC_ID NewApicId;
> +
> +    //
> +    // Write CurrentSelector (which is valid) to the CPU selector register.
> +    // Consequences:
> +    //
> +    // - Other register accesses will be permitted.
> +    //
> +    // - The QEMU_CPUHP_CMD_GET_PENDING command will start scanning for a CPU
> +    //   with pending events at CurrentSelector (inclusive).
> +    //
> +    QemuCpuhpWriteCpuSelector (MmCpuIo, CurrentSelector);
> +    //
> +    // Write the QEMU_CPUHP_CMD_GET_PENDING command. Consequences
> +    // (independently of each other):
> +    //
> +    // - If there is a CPU with pending events, starting at CurrentSelector
> +    //   (inclusive), the CPU selector will be updated to that CPU. Note that
> +    //   the scanning in QEMU may wrap around, because we must never clear the
> +    //   event bits.
> +    //
> +    // - The QEMU_CPUHP_RW_CMD_DATA register will return the (possibly updated)
> +    //   CPU selector value.
> +    //
> +    QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_PENDING);
> +    PendingSelector = QemuCpuhpReadCommandData (MmCpuIo);
> +    if (PendingSelector < CurrentSelector) {
> +      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u PendingSelector=%u: "
> +        "wrap-around\n", __FUNCTION__, CurrentSelector, PendingSelector));
> +      break;
> +    }
> +    CurrentSelector = PendingSelector;
> +
> +    //
> +    // Check the known status / event bits for the currently selected CPU.
> +    //
> +    CpuStatus = QemuCpuhpReadCpuStatus (MmCpuIo);
> +    if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) != 0) {
> +      //
> +      // The "insert" event guarantees the "enabled" status; plus it excludes
> +      // the "remove" event.
> +      //
> +      if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0 ||
> +          (CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 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: insert\n", __FUNCTION__,
> +        CurrentSelector));
> +
> +      ExtendIds   = PluggedApicIds;
> +      ExtendCount = PluggedCount;
> +    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> +      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", __FUNCTION__,
> +        CurrentSelector));
> +
> +      ExtendIds   = ToUnplugApicIds;
> +      ExtendCount = ToUnplugCount;
> +    } 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;
> +
> +    //
> +    // We've processed the CPU with (known) pending events, but we must never
> +    // clear events. Therefore we need to advance past this CPU manually;
> +    // otherwise, QEMU_CPUHP_CMD_GET_PENDING would stick to the currently
> +    // selected CPU.
> +    //
> +    CurrentSelector++;
> +  } while (CurrentSelector < PossibleCpuCount);
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: PluggedCount=%u ToUnplugCount=%u\n",
> +    __FUNCTION__, *PluggedCount, *ToUnplugCount));
> +  return EFI_SUCCESS;
> +}
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>