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

[Paolo Bonzini]

On 19/07/2016 15:29, Igor Mammedov wrote:
> On Tue, 19 Jul 2016 14:21:05 +0200
> Paolo Bonzini  wrote:
> 
>> On 19/07/2016 13:59, Eduardo Habkost wrote:
>>> If it's internal, do we have any reason to register a (writeable)
>>> property in the first place? Why not use a plain old
>>> "obj->field = value" C statement? Or, if a simple assignment
>>> isn't enough, why not a simple obj_set_field(value) C function?  
> So that arch neutral code won't have to pull obj type definition  
>>>
>>> I don't get it. If arch neutral code uses it, it should be
>>> available in an arch-neutral header.  
>>
>> I agree.  If arch-neutral code uses it, the method should be in CPUClass.
>
> it looks a bit like deQOMification, but if it's preferred
> we can do following for -smp.

Properties are primarily an interface for users.  Sometimes we use them
internally when they are anyway inaccessible to users, but if you want
to hide things from users there's already a suitable mechanism which is
class and interface methods.

Paolo

> 1. Add machine::{nr_cores,nr_threads,nr_sockets} fields
> 2. Add to concrete cpus classes that use above data (as globals currently)
>  a duplicate fields ex: X86CPU:{nr_cores,nr_threads}
> 3: Have X86CPU:{nr_cores,nr_threads} fields set buy PCMachine::pre_plug{} 
> handler
> 
> That way we'd have a bit of data duplication in X86CPU:{nr_cores,nr_threads}
> but still maintain role separation where CPUs won't have to to poke into
> its parent containers (machine). The sort of what we are doing currently with 
> apic-id.
> 
> 
>>
>> Paolo
>>
> and we would be able to reuse all machinery that uses properties
> instead of inventing yet another API or ad-hoc function calls.  
>>> Why is adding a new C function or setting a struct field worse
>>> than adding a new property name? I actually prefer the former,
>>> because it makes code review easier and allows the compiler to
>>> detect more mistakes.  
> 

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

[Igor Mammedov]

On Tue, 19 Jul 2016 14:21:05 +0200
Paolo Bonzini  wrote:

> On 19/07/2016 13:59, Eduardo Habkost wrote:
> >>> > > If it's internal, do we have any reason to register a (writeable)
> >>> > > property in the first place? Why not use a plain old
> >>> > > "obj->field = value" C statement? Or, if a simple assignment
> >>> > > isn't enough, why not a simple obj_set_field(value) C function?  
> >> > So that arch neutral code won't have to pull obj type definition  
> > 
> > I don't get it. If arch neutral code uses it, it should be
> > available in an arch-neutral header.  
> 
> I agree.  If arch-neutral code uses it, the method should be in CPUClass.
it looks a bit like deQOMification, but if it's preferred
we can do following for -smp.

1. Add machine::{nr_cores,nr_threads,nr_sockets} fields
2. Add to concrete cpus classes that use above data (as globals currently)
 a duplicate fields ex: X86CPU:{nr_cores,nr_threads}
3: Have X86CPU:{nr_cores,nr_threads} fields set buy PCMachine::pre_plug{} 
handler

That way we'd have a bit of data duplication in X86CPU:{nr_cores,nr_threads}
but still maintain role separation where CPUs won't have to to poke into
its parent containers (machine). The sort of what we are doing currently with 
apic-id.


> 
> Paolo
> 
> >> > and we would be able to reuse all machinery that uses properties
> >> > instead of inventing yet another API or ad-hoc function calls.  
> > Why is adding a new C function or setting a struct field worse
> > than adding a new property name? I actually prefer the former,
> > because it makes code review easier and allows the compiler to
> > detect more mistakes.  

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

[Paolo Bonzini]

On 19/07/2016 13:59, Eduardo Habkost wrote:
>>> > > If it's internal, do we have any reason to register a (writeable)
>>> > > property in the first place? Why not use a plain old
>>> > > "obj->field = value" C statement? Or, if a simple assignment
>>> > > isn't enough, why not a simple obj_set_field(value) C function?
>> > So that arch neutral code won't have to pull obj type definition
> 
> I don't get it. If arch neutral code uses it, it should be
> available in an arch-neutral header.

I agree.  If arch-neutral code uses it, the method should be in CPUClass.

Paolo

>> > and we would be able to reuse all machinery that uses properties
>> > instead of inventing yet another API or ad-hoc function calls.
> Why is adding a new C function or setting a struct field worse
> than adding a new property name? I actually prefer the former,
> because it makes code review easier and allows the compiler to
> detect more mistakes.

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

[Eduardo Habkost]

On Mon, Jul 18, 2016 at 09:23:04AM +0200, Igor Mammedov wrote:
> On Fri, 15 Jul 2016 18:33:53 -0300
> Eduardo Habkost  wrote:
> 
> > On Fri, Jul 15, 2016 at 08:38:35PM +0200, Igor Mammedov wrote:
> > > On Fri, 15 Jul 2016 14:43:53 -0300
> > > Eduardo Habkost  wrote:  
> > > > On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas Färber wrote:  
> > > > > Am 15.07.2016 um 18:10 schrieb 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  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 

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

[Eduardo Habkost]

On Sat, Jul 16, 2016 at 05:30:04PM +0200, Andrew Jones wrote:
> On Fri, Jul 15, 2016 at 06:33:53PM -0300, Eduardo Habkost wrote:
> > On Fri, Jul 15, 2016 at 08:38:35PM +0200, Igor Mammedov wrote:
> > > On Fri, 15 Jul 2016 14:43:53 -0300
> > > Eduardo Habkost  wrote:
> > > > On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas Färber wrote:
> > > > > Am 15.07.2016 um 18:10 schrieb 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  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 

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

[Igor Mammedov]

On Fri, 15 Jul 2016 18:33:53 -0300
Eduardo Habkost  wrote:

> On Fri, Jul 15, 2016 at 08:38:35PM +0200, Igor Mammedov wrote:
> > On Fri, 15 Jul 2016 14:43:53 -0300
> > Eduardo Habkost  wrote:  
> > > On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas Färber wrote:  
> > > > Am 15.07.2016 um 18:10 schrieb 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  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
> > > > 

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

[Andrew Jones]

On Fri, Jul 15, 2016 at 06:33:53PM -0300, Eduardo Habkost wrote:
> On Fri, Jul 15, 2016 at 08:38:35PM +0200, Igor Mammedov wrote:
> > On Fri, 15 Jul 2016 14:43:53 -0300
> > Eduardo Habkost  wrote:
> > > On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas Färber wrote:
> > > > Am 15.07.2016 um 18:10 schrieb 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  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,
> > > 

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

[Eduardo Habkost]

On Fri, Jul 15, 2016 at 08:38:35PM +0200, Igor Mammedov wrote:
> On Fri, 15 Jul 2016 14:43:53 -0300
> Eduardo Habkost  wrote:
> > On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas Färber wrote:
> > > Am 15.07.2016 um 18:10 schrieb 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  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 

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

[Igor Mammedov]

On Fri, 15 Jul 2016 14:43:53 -0300
Eduardo Habkost  wrote:

> On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas Färber wrote:
> > Am 15.07.2016 um 18:10 schrieb 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  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 

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

[Eduardo Habkost]

On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas Färber wrote:
> Am 15.07.2016 um 18:10 schrieb 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  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 

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

[Andreas Färber]

Am 15.07.2016 um 18:10 schrieb 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  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
   

[Qemu-devel] QOM: best way for parents to pass information to children? (was Re: [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  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