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

[Eduardo Habkost]

On Tue, Jun 06, 2017 at 09:19:30PM +0300, Roman Kagan wrote:
> Hyper-V identifies vcpus by the virtual processor (VP) index which is
> normally queried by the guest via HV_X64_MSR_VP_INDEX msr.
> 
> It has to be owned by QEMU in order to preserve it across migration.
> 
> However, the current implementation in KVM doesn't allow to set this
> msr, and KVM uses its own notion of VP index.  Fortunately, the way
> vcpus are created in QEMU/KVM basically guarantees that the KVM value is
> equal to QEMU cpu_index.

This might not be true in the future.  cpu_index is not a good
identifier for CPUs, and we would like to remove it in the
future.

But it looks like we have no choice, see extra comments below:

> 
> So, for back and forward compatibility, attempt to set the msr at vcpu
> init time to cpu_index, but ignore the errors; then query the msr value
> from KVM and assert that it's equal to cpu_index.  On current kernels
> this will work by luck; future ones will accept the value from
> userspace.
> 
> Signed-off-by: Roman Kagan <address@hidden>
> ---
>  target/i386/hyperv.h |  2 ++
>  target/i386/hyperv.c |  5 +++++
>  target/i386/kvm.c    | 26 ++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h
> index 0c3b562..35da0b1 100644
> --- a/target/i386/hyperv.h
> +++ b/target/i386/hyperv.h
> @@ -39,4 +39,6 @@ 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);
> +
>  #endif
> diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c
> index 227185c..27de5bc 100644
> --- a/target/i386/hyperv.c
> +++ b/target/i386/hyperv.c
> @@ -16,6 +16,11 @@
>  #include "hyperv.h"
>  #include "hyperv_proto.h"
>  
> +uint32_t hyperv_vp_index(X86CPU *cpu)
> +{
> +    return CPU(cpu)->cpu_index;
> +}
> +

You are introducing a wrapper, but you are using cs->cpu_index
directly at hyperv_set_vp_index().  The knowledge that vp_index
== cpu_index is duplicated on two places, and the functions risk
getting out of sync.

I suggest using the wrapper inside hyperv_set_vp_index() too.


>  int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
>  {
>      CPUX86State *env = &cpu->env;
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 251aa95..eb9cde4 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -610,11 +610,37 @@ static int kvm_arch_set_tsc_khz(CPUState *cs)
>      return 0;
>  }
>  
> +static void hyperv_set_vp_index(CPUState *cs)
> +{
> +    struct {
> +        struct kvm_msrs info;
> +        struct kvm_msr_entry entries[1];
> +    } msr_data;
> +    int ret;
> +
> +    msr_data.info.nmsrs = 1;
> +    msr_data.entries[0].index = HV_X64_MSR_VP_INDEX;
> +
> +    /*
> +     * Some kernels don't support setting HV_X64_MSR_VP_INDEX.  However,
> +     * they use VP_INDEX equal to cpu_index due to the way vcpus are created.
> +     * So ignore the errors from SET_MSRS, but verify that GET_MSRS returns 
> the
> +     * expected value.
> +     */

Oh.  This sounds like a problem.  As reference for others, this
is the KVM code in kvm_hv_get_msr():

        case HV_X64_MSR_VP_INDEX: {
                int r;
                struct kvm_vcpu *v;

                kvm_for_each_vcpu(r, v, vcpu->kvm) {
                        if (v == vcpu) {
                                data = r;
                                break;
                        }
                }
                break;
        }

The problem with that is that it will break as soon as we create
VCPUs in a different order.  Unsolvable on hosts that don't allow
HV_X64_MSR_VP_INDEX to be set, however.

> +    msr_data.entries[0].data = cs->cpu_index;
> +    kvm_vcpu_ioctl(cs, KVM_SET_MSRS, &msr_data);
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_MSRS, &msr_data);
> +    assert(ret == 1);
> +    assert(msr_data.entries[0].data == cs->cpu_index);

If KVM already initializes the MSR to cpu_index and we will abort
if it was not set to anything except cpu_index here, why exactly
do we need the KVM_SET_MSRS call?

> +}
> +
>  static int hyperv_handle_properties(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
>  
> +    hyperv_set_vp_index(cs);
> +

The purpose of hyperv_handle_properties() is just to initialize
cpu->features[] CPUID data based on the hyperv options.  I suggest
creating a new hyperv_vcpu_init() or x86_cpu_hyperv_init()
function for these initialization steps.

>      if (cpu->hyperv_time &&
>              kvm_check_extension(cs->kvm_state, KVM_CAP_HYPERV_TIME) <= 0) {
>          cpu->hyperv_time = false;
> -- 
> 2.9.4
> 

-- 
Eduardo