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

[Eric DeVolder]

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