Re: [PATCH v2 07/16] OvmfPkg/CpuHotplugSmm: add hotplug register block helper functions

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

On 2/26/20 11:11 PM, Laszlo Ersek wrote:
> Add a handful of simple functions for accessing QEMU's hotplug registers
> more conveniently. These functions thinly wrap some of the registers
> described in "docs/specs/acpi_cpu_hotplug.txt" in the QEMU tree. The
> functions hang (by design) if they encounter an internal failure.
> 
> 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/CpuHotplugSmm/CpuHotplugSmm.inf |   2 +
>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.h       |  47 +++++++
>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.c       | 136 ++++++++++++++++++++
>   3 files changed, 185 insertions(+)
> 
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> index fa70858a8dab..ac4ca4c1f4f2 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> @@ -4,44 +4,46 @@
>   # 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]
>     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]
>     gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire                     ## CONSUMES
>   
>   [Depex]
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> new file mode 100644
> index 000000000000..82f88f0b73bb
> --- /dev/null
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> @@ -0,0 +1,47 @@
> +/** @file
> +  Simple wrapper functions that access QEMU's modern CPU hotplug register
> +  block.
> +
> +  These functions thinly wrap 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
> +
> +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
> +  );
> +
> +#endif // QEMU_CPUHP_H_
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> new file mode 100644
> index 000000000000..31e46f51934a
> --- /dev/null
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> @@ -0,0 +1,136 @@
> +/** @file
> +  Simple wrapper functions that access QEMU's modern CPU hotplug register
> +  block.
> +
> +  These functions thinly wrap 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;
> +  EFI_STATUS Status;
> +
> +  CommandData2 = 0;
> +  Status = MmCpuIo->Io.Read (
> +                         MmCpuIo,
> +                         MM_IO_UINT32,
> +                         ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_R_CMD_DATA2,
> +                         1,
> +                         &CommandData2
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +  return CommandData2;
> +}
> +
> +UINT8
> +QemuCpuhpReadCpuStatus (
> +  IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
> +  )
> +{
> +  UINT8      CpuStatus;
> +  EFI_STATUS Status;
> +
> +  CpuStatus = 0;
> +  Status = MmCpuIo->Io.Read (
> +                         MmCpuIo,
> +                         MM_IO_UINT8,
> +                         ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_R_CPU_STAT,
> +                         1,
> +                         &CpuStatus
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +  return CpuStatus;
> +}
> +
> +UINT32
> +QemuCpuhpReadCommandData (
> +  IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
> +  )
> +{
> +  UINT32     CommandData;
> +  EFI_STATUS Status;
> +
> +  CommandData = 0;
> +  Status = MmCpuIo->Io.Read (
> +                         MmCpuIo,
> +                         MM_IO_UINT32,
> +                         ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA,
> +                         1,
> +                         &CommandData
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +  return CommandData;
> +}
> +
> +VOID
> +QemuCpuhpWriteCpuSelector (
> +  IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
> +  IN UINT32                       Selector
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  Status = MmCpuIo->Io.Write (
> +                         MmCpuIo,
> +                         MM_IO_UINT32,
> +                         ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL,
> +                         1,
> +                         &Selector
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +}
> +
> +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 ();
> +  }
> +}
> 

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