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

[Eric DeVolder <eric.devolder@oracle.com>]

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

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