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

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

On 03/02/20 21:34, Philippe Mathieu-Daudé wrote:
> 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)"?

Zero is not positive, zero is zero. Positive implies nonzero. :)

Thanks
Laszlo

> 
>> +
>> +  @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>
>