Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4317 bytes --]

On Fri, Jul 08, 2016 at 09:46:47AM +0200, Greg Kurz wrote:
> On Fri, 8 Jul 2016 15:25:33 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote:
> > > On Thu,  7 Jul 2016 20:20:23 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >   
> > > > Conditonally set stable_cpu_id for CPU threads that are created as part
> > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7
> > > > onwards.
> > > >   
> > > 
> > > The last sentence is a bit confusing since the enablement actually happens
> > > in patch 5/5.
> > >   
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/spapr_cpu_core.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > index b104778..0ec3513 100644
> > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > >      for (i = 0; i < cc->nr_threads; i++) {
> > > >          char id[32];
> > > >          obj = sc->threads + i * size;
> > > > +        CPUState *cs;
> > > >  
> > > >          object_initialize(obj, size, typename);
> > > > +        cs = CPU(obj);
> > > > +
> > > > +        /* Use core_id (which is actually cpu_dt_id) as stable CPU id */  
> > > 
> > > It isn't what I had in mind. More something like below:
> > > 
> > > In ppc_spapr_init():
> > > 
> > >     for (i = 0; i < spapr_max_cores; i++) {
> > >         spapr->cores[i]->stable_id = i * smp_threads;
> > >     }
> > > 
> > > 
> > > In spapr_cpu_core_realize():
> > > 
> > >     for (j = 0; j < cc->nr_threads; j++) {
> > >         stable_cpu_id = cc->stable_id + j;
> > >     }
> > > 
> > > So we need to introduce cc->stable_id.  
> > 
> > No, we don't.  Cores have had a stable ID since they were introduced.
> > 
> 
> I agree core_dt_id is stable but it is a DT concept.

There is no core_dt_id.  There's just core-id, which is machine
assigned (via the query hotpluggable cpus interface) and stable.

> static void ppc_spapr_init(MachineState *machine)
> {
> [...]
>         for (i = 0; i < spapr_max_cores; i++) {
>             int core_dt_id = i * smt;

..uh, ok, except for that poorly named variable.  But that's because
this is in the machine type, and it knows it's going to use the same
ids to give to the core object and to put in the device tree.

> [...]
>                 object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
>                                         &error_fatal);
> 
> This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it
> awkward it depends on the host setup.

True.  Possibly we should set these as i * (maximum plausible number
of threads).

The gotcha is that currently we're using the same "dt_id" to control
KVM's cpu id and that in turn controls the SMT level.  That's a poor
interface on the kernel side (my bad), but we have to live with it
now.  However we could de-couple that KVM id from the core-id.  It'd
no doubt cause some complications with kvm-xics, but we can probably
handle it.

> I'm suggesting we introduce cc->stable_id to be able to compute a simple
> stable_cpu_id in the range [0...max_cpus), like x86 and ARM.

I really don't see what properties this is supposed to have that are
different from the existing core-id.

> 
> > Instead we should be setting the thread stable ids based on the core
> > stable id.
> > 
> > > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id.
> > > 
> > > Makes sense ?
> > >   
> > > > +        if (cs->has_stable_cpu_id) {
> > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > +        }
> > > >          snprintf(id, sizeof(id), "thread[%d]", i);
> > > >          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> > > >          if (local_err) {  
> > >   
> > 
> 



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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]