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 Thu, 2026-06-25 at 15:36 -0700, Sean Christopherson wrote:
> > Hoist the checks on the Xen 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;

	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;

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

> And let's move your patch 4 to come *after* this semantic change.

Agreed.