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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan