[RFC,v4,09/11] spapr: Support topologies with unfilled cores

QEMU currently supports CPU topologies where there can be cores
which are not completely filled with all the threads as per the
specifed SMT mode.

Restore support for such topologies (Eg -smp 15,cores=4,threads=4)
The last core will always have the deficit even when -device options are
used to cold-plug the cores.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

On Thu, Aug 06, 2015 at 10:57:15AM +0530, Bharata B Rao wrote:
> QEMU currently supports CPU topologies where there can be cores
> which are not completely filled with all the threads as per the
> specifed SMT mode.
> 
> Restore support for such topologies (Eg -smp 15,cores=4,threads=4)
> The last core will always have the deficit even when -device options are
> used to cold-plug the cores.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Is there a reason to support these silly toplogies, or should we just
error out if this is specified?


> ---
>  hw/ppc/spapr.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 74637b3..004a8e1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -94,6 +94,8 @@
>  
>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
>  
> +static int smp_remaining_cpus;
> +
>  static XICSState *try_create_xics(const char *type, int nr_servers,
>                                    int nr_irqs, Error **errp)
>  {
> @@ -1700,6 +1702,7 @@ static void ppc_spapr_init(MachineState *machine)
>      int smp_max_cores = DIV_ROUND_UP(max_cpus, smp_threads);
>      int smp_cores = DIV_ROUND_UP(smp_cpus, smp_threads);
>  
> +    smp_remaining_cpus = smp_cpus;
>      msi_supported = true;
>  
>      QLIST_INIT(&spapr->phbs);
> @@ -2202,6 +2205,7 @@ static void spapr_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      Error *local_err = NULL;
>      void *fdt = NULL;
>      int i, fdt_offset = 0;
> +    int threads_per_core;
>  
>      /* Set NUMA node for the added CPUs  */
>      for (i = 0; i < nb_numa_nodes; i++) {
> @@ -2224,8 +2228,22 @@ static void spapr_cpu_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> +    /* Create SMT threads of the core.
> +     *
> +     * Support topologies like -smp 15,cores=4,threads=4 where one core
> +     * will have less than the specified SMT threads. The last core will
> +     * always have the deficit even when -device options are used to
> +     * cold-plug the cores.
> +     */
> +    if ((smp_remaining_cpus > 0) && (smp_remaining_cpus < smp_threads)) {
> +        threads_per_core = smp_remaining_cpus;
> +    } else {
> +        threads_per_core = smp_threads;
> +    }
> +    smp_remaining_cpus -= threads_per_core;
> +
>      /* Create SMT threads of the core. */
> -    for (i = 1; i < smp_threads; i++) {
> +    for (i = 1; i < threads_per_core; i++) {
>          cpu = cpu_ppc_init(current_machine->cpu_model);
>          if (!cpu) {
>              error_report("Unable to find PowerPC CPU definition: %s",

On 04/09/15 09:01, David Gibson wrote:
> On Thu, Aug 06, 2015 at 10:57:15AM +0530, Bharata B Rao wrote:
>> QEMU currently supports CPU topologies where there can be cores
>> which are not completely filled with all the threads as per the
>> specifed SMT mode.
>>
>> Restore support for such topologies (Eg -smp 15,cores=4,threads=4)
>> The last core will always have the deficit even when -device options are
>> used to cold-plug the cores.
>>
>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Is there a reason to support these silly toplogies, or should we just
> error out if this is specified?

FYI, I've recently submitted a patch that tries to catch such illegal
SMP configurations and simply errors out in that case:

http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg04549.html

It's not upstream yet, but already in Eduardo's x86 branch. I think this
will reject the bad topology from your example, too.

 Thomas

On Fri, Sep 04, 2015 at 10:44:57AM +0200, Thomas Huth wrote:
> On 04/09/15 09:01, David Gibson wrote:
> > On Thu, Aug 06, 2015 at 10:57:15AM +0530, Bharata B Rao wrote:
> >> QEMU currently supports CPU topologies where there can be cores
> >> which are not completely filled with all the threads as per the
> >> specifed SMT mode.
> >>
> >> Restore support for such topologies (Eg -smp 15,cores=4,threads=4)
> >> The last core will always have the deficit even when -device options are
> >> used to cold-plug the cores.
> >>
> >> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > Is there a reason to support these silly toplogies, or should we just
> > error out if this is specified?

Only reason was to ensure that existing guest with such topologies
continue to boot like before.

> 
> FYI, I've recently submitted a patch that tries to catch such illegal
> SMP configurations and simply errors out in that case:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg04549.html
> 
> It's not upstream yet, but already in Eduardo's x86 branch. I think this
> will reject the bad topology from your example, too.

It does reject -smp 15,cores=4,threads=4, but with

-smp 15,cores=4,threads=4,maxcpus=16, the guest still boots with weird
topology.

[root@localhost ~]# lscpu
Architecture:          ppc64
CPU op-mode(s):        32-bit, 64-bit
Byte Order:            Big Endian
CPU(s):                16
On-line CPU(s) list:   0-14
Off-line CPU(s) list:  15
Thread(s) per core:    3
Core(s) per socket:    1
Socket(s):             4
NUMA node(s):          1
Model:                 IBM pSeries (emulated by qemu)
L1d cache:             64K
L1i cache:             32K
NUMA node0 CPU(s):     0-14

[root@localhost ~]# ppc64_cpu --info
Core   0:    0*    1*    2*    3* 
Core   1:    4*    5*    6*    7* 
Core   2:    8*    9*   10*   11* 
Core   3:   12*   13*   14*   15

Should such topologies also be prevented from booting ?

Regards,
Bharata.