| Message ID | 20230418165219.2036-4-eric.devolder@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/acpi: bump MADT to revision 5 | expand |
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
On 4/19/23 09:56, Michael S. Tsirkin wrote: > 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. > Will do! >> + 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 >
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; > > /* 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 >
On Thu, 20 Apr 2023 13:35:58 +0530 Ani Sinha <ani@anisinha.ca> 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; onlinecapable is unnecessary complication. See my reply on v1, we don't really have a reason to bump MADT revision to 5, It brings no value whatsoever. (and kernel fix went another route instead of dealing with messed up MADT revision numbering in spec) All we need is to just bump revision to 3 or 4 to match actually used features. > > > > > /* 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 > > >
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 >> >> /* 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 >>
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 > > >
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; + 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);
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(-)