linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once
@ 2022-08-08 19:06 Coleman Dietsch
  2022-08-08 19:06 ` [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ Coleman Dietsch
  2022-08-09  0:32 ` [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once Sean Christopherson via Linux-kernel-mentees
  0 siblings, 2 replies; 14+ messages in thread
From: Coleman Dietsch @ 2022-08-08 19:06 UTC (permalink / raw)
  To: kvm
  Cc: x86, Sean Christopherson, Dave Hansen, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Paolo Bonzini, Thomas Gleixner, Pavel Skripkin,
	linux-kernel-mentees

Add a check for existing xen timers before initializing a new one.

Currently kvm_xen_init_timer() is called on every
KVM_XEN_VCPU_ATTR_TYPE_TIMER, which is causing the following ODEBUG
crash when vcpu->arch.xen.timer is already set.

ODEBUG: init active (active state 0)
object type: hrtimer hint: xen_timer_callbac0
RIP: 0010:debug_print_object+0x16e/0x250 lib/debugobjects.c:502
Call Trace:
__debug_object_init
debug_hrtimer_init
debug_init
hrtimer_init
kvm_xen_init_timer
kvm_xen_vcpu_set_attr
kvm_arch_vcpu_ioctl
kvm_vcpu_ioctl
vfs_ioctl

Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode")
Cc: stable@vger.kernel.org
Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
---
 arch/x86/kvm/xen.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index a0c05ccbf4b1..6e554041e862 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -713,7 +713,9 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 				break;
 			}
 			vcpu->arch.xen.timer_virq = data->u.timer.port;
-			kvm_xen_init_timer(vcpu);
+
+			if (!vcpu->arch.xen.timer.function)
+				kvm_xen_init_timer(vcpu);
 
 			/* Restart the timer if it's set */
 			if (data->u.timer.expires_ns)
-- 
2.34.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-08 19:06 [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once Coleman Dietsch
@ 2022-08-08 19:06 ` Coleman Dietsch
  2022-08-09  0:34   ` Sean Christopherson via Linux-kernel-mentees
  2022-08-09  9:22   ` David Woodhouse
  2022-08-09  0:32 ` [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once Sean Christopherson via Linux-kernel-mentees
  1 sibling, 2 replies; 14+ messages in thread
From: Coleman Dietsch @ 2022-08-08 19:06 UTC (permalink / raw)
  To: kvm
  Cc: x86, Sean Christopherson, Dave Hansen, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Paolo Bonzini, Thomas Gleixner, Pavel Skripkin,
	linux-kernel-mentees

Stop Xen timer (if it's running) prior to changing the IRQ vector and
potentially (re)starting the timer. Changing the IRQ vector while the
timer is still running can result in KVM injecting a garbage event, e.g.
vm_xen_inject_timer_irqs() could see a non-zero xen.timer_pending from
a previous timer but inject the new xen.timer_virq.

Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode")
Cc: stable@vger.kernel.org
Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
---
 arch/x86/kvm/xen.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 6e554041e862..280cb5dc7341 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -707,26 +707,25 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 		break;
 
 	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
-		if (data->u.timer.port) {
-			if (data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
-				r = -EINVAL;
-				break;
-			}
-			vcpu->arch.xen.timer_virq = data->u.timer.port;
-
-			if (!vcpu->arch.xen.timer.function)
-				kvm_xen_init_timer(vcpu);
-
-			/* Restart the timer if it's set */
-			if (data->u.timer.expires_ns)
-				kvm_xen_start_timer(vcpu, data->u.timer.expires_ns,
-						    data->u.timer.expires_ns -
-						    get_kvmclock_ns(vcpu->kvm));
-		} else if (kvm_xen_timer_enabled(vcpu)) {
-			kvm_xen_stop_timer(vcpu);
-			vcpu->arch.xen.timer_virq = 0;
+		if (data->u.timer.port &&
+		    data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
+			r = -EINVAL;
+			break;
 		}
 
+		if (!vcpu->arch.xen.timer.function)
+			kvm_xen_init_timer(vcpu);
+
+		/* Stop the timer (if it's running) before changing the vector */
+		kvm_xen_stop_timer(vcpu);
+		vcpu->arch.xen.timer_virq = data->u.timer.port;
+
+		/* Start the timer if the new value has a valid vector+expiry. */
+		if (data->u.timer.port && data->u.timer.expires_ns)
+			kvm_xen_start_timer(vcpu, data->u.timer.expires_ns,
+					    data->u.timer.expires_ns -
+					    get_kvmclock_ns(vcpu->kvm));
+
 		r = 0;
 		break;
 
-- 
2.34.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once
  2022-08-08 19:06 [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once Coleman Dietsch
  2022-08-08 19:06 ` [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ Coleman Dietsch
@ 2022-08-09  0:32 ` Sean Christopherson via Linux-kernel-mentees
  2022-08-09 12:59   ` Paolo Bonzini
  1 sibling, 1 reply; 14+ messages in thread
From: Sean Christopherson via Linux-kernel-mentees @ 2022-08-09  0:32 UTC (permalink / raw)
  To: Coleman Dietsch
  Cc: x86, kvm, Pavel Skripkin, Dave Hansen, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Paolo Bonzini, Thomas Gleixner,
	linux-kernel-mentees

On Mon, Aug 08, 2022, Coleman Dietsch wrote:
> Add a check for existing xen timers before initializing a new one.
> 
> Currently kvm_xen_init_timer() is called on every
> KVM_XEN_VCPU_ATTR_TYPE_TIMER, which is causing the following ODEBUG
> crash when vcpu->arch.xen.timer is already set.
> 
> ODEBUG: init active (active state 0)
> object type: hrtimer hint: xen_timer_callbac0
> RIP: 0010:debug_print_object+0x16e/0x250 lib/debugobjects.c:502
> Call Trace:
> __debug_object_init
> debug_hrtimer_init
> debug_init
> hrtimer_init
> kvm_xen_init_timer
> kvm_xen_vcpu_set_attr
> kvm_arch_vcpu_ioctl
> kvm_vcpu_ioctl
> vfs_ioctl
> 
> Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode")
> Cc: stable@vger.kernel.org
> Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
> Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
> Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-08 19:06 ` [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ Coleman Dietsch
@ 2022-08-09  0:34   ` Sean Christopherson via Linux-kernel-mentees
  2022-08-09  9:22   ` David Woodhouse
  1 sibling, 0 replies; 14+ messages in thread
From: Sean Christopherson via Linux-kernel-mentees @ 2022-08-09  0:34 UTC (permalink / raw)
  To: Coleman Dietsch
  Cc: x86, kvm, Pavel Skripkin, Dave Hansen, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Paolo Bonzini, Thomas Gleixner,
	linux-kernel-mentees

On Mon, Aug 08, 2022, Coleman Dietsch wrote:
> Stop Xen timer (if it's running) prior to changing the IRQ vector and
> potentially (re)starting the timer. Changing the IRQ vector while the
> timer is still running can result in KVM injecting a garbage event, e.g.
> vm_xen_inject_timer_irqs() could see a non-zero xen.timer_pending from
> a previous timer but inject the new xen.timer_virq.
> 
> Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode")
> Cc: stable@vger.kernel.org
> Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
> Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
> Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-08 19:06 ` [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ Coleman Dietsch
  2022-08-09  0:34   ` Sean Christopherson via Linux-kernel-mentees
@ 2022-08-09  9:22   ` David Woodhouse
  2022-08-09 12:59     ` Paolo Bonzini
  1 sibling, 1 reply; 14+ messages in thread
From: David Woodhouse @ 2022-08-09  9:22 UTC (permalink / raw)
  To: Coleman Dietsch, kvm
  Cc: x86, Sean Christopherson, Dave Hansen, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Paolo Bonzini, Thomas Gleixner, Pavel Skripkin,
	linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 569 bytes --]

On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
> Stop Xen timer (if it's running) prior to changing the IRQ vector and
> potentially (re)starting the timer. Changing the IRQ vector while the
> timer is still running can result in KVM injecting a garbage event, e.g.
> vm_xen_inject_timer_irqs() could see a non-zero xen.timer_pending from
> a previous timer but inject the new xen.timer_virq.

Hm, wasn't that already addressed in the first patch I saw, which just
called kvm_xen_stop_timer() unconditionally before (possibly) setting
it up again?

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-09  9:22   ` David Woodhouse
@ 2022-08-09 12:59     ` Paolo Bonzini
  2022-08-09 13:51       ` David Woodhouse
  0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2022-08-09 12:59 UTC (permalink / raw)
  To: David Woodhouse, Coleman Dietsch, kvm
  Cc: Dave Hansen, Sean Christopherson, x86, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Thomas Gleixner, Pavel Skripkin,
	linux-kernel-mentees

On 8/9/22 11:22, David Woodhouse wrote:
> On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
>> Stop Xen timer (if it's running) prior to changing the IRQ vector and
>> potentially (re)starting the timer. Changing the IRQ vector while the
>> timer is still running can result in KVM injecting a garbage event, e.g.
>> vm_xen_inject_timer_irqs() could see a non-zero xen.timer_pending from
>> a previous timer but inject the new xen.timer_virq.
> 
> Hm, wasn't that already addressed in the first patch I saw, which just
> called kvm_xen_stop_timer() unconditionally before (possibly) setting
> it up again?

Which patch is that?

Paolo

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once
  2022-08-09  0:32 ` [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once Sean Christopherson via Linux-kernel-mentees
@ 2022-08-09 12:59   ` Paolo Bonzini
  0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2022-08-09 12:59 UTC (permalink / raw)
  To: Sean Christopherson, Coleman Dietsch
  Cc: x86, kvm, Pavel Skripkin, Dave Hansen, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Thomas Gleixner, linux-kernel-mentees

On 8/9/22 02:32, Sean Christopherson wrote:
> On Mon, Aug 08, 2022, Coleman Dietsch wrote:
>> Add a check for existing xen timers before initializing a new one.
>>
>> Currently kvm_xen_init_timer() is called on every
>> KVM_XEN_VCPU_ATTR_TYPE_TIMER, which is causing the following ODEBUG
>> crash when vcpu->arch.xen.timer is already set.
>>
>> ODEBUG: init active (active state 0)
>> object type: hrtimer hint: xen_timer_callbac0
>> RIP: 0010:debug_print_object+0x16e/0x250 lib/debugobjects.c:502
>> Call Trace:
>> __debug_object_init
>> debug_hrtimer_init
>> debug_init
>> hrtimer_init
>> kvm_xen_init_timer
>> kvm_xen_vcpu_set_attr
>> kvm_arch_vcpu_ioctl
>> kvm_vcpu_ioctl
>> vfs_ioctl
>>
>> Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode")
>> Cc: stable@vger.kernel.org
>> Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
>> Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
>> Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
>> ---
> 
> Reviewed-by: Sean Christopherson <seanjc@google.com>
> 

Queued both (pending resolution of David's question), thanks.

Paolo

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-09 12:59     ` Paolo Bonzini
@ 2022-08-09 13:51       ` David Woodhouse
  2022-08-09 14:07         ` Sean Christopherson via Linux-kernel-mentees
  0 siblings, 1 reply; 14+ messages in thread
From: David Woodhouse @ 2022-08-09 13:51 UTC (permalink / raw)
  To: Paolo Bonzini, Coleman Dietsch, kvm
  Cc: Dave Hansen, Sean Christopherson, x86, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Thomas Gleixner, Pavel Skripkin,
	linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 850 bytes --]

On Tue, 2022-08-09 at 14:59 +0200, Paolo Bonzini wrote:
> On 8/9/22 11:22, David Woodhouse wrote:
> > On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
> > > Stop Xen timer (if it's running) prior to changing the IRQ vector and
> > > potentially (re)starting the timer. Changing the IRQ vector while the
> > > timer is still running can result in KVM injecting a garbage event, e.g.
> > > vm_xen_inject_timer_irqs() could see a non-zero xen.timer_pending from
> > > a previous timer but inject the new xen.timer_virq.
> > 
> > Hm, wasn't that already addressed in the first patch I saw, which just
> > called kvm_xen_stop_timer() unconditionally before (possibly) setting
> > it up again?
> 
> Which patch is that?

The one I acked in
https://lore.kernel.org/all/9bad724858b6a06c25ead865b2b3d9dfc216d01c.camel@infradead.org/

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-09 13:51       ` David Woodhouse
@ 2022-08-09 14:07         ` Sean Christopherson via Linux-kernel-mentees
  2022-08-09 14:16           ` David Woodhouse
  0 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson via Linux-kernel-mentees @ 2022-08-09 14:07 UTC (permalink / raw)
  To: David Woodhouse
  Cc: x86, kvm, Pavel Skripkin, Dave Hansen, linux-kernel, stable,
	syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
	H . Peter Anvin, Paolo Bonzini, Thomas Gleixner,
	linux-kernel-mentees

On Tue, Aug 09, 2022, David Woodhouse wrote:
> On Tue, 2022-08-09 at 14:59 +0200, Paolo Bonzini wrote:
> > On 8/9/22 11:22, David Woodhouse wrote:
> > > On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
> > > > Stop Xen timer (if it's running) prior to changing the IRQ vector and
> > > > potentially (re)starting the timer. Changing the IRQ vector while the
> > > > timer is still running can result in KVM injecting a garbage event, e.g.
> > > > vm_xen_inject_timer_irqs() could see a non-zero xen.timer_pending from
> > > > a previous timer but inject the new xen.timer_virq.
> > > 
> > > Hm, wasn't that already addressed in the first patch I saw, which just
> > > called kvm_xen_stop_timer() unconditionally before (possibly) setting
> > > it up again?
> > 
> > Which patch is that?
> 
> The one I acked in
> https://lore.kernel.org/all/9bad724858b6a06c25ead865b2b3d9dfc216d01c.camel@infradead.org/

It's effectively the same patch.  I had asked Coleman to split it into two separate
patches: (1) fix the re-initialization of an active timer bug and (2) stop the active
timer before changing the vector (this patch).
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-09 14:07         ` Sean Christopherson via Linux-kernel-mentees
@ 2022-08-09 14:16           ` David Woodhouse
  2022-08-09 14:31             ` Paolo Bonzini
  0 siblings, 1 reply; 14+ messages in thread
From: David Woodhouse @ 2022-08-09 14:16 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: x86, metikaya, kvm, Pavel Skripkin, Dave Hansen, linux-kernel,
	stable, syzbot+e54f930ed78eb0f85281, Ingo Molnar,
	Borislav Petkov, H . Peter Anvin, Paolo Bonzini, Thomas Gleixner,
	linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 2954 bytes --]

On Tue, 2022-08-09 at 14:07 +0000, Sean Christopherson wrote:
> On Tue, Aug 09, 2022, David Woodhouse wrote:
> > On Tue, 2022-08-09 at 14:59 +0200, Paolo Bonzini wrote:
> > > On 8/9/22 11:22, David Woodhouse wrote:
> > > > On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
> > > > > Stop Xen timer (if it's running) prior to changing the IRQ
> > > > > vector and
> > > > > potentially (re)starting the timer. Changing the IRQ vector
> > > > > while the
> > > > > timer is still running can result in KVM injecting a garbage
> > > > > event, e.g.
> > > > > vm_xen_inject_timer_irqs() could see a non-zero
> > > > > xen.timer_pending from
> > > > > a previous timer but inject the new xen.timer_virq.
> > > > 
> > > > Hm, wasn't that already addressed in the first patch I saw,
> > > > which just
> > > > called kvm_xen_stop_timer() unconditionally before (possibly)
> > > > setting
> > > > it up again?
> > > 
> > > Which patch is that?
> > 
> > The one I acked in
> > https://lore.kernel.org/all/9bad724858b6a06c25ead865b2b3d9dfc216d01c.camel@infradead.org/
> > 
> 
> It's effectively the same patch.  I had asked Coleman to split it into two separate
> patches: (1) fix the re-initialization of an active timer bug and (2) stop the active
> timer before changing the vector (this patch).
> 

But both bugs just require that the timer is stopped first. I preferred
the original which was less intrusive, which did just that:

	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
		/* Stop current timer if it is enabled */
		if (kvm_xen_timer_enabled(vcpu)) {
			kvm_xen_stop_timer(vcpu);
			vcpu->arch.xen.timer_virq = 0;
		}
		if (data->u.timer.port) {
			if (data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
				r = -EINVAL;
				break;
			}
			vcpu->arch.xen.timer_virq = data->u.timer.port;
			kvm_xen_init_timer(vcpu);

			/* Restart the timer if it's set */
			if (data->u.timer.expires_ns)
				kvm_xen_start_timer(vcpu, data->u.timer.expires_ns,
						    data->u.timer.expires_ns -
						    get_kvmclock_ns(vcpu->kvm));
		}
		r = 0;
		break;


I find the new version a bit harder to follow, with its init-then-stop-
then-start logic:

	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
		if (data->u.timer.port &&
		    data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
			r = -EINVAL;
			break;
                }

		if (!vcpu->arch.xen.timer.function)
			kvm_xen_init_timer(vcpu);

		/* Stop the timer (if it's running) before changing the vector */
		kvm_xen_stop_timer(vcpu);
		vcpu->arch.xen.timer_virq = data->u.timer.port;

		/* Start the timer if the new value has a valid vector+expiry. */
		if (data->u.timer.port && data->u.timer.expires_ns)
			kvm_xen_start_timer(vcpu, data->u.timer.expires_ns,
					    data->u.timer.expires_ns -
					    get_kvmclock_ns(vcpu->kvm));
                r = 0;
		break;

But I won't fight you for it. 


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-09 14:16           ` David Woodhouse
@ 2022-08-09 14:31             ` Paolo Bonzini
  2022-08-09 14:36               ` David Woodhouse
  2022-08-09 14:40               ` Sean Christopherson via Linux-kernel-mentees
  0 siblings, 2 replies; 14+ messages in thread
From: Paolo Bonzini @ 2022-08-09 14:31 UTC (permalink / raw)
  To: David Woodhouse, Sean Christopherson
  Cc: x86, metikaya, kvm, Pavel Skripkin, Dave Hansen, linux-kernel,
	stable, syzbot+e54f930ed78eb0f85281, Ingo Molnar,
	Borislav Petkov, H . Peter Anvin, Thomas Gleixner,
	linux-kernel-mentees

On 8/9/22 16:16, David Woodhouse wrote:
> I find the new version a bit harder to follow, with its init-then-stop-
> then-start logic:
> 
> 	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
> 		if (data->u.timer.port &&
> 		    data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
> 			r = -EINVAL;
> 			break;
>                  }
> 
> 		if (!vcpu->arch.xen.timer.function)
> 			kvm_xen_init_timer(vcpu);
> 
> 		/* Stop the timer (if it's running) before changing the vector */
> 		kvm_xen_stop_timer(vcpu);
> 		vcpu->arch.xen.timer_virq = data->u.timer.port;


I think this is fine, if anything the kvm_xen_stop_timer() call could be 
placed in an "else" but I'm leaning towards applying this version of the 
patch.

Paolo

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-09 14:31             ` Paolo Bonzini
@ 2022-08-09 14:36               ` David Woodhouse
  2022-08-09 14:40               ` Sean Christopherson via Linux-kernel-mentees
  1 sibling, 0 replies; 14+ messages in thread
From: David Woodhouse @ 2022-08-09 14:36 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson
  Cc: x86, metikaya, kvm, Pavel Skripkin, Dave Hansen, linux-kernel,
	stable, syzbot+e54f930ed78eb0f85281, Ingo Molnar,
	Borislav Petkov, H . Peter Anvin, Thomas Gleixner,
	linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 1061 bytes --]

On Tue, 2022-08-09 at 16:31 +0200, Paolo Bonzini wrote:
> On 8/9/22 16:16, David Woodhouse wrote:
> > I find the new version a bit harder to follow, with its init-then-stop-
> > then-start logic:
> > 
> > 	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
> > 		if (data->u.timer.port &&
> > 		    data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
> > 			r = -EINVAL;
> > 			break;
> >                  }
> > 
> > 		if (!vcpu->arch.xen.timer.function)
> > 			kvm_xen_init_timer(vcpu);
> > 
> > 		/* Stop the timer (if it's running) before changing the vector */
> > 		kvm_xen_stop_timer(vcpu);
> > 		vcpu->arch.xen.timer_virq = data->u.timer.port;
> 
> 
> I think this is fine, if anything the kvm_xen_stop_timer() call could be 
> placed in an "else" but I'm leaning towards applying this version of the 
> patch.

Fair enough. It should definitely work, and is functionally identical
in all interesting cases. In that case, for both of the patches from
this v3 series:

Acked-by: David Woodhouse <dwmw@amazon.co.uk>

Thanks.

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-09 14:31             ` Paolo Bonzini
  2022-08-09 14:36               ` David Woodhouse
@ 2022-08-09 14:40               ` Sean Christopherson via Linux-kernel-mentees
  2022-08-09 14:52                 ` David Woodhouse
  1 sibling, 1 reply; 14+ messages in thread
From: Sean Christopherson via Linux-kernel-mentees @ 2022-08-09 14:40 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: x86, metikaya, kvm, Pavel Skripkin, Dave Hansen, linux-kernel,
	stable, syzbot+e54f930ed78eb0f85281, linux-kernel-mentees,
	Ingo Molnar, Borislav Petkov, H . Peter Anvin, Thomas Gleixner,
	David Woodhouse

On Tue, Aug 09, 2022, Paolo Bonzini wrote:
> On 8/9/22 16:16, David Woodhouse wrote:
> > I find the new version a bit harder to follow, with its init-then-stop-
> > then-start logic:
> > 
> > 	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
> > 		if (data->u.timer.port &&
> > 		    data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) {
> > 			r = -EINVAL;
> > 			break;
> >                  }
> > 
> > 		if (!vcpu->arch.xen.timer.function)
> > 			kvm_xen_init_timer(vcpu);
> > 
> > 		/* Stop the timer (if it's running) before changing the vector */
> > 		kvm_xen_stop_timer(vcpu);
> > 		vcpu->arch.xen.timer_virq = data->u.timer.port;
> 
> 
> I think this is fine, if anything the kvm_xen_stop_timer() call could be
> placed in an "else" but I'm leaning towards applying this version of the
> patch.

I wanted to separate the "init" from the "stop+start", e.g. if there were a more
appropriate place for invoking kvm_xen_init_timer() I would have suggested moving
the call outside of KVM_XEN_VCPU_ATTR_TYPE_TIMER entirely.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ
  2022-08-09 14:40               ` Sean Christopherson via Linux-kernel-mentees
@ 2022-08-09 14:52                 ` David Woodhouse
  0 siblings, 0 replies; 14+ messages in thread
From: David Woodhouse @ 2022-08-09 14:52 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: x86, metikaya, kvm, Pavel Skripkin, Dave Hansen, linux-kernel,
	stable, syzbot+e54f930ed78eb0f85281, Ingo Molnar,
	Borislav Petkov, H . Peter Anvin, Thomas Gleixner,
	linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]

On Tue, 2022-08-09 at 14:40 +0000, Sean Christopherson wrote:
> I wanted to separate the "init" from the "stop+start", e.g. if there were a more
> appropriate place for invoking kvm_xen_init_timer() I would have suggested moving
> the call outside of KVM_XEN_VCPU_ATTR_TYPE_TIMER entirely.

Yeah, there's nowhere sensible that would apply to only Xen guests. I
looked at kvm_xen_init_vcpu() but that's unconditional.

I do note that we're now calling kvm_xen_init_timer() even when an
*unset* timer is being restored after live update/live migration. But I
think that's OK.

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-08-09 14:53 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-08 19:06 [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once Coleman Dietsch
2022-08-08 19:06 ` [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ Coleman Dietsch
2022-08-09  0:34   ` Sean Christopherson via Linux-kernel-mentees
2022-08-09  9:22   ` David Woodhouse
2022-08-09 12:59     ` Paolo Bonzini
2022-08-09 13:51       ` David Woodhouse
2022-08-09 14:07         ` Sean Christopherson via Linux-kernel-mentees
2022-08-09 14:16           ` David Woodhouse
2022-08-09 14:31             ` Paolo Bonzini
2022-08-09 14:36               ` David Woodhouse
2022-08-09 14:40               ` Sean Christopherson via Linux-kernel-mentees
2022-08-09 14:52                 ` David Woodhouse
2022-08-09  0:32 ` [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once Sean Christopherson via Linux-kernel-mentees
2022-08-09 12:59   ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).