From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: APIC_TMCCT register read bug Date: Mon, 15 Oct 2007 11:29:48 +0200 Message-ID: <4713330C.7050909@qumranet.com> References: <1192252790.16540.4.camel@saix14899> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Kevin Pedretti Return-path: In-Reply-To: <1192252790.16540.4.camel@saix14899> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.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/