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

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

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