Re: [Qemu-devel] [PATCH for-2.9] pseries: Enforce homogeneous threads-per-core

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

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

On Fri, Mar 31, 2017 at 11:04:55AM +0530, Bharata B Rao wrote:
> On Fri, Mar 31, 2017 at 10:21 AM, David Gibson <david@gibson.dropbear.id.au>
> wrote:
> 
> > For reasons that may be useful in future, CPU core objects, as used on the
> > pseries machine type have their own nr-threads property, potentially
> > allowing cores with different numbers of threads in the same system.
> >
> 
> I remember we retained this flexibility to support heterogenous systems in
> future ? I think we can go with this enforcement now and relax it later if
> we ever reach there.
> 
> 
> >
> > If the user/management uses the values specified in query-hotpluggable-cpus
> > as they're expected to do, this will never matter in pratice.  But that's
> > not actually enforced - it's possible to manually specify a core with
> > a different number of threads from that in -smp.  That will confuse the
> > platform - most immediately, this can be used to create a CPU thread with
> > index above max_cpus which leads to an assertion failure in
> > spapr_cpu_core_realize().
> >
> > For now, enforce that all cores must have the same, standard, number of
> > threads.  While we're at it, also enforce that the core ids are correctly
> > aligned based on that number of threads.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_cpu_core.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 6883f09..935ee62 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -167,6 +167,18 @@ static void spapr_cpu_core_realize(DeviceState *dev,
> > Error **errp)
> >      void *obj;
> >      int i, j;
> >
> > +    if (cc->nr_threads != smp_threads) {
> > +        error_setg(errp,
> > +                   "Invalid nr-threads=%d of CPU[core-id: %d], must be
> > %d",
> > +                   cc->nr_threads, cc->core_id, smp_threads);
> > +        return;
> > +    }
> >
> 
> The above check should move to pre_plug handler.

Ah, good point.

> > +    if ((cc->core_id % smp_threads) != 0) {
> > +        error_setg(errp, "Invalid CPU core-id=%d, must be a multiple of
> > %d",
> > +                   cc->core_id, smp_threads);
> > +        return;
> > +    }
> >
> 
> Not sure when you will hit this as the same check is already  present in
> pre_plug handler.

Oops, yes, didn't spot that.

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