qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 04/13] acpi: cphp: force switch to modern cpu


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH v3 04/13] acpi: cphp: force switch to modern cpu hotplug if APIC ID > 254
Date: Tue, 18 Oct 2016 14:37:39 -0200
User-agent: Mutt/1.7.0 (2016-08-17)

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?

> > >   
> > 
> > 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).

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?

> 
> > 
> > > 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().

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]