On 4/15/25 5:55 PM, Jan Beulich wrote
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+ new->next = desc->action;
+ smp_mb();
+#endif
+
+ desc->action = new;
+ smp_mb();
Aren't smp_wmb() sufficient on both places? If not, I think comments
want adding.
smp_wmb() will be sufficient but I think the barrier could be dropped at all
as __setup_irq() is called only in setup_irq() and __setup_irq() call is wrapped
by spinlock_{un}lock_irqsave() where spinlock_unlock_*() will put barrier.
+ return 0;
+}
+
+void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
+{
+ if ( desc != NULL )
Can desc really be NULL here?
It can't as irq_desc[] isn't dynamically allocated.
Isn't desc->lock required to be held?
It is and it is called in setup_irq() which calls spin_lock_irqsave().
Anyway, I think it could be dropped at all and use
'desc->handler->set_affinity(desc, cpu_mask);'
explicitly in setup_irq().
+ spin_lock_irqsave(&desc->lock, flags);
+
+ disabled = (desc->action == NULL);
+
+ if ( test_bit(_IRQ_GUEST, &desc->status) )
+ {
+ spin_unlock_irqrestore(&desc->lock, flags);
+ /*
+ * TODO: would be nice to have functionality to print which domain owns
+ * an IRQ.
+ */
+ printk(XENLOG_ERR "ERROR: IRQ %u is already in use by a domain\n",
irq);
+ return -EBUSY;
+ }
+
+ rc = __setup_irq(desc, irqflags, new);
+ if ( rc )
+ goto err;
+
+ /* First time the IRQ is setup */
+ if ( disabled )
+ {
+ /* disable irq by default */
+ set_bit(_IRQ_DISABLED, &desc->status);
Shouldn't this be set when we make it here?
It should be. I'll drop the setting of _IRQ_DISABLED.
+ /* route interrupt to xen */
+ intc_route_irq_to_xen(desc, IRQ_NO_PRIORITY);
+
+ /*
+ * we don't care for now which CPU will receive the
+ * interrupt
+ *
+ * TODO: Handle case where IRQ is setup on different CPU than
+ * the targeted CPU and the priority.
+ */
+ irq_set_affinity(desc, cpumask_of(smp_processor_id()));
+ desc->handler->startup(desc);
+ /* enable irq */
+ clear_bit(_IRQ_DISABLED, &desc->status);
Now it turns out this is really done twice: Once in aplic_irq_enable(),
and once here.
Agree, this is a job of *_startup()->*_aplic_irq_enable(). I'll drop that too.
~ Oleksii
