Re: [PATCH v3 05/10] KVM: x86/xen: Consolidate checks on Xen vCPU ID for singleshot timer hypercalls

[David Woodhouse]

On Fri, 2026-06-26 at 07:19 -0700, Sean Christopherson wrote:
> On Fri, Jun 26, 2026, David Woodhouse wrote:
> > On Thu, 2026-06-25 at 15:36 -0700, Sean Christopherson wrote:
> > > Hoist the checks on the Xe

+	if (vcpu->arch.xen.vcpu_id == XEN_VCPU_ID_INVALID)
> > > +		return false;n vCPU ID when handling set_singleshot_timer and
> > > stop_singleshot_timer hypercalls out of their individual case-statements,
> > > so that both checks on the ID are in common code.  kvm_xen_hcall_vcpu_op()
> > > is already doubly committed to handling only singleshot timer hypercalls,
> > > and even if that were to change in the future, the function could simply
> > > be renamed and turned into a helper specifically for timer hypercalls.
> > > 
> > > No functional change intended.
> > 
> > Makes sense. In fact these hypercalls are the *only* VCPUOP_xxx calls
> > for which Xen has that restriction (otherwise it would be pointless to
> > have the vcpu argument at all). Which is why we did the check in the
> > individual cases.
> 
> Sashiko pointed out that the patch is broken as-is, because the effective
> "default" case will reject hypercalls if the vcpu_id doesn't match instead of
> routing those to userspace.  The easiest way to deal with that is to pull the
> cmd check out of the switch-statement, e.g.
> 
> 	struct vcpu_set_singleshot_timer oneshot;
> 	struct x86_exception e;
> 
> 	if (cmd != VCPUOP_set_singleshot_timer &&
> 	    cmd != VCPUOP_stop_singleshot_timer)
> 		return false;
> 
> 	if (!kvm_xen_timer_enabled(vcpu))
> 		return false;
> 

You dropped the 

+	if (vcpu->arch.xen.vcpu_id == XEN_VCPU_ID_INVALID)
+		return false;

part. Although that's kind of fair since I did ask you to put this
patch first, before that.

But really, while this rearrangement you propose would work... this
patch was intended as a cosmetic cleanup, and this seems less pretty
than what we had before.

How about keeping your patch 4 which just returns false for invalid
vcpu_id, and dropping patch 5 completely? Or if you really want to
tackle it, start with this...

--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1745,8 +1745,10 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
                                                 params[1], &r);
                break;
        case __HYPERVISOR_vcpu_op:
-               handled = kvm_xen_hcall_vcpu_op(vcpu, longmode, params[0], params[1],
-                                               params[2], &r);
+               if (params[0] == VCPUOP_set_singleshot_timer ||
+                   params[0] == VCPUOP_stop_singleshot_timer)
+                       handled = kvm_xen_hcall_vcpu_op(vcpu, longmode, params[0], params[1],
+                                                       params[2], &r);
                break;
        case __HYPERVISOR_set_timer_op: {
                u64 timeout = params[0];

> 
> > But these are *also* the only VCPUOP calls we're ever likely to
> > accelerate in the kernel, so that's actually fine. I would prefer to
> > see a comment above the check though.
> 
> This?
> 
> 	/*
> 	 * Reject the hypercall if the guest is trying to start/stop the timer
> 	 * for a different vCPU.  Xen per-vCPU hypercalls take a target vCPU as
> 	 * a common parameter, as all per-vCPU hypercalls *except* single-shot
> 	 * timer updates can be cross-vCPU.
> 	 */
> 	if (vcpu->arch.xen.vcpu_id != vcpu_id) {
> 		*r = -EINVAL;
> 		return true;
> 	}

I guess, although I'm no longer keen on the consolidation at all.

Attachment: smime.p7s
Description: S/MIME cryptographic signature