Re: [PATCH v3 04/10] KVM: x86/xen: Punt singleshot timer hcalls to userspace if Xen vCPU ID isn't set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM returns an un-restorable value for KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID when the Xen vCPU ID is not set, breaking live migration.
- [Medium] kvm_xen_hcall_set_timer_op() handles single-shot timer hypercalls in the kernel even when the Xen vCPU ID is invalid, contradicting the commit's intent.
--

commit 02813e860ee3ca993ca34adffcb813bb2bcccc96
Author: Sean Christopherson <seanjc@google.com>

KVM: x86/xen: Punt singleshot timer hcalls to userspace if Xen vCPU ID isn't set

Explicitly invalidate KVM's internal Xen vCPU ID during vCPU creation
instead of *trying* to set the Xen ID to the vCPU index by default, and
forward singleshot timer hypercalls to userspace if the VMM hasn't set the
Xen ID via KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID.  Using the vCPU's index as its
default Xen ID is reasonable in concept, but in practice is horribly flawed
as the index is left as '0' until after vCPU initialization completes, i.e.
every vCPU gets a Xen ID of '0' by default.

Link: https://lore.kernel.org/all/20260612233017.1F9771F000E9@smtp.kernel.org

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 694b31c1fcc9a..27c1aeeab8afb 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
[ ... ]
> @@ -1610,6 +1611,9 @@ static bool kvm_xen_hcall_vcpu_op(struct kvm_vcpu *vcpu, bool longmode, int cmd,
>  	if (!kvm_xen_timer_enabled(vcpu))
>  		return false;
>  
> +	if (vcpu->arch.xen.vcpu_id == XEN_VCPU_ID_INVALID)
> +		return false;
> +

[Severity: Medium]
Does this bypass the userspace punting rule for the __HYPERVISOR_set_timer_op
hypercall?

While this correctly punts for kvm_xen_hcall_vcpu_op(), the older hypercall is
still handled natively without the check:

arch/x86/kvm/xen.c:kvm_xen_hcall_set_timer_op() {
    if (!kvm_xen_timer_enabled(vcpu))
        return false;

    if (timeout)
        kvm_xen_start_timer(vcpu, timeout, true);
    else
        kvm_xen_stop_timer(vcpu);

    *r = 0;
    return true;
}

If a Xen guest issues __HYPERVISOR_set_timer_op on a vCPU where the VMM hasn't
configured the ID, it is still handled in the kernel.

Will this create an inconsistent ABI where one single-shot timer hypercall
variant is punted, while the other is handled natively?

>  	switch (cmd) {
>  	case VCPUOP_set_singleshot_timer:
>  		if (vcpu->arch.xen.vcpu_id != vcpu_id) {
[ ... ]
> @@ -2299,7 +2303,7 @@ static bool kvm_xen_hcall_evtchn_send(struct kvm_vcpu *vcpu, u64 param, u64 *r)
>  
>  void kvm_xen_init_vcpu(struct kvm_vcpu *vcpu)
>  {
> -	vcpu->arch.xen.vcpu_id = vcpu->vcpu_idx;
> +	vcpu->arch.xen.vcpu_id = XEN_VCPU_ID_INVALID;

[Severity: High]
Does this initialization break live migration when the ID isn't configured?

When userspace calls KVM_GET_XEN_VCPU_ATTR, it retrieves the uninitialized ID
as XEN_VCPU_ID_INVALID (which is U32_MAX).

On the destination host, attempting to restore this state fails because
kvm_xen_vcpu_set_attr() explicitly rejects values >= KVM_MAX_VCPUS:

arch/x86/kvm/xen.c:kvm_xen_vcpu_set_attr() {
    ...
    case KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID:
        if (data->u.vcpu_id >= KVM_MAX_VCPUS)
            r = -EINVAL;
        else {
    ...
}

Will this cause KVM to unconditionally reject the valid state returned by
GET_ATTR and return -EINVAL during live migration?

>  	vcpu->arch.xen.poll_evtchn = 0;
>  
>  	timer_setup(&vcpu->arch.xen.poll_timer, cancel_evtchn_poll, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625223623.3376478-1-seanjc@google.com?part=4