[edk2-devel] [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 at citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> Cc: Igor Mammedov <imammedo at redhat.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Julien Grall <julien.grall at arm.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> 
> Acked-by: Anthony PERARD <anthony.perard at citrix.com>
> 
> Xen falls into case (1) and OVMF still boots fine with the series
> applied.

Awesome, thank you very much for checking!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#49430): https://edk2.groups.io/g/devel/message/49430
Mute This Topic: https://groups.io/mt/36465055/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-