Re: [Qemu-devel] [PATCH] KVM: emulate lapic tsc deadline timer for hvm

[Tian, Kevin]

> From: Marcelo Tosatti
> Sent: Tuesday, August 23, 2011 6:47 PM
> 
> > >+          if (!apic->lapic_timer.tscdeadline)
> > >+                  return;
> > >+
> > >+          tsc_target = kvm_x86_ops->
> > >+                  guest_to_host_tsc(apic->lapic_timer.tscdeadline);
> > >+          rdtscll(tsc_now);
> > >+          tsc_delta = tsc_target - tsc_now;
> >
> > This only works if we have a constant tsc, that's not true for large

that type of machine exposes tricky issues to time virtualization in 
general.

> > multiboard machines.  Need to do this with irqs disabled as well
> > (reading both 'now' and 'tsc_now' in the same critical section).
> 
> Should look like this:
> 
> local_irq_disable();
> u64 guest_tsc = kvm_x86_ops->read_l1_tsc(vcpu);
> if (guest_tsc <= tscdeadline)
>         hrtimer_start(now);
> else {
>       ns = convert_to_ns(guest_tsc - tscdeadline);
>       hrtimer_start(now + ns);
> }
> local_irq_enable();

Above is an overkill. only calculating guest tsc delta needs be protected.

On the other hand, I don't think masking irq here adds obvious benefit.
Virtualization doesn't ensure micro-level time accuracy, since there're
many events delaying virtual time interrupt injection even when timer
emulation logic triggers it timely. That's even the case on bare metal
though with smaller scope, and thus most guests are able to handle 
such situation. masking irq in non-critical regions simply slow down the
system response on other events. 

> 
> Note the vcpus tsc can have different frequency than the hosts, so
> vcpu_tsc_khz() should be used to convert to nanoseconds, not tsc_khz.
> 

yes, that's a good suggestion.

btw, Jinsong is on vacation this week. He will update the patch according
to you comment when back.

Thanks
Kevin