On Tue, 2024-02-27 at 17:22 +0000, David Woodhouse wrote:
>
> > > > 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.
>
> It's open-coded in the runstate update in the Xen shim, as I said.
> That's what needs changing, which is separate from the problem at
> hand in this patch.
>
> > I'll look at the problem at hand tomorrow.
>
> Ta.
How about this? It's fugly as hell, it puts PREEMPT_RT knowledge into
the code to make the local trylock function *conditionally* disable
interrupts. I hate it, but let's start with is it even *correct*?
This is addressing the existing runstate code I mentioned above. If it
works then it could theoretically be used in the patch in $SUBJECT too,
instead of the open-coded irqsave and trylock.
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index efca40cc8c4f..78e23f151065 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -270,6 +270,26 @@ static void kvm_xen_init_timer(struct kvm_vcpu *vcpu)
vcpu->arch.xen.timer.function = xen_timer_callback;
}
+/*
+ * With CONFIG_PREEMPT_RT, 'irqsave' locking doesn't really disable IRQs as
+ * "all IRQs are threaded" (except the hrtimer IRQs). So, open-coding a
+ * local_irq_save() before a read_trylock() is wrong for PREEMPT_RT.
+ */
+static inline bool read_trylock_irqsave(rwlock_t *lck, unsigned long *flags)
+{
+#ifndef CONFIG_PREEMPT_RT
+ local_irq_save(*flags);
+#endif
+ if (!read_trylock(lck)) {
+#ifndef CONFIG_PREEMPT_RT
+ local_irq_restore(*flags);
+#endif
+ return false;
+ }
+
+ return true;
+}
+
static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
{
struct kvm_vcpu_xen *vx = &v->arch.xen;
@@ -373,11 +393,8 @@ 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);
+ if (!read_trylock_irqsave(&gpc1->lock, &flags))
return;
- }
} else {
read_lock_irqsave(&gpc1->lock, flags);
}
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
