| Message ID | 20241014192205.253479-4-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arch agnostic ACPI changes to support vCPU Hotplug (on Archs like ARM) | expand |
Hi Salil, On Mon, Oct 14, 2024 at 08:22:04PM +0100, Salil Mehta wrote: > Date: Mon, 14 Oct 2024 20:22:04 +0100 > From: Salil Mehta <salil.mehta@huawei.com> > Subject: [PATCH V1 3/4] hw/acpi: Reflect ACPI vCPU {present,enabled} states > in ACPI _STA.{PRES,ENA} Bits > X-Mailer: git-send-email 2.34.1 > > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the `_STA.PRES` > (presence) and `_STA.ENA` (enabled) bits when the guest kernel evaluates the > ACPI `_STA` method during initialization, as well as when vCPUs are hot-plugged > or hot-unplugged. The presence of unplugged vCPUs may need to be deliberately > *simulated* at the ACPI level to maintain a *persistent* view of vCPUs for the > guest kernel. > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > It seems this patch changes ACPI table layout and then breaks current ACPI table qtest. I'm not sure how to do such modifications. Maybe you should first disable the related checks, then modify the code, update the qtest, and finally re-enable the checks for qtest. This can help to avoid any qtest failure due to this patch? I think it should get Igor's advice on this. :) Attach the error I met: ▶ 2/920 ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: (all_tables_match) ERROR ▶ 3/920 ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: (all_tables_match) ERROR 2/920 qemu:qtest+qtest-i386 / qtest-i386/bios-tables-test ERROR 1.24s killed by signal 6 SIGABRT >>> G_TEST_DBUS_DAEMON=/media/liuzhao/data/qemu-cook/tests/dbus-vmstate-daemon.sh ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MESON_TEST_ITERATION=1 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_BINARY=./qemu-system-i386 MALLOC_PERTURB_=142 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img PYTHON=/media/liuzhao/data/qemu-cook/build/pyvenv/bin/python3 /media/liuzhao/data/qemu-cook/build/tests/qtest/bios-tables-test --tap -k ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― stderr: acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml-VRT5V2], Expected [aml:tests/data/acpi/x86/pc/DSDT]. See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-TTT5V2.dsl, aml:/tmp/aml-VRT5V2], Expected [asl:/tmp/asl-XXM5V2.dsl, aml:tests/data/acpi/x86/pc/DSDT]. ** ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: (all_tables_match) (test program exited with status code -6) ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― 3/920 qemu:qtest+qtest-x86_64 / qtest-x86_64/bios-tables-test ERROR 1.25s killed by signal 6 SIGABRT >>> G_TEST_DBUS_DAEMON=/media/liuzhao/data/qemu-cook/tests/dbus-vmstate-daemon.sh ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MESON_TEST_ITERATION=1 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img PYTHON=/media/liuzhao/data/qemu-cook/build/pyvenv/bin/python3 MALLOC_PERTURB_=41 QTEST_QEMU_BINARY=./qemu-system-x86_64 /media/liuzhao/data/qemu-cook/build/tests/qtest/bios-tables-test --tap -k ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― stderr: acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml-D5K5V2], Expected [aml:tests/data/acpi/x86/pc/DSDT]. See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-G6K5V2.dsl, aml:/tmp/aml-D5K5V2], Expected [asl:/tmp/asl-AQD5V2.dsl, aml:tests/data/acpi/x86/pc/DSDT]. ** ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: (all_tables_match) (test program exited with status code -6) Regards, Zhao
On Fri, 18 Oct 2024 13:12:52 +0800 Zhao Liu <zhao1.liu@intel.com> wrote: > Hi Salil, > > On Mon, Oct 14, 2024 at 08:22:04PM +0100, Salil Mehta wrote: > > Date: Mon, 14 Oct 2024 20:22:04 +0100 > > From: Salil Mehta <salil.mehta@huawei.com> > > Subject: [PATCH V1 3/4] hw/acpi: Reflect ACPI vCPU {present,enabled} states > > in ACPI _STA.{PRES,ENA} Bits > > X-Mailer: git-send-email 2.34.1 > > > > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the `_STA.PRES` > > (presence) and `_STA.ENA` (enabled) bits when the guest kernel evaluates the > > ACPI `_STA` method during initialization, as well as when vCPUs are hot-plugged > > or hot-unplugged. The presence of unplugged vCPUs may need to be deliberately > > *simulated* at the ACPI level to maintain a *persistent* view of vCPUs for the > > guest kernel. > > > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > --- > > hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > It seems this patch changes ACPI table layout and then breaks current > ACPI table qtest. I'm not sure how to do such modifications. Maybe you > should first disable the related checks, then modify the code, update > the qtest, and finally re-enable the checks for qtest. This can help > to avoid any qtest failure due to this patch? see comment at the top of tests/qtest/bios-tables-test.c > > I think it should get Igor's advice on this. :) > > Attach the error I met: > > ▶ 2/920 ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: (all_tables_match) ERROR > ▶ 3/920 ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: (all_tables_match) ERROR > 2/920 qemu:qtest+qtest-i386 / qtest-i386/bios-tables-test ERROR 1.24s killed by signal 6 SIGABRT > >>> G_TEST_DBUS_DAEMON=/media/liuzhao/data/qemu-cook/tests/dbus-vmstate-daemon.sh ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MESON_TEST_ITERATION=1 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_BINARY=./qemu-system-i386 MALLOC_PERTURB_=142 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img PYTHON=/media/liuzhao/data/qemu-cook/build/pyvenv/bin/python3 /media/liuzhao/data/qemu-cook/build/tests/qtest/bios-tables-test --tap -k > ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― > stderr: > acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml-VRT5V2], Expected [aml:tests/data/acpi/x86/pc/DSDT]. > See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. > acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-TTT5V2.dsl, aml:/tmp/aml-VRT5V2], Expected [asl:/tmp/asl-XXM5V2.dsl, aml:tests/data/acpi/x86/pc/DSDT]. > ** > ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: (all_tables_match) > > (test program exited with status code -6) > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― > > 3/920 qemu:qtest+qtest-x86_64 / qtest-x86_64/bios-tables-test ERROR 1.25s killed by signal 6 SIGABRT > >>> G_TEST_DBUS_DAEMON=/media/liuzhao/data/qemu-cook/tests/dbus-vmstate-daemon.sh ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MESON_TEST_ITERATION=1 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img PYTHON=/media/liuzhao/data/qemu-cook/build/pyvenv/bin/python3 MALLOC_PERTURB_=41 QTEST_QEMU_BINARY=./qemu-system-x86_64 /media/liuzhao/data/qemu-cook/build/tests/qtest/bios-tables-test --tap -k > ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― > stderr: > acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml-D5K5V2], Expected [aml:tests/data/acpi/x86/pc/DSDT]. > See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. > acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-G6K5V2.dsl, aml:/tmp/aml-D5K5V2], Expected [asl:/tmp/asl-AQD5V2.dsl, aml:tests/data/acpi/x86/pc/DSDT]. > ** > ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: (all_tables_match) > > (test program exited with status code -6) > > > Regards, > Zhao > >
On Mon, 14 Oct 2024 20:22:04 +0100 Salil Mehta <salil.mehta@huawei.com> wrote: > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the `_STA.PRES` > (presence) and `_STA.ENA` (enabled) bits when the guest kernel evaluates the > ACPI `_STA` method during initialization, as well as when vCPUs are hot-plugged > or hot-unplugged. The presence of unplugged vCPUs may need to be deliberately > *simulated* at the ACPI level to maintain a *persistent* view of vCPUs for the > guest kernel. given questionable future of is_present/is_enabled fields, it probably premature to review this part. The only thing, I have to say here is repeating spec/doc update patch describing how it should work should come 1st, so that we could compare this impl. with it. > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 700aa855e9..23ea2b9c70 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -63,10 +63,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 |= cdev->is_enabled ? 1 : 0; > val |= cdev->is_inserting ? 2 : 0; > val |= cdev->is_removing ? 4 : 0; > val |= cdev->fw_remove ? 16 : 0; > + val |= cdev->is_present ? 32 : 0; > trace_cpuhp_acpi_read_flags(cpu_st->selector, val); > break; > case ACPI_CPU_CMD_DATA_OFFSET_RW: > @@ -376,6 +377,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, > @@ -436,7 +438,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); > > @@ -466,6 +470,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); > @@ -494,13 +499,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));
Hi Salil, On 10/14/24 16:22, Salil Mehta wrote: > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the `_STA.PRES` > (presence) and `_STA.ENA` (enabled) bits when the guest kernel evaluates the > ACPI `_STA` method during initialization, as well as when vCPUs are hot-plugged > or hot-unplugged. The presence of unplugged vCPUs may need to be deliberately > *simulated* at the ACPI level to maintain a *persistent* view of vCPUs for the > guest kernel. > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 700aa855e9..23ea2b9c70 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -63,10 +63,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 |= cdev->is_enabled ? 1 : 0; > val |= cdev->is_inserting ? 2 : 0; > val |= cdev->is_removing ? 4 : 0; > val |= cdev->fw_remove ? 16 : 0; > + val |= cdev->is_present ? 32 : 0; > trace_cpuhp_acpi_read_flags(cpu_st->selector, val); > break; > case ACPI_CPU_CMD_DATA_OFFSET_RW: > @@ -376,6 +377,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, > @@ -436,7 +438,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); > > @@ -466,6 +470,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); > @@ -494,13 +499,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)); Here, the return value for _STA method is set to reflect the state of CPU_PRESENT and CPU_ENABLED fields. I can't see these two fields being mapped to AcpiCpuStatus.{is_present,is_enabled}. They look to be mapped to the MMIO region (base_addr), which doesn't mapped to AcpiCpuStatus afaics. So where CPU_PRESENT and CPU_ENABLED are set and where exactly they reside? Cheers, Gustavo > + } > + aml_append(ifctx, else_ctx); > } > aml_append(method, ifctx); > aml_append(method, aml_release(ctrl_lock));
Hi Zhao, Sorry, for the late reply. I was away last week with only intermittent access to the mails. > From: Zhao Liu <zhao1.liu@intel.com> > Sent: Friday, October 18, 2024 6:13 AM > To: Salil Mehta <salil.mehta@huawei.com> > > Hi Salil, > > On Mon, Oct 14, 2024 at 08:22:04PM +0100, Salil Mehta wrote: > > Date: Mon, 14 Oct 2024 20:22:04 +0100 > > From: Salil Mehta <salil.mehta@huawei.com> > > Subject: [PATCH V1 3/4] hw/acpi: Reflect ACPI vCPU {present,enabled} > > states in ACPI _STA.{PRES,ENA} Bits > > X-Mailer: git-send-email 2.34.1 > > > > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the > > `_STA.PRES` > > (presence) and `_STA.ENA` (enabled) bits when the guest kernel > > evaluates the ACPI `_STA` method during initialization, as well as > > when vCPUs are hot-plugged or hot-unplugged. The presence of > unplugged > > vCPUs may need to be deliberately > > *simulated* at the ACPI level to maintain a *persistent* view of vCPUs > > for the guest kernel. > > > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > --- > > hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > It seems this patch changes ACPI table layout and then breaks current ACPI > table qtest. I'm not sure how to do such modifications. Maybe you should > first disable the related checks, then modify the code, update the qtest, and > finally re-enable the checks for qtest. This can help to avoid any qtest failure > due to this patch? Thanks for reporting. Let me get back to you on this. > > I think it should get Igor's advice on this. :) > > Attach the error I met: > > ▶ 2/920 ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: > assertion failed: (all_tables_match) ERROR > ▶ 3/920 ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: > assertion failed: (all_tables_match) ERROR > 2/920 qemu:qtest+qtest-i386 / qtest-i386/bios-tables-test > ERROR 1.24s killed by signal 6 SIGABRT > >>> G_TEST_DBUS_DAEMON=/media/liuzhao/data/qemu- > cook/tests/dbus-vmstate- > >>> daemon.sh > >>> ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 > >>> MESON_TEST_ITERATION=1 > >>> > UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:pr > int > >>> _stacktrace=1 QTEST_QEMU_BINARY=./qemu-system-i386 > >>> MALLOC_PERTURB_=142 > >>> > MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:pri > nt_ > >>> stacktrace=1 > >>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage- > daemon/qemu-storage-daemo > >>> n QTEST_QEMU_IMG=./qemu-img > >>> PYTHON=/media/liuzhao/data/qemu-cook/build/pyvenv/bin/python3 > >>> /media/liuzhao/data/qemu-cook/build/tests/qtest/bios-tables-test > >>> --tap -k > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ――――――― ✀ > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ――――――― > stderr: > acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml- > VRT5V2], Expected [aml:tests/data/acpi/x86/pc/DSDT]. > See source file tests/qtest/bios-tables-test.c for instructions on how to > update expected files. > acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-TTT5V2.dsl, > aml:/tmp/aml-VRT5V2], Expected [asl:/tmp/asl-XXM5V2.dsl, > aml:tests/data/acpi/x86/pc/DSDT]. > ** > ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: > (all_tables_match) > > (test program exited with status code -6) > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ―――――――――――――――――― > > 3/920 qemu:qtest+qtest-x86_64 / qtest-x86_64/bios-tables-test > ERROR 1.25s killed by signal 6 SIGABRT > >>> G_TEST_DBUS_DAEMON=/media/liuzhao/data/qemu- > cook/tests/dbus-vmstate- > >>> daemon.sh > >>> ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 > >>> MESON_TEST_ITERATION=1 > >>> > UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:pr > int > >>> _stacktrace=1 > >>> > MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:pri > nt_ > >>> stacktrace=1 > >>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage- > daemon/qemu-storage-daemo > >>> n QTEST_QEMU_IMG=./qemu-img > >>> PYTHON=/media/liuzhao/data/qemu-cook/build/pyvenv/bin/python3 > >>> MALLOC_PERTURB_=41 QTEST_QEMU_BINARY=./qemu-system- > x86_64 > >>> /media/liuzhao/data/qemu-cook/build/tests/qtest/bios-tables-test > >>> --tap -k > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ――――――― ✀ > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ――――――― > stderr: > acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml- > D5K5V2], Expected [aml:tests/data/acpi/x86/pc/DSDT]. > See source file tests/qtest/bios-tables-test.c for instructions on how to > update expected files. > acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-G6K5V2.dsl, > aml:/tmp/aml-D5K5V2], Expected [asl:/tmp/asl-AQD5V2.dsl, > aml:tests/data/acpi/x86/pc/DSDT]. > ** > ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion failed: > (all_tables_match) > > (test program exited with status code -6) > > > Regards, > Zhao > >
> From: Igor Mammedov <imammedo@redhat.com> > Sent: Friday, October 18, 2024 3:19 PM > To: Zhao Liu <zhao1.liu@intel.com> > Cc: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; > qemu-arm@nongnu.org; mst@redhat.com; maz@kernel.org; jean- > philippe@linaro.org; Jonathan Cameron > <jonathan.cameron@huawei.com>; lpieralisi@kernel.org; > peter.maydell@linaro.org; richard.henderson@linaro.org; > andrew.jones@linux.dev; david@redhat.com; philmd@linaro.org; > eric.auger@redhat.com; will@kernel.org; ardb@kernel.org; > oliver.upton@linux.dev; pbonzini@redhat.com; gshan@redhat.com; > rafael@kernel.org; borntraeger@linux.ibm.com; alex.bennee@linaro.org; > npiggin@gmail.com; harshpb@linux.ibm.com; linux@armlinux.org.uk; > darren@os.amperecomputing.com; ilkka@os.amperecomputing.com; > vishnu@os.amperecomputing.com; karl.heubaum@oracle.com; > miguel.luis@oracle.com; salil.mehta@opnsrc.net; zhukeqian > <zhukeqian1@huawei.com>; wangxiongfeng (C) > <wangxiongfeng2@huawei.com>; wangyanan (Y) > <wangyanan55@huawei.com>; jiakernel2@gmail.com; > maobibo@loongson.cn; lixianglai@loongson.cn; shahuang@redhat.com; > Linuxarm <linuxarm@huawei.com>; gustavo.romero@linaro.org > Subject: Re: [PATCH V1 3/4] hw/acpi: Reflect ACPI vCPU {present,enabled} > states in ACPI _STA.{PRES,ENA} Bits > > On Fri, 18 Oct 2024 13:12:52 +0800 > Zhao Liu <zhao1.liu@intel.com> wrote: > > > Hi Salil, > > > > On Mon, Oct 14, 2024 at 08:22:04PM +0100, Salil Mehta wrote: > > > Date: Mon, 14 Oct 2024 20:22:04 +0100 > > > From: Salil Mehta <salil.mehta@huawei.com> > > > Subject: [PATCH V1 3/4] hw/acpi: Reflect ACPI vCPU {present,enabled} > > > states in ACPI _STA.{PRES,ENA} Bits > > > X-Mailer: git-send-email 2.34.1 > > > > > > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the > > > `_STA.PRES` > > > (presence) and `_STA.ENA` (enabled) bits when the guest kernel > > > evaluates the ACPI `_STA` method during initialization, as well as > > > when vCPUs are hot-plugged or hot-unplugged. The presence of > > > unplugged vCPUs may need to be deliberately > > > *simulated* at the ACPI level to maintain a *persistent* view of > > > vCPUs for the guest kernel. > > > > > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > > --- > > > hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- > > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > > > > It seems this patch changes ACPI table layout and then breaks current > > ACPI table qtest. I'm not sure how to do such modifications. Maybe you > > should first disable the related checks, then modify the code, update > > the qtest, and finally re-enable the checks for qtest. This can help > > to avoid any qtest failure due to this patch? > > see comment at the top of tests/qtest/bios-tables-test.c Thanks for the pointers. > > > > > I think it should get Igor's advice on this. :) > > > > Attach the error I met: > > > > ▶ 2/920 ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: > assertion failed: (all_tables_match) ERROR > > ▶ 3/920 ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: > assertion failed: (all_tables_match) ERROR > > 2/920 qemu:qtest+qtest-i386 / qtest-i386/bios-tables-test > ERROR 1.24s killed by signal 6 SIGABRT > > >>> G_TEST_DBUS_DAEMON=/media/liuzhao/data/qemu- > cook/tests/dbus-vmstat > > >>> e-daemon.sh > > >>> > ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 > > >>> MESON_TEST_ITERATION=1 > > >>> > UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:pr > i > > >>> nt_stacktrace=1 QTEST_QEMU_BINARY=./qemu-system-i386 > > >>> MALLOC_PERTURB_=142 > > >>> > MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:pri > n > > >>> t_stacktrace=1 > > >>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage- > daemon/qemu-storage-dae > > >>> mon QTEST_QEMU_IMG=./qemu-img > > >>> PYTHON=/media/liuzhao/data/qemu- > cook/build/pyvenv/bin/python3 > > >>> /media/liuzhao/data/qemu-cook/build/tests/qtest/bios-tables-test > > >>> --tap -k > > > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ―――― > > ――― ✀ > > > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ―――― > > ――― > > stderr: > > acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml- > VRT5V2], Expected [aml:tests/data/acpi/x86/pc/DSDT]. > > See source file tests/qtest/bios-tables-test.c for instructions on how to > update expected files. > > acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-TTT5V2.dsl, > aml:/tmp/aml-VRT5V2], Expected [asl:/tmp/asl-XXM5V2.dsl, > aml:tests/data/acpi/x86/pc/DSDT]. > > ** > > ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion > > failed: (all_tables_match) > > > > (test program exited with status code -6) > > > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ―――― > > > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ―――― > > ―――――――――― > > > > 3/920 qemu:qtest+qtest-x86_64 / qtest-x86_64/bios-tables-test > ERROR 1.25s killed by signal 6 SIGABRT > > >>> G_TEST_DBUS_DAEMON=/media/liuzhao/data/qemu- > cook/tests/dbus-vmstat > > >>> e-daemon.sh > > >>> > ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 > > >>> MESON_TEST_ITERATION=1 > > >>> > UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:pr > i > > >>> nt_stacktrace=1 > > >>> > MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:pri > n > > >>> t_stacktrace=1 > > >>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage- > daemon/qemu-storage-dae > > >>> mon QTEST_QEMU_IMG=./qemu-img > > >>> PYTHON=/media/liuzhao/data/qemu- > cook/build/pyvenv/bin/python3 > > >>> MALLOC_PERTURB_=41 QTEST_QEMU_BINARY=./qemu-system- > x86_64 > > >>> /media/liuzhao/data/qemu-cook/build/tests/qtest/bios-tables-test > > >>> --tap -k > > > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ―――― > > ――― ✀ > > > ――――――――――――――――――――――――――――――――― > ――――――――――――――――――――――――――――――――― > ―――― > > ――― > > stderr: > > acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml- > D5K5V2], Expected [aml:tests/data/acpi/x86/pc/DSDT]. > > See source file tests/qtest/bios-tables-test.c for instructions on how to > update expected files. > > acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-G6K5V2.dsl, > aml:/tmp/aml-D5K5V2], Expected [asl:/tmp/asl-AQD5V2.dsl, > aml:tests/data/acpi/x86/pc/DSDT]. > > ** > > ERROR:../tests/qtest/bios-tables-test.c:553:test_acpi_asl: assertion > > failed: (all_tables_match) > > > > (test program exited with status code -6) > > > > > > Regards, > > Zhao > > > > >
Hi Igor, > From: Igor Mammedov <imammedo@redhat.com> > Sent: Friday, October 18, 2024 3:25 PM > To: Salil Mehta <salil.mehta@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; mst@redhat.com; > maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron > <jonathan.cameron@huawei.com>; lpieralisi@kernel.org; > peter.maydell@linaro.org; richard.henderson@linaro.org; > andrew.jones@linux.dev; david@redhat.com; philmd@linaro.org; > eric.auger@redhat.com; will@kernel.org; ardb@kernel.org; > oliver.upton@linux.dev; pbonzini@redhat.com; gshan@redhat.com; > rafael@kernel.org; borntraeger@linux.ibm.com; alex.bennee@linaro.org; > npiggin@gmail.com; harshpb@linux.ibm.com; linux@armlinux.org.uk; > darren@os.amperecomputing.com; ilkka@os.amperecomputing.com; > vishnu@os.amperecomputing.com; karl.heubaum@oracle.com; > miguel.luis@oracle.com; salil.mehta@opnsrc.net; zhukeqian > <zhukeqian1@huawei.com>; wangxiongfeng (C) > <wangxiongfeng2@huawei.com>; wangyanan (Y) > <wangyanan55@huawei.com>; jiakernel2@gmail.com; > maobibo@loongson.cn; lixianglai@loongson.cn; shahuang@redhat.com; > zhao1.liu@intel.com; Linuxarm <linuxarm@huawei.com>; > gustavo.romero@linaro.org > Subject: Re: [PATCH V1 3/4] hw/acpi: Reflect ACPI vCPU {present,enabled} > states in ACPI _STA.{PRES,ENA} Bits > > On Mon, 14 Oct 2024 20:22:04 +0100 > Salil Mehta <salil.mehta@huawei.com> wrote: > > > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the > > `_STA.PRES` > > (presence) and `_STA.ENA` (enabled) bits when the guest kernel > > evaluates the ACPI `_STA` method during initialization, as well as > > when vCPUs are hot-plugged or hot-unplugged. The presence of > unplugged > > vCPUs may need to be deliberately > > *simulated* at the ACPI level to maintain a *persistent* view of vCPUs > > for the guest kernel. > > given questionable future of is_present/is_enabled fields, it probably > premature to review this part. Sure, let us discuss the pain points. > The only thing, I have to say here is repeating spec/doc update patch > describing how it should work should come 1st, so that we could compare > this impl. with it. From the user perspective, there should not be any change in how he plug/unplugs the ARM vCPUs. This is a requirement, as we have to assists folks who wants to migrate from x86 world to ARM world seamlessly (without affecting performance parameters) and this feature is one small part of that bigger effort. > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > --- > > hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index > > 700aa855e9..23ea2b9c70 100644 > > --- a/hw/acpi/cpu.c > > +++ b/hw/acpi/cpu.c > > @@ -63,10 +63,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 |= cdev->is_enabled ? 1 : 0; > > val |= cdev->is_inserting ? 2 : 0; > > val |= cdev->is_removing ? 4 : 0; > > val |= cdev->fw_remove ? 16 : 0; > > + val |= cdev->is_present ? 32 : 0; > > trace_cpuhp_acpi_read_flags(cpu_st->selector, val); > > break; > > case ACPI_CPU_CMD_DATA_OFFSET_RW: > > @@ -376,6 +377,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, @@ -436,7 +438,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); > > > > @@ -466,6 +470,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); @@ -494,13 +499,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)); >
Hi Gustavo, > From: Gustavo Romero <gustavo.romero@linaro.org> > Sent: Monday, October 21, 2024 3:10 AM > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; > qemu-arm@nongnu.org; mst@redhat.com > > Hi Salil, > > On 10/14/24 16:22, Salil Mehta wrote: > > Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the > > `_STA.PRES` > > (presence) and `_STA.ENA` (enabled) bits when the guest kernel > > evaluates the ACPI `_STA` method during initialization, as well as > > when vCPUs are hot-plugged or hot-unplugged. The presence of > unplugged > > vCPUs may need to be deliberately > > *simulated* at the ACPI level to maintain a *persistent* view of vCPUs > > for the guest kernel. > > > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > --- > > hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index > > 700aa855e9..23ea2b9c70 100644 > > --- a/hw/acpi/cpu.c > > +++ b/hw/acpi/cpu.c > > @@ -63,10 +63,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 |= cdev->is_enabled ? 1 : 0; > > val |= cdev->is_inserting ? 2 : 0; > > val |= cdev->is_removing ? 4 : 0; > > val |= cdev->fw_remove ? 16 : 0; > > + val |= cdev->is_present ? 32 : 0; > > trace_cpuhp_acpi_read_flags(cpu_st->selector, val); > > break; > > case ACPI_CPU_CMD_DATA_OFFSET_RW: > > @@ -376,6 +377,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, @@ -436,7 +438,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); > > > > @@ -466,6 +470,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); @@ -494,13 +499,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)); > > Here, the return value for _STA method is set to reflect the state of > CPU_PRESENT and CPU_ENABLED fields. I can't see these two fields being > mapped to AcpiCpuStatus.{is_present,is_enabled}. They look to be mapped > to the MMIO region (base_addr), which doesn't mapped to AcpiCpuStatus > afaics. So where CPU_PRESENT and CPU_ENABLED are set and where > exactly they reside? You don’t set _STA field from guest for CPUs. We only fetch values being reflected by the VMM. In general, ACPI _STA method is for fetching status of a device. Please check below: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html?highlight=_sta#sta-device-status We are banking on below status bits: Bit [0] - Set if the device is present. Bit [1] - Set if the device is enabled and decoding its resources. Hence, AcpiCpuStatus::(is_present, is_enabled} are set in the VMM and their status is shared to the guest kernel when the ACPI _STA method is evaluated by the OSPM. Patch 3/4: https://lore.kernel.org/qemu-devel/20241014192205.253479-4-salil.mehta@huawei.com/ #HUNK diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index 700aa855e9..23ea2b9c70 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -63,10 +63,11 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size) cdev = &cpu_st->devs[cpu_st->selector];---------> check this code excerpt switch (addr) { case ACPI_CPU_FLAGS_OFFSET_RW: /* pack and return is_* fields */ - val |= cdev->cpu ? 1 : 0; + val |= cdev->is_enabled ? 1 : 0;---------------> this change val |= cdev->is_inserting ? 2 : 0; val |= cdev->is_removing ? 4 : 0; val |= cdev->fw_remove ? 16 : 0; + val |= cdev->is_present ? 32 : 0;-------------> and this change trace_cpuhp_acpi_read_flags(cpu_st->selector, val); break; case ACPI_CPU_CMD_DATA_OFFSET_RW: Thanks > > > Cheers, > Gustavo > > > + } > > + aml_append(ifctx, else_ctx); > > } > > aml_append(method, ifctx); > > aml_append(method, aml_release(ctrl_lock));
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index 700aa855e9..23ea2b9c70 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -63,10 +63,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 |= cdev->is_enabled ? 1 : 0; val |= cdev->is_inserting ? 2 : 0; val |= cdev->is_removing ? 4 : 0; val |= cdev->fw_remove ? 16 : 0; + val |= cdev->is_present ? 32 : 0; trace_cpuhp_acpi_read_flags(cpu_st->selector, val); break; case ACPI_CPU_CMD_DATA_OFFSET_RW: @@ -376,6 +377,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, @@ -436,7 +438,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); @@ -466,6 +470,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); @@ -494,13 +499,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));
Reflect the ACPI CPU hotplug `is_{present, enabled}` states in the `_STA.PRES` (presence) and `_STA.ENA` (enabled) bits when the guest kernel evaluates the ACPI `_STA` method during initialization, as well as when vCPUs are hot-plugged or hot-unplugged. The presence of unplugged vCPUs may need to be deliberately *simulated* at the ACPI level to maintain a *persistent* view of vCPUs for the guest kernel. Signed-off-by: Salil Mehta <salil.mehta@huawei.com> --- hw/acpi/cpu.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)