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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan