Re: [Qemu-devel] [PATCH v2 07/23] hyperv: ensure VP index equal to QEMU cpu_index

[Igor Mammedov]

On Wed, 21 Jun 2017 19:24:08 +0300
Roman Kagan <address@hidden> wrote:

> Hyper-V identifies vCPUs by Virtual Processor (VP) index which can be
> queried by the guest via HV_X64_MSR_VP_INDEX msr.  It is defined by the
> spec as a sequential number which can't exceed the maximum number of
> vCPUs per VM.
> 
> It has to be owned by QEMU in order to preserve it across migration.
> 
> However, the initial implementation in KVM didn't allow to set this
> msr, and KVM used its own notion of VP index.  Fortunately, the way
> vCPUs are created in QEMU/KVM makes it likely that the KVM value is
> equal to QEMU cpu_index.
> 
> So choose cpu_index as the value for vp_index, and push that to KVM on
> kernels that support setting the msr.  On older ones that don't, query
> the kernel value and assert that it's in sync with QEMU.
> 
> Besides, since handling errors from vCPU init at hotplug time is
> impossible, disable vCPU hotplug.
proper place to check if cpu might be created is at 
pc_cpu_pre_plug() where you can gracefully abort cpu creation process. 

Also it's possible to create cold-plugged CPUs in out of order
sequence using
 -device cpu-foo on CLI
will be hyperv kvm/guest side ok with it?

> This patch also introduces accessor functions to wrap the mapping
> between a vCPU and its vp_index.  Besides, a few variables are renamed
> to avoid confusion of vp_index with vcpu_id (== apic_id).
> 
> Signed-off-by: Roman Kagan <address@hidden>
> ---
> v1 -> v2:
>  - were patches 5, 6 in v1
>  - move vp_index initialization to hyperv_init_vcpu
>  - check capability before trying to set the msr
>  - set the msr on the usual kvm_put_msrs path
>  - disable cpu hotplug if msr is not settable
> 
>  target/i386/hyperv.h |  5 ++++-
>  target/i386/hyperv.c | 16 +++++++++++++---
>  target/i386/kvm.c    | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
> index 0c3b562..82f4757 100644
> --- a/target/i386/hyperv.h
> +++ b/target/i386/hyperv.h
> @@ -32,11 +32,14 @@ struct HvSintRoute {
>  
>  int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit);
>  
> -HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> +HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
>                                        HvSintAckClb sint_ack_clb);
>  
>  void kvm_hv_sint_route_destroy(HvSintRoute *sint_route);
>  
>  int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route);
>  
> +uint32_t hyperv_vp_index(X86CPU *cpu);
> +X86CPU *hyperv_find_vcpu(uint32_t vp_index);
> +
>  #endif
> diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
> index 227185c..4f57447 100644
> --- a/target/i386/hyperv.c
> +++ b/target/i386/hyperv.c
> @@ -16,6 +16,16 @@
>  #include "hyperv.h"
>  #include "hyperv_proto.h"
>  
> +uint32_t hyperv_vp_index(X86CPU *cpu)
> +{
> +    return CPU(cpu)->cpu_index;
> +}


> +X86CPU *hyperv_find_vcpu(uint32_t vp_index)
> +{
> +    return X86_CPU(qemu_get_cpu(vp_index));
> +}
this helper isn't used in this patch, add it in the patch that would actually 
use it

also if  qemu_get_cpu() were called from each CPU init,
it would incur O(N^2) complexity, could you do without it?

> +
>  int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
>  {
>      CPUX86State *env = &cpu->env;
> @@ -72,7 +82,7 @@ static void kvm_hv_sint_ack_handler(EventNotifier *notifier)
>      }
>  }
>  
> -HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, uint32_t sint,
> +HvSintRoute *kvm_hv_sint_route_create(uint32_t vp_index, uint32_t sint,
>                                        HvSintAckClb sint_ack_clb)
>  {
>      HvSintRoute *sint_route;
> @@ -92,7 +102,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, 
> uint32_t sint,
>      event_notifier_set_handler(&sint_route->sint_ack_notifier,
>                                 kvm_hv_sint_ack_handler);
>  
> -    gsi = kvm_irqchip_add_hv_sint_route(kvm_state, vcpu_id, sint);
> +    gsi = kvm_irqchip_add_hv_sint_route(kvm_state, vp_index, sint);
>      if (gsi < 0) {
>          goto err_gsi;
>      }
> @@ -105,7 +115,7 @@ HvSintRoute *kvm_hv_sint_route_create(uint32_t vcpu_id, 
> uint32_t sint,
>      }
>      sint_route->gsi = gsi;
>      sint_route->sint_ack_clb = sint_ack_clb;
> -    sint_route->vcpu_id = vcpu_id;
> +    sint_route->vcpu_id = vp_index;
                   ^^^ - shouldn't it also be re-named?

maybe split all renaming into separate patch ...

>      sint_route->sint = sint;
>  
>      return sint_route;
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 2795b63..9bf7f08 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -86,6 +86,7 @@ static bool has_msr_hv_hypercall;
>  static bool has_msr_hv_crash;
>  static bool has_msr_hv_reset;
>  static bool has_msr_hv_vpindex;
> +static bool is_hv_vpindex_settable;
>  static bool has_msr_hv_runtime;
>  static bool has_msr_hv_synic;
>  static bool has_msr_hv_stimer;
> @@ -665,6 +666,43 @@ static int hyperv_handle_properties(CPUState *cs)
>      return 0;
>  }
>  
> +static int hyperv_init_vcpu(X86CPU *cpu)
> +{
> +    if (cpu->hyperv_vpindex && !is_hv_vpindex_settable) {
> +        /*
> +         * the kernel doesn't support setting vp_index; assert that its value
> +         * is in sync
> +         */
> +        int ret;
> +        struct {
> +            struct kvm_msrs info;
> +            struct kvm_msr_entry entries[1];
> +        } msr_data = {
> +            .info.nmsrs = 1,
> +            .entries[0].index = HV_X64_MSR_VP_INDEX,
> +        };
> +
> +        /*
> +         * handling errors from vcpu init at hotplug time is impossible, so
> +         * disallow cpu hotplug
> +         */
> +        MACHINE_GET_CLASS(current_machine)->hot_add_cpu = NULL;
one shouldn't alter machine this way nor
it would disable cpu hotplug, it would disable only cpu-add interface
but device_add() would still work.


> +        ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
> +        if (ret < 0) {
when this can fail?

> +            return ret;
> +        }
> +        assert(ret == 1);
> +
> +        if (msr_data.entries[0].data != hyperv_vp_index(cpu)) {
> +            fprintf(stderr, "kernel's vp_index != QEMU's vp_index\n");
error_report()

> +            return -ENXIO;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static Error *invtsc_mig_blocker;
>  
>  #define KVM_MAX_CPUID_ENTRIES  100
> @@ -1013,6 +1051,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          has_msr_tsc_aux = false;
>      }
>  
> +    if (hyperv_enabled(cpu)) {
> +        r = hyperv_init_vcpu(cpu);
> +        if (r) {
> +            goto fail;
> +        }
> +    }
> +
>      return 0;
>  
>   fail:
> @@ -1204,6 +1249,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      has_pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
>  #endif
>  
> +    is_hv_vpindex_settable = kvm_check_extension(s, KVM_CAP_HYPERV_VP_INDEX);
> +
>      ret = kvm_get_supported_msrs(s);
>      if (ret < 0) {
>          return ret;
> @@ -1748,6 +1795,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
>          if (has_msr_hv_runtime) {
>              kvm_msr_entry_add(cpu, HV_X64_MSR_VP_RUNTIME, 
> env->msr_hv_runtime);
>          }
> +        if (cpu->hyperv_vpindex && has_msr_hv_vpindex &&
> +            is_hv_vpindex_settable) {
> +            kvm_msr_entry_add(cpu, HV_X64_MSR_VP_INDEX, 
> hyperv_vp_index(cpu));
> +        }
>          if (cpu->hyperv_synic) {
>              int j;
>