Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325614 > unrolled thread

[PATCH 0/4] KVM: x86: change and fix PIT discard tick policy

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] KVM: x86: change and fix PIT discard tick policy Radim Krčmář <rkrcmar@redhat.com> - 2016-02-03 17:30 +0100
    [PATCH 2/4] KVM: x86: refactor PIT state inject_lock Radim Krčmář <rkrcmar@redhat.com> - 2016-02-03 17:30 +0100
      Re: [PATCH 2/4] KVM: x86: refactor PIT state inject_lock Paolo Bonzini <pbonzini@redhat.com> - 2016-02-03 17:50 +0100
        Re: [PATCH 2/4] KVM: x86: refactor PIT state inject_lock Radim Krčmář <rkrcmar@redhat.com> - 2016-02-04 14:20 +0100
    [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy Radim Krčmář <rkrcmar@redhat.com> - 2016-02-03 17:30 +0100
      Re: [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy Paolo Bonzini <pbonzini@redhat.com> - 2016-02-03 18:00 +0100
        Re: [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy Radim Krčmář <rkrcmar@redhat.com> - 2016-02-04 14:20 +0100
    [PATCH 1/4] KVM: x86: fix interrupt dropping race in PIT Radim Krčmář <rkrcmar@redhat.com> - 2016-02-03 17:30 +0100
      Re: [PATCH 1/4] KVM: x86: fix interrupt dropping race in PIT Radim Krčmář <rkrcmar@redhat.com> - 2016-02-04 14:40 +0100

#1325614 — [PATCH 0/4] KVM: x86: change and fix PIT discard tick policy

[3/4] fixes a bug where legacy NMI watchdog doesn't receive NMIs if a
CPU that handles PIT interrupts hangs with masked interrupts.

Remaining patches try to make the code nicer where necessary.
([1/4] makes you think that it fixes a bug, but that would be fixed by
 skipping the path in [3/4].)

[3/4] only changes the discard policy.  A change that allows legacy NMI
watchdog with the reinject policy is below, but conscience doesn't allow
me to sign it off.

# diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
@@ -274,6 +274,10 @@ static void pit_do_work(struct kthread_work *work)
 	int i;
 	struct kvm_kpit_state *ps = &pit->pit_state;
 
+	if (atomic_read(&kvm->arch.vapics_in_nmi_mode) > 0)
+		kvm_for_each_vcpu(i, vcpu, kvm)
+			kvm_apic_nmi_wd_deliver(vcpu);
+
 	if (ps->reinject && !atomic_xchg(&ps->irq_ack, 0))
 		return;
 
@@ -289,9 +293,6 @@ static void pit_do_work(struct kthread_work *work)
	 * VCPUs and only when LVT0 is in NMI mode.  The interrupt can
	 * also be simultaneously delivered through PIC and IOAPIC.
 	 */
-	if (atomic_read(&kvm->arch.vapics_in_nmi_mode) > 0)
-		kvm_for_each_vcpu(i, vcpu, kvm)
-			kvm_apic_nmi_wd_deliver(vcpu);
 }
 
 static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)


Radim Krčmář (4):
  KVM: x86: fix interrupt dropping race in PIT
  KVM: x86: refactor PIT state inject_lock
  KVM: x86: change PIT discard tick policy
  KVM: x86: remove notifiers from PIT discard policy

 arch/x86/kvm/i8254.c | 122 ++++++++++++++++++++++++++-------------------------
 arch/x86/kvm/i8254.h |   4 +-
 arch/x86/kvm/x86.c   |   6 +--
 3 files changed, 67 insertions(+), 65 deletions(-)

-- 
2.7.0

[toc] | [next] | [standalone]

#1325616 — [PATCH 2/4] KVM: x86: refactor PIT state inject_lock

Following patches would be even uglier if inject_lock didn't go away.

Patch changes the virtual wire comment to better describe our situation.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/i8254.c | 67 ++++++++++++++++++++++------------------------------
 arch/x86/kvm/i8254.h |  3 +--
 2 files changed, 29 insertions(+), 41 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index de5f5018026f..a137eb381012 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -236,16 +236,13 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
 {
 	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
 						 irq_ack_notifier);
-	int value;
 
-	spin_lock(&ps->inject_lock);
+	atomic_set(&ps->irq_ack, 1);
 	if (atomic_add_unless(&ps->pending, -1, 0))
 		/* in this case, we had multiple outstanding pit interrupts
 		 * that we needed to inject.  Reinject
 		 */
 		queue_kthread_work(&ps->pit->worker, &ps->pit->expired);
-	ps->irq_ack = 1;
-	spin_unlock(&ps->inject_lock);
 }
 
 void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
@@ -276,34 +273,25 @@ static void pit_do_work(struct kthread_work *work)
 	struct kvm_vcpu *vcpu;
 	int i;
 	struct kvm_kpit_state *ps = &pit->pit_state;
-	int inject = 0;
 
-	/* Try to inject pending interrupts when
-	 * last one has been acked.
+	if (!atomic_xchg(&ps->irq_ack, 0))
+		return;
+
+	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1, false);
+	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0, false);
+
+	/*
+	 * Provides NMI watchdog support via Virtual Wire mode.
+	 * The route is: PIT -> LVT0 in NMI mode.
+	 *
+	 * Note: Our Virtual Wire implementation does not follow
+	 * the MP specification.  We propagate a PIT interrupt to all
+	 * VCPUs and only when LVT0 is in NMI mode.  The interrupt can
+	 * also be simultaneously delivered through PIC and IOAPIC.
 	 */
-	spin_lock(&ps->inject_lock);
-	if (ps->irq_ack) {
-		ps->irq_ack = 0;
-		inject = 1;
-	}
-	spin_unlock(&ps->inject_lock);
-	if (inject) {
-		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1, false);
-		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0, false);
-
-		/*
-		 * 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.
-		 */
-		if (atomic_read(&kvm->arch.vapics_in_nmi_mode) > 0)
-			kvm_for_each_vcpu(i, vcpu, kvm)
-				kvm_apic_nmi_wd_deliver(vcpu);
-	}
+	if (atomic_read(&kvm->arch.vapics_in_nmi_mode) > 0)
+		kvm_for_each_vcpu(i, vcpu, kvm)
+			kvm_apic_nmi_wd_deliver(vcpu);
 }
 
 static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
@@ -323,6 +311,12 @@ static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
 		return HRTIMER_NORESTART;
 }
 
+static void kvm_pit_reset_reinject(struct kvm_pit *pit)
+{
+	atomic_set(&pit->pit_state.pending, 0);
+	atomic_set(&pit->pit_state.irq_ack, 1);
+}
+
 static void create_pit_timer(struct kvm *kvm, u32 val, int is_period)
 {
 	struct kvm_kpit_state *ps = &kvm->arch.vpit->pit_state;
@@ -345,8 +339,7 @@ static void create_pit_timer(struct kvm *kvm, u32 val, int is_period)
 	ps->timer.function = pit_timer_fn;
 	ps->kvm = ps->pit->kvm;
 
-	atomic_set(&ps->pending, 0);
-	ps->irq_ack = 1;
+	kvm_pit_reset_reinject(ps->pit);
 
 	/*
 	 * Do not allow the guest to program periodic timers with small
@@ -646,18 +639,15 @@ void kvm_pit_reset(struct kvm_pit *pit)
 	}
 	mutex_unlock(&pit->pit_state.lock);
 
-	atomic_set(&pit->pit_state.pending, 0);
-	pit->pit_state.irq_ack = 1;
+	kvm_pit_reset_reinject(pit);
 }
 
 static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, bool mask)
 {
 	struct kvm_pit *pit = container_of(kimn, struct kvm_pit, mask_notifier);
 
-	if (!mask) {
-		atomic_set(&pit->pit_state.pending, 0);
-		pit->pit_state.irq_ack = 1;
-	}
+	if (!mask)
+		kvm_pit_reset_reinject(pit);
 }
 
 static const struct kvm_io_device_ops pit_dev_ops = {
@@ -691,7 +681,6 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 
 	mutex_init(&pit->pit_state.lock);
 	mutex_lock(&pit->pit_state.lock);
-	spin_lock_init(&pit->pit_state.inject_lock);
 
 	pid = get_pid(task_tgid(current));
 	pid_nr = pid_vnr(pid);
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index c84990b42b5b..f8cf4b84f435 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -33,8 +33,7 @@ struct kvm_kpit_state {
 	u32    speaker_data_on;
 	struct mutex lock;
 	struct kvm_pit *pit;
-	spinlock_t inject_lock;
-	unsigned long irq_ack;
+	atomic_t irq_ack;
 	struct kvm_irq_ack_notifier irq_ack_notifier;
 };
 
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1325646 — Re: [PATCH 2/4] KVM: x86: refactor PIT state inject_lock

On 03/02/2016 17:23, Radim Krčmář wrote:
> Following patches would be even uglier if inject_lock didn't go away.
> 
> Patch changes the virtual wire comment to better describe our situation.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/i8254.c | 67 ++++++++++++++++++++++------------------------------
>  arch/x86/kvm/i8254.h |  3 +--
>  2 files changed, 29 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index de5f5018026f..a137eb381012 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -236,16 +236,13 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
>  {
>  	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
>  						 irq_ack_notifier);
> -	int value;
>  
> -	spin_lock(&ps->inject_lock);
> +	atomic_set(&ps->irq_ack, 1);

smp_mb__before_atomic();

>  	if (atomic_add_unless(&ps->pending, -1, 0))
>  		/* in this case, we had multiple outstanding pit interrupts
>  		 * that we needed to inject.  Reinject
>  		 */
>  		queue_kthread_work(&ps->pit->worker, &ps->pit->expired);
> -	ps->irq_ack = 1;
> -	spin_unlock(&ps->inject_lock);
>  }
>  
>  void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
> @@ -276,34 +273,25 @@ static void pit_do_work(struct kthread_work *work)
>  	struct kvm_vcpu *vcpu;
>  	int i;
>  	struct kvm_kpit_state *ps = &pit->pit_state;
> -	int inject = 0;
>  
> -	/* Try to inject pending interrupts when
> -	 * last one has been acked.
> +	if (!atomic_xchg(&ps->irq_ack, 0))
> +		return;
> +
> +	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1, false);
> +	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0, false);
> +
> +	/*
> +	 * Provides NMI watchdog support via Virtual Wire mode.
> +	 * The route is: PIT -> LVT0 in NMI mode.
> +	 *
> +	 * Note: Our Virtual Wire implementation does not follow
> +	 * the MP specification.  We propagate a PIT interrupt to all
> +	 * VCPUs and only when LVT0 is in NMI mode.  The interrupt can
> +	 * also be simultaneously delivered through PIC and IOAPIC.
>  	 */
> -	spin_lock(&ps->inject_lock);
> -	if (ps->irq_ack) {
> -		ps->irq_ack = 0;
> -		inject = 1;
> -	}
> -	spin_unlock(&ps->inject_lock);
> -	if (inject) {
> -		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1, false);
> -		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0, false);
> -
> -		/*
> -		 * 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.
> -		 */
> -		if (atomic_read(&kvm->arch.vapics_in_nmi_mode) > 0)
> -			kvm_for_each_vcpu(i, vcpu, kvm)
> -				kvm_apic_nmi_wd_deliver(vcpu);
> -	}
> +	if (atomic_read(&kvm->arch.vapics_in_nmi_mode) > 0)
> +		kvm_for_each_vcpu(i, vcpu, kvm)
> +			kvm_apic_nmi_wd_deliver(vcpu);
>  }
>  
>  static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
> @@ -323,6 +311,12 @@ static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
>  		return HRTIMER_NORESTART;
>  }
>  
> +static void kvm_pit_reset_reinject(struct kvm_pit *pit)
> +{
> +	atomic_set(&pit->pit_state.pending, 0);

smp_wmb()?

Looks safe otherwise.  (Please also add a comment before the memory
barriers to show the pairing).

Paolo

[toc] | [prev] | [next] | [standalone]

#1326787 — Re: [PATCH 2/4] KVM: x86: refactor PIT state inject_lock

2016-02-03 17:45+0100, Paolo Bonzini:
> On 03/02/2016 17:23, Radim Krčmář wrote:
>> Following patches would be even uglier if inject_lock didn't go away.
>> 
>> Patch changes the virtual wire comment to better describe our situation.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>> @@ -236,16 +236,13 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
>>  {
>>  	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
>>  						 irq_ack_notifier);
>> -	int value;
>>  
>> -	spin_lock(&ps->inject_lock);

(Our code doesn't need barrier between dereferencing the pointer and
 reading its contents, and this bug is not possible happen on x86.)

>> +	atomic_set(&ps->irq_ack, 1);
> 
> smp_mb__before_atomic();

irq_ack has to be set before queueing pit_do_work, or could lose the
interrupt.
atomic_add_unless implies full barriers, so I think we're ok here.

>>  	if (atomic_add_unless(&ps->pending, -1, 0))
>>  		queue_kthread_work(&ps->pit->worker, &ps->pit->expired);
>>  void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
>> @@ -323,6 +311,12 @@ static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
>>  		return HRTIMER_NORESTART;
>>  }
>>  
>> +static void kvm_pit_reset_reinject(struct kvm_pit *pit)
>> +{
>> +	atomic_set(&pit->pit_state.pending, 0);
> 
> smp_wmb()?

I don't think that we need to ensure the order of pending and irq_ack
because there isn't a dependency between these two.  The reset is a slap
out of nowhere ... I think we'd need locks to make it behave correctly.

The worst that can happen is one virtual wire NMI injection when the
IOAPIC interrupt was in IRR and number of injections therefore won't
match. The race goes like this:

kvm_pit_ack_irq is running and we have pending > 0, so pit_do_work is
scheduled and executed.   pit_do_work sets irq_ack from 1 to 0.
Then pit_timer_fn gets executed, increases pending and queues
pit_do_work.  Before pit_do_work has a chance to run, we set pending=0
and irq_ack=1 in kvm_pit_reset_reinject.  pit_do_work is executed, sets
irq_ack from 1 to 0, but injects only the NMI, because interrupt is
already in IRR.  When the interrupt does EOI, we don't reinject it,
because pending==0.

Barriers can't solve this, locking would be pretty awkward and I think
that having one interrupt more or less is ok for the delay policy. :)

|  +	atomic_set(&pit->pit_state.irq_ack, 1);

Callers of kvm_pit_reset_reinject are providing barriers, so assignment
can't be reordered into inappropriate places, but it wouldn't hurt to
add barriers here.

> Looks safe otherwise.  (Please also add a comment before the memory
> barriers to show the pairing).

Thanks, I'll comment what I can.

[toc] | [prev] | [next] | [standalone]

#1325618 — [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy

Discard policy doesn't rely on information from notifiers, so we don't
need to register notifiers unconditionally.

Use of ps->lock doesn't make sense, but isn't any worse than before.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++--------------
 arch/x86/kvm/i8254.h |  1 +
 arch/x86/kvm/x86.c   |  6 +++---
 3 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index fc554fbf71a7..f1dfc250bbe0 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -237,9 +237,6 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
 	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
 						 irq_ack_notifier);
 
-	if (!ps->reinject)
-		return;
-
 	atomic_set(&ps->irq_ack, 1);
 	if (atomic_add_unless(&ps->pending, -1, 0))
 		/* in this case, we had multiple outstanding pit interrupts
@@ -708,14 +705,13 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 	hrtimer_init(&pit_state->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
 	pit_state->irq_ack_notifier.gsi = 0;
 	pit_state->irq_ack_notifier.irq_acked = kvm_pit_ack_irq;
-	kvm_register_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier);
-	pit_state->reinject = true;
 	mutex_unlock(&pit->pit_state.lock);
 
-	kvm_pit_reset(pit);
-
 	pit->mask_notifier.func = pit_mask_notifer;
-	kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
+
+	kvm_pit_set_reinject(pit, true);
+
+	kvm_pit_reset(pit);
 
 	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
 	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS,
@@ -738,14 +734,37 @@ fail_unregister:
 	kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &pit->dev);
 
 fail:
-	kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
-	kvm_unregister_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier);
+	kvm_pit_set_reinject(pit, false);
 	kvm_free_irq_source_id(kvm, pit->irq_source_id);
 	kthread_stop(pit->worker_task);
 	kfree(pit);
 	return NULL;
 }
 
+void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject)
+{
+	/* Implicit BUG on NULL dereference. */
+	struct kvm_kpit_state *ps = &pit->pit_state;
+	struct kvm *kvm = pit->kvm;
+
+	mutex_lock(&ps->lock);
+
+	if (ps->reinject == reinject)
+		goto out;
+
+	ps->reinject = reinject;
+	if (reinject) {
+		kvm_pit_reset_reinject(pit);
+		kvm_register_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
+		kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
+	} else {
+		kvm_unregister_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
+		kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
+	}
+out:
+	mutex_unlock(&ps->lock);
+}
+
 void kvm_free_pit(struct kvm *kvm)
 {
 	struct hrtimer *timer;
@@ -754,10 +773,7 @@ void kvm_free_pit(struct kvm *kvm)
 		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &kvm->arch.vpit->dev);
 		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
 					      &kvm->arch.vpit->speaker_dev);
-		kvm_unregister_irq_mask_notifier(kvm, 0,
-					       &kvm->arch.vpit->mask_notifier);
-		kvm_unregister_irq_ack_notifier(kvm,
-				&kvm->arch.vpit->pit_state.irq_ack_notifier);
+		kvm_pit_set_reinject(kvm->arch.vpit, false);
 		mutex_lock(&kvm->arch.vpit->pit_state.lock);
 		timer = &kvm->arch.vpit->pit_state.timer;
 		hrtimer_cancel(timer);
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index f8cf4b84f435..7ac94bc0799f 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -60,5 +60,6 @@ void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val, int hpet_legacy_s
 struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags);
 void kvm_free_pit(struct kvm *kvm);
 void kvm_pit_reset(struct kvm_pit *pit);
+void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject);
 
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5b937fdebc66..6bc03141e809 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3652,9 +3652,9 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
 {
 	if (!kvm->arch.vpit)
 		return -ENXIO;
-	mutex_lock(&kvm->arch.vpit->pit_state.lock);
-	kvm->arch.vpit->pit_state.reinject = control->pit_reinject;
-	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
+
+	kvm_pit_set_reinject(kvm->arch.vpit, control->pit_reinject);
+
 	return 0;
 }
 
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1325681 — Re: [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy

On 03/02/2016 17:23, Radim Krčmář wrote:
> Discard policy doesn't rely on information from notifiers, so we don't
> need to register notifiers unconditionally.
> 
> Use of ps->lock doesn't make sense, but isn't any worse than before.

Oh, it's perfectly okay.  Too fine-grained locks are bad, and lock
contention on ps->lock is a non-issue.

Can you however add a patch that says what fields of kvm_kpit_state are
protected by which locks?  Then this patch will just add

	/* Protected by kvm_kpit_state lock.  */

above the reinject field.

Otherwise

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++--------------
>  arch/x86/kvm/i8254.h |  1 +
>  arch/x86/kvm/x86.c   |  6 +++---
>  3 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index fc554fbf71a7..f1dfc250bbe0 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -237,9 +237,6 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
>  	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
>  						 irq_ack_notifier);
>  
> -	if (!ps->reinject)
> -		return;
> -
>  	atomic_set(&ps->irq_ack, 1);
>  	if (atomic_add_unless(&ps->pending, -1, 0))
>  		/* in this case, we had multiple outstanding pit interrupts
> @@ -708,14 +705,13 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
>  	hrtimer_init(&pit_state->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>  	pit_state->irq_ack_notifier.gsi = 0;
>  	pit_state->irq_ack_notifier.irq_acked = kvm_pit_ack_irq;
> -	kvm_register_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier);
> -	pit_state->reinject = true;
>  	mutex_unlock(&pit->pit_state.lock);
>  
> -	kvm_pit_reset(pit);
> -
>  	pit->mask_notifier.func = pit_mask_notifer;
> -	kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> +
> +	kvm_pit_set_reinject(pit, true);
> +
> +	kvm_pit_reset(pit);
>  
>  	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
>  	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS,
> @@ -738,14 +734,37 @@ fail_unregister:
>  	kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &pit->dev);
>  
>  fail:
> -	kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> -	kvm_unregister_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier);
> +	kvm_pit_set_reinject(pit, false);
>  	kvm_free_irq_source_id(kvm, pit->irq_source_id);
>  	kthread_stop(pit->worker_task);
>  	kfree(pit);
>  	return NULL;
>  }
>  
> +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject)
> +{
> +	/* Implicit BUG on NULL dereference. */
> +	struct kvm_kpit_state *ps = &pit->pit_state;
> +	struct kvm *kvm = pit->kvm;
> +
> +	mutex_lock(&ps->lock);
> +
> +	if (ps->reinject == reinject)
> +		goto out;
> +
> +	ps->reinject = reinject;
> +	if (reinject) {
> +		kvm_pit_reset_reinject(pit);
> +		kvm_register_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
> +		kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> +	} else {
> +		kvm_unregister_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
> +		kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> +	}
> +out:
> +	mutex_unlock(&ps->lock);
> +}
> +
>  void kvm_free_pit(struct kvm *kvm)
>  {
>  	struct hrtimer *timer;
> @@ -754,10 +773,7 @@ void kvm_free_pit(struct kvm *kvm)
>  		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &kvm->arch.vpit->dev);
>  		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
>  					      &kvm->arch.vpit->speaker_dev);
> -		kvm_unregister_irq_mask_notifier(kvm, 0,
> -					       &kvm->arch.vpit->mask_notifier);
> -		kvm_unregister_irq_ack_notifier(kvm,
> -				&kvm->arch.vpit->pit_state.irq_ack_notifier);
> +		kvm_pit_set_reinject(kvm->arch.vpit, false);
>  		mutex_lock(&kvm->arch.vpit->pit_state.lock);
>  		timer = &kvm->arch.vpit->pit_state.timer;
>  		hrtimer_cancel(timer);
> diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
> index f8cf4b84f435..7ac94bc0799f 100644
> --- a/arch/x86/kvm/i8254.h
> +++ b/arch/x86/kvm/i8254.h
> @@ -60,5 +60,6 @@ void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val, int hpet_legacy_s
>  struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags);
>  void kvm_free_pit(struct kvm *kvm);
>  void kvm_pit_reset(struct kvm_pit *pit);
> +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject);
>  
>  #endif
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5b937fdebc66..6bc03141e809 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3652,9 +3652,9 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
>  {
>  	if (!kvm->arch.vpit)
>  		return -ENXIO;
> -	mutex_lock(&kvm->arch.vpit->pit_state.lock);
> -	kvm->arch.vpit->pit_state.reinject = control->pit_reinject;
> -	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
> +
> +	kvm_pit_set_reinject(kvm->arch.vpit, control->pit_reinject);
> +
>  	return 0;
>  }
>  
> 

[toc] | [prev] | [next] | [standalone]

#1326784 — Re: [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy

2016-02-03 17:51+0100, Paolo Bonzini:
> On 03/02/2016 17:23, Radim Krčmář wrote:
>> Discard policy doesn't rely on information from notifiers, so we don't
>> need to register notifiers unconditionally.
>> 
>> Use of ps->lock doesn't make sense, but isn't any worse than before.

Oops, it is worse than before ... toggling KVM_REINJECT_CONTROL when the
guest is running and reading reinject without locking is now far more
complex.  This patch should have also ignored KVM_REINJECT_CONTROL when
PIT has been started.

> Oh, it's perfectly okay.  Too fine-grained locks are bad, and lock
> contention on ps->lock is a non-issue.
> 
> Can you however add a patch that says what fields of kvm_kpit_state are
> protected by which locks?

Ok.  (I'll be careful to not rewrite the whole PIT while at it. :])

>                            Then this patch will just add
> 
> 	/* Protected by kvm_kpit_state lock.  */
> 
> above the reinject field.

There was no need to lock reinject in the past and v2 will hopefully
achieve it again.

> Otherwise
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks.  (Might not be applicable to v2, though; sorry.)

[toc] | [prev] | [next] | [standalone]

#1325621 — [PATCH 1/4] KVM: x86: fix interrupt dropping race in PIT

A misuse of atomic operations opened a window

             kvm_pit_ack_irq:              |         pit_timer_fn:
  value = atomic_dec_return(&ps->pending); |
                                           |  !atomic_read(&ps->pending)
  if (value < 0) atomic_inc(&ps->pending); |

If ps->pending starts as 0 and we are using the discard policy, we don't
inject any interrupt in kvm_pit_ack_irq or pit_timer_fn, leading to a
missed PIT cycle.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/i8254.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index b0ea42b78ccd..de5f5018026f 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -239,13 +239,7 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
 	int value;
 
 	spin_lock(&ps->inject_lock);
-	value = atomic_dec_return(&ps->pending);
-	if (value < 0)
-		/* spurious acks can be generated if, for example, the
-		 * PIC is being reset.  Handle it gracefully here
-		 */
-		atomic_inc(&ps->pending);
-	else if (value > 0)
+	if (atomic_add_unless(&ps->pending, -1, 0))
 		/* in this case, we had multiple outstanding pit interrupts
 		 * that we needed to inject.  Reinject
 		 */
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1326802 — Re: [PATCH 1/4] KVM: x86: fix interrupt dropping race in PIT

2016-02-03 17:23+0100, Radim Krčmář:
> A misuse of atomic operations opened a window
> 
>              kvm_pit_ack_irq:              |         pit_timer_fn:
>   value = atomic_dec_return(&ps->pending); |
>                                            |  !atomic_read(&ps->pending)
>   if (value < 0) atomic_inc(&ps->pending); |
> 
> If ps->pending starts as 0 and we are using the discard policy, we don't
> inject any interrupt in kvm_pit_ack_irq or pit_timer_fn, leading to a
> missed PIT cycle.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> @@ -239,13 +239,7 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
>  	int value;
>  
>  	spin_lock(&ps->inject_lock);
> -	value = atomic_dec_return(&ps->pending);
> -	if (value < 0)
> -		/* spurious acks can be generated if, for example, the
> -		 * PIC is being reset.  Handle it gracefully here
> -		 */
> -		atomic_inc(&ps->pending);
> -	else if (value > 0)
> +	if (atomic_add_unless(&ps->pending, -1, 0))

This is not right, I'll redo the series.  Sorry.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web