[Qemu-arm] QOM: best way for parents to pass information to children? (was Re: [Qemu-devel] [PATCH RFC 07/16] qom/cpu: make nr-cores, nr-threads real properties)

[Eduardo Habkost]

On Fri, Jul 15, 2016 at 11:11:38AM +0200, Igor Mammedov wrote:
> On Fri, 15 Jul 2016 08:35:30 +0200
> Andrew Jones <address@hidden> wrote:
> > On Thu, Jul 14, 2016 at 05:07:43PM -0300, Eduardo Habkost wrote:
> > > 
> > > First of all, sorry for the horrible delay in replying to this
> > > thread.
> > > 
> > > On Wed, Jun 15, 2016 at 10:56:20AM +1000, David Gibson wrote:  
> > > > On Tue, Jun 14, 2016 at 08:19:49AM +0200, Andrew Jones wrote:  
> > > > > On Tue, Jun 14, 2016 at 12:12:16PM +1000, David Gibson wrote:  
> > > > > > On Sun, Jun 12, 2016 at 03:48:10PM +0200, Andrew Jones wrote:  
[...]
> > > > > > > > > +static Property cpu_common_properties[] = {
> > > > > > > > > +    DEFINE_PROP_INT32("nr-cores", CPUState, nr_cores, 1),
> > > > > > > > > +    DEFINE_PROP_INT32("nr-threads", CPUState, nr_threads, 1),
> > > > > > > > > +    DEFINE_PROP_END_OF_LIST()
> > > > > > > > > +};  
> > > > > > > > 
> > > > > > > > Are you aware of the current CPU hotplug discussion that is 
> > > > > > > > going on?  
> > > > > > > 
> > > > > > > I'm aware of it going on, but haven't been following it.
> > > > > > >   
> > > > > > > > I'm not very involved there, but I think some of these reworks 
> > > > > > > > also move
> > > > > > > > "nr_threads" into the CPU state already, e.g. see:  
> > > > > > > 
> > > > > > > nr_threads (and nr_cores) are already state in CPUState. This 
> > > > > > > patch just
> > > > > > > exposes that state via properties.
> > > > > > >   
> > > > > > > > 
> > > > > > > > https://github.com/dgibson/qemu/commit/9d07719784ecbeebea71
> > > > > > > > 
> > > > > > > > ... so you might want to check these patches first to see 
> > > > > > > > whether you
> > > > > > > > can base your rework on them?  
> > > > > > > 
> > > > > > > Every cpu, and thus every machine, uses CPUState for its cpus. I'm
> > > > > > > not sure every machine will want to use that new abstract core 
> > > > > > > class
> > > > > > > though. If they did, then we could indeed use nr_threads from 
> > > > > > > there
> > > > > > > instead (and remove it from CPUState), but we'd still need 
> > > > > > > nr_cores
> > > > > > > from the abstract cpu package class (CPUState).  
> > > > > > 
> > > > > > Hmm.  Since the CPUState object represents just a single thread, it
> > > > > > seems weird to me that it would have nr_threads and nr_cores
> > > > > > information.  
> > > 
> > > Agreed it is weird, and I think we should try to move it away
> > > from CPUState, not make it part of the TYPE_CPU interface.
> > > nr_threads belongs to the actual container of the Thread objects,
> > > and nr_cores in the actual container of the Core objects.
> > > 
> > > The problem is how to implement that in a non-intrusive way that
> > > would require changing the object hierarchy of all architectures.
> > > 
> > >   
> > > > > > 
> > > > > > Exposing those as properties makes that much worse, because it's now
> > > > > > ABI, rather than internal detail we can clean up at some future 
> > > > > > time.  
> > > > > 
> > > > > CPUState is supposed to be "State of one CPU core or thread", which
> > > > > justifies having nr_threads state, as it may be describing a core.  
> > > > 
> > > > Um.. does it ever actually represent a (multithread) core in practice?
> > > > It would need to have duplicated register state for every thread were
> > > > that the case.  
> > > 
> > > AFAIK, CPUState is still always thread state. Or has this changed
> > > in some architectures, already?
> > >   
> > > >   
> > > > > I guess there's no justification for having nr_cores in there though.
> > > > > I agree adding the Core class is a good idea, assuming it will get 
> > > > > used
> > > > > by all machines, and CPUState then gets changed to a Thread class. The
> > > > > question then, though, is do we also create a Socket class that 
> > > > > contains
> > > > > nr_cores?  
> > > > 
> > > > That was roughly our intention with the way the cross platform hotplug
> > > > stuff is evolving.  But the intention was that the Socket objects
> > > > would only need to be constructed for machine types where it makes
> > > > sense.  So for example on the paravirt pseries platform, we'll only
> > > > have Core objects, because the socket distinction isn't really
> > > > meaningful.
> > > >   
> > > > > And how will a Thread method get that information when it
> > > > > needs to emulate, e.g. CPUID, that requires it? It's a bit messy, so
> > > > > I'm open to all suggestions on it.  
> > > > 
> > > > So, if the Thread needs this information, I'm not opposed to it having
> > > > it internally (presumably populated earlier from the Core object).
> > > > But I am opposed to it being a locked in part of the interface by
> > > > having it as an exposed property.  
> > > 
> > > I agree we don't want to make this part of the external
> > > interface. In this case, if we don't add the properties, how
> > > exactly is the Machine or Core code supposed to pass that
> > > information to the Thread object?
> > > 
> > > Maybe the intermediate steps could be:
> > > 
> > > * Make the Thread code that uses CPUState::nr_{cores,threads} and
> > >   smp_{cores,threads} get that info from MachineState instead.  
> > 
> > I have some patches already headed down this road.
> > 
> > > * On the architectures where we already have a reasonable
> > >   Socket/Core/Thread hierarchy, let the Thread code simply ask
> > >   for that information from its parent.  
> > 
> > I guess that's just spapr so far, or at least spapr is the closest.
> > Indeed this appears to be the cleanest approach, so architectures
> > adding support for cpu topology should likely strive to implement it
> > this way.
> If I recall correctly, the only thing about accessing parent is that
> in QOM design accessing parent from child wasn't accepted well,
> i.e. child shouldn't be aware nor access parent object.

Can anybody explain why?

In this case, what's the best way for a parent to pass
information to its children without adding new externally-visible
properties that the user is never supposed to set directly?

Should Thread objects have an additional link to the parent Core
object, just to be able to get the information it needs?

-- 
Eduardo