Re: [Qemu-ppc] [Qemu-devel] [RFC PATCH v1 05/10] cpu: Abstract CPU core type

[Igor Mammedov]

On Thu, 10 Mar 2016 10:42:26 +1100
David Gibson <address@hidden> wrote:

> On Wed, Mar 09, 2016 at 11:40:53AM +0100, Igor Mammedov wrote:
> > On Tue, 8 Mar 2016 15:26:27 +1100
> > David Gibson <address@hidden> wrote:  
> > > On Mon, Mar 07, 2016 at 11:29:29AM +0100, Igor Mammedov wrote:  
> > > > On Mon, 7 Mar 2016 14:36:55 +1100
> > > > David Gibson <address@hidden> wrote:  
> [snip]
> > > > > > > > on top of that I'd add numeric 'threads' property to base class 
> > > > > > > > so
> > > > > > > > all derived cores would inherit it.
> > > > > > > > 
> > > > > > > > Then as easy integration with -smp threads=x, a machine could 
> > > > > > > > push
> > > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > > make every created cpu-core object to have threads set
> > > > > > > > at instance_init() time (device_init).
> > > > > > > > 
> > > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > > >   device_add spapr-core,core=x
> > > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > > but if user wishes he/she still could override global by 
> > > > > > > > explicitly
> > > > > > > > providing thread property at device_add time:
> > > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > > 
> > > > > > > > wrt this series it would mean, instead of creating threads in 
> > > > > > > > property
> > > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > > but since realize is allowed to fail it should be fine do so.   
> > > > > > > >      
> > > > > > > 
> > > > > > > Ok that would suit us as there are two properties on which thread 
> > > > > > > creation
> > > > > > > is dependent upon: nr_threads and cpu_model. If thread objects 
> > > > > > > can be
> > > > > > > created at core realize time, then we don't have to resort to the 
> > > > > > > ugliness
> > > > > > > of creating the threads from either of the property setters. I 
> > > > > > > always
> > > > > > > assumed that we shouldn't be creating objects from realize, but 
> > > > > > > if that
> > > > > > > is fine, it is good.      
> > > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > > to create internal objects there, as far as proper cleanups are done
> > > > > > for failure path.      
> > > > >     
> > > > [...]    
> > > > > I'm not clear from the above if you're also intending to move at least
> > > > > the adding of the threads as child properties is supposed to go into
> > > > > the base type,    
> > > > I'm not sure that I've got question, could you please rephrase?    
> > > 
> > > So, it seems like we're agreed that moving the nr_threads property to
> > > the base type is a good idea.
> > > 
> > > My question is, do we also move the object_property_add_child() calls
> > > for each thread to the base type (possibly via a helper function or
> > > method the base type provides to derived types)?  
> > I can't think of a reason to do so,
> > why can't subtype-core.realize() do it?  
> 
> It can, but I'm always suspicious of boilerplate stuff that every
> subtype *has* to do in order to work properly.
> 
> > What would one gain creating callbacks and calling them from base class?  
> 
> Enforcing - or at least making as easy as possible - consistency in
> the child object naming.
> 

I guess a patch would help to understand if it's worth of effort of adding
extra callbacks in base class.