Re: [PATCH v6 18/23] xen/riscv: implement IRQ routing for device passthrough

On 7/30/26 9:18 AM, Jan Beulich wrote:
> On 29.07.2026 17:23, Oleksii Kurochko wrote:
>>
>>
>> On 7/29/26 5:02 PM, Oleksii Kurochko wrote:
>>>
>>>
>>> On 7/29/26 4:15 PM, Jan Beulich wrote:
>>>> On 29.07.2026 13:59, Oleksii Kurochko wrote:
>>>>> On 7/23/26 3:30 PM, Jan Beulich wrote:
>>>>>> On 20.07.2026 17:59, Oleksii Kurochko wrote:
>>>>>>> +/* Route an IRQ to a specific guest */
>>>>>>> +int route_irq_to_guest(struct domain *d, unsigned int virq,
>>>>>>> +                       unsigned int irq, const char *devname)
>>>>>>> +{
>>>>>>> +    struct irqaction *action;
>>>>>>> +    struct irq_guest *info;
>>>>>>> +    struct irq_desc *desc;
>>>>>>> +    unsigned long flags;
>>>>>>> +    int retval = 0;
>>>>>>> +
>>>>>>> +    if ( d->is_dying )
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    desc = irq_to_desc(irq);
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * release_irq() frees this action via xvfree(), relying on
>>>>>>> action
>>>>>>> +     * being the first member of struct irq_guest so that &info-
>>>>>>>> action
>>>>>>> +     * coincides with info itself. Guard the layout so a future field
>>>>>>> +     * reorder can't silently turn that into a free() of a mid-
>>>>>>> allocation
>>>>>>> +     * pointer.
>>>>>>> +     */
>>>>>>> +    BUILD_BUG_ON(offsetof(struct irq_guest, action) != 0);
>>>>>>
>>>>>> Can't release_irq() simply use container_of()? One way or another it
>>>>>> feels
>>>>>> like you're painting yourself into a particular corner ...
>>>>>
>>>>> If it isn't the best option then it is needed to follow they way we had
>>>>> before:
>>>>
>>>> I don't understand why you think you need to go back.
>>>
>>> Because, based on your reply—specifically, "One way or another it feels
>>> like you're painting yourself into a particular corner..." — it seems
>>> that even if I replaced BUILD_BUG_ON() with container_of() in
>>> release_irq(), you would still consider it a bad solution. Did I
>>> misunderstand your point?
>>
>> One more thing: I'm not really sure it's safe to do the following in
>> release_irq():
>>
>> if ( action->free_on_release )
>>       xvfree(container_of(action, struct irq_guest, action));
>>
>> release_irq() is a generic API, but this kind of allocation is only
>> needed for guest IRQs. Wouldn't it be better to set:
>>
>> action->free_on_release = false;
>>
>> for guest IRQs, and then free the memory in release_guest_irq() after
>> the call to release_irq()?
> 
> Perhaps.
> 
>> Wouldn't that be a better approach than calling
>> `xvfree(container_of(...))` from within the generic release_irq()?
> 
> Perhaps.
> 
> What I'd really like to see you do is come up with an approach that is
> both self-consistent and future-proof. With the latter aspect meaning that
> it should be (reasonably) easy to identify places that need changing if
> e.g. the "->free_on_release is only ever one value" property goes away.
> 

An approach with action->free_on_release = false; together with 
vfree(info) in release_guest_irq() seems to be the best option. Since 
action is no longer allocated dynamically, it should not be freed 
through free_on_release, so setting it to false makes that explicit. 
Additionally, guest IRQs have extra state (struct irq_guest), and it is 
the responsibility of release_guest_irq() to clean it up.

This also keeps the semantics of ->free_on_release consistent for Xen 
IRQs. If it is set to true, release_irq() is responsible for freeing 
struct irq_action; if it is false, release_irq() should not free it.

For guest interrupts, release_irq() should not free anything because 
struct irq_action is embedded in struct irq_guest, which is freed by 
release_guest_irq(). If, in the future, guest interrupts need 
->free_on_release = true, the only required change would be to allocate 
struct irq_action dynamically and set ->free_on_release = true. All 
other is already covered. That makes the design, in my opinion, 
self-consistent and future-proof.

~ Oleksii