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

[Igor Mammedov]

On Fri, 8 Jul 2016 17:39:52 +1000
David Gibson <address@hidden> wrote:

> On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote:
> > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote:  
> > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao 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.
> > > > 
> > > > Signed-off-by: Bharata B Rao <address@hidden>
> > > > ---
> > > >  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 
> > > > */
> > > > +        if (cs->has_stable_cpu_id) {
> > > > +            cs->stable_cpu_id = cc->core_id + i;
> > > > +        }  
> > > 
> > > Testing cs->has_stable_cpu_id here in machine type specific code seems
> > > really weird.  It's the machine type that knows whether it has a
> > > stable ID to give to the CPU or not, rather than the other way around.
> > > 
> > > Since we haven't yet had a release with cpu cores, I think the right
> > > thing is for cpu_core to unconditionally set the stable ID (and set
> > > has_stable_id to true).  
> > 
> > Right, we can set cpu_stable_id unconditionally here since this code path
> > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id
> > will get set as part of the property we defined in SPAPR_COMPAT_2_7.  
> 
> Hrm, that's true.  But when you describe it like that it sounds like a
> really non-obvious and fragile dependency between different components.
that's how compat stuff is typically done for devices,
CPUs shouldn't be an exception. 
(consistency with other devices helps here in long run)
 
> > > The backup path that does thread-based cpu
> > > init, can set has_stable_id to false (if that's not the default).  
> > 
> > Default is off, but turning it on for 2.7 will be inherited by 2.6
> > and others below. Hence I have code to explicitly disable this prop
> > for 2.6 and below via SPAPR_COMPAT_2_6.  
> 
> This is all seeming terribly awkward.
Typically default is set the way so new machine type doesn't have
to enable it explicitly.

However the way it's done here helps not to touch/check every user
that uses cpu_index, limiting series impact only to code that
asks for it, it look a lot safer to got this rout for now.


>  Can we try investigating a
> different approach:
> 
>     1. Rename cpu_index to cpu_id, but it's still used in the same
>        places it's used.
> 
>     2. Remove assumptions that cpu_id values are contiguous or
>        dense
>     
>     3. Machine type decides whether it wants to populate the cpu_id
>        values explicitly, or leave it to generic code to calculate
>        them as cpu_index is calculated now.
> 
>     4. Ideally, generic code enforces that the machine type populates
>        either all or none of the cpu_id values.
> 
> Does that seem workable?
Ideally we will get there some day (and may be get rid of cpu_index altogether),
but for now it seems too invasive with a lot of chances to introduce non obvious
regression.

So I'd keep approach used in this series.