[PATCH][Resend] Fix Warnining in arch/x86/kvm/vmx.c

[PATCH][Resend] Fix Warnining in arch/x86/kvm/vmx.c

[Subrata Modak]

Hi Avi/Yaniv,

With gcc --version 4.4.1 20090429 (prerelease)

I get the following warning:
arch/x86/kvm/vmx.c: In function ‘vmx_intr_assist’:
arch/x86/kvm/vmx.c:3233: warning: ‘max_irr’ may be used uninitialized in this function
arch/x86/kvm/vmx.c:3233: note: ‘max_irr’ was declared here

Investigation found that:

3231 static void update_tpr_threshold(struct kvm_vcpu *vcpu)
3232 {
3233         int max_irr, tpr;
3234 
3235         if (!vm_need_tpr_shadow(vcpu->kvm))
3236                 return;
3237 
3238         if (!kvm_lapic_enabled(vcpu) ||
3239             ((max_irr = kvm_lapic_find_highest_irr(vcpu)) == -1)) {

	(max_irr = kvm_lapic_find_highest_irr(vcpu)) == -1

may not get a chance to evaluate if:

	!kvm_lapic_enabled(vcpu)

evaluates to true (as the expressions are Or-ed).

3240                 vmcs_write32(TPR_THRESHOLD, 0);
3241                 return;
3242         }
3243 
3244         tpr = (kvm_lapic_get_cr8(vcpu) & 0x0f) << 4;
3245         vmcs_write32(TPR_THRESHOLD, (max_irr > tpr) ? tpr >> 4 : max_irr >> 4);

Using (max_irr > tpr) and max_irr >> 4, without max_irr getting initialized can
cause trouble.

3246 }

I would like to propose a small fix for this by interchanging the
operands in ||, so that max_irr is initialized in all instances,
and, the warning fades away, without compromising the criteria of
conditional evaluation inside if().

Signed-Off-By: Subrata Modak <subrata@linux.vnet.ibm.com>,
To: Avi Kivity <avi@qumranet.com>
To: Yaniv Kamay  <yaniv@qumranet.com>
To: <kvm@vger.kernel.org>
Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Cc: Sachin P Sant <sachinp@linux.vnet.ibm.com>
Subject: [PATCH][Resend] Fix Warnining in arch/x86/kvm/vmx.c
---

--- a/arch/x86/kvm/vmx.c	2009-05-12 15:28:42.000000000 +0530
+++ b/arch/x86/kvm/vmx.c	2009-05-12 15:51:02.000000000 +0530
@@ -3235,8 +3235,8 @@ static void update_tpr_threshold(struct 
 	if (!vm_need_tpr_shadow(vcpu->kvm))
 		return;
 
-	if (!kvm_lapic_enabled(vcpu) ||
-	    ((max_irr = kvm_lapic_find_highest_irr(vcpu)) == -1)) {
+	if (((max_irr = kvm_lapic_find_highest_irr(vcpu)) == -1) ||
+	     !kvm_lapic_enabled(vcpu)) {
 		vmcs_write32(TPR_THRESHOLD, 0);
 		return;
 	}

---
Regards--
Subrata

Re: [PATCH][Resend] Fix Warnining in arch/x86/kvm/vmx.c

[Avi Kivity]

Subrata Modak wrote:
> Hi Avi/Yaniv,
>
> With gcc --version 4.4.1 20090429 (prerelease)
>
> I get the following warning:
> arch/x86/kvm/vmx.c: In function ‘vmx_intr_assist’:
> arch/x86/kvm/vmx.c:3233: warning: ‘max_irr’ may be used uninitialized in this function
> arch/x86/kvm/vmx.c:3233: note: ‘max_irr’ was declared here
>
> Investigation found that:
>
> 3231 static void update_tpr_threshold(struct kvm_vcpu *vcpu)
> 3232 {
> 3233         int max_irr, tpr;
> 3234 
> 3235         if (!vm_need_tpr_shadow(vcpu->kvm))
> 3236                 return;
> 3237 
> 3238         if (!kvm_lapic_enabled(vcpu) ||
> 3239             ((max_irr = kvm_lapic_find_highest_irr(vcpu)) == -1)) {
>
>   

This function no longer exists; can you check if the current code is 
susceptible?

> 	(max_irr = kvm_lapic_find_highest_irr(vcpu)) == -1
>
> may not get a chance to evaluate if:
>
> 	!kvm_lapic_enabled(vcpu)
>
> evaluates to true (as the expressions are Or-ed).
>
> 3240                 vmcs_write32(TPR_THRESHOLD, 0);
> 3241                 return;
> 3242         }
> 3243 
> 3244         tpr = (kvm_lapic_get_cr8(vcpu) & 0x0f) << 4;
> 3245         vmcs_write32(TPR_THRESHOLD, (max_irr > tpr) ? tpr >> 4 : max_irr >> 4);
>
> Using (max_irr > tpr) and max_irr >> 4, without max_irr getting initialized can
> cause trouble.
>   

With !kvm_lapic_enabled(), TPR_THRESHOLD is meaningless.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.