Message-ID: <ZmMjHwavCLk0lRd7@google.com>
Date: Fri, 7 Jun 2024 08:11:27 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Alejandro Jimenez <alejandro.j.jimenez@...cle.com>
Cc: kvm@...r.kernel.org, pbonzini@...hat.com, linux-kernel@...r.kernel.org, 
	suravee.suthikulpanit@....com, vashegde@....com, mlevitsk@...hat.com, 
	joao.m.martins@...cle.com, boris.ostrovsky@...cle.com, mark.kanda@...cle.com
Subject: Re: [PATCH 4/4] KVM: x86: Add vCPU stat for APICv interrupt
 injections causing #VMEXIT

On Thu, Jun 06, 2024, Alejandro Jimenez wrote:
> On 6/3/24 20:14, Sean Christopherson wrote:
> > On Mon, Apr 29, 2024, Alejandro Jimenez wrote:
> > > Even when APICv/AVIC is active, certain guest accesses to its local APIC(s)
> > > cannot be fully accelerated, and cause a #VMEXIT to allow the VMM to
> > > emulate the behavior and side effects. Expose a counter stat for these
> > > specific #VMEXIT types.
> > > 
> > > Suggested-by: Paolo Bonzini <pbonzini@...hat.com>
> > > Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@...cle.com>
> > > ---
> > >   arch/x86/include/asm/kvm_host.h | 1 +
> > >   arch/x86/kvm/svm/avic.c         | 7 +++++++
> > >   arch/x86/kvm/vmx/vmx.c          | 2 ++
> > >   arch/x86/kvm/x86.c              | 1 +
> > >   4 files changed, 11 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index e7e3213cefae..388979dfe9f3 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -1576,6 +1576,7 @@ struct kvm_vcpu_stat {
> > >   	u64 guest_mode;
> > >   	u64 notify_window_exits;
> > >   	u64 apicv_active;
> > > +	u64 apicv_unaccelerated_inj;
> > 
> > The stat name doesn't match the changelog or the code.  The AVIC updates in
> > avic_incomplete_ipi_interception() are unaccelerated _injection_, they're
> > unaccelarated _delivery_.  And in those cases, the fact that delivery wasn't
> > accelerated is relatively uninteresting in most cases.
> > 
> 
> Yeah, this was my flawed attempt to interpret/implement Paolo's comment in
> the RFC thread:
> 
> "... for example I'd add an interrupt_injections stat for unaccelerated
> injections causing a vmexit or otherwise hitting lapic.c"

KVM essentially already has this stat, irq_injections.  Sort of.  The problem is
that the stat isn't bumped when APICv is enabled because the IRQ isn't *directly*
injected.  KVM does "inject" the IRQ into the IRR (and RVI on Intel), but KVM
doesn't go through .inject_irq().

For APICv, KVM could bump the stat when manually moving the IRQ from the IRR to
RVI, but that'd be more than a bit misleading with respect to AVIC.  With AVIC,
the CPU itself processes the IRR on VMRUN, i.e. there's no software intervention
needed to get the CPU to inject the IRQ.  But practically speaking, there's no
meaningful difference between the two flows; in both cases an IRQ arrived while
the target vCPU wasn't actively running the guest.  And that means KVM would need
to parse the IRR on AMD just to bump a stat.

It'd also be misleading to some extent in general, because when the target vCPU
is in its inner run loop, but not actually post-VM-Enter, KVM doesn't kick the
vCPU because either KVM or the CPU will automatically process the pending IRQ.
I.e. KVM would bump the stat cases where the injection isn't fully accelerated,
but that's somewhat disingenuous because KVM didn't need to slow down the vCPU
in order to deliver the interrupt.

And KVM already has an irq_exits stat, which can be used to get a rough feel for
how often KVM is kicking a vCPU (though timer ticks likely dominate the stat).

> > And avic_unaccelerated_access_interception() and handle_apic_write() don't
> > necessarily have anything to do with injection.
> 
> apicv_unaccelerated_acccess is perhaps a better name (assuming stat is
> updated in handle_apic_access() as well)?

This is again not super interesting.  If we were to add this stat, I would lobby
hard for turning "exits" into an array that accounts each individual VM-Exit,
though with some massaging to reconcile differences between VMX and SVM.

Unaccelerated APIC exits aren't completely uninteresting, but they suffer similar
issues to the "exits" stat: a few flavors of APIC exits would dominate the stats,
and _those_ exits aren't very interesting.

> > On the flip side, the slow paths for {svm,vmx}_deliver_interrupt() are very
> > explicitly unnaccelerated injection.
> 
> Now that you highlight this, I think it might be closer to Paolo's idea. i.e.
> a stat for the slow path on these can be contrasted/compared with the
> kvm_apicv_accept_irq tracepoint that is hit on the fast path.  My initial
> reaction would be to update a stat for the fast path, as a confirmation that
> apicv is active which is how/why I typically use the kvm_apicv_accept_irq
> tracepoint, but that becomes redundant by having the apicv_active stat on
> PATCH 1.
> 
> So, if you don't think it is useful to have a general
> apicv_unaccelerated_acccess counter, I can drop this patch.

The one thing I can think of that might be somewhat interesting is when
kvm_apic_send_ipi() is invoked to deliver an IPI.  If KVM manually sends the IPI,
and IPI virtualization is enabled (on-by-default in AVIC, and an add-on feature
for APICv), then it means IPI virtualization isn't doing it's job for whatever
reason.  But even then, I'm doubt it's worth a stat, because it likely just means
the guest is doing something weird, not that there's a problem in KVM.

Powered by blists - more mailing lists