Re: [RFC] Paravirt timer for KVM

Hollis Blanchard
2007-10-12T22:09:59Z

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