[PATCH 0/2] KVM: x86: Fix and optimize in-kernel NMI watchdog support - v2

[PATCH 0/2] KVM: x86: Fix and optimize in-kernel NMI watchdog support - v2

[Jan Kiszka]

This is a reworked version of the NMI watchdog fixes, taking the
discussion with Sheng into account. There are two further (minor) issues
I found in the APIC emulation at this chance, but those can be handled
separately.

Jan

--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

[-- Attachment #1: kvm-x86-fix-nmi-wd-emulation.patch --]
[-- Type: text/plain, Size: 3735 bytes --]

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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);
 	}
 }

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

[Sheng Yang]

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

[PATCH 2/2] KVM: x86: Optimize NMI watchdog delivery

[Jan Kiszka]

[-- Attachment #1: kvm-x86-optimize-NMI-watchdog-delivery.patch --]
[-- Type: text/plain, Size: 3037 bytes --]

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/i8254.c       |   11 ++++++-----
 arch/x86/kvm/lapic.c       |   23 ++++++++++++++++++++---
 include/asm-x86/kvm_host.h |    1 +
 3 files changed, 27 insertions(+), 8 deletions(-)

Index: b/arch/x86/kvm/i8254.c
===================================================================
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -618,11 +618,12 @@ static void __inject_pit_timer_intr(stru
 	 * 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)
-			kvm_apic_nmi_wd_deliver(vcpu);
-	}
+	if (kvm->arch.vapics_in_nmi_mode > 0)
+		for (i = 0; i < KVM_MAX_VCPUS; ++i) {
+			vcpu = kvm->vcpus[i];
+			if (vcpu)
+				kvm_apic_nmi_wd_deliver(vcpu);
+		}
 }
 
 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;

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

[Sheng Yang]

On Mon, Oct 20, 2008 at 10:20:03AM +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.
> 
> 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       |   11 ++++++-----
>  arch/x86/kvm/lapic.c       |   23 ++++++++++++++++++++---
>  include/asm-x86/kvm_host.h |    1 +
>  3 files changed, 27 insertions(+), 8 deletions(-)
> 
> Index: b/arch/x86/kvm/i8254.c
> ===================================================================
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -618,11 +618,12 @@ static void __inject_pit_timer_intr(stru
>  	 * 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)
> -			kvm_apic_nmi_wd_deliver(vcpu);
> -	}
> +	if (kvm->arch.vapics_in_nmi_mode > 0)
> +		for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> +			vcpu = kvm->vcpus[i];
> +			if (vcpu)
> +				kvm_apic_nmi_wd_deliver(vcpu);
> +		}
>  }
>  
>  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

Re: [PATCH 0/2] KVM: x86: Fix and optimize in-kernel NMI watchdog support - v2

[Avi Kivity]

Jan Kiszka wrote:
> This is a reworked version of the NMI watchdog fixes, taking the
> discussion with Sheng into account. There are two further (minor) issues
> I found in the APIC emulation at this chance, but those can be handled
> separately.
>
>   

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function