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

[Sean Christopherson <seanjc@google.com>]

On Fri, Jun 26, 2026, David Woodhouse wrote:
> 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.

Heh, it's still there, I just didn't include it in this snippet.

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

I disagree.  The old code uses a common check for kvm_xen_timer_enabled(), which
is confusing and actively dangerous, as evidenced by my goof, because it's easy
to miss that the only reason KVM can bail early for !kvm_xen_timer_enabled() is
because it returns false.  Punting hypercalls that are never accelerated by KVM
because some other hypercall happens to be disabled is nasty.

> 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);

But then kvm_xen_hcall_vcpu_op() is a misleading name, and this also splits the
checks on @cmd.  E.g. even with this:

		if (params[0] != VCPUOP_set_singleshot_timer &&
		    params[0] != VCPUOP_stop_singleshot_timer)
			break;

		handled = kvm_xen_hcall_single_shot_timer(vcpu, longmode, params[0],
							  params[1], params[2], &r);
		break;

Then kvm_xen_hcall_single_shot_timer() either looks flawed if it does:

	if (cmd == VCPUOP_set_singleshot_timer) {
		...

		kvm_xen_start_timer(vcpu, oneshot.timeout_abs_ns, false);
	} else {
		kvm_xen_stop_timer(vcpu);
	}

because it doesn't explicitly check that cmd is set_singleshot_timer or
stop_singleshot_timer.  We could do:

	if (cmd == VCPUOP_set_singleshot_timer) {
		...

		kvm_xen_start_timer(vcpu, oneshot.timeout_abs_ns, false);
	} else {
		WARN_ON_ONCE(cmd != VCPUOP_stop_singleshot_timer);

		kvm_xen_stop_timer(vcpu);
	}

but that's rather ridiculous given that there's exactly one path to this code.

IMO, this:

	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;

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

	/*
	 * 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;
	}

	if (cmd == VCPUOP_set_singleshot_timer) {
		/*
		 * The only difference for 32-bit compat is the 4 bytes of
		 * padding after the interesting part of the structure. So
		 * for a faithful emulation of Xen we have to *try* to copy
		 * the padding and return -EFAULT if we can't. Otherwise we
		 * might as well just have copied the 12-byte 32-bit struct.
		 */
		BUILD_BUG_ON(offsetof(struct compat_vcpu_set_singleshot_timer, timeout_abs_ns) !=
			     offsetof(struct vcpu_set_singleshot_timer, timeout_abs_ns));
		BUILD_BUG_ON(sizeof_field(struct compat_vcpu_set_singleshot_timer, timeout_abs_ns) !=
			     sizeof_field(struct vcpu_set_singleshot_timer, timeout_abs_ns));
		BUILD_BUG_ON(offsetof(struct compat_vcpu_set_singleshot_timer, flags) !=
			     offsetof(struct vcpu_set_singleshot_timer, flags));
		BUILD_BUG_ON(sizeof_field(struct compat_vcpu_set_singleshot_timer, flags) !=
			     sizeof_field(struct vcpu_set_singleshot_timer, flags));

		if (kvm_read_guest_virt(vcpu, param, &oneshot, longmode ? sizeof(oneshot) :
					sizeof(struct compat_vcpu_set_singleshot_timer), &e)) {
			*r = -EFAULT;
			return true;
		}

		kvm_xen_start_timer(vcpu, oneshot.timeout_abs_ns, false);
	} else {
		kvm_xen_stop_timer(vcpu);
	}

	*r = 0;
	return true;

is logically more sound and easier to read than what we currently have:

	struct vcpu_set_singleshot_timer oneshot;
	struct x86_exception e;

	if (!kvm_xen_timer_enabled(vcpu))
		return false;

	switch (cmd) {
	case VCPUOP_set_singleshot_timer:
		if (vcpu->arch.xen.vcpu_id != vcpu_id) {
			*r = -EINVAL;
			return true;
		}

		/*
		 * The only difference for 32-bit compat is the 4 bytes of
		 * padding after the interesting part of the structure. So
		 * for a faithful emulation of Xen we have to *try* to copy
		 * the padding and return -EFAULT if we can't. Otherwise we
		 * might as well just have copied the 12-byte 32-bit struct.
		 */
		BUILD_BUG_ON(offsetof(struct compat_vcpu_set_singleshot_timer, timeout_abs_ns) !=
			     offsetof(struct vcpu_set_singleshot_timer, timeout_abs_ns));
		BUILD_BUG_ON(sizeof_field(struct compat_vcpu_set_singleshot_timer, timeout_abs_ns) !=
			     sizeof_field(struct vcpu_set_singleshot_timer, timeout_abs_ns));
		BUILD_BUG_ON(offsetof(struct compat_vcpu_set_singleshot_timer, flags) !=
			     offsetof(struct vcpu_set_singleshot_timer, flags));
		BUILD_BUG_ON(sizeof_field(struct compat_vcpu_set_singleshot_timer, flags) !=
			     sizeof_field(struct vcpu_set_singleshot_timer, flags));

		if (kvm_read_guest_virt(vcpu, param, &oneshot, longmode ? sizeof(oneshot) :
					sizeof(struct compat_vcpu_set_singleshot_timer), &e)) {
			*r = -EFAULT;
			return true;
		}

		kvm_xen_start_timer(vcpu, oneshot.timeout_abs_ns, false);
		*r = 0;
		return true;

	case VCPUOP_stop_singleshot_timer:
		if (vcpu->arch.xen.vcpu_id != vcpu_id) {
			*r = -EINVAL;
			return true;
		}
		kvm_xen_stop_timer(vcpu);
		*r = 0;
		return true;
	}

	return false;

especially once the "vcpu->arch.xen.vcpu_id == XEN_VCPU_ID_INVALID" check comes
along, because while this technically works, it's even more confusing because
vcpu->arch.xen.vcpu_id isn't necessarily check for the other ops, e.g. if @vcpu_id
targets a different vCPU.

	struct vcpu_set_singleshot_timer oneshot;
	struct x86_exception e;

	if (!kvm_xen_timer_enabled(vcpu))
		return false;

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

	switch (cmd) {
	case VCPUOP_set_singleshot_timer:
		if (vcpu->arch.xen.vcpu_id != vcpu_id) {
			*r = -EINVAL;
			return true;
		}

		/*
		 * The only difference for 32-bit compat is the 4 bytes of
		 * padding after the interesting part of the structure. So
		 * for a faithful emulation of Xen we have to *try* to copy
		 * the padding and return -EFAULT if we can't. Otherwise we
		 * might as well just have copied the 12-byte 32-bit struct.
		 */
		BUILD_BUG_ON(offsetof(struct compat_vcpu_set_singleshot_timer, timeout_abs_ns) !=
			     offsetof(struct vcpu_set_singleshot_timer, timeout_abs_ns));
		BUILD_BUG_ON(sizeof_field(struct compat_vcpu_set_singleshot_timer, timeout_abs_ns) !=
			     sizeof_field(struct vcpu_set_singleshot_timer, timeout_abs_ns));
		BUILD_BUG_ON(offsetof(struct compat_vcpu_set_singleshot_timer, flags) !=
			     offsetof(struct vcpu_set_singleshot_timer, flags));
		BUILD_BUG_ON(sizeof_field(struct compat_vcpu_set_singleshot_timer, flags) !=
			     sizeof_field(struct vcpu_set_singleshot_timer, flags));

		if (kvm_read_guest_virt(vcpu, param, &oneshot, longmode ? sizeof(oneshot) :
					sizeof(struct compat_vcpu_set_singleshot_timer), &e)) {
			*r = -EFAULT;
			return true;
		}

		kvm_xen_start_timer(vcpu, oneshot.timeout_abs_ns, false);
		*r = 0;
		return true;

	case VCPUOP_stop_singleshot_timer:
		if (vcpu->arch.xen.vcpu_id != vcpu_id) {
			*r = -EINVAL;
			return true;
		}
		kvm_xen_stop_timer(vcpu);
		*r = 0;
		return true;
	}

	return false;