Re: [PATCH v4 21/25] xen/riscv: implement IRQ routing for device passthrough

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 7/2/26 4:32 PM, Jan Beulich wrote:
> On 02.07.2026 11:33, Oleksii Kurochko wrote:
>>
>>
>> On 7/2/26 8:38 AM, Jan Beulich wrote:
>>
>>>>>> +    spin_unlock_irqrestore(&desc->lock, flags);
>>>>>> +
>>>>>> +    release_irq(desc->irq, info);
>>>>>> +    xvfree(info);
>>>>>
>>>>> If, in release_irq(), action isn't freed, it's ->dev_id field will now have
>>>>> a dangling pointer. (I think I did point this out before.)
>>>>
>>>> It should freed in release_irq() as route_irq_to_guest() always set
>>>> action->free_on_release = true;
>>>
>>> Well, "free_on_release" must exits for a purpose. I.e. there must be, now
>>> or soon, cases where it's set to false. Else simply drop the field.
>>
>> I can't simply remove this field because it is part of the common
>> `struct irqaction` and is used by other arc-s. (I assume that it is not
>> what you fully meant...)
>>
>> IIUC, this field is used to determine whether an irqaction is
>> heap-allocated (and therefore should be freed) or statically allocated
>> (and therefore should not be freed).
> 
> Yes. However, all uses of the field are in arch-specific code. So in
> principle it could be #ifdef-ed out for RISC-V. There may be a better
> option, though:
> 
>> At the moment, all IRQ actions are heap-allocated (on RISC-V), so
>> free_on_release should always be set to true. In particular, the code
>> snippet you asked about releases a guest interrupt, and guest interrupt
>> actions are always heap-allocated. As a result, when release_guest_irq()
>> calls release_irq(), the associated irqaction will be freed.
>>
>> So, from what I can see, the current behavior is correct and I think it
>> should be left as it is. Do you have any concerns about this?
> 
> If you only ever have the field set to true, use ASSERT() in place of
> if().
> 

It looks like that if we want an `ASSERT()`, it should be in 
`release_guest_irq()`. However, that would require duplicating part of 
`release_irq()` to locate the corresponding `irqaction` and verify 
`free_on_release`. Since guest IRQs are created through 
`route_irq_to_guest()`, which always sets `action->free_on_release = 
true`, we are already safe.

I don't see much benefit in replacing the `if` statement with an 
`ASSERT()` in `release_irq()`. The current implementation is generic and 
would also work for Xen-owned, statically allocated interrupts (even 
though there are none today). If we replace the `if` with an `ASSERT()` 
now, anyone introducing a Xen-owned, statically allocated interrupt in 
the future would simply have to revert the change and restore the `if` 
statement, bringing us back to the current implementation.

Since the existing code doesn't break any logic and already handles both 
the current use case and potential future extensions (statically 
allocated irqs), it seems reasonable to keep it as it is.

~ Oleksii