Re: [PATCH 1/2] KVM: In-kernel PIT model

Avi Kivity
2008-01-28T11:59:18Z

Yang, Sheng wrote:
> From af5d9ee2a189c2f0998c5c46d1fcd25c5cb72f8c Mon Sep 17 00:00:00 2001
> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Mon, 28 Jan 2008 05:10:22 +0800
> Subject: [PATCH] KVM: In-kernel PIT model
>
> The patch moved PIT from userspace to kernel, and increase the timer accuracy
> greatly.
>

> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> new file mode 100644
> index 0000000..73b8240
> --- /dev/null
> +++ b/arch/x86/kvm/i8254.c
> +/* Compute with 96 bit intermediate result: (a*b)/c */
> +static u64 muldiv64(u64 a, u32 b, u32 c)
> +{
> + union {
> + u64 ll;
> + struct {
> +#ifdef WORDS_BIGENDIAN
> + u32 high, low;
> +#else
> + u32 low, high;
> +#endif
>

Since this is in arch/x86, we're implicitly little endian, so you can
remove the ifdef.

> +
> +static int pit_get_count(struct kvm *kvm, int channel)
> +{
> + struct pit_channel_state *c =
> + &kvm->arch.vpit->pit_state.channels[channel];
> + s64 d, t;
> + int counter;
> +
> + ASSERT(mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
> +
> + t = ktime_to_ns(ktime_sub(ktime_get(), c->count_load_time));
> + d = muldiv64(t, PIT_FREQ, 1e9);
>

1e9 is a float, which is frowned upon in the kernel. While it is
converted in compile time, it is better to use an int (like NSEC_PER_SEC).

> diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
> new file mode 100644
> index 0000000..fcff9fb
> --- /dev/null
> +++ b/arch/x86/kvm/i8254.h
> @@ -0,0 +1,62 @@
> +
> +extern unsigned int cpu_khz;
>

unused, please drop.

> +
> +struct pit_timer {
> +
> +struct pit_channel_state {
> +
> +struct pit_state {
>

These names are too generic, please prefix with kvm_ so nobody confuses
them with host hardware.

> +
> +struct kvm_pit {
> + unsigned long base_addresss;
> + struct kvm_io_device dev;
> + struct kvm_io_device speaker_dev;
> + struct kvm *kvm;
> + struct pit_state pit_state;
> + atomic_t inject_pending; /* if inject pending interrupts */
>

inject_pending is only used with atomic_set() and atomic_read(); a
simple bool is atomic for simple read/write ops.

Need save/restore support.

--
error compiling committee.c: too many arguments to function

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.