Re: [RFC] Paravirt timer for KVM

[Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

On Fri, 2007-10-12 at 13:08 -0300, Glauber de Oliveira Costa wrote:
> +
> +/* The hypervisor will put information about time periodically here */
> +struct kvm_hv_clock hv_clock;

...

> +void __init kvmclock_init(void)
> +{
> +
> +       unsigned long shared_page = (unsigned long)&hv_clock;

What makes you think this is page-aligned?

> +       /*
> +        * If we can't use the paravirt clock, just go with
> +        * the usual timekeeping
> +        */
> +       if (!kvm_para_available() || no_kvmclock)
> +               return;
> +
> +       if (kvm_hypercall1(KVM_HCALL_SET_SHARED_PAGE, shared_page))
> +               return;

Here you're passing a virtual address across the guest/host interface,
which is something we've previously agreed is bad.

> @@ -1406,6 +1408,7 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_emulate_halt);
>  
> +static struct kvm_hv_clock hv_clock;
>  int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  {
>         unsigned long nr, a0, a1, a2, a3, ret;
> @@ -1426,7 +1429,32 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>                 a3 &= 0xFFFFFFFF;
>         }
>  
> +       ret = 0;
>         switch (nr) {
> +       case  KVM_HCALL_SET_SHARED_PAGE:
> +               if (!irqchip_in_kernel(vcpu->kvm)) {
> +                       ret = -1;
> +                       break;
> +               }
> +
> +               vcpu->kvm->clock_addr = a0;
> +               getnstimeofday(&hv_clock.wc);
> +               hv_clock.stable_tsc = check_tsc_unstable();
> +               hv_clock.tsc_mult = clocksource_khz2mult(tsc_khz, 22);
> +               rdtscll(hv_clock.last_tsc);
> +               emulator_write_emulated(vcpu->kvm->clock_addr, &hv_clock,
> +                                               sizeof(hv_clock), vcpu);

Are you intending this shared page to contain other data in the future,
or is it just poorly named?

> +struct kvm_hv_clock {
> +       int stable_tsc; /* use raw tsc for clock_read */
> +       unsigned long tsc_mult;
> +       struct timespec now;
> +       u64 last_tsc;
> +       /* That's the wall clock, not the water closet */
> +       struct timespec wc;
> +};

This isn't even good for x86: you're using "long" in a binary interface!
You're also using timespec, and what sort of binary compatibility
guarantees would you like to make about that?

-- 
Hollis Blanchard
IBM Linux Technology Center


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/