Re: [PATCH 3/3] KVM: x86: Optimize NMI watchdog delivery

[Sheng Yang <yasker@gmail.com>]

On Fri, Oct 17, 2008 at 07:23:01PM +0200, Jan Kiszka wrote:
> Sheng Yang wrote:
> > On Wed, Oct 15, 2008 at 04:27:51PM +0200, Jan Kiszka wrote:
> >> As suggested by Avi, this patch introduces a counter of VCPUs that have
> >> LVT0 set to NMI mode. Only if the counter > 0, we push the PIT ticks via
> >> all LAPIC LVT0 lines to enable NMI watchdog support.
> >>
> > 
> > I feel a little strange about: if *counter > 0*, we push to *all*. Can we
> > only push NMIs to the ones that set NMI for LVT0?
> 
> We don't do that due to !kvm_apic_accept_pic_intr(). The counter is only
> about optimizing that case where we don't have to walk the whole chain,
> asking every vcpu if it would like to receive the IRQ.

I don't agree to use kvm_apic_accept_pic_intr() here, as I explained in the
first mail. It's not a normal path, and current KVM handle it well.
> 
> > 
> > How about add a field in struct kvm_lapic? We can quickly know if we need to
> > inject NMI for this vcpu. Well, though kernel mostly enable NMI watchdog on
> > all vcpu, I think this is more precise, and match the logic, and avoid one
> > more field in kvm_arch...
> 
> The point of this patch is to avoid touching vcpu structures AT ALL when
> there is no interest in the NMI watchdog (normally, OSes will either
> enable the WD trick for all CPUSs or keep it off).

Logically, I think lapic is more proper place. And put a bool there won't
affect much. I think we can do it more straightly here.

--
regards
Yang, Sheng

> 
> Jan
> 
> > 
> > --
> > regards
> > Yang, Sheng
> > 
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  arch/x86/kvm/i8254.c       |   13 +++++++------
> >>  arch/x86/kvm/lapic.c       |   23 ++++++++++++++++++++---
> >>  include/asm-x86/kvm_host.h |    1 +
> >>  3 files changed, 28 insertions(+), 9 deletions(-)
> >>
> >> Index: b/arch/x86/kvm/i8254.c
> >> ===================================================================
> >> --- a/arch/x86/kvm/i8254.c
> >> +++ b/arch/x86/kvm/i8254.c
> >> @@ -607,12 +607,13 @@ static void __inject_pit_timer_intr(stru
> >>  	 * The route is: PIT -> PIC -> LVT0 in NMI mode,
> >>  	 * timer IRQs will continue to flow through the IOAPIC.
> >>  	 */
> >> -	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> >> -		vcpu = kvm->vcpus[i];
> >> -		if (!vcpu || !kvm_apic_accept_pic_intr(vcpu))
> >> -			continue;
> >> -		kvm_apic_local_deliver(vcpu, APIC_LVT0);
> >> -	}
> >> +	if (kvm->arch.vapics_in_nmi_mode > 0)
> >> +		for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> >> +			vcpu = kvm->vcpus[i];
> >> +			if (!vcpu || !kvm_apic_accept_pic_intr(vcpu))
> >> +				continue;
> >> +			kvm_apic_local_deliver(vcpu, APIC_LVT0);
> >> +		}
> >>  }
> >>  
> >>  void kvm_inject_pit_timer_irqs(struct kvm_vcpu *vcpu)
> >> Index: b/arch/x86/kvm/lapic.c
> >> ===================================================================
> >> --- a/arch/x86/kvm/lapic.c
> >> +++ b/arch/x86/kvm/lapic.c
> >> @@ -130,6 +130,11 @@ static inline int apic_lvtt_period(struc
> >>  	return apic_get_reg(apic, APIC_LVTT) & APIC_LVT_TIMER_PERIODIC;
> >>  }
> >>  
> >> +static inline int apic_lvt_nmi_mode(u32 lvt_val)
> >> +{
> >> +	return (lvt_val & (APIC_MODE_MASK | APIC_LVT_MASKED)) == APIC_DM_NMI;
> >> +}
> >> +
> >>  static unsigned int apic_lvt_mask[APIC_LVT_NUM] = {
> >>  	LVT_MASK | APIC_LVT_TIMER_PERIODIC,	/* LVTT */
> >>  	LVT_MASK | APIC_MODE_MASK,	/* LVTTHMR */
> >> @@ -672,6 +677,20 @@ static void start_apic_timer(struct kvm_
> >>  					apic->timer.period)));
> >>  }
> >>  
> >> +static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
> >> +{
> >> +	int nmi_wd_enabled = apic_lvt_nmi_mode(apic_get_reg(apic, APIC_LVT0));
> >> +
> >> +	if (apic_lvt_nmi_mode(lvt0_val)) {
> >> +		if (!nmi_wd_enabled) {
> >> +			apic_debug("Receive NMI setting on APIC_LVT0 "
> >> +				   "for cpu %d\n", apic->vcpu->vcpu_id);
> >> +			apic->vcpu->kvm->arch.vapics_in_nmi_mode++;
> >> +		}
> >> +	} else if (nmi_wd_enabled)
> >> +		apic->vcpu->kvm->arch.vapics_in_nmi_mode--;
> >> +}
> >> +
> >>  static void apic_mmio_write(struct kvm_io_device *this,
> >>  			    gpa_t address, int len, const void *data)
> >>  {
> >> @@ -753,9 +772,7 @@ static void apic_mmio_write(struct kvm_i
> >>  		break;
> >>  
> >>  	case APIC_LVT0:
> >> -		if (val == APIC_DM_NMI)
> >> -			apic_debug("Receive NMI setting on APIC_LVT0 "
> >> -				"for cpu %d\n", apic->vcpu->vcpu_id);
> >> +		apic_manage_nmi_watchdog(apic, val);
> >>  	case APIC_LVTT:
> >>  	case APIC_LVTTHMR:
> >>  	case APIC_LVTPC:
> >> Index: b/include/asm-x86/kvm_host.h
> >> ===================================================================
> >> --- a/include/asm-x86/kvm_host.h
> >> +++ b/include/asm-x86/kvm_host.h
> >> @@ -359,6 +359,7 @@ struct kvm_arch{
> >>  	struct kvm_ioapic *vioapic;
> >>  	struct kvm_pit *vpit;
> >>  	struct hlist_head irq_ack_notifier_list;
> >> +	int vapics_in_nmi_mode;
> >>  
> >>  	int round_robin_prev_vcpu;
> >>  	unsigned int tss_addr;
> >>
> >> --
> >> 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
> > --
> > 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
> > 
> 
>