Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #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 #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]