| Message ID | bf1ecc8dad6061914730a2a2d57af6b37c3a4f8d.1730754238.git.mst@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/65] softmmu: Expand comments describing max_bounce_buffer_size | expand |
On Mon, 4 Nov 2024 16:09:26 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote: > From: Salil Mehta <salil.mehta@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/c0b416b11e5af6505e558866f0eb6c9f3709173e > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > Message-Id: <20241103102419.202225-4-salil.mehta@huawei.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@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. > { > - 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));
> From: Igor Mammedov <imammedo@redhat.com> > Sent: Tuesday, November 5, 2024 12:50 PM > To: Michael S. Tsirkin <mst@redhat.com> > Cc: qemu-devel@nongnu.org; Peter Maydell <peter.maydell@linaro.org>; > Salil Mehta <salil.mehta@huawei.com>; Ani Sinha <anisinha@redhat.com>; > Eduardo Habkost <eduardo@habkost.net>; Marcel Apfelbaum > <marcel.apfelbaum@gmail.com>; Philippe Mathieu-Daudé > <philmd@linaro.org>; wangyanan (Y) <wangyanan55@huawei.com>; Zhao > Liu <zhao1.liu@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" <mst@redhat.com> wrote: > > > From: Salil Mehta <salil.mehta@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.mehta@huawei.com> > > Message-Id: <20241103102419.202225-4-salil.mehta@huawei.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@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.0ae01a84@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)); >
On Tue, 5 Nov 2024 21:12:00 +0000 Salil Mehta <salil.mehta@huawei.com> wrote: > > From: Igor Mammedov <imammedo@redhat.com> > > Sent: Tuesday, November 5, 2024 12:50 PM > > To: Michael S. Tsirkin <mst@redhat.com> > > Cc: qemu-devel@nongnu.org; Peter Maydell <peter.maydell@linaro.org>; > > Salil Mehta <salil.mehta@huawei.com>; Ani Sinha <anisinha@redhat.com>; > > Eduardo Habkost <eduardo@habkost.net>; Marcel Apfelbaum > > <marcel.apfelbaum@gmail.com>; Philippe Mathieu-Daudé > > <philmd@linaro.org>; wangyanan (Y) <wangyanan55@huawei.com>; Zhao > > Liu <zhao1.liu@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" <mst@redhat.com> wrote: > > > > > From: Salil Mehta <salil.mehta@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.mehta@huawei.com> > > > Message-Id: <20241103102419.202225-4-salil.mehta@huawei.com> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > Signed-off-by: Michael S. Tsirkin <mst@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.0ae01a84@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? here is AML change from the next patch: - If ((\_SB.PCI0.PRES.CPEN == One)) - { - Local0 = 0x0F + If ((\_SB.PCI0.PRES.CPRS == One)) + { 'enable check' branch is not taken unless presence is set. However on old qemu there is no presence bit in register block and it always 0, isn't it? If above is true, the new _STA running on old QEMU after migration will always return 0 and thus break x86 cphp. + If ((\_SB.PCI0.PRES.CPEN == One)) + { + Local0 = 0x0F + } + Else + { + Local0 = 0x0D + } } > > > > > { > > > - 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)); > > >
HI Igor, Thank for you replying back. > From: qemu-devel-bounces+salil.mehta=huawei.com@nongnu.org <qemu- > devel-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor > Mammedov > Sent: Wednesday, November 6, 2024 9:01 AM > To: Salil Mehta <salil.mehta@huawei.com> > > On Tue, 5 Nov 2024 21:12:00 +0000 > Salil Mehta <salil.mehta@huawei.com> wrote: > > > > From: Igor Mammedov <imammedo@redhat.com> > > > Sent: Tuesday, November 5, 2024 12:50 PM > > > To: Michael S. Tsirkin <mst@redhat.com> > > > Cc: qemu-devel@nongnu.org; Peter Maydell > > > <peter.maydell@linaro.org>; Salil Mehta <salil.mehta@huawei.com>; > > > Ani Sinha <anisinha@redhat.com>; Eduardo Habkost > > > <eduardo@habkost.net>; Marcel Apfelbaum > > > <marcel.apfelbaum@gmail.com>; Philippe Mathieu-Daudé > > > <philmd@linaro.org>; wangyanan (Y) <wangyanan55@huawei.com>; > Zhao > > > Liu <zhao1.liu@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" <mst@redhat.com> wrote: > > > > > > > From: Salil Mehta <salil.mehta@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.mehta@huawei.com> > > > > Message-Id: <20241103102419.202225-4-salil.mehta@huawei.com> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > Signed-off-by: Michael S. Tsirkin <mst@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.0ae01a84@imammedo.us > > ers.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? > > > here is AML change from the next patch: > > - If ((\_SB.PCI0.PRES.CPEN == One)) > - { > - Local0 = 0x0F > + If ((\_SB.PCI0.PRES.CPRS == One)) > + { > > 'enable check' branch is not taken unless presence is set. > However on old qemu there is no presence bit in register block and it always > 0, isn't it? Thanks. I can see your point now. We can rearrange the check as follows: If ((\_SB.PCI0.PRES.CPEN == One)) { Local0 = 0x0F } + Else + { + If ((\_SB.PCI0.PRES.CPRS == One)) + { + Local0 = 0x0D + } + } If you are okay then I'll send above change as a Fix to [PULL 60/65], [PULL 61/65] Please confirm. Many thanks! Best regards Salil. > > If above is true, the new _STA running on old QEMU after migration will > always return 0 and thus break x86 cphp. > > > + If ((\_SB.PCI0.PRES.CPEN == One)) > + { > + Local0 = 0x0F > + } > + Else > + { > + Local0 = 0x0D > + } > } > > > > > > > > > { > > > > - 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)); > > > > > >
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)); { - 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));