Re: [PATCH V1 0/4] Arch agnostic ACPI changes to support vCPU Hotplug (on Archs like ARM)

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 14 Oct 2024 20:22:01 +0100
Salil Mehta <salil.mehta@huawei.com> wrote:

> Certain CPU architecture specifications [1][2][3] prohibit changes to the CPUs
> *presence* after the kernel has booted. This is because many system
> initializations depend on the exact CPU count at boot time and do not expect it
> to change afterward. For example, components like interrupt controllers that are
> closely coupled with CPUs, or various per-CPU features, may not support
> configuration changes once the kernel has been initialized.
> 
> This requirement poses a challenge for virtualization features like vCPU
> hotplug. To address this, changes to the ACPI AML are necessary to update the
> `_STA.PRES` (presence) and `_STA.ENA` (enabled) bits accordingly during guest
> initialization, as well as when vCPUs are hot-plugged or hot-unplugged. The
> presence of unplugged vCPUs may need to be deliberately *simulated* at the ACPI
> level to maintain a *persistent* view of vCPUs for the guest kernel.

the series is peppered with *simulated* idea, which after looking at code
I read as 'fake'. While it's obvious to author why things need to be faked
at this time, it will be forgotten later on. And cause a lot swearing from
whoever will have to deal with this code down the road.

Salil, I'm sorry that review comes out as mostly negative but for me having to
repeat 'simulated' some many times, hints that the there is
something wrong with design and that we should re-evaluate the approach.

ps:
see comments on 1/4 for suggestions


> This patch set introduces the following features:
> 
> 1. ACPI Interface with Explicit PRESENT and ENABLED CPU States: It allows the
>    guest kernel to evaluate these states using the `_STA` ACPI method.
>    
> 2. Initialization of ACPI CPU States: These states are initialized during
>    `machvirt_init` and when vCPUs are hot-(un)plugged. This enables hotpluggable
>    vCPUs to be exposed to the guest kernel via ACPI.
> 
> 3. Support for Migrating ACPI CPU States: The patch set ensures the migration of
>    the newly introduced `is_{present,enabled}` ACPI CPU states to the
>    destination VM.
> 
> The approach is flexible enough to accommodate ARM-like architectures that
> intend to implement vCPU hotplug functionality. It is suitable for architectures
> facing similar constraints to ARM or those that plan to implement vCPU
> hotplugging independently of hardware support (if available).
> 
> This patch set is derived from the ARM-specific vCPU hotplug implementation [4]
> and includes migration components adaptable to other architectures, following
> suggestions [5] made by Igor Mammedov <imammedo@redhat.com>.
> 
> It can be applied independently, ensuring compatibility with existing hotplug
> support in other architectures. I have tested this patch set in conjunction with
> the ARM-specific vCPU hotplug changes (included in the upcoming RFC V5 [6]), and
> everything worked as expected. I kindly request maintainers of other
> architectures to provide a "Tested-by" after running their respective regression
> tests.
> 
> Many thanks!
> 
> 
> References:
> [1] KVMForum 2023 Presentation: Challenges Revisited in Supporting Virt CPU Hotplug on
>     architectures that don’t Support CPU Hotplug (like ARM64)
>     a. Kernel Link: https://kvm-forum.qemu.org/2023/KVM-forum-cpu-hotplug_7OJ1YyJ.pdf
>     b. Qemu Link:  https://kvm-forum.qemu.org/2023/Challenges_Revisited_in_Supporting_Virt_CPU_Hotplug_-__ii0iNb3.pdf
> [2] KVMForum 2020 Presentation: Challenges in Supporting Virtual CPU Hotplug on
>     SoC Based Systems (like ARM64)
>     Link: https://kvmforum2020.sched.com/event/eE4m
> [3] Check comment 5 in the bugzilla entry
>     Link: https://bugzilla.tianocore.org/show_bug.cgi?id=4481#c5
> [4] [PATCH RFC V4 00/33] Support of Virtual CPU Hotplug for ARMv8 Arch
>     Link: https://lore.kernel.org/qemu-devel/20241009031815.250096-1-salil.mehta@huawei.com/T/#mf32be203baa568a871dc625b732f666a4c4f1e68
> [5] Architecture agnostic ACPI VMSD state migration (Discussion)
>     Link: https://lore.kernel.org/qemu-devel/20240715155436.577d34c5@imammedo.users.ipa.redhat.com/
> [6] Upcoming RFC V5, Support of Virtual CPU Hotplug for ARMv8 Arch
>     Link: https://github.com/salil-mehta/qemu/commits/virt-cpuhp-armv8/rfc-v5
> 
> Salil Mehta (4):
>   hw/acpi: Initialize ACPI Hotplug CPU Status with Support for vCPU
>     `Persistence`
>   hw/acpi: Update ACPI CPU Status `is_{present, enabled}` during vCPU
>     hot(un)plug
>   hw/acpi: Reflect ACPI vCPU {present,enabled} states in ACPI
>     _STA.{PRES,ENA} Bits
>   hw/acpi: Populate vCPU Hotplug VMSD to migrate `is_{present,enabled}`
>     states
> 
>  cpu-target.c         patches.vcpuhp.rfc-v5.arch.agnostic.acpi          |  1 +
>  hw/acpi/cpu.c                  | 70 +++++++++++++++++++++++++++++++---
>  hw/acpi/generic_event_device.c | 11 ++++++
>  include/hw/acpi/cpu.h          | 21 ++++++++++
>  include/hw/core/cpu.h          | 21 ++++++++++
>  5 files changed, 119 insertions(+), 5 deletions(-)
>