| Message ID | 20210518144339.1987982-6-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC | expand |
On 18/05/21 16:43, Vitaly Kuznetsov wrote: > APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon > SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is, > however, possible to track whether the feature was actually used by the > guest and only inhibit APICv/AVIC when needed. > > TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let > Windows know that AutoEOI feature should be avoided. While it's up to > KVM userspace to set the flag, KVM can help a bit by exposing global > APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI > is still preferred. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> Should it also disable APICv unconditionally if HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID? That should avoid ping-pong between enabled and disabled APICv even in pathological cases that we cannot think about. Paolo > --- > arch/x86/include/asm/kvm_host.h | 3 +++ > arch/x86/kvm/hyperv.c | 27 +++++++++++++++++++++------ > 2 files changed, 24 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index bf5807d35339..5e03ab4c0e4f 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -936,6 +936,9 @@ struct kvm_hv { > /* How many vCPUs have VP index != vCPU index */ > atomic_t num_mismatched_vp_indexes; > > + /* How many SynICs use 'AutoEOI' feature */ > + atomic_t synic_auto_eoi_used; > + > struct hv_partition_assist_pg *hv_pa_pg; > struct kvm_hv_syndbg hv_syndbg; > }; > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index f98370a39936..89e7d5b99279 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic, > static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > int vector) > { > + struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic); > + struct kvm_hv *hv = to_kvm_hv(vcpu->kvm); > + int auto_eoi_old, auto_eoi_new; > + > if (vector < HV_SYNIC_FIRST_VALID_VECTOR) > return; > > @@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > else > __clear_bit(vector, synic->vec_bitmap); > > + auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256); > + > if (synic_has_vector_auto_eoi(synic, vector)) > __set_bit(vector, synic->auto_eoi_bitmap); > else > __clear_bit(vector, synic->auto_eoi_bitmap); > + > + auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256); > + > + /* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */ > + if (!auto_eoi_old && auto_eoi_new) { > + if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1) > + kvm_request_apicv_update(vcpu->kvm, false, > + APICV_INHIBIT_REASON_HYPERV); > + } else if (!auto_eoi_new && auto_eoi_old) { > + if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0) > + kvm_request_apicv_update(vcpu->kvm, true, > + APICV_INHIBIT_REASON_HYPERV); > + } > } > > static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, > @@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages) > > synic = to_hv_synic(vcpu); > > - /* > - * Hyper-V SynIC auto EOI SINT's are > - * not compatible with APICV, so request > - * to deactivate APICV permanently. > - */ > - kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV); > synic->active = true; > synic->dont_zero_synic_pages = dont_zero_synic_pages; > synic->control = HV_SYNIC_CONTROL_ENABLE; > @@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, > ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED; > if (!cpu_smt_possible()) > ent->eax |= HV_X64_NO_NONARCH_CORESHARING; > + if (enable_apicv) > + ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED; > /* > * Default number of spinlock retry attempts, matches > * HyperV 2016. >
Paolo Bonzini <pbonzini@redhat.com> writes: > On 18/05/21 16:43, Vitaly Kuznetsov wrote: >> APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon >> SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is, >> however, possible to track whether the feature was actually used by the >> guest and only inhibit APICv/AVIC when needed. >> >> TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let >> Windows know that AutoEOI feature should be avoided. While it's up to >> KVM userspace to set the flag, KVM can help a bit by exposing global >> APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI >> is still preferred. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > Should it also disable APICv unconditionally if > HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID? That should > avoid ping-pong between enabled and disabled APICv even in pathological > cases that we cannot think about. When you run Hyper-V on KVM it doesn't use SynIC (let alone AutoEOI) but we still inhibit APICv unconditionally. The patch as-is improves this without any userspace changes required and I see it as a benefit. Going forward, we will definitely add something like 'hv-synic-noaeoi' to QEMU to make non-nesting setups benefit too but it'll take a while for this option to propagate to real world configurations (sigh).
On 25/05/21 08:23, Vitaly Kuznetsov wrote: >> Should it also disable APICv unconditionally if >> HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID? That should >> avoid ping-pong between enabled and disabled APICv even in pathological >> cases that we cannot think about. > When you run Hyper-V on KVM it doesn't use SynIC (let alone AutoEOI) but > we still inhibit APICv unconditionally. The patch as-is improves this > without any userspace changes required and I see it as a benefit. Going > forward, we will definitely add something like 'hv-synic-noaeoi' to QEMU > to make non-nesting setups benefit too but it'll take a while for this > option to propagate to real world configurations (sigh). Ok, if enable<->disable APICv becomes an issue we can also consider disabling APICv forever if AutoEOI is ever used. Paolo
On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote: > APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon > SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is, > however, possible to track whether the feature was actually used by the > guest and only inhibit APICv/AVIC when needed. > > TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let > Windows know that AutoEOI feature should be avoided. While it's up to > KVM userspace to set the flag, KVM can help a bit by exposing global > APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI > is still preferred. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 3 +++ > arch/x86/kvm/hyperv.c | 27 +++++++++++++++++++++------ > 2 files changed, 24 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index bf5807d35339..5e03ab4c0e4f 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -936,6 +936,9 @@ struct kvm_hv { > /* How many vCPUs have VP index != vCPU index */ > atomic_t num_mismatched_vp_indexes; > > + /* How many SynICs use 'AutoEOI' feature */ > + atomic_t synic_auto_eoi_used; > + > struct hv_partition_assist_pg *hv_pa_pg; > struct kvm_hv_syndbg hv_syndbg; > }; > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index f98370a39936..89e7d5b99279 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic, > static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > int vector) > { > + struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic); > + struct kvm_hv *hv = to_kvm_hv(vcpu->kvm); > + int auto_eoi_old, auto_eoi_new; > + > if (vector < HV_SYNIC_FIRST_VALID_VECTOR) > return; > > @@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > else > __clear_bit(vector, synic->vec_bitmap); > > + auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256); > + > if (synic_has_vector_auto_eoi(synic, vector)) > __set_bit(vector, synic->auto_eoi_bitmap); > else > __clear_bit(vector, synic->auto_eoi_bitmap); > + > + auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256); > + > + /* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */ > + if (!auto_eoi_old && auto_eoi_new) { > + if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1) > + kvm_request_apicv_update(vcpu->kvm, false, > + APICV_INHIBIT_REASON_HYPERV); > + } else if (!auto_eoi_new && auto_eoi_old) { > + if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0) > + kvm_request_apicv_update(vcpu->kvm, true, > + APICV_INHIBIT_REASON_HYPERV); > + } A summary of a bug as I explained in my main reply to the patch series: synic_update_vector can be called on vmexit, holding the SRCU lock, and it can't currently call kvm_request_apicv_update with SRCU lock held. because kvm_request_apicv_update indirectly calls synchronize_srcu. Either we have to add a parameter 'host' synic_update_vector that will specify that this function was called on msr write from userspace, or from the guest, and for the latter drop the srcu lock around kvm_request_apicv_update as it is done in svm_toggle_avic_for_irq_window or we should think on how we can make kvm_request_apicv_update avoid the need to use srcu lock. We can for example make it not run avic memslot update on current vcpu, for the synic case, or maybe we can make it avoid memslots update completely. Other than this bug, especially after I did read the SynIC spec, this looks reasonable. One thing though that I noticed in the SynIC spec is that regardless of AutoEOI setting, when we do intercept EOI (which we can't with AVIC) (in apic_set_eoi) we call kvm_hv_synic_send_eoi, which seems to try to raise stimer again on this SINIx. This is not relevant if STIMER is in direct mode though, then I think we don't really send anything through SynIC anyway. So besides the SRCU bug: Tested-by: Maxim Levitsky <mlevitsk@redhat.com> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky > } > > static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, > @@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages) > > synic = to_hv_synic(vcpu); > > - /* > - * Hyper-V SynIC auto EOI SINT's are > - * not compatible with APICV, so request > - * to deactivate APICV permanently. > - */ > - kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV); > synic->active = true; > synic->dont_zero_synic_pages = dont_zero_synic_pages; > synic->control = HV_SYNIC_CONTROL_ENABLE; > @@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, > ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED; > if (!cpu_smt_possible()) > ent->eax |= HV_X64_NO_NONARCH_CORESHARING; > + if (enable_apicv) > + ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED; > /* > * Default number of spinlock retry attempts, matches > * HyperV 2016.
On Mon, 2021-05-24 at 18:21 +0200, Paolo Bonzini wrote: > On 18/05/21 16:43, Vitaly Kuznetsov wrote: > > APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon > > SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is, > > however, possible to track whether the feature was actually used by the > > guest and only inhibit APICv/AVIC when needed. > > > > TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let > > Windows know that AutoEOI feature should be avoided. While it's up to > > KVM userspace to set the flag, KVM can help a bit by exposing global > > APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI > > is still preferred. > > > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > Should it also disable APICv unconditionally if > HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID? That should > avoid ping-pong between enabled and disabled APICv even in pathological > cases that we cannot think about. Probably not worth it, as the guest might still not use it. We already disable/enable AVIC at the rate of a few iterations per second when PIC sends its interrupts via ExtINT, and we need an interrupt window. This is sadly something that windows still does use and it seems to work. Best regards, Maxim Levitsky > > Paolo > > > --- > > arch/x86/include/asm/kvm_host.h | 3 +++ > > arch/x86/kvm/hyperv.c | 27 +++++++++++++++++++++------ > > 2 files changed, 24 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index bf5807d35339..5e03ab4c0e4f 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -936,6 +936,9 @@ struct kvm_hv { > > /* How many vCPUs have VP index != vCPU index */ > > atomic_t num_mismatched_vp_indexes; > > > > + /* How many SynICs use 'AutoEOI' feature */ > > + atomic_t synic_auto_eoi_used; > > + > > struct hv_partition_assist_pg *hv_pa_pg; > > struct kvm_hv_syndbg hv_syndbg; > > }; > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > > index f98370a39936..89e7d5b99279 100644 > > --- a/arch/x86/kvm/hyperv.c > > +++ b/arch/x86/kvm/hyperv.c > > @@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic, > > static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > > int vector) > > { > > + struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic); > > + struct kvm_hv *hv = to_kvm_hv(vcpu->kvm); > > + int auto_eoi_old, auto_eoi_new; > > + > > if (vector < HV_SYNIC_FIRST_VALID_VECTOR) > > return; > > > > @@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > > else > > __clear_bit(vector, synic->vec_bitmap); > > > > + auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256); > > + > > if (synic_has_vector_auto_eoi(synic, vector)) > > __set_bit(vector, synic->auto_eoi_bitmap); > > else > > __clear_bit(vector, synic->auto_eoi_bitmap); > > + > > + auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256); > > + > > + /* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */ > > + if (!auto_eoi_old && auto_eoi_new) { > > + if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1) > > + kvm_request_apicv_update(vcpu->kvm, false, > > + APICV_INHIBIT_REASON_HYPERV); > > + } else if (!auto_eoi_new && auto_eoi_old) { > > + if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0) > > + kvm_request_apicv_update(vcpu->kvm, true, > > + APICV_INHIBIT_REASON_HYPERV); > > + } > > } > > > > static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, > > @@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages) > > > > synic = to_hv_synic(vcpu); > > > > - /* > > - * Hyper-V SynIC auto EOI SINT's are > > - * not compatible with APICV, so request > > - * to deactivate APICV permanently. > > - */ > > - kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV); > > synic->active = true; > > synic->dont_zero_synic_pages = dont_zero_synic_pages; > > synic->control = HV_SYNIC_CONTROL_ENABLE; > > @@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, > > ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED; > > if (!cpu_smt_possible()) > > ent->eax |= HV_X64_NO_NONARCH_CORESHARING; > > + if (enable_apicv) > > + ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED; > > /* > > * Default number of spinlock retry attempts, matches > > * HyperV 2016. > >
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index bf5807d35339..5e03ab4c0e4f 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -936,6 +936,9 @@ struct kvm_hv { /* How many vCPUs have VP index != vCPU index */ atomic_t num_mismatched_vp_indexes; + /* How many SynICs use 'AutoEOI' feature */ + atomic_t synic_auto_eoi_used; + struct hv_partition_assist_pg *hv_pa_pg; struct kvm_hv_syndbg hv_syndbg; }; diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index f98370a39936..89e7d5b99279 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic, static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, int vector) { + struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic); + struct kvm_hv *hv = to_kvm_hv(vcpu->kvm); + int auto_eoi_old, auto_eoi_new; + if (vector < HV_SYNIC_FIRST_VALID_VECTOR) return; @@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, else __clear_bit(vector, synic->vec_bitmap); + auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256); + if (synic_has_vector_auto_eoi(synic, vector)) __set_bit(vector, synic->auto_eoi_bitmap); else __clear_bit(vector, synic->auto_eoi_bitmap); + + auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256); + + /* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */ + if (!auto_eoi_old && auto_eoi_new) { + if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1) + kvm_request_apicv_update(vcpu->kvm, false, + APICV_INHIBIT_REASON_HYPERV); + } else if (!auto_eoi_new && auto_eoi_old) { + if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0) + kvm_request_apicv_update(vcpu->kvm, true, + APICV_INHIBIT_REASON_HYPERV); + } } static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, @@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages) synic = to_hv_synic(vcpu); - /* - * Hyper-V SynIC auto EOI SINT's are - * not compatible with APICV, so request - * to deactivate APICV permanently. - */ - kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV); synic->active = true; synic->dont_zero_synic_pages = dont_zero_synic_pages; synic->control = HV_SYNIC_CONTROL_ENABLE; @@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED; if (!cpu_smt_possible()) ent->eax |= HV_X64_NO_NONARCH_CORESHARING; + if (enable_apicv) + ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED; /* * Default number of spinlock retry attempts, matches * HyperV 2016.
APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is, however, possible to track whether the feature was actually used by the guest and only inhibit APICv/AVIC when needed. TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let Windows know that AutoEOI feature should be avoided. While it's up to KVM userspace to set the flag, KVM can help a bit by exposing global APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI is still preferred. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/include/asm/kvm_host.h | 3 +++ arch/x86/kvm/hyperv.c | 27 +++++++++++++++++++++------ 2 files changed, 24 insertions(+), 6 deletions(-)