Re: [Qemu-devel] [PATCH RFC 07/16] qom/cpu: make nr-cores, nr-threads real properties

[Andrew Jones]

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:
> > On Sat, Jun 11, 2016 at 08:54:35AM +0200, Thomas Huth wrote:
> > > On 10.06.2016 19:40, Andrew Jones wrote:
> > > > Signed-off-by: Andrew Jones <address@hidden>
> > > > ---
> > > >  qom/cpu.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > > index 751e992de8823..024cda3eb98c8 100644
> > > > --- a/qom/cpu.c
> > > > +++ b/qom/cpu.c
> > > > @@ -28,6 +28,7 @@
> > > >  #include "exec/log.h"
> > > >  #include "qemu/error-report.h"
> > > >  #include "sysemu/sysemu.h"
> > > > +#include "hw/qdev-properties.h"
> > > >  
> > > >  bool cpu_exists(int64_t id)
> > > >  {
> > > > @@ -342,6 +343,12 @@ static int64_t cpu_common_get_arch_id(CPUState 
> > > > *cpu)
> > > >      return cpu->cpu_index;
> > > >  }
> > > >  
> > > > +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.
> 
> 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.
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? 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.

Thanks,
drew