* [PATCH 0/3] Fixes CPUs AML & acpi-bios-tables to be x86 backward compatible
@ 2024-11-06 13:03 Salil Mehta via
2024-11-06 13:03 ` [PATCH 1/3] qtest: allow ACPI DSDT Table changes Salil Mehta via
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Salil Mehta via @ 2024-11-06 13:03 UTC (permalink / raw)
To: qemu-devel, qemu-arm, mst, imammedo
Cc: salil.mehta, jonathan.cameron, peter.maydell, richard.henderson,
anisinha, eduardo, marcel.apfelbaum, david, philmd, peterx,
pbonzini, gshan, borntraeger, alex.bennee, linux, darren, ilkka,
vishnu, karl.heubaum, miguel.luis, salil.mehta, zhukeqian1,
wangxiongfeng2, wangyanan55, zhao1.liu, linuxarm, gustavo.romero
Fixes the the CPUs AML code and its corresponding golden masters ACPI
tables files for backward compatability of live migration on x86 platforms
i.e. when newer Qemu is migrated to older Qemu without `CPRS` Bit present
in the register block.
Fixes [PULL 60/65], [PULL 61/65]:
Message-ID: <bf1ecc8dad6061914730a2a2d57af6b37c3a4f8d.1730754238.git.mst@redhat.com>
Message-ID: <4d62d15b11909e9af121577e707b88f2e4524371.1730754238.git.mst@redhat.com>
Salil Mehta (3):
qtest: allow ACPI DSDT Table changes
Fix: Reverse CPUs presence check logic for x86 backward compatability
tests/qtest/bios-tables-test: Fix DSDT golden masters for x86/{pc,q35}
hw/acpi/cpu.c | 22 +++++++++---------
tests/data/acpi/x86/pc/DSDT | Bin 8561 -> 8561 bytes
tests/data/acpi/x86/pc/DSDT.acpierst | Bin 8472 -> 8472 bytes
tests/data/acpi/x86/pc/DSDT.acpihmat | Bin 9886 -> 9886 bytes
tests/data/acpi/x86/pc/DSDT.bridge | Bin 15432 -> 15432 bytes
tests/data/acpi/x86/pc/DSDT.cphp | Bin 9025 -> 9025 bytes
tests/data/acpi/x86/pc/DSDT.dimmpxm | Bin 10215 -> 10215 bytes
tests/data/acpi/x86/pc/DSDT.hpbridge | Bin 8512 -> 8512 bytes
tests/data/acpi/x86/pc/DSDT.hpbrroot | Bin 5068 -> 5068 bytes
tests/data/acpi/x86/pc/DSDT.ipmikcs | Bin 8633 -> 8633 bytes
tests/data/acpi/x86/pc/DSDT.memhp | Bin 9920 -> 9920 bytes
tests/data/acpi/x86/pc/DSDT.nohpet | Bin 8419 -> 8419 bytes
tests/data/acpi/x86/pc/DSDT.numamem | Bin 8567 -> 8567 bytes
tests/data/acpi/x86/pc/DSDT.roothp | Bin 12354 -> 12354 bytes
tests/data/acpi/x86/q35/DSDT | Bin 8389 -> 8389 bytes
tests/data/acpi/x86/q35/DSDT.acpierst | Bin 8406 -> 8406 bytes
tests/data/acpi/x86/q35/DSDT.acpihmat | Bin 9714 -> 9714 bytes
.../acpi/x86/q35/DSDT.acpihmat-noinitiator | Bin 8668 -> 8668 bytes
tests/data/acpi/x86/q35/DSDT.applesmc | Bin 8435 -> 8435 bytes
tests/data/acpi/x86/q35/DSDT.bridge | Bin 12002 -> 12002 bytes
tests/data/acpi/x86/q35/DSDT.core-count | Bin 12947 -> 12947 bytes
tests/data/acpi/x86/q35/DSDT.core-count2 | Bin 33804 -> 33804 bytes
tests/data/acpi/x86/q35/DSDT.cphp | Bin 8853 -> 8853 bytes
tests/data/acpi/x86/q35/DSDT.cxl | Bin 13182 -> 13182 bytes
tests/data/acpi/x86/q35/DSDT.dimmpxm | Bin 10043 -> 10043 bytes
tests/data/acpi/x86/q35/DSDT.ipmibt | Bin 8464 -> 8464 bytes
tests/data/acpi/x86/q35/DSDT.ipmismbus | Bin 8477 -> 8477 bytes
tests/data/acpi/x86/q35/DSDT.ivrs | Bin 8406 -> 8406 bytes
tests/data/acpi/x86/q35/DSDT.memhp | Bin 9748 -> 9748 bytes
tests/data/acpi/x86/q35/DSDT.mmio64 | Bin 9519 -> 9519 bytes
tests/data/acpi/x86/q35/DSDT.multi-bridge | Bin 13242 -> 13242 bytes
tests/data/acpi/x86/q35/DSDT.noacpihp | Bin 8269 -> 8269 bytes
tests/data/acpi/x86/q35/DSDT.nohpet | Bin 8247 -> 8247 bytes
tests/data/acpi/x86/q35/DSDT.numamem | Bin 8395 -> 8395 bytes
tests/data/acpi/x86/q35/DSDT.pvpanic-isa | Bin 8490 -> 8490 bytes
tests/data/acpi/x86/q35/DSDT.thread-count | Bin 12947 -> 12947 bytes
tests/data/acpi/x86/q35/DSDT.thread-count2 | Bin 33804 -> 33804 bytes
tests/data/acpi/x86/q35/DSDT.tis.tpm12 | Bin 8995 -> 8995 bytes
tests/data/acpi/x86/q35/DSDT.tis.tpm2 | Bin 9021 -> 9021 bytes
tests/data/acpi/x86/q35/DSDT.type4-count | Bin 18623 -> 18623 bytes
tests/data/acpi/x86/q35/DSDT.viot | Bin 14649 -> 14649 bytes
tests/data/acpi/x86/q35/DSDT.xapic | Bin 35752 -> 35752 bytes
42 files changed, 11 insertions(+), 11 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] qtest: allow ACPI DSDT Table changes
2024-11-06 13:03 [PATCH 0/3] Fixes CPUs AML & acpi-bios-tables to be x86 backward compatible Salil Mehta via
@ 2024-11-06 13:03 ` Salil Mehta via
2024-11-06 13:03 ` [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability Salil Mehta via
2024-11-06 13:03 ` [PATCH 3/3] tests/qtest/bios-tables-test: Fix DSDT golden masters for x86/{pc, q35} Salil Mehta via
2 siblings, 0 replies; 11+ messages in thread
From: Salil Mehta via @ 2024-11-06 13:03 UTC (permalink / raw)
To: qemu-devel, qemu-arm, mst, imammedo
Cc: salil.mehta, jonathan.cameron, peter.maydell, richard.henderson,
anisinha, eduardo, marcel.apfelbaum, david, philmd, peterx,
pbonzini, gshan, borntraeger, alex.bennee, linux, darren, ilkka,
vishnu, karl.heubaum, miguel.luis, salil.mehta, zhukeqian1,
wangxiongfeng2, wangyanan55, zhao1.liu, linuxarm, gustavo.romero
list changed files in tests/qtest/bios-tables-test-allowed-diff.h
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Message-ID: <20241106100047.18901c9d@imammedo.users.ipa.redhat.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 41 +++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..512d40665d 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,42 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/x86/pc/DSDT",
+"tests/data/acpi/x86/pc/DSDT.acpierst",
+"tests/data/acpi/x86/pc/DSDT.acpihmat",
+"tests/data/acpi/x86/pc/DSDT.bridge",
+"tests/data/acpi/x86/pc/DSDT.cphp",
+"tests/data/acpi/x86/pc/DSDT.dimmpxm",
+"tests/data/acpi/x86/pc/DSDT.hpbridge",
+"tests/data/acpi/x86/pc/DSDT.hpbrroot",
+"tests/data/acpi/x86/pc/DSDT.ipmikcs",
+"tests/data/acpi/x86/pc/DSDT.memhp",
+"tests/data/acpi/x86/pc/DSDT.nohpet",
+"tests/data/acpi/x86/pc/DSDT.numamem",
+"tests/data/acpi/x86/pc/DSDT.roothp",
+"tests/data/acpi/x86/q35/DSDT",
+"tests/data/acpi/x86/q35/DSDT.acpierst",
+"tests/data/acpi/x86/q35/DSDT.acpihmat",
+"tests/data/acpi/x86/q35/DSDT.acpihmat-noinitiator",
+"tests/data/acpi/x86/q35/DSDT.applesmc",
+"tests/data/acpi/x86/q35/DSDT.bridge",
+"tests/data/acpi/x86/q35/DSDT.core-count",
+"tests/data/acpi/x86/q35/DSDT.core-count2",
+"tests/data/acpi/x86/q35/DSDT.cphp",
+"tests/data/acpi/x86/q35/DSDT.cxl",
+"tests/data/acpi/x86/q35/DSDT.dimmpxm",
+"tests/data/acpi/x86/q35/DSDT.ipmibt",
+"tests/data/acpi/x86/q35/DSDT.ipmismbus",
+"tests/data/acpi/x86/q35/DSDT.ivrs",
+"tests/data/acpi/x86/q35/DSDT.memhp",
+"tests/data/acpi/x86/q35/DSDT.mmio64",
+"tests/data/acpi/x86/q35/DSDT.multi-bridge",
+"tests/data/acpi/x86/q35/DSDT.noacpihp",
+"tests/data/acpi/x86/q35/DSDT.nohpet",
+"tests/data/acpi/x86/q35/DSDT.numamem",
+"tests/data/acpi/x86/q35/DSDT.pvpanic-isa",
+"tests/data/acpi/x86/q35/DSDT.thread-count",
+"tests/data/acpi/x86/q35/DSDT.thread-count2",
+"tests/data/acpi/x86/q35/DSDT.tis.tpm12",
+"tests/data/acpi/x86/q35/DSDT.tis.tpm2",
+"tests/data/acpi/x86/q35/DSDT.type4-count",
+"tests/data/acpi/x86/q35/DSDT.viot",
+"tests/data/acpi/x86/q35/DSDT.xapic",
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability
2024-11-06 13:03 [PATCH 0/3] Fixes CPUs AML & acpi-bios-tables to be x86 backward compatible Salil Mehta via
2024-11-06 13:03 ` [PATCH 1/3] qtest: allow ACPI DSDT Table changes Salil Mehta via
@ 2024-11-06 13:03 ` Salil Mehta via
2024-11-06 13:56 ` Igor Mammedov
2024-11-06 13:03 ` [PATCH 3/3] tests/qtest/bios-tables-test: Fix DSDT golden masters for x86/{pc, q35} Salil Mehta via
2 siblings, 1 reply; 11+ messages in thread
From: Salil Mehta via @ 2024-11-06 13:03 UTC (permalink / raw)
To: qemu-devel, qemu-arm, mst, imammedo
Cc: salil.mehta, jonathan.cameron, peter.maydell, richard.henderson,
anisinha, eduardo, marcel.apfelbaum, david, philmd, peterx,
pbonzini, gshan, borntraeger, alex.bennee, linux, darren, ilkka,
vishnu, karl.heubaum, miguel.luis, salil.mehta, zhukeqian1,
wangxiongfeng2, wangyanan55, zhao1.liu, linuxarm, gustavo.romero
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 <imammedo@redhat.com>
Message-ID: <20241106100047.18901c9d@imammedo.users.ipa.redhat.com>
Signed-off-by: Salil Mehta <salil.mehta@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));
}
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] tests/qtest/bios-tables-test: Fix DSDT golden masters for x86/{pc, q35}
2024-11-06 13:03 [PATCH 0/3] Fixes CPUs AML & acpi-bios-tables to be x86 backward compatible Salil Mehta via
2024-11-06 13:03 ` [PATCH 1/3] qtest: allow ACPI DSDT Table changes Salil Mehta via
2024-11-06 13:03 ` [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability Salil Mehta via
@ 2024-11-06 13:03 ` Salil Mehta via
2 siblings, 0 replies; 11+ messages in thread
From: Salil Mehta via @ 2024-11-06 13:03 UTC (permalink / raw)
To: qemu-devel, qemu-arm, mst, imammedo
Cc: salil.mehta, jonathan.cameron, peter.maydell, richard.henderson,
anisinha, eduardo, marcel.apfelbaum, david, philmd, peterx,
pbonzini, gshan, borntraeger, alex.bennee, linux, darren, ilkka,
vishnu, karl.heubaum, miguel.luis, salil.mehta, zhukeqian1,
wangxiongfeng2, wangyanan55, zhao1.liu, linuxarm, gustavo.romero
Update DSDT golden master files for x86/pc and x86/q35 platforms to fix
the earlier changes made in the architecture-agnostic CPU AML. These
updates notify the guest OS of vCPU hot-plug and hot-unplug status using
the ACPI `_STA.Enabled` bit. Earlier order of checking `presence` and
`enabled` broke the x86 migration to platforms using older version of
the Qemu.
The following is a diff of the changes in the .dsl file generated with IASL:
* Disassembling to symbolic ASL+ operators
*
- * Disassembly of tests/data/acpi/x86/pc/DSDT, Wed Nov 6 11:41:11 2024
+ * Disassembly of /tmp/aml-6S3FW2, Wed Nov 6 11:41:11 2024
*
* Original Table Header:
* Signature "DSDT"
* Length 0x00002171 (8561)
* Revision 0x01 **** 32-bit table (V1), no 64-bit math support
- * Checksum 0xAA
+ * Checksum 0xB0
* OEM ID "BOCHS "
* OEM Table ID "BXPC "
* OEM Revision 0x00000001 (1)
@@ -1515,16 +1515,13 @@
Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
\_SB.PCI0.PRES.CSEL = Arg0
Local0 = Zero
- If ((\_SB.PCI0.PRES.CPRS == One))
- {
- If ((\_SB.PCI0.PRES.CPEN == One))
- {
- Local0 = 0x0F
- }
- Else
- {
- Local0 = 0x0D
- }
+ If ((\_SB.PCI0.PRES.CPEN == One))
+ {
+ Local0 = 0x0F
+ }
+ ElseIf ((\_SB.PCI0.PRES.CPRS == One))
+ {
+ Local0 = 0x0D
}
Release (\_SB.PCI0.PRES.CPLK)
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Message-ID: <20241106100047.18901c9d@imammedo.users.ipa.redhat.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
tests/data/acpi/x86/pc/DSDT | Bin 8561 -> 8561 bytes
tests/data/acpi/x86/pc/DSDT.acpierst | Bin 8472 -> 8472 bytes
tests/data/acpi/x86/pc/DSDT.acpihmat | Bin 9886 -> 9886 bytes
tests/data/acpi/x86/pc/DSDT.bridge | Bin 15432 -> 15432 bytes
tests/data/acpi/x86/pc/DSDT.cphp | Bin 9025 -> 9025 bytes
tests/data/acpi/x86/pc/DSDT.dimmpxm | Bin 10215 -> 10215 bytes
tests/data/acpi/x86/pc/DSDT.hpbridge | Bin 8512 -> 8512 bytes
tests/data/acpi/x86/pc/DSDT.hpbrroot | Bin 5068 -> 5068 bytes
tests/data/acpi/x86/pc/DSDT.ipmikcs | Bin 8633 -> 8633 bytes
tests/data/acpi/x86/pc/DSDT.memhp | Bin 9920 -> 9920 bytes
tests/data/acpi/x86/pc/DSDT.nohpet | Bin 8419 -> 8419 bytes
tests/data/acpi/x86/pc/DSDT.numamem | Bin 8567 -> 8567 bytes
tests/data/acpi/x86/pc/DSDT.roothp | Bin 12354 -> 12354 bytes
tests/data/acpi/x86/q35/DSDT | Bin 8389 -> 8389 bytes
tests/data/acpi/x86/q35/DSDT.acpierst | Bin 8406 -> 8406 bytes
tests/data/acpi/x86/q35/DSDT.acpihmat | Bin 9714 -> 9714 bytes
.../acpi/x86/q35/DSDT.acpihmat-noinitiator | Bin 8668 -> 8668 bytes
tests/data/acpi/x86/q35/DSDT.applesmc | Bin 8435 -> 8435 bytes
tests/data/acpi/x86/q35/DSDT.bridge | Bin 12002 -> 12002 bytes
tests/data/acpi/x86/q35/DSDT.core-count | Bin 12947 -> 12947 bytes
tests/data/acpi/x86/q35/DSDT.core-count2 | Bin 33804 -> 33804 bytes
tests/data/acpi/x86/q35/DSDT.cphp | Bin 8853 -> 8853 bytes
tests/data/acpi/x86/q35/DSDT.cxl | Bin 13182 -> 13182 bytes
tests/data/acpi/x86/q35/DSDT.dimmpxm | Bin 10043 -> 10043 bytes
tests/data/acpi/x86/q35/DSDT.ipmibt | Bin 8464 -> 8464 bytes
tests/data/acpi/x86/q35/DSDT.ipmismbus | Bin 8477 -> 8477 bytes
tests/data/acpi/x86/q35/DSDT.ivrs | Bin 8406 -> 8406 bytes
tests/data/acpi/x86/q35/DSDT.memhp | Bin 9748 -> 9748 bytes
tests/data/acpi/x86/q35/DSDT.mmio64 | Bin 9519 -> 9519 bytes
tests/data/acpi/x86/q35/DSDT.multi-bridge | Bin 13242 -> 13242 bytes
tests/data/acpi/x86/q35/DSDT.noacpihp | Bin 8269 -> 8269 bytes
tests/data/acpi/x86/q35/DSDT.nohpet | Bin 8247 -> 8247 bytes
tests/data/acpi/x86/q35/DSDT.numamem | Bin 8395 -> 8395 bytes
tests/data/acpi/x86/q35/DSDT.pvpanic-isa | Bin 8490 -> 8490 bytes
tests/data/acpi/x86/q35/DSDT.thread-count | Bin 12947 -> 12947 bytes
tests/data/acpi/x86/q35/DSDT.thread-count2 | Bin 33804 -> 33804 bytes
tests/data/acpi/x86/q35/DSDT.tis.tpm12 | Bin 8995 -> 8995 bytes
tests/data/acpi/x86/q35/DSDT.tis.tpm2 | Bin 9021 -> 9021 bytes
tests/data/acpi/x86/q35/DSDT.type4-count | Bin 18623 -> 18623 bytes
tests/data/acpi/x86/q35/DSDT.viot | Bin 14649 -> 14649 bytes
tests/data/acpi/x86/q35/DSDT.xapic | Bin 35752 -> 35752 bytes
tests/qtest/bios-tables-test-allowed-diff.h | 41 ------------------
42 files changed, 41 deletions(-)
diff --git a/tests/data/acpi/x86/pc/DSDT b/tests/data/acpi/x86/pc/DSDT
index 46c8ffda011e71ce0a0ee707d320f13e9afd74f8..bbcbca90276747c2c1c733fecef13ca5e418c2e0 100644
GIT binary patch
delta 37
vcmV+=0NVfYLh(WhL{mgmaUlQz0kE+O;0==j4jTa+k`5FDQd0r59S&3z-RKGj
delta 53
zcmV-50LuUILh(WhL{mgmaUlQz0jjYI;0+cxlUy$ZUsFO~P(w*DP*O!xLr_vv0h8$s
L903Kh9S&3z%f=73
diff --git a/tests/data/acpi/x86/pc/DSDT.acpierst b/tests/data/acpi/x86/pc/DSDT.acpierst
index 8332df33f33ddaa4241752dcc2cf83ebd1cccd80..53f9668d5cd37affb0ff623a29c7069f09771daf 100644
GIT binary patch
delta 37
vcmV+=0NVeULYP7dL{mgm7$E=v0r;^B;0==j4jTa+k`5FDQd0r59S-{v!s!X>
delta 53
zcmV-50LuTELYP7dL{mgm7$E=v0rIg5;0+cxlUy$ZUsFO~P(w*DP*O!xLr_vv0h8$s
L903Kh9S-{vq?Zqw
diff --git a/tests/data/acpi/x86/pc/DSDT.acpihmat b/tests/data/acpi/x86/pc/DSDT.acpihmat
index bf093f6d8290a10a0823299238d5196d19b8e77d..f1433f480e84de07c28ebd29c503867137322ab2 100644
GIT binary patch
delta 53
zcmV-50LuTKO`c5(L{mgmo+bbQ0d27g>kX3(4jUL8pc<20F9csxLSIlrNia}SMN>mi
LQd0r5C=PZQrD_ir
delta 61
zcmV-D0K)&CO`c5(L{mgmo+bbQ0cWua>kSq+lUy$ZUsFO~P(w*DP*O!xLr_vv0h199
T6bVI60dNWrV4(%GC=PZQBT*2U
diff --git a/tests/data/acpi/x86/pc/DSDT.bridge b/tests/data/acpi/x86/pc/DSDT.bridge
index c0bf671621348f698d2535cfd734dc05a9c7256a..df93da014bce8893c6be790dc2204a33e61d6865 100644
GIT binary patch
delta 37
vcmV+=0NVe^c*uAPL{mgmNIU=l0lu*c;0==j4jTa+k`5FDQd0r59S$ol;EW0r
delta 53
zcmV-50LuT!c*uAPL{mgmNIU=l0l2XW;0+cxlUy$ZUsFO~P(w*DP*O!xLr_vv0h8$s
L903Kh9S$ol&<YR2
diff --git a/tests/data/acpi/x86/pc/DSDT.cphp b/tests/data/acpi/x86/pc/DSDT.cphp
index e72a2df567d28b08b17051dd5612fb77abaf6e20..45c17abeb58faee635ab536147ee8a2838b77d1d 100644
GIT binary patch
delta 53
zcmV-50LuTtM!`l3L{mgmK_dVF0Ti(c91fE`4jUL8pc<20F9csxLSIlrNia}SMN>mi
LQd0r5Sq|tFeaH@%
delta 76
zcmV-S0JHzWM!`l3L{mgmK_dVF0S>VW91b%#lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&94(JpdOBFu=
diff --git a/tests/data/acpi/x86/pc/DSDT.dimmpxm b/tests/data/acpi/x86/pc/DSDT.dimmpxm
index 17a53d9ef714c973c934fdbd2021b2c03fa08a87..41ef5b14b55d9cecd77d7908c24c43508e36eeab 100644
GIT binary patch
delta 84
zcmV-a0IUD!Pv=hxL{mgm=O+LF0nxDv7Y;WXlUy$ZUsFO~P(w*DP*O!xLr_Id0dNWr
qV4)nK8k1Zv1Yc7^Ur<9yFi=uOQ$tWvQvq-a4PcWX5EZj+4tg0v9vE!^
delta 44
zcmV+{0Mq~HPv=hxL{mgm=O+LF0n4!p7Y+e8lRpj>lOPTp1qE;l4PcWX5EZj+4tg0b
CSqt<4
diff --git a/tests/data/acpi/x86/pc/DSDT.hpbridge b/tests/data/acpi/x86/pc/DSDT.hpbridge
index 26f5a1875162459daaa285fef8b6c63678f7726a..0d746c1850e7c3775023cd9e47d4969f19c04cf7 100644
GIT binary patch
delta 37
vcmV+=0NVe+Lcl@_L{mgmKp_AC0pzg?;0==j4jTa+k`5FDQd0r59S$QC&khN9
delta 53
zcmV-50LuTsLcl@_L{mgmKp_AC0p76+;0+cxlUy$ZUsFO~P(w*DP*O!xLr_vv0h8$s
L903Kh9S$QCw)_t!
diff --git a/tests/data/acpi/x86/pc/DSDT.hpbrroot b/tests/data/acpi/x86/pc/DSDT.hpbrroot
index e0c7c2a56d5d8afb5f78134c9b175d6153dca776..09c0d387d667f865e26f6fcb5314c95a4e6b1395 100644
GIT binary patch
delta 76
zcmV-S0JHzhC(I`bL{mgm%o6|r0o1VylMFK&lUy$ZUsFO~P(w*DP*O!xLr_Id0dNWr
iV4)nK8k1Zv1Yc7^Ur<9yFi=uOQ$tWvQvtKk4EzRkSr<Y8
delta 53
zcmV-50LuT&C(I`bL{mgm%o6|r0nV`slMEI&lUy$ZUsFO~P(w*DP*O!xLr_vv0h62z
L903Kh&<y+ry2%h%
diff --git a/tests/data/acpi/x86/pc/DSDT.ipmikcs b/tests/data/acpi/x86/pc/DSDT.ipmikcs
index 119bd256adfb443922cf988e4f4ac3278a1d55ba..5a5f763efbf4ab32bf64008bc3aed73fd4d0b38e 100644
GIT binary patch
delta 37
vcmV+=0NVe#L%Bl=L{mgmxgh`m0W7f!;0==j4jTa+k`5FDQd0r59S)om%K{0d
delta 53
zcmV-50LuTlL%Bl=L{mgmxgh`m0Vc5u;0+cxlUy$ZUsFO~P(w*DP*O!xLr_vv0h8$s
L903Kh9S)omu0Rh|
diff --git a/tests/data/acpi/x86/pc/DSDT.memhp b/tests/data/acpi/x86/pc/DSDT.memhp
index 9ca8a6e2f7898ccd304de44a34bd318a209917e9..d7e156617ecc8d011192a22398ab47d2ac27163b 100644
GIT binary patch
delta 37
vcmV+=0NVe+O~6eGL{mgmz$O3y0h6%`;0==j4jTa+k`5FDQd0r59S)-y+Eofh
delta 53
zcmV-50LuTsO~6eGL{mgmz$O3y0gbT=;0+cxlUy$ZUsFO~P(w*DP*O!xLr_vv0h8$s
L903Kh9S)-y#^Vq5
diff --git a/tests/data/acpi/x86/pc/DSDT.nohpet b/tests/data/acpi/x86/pc/DSDT.nohpet
index 6127a2c9ac43e961e7e8ca01673117382c20bedb..29e55cb624c1d19097bf8732d88dca896a458770 100644
GIT binary patch
delta 76
zcmV-S0JHz&LE}LRL{mgm;~)S40eZ0tQVla2lUy$ZUsFO~P(w*DP*O!xLr_Id0dNWr
iV4)nK8k1Zv1Yc7^Ur<9yFi=uOQ$tWvQvtJ&4OA0zX%<re
delta 53
zcmV-50LuU4LE}LRL{mgm;~)S40d%nnQVkY2lUy$ZUsFO~P(w*DP*O!xLr_vv0h3z|
L903Khj}254xg-xL
diff --git a/tests/data/acpi/x86/pc/DSDT.numamem b/tests/data/acpi/x86/pc/DSDT.numamem
index 18c815efa890503f5e0ca94665239f58fe283735..73e8ec60fdff3be9ea5c2fe246a14a7cb6cf1543 100644
GIT binary patch
delta 37
vcmV+=0NVfeLia)nL{mgmcOd`(0ZXw8;0==j4jTa+k`5FDQd0r59S&L(*EtEg
delta 53
zcmV-50LuUOLia)nL{mgmcOd`(0Y$M2;0+cxlUy$ZUsFO~P(w*DP*O!xLr_vv0h8$s
L903Kh9S&L(zvmBW
diff --git a/tests/data/acpi/x86/pc/DSDT.roothp b/tests/data/acpi/x86/pc/DSDT.roothp
index 8256503220c11cbb902c6967fccba87e711bcd5a..6fcf58475ed9c9ec2bfdc54c162a500116dd5ab2 100644
GIT binary patch
delta 37
vcmV+=0NVe;V8UPuL{mgmLNEXT0X(q^;0==j4jTa+k`5FDQd0r59S$WT%9;r=
delta 53
zcmV-50LuTuV8UPuL{mgmLNEXT0XDG;;0+cxlUy$ZUsFO~P(w*DP*O!xLr_vv0h8$s
L903Kh9S$WTtr!mA
diff --git a/tests/data/acpi/x86/q35/DSDT b/tests/data/acpi/x86/q35/DSDT
index b0bbff7686c9a56129bfa3408e62f142cc482713..3ed6309318703c1201e08afb15ef4e42edea64ed 100644
GIT binary patch
delta 53
zcmV-50LuTxLB&A|L{mgm#UKCx0gkZ>EgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8qPxttbyn
delta 76
zcmV-S0JHzaLB&A|L{mgm#UKCx0f?~*EgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8e0Zn^cB|t
diff --git a/tests/data/acpi/x86/q35/DSDT.acpierst b/tests/data/acpi/x86/q35/DSDT.acpierst
index f91cbe55fcfeea319babf7c9a0c6a6ccdc3320d1..acc29ac6ee5f87f620357356b0174f0df05abb7f 100644
GIT binary patch
delta 53
zcmV-50LuT?LDoSEL{mgm)*t`?0WPr$EgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8q??sD2MH
delta 76
zcmV-S0JHzrLDoSEL{mgm)*t`?0VuHwEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8f*qwKNY(G
diff --git a/tests/data/acpi/x86/q35/DSDT.acpihmat b/tests/data/acpi/x86/q35/DSDT.acpihmat
index 0949fb9d67c70dc882e50501ece421114ad8080b..3711ade7ab3cbb838e0695a8a354cc3511195d84 100644
GIT binary patch
delta 61
zcmV-D0K)(BOY%z!L{mgm@+ANO0ZXw8I2w~#8XFuOpc<20F9csxLSIlrNia}SMN>mi
TQd0qN3JqYBK^zscjv8bON;47R
delta 44
zcmV+{0Mq~SOY%z!L{mgm@+ANO0Y$M2I2r*rlVBPZlR+9B1qE;l4PcW&92K*U8e|JE
Cu?#{0
diff --git a/tests/data/acpi/x86/q35/DSDT.acpihmat-noinitiator b/tests/data/acpi/x86/q35/DSDT.acpihmat-noinitiator
index 0fa4daa35cf95f93ba8c15f478460fe4e14e6d9e..7ef3af06c3db708a41054d19962d688019c3816d 100644
GIT binary patch
delta 38
wcmV+>0NMZCL)=3OL{mgm+#vt}0R^!NP#TkQ8XEx|lW-ao15#4~vyB>02jQy<fB*mh
delta 53
zcmV-50LuT|L)=3OL{mgm+#vt}0spZIP#P9DlUy$ZUsFO~P(w*DP*O!xLr_vv0h3u8
L903KhjT%n}%_R@B
diff --git a/tests/data/acpi/x86/q35/DSDT.applesmc b/tests/data/acpi/x86/q35/DSDT.applesmc
index a5d032b7d96113c9393036b2ba831adb6d584142..a6375cdf7caef4e1a2e4a043e4c04e17f58e70ae 100644
GIT binary patch
delta 53
zcmV-50LuUKLGwWhL{mgm^B@2K0e-OxEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8r_K#jFqX
delta 76
zcmV-S0JHz|LGwWhL{mgm^B@2K0eG<rEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8i@vkZWez4
diff --git a/tests/data/acpi/x86/q35/DSDT.bridge b/tests/data/acpi/x86/q35/DSDT.bridge
index 3464f552974672bde25eb15f1c93c309c57ef5cb..54d397735532d600fb49308b2f1a5a1b5fc04dbd 100644
GIT binary patch
delta 53
zcmV-50LuU3UE*B|L{mgm;w}IH0jjYIEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8rSH&-xHB
delta 76
zcmV-S0JHz%UE*B|L{mgm;w}IH0i>}CEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8h8|xWfr>t
diff --git a/tests/data/acpi/x86/q35/DSDT.core-count b/tests/data/acpi/x86/q35/DSDT.core-count
index 08f5d5f54bcb61235b98fc85bb814046dd038c13..32d891d59362351e6af41b2051982f5e16c553f8 100644
GIT binary patch
delta 53
zcmV-50LuT9Ws_wJL{mgmlQIAR0r#;As~nTe92*!Mpc<20F9csxLSIlrNia}SMN>mi
LQd0r5=p3sR*2WQ7
delta 61
zcmV-D0K)&1Ws_wJL{mgmlQIAR0r9a4s~i?LlUy$ZUsFO~P(w*DP*O!xLr_vv0h7@j
T6bVI60dNWrV4(%G=p3sROgj?W
diff --git a/tests/data/acpi/x86/q35/DSDT.core-count2 b/tests/data/acpi/x86/q35/DSDT.core-count2
index d29a7108f82110ce9f9b4e006501215d41c5420a..5f408f64297e9aeb3c8b0b8dce90dfb6292d323f 100644
GIT binary patch
delta 86
zcmV-c0IC0shysj=0t!S^L{tof0003<u?klvHyV>%F9csxLSIlrNia}SMN>miMNR>5
s3J+kR9H1JLTrUJ)Q$k-*LrE}DQbkikP*PI?a0(4zlVK<ov#lmyT>xPiSO5S3
delta 46
zcmV+}0MY-9hysj=0t!S^L{tof0003(u?klv0XLI>CKZ!lCL9F?a0(4zlVK<ov#lmy
ET_1!Dj{pDw
diff --git a/tests/data/acpi/x86/q35/DSDT.cphp b/tests/data/acpi/x86/q35/DSDT.cphp
index 7fd59bf6702c04a622f05ae356a2ea37312ab403..efa970f93d0ca69b4e77b6628157e5fdd8e4d759 100644
GIT binary patch
delta 61
zcmV-D0K)&3MU_PgL{mgml_CHD0rIg5X&RG?8XFuOpc<20F9csxLSIlrNia}SMN>mi
TQd0qN3JqYB5FHh>zZ%vDH)|3z
delta 68
zcmV-K0K5N{MU_PgL{mgml_CHD0qn5~X&M1HlaU$~A)p$QTrUJ)Q$k-*LrE}DQbkik
aP(@Awa0(A#p#^XX4PcWH9Tl^`8rBD`@)OJe
diff --git a/tests/data/acpi/x86/q35/DSDT.cxl b/tests/data/acpi/x86/q35/DSDT.cxl
index 92769c630dc362c781c4e9a83d6f8be306121c5a..ee6939a341be35d4be2d5f865efd230ce31fd7bc 100644
GIT binary patch
delta 53
zcmV-50LuUVX8vXhL{mgmelq|70iv-AEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8n(7%$yIj
delta 76
zcmV-S0JH!8X8vXhL{mgmelq|70i3Z4EgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8Wb6hY!*KN
diff --git a/tests/data/acpi/x86/q35/DSDT.dimmpxm b/tests/data/acpi/x86/q35/DSDT.dimmpxm
index 1db0bf454a203006f866e6752d06422ae675cbd3..4ec8564ff941acb9af07ffc35e7b21e5adf74c72 100644
GIT binary patch
delta 59
zcmV-B0L1^hPP<MDL{mgmJ0}1D0j{wMWEvhClUy$ZUsFO~P(w*DP*O!xLr_Id0dNWr
RV4)nK8k27t6|<olnhXIG5mf*H
delta 53
zcmV-50LuTnPP<MDL{mgmJ0}1D0jRMGWEvJXlUy$ZUsFO~P(w*DP*O!xLr_vv0h4VS
L903Khp&FVDptlbp
diff --git a/tests/data/acpi/x86/q35/DSDT.ipmibt b/tests/data/acpi/x86/q35/DSDT.ipmibt
index 25f43ae8efb55364a739e6b5e3cb4e71e61862b0..b304c28b64769a106d6c93d5ac195bc28a6949ab 100644
GIT binary patch
delta 53
zcmV-50LuT6LXbiVL{mgm5Fr2n0l~2fEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8s{nfp8A4
delta 76
zcmV-S0JHy)LXbiVL{mgm5Fr2n0lTpZEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8m0yyO%*u+
diff --git a/tests/data/acpi/x86/q35/DSDT.ipmismbus b/tests/data/acpi/x86/q35/DSDT.ipmismbus
index 32bcd25bda9e9d2775790385f8da6a11e9d5cb46..8dc12ebb2aa45e2e147a3016ce8b0edc4c30b90b 100644
GIT binary patch
delta 53
zcmV-50LuTJLY+biL{mgm9U%Y!0Wz@)EgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8tZ!b43nQ
delta 76
zcmV-S0JHy{LY+biL{mgm9U%Y!0W7f!EgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8nXrn$`sxJ
diff --git a/tests/data/acpi/x86/q35/DSDT.ivrs b/tests/data/acpi/x86/q35/DSDT.ivrs
index f91cbe55fcfeea319babf7c9a0c6a6ccdc3320d1..acc29ac6ee5f87f620357356b0174f0df05abb7f 100644
GIT binary patch
delta 53
zcmV-50LuT?LDoSEL{mgm)*t`?0WPr$EgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8q??sD2MH
delta 76
zcmV-S0JHzrLDoSEL{mgm)*t`?0VuHwEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8f*qwKNY(G
diff --git a/tests/data/acpi/x86/q35/DSDT.memhp b/tests/data/acpi/x86/q35/DSDT.memhp
index be90eb71d8dda8fe54c79ffffe103986ee06ae3a..2b1ac856b94d4e33c4ced07964bf796c5d915bc0 100644
GIT binary patch
delta 53
zcmV-50LuTAOq5ItL{mgm6ea)w0dTPjEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8t8wd`S+A
delta 76
zcmV-S0JHy;Oq5ItL{mgm6ea)w0cx=dEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8mbEyG8Gm8
diff --git a/tests/data/acpi/x86/q35/DSDT.mmio64 b/tests/data/acpi/x86/q35/DSDT.mmio64
index 01f276a6aff38a1d4f58640a9e6d120fc9a04b61..412f9cb1da9bca5756a347b8eee33ac7641bc9c0 100644
GIT binary patch
delta 53
zcmV-50LuTbO0P-^L{mgmFC_o~0Wq-(EgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8u4~fr<{Y
delta 76
zcmV-S0JHzEO0P-^L{mgmFC_o~0V}ZzEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8pR47o)tX+
diff --git a/tests/data/acpi/x86/q35/DSDT.multi-bridge b/tests/data/acpi/x86/q35/DSDT.multi-bridge
index 1bd2ee8d2ebd3c9e0ed89a86478691f2e06f2590..4d77514e40ed8c7a0704ffeac98aaf0ea42a97f9 100644
GIT binary patch
delta 53
zcmV-50LuTmXS!z!L{mgmx-$R(0i&@BEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8p@(yipIQ
delta 76
zcmV-S0JHzPXS!z!L{mgmx-$R(0iCf5EgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8c-Q`k`^)m
diff --git a/tests/data/acpi/x86/q35/DSDT.noacpihp b/tests/data/acpi/x86/q35/DSDT.noacpihp
index 45cc2bcffa42d73db110afd5075556dcfe5d9936..0cced3ab840a3f937d2a8715a153547e2aebb4c1 100644
GIT binary patch
delta 37
vcmV+=0NVe}K+Ql3L{mgmO&|aO0U@yp;24tu85;o{k{J{OQd0r59T_kO#2*O*
delta 53
zcmV-50LuT(K+Ql3L{mgmO&|aO0UNOj;20J+lUy$ZUsFO~P(w*DP*O!xLr_vv0h8$%
L903Kh9T_kOq2~^#
diff --git a/tests/data/acpi/x86/q35/DSDT.nohpet b/tests/data/acpi/x86/q35/DSDT.nohpet
index f110504b9c813aa07802fc17d2869596a2eeca6f..1c39b95fd330143abda31d60f1b47c3ee80d2b4d 100644
GIT binary patch
delta 53
zcmV-50LuTjK({~&L{mgmHy{820avjKpBa<D85<ZJpc<20F9csxLSIlrNia}SMN>mi
LQd0r5+!<R2riBl@
delta 53
zcmV-50LuTjK({~&L{mgmHy{820a39EpBWZ6lUy$ZUsFO~P(w*DP*O!xLr_vv0h6d1
L903Kh+!<R2l{gOs
diff --git a/tests/data/acpi/x86/q35/DSDT.numamem b/tests/data/acpi/x86/q35/DSDT.numamem
index 6090958f39875f5806e72e23f32cb4b3ae840627..944d7938feb5d87a6056f14cc83f98dddd782304 100644
GIT binary patch
delta 53
zcmV-50LuT%LCZl3L{mgm%OC&%0V%NxEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8qh%p-B$^
delta 76
zcmV-S0JHzgLCZl3L{mgm%OC&%0VA;rEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8es-apcRS$
diff --git a/tests/data/acpi/x86/q35/DSDT.pvpanic-isa b/tests/data/acpi/x86/q35/DSDT.pvpanic-isa
index 7a8e568315a43f1fa98068d8e78995c98064fb91..1d1e06322cb4cf5c6ff9e6f86a29c691abe6d5d1 100644
GIT binary patch
delta 53
zcmV-50LuTWLaIUvL{mgmDj@&>0YI?|EgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8t=>eFYAc
delta 76
zcmV-S0JHz9LaIUvL{mgmDj@&>0Xne?EgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8o&k@O%)&j
diff --git a/tests/data/acpi/x86/q35/DSDT.thread-count b/tests/data/acpi/x86/q35/DSDT.thread-count
index 08f5d5f54bcb61235b98fc85bb814046dd038c13..32d891d59362351e6af41b2051982f5e16c553f8 100644
GIT binary patch
delta 53
zcmV-50LuT9Ws_wJL{mgmlQIAR0r#;As~nTe92*!Mpc<20F9csxLSIlrNia}SMN>mi
LQd0r5=p3sR*2WQ7
delta 61
zcmV-D0K)&1Ws_wJL{mgmlQIAR0r9a4s~i?LlUy$ZUsFO~P(w*DP*O!xLr_vv0h7@j
T6bVI60dNWrV4(%G=p3sROgj?W
diff --git a/tests/data/acpi/x86/q35/DSDT.thread-count2 b/tests/data/acpi/x86/q35/DSDT.thread-count2
index d29a7108f82110ce9f9b4e006501215d41c5420a..5f408f64297e9aeb3c8b0b8dce90dfb6292d323f 100644
GIT binary patch
delta 86
zcmV-c0IC0shysj=0t!S^L{tof0003<u?klvHyV>%F9csxLSIlrNia}SMN>miMNR>5
s3J+kR9H1JLTrUJ)Q$k-*LrE}DQbkikP*PI?a0(4zlVK<ov#lmyT>xPiSO5S3
delta 46
zcmV+}0MY-9hysj=0t!S^L{tof0003(u?klv0XLI>CKZ!lCL9F?a0(4zlVK<ov#lmy
ET_1!Dj{pDw
diff --git a/tests/data/acpi/x86/q35/DSDT.tis.tpm12 b/tests/data/acpi/x86/q35/DSDT.tis.tpm12
index 29a416f0508655d2bfde01fff4d25ad7f89581d9..a06de42fd12f395aaebbb801368efffbe1be2371 100644
GIT binary patch
delta 53
zcmV-50LuTPMx#awL{mgmBO?F+0lu*cEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8tr+jtvh3
delta 76
zcmV-S0JHz2Mx#awL{mgmBO?F+0l2XWEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8o3BEsuhj^
diff --git a/tests/data/acpi/x86/q35/DSDT.tis.tpm2 b/tests/data/acpi/x86/q35/DSDT.tis.tpm2
index 59288f02c43cf2efc1555599131fde05dbbaa1cd..f39e769d932eb89f9ed620ff57a37707155ddbe3 100644
GIT binary patch
delta 53
zcmV-50LuTpM!iM~L{mgmJtF`B0eG<rEgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8ulBlAjMH
delta 76
zcmV-S0JHzSM!iM~L{mgmJtF`B0dlblEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8q)|j^cAoG
diff --git a/tests/data/acpi/x86/q35/DSDT.type4-count b/tests/data/acpi/x86/q35/DSDT.type4-count
index eaca76e8e61eb62f75dbdf093e803eea34330deb..8cea3b46ac597ab5456476d0c61c88e2e4ae2757 100644
GIT binary patch
delta 77
zcmV-T0J8tTkpaJv0SZJ@L{z^>0003au?oW<Ga8dzF9csxLSIlrNia}SMN>miMNR>5
j3J+kR9H1JLTrUJ)Q$k-*LrE}DQbkikP*PI?vLL@GgcBCv
delta 54
zcmV-60LlNqkpaJv0SZJ@L{z^>0003Uu?oW<7B`bzF9csxLSIlrNia}SMN>miQd0qw
M%pV*91+pN&D9IcUH~;_u
diff --git a/tests/data/acpi/x86/q35/DSDT.viot b/tests/data/acpi/x86/q35/DSDT.viot
index 71379a48ddebf7e1f5e853fcbbb6befd561cb8e4..0a16eb366cc299efcbc88745028c63951e7b0e57 100644
GIT binary patch
delta 53
zcmV-50LuTla=CH}L{mgmIXM6T0Wh%&EgF+f8XFiKpc<20F9csxLSIlrNia}SMN>mi
LQd0r5Y8uZTnx79*
delta 76
zcmV-S0JHzOa=CH}L{mgmIXM6T0V=TyEgCa7lUy$ZUsFO~P(w*DP*O!xLr_vv0iYU_
iTrUJ)Q$k-*LrE}DQbkikP(@Awa0(A#p#`&Q8qXd=N)_Ay
diff --git a/tests/data/acpi/x86/q35/DSDT.xapic b/tests/data/acpi/x86/q35/DSDT.xapic
index 9059812b5892ba7ac5c9bd312fd9f45a4f59f105..b8a4f47013cd3424d9a05643ece52b0abb3dc326 100644
GIT binary patch
delta 77
zcmV-T0J8t6mjbAl0t!S^L{zAY0003su?oW{Ga8dzF9csxLSIlrNia}SMN>miMNR>5
j3J+kR9H1JLTrUJ)Q$k-*LrE}DQbkikP*PI?vM8uyi{BSA
delta 54
zcmV-60LlNTmjbAl0t!S^L{zAY0003mu?oW{7B`bzF9csxLSIlrNia}SMN>miQd0qw
M%qJWH1+plpW6i!0k^lez
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 512d40665d..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,42 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/x86/pc/DSDT",
-"tests/data/acpi/x86/pc/DSDT.acpierst",
-"tests/data/acpi/x86/pc/DSDT.acpihmat",
-"tests/data/acpi/x86/pc/DSDT.bridge",
-"tests/data/acpi/x86/pc/DSDT.cphp",
-"tests/data/acpi/x86/pc/DSDT.dimmpxm",
-"tests/data/acpi/x86/pc/DSDT.hpbridge",
-"tests/data/acpi/x86/pc/DSDT.hpbrroot",
-"tests/data/acpi/x86/pc/DSDT.ipmikcs",
-"tests/data/acpi/x86/pc/DSDT.memhp",
-"tests/data/acpi/x86/pc/DSDT.nohpet",
-"tests/data/acpi/x86/pc/DSDT.numamem",
-"tests/data/acpi/x86/pc/DSDT.roothp",
-"tests/data/acpi/x86/q35/DSDT",
-"tests/data/acpi/x86/q35/DSDT.acpierst",
-"tests/data/acpi/x86/q35/DSDT.acpihmat",
-"tests/data/acpi/x86/q35/DSDT.acpihmat-noinitiator",
-"tests/data/acpi/x86/q35/DSDT.applesmc",
-"tests/data/acpi/x86/q35/DSDT.bridge",
-"tests/data/acpi/x86/q35/DSDT.core-count",
-"tests/data/acpi/x86/q35/DSDT.core-count2",
-"tests/data/acpi/x86/q35/DSDT.cphp",
-"tests/data/acpi/x86/q35/DSDT.cxl",
-"tests/data/acpi/x86/q35/DSDT.dimmpxm",
-"tests/data/acpi/x86/q35/DSDT.ipmibt",
-"tests/data/acpi/x86/q35/DSDT.ipmismbus",
-"tests/data/acpi/x86/q35/DSDT.ivrs",
-"tests/data/acpi/x86/q35/DSDT.memhp",
-"tests/data/acpi/x86/q35/DSDT.mmio64",
-"tests/data/acpi/x86/q35/DSDT.multi-bridge",
-"tests/data/acpi/x86/q35/DSDT.noacpihp",
-"tests/data/acpi/x86/q35/DSDT.nohpet",
-"tests/data/acpi/x86/q35/DSDT.numamem",
-"tests/data/acpi/x86/q35/DSDT.pvpanic-isa",
-"tests/data/acpi/x86/q35/DSDT.thread-count",
-"tests/data/acpi/x86/q35/DSDT.thread-count2",
-"tests/data/acpi/x86/q35/DSDT.tis.tpm12",
-"tests/data/acpi/x86/q35/DSDT.tis.tpm2",
-"tests/data/acpi/x86/q35/DSDT.type4-count",
-"tests/data/acpi/x86/q35/DSDT.viot",
-"tests/data/acpi/x86/q35/DSDT.xapic",
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability
2024-11-06 13:03 ` [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability Salil Mehta via
@ 2024-11-06 13:56 ` Igor Mammedov
2024-11-06 14:45 ` Salil Mehta via
0 siblings, 1 reply; 11+ messages in thread
From: Igor Mammedov @ 2024-11-06 13:56 UTC (permalink / raw)
To: Salil Mehta
Cc: qemu-devel, qemu-arm, mst, jonathan.cameron, peter.maydell,
richard.henderson, anisinha, eduardo, marcel.apfelbaum, david,
philmd, peterx, pbonzini, gshan, borntraeger, alex.bennee, linux,
darren, ilkka, vishnu, karl.heubaum, miguel.luis, salil.mehta,
zhukeqian1, wangxiongfeng2, wangyanan55, zhao1.liu, linuxarm,
gustavo.romero
On Wed, 6 Nov 2024 13:03:30 +0000
Salil Mehta <salil.mehta@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 <imammedo@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.18901c9d@imammedo.users.ipa.redhat.com>
> Signed-off-by: Salil Mehta <salil.mehta@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));
> }
^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability
2024-11-06 13:56 ` Igor Mammedov
@ 2024-11-06 14:45 ` Salil Mehta via
2024-11-06 16:07 ` Igor Mammedov
0 siblings, 1 reply; 11+ messages in thread
From: Salil Mehta via @ 2024-11-06 14:45 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, mst@redhat.com,
Jonathan Cameron, peter.maydell@linaro.org,
richard.henderson@linaro.org, anisinha@redhat.com,
eduardo@habkost.net, marcel.apfelbaum@gmail.com, david@redhat.com,
philmd@linaro.org, peterx@redhat.com, pbonzini@redhat.com,
gshan@redhat.com, borntraeger@linux.ibm.com,
alex.bennee@linaro.org, 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,
wangxiongfeng (C), wangyanan (Y), zhao1.liu@intel.com, Linuxarm,
gustavo.romero@linaro.org
Hi Igor,
> From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
> arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
> Mammedov
> Sent: Wednesday, November 6, 2024 1:57 PM
> To: Salil Mehta <salil.mehta@huawei.com>
>
> On Wed, 6 Nov 2024 13:03:30 +0000
> Salil Mehta <salil.mehta@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 <imammedo@redhat.com>
> 'Reported-by' maybe, but certainly not suggested.
No issues. I can change.
>
> 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).
This is a wrong assumption. Presence bit can change in future. We have taken
into account this aspect by design in the kernel code as well. Both virtual
and physical CPU hot plug can co-exists or entirely as sole features. This is
a requirement.
>
> 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;
This has to be fetched from architecture code. Please see other changes in the V3
patch-set.
> 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
We MUST fetch this from architecture code as this can dynamically change in
future and hence, we need to keep that flexibility
>
> 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).
The CPUs AML in the V2 patch-set would have broken the x86 functionality.
Thanks
Salil.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability
2024-11-06 14:45 ` Salil Mehta via
@ 2024-11-06 16:07 ` Igor Mammedov
2024-11-06 19:05 ` Salil Mehta via
0 siblings, 1 reply; 11+ messages in thread
From: Igor Mammedov @ 2024-11-06 16:07 UTC (permalink / raw)
To: Salil Mehta
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, mst@redhat.com,
Jonathan Cameron, peter.maydell@linaro.org,
richard.henderson@linaro.org, anisinha@redhat.com,
eduardo@habkost.net, marcel.apfelbaum@gmail.com, david@redhat.com,
philmd@linaro.org, peterx@redhat.com, pbonzini@redhat.com,
gshan@redhat.com, borntraeger@linux.ibm.com,
alex.bennee@linaro.org, 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,
wangxiongfeng (C), wangyanan (Y), zhao1.liu@intel.com, Linuxarm,
gustavo.romero@linaro.org
On Wed, 6 Nov 2024 14:45:42 +0000
Salil Mehta <salil.mehta@huawei.com> wrote:
> Hi Igor,
>
> > From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
> > arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
> > Mammedov
> > Sent: Wednesday, November 6, 2024 1:57 PM
> > To: Salil Mehta <salil.mehta@huawei.com>
> >
> > On Wed, 6 Nov 2024 13:03:30 +0000
> > Salil Mehta <salil.mehta@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 <imammedo@redhat.com>
> > 'Reported-by' maybe, but certainly not suggested.
>
>
> No issues. I can change.
>
>
> >
> > 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).
>
>
> This is a wrong assumption. Presence bit can change in future. We have taken
> into account this aspect by design in the kernel code as well. Both virtual
Above does imply that support for really hotpluggable CPUs might be implemented
in the future.
Can you point to relevant kernel code/patches?
> and physical CPU hot plug can co-exists or entirely as sole features. This is
> a requirement.
I don't see any _must_ requirements here whatsoever. If/when ARM reaches point
where standby and hotplug cpus are mixed within VM instance, we might have to
expose presence bit to guest dynamically but it is totally not needed within
observable future and premature.
Your cpu class presence check also works target-wise just with more boiler code
+ not needed presence bit ABI for guest side,
The same as my suggestion only from other side.
But regardless of that as long as machine has doesn't mix always present CPUs
with hotpluggable ones within the same instance, changing AML side
as I've just suggested works.
If ARM ever gets real cpu hotplug as your comment above hints, my suggestion
also works fine. With only difference that board code would turn off
always_present_cpus if hotpluggable CPUs are used instead of standby.
>
> >
> > 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;
>
>
> This has to be fetched from architecture code. Please see other changes in the V3
> patch-set.
I've seen, that and it doesn't have to be fetched dynamically.
In my opinion the series was not ready to be merged
(Michael probably picked it by mistake).
We don't really need this in 9.2 as it is only ARM cpu 'hotplug'
related, and the later is not ready for 9.2.
I'd prefer the series being reverted and we continue improving series,
instead of rushing it and fixing broken thing up.
>
>
> > 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
>
>
> We MUST fetch this from architecture code as this can dynamically change in
> future and hence, we need to keep that flexibility
CPUHotplugFeatures::always_present_cpus can be set dynamically per VM instance
or per Machine type.
> >
> > 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).
>
>
> The CPUs AML in the V2 patch-set would have broken the x86 functionality.
Frankly speaking, I don't see much progress. All that happens on respins
is flipping between what I asked to remove before to some earlier concept.
And all of that for the purpose to workaround/accommodate fake cpu hotplug hacks.
> Thanks
> Salil.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability
2024-11-06 16:07 ` Igor Mammedov
@ 2024-11-06 19:05 ` Salil Mehta via
2024-11-07 16:57 ` Igor Mammedov
0 siblings, 1 reply; 11+ messages in thread
From: Salil Mehta via @ 2024-11-06 19:05 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, mst@redhat.com,
Jonathan Cameron, peter.maydell@linaro.org,
richard.henderson@linaro.org, anisinha@redhat.com,
eduardo@habkost.net, marcel.apfelbaum@gmail.com, david@redhat.com,
philmd@linaro.org, peterx@redhat.com, pbonzini@redhat.com,
gshan@redhat.com, borntraeger@linux.ibm.com,
alex.bennee@linaro.org, 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,
wangxiongfeng (C), wangyanan (Y), zhao1.liu@intel.com, Linuxarm,
gustavo.romero@linaro.org
Hi Igor,
Thanks for replying back and the reviews. Please find my replies
inline.
> From: Igor Mammedov <imammedo@redhat.com>
> Sent: Wednesday, November 6, 2024 4:08 PM
> To: Salil Mehta <salil.mehta@huawei.com>
>
> On Wed, 6 Nov 2024 14:45:42 +0000
> Salil Mehta <salil.mehta@huawei.com> wrote:
>
> > Hi Igor,
> >
> > > From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org
> <qemu-
> > > arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
> > > Mammedov
> > > Sent: Wednesday, November 6, 2024 1:57 PM
> > > To: Salil Mehta <salil.mehta@huawei.com>
> > >
> > > On Wed, 6 Nov 2024 13:03:30 +0000
> > > Salil Mehta <salil.mehta@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 <imammedo@redhat.com>
> 'Reported-by'
> > > maybe, but certainly not suggested.
> >
> >
> > No issues. I can change.
> >
> >
> > >
> > > 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).
> >
> >
> > This is a wrong assumption. Presence bit can change in future. We have
> > taken into account this aspect by design in the kernel code as well.
> > Both virtual
>
> Above does imply that support for really hotpluggable CPUs might be
> implemented in the future.
> Can you point to relevant kernel code/patches?
Let me make it clear so that there is no confusion, there is no support of
physical "CPU" hot-plug on ARM platforms right now and nor will there be
any in future as it does not makes sense to have. The point I made in the
patches is about hot-plug was at different granularity which has not been
denied by ARM.
>
> > and physical CPU hot plug can co-exists or entirely as sole features.
> > This is a requirement.
>
>
> I don't see any _must_ requirements here whatsoever. If/when ARM
> reaches point where standby and hotplug cpus are mixed within VM
> instance, we might have to expose presence bit to guest dynamically but it
> is totally not needed within observable future and premature.
> Your cpu class presence check also works target-wise just with more boiler
> code
> + not needed presence bit ABI for guest side,
> The same as my suggestion only from other side.
>
> But regardless of that as long as machine has doesn't mix always present
> CPUs with hotpluggable ones within the same instance, changing AML side
> as I've just suggested works.
Sure, I'm not disagreeing. It will work by adding the flag you've suggested
but it will work even by not adding any flag and not defining a `persistent`
hook for any architecture.
>
> If ARM ever gets real cpu hotplug as your comment above hints, my
> suggestion also works fine. With only difference that board code would turn
> off always_present_cpus if hotpluggable CPUs are used instead of standby.
Sure, but is it necessary to define a new flag when you can do even without it?
Having few lines of extra code (literally 2-3 only) should not be a major cause of
worry, especially, if it makes design more clear and easy to understand. This is
not a performance code either.
Again, I appreciate your compact logic. I'm not disagreeing with it.
> > > 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;
> >
> >
> > This has to be fetched from architecture code. Please see other
> > changes in the V3 patch-set.
>
> I've seen, that and it doesn't have to be fetched dynamically.
Agreed, it is not necessary to be fetched. Hence, if you do not define the
hook. In later case, it assumes the existing logic, which x86 has been following.
It will work.
> In my opinion the series was not ready to be merged (Michael probably
> picked it by mistake).
>
> We don't really need this in 9.2 as it is only ARM cpu 'hotplug'
> related, and the later is not ready for 9.2.
> I'd prefer the series being reverted and we continue improving series,
> instead of rushing it and fixing broken thing up.
>
>
> >
> >
> > > 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
> >
> >
> > We MUST fetch this from architecture code as this can dynamically
> > change in future and hence, we need to keep that flexibility
>
> CPUHotplugFeatures::always_present_cpus can be set dynamically per VM
> instance or per Machine type.
Yes, but you need a code for that and I'm not sure what is being saved by
introducing an extra flag then?
> > > 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).
> >
> >
> > The CPUs AML in the V2 patch-set would have broken the x86
> functionality.
>
> Frankly speaking, I don't see much progress. All that happens on respins is
> flipping between what I asked to remove before to some earlier concept.
I think then you've missed the code which addressed one of your key concerns
in the V1 patch-set that would have broken x86 migration i.e. presence of the
`is_enabled` state in the CPU Hot-plug VMSD. That has been removed. Maybe
you would like to go through the change log of the V3 patch-set
https://lore.kernel.org/qemu-devel/20241018163118.0ae01a84@imammedo.users.ipa.redhat.com/
Below is the relevant excerpt from your many comments:
[...]
> .fields = (const VMStateField[]) {
> VMSTATE_BOOL(is_inserting, AcpiCpuStatus),
> VMSTATE_BOOL(is_removing, AcpiCpuStatus),
> + VMSTATE_BOOL(is_present, AcpiCpuStatus),
> + VMSTATE_BOOL(is_enabled, AcpiCpuStatus),
that's likely will break x86 migration,
but before bothering peterx, maybe we won't need this hunk if
is_enabled is migrated as part of vCPU state.
[...]
> And all of that for the purpose to workaround/accommodate fake cpu
> hotplug hacks.
I've to respectfully disagree on this. This is your assumption which is far from
reality. The accompanying main series of this will never have vCPUs which are
*not* present. BTW, these changes have been tested by Oracle folks with that
series and are known to work.
https://lore.kernel.org/qemu-devel/C4FEC9E7-69DB-428A-A85F-30170F98814B@oracle.com/
Thanks!
Best regards
Salil.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability
2024-11-06 19:05 ` Salil Mehta via
@ 2024-11-07 16:57 ` Igor Mammedov
2024-11-07 18:59 ` Salil Mehta via
0 siblings, 1 reply; 11+ messages in thread
From: Igor Mammedov @ 2024-11-07 16:57 UTC (permalink / raw)
To: Salil Mehta
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, mst@redhat.com,
Jonathan Cameron, peter.maydell@linaro.org,
richard.henderson@linaro.org, anisinha@redhat.com,
eduardo@habkost.net, marcel.apfelbaum@gmail.com, david@redhat.com,
philmd@linaro.org, peterx@redhat.com, pbonzini@redhat.com,
gshan@redhat.com, borntraeger@linux.ibm.com,
alex.bennee@linaro.org, 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,
wangxiongfeng (C), wangyanan (Y), zhao1.liu@intel.com, Linuxarm,
gustavo.romero@linaro.org
On Wed, 6 Nov 2024 19:05:15 +0000
Salil Mehta <salil.mehta@huawei.com> wrote:
> Hi Igor,
>
> Thanks for replying back and the reviews. Please find my replies
> inline.
>
> > From: Igor Mammedov <imammedo@redhat.com>
> > Sent: Wednesday, November 6, 2024 4:08 PM
> > To: Salil Mehta <salil.mehta@huawei.com>
> >
> > On Wed, 6 Nov 2024 14:45:42 +0000
> > Salil Mehta <salil.mehta@huawei.com> wrote:
> >
> > > Hi Igor,
> > >
> > > > From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org
> > <qemu-
> > > > arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
> > > > Mammedov
> > > > Sent: Wednesday, November 6, 2024 1:57 PM
> > > > To: Salil Mehta <salil.mehta@huawei.com>
> > > >
> > > > On Wed, 6 Nov 2024 13:03:30 +0000
> > > > Salil Mehta <salil.mehta@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 <imammedo@redhat.com>
> > 'Reported-by'
> > > > maybe, but certainly not suggested.
> > >
> > >
> > > No issues. I can change.
> > >
> > >
> > > >
> > > > 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).
> > >
> > >
> > > This is a wrong assumption. Presence bit can change in future. We have
> > > taken into account this aspect by design in the kernel code as well.
> > > Both virtual
> >
> > Above does imply that support for really hotpluggable CPUs might be
> > implemented in the future.
> > Can you point to relevant kernel code/patches?
>
>
> Let me make it clear so that there is no confusion, there is no support of
> physical "CPU" hot-plug on ARM platforms right now and nor will there be
> any in future as it does not makes sense to have. The point I made in the
> patches is about hot-plug was at different granularity which has not been
> denied by ARM.
_STA and ACPI cphp registers are per logical CPU and can't be anything else
per spec.
how different granularity is relevant here?
>
> >
> > > and physical CPU hot plug can co-exists or entirely as sole features.
> > > This is a requirement.
> >
> >
> > I don't see any _must_ requirements here whatsoever. If/when ARM
> > reaches point where standby and hotplug cpus are mixed within VM
> > instance, we might have to expose presence bit to guest dynamically but it
> > is totally not needed within observable future and premature.
>
>
> > Your cpu class presence check also works target-wise just with more boiler
> > code
> > + not needed presence bit ABI for guest side,
> > The same as my suggestion only from other side.
> >
> > But regardless of that as long as machine has doesn't mix always present
> > CPUs with hotpluggable ones within the same instance, changing AML side
> > as I've just suggested works.
>
>
> Sure, I'm not disagreeing. It will work by adding the flag you've suggested
> but it will work even by not adding any flag and not defining a `persistent`
> hook for any architecture.
hook is more complicated and hidden way, than directly passing down
configuration data to routine that builds AML where it is needed,
but that's cosmetics in the end.
However there is more to your hook approach, it exposes is_present bit
in cphp flag register which is ABI to guest which we will have to
maintain forever and guest will do not necessary round-trip to QEMU
to query it, while alternative is much simpler AML change without any
ABI changes to care about.
The point is we shouldn't add new ABI unless we have to.
In this case new ABI (is_presence flag) is not _must_,
and much simpler static AML change is sufficient.
> >
> > If ARM ever gets real cpu hotplug as your comment above hints, my
> > suggestion also works fine. With only difference that board code would turn
> > off always_present_cpus if hotpluggable CPUs are used instead of standby.
>
>
> Sure, but is it necessary to define a new flag when you can do even without it?
> Having few lines of extra code (literally 2-3 only) should not be a major cause of
> worry, especially, if it makes design more clear and easy to understand. This is
> not a performance code either.
>
> Again, I appreciate your compact logic. I'm not disagreeing with it.
>
>
> > > > 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;
> > >
> > >
> > > This has to be fetched from architecture code. Please see other
> > > changes in the V3 patch-set.
> >
> > I've seen, that and it doesn't have to be fetched dynamically.
>
>
> Agreed, it is not necessary to be fetched. Hence, if you do not define the
> hook. In later case, it assumes the existing logic, which x86 has been following.
> It will work.
It is better to get rid of not necessary hook altogether, and
replace it with a simple AML change.
> > In my opinion the series was not ready to be merged (Michael probably
> > picked it by mistake).
> >
> > We don't really need this in 9.2 as it is only ARM cpu 'hotplug'
> > related, and the later is not ready for 9.2.
> > I'd prefer the series being reverted and we continue improving series,
> > instead of rushing it and fixing broken thing up.
> >
> >
> > >
> > >
> > > > 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
> > >
> > >
> > > We MUST fetch this from architecture code as this can dynamically
> > > change in future and hence, we need to keep that flexibility
> >
> > CPUHotplugFeatures::always_present_cpus can be set dynamically per VM
> > instance or per Machine type.
>
>
> Yes, but you need a code for that and I'm not sure what is being saved by
> introducing an extra flag then?
beside snippet, I've suggested. You need to delete all is_present callback
related logic, than it save quite a bit, and not only on lines of code.
I guess, I need to send a patch to get point across.
>
>
> > > > 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).
> > >
> > >
> > > The CPUs AML in the V2 patch-set would have broken the x86
> > functionality.
> >
> > Frankly speaking, I don't see much progress. All that happens on respins is
> > flipping between what I asked to remove before to some earlier concept.
>
>
> I think then you've missed the code which addressed one of your key concerns
> in the V1 patch-set that would have broken x86 migration i.e. presence of the
> `is_enabled` state in the CPU Hot-plug VMSD. That has been removed. Maybe
> you would like to go through the change log of the V3 patch-set
>
> https://lore.kernel.org/qemu-devel/20241018163118.0ae01a84@imammedo.users.ipa.redhat.com/
>
> Below is the relevant excerpt from your many comments:
>
> [...]
> > .fields = (const VMStateField[]) {
> > VMSTATE_BOOL(is_inserting, AcpiCpuStatus),
> > VMSTATE_BOOL(is_removing, AcpiCpuStatus),
> > + VMSTATE_BOOL(is_present, AcpiCpuStatus),
> > + VMSTATE_BOOL(is_enabled, AcpiCpuStatus),
>
> that's likely will break x86 migration,
> but before bothering peterx, maybe we won't need this hunk if
> is_enabled is migrated as part of vCPU state.
>
what has been done in v3 is moving is_present/is_enabled,
elsewhere (callbacks in this case). While argument was that
both are not necessary to begin with.
> [...]
>
>
> > And all of that for the purpose to workaround/accommodate fake cpu
> > hotplug hacks.
>
>
> I've to respectfully disagree on this. This is your assumption which is far from
> reality. The accompanying main series of this will never have vCPUs which are
> *not* present.
Reality of your last posted main series (v5), disagrees with what you are saying here
[PATCH RFC V5 12/30] arm/virt: Release objects for *disabled* possible vCPUs after init
https://patchew.org/QEMU/20241015100012.254223-1-salil.mehta@huawei.com/20241015100012.254223-13-salil.mehta@huawei.com/
after this patch, new is_present hook would let you bury the lie
about CPU being present inside ARM specific code.
> BTW, these changes have been tested by Oracle folks with that
> series and are known to work.
>
> https://lore.kernel.org/qemu-devel/C4FEC9E7-69DB-428A-A85F-30170F98814B@oracle.com/
One can write anything and it can even work somehow,
that however doesn't mean it's something merge-able,
maintainable, ...whatever else come to mind...
>
> Thanks!
>
> Best regards
> Salil.
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability
2024-11-07 16:57 ` Igor Mammedov
@ 2024-11-07 18:59 ` Salil Mehta via
2024-11-08 16:49 ` Igor Mammedov
0 siblings, 1 reply; 11+ messages in thread
From: Salil Mehta via @ 2024-11-07 18:59 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, mst@redhat.com,
Jonathan Cameron, peter.maydell@linaro.org,
richard.henderson@linaro.org, anisinha@redhat.com,
eduardo@habkost.net, marcel.apfelbaum@gmail.com, david@redhat.com,
philmd@linaro.org, peterx@redhat.com, pbonzini@redhat.com,
gshan@redhat.com, borntraeger@linux.ibm.com,
alex.bennee@linaro.org, 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,
wangxiongfeng (C), wangyanan (Y), zhao1.liu@intel.com, Linuxarm,
gustavo.romero@linaro.org
Hi Igor,
Many thanks for taking time to reply.
> From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
> arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
> Mammedov
> Sent: Thursday, November 7, 2024 4:57 PM
> To: Salil Mehta <salil.mehta@huawei.com>
>
> On Wed, 6 Nov 2024 19:05:15 +0000
> Salil Mehta <salil.mehta@huawei.com> wrote:
>
> > Hi Igor,
> >
> > Thanks for replying back and the reviews. Please find my replies
> > inline.
> >
> > > From: Igor Mammedov <imammedo@redhat.com>
> > > Sent: Wednesday, November 6, 2024 4:08 PM
> > > To: Salil Mehta <salil.mehta@huawei.com>
> > >
> > > On Wed, 6 Nov 2024 14:45:42 +0000
> > > Salil Mehta <salil.mehta@huawei.com> wrote:
> > >
> > > > Hi Igor,
> > > >
> > > > > From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org
> > > <qemu-
> > > > > arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of
> > > Igor > > Mammedov > > Sent: Wednesday, November 6, 2024 1:57 PM
> > > > > To: Salil Mehta <salil.mehta@huawei.com> > > > > On Wed, 6
> > > Nov 2024 13:03:30 +0000 > > Salil Mehta <salil.mehta@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 <imammedo@redhat.com>
> > > 'Reported-by'
> > > > > maybe, but certainly not suggested.
> > > >
> > > >
> > > > No issues. I can change.
> > > >
> > > >
> > > > >
> > > > > 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).
> > > >
> > > >
> > > > This is a wrong assumption. Presence bit can change in future. We
> > > have > taken into account this aspect by design in the kernel code as
> well.
> > > > Both virtual
> > >
> > > Above does imply that support for really hotpluggable CPUs might be
> > > implemented in the future.
> > > Can you point to relevant kernel code/patches?
> >
> >
> > Let me make it clear so that there is no confusion, there is no
> > support of physical "CPU" hot-plug on ARM platforms right now and nor
> > will there be any in future as it does not makes sense to have. The
> > point I made in the patches is about hot-plug was at different
> > granularity which has not been denied by ARM.
>
> _STA and ACPI cphp registers are per logical CPU and can't be anything else
> per spec.
>
> how different granularity is relevant here?
It is because hot-plug at socket level (or similar level) has not been denied.
> > > > and physical CPU hot plug can co-exists or entirely as sole features.
> > > > This is a requirement.
> > >
> > >
> > > I don't see any _must_ requirements here whatsoever. If/when ARM
> > > reaches point where standby and hotplug cpus are mixed within VM
> > > instance, we might have to expose presence bit to guest dynamically
> > > but it is totally not needed within observable future and premature.
> >
> >
> > > Your cpu class presence check also works target-wise just with more
> > > boiler code + not needed presence bit ABI for guest side, The
> > > same as my suggestion only from other side.
> > >
> > > But regardless of that as long as machine has doesn't mix always
> > > present CPUs with hotpluggable ones within the same instance,
> > > changing AML side as I've just suggested works.
> >
> >
> > Sure, I'm not disagreeing. It will work by adding the flag you've
> > suggested but it will work even by not adding any flag and not
> > defining a `persistent` hook for any architecture.
>
> hook is more complicated and hidden way, than directly passing down
> configuration data to routine that builds AML where it is needed, but that's
> cosmetics in the end.
It is indeed an abstraction and an intentional one because decision whether
some or all CPUs should remain present even after unplug action is
specific to an architecture. Hence, should be left with that part.
>
> However there is more to your hook approach, it exposes is_present bit in
> cphp flag register which is ABI to guest which we will have to maintain
> forever and guest will do not necessary round-trip to QEMU to query it,
> while alternative is much simpler AML change without any ABI changes to
> care about.
I understand your predicament about ABI but I've to respectfully disagree on
the assumption that guest will never round trip and check for the presence bit.
>
> The point is we shouldn't add new ABI unless we have to.
Ok sure. point taken.
> In this case new ABI (is_presence flag) is not _must_, and much simpler
> static AML change is sufficient.
Agreed, there can be multiple ways to get rid of it . But I'm not sure if its
simpler or easier to maintain because CPUs AML code has become even
more obscure by that compact logic. It is not easy to understand at the
first look. This is not a performance leg and maintainability should take
over the precedence.
But I understand your primary concern now (i.e. to get rid of CPU_PRESENT
Bit) and allow me to propose in the lines of what you want but maybe in a
slightly different way. I would still like to retain hooks though.
> > > If ARM ever gets real cpu hotplug as your comment above hints, my
> > > suggestion also works fine. With only difference that board code
> > > would turn off always_present_cpus if hotpluggable CPUs are used
> instead of standby.
> >
> >
> > Sure, but is it necessary to define a new flag when you can do even
> without it?
> > Having few lines of extra code (literally 2-3 only) should not be a
> > major cause of worry, especially, if it makes design more clear and
> > easy to understand. This is not a performance code either.
> >
> > Again, I appreciate your compact logic. I'm not disagreeing with it.
> >
> >
> > > > > 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;
> > > >
> > > >
> > > > This has to be fetched from architecture code. Please see other
> > > > changes in the V3 patch-set.
> > >
> > > I've seen, that and it doesn't have to be fetched dynamically.
> >
> >
> > Agreed, it is not necessary to be fetched. Hence, if you do not define
> > the hook. In later case, it assumes the existing logic, which x86 has been
> following.
> > It will work.
>
> It is better to get rid of not necessary hook altogether, and replace it with a
> simple AML change.
We still need a way to initialize flags in the architecture specific way. Hooks are
clean way to fetch that architecture specific state rather fiddling with flags
at multiple levels (initialization, migration etc) and you will be duplicating
CPU states at ACPI level and QOM vCPU object level . To be frank that's not
a very good design.
That said, I believe your end goal of getting rid of the change in the ABI
can still be achieved.
> > > In my opinion the series was not ready to be merged (Michael
> > > probably picked it by mistake).
> > >
> > > We don't really need this in 9.2 as it is only ARM cpu 'hotplug'
> > > related, and the later is not ready for 9.2.
> > > I'd prefer the series being reverted and we continue improving
> > > series, instead of rushing it and fixing broken thing up.
> > >
> > >
> > > >
> > > >
> > > > > 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
> > > >
> > > >
> > > > We MUST fetch this from architecture code as this can dynamically
> > > > change in future and hence, we need to keep that flexibility
> > >
> > > CPUHotplugFeatures::always_present_cpus can be set dynamically per
> > > VM instance or per Machine type.
> >
> >
> > Yes, but you need a code for that and I'm not sure what is being saved
> > by introducing an extra flag then?
>
> beside snippet, I've suggested. You need to delete all is_present callback
> related logic, than it save quite a bit, and not only on lines of code.
>
> I guess, I need to send a patch to get point across.
There is some misunderstanding. We are not maintaining any `is_present`
state at the ACPI level. Both flags `is_present` and `is_enabled` were
removed in V3 patch-set.
> > > > > 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).
> > > >
> > > >
> > > > The CPUs AML in the V2 patch-set would have broken the x86
> > > functionality.
> > >
> > > Frankly speaking, I don't see much progress. All that happens on
> > > respins is flipping between what I asked to remove before to some
> earlier concept.
> >
> >
> > I think then you've missed the code which addressed one of your key
> > concerns in the V1 patch-set that would have broken x86 migration i.e.
> > presence of the `is_enabled` state in the CPU Hot-plug VMSD. That has
> > been removed. Maybe you would like to go through the change log of the
> > V3 patch-set
> >
> > https://lore.kernel.org/qemu-
> devel/20241018163118.0ae01a84@imammedo.us
> > ers.ipa.redhat.com/
> >
> > Below is the relevant excerpt from your many comments:
> >
> > [...]
> > > .fields = (const VMStateField[]) {
> > > VMSTATE_BOOL(is_inserting, AcpiCpuStatus),
> > > VMSTATE_BOOL(is_removing, AcpiCpuStatus),
> > > + VMSTATE_BOOL(is_present, AcpiCpuStatus),
> > > + VMSTATE_BOOL(is_enabled, AcpiCpuStatus),
> >
> > that's likely will break x86 migration, but before bothering peterx,
> > maybe we won't need this hunk if is_enabled is migrated as part of
> > vCPU state.
> >
>
> what has been done in v3 is moving is_present/is_enabled, elsewhere
> (callbacks in this case). While argument was that both are not necessary to
> begin with.
We need to have a way to check whether CPU is _STA.Enabled or not and
the meaning of the `disabled` is best decided by the architecture specific
code not the ACPI code. It could mean different across architectures and
the implementations. We need to keep that flexibility in design.
The thin Qemu ACPI code is just an interfacing logic with the guest. It is
best not to duplicate the state of the QOM objects inside the ACPI. That's
a bad design practice because we will have to worry about keeping those
states consistent at both the places all the time whether it is during
initialization or migration.
>
> > [...]
> >
> >
> > > And all of that for the purpose to workaround/accommodate fake cpu
> > > hotplug hacks.
> >
> >
> > I've to respectfully disagree on this. This is your assumption which
> > is far from reality. The accompanying main series of this will never
> > have vCPUs which are
> > *not* present.
>
> Reality of your last posted main series (v5), disagrees with what you are
> saying here
>
> [PATCH RFC V5 12/30] arm/virt: Release objects for *disabled* possible
> vCPUs after init
> https://patchew.org/QEMU/20241015100012.254223-1-
> salil.mehta@huawei.com/20241015100012.254223-13-
> salil.mehta@huawei.com/
I think you missed what I mentioned above " The accompanying main series...".
To get more information about that series, please check the details mentioned
in the cover-letter of the V3 patch-set (link below) which clearly says,
https://lore.kernel.org/qemu-devel/20241103102419.202225-1-salil.mehta@huawei.com/
Excerpt from the cover letter:
========================
[...]
This patch-set has been tested alongside ARM-specific
vCPU hotplug changes (included in the upcoming RFC V6 [4]), and
hot(un)plug functionalities performed as expected which concerns this
patch-set. Please have a look.
[...]
References
==========
[...]
[4] Upcoming RFC V6, Support of Virtual CPU Hotplug for ARMv8 Arch
Link: https://github.com/salil-mehta/qemu/commits/virt-cpuhp-armv8/rfc-v6-rc5
[...]
> after this patch, new is_present hook would let you bury the lie about CPU
> being present inside ARM specific code.
I would request you to check the facts again! (please see previous pointers)
ACPI logic should not worry about what is happening inside architecture
code. It is beyond its realms and should be by design. It is an interfacing upper
layer gelling the Guest with the VMM.
> > BTW, these changes have been tested by Oracle folks with that series
> > and are known to work.
> >
> > https://lore.kernel.org/qemu-devel/C4FEC9E7-69DB-428A-A85F-
> 30170F98814
> > B@oracle.com/
>
> One can write anything and it can even work somehow, that however
> doesn't mean it's something merge-able, maintainable, ...whatever else
> come to mind...
It would be very helpful if you can `objectively` point the problems in the
main series so that there is a context and we can have a productive
discussion there. There is always a scope of improvement.
Many thanks!
Best regards
Salil.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability
2024-11-07 18:59 ` Salil Mehta via
@ 2024-11-08 16:49 ` Igor Mammedov
0 siblings, 0 replies; 11+ messages in thread
From: Igor Mammedov @ 2024-11-08 16:49 UTC (permalink / raw)
To: Salil Mehta
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, mst@redhat.com,
Jonathan Cameron, peter.maydell@linaro.org,
richard.henderson@linaro.org, anisinha@redhat.com,
eduardo@habkost.net, marcel.apfelbaum@gmail.com, david@redhat.com,
philmd@linaro.org, peterx@redhat.com, pbonzini@redhat.com,
gshan@redhat.com, borntraeger@linux.ibm.com,
alex.bennee@linaro.org, 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,
wangxiongfeng (C), wangyanan (Y), zhao1.liu@intel.com, Linuxarm,
gustavo.romero@linaro.org
On Thu, 7 Nov 2024 18:59:07 +0000
Salil Mehta <salil.mehta@huawei.com> wrote:
> Hi Igor,
>
> Many thanks for taking time to reply.
>
> > From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
> > arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
> > Mammedov
> > Sent: Thursday, November 7, 2024 4:57 PM
> > To: Salil Mehta <salil.mehta@huawei.com>
> >
> > On Wed, 6 Nov 2024 19:05:15 +0000
> > Salil Mehta <salil.mehta@huawei.com> wrote:
> >
> > > Hi Igor,
> > >
> > > Thanks for replying back and the reviews. Please find my replies
> > > inline.
> > >
> > > > From: Igor Mammedov <imammedo@redhat.com>
> > > > Sent: Wednesday, November 6, 2024 4:08 PM
> > > > To: Salil Mehta <salil.mehta@huawei.com>
> > > >
> > > > On Wed, 6 Nov 2024 14:45:42 +0000
> > > > Salil Mehta <salil.mehta@huawei.com> wrote:
> > > >
> > > > > Hi Igor,
> > > > >
> > > > > > From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org
> > > > <qemu-
> > > > > > arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of
> > > > Igor > > Mammedov > > Sent: Wednesday, November 6, 2024 1:57 PM
> > > > > > To: Salil Mehta <salil.mehta@huawei.com> > > > > On Wed, 6
> > > > Nov 2024 13:03:30 +0000 > > Salil Mehta <salil.mehta@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 <imammedo@redhat.com>
> > > > 'Reported-by'
> > > > > > maybe, but certainly not suggested.
> > > > >
> > > > >
> > > > > No issues. I can change.
> > > > >
> > > > >
> > > > > >
> > > > > > 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).
> > > > >
> > > > >
> > > > > This is a wrong assumption. Presence bit can change in future. We
> > > > have > taken into account this aspect by design in the kernel code as
> > well.
> > > > > Both virtual
> > > >
> > > > Above does imply that support for really hotpluggable CPUs might be
> > > > implemented in the future.
> > > > Can you point to relevant kernel code/patches?
> > >
> > >
> > > Let me make it clear so that there is no confusion, there is no
> > > support of physical "CPU" hot-plug on ARM platforms right now and nor
> > > will there be any in future as it does not makes sense to have. The
> > > point I made in the patches is about hot-plug was at different
> > > granularity which has not been denied by ARM.
> >
> > _STA and ACPI cphp registers are per logical CPU and can't be anything else
> > per spec.
> >
> > how different granularity is relevant here?
>
>
> It is because hot-plug at socket level (or similar level) has not been denied.
It's the same for x86.
However anything above logical CPU level is not CPU devices,
but container devices. Even if you plugged whole socket,
in QEMU that would result in _STA evaluated on all leaf logical CPUs
and guest will online those CPUs individually.
Granularity here is irrelevant, it handles only vCPUs an nothing else.
If there were need to do something with containers (socket/cluster/...)
in hypothetical future, that should have it's own AML code instead of
abusing vCPU one, and well it certainly should be added only at that
time and not earlier.
> > > > > and physical CPU hot plug can co-exists or entirely as sole features.
> > > > > This is a requirement.
> > > >
> > > >
> > > > I don't see any _must_ requirements here whatsoever. If/when ARM
> > > > reaches point where standby and hotplug cpus are mixed within VM
> > > > instance, we might have to expose presence bit to guest dynamically
> > > > but it is totally not needed within observable future and premature.
> > >
> > >
> > > > Your cpu class presence check also works target-wise just with more
> > > > boiler code + not needed presence bit ABI for guest side, The
> > > > same as my suggestion only from other side.
> > > >
> > > > But regardless of that as long as machine has doesn't mix always
> > > > present CPUs with hotpluggable ones within the same instance,
> > > > changing AML side as I've just suggested works.
> > >
> > >
> > > Sure, I'm not disagreeing. It will work by adding the flag you've
> > > suggested but it will work even by not adding any flag and not
> > > defining a `persistent` hook for any architecture.
> >
> > hook is more complicated and hidden way, than directly passing down
> > configuration data to routine that builds AML where it is needed, but that's
> > cosmetics in the end.
>
>
> It is indeed an abstraction and an intentional one because decision whether
> some or all CPUs should remain present even after unplug action is
> specific to an architecture. Hence, should be left with that part.
Arch specific static AML is much simpler in this case, compared to callbacks.
and this AML is still controlled by machine specific code, which
opts in into always_present cpus feature (it's machine that build's AML
code depending on options).
We do the same in x86, see other flags in CPUHotplugFeatures.
Guest opts in to change curtain parts of AML depending on machine config,
same should be done for ARM.
> > However there is more to your hook approach, it exposes is_present bit in
> > cphp flag register which is ABI to guest which we will have to maintain
> > forever and guest will do not necessary round-trip to QEMU to query it,
> > while alternative is much simpler AML change without any ABI changes to
> > care about.
>
>
> I understand your predicament about ABI but I've to respectfully disagree on
> the assumption that guest will never round trip and check for the presence bit.
It doesn't need it now, isn't it?
When guest really needs to get presence flag dynamically in the future,
that's fine, but do introduce it at that time when it's actually used,
i.e. not for hypothetical case.
> >
> > The point is we shouldn't add new ABI unless we have to.
>
>
> Ok sure. point taken.
>
>
> > In this case new ABI (is_presence flag) is not _must_, and much simpler
> > static AML change is sufficient.
>
>
> Agreed, there can be multiple ways to get rid of it . But I'm not sure if its
> simpler or easier to maintain because CPUs AML code has become even
> more obscure by that compact logic. It is not easy to understand at the
> first look. This is not a performance leg and maintainability should take
> over the precedence.
As one who wrote and takes care ACPI and cpuhp, I'd know what code is easier
to read/maintain in the arrea and to a lesser degree elsewhere in QEMU.
When it comes to arch specific side, I usually leave it upto arch maintainers
as they know specifics there much better.
(unless I notice something that is too much to look away and asks to for bashing)
> But I understand your primary concern now (i.e. to get rid of CPU_PRESENT
> Bit) and allow me to propose in the lines of what you want but maybe in a
> slightly different way. I would still like to retain hooks though.
hooks do have it's use when logic they are executing is complex, dynamic and
differs a lot between impl, but indirection they introduce makes
reading code and maintaining it harder (debugging it becomes outright nightmare).
Example: I did the same as you in the past, adding madt hook for some future
use, in several years it began to get in the way and eventually was removed.
Burden to clean up (possible in the future but not happened things) falls
on new contributors. Moral of story is: not over-engineer.
If hook can be replaced by config data and data driven code, I'd prefer
it from ease to read/maintainability point of view.
> > > > If ARM ever gets real cpu hotplug as your comment above hints, my
> > > > suggestion also works fine. With only difference that board code
> > > > would turn off always_present_cpus if hotpluggable CPUs are used
> > instead of standby.
> > >
> > >
> > > Sure, but is it necessary to define a new flag when you can do even
> > without it?
> > > Having few lines of extra code (literally 2-3 only) should not be a
> > > major cause of worry, especially, if it makes design more clear and
> > > easy to understand. This is not a performance code either.
> > >
> > > Again, I appreciate your compact logic. I'm not disagreeing with it.
> > >
> > >
> > > > > > 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;
> > > > >
> > > > >
> > > > > This has to be fetched from architecture code. Please see other
> > > > > changes in the V3 patch-set.
> > > >
> > > > I've seen, that and it doesn't have to be fetched dynamically.
> > >
> > >
> > > Agreed, it is not necessary to be fetched. Hence, if you do not define
> > > the hook. In later case, it assumes the existing logic, which x86 has been
> > following.
> > > It will work.
> >
> > It is better to get rid of not necessary hook altogether, and replace it with a
> > simple AML change.
>
>
> We still need a way to initialize flags in the architecture specific way. Hooks are
> clean way to fetch that architecture specific state rather fiddling with flags
> at multiple levels (initialization, migration etc) and you will be duplicating
> CPU states at ACPI level and QOM vCPU object level . To be frank that's not
> a very good design.
cleaner way is to use properties to access object state in arch agnostic way.
and cpuhp didn't access any cpu state before your bf1ecc8dad60619 commit
...
- val |= cdev->cpu ? 1 : 0;
+ val |= check_cpu_enabled_status(DEVICE(cdev->cpu)) ? 1 : 0
...
poking into CPU internals (masked by helper function/hook) from another device
that is not even parent of CPU, which is layer violation and
should be avoided if possible.
In case of cpuhp there is hotplug handlers infrastructure, to wire up
and connect things.
I particular, machine called acpi_cpu_plug_cb() which may access cpu,
and in your case it can check 'realized' property and store 'enabled'
state in AcpiCpuStatus state. It would be even better if it used
a dedicated property "enabled", instead of abusing 'realized' one.
And do opposite on unplug (can be done from unplug handler).
> That said, I believe your end goal of getting rid of the change in the ABI
> can still be achieved.
>
>
> > > > In my opinion the series was not ready to be merged (Michael
> > > > probably picked it by mistake).
> > > >
> > > > We don't really need this in 9.2 as it is only ARM cpu 'hotplug'
> > > > related, and the later is not ready for 9.2.
> > > > I'd prefer the series being reverted and we continue improving
> > > > series, instead of rushing it and fixing broken thing up.
> > > >
> > > >
> > > > >
> > > > >
> > > > > > 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
> > > > >
> > > > >
> > > > > We MUST fetch this from architecture code as this can dynamically
> > > > > change in future and hence, we need to keep that flexibility
> > > >
> > > > CPUHotplugFeatures::always_present_cpus can be set dynamically per
> > > > VM instance or per Machine type.
> > >
> > >
> > > Yes, but you need a code for that and I'm not sure what is being saved
> > > by introducing an extra flag then?
> >
> > beside snippet, I've suggested. You need to delete all is_present callback
> > related logic, than it save quite a bit, and not only on lines of code.
> >
> > I guess, I need to send a patch to get point across.
>
>
> There is some misunderstanding. We are not maintaining any `is_present`
> state at the ACPI level. Both flags `is_present` and `is_enabled` were
> removed in V3 patch-set.
That was removed from AcpiCpuStatus, however
v3 does introduce present bit in register and callback to fetch it.
It is the same thing, just other way around.
> > > > > > 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).
> > > > >
> > > > >
> > > > > The CPUs AML in the V2 patch-set would have broken the x86
> > > > functionality.
> > > >
> > > > Frankly speaking, I don't see much progress. All that happens on
> > > > respins is flipping between what I asked to remove before to some
> > earlier concept.
> > >
> > >
> > > I think then you've missed the code which addressed one of your key
> > > concerns in the V1 patch-set that would have broken x86 migration i.e.
> > > presence of the `is_enabled` state in the CPU Hot-plug VMSD. That has
> > > been removed. Maybe you would like to go through the change log of the
> > > V3 patch-set
> > >
> > > https://lore.kernel.org/qemu-
> > devel/20241018163118.0ae01a84@imammedo.us
> > > ers.ipa.redhat.com/
> > >
> > > Below is the relevant excerpt from your many comments:
> > >
> > > [...]
> > > > .fields = (const VMStateField[]) {
> > > > VMSTATE_BOOL(is_inserting, AcpiCpuStatus),
> > > > VMSTATE_BOOL(is_removing, AcpiCpuStatus),
> > > > + VMSTATE_BOOL(is_present, AcpiCpuStatus),
> > > > + VMSTATE_BOOL(is_enabled, AcpiCpuStatus),
> > >
> > > that's likely will break x86 migration, but before bothering peterx,
> > > maybe we won't need this hunk if is_enabled is migrated as part of
> > > vCPU state.
> > >
> >
> > what has been done in v3 is moving is_present/is_enabled, elsewhere
> > (callbacks in this case). While argument was that both are not necessary to
> > begin with.
>
>
> We need to have a way to check whether CPU is _STA.Enabled or not and
> the meaning of the `disabled` is best decided by the architecture specific
> code not the ACPI code. It could mean different across architectures and
> the implementations. We need to keep that flexibility in design.
I'm not arguing about 'enabled' in principle, only about the way it's
implemented. And that 'present' shouldn't be part of dynamic interface.
If you ever need present as dynamic in the future, introduce it at that
time along with patches that would use it.
> The thin Qemu ACPI code is just an interfacing logic with the guest. It is
> best not to duplicate the state of the QOM objects inside the ACPI. That's
> a bad design practice because we will have to worry about keeping those
> states consistent at both the places all the time whether it is during
> initialization or migration.
if we'd follow this logic, we would have a QEMU with direct access across
different objects all over the place. That might work for small project
and avoid duplication, but that is very fragile and won't scale to
project of QEMU size.
And to configure devices we have init/property/hotplug handlers,
so that machine could connect separate objects together in meaning full
way and establish well known control flow.
> > > [...]
> > >
> > >
> > > > And all of that for the purpose to workaround/accommodate fake cpu
> > > > hotplug hacks.
> > >
> > >
> > > I've to respectfully disagree on this. This is your assumption which
> > > is far from reality. The accompanying main series of this will never
> > > have vCPUs which are
> > > *not* present.
> >
> > Reality of your last posted main series (v5), disagrees with what you are
> > saying here
> >
> > [PATCH RFC V5 12/30] arm/virt: Release objects for *disabled* possible
> > vCPUs after init
> > https://patchew.org/QEMU/20241015100012.254223-1-
> > salil.mehta@huawei.com/20241015100012.254223-13-
> > salil.mehta@huawei.com/
>
>
> I think you missed what I mentioned above " The accompanying main series...".
> To get more information about that series, please check the details mentioned
> in the cover-letter of the V3 patch-set (link below) which clearly says,
>
> https://lore.kernel.org/qemu-devel/20241103102419.202225-1-salil.mehta@huawei.com/
>
> Excerpt from the cover letter:
> ========================
> [...]
> This patch-set has been tested alongside ARM-specific
> vCPU hotplug changes (included in the upcoming RFC V6 [4]), and
> hot(un)plug functionalities performed as expected which concerns this
> patch-set. Please have a look.
> [...]
>
> References
> ==========
> [...]
> [4] Upcoming RFC V6, Support of Virtual CPU Hotplug for ARMv8 Arch
> Link: https://github.com/salil-mehta/qemu/commits/virt-cpuhp-armv8/rfc-v6-rc5
> [...]
that v6 series is not posted to the list, so it can disappear
or change any moment. Using it as reference is pointless.
There is no point to comment on something that is not on
the list as later on the github branch can be totally different
from what it was at the time comment was made.
That is also a reason why (v3) ARM cpuhp specific ACPI patches
shouldn't have been merged separately from main series.
It was fine when generic acpi cpu hotplug patches where posted as
separate series as those were working with several different RFCs
for cpuhp for different targets that would reuse it.
However posting ARM specific cpuhp acpi changes separately,
without actual user along with them only lead to confusion
and mess we have now.
I suggest you to send revert for merged v3,
and resend those (fixed up) patches as part of main series
where they can be properly reviewed.
(or I can sent it along with a couple of RFC patches doing
the same but in acceptable way from ACPI pov.
The later could be picked up into your next main series)
> > after this patch, new is_present hook would let you bury the lie about CPU
> > being present inside ARM specific code.
>
>
> I would request you to check the facts again! (please see previous pointers)
>
> ACPI logic should not worry about what is happening inside architecture
> code. It is beyond its realms and should be by design. It is an interfacing upper
> layer gelling the Guest with the VMM.
>
>
> > > BTW, these changes have been tested by Oracle folks with that series
> > > and are known to work.
> > >
> > > https://lore.kernel.org/qemu-devel/C4FEC9E7-69DB-428A-A85F-
> > 30170F98814
> > > B@oracle.com/
> >
> > One can write anything and it can even work somehow, that however
> > doesn't mean it's something merge-able, maintainable, ...whatever else
> > come to mind...
>
>
> It would be very helpful if you can `objectively` point the problems in the
> main series so that there is a context and we can have a productive
> discussion there. There is always a scope of improvement.
I did point to problems in the past (the latest was v2 review),
repeating it now again.
Design wise issue is that ARM architecture doesn't support
cpu hotplug. That's the reason why ARM kernel folks haven't
implemented it there since it's not what hardware does.
Quotes from [2]
'There is no hardware that does anything of this.'
'Whatever is defined for virtual machines needs to work on physical
hardware too – the OS doesn’t know its in a VM'
Above is also a good reason not to try implementing it on
QEMU side as hotplug as well.
My suggestion was to try instead standby CPUs concept
that ARM does support, which looks like hotplug on
guest side (kernel largely freely mixes up hotplug term
with CPUs onlining).
The only reply I got to this:
unsure, and 'requirements' as if it set in stone and pushed on
QEMU using some presentations [2] as reference, which is basically
a set of issues and wishes plus some ideas how it could be achieved.
Those wishes are by no means any form of consensus on how things should be.
* 'must reuse device_add/del for smooth transition from x86'
this one I guess comes from flowing quote:
'Should have look-and-feel of x86_64 platform VCPU hot-plug feature'
and then accompanied kernel presentation phrase:
'No appetite for updating numerous specifications to describe hotplug support'
On that one can easily counter:
'then make it virt hardware behave like x86 and write specs for it,
instead of hacking the heck out of QEMU'
But that is beside the point, management software can easily adapt
to dedicated QMP command to online CPUs without abusing device_add/del.
ARM's 'cpu hotplug' doesn't have to be drop in replacement for ARM,
you do configure ARM VM differently, you can do 'hotplug'
differently as well, nothing prevents it beside having a will to do it.
* 'for large VM to start fast'
that one I guess, comes from following phrase:
'allowing for faster boot time'
but whole bullet point talks about scalability
'Kata Containers VM starts with a minimum amount of resources, (hot-plug everything)'
that somehow turned in to requirement:
'start VM as fast as if real CPU hotplug is supported regardless of the costs'
as result main series hack is to avoid creation of possible CPUs,
Depending on series revision it ranged from creation of half initialized vCPU
objects and then deleting them to creating half initialized vCPU
and letting them dangle in memory without completing its creation
in the latest series incarnation (v6). And all of that is to avoid
calling expensive in current impl. cpu_realize.
The rest series is some necessary boiler plate code + other hacks
trying to convince the rest of QEMU that dangling half initialized CPU objects
are nothing to worry about.
well with standby cpu design you can also have 'allowing for faster boot time'
it is not as fast as x86 with its hotplug but still faster than onlining
all possible CPUs at boot time on guest side.
Even 'faster startup' wish is stretching things, one just puts off delays
to a later time in guest lifetime where every single vCPU hotplug
incurs even bigger time penalty as guest kernel has to stop everything
to hotplug a CPU.
The point is: 'standby cpus' design also leads to faster boot
on guest side compared normal boot with all vCPUs.
As for comparing with even faster x86, fix cpu_realize to be
faster (as I demonstrated in v2 review) or fix design to
implement real hotplug [1], instead of another quote from [2]
'Creative Hack – Which Works! Really?'
So my suggestion to try 'standby cpus' approach still stays,
fully realizing CPUs.
Also, you can't reuse device_add with it as that means you'd be forced to
hack workflow one way or another for what it wasn't designed.
Add a new QMP command instead 'device-on/device-off' that
would fit standby concept better.
You will very likely end up with much simpler series that it's now,
probably even more simple than recent loongarch cpu hotplug [3]
PS:
1) If hotplug support is really need, try to convince ARM
to implement it (as minimum for virt environment,
that likely would require some spec & kernel work first).
Or wait until business needs wake up ARM's appetite to do it right.
2)
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
3)
PS2:
As a recent example of platform that does have 'appetite' to implement 'real' cpu hotplug see
[PATCH v3 0/5] hw/loongarch/virt: Add cpu hotplug support
https://lore.kernel.org/all/20241104063435.4130262-6-maobibo@loongson.cn/T/
and compare with ARM RFC,
it looks much more cleaner and strait-forward than ARM variant when one
doesn't need hack QEMU here and there to make it work.
PS3:
joke:
typical QEMU maintainer response to large series
'rewrite half of QEMU 1st, then do you thing and we will see'
ARM cpuhp gets much milder treatment
'rewrite your series in other way that matches real world'
> Many thanks!
>
> Best regards
> Salil.
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-11-08 16:50 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-06 13:03 [PATCH 0/3] Fixes CPUs AML & acpi-bios-tables to be x86 backward compatible Salil Mehta via
2024-11-06 13:03 ` [PATCH 1/3] qtest: allow ACPI DSDT Table changes Salil Mehta via
2024-11-06 13:03 ` [PATCH 2/3] Fix: Reverse CPUs presence check logic for x86 backward compatability Salil Mehta via
2024-11-06 13:56 ` Igor Mammedov
2024-11-06 14:45 ` Salil Mehta via
2024-11-06 16:07 ` Igor Mammedov
2024-11-06 19:05 ` Salil Mehta via
2024-11-07 16:57 ` Igor Mammedov
2024-11-07 18:59 ` Salil Mehta via
2024-11-08 16:49 ` Igor Mammedov
2024-11-06 13:03 ` [PATCH 3/3] tests/qtest/bios-tables-test: Fix DSDT golden masters for x86/{pc, q35} Salil Mehta via
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).