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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/20/26 5:21 PM, Jan Beulich wrote:
> On 20.04.2026 16:34, Oleksii Kurochko wrote:
> > On 4/20/26 3:45 PM, Jan Beulich wrote:
> > > On 20.04.2026 13:39, Oleksii Kurochko wrote:
> > > > On 4/16/26 2:51 PM, Jan Beulich wrote:
> > > > > On 14.04.2026 13:29, Oleksii Kurochko wrote:
> > > > > > On 4/2/26 2:22 PM, Jan Beulich wrote:
> > > > > > > On 10.03.2026 18:08, Oleksii Kurochko wrote:
> > > > > > > > +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?
> > > > > > What do you mean by truly virtual IRQs?
> > > > > Ones where no aspects are represented by any piece of hardware.
> > > > >
> > > > > > I can't totally agree that the current approach isn't use virtual IRQs,
> > > > > > yes, they are 1:1 mapped but on the other side Xen is responsible to
> > > > > > give an IRQ number for guest's device and Xen is responsible that guest
> > > > > > isn't trying to reach IRQ which not belongs to it.
> > > > > In a non-virtualized environment I expect IRQs are going to be "sparse"
> > > > > (i.e. with perhaps large blocks of items used elsewhere). If you had
> > > > > proper translation of IRQ numbers, the same could be true for your
> > > > > guests.
> > > > Partial FDT, which is used to tell which device be passthroughed to
> > > > guest, is using physical IRQ number (which I am just considering for
> > > > simplicity to be 1:1 mapped to virtual IRQ number). So if we have the
> > > > following configuration:
> > > >      Physical (bare-metal) IRQ layout is sparse:
> > > >        IRQ 5  → UART -> domU0
> > > >        IRQ 23 → Ethernet -> domU1
> > > >        IRQ 47 → PCIe -> domU0
> > > >        IRQ 100 → Storage -> domU1
> > > > (gaps everywhere, driven by hardware wiring)
> > > >
> > > > For such configuration we will have for each domain auth_irq_bmp[] which
> > > > contains:
> > > >     IRQ 5 and IRQ47 for domU0
> > > > and
> > > >     IRQ 23 and IRQ 100 for domU1
> > > >
> > > > And here vIRQ5 = pIRQ5, vIRQ47 = pIRQ47 and so on. auth_irq_bmp just
> > > > transform xIRQ number to bit position which it will have in real APLIC
> > > > register. Just as an example, lets take vIRQ5 and vIRQ47.
> > > >
> > > > As reading or writing register setie[k] reads or potentially modifies
> > > > the enable bits for interrupt sources k × 32 through k × 32 + 31. For an
> > > > implemented interrupt source i within that range, the enable bit for
> > > > source i corresponds with register bit (i mod 32).
> > > > So for:
> > > >     - vIRQ5 == pIRQ5 we have to set bit 5 in setie[0]
> > > >     - vIRQ47 == pIRQ47 we have to set bit 15 in setie[1]
> > > >
> > > > Probably it was not the best idea to declare auth_irq_bmp as it will
> > > > look in h/w and maybe just 'bool auth_irq_bmp[1024]' would be more clearer.
> > > >
> > > > So irqs number are still stay "sparsed" in guest.
> > > Well, twice (or more) as sparse in the example you give, compared to the
> > > host.
> > Just to be sure that I fully understand your concern here.
> >
> > The difference between xIRQ5 and xIRQ47 is 42 bits (if for 1 irq we are
> > using 1 bit) which leads to that we have somewhere allocated 48 bit
> > bitmap where only two bits will be set, all others will be zero.
> Why 48-bit bitmap? As you inherit the property from the host, it'll be e.g.
> 1024 bits. Compared to the host, each guest will have yet fewer bits set in
> there.
48-bit bitmap specifically in this example because IRQ47 is the highest 
mentioned here, so if we won't compress zeros between IRQ0-4 and IRQ6-46 
it will be needed 48-bit to cover IRQs from IRQ0 to IRQ47. I thought 
about that as it seems like I misunderstood what you mean by "sparsed in 
guest" and the way how I have to deal with that.
> > Instead it would be better to have to do mapping: pIRQ5 -> vIRQ1, pIRQ47
> > ->vIRQ2, right?
> Which specific mapping I don't care very much. There may also be conventions
> to adhere to (on x86 for example there are).
If just bitmap is okay here ... then I am okay with using it.

> > If yes, won't we still store somewhere this mapping? it seems like
> > having 'unsigned int auth_irq_bmp[1024]' is a good option where index
> > will be vIRQ number and 'unsigned int' will be pIRQ number. But at the
> > moment I think that we could go with 1:1 IRQ number mapping and then
> > have 'bool auth_irq_bmp[1024]' will be more then enough and will safe
> > some memory.
> Well, if using 1:1 mapping was clearly identified as "for the time being",
> then that might be acceptable (for the time being).
>
> As to you (again) suggesting "bool auth_irq_bmp[1024]" - why would you use
> an array of bool-s when a bitmap can do the same in 1/8th of the space?
... just bitmap will better.

Thanks.

~ Oleksii