Re: [edk2-devel] [PATCH v2 2/3] OvmfPkg/IndustryStandard: define macros for QEMU's CPU hotplug registers

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

Hi Laszlo,

On 10/23/19 12:15 AM, Laszlo Ersek wrote:
> In v1.5.0, QEMU's "pc" (i440fx) board gained a "CPU present bitmap"
> register block. In v2.0.0, this was extended to the "q35" board.
> 
> In v2.7.0, a new (read/write) register interface was laid over the "CPU
> present bitmap", with an option for the guest to switch the register block
> to the new (a.k.a. modern) interface.

This historical information is helpful to understand when these QEMU 
models started to diverge from the original chipset datasheet.

> Both interfaces are documented in "docs/specs/acpi_cpu_hotplug.txt" in the
> QEMU tree.
> 
> Add macros for a minimal subset of the modern interface, just so we can
> count the possible CPUs (as opposed to boot CPUs) in a later patch in this
> series.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>      v2:
>      - use QEMU's existent CPU hotplug register block, rather than a new
>        named file in fw_cfg [Igor]
> 
>   OvmfPkg/Include/IndustryStandard/I440FxPiix4.h    |  5 +++
>   OvmfPkg/Include/IndustryStandard/Q35MchIch9.h     |  2 +
>   OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 43 ++++++++++++++++++++
>   3 files changed, 50 insertions(+)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/I440FxPiix4.h b/OvmfPkg/Include/IndustryStandard/I440FxPiix4.h
> index e7d7fde14c65..3973ff0a95b4 100644
> --- a/OvmfPkg/Include/IndustryStandard/I440FxPiix4.h
> +++ b/OvmfPkg/Include/IndustryStandard/I440FxPiix4.h
> @@ -44,6 +44,11 @@
>                                     BIT10 | BIT9  | BIT8  | BIT7  | BIT6)
>   
>   #define PIIX4_PMREGMISC        0x80
>   #define PIIX4_PMREGMISC_PMIOSE   BIT0
>   
> +//
> +// IO ports
> +//
> +#define PIIX4_CPU_HOTPLUG_BASE 0xAF00

OK

> +
>   #endif
> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> index 391cb4622226..2ac16f19c62e 100644
> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> @@ -104,10 +104,12 @@
>   // IO ports
>   //
>   #define ICH9_APM_CNT 0xB2
>   #define ICH9_APM_STS 0xB3
>   
> +#define ICH9_CPU_HOTPLUG_BASE 0x0CD8

OK

> +
>   //
>   // IO ports relative to PMBASE
>   //
>   #define ICH9_PMBASE_OFS_SMI_EN 0x30
>   #define ICH9_SMI_EN_APMC_EN      BIT5
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> new file mode 100644
> index 000000000000..cf0745610f2c
> --- /dev/null
> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> @@ -0,0 +1,43 @@
> +/** @file
> +  Macros for accessing QEMU's CPU hotplug register block.
> +
> +  Copyright (C) 2019, Red Hat, Inc.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Specification Reference:
> +
> +  - "docs/specs/acpi_cpu_hotplug.txt" in the QEMU source tree.
> +
> +    The original (now "legacy") CPU hotplug interface appeared in QEMU v1.5.0.
> +    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

I don't understand this one. Read register 0x0 is marked "reserved"
in the spec, and CMD_DATA are registers [0x8 .. 0xb].

> +
> +#define QEMU_CPUHP_R_CPU_STAT                0x4
> +#define QEMU_CPUHP_STAT_ENABLED                BIT0

OK

> +
> +#define QEMU_CPUHP_RW_CMD_DATA               0x8

Here the spec says "DWORD access" but the implementation use
"BYTE access" (see AcpiCpuHotplug_ops in hw/acpi/cpu_hotplug.c).
I understand this as "there are 4 consecutive BYTE register).
Not this patch problem although.

> +
> +#define QEMU_CPUHP_W_CPU_SEL                 0x0

Another "DWORD access". OK.

> +
> +#define QEMU_CPUHP_W_CMD                     0x5
> +#define QEMU_CPUHP_CMD_GET_PENDING             0x0

'PENDING' is more meaningful than "CPU device with
inserting/removing events". OK

> +
> +#endif // QEMU_CPU_HOTPLUG_H_
>