Re: [PATCH v1 16/27] xen/riscv: implement IRQ mapping for device passthrough

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2026 18:08, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/setup.h
> +++ b/xen/arch/riscv/include/asm/setup.h
> @@ -5,6 +5,10 @@
>  
>  #include <xen/types.h>
>  
> +struct domain;
> +struct dt_device_node;
> +struct rangeset;
> +
>  #define max_init_domid (0)
>  
>  void setup_mm(void);
> @@ -13,6 +17,19 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned 
> long len);
>  
>  void init_csr_masks(void);
>  
> +/* TODO: move somewhere to common header? */

Counter question: Why ...

> +/*
> + * 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);

... is this not an inline function, when ...

> --- a/xen/arch/riscv/intc.c
> +++ b/xen/arch/riscv/intc.c
> @@ -79,3 +79,11 @@ int __init intc_make_domu_dt_node(const struct kernel_info 
> *kinfo)
>  
>      return -ENOSYS;
>  }
> +
> +int map_device_irqs_to_domain(struct domain *d, struct dt_device_node *dev,
> +                              bool need_mapping,
> +                              struct rangeset *irq_ranges)
> +{
> +    return d->arch.vintc->ops->map_device_irqs_to_domain(d, dev, 
> need_mapping,
> +                                                         irq_ranges);
> +}

... it's merely a wrapper around an indirect function call? And then the
function isn't used anywhere anyway.

> --- a/xen/arch/riscv/vaplic.c
> +++ b/xen/arch/riscv/vaplic.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <xen/errno.h>
> +#include <xen/iocap.h>
>  #include <xen/sched.h>
>  #include <xen/xvmalloc.h>
>  
> @@ -19,6 +20,113 @@
>  
>  #include "aplic-priv.h"
>  
> +struct vaplic_priv {
> +    /* Contains a legal interrupts for a domain */
> +    uint32_t auth_irq_bmp[APLIC_NUM_REGS];
> +};

With it apparently plural that is correct, the first "a" wants dropping
from the comment.

> +static bool is_irq_shared_among_domains(const struct domain *d,
> +                                        const unsigned int irq_num)
> +{
> +    struct domain *tmp;

const

> +    unsigned int reg_num = irq_num / APLIC_NUM_REGS;
> +    unsigned int bit_pos = irq_num % APLIC_NUM_REGS;
> +
> +    for_each_domain ( tmp )

See other uses of the construct for how this needs synchronizing. But looping
over all domains looks pretty inefficient anyway for ...

> +    {
> +        uint32_t *auth_irq_bmp;
> +
> +        if ( tmp == d )
> +            continue;
> +
> +        auth_irq_bmp = tmp->arch.vintc->private;
> +
> +        if ( auth_irq_bmp[reg_num] & BIT(bit_pos, U) )
> +        {
> +            printk("%s: irq%d is shared between %pd and %pd\n", __func__,
> +                   irq_num, tmp, d);
> +
> +            return true;
> +        }

... the intended purpose. If IRQs can't be shared, can't you maintain global
state of which ones are in use _somewhere_?

> +    }
> +
> +    return false;
> +}

This together with ...

> +int vaplic_map_device_irqs_to_domain(struct domain *d,
> +                                     struct dt_device_node *dev,
> +                                     bool need_mapping,
> +                                     struct rangeset *irq_ranges)
> +{
> +    unsigned int i, nirq;
> +    int res, irq;
> +    struct dt_raw_irq rirq;
> +    uint32_t *auth_irq_bmp = d->arch.vintc->private;
> +    unsigned int reg_num;
> +
> +    nirq = dt_number_of_irq(dev);
> +
> +    /* Give permission and map IRQs */
> +    for ( i = 0; i < nirq; i++ )
> +    {
> +        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.
> +         */
> +        if ( rirq.controller != dt_interrupt_controller )
> +        {
> +            dt_dprintk("irq %u not connected to primary controller."
> +                       "Connected to %s\n", i,
> +                       dt_node_full_name(rirq.controller));
> +            continue;
> +        }
> +
> +        irq = platform_get_irq(dev, i);
> +        if ( irq < 0 )
> +        {
> +            printk("Unable to get irq %u for %s\n", i, 
> dt_node_full_name(dev));
> +            return irq;
> +        }
> +
> +        res = irq_permit_access(d, irq);
> +        if ( res )
> +        {
> +            printk(XENLOG_ERR "Unable to permit to %pd access to IRQ %u\n", 
> d,
> +                   irq);

This time the other way around: %d please with plain int. (Again at least
once further down.)

> +            return res;
> +        }
> +
> +        reg_num = irq / APLIC_NUM_REGS;
> +
> +        if ( is_irq_shared_among_domains(d, irq) )
> +        {
> +            printk("%s: Shared IRQ isn't supported\n", __func__);
> +            return -EINVAL;
> +        }
> +
> +        auth_irq_bmp[reg_num] |= BIT(irq % APLIC_NUM_REGS, U);

... all of this leaves me with the impression that IRQ numbering isn't really
virtualized. IRQs are merely split into groups, one group per domain (and
maybe some unused). How are you going to fit in truly virtual IRQs?

> +        dt_dprintk("  - IRQ: %u\n", irq);
> +
> +        if ( irq_ranges )
> +        {
> +            res = rangeset_add_singleton(irq_ranges, irq);
> +            if ( res )
> +                return res;
> +        }

What is irq_ranges?

> @@ -34,6 +142,7 @@ static int __init cf_check vcpu_vaplic_init(struct vcpu *v)
>  
>  static const struct vintc_ops vaplic_ops = {
>      .vcpu_init = vcpu_vaplic_init,
> +    .map_device_irqs_to_domain = vaplic_map_device_irqs_to_domain,
>  };

What about the inverse function, needed for domain cleanup?

Jan