Re: APIC_TMCCT register read bug]

Re: APIC_TMCCT register read bug]

[Kevin Pedretti]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

On Mon, 2007-10-15 at 11:29 +0200, Avi Kivity wrote:
> Patch looks good, but I'm missing a signed-off-by: line.
>
> Eddie, can you also take a look?

I split the patch in two.  The first one fixes the host OS kernel
divide by zero problem.  The second cleans up apic_get_tmcct() a bit.
Both now include a signed-off-by.

Kevin

[-- Attachment #2: patch1 --]
[-- Type: application/octet-stream, Size: 993 bytes --]

kvm_lapic_reset() was initializing apic->timer.divide_count to 0,
which could potentially lead to a divide by zero error in 
apic_get_tmcct().  Any guest that reads the APIC's CCR (current count)
register before setting DCR (divide configuration) would trigger a divide
by zero exception in the host kernel, leading to a host-OS crash.

This patch results in apic->timer.divide_count being initialized to
2 at reset, eliminating the bug (DCR=0 at reset, meaning divide by 2).

Signed-off-by: Kevin Pedretti <kevin.pedretti@gmail.com>

--- kvm-46.a/kernel/lapic.c	2007-10-10 02:06:36.000000000 -0600
+++ kvm-46.b/kernel/lapic.c	2007-10-17 10:01:59.000000000 -0600
@@ -844,7 +844,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vc
 		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;

[-- Attachment #3: patch2 --]
[-- Type: application/octet-stream, Size: 1908 bytes --]

Better handle wrap-around cases when reading the APIC CCR
(current count register).  Also, if ICR is 0, CCR should also
be 0... previously reading CCR before setting ICR would result
in a large kinda-random number.

Signed-off-by: Kevin Pedretti <kevin.pedretti@gmail.com>

--- kvm-46.a/kernel/lapic.c	2007-10-10 02:06:36.000000000 -0600
+++ kvm-46.b/kernel/lapic.c	2007-10-17 10:34:09.000000000 -0600
@@ -487,12 +487,19 @@ static void apic_send_ipi(struct kvm_lap
 
 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 @@ static u32 apic_get_tmcct(struct kvm_lap
 
 	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;

[-- Attachment #4: Type: text/plain, Size: 314 bytes --]

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

[-- Attachment #5: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: APIC_TMCCT register read bug]

[Dong, Eddie]

kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org wrote:
> On Mon, 2007-10-15 at 11:29 +0200, Avi Kivity wrote:
>> Patch looks good, but I'm missing a signed-off-by: line.
>> 
>> Eddie, can you also take a look?
> 
> I split the patch in two.  The first one fixes the host OS kernel
> divide by zero problem.  The second cleans up apic_get_tmcct() a bit.
> Both now include a signed-off-by.
> 
> Kevin

Good! A minor comments:

+			while (counter_passed > tmcct)
+				counter_passed -= tmcct;

may be: counter_passed %= tmcct, or use __div64_32.

+			tmcct -= counter_passed;


BTW, would u like to give clean div64_64 to div64_32 too?
I think 64/32 is enough and faster.
thx, eddie

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

Re: APIC_TMCCT register read bug]

[Avi Kivity]

Kevin Pedretti wrote:
> On Mon, 2007-10-15 at 11:29 +0200, Avi Kivity wrote:
>   
>> Patch looks good, but I'm missing a signed-off-by: line.
>>
>> Eddie, can you also take a look?
>>     
>
> I split the patch in two.  The first one fixes the host OS kernel
> divide by zero problem.  The second cleans up apic_get_tmcct() a bit.
> Both now include a signed-off-by.
>   

Applied both, thanks.


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