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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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