Re: [PATCH v2 19/26] xen/riscv: implement IRQ routing for device passthrough

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/3/26 6:01 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/device.c
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <xen/device_tree.h>
> > +#include <xen/errno.h>
> > +#include <xen/iocap.h>
> > +#include <xen/rangeset.h>
> > +#include <xen/sched.h>
> > +
> > +#include <asm/intc.h>
> > +
> > +int map_irq_to_domain(struct domain *d, unsigned int irq,
> > +                      bool need_mapping, const char *devname)
> > +{
> > +    int res;
> > +
> > +    res = irq_permit_access(d, irq);
> > +    if ( res )
> > +    {
> > +        printk(XENLOG_ERR "Unable to permit to %pd access to IRQ %u\n", d, 
> > irq);
> Nit: Drop the first "to"?
Sure, I will drop that.

> > +        return res;
> > +    }
> > +
> > +    if ( need_mapping )
> > +    {
> > +        /*
> > +         * Checking the return of vintc_reserve_virq is not
> > +         * necessary. It should not fail except when we try to map
> > +         * the IRQ twice. This can legitimately happen if the IRQ is shared.
> > +         */
> > +        vintc_reserve_virq(d, irq);
> > +
> > +        res = route_irq_to_guest(d, irq, irq, devname);
> > +        if ( res < 0 )
> > +        {
> > +            printk(XENLOG_ERR "Unable to map IRQ%u to %pd\n", irq, d);
> > +            return res;
> > +        }
> > +    }
> > +
> > +    dt_dprintk("  - IRQ: %u\n", irq);
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * map_device_irqs_to_domain retrieves the interrupts configuration from
> > + * a device tree node and maps those interrupts to the target domain.
> > + *
> > + * Returns:
> > + *   < 0 error
> > + *   0   success
> > + */
> > +int map_device_irqs_to_domain(struct domain *d,
> > +                              struct dt_device_node *dev,
> > +                              bool need_mapping,
> > +                              struct rangeset *irq_ranges)
> > +{
> > +    unsigned int i, nirq;
> > +    int res, irq;
> > +    struct dt_raw_irq rirq;
> Move the latter three variables to the loop's scope and ...
>
> > +    nirq = dt_number_of_irq(dev);
> ... make this the variable's initializer?
It makes sense. I will do that.

> > +    /* Give permission and map IRQs */
> > +    for ( i = 0; i < nirq; i++ )
> > +    {
> > +        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 IRQ that have no physical meaning
> > +         * ie: IRQ 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,
> > +                       dt_node_full_name(rirq.controller));
> > +            continue;
> > +        }
> > +
> > +        irq = platform_get_irq(dev, i);
> > +        if ( irq < 0 )
> > +        {
> > +            printk("Unable to get irq %u for %s\n", i, dt_node_full_name(dev));
> > +            return irq;
> > +        }
> > +
> > +        res = map_irq_to_domain(d, irq, need_mapping, dt_node_name(dev));
> > +        if ( res )
> > +            return res;
> > +
> > +
> > +        /*
> > +         * At the moment there is only one user of map_device_irqs_to_domain()
> > +         * for RISC-V which calls it irq_ranges == NULL.
> > +         */
> > +        if ( irq_ranges )
> > +            return -EOPNOTSUPP;
> Why is this checked last, and inside the loop (when it's loop invariant)?
Just to show the place where irq_ranges will be handled. But currently I 
agree it would be better just move outside the loop.
> > --- a/xen/arch/riscv/include/asm/intc.h
> > +++ b/xen/arch/riscv/include/asm/intc.h
> > @@ -13,8 +13,11 @@ enum intc_version {
> >   };
> >   
> >   struct cpu_user_regs;
> > +struct domain;
> I can spot why this is needed, but ...
>
> > +struct dt_device_node;
> >   struct irq_desc;
> >   struct kernel_info;
> > +struct rangeset;
> >   struct vcpu;
> ... I'm at a loss to explain the need for these two additions.
Rudements from previous version of this patch series. I will drop them.
The same below ...

> > --- a/xen/arch/riscv/include/asm/setup.h
> > +++ b/xen/arch/riscv/include/asm/setup.h
> > @@ -5,6 +5,10 @@
> >   
> >   #include <xen/types.h>
> >   
> > +struct domain;
> > +struct dt_device_node;
> > +struct rangeset;
> Same here - why would they be needed when you make no other changes
> to this header?
... here.

> > --- a/xen/arch/riscv/intc.c
> > +++ b/xen/arch/riscv/intc.c
> > @@ -7,7 +7,9 @@
> >   #include <xen/init.h>
> >   #include <xen/irq.h>
> >   #include <xen/lib.h>
> > +#include <xen/sched.h>
> >   #include <xen/spinlock.h>
> > +#include <xen/xvmalloc.h>
> >   
> >   #include <asm/aia.h>
> >   #include <asm/intc.h>
> > @@ -86,6 +88,22 @@ unsigned int intc_irq_nums(void)
> >       return intc_hw_ops->irq_nums();
> >   }
> >   
> > +int intc_route_irq_to_guest(struct irq_desc *desc,
> > +                            unsigned int priority)
> > +{
> > +    ASSERT(spin_is_locked(&desc->lock));
> > +
> > +    ASSERT(intc_hw_ops->guest_irq_type);
> > +
> > +    desc->handler = intc_hw_ops->guest_irq_type;
> > +    set_bit(_IRQ_GUEST, &desc->status);
> Is desc->status accessed anywhere without holding desc->lock? If not,
> __set_bit() or simply |= ?
In release_irq() it could be used without lock:
...
    /* Wait to make sure it's not being used on another CPU */
    do { smp_mb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );

> > @@ -112,6 +130,14 @@ int domain_vintc_init(struct domain *d)
> >           break;
> >       }
> >   
> > +    if ( !ret )
> > +    {
> > +        d->arch.vintc->allocated_irqs =
> > +            xvzalloc_array(unsigned long, 
> > BITS_TO_LONGS(d->arch.vintc->irq_nums));
> > +        if ( !d->arch.vintc->allocated_irqs )
> > +            ret = -ENOMEM;
> > +    }
> > +
> >       return ret;
> >   }
> >   
> > @@ -129,4 +155,14 @@ void domain_vintc_deinit(struct domain *d)
> >           printk("vintc (ver:%d) isn't implemented\n", ver);
> >           break;
> >       }
> > +
> > +    xvfree(d->arch.vintc->allocated_irqs);
> > +}
> XVFREE()
>
> > +bool vintc_reserve_virq(const struct domain *d, unsigned int virq)
> > +{
> > +    if ( virq >= d->arch.vintc->irq_nums )
> > +        return false;
> > +
> > +    return !test_and_set_bit(virq, d->arch.vintc->allocated_irqs);
> >   }
> As to function / field naming: You don't look to be allocating IRQs. So
> is there a reason the field name gives the impression of allocation?
> Simply s/allocated/used/ or some such?
I am okay to rename to 'used' instead of 'allocated'.

> > @@ -221,3 +239,160 @@ 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;
> > +}
> > +
> > +void release_irq(unsigned int irq, const void *dev_id)
> > +{
> > +    struct irq_desc *desc;
> > +    unsigned long flags;
> > +    struct irqaction *action, **action_ptr;
> > +
> > +    desc = irq_to_desc(irq);
> > +
> > +    spin_lock_irqsave(&desc->lock,flags);
> > +
> > +    action_ptr = &desc->action;
> > +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
> > +    for ( ;; )
> > +    {
> > +        action = *action_ptr;
> > +        if ( !action )
> > +        {
> > +            printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq);
> > +            spin_unlock_irqrestore(&desc->lock, flags);
> > +            return;
> > +        }
> > +
> > +        if ( action->dev_id == dev_id )
> > +            break;
> > +
> > +        action_ptr = &action->next;
> > +    }
> > +
> > +    /* Found it - remove it from the action list */
> > +    *action_ptr = action->next;
> > +#else
> > +    action = *action_ptr;
It is needed to add *action_ptr = NULL here to deal with ...

> > +#endif
> > +
> > +    /* If this was the last action, shut down the IRQ */
> > +    if ( !desc->action )
> > +    {
> > +        desc->handler->shutdown(desc);
> > +        clear_bit(_IRQ_GUEST, &desc->status);
> > +    }
> > +
> > +    spin_unlock_irqrestore(&desc->lock,flags);
> > +
> > +    /* Wait to make sure it's not being used on another CPU */
> > +    do { smp_mb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );
> > +
> > +    if ( action->free_on_release )
> > +        xvfree(action);
> When !IRQ_HAS_MULTIPLE_ACTION desc->action becomes a dangling pointer here.
... it could be xvfree here as action is local variable.

> > +/* Route an IRQ to a specific guest */
> > +int route_irq_to_guest(struct domain *d, unsigned int virq,
> > +                       unsigned int irq, const char *devname)
> > +{
> > +    struct irqaction *action;
> > +    struct irq_guest *info;
> > +    struct irq_desc *desc;
> > +    unsigned long flags;
> > +    int retval = 0;
> > +
> > +    desc = irq_to_desc(irq);
> > +
> > +    action = xvmalloc(struct irqaction);
> > +    if ( !action )
> > +        return -ENOMEM;
> This is freed by release_irq(), but ...
>
> > +    info = xvmalloc(struct irq_guest);
> > +    if ( !info )
> ... where is the (non-error-path) freeing of this?
Agree it should be freed.

I am thing about just to update release_irq():

if ( action->free_on_release )
{
    xvfree(action->dev_id);
    xvfree(action);
}

But I think it is conceptually is incorrect as owner of ->dev_id in this 
case is guest so it would be better if guest will do that. So I think it 
would be better to intoduce now release_guest_irq():
int release_guest_irq(struct domain *d, unsigned int virq)
{
    struct irq_desc *desc;
    struct irq_guest *info;
    unsigned long flags;

    desc = irq_to_desc(virq);

    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;

    spin_unlock_irqrestore(&desc->lock, flags);

    release_irq(desc->irq, info);
    xvfree(info);

    return 0;

 unlock_err:
    spin_unlock_irqrestore(&desc->lock, flags);
    return -EINVAL;
}

and then call it in domain_vintc_deinit() for all virqs of a domain.

> > +    {
> > +        xvfree(action);
> > +        return -ENOMEM;
> > +    }
> > +
> > +    info->d = d;
> > +    info->virq = virq;
> > +
> > +    action->dev_id = info;
> > +    action->name = devname;
> > +    action->free_on_release = 1;
> true
>
> > +    spin_lock_irqsave(&desc->lock, flags);
> > +
> > +    /*
> > +     * If the IRQ is already used by someone
> > +     *  - If it's the same domain -> Xen doesn't need to update the IRQ desc.
> > +     *  For safety check if we are not trying to assign the IRQ to a
> > +     *  different vIRQ.
> > +     *  - Otherwise -> For now, don't allow the IRQ to be shared between
> > +     *  Xen and domains.
> > +     */
> > +    if ( desc->action != NULL )
> > +    {
> > +        if ( test_bit(_IRQ_GUEST, &desc->status) )
> > +        {
> > +            struct domain *ad = irq_get_domain(desc);
> > +
> > +            if ( d != ad )
> > +            {
> > +                printk(XENLOG_G_ERR "IRQ %u is already used by domain %u\n",
> > +                       irq, ad->domain_id);
> > +                retval = -EBUSY;
> > +            }
> > +            else if ( irq_get_guest_info(desc)->virq != virq )
> > +            {
> > +                printk(XENLOG_G_ERR
> > +                       "d%u: IRQ %u is already assigned to vIRQ %u\n",
> > +                       d->domain_id, irq, irq_get_guest_info(desc)->virq);
> Please can you get used to using %pd?
I'll do my best. Thanks for consistently pointing that out. I appreciate it.


> > +                retval = -EBUSY;
> > +            }
> > +        }
> > +        else
> > +        {
> > +            printk(XENLOG_G_ERR "IRQ %u is already used by Xen\n", irq);
> > +            retval = -EBUSY;
> > +        }
> > +        goto out;
> > +    }
> > +
> > +    retval = _setup_irq(desc, 0, action);
> > +    if ( retval )
> > +        goto out;
> > +
> > +    retval = intc_route_irq_to_guest(desc, IRQ_NO_PRIORITY);
> > +
> > +    spin_unlock_irqrestore(&desc->lock, flags);
> > +
> > +    if ( retval )
> > +    {
> > +        release_irq(desc->irq, info);
> Is de-referencing desc legitimate / race free with desc->lock not held?
desc itself cannot be freed, it's a pointer into the statically 
allocated array irq_desc[NR_IRQS] (riscv/irq.c:29). The descriptor 
object lives for the lifetime of the system.
desc->irq is write-once, it's set during init_irq_data() at boot 
(riscv/irq.c:172) and never modified again. It's effectively an 
immutable field after initialization, so reading it without the lock 
held is safe.
Thanks.

~ Oleksii