[PATCH v3] KVM: x86: Send EOI to SynIC vectors on accelerated EOI-induced VM-Exits

[PATCH v3] KVM: x86: Send EOI to SynIC vectors on accelerated EOI-induced VM-Exits

[Wang Guangju]

When EOI virtualization is performed on VMX, kvm_apic_set_eoi_accelerated()
is called upon EXIT_REASON_EOI_INDUCED but unlike its non-accelerated
apic_set_eoi() sibling, Hyper-V SINT vectors are left unhandled.

Send EOI to Hyper-V SINT vectors when handling acclerated EOI-induced
VM-Exits. KVM Hyper-V needs to handle the SINT EOI irrespective of whether
the EOI is acclerated or not.

Rename kvm_apic_set_eoi_accelerated() to kvm_apic_set_eoi() and let the
non-accelerated helper call the "acclerated" version. That will document
the delta between the non-accelerated path and the accelerated path.
In addition, guarantee to trace even if there's no valid vector to EOI in
the non-accelerated path in order to keep the semantics of the function
intact.

Fixes: 5c919412fe61 ("kvm/x86: Hyper-V synthetic interrupt controller")
Cc: <stable@vger.kernel.org>
Tested-by: Wang Guangju <wangguangju@baidu.com>
Suggested-by: Sean Christopherson <seanjc@google.com>
Suggested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Co-developed-by: Li Rongqing <lirongqing@baidu.com>
Signed-off-by: Wang Guangju <wangguangju@baidu.com>
---
 v1 -> v2: Updated the commit message and implement a new inline function
 of apic_set_eoi_vector()

 v2 -> v3: Updated the subject and commit message, drop func 
 apic_set_eoi_vector() and rename kvm_apic_set_eoi_accelerated() 
 to kvm_apic_set_eoi()

 arch/x86/kvm/lapic.c   | 45 ++++++++++++++++++++++-----------------------
 arch/x86/kvm/lapic.h   |  2 +-
 arch/x86/kvm/vmx/vmx.c |  3 ++-
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index f03facc..b2e72ab 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1269,46 +1269,45 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
 	kvm_ioapic_update_eoi(apic->vcpu, vector, trigger_mode);
 }
 
+/*
+ * Send EOI for a valid vector.  The caller, or hardware when this is invoked
+ * after an accelerated EOI VM-Exit, is responsible for updating the vISR and
+ * vPPR.
+ */
+void kvm_apic_set_eoi(struct kvm_lapic *apic, int vector)
+{
+	trace_kvm_eoi(apic, vector);
+
+	if (to_hv_vcpu(apic->vcpu) &&
+	    test_bit(vector, to_hv_synic(apic->vcpu)->vec_bitmap))
+		kvm_hv_synic_send_eoi(apic->vcpu, vector);
+
+	kvm_ioapic_send_eoi(apic, vector);
+	kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
+}
+EXPORT_SYMBOL_GPL(kvm_apic_set_eoi);
+
 static int apic_set_eoi(struct kvm_lapic *apic)
 {
 	int vector = apic_find_highest_isr(apic);
 
-	trace_kvm_eoi(apic, vector);
-
 	/*
 	 * Not every write EOI will has corresponding ISR,
 	 * one example is when Kernel check timer on setup_IO_APIC
 	 */
-	if (vector == -1)
+	if (vector == -1) {
+		trace_kvm_eoi(apic, vector);
 		return vector;
+	}
 
 	apic_clear_isr(vector, apic);
 	apic_update_ppr(apic);
 
-	if (to_hv_vcpu(apic->vcpu) &&
-	    test_bit(vector, to_hv_synic(apic->vcpu)->vec_bitmap))
-		kvm_hv_synic_send_eoi(apic->vcpu, vector);
+	kvm_apic_set_eoi(apic, vector);
 
-	kvm_ioapic_send_eoi(apic, vector);
-	kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
 	return vector;
 }
 
-/*
- * this interface assumes a trap-like exit, which has already finished
- * desired side effect including vISR and vPPR update.
- */
-void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector)
-{
-	struct kvm_lapic *apic = vcpu->arch.apic;
-
-	trace_kvm_eoi(apic, vector);
-
-	kvm_ioapic_send_eoi(apic, vector);
-	kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
-}
-EXPORT_SYMBOL_GPL(kvm_apic_set_eoi_accelerated);
-
 void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
 	struct kvm_lapic_irq irq;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 762bf61..48260fa 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -126,7 +126,7 @@ u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
 void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data);
 
 void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset);
-void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector);
+void kvm_apic_set_eoi(struct kvm_lapic *apic, int vector);
 
 int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr);
 void kvm_lapic_sync_from_vapic(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9258468..f8b9eb1 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5519,9 +5519,10 @@ static int handle_apic_eoi_induced(struct kvm_vcpu *vcpu)
 {
 	unsigned long exit_qualification = vmx_get_exit_qual(vcpu);
 	int vector = exit_qualification & 0xff;
+	struct kvm_lapic *apic = vcpu->arch.apic;
 
 	/* EOI-induced VM exit is trap-like and thus no need to adjust IP */
-	kvm_apic_set_eoi_accelerated(vcpu, vector);
+	kvm_apic_set_eoi(apic, vector);
 	return 1;
 }
 
-- 
2.9.4

Re: [PATCH v3] KVM: x86: Send EOI to SynIC vectors on accelerated EOI-induced VM-Exits

[Maxim Levitsky]

On Tue, 2022-07-12 at 20:32 +0800, Wang Guangju wrote:
> When EOI virtualization is performed on VMX, kvm_apic_set_eoi_accelerated()
> is called upon EXIT_REASON_EOI_INDUCED but unlike its non-accelerated
> apic_set_eoi() sibling, Hyper-V SINT vectors are left unhandled.
> 
> Send EOI to Hyper-V SINT vectors when handling acclerated EOI-induced
> VM-Exits. KVM Hyper-V needs to handle the SINT EOI irrespective of whether
> the EOI is acclerated or not.

How does this relate to the AutoEOI feature, and the fact that on AVIC,
it can't intercept EOI at all (*)?

Best regards,
	Maxim Levitsky


(*) AVIC does intercept EOI write but only for level triggered interrupts.

> 
> Rename kvm_apic_set_eoi_accelerated() to kvm_apic_set_eoi() and let the
> non-accelerated helper call the "acclerated" version. That will document
> the delta between the non-accelerated path and the accelerated path.
> In addition, guarantee to trace even if there's no valid vector to EOI in
> the non-accelerated path in order to keep the semantics of the function
> intact.
> 
> Fixes: 5c919412fe61 ("kvm/x86: Hyper-V synthetic interrupt controller")
> Cc: <stable@vger.kernel.org>
> Tested-by: Wang Guangju <wangguangju@baidu.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Suggested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Co-developed-by: Li Rongqing <lirongqing@baidu.com>
> Signed-off-by: Wang Guangju <wangguangju@baidu.com>
> ---
>  v1 -> v2: Updated the commit message and implement a new inline function
>  of apic_set_eoi_vector()
> 
>  v2 -> v3: Updated the subject and commit message, drop func 
>  apic_set_eoi_vector() and rename kvm_apic_set_eoi_accelerated() 
>  to kvm_apic_set_eoi()
> 
>  arch/x86/kvm/lapic.c   | 45 ++++++++++++++++++++++-----------------------
>  arch/x86/kvm/lapic.h   |  2 +-
>  arch/x86/kvm/vmx/vmx.c |  3 ++-
>  3 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index f03facc..b2e72ab 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1269,46 +1269,45 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>         kvm_ioapic_update_eoi(apic->vcpu, vector, trigger_mode);
>  }
>  
> +/*
> + * Send EOI for a valid vector.  The caller, or hardware when this is invoked
> + * after an accelerated EOI VM-Exit, is responsible for updating the vISR and
> + * vPPR.
> + */
> +void kvm_apic_set_eoi(struct kvm_lapic *apic, int vector)
> +{
> +       trace_kvm_eoi(apic, vector);
> +
> +       if (to_hv_vcpu(apic->vcpu) &&
> +           test_bit(vector, to_hv_synic(apic->vcpu)->vec_bitmap))
> +               kvm_hv_synic_send_eoi(apic->vcpu, vector);
> +
> +       kvm_ioapic_send_eoi(apic, vector);
> +       kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
> +}
> +EXPORT_SYMBOL_GPL(kvm_apic_set_eoi);
> +
>  static int apic_set_eoi(struct kvm_lapic *apic)
>  {
>         int vector = apic_find_highest_isr(apic);
>  
> -       trace_kvm_eoi(apic, vector);
> -
>         /*
>          * Not every write EOI will has corresponding ISR,
>          * one example is when Kernel check timer on setup_IO_APIC
>          */
> -       if (vector == -1)
> +       if (vector == -1) {
> +               trace_kvm_eoi(apic, vector);
>                 return vector;
> +       }
>  
>         apic_clear_isr(vector, apic);
>         apic_update_ppr(apic);
>  
> -       if (to_hv_vcpu(apic->vcpu) &&
> -           test_bit(vector, to_hv_synic(apic->vcpu)->vec_bitmap))
> -               kvm_hv_synic_send_eoi(apic->vcpu, vector);
> +       kvm_apic_set_eoi(apic, vector);
>  
> -       kvm_ioapic_send_eoi(apic, vector);
> -       kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
>         return vector;
>  }
>  
> -/*
> - * this interface assumes a trap-like exit, which has already finished
> - * desired side effect including vISR and vPPR update.
> - */
> -void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector)
> -{
> -       struct kvm_lapic *apic = vcpu->arch.apic;
> -
> -       trace_kvm_eoi(apic, vector);
> -
> -       kvm_ioapic_send_eoi(apic, vector);
> -       kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
> -}
> -EXPORT_SYMBOL_GPL(kvm_apic_set_eoi_accelerated);
> -
>  void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
>  {
>         struct kvm_lapic_irq irq;
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 762bf61..48260fa 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -126,7 +126,7 @@ u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
>  void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data);
>  
>  void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset);
> -void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector);
> +void kvm_apic_set_eoi(struct kvm_lapic *apic, int vector);
>  
>  int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr);
>  void kvm_lapic_sync_from_vapic(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9258468..f8b9eb1 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5519,9 +5519,10 @@ static int handle_apic_eoi_induced(struct kvm_vcpu *vcpu)
>  {
>         unsigned long exit_qualification = vmx_get_exit_qual(vcpu);
>         int vector = exit_qualification & 0xff;
> +       struct kvm_lapic *apic = vcpu->arch.apic;
>  
>         /* EOI-induced VM exit is trap-like and thus no need to adjust IP */
> -       kvm_apic_set_eoi_accelerated(vcpu, vector);
> +       kvm_apic_set_eoi(apic, vector);
>         return 1;
>  }
>  

Re: [PATCH v3] KVM: x86: Send EOI to SynIC vectors on accelerated EOI-induced VM-Exits

[Sean Christopherson]

On Tue, Jul 12, 2022, Maxim Levitsky wrote:
> On Tue, 2022-07-12 at 20:32 +0800, Wang Guangju wrote:
> > When EOI virtualization is performed on VMX, kvm_apic_set_eoi_accelerated()
> > is called upon EXIT_REASON_EOI_INDUCED but unlike its non-accelerated
> > apic_set_eoi() sibling, Hyper-V SINT vectors are left unhandled.
> > 
> > Send EOI to Hyper-V SINT vectors when handling acclerated EOI-induced
> > VM-Exits. KVM Hyper-V needs to handle the SINT EOI irrespective of whether
> > the EOI is acclerated or not.
> 
> How does this relate to the AutoEOI feature, and the fact that on AVIC,
> it can't intercept EOI at all (*)?
> 
> Best regards,
> 	Maxim Levitsky
> 
> 
> (*) AVIC does intercept EOI write but only for level triggered interrupts.

If there are one or more AutoEOI vectors, KVM disables AVIC.  Which begs the question
of why SVM doesn't disable the AVIC if there's an edge-triggered I/O APIC interrupt
that has a notifier, which is where kvm_hv_notify_acked_sint() eventually ends up.
vmx_load_eoi_exitmap() sets the EOI intercept for all such vectors, and for _all_
SynIC vectors (see vcpu_load_eoi_exitmap()), but AFAICT SVM relies purely on the
level-triggered behavior.

KVM manually disables AVIC for PIT reinjection, which uses an ack notifier;
AFAICT that's a one-off hack to workaround AVIC not playing nice with notifiers.

So yeah, it seems like the proper fix would be to add svm_load_eoi_exitmap() and
replace the PIT inhibit with a generic ACK inhibit that is set if there is at least
one edge-triggered vector present in the eoi_exit_bitmap.

Tangentially related to all of this, it's bizarre/confusing the KVM_CREATE_PIT{2}
is allowed regardless of whether or not the I/O APIC is in-kernel.  I don't see
how it can possibly work since create_pit_timer() silently does nothing if the I/O
APIC isn't in-kernel.