Re: [Qemu-devel] [PATCH v3 04/13] acpi: cphp: force switch to modern cpu hotplug if APIC ID > 254

[Igor Mammedov]

On Tue, 18 Oct 2016 11:38:31 -0200
Eduardo Habkost <address@hidden> wrote:

> On Thu, Oct 13, 2016 at 11:52:38AM +0200, Igor Mammedov wrote:
> > Switch to modern cpu hotplug at machine startup time if
> > a cpu present at boot has apic-id in range unsupported
> > by legacy cpu hotplug interface (i.e. > 254), to avoid
> > killing QEMU from legacy cpu hotplug code with error:
> >    "acpi: invalid cpu id: #apic-id#"
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  hw/acpi/cpu_hotplug.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > index e19d902..c2ab9b8 100644
> > --- a/hw/acpi/cpu_hotplug.c
> > +++ b/hw/acpi/cpu_hotplug.c
> > @@ -63,7 +63,8 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, 
> > CPUState *cpu,
> >  
> >      cpu_id = k->get_arch_id(cpu);
> >      if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
> > -        error_setg(errp, "acpi: invalid cpu id: %" PRIi64, cpu_id);
> > +        object_property_set_bool(g->device, false, "cpu-hotplug-legacy",
> > +                                 &error_abort);  
> 
> What happens we are in legacy mode and this is triggered during
> hotplug instead of machine init? Would it break something, or is
> it safe?
 case 1: guest with legacy hotplug AML (migrated for example) would use
         legacy interface and it won't be possible to trigger this path
         as target should be started with the same CLI as source
         (hence < 255 cpus)
 case 2: guest started on new QEMU will have new hotplug AML which switches
         QEMU to modern cpu hotplug interface at ACPI tables _INI time
         so this path is unreachable.

Originally it's been static rule:
    since 2.7 use new hotplug interface and old one for older machine types
Well it's complex but Michael insisted on keeping legacy hotplug
by default and do dynamic switching, so here we are.

this behavior is since 2.7:
    commit 679dd1a957df418453efdd3ed2914dba5cd73773
    pc: use new CPU hotplug interface since 2.7 machine type

 
> Unrelated to this patch: piix4_set_cpu_hotplug_legacy() has an
> assert(!value). I assume this means we must replace the QOM
> property with something that the user can't fiddle with, right?
it's readonly to user after machine starts, but allows user
to play modern hotplug interface on old machine types if needed.

assert is there to trap mistake of switching to legacy mode
(which is default) from compat_properties.

> 
> >          return;
> >      }
> >  
> > @@ -85,13 +86,14 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, 
> > Object *owner,
> >  {
> >      CPUState *cpu;
> >  
> > -    CPU_FOREACH(cpu) {
> > -        acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort);
> > -    }
> >      memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
> >                            gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
> >      memory_region_add_subregion(parent, base, &gpe_cpu->io);
> >      gpe_cpu->device = owner;
> > +
> > +    CPU_FOREACH(cpu) {
> > +        acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort);
> > +    }
> >  }
> >  
> >  void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
> > -- 
> > 2.7.4
> >   
>