Re: [patch 3/3] KVM: PIT: fix injection logic and count

[Marcelo Tosatti <mtosatti@redhat.com>]

Hi Sheng,

On Mon, Jul 28, 2008 at 01:56:59PM +0800, Yang, Sheng wrote:
> Hi Marcelo
> 
> Thanks for your work! It finally reslove my problem on failing to ack 
> some injected interrupts. :)
> 
> On Sunday 27 July 2008 04:01:01 Marcelo Tosatti wrote:
> > The PIT injection logic is problematic under the following cases:
> >
> > 1) If there is a higher priority vector to be delivered by the time
> > kvm_pit_timer_intr_post is invoked ps->inject_pending won't be set.
> > This opens the possibility for missing many PIT event injections
> > (say if guest executes hlt at this point).
> 
> IRQ ack is of course the better way.
> 
> But I am still confuse about why PIT event lost. If higher priority 
> vector there, it would inject first, but finally PIT one should be 
> injected and because PIT inject one interrupt after another had been 
> injected, so no PIT interrupt should be ignored.

The problem is (was) the check for ps->inject_pending in
pit_has_pending_timer(). If the guest hlt'ed just after a higher
priority vector had been injected ps->inject_pending would not
be cleared, so we wouldnt break out of the kvm_vcpu_block() loop
immediately.

In my test case, it would block in kvm_vcpu_block() until a lapic timer
fired, which would then inject that. Repeat forever until the guest
exits for some reason other than hlt.

> > 2) ps->inject_pending is racy with more than two vcpus. Since
> > there's no locking around read/dec of pt->pending, two vcpu's can
> > inject two interrupts for a single pt->pending count.
> >
> > Fix 1 by using an irq ack notifier: only reinject when the previous
> > irq has been acked. Fix 2 with appropriate locking around
> > manipulation of pending count and irq_ack by the injection / ack
> > paths.
> >
> > Also, count_load_time should be set at the time the count is
> > reloaded, not when the interrupt is injected (BTW, LAPIC uses the
> > same apparently broken scheme, could someone explain what was the
> > reasoning behind that? kvm_apic_timer_intr_post).
> 
> PIT did so because we don't know how long we should wait for interrupt 
> injection, so that point is nearer the real injection point. I think 
> lapic have the similar reason.
> 
> I think keep timer_intr_post() for time update is reasonable.

But the real hardware does not do any adjustment of that kind right? The
issue is, when the guest reads the CH0 register (0x40) it expects the
actual count (ie. how long until the timer fires), not any other value.

Failing to return the proper "count" value via CH0 read breaks
"clock=pit" on older Linux 2.6 kernels for example (and really, I don't
see any advantage of setting load count to when injection happens).