Re: [PATCH v2 3/4] hw/acpi: i386: bump MADT to revision 5

[Michael S. Tsirkin]

On Thu, Apr 20, 2023 at 09:22:57AM -0500, Eric DeVolder wrote:
> 
> 
> On 4/20/23 03:05, Ani Sinha wrote:
> > On Tue, Apr 18, 2023 at 10:22 PM 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).
> > > 
> > > Link: 
> > > https://lore.kernel.org/linux-acpi/20230327191026.3454-1-eric.devolder@oracle.com/T/#t
> > > 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..286c1c5c32 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 : false;
> > 
> > how about "processor_enabled" instead of just "enabled" as the variable 
> > name.
> > 
> > > +    /*
> > > +     * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
> > > +     * if Enabled is set.
> > > +     */
> > > +    bool onlinecapable = enabled ? false : true;
> > 
> > ugh, how about uint32 onlinecapable = enabled? 0x0 : 0x2 ?
> > 
> > > +    uint32_t flags = onlinecapable ? 0x2 : 0x0 | /* Online Capable */
> > > +                     enabled ? 0x1 : 0x0; /* Enabled */
> > 
> > then here, flags = onlinecapable | processor_enabled? 0x1 : 0x0;
> > 
> 
> Colleague Miguel Luis pointed out that this is simpler and equivalent:
> 
> uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ? 1 /*
> Enabled */ : 2 /* Online Capable */;
> 
> Is that acceptable?
> eric


Looks ok to me.

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