[PATCH RFC DO-NOT-APPLY] x86/IRQ: don't pass offline CPU(s) to on_selected_cpus()

[Jan Beulich <jbeulich@xxxxxxxx>]

on_selected_cpus() asserts that it's only passed online CPUs. Between
__cpu_disable() removing a CPU from the online map and fixup_eoi()
cleaning up for the CPU being offlined there's a window, though, where
the CPU in question can still appear in an action's cpu_eoi_map. Remove
offline CPUs, as they're (supposed to be) taken care of by fixup_eoi().

Move the entire tail of desc_guest_eoi() into a new helper, thus
streamlining processinf also for other call sites when the sole
remaining CPU is the local one. Move and split the assertion, to be a
functionally equivalent replacement for the earlier BUG_ON() in
__pirq_guest_unbind(). Note that we can't use scratch_cpumask there, in
particular in the context of a timer handler and with data held there
needing to stay intact across process_pending_softirqs().

Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
While this builds okay, for now I didn't even consider testing it: Both
aspects below (plus the ugly locking arrangement) speak against dealing
with the issue this way, imo. Cc-ing REST rather than just x86 for this
reason.

RFC: Don't we need {get,put}_cpu_maps() around quite a few more uses of
     cpu_online_map (e.g. _clear_irq_vector() when called from
     destroy_irq(), or about every caller of smp_call_function())? Roger
     suggests using stop-machine context for CPU {on,off}lining, but I
     seem to vaguely recall that there was a reason not to do so at
     least for the offlining part.

RFC: I doubt process_pending_softirqs() is okay to use from a timer
     handler. (Callers of, in particular, {desc,pirq}_guest_eoi() would
     also need checking for process_pending_softirqs() to be okay to use
     in their contexts.)

--- unstable.orig/xen/arch/x86/irq.c    2021-04-08 11:10:47.000000000 +0200
+++ unstable/xen/arch/x86/irq.c 2025-02-11 09:54:38.532567287 +0100
@@ -6,6 +6,7 @@
  */
 
 #include <xen/init.h>
+#include <xen/cpu.h>
 #include <xen/delay.h>
 #include <xen/errno.h>
 #include <xen/event.h>
@@ -1183,7 +1184,7 @@ static inline void clear_pirq_eoi(struct
     }
 }
 
-static void cf_check set_eoi_ready(void *data);
+static bool invoke_set_eoi_ready(struct irq_desc *desc, bool wait);
 
 static void cf_check irq_guest_eoi_timer_fn(void *data)
 {
@@ -1224,18 +1225,13 @@ static void cf_check irq_guest_eoi_timer
 
     switch ( action->ack_type )
     {
-        cpumask_t *cpu_eoi_map;
-
     case ACKTYPE_UNMASK:
         if ( desc->handler->end )
             desc->handler->end(desc, 0);
         break;
 
     case ACKTYPE_EOI:
-        cpu_eoi_map = this_cpu(scratch_cpumask);
-        cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
-        spin_unlock_irq(&desc->lock);
-        on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
+        invoke_set_eoi_ready(desc, false);
         return;
     }
 
@@ -1468,6 +1464,49 @@ static void cf_check set_eoi_ready(void
     flush_ready_eoi();
 }
 
+/*
+ * Locking is somewhat special here: In all cases the function is invoked with
+ * desc's lock held.  When @wait is true, the function tries to avoid 
unlocking.
+ * It always returns whether the lock is still held.
+ */
+static bool invoke_set_eoi_ready(struct irq_desc *desc, bool wait)
+{
+    const irq_guest_action_t *action = guest_action(desc);
+    cpumask_t cpu_eoi_map;
+
+    cpumask_copy(&cpu_eoi_map, action->cpu_eoi_map);
+
+    if ( __cpumask_test_and_clear_cpu(smp_processor_id(), &cpu_eoi_map) )
+    {
+        ASSERT(action->ack_type == ACKTYPE_EOI);
+        __set_eoi_ready(desc);
+        spin_unlock(&desc->lock);
+        flush_ready_eoi();
+        local_irq_enable();
+    }
+    else if ( wait && cpumask_empty(&cpu_eoi_map) )
+        return true;
+    else
+    {
+        ASSERT(action->ack_type == ACKTYPE_EOI);
+        spin_unlock_irq(&desc->lock);
+    }
+
+    if ( cpumask_empty(&cpu_eoi_map) )
+        return false;
+
+    while ( !get_cpu_maps() )
+        process_pending_softirqs();
+
+    cpumask_and(&cpu_eoi_map, &cpu_eoi_map, &cpu_online_map);
+    if ( !cpumask_empty(&cpu_eoi_map) )
+        on_selected_cpus(&cpu_eoi_map, set_eoi_ready, desc, wait);
+
+    put_cpu_maps();
+
+    return false;
+}
+
 void pirq_guest_eoi(struct pirq *pirq)
 {
     struct irq_desc *desc;
@@ -1481,7 +1520,6 @@ void pirq_guest_eoi(struct pirq *pirq)
 void desc_guest_eoi(struct irq_desc *desc, struct pirq *pirq)
 {
     irq_guest_action_t *action = guest_action(desc);
-    cpumask_t           cpu_eoi_map;
 
     if ( unlikely(!action) ||
          unlikely(!test_and_clear_bool(pirq->masked)) ||
@@ -1502,24 +1540,7 @@ void desc_guest_eoi(struct irq_desc *des
         return;
     }
 
-    ASSERT(action->ack_type == ACKTYPE_EOI);
-        
-    cpumask_copy(&cpu_eoi_map, action->cpu_eoi_map);
-
-    if ( __cpumask_test_and_clear_cpu(smp_processor_id(), &cpu_eoi_map) )
-    {
-        __set_eoi_ready(desc);
-        spin_unlock(&desc->lock);
-        flush_ready_eoi();
-        local_irq_enable();
-    }
-    else
-    {
-        spin_unlock_irq(&desc->lock);
-    }
-
-    if ( !cpumask_empty(&cpu_eoi_map) )
-        on_selected_cpus(&cpu_eoi_map, set_eoi_ready, desc, 0);
+    invoke_set_eoi_ready(desc, false);
 }
 
 int pirq_guest_unmask(struct domain *d)
@@ -1734,7 +1755,6 @@ static irq_guest_action_t *__pirq_guest_
     struct domain *d, struct pirq *pirq, struct irq_desc *desc)
 {
     irq_guest_action_t *action = guest_action(desc);
-    cpumask_t           cpu_eoi_map;
     int                 i;
 
     if ( unlikely(action == NULL) )
@@ -1765,12 +1785,7 @@ static irq_guest_action_t *__pirq_guest_
         if ( test_and_clear_bool(pirq->masked) &&
              (--action->in_flight == 0) &&
              (action->nr_guests != 0) )
-        {
-            cpumask_copy(&cpu_eoi_map, action->cpu_eoi_map);
-            spin_unlock_irq(&desc->lock);
-            on_selected_cpus(&cpu_eoi_map, set_eoi_ready, desc, 0);
-            spin_lock_irq(&desc->lock);
-        }
+            invoke_set_eoi_ready(desc, false);
         break;
     }
 
@@ -1796,14 +1811,8 @@ static irq_guest_action_t *__pirq_guest_
      * would need to flush all ready EOIs before returning as otherwise the
      * desc->handler could change and we would call the wrong 'end' hook.
      */
-    cpumask_copy(&cpu_eoi_map, action->cpu_eoi_map);
-    if ( !cpumask_empty(&cpu_eoi_map) )
-    {
-        BUG_ON(action->ack_type != ACKTYPE_EOI);
-        spin_unlock_irq(&desc->lock);
-        on_selected_cpus(&cpu_eoi_map, set_eoi_ready, desc, 1);
+    if ( !invoke_set_eoi_ready(desc, true) )
         spin_lock_irq(&desc->lock);
-    }
 
     BUG_ON(!cpumask_empty(action->cpu_eoi_map));