Re: [PATCH 0/2] hw/acpi: bump MADT to revision 5

[Michael S. Tsirkin]

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
> > > 
> > > 
> > > 
> >