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

From: David Woodhouse
Date: Tue Aug 09 2022 - 10:17:17 EST


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@xxxxxxxxxxxxx/
> >
>
> 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: smime.p7s
Description: S/MIME cryptographic signature