[PATCH v2 0/3] OvmfPkg: distinguish boot CPU count from possible CPU count

[PATCH v2 0/3] OvmfPkg: distinguish boot CPU count from possible CPU count

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: max_cpus_bz_1515_v2
Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=1515

Version 1 was:

  [edk2-devel] [PATCH 0/4]
  UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count

  http://mid.mail-archive.com/20191008112714.6215-1-lersek@redhat.com
  https://edk2.groups.io/g/devel/message/48562

Since then, patch#1 of v1 has been rewritten and pushed as commit range
a7e2d20193e8..778832bcad33.

This v2 posting reworks the rest (patches #2 through #4) of the v1
series. Changes are listed per patch, in the notes sections.

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@arm.com>

Thanks
Laszlo

Laszlo Ersek (3):
  OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults
  OvmfPkg/IndustryStandard: define macros for QEMU's CPU hotplug
    registers
  OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU
    hotplug

 OvmfPkg/Include/IndustryStandard/I440FxPiix4.h    |   5 +
 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h     |   2 +
 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |  43 +++++
 OvmfPkg/OvmfPkgIa32.dsc                           |   2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                        |   2 +-
 OvmfPkg/OvmfPkgX64.dsc                            |   2 +-
 OvmfPkg/OvmfXen.dsc                               |   4 -
 OvmfPkg/PlatformPei/Platform.c                    | 172 +++++++++++++++++---
 OvmfPkg/PlatformPei/PlatformPei.inf               |   2 +-
 9 files changed, 201 insertions(+), 33 deletions(-)
 create mode 100644 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h

-- 
2.19.1.3.g30247aa5d201

[PATCH v2 1/3] OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults

[Laszlo Ersek]

PcdCpuMaxLogicalProcessorNumber and PcdCpuApInitTimeOutInMicroSeconds are
only referenced in "OvmfPkg/PlatformPei/PlatformPei.inf", and OvmfXen does
not include that module. Remove the unnecessary dynamic PCD defaults from
"OvmfXen.dsc".

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@arm.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

Notes:
    v2:
    - pick up Phil's R-b
    - pick up Ard's A-b

 OvmfPkg/OvmfXen.dsc | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 8c11efe9b709..1b18f1769bbc 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -480,14 +480,10 @@ [PcdsDynamicDefault]
 
   # Noexec settings for DXE.
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
 
-  # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
-
   # Set memory encryption mask
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
 
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
 
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH v2 1/3] OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults

[Anthony PERARD]

On Wed, Oct 23, 2019 at 12:15:52AM +0200, Laszlo Ersek wrote:
> PcdCpuMaxLogicalProcessorNumber and PcdCpuApInitTimeOutInMicroSeconds are
> only referenced in "OvmfPkg/PlatformPei/PlatformPei.inf", and OvmfXen does
> not include that module. Remove the unnecessary dynamic PCD defaults from
> "OvmfXen.dsc".
> 
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

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

[Laszlo Ersek]

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.

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
+
 #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
+
 //
 // 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
+
+#define QEMU_CPUHP_R_CPU_STAT                0x4
+#define QEMU_CPUHP_STAT_ENABLED                BIT0
+
+#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
+
+#endif // QEMU_CPU_HOTPLUG_H_
-- 
2.19.1.3.g30247aa5d201

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

[Philippe Mathieu-Daudé]

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_
> 

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

[Laszlo Ersek]

On 10/23/19 14:05, Philippe Mathieu-Daudé wrote:
> 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].

The documentation (spec) is being cleaned up, and also extended. Please
see the following QEMU patch set (warning: long discussion):

  [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to
                         cpu hotplug MMIO interface

  http://mid.mail-archive.com/20191009132252.17860-1-imammedo@redhat.com

In that qemu RFC series, the first two patches are of primary interest
here, for this edk2 / OvmfPkg patch set. The first two patches will not
extend the modern interface, they will just document it better (clean up
the documentation for the existent behavior).

If you look at the next patch in this series, you'll see that the
OvmfPkg/PlatformPei code uses QEMU_CPUHP_R_CMD_DATA2 considering *both*
situations, that is,
- when the offset range [0..3] is marked as reserved for reading,
- and when it is specified as Command Data 2 (by the last patch in the
QEMU RFC series).

Therefore, we could call this register

  QEMU_CPUHP_R_RESERVED_0

for now, but very soon we'd have to rename it to the above-proposed

  QEMU_CPUHP_R_CMD_DATA2

anyway. Again, the code considers both cases.

(In fact, the code in the next patch considers *four* cases:
- hotplug register block absent,
- hotplug register block present, but only legacy mode is supported,
- hotplug register block present, modern mode available, but offset 0 is
still reserved for reading,
- hotplug register block present, modern mode available, offset 0
defined as Cmd Data 2.)


> 
>> +
>> +#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).

You are looking at the legacy mode of the register block (documented
near the top of the "docs/specs/acpi_cpu_hotplug.txt" file).

The operations for modern mode are in "hw/acpi/cpu.c", object
"cpu_hotplug_ops".


> 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

Thanks!
Laszlo

> 
>> +
>> +#endif // QEMU_CPU_HOTPLUG_H_
>>

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

[Philippe Mathieu-Daudé]

On 10/24/19 12:29 PM, Laszlo Ersek wrote:
> On 10/23/19 14:05, Philippe Mathieu-Daudé wrote:
>> 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].
> 
> The documentation (spec) is being cleaned up, and also extended. Please
> see the following QEMU patch set (warning: long discussion):
> 
>    [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to
>                           cpu hotplug MMIO interface
> 
>    http://mid.mail-archive.com/20191009132252.17860-1-imammedo@redhat.com
> 
> In that qemu RFC series, the first two patches are of primary interest
> here, for this edk2 / OvmfPkg patch set. The first two patches will not
> extend the modern interface, they will just document it better (clean up
> the documentation for the existent behavior).

OK I read it, but it is still RFC, so this patch shouldn't get merged 
until the QEMU counterpart get merged, right?

> If you look at the next patch in this series, you'll see that the
> OvmfPkg/PlatformPei code uses QEMU_CPUHP_R_CMD_DATA2 considering *both*
> situations, that is,
> - when the offset range [0..3] is marked as reserved for reading,
> - and when it is specified as Command Data 2 (by the last patch in the
> QEMU RFC series).
> 
> Therefore, we could call this register
> 
>    QEMU_CPUHP_R_RESERVED_0
> 
> for now, but very soon we'd have to rename it to the above-proposed
> 
>    QEMU_CPUHP_R_CMD_DATA2
> 
> anyway. Again, the code considers both cases.
> 
> (In fact, the code in the next patch considers *four* cases:
> - hotplug register block absent,
> - hotplug register block present, but only legacy mode is supported,
> - hotplug register block present, modern mode available, but offset 0 is
> still reserved for reading,
> - hotplug register block present, modern mode available, offset 0
> defined as Cmd Data 2.)

Understood.

This patch is fine then.

Since the QEMU spec is RFC and not merged, I'm worried it might
change. I'd rather review this patch again once the QEMU spec
update is accepted/merged.

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

>>
>>> +
>>> +#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).
> 
> You are looking at the legacy mode of the register block (documented
> near the top of the "docs/specs/acpi_cpu_hotplug.txt" file).
> 
> The operations for modern mode are in "hw/acpi/cpu.c", object
> "cpu_hotplug_ops".
> 
> 
>> 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
> 
> Thanks!
> Laszlo
> 
>>
>>> +
>>> +#endif // QEMU_CPU_HOTPLUG_H_
>>>

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

[Laszlo Ersek]

On 10/24/19 17:12, Philippe Mathieu-Daudé wrote:
> On 10/24/19 12:29 PM, Laszlo Ersek wrote:
>> On 10/23/19 14:05, Philippe Mathieu-Daudé wrote:
>>> 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].
>>
>> The documentation (spec) is being cleaned up, and also extended. Please
>> see the following QEMU patch set (warning: long discussion):
>>
>>    [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to
>>                           cpu hotplug MMIO interface
>>
>>    http://mid.mail-archive.com/20191009132252.17860-1-imammedo@redhat.com
>>
>> In that qemu RFC series, the first two patches are of primary interest
>> here, for this edk2 / OvmfPkg patch set. The first two patches will not
>> extend the modern interface, they will just document it better (clean up
>> the documentation for the existent behavior).
> 
> OK I read it, but it is still RFC, so this patch shouldn't get merged
> until the QEMU counterpart get merged, right?
> 
>> If you look at the next patch in this series, you'll see that the
>> OvmfPkg/PlatformPei code uses QEMU_CPUHP_R_CMD_DATA2 considering *both*
>> situations, that is,
>> - when the offset range [0..3] is marked as reserved for reading,
>> - and when it is specified as Command Data 2 (by the last patch in the
>> QEMU RFC series).
>>
>> Therefore, we could call this register
>>
>>    QEMU_CPUHP_R_RESERVED_0
>>
>> for now, but very soon we'd have to rename it to the above-proposed
>>
>>    QEMU_CPUHP_R_CMD_DATA2
>>
>> anyway. Again, the code considers both cases.
>>
>> (In fact, the code in the next patch considers *four* cases:
>> - hotplug register block absent,
>> - hotplug register block present, but only legacy mode is supported,
>> - hotplug register block present, modern mode available, but offset 0 is
>> still reserved for reading,
>> - hotplug register block present, modern mode available, offset 0
>> defined as Cmd Data 2.)
> 
> Understood.
> 
> This patch is fine then.
> 
> Since the QEMU spec is RFC and not merged, I'm worried it might
> change. I'd rather review this patch again once the QEMU spec
> update is accepted/merged.

Yes, we can certainly delay pushing this set until the QEMU set is merged.

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

Thanks!
Laszlo

> 
>>>
>>>> +
>>>> +#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).
>>
>> You are looking at the legacy mode of the register block (documented
>> near the top of the "docs/specs/acpi_cpu_hotplug.txt" file).
>>
>> The operations for modern mode are in "hw/acpi/cpu.c", object
>> "cpu_hotplug_ops".
>>
>>
>>> 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
>>
>> Thanks!
>> Laszlo
>>
>>>
>>>> +
>>>> +#endif // QEMU_CPU_HOTPLUG_H_
>>>>

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

[Laszlo Ersek]

Hi Phil,

On 10/24/19 17:12, Philippe Mathieu-Daudé wrote:
> On 10/24/19 12:29 PM, Laszlo Ersek wrote:
>> On 10/23/19 14:05, Philippe Mathieu-Daudé wrote:
>>> 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].
>>
>> The documentation (spec) is being cleaned up, and also extended. Please
>> see the following QEMU patch set (warning: long discussion):
>>
>>    [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to
>>                           cpu hotplug MMIO interface
>>
>>    http://mid.mail-archive.com/20191009132252.17860-1-imammedo@redhat.com
>>
>> In that qemu RFC series, the first two patches are of primary interest
>> here, for this edk2 / OvmfPkg patch set. The first two patches will not
>> extend the modern interface, they will just document it better (clean up
>> the documentation for the existent behavior).
> 
> OK I read it, but it is still RFC, so this patch shouldn't get merged
> until the QEMU counterpart get merged, right?
> 
>> If you look at the next patch in this series, you'll see that the
>> OvmfPkg/PlatformPei code uses QEMU_CPUHP_R_CMD_DATA2 considering *both*
>> situations, that is,
>> - when the offset range [0..3] is marked as reserved for reading,
>> - and when it is specified as Command Data 2 (by the last patch in the
>> QEMU RFC series).
>>
>> Therefore, we could call this register
>>
>>    QEMU_CPUHP_R_RESERVED_0
>>
>> for now, but very soon we'd have to rename it to the above-proposed
>>
>>    QEMU_CPUHP_R_CMD_DATA2
>>
>> anyway. Again, the code considers both cases.
>>
>> (In fact, the code in the next patch considers *four* cases:
>> - hotplug register block absent,
>> - hotplug register block present, but only legacy mode is supported,
>> - hotplug register block present, modern mode available, but offset 0 is
>> still reserved for reading,
>> - hotplug register block present, modern mode available, offset 0
>> defined as Cmd Data 2.)
> 
> Understood.
> 
> This patch is fine then.
> 
> Since the QEMU spec is RFC and not merged, I'm worried it might
> change. I'd rather review this patch again once the QEMU spec
> update is accepted/merged.

Igor's QEMU work is now upstream: 3e08b2b9cb64..3a61c8db9d25

Can you please re-check this patch? No changes should be necessary.

In particular,

- QEMU commit e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
field", 2020-01-22) covers the definition of QEMU_CPUHP_R_CMD_DATA2, in
this patch,

- QEMU commit ae340aa3d256 ("acpi: cpuhp: spec: add typical usecases",
2020-01-22), section "detect and enable modern CPU hotplug interface",
matches the QEMU_CPUHP_R_CMD_DATA2 usage in the next patch of this series.

Thanks!
Laszlo

> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 
>>>
>>>> +
>>>> +#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).
>>
>> You are looking at the legacy mode of the register block (documented
>> near the top of the "docs/specs/acpi_cpu_hotplug.txt" file).
>>
>> The operations for modern mode are in "hw/acpi/cpu.c", object
>> "cpu_hotplug_ops".
>>
>>
>>> 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
>>
>> Thanks!
>> Laszlo
>>
>>>
>>>> +
>>>> +#endif // QEMU_CPU_HOTPLUG_H_
>>>>
> 
> 
> 

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

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 1/24/20 12:40 PM, Laszlo Ersek wrote:
> On 10/24/19 17:12, Philippe Mathieu-Daudé wrote:
>> On 10/24/19 12:29 PM, Laszlo Ersek wrote:
>>> On 10/23/19 14:05, Philippe Mathieu-Daudé wrote:
>>>> 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].
>>>
>>> The documentation (spec) is being cleaned up, and also extended. Please
>>> see the following QEMU patch set (warning: long discussion):
>>>
>>>     [qemu-devel] [RFC 0/3] acpi: cphp: add CPHP_GET_CPU_ID_CMD command to
>>>                            cpu hotplug MMIO interface
>>>
>>>     http://mid.mail-archive.com/20191009132252.17860-1-imammedo@redhat.com
>>>
>>> In that qemu RFC series, the first two patches are of primary interest
>>> here, for this edk2 / OvmfPkg patch set. The first two patches will not
>>> extend the modern interface, they will just document it better (clean up
>>> the documentation for the existent behavior).
>>
>> OK I read it, but it is still RFC, so this patch shouldn't get merged
>> until the QEMU counterpart get merged, right?
>>
>>> If you look at the next patch in this series, you'll see that the
>>> OvmfPkg/PlatformPei code uses QEMU_CPUHP_R_CMD_DATA2 considering *both*
>>> situations, that is,
>>> - when the offset range [0..3] is marked as reserved for reading,
>>> - and when it is specified as Command Data 2 (by the last patch in the
>>> QEMU RFC series).
>>>
>>> Therefore, we could call this register
>>>
>>>     QEMU_CPUHP_R_RESERVED_0
>>>
>>> for now, but very soon we'd have to rename it to the above-proposed
>>>
>>>     QEMU_CPUHP_R_CMD_DATA2
>>>
>>> anyway. Again, the code considers both cases.
>>>
>>> (In fact, the code in the next patch considers *four* cases:
>>> - hotplug register block absent,
>>> - hotplug register block present, but only legacy mode is supported,
>>> - hotplug register block present, modern mode available, but offset 0 is
>>> still reserved for reading,
>>> - hotplug register block present, modern mode available, offset 0
>>> defined as Cmd Data 2.)
>>
>> Understood.
>>
>> This patch is fine then.
>>
>> Since the QEMU spec is RFC and not merged, I'm worried it might
>> change. I'd rather review this patch again once the QEMU spec
>> update is accepted/merged.
> 
> Igor's QEMU work is now upstream: 3e08b2b9cb64..3a61c8db9d25
> 
> Can you please re-check this patch? No changes should be necessary.
> 
> In particular,
> 
> - QEMU commit e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
> field", 2020-01-22) covers the definition of QEMU_CPUHP_R_CMD_DATA2, in
> this patch,
> 
> - QEMU commit ae340aa3d256 ("acpi: cpuhp: spec: add typical usecases",
> 2020-01-22), section "detect and enable modern CPU hotplug interface",
> matches the QEMU_CPUHP_R_CMD_DATA2 usage in the next patch of this series.

Re-checked correctly. Thanks for waiting/tracking it get merged.

My Reviewed-by stands.

>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>
>>>>
>>>>> +
>>>>> +#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).
>>>
>>> You are looking at the legacy mode of the register block (documented
>>> near the top of the "docs/specs/acpi_cpu_hotplug.txt" file).
>>>
>>> The operations for modern mode are in "hw/acpi/cpu.c", object
>>> "cpu_hotplug_ops".
>>>
>>>
>>>> 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
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>>
>>>>> +
>>>>> +#endif // QEMU_CPU_HOTPLUG_H_
>>>>>
>>
>> 
>>
> 

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

[Laszlo Ersek]

On 01/29/20 15:43, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 1/24/20 12:40 PM, Laszlo Ersek wrote:
>> On 10/24/19 17:12, Philippe Mathieu-Daudé wrote:

>>> This patch is fine then.
>>>
>>> Since the QEMU spec is RFC and not merged, I'm worried it might
>>> change. I'd rather review this patch again once the QEMU spec
>>> update is accepted/merged.
>>
>> Igor's QEMU work is now upstream: 3e08b2b9cb64..3a61c8db9d25
>>
>> Can you please re-check this patch? No changes should be necessary.
>>
>> In particular,
>>
>> - QEMU commit e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
>> field", 2020-01-22) covers the definition of QEMU_CPUHP_R_CMD_DATA2, in
>> this patch,
>>
>> - QEMU commit ae340aa3d256 ("acpi: cpuhp: spec: add typical usecases",
>> 2020-01-22), section "detect and enable modern CPU hotplug interface",
>> matches the QEMU_CPUHP_R_CMD_DATA2 usage in the next patch of this
>> series.
> 
> Re-checked correctly. Thanks for waiting/tracking it get merged.
> 
> My Reviewed-by stands.
> 
>>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thank you. I've also retested it now; it works.

Merged via <https://github.com/tianocore/edk2/pull/319>; commit range
c8b8157e126a..83357313dd67.

Thanks!
Laszlo

[PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Laszlo Ersek]

MaxCpuCountInitialization() currently handles the following options:

(1) QEMU does not report the boot CPU count (FW_CFG_NB_CPUS is 0)

    In this case, PlatformPei makes MpInitLib enumerate APs up to the
    default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
    the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
    (Whichever is reached first.)

    Time-limited AP enumeration had never been reliable on QEMU/KVM, which
    is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.

(2) QEMU reports the boot CPU count (FW_CFG_NB_CPUS is nonzero)

    In this case, PlatformPei sets

    - PcdCpuMaxLogicalProcessorNumber to the reported boot CPU count
      (FW_CFG_NB_CPUS, which exports "PCMachineState.boot_cpus"),

    - and PcdCpuApInitTimeOutInMicroSeconds to practically "infinity"
      (MAX_UINT32, ~71 minutes).

    That causes MpInitLib to enumerate exactly the present (boot) APs.

    With CPU hotplug in mind, this method is not good enough. Because,
    using QEMU terminology, UefiCpuPkg expects
    PcdCpuMaxLogicalProcessorNumber to provide the "possible CPUs" count
    ("MachineState.smp.max_cpus"), which includes present and not present
    CPUs both (with not present CPUs being subject for hot-plugging).
    FW_CFG_NB_CPUS does not include not present CPUs.

Rewrite MaxCpuCountInitialization() for handling the following cases:

(1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
    to values different from the defaults.)

(2) QEMU reports the boot CPU count ("PCMachineState.boot_cpus", via
    FW_CFG_NB_CPUS), but not the possible CPUs count
    ("MachineState.smp.max_cpus").

    In this case, the behavior remains unchanged.

    The way MpInitLib is instructed to do the same differs however: we now
    set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
    (while continuing to set PcdCpuMaxLogicalProcessorNumber identically).
    PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant.

(3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus", via
    FW_CFG_NB_CPUS), and the possible CPUs count
    ("MachineState.smp.max_cpus").

    We tell UefiCpuPkg about the possible CPUs count through
    PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
    count for precise and quick AP enumeration, via
    PcdCpuBootLogicalProcessorNumber. PcdCpuApInitTimeOutInMicroSeconds is
    irrelevant again.

This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
As a side effect, the patch also enables S3 to work with CPU hotplug at
once, *without* SMM_REQUIRE.

(Without the patch, S3 resume fails, if a CPU is hot-plugged at OS
runtime, prior to suspend: the FW_CFG_NB_CPUS increase seen during resume
causes PcdCpuMaxLogicalProcessorNumber to increase as well, which is not
permitted.

With the patch, PcdCpuMaxLogicalProcessorNumber stays the same, namely
"MachineState.smp.max_cpus". Therefore, the CPU structures allocated
during normal boot can accommodate the CPUs at S3 resume that have been
hotplugged prior to S3 suspend.)

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@arm.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    
    - use "possible CPUs" term in the code and the commit message [Igor]
    
    - add details about S3 to the commit message [Igor]
    
    - use QEMU's existent CPU hotplug register block, rather than a new
      named file in fw_cfg [Igor]
    
    - tested on QEMU v2.6.2 (legacy-only CPU hotplug register block), v2.7.1
      (modern register block, but buggy fw_cfg), v2.8.1.1 (no QEMU issues),
      v4.0.0 (no QEMU issues)

 OvmfPkg/OvmfPkgIa32.dsc             |   2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc          |   2 +-
 OvmfPkg/OvmfPkgX64.dsc              |   2 +-
 OvmfPkg/PlatformPei/PlatformPei.inf |   2 +-
 OvmfPkg/PlatformPei/Platform.c      | 172 +++++++++++++++++---
 5 files changed, 151 insertions(+), 29 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 4301e7821902..68d8a9fb9655 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -553,11 +553,11 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
 
   # Set memory encryption mask
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
 
 !if $(SMM_REQUIRE) == TRUE
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 803fd74ae8e4..e5a6260b6088 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -565,11 +565,11 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
 
   # Set memory encryption mask
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
 
 !if $(SMM_REQUIRE) == TRUE
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 5dbd1b793a90..f5d904945103 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -564,11 +564,11 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
 
   # Set memory encryption mask
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
 
 !if $(SMM_REQUIRE) == TRUE
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index d9fd9c8f05b3..30eaebdfae63 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -96,11 +96,11 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
   gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
 
 [FixedPcd]
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
 
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 3ba2459872d9..e5e8581752b5 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -28,11 +28,14 @@
 #include <Library/QemuFwCfgLib.h>
 #include <Library/QemuFwCfgS3Lib.h>
 #include <Library/ResourcePublicationLib.h>
 #include <Guid/MemoryTypeInformation.h>
 #include <Ppi/MasterBootMode.h>
+#include <IndustryStandard/I440FxPiix4.h>
 #include <IndustryStandard/Pci22.h>
+#include <IndustryStandard/Q35MchIch9.h>
+#include <IndustryStandard/QemuCpuHotplug.h>
 #include <OvmfPlatforms.h>
 
 #include "Platform.h"
 #include "Cmos.h"
 
@@ -562,47 +565,165 @@ S3Verification (
 #endif
 }
 
 
 /**
-  Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modules.
-  Set the mMaxCpuCount variable.
+  Fetch the boot CPU count and the possible CPU count from QEMU, and expose
+  them to UefiCpuPkg modules. Set the mMaxCpuCount variable.
 **/
 VOID
 MaxCpuCountInitialization (
   VOID
   )
 {
-  UINT16        ProcessorCount;
+  UINT16        BootCpuCount;
   RETURN_STATUS PcdStatus;
 
+  //
+  // Try to fetch the boot CPU count.
+  //
   QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount);
-  ProcessorCount = QemuFwCfgRead16 ();
-  //
-  // If the fw_cfg key or fw_cfg entirely is unavailable, load mMaxCpuCount
-  // from the PCD default. No change to PCDs.
-  //
-  if (ProcessorCount == 0) {
+  BootCpuCount = QemuFwCfgRead16 ();
+  if (BootCpuCount == 0) {
+    //
+    // QEMU doesn't report the boot CPU count. (BootCpuCount == 0) will let
+    // MpInitLib count APs up to (PcdCpuMaxLogicalProcessorNumber - 1), or
+    // until PcdCpuApInitTimeOutInMicroSeconds elapses (whichever is reached
+    // first).
+    //
+    DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n", __FUNCTION__));
     mMaxCpuCount = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
-    return;
+  } else {
+    //
+    // We will expose BootCpuCount to MpInitLib. MpInitLib will count APs up to
+    // (BootCpuCount - 1) precisely, regardless of timeout.
+    //
+    // Now try to fetch the possible CPU count.
+    //
+    UINTN CpuHpBase;
+    UINT32 CmdData2;
+
+    CpuHpBase = ((mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) ?
+                 ICH9_CPU_HOTPLUG_BASE : PIIX4_CPU_HOTPLUG_BASE);
+
+    //
+    // If only legacy mode is available in the CPU hotplug register block, or
+    // the register block is completely missing, then the writes below are
+    // no-ops.
+    //
+    // 1. Switch the hotplug register block to modern mode.
+    //
+    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
+    //
+    // 2. Select a valid CPU for deterministic reading of
+    //    QEMU_CPUHP_R_CMD_DATA2.
+    //
+    //    CPU#0 is always valid; it is the always present and non-removable
+    //    BSP.
+    //
+    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
+    //
+    // 3. Send a command after which QEMU_CPUHP_R_CMD_DATA2 is specified to
+    //    read as zero, and which does not invalidate the selector. (The
+    //    selector may change, but it must not become invalid.)
+    //
+    //    Send QEMU_CPUHP_CMD_GET_PENDING, as it will prove useful later.
+    //
+    IoWrite8 (CpuHpBase + QEMU_CPUHP_W_CMD, QEMU_CPUHP_CMD_GET_PENDING);
+    //
+    // 4. Read QEMU_CPUHP_R_CMD_DATA2.
+    //
+    //    If the register block is entirely missing, then this is an unassigned
+    //    IO read, returning all-bits-one.
+    //
+    //    If only legacy mode is available, then bit#0 stands for CPU#0 in the
+    //    "CPU present bitmap". CPU#0 is always present.
+    //
+    //    Otherwise, QEMU_CPUHP_R_CMD_DATA2 is either still reserved (returning
+    //    all-bits-zero), or it is specified to read as zero after the above
+    //    steps. Both cases confirm modern mode.
+    //
+    CmdData2 = IoRead32 (CpuHpBase + QEMU_CPUHP_R_CMD_DATA2);
+    DEBUG ((DEBUG_VERBOSE, "%a: CmdData2=0x%x\n", __FUNCTION__, CmdData2));
+    if (CmdData2 != 0) {
+      //
+      // QEMU doesn't support the modern CPU hotplug interface. Assume that the
+      // possible CPU count equals the boot CPU count (precluding hotplug).
+      //
+      DEBUG ((DEBUG_WARN, "%a: modern CPU hotplug interface unavailable\n",
+        __FUNCTION__));
+      mMaxCpuCount = BootCpuCount;
+    } else {
+      //
+      // Grab the possible CPU count from the modern CPU hotplug interface.
+      //
+      UINT32 Present, Possible, Selected;
+
+      Present = 0;
+      Possible = 0;
+
+      //
+      // We've sent QEMU_CPUHP_CMD_GET_PENDING last; this ensures
+      // QEMU_CPUHP_RW_CMD_DATA can now be read usefully. However,
+      // QEMU_CPUHP_CMD_GET_PENDING may have selected a CPU with actual pending
+      // hotplug events; therefore, select CPU#0 forcibly.
+      //
+      IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
+
+      do {
+        UINT8 CpuStatus;
+
+        //
+        // Read the status of the currently selected CPU. This will help with a
+        // sanity check against "BootCpuCount".
+        //
+        CpuStatus = IoRead8 (CpuHpBase + QEMU_CPUHP_R_CPU_STAT);
+        if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) != 0) {
+          ++Present;
+        }
+        //
+        // Attempt to select the next CPU.
+        //
+        ++Possible;
+        IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
+        //
+        // If the selection is successful, then the following read will return
+        // the selector (which we know is positive at this point). Otherwise,
+        // the read will return 0.
+        //
+        Selected = IoRead32 (CpuHpBase + QEMU_CPUHP_RW_CMD_DATA);
+        ASSERT (Selected == Possible || Selected == 0);
+      } while (Selected > 0);
+
+      //
+      // Sanity check: fw_cfg and the modern CPU hotplug interface should
+      // return the same boot CPU count.
+      //
+      if (BootCpuCount != Present) {
+        DEBUG ((DEBUG_WARN, "%a: QEMU v2.7 reset bug: BootCpuCount=%d "
+          "Present=%u\n", __FUNCTION__, BootCpuCount, Present));
+        //
+        // The handling of QemuFwCfgItemSmpCpuCount, across CPU hotplug plus
+        // platform reset (including S3), was corrected in QEMU commit
+        // e3cadac073a9 ("pc: fix FW_CFG_NB_CPUS to account for -device added
+        // CPUs", 2016-11-16), part of release v2.8.0.
+        //
+        BootCpuCount = (UINT16)Present;
+      }
+
+      mMaxCpuCount = Possible;
+    }
   }
-  //
-  // Otherwise, set mMaxCpuCount to the value reported by QEMU.
-  //
-  mMaxCpuCount = ProcessorCount;
-  //
-  // Additionally, tell UefiCpuPkg modules (a) the exact number of VCPUs, (b)
-  // to wait, in the initial AP bringup, exactly as long as it takes for all of
-  // the APs to report in. For this, we set the longest representable timeout
-  // (approx. 71 minutes).
-  //
-  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, ProcessorCount);
+
+  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n", __FUNCTION__,
+    BootCpuCount, mMaxCpuCount));
+  ASSERT (BootCpuCount <= mMaxCpuCount);
+
+  PcdStatus = PcdSet32S (PcdCpuBootLogicalProcessorNumber, BootCpuCount);
   ASSERT_RETURN_ERROR (PcdStatus);
-  PcdStatus = PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UINT32);
+  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCount);
   ASSERT_RETURN_ERROR (PcdStatus);
-  DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n", __FUNCTION__,
-    ProcessorCount));
 }
 
 
 /**
   Perform Platform PEI initialization.
@@ -636,17 +757,18 @@ InitializePlatform (
   }
 
   S3Verification ();
   BootModeInitialization ();
   AddressWidthInitialization ();
-  MaxCpuCountInitialization ();
 
   //
   // Query Host Bridge DID
   //
   mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
 
+  MaxCpuCountInitialization ();
+
   if (FeaturePcdGet (PcdSmmSmramRequire)) {
     Q35TsegMbytesInitialization ();
   }
 
   PublishPeiMemory ();
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Anthony PERARD]

On Wed, Oct 23, 2019 at 12:15:54AM +0200, Laszlo Ersek wrote:
> MaxCpuCountInitialization() currently handles the following options:
> 
> (1) QEMU does not report the boot CPU count (FW_CFG_NB_CPUS is 0)
> 
>     In this case, PlatformPei makes MpInitLib enumerate APs up to the
>     default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
>     the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
>     (Whichever is reached first.)
> 
>     Time-limited AP enumeration had never been reliable on QEMU/KVM, which
>     is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
> 
> (2) QEMU reports the boot CPU count (FW_CFG_NB_CPUS is nonzero)
> 
>     In this case, PlatformPei sets
> 
>     - PcdCpuMaxLogicalProcessorNumber to the reported boot CPU count
>       (FW_CFG_NB_CPUS, which exports "PCMachineState.boot_cpus"),
> 
>     - and PcdCpuApInitTimeOutInMicroSeconds to practically "infinity"
>       (MAX_UINT32, ~71 minutes).
> 
>     That causes MpInitLib to enumerate exactly the present (boot) APs.
> 
>     With CPU hotplug in mind, this method is not good enough. Because,
>     using QEMU terminology, UefiCpuPkg expects
>     PcdCpuMaxLogicalProcessorNumber to provide the "possible CPUs" count
>     ("MachineState.smp.max_cpus"), which includes present and not present
>     CPUs both (with not present CPUs being subject for hot-plugging).
>     FW_CFG_NB_CPUS does not include not present CPUs.
> 
> Rewrite MaxCpuCountInitialization() for handling the following cases:
> 
> (1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
>     to values different from the defaults.)
> 
> (2) QEMU reports the boot CPU count ("PCMachineState.boot_cpus", via
>     FW_CFG_NB_CPUS), but not the possible CPUs count
>     ("MachineState.smp.max_cpus").
> 
>     In this case, the behavior remains unchanged.
> 
>     The way MpInitLib is instructed to do the same differs however: we now
>     set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
>     (while continuing to set PcdCpuMaxLogicalProcessorNumber identically).
>     PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant.
> 
> (3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus", via
>     FW_CFG_NB_CPUS), and the possible CPUs count
>     ("MachineState.smp.max_cpus").
> 
>     We tell UefiCpuPkg about the possible CPUs count through
>     PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
>     count for precise and quick AP enumeration, via
>     PcdCpuBootLogicalProcessorNumber. PcdCpuApInitTimeOutInMicroSeconds is
>     irrelevant again.
> 
> This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
> As a side effect, the patch also enables S3 to work with CPU hotplug at
> once, *without* SMM_REQUIRE.
> 
> (Without the patch, S3 resume fails, if a CPU is hot-plugged at OS
> runtime, prior to suspend: the FW_CFG_NB_CPUS increase seen during resume
> causes PcdCpuMaxLogicalProcessorNumber to increase as well, which is not
> permitted.
> 
> With the patch, PcdCpuMaxLogicalProcessorNumber stays the same, namely
> "MachineState.smp.max_cpus". Therefore, the CPU structures allocated
> during normal boot can accommodate the CPUs at S3 resume that have been
> hotplugged prior to S3 suspend.)
> 
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Xen falls into case (1) and OVMF still boots fine with the series
applied.

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Laszlo Ersek]

On 10/24/19 16:27, Anthony PERARD wrote:
> On Wed, Oct 23, 2019 at 12:15:54AM +0200, Laszlo Ersek wrote:
>> MaxCpuCountInitialization() currently handles the following options:
>>
>> (1) QEMU does not report the boot CPU count (FW_CFG_NB_CPUS is 0)
>>
>>     In this case, PlatformPei makes MpInitLib enumerate APs up to the
>>     default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
>>     the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
>>     (Whichever is reached first.)
>>
>>     Time-limited AP enumeration had never been reliable on QEMU/KVM, which
>>     is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
>>
>> (2) QEMU reports the boot CPU count (FW_CFG_NB_CPUS is nonzero)
>>
>>     In this case, PlatformPei sets
>>
>>     - PcdCpuMaxLogicalProcessorNumber to the reported boot CPU count
>>       (FW_CFG_NB_CPUS, which exports "PCMachineState.boot_cpus"),
>>
>>     - and PcdCpuApInitTimeOutInMicroSeconds to practically "infinity"
>>       (MAX_UINT32, ~71 minutes).
>>
>>     That causes MpInitLib to enumerate exactly the present (boot) APs.
>>
>>     With CPU hotplug in mind, this method is not good enough. Because,
>>     using QEMU terminology, UefiCpuPkg expects
>>     PcdCpuMaxLogicalProcessorNumber to provide the "possible CPUs" count
>>     ("MachineState.smp.max_cpus"), which includes present and not present
>>     CPUs both (with not present CPUs being subject for hot-plugging).
>>     FW_CFG_NB_CPUS does not include not present CPUs.
>>
>> Rewrite MaxCpuCountInitialization() for handling the following cases:
>>
>> (1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
>>     to values different from the defaults.)
>>
>> (2) QEMU reports the boot CPU count ("PCMachineState.boot_cpus", via
>>     FW_CFG_NB_CPUS), but not the possible CPUs count
>>     ("MachineState.smp.max_cpus").
>>
>>     In this case, the behavior remains unchanged.
>>
>>     The way MpInitLib is instructed to do the same differs however: we now
>>     set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
>>     (while continuing to set PcdCpuMaxLogicalProcessorNumber identically).
>>     PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant.
>>
>> (3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus", via
>>     FW_CFG_NB_CPUS), and the possible CPUs count
>>     ("MachineState.smp.max_cpus").
>>
>>     We tell UefiCpuPkg about the possible CPUs count through
>>     PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
>>     count for precise and quick AP enumeration, via
>>     PcdCpuBootLogicalProcessorNumber. PcdCpuApInitTimeOutInMicroSeconds is
>>     irrelevant again.
>>
>> This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
>> As a side effect, the patch also enables S3 to work with CPU hotplug at
>> once, *without* SMM_REQUIRE.
>>
>> (Without the patch, S3 resume fails, if a CPU is hot-plugged at OS
>> runtime, prior to suspend: the FW_CFG_NB_CPUS increase seen during resume
>> causes PcdCpuMaxLogicalProcessorNumber to increase as well, which is not
>> permitted.
>>
>> With the patch, PcdCpuMaxLogicalProcessorNumber stays the same, namely
>> "MachineState.smp.max_cpus". Therefore, the CPU structures allocated
>> during normal boot can accommodate the CPUs at S3 resume that have been
>> hotplugged prior to S3 suspend.)
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> Xen falls into case (1) and OVMF still boots fine with the series
> applied.

Awesome, thank you very much for checking!
Laszlo

Re: [edk2-devel] [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Philippe Mathieu-Daudé]

On 10/23/19 12:15 AM, Laszlo Ersek wrote:
> MaxCpuCountInitialization() currently handles the following options:
> 
> (1) QEMU does not report the boot CPU count (FW_CFG_NB_CPUS is 0)
> 
>      In this case, PlatformPei makes MpInitLib enumerate APs up to the
>      default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
>      the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
>      (Whichever is reached first.)
> 
>      Time-limited AP enumeration had never been reliable on QEMU/KVM, which
>      is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
> 
> (2) QEMU reports the boot CPU count (FW_CFG_NB_CPUS is nonzero)
> 
>      In this case, PlatformPei sets
> 
>      - PcdCpuMaxLogicalProcessorNumber to the reported boot CPU count
>        (FW_CFG_NB_CPUS, which exports "PCMachineState.boot_cpus"),
> 
>      - and PcdCpuApInitTimeOutInMicroSeconds to practically "infinity"
>        (MAX_UINT32, ~71 minutes).
> 
>      That causes MpInitLib to enumerate exactly the present (boot) APs.
> 
>      With CPU hotplug in mind, this method is not good enough. Because,
>      using QEMU terminology, UefiCpuPkg expects
>      PcdCpuMaxLogicalProcessorNumber to provide the "possible CPUs" count
>      ("MachineState.smp.max_cpus"), which includes present and not present
>      CPUs both (with not present CPUs being subject for hot-plugging).
>      FW_CFG_NB_CPUS does not include not present CPUs.
> 
> Rewrite MaxCpuCountInitialization() for handling the following cases:
> 
> (1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
>      to values different from the defaults.)
> 
> (2) QEMU reports the boot CPU count ("PCMachineState.boot_cpus", via
>      FW_CFG_NB_CPUS), but not the possible CPUs count
>      ("MachineState.smp.max_cpus").
> 
>      In this case, the behavior remains unchanged.
> 
>      The way MpInitLib is instructed to do the same differs however: we now
>      set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
>      (while continuing to set PcdCpuMaxLogicalProcessorNumber identically).
>      PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant.
> 
> (3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus", via
>      FW_CFG_NB_CPUS), and the possible CPUs count
>      ("MachineState.smp.max_cpus").
> 
>      We tell UefiCpuPkg about the possible CPUs count through
>      PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
>      count for precise and quick AP enumeration, via
>      PcdCpuBootLogicalProcessorNumber. PcdCpuApInitTimeOutInMicroSeconds is
>      irrelevant again.
> 
> This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
> As a side effect, the patch also enables S3 to work with CPU hotplug at
> once, *without* SMM_REQUIRE.
> 
> (Without the patch, S3 resume fails, if a CPU is hot-plugged at OS
> runtime, prior to suspend: the FW_CFG_NB_CPUS increase seen during resume
> causes PcdCpuMaxLogicalProcessorNumber to increase as well, which is not
> permitted.
> 
> With the patch, PcdCpuMaxLogicalProcessorNumber stays the same, namely
> "MachineState.smp.max_cpus". Therefore, the CPU structures allocated
> during normal boot can accommodate the CPUs at S3 resume that have been
> hotplugged prior to S3 suspend.)
> 
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>      v2:
>      
>      - use "possible CPUs" term in the code and the commit message [Igor]
>      
>      - add details about S3 to the commit message [Igor]
>      
>      - use QEMU's existent CPU hotplug register block, rather than a new
>        named file in fw_cfg [Igor]
>      
>      - tested on QEMU v2.6.2 (legacy-only CPU hotplug register block), v2.7.1
>        (modern register block, but buggy fw_cfg), v2.8.1.1 (no QEMU issues),
>        v4.0.0 (no QEMU issues)
> 
>   OvmfPkg/OvmfPkgIa32.dsc             |   2 +-
>   OvmfPkg/OvmfPkgIa32X64.dsc          |   2 +-
>   OvmfPkg/OvmfPkgX64.dsc              |   2 +-
>   OvmfPkg/PlatformPei/PlatformPei.inf |   2 +-
>   OvmfPkg/PlatformPei/Platform.c      | 172 +++++++++++++++++---
>   5 files changed, 151 insertions(+), 29 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 4301e7821902..68d8a9fb9655 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -553,11 +553,11 @@ [PcdsDynamicDefault]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>   
>     # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
>     gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>   
>     # Set memory encryption mask
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>   
>   !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 803fd74ae8e4..e5a6260b6088 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -565,11 +565,11 @@ [PcdsDynamicDefault]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>   
>     # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
>     gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>   
>     # Set memory encryption mask
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>   
>   !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 5dbd1b793a90..f5d904945103 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -564,11 +564,11 @@ [PcdsDynamicDefault]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>   
>     # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
>     gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>   
>     # Set memory encryption mask
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>   
>   !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index d9fd9c8f05b3..30eaebdfae63 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -96,11 +96,11 @@ [Pcd]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
>     gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
>     gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
>     gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>   
>   [FixedPcd]
>     gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>   
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 3ba2459872d9..e5e8581752b5 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -28,11 +28,14 @@
>   #include <Library/QemuFwCfgLib.h>
>   #include <Library/QemuFwCfgS3Lib.h>
>   #include <Library/ResourcePublicationLib.h>
>   #include <Guid/MemoryTypeInformation.h>
>   #include <Ppi/MasterBootMode.h>
> +#include <IndustryStandard/I440FxPiix4.h>
>   #include <IndustryStandard/Pci22.h>
> +#include <IndustryStandard/Q35MchIch9.h>
> +#include <IndustryStandard/QemuCpuHotplug.h>
>   #include <OvmfPlatforms.h>
>   
>   #include "Platform.h"
>   #include "Cmos.h"
>   
> @@ -562,47 +565,165 @@ S3Verification (
>   #endif
>   }
>   
>   
>   /**
> -  Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modules.
> -  Set the mMaxCpuCount variable.
> +  Fetch the boot CPU count and the possible CPU count from QEMU, and expose
> +  them to UefiCpuPkg modules. Set the mMaxCpuCount variable.
>   **/
>   VOID
>   MaxCpuCountInitialization (
>     VOID
>     )
>   {
> -  UINT16        ProcessorCount;
> +  UINT16        BootCpuCount;
>     RETURN_STATUS PcdStatus;
>   
> +  //
> +  // Try to fetch the boot CPU count.
> +  //
>     QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount);
> -  ProcessorCount = QemuFwCfgRead16 ();
> -  //
> -  // If the fw_cfg key or fw_cfg entirely is unavailable, load mMaxCpuCount
> -  // from the PCD default. No change to PCDs.
> -  //
> -  if (ProcessorCount == 0) {
> +  BootCpuCount = QemuFwCfgRead16 ();
> +  if (BootCpuCount == 0) {
> +    //
> +    // QEMU doesn't report the boot CPU count. (BootCpuCount == 0) will let
> +    // MpInitLib count APs up to (PcdCpuMaxLogicalProcessorNumber - 1), or
> +    // until PcdCpuApInitTimeOutInMicroSeconds elapses (whichever is reached
> +    // first).
> +    //
> +    DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n", __FUNCTION__));
>       mMaxCpuCount = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> -    return;
> +  } else {
> +    //
> +    // We will expose BootCpuCount to MpInitLib. MpInitLib will count APs up to
> +    // (BootCpuCount - 1) precisely, regardless of timeout.
> +    //
> +    // Now try to fetch the possible CPU count.
> +    //
> +    UINTN CpuHpBase;
> +    UINT32 CmdData2;
> +
> +    CpuHpBase = ((mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) ?
> +                 ICH9_CPU_HOTPLUG_BASE : PIIX4_CPU_HOTPLUG_BASE);
> +
> +    //
> +    // If only legacy mode is available in the CPU hotplug register block, or
> +    // the register block is completely missing, then the writes below are
> +    // no-ops.
> +    //
> +    // 1. Switch the hotplug register block to modern mode.
> +    //
> +    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
> +    //
> +    // 2. Select a valid CPU for deterministic reading of
> +    //    QEMU_CPUHP_R_CMD_DATA2.
> +    //
> +    //    CPU#0 is always valid; it is the always present and non-removable
> +    //    BSP.
> +    //
> +    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
> +    //
> +    // 3. Send a command after which QEMU_CPUHP_R_CMD_DATA2 is specified to
> +    //    read as zero, and which does not invalidate the selector. (The
> +    //    selector may change, but it must not become invalid.)
> +    //
> +    //    Send QEMU_CPUHP_CMD_GET_PENDING, as it will prove useful later.
> +    //
> +    IoWrite8 (CpuHpBase + QEMU_CPUHP_W_CMD, QEMU_CPUHP_CMD_GET_PENDING);
> +    //
> +    // 4. Read QEMU_CPUHP_R_CMD_DATA2.
> +    //
> +    //    If the register block is entirely missing, then this is an unassigned
> +    //    IO read, returning all-bits-one.
> +    //
> +    //    If only legacy mode is available, then bit#0 stands for CPU#0 in the
> +    //    "CPU present bitmap". CPU#0 is always present.
> +    //
> +    //    Otherwise, QEMU_CPUHP_R_CMD_DATA2 is either still reserved (returning
> +    //    all-bits-zero), or it is specified to read as zero after the above
> +    //    steps. Both cases confirm modern mode.
> +    //
> +    CmdData2 = IoRead32 (CpuHpBase + QEMU_CPUHP_R_CMD_DATA2);
> +    DEBUG ((DEBUG_VERBOSE, "%a: CmdData2=0x%x\n", __FUNCTION__, CmdData2));
> +    if (CmdData2 != 0) {
> +      //
> +      // QEMU doesn't support the modern CPU hotplug interface. Assume that the
> +      // possible CPU count equals the boot CPU count (precluding hotplug).
> +      //
> +      DEBUG ((DEBUG_WARN, "%a: modern CPU hotplug interface unavailable\n",
> +        __FUNCTION__));
> +      mMaxCpuCount = BootCpuCount;
> +    } else {
> +      //
> +      // Grab the possible CPU count from the modern CPU hotplug interface.
> +      //
> +      UINT32 Present, Possible, Selected;
> +
> +      Present = 0;
> +      Possible = 0;
> +
> +      //
> +      // We've sent QEMU_CPUHP_CMD_GET_PENDING last; this ensures
> +      // QEMU_CPUHP_RW_CMD_DATA can now be read usefully. However,
> +      // QEMU_CPUHP_CMD_GET_PENDING may have selected a CPU with actual pending
> +      // hotplug events; therefore, select CPU#0 forcibly.
> +      //
> +      IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);

Nitpicking, I find this easier to read (matches the comment):

          IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);

Rest very well documented, thanks.

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

> +
> +      do {
> +        UINT8 CpuStatus;
> +
> +        //
> +        // Read the status of the currently selected CPU. This will help with a
> +        // sanity check against "BootCpuCount".
> +        //
> +        CpuStatus = IoRead8 (CpuHpBase + QEMU_CPUHP_R_CPU_STAT);
> +        if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) != 0) {
> +          ++Present;
> +        }
> +        //
> +        // Attempt to select the next CPU.
> +        //
> +        ++Possible;
> +        IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
> +        //
> +        // If the selection is successful, then the following read will return
> +        // the selector (which we know is positive at this point). Otherwise,
> +        // the read will return 0.
> +        //
> +        Selected = IoRead32 (CpuHpBase + QEMU_CPUHP_RW_CMD_DATA);
> +        ASSERT (Selected == Possible || Selected == 0);
> +      } while (Selected > 0);
> +
> +      //
> +      // Sanity check: fw_cfg and the modern CPU hotplug interface should
> +      // return the same boot CPU count.
> +      //
> +      if (BootCpuCount != Present) {
> +        DEBUG ((DEBUG_WARN, "%a: QEMU v2.7 reset bug: BootCpuCount=%d "
> +          "Present=%u\n", __FUNCTION__, BootCpuCount, Present));
> +        //
> +        // The handling of QemuFwCfgItemSmpCpuCount, across CPU hotplug plus
> +        // platform reset (including S3), was corrected in QEMU commit
> +        // e3cadac073a9 ("pc: fix FW_CFG_NB_CPUS to account for -device added
> +        // CPUs", 2016-11-16), part of release v2.8.0.
> +        //
> +        BootCpuCount = (UINT16)Present;
> +      }
> +
> +      mMaxCpuCount = Possible;
> +    }
>     }
> -  //
> -  // Otherwise, set mMaxCpuCount to the value reported by QEMU.
> -  //
> -  mMaxCpuCount = ProcessorCount;
> -  //
> -  // Additionally, tell UefiCpuPkg modules (a) the exact number of VCPUs, (b)
> -  // to wait, in the initial AP bringup, exactly as long as it takes for all of
> -  // the APs to report in. For this, we set the longest representable timeout
> -  // (approx. 71 minutes).
> -  //
> -  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, ProcessorCount);
> +
> +  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n", __FUNCTION__,
> +    BootCpuCount, mMaxCpuCount));
> +  ASSERT (BootCpuCount <= mMaxCpuCount);
> +
> +  PcdStatus = PcdSet32S (PcdCpuBootLogicalProcessorNumber, BootCpuCount);
>     ASSERT_RETURN_ERROR (PcdStatus);
> -  PcdStatus = PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UINT32);
> +  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCount);
>     ASSERT_RETURN_ERROR (PcdStatus);
> -  DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n", __FUNCTION__,
> -    ProcessorCount));
>   }
>   
>   
>   /**
>     Perform Platform PEI initialization.
> @@ -636,17 +757,18 @@ InitializePlatform (
>     }
>   
>     S3Verification ();
>     BootModeInitialization ();
>     AddressWidthInitialization ();
> -  MaxCpuCountInitialization ();
>   
>     //
>     // Query Host Bridge DID
>     //
>     mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
>   
> +  MaxCpuCountInitialization ();
> +
>     if (FeaturePcdGet (PcdSmmSmramRequire)) {
>       Q35TsegMbytesInitialization ();
>     }
>   
>     PublishPeiMemory ();
> 

Re: [edk2-devel] [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Laszlo Ersek]

On 10/24/19 17:33, Philippe Mathieu-Daudé wrote:
> On 10/23/19 12:15 AM, Laszlo Ersek wrote:
>> MaxCpuCountInitialization() currently handles the following options:
>>
>> (1) QEMU does not report the boot CPU count (FW_CFG_NB_CPUS is 0)
>>
>>      In this case, PlatformPei makes MpInitLib enumerate APs up to the
>>      default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
>>      the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
>>      (Whichever is reached first.)
>>
>>      Time-limited AP enumeration had never been reliable on QEMU/KVM,
>> which
>>      is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
>>
>> (2) QEMU reports the boot CPU count (FW_CFG_NB_CPUS is nonzero)
>>
>>      In this case, PlatformPei sets
>>
>>      - PcdCpuMaxLogicalProcessorNumber to the reported boot CPU count
>>        (FW_CFG_NB_CPUS, which exports "PCMachineState.boot_cpus"),
>>
>>      - and PcdCpuApInitTimeOutInMicroSeconds to practically "infinity"
>>        (MAX_UINT32, ~71 minutes).
>>
>>      That causes MpInitLib to enumerate exactly the present (boot) APs.
>>
>>      With CPU hotplug in mind, this method is not good enough. Because,
>>      using QEMU terminology, UefiCpuPkg expects
>>      PcdCpuMaxLogicalProcessorNumber to provide the "possible CPUs" count
>>      ("MachineState.smp.max_cpus"), which includes present and not
>> present
>>      CPUs both (with not present CPUs being subject for hot-plugging).
>>      FW_CFG_NB_CPUS does not include not present CPUs.
>>
>> Rewrite MaxCpuCountInitialization() for handling the following cases:
>>
>> (1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
>>      to values different from the defaults.)
>>
>> (2) QEMU reports the boot CPU count ("PCMachineState.boot_cpus", via
>>      FW_CFG_NB_CPUS), but not the possible CPUs count
>>      ("MachineState.smp.max_cpus").
>>
>>      In this case, the behavior remains unchanged.
>>
>>      The way MpInitLib is instructed to do the same differs however:
>> we now
>>      set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
>>      (while continuing to set PcdCpuMaxLogicalProcessorNumber
>> identically).
>>      PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant.
>>
>> (3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus", via
>>      FW_CFG_NB_CPUS), and the possible CPUs count
>>      ("MachineState.smp.max_cpus").
>>
>>      We tell UefiCpuPkg about the possible CPUs count through
>>      PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
>>      count for precise and quick AP enumeration, via
>>      PcdCpuBootLogicalProcessorNumber.
>> PcdCpuApInitTimeOutInMicroSeconds is
>>      irrelevant again.
>>
>> This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
>> As a side effect, the patch also enables S3 to work with CPU hotplug at
>> once, *without* SMM_REQUIRE.
>>
>> (Without the patch, S3 resume fails, if a CPU is hot-plugged at OS
>> runtime, prior to suspend: the FW_CFG_NB_CPUS increase seen during resume
>> causes PcdCpuMaxLogicalProcessorNumber to increase as well, which is not
>> permitted.
>>
>> With the patch, PcdCpuMaxLogicalProcessorNumber stays the same, namely
>> "MachineState.smp.max_cpus". Therefore, the CPU structures allocated
>> during normal boot can accommodate the CPUs at S3 resume that have been
>> hotplugged prior to S3 suspend.)
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>      v2:
>>           - use "possible CPUs" term in the code and the commit
>> message [Igor]
>>           - add details about S3 to the commit message [Igor]
>>           - use QEMU's existent CPU hotplug register block, rather
>> than a new
>>        named file in fw_cfg [Igor]
>>           - tested on QEMU v2.6.2 (legacy-only CPU hotplug register
>> block), v2.7.1
>>        (modern register block, but buggy fw_cfg), v2.8.1.1 (no QEMU
>> issues),
>>        v4.0.0 (no QEMU issues)
>>
>>   OvmfPkg/OvmfPkgIa32.dsc             |   2 +-
>>   OvmfPkg/OvmfPkgIa32X64.dsc          |   2 +-
>>   OvmfPkg/OvmfPkgX64.dsc              |   2 +-
>>   OvmfPkg/PlatformPei/PlatformPei.inf |   2 +-
>>   OvmfPkg/PlatformPei/Platform.c      | 172 +++++++++++++++++---
>>   5 files changed, 151 insertions(+), 29 deletions(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 4301e7821902..68d8a9fb9655 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -553,11 +553,11 @@ [PcdsDynamicDefault]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>>       # UefiCpuPkg PCDs related to initial AP bringup and general AP
>> management.
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>>       # Set memory encryption mask
>>    
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>>     !if $(SMM_REQUIRE) == TRUE
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 803fd74ae8e4..e5a6260b6088 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -565,11 +565,11 @@ [PcdsDynamicDefault]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>>       # UefiCpuPkg PCDs related to initial AP bringup and general AP
>> management.
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>>       # Set memory encryption mask
>>    
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>>     !if $(SMM_REQUIRE) == TRUE
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 5dbd1b793a90..f5d904945103 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -564,11 +564,11 @@ [PcdsDynamicDefault]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>>       # UefiCpuPkg PCDs related to initial AP bringup and general AP
>> management.
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>>       # Set memory encryption mask
>>    
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>>     !if $(SMM_REQUIRE) == TRUE
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index d9fd9c8f05b3..30eaebdfae63 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -96,11 +96,11 @@ [Pcd]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>>     gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>>     [FixedPcd]
>>     gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>   diff --git a/OvmfPkg/PlatformPei/Platform.c
>> b/OvmfPkg/PlatformPei/Platform.c
>> index 3ba2459872d9..e5e8581752b5 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -28,11 +28,14 @@
>>   #include <Library/QemuFwCfgLib.h>
>>   #include <Library/QemuFwCfgS3Lib.h>
>>   #include <Library/ResourcePublicationLib.h>
>>   #include <Guid/MemoryTypeInformation.h>
>>   #include <Ppi/MasterBootMode.h>
>> +#include <IndustryStandard/I440FxPiix4.h>
>>   #include <IndustryStandard/Pci22.h>
>> +#include <IndustryStandard/Q35MchIch9.h>
>> +#include <IndustryStandard/QemuCpuHotplug.h>
>>   #include <OvmfPlatforms.h>
>>     #include "Platform.h"
>>   #include "Cmos.h"
>>   @@ -562,47 +565,165 @@ S3Verification (
>>   #endif
>>   }
>>       /**
>> -  Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg
>> modules.
>> -  Set the mMaxCpuCount variable.
>> +  Fetch the boot CPU count and the possible CPU count from QEMU, and
>> expose
>> +  them to UefiCpuPkg modules. Set the mMaxCpuCount variable.
>>   **/
>>   VOID
>>   MaxCpuCountInitialization (
>>     VOID
>>     )
>>   {
>> -  UINT16        ProcessorCount;
>> +  UINT16        BootCpuCount;
>>     RETURN_STATUS PcdStatus;
>>   +  //
>> +  // Try to fetch the boot CPU count.
>> +  //
>>     QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount);
>> -  ProcessorCount = QemuFwCfgRead16 ();
>> -  //
>> -  // If the fw_cfg key or fw_cfg entirely is unavailable, load
>> mMaxCpuCount
>> -  // from the PCD default. No change to PCDs.
>> -  //
>> -  if (ProcessorCount == 0) {
>> +  BootCpuCount = QemuFwCfgRead16 ();
>> +  if (BootCpuCount == 0) {
>> +    //
>> +    // QEMU doesn't report the boot CPU count. (BootCpuCount == 0)
>> will let
>> +    // MpInitLib count APs up to (PcdCpuMaxLogicalProcessorNumber -
>> 1), or
>> +    // until PcdCpuApInitTimeOutInMicroSeconds elapses (whichever is
>> reached
>> +    // first).
>> +    //
>> +    DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n",
>> __FUNCTION__));
>>       mMaxCpuCount = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>> -    return;
>> +  } else {
>> +    //
>> +    // We will expose BootCpuCount to MpInitLib. MpInitLib will count
>> APs up to
>> +    // (BootCpuCount - 1) precisely, regardless of timeout.
>> +    //
>> +    // Now try to fetch the possible CPU count.
>> +    //
>> +    UINTN CpuHpBase;
>> +    UINT32 CmdData2;
>> +
>> +    CpuHpBase = ((mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) ?
>> +                 ICH9_CPU_HOTPLUG_BASE : PIIX4_CPU_HOTPLUG_BASE);
>> +
>> +    //
>> +    // If only legacy mode is available in the CPU hotplug register
>> block, or
>> +    // the register block is completely missing, then the writes
>> below are
>> +    // no-ops.
>> +    //
>> +    // 1. Switch the hotplug register block to modern mode.
>> +    //
>> +    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
>> +    //
>> +    // 2. Select a valid CPU for deterministic reading of
>> +    //    QEMU_CPUHP_R_CMD_DATA2.
>> +    //
>> +    //    CPU#0 is always valid; it is the always present and
>> non-removable
>> +    //    BSP.
>> +    //
>> +    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
>> +    //
>> +    // 3. Send a command after which QEMU_CPUHP_R_CMD_DATA2 is
>> specified to
>> +    //    read as zero, and which does not invalidate the selector. (The
>> +    //    selector may change, but it must not become invalid.)
>> +    //
>> +    //    Send QEMU_CPUHP_CMD_GET_PENDING, as it will prove useful
>> later.
>> +    //
>> +    IoWrite8 (CpuHpBase + QEMU_CPUHP_W_CMD, QEMU_CPUHP_CMD_GET_PENDING);
>> +    //
>> +    // 4. Read QEMU_CPUHP_R_CMD_DATA2.
>> +    //
>> +    //    If the register block is entirely missing, then this is an
>> unassigned
>> +    //    IO read, returning all-bits-one.
>> +    //
>> +    //    If only legacy mode is available, then bit#0 stands for
>> CPU#0 in the
>> +    //    "CPU present bitmap". CPU#0 is always present.
>> +    //
>> +    //    Otherwise, QEMU_CPUHP_R_CMD_DATA2 is either still reserved
>> (returning
>> +    //    all-bits-zero), or it is specified to read as zero after
>> the above
>> +    //    steps. Both cases confirm modern mode.
>> +    //
>> +    CmdData2 = IoRead32 (CpuHpBase + QEMU_CPUHP_R_CMD_DATA2);
>> +    DEBUG ((DEBUG_VERBOSE, "%a: CmdData2=0x%x\n", __FUNCTION__,
>> CmdData2));
>> +    if (CmdData2 != 0) {
>> +      //
>> +      // QEMU doesn't support the modern CPU hotplug interface.
>> Assume that the
>> +      // possible CPU count equals the boot CPU count (precluding
>> hotplug).
>> +      //
>> +      DEBUG ((DEBUG_WARN, "%a: modern CPU hotplug interface
>> unavailable\n",
>> +        __FUNCTION__));
>> +      mMaxCpuCount = BootCpuCount;
>> +    } else {
>> +      //
>> +      // Grab the possible CPU count from the modern CPU hotplug
>> interface.
>> +      //
>> +      UINT32 Present, Possible, Selected;
>> +
>> +      Present = 0;
>> +      Possible = 0;
>> +
>> +      //
>> +      // We've sent QEMU_CPUHP_CMD_GET_PENDING last; this ensures
>> +      // QEMU_CPUHP_RW_CMD_DATA can now be read usefully. However,
>> +      // QEMU_CPUHP_CMD_GET_PENDING may have selected a CPU with
>> actual pending
>> +      // hotplug events; therefore, select CPU#0 forcibly.
>> +      //
>> +      IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
> 
> Nitpicking, I find this easier to read (matches the comment):
> 
>          IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);

I considered that, but I chose this form consciously, in the end.
Namely, if you search this patch for

  IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);

you will find that there are two instances of it: one before the loop,
and one inside the loop. I wanted to make those operations *look*
identical as well, not just *be* semantically identical.

The same goal prevented me from changing something else too: namely, in
the loop body, the following:

        ++Possible;
        IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);

could be condensed as:

        IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, ++Possible);

But, that would again break the syntactical identity. And, again, for
some reason I found it helpful to keep those two function calls
syntactically identical.


If you think the above effort doesn't actually improve readability, then
I'm happy to respin with *both* changes at the same time (i.e., pass
constant 0 in the first location, and pass "++Possible" in the second
location). I don't think that implementing either change *in isolation*
would be good.

Thanks!
Laszlo

> 
> Rest very well documented, thanks.
> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 
>> +
>> +      do {
>> +        UINT8 CpuStatus;
>> +
>> +        //
>> +        // Read the status of the currently selected CPU. This will
>> help with a
>> +        // sanity check against "BootCpuCount".
>> +        //
>> +        CpuStatus = IoRead8 (CpuHpBase + QEMU_CPUHP_R_CPU_STAT);
>> +        if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) != 0) {
>> +          ++Present;
>> +        }
>> +        //
>> +        // Attempt to select the next CPU.
>> +        //
>> +        ++Possible;
>> +        IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
>> +        //
>> +        // If the selection is successful, then the following read
>> will return
>> +        // the selector (which we know is positive at this point).
>> Otherwise,
>> +        // the read will return 0.
>> +        //
>> +        Selected = IoRead32 (CpuHpBase + QEMU_CPUHP_RW_CMD_DATA);
>> +        ASSERT (Selected == Possible || Selected == 0);
>> +      } while (Selected > 0);
>> +
>> +      //
>> +      // Sanity check: fw_cfg and the modern CPU hotplug interface
>> should
>> +      // return the same boot CPU count.
>> +      //
>> +      if (BootCpuCount != Present) {
>> +        DEBUG ((DEBUG_WARN, "%a: QEMU v2.7 reset bug: BootCpuCount=%d "
>> +          "Present=%u\n", __FUNCTION__, BootCpuCount, Present));
>> +        //
>> +        // The handling of QemuFwCfgItemSmpCpuCount, across CPU
>> hotplug plus
>> +        // platform reset (including S3), was corrected in QEMU commit
>> +        // e3cadac073a9 ("pc: fix FW_CFG_NB_CPUS to account for
>> -device added
>> +        // CPUs", 2016-11-16), part of release v2.8.0.
>> +        //
>> +        BootCpuCount = (UINT16)Present;
>> +      }
>> +
>> +      mMaxCpuCount = Possible;
>> +    }
>>     }
>> -  //
>> -  // Otherwise, set mMaxCpuCount to the value reported by QEMU.
>> -  //
>> -  mMaxCpuCount = ProcessorCount;
>> -  //
>> -  // Additionally, tell UefiCpuPkg modules (a) the exact number of
>> VCPUs, (b)
>> -  // to wait, in the initial AP bringup, exactly as long as it takes
>> for all of
>> -  // the APs to report in. For this, we set the longest representable
>> timeout
>> -  // (approx. 71 minutes).
>> -  //
>> -  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber,
>> ProcessorCount);
>> +
>> +  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n",
>> __FUNCTION__,
>> +    BootCpuCount, mMaxCpuCount));
>> +  ASSERT (BootCpuCount <= mMaxCpuCount);
>> +
>> +  PcdStatus = PcdSet32S (PcdCpuBootLogicalProcessorNumber,
>> BootCpuCount);
>>     ASSERT_RETURN_ERROR (PcdStatus);
>> -  PcdStatus = PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UINT32);
>> +  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCount);
>>     ASSERT_RETURN_ERROR (PcdStatus);
>> -  DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n",
>> __FUNCTION__,
>> -    ProcessorCount));
>>   }
>>       /**
>>     Perform Platform PEI initialization.
>> @@ -636,17 +757,18 @@ InitializePlatform (
>>     }
>>       S3Verification ();
>>     BootModeInitialization ();
>>     AddressWidthInitialization ();
>> -  MaxCpuCountInitialization ();
>>       //
>>     // Query Host Bridge DID
>>     //
>>     mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
>>   +  MaxCpuCountInitialization ();
>> +
>>     if (FeaturePcdGet (PcdSmmSmramRequire)) {
>>       Q35TsegMbytesInitialization ();
>>     }
>>       PublishPeiMemory ();
>>

Re: [edk2-devel] [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Philippe Mathieu-Daudé]

On 10/25/19 10:29 AM, Laszlo Ersek wrote:
> On 10/24/19 17:33, Philippe Mathieu-Daudé wrote:
>> On 10/23/19 12:15 AM, Laszlo Ersek wrote:
>>> MaxCpuCountInitialization() currently handles the following options:
>>>
>>> (1) QEMU does not report the boot CPU count (FW_CFG_NB_CPUS is 0)
>>>
>>>       In this case, PlatformPei makes MpInitLib enumerate APs up to the
>>>       default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
>>>       the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
>>>       (Whichever is reached first.)
>>>
>>>       Time-limited AP enumeration had never been reliable on QEMU/KVM,
>>> which
>>>       is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
>>>
>>> (2) QEMU reports the boot CPU count (FW_CFG_NB_CPUS is nonzero)
>>>
>>>       In this case, PlatformPei sets
>>>
>>>       - PcdCpuMaxLogicalProcessorNumber to the reported boot CPU count
>>>         (FW_CFG_NB_CPUS, which exports "PCMachineState.boot_cpus"),
>>>
>>>       - and PcdCpuApInitTimeOutInMicroSeconds to practically "infinity"
>>>         (MAX_UINT32, ~71 minutes).
>>>
>>>       That causes MpInitLib to enumerate exactly the present (boot) APs.
>>>
>>>       With CPU hotplug in mind, this method is not good enough. Because,
>>>       using QEMU terminology, UefiCpuPkg expects
>>>       PcdCpuMaxLogicalProcessorNumber to provide the "possible CPUs" count
>>>       ("MachineState.smp.max_cpus"), which includes present and not
>>> present
>>>       CPUs both (with not present CPUs being subject for hot-plugging).
>>>       FW_CFG_NB_CPUS does not include not present CPUs.
>>>
>>> Rewrite MaxCpuCountInitialization() for handling the following cases:
>>>
>>> (1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
>>>       to values different from the defaults.)
>>>
>>> (2) QEMU reports the boot CPU count ("PCMachineState.boot_cpus", via
>>>       FW_CFG_NB_CPUS), but not the possible CPUs count
>>>       ("MachineState.smp.max_cpus").
>>>
>>>       In this case, the behavior remains unchanged.
>>>
>>>       The way MpInitLib is instructed to do the same differs however:
>>> we now
>>>       set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
>>>       (while continuing to set PcdCpuMaxLogicalProcessorNumber
>>> identically).
>>>       PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant.
>>>
>>> (3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus", via
>>>       FW_CFG_NB_CPUS), and the possible CPUs count
>>>       ("MachineState.smp.max_cpus").
>>>
>>>       We tell UefiCpuPkg about the possible CPUs count through
>>>       PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
>>>       count for precise and quick AP enumeration, via
>>>       PcdCpuBootLogicalProcessorNumber.
>>> PcdCpuApInitTimeOutInMicroSeconds is
>>>       irrelevant again.
>>>
>>> This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
>>> As a side effect, the patch also enables S3 to work with CPU hotplug at
>>> once, *without* SMM_REQUIRE.
>>>
>>> (Without the patch, S3 resume fails, if a CPU is hot-plugged at OS
>>> runtime, prior to suspend: the FW_CFG_NB_CPUS increase seen during resume
>>> causes PcdCpuMaxLogicalProcessorNumber to increase as well, which is not
>>> permitted.
>>>
>>> With the patch, PcdCpuMaxLogicalProcessorNumber stays the same, namely
>>> "MachineState.smp.max_cpus". Therefore, the CPU structures allocated
>>> during normal boot can accommodate the CPUs at S3 resume that have been
>>> hotplugged prior to S3 suspend.)
>>>
>>> Cc: Anthony Perard <anthony.perard@citrix.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>       v2:
>>>            - use "possible CPUs" term in the code and the commit
>>> message [Igor]
>>>            - add details about S3 to the commit message [Igor]
>>>            - use QEMU's existent CPU hotplug register block, rather
>>> than a new
>>>         named file in fw_cfg [Igor]
>>>            - tested on QEMU v2.6.2 (legacy-only CPU hotplug register
>>> block), v2.7.1
>>>         (modern register block, but buggy fw_cfg), v2.8.1.1 (no QEMU
>>> issues),
>>>         v4.0.0 (no QEMU issues)
>>>
>>>    OvmfPkg/OvmfPkgIa32.dsc             |   2 +-
>>>    OvmfPkg/OvmfPkgIa32X64.dsc          |   2 +-
>>>    OvmfPkg/OvmfPkgX64.dsc              |   2 +-
>>>    OvmfPkg/PlatformPei/PlatformPei.inf |   2 +-
>>>    OvmfPkg/PlatformPei/Platform.c      | 172 +++++++++++++++++---
>>>    5 files changed, 151 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>> index 4301e7821902..68d8a9fb9655 100644
>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>> @@ -553,11 +553,11 @@ [PcdsDynamicDefault]
>>>      gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>>>      gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>>>        # UefiCpuPkg PCDs related to initial AP bringup and general AP
>>> management.
>>>      gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>>>        # Set memory encryption mask
>>>     
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>>>      !if $(SMM_REQUIRE) == TRUE
>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> index 803fd74ae8e4..e5a6260b6088 100644
>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> @@ -565,11 +565,11 @@ [PcdsDynamicDefault]
>>>      gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>>>      gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>>>        # UefiCpuPkg PCDs related to initial AP bringup and general AP
>>> management.
>>>      gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>>>        # Set memory encryption mask
>>>     
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>>>      !if $(SMM_REQUIRE) == TRUE
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index 5dbd1b793a90..f5d904945103 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -564,11 +564,11 @@ [PcdsDynamicDefault]
>>>      gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>>>      gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>>>        # UefiCpuPkg PCDs related to initial AP bringup and general AP
>>> management.
>>>      gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>>>        # Set memory encryption mask
>>>     
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>>>      !if $(SMM_REQUIRE) == TRUE
>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> index d9fd9c8f05b3..30eaebdfae63 100644
>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> @@ -96,11 +96,11 @@ [Pcd]
>>>      gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>>>      gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>>>      gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
>>>      gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
>>>      gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
>>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
>>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
>>>      gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>>>      [FixedPcd]
>>>      gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>>    diff --git a/OvmfPkg/PlatformPei/Platform.c
>>> b/OvmfPkg/PlatformPei/Platform.c
>>> index 3ba2459872d9..e5e8581752b5 100644
>>> --- a/OvmfPkg/PlatformPei/Platform.c
>>> +++ b/OvmfPkg/PlatformPei/Platform.c
>>> @@ -28,11 +28,14 @@
>>>    #include <Library/QemuFwCfgLib.h>
>>>    #include <Library/QemuFwCfgS3Lib.h>
>>>    #include <Library/ResourcePublicationLib.h>
>>>    #include <Guid/MemoryTypeInformation.h>
>>>    #include <Ppi/MasterBootMode.h>
>>> +#include <IndustryStandard/I440FxPiix4.h>
>>>    #include <IndustryStandard/Pci22.h>
>>> +#include <IndustryStandard/Q35MchIch9.h>
>>> +#include <IndustryStandard/QemuCpuHotplug.h>
>>>    #include <OvmfPlatforms.h>
>>>      #include "Platform.h"
>>>    #include "Cmos.h"
>>>    @@ -562,47 +565,165 @@ S3Verification (
>>>    #endif
>>>    }
>>>        /**
>>> -  Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg
>>> modules.
>>> -  Set the mMaxCpuCount variable.
>>> +  Fetch the boot CPU count and the possible CPU count from QEMU, and
>>> expose
>>> +  them to UefiCpuPkg modules. Set the mMaxCpuCount variable.
>>>    **/
>>>    VOID
>>>    MaxCpuCountInitialization (
>>>      VOID
>>>      )
>>>    {
>>> -  UINT16        ProcessorCount;
>>> +  UINT16        BootCpuCount;
>>>      RETURN_STATUS PcdStatus;
>>>    +  //
>>> +  // Try to fetch the boot CPU count.
>>> +  //
>>>      QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount);
>>> -  ProcessorCount = QemuFwCfgRead16 ();
>>> -  //
>>> -  // If the fw_cfg key or fw_cfg entirely is unavailable, load
>>> mMaxCpuCount
>>> -  // from the PCD default. No change to PCDs.
>>> -  //
>>> -  if (ProcessorCount == 0) {
>>> +  BootCpuCount = QemuFwCfgRead16 ();
>>> +  if (BootCpuCount == 0) {
>>> +    //
>>> +    // QEMU doesn't report the boot CPU count. (BootCpuCount == 0)
>>> will let
>>> +    // MpInitLib count APs up to (PcdCpuMaxLogicalProcessorNumber -
>>> 1), or
>>> +    // until PcdCpuApInitTimeOutInMicroSeconds elapses (whichever is
>>> reached
>>> +    // first).
>>> +    //
>>> +    DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n",
>>> __FUNCTION__));
>>>        mMaxCpuCount = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>>> -    return;
>>> +  } else {
>>> +    //
>>> +    // We will expose BootCpuCount to MpInitLib. MpInitLib will count
>>> APs up to
>>> +    // (BootCpuCount - 1) precisely, regardless of timeout.
>>> +    //
>>> +    // Now try to fetch the possible CPU count.
>>> +    //
>>> +    UINTN CpuHpBase;
>>> +    UINT32 CmdData2;
>>> +
>>> +    CpuHpBase = ((mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) ?
>>> +                 ICH9_CPU_HOTPLUG_BASE : PIIX4_CPU_HOTPLUG_BASE);
>>> +
>>> +    //
>>> +    // If only legacy mode is available in the CPU hotplug register
>>> block, or
>>> +    // the register block is completely missing, then the writes
>>> below are
>>> +    // no-ops.
>>> +    //
>>> +    // 1. Switch the hotplug register block to modern mode.
>>> +    //
>>> +    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
>>> +    //
>>> +    // 2. Select a valid CPU for deterministic reading of
>>> +    //    QEMU_CPUHP_R_CMD_DATA2.
>>> +    //
>>> +    //    CPU#0 is always valid; it is the always present and
>>> non-removable
>>> +    //    BSP.
>>> +    //
>>> +    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
>>> +    //
>>> +    // 3. Send a command after which QEMU_CPUHP_R_CMD_DATA2 is
>>> specified to
>>> +    //    read as zero, and which does not invalidate the selector. (The
>>> +    //    selector may change, but it must not become invalid.)
>>> +    //
>>> +    //    Send QEMU_CPUHP_CMD_GET_PENDING, as it will prove useful
>>> later.
>>> +    //
>>> +    IoWrite8 (CpuHpBase + QEMU_CPUHP_W_CMD, QEMU_CPUHP_CMD_GET_PENDING);
>>> +    //
>>> +    // 4. Read QEMU_CPUHP_R_CMD_DATA2.
>>> +    //
>>> +    //    If the register block is entirely missing, then this is an
>>> unassigned
>>> +    //    IO read, returning all-bits-one.
>>> +    //
>>> +    //    If only legacy mode is available, then bit#0 stands for
>>> CPU#0 in the
>>> +    //    "CPU present bitmap". CPU#0 is always present.
>>> +    //
>>> +    //    Otherwise, QEMU_CPUHP_R_CMD_DATA2 is either still reserved
>>> (returning
>>> +    //    all-bits-zero), or it is specified to read as zero after
>>> the above
>>> +    //    steps. Both cases confirm modern mode.
>>> +    //
>>> +    CmdData2 = IoRead32 (CpuHpBase + QEMU_CPUHP_R_CMD_DATA2);
>>> +    DEBUG ((DEBUG_VERBOSE, "%a: CmdData2=0x%x\n", __FUNCTION__,
>>> CmdData2));
>>> +    if (CmdData2 != 0) {
>>> +      //
>>> +      // QEMU doesn't support the modern CPU hotplug interface.
>>> Assume that the
>>> +      // possible CPU count equals the boot CPU count (precluding
>>> hotplug).
>>> +      //
>>> +      DEBUG ((DEBUG_WARN, "%a: modern CPU hotplug interface
>>> unavailable\n",
>>> +        __FUNCTION__));
>>> +      mMaxCpuCount = BootCpuCount;
>>> +    } else {
>>> +      //
>>> +      // Grab the possible CPU count from the modern CPU hotplug
>>> interface.
>>> +      //
>>> +      UINT32 Present, Possible, Selected;
>>> +
>>> +      Present = 0;
>>> +      Possible = 0;
>>> +
>>> +      //
>>> +      // We've sent QEMU_CPUHP_CMD_GET_PENDING last; this ensures
>>> +      // QEMU_CPUHP_RW_CMD_DATA can now be read usefully. However,
>>> +      // QEMU_CPUHP_CMD_GET_PENDING may have selected a CPU with
>>> actual pending
>>> +      // hotplug events; therefore, select CPU#0 forcibly.
>>> +      //
>>> +      IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
>>
>> Nitpicking, I find this easier to read (matches the comment):
>>
>>           IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
> 
> I considered that, but I chose this form consciously, in the end.
> Namely, if you search this patch for
> 
>    IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
> 
> you will find that there are two instances of it: one before the loop,
> and one inside the loop. I wanted to make those operations *look*
> identical as well, not just *be* semantically identical.

I see.

> The same goal prevented me from changing something else too: namely, in
> the loop body, the following:
> 
>          ++Possible;
>          IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
> 
> could be condensed as:
> 
>          IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, ++Possible);
> 
> But, that would again break the syntactical identity. And, again, for
> some reason I found it helpful to keep those two function calls
> syntactically identical.

Oh I really prefer the 2 different lines form, it is a no-brain
effort to review, one logical operation at a time.

> If you think the above effort doesn't actually improve readability, then
> I'm happy to respin with *both* changes at the same time (i.e., pass
> constant 0 in the first location, and pass "++Possible" in the second
> location). I don't think that implementing either change *in isolation*
> would be good.

No, since I prefer to not use the condensed form, let's keep this
patch as it.

Regards,

Phil.

>> Rest very well documented, thanks.
>>
>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>>
>>> +
>>> +      do {
>>> +        UINT8 CpuStatus;
>>> +
>>> +        //
>>> +        // Read the status of the currently selected CPU. This will
>>> help with a
>>> +        // sanity check against "BootCpuCount".
>>> +        //
>>> +        CpuStatus = IoRead8 (CpuHpBase + QEMU_CPUHP_R_CPU_STAT);
>>> +        if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) != 0) {
>>> +          ++Present;
>>> +        }
>>> +        //
>>> +        // Attempt to select the next CPU.
>>> +        //
>>> +        ++Possible;
>>> +        IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
>>> +        //
>>> +        // If the selection is successful, then the following read
>>> will return
>>> +        // the selector (which we know is positive at this point).
>>> Otherwise,
>>> +        // the read will return 0.
>>> +        //
>>> +        Selected = IoRead32 (CpuHpBase + QEMU_CPUHP_RW_CMD_DATA);
>>> +        ASSERT (Selected == Possible || Selected == 0);
>>> +      } while (Selected > 0);
>>> +
>>> +      //
>>> +      // Sanity check: fw_cfg and the modern CPU hotplug interface
>>> should
>>> +      // return the same boot CPU count.
>>> +      //
>>> +      if (BootCpuCount != Present) {
>>> +        DEBUG ((DEBUG_WARN, "%a: QEMU v2.7 reset bug: BootCpuCount=%d "
>>> +          "Present=%u\n", __FUNCTION__, BootCpuCount, Present));
>>> +        //
>>> +        // The handling of QemuFwCfgItemSmpCpuCount, across CPU
>>> hotplug plus
>>> +        // platform reset (including S3), was corrected in QEMU commit
>>> +        // e3cadac073a9 ("pc: fix FW_CFG_NB_CPUS to account for
>>> -device added
>>> +        // CPUs", 2016-11-16), part of release v2.8.0.
>>> +        //
>>> +        BootCpuCount = (UINT16)Present;
>>> +      }
>>> +
>>> +      mMaxCpuCount = Possible;
>>> +    }
>>>      }
>>> -  //
>>> -  // Otherwise, set mMaxCpuCount to the value reported by QEMU.
>>> -  //
>>> -  mMaxCpuCount = ProcessorCount;
>>> -  //
>>> -  // Additionally, tell UefiCpuPkg modules (a) the exact number of
>>> VCPUs, (b)
>>> -  // to wait, in the initial AP bringup, exactly as long as it takes
>>> for all of
>>> -  // the APs to report in. For this, we set the longest representable
>>> timeout
>>> -  // (approx. 71 minutes).
>>> -  //
>>> -  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber,
>>> ProcessorCount);
>>> +
>>> +  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n",
>>> __FUNCTION__,
>>> +    BootCpuCount, mMaxCpuCount));
>>> +  ASSERT (BootCpuCount <= mMaxCpuCount);
>>> +
>>> +  PcdStatus = PcdSet32S (PcdCpuBootLogicalProcessorNumber,
>>> BootCpuCount);
>>>      ASSERT_RETURN_ERROR (PcdStatus);
>>> -  PcdStatus = PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UINT32);
>>> +  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCount);
>>>      ASSERT_RETURN_ERROR (PcdStatus);
>>> -  DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n",
>>> __FUNCTION__,
>>> -    ProcessorCount));
>>>    }
>>>        /**
>>>      Perform Platform PEI initialization.
>>> @@ -636,17 +757,18 @@ InitializePlatform (
>>>      }
>>>        S3Verification ();
>>>      BootModeInitialization ();
>>>      AddressWidthInitialization ();
>>> -  MaxCpuCountInitialization ();
>>>        //
>>>      // Query Host Bridge DID
>>>      //
>>>      mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
>>>    +  MaxCpuCountInitialization ();
>>> +
>>>      if (FeaturePcdGet (PcdSmmSmramRequire)) {
>>>        Q35TsegMbytesInitialization ();
>>>      }
>>>        PublishPeiMemory ();
>>>
> 

Re: [PATCH v2 0/3] OvmfPkg: distinguish boot CPU count from possible CPU count

[Ard Biesheuvel]

On Wed, 23 Oct 2019 at 00:16, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Repo:   https://github.com/lersek/edk2.git
> Branch: max_cpus_bz_1515_v2
> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>
> Version 1 was:
>
>   [edk2-devel] [PATCH 0/4]
>   UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count
>
>   http://mid.mail-archive.com/20191008112714.6215-1-lersek@redhat.com
>   https://edk2.groups.io/g/devel/message/48562
>
> Since then, patch#1 of v1 has been rewritten and pushed as commit range
> a7e2d20193e8..778832bcad33.
>
> This v2 posting reworks the rest (patches #2 through #4) of the v1
> series. Changes are listed per patch, in the notes sections.
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (3):
>   OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults
>   OvmfPkg/IndustryStandard: define macros for QEMU's CPU hotplug
>     registers
>   OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU
>     hotplug
>

For patches #2 and #3

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


>  OvmfPkg/Include/IndustryStandard/I440FxPiix4.h    |   5 +
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h     |   2 +
>  OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |  43 +++++
>  OvmfPkg/OvmfPkgIa32.dsc                           |   2 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                        |   2 +-
>  OvmfPkg/OvmfPkgX64.dsc                            |   2 +-
>  OvmfPkg/OvmfXen.dsc                               |   4 -
>  OvmfPkg/PlatformPei/Platform.c                    | 172 +++++++++++++++++---
>  OvmfPkg/PlatformPei/PlatformPei.inf               |   2 +-
>  9 files changed, 201 insertions(+), 33 deletions(-)
>  create mode 100644 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
>
> --
> 2.19.1.3.g30247aa5d201
>