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

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

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.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/