Re: [PATCH] KVM: arm/arm64: BUG FIX: Do not inject spurious interrupts

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Aug 28, 2015 at 12:11:17PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > It's a bit weird to just sned this as a new patch without replying to my
> > mail from yesterday with feedback
> 
>  Sorry. But changes are actually minimal, and i remember that i replied to you with the promise of
> testing your suggestion. So, done, works fine. :)
> 
> > I think you're missing a potential change to the irq_pending_on_cpu
> > field here, which you have to compute by calling vgic_update_state()
> > like we do elsewhere when we change status bits
> 
>  I have just checked this. vgic_update_state() never resets this bit. This bit is reset only in
> __kvm_vgic_flush_hwstate() and only if we have consumed completely everything. I have followed
> through the code and looks like it's perfectly safe to have this bit set while nothing is actually
> pendng. Following __kvm_vgic_flush_hwstate(), having this bit cleared is actually a shorthand for
> "no interrupt is pending at all". If it is set without any interrupt actually being pending (this
> ends up in pa_percpu and pa_shared being all zeroes), all three for_each_set_bit() loops will just
> not do anything, and we still get to "epilog:" label, just after a bit longer check. And, since we
> are here, the guest has already been disturbed.

ok, looks like it is functionally correct, but I'm not thrilled about us
setting the state in the VGIC to something is pending on the CPU, when
really there is not.  In that case, we should explicitly mark the bit as
a hint and not rely on its correctness and your patch should explain
this in vgic_update_irq_pending().

The alternative is to just call compute_pending which does nothing more
than a few bitwise and/or operations plus a couple of handfuls of
load/stores on this IRQ injection path, so I don't see the problem doing
this.

Does the code look awful?  If so, why?

> 
> > different from the incorrect approach I suggested yesterday where we
> > always just clear the bit for that vcpu).
> 
>  Yes, it is extremely bad idea to clear it because this bit summarizes all interrupts for this vcpu,
> and clearing it means that we are going to lose everything.

yes, I see this.

>  An alternate would be: clear the bit, THEN call vgic_update_state() which would set it back if
> necessary. But does this extra bit of complexity worth anything, given one paragraph above?
> 
I'm not sure your suggested approach works, because you could still
loose state for other IRQs I think.

-Christoffer