From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yang, Sheng" Subject: Re: [PATCH 1/6] KVM: In kernel pit model Date: Wed, 5 Mar 2008 19:35:40 +0800 Message-ID: <200803051935.40229.sheng.yang@intel.com> References: <200803041822.44757.sheng.yang@intel.com> <200803051504.40409.sheng.yang@intel.com> <20080305091529.GA25357@elte.hu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel@lists.sourceforge.net, Avi Kivity To: Ingo Molnar Return-path: In-Reply-To: <20080305091529.GA25357@elte.hu> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces@lists.sourceforge.net Errors-To: kvm-devel-bounces@lists.sourceforge.net List-Id: kvm.vger.kernel.org Thanks for comments! On Wednesday 05 March 2008 17:15:29 Ingo Molnar wrote: > * Yang, Sheng 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/