Re: [Qemu-devel] [PATCH] PPC/KVM: early validation of vcpu id

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3248 bytes --]

On Tue, Apr 26, 2016 at 03:41:04PM +0200, Greg Kurz wrote:
> The KVM API restricts vcpu ids to be < KVM_CAP_MAX_VCPUS. On PowerPC
> targets, depending on the number of threads per core in the host and
> in the guest, some topologies do generate higher vcpu ids actually.
> When this happens, QEMU bails out with the following error:
> 
> kvm_init_vcpu failed: Invalid argument
> 
> The KVM_CREATE_VCPU ioctl has several EINVAL return paths, so it is
> not possible to fully disambiguate.
> 
> This patch adds a check in the code that computes vcpu ids, so that
> we can detect the error earlier, and print a friendlier message instead
> of calling KVM_CREATE_VCPU with an obviously bogus vcpu id.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Applied to ppc-for-2.7, thanks.

I'm still kicking myself for doing the SMT / cpu ids that way way back
when.  Should have just had a "SET_SMT" ioctl() and allocated the cpu
ids sequentially.  Too clever by half :(

> ---
>  include/sysemu/kvm.h        |    2 ++
>  kvm-all.c                   |    6 ++++++
>  target-ppc/translate_init.c |    8 ++++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 0e18f15c9493..27bf50ef379e 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -344,6 +344,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s);
>  
>  int kvm_arch_init_vcpu(CPUState *cpu);
>  
> +bool kvm_vcpu_id_is_valid(int vcpu_id);
> +
>  /* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
>  unsigned long kvm_arch_vcpu_id(CPUState *cpu);
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index e7b66df197fd..3625a3e07457 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1459,6 +1459,12 @@ static int kvm_max_vcpus(KVMState *s)
>      return (ret) ? ret : kvm_recommended_vcpus(s);
>  }
>  
> +bool kvm_vcpu_id_is_valid(int vcpu_id)
> +{
> +    KVMState *s = KVM_STATE(current_machine->accelerator);
> +    return vcpu_id >= 0 && vcpu_id < kvm_max_vcpus(s);
> +}
> +
>  static int kvm_init(MachineState *ms)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index f51572552bc2..6c89b18a05f9 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9247,6 +9247,14 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>  #if !defined(CONFIG_USER_ONLY)
>      cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
>          + (cs->cpu_index % smp_threads);
> +
> +    if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->cpu_dt_id)) {
> +        error_setg(errp, "Can't create CPU with id %d in KVM", cpu->cpu_dt_id);
> +        error_append_hint(errp, "Adjust the number of cpus to %d "
> +                          "or try to raise the number of threads per core\n",
> +                          cpu->cpu_dt_id * smp_threads / max_smt);
> +        return;
> +    }
>  #endif
>  
>      if (tcg_enabled()) {
> 

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