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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Apr 18, 2023 at 12:52:18PM -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).
> 
> 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;
> +    /*
> +     * ACPI 6.3 5.2.12.2 Local APIC Flags: OnlineCapable must be 0
> +     * if Enabled is set.
> +     */
> +    bool onlinecapable = enabled ? false : true;

Pls write this as onlinecapable = !enabled or just open-code on the next
line - does not look like this variable adds  a lot of value.

> +    uint32_t flags = onlinecapable ? 0x2 : 0x0 | /* Online Capable */
> +                     enabled ? 0x1 : 0x0; /* Enabled */
>  
>      /* 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