Re: [patch 4/8] KVM: x86: replace hrtimer based timer emulation

[Gleb Natapov <gleb@redhat.com>]

On Wed, Jul 08, 2009 at 12:42:52PM -0300, Marcelo Tosatti wrote:
> On Wed, Jul 08, 2009 at 04:39:56PM +0300, Gleb Natapov wrote:
> > On Wed, Jul 08, 2009 at 10:17:21AM -0300, Marcelo Tosatti wrote:
> > > 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?
> > > 
> > This is correctness issue. We should be as close to real CPU as
> > possible. This will save us may hours of debugging later :)
> 
> Hum, fine. Will update the kvm_timer_mask patch below and let you know.
> 
> > > > or timer event can generate NMI instead of interrupt.
> > > 
> > > In the NMI case it should not unhalt the processor?
> > Why? It should. It should jump to NMI handler.
> 
> I meant unhalt as in KVM_REQ_UNHALT so vcpu_enter_guest runs.
> 
Yes. It should. Inside vcpu_enter_guest() NMI will be injected
and nmi handler will be executed.

> What did you mention about ISR/IRR again?
On real HW the following may happen:
 Timer interrupt delivered to apic and placed into IRR
 Timer interrupt delivered to cpu and moved from IRR to ISR
 New timer interrupt delivered to apic and placed into IRR before
  previous one is acked.

In your patch ktimer->can_inject is set to false when timer is injected
and next interrupt is injected only after OS acks previous timer. So the
above situation cannot happen. I don't know if this important or not. It
is possible to write code that will work only with former behaviour, but
I don't see why somebody will want to do that. We can emulate former behaviour
though. If we will no rely on acks to count delivered event but make ->inject
callback return status that will indicate if interrupt was delivered to apic or
not.

--
			Gleb.