Re: [RFC][PATCH 1/3] Introduce a workqueue to deliver PIT timer interrupts.

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Jun 08, 2010 at 01:55:01PM -0400, Chris Lalancette wrote:
> We really want to "kvm_set_irq" during the hrtimer callback,
> but that is risky because that is during interrupt context.
> Instead, offload the work to a workqueue, which is a bit safer
> and should provide most of the same functionality.
> 
> Signed-off-by: Chris Lalancette <clalance@redhat.com>
> ---
>  arch/x86/kvm/i8254.c |  106 ++++++++++++++++++++++++++++++-------------------
>  arch/x86/kvm/i8254.h |    4 ++
>  arch/x86/kvm/irq.c   |    1 -
>  arch/x86/kvm/x86.c   |    4 ++
>  4 files changed, 73 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 188d827..dd655ef 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -34,6 +34,7 @@
>  
>  #include <linux/kvm_host.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>  
>  #include "irq.h"
>  #include "i8254.h"
> @@ -49,6 +50,8 @@
>  #define RW_STATE_WORD0 3
>  #define RW_STATE_WORD1 4
>  
> +static struct workqueue_struct *pit_wq;
> +
>  /* Compute with 96 bit intermediate result: (a*b)/c */
>  static u64 muldiv64(u64 a, u32 b, u32 c)
>  {
> @@ -281,6 +284,58 @@ static struct kvm_timer_ops kpit_ops = {
>  	.is_periodic = kpit_is_periodic,
>  };
>  
> +static void pit_do_work(struct work_struct *work)
> +{
> +       struct kvm_pit *pit = container_of(work, struct kvm_pit, expired);
> +       struct kvm *kvm = pit->kvm;
> +       struct kvm_vcpu *vcpu;
> +       int i;
> +       struct kvm_kpit_state *ps = &pit->pit_state;
> +       int inject = 0;
> +
> +       /* Try to inject pending interrupts when
> +        * last one has been acked.
> +        */
> +       raw_spin_lock(&ps->inject_lock);
> +       if (ps->irq_ack) {
> +               ps->irq_ack = 0;
> +               inject = 1;
> +       }
> +       raw_spin_unlock(&ps->inject_lock);
> +       if (inject) {
> +               kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
> +               kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
> +
> +               /*
> +                * Provides NMI watchdog support via Virtual Wire mode.
> +                * The route is: PIT -> PIC -> LVT0 in NMI mode.
> +                *
> +                * Note: Our Virtual Wire implementation is simplified, only
> +                * propagating PIT interrupts to all VCPUs when they have set
> +                * LVT0 to NMI delivery. Other PIC interrupts are just sent to
> +                * VCPU0, and only if its LVT0 is in EXTINT mode.
> +                */
> +               if (kvm->arch.vapics_in_nmi_mode > 0)
> +                       kvm_for_each_vcpu(i, vcpu, kvm)
> +                               kvm_apic_nmi_wd_deliver(vcpu);
> +       }
> +}
> +
> +static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
> +{
> +       struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
> +       struct kvm_pit *pt = ktimer->kvm->arch.vpit;
> +
> +       queue_work(pit_wq, &pt->expired);
> +
> +       if (ktimer->t_ops->is_periodic(ktimer)) {
> +               hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> +              return HRTIMER_RESTART;
> +       }
> +       else
> +              return HRTIMER_NORESTART;
> +}

Certain guests do not compensate for lost ticks (older RHEL3 for ex),
and this completly reinjection.

Perhaps you can increase the pending counter on pit_timer_fn, and
reinject on ACK?

Also, due to
http://www.mail-archive.com/kvm-devel@lists.sourceforge.net/msg13250.html,
it should be safer to keep PIT locked to vcpu0 if IOAPIC dest mode is 
lowprio.