Re: [PATCH v2 7/8] KVM: x86/xen: avoid blocking in hardirq context in kvm_xen_set_evtchn_fast()

From: Thomas Gleixner <tglx@linutronix.de>
To: David Woodhouse <dwmw2@infradead.org>,
	Steven Rostedt <rostedt@goodmis.org>
Cc: kvm@vger.kernel.org, Sean Christopherson <seanjc@google.com>,
	Paul Durrant <paul@xen.org>, Paolo Bonzini <pbonzini@redhat.com>,
	Michal Luczaj <mhal@rbox.co>, Paul Durrant <pdurrant@amazon.com>,
	David Woodhouse <dwmw@amazon.co.uk>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org
Subject: Re: [PATCH v2 7/8] KVM: x86/xen: avoid blocking in hardirq context in kvm_xen_set_evtchn_fast()
Date: Tue, 27 Feb 2024 18:18:58 +0100	[thread overview]
Message-ID: <871q8xakql.ffs@tglx> (raw)
In-Reply-To: <8C04AFD0-FC16-4572-AD23-FC7EEF663F11@infradead.org>

On Tue, Feb 27 2024 at 17:00, David Woodhouse wrote:
> On 27 February 2024 14:43:26 GMT, Steven Rostedt <rostedt@goodmis.org> wrote:
>>On Tue, 27 Feb 2024 11:49:21 +0000
>>David Woodhouse <dwmw2@infradead.org> wrote:
>>
>>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>>> index c16b6d394d55..d8b5326ecebc 100644
>>> --- a/arch/x86/kvm/xen.c
>>> +++ b/arch/x86/kvm/xen.c
>>> @@ -1736,9 +1736,23 @@ static int set_shinfo_evtchn_pending(struct kvm_vcpu *vcpu, u32 port)
>>>  	unsigned long flags;
>>>  	int rc = -EWOULDBLOCK;
>>>  
>>> -	read_lock_irqsave(&gpc->lock, flags);
>>> +	local_irq_save(flags);
>>> +	if (!read_trylock(&gpc->lock)) {
>>
>>Note, directly disabling interrupts in PREEMPT_RT is just as bad as doing
>>so in non RT and the taking a mutex.
>
> Well, it *isn't* a mutex in non-RT. It's basically a spinlock. And
> even though RT turns it into a mutex this is only a trylock with a
> fall back to a slow path in process context, so it ends up being a
> very short period of irqdisable/lock – just to validate the target
> address of the cache, and write there. (Or fall back to that slow
> path, if the cache needs revalidating).
>
> So I think this is probably OK, but...
>
>>Worse yet, in PREEMPT_RT read_unlock_irqrestore() isn't going to re-enable
>>interrupts.
>
> ... that's kind of odd. I think there's code elsewhere which assumes
> it will, and pairs it with an explicit local_irq_save() like the
> above, in a different atomic context (updating runstate time info when
> being descheduled).

While it is legit, it simply cannot work for RT. We fixed the few places
which had it open coded (mostly for no good reason).

> So *that* needs changing for RT?

No. It's not required anywhere and we really don't change that just to
accomodate the xen shim.

I'll look at the problem at hand tomorrow.

Thanks,

        tglx