Re: [PATCH v3 18/23] xen/riscv: implement IRQ routing for device passthrough

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/22/26 5:57 PM, Jan Beulich wrote:
> On 17.06.2026 13:17, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/device.c
> > @@ -0,0 +1,102 @@
> > +/* 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);
> Such generally needs an XSM check up front, the more that the function isn't
> __init, i.e. is (apparently) intended for runtime use as well.
I think it really should be __init as it is used only during domain 
construction (boot/build-time only). For xl-created domUs it should be 
used route_irq_to_guest() called from XEN_DOMCTL_bind_pt_irq hypercall.
I will add __init for map_irq_to_domain() and correspondingly for 
map_device_irqs_to_domain().

> > +    if ( res )
> > +    {
> > +        printk(XENLOG_ERR "Unable to permit %pd access to IRQ %u\n", d, irq);
> > +        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 = 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 IRQ that have no physical meaning
> > +         * ie: IRQ whose controller is not APLIC/IMSIC/PLIC.
> > +         */
> Nit: Does this comment mean to use singular or plural for IRQ?
Plural would be better here.

> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -538,10 +538,11 @@ int __init imsic_init(const struct dt_device_node *node)
> >   
> >   static int __init guest_imsic_make_reg_property(struct domain *d, void *fdt)
> >   {
> > +    paddr_t base = GUEST_IMSIC_S_BASE;
> >       paddr_t size = IMSIC_MMIO_PAGE_SZ * d->max_vcpus;
> >       __be32 regs[4] = {
> > -        cpu_to_be32(GUEST_IMSIC_S_BASE >> 32),
> > -        cpu_to_be32(GUEST_IMSIC_S_BASE),
> > +        cpu_to_be32(base >> 32),
> > +        cpu_to_be32(base),
> >           cpu_to_be32(size >> 32),
> >           cpu_to_be32(size),
> >       };
> What is this change about?
It doesn't really make sense. I think it I added it to fix type issue 
when GUEST_IMSIC_S_BASE was incorrectly wrapped. I will drop it.
> Does it perhaps belong into an earlier patch?
Yes, it should be part of prev. commit.

> > --- a/xen/arch/riscv/include/asm/intc.h
> > +++ b/xen/arch/riscv/include/asm/intc.h
> > @@ -13,6 +13,7 @@ enum intc_version {
> >   };
> >   
> >   struct cpu_user_regs;
> > +struct domain;
> >   struct irq_desc;
> >   struct kernel_info;
> >   struct vcpu;
> > @@ -32,6 +33,9 @@ struct intc_hw_operations {
> >       /* hw_irq_controller to enable/disable/eoi host irq */
> >       const struct hw_interrupt_type *host_irq_type;
> >   
> > +    /* hw_irq_controller to enable/disable/eoi guest irq */
> > +    const struct hw_interrupt_type *guest_irq_type;
> It's likely my limited RISC-V knowledge that I find this extremely odd:
> Separate struct hw_interrupt_type-s for host and guest?
The guest and host interrupt controllers may handle some 
hw_irq_controller operations differently, even though the operations 
themselves are conceptually the same. The hw_irq_controller interface 
provides fairly abstract interrupt controller operations, but the 
underlying implementation may differ depending on whether the controller 
is used by the host or a guest.
As an example, the Arm code already follows this approach:

/* XXX different for level vs edge */
static hw_irq_controller gicv2_host_irq_type = {
    .typename     = "gic-v2",
    .startup      = gicv2_irq_startup,
    .shutdown     = gicv2_irq_shutdown,
    .enable       = gicv2_irq_enable,
    .disable      = gicv2_irq_disable,
    .ack          = gicv2_irq_ack,
    .end          = gicv2_host_irq_end,
    .set_affinity = gicv2_irq_set_affinity,
};

static hw_irq_controller gicv2_guest_irq_type = {
    .typename     = "gic-v2",
    .startup      = gicv2_irq_startup,
    .shutdown     = gicv2_irq_shutdown,
    .enable       = gicv2_irq_enable,
    .disable      = gicv2_irq_disable,
    .ack          = gicv2_irq_ack,
    .end          = gicv2_guest_irq_end,
    .set_affinity = gicv2_irq_set_affinity,
};

These implementations reuse almost all interrupt controller operations, 
differing only in the .end callback.
In my case, I have a similar situation with the PLIC implementation, 
where the .ack and .end operations differ between the host and guest 
cases. However, I was planning to upstream the PLIC support a bit later.
> > @@ -62,6 +66,8 @@ struct vintc_ops {
> >   };
> >   
> >   struct vintc {
> > +    unsigned int irq_nums;
> I did ask before: Which word does "nums" stand for?
I will rename it to nr_irqs.

> > @@ -106,12 +124,25 @@ 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->irq_nums));
> > +        if ( !d->arch.vintc->used_irqs )
> > +            ret = -ENOMEM;
> > +    }
> > +
> >       return ret;
> >   }
> >   
> >   void domain_vintc_deinit(struct domain *d)
> >   {
> >       const enum intc_version ver = intc_hw_ops->info->hw_version;
> > +    unsigned int virq;
> > +
> > +    for ( virq = 0; virq < d->arch.vintc->irq_nums; virq++ )
> Here you de-reference d->arch.intc. One of the purposes of ...
>
> > +        if ( test_bit(virq, d->arch.vintc->used_irqs) )
> > +            release_guest_irq(d, virq);
> >   
> >       switch ( ver )
> >       {
> > @@ -122,4 +153,14 @@ void domain_vintc_deinit(struct domain *d)
> >       default:
> >           break;
> >       }
> > +
> > +    XVFREE(d->arch.vintc->used_irqs);
> ... this is to allow the function to be idempotent, i.e. to recognize that
> it was called before (or no setup was done at all), and hence it doesn't
> need to do anything.
I will add before for loop:
     if ( !d->arch.vintc )
        return;

> > +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);
> Nit: Missing blank after comma (again at least once further down).
Thanks. I will fix it.

> > +    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;
> > +    *action_ptr = NULL;
> > +#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) );
> Can you explain to me what the purpose of this barrier is?
if  do_IRQ() was called and:
    desc->status |= IRQ_INPROGRESS;
was called we have to wait while irq will be handled to avoid NULL 
pointer derefenece caused by in do_IRQ():
    action = desc->action;

So if release_irq() and do_irq() are called on different CPUs we want to 
be sure that do_IRQ() make desc->status visiable for all CPUs.
> > +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;
> > +
> > +    spin_unlock_irqrestore(&desc->lock, flags);
> > +
> > +    release_irq(desc->irq, info);
> > +    xvfree(info);
> So you drop the lock keeping the info associated with desc in place. How
> do you know what you free here is the correct thing, and isn't in use
> elsewhere?
The object freed is captured under desc->lock (info = 
irq_get_guest_info(desc)), so it is by construction the dev_id of the 
action attached to this desc, it can't be a stale or wrong pointer.
"In use elsewhere" splits into two cases:

An in-flight interrupt handler on another CPU. The guest handler 
dereferences info (info->d). This is why the free is ordered after 
release_irq(): release_irq() detaches the action and then spins on 
_IRQ_INPROGRESS before returning, so once it returns no CPU is in the 
handler, and only then do we xvfree(info).
A second release_guest_irq() for the same vIRQ. You're right that, as 
originally written, dropping the lock left _IRQ_GUEST set and the action 
attached, so a concurrent caller could re-derive and double-free the 
same info. In practice the sole caller is domain_vintc_deinit() 
(serialised per-domain teardown), but the function shouldn't rely on 
that. I've changed it to clear _IRQ_GUEST while still holding 
desc->lock; a concurrent caller now fails the _IRQ_GUEST check and bails 
out, so exclusive ownership of info is provable from the lock rather 
than from caller serialisation:
diff --git a/xen/arch/riscv/irq.c b/xen/arch/riscv/irq.c
index 2f9461a23b5f..69b57eaf95f7 100644
--- a/xen/arch/riscv/irq.c
+++ b/xen/arch/riscv/irq.c
@@ -311,6 +311,8 @@ int release_guest_irq(struct domain *d, unsigned int 
virq)
     if ( d != info->d )
         goto unlock_err;

+    __clear_bit(_IRQ_GUEST, &desc->status);
+
     spin_unlock_irqrestore(&desc->lock, flags);

     release_irq(desc->irq, info);


Thanks.

~ Oleksii