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

[Bart Van Assche <bvanassche@acm.org>]

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@google.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: kvm@vger.kernel.org
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   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.