| Message ID | 20170331045144.9307-1-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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;
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(+)