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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>>>> +        dt_dprintk("  - IRQ: %u\n", irq);
>>>>> +
>>>>> +        if ( irq_ranges )
>>>>> +        {
>>>>> +            res = rangeset_add_singleton(irq_ranges, irq);
>>>>> +            if ( res )
>>>>> +                return res;
>>>>> +        }
>>>>
>>>> What is irq_ranges?
>>>
>>> IIUC based on Arm code irq_ranges is an optional output accumulator, the
>>> caller allocates and passes it in when it needs to track which IRQs were
>>> mapped (overlay use case), or passes NULL when that tracking is not needed.
>>>
>>> I added here as map_device_irqs_to_domain() is called from the common
>>> code and so maybe one day someone will decide to pass irq_ranges to this
>>> functions. At the moment, for RISC-V it is the only one user of
>>> map_device_irqs_to_domain() and it passes NULL.
>>
>> Simply assert then that it's NULL?
> 
> Won't BUG_ON() be better here as it BUG_ON() macros is always defined 
> and doesn't matter if release or debug build are used.

Depends on the context, really.

> Or maybe you meant:
>   if ( irq_ranges )
>       assert_failed("irq_ranges arg isn't supported\n");

Definitely not. assert_failed() shouldn't be called directly, as I had
told you on at least one earlier occasion.

Jan