Hi Vitaly:
Thanks a lot for your review.
On Sun, Oct 13, 2019 at 4:49 PM Vitaly Kuznetsov <address@hidden> wrote:
>
> address@hidden writes:
>
> > From: Tianyu Lan <address@hidden>
> >
>
> (Please also Cc: Roman on you Hyper-V related submissions to QEMU who's
> known to be a great reviewer)
Good suggestion!
>
> > 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 <address@hidden>
> > ---
> > 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).
>
Thanks for suggestion. I just try the update-linux-headers.sh and there are a lot
of changes which are not related with my patch. I also include these
changes in my patch, right?
> >
> > #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).
>
Nice catch. I ignored passthrough mode and will update in the new version.
> 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.
>
Yes, I found kvm_hyperv_properties didn’t support no CPUID case and so gave up
to use it. I will have a try in the next version. Thanks.
--
Best regards
Tianyu Lan