Re: [PATCH 01/62] kvm: Make pi_enable_wakeup_handler() easier to analyze

[Bart Van Assche]

On 2/24/26 10:20 AM, Sean Christopherson wrote:
> For the scope, please use:
>
>     KVM: VMX:
>
> On Mon, Feb 23, 2026, Bart Van Assche wrote:
> > The Clang thread-safety analyzer does not support comparing expressions
> > that use per_cpu(). Hence introduce a new local variable to capture the
> > address of a per-cpu spinlock. This patch prepares for enabling the
> > Clang thread-safety analyzer.
> >
> > Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Cc: kvm@xxxxxxxxxxxxxxx
> > Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> > ---
> >   arch/x86/kvm/vmx/posted_intr.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c
> > index 4a6d9a17da23..f8711b7b85a8 100644
> > --- a/arch/x86/kvm/vmx/posted_intr.c
> > +++ b/arch/x86/kvm/vmx/posted_intr.c
> > @@ -164,6 +164,7 @@ static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu)
> >   	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> >   	struct vcpu_vt *vt = to_vt(vcpu);
> >   	struct pi_desc old, new;
> > +	raw_spinlock_t *wakeup_lock;
> >   
> >   	lockdep_assert_irqs_disabled();
> >   
> > @@ -179,11 +180,11 @@ static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu)
> >   	 * entirety of the sched_out critical section, i.e. the wakeup handler
> >   	 * can't run while the scheduler locks are held.
> >   	 */
> > -	raw_spin_lock_nested(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu),
> > -			     PI_LOCK_SCHED_OUT);
> > +	wakeup_lock = &per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu);
>
> Addressing this piecemeal doesn't seem maintainable in the long term.  The odds
> of unintentionally regressing the coverage with a cleanup are rather high.  Or
> we'll end up with confused and/or grumpy developers because they're required to
> write code in a very specific way because of what are effectively shortcomings
> in the compiler.

I think it's worth mentioning that the number of patches similar to the
above is small. If I remember correctly, I only encountered two similar
cases in the entire kernel tree.

Regarding why the above patch is necessary, I don't think that it is
fair to blame the compiler in this case. The macros that implement
per_cpu() make it impossible for the compiler to conclude that the
pointers passed to the raw_spin_lock_nested() and raw_spin_unlock()
calls are identical:

/*
 * Add an offset to a pointer.  Use RELOC_HIDE() to prevent the compiler
 * from making incorrect assumptions about the pointer value.
 */
#define SHIFT_PERCPU_PTR(__p, __offset)				\
	RELOC_HIDE(PERCPU_PTR(__p), (__offset))

#define RELOC_HIDE(ptr, off)					\
({								\
	unsigned long __ptr;					\
	__asm__ ("" : "=r"(__ptr) : "0"(ptr));			\
	(typeof(ptr)) (__ptr + (off));				\
})

By the way, the above patch is not the only possible solution for
addressing the thread-safety warning Clang reports for this function. 
Another possibility is adding __no_context_analysis to the function
definition. Is the latter perhaps what you prefer?

Bart.