Re: [PATCH v4 21/25] xen/riscv: implement IRQ routing for device passthrough

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/29/26 5:55 PM, Jan Beulich wrote:
> On 26.06.2026 17:46, Oleksii Kurochko wrote:
> > dom0less device passthrough requires granting guest domains access to
> > device interrupts.  Introduce map_device_irqs_to_domain() to enumerate
> > a DT node's interrupt properties, skipping those not owned by
> > the primary interrupt controller (as at the moment I haven't seen usages
> > of it), and map_irq_to_domain() to grant domain access and configure
> > Xen's interrupt descriptor accordingly. Sharing IRQ between domains is
> > rejected.
> >
> > Both map_irq_to_domain() and map_device_irqs_to_domain() are marked
> > __overlay_init, mirroring Arm: without CONFIG_OVERLAY_DTB this expands to
> > __init, so the functions are init-only and need no XSM check; with
> > CONFIG_OVERLAY_DTB they become runtime-callable, but the only runtime
> > entry point is dt_overlay_domctl(), which performs the XSM checks at the
> > domctl layer.  RISC-V does not wire up DT overlay yet, so today these are
> > strictly __init; if/when overlay support is added, the domctl-level XSM
> > gating must be added together with it, as on Arm.
> >
> > route_irq_to_guest() and release_irq() manage irq_desc ownership for
> > guest-assigned interrupts.  Each assignment carries a small irq_guest
> > structure as irqaction::dev_id, recording the owning domain and virtual
> > IRQ number which is 1:1 mapped to physical IRQ number.  A per-domain
> > vIRQ allocation bitmap (used_irqs in struct vintc), managed by
> > vintc_reserve_virq(), prevents the same vIRQ being claimed twice.
> >
> > Host and guest interrupts may differ in some operations (EOI timing in
> > particular, possibly others): a host IRQ is completed once Xen's handler
> > runs, whereas a passthrough IRQ must defer the physical completion until
> > the guest issues its own EOI, otherwise a still-asserted level line would
> > immediately retrigger and storm.  This affects only the .end callback;
> > the rest of hw_interrupt_type is shared, hence the separate host and
> > guest hw_interrupt_type instances.
> >
> > With APLIC+IMSIC, guest interrupts are delivered directly by hardware
> > through the IMSIC, bypassing do_IRQ(). The _IRQ_GUEST branch in
> > do_IRQ() is therefore left as BUG() until a platform without direct
> > IMSIC delivery is encountered.
> And this is secure, i.e. one guest (by mishandling things, e.g. simply
> never claiming / servicing an interrupt) cannot affect another guest?
It will be just affect this specific interrupt which isn't claimed/serviced.

> > +int __overlay_init map_device_irqs_to_domain(struct domain *d,
> > +                                             struct dt_device_node *dev,
> > +                                             bool need_mapping,
> > +                                             struct rangeset *irq_ranges)
> > +{
> > +    unsigned int i, nirq = dt_number_of_irq(dev);
> > +
> > +    if ( irq_ranges )
> > +        return -EOPNOTSUPP;
> > +
> > +    /* Give permission and map IRQs */
> > +    for ( i = 0; i < nirq; i++ )
> > +    {
> > +        int res, irq;
> > +        struct dt_raw_irq rirq;
> > +
> > +        res = dt_device_get_raw_irq(dev, i, &rirq);
> > +        if ( res )
> > +        {
> > +            printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> > +                   i, dt_node_full_name(dev));
> > +            return res;
> > +        }
> > +
> > +        /*
> > +         * Don't map IRQs that have no physical meaning
> > +         * ie: IRQs whose controller is not APLIC/IMSIC/PLIC.
> > +         */
> > +        if ( rirq.controller != dt_interrupt_controller )
> > +        {
> > +            dt_dprintk("irq %u not connected to primary controller."
> > +                       "Connected to %s\n", i,
> Nit: By splitting a format string like this, you pretty effectively hide
> that there's a blank missing after the full stop.
>
> Further after an already wrapped function argument there shouldn't follow
> another one, to maintain visual clarity.
I would be okay to have a format string a little bit long. I would 
change that line to (if you are okay with that):
  ...("irq %u not connected to primary controller. Connected to %s\n",
      i, dt_node_full_name(rirq.controller));

> > @@ -101,12 +119,28 @@ int domain_vintc_init(struct domain *d)
> >           break;
> >       }
> >   
> > +    if ( !ret )
> > +    {
> > +        d->arch.vintc->used_irqs =
> > +            xvzalloc_array(unsigned long, 
> > BITS_TO_LONGS(d->arch.vintc->nr_virqs));
> Nit: Overlong line.
I will reformat that in the following way:
        d->arch.vintc->used_irqs =
            xvzalloc_array(unsigned long,
                           BITS_TO_LONGS(d->arch.vintc->nr_virqs));

> > +        if ( !d->arch.vintc->used_irqs )
> > +            ret = -ENOMEM;
> > +    }
> > +
> >       return ret;
> >   }
> >   
> >   void domain_vintc_deinit(struct domain *d)
> >   {
> >       const enum intc_variant variant = intc_hw_ops->info->hw_variant;
> > +    unsigned int virq;
> > +
> > +    if ( !d->arch.vintc )
> > +        return;
> Seeing this and ...
>
> > +    for ( virq = 0; virq < d->arch.vintc->nr_virqs; virq++ )
> > +        if ( test_bit(virq, d->arch.vintc->used_irqs) )
> > +            release_guest_irq(d, virq);
> >   
> >       switch ( variant )
> >       {
> > @@ -117,4 +151,14 @@ void domain_vintc_deinit(struct domain *d)
> >       default:
> >           break;
> >       }
> > +
> > +    XVFREE(d->arch.vintc->used_irqs);
> ... this, ...
>
> > +}
> ... where is d->arch.vintc being freed? That would logically look to
> belong into this function.
Right, it is an issue: d->arch.vintc is being NULLed inside 
domain_vaplic_deinit(), which are called from domain_vintc_deinit(), so 
XVFREE(d->arch.vintc->used_irqs) must be moved to just after the for() 
loop (before the switch).
As for d->arch.vintc itself, it should only be NULLed and not freed, 
since it is a pointer to &vaplic->vintc which is embedded in the vaplic 
struct and not separately allocated.
> > --- a/xen/arch/riscv/irq.c
> > +++ b/xen/arch/riscv/irq.c
> > @@ -12,11 +12,20 @@
> >   #include <xen/errno.h>
> >   #include <xen/init.h>
> >   #include <xen/irq.h>
> > +#include <xen/sched.h>
> >   #include <xen/spinlock.h>
> > +#include <xen/xvmalloc.h>
> >   
> >   #include <asm/hardirq.h>
> >   #include <asm/intc.h>
> >   
> > +/* Describe an IRQ assigned to a guest */
> > +struct irq_guest
> > +{
> > +    struct domain *d;
> > +    unsigned int virq;
> > +};
> > +
> >   static irq_desc_t irq_desc[NR_IRQS];
> >   
> >   static bool irq_validate_new_type(unsigned int curr, unsigned int new)
> > @@ -192,6 +201,15 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq)
> >       if ( desc->handler->ack )
> >           desc->handler->ack(desc);
> >   
> > +    if ( desc->status & IRQ_GUEST )
> > +        /*
> > +         * As at the moment APLIC + IMSIC is used for guest interrupts will
> > +         * be directly passed to guest. But if/when IMSIC won't be available
> > +         * all interrupts will go through Xenand here an irq injection
> > +         * will be necessary to do.
> > +         */
> > +        panic("unimplemented");
> The first comment sentence doesn't parse for me. In the 2nd there's a blank
> missing between "Xen" and "and".
I will rephrase it for clarity:

/*
 * With APLIC + IMSIC, guest interrupts bypass Xen and are delivered
 * directly to the guest. Without IMSIC, interrupts would be trapped
 * by Xen and would need injecting into the guest here.
 */

It looks more clearer to me.


> > @@ -221,3 +239,215 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq)
> >       spin_unlock(&desc->lock);
> >       irq_exit();
> >   }
> > +
> > +static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
> > +{
> > +    ASSERT(spin_is_locked(&desc->lock));
> > +    ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> > +    ASSERT(desc->action != NULL);
> > +
> > +    return desc->action->dev_id;
> > +}
> > +
> > +static inline struct domain *irq_get_domain(struct irq_desc *desc)
> > +{
> > +    return irq_get_guest_info(desc)->d;
> > +}
> Does this really need a separate helper? (You effectively open-code it
> anyway in release_guest_irq().)
Considering that I used irq_get_domain() once at all (even in 
downstream) I will drop that.
> For both functions: "inline" generally wants limiting to header files.
Regarding inline I think that I don't understand, it isn't in the header 
file as irq_get_guest_info() is used only in this file.
Do I understand you correctly and it is needed just to drop "inline" for
irq_get_guest_info()?

> > +int release_guest_irq(struct domain *d, unsigned int virq)
> > +{
> > +    struct irq_desc *desc = irq_to_desc(virq);
> > +    struct irq_guest *info;
> > +    unsigned long flags;
> > +
> > +    spin_lock_irqsave(&desc->lock, flags);
> > +
> > +    if ( !test_bit(_IRQ_GUEST, &desc->status) )
> > +        goto unlock_err;
> > +
> > +    info = irq_get_guest_info(desc);
> > +    if ( d != info->d )
> > +        goto unlock_err;
> > +
> > +    /*
> > +     * Live IRQ unrouting from a running domain is not supported: the tear-down
> > +     * drops desc->lock across release_irq()/xvfree() and relies on no
> > +     * concurrent route_irq_to_guest() being issued for this domain. Only 
> > permit
> > +     * it for a dying domain, where assignment is frozen and no new routes can
> > +     * appear.
> > +     */
> > +    if ( !d->is_dying )
> > +    {
> > +        spin_unlock_irqrestore(&desc->lock, flags);
> > +        return -EBUSY;
> > +    }
> Yet route_irq_to_guest() looks to happily act on dying guests. IOW assignment
> doesn't look to be frozen, despite the comment saying so.
Right, it is needed to add:
    if ( d->is_dying )
        return -EINVAL;
at the top of route_irq_to_guest().


> > +    /*
> > +     * Clear _IRQ_GUEST while still holding the lock so that a concurrent
> > +     * release_guest_irq() for the same IRQ observes it and bails out, rather
> > +     * than capturing the same 'info' and double-freeing it below.
> > +     */
> > +    clear_bit(_IRQ_GUEST, &desc->status);
> You use __set_bit() / __clear_bit() elsewhere - why not here?
As it is under spinlock it could __clear_bit here.

> > +    spin_unlock_irqrestore(&desc->lock, flags);
> > +
> > +    release_irq(desc->irq, info);
> > +    xvfree(info);
> If, in release_irq(), action isn't freed, it's ->dev_id field will now have
> a dangling pointer. (I think I did point this out before.)
It should freed in release_irq() as route_irq_to_guest() always set 
action->free_on_release = true;
Thanks.

~ Oleksii