Hi Gavin, On Thu, Oct 17, 2024 at 6:27 AM Gavin Shan <gs...@redhat.com> wrote:
> On 10/15/24 5:22 AM, 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(), > > }; > > Would the property be consistent on all CPUs? I mean it's invalid for this > property to be true on the first CPU while it's false on other CPUs. If my > understanding is correct, this property would be part of MachineState > instead > of CPUState, and it's not configurable. Is there a particular reason why > the > property should be tied with CPUState and configurable? > Yes, it might make more sense to keep it at the machine level or configured via firmware. It might not look very obvious but the idea of keeping ACPI persistence at CPUState level was to extend this feature and have some group of vCPUs to be hot pluggable while others could be forced to be non-hotpluggable. This could be useful if in future hot pluggability is extended to per-die/per-numa level. I also removed the 'CPUState::disabled' flag which was more of an architecture specific thing. > Besides, the property name "acpi-persistent" isn't indicative enough. CPU > objects can be described through ACPI table or device tree node. So a more > generic property name may be needed, for example "always_present"? If we > need move this property from CPUState to MachineStaste or MachineClass, > the name would be "cpu_always_present" or something like that. > This property is more related to the persistence of the vCPU state at the ACPI level. Therefore, I changed the name from 'always_present' which I used in one of the preliminary prototype presented at the KVM Forum 2023. > > > 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; > > + } > > + } > > state->devs[i].arch_id = id_list->cpus[i].arch_id; > > } > > memory_region_init_io(&state->ctrl_reg, owner, &cpu_hotplug_ops, > state, > > The code is already self-explaining and the comments explaining how the > property > "acpi-persistent" is handled seems a bit duplicate. If you agree, lets > make comments > short and concise. > We can definitely make it more succinct > > > 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; > > + > > The comments can be short and concise either. Reader can refer to the > commit log for all the details. > Yes, we can thanks > > > /* TODO Move common fields from CPUArchState here. */ > > int cpu_index; > > int cluster_index; > > Thanks, > Gavin > >
