Re: [PATCH 1/6] KVM: In kernel pit model

[Ingo Molnar <mingo@elte.hu>]

* Yang, Sheng <sheng.yang@intel.com> wrote:

> +#if 1
> +#define pit_debug(fmt, arg...) printk(KERN_WARNING fmt, ##arg)
> +#else
> +#define pit_debug(fmt, arg...)
> +#endif

this should use pr_debug() instead i guess.

> +#ifndef CONFIG_X86_64
> +#define mod_64(x, y) ((x) - (y) * div64_64(x, y))
> +#else
> +#define mod_64(x, y) ((x) % (y))
> +#endif

> +/* Compute with 96 bit intermediate result: (a*b)/c */
> +static u64 muldiv64(u64 a, u32 b, u32 c)
> +{
> +	union {
> +		u64 ll;
> +		struct {
> +			u32 low, high;
> +		} l;
> +	} u, res;
> +	u64 rl, rh;
> +
> +	u.ll = a;
> +	rl = (u64)u.l.low * (u64)b;
> +	rh = (u64)u.l.high * (u64)b;
> +	rh += (rl >> 32);
> +	res.l.high = div64_64(rh, c);
> +	res.l.low = div64_64(((mod_64(rh, c) << 32) + (rl & 0xffffffff)), c);
> +	return res.ll;
> +}

eventually these should move into a generic file, for example 
lib/div64.c.

> +	ASSERT(mutex_is_locked(&kvm->arch.vpit->pit_state.lock));

could we please standardize on WARN_ON(!(x)) instead?

> +static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
> +{
> +	struct kvm_kpit_state *ps;
> +	int restart_timer = 0;
> +
> +	ps = container_of(data, struct kvm_kpit_state, pit_timer.timer);
> +
> +	restart_timer = __pit_timer_fn(ps);
> +
> +	if (restart_timer)
> +		return HRTIMER_RESTART;
> +	else
> +		return HRTIMER_NORESTART;
> +}

elegant use of hrtimers! :-)

> +	if (val == 0)
> +		val = 0x10000;

magic constant.

> +	val  &= 0xff;
> +	addr &= 3;

magic constants.

	Ingo

-------------------------------------------------------------------------
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/