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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.07.2026 18:04, Oleksii Kurochko wrote:
> 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.)

We'll be back to this if ...

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

... such a change was made. Imo you have two choices: Deal with the issue
properly right away, keeping the if(). Or assume "free_on_release" is only
ever true, and add an assertion to this effect (indicating that code needs
auditing if that assumption is broken).

Jan