Re: [Qemu-devel] [PATCH 2/3] kvm: check KVM_CAP_NR_VCPUS with kvm_vm_check_extension()

[Thomas Huth]

On 14.09.2017 21:25, Greg Kurz wrote:
> On a modern server-class ppc host with the following CPU topology:
> 
> Architecture:          ppc64le
> Byte Order:            Little Endian
> CPU(s):                32
> On-line CPU(s) list:   0,8,16,24
> Off-line CPU(s) list:  1-7,9-15,17-23,25-31
> Thread(s) per core:    1
> 
> If both KVM PR and KVM HV loaded and we pass:
> 
>         -machine pseries,accel=kvm,kvm-type=PR -smp 8
> 
> We expect QEMU to warn that this exceeds the number of online CPUs:
> 
> Warning: Number of SMP cpus requested (8) exceeds the recommended
>  cpus supported by KVM (4)
> Warning: Number of hotpluggable cpus requested (8) exceeds the
>  recommended cpus supported by KVM (4)
> 
> but nothing is printed...
> 
> This happens because on ppc the KVM_CAP_NR_VCPUS capability is VM
> specific  ndreally depends on the KVM type, but we currently use it
> as a global capability. And KVM returns a fallback value based on
> KVM HV being present. Maybe KVM on POWER shouldn't presume anything
> as long as it doesn't have a VM, but in all cases, we should call
> KVM_CREATE_VM first and use KVM_CAP_NR_VCPUS as a VM capability.
> 
> This patch hence changes kvm_recommended_vcpus() accordingly and
> moves the sanity checking of smp_cpus after the VM creation.
> 
> It is okay for the other archs that also implement KVM_CAP_NR_VCPUS,
> ie, mips, s390, x86 and arm, because they don't depend on the VM
> being created or not.
> 
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  accel/kvm/kvm-all.c |   45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 323c567cfb68..d10534de2da1 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1533,7 +1533,7 @@ static void kvm_irqchip_create(MachineState *machine, 
> KVMState *s)
>   */
>  static int kvm_recommended_vcpus(KVMState *s)
>  {
> -    int ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
> +    int ret = kvm_vm_check_extension(s, KVM_CAP_NR_VCPUS);
>      return (ret) ? ret : 4;
>  }
>  
> @@ -1623,27 +1623,6 @@ static int kvm_init(MachineState *ms)
>          s->nr_slots = 32;
>      }
>  
> -    /* check the vcpu limits */
> -    soft_vcpus_limit = kvm_recommended_vcpus(s);
> -    hard_vcpus_limit = kvm_max_vcpus(s);
> -
> -    while (nc->name) {
> -        if (nc->num > soft_vcpus_limit) {
> -            fprintf(stderr,
> -                    "Warning: Number of %s cpus requested (%d) exceeds "
> -                    "the recommended cpus supported by KVM (%d)\n",
> -                    nc->name, nc->num, soft_vcpus_limit);
> -
> -            if (nc->num > hard_vcpus_limit) {
> -                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
> -                        "the maximum cpus supported by KVM (%d)\n",
> -                        nc->name, nc->num, hard_vcpus_limit);
> -                exit(1);
> -            }
> -        }
> -        nc++;
> -    }
> -
>      kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
>      if (mc->kvm_type) {
>          type = mc->kvm_type(kvm_type);
> @@ -1678,6 +1657,28 @@ static int kvm_init(MachineState *ms)
>      }
>  
>      s->vmfd = ret;
> +
> +    /* check the vcpu limits */
> +    soft_vcpus_limit = kvm_recommended_vcpus(s);
> +    hard_vcpus_limit = kvm_max_vcpus(s);
> +
> +    while (nc->name) {
> +        if (nc->num > soft_vcpus_limit) {
> +            fprintf(stderr,
> +                    "Warning: Number of %s cpus requested (%d) exceeds "
> +                    "the recommended cpus supported by KVM (%d)\n",
> +                    nc->name, nc->num, soft_vcpus_limit);
> +
> +            if (nc->num > hard_vcpus_limit) {
> +                fprintf(stderr, "Number of %s cpus requested (%d) exceeds "
> +                        "the maximum cpus supported by KVM (%d)\n",
> +                        nc->name, nc->num, hard_vcpus_limit);
> +                exit(1);
> +            }
> +        }
> +        nc++;
> +    }
> +
>      missing_cap = kvm_check_extension_list(s, kvm_required_capabilites);
>      if (!missing_cap) {
>          missing_cap =
> 

Reviewed-by: Thomas Huth <address@hidden>