Re: [RFC] Paravirt timer for KVM

Anthony Liguori
2007-10-12T18:10:51Z

Glauber de Oliveira Costa wrote:
> Hi,
>
> Attached is a first draft to a paravirt implementation for a timer to
> KVM. It is inspired in anthony's last patch about it, but not that
> much based on it.
>
> I'm not using hypercalls to get the current time, but rather,
> registering an address that will get timer updates once in a while.
>
> Also, it includes a clockevent oneshot implementation (which is the
> very thing of this patch), that will allow us interest things like
> dynticks.
>
> It's still not yet working on SMP, and I'm currently not sure why (ok,
> ok, if you actually read the patch, it will become obvious the why: it
> only delivers interrupt for vector 0x20, but I'm further with it, this
> patch is just a snapshot)
>
> My next TODOs with it are:
> * Get SMP working
> * Try something for stolen time, as jeremy's last suggestion for anthony's patch
> * Measure the time it takes for a hypercall, and subtract this time
> for calculating the expiry time for the timer event.
> * Testing and fixing bugs: I'm sure they exist!
>
> Meanwhile, all your suggestions are welcome.
>
>

<snip>

> +
> +void __init kvmclock_init(void)
> +{
> +
> + unsigned long shared_page = (unsigned long)&hv_clock;
> + /*
> + * If we can't use the paravirt clock, just go with
> + * the usual timekeeping
> + */
> + if (!kvm_para_available() || no_kvmclock)
> + return;
>

You should also check kvm_para_has_feature() and define a feature flag
for the clock.

> + if (kvm_hypercall1(KVM_HCALL_SET_SHARED_PAGE, shared_page))
> + return;
> +
> + paravirt_ops.get_wallclock = kvm_get_wallclock;
> + paravirt_ops.set_wallclock = kvm_set_wallclock;
> + paravirt_ops.sched_clock = kvm_sched_clock;
> + paravirt_ops.time_init = kvm_time_init;
> + /*
> + * If we let the normal APIC initialization code run, they will
> + * override our event handler, relying that the APIC will deliver
> + * the interrupts in the LOCAL_TIMER_VECTOR. The easy solution is
> + * keep the PIT running until then
> + */
> + paravirt_ops.setup_boot_clock = kvm_disable_pit;
> +}

> diff --git a/drivers/kvm/irq.c b/drivers/kvm/irq.c
> index 0f663fe..7baf798 100644
> --- a/drivers/kvm/irq.c
> +++ b/drivers/kvm/irq.c
> @@ -32,6 +32,8 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
> {
> struct kvm_pic *s;
>
> + if (v->timer_vector != -1)
> + return 1;
> if (kvm_apic_has_interrupt(v) == -1) { /* LAPIC */
> if (kvm_apic_accept_pic_intr(v)) {
> s = pic_irqchip(v->kvm); /* PIC */
> @@ -43,6 +45,12 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
> }
> EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
>
> +static int kvm_get_pvclock_interrupt(struct kvm_vcpu *v)
> +{
> + int ret = v->timer_vector;
> + v->timer_vector = -1;
> + return ret;
> +}
> /*
> * Read pending interrupt vector and intack.
> */
> @@ -51,7 +59,9 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
> struct kvm_pic *s;
> int vector;
>
> - vector = kvm_get_apic_interrupt(v); /* APIC */
> + vector = kvm_get_pvclock_interrupt(v);
> + if (vector == -1)
> + vector = kvm_get_apic_interrupt(v); /* APIC */
>

It might be better to just rely on the in-kernel APIC to inject an
interrupt for the clock (via kvm_pic_set_irq()).

Regards,

Anthony LIguori

> if (vector == -1) {
> if (kvm_apic_accept_pic_intr(v)) {
> s = pic_irqchip(v->kvm)