From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 1/2] KVM: x86: Fix and refactor NMI watchdog emulation Date: Mon, 20 Oct 2008 20:10:45 +0800 Message-ID: <20081020121045.GD30536@yukikaze> References: <20081020082001.537654768@mchn012c.ww002.siemens.net> <20081020082001.754042528@mchn012c.ww002.siemens.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, avi@redhat.com, jiajun.xu@intel.com, sheng@linux.intel.com To: Jan Kiszka Return-path: Received: from ti-out-0910.google.com ([209.85.142.190]:38399 "EHLO ti-out-0910.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751830AbYJTMKy (ORCPT ); Mon, 20 Oct 2008 08:10:54 -0400 Received: by ti-out-0910.google.com with SMTP id b6so824818tic.23 for ; Mon, 20 Oct 2008 05:10:52 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20081020082001.754042528@mchn012c.ww002.siemens.net> Sender: kvm-owner@vger.kernel.org List-ID: 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 Acked-by: Sheng Yang -- 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