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

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

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