Re: [PATCH v2 03/20] KVM: x86/xen: Remove unnecessary irqsave from GPC lock usage in xen.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling read_unlock() on an rwlock_t inside kvm_sched_out() can cause deadlocks on PREEMPT_RT because waking a waiting writer requires acquiring runqueue locks while the current runqueue lock is already held.
--

commit 625a2fc07daa67a8fcafaa64daaf9791f5012ecd
Author: David Woodhouse <dwmw@amazon.co.uk>
Subject: KVM: x86/xen: Remove unnecessary irqsave from GPC lock usage in xen.c

This commit converts read_lock_irqsave() and read_unlock_irqrestore() 
calls to plain read_lock() and read_unlock() for gpc->lock in several 
xen.c functions, as the hardirq path now safely uses read_trylock().

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c

[ ... ]

> @@ -421,16 +419,14 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
>  	 * gfn_to_pfn caches that cover the region.
>  	 */
>  	if (atomic) {
> -		local_irq_save(flags);
>  		if (!read_trylock(&gpc1->lock)) {
> -			local_irq_restore(flags);
>  			return;
>  		}
>  	} else {
> -		read_lock_irqsave(&gpc1->lock, flags);
> +		read_lock(&gpc1->lock);
>  	}
>  	while (!kvm_gpc_check(gpc1, user_len1)) {
> -		read_unlock_irqrestore(&gpc1->lock, flags);
> +		read_unlock(&gpc1->lock);

[Severity: High]
This is a pre-existing issue, but can calling read_unlock() here lead to a
deadlock on PREEMPT_RT?

kvm_xen_update_runstate_guest() is called with atomic=true from
kvm_sched_out(), which is invoked via preempt notifiers while holding the
runqueue lock for the current CPU.

On CONFIG_PREEMPT_RT, rwlock_t is mapped to an rt_rwlock. If a writer is
waiting (for example, from a concurrent kvm_gpc_deactivate()),
read_unlock() calls __rwbase_read_unlock() to wake the blocked writer.

__rwbase_read_unlock() observes the waiting writer and calls
rt_mutex_wake_up_q(), which synchronously calls try_to_wake_up().

try_to_wake_up() will then attempt to acquire the pi_lock and the target
task's rq_lock. Since the current CPU's runqueue lock is already held by
the scheduler, acquiring another rq_lock without double_rq_lock() violates
lock ordering and can cause an ABBA deadlock.

Is there a plan to handle this rt_rwlock wake-up behavior from the preempt
notifier context?

>  
>  		/* When invoked from kvm_sched_out() we cannot sleep */
>  		if (atomic)
> @@ -439,7 +435,7 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)

[ ... ]

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