Re: [PATCH] target/i386/kvm: Add Hyper-V direct tlb flush support

[Vitaly Kuznetsov <vkuznets@redhat.com>]

lantianyu1986@gmail.com writes:

> From: Tianyu Lan <Tianyu.Lan@microsoft.com>
>

(Please also Cc: Roman on you Hyper-V related submissions to QEMU who's
known to be a great reviewer)

> Hyper-V direct tlb flush targets KVM on Hyper-V guest.
> Enable direct TLB flush for its guests meaning that TLB
> flush hypercalls are handled by Level 0 hypervisor (Hyper-V)
> bypassing KVM in Level 1. Due to the different ABI for hypercall
> parameters between Hyper-V and KVM, KVM capabilities should be
> hidden when enable Hyper-V direct tlb flush otherwise KVM
> hypercalls may be intercepted by Hyper-V. Add new parameter
> "hv-direct-tlbflush". Check expose_kvm and Hyper-V tlb flush
> capability status before enabling the feature.
>
> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> ---
>  docs/hyperv.txt           | 12 ++++++++++++
>  linux-headers/linux/kvm.h |  1 +
>  target/i386/cpu.c         |  2 ++
>  target/i386/cpu.h         |  1 +
>  target/i386/kvm.c         | 21 +++++++++++++++++++++
>  5 files changed, 37 insertions(+)
>
> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> index 8fdf25c829..ceab8c21fe 100644
> --- a/docs/hyperv.txt
> +++ b/docs/hyperv.txt
> @@ -184,6 +184,18 @@ enabled.
>  
>  Requires: hv-vpindex, hv-synic, hv-time, hv-stimer
>  
> +3.18. hv-direct-tlbflush
> +=======================
> +The enlightenment targets KVM on Hyper-V guest. Enable direct TLB flush for
> +its guests meaning that TLB flush hypercalls are handled by Level 0 hypervisor
> +(Hyper-V) bypassing KVM in Level 1. Due to the different ABI for hypercall
> +parameters between Hyper-V and KVM, enabling this capability effectively
> +disables all hypercall handling by KVM (as some KVM hypercall may be mistakenly
> +treated as TLB flush hypercalls by Hyper-V). So kvm capability should not show
> +to guest when enable this capability. If not, user will fail to enable this
> +capability.
> +
> +Requires: hv-tlbflush, -kvm
>  
>  4. Development features
>  ========================
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 18892d6541..923fb33a01 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -995,6 +995,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_ARM_SVE 170
>  #define KVM_CAP_ARM_PTRAUTH_ADDRESS 171
>  #define KVM_CAP_ARM_PTRAUTH_GENERIC 172
> +#define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 174

I was once told that scripts/update-linux-headers.sh script is supposed
to be used instead of cherry-picking stuff you need (adn this would be a
separate patch - update linux headers to smth).

>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 44f1bbdcac..7bc7fee512 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6156,6 +6156,8 @@ static Property x86_cpu_properties[] = {
>                        HYPERV_FEAT_IPI, 0),
>      DEFINE_PROP_BIT64("hv-stimer-direct", X86CPU, hyperv_features,
>                        HYPERV_FEAT_STIMER_DIRECT, 0),
> +    DEFINE_PROP_BIT64("hv-direct-tlbflush", X86CPU, hyperv_features,
> +                      HYPERV_FEAT_DIRECT_TLBFLUSH, 0),
>      DEFINE_PROP_BOOL("hv-passthrough", X86CPU, hyperv_passthrough, false),
>  
>      DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index eaa5395aa5..3cb105f7d6 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -907,6 +907,7 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  #define HYPERV_FEAT_EVMCS               12
>  #define HYPERV_FEAT_IPI                 13
>  #define HYPERV_FEAT_STIMER_DIRECT       14
> +#define HYPERV_FEAT_DIRECT_TLBFLUSH     15
>  
>  #ifndef HYPERV_SPINLOCK_NEVER_RETRY
>  #define HYPERV_SPINLOCK_NEVER_RETRY             0xFFFFFFFF
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 11b9c854b5..8e999dbcf1 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1235,6 +1235,27 @@ static int hyperv_handle_properties(CPUState *cs,
>          r |= 1;
>      }
>  
> +    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_DIRECT_TLBFLUSH)) {
> +        if (!kvm_check_extension(cs->kvm_state,
> +            KVM_CAP_HYPERV_DIRECT_TLBFLUSH)) {
> +            fprintf(stderr,
> +                    "Kernel doesn't support Hyper-V direct tlbflush.\n");
> +            r = -ENOSYS;
> +            goto free;
> +        }
> +
> +        if (cpu->expose_kvm ||
> +            !hyperv_feat_enabled(cpu, HYPERV_FEAT_TLBFLUSH)) {
> +            fprintf(stderr, "Hyper-V direct tlbflush requires Hyper-V %s"
> +                    " and not expose KVM.\n",
> +                    kvm_hyperv_properties[HYPERV_FEAT_TLBFLUSH].desc);
> +            r = -ENOSYS;
> +            goto free;
> +        }
> +
> +        kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_DIRECT_TLBFLUSH, 0, 0);
> +    }

We also have hv-passthrough mode where all Hyper-V enlightenments are
supposed to be enabled; I'd suggest you do the same we currently do with
HYPERV_FEAT_EVMCS:

    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_DIRECT_TLBFLUSH) ||
        cpu->hyperv_passthrough) {

        r = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_DIRECT_TLBFLUSH, 0, 0);

        if (hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) && r) {
            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
                    kvm_hyperv_properties[HYPERV_FEAT_DIRECT_TLBFLUSH].desc);
            return -ENOSYS;
        }

No need to check for a capability if you're going to enable it right
away (and btw - this can fail).

You also need to use kvm_hyperv_properties to track dependencies between
features and not do it manually here (like you required
HYPERV_FEAT_TLBFLUSH). This is going to be the first feature without its
own CPUID bits assigned so some tweaks to kvm_hyperv_properties handling
may be required. Or we can use HYPERV_FEAT_TLBFLUSH bits, I'm not sure
here.


> +
>      /* Not exposed by KVM but needed to make CPU hotplug in Windows work */
>      env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;

-- 
Vitaly