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 14:37:39 -0200
Eduardo Habkost <address@hidden> wrote:

> On Tue, Oct 18, 2016 at 05:23:04PM +0200, Igor Mammedov wrote:
> > On Tue, 18 Oct 2016 13:05:51 -0200
> > Eduardo Habkost <address@hidden> wrote:
> >   
> > > On Tue, Oct 18, 2016 at 04:34:53PM +0200, Igor Mammedov wrote:  
> > > > 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.    
> > > 
> > > I see. Thanks for the explanation!
> > >   
> > > > 
> > > > 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.  
> 
> Do you rememer the reasoning for that?
Ancient SeaBIOS that doesn't get ACPI tables from QEMU supports only
legacy interface. So it's been argued that new machines should start
in legacy mode so that CPU hotplug would be operational with old SeaBIOS.
That's not applicable in practice as QEMU with new hotplug support ships
with SeaBIOS supporting ACPI tables from QEMU.
So the only case, when it could happen, is user overrides default bios
image with old one for purposes of debugging old bios with new machine
type. (that user could have used old machine type for that purpose and
we could have kept code simpler, but train already left and we are
stuck with supporting dynamic switching).

> > > >     
> > > 
> > > This means PCMachineClass::legacy_cpu_hotplug means "legacy CPU
> > > hotplug _only_", because legacy CPU hotplug is always available
> > > on startup, right?  
> > Sorry, I can't parse question, could you rephrase?  
> 
> I was just trying to clarify the meaning of
> PCMachineClass::legacy_cpu_hotplug.
> 
> I thought legacy CPU hotplug was available only if
> PCMC::legacy_cpu_hotplug was set. But legacy hotplug is still
> available even on pc-2.7 (and then switched dynamically).
both hoptlug methods available even on pre pc-2.7 machines,
and legacy hotplug is used by default. User can one way switch
mode to modern hotplug by setting cpu-hotplug-legacy prop to false
which is done by writing 0 at the beginning of legacy CPU bitmap
from AML by guest.

> 
> So the difference is that PCMC::legacy_cpu_hotplug only disables
> the ability to dynamically switch to modern hotplug, but doesn't
> disable legacy hotplug completely. Correct?
yep,
more exactly PCMC::legacy_cpu_hotplug picks whether legacy or modern
AML code should be generated by QEMU.

After guest is switched to modern mode, it can't switch back
'abort(!value)' takes care about that.

> 
> >   
> > >   
> > > > 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.    
> > > 
> > > What exactly makes the property read-only to the user? Maybe we
> > > should make the setter return an error instead, as all
> > > object_property_set_bool("cpu-hotplug-legacy") calls already use
> > > &error_abort?  
> > My mistake,
> > it's dynamic property with custom setters in piix4/ich9_pm and user
> > potentially can write there.
> > 
> > I was under impression that errors are ignored if property comes from
> > compat_props, if returning error would prevent machine from starting
> > when property comes from compat_props I can fix cpu-hotplug-legacy to
> > return error.  
> 
> compat_props set errp to &error_abort, see
> machine_register_compat_props().
Than I guess that assert could be replaced by error_setg(),
I'll post a separate patch for this (as it's unrelated to this series).