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

["Anthony PERARD" <anthony.perard@citrix.com>]

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