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

[David Woodhouse]

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.

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.

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

Reviewed-by: David Woodhouse <dwmw@xxxxxxxxxxxx>

> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/kvm/xen.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 27c1aeeab8af..db10f12d10cf 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -1614,13 +1614,13 @@ static bool kvm_xen_hcall_vcpu_op(struct kvm_vcpu *vcpu, bool longmode, int cmd,
>  	if (vcpu->arch.xen.vcpu_id == XEN_VCPU_ID_INVALID)
>  		return false;
>  

	/*
	 * Only the set/stop singleshot timer calls are accelerated
	 * by KVM, and they both operate only on the same vCPU.
	 */
> +	if (vcpu->arch.xen.vcpu_id != vcpu_id) {
> +		*r = -EINVAL;
> +		return true;
> +	}
> +
>  	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
> @@ -1648,10 +1648,6 @@ static bool kvm_xen_hcall_vcpu_op(struct kvm_vcpu *vcpu, bool longmode, int cmd,
>  		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;

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