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

["Yang, Sheng" <sheng.yang@intel.com>]

Thanks for comments!

On Wednesday 05 March 2008 17:15:29 Ingo Molnar wrote:
> * 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.

Um... I followed example on ./virt/kvm/ioapic.c here. Though I think it's good 
to substitute all self defined debug printk with pr_debug, why KVM have 
little pr_xxx(the only ones are in x86.c)? Maybe for KVM is acting more like 
a separate driver, and using printk is easier for separate debug? I really 
don't know...

> > +#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.

That's my hope (of course with big endian support). But is there any user 
outside KVM? I think it may not easy to get into the generic part. 

> > +	ASSERT(mutex_is_locked(&kvm->arch.vpit->pit_state.lock));
>
> could we please standardize on WARN_ON(!(x)) instead?

Sure. :)

> > +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.

I will update these constants. :) In fact, I have thought of these before, but 
not insist... 

Thanks!

Yang, Sheng
>
> 	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/