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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> +    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?

> --- 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? Does it perhaps belong into an earlier patch?

> --- 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?

> @@ -62,6 +66,8 @@ struct vintc_ops {
>  };
>  
>  struct vintc {
> +    unsigned int irq_nums;

I did ask before: Which word does "nums" stand for?

> @@ -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.

> +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).

> +    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?

> +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?

Jan