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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/25/26 8:08 AM, Jan Beulich wrote:
> 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.
I think that I don't fully understand what is wrong with splitting. If 
there are cases exist when I need such separation for virtual interrupt 
controller operations then it looks fine to introduce such separation, 
right?
Lets take an example of PLIC.

For each source the PLIC has a "gateway":
1. Claim (read CONTEXT_CLAIM): returns the pending IRQ id and closes the 
gateway for that source, it will not forward that source to any context 
again until completed.
2. Complete (write the id back to CONTEXT_CLAIM): reopens the gateway. 
If the device line is still asserted (level high), the PLIC immediately 
re-marks it pending and delivers it again.
The "closed gateway" between claim and complete is effectively the 
hardware masking the source while it's being serviced.
Then if we will handle guest interrupt in the following way:
1. Passthrough device asserts its line (level stays high).
2. Xen takes the physical IRQ, claims (gateway closes), completes 
(gateway reopens), injects a virtual IRQ into the guest's vPLIC.
3. The guest hasn't run yet, it hasn't touched the device's registers, 
so the device line is still high.
4. The PLIC sees the source still asserted with an open gateway -> marks 
pending -> fires another physical interrupt into Xen -> ... -> repeat.
So we get a storm of physical interrupts for a device the guest hasn't 
even begun servicing. The device line only drops when the guest driver 
writes the device's own registers, which happens long after, and on the 
guest's schedule.
So the solution is that the physical complete must wait until the guest 
has actually quiesced the device. The only signal Xen gets for "guest is 
done" is the guest writing its virtual complete to the emulated vPLIC. So:
1. guest_irq->ack: the claim already happened (the readl(CONTEXT_CLAIM) 
in plic_handle_interrupt); ack just records which context claimed it. 
The gateway stays closed - good, the source is masked while the guest works.
2. inject vIRQ → guest services the device (line drops) -> guest writes 
vPLIC complete.
3. guest_irq->end: now do the physical complete, reopening the gateway. 
Device is quiet -> no spurious re-trigger; if it's a new legitimate 
assertion, it fires once, correctly.
Is it clear enough now?

I understand that it would be much easier if you had access to the 
PLIC/vPLIC code, but I don't see much benefit in introducing it at this 
point. Perhaps we could proceed with the proposed approach and revisit 
the design once the PLIC/vPLIC code that uses it is available.
> > > > +#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()).
Agree, smp_rmb() would be enough here. I will use it here and update the 
comment about write-side counterpart.
> > > > +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?
If an irq will be at that moment in use by guest it won't be mapped to 
guest because of the checks inside route_irq_to_guest():
        if ( test_bit(_IRQ_GUEST, &desc->status) )
        {
            struct domain *ad = irq_get_domain(desc);

            if ( d != ad )
            {
                printk(XENLOG_G_ERR "IRQ %u is already used by %pd\n",
                       irq, ad);
                retval = -EBUSY;
            }
            else if ( irq_get_guest_info(desc)->virq != virq )
            {
                printk(XENLOG_G_ERR
                       "%pd: IRQ %u is already assigned to vIRQ %u\n",
                       d, irq, irq_get_guest_info(desc)->virq);
                retval = -EBUSY;
            }
        }

If route_irq_to_guest() will be called at the moment when release_irq() 
above was called but before xvfree() then it shouldn't be a problem as 
route_irq_to_guest() will allocate new info so for old info no users 
anymore. So at the moment when is executed ...:
+int release_guest_irq(struct domain *d, unsigned int virq)
+{
...
+    release_irq(desc->irq, info);
+    xvfree(info);
+    spin_unlock_irqrestore(&desc->lock, flags);

... release_guest_irq() is the exclusive owner of 'info' and so safe to 
tear down  + free.
The only fix I see that should be done it is set clear_bit(_IRQ_GUEST, 
&desc->status); before spin_unlock_irqrestore() to be sure that 
double-free of info isn't occured if release_guest_irq() will be called 
for the same virq. Perhpas it also make sense to add 
irq_set_guest_info(desc, NULL); just to be sure that desc->info is NULL.
~ Oleksii