On Thu, Jul 28, 2022 at 08:41:14PM +0000, Sean Christopherson wrote: > Be more specific in the shortlog. "Fix a bug in XYZ" doesn't provide any info > about the bug itself, and can even become frustratingly stale if XYZ is renamed. > I believe we should end up with two patches (see below), e.g. > > KVM: x86/xen: Initialize Xen timer only once (when it's NOT running) > > and > > KVM: x86/xen: Stop Xen timer before changing the IRQ vector > Got it, I will work on splitting the v2 into a patch set as you suggested (with better names of course). > Note, I'm assuming timer_virq is a vector of some form, I haven't actually looked > that far into the code. > > On Thu, Jul 28, 2022, Coleman Dietsch wrote: > > This crash appears to be happening when vcpu->arch.xen.timer is already set > > Instead of saying "This crash", provide the actual splat (sanitized to make it > more readable). That way readers, reviewers, and archaeologists don't need to > open up a hyperlink to get details on what broken. > > > and kvm_xen_init_timer(vcpu) is called. > > Wrap changelogs at ~75 chars. > > > During testing with the syzbot reproducer code it seemed apparent that the > > else if statement in the KVM_XEN_VCPU_ATTR_TYPE_TIMER switch case was not > > being reached, which is where the kvm_xen_stop_timer(vcpu) call is located. > > Neither the shortlog nor the changelog actually says anything about what is actually > being changed. > I will make sure to address all these issues in the v2 patch set. > > Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42 > > Reported-and-tested-by: syzbot+e54f930ed78eb0f85281@xxxxxxxxxxxxxxxxxxxxxxxxx > > Signed-off-by: Coleman Dietsch <dietschc@xxxxxxx> > > --- > > arch/x86/kvm/xen.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c > > index 610beba35907..4b4b985813c5 100644 > > --- a/arch/x86/kvm/xen.c > > +++ b/arch/x86/kvm/xen.c > > @@ -707,6 +707,12 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data) > > break; > > > > 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; > > I'm not entirely sure this is correct. Probably doesn't matter, but there's a > subtle ABI change here in that invoking the ioctl with a "bad" priority will > cancel any existing timer. > I will try to get some clarification before I send in the next patch. > And there appear to be two separate bugs: initializing the hrtimer while it's > running, and not canceling a running timer before changing timer_virq. > This does seem to be the case so I will be splitting v2 into a patch set. > Calling kvm_xen_init_timer() on "every" KVM_XEN_VCPU_ATTR_TYPE_TIMER is odd and > unnecessary, it only needs to be called once during vCPU setup. If Xen doesn't > have such a hook, then a !ULL check can be done on vcpu->arch.xen.timer.function > to initialize the timer on-demand. > Yes I also thought that was a bit odd that kvm_xen_init_timer() is called on "every" KVM_XEN_VCPU_ATTR_TYPE_TIMER > With that out of the way, the code can be streamlined a bit, e.g. something like > this? > > 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; > > 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; > I agree this code could use some cleanup, I'll see what I can do. > > @@ -720,9 +726,6 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data) > > 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; > > } > > > > r = 0; > > -- > > 2.34.1 > > Thank you for the feedback Sean, it has been most helpful!
