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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> +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.

> @@ -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.

> +        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.

> --- 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".

> @@ -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().)

For both functions: "inline" generally wants limiting to header files.

> +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.

> +    /*
> +     * 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?

> +    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.)

Jan