Hi Gustavo On Thu, Oct 17, 2024 at 9:25 PM Gustavo Romero <gustavo.rom...@linaro.org> wrote:
> Hi Salil, > > On 10/14/24 16:22, Salil Mehta wrote: > > Certain CPU architecture specifications [1][2][3] prohibit changes to CPU > > presence after the kernel has booted. This limitation exists because > many system > > initializations rely on the exact CPU count at boot time and do not > expect it to > > change later. For example, components like interrupt controllers, which > are > > closely tied to CPUs, or various per-CPU features, may not support > configuration > > changes once the kernel has been initialized. This presents a challenge > for > > virtualization features such as vCPU hotplug. > > > > To address this issue, introduce an `is_enabled` state in the > `AcpiCpuStatus`, > > which reflects whether a vCPU has been hot-plugged or hot-unplugged in > QEMU, > > marking it as (un)available in the Guest Kernel. The `is_present` state > should > > be set based on the `acpi_persistent` flag. In cases where unplugged > vCPUs need > > to be deliberately simulated in the ACPI to maintain a persistent view > of vCPUs, > > this flag ensures the guest kernel continues to see those vCPUs. > > > > Additionally, introduce an `acpi_persistent` property that can be used to > > initialize the ACPI vCPU presence state accordingly. Architectures > requiring > > ACPI to expose a persistent view of vCPUs can override its default > value. Refer > > to the patch-set implelenting vCPU hotplug support for ARM for more > details on > > its usage. > > > > 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 > > > > Signed-off-by: Salil Mehta <salil.me...@huawei.com> > > --- > > cpu-target.c | 1 + > > hw/acpi/cpu.c | 35 ++++++++++++++++++++++++++++++++++- > > include/hw/acpi/cpu.h | 21 +++++++++++++++++++++ > > include/hw/core/cpu.h | 21 +++++++++++++++++++++ > > 4 files changed, 77 insertions(+), 1 deletion(-) > > > > diff --git a/cpu-target.c b/cpu-target.c > > index 499facf774..c8a29ab495 100644 > > --- a/cpu-target.c > > +++ b/cpu-target.c > > @@ -200,6 +200,7 @@ static Property cpu_common_props[] = { > > */ > > DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION, > > MemoryRegion *), > > + DEFINE_PROP_BOOL("acpi-persistent", CPUState, acpi_persistent, > false), > > #endif > > DEFINE_PROP_END_OF_LIST(), > > }; > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > > index 5cb60ca8bc..083c4010c2 100644 > > --- a/hw/acpi/cpu.c > > +++ b/hw/acpi/cpu.c > > @@ -225,7 +225,40 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object > *owner, > > state->dev_count = id_list->len; > > state->devs = g_new0(typeof(*state->devs), state->dev_count); > > for (i = 0; i < id_list->len; i++) { > > - state->devs[i].cpu = CPU(id_list->cpus[i].cpu); > > + struct CPUState *cpu = CPU(id_list->cpus[i].cpu); > > + /* > > + * In most architectures, CPUs that are marked as ACPI > 'present' are > > + * also ACPI 'enabled' by default. These states remain > consistent at > > + * both the QOM and ACPI levels. > > + */ > > + if (cpu) { > > + state->devs[i].is_enabled = true; > > + state->devs[i].is_present = true; > > + state->devs[i].cpu = cpu; > > + } else { > > + state->devs[i].is_enabled = false; > > + /* > > + * In some architectures, even 'unplugged' or 'disabled' > QOM CPUs > > + * may be exposed as ACPI 'present.' This approach provides > a > > + * persistent view of the vCPUs to the guest kernel. This > could be > > + * due to an architectural constraint that requires every > per-CPU > > + * component to be present at boot time, meaning the exact > count of > > + * vCPUs must be known and cannot be altered after the > kernel has > > + * booted. As a result, the vCPU states at the QOM and ACPI > levels > > + * might become inconsistent. However, in such cases, the > presence > > + * of vCPUs has been deliberately simulated at the ACPI > level. > > + */ > > + if (acpi_persistent_cpu(first_cpu)) { > > + state->devs[i].is_present = true; > > + /* > > + * `CPUHotplugState::AcpiCpuStatus::cpu` becomes > insignificant > > + * in this case > > + */ > > + } else { > > + state->devs[i].is_present = false; > > + state->devs[i].cpu = cpu; > > I think it's better to set cpu here explicitly to NULL in both cases > (persistent and non-persistent cases). Also, 'cpu' here is always NULL > since it's inside the else block of "if (cpu)" conditional. So how about > setting cpu to NULL at the end of the else block: > Good catch. Yes we can. It got under my hood after changes from RFC V4 to RFC V5 > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index d34c1e601e..b830c0e85b 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -251,14 +251,14 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object > *owner, > */ > if (acpi_persistent_cpu(first_cpu)) { > state->devs[i].is_present = true; > - /* > - * `CPUHotplugState::AcpiCpuStatus::cpu` becomes > insignificant > - * in this case > - */ > } else { > state->devs[i].is_present = false; > - state->devs[i].cpu = cpu; > } > + /* > + * `CPUHotplugState::AcpiCpuStatus::cpu` becomes insignificant > + * in this case > + */ > + state->devs[i].cpu = NULL; > } > state->devs[i].arch_id = id_list->cpus[i].arch_id; > } > > > Cheers, > Gustavo > > > + } > > + } > > state->devs[i].arch_id = id_list->cpus[i].arch_id; > > } > > memory_region_init_io(&state->ctrl_reg, owner, &cpu_hotplug_ops, > state, > > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h > > index 32654dc274..bd3f9973c9 100644 > > --- a/include/hw/acpi/cpu.h > > +++ b/include/hw/acpi/cpu.h > > @@ -26,6 +26,8 @@ typedef struct AcpiCpuStatus { > > uint64_t arch_id; > > bool is_inserting; > > bool is_removing; > > + bool is_present; > > + bool is_enabled; > > bool fw_remove; > > uint32_t ost_event; > > uint32_t ost_status; > > @@ -75,4 +77,23 @@ extern const VMStateDescription vmstate_cpu_hotplug; > > VMSTATE_STRUCT(cpuhp, state, 1, \ > > vmstate_cpu_hotplug, CPUHotplugState) > > > > +/** > > + * acpi_persistent_cpu: > > + * @cpu: The vCPU to check > > + * > > + * Checks if the vCPU state should always be reflected as *present* via > ACPI > > + * to the Guest. By default, this is False on all architectures and has > to be > > + * explicity set during initialization. > > + * > > + * Returns: True if it is ACPI 'persistent' CPU > > + * > > + */ > > +static inline bool acpi_persistent_cpu(CPUState *cpu) > > +{ > > + /* > > + * returns if 'Presence' of the vCPU is persistent and should be > simulated > > + * via ACPI even after vCPUs have been unplugged in QOM > > + */ > > + return cpu && cpu->acpi_persistent; > > +} > > #endif > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > > index 04e9ad4996..299e96c45b 100644 > > --- a/include/hw/core/cpu.h > > +++ b/include/hw/core/cpu.h > > @@ -542,6 +542,27 @@ struct CPUState { > > CPUPluginState *plugin_state; > > #endif > > > > + /* > > + * To implement the vCPU hotplug feature (which simulates CPU > hotplug > > + * behavior), we need to dynamically create and destroy QOM vCPU > objects, > > + * and (de)associate them with pre-existing KVM vCPUs while > (un)parking the > > + * KVM vCPU context. One challenge is ensuring that these > dynamically > > + * appearing or disappearing QOM vCPU objects are accurately > reflected > > + * through ACPI to the Guest Kernel. Due to architectural > constraints, > > + * changing the number of vCPUs after the guest kernel has booted > may not > > + * always be possible. > > + * > > + * In certain architectures, to provide the guest kernel with a > *persistent* > > + * view of vCPU presence, even when the QOM does not have a > corresponding > > + * vCPU object, ACPI may simulate the presence of vCPUs by marking > them as > > + * ACPI-disabled. This is achieved by setting `_STA.PRES=True` and > > + * `_STA.Ena=False` for unplugged vCPUs in QEMU's QOM. > > + * > > + * By default, this flag is set to `FALSE`, and it must be > explicitly set > > + * to `TRUE` for architectures like ARM. > > + */ > > + bool acpi_persistent; > > + > > /* TODO Move common fields from CPUArchState here. */ > > int cpu_index; > > int cluster_index; > >
