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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.06.2026 17:21, Oleksii Kurochko wrote:
> On 6/22/26 5:57 PM, Jan Beulich wrote:
>> On 17.06.2026 13:17, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/intc.h
>>> +++ b/xen/arch/riscv/include/asm/intc.h
>>> @@ -13,6 +13,7 @@ enum intc_version {
>>>   };
>>>   
>>>   struct cpu_user_regs;
>>> +struct domain;
>>>   struct irq_desc;
>>>   struct kernel_info;
>>>   struct vcpu;
>>> @@ -32,6 +33,9 @@ struct intc_hw_operations {
>>>       /* hw_irq_controller to enable/disable/eoi host irq */
>>>       const struct hw_interrupt_type *host_irq_type;
>>>   
>>> +    /* hw_irq_controller to enable/disable/eoi guest irq */
>>> +    const struct hw_interrupt_type *guest_irq_type;
>>
>> It's likely my limited RISC-V knowledge that I find this extremely odd:
>> Separate struct hw_interrupt_type-s for host and guest?
> 
> The guest and host interrupt controllers may handle some 
> hw_irq_controller operations differently, even though the operations 
> themselves are conceptually the same. The hw_irq_controller interface 
> provides fairly abstract interrupt controller operations, but the 
> underlying implementation may differ depending on whether the controller 
> is used by the host or a guest.
> 
> As an example, the Arm code already follows this approach:
> 
> /* XXX different for level vs edge */
> static hw_irq_controller gicv2_host_irq_type = {
>      .typename     = "gic-v2",
>      .startup      = gicv2_irq_startup,
>      .shutdown     = gicv2_irq_shutdown,
>      .enable       = gicv2_irq_enable,
>      .disable      = gicv2_irq_disable,
>      .ack          = gicv2_irq_ack,
>      .end          = gicv2_host_irq_end,
>      .set_affinity = gicv2_irq_set_affinity,
> };
> 
> static hw_irq_controller gicv2_guest_irq_type = {
>      .typename     = "gic-v2",
>      .startup      = gicv2_irq_startup,
>      .shutdown     = gicv2_irq_shutdown,
>      .enable       = gicv2_irq_enable,
>      .disable      = gicv2_irq_disable,
>      .ack          = gicv2_irq_ack,
>      .end          = gicv2_guest_irq_end,
>      .set_affinity = gicv2_irq_set_affinity,
> };
> 
> These implementations reuse almost all interrupt controller operations, 
> differing only in the .end callback.

Which I'm having trouble with as well. Interrupts are handled by Xen. What
guests get to see are virtualized interrupts (no matter how much HW
acceleration may be in use). Hence I'm having difficulty to see such a
split justified.

>>> +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
>>> +    for ( ;; )
>>> +    {
>>> +        action = *action_ptr;
>>> +        if ( !action )
>>> +        {
>>> +            printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", 
>>> irq);
>>> +            spin_unlock_irqrestore(&desc->lock, flags);
>>> +            return;
>>> +        }
>>> +
>>> +        if ( action->dev_id == dev_id )
>>> +            break;
>>> +
>>> +        action_ptr = &action->next;
>>> +    }
>>> +
>>> +    /* Found it - remove it from the action list */
>>> +    *action_ptr = action->next;
>>> +#else
>>> +    action = *action_ptr;
>>> +    *action_ptr = NULL;
>>> +#endif
>>> +
>>> +    /* If this was the last action, shut down the IRQ */
>>> +    if ( !desc->action )
>>> +    {
>>> +        desc->handler->shutdown(desc);
>>> +        __clear_bit(_IRQ_GUEST, &desc->status);
>>> +    }
>>> +
>>> +    spin_unlock_irqrestore(&desc->lock,flags);
>>> +
>>> +    /* Wait to make sure it's not being used on another CPU */
>>> +    do { smp_mb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );
>>
>> Can you explain to me what the purpose of this barrier is?
> 
> if  do_IRQ() was called and:
>      desc->status |= IRQ_INPROGRESS;
> was called we have to wait while irq will be handled to avoid NULL 
> pointer derefenece caused by in do_IRQ():
>      action = desc->action;
> 
> So if release_irq() and do_irq() are called on different CPUs we want to 
> be sure that do_IRQ() make desc->status visiable for all CPUs.

For that you need smp_rmb(), not smp_mb(). And then it needs to be clear what
the write-side counterpart is (presumably the spin-unlock in do_IRQ()).

>>> +int release_guest_irq(struct domain *d, unsigned int virq)
>>> +{
>>> +    struct irq_desc *desc = irq_to_desc(virq);
>>> +    struct irq_guest *info;
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&desc->lock, flags);
>>> +
>>> +    if ( !test_bit(_IRQ_GUEST, &desc->status) )
>>> +        goto unlock_err;
>>> +
>>> +    info = irq_get_guest_info(desc);
>>> +    if ( d != info->d )
>>> +        goto unlock_err;
>>> +
>>> +    spin_unlock_irqrestore(&desc->lock, flags);
>>> +
>>> +    release_irq(desc->irq, info);
>>> +    xvfree(info);
>>
>> So you drop the lock keeping the info associated with desc in place. How
>> do you know what you free here is the correct thing, and isn't in use
>> elsewhere?
> 
> The object freed is captured under desc->lock (info = 
> irq_get_guest_info(desc)), so it is by construction the dev_id of the 
> action attached to this desc, it can't be a stale or wrong pointer.

Why would this be? Another request_irq() (or whatever it is) can race this,
can't it?

Jan