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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Wed, 26 Feb 2020 at 23:12, Laszlo Ersek <lersek@redhat.com> 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>

Reviewed-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.
> +
> +  @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;
> +}
> --
> 2.19.1.3.g30247aa5d201
>
>