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

[David Gibson]

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:
> > > 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.

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.

> 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.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature