On Wed, 6 Nov 2024 13:03:30 +0000
Salil Mehta <salil.me...@huawei.com> wrote:
> Checking `is_present` first can break x86 migration from new Qemu
> version to old Qemu. This is because CPRS Bit is not defined in the
> older Qemu register block and will always be 0 resulting in check always
> failing. Reversing the logic to first check `is_enabled` can alleviate
> below problem:
>
> - If ((\_SB.PCI0.PRES.CPEN == One))
> - {
> - Local0 = 0x0F
> + If ((\_SB.PCI0.PRES.CPRS == One))
> + {
> + If ((\_SB.PCI0.PRES.CPEN == One))
> + {
> + Local0 = 0x0F
> + }
> + Else
> + {
> + Local0 = 0x0D
> + }
> }
>
> Suggested-by: Igor Mammedov <imamm...@redhat.com>
'Reported-by' maybe, but certainly not suggested.
After more thinking and given presence is system wide that doesn't change
at runtime, I don't see any reason for introducing presence bit as ABI
(and undocumented on top of that).
Instead changing AML code to account for it would be better,
something like this:
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 32654dc274..4a3e591120 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -55,6 +55,7 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
typedef struct CPUHotplugFeatures {
bool acpi_1_compatible;
bool has_legacy_cphp;
+ bool always_present_cpus;
bool fw_unplugs_cpu;
const char *smi_path;
} CPUHotplugFeatures;
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 5cb60ca8bc..2bcce2b31c 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -452,15 +452,16 @@ void build_cpus_aml(Aml *table, MachineState *machine,
CPUHotplugFeatures opts,
method = aml_method(CPU_STS_METHOD, 1, AML_SERIALIZED);
{
+ uint8_t default_sta = opts.always_present_cpus?0xd:0;
Aml *idx = aml_arg(0);
- Aml *sta = aml_local(0);
+ Aml *sta = aml_local(default_sta);
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));
{
- aml_append(ifctx, aml_store(aml_int(0xF), sta));
+ aml_append(ifctx, aml_or(aml_int(0xF), sta, sta));
}
aml_append(method, ifctx);
aml_append(method, aml_release(ctrl_lock))
then for ARM set
CPUHotplugFeatures::always_present_cpus = true
to get present flag always enabled
After that revert _all_ other presence bit related changes
that were just merged.
(I did ask to get rid of that in previous reviews but it
came back again for no good reason).
> Message-ID: <20241106100047.18901...@imammedo.users.ipa.redhat.com>
> Signed-off-by: Salil Mehta <salil.me...@huawei.com>
> ---
> hw/acpi/cpu.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 23443f09a5..b2f7a2b27e 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -490,22 +490,22 @@ void build_cpus_aml(Aml *table, MachineState *machine,
> CPUHotplugFeatures opts,
> 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_present, one));
> + ifctx = aml_if(aml_equal(is_enabled, one));
> {
> - 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 and enabled */
> + aml_append(ifctx, aml_store(aml_int(0xF), sta));
> + }
> + aml_append(method, ifctx);
> + else_ctx = aml_else();
> + {
> + ifctx2 = aml_if(aml_equal(is_present, one));
> {
> /* cpu is present but disabled */
> - aml_append(else_ctx, aml_store(aml_int(0xD), sta));
> + aml_append(ifctx2, aml_store(aml_int(0xD), sta));
> }
> - aml_append(ifctx, else_ctx);
> + aml_append(else_ctx, ifctx2);
> }
> - aml_append(method, ifctx);
> + aml_append(method, else_ctx);
> aml_append(method, aml_release(ctrl_lock));
> aml_append(method, aml_return(sta));
> }
