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

[Igor Mammedov]

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