Re: [RFC][PATCH 1/4] KVM: LAPIC: Unified the duplicate calling of setting IRR

[Avi Kivity <avi@qumranet.com>]

Yang, Sheng wrote:
> From 650cad44069541fcd9fea8be6a78837e812b3dfd Mon Sep 17 00:00:00 2001
> From: Sheng Yang <sheng.yang@intel.com>
> Date: Thu, 8 May 2008 09:58:50 +0800
> Subject: [PATCH 1/4] KVM: LAPIC: Unified the duplicate calling of setting IRR
>
> It's strange got two callings of setting IRR seperately for IOAPIC and IPI in
> lapic. The patch unified them into __apic_set_irq().
>
> Signed-off-by: Sheng Yang <sheng.yang@intel.com>
> ---
>  arch/x86/kvm/lapic.c |   69 +++++++++++++++++++++++--------------------------
>  1 files changed, 32 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 7652f88..6226fe0 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -184,20 +184,40 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_lapic_find_highest_irr);
>
> -int kvm_apic_set_irq(struct kvm_vcpu *vcpu, u8 vec, u8 trig)
> +static int __apic_set_irq(struct kvm_vcpu *vcpu, u8 vector, u8 trig_mode)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>
> -	if (!apic_test_and_set_irr(vec, apic)) {
> -		/* a new pending irq is set in IRR */
> -		if (trig)
> -			apic_set_vector(vec, apic->regs + APIC_TMR);
> -		else
> -			apic_clear_vector(vec, apic->regs + APIC_TMR);
> -		kvm_vcpu_kick(apic->vcpu);
> -		return 1;
> +	/* FIXME add logic for vcpu on reset */
> +	if (unlikely(!apic_enabled(apic)))
> +		return 0;
> +
> +	if (apic_test_and_set_irr(vector, apic)) {
> +		if (trig_mode)
> +			apic_debug("level trig mode repeatedly for vector %d\n",
> +					vector);
> +		return 0;
>  	}
> -	return 0;
> +
> +	if (trig_mode) {
> +		apic_debug("level trig mode for vector %d\n", vector);
> +		apic_set_vector(vector, apic->regs + APIC_TMR);
> +	} else
> +		apic_clear_vector(vector, apic->regs + APIC_TMR);
> +
> +	if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE)
> +		kvm_vcpu_kick(vcpu);
>   
> +	else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED) {
> +		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> +		if (waitqueue_active(&vcpu->wq))
> +			wake_up_interruptible(&vcpu->wq);
> +	}
>   

This can race against the vcpu executing hlt if the compiler reorders 
the tests (and maybe against waking up from hlt if it does not).  It's 
best to be conservative like the original code and not optimize away 
kicks, unless many of them are unnecessary.

> +	return 1;
> +}
> +
> +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, u8 vec, u8 trig)
> +{
> +	return __apic_set_irq(vcpu, vec, trig);
>  }
>   

Why create a new function instead of modifying the original?


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


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone