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@i...
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.