| Message ID | 20230328155926.2277-3-eric.devolder@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/acpi: bump MADT to revision 5 | expand |
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. > --- > 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 > > >
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 >> >> >> >
On 3/29/23 08:16, Eric DeVolder wrote: > > > 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 > Actually, if, for example, one had -smp 30,maxcpus=32, then there would be two hotpluggable cpus reported, the last two with the Enabled=0 and Online Capable=1. If one had -smp 32 (ie "cpu hotplug off"), then all cpus would be reported as Enabled and no cpu would have its Online Capable flag set. Granted in both cases, MADT.revision would report 5, but it would still be accurate. 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 >>> >>> >>> >>
On Wed, Mar 29, 2023 at 08:19:22AM -0500, Eric DeVolder wrote: > > > On 3/29/23 08:16, Eric DeVolder wrote: > > > > > > 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 > > > > Actually, if, for example, one had -smp 30,maxcpus=32, then there would be > two hotpluggable cpus reported, the last two with the Enabled=0 and Online > Capable=1. If one had -smp 32 (ie "cpu hotplug off"), then all cpus would be > reported as Enabled and no cpu would have its Online Capable flag set. > > Granted in both cases, MADT.revision would report 5, but it would still be accurate. > > eric sounds good. > > > > > > > > > > > > > > > > > > > --- > > > > 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 > > > > > > > > > > > > > > >
On Wed, 29 Mar 2023 08:16:26 -0500 Eric DeVolder <eric.devolder@oracle.com> wrote: > 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 I'm not sure that it's possible disable CPU hotplug at all. even if one doesn't have maxcpus on CLI present CPUs are described as hotpluggbale and can be unplugged and re-plugged later.
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) > 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);
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); >
On 4/12/23 02:58, Igor Mammedov wrote: > 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. If bumping to only 3 or 4, then there is no need for this patch series. > >> >> >>> 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 */ ... >> Done. I've gone ahead and posted a v2 with these changes; keeping MADT.revision at 5. 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); >> >
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);
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> --- hw/i386/acpi-common.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)