Re: [PATCH v8 16/20] xen/riscv: implement IRQ routing for device passthrough

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2026 17:19, 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.

Irrespective of there not being any .end() hook yet, I think the two would
better be split properly right away. aplic_guest_irq_type's .name could
then also properly point to e.g. "aplic-guest".

> --- a/xen/arch/riscv/irq.c
> +++ b/xen/arch/riscv/irq.c
> @@ -12,11 +12,26 @@
>  #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;
> +    /*
> +     * The action of a guest IRQ has the same lifetime as this structure, so
> +     * embed it here to have both covered by a single allocation. 
> Consequently
> +     * it must not be freed by release_irq() (see free_on_release below).
> +     */
> +    struct irqaction action;

Why the mention of release_irq(), when release_guest_irq() doesn't use that
function? (In fact release_irq() looks to be unused altogether.)

> @@ -227,3 +250,235 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int 
> irq)
>      spin_unlock(&desc->lock);
>      irq_exit();
>  }
> +
> +static struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(test_bit(_IRQ_GUEST, &desc->status));

Nit: I don't quite see why this cannot be the simpler

    ASSERT(desc->status & IRQ_GUEST);

> +static struct irqaction *irq_detach_action(struct irq_desc *desc,
> +                                           const void *dev_id)
> +{
> +    struct irqaction *action, **action_ptr = &desc->action;
> +
> +    ASSERT(spin_is_locked(&desc->lock));
> +
> +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
> +    for ( ;; )
> +    {
> +        action = *action_ptr;
> +        if ( !action || (action->dev_id == dev_id) )
> +            break;
> +
> +        action_ptr = &action->next;
> +    }
> +#else
> +    action = *action_ptr;
> +#endif
> +
> +    if ( !action )
> +    {
> +        printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n",
> +               desc->irq);
> +        return NULL;
> +    }
> +
> +    /* Found it - remove it from the action list */
> +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
> +    *action_ptr = action->next;
> +#else
> +    *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);

Similarly

        desc->status &= ~IRQ_GUEST;

here then.

> +/*
> + * Complete the release of an action detached by irq_detach_action().
> + *
> + * To be called with desc->lock dropped: the lock cannot be held all the way
> + * through, as waiting for a handler still running on another CPU to complete
> + * requires do_IRQ() to be able to acquire the very same lock.
> + *
> + * Once this function has returned, the action (and hence any object 
> embedding
> + * it) is no longer referenced by anyone and may be freed by the caller.
> + */
> +static void irq_release_action(const struct irq_desc *desc,
> +                               struct irqaction *action)
> +{
> +    /*
> +     * Wait to make sure it's not being used on another CPU.
> +     *
> +     * The read barrier pairs with the spin_unlock() in do_IRQ(): once we
> +     * observe _IRQ_INPROGRESS cleared, we are guaranteed to also see the
> +     * writes do_IRQ() made to desc (e.g. desc->action) before releasing the
> +     * lock, so it is safe to free the action below.
> +     */

I fear I don't understand this: What writes to desc->action would do_IRQ()
ever want to do? I could see if you gave desc->status as example here;
really I don't think any other field (apart from perhaps statistics) would
ever want modifying there.

> +    do { smp_rmb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );

Please split this across three lines, to conform to style. (Also same nit
as above.)

> +    if ( action->free_on_release )
> +        xvfree(action);

How does this being done here fit with the last paragraph of the comment
ahead of the function?

> +int release_guest_irq(struct domain *d, unsigned int virq)
> +{
> +    struct irq_desc *desc = irq_to_desc(virq);
> +    struct irqaction *action;
> +    struct irq_guest *info;
> +    unsigned long flags;
> +    int ret = -EINVAL;
> +
> +    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 )

This looks to be the only use of "d" - any reason the function parameter cannot
be pointer-to-const?

> +        goto unlock_err;
> +
> +    /*
> +     * Detaching the action happens with desc->lock still held, so that a
> +     * concurrent release_guest_irq() for the same IRQ sees _IRQ_GUEST 
> already

I think "sees" is misleading here, as it suggests that a racing check can occur.
With the lock held, that's impossible. Hence imo better "will see" (i.e. only
after having got hold of the lock).

> +/* 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 irq_guest *info;
> +    struct irq_desc *desc = irq_to_desc(irq);
> +    unsigned long flags;
> +    int retval = 0;
> +
> +    if ( d->is_dying )
> +        return -EINVAL;
> +
> +    info = xvzalloc(struct irq_guest);

With zeroing used here, ...

> +    if ( !info )
> +        return -ENOMEM;
> +
> +    info->d = d;
> +    info->virq = virq;
> +
> +    info->action.dev_id = info;
> +    info->action.name = devname;
> +    /* The action is part of 'info', thus it is freed together with it. */
> +    info->action.free_on_release = false;

... this is dead code.

> +    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_guest_info(desc)->d;
> +
> +            if ( d != ad )
> +            {
> +                printk(XENLOG_G_ERR "IRQ %u is already used by %pd\n",
> +                       irq, ad);

Perhaps best to also have %pd: at the start of this message, just like ...

> +                retval = -EBUSY;
> +            }
> +            else if ( irq_get_guest_info(desc)->virq != virq )
> +            {
> +                printk(XENLOG_G_ERR
> +                       "%pd: IRQ %u is already assigned to vIRQ %u\n",
> +                       d, irq, irq_get_guest_info(desc)->virq);

... you have it here?

Jan