Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Igor Mammedov]

On Thu, 10 Mar 2016 20:15:23 +0530
Bharata B Rao <address@hidden> wrote:

> On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote:
> > On Thu, 10 Mar 2016 11:32:44 +0530
> > Bharata B Rao <address@hidden> wrote:
> >   
> > > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:  
> > > > On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:    
> > > > > On Tue, 8 Mar 2016 20:04:12 +0530
> > > > > Bharata B Rao <address@hidden> wrote:
> > > > >     
> > > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:    
> > > > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > > > ---
> > > > > > > replaced link set check removed in previous patch
> > > > > > > ---
> > > > > > >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> > > > > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > > index 6890a44..db33c29 100644
> > > > > > > --- a/hw/ppc/spapr.c
> > > > > > > +++ b/hw/ppc/spapr.c
> > > > > > > @@ -2297,6 +2297,27 @@ void 
> > > > > > > *spapr_populate_hotplug_cpu_dt(DeviceState *dev, CPUState *cs,
> > > > > > >      return fdt;
> > > > > > >  }
> > > > > > > 
> > > > > > > +static void spapr_machine_device_pre_plug(HotplugHandler 
> > > > > > > *hotplug_dev,
> > > > > > > +                                          DeviceState *dev, 
> > > > > > > Error **errp)
> > > > > > > +{
> > > > > > > +    sPAPRMachineClass *smc = 
> > > > > > > SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > > > > +
> > > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > > > > +        int core = object_property_get_int(OBJECT(dev), 
> > > > > > > CPU_CORE_ID_PROP,
> > > > > > > +                                           &error_abort);
> > > > > > > +
> > > > > > > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > > > > +            error_setg(errp, "CPU hotplug not supported for this 
> > > > > > > machine");
> > > > > > > +            return;
> > > > > > > +        }
> > > > > > > +        if (spapr->cores[core]) {
> > > > > > > +            error_setg(errp, "core %d is already present", core);
> > > > > > > +            return;
> > > > > > > +        }      
> > > > > > 
> > > > > > Wondering why can't we do the above check from core's realizefn and 
> > > > > > fail
> > > > > > the core hotplug from realizefn ?    
> > > > > that's rather simple, in ideal QOM world child shouldn't
> > > > > poke into parents internal if it could be helped.
> > > > > So hook provides responsibility separation where
> > > > > board/or something else(HotplugHandler) can do a necessary
> > > > > wiring of a component which is being hotplugged, without
> > > > > forcing hotplugged device being aware about it.    
> > > > 
> > > > Oh.. yes.  Sorry, somehow I got confused and thought you were
> > > > suggesting a 'pre_realize()' method on the *object* rather than a
> > > > pre_plug hotplughandler hook.
> > > >     
> > > > > That's what HotplugHandler->plug callback is doing for
> > > > > post realize and HotplugHandler->pre_plug will do similar
> > > > > thing but allowing board to execute preliminary tasks
> > > > > (like check/set properties, amend its internal state)
> > > > > before object is realized.    
> > > >     
> > > > > That will make realize() cleaner as it won't have to hack
> > > > > into data it shouldn't and would prevent us calling unrealize()
> > > > > if we were to check it later at HotplugHandler->plug time.
> > > > > (i.e. realize() won't even have a chance to introduce side
> > > > > effects that should be undone with unlealize())    
> > > > 
> > > > Hmm.. how big a deal is it to roll back from the existing plug()
> > > > handler?  
> > realize shouldn't complete without error if object properties are
> > wrong /for ex: i.e. you create kvm vcpu thread, configure it
> > as already existing vcpu and have a lot fun afterwards/.
> > 
> > For example: now on x86 we do duplicate CPU check wrong way
> > by checking for duplicate of apic property from CPU code by
> > looping through existing CPUs. Instead it would be much cleaner
> > to move that check to machine which owns apic id assignment
> > and make it check for duplicate in pre_plug() handler.
> > 
> >   
> > > Since plug() handler is post-realize, rolling back involves
> > > deleting the threads of the core we created and finally deleting the core
> > > itself.  
> > Even rolling back will leave some after effects, like created
> > KVM VCPU thread which can't be deleted and who know what else.
> >   
> > >We aleady do this kind of roll back when core hotplug is attemptedi
> > > on machine type version that don't support hotplug.  
> > that's seems to be wrong, it shouldn't even come to cpu.realize()
> > if hotplug is not supported.  
> 
> Hmm that's how we dis-allowed memory hotplug on machine type versions
> that don't support memory hotplug, i,e., we failed hotplug from
> ->plug() handler. So ->pre_plug() seems to be the ideal place for  
> such early failures ?
It sure looks like it's.
most of pc_dimm_memory_plug() should be done at pre_plug()
except of following hunk which should stay a part of plug()

pc_dimm_memory_plug():
...
    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
    vmstate_register_ram(mr, dev);
    numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
...

Ok, I'll try to repost QMP series on top of the latest SPAPR today,
taking in account libvirt feedback I've got this week.

> 
> Regards,
> Bharata.
> 
>