> From: Igor Mammedov <imamm...@redhat.com> > Sent: Tuesday, November 5, 2024 12:50 PM > To: Michael S. Tsirkin <m...@redhat.com> > Cc: qemu-devel@nongnu.org; Peter Maydell <peter.mayd...@linaro.org>; > Salil Mehta <salil.me...@huawei.com>; Ani Sinha <anisi...@redhat.com>; > Eduardo Habkost <edua...@habkost.net>; Marcel Apfelbaum > <marcel.apfelb...@gmail.com>; Philippe Mathieu-Daudé > <phi...@linaro.org>; wangyanan (Y) <wangyana...@huawei.com>; Zhao > Liu <zhao1....@intel.com> > Subject: Re: [PULL 60/65] hw/acpi: Update ACPI `_STA` method with QOM > vCPU ACPI Hotplug states > > On Mon, 4 Nov 2024 16:09:26 -0500 > "Michael S. Tsirkin" <m...@redhat.com> wrote: > > > From: Salil Mehta <salil.me...@huawei.com> > > > > Reflect the QOM vCPUs ACPI CPU hotplug states in the `_STA.Present` > > and and `_STA.Enabled` bits when the guest kernel evaluates the ACPI > > `_STA` method during initialization, as well as when vCPUs are > > hot-plugged or hot-unplugged. If the CPU is present then the its > > `enabled` status can be fetched using architecture-specific code [1]. > > > > Reference: > > [1] Example implementation of architecture-specific hook to fetch CPU > > `enabled status > > Link: > > https://github.com/salil- > mehta/qemu/commit/c0b416b11e5af6505e558866f0e > > b6c9f3709173e > > > > Signed-off-by: Salil Mehta <salil.me...@huawei.com> > > Message-Id: <20241103102419.202225-4-salil.me...@huawei.com> > > Reviewed-by: Michael S. Tsirkin <m...@redhat.com> > > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > > --- > > include/hw/core/cpu.h | 1 + > > hw/acpi/cpu.c | 38 ++++++++++++++++++++++++++++++++++---- > > 2 files changed, 35 insertions(+), 4 deletions(-) > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index > > e7de77dc6d..db8a6fbc6e 100644 > > --- a/include/hw/core/cpu.h > > +++ b/include/hw/core/cpu.h > > @@ -159,6 +159,7 @@ struct CPUClass { > > void (*query_cpu_fast)(CPUState *cpu, CpuInfoFast *value); > > int64_t (*get_arch_id)(CPUState *cpu); > > bool (*cpu_persistent_status)(CPUState *cpu); > > + bool (*cpu_enabled_status)(CPUState *cpu); > > void (*set_pc)(CPUState *cpu, vaddr value); > > vaddr (*get_pc)(CPUState *cpu); > > int (*gdb_read_register)(CPUState *cpu, GByteArray *buf, int > > reg); diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index > > 9b03b4292e..23443f09a5 100644 > > --- a/hw/acpi/cpu.c > > +++ b/hw/acpi/cpu.c > > @@ -50,6 +50,18 @@ void acpi_cpu_ospm_status(CPUHotplugState > *cpu_st, ACPIOSTInfoList ***list) > > } > > } > > > > +static bool check_cpu_enabled_status(DeviceState *dev) { > > + CPUClass *k = dev ? CPU_GET_CLASS(dev) : NULL; > > + CPUState *cpu = CPU(dev); > > + > > + if (cpu && (!k->cpu_enabled_status || k->cpu_enabled_status(cpu))) > { > > + return true; > > + } > > + > > + return false; > > +} > > + > > static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned > > size) { > > uint64_t val = 0; > > @@ -63,10 +75,11 @@ static uint64_t cpu_hotplug_rd(void *opaque, > hwaddr addr, unsigned size) > > cdev = &cpu_st->devs[cpu_st->selector]; > > switch (addr) { > > case ACPI_CPU_FLAGS_OFFSET_RW: /* pack and return is_* fields */ > > - val |= cdev->cpu ? 1 : 0; > > + val |= check_cpu_enabled_status(DEVICE(cdev->cpu)) ? 1 : 0; > > val |= cdev->is_inserting ? 2 : 0; > > val |= cdev->is_removing ? 4 : 0; > > val |= cdev->fw_remove ? 16 : 0; > > + val |= cdev->cpu ? 32 : 0; > > trace_cpuhp_acpi_read_flags(cpu_st->selector, val); > > break; > > case ACPI_CPU_CMD_DATA_OFFSET_RW: > > @@ -349,6 +362,7 @@ const VMStateDescription vmstate_cpu_hotplug = > { > > #define CPU_REMOVE_EVENT "CRMV" > > #define CPU_EJECT_EVENT "CEJ0" > > #define CPU_FW_EJECT_EVENT "CEJF" > > +#define CPU_PRESENT "CPRS" > > > > void build_cpus_aml(Aml *table, MachineState *machine, > CPUHotplugFeatures opts, > > build_madt_cpu_fn build_madt_cpu, hwaddr > > base_addr, @@ -409,7 +423,9 @@ void build_cpus_aml(Aml *table, > MachineState *machine, CPUHotplugFeatures opts, > > aml_append(field, aml_named_field(CPU_EJECT_EVENT, 1)); > > /* tell firmware to do device eject, write only */ > > aml_append(field, aml_named_field(CPU_FW_EJECT_EVENT, 1)); > > - aml_append(field, aml_reserved_field(3)); > > + /* 1 if present, read only */ > > + aml_append(field, aml_named_field(CPU_PRESENT, 1)); > > + aml_append(field, aml_reserved_field(2)); > > aml_append(field, aml_named_field(CPU_COMMAND, 8)); > > aml_append(cpu_ctrl_dev, field); > > > > @@ -439,6 +455,7 @@ void build_cpus_aml(Aml *table, MachineState > *machine, CPUHotplugFeatures opts, > > Aml *ctrl_lock = aml_name("%s.%s", cphp_res_path, CPU_LOCK); > > Aml *cpu_selector = aml_name("%s.%s", cphp_res_path, > CPU_SELECTOR); > > Aml *is_enabled = aml_name("%s.%s", cphp_res_path, > > CPU_ENABLED); > > + Aml *is_present = aml_name("%s.%s", cphp_res_path, > > + CPU_PRESENT); > > Aml *cpu_cmd = aml_name("%s.%s", cphp_res_path, > CPU_COMMAND); > > Aml *cpu_data = aml_name("%s.%s", cphp_res_path, CPU_DATA); > > Aml *ins_evt = aml_name("%s.%s", cphp_res_path, > > CPU_INSERT_EVENT); @@ -467,13 +484,26 @@ void build_cpus_aml(Aml > *table, MachineState *machine, CPUHotplugFeatures opts, > > { > > Aml *idx = aml_arg(0); > > Aml *sta = aml_local(0); > > + Aml *ifctx2; > > + Aml *else_ctx; > > > > aml_append(method, aml_acquire(ctrl_lock, 0xFFFF)); > > aml_append(method, aml_store(idx, cpu_selector)); > > aml_append(method, aml_store(zero, sta)); > > - ifctx = aml_if(aml_equal(is_enabled, one)); > > + ifctx = aml_if(aml_equal(is_present, one)); > > very likely this will break hotplug on after migration to older QEMU.
The above are local variables and are not being migrated. These are not the same as the earlier comment you provided here. I've removed those `is_present,enabled` states to address your earlier concerns: https://lore.kernel.org/qemu-devel/20241018163118.0ae01...@imammedo.users.ipa.redhat.com/ State-1: Possible State of ACPI _STA (without patches) _STA.Present = 0 _STA.Enabled = 0 _STA.Present = 1 _STA.Enabled = 1 State-2: Possible State of ACPI _STA (with patches) _STA.Present = 0 _STA.Enabled = 0 _STA.Present = 1 _STA.Enabled = 1 _STA.Present = 1 _STA.Enabled = 0 [New return state which should not affect x86] State-1 is subset of State-2. If vCPU HP feature is off on the newer Qemu then, State-1 becomes proper subset of State-2. Later is also the state of the older Qemu? > > { > > - aml_append(ifctx, aml_store(aml_int(0xF), sta)); > > + ifctx2 = aml_if(aml_equal(is_enabled, one)); > > + { > > + /* cpu is present and enabled */ > > + aml_append(ifctx2, aml_store(aml_int(0xF), sta)); > > + } > > + aml_append(ifctx, ifctx2); > > + else_ctx = aml_else(); > > + { > > + /* cpu is present but disabled */ > > + aml_append(else_ctx, aml_store(aml_int(0xD), sta)); > > + } > > + aml_append(ifctx, else_ctx); > > } > > aml_append(method, ifctx); > > aml_append(method, aml_release(ctrl_lock)); >
