Re: [Qemu-ppc] [PATCH v3] spapr: set vsmt to MAX(8, smp_threads)

[Greg Kurz]

On Fri,  9 Feb 2018 09:18:58 +0100
Laurent Vivier <address@hidden> wrote:

> We ignore silently the value of smp_threads when we set
> the default VSMT value, and if smp_threads is greater than VSMT
> kernel is going into trouble later.
> 

Hi Laurent,

I've looked a bit more and I'm not sure what kernel troubles you're referring 
to,
but several places in QEMU where we use kvm_ppc_smt() later on do assume that
smp_threads > kvm_ppc_smt(). Basically, everywhere we compute a vCPU id:

In spapr_init_cpus() when creating DRC connectors:

        int core_id = i * smp_threads;

        if (mc->has_hotpluggable_cpus) {
            spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
                                   (core_id / smp_threads) * smt);
        }

or in spapr_cpu_core_realize() when creating vCPUs:

        cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i;

It is visible by adding some printfs in the current code base. This is what
happens when passing -smp cores=2,threads=16 without your patch:

DRC connector to vcpu_id 0
CPU vcpu_id 0
CPU vcpu_id 1
CPU vcpu_id 2
CPU vcpu_id 3
CPU vcpu_id 4
CPU vcpu_id 5
CPU vcpu_id 6
CPU vcpu_id 7
CPU vcpu_id 8
CPU vcpu_id 9
CPU vcpu_id 10
CPU vcpu_id 11
CPU vcpu_id 12
CPU vcpu_id 13
CPU vcpu_id 14
CPU vcpu_id 15
DRC connector to vcpu_id 8
                        ^^^
                     should be 16
CPU vcpu_id 8
           ^^^
   should start numbering at 16
CPU vcpu_id 9
CPU vcpu_id 10
CPU vcpu_id 11
CPU vcpu_id 12
CPU vcpu_id 13
CPU vcpu_id 14
CPU vcpu_id 15
CPU vcpu_id 16
CPU vcpu_id 17
CPU vcpu_id 18
CPU vcpu_id 19
CPU vcpu_id 20
CPU vcpu_id 21
CPU vcpu_id 22
CPU vcpu_id 23
qemu-system-ppc64: kvm_init_vcpu failed: File exists
                                              ^^^^
                                 CPU 8 already created by the first core

I'm not feeling comfortable with the rest of the code silently depending on
the fact that spapr_set_vsmt_mode() terminates QEMU if it cannot enforce
smp_threads <= kvm_ppc_smt().

Anyway, with your patch, the same command line as above gives:

qemu-system-ppc64: Failed to set KVM's VSMT mode to 16 (errno -22)
On PPC, a VM with 16 threads/core on a host with 8 threads/core requires
 the use of VSMT mode 16.
This KVM seems to be too old to support VSMT.

This hammer is big enough to fix the vCPU ids miscalculations, so:

Reviewed-by: Greg Kurz <address@hidden>

> Fixes: 8904e5a750
> ("spapr: Adjust default VSMT value for better migration compatibility")
> 
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
> 
> Notes:
>     v3: use MAX(8, smp_threads) and let KVM to return an error
>         if nb_threads is too big
>         update subject to reflect the change
>     
>     v2: display a specific error message when the default VSMT is used
>         fix subject
> 
>  hw/ppc/spapr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 32a876be56..c8a1eefa17 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2310,7 +2310,7 @@ static void spapr_set_vsmt_mode(sPAPRMachineState 
> *spapr, Error **errp)
>           * the value that we'd get with KVM on POWER8, the
>           * overwhelmingly common case in production systems.
>           */
> -        spapr->vsmt = 8;
> +        spapr->vsmt = MAX(8, smp_threads);
>      }
>  
>      /* KVM: If necessary, set the SMT mode: */