Re: [PATCH 1/2] KVM: x86: Fix and refactor NMI watchdog emulation

[Sheng Yang <yasker@gmail.com>]

On Mon, Oct 20, 2008 at 10:20:02AM +0200, Jan Kiszka wrote:
> This patch refactors the NMI watchdog delivery patch, consolidating
> tests and providing a proper API for delivering watchdog events.
> 
> An included micro-optimization is to check only for apic_hw_enabled in
> kvm_apic_local_deliver (the test for LVT mask is covering the
> soft-disabled case already).

Oh... Please ignore my comments on the other patch for software enable bit.
:) (But I think this kind of optimization is reducing the readability a
little as well... It's easy to get curious about why only hardware enable is
OK)

Others seems OK to me.

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Acked-by: Sheng Yang <sheng@linux.intel.com>

--
regards
Yang, Sheng
> ---
>  arch/x86/kvm/i8254.c |   15 +++++++++------
>  arch/x86/kvm/irq.h   |    2 +-
>  arch/x86/kvm/lapic.c |   20 ++++++++++----------
>  3 files changed, 20 insertions(+), 17 deletions(-)
> 
> Index: b/arch/x86/kvm/i8254.c
> ===================================================================
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -610,15 +610,18 @@ static void __inject_pit_timer_intr(stru
>  	mutex_unlock(&kvm->lock);
>  
>  	/*
> -	 * Provides NMI watchdog support in IOAPIC mode.
> -	 * The route is: PIT -> PIC -> LVT0 in NMI mode,
> -	 * timer IRQs will continue to flow through the IOAPIC.
> +	 * Provides NMI watchdog support via Virtual Wire mode.
> +	 * The route is: PIT -> PIC -> LVT0 in NMI mode.
> +	 *
> +	 * Note: Our Virtual Wire implementation is simplified, only
> +	 * propagating PIT interrupts to all VCPUs when they have set
> +	 * LVT0 to NMI delivery. Other PIC interrupts are just sent to
> +	 * VCPU0, and only if its LVT0 is in EXTINT mode.
>  	 */
>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>  		vcpu = kvm->vcpus[i];
> -		if (!vcpu)
> -			continue;
> -		kvm_apic_local_deliver(vcpu, APIC_LVT0);
> +		if (vcpu)
> +			kvm_apic_nmi_wd_deliver(vcpu);
>  	}
>  }
>  
> Index: b/arch/x86/kvm/irq.h
> ===================================================================
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -87,7 +87,7 @@ void kvm_pic_reset(struct kvm_kpic_state
>  void kvm_timer_intr_post(struct kvm_vcpu *vcpu, int vec);
>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
>  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
> -int kvm_apic_local_deliver(struct kvm_vcpu *vcpu, int lvt_type);
> +void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu);
>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu);
>  void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu);
>  void __kvm_migrate_timers(struct kvm_vcpu *vcpu);
> Index: b/arch/x86/kvm/lapic.c
> ===================================================================
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -975,14 +975,12 @@ int apic_has_pending_timer(struct kvm_vc
>  	return 0;
>  }
>  
> -int kvm_apic_local_deliver(struct kvm_vcpu *vcpu, int lvt_type)
> +static int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
>  {
> -	struct kvm_lapic *apic = vcpu->arch.apic;
> +	u32 reg = apic_get_reg(apic, lvt_type);
>  	int vector, mode, trig_mode;
> -	u32 reg;
>  
> -	if (apic && apic_enabled(apic)) {
> -		reg = apic_get_reg(apic, lvt_type);
> +	if (apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) {
>  		vector = reg & APIC_VECTOR_MASK;
>  		mode = reg & APIC_MODE_MASK;
>  		trig_mode = reg & APIC_LVT_LEVEL_TRIGGER;
> @@ -991,9 +989,12 @@ int kvm_apic_local_deliver(struct kvm_vc
>  	return 0;
>  }
>  
> -static inline int __inject_apic_timer_irq(struct kvm_lapic *apic)
> +void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_apic_local_deliver(apic->vcpu, APIC_LVTT);
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	if (apic)
> +		kvm_apic_local_deliver(apic, APIC_LVT0);
>  }
>  
>  static enum hrtimer_restart apic_timer_fn(struct hrtimer *data)
> @@ -1088,9 +1089,8 @@ void kvm_inject_apic_timer_irqs(struct k
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  
> -	if (apic && apic_lvt_enabled(apic, APIC_LVTT) &&
> -		atomic_read(&apic->timer.pending) > 0) {
> -		if (__inject_apic_timer_irq(apic))
> +	if (apic && atomic_read(&apic->timer.pending) > 0) {
> +		if (kvm_apic_local_deliver(apic, APIC_LVTT))
>  			atomic_dec(&apic->timer.pending);
>  	}
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html