Re: [PATCH 7/7] KVM: arm64: Add irq_inject counter for kvm_stat

From: Marc Zyngier <maz@kernel.org>
To: Yoan Picchi <yoan.picchi@arm.com>
Cc: james.morse@arm.com, julien.thierry.kdev@gmail.com,
	suzuki.poulose@arm.com, linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, catalin.marinas@arm.com,
	will@kernel.org
Subject: Re: [PATCH 7/7] KVM: arm64: Add irq_inject counter for kvm_stat
Date: Tue, 23 Mar 2021 18:36:31 +0000	[thread overview]
Message-ID: <87ft0lk98w.wl-maz@kernel.org> (raw)
In-Reply-To: <c1ca65a5-1b7e-8ec2-ecbb-8ceda973bb14@arm.com>

On Tue, 23 Mar 2021 17:53:42 +0000,
Yoan Picchi <yoan.picchi@arm.com> wrote:
> 
> Hi Mark.

s/k/c/, please!

> 
> Thanks for all the reviews. I am a beginner and you gave me a lot to
> learn about.  I will reply to the other patch progressively once I
> understand better the issues.

I think you should consider what I said in my reply to the cover
letter before going all out on every counter you have introduced in
this series.

[...]

> >> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> >> index 1c597c988..9e504243b 100644
> >> --- a/arch/arm64/kvm/vgic/vgic.c
> >> +++ b/arch/arm64/kvm/vgic/vgic.c
> >> @@ -458,6 +458,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >>     	raw_spin_lock_irqsave(&irq->irq_lock, flags);
> >>   +	kvm->stat.irq_inject++;
> >> +
> >>   	if (!vgic_validate_injection(irq, level, owner)) {
> > So even if the injection failed, you report an injection? And what
> > about injection that occur via the MMIO interface? What about direct
> > injection? What about a level interrupt that is forever high?
> > 
> > 	M.
> > 
> This one I actually started to fix this afternoon by moving the
> counter into vgic_queue_irq_unlock().  This way it is only
> incremented when the interrupt is inserted into a vcpu, and it also
> takes care of the vgic_mmio injections. I also fixed the issue with
> the interrupt line so it only increment when the line change of
> level.

But if you do that, you start counting interrupts the guest itself
generates. What is the exact semantic of this counter? userspace
injected interrupts? Acked interrupts? Any interrupt?

Take my level interrupt example. The interrupt will be forever
pending, the guest will take as many interrupt as it can process, and
yet your counter will have been incremented *once*. What does your
counter mean then?

> I'm not sure about what you mean by direct injection yet though.

GICv4.{0,1}, where an interrupt gets directly delivered to the guest
without (too much) SW intervention. With this, directly injected LPIs
will never result in the counter being incremented, and yet can pin
the guest to the ground under interrupt load.

Again, defining the exact behaviour of the counter would avoid me
ranting away...

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel