On Wed, Jul 08, 2009 at 03:58:19PM +0300, Gleb Natapov wrote:
> Excellent patch series.
> 
> On Sun, Jul 05, 2009 at 10:55:15PM -0300, Marcelo Tosatti wrote:
> >  int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
> >  {
> > -	int ret;
> > +	ktime_t now, expires;
> >  
> > -	ret = pit_has_pending_timer(vcpu);
> > -	ret |= apic_has_pending_timer(vcpu);
> > +	expires = kvm_vcpu_next_timer_event(vcpu);
> > +	now = ktime_get();
> > +	if (expires.tv64 <= now.tv64) {
> > +		if (kvm_arch_interrupt_allowed(vcpu))
> > +			set_bit(KVM_REQ_UNHALT, &vcpu->requests);
> You shouldn't unhalt vcpu here. Not every timer event will generate
> interrupt (vector can be masked in pic/ioapic)

Yeah. Note however that kvm_vcpu_next_timer_event only returns the
expiration time for events that have been acked (for timers that have
had events injected, but not acked it returns KTIME_MAX).

So, the code above will set one spurious unhalt if:

- inject timer irq
- guest acks irq
- guest mask irq
- unhalt (once)

I had a "kvm_timer_mask" callback before (along with the attached
patch), but decided to keep it simpler using the ack trick above.

I suppose one spurious unhalt is harmless, or is it a correctness issue?

> or timer event can generate NMI instead of interrupt.

In the NMI case it should not unhalt the processor?

(but yes, bypassing the irq injection system its not a very beatiful
shortcut, but its done in other places too eg i8254.c NMI injection via
all cpus LINT0).

> Leaving this code out probably means
> that you can't remove kvm_inject_pending_timer_irqs() call from
> __vcpu_run().
> 
> > +		return 1;
> > +	}
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL(kvm_cpu_has_pending_timer);
> >  
> 
> --
> 			Gleb.