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

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

[David Gibson]

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.

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;
+    }
+    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;
+    }
+
     sc->threads = g_malloc0(size * cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
         int node_id;
-- 
2.9.3

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

[Bharata B Rao]

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.


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

Regards,
Bharata.

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

[Igor Mammedov]

On Fri, 31 Mar 2017 11:04:55 +0530
Bharata B Rao <bharata.rao@gmail.com> 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.
Agreed,
this property validation should be done at machine level (pre_plug)

> 
> 
> > +    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.
> 
> Regards,
> Bharata.

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

[David Gibson]

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