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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 4881 bytes --]

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.

> 
> 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).

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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]