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.devol...@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
> > > >
> > > >
> > > >
> > >
