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 <address@hidden>
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 <address@hidden>
> ---
>  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.