* [PATCH 0/2] hw/acpi: bump MADT to revision 5
@ 2023-03-28 15:59 Eric DeVolder
2023-03-28 15:59 ` [PATCH 1/2] hw/acpi: arm: " Eric DeVolder
` (3 more replies)
0 siblings, 4 replies; 22+ messages in thread
From: Eric DeVolder @ 2023-03-28 15:59 UTC (permalink / raw)
To: shannon.zhaosl, mst, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo
Cc: boris.ostrovsky, eric.devolder
The following Linux kernel change broke CPU hotplug for MADT revision
less than 5.
commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
As part of the investigation into resolving this breakage, I learned
that i386 QEMU reports revision 1, while technically it is at revision 3.
(Arm QEMU reports revision 4, and that is valid/correct.)
ACPI 6.3 bumps MADT revision to 5 as it introduces an Online Capable
flag that the above Linux patch utilizes to denote hot pluggable CPUs.
So in order to bump MADT to the current revision of 5, need to
validate that all MADT table changes between 1 and 5 are present
in QEMU.
Below is a table summarizing the changes to the MADT. This information
gleamed from the ACPI specs on uefi.org.
ACPI MADT What
Version Version
1.0 MADT not present
2.0 1 Section 5.2.10.4
3.0 2 Section 5.2.11.4
5.2.11.13 Local SAPIC Structure added two new fields:
ACPI Processor UID Value
ACPI Processor UID String
5.2.10.14 Platform Interrupt Sources Structure:
Reserved changed to Platform Interrupt Sources Flags
3.0b 2 Section 5.2.11.4
Added a section describing guidelines for the ordering of
processors in the MADT to support proper boot processor
and multi-threaded logical processor operation.
4.0 3 Section 5.2.12
Adds Processor Local x2APIC structure type 9
Adds Local x2APIC NMI structure type 0xA
5.0 3 Section 5.2.12
6.0 3 Section 5.2.12
6.0a 4 Section 5.2.12
Adds ARM GIC structure types 0xB-0xF
6.2a 45 Section 5.2.12 <--- yep it says version 45!
6.2b 5 Section 5.2.12
GIC ITS last Reserved offset changed to 16 from 20 (typo)
6.3 5 Section 5.2.12
Adds Local APIC Flags Online Capable!
Adds GICC SPE Overflow Interrupt field
6.4 5 Section 5.2.12
Adds Multiprocessor Wakeup Structure type 0x10
(change notes says structure previously misplaced?)
6.5 5 Section 5.2.12
For the MADT revision change 1 -> 2, the spec has a change to the
SAPIC structure. In general, QEMU does not generate/support SAPIC.
So the QEMU i386 MADT revision can safely be moved to 2.
For the MADT revision change 2 -> 3, the spec adds Local x2APIC
structures. QEMU has long supported x2apic ACPI structures. A simple
search of x2apic within QEMU source and hw/i386/acpi-common.c
specifically reveals this. So the QEMU i386 MADT revision can safely
be moved to 3.
For the MADT revision change 3 -> 4, the spec adds support for the ARM
GIC structures. QEMU ARM does in fact generate and report revision 4.
As these will not be used by i386 QEMU, so then the QEMU i386 MADT
revision can safely be moved to 4 as well.
Now for the MADT revision change 4 -> 5, the spec adds the Online
Capable flag to the Local APIC structure, and the ARM GICC SPE
Overflow Interrupt field.
For the ARM SPE, an existing 3-byte Reserved field is broken into a 1-
byte Reserved field and a 2-byte SPE field. The spec says that is SPE
Overflow is not supported, it should be zero.
For the i386 Local APIC flag Online Capable, the spec has certain rules
about this value. And in particuar setting this value now explicitly
indicates a hotpluggable CPU.
So this patch makes the needed changes to move both ARM and i386 MADT
to revision 5. These are not complicated, thankfully.
Without these changes, the information below shows "how" CPU hotplug
breaks with the current upstream Linux kernel 6.3. For example, a Linux
guest started with:
qemu-system-x86_64 -smp 30,maxcpus=32 ...
and then attempting to hotplug a CPU:
(QEMU) device_add id=cpu30 driver=host-x86_64-cpu socket-id=0 core-id=30 thread-id=0
fails with the following:
APIC: NR_CPUS/possible_cpus limit of 30 reached. Processor 30/0x.
ACPI: Unable to map lapic to logical cpu number
acpi LNXCPU:1e: Enumeration failure
# dmesg | grep smpboot
smpboot: Allowing 30 CPUs, 0 hotplug CPUs
smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x)
smpboot: Max logical packages: 1
smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
# iasl -d /sys/firmware/tables/acpi/APIC
[000h 0000 4] Signature : "APIC" [Multiple APIC Descript
[004h 0004 4] Table Length : 00000170
[008h 0008 1] Revision : 01 <=====
[009h 0009 1] Checksum : 9C
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
...
[114h 0276 1] Subtable Type : 00 [Processor Local APIC]
[115h 0277 1] Length : 08
[116h 0278 1] Processor ID : 1D
[117h 0279 1] Local Apic ID : 1D
[118h 0280 4] Flags (decoded below) : 00000001
Processor Enabled : 1 <=====
[11Ch 0284 1] Subtable Type : 00 [Processor Local APIC]
[11Dh 0285 1] Length : 08
[11Eh 0286 1] Processor ID : 1E
[11Fh 0287 1] Local Apic ID : 1E
[120h 0288 4] Flags (decoded below) : 00000000
Processor Enabled : 0 <=====
[124h 0292 1] Subtable Type : 00 [Processor Local APIC]
[125h 0293 1] Length : 08
[126h 0294 1] Processor ID : 1F
[127h 0295 1] Local Apic ID : 1F
[128h 0296 4] Flags (decoded below) : 00000000
Processor Enabled : 0 <=====
The (latest upstream) Linux kernel sees 30 Enabled processors, and
does not consider processors 31 and 32 to be hotpluggable.
With this patch series applied, by bumping MADT to revision 5, the
latest upstream Linux kernel correctly identifies 30 CPUs plus 2
hotpluggable CPUS.
CPU30 has been hot-added
smpboot: Booting Node 0 Processor 30 APIC 0x1e
Will online and init hotplugged CPU: 30
# dmesg | grep smpboot
smpboot: Allowing 32 CPUs, 2 hotplug CPUs
smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x6, model: 0x56, stepping: 0x3)
smpboot: Max logical packages: 2
smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
# iasl -d /sys/firmware/tables/acpi/APIC
[000h 0000 004h] Signature : "APIC" [Multiple APIC Descript
[004h 0004 004h] Table Length : 00000170
[008h 0008 001h] Revision : 05 <=====
[009h 0009 001h] Checksum : 94
[00Ah 0010 006h] Oem ID : "BOCHS "
[010h 0016 008h] Oem Table ID : "BXPC "
[018h 0024 004h] Oem Revision : 00000001
[01Ch 0028 004h] Asl Compiler ID : "BXPC"
[020h 0032 004h] Asl Compiler Revision : 00000001
...
[114h 0276 001h] Subtable Type : 00 [Processor Local APIC]
[115h 0277 001h] Length : 08
[116h 0278 001h] Processor ID : 1D
[117h 0279 001h] Local Apic ID : 1D
[118h 0280 004h] Flags (decoded below) : 00000001
Processor Enabled : 1 <=====
Runtime Online Capable : 0 <=====
[11Ch 0284 001h] Subtable Type : 00 [Processor Local APIC]
[11Dh 0285 001h] Length : 08
[11Eh 0286 001h] Processor ID : 1E
[11Fh 0287 001h] Local Apic ID : 1E
[120h 0288 004h] Flags (decoded below) : 00000002
Processor Enabled : 0 <=====
Runtime Online Capable : 1 <=====
[124h 0292 001h] Subtable Type : 00 [Processor Local APIC]
[125h 0293 001h] Length : 08
[126h 0294 001h] Processor ID : 1F
[127h 0295 001h] Local Apic ID : 1F
[128h 0296 004h] Flags (decoded below) : 00000002
Processor Enabled : 0 <=====
Runtime Online Capable : 1 <=====
Regards,
Eric
Eric DeVolder (2):
hw/acpi: arm: bump MADT to revision 5
hw/acpi: i386: bump MADT to revision 5
hw/arm/virt-acpi-build.c | 6 ++++--
hw/i386/acpi-common.c | 13 ++++++++++---
2 files changed, 14 insertions(+), 5 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] hw/acpi: arm: bump MADT to revision 5
2023-03-28 15:59 [PATCH 0/2] hw/acpi: bump MADT to revision 5 Eric DeVolder
@ 2023-03-28 15:59 ` Eric DeVolder
2023-04-11 14:49 ` Igor Mammedov
2023-03-28 15:59 ` [PATCH 2/2] hw/acpi: i386: " Eric DeVolder
` (2 subsequent siblings)
3 siblings, 1 reply; 22+ messages in thread
From: Eric DeVolder @ 2023-03-28 15:59 UTC (permalink / raw)
To: shannon.zhaosl, mst, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo
Cc: boris.ostrovsky, eric.devolder
Currently ARM QEMU generates, and reports, MADT revision 4. ACPI 6.3
introduces MADT revision 5.
For MADT revision 5, the GICC structure adds an SPE Overflow Interrupt
field. This new 2-byte field is created from the existing 3-byte
Reserved field. The spec indicates if the SPE overflow interrupt is
not supported, to zero the field.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
hw/arm/virt-acpi-build.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4156111d49..23268dd981 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -705,7 +705,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
int i;
VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
const MemMapEntry *memmap = vms->memmap;
- AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
+ AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = vms->oem_id,
.oem_table_id = vms->oem_table_id };
acpi_table_begin(&table, table_data);
@@ -763,7 +763,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
/* Processor Power Efficiency Class */
build_append_int_noprefix(table_data, 0, 1);
/* Reserved */
- build_append_int_noprefix(table_data, 0, 3);
+ build_append_int_noprefix(table_data, 0, 1);
+ /* SPE overflow Interrupt */
+ build_append_int_noprefix(table_data, 0, 2);
}
if (vms->gic_version != VIRT_GIC_VERSION_2) {
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-03-28 15:59 [PATCH 0/2] hw/acpi: bump MADT to revision 5 Eric DeVolder
2023-03-28 15:59 ` [PATCH 1/2] hw/acpi: arm: " Eric DeVolder
@ 2023-03-28 15:59 ` Eric DeVolder
2023-03-29 5:03 ` Michael S. Tsirkin
2023-04-11 16:00 ` Igor Mammedov
2023-03-28 16:37 ` [PATCH 0/2] hw/acpi: " Eric DeVolder
2023-03-29 5:19 ` Michael S. Tsirkin
3 siblings, 2 replies; 22+ messages in thread
From: Eric DeVolder @ 2023-03-28 15:59 UTC (permalink / raw)
To: shannon.zhaosl, mst, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo
Cc: boris.ostrovsky, eric.devolder
Currently i386 QEMU generates MADT revision 3, and reports
MADT revision 1. ACPI 6.3 introduces MADT revision 5.
For MADT revision 4, that introduces ARM GIC structures, which do
not apply to i386.
For MADT revision 5, the Local APIC flags introduces the Online
Capable bitfield.
Making MADT generate and report revision 5 will solve problems with
CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
hw/i386/acpi-common.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
index 52e5c1439a..1e3a13a36c 100644
--- a/hw/i386/acpi-common.c
+++ b/hw/i386/acpi-common.c
@@ -38,8 +38,15 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
{
uint32_t apic_id = apic_ids->cpus[uid].arch_id;
/* Flags – Local APIC Flags */
- uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
- 1 /* Enabled */ : 0;
+ bool enabled = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
+ true /* Enabled */ : false;
+ /*
+ * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
+ * if Enabled is set.
+ */
+ bool onlinecapable = enabled ? false : true; /* Online Capable */
+ uint32_t flags = onlinecapable ? 0x2 : 0x0 |
+ enabled ? 0x1 : 0x0;
/* ACPI spec says that LAPIC entry for non present
* CPU may be omitted from MADT or it must be marked
@@ -102,7 +109,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
MachineClass *mc = MACHINE_GET_CLASS(x86ms);
const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
- AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
+ AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = oem_id,
.oem_table_id = oem_table_id };
acpi_table_begin(&table, table_data);
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-28 15:59 [PATCH 0/2] hw/acpi: bump MADT to revision 5 Eric DeVolder
2023-03-28 15:59 ` [PATCH 1/2] hw/acpi: arm: " Eric DeVolder
2023-03-28 15:59 ` [PATCH 2/2] hw/acpi: i386: " Eric DeVolder
@ 2023-03-28 16:37 ` Eric DeVolder
2023-03-29 5:19 ` Michael S. Tsirkin
3 siblings, 0 replies; 22+ messages in thread
From: Eric DeVolder @ 2023-03-28 16:37 UTC (permalink / raw)
To: shannon.zhaosl, mst, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo
Cc: boris.ostrovsky
I forgot to include the updated ACPI tables. I will do that as part of v2.
In the meantime, I appreciate any feedback...
eric
On 3/28/23 10:59, Eric DeVolder wrote:
> The following Linux kernel change broke CPU hotplug for MADT revision
> less than 5.
>
> commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
>
> As part of the investigation into resolving this breakage, I learned
> that i386 QEMU reports revision 1, while technically it is at revision 3.
> (Arm QEMU reports revision 4, and that is valid/correct.)
>
> ACPI 6.3 bumps MADT revision to 5 as it introduces an Online Capable
> flag that the above Linux patch utilizes to denote hot pluggable CPUs.
>
> So in order to bump MADT to the current revision of 5, need to
> validate that all MADT table changes between 1 and 5 are present
> in QEMU.
>
> Below is a table summarizing the changes to the MADT. This information
> gleamed from the ACPI specs on uefi.org.
>
> ACPI MADT What
> Version Version
> 1.0 MADT not present
> 2.0 1 Section 5.2.10.4
> 3.0 2 Section 5.2.11.4
> 5.2.11.13 Local SAPIC Structure added two new fields:
> ACPI Processor UID Value
> ACPI Processor UID String
> 5.2.10.14 Platform Interrupt Sources Structure:
> Reserved changed to Platform Interrupt Sources Flags
> 3.0b 2 Section 5.2.11.4
> Added a section describing guidelines for the ordering of
> processors in the MADT to support proper boot processor
> and multi-threaded logical processor operation.
> 4.0 3 Section 5.2.12
> Adds Processor Local x2APIC structure type 9
> Adds Local x2APIC NMI structure type 0xA
> 5.0 3 Section 5.2.12
> 6.0 3 Section 5.2.12
> 6.0a 4 Section 5.2.12
> Adds ARM GIC structure types 0xB-0xF
> 6.2a 45 Section 5.2.12 <--- yep it says version 45!
> 6.2b 5 Section 5.2.12
> GIC ITS last Reserved offset changed to 16 from 20 (typo)
> 6.3 5 Section 5.2.12
> Adds Local APIC Flags Online Capable!
> Adds GICC SPE Overflow Interrupt field
> 6.4 5 Section 5.2.12
> Adds Multiprocessor Wakeup Structure type 0x10
> (change notes says structure previously misplaced?)
> 6.5 5 Section 5.2.12
>
> For the MADT revision change 1 -> 2, the spec has a change to the
> SAPIC structure. In general, QEMU does not generate/support SAPIC.
> So the QEMU i386 MADT revision can safely be moved to 2.
>
> For the MADT revision change 2 -> 3, the spec adds Local x2APIC
> structures. QEMU has long supported x2apic ACPI structures. A simple
> search of x2apic within QEMU source and hw/i386/acpi-common.c
> specifically reveals this. So the QEMU i386 MADT revision can safely
> be moved to 3.
>
> For the MADT revision change 3 -> 4, the spec adds support for the ARM
> GIC structures. QEMU ARM does in fact generate and report revision 4.
> As these will not be used by i386 QEMU, so then the QEMU i386 MADT
> revision can safely be moved to 4 as well.
>
> Now for the MADT revision change 4 -> 5, the spec adds the Online
> Capable flag to the Local APIC structure, and the ARM GICC SPE
> Overflow Interrupt field.
>
> For the ARM SPE, an existing 3-byte Reserved field is broken into a 1-
> byte Reserved field and a 2-byte SPE field. The spec says that is SPE
> Overflow is not supported, it should be zero.
>
> For the i386 Local APIC flag Online Capable, the spec has certain rules
> about this value. And in particuar setting this value now explicitly
> indicates a hotpluggable CPU.
>
> So this patch makes the needed changes to move both ARM and i386 MADT
> to revision 5. These are not complicated, thankfully.
>
> Without these changes, the information below shows "how" CPU hotplug
> breaks with the current upstream Linux kernel 6.3. For example, a Linux
> guest started with:
>
> qemu-system-x86_64 -smp 30,maxcpus=32 ...
>
> and then attempting to hotplug a CPU:
>
> (QEMU) device_add id=cpu30 driver=host-x86_64-cpu socket-id=0 core-id=30 thread-id=0
>
> fails with the following:
>
> APIC: NR_CPUS/possible_cpus limit of 30 reached. Processor 30/0x.
> ACPI: Unable to map lapic to logical cpu number
> acpi LNXCPU:1e: Enumeration failure
>
> # dmesg | grep smpboot
> smpboot: Allowing 30 CPUs, 0 hotplug CPUs
> smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x)
> smpboot: Max logical packages: 1
> smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
>
> # iasl -d /sys/firmware/tables/acpi/APIC
> [000h 0000 4] Signature : "APIC" [Multiple APIC Descript
> [004h 0004 4] Table Length : 00000170
> [008h 0008 1] Revision : 01 <=====
> [009h 0009 1] Checksum : 9C
> [00Ah 0010 6] Oem ID : "BOCHS "
> [010h 0016 8] Oem Table ID : "BXPC "
> [018h 0024 4] Oem Revision : 00000001
> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> [020h 0032 4] Asl Compiler Revision : 00000001
>
> ...
>
> [114h 0276 1] Subtable Type : 00 [Processor Local APIC]
> [115h 0277 1] Length : 08
> [116h 0278 1] Processor ID : 1D
> [117h 0279 1] Local Apic ID : 1D
> [118h 0280 4] Flags (decoded below) : 00000001
> Processor Enabled : 1 <=====
>
> [11Ch 0284 1] Subtable Type : 00 [Processor Local APIC]
> [11Dh 0285 1] Length : 08
> [11Eh 0286 1] Processor ID : 1E
> [11Fh 0287 1] Local Apic ID : 1E
> [120h 0288 4] Flags (decoded below) : 00000000
> Processor Enabled : 0 <=====
>
> [124h 0292 1] Subtable Type : 00 [Processor Local APIC]
> [125h 0293 1] Length : 08
> [126h 0294 1] Processor ID : 1F
> [127h 0295 1] Local Apic ID : 1F
> [128h 0296 4] Flags (decoded below) : 00000000
> Processor Enabled : 0 <=====
>
> The (latest upstream) Linux kernel sees 30 Enabled processors, and
> does not consider processors 31 and 32 to be hotpluggable.
>
> With this patch series applied, by bumping MADT to revision 5, the
> latest upstream Linux kernel correctly identifies 30 CPUs plus 2
> hotpluggable CPUS.
>
> CPU30 has been hot-added
> smpboot: Booting Node 0 Processor 30 APIC 0x1e
> Will online and init hotplugged CPU: 30
>
> # dmesg | grep smpboot
> smpboot: Allowing 32 CPUs, 2 hotplug CPUs
> smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x6, model: 0x56, stepping: 0x3)
> smpboot: Max logical packages: 2
> smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
>
> # iasl -d /sys/firmware/tables/acpi/APIC
> [000h 0000 004h] Signature : "APIC" [Multiple APIC Descript
> [004h 0004 004h] Table Length : 00000170
> [008h 0008 001h] Revision : 05 <=====
> [009h 0009 001h] Checksum : 94
> [00Ah 0010 006h] Oem ID : "BOCHS "
> [010h 0016 008h] Oem Table ID : "BXPC "
> [018h 0024 004h] Oem Revision : 00000001
> [01Ch 0028 004h] Asl Compiler ID : "BXPC"
> [020h 0032 004h] Asl Compiler Revision : 00000001
>
> ...
>
> [114h 0276 001h] Subtable Type : 00 [Processor Local APIC]
> [115h 0277 001h] Length : 08
> [116h 0278 001h] Processor ID : 1D
> [117h 0279 001h] Local Apic ID : 1D
> [118h 0280 004h] Flags (decoded below) : 00000001
> Processor Enabled : 1 <=====
> Runtime Online Capable : 0 <=====
>
> [11Ch 0284 001h] Subtable Type : 00 [Processor Local APIC]
> [11Dh 0285 001h] Length : 08
> [11Eh 0286 001h] Processor ID : 1E
> [11Fh 0287 001h] Local Apic ID : 1E
> [120h 0288 004h] Flags (decoded below) : 00000002
> Processor Enabled : 0 <=====
> Runtime Online Capable : 1 <=====
>
> [124h 0292 001h] Subtable Type : 00 [Processor Local APIC]
> [125h 0293 001h] Length : 08
> [126h 0294 001h] Processor ID : 1F
> [127h 0295 001h] Local Apic ID : 1F
> [128h 0296 004h] Flags (decoded below) : 00000002
> Processor Enabled : 0 <=====
> Runtime Online Capable : 1 <=====
>
> Regards,
> Eric
>
>
> Eric DeVolder (2):
> hw/acpi: arm: bump MADT to revision 5
> hw/acpi: i386: bump MADT to revision 5
>
> hw/arm/virt-acpi-build.c | 6 ++++--
> hw/i386/acpi-common.c | 13 ++++++++++---
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-03-28 15:59 ` [PATCH 2/2] hw/acpi: i386: " Eric DeVolder
@ 2023-03-29 5:03 ` Michael S. Tsirkin
2023-03-29 13:16 ` Eric DeVolder
2023-04-11 16:00 ` Igor Mammedov
1 sibling, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2023-03-29 5:03 UTC (permalink / raw)
To: Eric DeVolder
Cc: shannon.zhaosl, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On Tue, Mar 28, 2023 at 11:59:26AM -0400, Eric DeVolder wrote:
> Currently i386 QEMU generates MADT revision 3, and reports
> MADT revision 1. ACPI 6.3 introduces MADT revision 5.
>
> For MADT revision 4, that introduces ARM GIC structures, which do
> not apply to i386.
>
> For MADT revision 5, the Local APIC flags introduces the Online
> Capable bitfield.
>
> Making MADT generate and report revision 5 will solve problems with
> CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
I am looking for ways to reduce risk of breakage with this.
We don't currently have a reason to change it if cpu
hotplug is off, do we? Maybe make it conditional on that.
> ---
> hw/i386/acpi-common.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> index 52e5c1439a..1e3a13a36c 100644
> --- a/hw/i386/acpi-common.c
> +++ b/hw/i386/acpi-common.c
> @@ -38,8 +38,15 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
> {
> uint32_t apic_id = apic_ids->cpus[uid].arch_id;
> /* Flags – Local APIC Flags */
> - uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> - 1 /* Enabled */ : 0;
> + bool enabled = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> + true /* Enabled */ : false;
> + /*
> + * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
> + * if Enabled is set.
> + */
> + bool onlinecapable = enabled ? false : true; /* Online Capable */
> + uint32_t flags = onlinecapable ? 0x2 : 0x0 |
> + enabled ? 0x1 : 0x0;
>
> /* ACPI spec says that LAPIC entry for non present
> * CPU may be omitted from MADT or it must be marked
> @@ -102,7 +109,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> MachineClass *mc = MACHINE_GET_CLASS(x86ms);
> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
> AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
> - AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
> + AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = oem_id,
> .oem_table_id = oem_table_id };
>
> acpi_table_begin(&table, table_data);
> --
> 2.31.1
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-28 15:59 [PATCH 0/2] hw/acpi: bump MADT to revision 5 Eric DeVolder
` (2 preceding siblings ...)
2023-03-28 16:37 ` [PATCH 0/2] hw/acpi: " Eric DeVolder
@ 2023-03-29 5:19 ` Michael S. Tsirkin
2023-03-29 13:14 ` Eric DeVolder
3 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2023-03-29 5:19 UTC (permalink / raw)
To: Eric DeVolder
Cc: shannon.zhaosl, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
Hmm I don't think we can reasonably make such a change for 8.0.
Seems too risky.
Also, I feel we want to have an internal (with "x-" prefix") flag to
revert to old behaviour, in case of breakage on some guests. and maybe
we want to keep old revision for old machine types.
On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
> The following Linux kernel change broke CPU hotplug for MADT revision
> less than 5.
>
> commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
Presumably it's being fixed? Link to discussion? Patch fixing that in
Linux?
> As part of the investigation into resolving this breakage, I learned
> that i386 QEMU reports revision 1, while technically it is at revision 3.
> (Arm QEMU reports revision 4, and that is valid/correct.)
>
> ACPI 6.3 bumps MADT revision to 5 as it introduces an Online Capable
> flag that the above Linux patch utilizes to denote hot pluggable CPUs.
>
> So in order to bump MADT to the current revision of 5, need to
> validate that all MADT table changes between 1 and 5 are present
> in QEMU.
>
> Below is a table summarizing the changes to the MADT. This information
> gleamed from the ACPI specs on uefi.org.
>
> ACPI MADT What
> Version Version
> 1.0 MADT not present
> 2.0 1 Section 5.2.10.4
> 3.0 2 Section 5.2.11.4
> 5.2.11.13 Local SAPIC Structure added two new fields:
> ACPI Processor UID Value
> ACPI Processor UID String
> 5.2.10.14 Platform Interrupt Sources Structure:
> Reserved changed to Platform Interrupt Sources Flags
> 3.0b 2 Section 5.2.11.4
> Added a section describing guidelines for the ordering of
> processors in the MADT to support proper boot processor
> and multi-threaded logical processor operation.
> 4.0 3 Section 5.2.12
> Adds Processor Local x2APIC structure type 9
> Adds Local x2APIC NMI structure type 0xA
> 5.0 3 Section 5.2.12
> 6.0 3 Section 5.2.12
> 6.0a 4 Section 5.2.12
> Adds ARM GIC structure types 0xB-0xF
> 6.2a 45 Section 5.2.12 <--- yep it says version 45!
> 6.2b 5 Section 5.2.12
> GIC ITS last Reserved offset changed to 16 from 20 (typo)
> 6.3 5 Section 5.2.12
> Adds Local APIC Flags Online Capable!
> Adds GICC SPE Overflow Interrupt field
> 6.4 5 Section 5.2.12
> Adds Multiprocessor Wakeup Structure type 0x10
> (change notes says structure previously misplaced?)
> 6.5 5 Section 5.2.12
>
> For the MADT revision change 1 -> 2, the spec has a change to the
> SAPIC structure. In general, QEMU does not generate/support SAPIC.
> So the QEMU i386 MADT revision can safely be moved to 2.
>
> For the MADT revision change 2 -> 3, the spec adds Local x2APIC
> structures. QEMU has long supported x2apic ACPI structures. A simple
> search of x2apic within QEMU source and hw/i386/acpi-common.c
> specifically reveals this.
But not unconditionally.
> So the QEMU i386 MADT revision can safely
> be moved to 3.
>
> For the MADT revision change 3 -> 4, the spec adds support for the ARM
> GIC structures. QEMU ARM does in fact generate and report revision 4.
> As these will not be used by i386 QEMU, so then the QEMU i386 MADT
> revision can safely be moved to 4 as well.
>
> Now for the MADT revision change 4 -> 5, the spec adds the Online
> Capable flag to the Local APIC structure, and the ARM GICC SPE
> Overflow Interrupt field.
>
> For the ARM SPE, an existing 3-byte Reserved field is broken into a 1-
> byte Reserved field and a 2-byte SPE field. The spec says that is SPE
> Overflow is not supported, it should be zero.
>
> For the i386 Local APIC flag Online Capable, the spec has certain rules
> about this value. And in particuar setting this value now explicitly
> indicates a hotpluggable CPU.
>
> So this patch makes the needed changes to move both ARM and i386 MADT
> to revision 5. These are not complicated, thankfully.
>
> Without these changes, the information below shows "how" CPU hotplug
> breaks with the current upstream Linux kernel 6.3. For example, a Linux
> guest started with:
>
> qemu-system-x86_64 -smp 30,maxcpus=32 ...
>
> and then attempting to hotplug a CPU:
>
> (QEMU) device_add id=cpu30 driver=host-x86_64-cpu socket-id=0 core-id=30 thread-id=0
>
> fails with the following:
>
> APIC: NR_CPUS/possible_cpus limit of 30 reached. Processor 30/0x.
> ACPI: Unable to map lapic to logical cpu number
> acpi LNXCPU:1e: Enumeration failure
>
> # dmesg | grep smpboot
> smpboot: Allowing 30 CPUs, 0 hotplug CPUs
> smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x)
> smpboot: Max logical packages: 1
> smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
>
> # iasl -d /sys/firmware/tables/acpi/APIC
> [000h 0000 4] Signature : "APIC" [Multiple APIC Descript
> [004h 0004 4] Table Length : 00000170
> [008h 0008 1] Revision : 01 <=====
> [009h 0009 1] Checksum : 9C
> [00Ah 0010 6] Oem ID : "BOCHS "
> [010h 0016 8] Oem Table ID : "BXPC "
> [018h 0024 4] Oem Revision : 00000001
> [01Ch 0028 4] Asl Compiler ID : "BXPC"
> [020h 0032 4] Asl Compiler Revision : 00000001
>
> ...
>
> [114h 0276 1] Subtable Type : 00 [Processor Local APIC]
> [115h 0277 1] Length : 08
> [116h 0278 1] Processor ID : 1D
> [117h 0279 1] Local Apic ID : 1D
> [118h 0280 4] Flags (decoded below) : 00000001
> Processor Enabled : 1 <=====
>
> [11Ch 0284 1] Subtable Type : 00 [Processor Local APIC]
> [11Dh 0285 1] Length : 08
> [11Eh 0286 1] Processor ID : 1E
> [11Fh 0287 1] Local Apic ID : 1E
> [120h 0288 4] Flags (decoded below) : 00000000
> Processor Enabled : 0 <=====
>
> [124h 0292 1] Subtable Type : 00 [Processor Local APIC]
> [125h 0293 1] Length : 08
> [126h 0294 1] Processor ID : 1F
> [127h 0295 1] Local Apic ID : 1F
> [128h 0296 4] Flags (decoded below) : 00000000
> Processor Enabled : 0 <=====
>
> The (latest upstream) Linux kernel sees 30 Enabled processors, and
> does not consider processors 31 and 32 to be hotpluggable.
>
> With this patch series applied, by bumping MADT to revision 5, the
> latest upstream Linux kernel correctly identifies 30 CPUs plus 2
> hotpluggable CPUS.
>
> CPU30 has been hot-added
> smpboot: Booting Node 0 Processor 30 APIC 0x1e
> Will online and init hotplugged CPU: 30
>
> # dmesg | grep smpboot
> smpboot: Allowing 32 CPUs, 2 hotplug CPUs
> smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x6, model: 0x56, stepping: 0x3)
> smpboot: Max logical packages: 2
> smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
>
> # iasl -d /sys/firmware/tables/acpi/APIC
> [000h 0000 004h] Signature : "APIC" [Multiple APIC Descript
> [004h 0004 004h] Table Length : 00000170
> [008h 0008 001h] Revision : 05 <=====
> [009h 0009 001h] Checksum : 94
> [00Ah 0010 006h] Oem ID : "BOCHS "
> [010h 0016 008h] Oem Table ID : "BXPC "
> [018h 0024 004h] Oem Revision : 00000001
> [01Ch 0028 004h] Asl Compiler ID : "BXPC"
> [020h 0032 004h] Asl Compiler Revision : 00000001
>
> ...
>
> [114h 0276 001h] Subtable Type : 00 [Processor Local APIC]
> [115h 0277 001h] Length : 08
> [116h 0278 001h] Processor ID : 1D
> [117h 0279 001h] Local Apic ID : 1D
> [118h 0280 004h] Flags (decoded below) : 00000001
> Processor Enabled : 1 <=====
> Runtime Online Capable : 0 <=====
>
> [11Ch 0284 001h] Subtable Type : 00 [Processor Local APIC]
> [11Dh 0285 001h] Length : 08
> [11Eh 0286 001h] Processor ID : 1E
> [11Fh 0287 001h] Local Apic ID : 1E
> [120h 0288 004h] Flags (decoded below) : 00000002
> Processor Enabled : 0 <=====
> Runtime Online Capable : 1 <=====
>
> [124h 0292 001h] Subtable Type : 00 [Processor Local APIC]
> [125h 0293 001h] Length : 08
> [126h 0294 001h] Processor ID : 1F
> [127h 0295 001h] Local Apic ID : 1F
> [128h 0296 004h] Flags (decoded below) : 00000002
> Processor Enabled : 0 <=====
> Runtime Online Capable : 1 <=====
>
> Regards,
> Eric
Can you please report which guests were tested?
>
> Eric DeVolder (2):
> hw/acpi: arm: bump MADT to revision 5
> hw/acpi: i386: bump MADT to revision 5
>
> hw/arm/virt-acpi-build.c | 6 ++++--
> hw/i386/acpi-common.c | 13 ++++++++++---
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> --
> 2.31.1
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-29 5:19 ` Michael S. Tsirkin
@ 2023-03-29 13:14 ` Eric DeVolder
2023-03-29 16:47 ` Michael S. Tsirkin
0 siblings, 1 reply; 22+ messages in thread
From: Eric DeVolder @ 2023-03-29 13:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: shannon.zhaosl, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On 3/29/23 00:19, Michael S. Tsirkin wrote:
> Hmm I don't think we can reasonably make such a change for 8.0.
> Seems too risky.
> Also, I feel we want to have an internal (with "x-" prefix") flag to
> revert to old behaviour, in case of breakage on some guests. and maybe
> we want to keep old revision for old machine types.
Ok, what option name, for keeping old behavior, would you like?
>
>
> On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
>> The following Linux kernel change broke CPU hotplug for MADT revision
>> less than 5.
>>
>> commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
>
> Presumably it's being fixed? Link to discussion? Patch fixing that in
> Linux?
https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
>
>
>> As part of the investigation into resolving this breakage, I learned
>> that i386 QEMU reports revision 1, while technically it is at revision 3.
>> (Arm QEMU reports revision 4, and that is valid/correct.)
>>
>> ACPI 6.3 bumps MADT revision to 5 as it introduces an Online Capable
>> flag that the above Linux patch utilizes to denote hot pluggable CPUs.
>>
>> So in order to bump MADT to the current revision of 5, need to
>> validate that all MADT table changes between 1 and 5 are present
>> in QEMU.
>>
>> Below is a table summarizing the changes to the MADT. This information
>> gleamed from the ACPI specs on uefi.org.
>>
>> ACPI MADT What
>> Version Version
>> 1.0 MADT not present
>> 2.0 1 Section 5.2.10.4
>> 3.0 2 Section 5.2.11.4
>> 5.2.11.13 Local SAPIC Structure added two new fields:
>> ACPI Processor UID Value
>> ACPI Processor UID String
>> 5.2.10.14 Platform Interrupt Sources Structure:
>> Reserved changed to Platform Interrupt Sources Flags
>> 3.0b 2 Section 5.2.11.4
>> Added a section describing guidelines for the ordering of
>> processors in the MADT to support proper boot processor
>> and multi-threaded logical processor operation.
>> 4.0 3 Section 5.2.12
>> Adds Processor Local x2APIC structure type 9
>> Adds Local x2APIC NMI structure type 0xA
>> 5.0 3 Section 5.2.12
>> 6.0 3 Section 5.2.12
>> 6.0a 4 Section 5.2.12
>> Adds ARM GIC structure types 0xB-0xF
>> 6.2a 45 Section 5.2.12 <--- yep it says version 45!
>> 6.2b 5 Section 5.2.12
>> GIC ITS last Reserved offset changed to 16 from 20 (typo)
>> 6.3 5 Section 5.2.12
>> Adds Local APIC Flags Online Capable!
>> Adds GICC SPE Overflow Interrupt field
>> 6.4 5 Section 5.2.12
>> Adds Multiprocessor Wakeup Structure type 0x10
>> (change notes says structure previously misplaced?)
>> 6.5 5 Section 5.2.12
>>
>> For the MADT revision change 1 -> 2, the spec has a change to the
>> SAPIC structure. In general, QEMU does not generate/support SAPIC.
>> So the QEMU i386 MADT revision can safely be moved to 2.
>>
>> For the MADT revision change 2 -> 3, the spec adds Local x2APIC
>> structures. QEMU has long supported x2apic ACPI structures. A simple
>> search of x2apic within QEMU source and hw/i386/acpi-common.c
>> specifically reveals this.
>
> But not unconditionally.
I don't think that reporting revision 3 requires that generation of x2apic; one could still see
apic, x2apic, or sapic in theory. I realize qemu doesn't do sapic...
>
>> So the QEMU i386 MADT revision can safely
>> be moved to 3.
>>
>> For the MADT revision change 3 -> 4, the spec adds support for the ARM
>> GIC structures. QEMU ARM does in fact generate and report revision 4.
>> As these will not be used by i386 QEMU, so then the QEMU i386 MADT
>> revision can safely be moved to 4 as well.
>>
>> Now for the MADT revision change 4 -> 5, the spec adds the Online
>> Capable flag to the Local APIC structure, and the ARM GICC SPE
>> Overflow Interrupt field.
>>
>> For the ARM SPE, an existing 3-byte Reserved field is broken into a 1-
>> byte Reserved field and a 2-byte SPE field. The spec says that is SPE
>> Overflow is not supported, it should be zero.
>>
>> For the i386 Local APIC flag Online Capable, the spec has certain rules
>> about this value. And in particuar setting this value now explicitly
>> indicates a hotpluggable CPU.
>>
>> So this patch makes the needed changes to move both ARM and i386 MADT
>> to revision 5. These are not complicated, thankfully.
>>
>> Without these changes, the information below shows "how" CPU hotplug
>> breaks with the current upstream Linux kernel 6.3. For example, a Linux
>> guest started with:
>>
>> qemu-system-x86_64 -smp 30,maxcpus=32 ...
>>
>> and then attempting to hotplug a CPU:
>>
>> (QEMU) device_add id=cpu30 driver=host-x86_64-cpu socket-id=0 core-id=30 thread-id=0
>>
>> fails with the following:
>>
>> APIC: NR_CPUS/possible_cpus limit of 30 reached. Processor 30/0x.
>> ACPI: Unable to map lapic to logical cpu number
>> acpi LNXCPU:1e: Enumeration failure
>>
>> # dmesg | grep smpboot
>> smpboot: Allowing 30 CPUs, 0 hotplug CPUs
>> smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x)
>> smpboot: Max logical packages: 1
>> smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
>>
>> # iasl -d /sys/firmware/tables/acpi/APIC
>> [000h 0000 4] Signature : "APIC" [Multiple APIC Descript
>> [004h 0004 4] Table Length : 00000170
>> [008h 0008 1] Revision : 01 <=====
>> [009h 0009 1] Checksum : 9C
>> [00Ah 0010 6] Oem ID : "BOCHS "
>> [010h 0016 8] Oem Table ID : "BXPC "
>> [018h 0024 4] Oem Revision : 00000001
>> [01Ch 0028 4] Asl Compiler ID : "BXPC"
>> [020h 0032 4] Asl Compiler Revision : 00000001
>>
>> ...
>>
>> [114h 0276 1] Subtable Type : 00 [Processor Local APIC]
>> [115h 0277 1] Length : 08
>> [116h 0278 1] Processor ID : 1D
>> [117h 0279 1] Local Apic ID : 1D
>> [118h 0280 4] Flags (decoded below) : 00000001
>> Processor Enabled : 1 <=====
>>
>> [11Ch 0284 1] Subtable Type : 00 [Processor Local APIC]
>> [11Dh 0285 1] Length : 08
>> [11Eh 0286 1] Processor ID : 1E
>> [11Fh 0287 1] Local Apic ID : 1E
>> [120h 0288 4] Flags (decoded below) : 00000000
>> Processor Enabled : 0 <=====
>>
>> [124h 0292 1] Subtable Type : 00 [Processor Local APIC]
>> [125h 0293 1] Length : 08
>> [126h 0294 1] Processor ID : 1F
>> [127h 0295 1] Local Apic ID : 1F
>> [128h 0296 4] Flags (decoded below) : 00000000
>> Processor Enabled : 0 <=====
>>
>> The (latest upstream) Linux kernel sees 30 Enabled processors, and
>> does not consider processors 31 and 32 to be hotpluggable.
>>
>> With this patch series applied, by bumping MADT to revision 5, the
>> latest upstream Linux kernel correctly identifies 30 CPUs plus 2
>> hotpluggable CPUS.
>>
>> CPU30 has been hot-added
>> smpboot: Booting Node 0 Processor 30 APIC 0x1e
>> Will online and init hotplugged CPU: 30
>>
>> # dmesg | grep smpboot
>> smpboot: Allowing 32 CPUs, 2 hotplug CPUs
>> smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x6, model: 0x56, stepping: 0x3)
>> smpboot: Max logical packages: 2
>> smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
>>
>> # iasl -d /sys/firmware/tables/acpi/APIC
>> [000h 0000 004h] Signature : "APIC" [Multiple APIC Descript
>> [004h 0004 004h] Table Length : 00000170
>> [008h 0008 001h] Revision : 05 <=====
>> [009h 0009 001h] Checksum : 94
>> [00Ah 0010 006h] Oem ID : "BOCHS "
>> [010h 0016 008h] Oem Table ID : "BXPC "
>> [018h 0024 004h] Oem Revision : 00000001
>> [01Ch 0028 004h] Asl Compiler ID : "BXPC"
>> [020h 0032 004h] Asl Compiler Revision : 00000001
>>
>> ...
>>
>> [114h 0276 001h] Subtable Type : 00 [Processor Local APIC]
>> [115h 0277 001h] Length : 08
>> [116h 0278 001h] Processor ID : 1D
>> [117h 0279 001h] Local Apic ID : 1D
>> [118h 0280 004h] Flags (decoded below) : 00000001
>> Processor Enabled : 1 <=====
>> Runtime Online Capable : 0 <=====
>>
>> [11Ch 0284 001h] Subtable Type : 00 [Processor Local APIC]
>> [11Dh 0285 001h] Length : 08
>> [11Eh 0286 001h] Processor ID : 1E
>> [11Fh 0287 001h] Local Apic ID : 1E
>> [120h 0288 004h] Flags (decoded below) : 00000002
>> Processor Enabled : 0 <=====
>> Runtime Online Capable : 1 <=====
>>
>> [124h 0292 001h] Subtable Type : 00 [Processor Local APIC]
>> [125h 0293 001h] Length : 08
>> [126h 0294 001h] Processor ID : 1F
>> [127h 0295 001h] Local Apic ID : 1F
>> [128h 0296 004h] Flags (decoded below) : 00000002
>> Processor Enabled : 0 <=====
>> Runtime Online Capable : 1 <=====
>>
>> Regards,
>> Eric
>
> Can you please report which guests were tested?
I've been using primarily upstream Linux. Kernels at and before 6.2.0 didn't have the "broken" patch
mentioned above, and worked (for the reasons cited in the patch discussion to "fix" that patch). Any
kernel since has the "broken" patch and will exhibit the issue.
I've been using q35.
If there are other samples you'd like to see, let me know and I'll try.
Also, my responses will be delayed as I'm traveling the remainder of the week.
Thanks!
eric
>
>
>>
>> Eric DeVolder (2):
>> hw/acpi: arm: bump MADT to revision 5
>> hw/acpi: i386: bump MADT to revision 5
>>
>> hw/arm/virt-acpi-build.c | 6 ++++--
>> hw/i386/acpi-common.c | 13 ++++++++++---
>> 2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> --
>> 2.31.1
>>
>>
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-03-29 5:03 ` Michael S. Tsirkin
@ 2023-03-29 13:16 ` Eric DeVolder
2023-03-29 13:19 ` Eric DeVolder
2023-03-31 16:29 ` Igor Mammedov
0 siblings, 2 replies; 22+ messages in thread
From: Eric DeVolder @ 2023-03-29 13:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: shannon.zhaosl, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On 3/29/23 00:03, Michael S. Tsirkin wrote:
> On Tue, Mar 28, 2023 at 11:59:26AM -0400, Eric DeVolder wrote:
>> Currently i386 QEMU generates MADT revision 3, and reports
>> MADT revision 1. ACPI 6.3 introduces MADT revision 5.
>>
>> For MADT revision 4, that introduces ARM GIC structures, which do
>> not apply to i386.
>>
>> For MADT revision 5, the Local APIC flags introduces the Online
>> Capable bitfield.
>>
>> Making MADT generate and report revision 5 will solve problems with
>> CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>
> I am looking for ways to reduce risk of breakage with this.
> We don't currently have a reason to change it if cpu
> hotplug is off, do we? Maybe make it conditional on that.
By "cpu hotplug off", do you mean, for example, no maxcpus= option?
In other words, how should I detect "cpu hotplug off"?
eric
>
>
>
>
>
>> ---
>> hw/i386/acpi-common.c | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
>> index 52e5c1439a..1e3a13a36c 100644
>> --- a/hw/i386/acpi-common.c
>> +++ b/hw/i386/acpi-common.c
>> @@ -38,8 +38,15 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
>> {
>> uint32_t apic_id = apic_ids->cpus[uid].arch_id;
>> /* Flags – Local APIC Flags */
>> - uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
>> - 1 /* Enabled */ : 0;
>> + bool enabled = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
>> + true /* Enabled */ : false;
>> + /*
>> + * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
>> + * if Enabled is set.
>> + */
>> + bool onlinecapable = enabled ? false : true; /* Online Capable */
>> + uint32_t flags = onlinecapable ? 0x2 : 0x0 |
>> + enabled ? 0x1 : 0x0;
>>
>> /* ACPI spec says that LAPIC entry for non present
>> * CPU may be omitted from MADT or it must be marked
>> @@ -102,7 +109,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
>> MachineClass *mc = MACHINE_GET_CLASS(x86ms);
>> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
>> AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
>> - AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
>> + AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = oem_id,
>> .oem_table_id = oem_table_id };
>>
>> acpi_table_begin(&table, table_data);
>> --
>> 2.31.1
>>
>>
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-03-29 13:16 ` Eric DeVolder
@ 2023-03-29 13:19 ` Eric DeVolder
2023-03-29 16:55 ` Michael S. Tsirkin
2023-03-31 16:29 ` Igor Mammedov
1 sibling, 1 reply; 22+ messages in thread
From: Eric DeVolder @ 2023-03-29 13:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: shannon.zhaosl, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On 3/29/23 08:16, Eric DeVolder wrote:
>
>
> On 3/29/23 00:03, Michael S. Tsirkin wrote:
>> On Tue, Mar 28, 2023 at 11:59:26AM -0400, Eric DeVolder wrote:
>>> Currently i386 QEMU generates MADT revision 3, and reports
>>> MADT revision 1. ACPI 6.3 introduces MADT revision 5.
>>>
>>> For MADT revision 4, that introduces ARM GIC structures, which do
>>> not apply to i386.
>>>
>>> For MADT revision 5, the Local APIC flags introduces the Online
>>> Capable bitfield.
>>>
>>> Making MADT generate and report revision 5 will solve problems with
>>> CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
>>>
>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>>
>> I am looking for ways to reduce risk of breakage with this.
>> We don't currently have a reason to change it if cpu
>> hotplug is off, do we? Maybe make it conditional on that.
>
> By "cpu hotplug off", do you mean, for example, no maxcpus= option?
> In other words, how should I detect "cpu hotplug off"?
> eric
>
Actually, if, for example, one had -smp 30,maxcpus=32, then there would be two hotpluggable cpus
reported, the last two with the Enabled=0 and Online Capable=1. If one had -smp 32 (ie "cpu hotplug
off"), then all cpus would be reported as Enabled and no cpu would have its Online Capable flag set.
Granted in both cases, MADT.revision would report 5, but it would still be accurate.
eric
>>
>>
>>
>>
>>
>>> ---
>>> hw/i386/acpi-common.c | 13 ++++++++++---
>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
>>> index 52e5c1439a..1e3a13a36c 100644
>>> --- a/hw/i386/acpi-common.c
>>> +++ b/hw/i386/acpi-common.c
>>> @@ -38,8 +38,15 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
>>> {
>>> uint32_t apic_id = apic_ids->cpus[uid].arch_id;
>>> /* Flags – Local APIC Flags */
>>> - uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
>>> - 1 /* Enabled */ : 0;
>>> + bool enabled = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
>>> + true /* Enabled */ : false;
>>> + /*
>>> + * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
>>> + * if Enabled is set.
>>> + */
>>> + bool onlinecapable = enabled ? false : true; /* Online Capable */
>>> + uint32_t flags = onlinecapable ? 0x2 : 0x0 |
>>> + enabled ? 0x1 : 0x0;
>>> /* ACPI spec says that LAPIC entry for non present
>>> * CPU may be omitted from MADT or it must be marked
>>> @@ -102,7 +109,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
>>> MachineClass *mc = MACHINE_GET_CLASS(x86ms);
>>> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
>>> AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
>>> - AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
>>> + AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = oem_id,
>>> .oem_table_id = oem_table_id };
>>> acpi_table_begin(&table, table_data);
>>> --
>>> 2.31.1
>>>
>>>
>>>
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-29 13:14 ` Eric DeVolder
@ 2023-03-29 16:47 ` Michael S. Tsirkin
2023-03-30 7:36 ` Ani Sinha
2023-03-31 16:25 ` Igor Mammedov
0 siblings, 2 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2023-03-29 16:47 UTC (permalink / raw)
To: Eric DeVolder
Cc: shannon.zhaosl, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On Wed, Mar 29, 2023 at 08:14:37AM -0500, Eric DeVolder wrote:
>
>
> On 3/29/23 00:19, Michael S. Tsirkin wrote:
> > Hmm I don't think we can reasonably make such a change for 8.0.
> > Seems too risky.
> > Also, I feel we want to have an internal (with "x-" prefix") flag to
> > revert to old behaviour, in case of breakage on some guests. and maybe
> > we want to keep old revision for old machine types.
> Ok, what option name, for keeping old behavior, would you like?
Don't much care. x-madt-rev?
> >
> >
> > On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
> > > The following Linux kernel change broke CPU hotplug for MADT revision
> > > less than 5.
> > >
> > > commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
> >
> > Presumably it's being fixed? Link to discussion? Patch fixing that in
> > Linux?
>
> https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
Great! Maybe stick a Link: tag in the commit log.
> >
> >
> > > As part of the investigation into resolving this breakage, I learned
> > > that i386 QEMU reports revision 1, while technically it is at revision 3.
> > > (Arm QEMU reports revision 4, and that is valid/correct.)
> > >
> > > ACPI 6.3 bumps MADT revision to 5 as it introduces an Online Capable
> > > flag that the above Linux patch utilizes to denote hot pluggable CPUs.
> > >
> > > So in order to bump MADT to the current revision of 5, need to
> > > validate that all MADT table changes between 1 and 5 are present
> > > in QEMU.
> > >
> > > Below is a table summarizing the changes to the MADT. This information
> > > gleamed from the ACPI specs on uefi.org.
> > >
> > > ACPI MADT What
> > > Version Version
> > > 1.0 MADT not present
> > > 2.0 1 Section 5.2.10.4
> > > 3.0 2 Section 5.2.11.4
> > > 5.2.11.13 Local SAPIC Structure added two new fields:
> > > ACPI Processor UID Value
> > > ACPI Processor UID String
> > > 5.2.10.14 Platform Interrupt Sources Structure:
> > > Reserved changed to Platform Interrupt Sources Flags
> > > 3.0b 2 Section 5.2.11.4
> > > Added a section describing guidelines for the ordering of
> > > processors in the MADT to support proper boot processor
> > > and multi-threaded logical processor operation.
> > > 4.0 3 Section 5.2.12
> > > Adds Processor Local x2APIC structure type 9
> > > Adds Local x2APIC NMI structure type 0xA
> > > 5.0 3 Section 5.2.12
> > > 6.0 3 Section 5.2.12
> > > 6.0a 4 Section 5.2.12
> > > Adds ARM GIC structure types 0xB-0xF
> > > 6.2a 45 Section 5.2.12 <--- yep it says version 45!
> > > 6.2b 5 Section 5.2.12
> > > GIC ITS last Reserved offset changed to 16 from 20 (typo)
> > > 6.3 5 Section 5.2.12
> > > Adds Local APIC Flags Online Capable!
> > > Adds GICC SPE Overflow Interrupt field
> > > 6.4 5 Section 5.2.12
> > > Adds Multiprocessor Wakeup Structure type 0x10
> > > (change notes says structure previously misplaced?)
> > > 6.5 5 Section 5.2.12
> > >
> > > For the MADT revision change 1 -> 2, the spec has a change to the
> > > SAPIC structure. In general, QEMU does not generate/support SAPIC.
> > > So the QEMU i386 MADT revision can safely be moved to 2.
> > >
> > > For the MADT revision change 2 -> 3, the spec adds Local x2APIC
> > > structures. QEMU has long supported x2apic ACPI structures. A simple
> > > search of x2apic within QEMU source and hw/i386/acpi-common.c
> > > specifically reveals this.
> >
> > But not unconditionally.
>
> I don't think that reporting revision 3 requires that generation of x2apic;
> one could still see apic, x2apic, or sapic in theory. I realize qemu doesn't
> do sapic...
>
> >
> > > So the QEMU i386 MADT revision can safely
> > > be moved to 3.
> > >
> > > For the MADT revision change 3 -> 4, the spec adds support for the ARM
> > > GIC structures. QEMU ARM does in fact generate and report revision 4.
> > > As these will not be used by i386 QEMU, so then the QEMU i386 MADT
> > > revision can safely be moved to 4 as well.
> > >
> > > Now for the MADT revision change 4 -> 5, the spec adds the Online
> > > Capable flag to the Local APIC structure, and the ARM GICC SPE
> > > Overflow Interrupt field.
> > >
> > > For the ARM SPE, an existing 3-byte Reserved field is broken into a 1-
> > > byte Reserved field and a 2-byte SPE field. The spec says that is SPE
> > > Overflow is not supported, it should be zero.
> > >
> > > For the i386 Local APIC flag Online Capable, the spec has certain rules
> > > about this value. And in particuar setting this value now explicitly
> > > indicates a hotpluggable CPU.
> > >
> > > So this patch makes the needed changes to move both ARM and i386 MADT
> > > to revision 5. These are not complicated, thankfully.
> > >
> > > Without these changes, the information below shows "how" CPU hotplug
> > > breaks with the current upstream Linux kernel 6.3. For example, a Linux
> > > guest started with:
> > >
> > > qemu-system-x86_64 -smp 30,maxcpus=32 ...
> > >
> > > and then attempting to hotplug a CPU:
> > >
> > > (QEMU) device_add id=cpu30 driver=host-x86_64-cpu socket-id=0 core-id=30 thread-id=0
> > >
> > > fails with the following:
> > >
> > > APIC: NR_CPUS/possible_cpus limit of 30 reached. Processor 30/0x.
> > > ACPI: Unable to map lapic to logical cpu number
> > > acpi LNXCPU:1e: Enumeration failure
> > >
> > > # dmesg | grep smpboot
> > > smpboot: Allowing 30 CPUs, 0 hotplug CPUs
> > > smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x)
> > > smpboot: Max logical packages: 1
> > > smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
> > >
> > > # iasl -d /sys/firmware/tables/acpi/APIC
> > > [000h 0000 4] Signature : "APIC" [Multiple APIC Descript
> > > [004h 0004 4] Table Length : 00000170
> > > [008h 0008 1] Revision : 01 <=====
> > > [009h 0009 1] Checksum : 9C
> > > [00Ah 0010 6] Oem ID : "BOCHS "
> > > [010h 0016 8] Oem Table ID : "BXPC "
> > > [018h 0024 4] Oem Revision : 00000001
> > > [01Ch 0028 4] Asl Compiler ID : "BXPC"
> > > [020h 0032 4] Asl Compiler Revision : 00000001
> > >
> > > ...
> > >
> > > [114h 0276 1] Subtable Type : 00 [Processor Local APIC]
> > > [115h 0277 1] Length : 08
> > > [116h 0278 1] Processor ID : 1D
> > > [117h 0279 1] Local Apic ID : 1D
> > > [118h 0280 4] Flags (decoded below) : 00000001
> > > Processor Enabled : 1 <=====
> > >
> > > [11Ch 0284 1] Subtable Type : 00 [Processor Local APIC]
> > > [11Dh 0285 1] Length : 08
> > > [11Eh 0286 1] Processor ID : 1E
> > > [11Fh 0287 1] Local Apic ID : 1E
> > > [120h 0288 4] Flags (decoded below) : 00000000
> > > Processor Enabled : 0 <=====
> > >
> > > [124h 0292 1] Subtable Type : 00 [Processor Local APIC]
> > > [125h 0293 1] Length : 08
> > > [126h 0294 1] Processor ID : 1F
> > > [127h 0295 1] Local Apic ID : 1F
> > > [128h 0296 4] Flags (decoded below) : 00000000
> > > Processor Enabled : 0 <=====
> > >
> > > The (latest upstream) Linux kernel sees 30 Enabled processors, and
> > > does not consider processors 31 and 32 to be hotpluggable.
> > >
> > > With this patch series applied, by bumping MADT to revision 5, the
> > > latest upstream Linux kernel correctly identifies 30 CPUs plus 2
> > > hotpluggable CPUS.
> > >
> > > CPU30 has been hot-added
> > > smpboot: Booting Node 0 Processor 30 APIC 0x1e
> > > Will online and init hotplugged CPU: 30
> > >
> > > # dmesg | grep smpboot
> > > smpboot: Allowing 32 CPUs, 2 hotplug CPUs
> > > smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x6, model: 0x56, stepping: 0x3)
> > > smpboot: Max logical packages: 2
> > > smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
> > >
> > > # iasl -d /sys/firmware/tables/acpi/APIC
> > > [000h 0000 004h] Signature : "APIC" [Multiple APIC Descript
> > > [004h 0004 004h] Table Length : 00000170
> > > [008h 0008 001h] Revision : 05 <=====
> > > [009h 0009 001h] Checksum : 94
> > > [00Ah 0010 006h] Oem ID : "BOCHS "
> > > [010h 0016 008h] Oem Table ID : "BXPC "
> > > [018h 0024 004h] Oem Revision : 00000001
> > > [01Ch 0028 004h] Asl Compiler ID : "BXPC"
> > > [020h 0032 004h] Asl Compiler Revision : 00000001
> > >
> > > ...
> > >
> > > [114h 0276 001h] Subtable Type : 00 [Processor Local APIC]
> > > [115h 0277 001h] Length : 08
> > > [116h 0278 001h] Processor ID : 1D
> > > [117h 0279 001h] Local Apic ID : 1D
> > > [118h 0280 004h] Flags (decoded below) : 00000001
> > > Processor Enabled : 1 <=====
> > > Runtime Online Capable : 0 <=====
> > >
> > > [11Ch 0284 001h] Subtable Type : 00 [Processor Local APIC]
> > > [11Dh 0285 001h] Length : 08
> > > [11Eh 0286 001h] Processor ID : 1E
> > > [11Fh 0287 001h] Local Apic ID : 1E
> > > [120h 0288 004h] Flags (decoded below) : 00000002
> > > Processor Enabled : 0 <=====
> > > Runtime Online Capable : 1 <=====
> > >
> > > [124h 0292 001h] Subtable Type : 00 [Processor Local APIC]
> > > [125h 0293 001h] Length : 08
> > > [126h 0294 001h] Processor ID : 1F
> > > [127h 0295 001h] Local Apic ID : 1F
> > > [128h 0296 004h] Flags (decoded below) : 00000002
> > > Processor Enabled : 0 <=====
> > > Runtime Online Capable : 1 <=====
> > >
> > > Regards,
> > > Eric
> >
> > Can you please report which guests were tested?
>
> I've been using primarily upstream Linux. Kernels at and before 6.2.0 didn't
> have the "broken" patch mentioned above, and worked (for the reasons cited
> in the patch discussion to "fix" that patch). Any kernel since has the
> "broken" patch and will exhibit the issue.
>
> I've been using q35.
>
> If there are other samples you'd like to see, let me know and I'll try.
>
> Also, my responses will be delayed as I'm traveling the remainder of the week.
>
> Thanks!
> eric
As a minimum some windows versions. The older the better.
>
> >
> >
> > >
> > > Eric DeVolder (2):
> > > hw/acpi: arm: bump MADT to revision 5
> > > hw/acpi: i386: bump MADT to revision 5
> > >
> > > hw/arm/virt-acpi-build.c | 6 ++++--
> > > hw/i386/acpi-common.c | 13 ++++++++++---
> > > 2 files changed, 14 insertions(+), 5 deletions(-)
> > >
> > > --
> > > 2.31.1
> > >
> > >
> > >
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-03-29 13:19 ` Eric DeVolder
@ 2023-03-29 16:55 ` Michael S. Tsirkin
0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2023-03-29 16:55 UTC (permalink / raw)
To: Eric DeVolder
Cc: shannon.zhaosl, imammedo, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On Wed, Mar 29, 2023 at 08:19:22AM -0500, Eric DeVolder wrote:
>
>
> On 3/29/23 08:16, Eric DeVolder wrote:
> >
> >
> > On 3/29/23 00:03, Michael S. Tsirkin wrote:
> > > On Tue, Mar 28, 2023 at 11:59:26AM -0400, Eric DeVolder wrote:
> > > > Currently i386 QEMU generates MADT revision 3, and reports
> > > > MADT revision 1. ACPI 6.3 introduces MADT revision 5.
> > > >
> > > > For MADT revision 4, that introduces ARM GIC structures, which do
> > > > not apply to i386.
> > > >
> > > > For MADT revision 5, the Local APIC flags introduces the Online
> > > > Capable bitfield.
> > > >
> > > > Making MADT generate and report revision 5 will solve problems with
> > > > CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
> > > >
> > > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > >
> > > I am looking for ways to reduce risk of breakage with this.
> > > We don't currently have a reason to change it if cpu
> > > hotplug is off, do we? Maybe make it conditional on that.
> >
> > By "cpu hotplug off", do you mean, for example, no maxcpus= option?
> > In other words, how should I detect "cpu hotplug off"?
> > eric
> >
>
> Actually, if, for example, one had -smp 30,maxcpus=32, then there would be
> two hotpluggable cpus reported, the last two with the Enabled=0 and Online
> Capable=1. If one had -smp 32 (ie "cpu hotplug off"), then all cpus would be
> reported as Enabled and no cpu would have its Online Capable flag set.
>
> Granted in both cases, MADT.revision would report 5, but it would still be accurate.
>
> eric
sounds good.
> > >
> > >
> > >
> > >
> > >
> > > > ---
> > > > hw/i386/acpi-common.c | 13 ++++++++++---
> > > > 1 file changed, 10 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> > > > index 52e5c1439a..1e3a13a36c 100644
> > > > --- a/hw/i386/acpi-common.c
> > > > +++ b/hw/i386/acpi-common.c
> > > > @@ -38,8 +38,15 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
> > > > {
> > > > uint32_t apic_id = apic_ids->cpus[uid].arch_id;
> > > > /* Flags – Local APIC Flags */
> > > > - uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> > > > - 1 /* Enabled */ : 0;
> > > > + bool enabled = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> > > > + true /* Enabled */ : false;
> > > > + /*
> > > > + * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
> > > > + * if Enabled is set.
> > > > + */
> > > > + bool onlinecapable = enabled ? false : true; /* Online Capable */
> > > > + uint32_t flags = onlinecapable ? 0x2 : 0x0 |
> > > > + enabled ? 0x1 : 0x0;
> > > > /* ACPI spec says that LAPIC entry for non present
> > > > * CPU may be omitted from MADT or it must be marked
> > > > @@ -102,7 +109,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> > > > MachineClass *mc = MACHINE_GET_CLASS(x86ms);
> > > > const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
> > > > AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
> > > > - AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
> > > > + AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = oem_id,
> > > > .oem_table_id = oem_table_id };
> > > > acpi_table_begin(&table, table_data);
> > > > --
> > > > 2.31.1
> > > >
> > > >
> > > >
> > >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-29 16:47 ` Michael S. Tsirkin
@ 2023-03-30 7:36 ` Ani Sinha
2023-03-30 13:44 ` Michael S. Tsirkin
2023-03-31 16:25 ` Igor Mammedov
1 sibling, 1 reply; 22+ messages in thread
From: Ani Sinha @ 2023-03-30 7:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eric DeVolder, shannon.zhaosl, imammedo, ani, peter.maydell,
qemu-arm, qemu-devel, marcel.apfelbaum, pbonzini,
richard.henderson, eduardo, boris.ostrovsky
On Wed, 29 Mar 2023, Michael S. Tsirkin wrote:
> On Wed, Mar 29, 2023 at 08:14:37AM -0500, Eric DeVolder wrote:
> >
> >
> > On 3/29/23 00:19, Michael S. Tsirkin wrote:
> > > Hmm I don't think we can reasonably make such a change for 8.0.
> > > Seems too risky.
> > > Also, I feel we want to have an internal (with "x-" prefix") flag to
> > > revert to old behaviour, in case of breakage on some guests. and maybe
> > > we want to keep old revision for old machine types.
> > Ok, what option name, for keeping old behavior, would you like?
>
> Don't much care. x-madt-rev?
>
> > >
> > >
> > > On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
> > > > The following Linux kernel change broke CPU hotplug for MADT revision
> > > > less than 5.
> > > >
> > > > commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
> > >
> > > Presumably it's being fixed? Link to discussion? Patch fixing that in
> > > Linux?
> >
> > https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
>
> Great! Maybe stick a Link: tag in the commit log.
If the original bug is in the kernel and kernel upstream has accepted both
your fix and Mario's patch on the acpi revision mess, I see no urgency to
fix this in QEMU.
Maybe we can address this in the 8.1 development window.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-30 7:36 ` Ani Sinha
@ 2023-03-30 13:44 ` Michael S. Tsirkin
2023-03-30 14:02 ` Ani Sinha
0 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2023-03-30 13:44 UTC (permalink / raw)
To: Ani Sinha
Cc: Eric DeVolder, shannon.zhaosl, imammedo, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On Thu, Mar 30, 2023 at 01:06:36PM +0530, Ani Sinha wrote:
>
>
> On Wed, 29 Mar 2023, Michael S. Tsirkin wrote:
>
> > On Wed, Mar 29, 2023 at 08:14:37AM -0500, Eric DeVolder wrote:
> > >
> > >
> > > On 3/29/23 00:19, Michael S. Tsirkin wrote:
> > > > Hmm I don't think we can reasonably make such a change for 8.0.
> > > > Seems too risky.
> > > > Also, I feel we want to have an internal (with "x-" prefix") flag to
> > > > revert to old behaviour, in case of breakage on some guests. and maybe
> > > > we want to keep old revision for old machine types.
> > > Ok, what option name, for keeping old behavior, would you like?
> >
> > Don't much care. x-madt-rev?
> >
> > > >
> > > >
> > > > On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
> > > > > The following Linux kernel change broke CPU hotplug for MADT revision
> > > > > less than 5.
> > > > >
> > > > > commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
> > > >
> > > > Presumably it's being fixed? Link to discussion? Patch fixing that in
> > > > Linux?
> > >
> > > https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
> >
> > Great! Maybe stick a Link: tag in the commit log.
>
> If the original bug is in the kernel and kernel upstream has accepted both
> your fix and Mario's patch on the acpi revision mess, I see no urgency to
> fix this in QEMU.
>
> Maybe we can address this in the 8.1 development window.
Why "maybe"? Eric is working on a patch I don't see why we won't
address it.
--
MST
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-30 13:44 ` Michael S. Tsirkin
@ 2023-03-30 14:02 ` Ani Sinha
2023-03-30 14:11 ` Michael S. Tsirkin
0 siblings, 1 reply; 22+ messages in thread
From: Ani Sinha @ 2023-03-30 14:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Ani Sinha, Eric DeVolder, shannon.zhaosl, imammedo, peter.maydell,
qemu-arm, qemu-devel, marcel.apfelbaum, pbonzini,
richard.henderson, eduardo, boris.ostrovsky
On Thu, 30 Mar 2023, Michael S. Tsirkin wrote:
> On Thu, Mar 30, 2023 at 01:06:36PM +0530, Ani Sinha wrote:
> >
> >
> > On Wed, 29 Mar 2023, Michael S. Tsirkin wrote:
> >
> > > On Wed, Mar 29, 2023 at 08:14:37AM -0500, Eric DeVolder wrote:
> > > >
> > > >
> > > > On 3/29/23 00:19, Michael S. Tsirkin wrote:
> > > > > Hmm I don't think we can reasonably make such a change for 8.0.
> > > > > Seems too risky.
> > > > > Also, I feel we want to have an internal (with "x-" prefix") flag to
> > > > > revert to old behaviour, in case of breakage on some guests. and maybe
> > > > > we want to keep old revision for old machine types.
> > > > Ok, what option name, for keeping old behavior, would you like?
> > >
> > > Don't much care. x-madt-rev?
> > >
> > > > >
> > > > >
> > > > > On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
> > > > > > The following Linux kernel change broke CPU hotplug for MADT revision
> > > > > > less than 5.
> > > > > >
> > > > > > commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
> > > > >
> > > > > Presumably it's being fixed? Link to discussion? Patch fixing that in
> > > > > Linux?
> > > >
> > > > https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
> > >
> > > Great! Maybe stick a Link: tag in the commit log.
> >
> > If the original bug is in the kernel and kernel upstream has accepted both
> > your fix and Mario's patch on the acpi revision mess, I see no urgency to
> > fix this in QEMU.
> >
> > Maybe we can address this in the 8.1 development window.
>
> Why "maybe"? Eric is working on a patch I don't see why we won't
> address it.
>
I am not opposed to addressing it. I just think it might be too risky for
8.0 given that we are already past the hard feature freeze date.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-30 14:02 ` Ani Sinha
@ 2023-03-30 14:11 ` Michael S. Tsirkin
0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2023-03-30 14:11 UTC (permalink / raw)
To: Ani Sinha
Cc: Eric DeVolder, shannon.zhaosl, imammedo, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On Thu, Mar 30, 2023 at 07:32:59PM +0530, Ani Sinha wrote:
>
>
> On Thu, 30 Mar 2023, Michael S. Tsirkin wrote:
>
> > On Thu, Mar 30, 2023 at 01:06:36PM +0530, Ani Sinha wrote:
> > >
> > >
> > > On Wed, 29 Mar 2023, Michael S. Tsirkin wrote:
> > >
> > > > On Wed, Mar 29, 2023 at 08:14:37AM -0500, Eric DeVolder wrote:
> > > > >
> > > > >
> > > > > On 3/29/23 00:19, Michael S. Tsirkin wrote:
> > > > > > Hmm I don't think we can reasonably make such a change for 8.0.
> > > > > > Seems too risky.
> > > > > > Also, I feel we want to have an internal (with "x-" prefix") flag to
> > > > > > revert to old behaviour, in case of breakage on some guests. and maybe
> > > > > > we want to keep old revision for old machine types.
> > > > > Ok, what option name, for keeping old behavior, would you like?
> > > >
> > > > Don't much care. x-madt-rev?
> > > >
> > > > > >
> > > > > >
> > > > > > On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
> > > > > > > The following Linux kernel change broke CPU hotplug for MADT revision
> > > > > > > less than 5.
> > > > > > >
> > > > > > > commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
> > > > > >
> > > > > > Presumably it's being fixed? Link to discussion? Patch fixing that in
> > > > > > Linux?
> > > > >
> > > > > https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
> > > >
> > > > Great! Maybe stick a Link: tag in the commit log.
> > >
> > > If the original bug is in the kernel and kernel upstream has accepted both
> > > your fix and Mario's patch on the acpi revision mess, I see no urgency to
> > > fix this in QEMU.
> > >
> > > Maybe we can address this in the 8.1 development window.
> >
> > Why "maybe"? Eric is working on a patch I don't see why we won't
> > address it.
> >
>
> I am not opposed to addressing it. I just think it might be too risky for
> 8.0 given that we are already past the hard feature freeze date.
This does not look to me like a justified change for 8.0.
We have rev = 1 for years.
--
MST
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-29 16:47 ` Michael S. Tsirkin
2023-03-30 7:36 ` Ani Sinha
@ 2023-03-31 16:25 ` Igor Mammedov
2023-04-04 14:52 ` Eric DeVolder
1 sibling, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2023-03-31 16:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eric DeVolder, shannon.zhaosl, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On Wed, 29 Mar 2023 12:47:05 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Mar 29, 2023 at 08:14:37AM -0500, Eric DeVolder wrote:
> >
> >
> > On 3/29/23 00:19, Michael S. Tsirkin wrote:
> > > Hmm I don't think we can reasonably make such a change for 8.0.
> > > Seems too risky.
> > > Also, I feel we want to have an internal (with "x-" prefix") flag to
> > > revert to old behaviour, in case of breakage on some guests. and maybe
> > > we want to keep old revision for old machine types.
> > Ok, what option name, for keeping old behavior, would you like?
>
> Don't much care. x-madt-rev?
if it works fine (cold & hot-plug) with older linux/windows guests
I'd rather avoid adding compat knob (we typically do that in ACPI tables
only when change breaks something).
(as old guest I'd define WinXP sp3 (/me wonders if we still care about
dead EOLed OS) perhaps WS2008 would be a better minimum target these days
and RHEL6 (or some older ACPI enabled kernel with hotplug support))
>
> > >
> > >
> > > On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
> > > > The following Linux kernel change broke CPU hotplug for MADT revision
> > > > less than 5.
> > > >
> > > > commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
> > >
> > > Presumably it's being fixed? Link to discussion? Patch fixing that in
> > > Linux?
> >
> > https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
>
> Great! Maybe stick a Link: tag in the commit log.
So it's guest bug which is in process of being fixed.
(i.e. QEMU technically correct as long as MADT revision < 5)
In this case I'd not touch x86 MADT at all (It should be upto
downstream distros to fix guest kernel).
Probably the same applies to ARM variant
i.e. we should bump rev only when current one gets in the way
(aka we are pulling in new fields/definitions from new version)
> > > > As part of the investigation into resolving this breakage, I learned
> > > > that i386 QEMU reports revision 1, while technically it is at revision 3.
> > > > (Arm QEMU reports revision 4, and that is valid/correct.)
> > > >
> > > > ACPI 6.3 bumps MADT revision to 5 as it introduces an Online Capable
> > > > flag that the above Linux patch utilizes to denote hot pluggable CPUs.
> > > >
> > > > So in order to bump MADT to the current revision of 5, need to
> > > > validate that all MADT table changes between 1 and 5 are present
> > > > in QEMU.
> > > >
> > > > Below is a table summarizing the changes to the MADT. This information
> > > > gleamed from the ACPI specs on uefi.org.
> > > >
> > > > ACPI MADT What
> > > > Version Version
> > > > 1.0 MADT not present
> > > > 2.0 1 Section 5.2.10.4
> > > > 3.0 2 Section 5.2.11.4
> > > > 5.2.11.13 Local SAPIC Structure added two new fields:
> > > > ACPI Processor UID Value
> > > > ACPI Processor UID String
> > > > 5.2.10.14 Platform Interrupt Sources Structure:
> > > > Reserved changed to Platform Interrupt Sources Flags
> > > > 3.0b 2 Section 5.2.11.4
> > > > Added a section describing guidelines for the ordering of
> > > > processors in the MADT to support proper boot processor
> > > > and multi-threaded logical processor operation.
> > > > 4.0 3 Section 5.2.12
> > > > Adds Processor Local x2APIC structure type 9
> > > > Adds Local x2APIC NMI structure type 0xA
> > > > 5.0 3 Section 5.2.12
> > > > 6.0 3 Section 5.2.12
> > > > 6.0a 4 Section 5.2.12
> > > > Adds ARM GIC structure types 0xB-0xF
> > > > 6.2a 45 Section 5.2.12 <--- yep it says version 45!
> > > > 6.2b 5 Section 5.2.12
> > > > GIC ITS last Reserved offset changed to 16 from 20 (typo)
> > > > 6.3 5 Section 5.2.12
> > > > Adds Local APIC Flags Online Capable!
> > > > Adds GICC SPE Overflow Interrupt field
> > > > 6.4 5 Section 5.2.12
> > > > Adds Multiprocessor Wakeup Structure type 0x10
> > > > (change notes says structure previously misplaced?)
> > > > 6.5 5 Section 5.2.12
> > > >
> > > > For the MADT revision change 1 -> 2, the spec has a change to the
> > > > SAPIC structure. In general, QEMU does not generate/support SAPIC.
> > > > So the QEMU i386 MADT revision can safely be moved to 2.
> > > >
> > > > For the MADT revision change 2 -> 3, the spec adds Local x2APIC
> > > > structures. QEMU has long supported x2apic ACPI structures. A simple
> > > > search of x2apic within QEMU source and hw/i386/acpi-common.c
> > > > specifically reveals this.
> > >
> > > But not unconditionally.
> >
> > I don't think that reporting revision 3 requires that generation of x2apic;
> > one could still see apic, x2apic, or sapic in theory. I realize qemu doesn't
> > do sapic...
> >
> > >
> > > > So the QEMU i386 MADT revision can safely
> > > > be moved to 3.
> > > >
> > > > For the MADT revision change 3 -> 4, the spec adds support for the ARM
> > > > GIC structures. QEMU ARM does in fact generate and report revision 4.
> > > > As these will not be used by i386 QEMU, so then the QEMU i386 MADT
> > > > revision can safely be moved to 4 as well.
> > > >
> > > > Now for the MADT revision change 4 -> 5, the spec adds the Online
> > > > Capable flag to the Local APIC structure, and the ARM GICC SPE
> > > > Overflow Interrupt field.
> > > >
> > > > For the ARM SPE, an existing 3-byte Reserved field is broken into a 1-
> > > > byte Reserved field and a 2-byte SPE field. The spec says that is SPE
> > > > Overflow is not supported, it should be zero.
> > > >
> > > > For the i386 Local APIC flag Online Capable, the spec has certain rules
> > > > about this value. And in particuar setting this value now explicitly
> > > > indicates a hotpluggable CPU.
> > > >
> > > > So this patch makes the needed changes to move both ARM and i386 MADT
> > > > to revision 5. These are not complicated, thankfully.
> > > >
> > > > Without these changes, the information below shows "how" CPU hotplug
> > > > breaks with the current upstream Linux kernel 6.3. For example, a Linux
> > > > guest started with:
> > > >
> > > > qemu-system-x86_64 -smp 30,maxcpus=32 ...
> > > >
> > > > and then attempting to hotplug a CPU:
> > > >
> > > > (QEMU) device_add id=cpu30 driver=host-x86_64-cpu socket-id=0 core-id=30 thread-id=0
> > > >
> > > > fails with the following:
> > > >
> > > > APIC: NR_CPUS/possible_cpus limit of 30 reached. Processor 30/0x.
> > > > ACPI: Unable to map lapic to logical cpu number
> > > > acpi LNXCPU:1e: Enumeration failure
> > > >
> > > > # dmesg | grep smpboot
> > > > smpboot: Allowing 30 CPUs, 0 hotplug CPUs
> > > > smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x)
> > > > smpboot: Max logical packages: 1
> > > > smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
> > > >
> > > > # iasl -d /sys/firmware/tables/acpi/APIC
> > > > [000h 0000 4] Signature : "APIC" [Multiple APIC Descript
> > > > [004h 0004 4] Table Length : 00000170
> > > > [008h 0008 1] Revision : 01 <=====
> > > > [009h 0009 1] Checksum : 9C
> > > > [00Ah 0010 6] Oem ID : "BOCHS "
> > > > [010h 0016 8] Oem Table ID : "BXPC "
> > > > [018h 0024 4] Oem Revision : 00000001
> > > > [01Ch 0028 4] Asl Compiler ID : "BXPC"
> > > > [020h 0032 4] Asl Compiler Revision : 00000001
> > > >
> > > > ...
> > > >
> > > > [114h 0276 1] Subtable Type : 00 [Processor Local APIC]
> > > > [115h 0277 1] Length : 08
> > > > [116h 0278 1] Processor ID : 1D
> > > > [117h 0279 1] Local Apic ID : 1D
> > > > [118h 0280 4] Flags (decoded below) : 00000001
> > > > Processor Enabled : 1 <=====
> > > >
> > > > [11Ch 0284 1] Subtable Type : 00 [Processor Local APIC]
> > > > [11Dh 0285 1] Length : 08
> > > > [11Eh 0286 1] Processor ID : 1E
> > > > [11Fh 0287 1] Local Apic ID : 1E
> > > > [120h 0288 4] Flags (decoded below) : 00000000
> > > > Processor Enabled : 0 <=====
> > > >
> > > > [124h 0292 1] Subtable Type : 00 [Processor Local APIC]
> > > > [125h 0293 1] Length : 08
> > > > [126h 0294 1] Processor ID : 1F
> > > > [127h 0295 1] Local Apic ID : 1F
> > > > [128h 0296 4] Flags (decoded below) : 00000000
> > > > Processor Enabled : 0 <=====
> > > >
> > > > The (latest upstream) Linux kernel sees 30 Enabled processors, and
> > > > does not consider processors 31 and 32 to be hotpluggable.
> > > >
> > > > With this patch series applied, by bumping MADT to revision 5, the
> > > > latest upstream Linux kernel correctly identifies 30 CPUs plus 2
> > > > hotpluggable CPUS.
> > > >
> > > > CPU30 has been hot-added
> > > > smpboot: Booting Node 0 Processor 30 APIC 0x1e
> > > > Will online and init hotplugged CPU: 30
> > > >
> > > > # dmesg | grep smpboot
> > > > smpboot: Allowing 32 CPUs, 2 hotplug CPUs
> > > > smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x6, model: 0x56, stepping: 0x3)
> > > > smpboot: Max logical packages: 2
> > > > smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
> > > >
> > > > # iasl -d /sys/firmware/tables/acpi/APIC
> > > > [000h 0000 004h] Signature : "APIC" [Multiple APIC Descript
> > > > [004h 0004 004h] Table Length : 00000170
> > > > [008h 0008 001h] Revision : 05 <=====
> > > > [009h 0009 001h] Checksum : 94
> > > > [00Ah 0010 006h] Oem ID : "BOCHS "
> > > > [010h 0016 008h] Oem Table ID : "BXPC "
> > > > [018h 0024 004h] Oem Revision : 00000001
> > > > [01Ch 0028 004h] Asl Compiler ID : "BXPC"
> > > > [020h 0032 004h] Asl Compiler Revision : 00000001
> > > >
> > > > ...
> > > >
> > > > [114h 0276 001h] Subtable Type : 00 [Processor Local APIC]
> > > > [115h 0277 001h] Length : 08
> > > > [116h 0278 001h] Processor ID : 1D
> > > > [117h 0279 001h] Local Apic ID : 1D
> > > > [118h 0280 004h] Flags (decoded below) : 00000001
> > > > Processor Enabled : 1 <=====
> > > > Runtime Online Capable : 0 <=====
> > > >
> > > > [11Ch 0284 001h] Subtable Type : 00 [Processor Local APIC]
> > > > [11Dh 0285 001h] Length : 08
> > > > [11Eh 0286 001h] Processor ID : 1E
> > > > [11Fh 0287 001h] Local Apic ID : 1E
> > > > [120h 0288 004h] Flags (decoded below) : 00000002
> > > > Processor Enabled : 0 <=====
> > > > Runtime Online Capable : 1 <=====
> > > >
> > > > [124h 0292 001h] Subtable Type : 00 [Processor Local APIC]
> > > > [125h 0293 001h] Length : 08
> > > > [126h 0294 001h] Processor ID : 1F
> > > > [127h 0295 001h] Local Apic ID : 1F
> > > > [128h 0296 004h] Flags (decoded below) : 00000002
> > > > Processor Enabled : 0 <=====
> > > > Runtime Online Capable : 1 <=====
> > > >
> > > > Regards,
> > > > Eric
> > >
> > > Can you please report which guests were tested?
> >
> > I've been using primarily upstream Linux. Kernels at and before 6.2.0 didn't
> > have the "broken" patch mentioned above, and worked (for the reasons cited
> > in the patch discussion to "fix" that patch). Any kernel since has the
> > "broken" patch and will exhibit the issue.
> >
> > I've been using q35.
> >
> > If there are other samples you'd like to see, let me know and I'll try.
> >
> > Also, my responses will be delayed as I'm traveling the remainder of the week.
> >
> > Thanks!
> > eric
>
> As a minimum some windows versions. The older the better.
>
>
> >
> > >
> > >
> > > >
> > > > Eric DeVolder (2):
> > > > hw/acpi: arm: bump MADT to revision 5
> > > > hw/acpi: i386: bump MADT to revision 5
> > > >
> > > > hw/arm/virt-acpi-build.c | 6 ++++--
> > > > hw/i386/acpi-common.c | 13 ++++++++++---
> > > > 2 files changed, 14 insertions(+), 5 deletions(-)
> > > >
> > > > --
> > > > 2.31.1
> > > >
> > > >
> > > >
> > >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-03-29 13:16 ` Eric DeVolder
2023-03-29 13:19 ` Eric DeVolder
@ 2023-03-31 16:29 ` Igor Mammedov
1 sibling, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2023-03-31 16:29 UTC (permalink / raw)
To: Eric DeVolder
Cc: Michael S. Tsirkin, shannon.zhaosl, ani, peter.maydell, qemu-arm,
qemu-devel, marcel.apfelbaum, pbonzini, richard.henderson,
eduardo, boris.ostrovsky
On Wed, 29 Mar 2023 08:16:26 -0500
Eric DeVolder <eric.devolder@oracle.com> wrote:
> On 3/29/23 00:03, Michael S. Tsirkin wrote:
> > On Tue, Mar 28, 2023 at 11:59:26AM -0400, Eric DeVolder wrote:
> >> Currently i386 QEMU generates MADT revision 3, and reports
> >> MADT revision 1. ACPI 6.3 introduces MADT revision 5.
> >>
> >> For MADT revision 4, that introduces ARM GIC structures, which do
> >> not apply to i386.
> >>
> >> For MADT revision 5, the Local APIC flags introduces the Online
> >> Capable bitfield.
> >>
> >> Making MADT generate and report revision 5 will solve problems with
> >> CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
> >>
> >> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> >
> > I am looking for ways to reduce risk of breakage with this.
> > We don't currently have a reason to change it if cpu
> > hotplug is off, do we? Maybe make it conditional on that.
>
> By "cpu hotplug off", do you mean, for example, no maxcpus= option?
> In other words, how should I detect "cpu hotplug off"?
> eric
I'm not sure that it's possible disable CPU hotplug at all.
even if one doesn't have maxcpus on CLI present CPUs are described
as hotpluggbale and can be unplugged and re-plugged later.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5
2023-03-31 16:25 ` Igor Mammedov
@ 2023-04-04 14:52 ` Eric DeVolder
0 siblings, 0 replies; 22+ messages in thread
From: Eric DeVolder @ 2023-04-04 14:52 UTC (permalink / raw)
To: Igor Mammedov, Michael S. Tsirkin
Cc: shannon.zhaosl, ani, peter.maydell, qemu-arm, qemu-devel,
marcel.apfelbaum, pbonzini, richard.henderson, eduardo,
boris.ostrovsky
I'm back from travel and catching up. More info below.
eric
On 3/31/23 11:25, Igor Mammedov wrote:
> On Wed, 29 Mar 2023 12:47:05 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Wed, Mar 29, 2023 at 08:14:37AM -0500, Eric DeVolder wrote:
>>>
>>>
>>> On 3/29/23 00:19, Michael S. Tsirkin wrote:
>>>> Hmm I don't think we can reasonably make such a change for 8.0.
>>>> Seems too risky.
>>>> Also, I feel we want to have an internal (with "x-" prefix") flag to
>>>> revert to old behaviour, in case of breakage on some guests. and maybe
>>>> we want to keep old revision for old machine types.
>>> Ok, what option name, for keeping old behavior, would you like?
>>
>> Don't much care. x-madt-rev?
>
> if it works fine (cold & hot-plug) with older linux/windows guests
> I'd rather avoid adding compat knob (we typically do that in ACPI tables
> only when change breaks something).
>
> (as old guest I'd define WinXP sp3 (/me wonders if we still care about
> dead EOLed OS) perhaps WS2008 would be a better minimum target these days
> and RHEL6 (or some older ACPI enabled kernel with hotplug support))
Thus far I've tested this patch series with the following guests. In both cases below,
the guest started with -smp 8,maxcpus=16, and I simply attempted to hot plug and unplug
cpu8, with: device_add id=cpu8 driver=host-x86_64-cpu socket-id=0 core-id=8 thread-id=0.
1) Windows Server 2008.
In Windows, after logging-in, I close the first full-screen window
and then "Server Manager" window pops up. I navigate to left hand
pane and choose Diagnostics -> Device Manager -> Processors.
I count them before and after.
When hotplugging a cpu, you can see the new processor show up in the
processor list. That is, it goes from 8 to 9.
When hot unplugging the CPU, Windows refuses:
The 'Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz' device is not
removable and cannot be ejected or unplugged.
2) RHEL 6.9
From dmesg:
ACPI: APIC 000000007ffe32f0 000F0 (v05 BOCHS BXPC 00000001 BXPC 00000001)
SMP: Allowing 16 CPUs, 8 hotplug CPUs
# cat /sys/devices/system/cpu/online
0-7
(QEMU) device_add ...
CPU 8 got hotplugged
Booting Node 0 Processor 8 APIC 0x8
kvm-clock: cpu 8, msr 2830ed00
Will online and init hotplugged CPU: 8
microcode: CPU8 sig=0x50663, pf=0x1, revision=0x700001c
platform microcode: firmware: requesting intel-ucode/06-56-03
# cat /sys/devices/system/cpu/online
0-8
(QEMU) device_del ...
Broke affinity for irq 24
CPU 8 is now offline
# cat /sys/devices/system/cpu/online
0-7
RHEL 6.9
kernel 2.6.32-696.el6.x86_64
build Feb 21 2017
So with these two older guest OS, it appears to still work. If there are others to be tested, let me
know.
>
>>
>>>>
>>>>
>>>> On Tue, Mar 28, 2023 at 11:59:24AM -0400, Eric DeVolder wrote:
>>>>> The following Linux kernel change broke CPU hotplug for MADT revision
>>>>> less than 5.
>>>>>
>>>>> commit e2869bd7af60 ("x86/acpi/boot: Do not register processors that cannot be onlined for x2APIC")
>>>>
>>>> Presumably it's being fixed? Link to discussion? Patch fixing that in
>>>> Linux?
>>>
>>> https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
>>
>> Great! Maybe stick a Link: tag in the commit log.
Sure, will include that with v2.
>
> So it's guest bug which is in process of being fixed.
> (i.e. QEMU technically correct as long as MADT revision < 5)
Iiuc, if QEMU generates x2apic tables with .revision = 1, that is not correct
(as x2APIC shows up in .revision=3).
But if QEMU generates, apic, sapic, or x2apic tables with .revision = 5, that
is correct (as all are valid options thru .revision 5).
>
> In this case I'd not touch x86 MADT at all (It should be upto
> downstream distros to fix guest kernel).
Fwiw, this has been fixed and should show up in 6.3-rc6 this weekend.
>
> Probably the same applies to ARM variant
> i.e. we should bump rev only when current one gets in the way
> (aka we are pulling in new fields/definitions from new version)
>
>
>>>>> As part of the investigation into resolving this breakage, I learned
>>>>> that i386 QEMU reports revision 1, while technically it is at revision 3.
>>>>> (Arm QEMU reports revision 4, and that is valid/correct.)
>>>>>
>>>>> ACPI 6.3 bumps MADT revision to 5 as it introduces an Online Capable
>>>>> flag that the above Linux patch utilizes to denote hot pluggable CPUs.
>>>>>
>>>>> So in order to bump MADT to the current revision of 5, need to
>>>>> validate that all MADT table changes between 1 and 5 are present
>>>>> in QEMU.
>>>>>
>>>>> Below is a table summarizing the changes to the MADT. This information
>>>>> gleamed from the ACPI specs on uefi.org.
>>>>>
>>>>> ACPI MADT What
>>>>> Version Version
>>>>> 1.0 MADT not present
>>>>> 2.0 1 Section 5.2.10.4
>>>>> 3.0 2 Section 5.2.11.4
>>>>> 5.2.11.13 Local SAPIC Structure added two new fields:
>>>>> ACPI Processor UID Value
>>>>> ACPI Processor UID String
>>>>> 5.2.10.14 Platform Interrupt Sources Structure:
>>>>> Reserved changed to Platform Interrupt Sources Flags
>>>>> 3.0b 2 Section 5.2.11.4
>>>>> Added a section describing guidelines for the ordering of
>>>>> processors in the MADT to support proper boot processor
>>>>> and multi-threaded logical processor operation.
>>>>> 4.0 3 Section 5.2.12
>>>>> Adds Processor Local x2APIC structure type 9
>>>>> Adds Local x2APIC NMI structure type 0xA
>>>>> 5.0 3 Section 5.2.12
>>>>> 6.0 3 Section 5.2.12
>>>>> 6.0a 4 Section 5.2.12
>>>>> Adds ARM GIC structure types 0xB-0xF
>>>>> 6.2a 45 Section 5.2.12 <--- yep it says version 45!
>>>>> 6.2b 5 Section 5.2.12
>>>>> GIC ITS last Reserved offset changed to 16 from 20 (typo)
>>>>> 6.3 5 Section 5.2.12
>>>>> Adds Local APIC Flags Online Capable!
>>>>> Adds GICC SPE Overflow Interrupt field
>>>>> 6.4 5 Section 5.2.12
>>>>> Adds Multiprocessor Wakeup Structure type 0x10
>>>>> (change notes says structure previously misplaced?)
>>>>> 6.5 5 Section 5.2.12
>>>>>
>>>>> For the MADT revision change 1 -> 2, the spec has a change to the
>>>>> SAPIC structure. In general, QEMU does not generate/support SAPIC.
>>>>> So the QEMU i386 MADT revision can safely be moved to 2.
>>>>>
>>>>> For the MADT revision change 2 -> 3, the spec adds Local x2APIC
>>>>> structures. QEMU has long supported x2apic ACPI structures. A simple
>>>>> search of x2apic within QEMU source and hw/i386/acpi-common.c
>>>>> specifically reveals this.
>>>>
>>>> But not unconditionally.
>>>
>>> I don't think that reporting revision 3 requires that generation of x2apic;
>>> one could still see apic, x2apic, or sapic in theory. I realize qemu doesn't
>>> do sapic...
>>>
>>>>
>>>>> So the QEMU i386 MADT revision can safely
>>>>> be moved to 3.
>>>>>
>>>>> For the MADT revision change 3 -> 4, the spec adds support for the ARM
>>>>> GIC structures. QEMU ARM does in fact generate and report revision 4.
>>>>> As these will not be used by i386 QEMU, so then the QEMU i386 MADT
>>>>> revision can safely be moved to 4 as well.
>>>>>
>>>>> Now for the MADT revision change 4 -> 5, the spec adds the Online
>>>>> Capable flag to the Local APIC structure, and the ARM GICC SPE
>>>>> Overflow Interrupt field.
>>>>>
>>>>> For the ARM SPE, an existing 3-byte Reserved field is broken into a 1-
>>>>> byte Reserved field and a 2-byte SPE field. The spec says that is SPE
>>>>> Overflow is not supported, it should be zero.
>>>>>
>>>>> For the i386 Local APIC flag Online Capable, the spec has certain rules
>>>>> about this value. And in particuar setting this value now explicitly
>>>>> indicates a hotpluggable CPU.
>>>>>
>>>>> So this patch makes the needed changes to move both ARM and i386 MADT
>>>>> to revision 5. These are not complicated, thankfully.
>>>>>
>>>>> Without these changes, the information below shows "how" CPU hotplug
>>>>> breaks with the current upstream Linux kernel 6.3. For example, a Linux
>>>>> guest started with:
>>>>>
>>>>> qemu-system-x86_64 -smp 30,maxcpus=32 ...
>>>>>
>>>>> and then attempting to hotplug a CPU:
>>>>>
>>>>> (QEMU) device_add id=cpu30 driver=host-x86_64-cpu socket-id=0 core-id=30 thread-id=0
>>>>>
>>>>> fails with the following:
>>>>>
>>>>> APIC: NR_CPUS/possible_cpus limit of 30 reached. Processor 30/0x.
>>>>> ACPI: Unable to map lapic to logical cpu number
>>>>> acpi LNXCPU:1e: Enumeration failure
>>>>>
>>>>> # dmesg | grep smpboot
>>>>> smpboot: Allowing 30 CPUs, 0 hotplug CPUs
>>>>> smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x)
>>>>> smpboot: Max logical packages: 1
>>>>> smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
>>>>>
>>>>> # iasl -d /sys/firmware/tables/acpi/APIC
>>>>> [000h 0000 4] Signature : "APIC" [Multiple APIC Descript
>>>>> [004h 0004 4] Table Length : 00000170
>>>>> [008h 0008 1] Revision : 01 <=====
>>>>> [009h 0009 1] Checksum : 9C
>>>>> [00Ah 0010 6] Oem ID : "BOCHS "
>>>>> [010h 0016 8] Oem Table ID : "BXPC "
>>>>> [018h 0024 4] Oem Revision : 00000001
>>>>> [01Ch 0028 4] Asl Compiler ID : "BXPC"
>>>>> [020h 0032 4] Asl Compiler Revision : 00000001
>>>>>
>>>>> ...
>>>>>
>>>>> [114h 0276 1] Subtable Type : 00 [Processor Local APIC]
>>>>> [115h 0277 1] Length : 08
>>>>> [116h 0278 1] Processor ID : 1D
>>>>> [117h 0279 1] Local Apic ID : 1D
>>>>> [118h 0280 4] Flags (decoded below) : 00000001
>>>>> Processor Enabled : 1 <=====
>>>>>
>>>>> [11Ch 0284 1] Subtable Type : 00 [Processor Local APIC]
>>>>> [11Dh 0285 1] Length : 08
>>>>> [11Eh 0286 1] Processor ID : 1E
>>>>> [11Fh 0287 1] Local Apic ID : 1E
>>>>> [120h 0288 4] Flags (decoded below) : 00000000
>>>>> Processor Enabled : 0 <=====
>>>>>
>>>>> [124h 0292 1] Subtable Type : 00 [Processor Local APIC]
>>>>> [125h 0293 1] Length : 08
>>>>> [126h 0294 1] Processor ID : 1F
>>>>> [127h 0295 1] Local Apic ID : 1F
>>>>> [128h 0296 4] Flags (decoded below) : 00000000
>>>>> Processor Enabled : 0 <=====
>>>>>
>>>>> The (latest upstream) Linux kernel sees 30 Enabled processors, and
>>>>> does not consider processors 31 and 32 to be hotpluggable.
>>>>>
>>>>> With this patch series applied, by bumping MADT to revision 5, the
>>>>> latest upstream Linux kernel correctly identifies 30 CPUs plus 2
>>>>> hotpluggable CPUS.
>>>>>
>>>>> CPU30 has been hot-added
>>>>> smpboot: Booting Node 0 Processor 30 APIC 0x1e
>>>>> Will online and init hotplugged CPU: 30
>>>>>
>>>>> # dmesg | grep smpboot
>>>>> smpboot: Allowing 32 CPUs, 2 hotplug CPUs
>>>>> smpboot: CPU0: Intel(R) Xeon(R) CPU D-1533 @ 2.10GHz (family: 0x6, model: 0x56, stepping: 0x3)
>>>>> smpboot: Max logical packages: 2
>>>>> smpboot: Total of 30 processors activated (125708.76 BogoMIPS)
>>>>>
>>>>> # iasl -d /sys/firmware/tables/acpi/APIC
>>>>> [000h 0000 004h] Signature : "APIC" [Multiple APIC Descript
>>>>> [004h 0004 004h] Table Length : 00000170
>>>>> [008h 0008 001h] Revision : 05 <=====
>>>>> [009h 0009 001h] Checksum : 94
>>>>> [00Ah 0010 006h] Oem ID : "BOCHS "
>>>>> [010h 0016 008h] Oem Table ID : "BXPC "
>>>>> [018h 0024 004h] Oem Revision : 00000001
>>>>> [01Ch 0028 004h] Asl Compiler ID : "BXPC"
>>>>> [020h 0032 004h] Asl Compiler Revision : 00000001
>>>>>
>>>>> ...
>>>>>
>>>>> [114h 0276 001h] Subtable Type : 00 [Processor Local APIC]
>>>>> [115h 0277 001h] Length : 08
>>>>> [116h 0278 001h] Processor ID : 1D
>>>>> [117h 0279 001h] Local Apic ID : 1D
>>>>> [118h 0280 004h] Flags (decoded below) : 00000001
>>>>> Processor Enabled : 1 <=====
>>>>> Runtime Online Capable : 0 <=====
>>>>>
>>>>> [11Ch 0284 001h] Subtable Type : 00 [Processor Local APIC]
>>>>> [11Dh 0285 001h] Length : 08
>>>>> [11Eh 0286 001h] Processor ID : 1E
>>>>> [11Fh 0287 001h] Local Apic ID : 1E
>>>>> [120h 0288 004h] Flags (decoded below) : 00000002
>>>>> Processor Enabled : 0 <=====
>>>>> Runtime Online Capable : 1 <=====
>>>>>
>>>>> [124h 0292 001h] Subtable Type : 00 [Processor Local APIC]
>>>>> [125h 0293 001h] Length : 08
>>>>> [126h 0294 001h] Processor ID : 1F
>>>>> [127h 0295 001h] Local Apic ID : 1F
>>>>> [128h 0296 004h] Flags (decoded below) : 00000002
>>>>> Processor Enabled : 0 <=====
>>>>> Runtime Online Capable : 1 <=====
>>>>>
>>>>> Regards,
>>>>> Eric
>>>>
>>>> Can you please report which guests were tested?
>>>
>>> I've been using primarily upstream Linux. Kernels at and before 6.2.0 didn't
>>> have the "broken" patch mentioned above, and worked (for the reasons cited
>>> in the patch discussion to "fix" that patch). Any kernel since has the
>>> "broken" patch and will exhibit the issue.
>>>
>>> I've been using q35.
>>>
>>> If there are other samples you'd like to see, let me know and I'll try.
>>>
>>> Also, my responses will be delayed as I'm traveling the remainder of the week.
>>>
>>> Thanks!
>>> eric
>>
>> As a minimum some windows versions. The older the better.
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>>> Eric DeVolder (2):
>>>>> hw/acpi: arm: bump MADT to revision 5
>>>>> hw/acpi: i386: bump MADT to revision 5
>>>>>
>>>>> hw/arm/virt-acpi-build.c | 6 ++++--
>>>>> hw/i386/acpi-common.c | 13 ++++++++++---
>>>>> 2 files changed, 14 insertions(+), 5 deletions(-)
>>>>>
>>>>> --
>>>>> 2.31.1
>>>>>
>>>>>
>>>>>
>>>>
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] hw/acpi: arm: bump MADT to revision 5
2023-03-28 15:59 ` [PATCH 1/2] hw/acpi: arm: " Eric DeVolder
@ 2023-04-11 14:49 ` Igor Mammedov
0 siblings, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2023-04-11 14:49 UTC (permalink / raw)
To: Eric DeVolder
Cc: shannon.zhaosl, mst, ani, peter.maydell, qemu-arm, qemu-devel,
marcel.apfelbaum, pbonzini, richard.henderson, eduardo,
boris.ostrovsky
On Tue, 28 Mar 2023 11:59:25 -0400
Eric DeVolder <eric.devolder@oracle.com> wrote:
> Currently ARM QEMU generates, and reports, MADT revision 4. ACPI 6.3
> introduces MADT revision 5.
>
> For MADT revision 5, the GICC structure adds an SPE Overflow Interrupt
> field. This new 2-byte field is created from the existing 3-byte
> Reserved field. The spec indicates if the SPE overflow interrupt is
> not supported, to zero the field.
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
needs ACK from ARM folks
> ---
> hw/arm/virt-acpi-build.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 4156111d49..23268dd981 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -705,7 +705,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> int i;
> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> const MemMapEntry *memmap = vms->memmap;
> - AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
> + AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = vms->oem_id,
> .oem_table_id = vms->oem_table_id };
>
> acpi_table_begin(&table, table_data);
> @@ -763,7 +763,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> /* Processor Power Efficiency Class */
> build_append_int_noprefix(table_data, 0, 1);
> /* Reserved */
> - build_append_int_noprefix(table_data, 0, 3);
> + build_append_int_noprefix(table_data, 0, 1);
> + /* SPE overflow Interrupt */
> + build_append_int_noprefix(table_data, 0, 2);
> }
>
> if (vms->gic_version != VIRT_GIC_VERSION_2) {
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-03-28 15:59 ` [PATCH 2/2] hw/acpi: i386: " Eric DeVolder
2023-03-29 5:03 ` Michael S. Tsirkin
@ 2023-04-11 16:00 ` Igor Mammedov
2023-04-12 7:58 ` Igor Mammedov
1 sibling, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2023-04-11 16:00 UTC (permalink / raw)
To: Eric DeVolder
Cc: shannon.zhaosl, mst, ani, peter.maydell, qemu-arm, qemu-devel,
marcel.apfelbaum, pbonzini, richard.henderson, eduardo,
boris.ostrovsky
On Tue, 28 Mar 2023 11:59:26 -0400
Eric DeVolder <eric.devolder@oracle.com> wrote:
> Currently i386 QEMU generates MADT revision 3, and reports
> MADT revision 1. ACPI 6.3 introduces MADT revision 5.
>
> For MADT revision 4, that introduces ARM GIC structures, which do
> not apply to i386.
>
> For MADT revision 5, the Local APIC flags introduces the Online
> Capable bitfield.
>
> Making MADT generate and report revision 5 will solve problems with
> CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
So spec mandates 3 possible states
00t - not present and not can't be added later ever
01t - present
10t - not present but might be added later
and outlawed 11t combination
00t - doesn't make much sense (i.e. why put such entry in MADT in the 1st place)
but looking at kernel commit aa06e20f1be, it looks like
ACPI_MADT_ONLINE_CAPABLE was introduced to accommodate
firmware/hw folks who would stuff MADT with LAPIC entries
for all possible CPU models, and then patch it depending on
actually used CPU model instead of dynamically creating LAPIC
entries. (insane)
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
> hw/i386/acpi-common.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> index 52e5c1439a..1e3a13a36c 100644
> --- a/hw/i386/acpi-common.c
> +++ b/hw/i386/acpi-common.c
> @@ -38,8 +38,15 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
> {
> uint32_t apic_id = apic_ids->cpus[uid].arch_id;
> /* Flags – Local APIC Flags */
> - uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> - 1 /* Enabled */ : 0;
> + bool enabled = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> + true /* Enabled */ : false;
> + /*
> + * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
> + * if Enabled is set.
> + */
> + bool onlinecapable = enabled ? false : true; /* Online Capable */
> + uint32_t flags = onlinecapable ? 0x2 : 0x0 |
> + enabled ? 0x1 : 0x0;
align the last line with onlinecapable ....'
move /* Enabled */ and /* Online Capable */ comments right to magic values
i.e. onlinecapable ? 0x2 : 0x0 | /* Online Capable */ ...
>
> /* ACPI spec says that LAPIC entry for non present
> * CPU may be omitted from MADT or it must be marked
> @@ -102,7 +109,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> MachineClass *mc = MACHINE_GET_CLASS(x86ms);
> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
> AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
> - AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
> + AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = oem_id,
> .oem_table_id = oem_table_id };
>
> acpi_table_begin(&table, table_data);
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-04-11 16:00 ` Igor Mammedov
@ 2023-04-12 7:58 ` Igor Mammedov
2023-04-18 16:58 ` Eric DeVolder
0 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2023-04-12 7:58 UTC (permalink / raw)
To: Eric DeVolder
Cc: shannon.zhaosl, mst, ani, peter.maydell, qemu-arm, qemu-devel,
marcel.apfelbaum, pbonzini, richard.henderson, eduardo,
boris.ostrovsky
On Tue, 11 Apr 2023 18:00:49 +0200
Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 28 Mar 2023 11:59:26 -0400
> Eric DeVolder <eric.devolder@oracle.com> wrote:
>
> > Currently i386 QEMU generates MADT revision 3, and reports
> > MADT revision 1. ACPI 6.3 introduces MADT revision 5.
> >
> > For MADT revision 4, that introduces ARM GIC structures, which do
> > not apply to i386.
> >
> > For MADT revision 5, the Local APIC flags introduces the Online
> > Capable bitfield.
> >
> > Making MADT generate and report revision 5 will solve problems with
> > CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
>
> So spec mandates 3 possible states
> 00t - not present and not can't be added later ever
> 01t - present
> 10t - not present but might be added later
> and outlawed 11t combination
>
> 00t - doesn't make much sense (i.e. why put such entry in MADT in the 1st place)
>
> but looking at kernel commit aa06e20f1be, it looks like
> ACPI_MADT_ONLINE_CAPABLE was introduced to accommodate
> firmware/hw folks who would stuff MADT with LAPIC entries
> for all possible CPU models, and then patch it depending on
> actually used CPU model instead of dynamically creating LAPIC
> entries. (insane)
on second thought, QEMU doesn't need rev 5 MADT with this flag complications.
Also I see that kernel side fix ended up in checking ACPI spec version instead
of dealing with MADT revisions mess.
So for x86 lets bump revision to 3 or 4 to be in sync with
what QEMU actually uses.
>
>
> > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > ---
> > hw/i386/acpi-common.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> > index 52e5c1439a..1e3a13a36c 100644
> > --- a/hw/i386/acpi-common.c
> > +++ b/hw/i386/acpi-common.c
> > @@ -38,8 +38,15 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
> > {
> > uint32_t apic_id = apic_ids->cpus[uid].arch_id;
> > /* Flags – Local APIC Flags */
> > - uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> > - 1 /* Enabled */ : 0;
> > + bool enabled = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> > + true /* Enabled */ : false;
> > + /*
> > + * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
> > + * if Enabled is set.
> > + */
> > + bool onlinecapable = enabled ? false : true; /* Online Capable */
>
> > + uint32_t flags = onlinecapable ? 0x2 : 0x0 |
> > + enabled ? 0x1 : 0x0;
> align the last line with onlinecapable ....'
>
> move /* Enabled */ and /* Online Capable */ comments right to magic values
> i.e. onlinecapable ? 0x2 : 0x0 | /* Online Capable */ ...
>
> >
> > /* ACPI spec says that LAPIC entry for non present
> > * CPU may be omitted from MADT or it must be marked
> > @@ -102,7 +109,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> > MachineClass *mc = MACHINE_GET_CLASS(x86ms);
> > const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
> > AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
> > - AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
> > + AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = oem_id,
> > .oem_table_id = oem_table_id };
> >
> > acpi_table_begin(&table, table_data);
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] hw/acpi: i386: bump MADT to revision 5
2023-04-12 7:58 ` Igor Mammedov
@ 2023-04-18 16:58 ` Eric DeVolder
0 siblings, 0 replies; 22+ messages in thread
From: Eric DeVolder @ 2023-04-18 16:58 UTC (permalink / raw)
To: Igor Mammedov
Cc: shannon.zhaosl, mst, ani, peter.maydell, qemu-arm, qemu-devel,
marcel.apfelbaum, pbonzini, richard.henderson, eduardo,
boris.ostrovsky
On 4/12/23 02:58, Igor Mammedov wrote:
> On Tue, 11 Apr 2023 18:00:49 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
>
>> On Tue, 28 Mar 2023 11:59:26 -0400
>> Eric DeVolder <eric.devolder@oracle.com> wrote:
>>
>>> Currently i386 QEMU generates MADT revision 3, and reports
>>> MADT revision 1. ACPI 6.3 introduces MADT revision 5.
>>>
>>> For MADT revision 4, that introduces ARM GIC structures, which do
>>> not apply to i386.
>>>
>>> For MADT revision 5, the Local APIC flags introduces the Online
>>> Capable bitfield.
>>>
>>> Making MADT generate and report revision 5 will solve problems with
>>> CPU hotplug (the Online Capable flag indicates hotpluggable CPUs).
>>
>> So spec mandates 3 possible states
>> 00t - not present and not can't be added later ever
>> 01t - present
>> 10t - not present but might be added later
>> and outlawed 11t combination
>>
>> 00t - doesn't make much sense (i.e. why put such entry in MADT in the 1st place)
>>
>> but looking at kernel commit aa06e20f1be, it looks like
>> ACPI_MADT_ONLINE_CAPABLE was introduced to accommodate
>> firmware/hw folks who would stuff MADT with LAPIC entries
>> for all possible CPU models, and then patch it depending on
>> actually used CPU model instead of dynamically creating LAPIC
>> entries. (insane)
>
> on second thought, QEMU doesn't need rev 5 MADT with this flag complications.
> Also I see that kernel side fix ended up in checking ACPI spec version instead
> of dealing with MADT revisions mess.
>
> So for x86 lets bump revision to 3 or 4 to be in sync with
> what QEMU actually uses.
If bumping to only 3 or 4, then there is no need for this patch series.
>
>>
>>
>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>>> ---
>>> hw/i386/acpi-common.c | 13 ++++++++++---
>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
>>> index 52e5c1439a..1e3a13a36c 100644
>>> --- a/hw/i386/acpi-common.c
>>> +++ b/hw/i386/acpi-common.c
>>> @@ -38,8 +38,15 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
>>> {
>>> uint32_t apic_id = apic_ids->cpus[uid].arch_id;
>>> /* Flags – Local APIC Flags */
>>> - uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
>>> - 1 /* Enabled */ : 0;
>>> + bool enabled = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
>>> + true /* Enabled */ : false;
>>> + /*
>>> + * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
>>> + * if Enabled is set.
>>> + */
>>> + bool onlinecapable = enabled ? false : true; /* Online Capable */
>>
>>> + uint32_t flags = onlinecapable ? 0x2 : 0x0 |
>>> + enabled ? 0x1 : 0x0;
>> align the last line with onlinecapable ....'
>>
>> move /* Enabled */ and /* Online Capable */ comments right to magic values
>> i.e. onlinecapable ? 0x2 : 0x0 | /* Online Capable */ ...
>>
Done.
I've gone ahead and posted a v2 with these changes; keeping MADT.revision at 5.
eric
>>>
>>> /* ACPI spec says that LAPIC entry for non present
>>> * CPU may be omitted from MADT or it must be marked
>>> @@ -102,7 +109,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
>>> MachineClass *mc = MACHINE_GET_CLASS(x86ms);
>>> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
>>> AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
>>> - AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
>>> + AcpiTable table = { .sig = "APIC", .rev = 5, .oem_id = oem_id,
>>> .oem_table_id = oem_table_id };
>>>
>>> acpi_table_begin(&table, table_data);
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-04-18 16:59 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-28 15:59 [PATCH 0/2] hw/acpi: bump MADT to revision 5 Eric DeVolder
2023-03-28 15:59 ` [PATCH 1/2] hw/acpi: arm: " Eric DeVolder
2023-04-11 14:49 ` Igor Mammedov
2023-03-28 15:59 ` [PATCH 2/2] hw/acpi: i386: " Eric DeVolder
2023-03-29 5:03 ` Michael S. Tsirkin
2023-03-29 13:16 ` Eric DeVolder
2023-03-29 13:19 ` Eric DeVolder
2023-03-29 16:55 ` Michael S. Tsirkin
2023-03-31 16:29 ` Igor Mammedov
2023-04-11 16:00 ` Igor Mammedov
2023-04-12 7:58 ` Igor Mammedov
2023-04-18 16:58 ` Eric DeVolder
2023-03-28 16:37 ` [PATCH 0/2] hw/acpi: " Eric DeVolder
2023-03-29 5:19 ` Michael S. Tsirkin
2023-03-29 13:14 ` Eric DeVolder
2023-03-29 16:47 ` Michael S. Tsirkin
2023-03-30 7:36 ` Ani Sinha
2023-03-30 13:44 ` Michael S. Tsirkin
2023-03-30 14:02 ` Ani Sinha
2023-03-30 14:11 ` Michael S. Tsirkin
2023-03-31 16:25 ` Igor Mammedov
2023-04-04 14:52 ` Eric DeVolder
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).