Re: APIC_TMCCT register read bug

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

Kevin Pedretti wrote:
> Hi,
>
> While booting a non-Linux OS under kvm-46, I noticed that reading
> APIC_TMCCT before initializing APIC_TDCR to something other than its
> boot time value would lead to a host kernel divide by zero exception.
> It's due to apic->timer.divide_count being set to 0 at boot... it should
> be set to 2 since APIC_TDCR=0 means 'divide count by 2'.  The last hunk
> of the attached patch results in apic->timer.divide_count being set to 2
> and eliminates the oops.
>
> The other changes to apic_get_tmcct() are intended to clean it up a bit,
> although completely untested other than to verify 0 is returned for a
> read of APIC_TMCCT at boot.  'apic' should not be used before the
> ASSERT() and using u32 for counter_passed makes it fairly easy to
> overflow.
>   

Patch looks good, but I'm missing a signed-off-by: line.

Eddie, can you also take a look?

> --- kvm-46.orig/kernel/lapic.c	2007-10-10 02:06:36.000000000 -0600
> +++ kvm-46.fix/kernel/lapic.c	2007-10-12 22:50:01.000000000 -0600
> @@ -487,12 +487,19 @@
>  
>  static u32 apic_get_tmcct(struct kvm_lapic *apic)
>  {
> -	u32 counter_passed;
> -	ktime_t passed, now = apic->timer.dev.base->get_time();
> -	u32 tmcct = apic_get_reg(apic, APIC_TMICT);
> +	u64 counter_passed;
> +	ktime_t passed, now;
> +	u32 tmcct;
>  
>  	ASSERT(apic != NULL);
>  
> +	now = apic->timer.dev.base->get_time();
> +	tmcct = apic_get_reg(apic, APIC_TMICT);
> +
> +	/* if initial count is 0, current count should also be 0 */
> +	if (tmcct == 0)
> +		return 0;
> +
>  	if (unlikely(ktime_to_ns(now) <=
>  		ktime_to_ns(apic->timer.last_update))) {
>  		/* Wrap around */
> @@ -507,15 +514,24 @@
>  
>  	counter_passed = div64_64(ktime_to_ns(passed),
>  				  (APIC_BUS_CYCLE_NS * apic->timer.divide_count));
> -	tmcct -= counter_passed;
>  
> -	if (tmcct <= 0) {
> -		if (unlikely(!apic_lvtt_period(apic)))
> +	if (counter_passed > tmcct) {
> +		if (unlikely(!apic_lvtt_period(apic))) {
> +			/* one-shot timers stick at 0 until reset */
>  			tmcct = 0;
> -		else
> -			do {
> -				tmcct += apic_get_reg(apic, APIC_TMICT);
> -			} while (tmcct <= 0);
> +		} else {
> +			/*
> +			 * periodic timers reset to APIC_TMICT when they
> +			 * hit 0. The while loop simulates this happening N
> +			 * times. (counter_passed %= tmcct) would also work,
> +			 * but might be slower or not work on 32-bit??
> +			 */
> +			while (counter_passed > tmcct)
> +				counter_passed -= tmcct;
> +			tmcct -= counter_passed;
> +		}
> +	} else {
> +		tmcct -= counter_passed;
>  	}
>  
>  	return tmcct;
> @@ -844,7 +860,7 @@
>  		apic_set_reg(apic, APIC_ISR + 0x10 * i, 0);
>  		apic_set_reg(apic, APIC_TMR + 0x10 * i, 0);
>  	}
> -	apic->timer.divide_count = 0;
> +	update_divide_count(apic);
>  	atomic_set(&apic->timer.pending, 0);
>  	if (vcpu->vcpu_id == 0)
>  		vcpu->apic_base |= MSR_IA32_APICBASE_BSP;



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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/