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 <drjo...@redhat.com>
> > > > ---
> > > > 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
