Re: [PATCH v2 15/20] KVM: x86/xen: Convert kvm_xen_set_evtchn_fast() to gpc's CLASS() APIs

[Sean Christopherson <seanjc@google.com>]

On Fri, May 29, 2026, sashiko-bot@kernel.org wrote:
> > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > index 8f822acb11a46..47750316f1322 100644
> > --- a/arch/x86/kvm/xen.c
> > +++ b/arch/x86/kvm/xen.c
> [ ... ]
> > @@ -1810,8 +1809,6 @@ static void __kvm_xen_set_evtchn_fast(struct kvm_vcpu *vcpu, int port_word_bit)
> >  		kick_vcpu = false;
> >  	}
> >  
> > -out_unlock:
> > -	read_unlock(&gpc->lock);
> >  out_kick:
> >  	if (kick_vcpu) {
> >  		kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
> 
> [Severity: High]
> Does this unintentionally hold the gpc lock across the IPI kick?

No, it intentionally holds the gpc lock across _sending_ the IPI kick.

> By replacing the explicit unlock with a function-scoped CLASS() variable, the
> vcpu_info_map destructor won't run until the end of the function. This means
> the lock is now held while calling kvm_vcpu_kick(), whereas the original
> code explicitly dropped it first.
> 
> [ ... ]
> > @@ -1888,9 +1881,6 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
> >  		rc = 1; /* Delivered to the bitmap in shared_info. */
> >  	}
> >  
> > -out_unlock:
> > -	read_unlock(&gpc->lock);
> > -
> >  	if (rc == 1)
> >  		__kvm_xen_set_evtchn_fast(vcpu, port_word_bit);
> >  	return rc;
> 
> [Severity: High]
> Does this now hold the VM-wide shinfo_cache lock while calling
> __kvm_xen_set_evtchn_fast() and kicking the vCPU?
> 
> Since shinfo_map is a function-scoped CLASS() variable, its destructor
> won't release the lock until after __kvm_xen_set_evtchn_fast() returns.
> This creates a nested locking dependency and holds locks over expensive
> cross-vCPU operations, potentially serializing event channel deliveries
> across the entire VM on the fast path.

__kvm_vcpu_kick() is neither expensive nor cross-vCPU.  In the wait=false case,
which is the behavior of kvm_vcpu_kick(), it sends IPIs via smp_send_reschedule(),
i.e. it's more or less just __apic_send_IPI(cpu, RESCHEDULE_VECTOR), which is a
single WRMSR on modern harware.

> Would it make sense to constrain the mapping to an explicit scope block
> to match the original release points and maintain the "No functional change
> intended" guarantee?

I'll just drop that claim.