Re: [PATCH v7 16/20] xen/riscv: implement IRQ routing for device passthrough

On 8/18/26 12:18 PM, Jan Beulich wrote:
> On 04.08.2026 17:48, Oleksii Kurochko wrote:
>> dom0less device passthrough requires granting guest domains access to
>> device interrupts. Introduce map_device_irqs_to_domain() to enumerate
>> a DT node's interrupt properties, skipping those not owned by
>> the primary interrupt controller (as at the moment I haven't seen usages
>> of it), and map_irq_to_domain() to grant domain access and configure
>> Xen's interrupt descriptor accordingly. Sharing IRQ between domains is
>> rejected.
>>
>> Both map_irq_to_domain() and map_device_irqs_to_domain() are marked
>> __overlay_init, mirroring Arm: without CONFIG_OVERLAY_DTB this expands to
>> __init, so the functions are init-only and need no XSM check; with
>> CONFIG_OVERLAY_DTB they become runtime-callable, but the only runtime
>> entry point is dt_overlay_domctl(), which performs the XSM checks at the
>> domctl layer.  RISC-V does not wire up DT overlay yet, so today these are
>> strictly __init; if/when overlay support is added, the domctl-level XSM
>> gating must be added together with it, as on Arm.
>>
>> route_irq_to_guest() and release_irq() manage irq_desc ownership for
>> guest-assigned interrupts. Each assignment carries a small irq_guest
>> structure as irqaction::dev_id, recording the owning domain and virtual
>> IRQ number which is 1:1 mapped to physical IRQ number. A per-domain
>> vIRQ allocation bitmap (used_irqs in struct vintc), managed by
>> vintc_reserve_virq(), prevents the same vIRQ being claimed twice.
>>
>> Host and guest interrupts may differ in some operations (EOI timing in
>> particular, possibly others): a host IRQ is completed once Xen's handler
>> runs, whereas a passthrough IRQ must defer the physical completion until
>> the guest issues its own EOI, otherwise a still-asserted level line would
>> immediately retrigger and storm. This affects only the .end callback;
>> the rest of hw_interrupt_type is shared, hence the separate host and
>> guest hw_interrupt_type instances.
>>
>> With APLIC+IMSIC, guest interrupts are delivered directly by hardware
>> through the IMSIC, bypassing do_IRQ(). The _IRQ_GUEST branch in
>> do_IRQ() is therefore left as BUG() until a platform without direct
>> IMSIC delivery is encountered.
>>
>> Signed-off-by: Oleksii Kurochko <[email protected]>

[...]

>> Updates
>>
>> Signed-off-by: Oleksii Kurochko <[email protected]>
>> Changes in v7:
>>   - Build device.c as device.init.o: everything it provides is
>>     __overlay_init, which is plain __init as long as CONFIG_OVERLAY_DTB
>>     stays Arm-only.  Unlike Arm, which picks device.o/device.init.o based
>>     on that config, RISC-V cannot enable it, so the choice is
>>     unconditional for now.
>>   - Don't have release_irq() free the guest IRQ info anymore: set
>>     free_on_release = false and free 'info' explicitly in
>>     release_guest_irq(), i.e. reinstate the xvfree() dropped in v5.  The
>>     action stays embedded in struct irq_guest, so a single allocation
>>     still covers both, but it no longer has to be the structure's first
>>     member: the offsetof() BUILD_BUG_ON and the xvfree() of a pointer
>>     that merely happened to coincide with the allocation base are gone.
>>     The ->dev_id concern from v5 doesn't apply: release_irq() clears
>>     desc->action under desc->lock and waits for in-flight handling before
>>     returning, so nothing can observe ->dev_id once 'info' is freed.
>>   - Move 'action' to the end of struct irq_guest and reword its comment
>>     accordingly.
>>   - Use xvzalloc() instead of xvmalloc() for struct irq_guest, so that
>>     the embedded action is fully initialized (action.handler was left
>>     uninitialized before).
>>   - route_irq_to_guest(): free 'info' via the common free_info label when
>>     intc_route_irq_to_guest() fails, now that release_irq() no longer
>>     frees it.
>>   - Drop a stray blank line ahead of release_irq().
>> ---
>> ---
> 
> Why does this appear a 2nd time? All of the above is already long / verbose
> enough.

A rebase issue. all these changes were initially in the separate patch 
and after squashed I missed to remove this part.

I will drop it.
> 
>> @@ -101,12 +119,31 @@ int domain_vintc_init(struct domain *d)
>>           break;
>>       }
>>   
>> +    if ( !ret )
>> +    {
>> +        d->arch.vintc->used_irqs =
>> +            xvzalloc_array(unsigned long,
>> +                           BITS_TO_LONGS(d->arch.vintc->nr_virqs));
>> +        if ( !d->arch.vintc->used_irqs )
>> +            ret = -ENOMEM;
>> +    }
>> +
>>       return ret;
>>   }
> 
> Patch 13 doesn't arrange for domain_vintc_deinit() to be called when
> domain_vintc_init() fails. Ideally that would change, or else you'd
> now need to call the function in the error case from here.


I will add a call of domain_vintc_deinit() in arch_domain_destroy() in 
patch 13:

  void arch_domain_destroy(struct domain *d)
  {
-    printk(XENLOG_WARNING "%s: unimplemented\n", __func__);
+    printk(XENLOG_WARNING "%s: not fully implemented\n", __func__);
+
+    domain_vintc_deinit(d);
  }

> In either
> case ...
> 
>>   void domain_vintc_deinit(struct domain *d)
>>   {
>>       const enum intc_variant variant = intc_hw_ops->info->hw_variant;
>> +    unsigned int virq;
>> +
>> +    if ( !d->arch.vintc )
>> +        return;
>> +
>> +    for ( virq = 0; virq < d->arch.vintc->nr_virqs; virq++ )
>> +        if ( test_bit(virq, d->arch.vintc->used_irqs) )
>> +            release_guest_irq(d, virq);
>> +
>> +    XVFREE(d->arch.vintc->used_irqs);
> 
> ... this function will then need to become resilient against being
> called with partially initialized state.

I will update it to:

  void domain_vintc_deinit(struct domain *d)
  {
      const enum intc_variant variant = intc_hw_ops->info->hw_variant;
-    unsigned int virq;

      if ( !d->arch.vintc )
          return;

-    for ( virq = 0; virq < d->arch.vintc->nr_virqs; virq++ )
-        if ( test_bit(virq, d->arch.vintc->used_irqs) )
-            release_guest_irq(d, virq);
+    if ( d->arch.vintc->used_irqs )
+    {
+        unsigned int virq;
+
+        for ( virq = 0; virq < d->arch.vintc->nr_virqs; virq++ )
+            if ( test_bit(virq, d->arch.vintc->used_irqs) )
+                release_guest_irq(d, virq);

-    XVFREE(d->arch.vintc->used_irqs);
+        XVFREE(d->arch.vintc->used_irqs);
+    }


> 
>> @@ -118,3 +155,11 @@ void domain_vintc_deinit(struct domain *d)
>>           break;
>>       }
>>   }
>> +
>> +bool vintc_reserve_virq(const struct domain *d, unsigned int virq)
>> +{
>> +    if ( virq >= d->arch.vintc->nr_virqs )
>> +        return false;
>> +
>> +    return !test_and_set_bit(virq, d->arch.vintc->used_irqs);
>> +}
> 
> Is the present caller of this going to remain the only one? If so,
> __overlay_init would want using here as well. If not - will future
> callers appear on paths which are exposed to guests? If in turn so,
> speculation safety may need adding here.

I don't see any others calls of it in downstream. So I will add 
__overlay_init.

> 
>> @@ -227,3 +250,206 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq)
>>       spin_unlock(&desc->lock);
>>       irq_exit();
>>   }
>> +
>> +static struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
>> +{
>> +    ASSERT(spin_is_locked(&desc->lock));
>> +    ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>> +    ASSERT(desc->action != NULL);
> 
> Btw, no need for the " != NULL" part.

I will drop it.

> 
>> +    return desc->action->dev_id;
>> +}
>> +
>> +void release_irq(unsigned int irq, const void *dev_id)
>> +{
>> +    struct irq_desc *desc;
>> +    unsigned long flags;
>> +    struct irqaction *action, **action_ptr;
>> +
>> +    desc = irq_to_desc(irq);
> 
> Can't this (once again) be the initializer of the variable?
> 
>> +    spin_lock_irqsave(&desc->lock, flags);
>> +
>> +    action_ptr = &desc->action;
> 
> Same for this one, which also doesn't require the lock to be held.

I will apply both remarks.

> 
>> +#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.
>> +     *
>> +     * The read barrier pairs with the spin_unlock() in do_IRQ(): once we
>> +     * observe _IRQ_INPROGRESS cleared, we are guaranteed to also see the
>> +     * writes do_IRQ() made to desc (e.g. desc->action) before releasing the
>> +     * lock, so it is safe to free the action below.
>> +     */
>> +    do { smp_rmb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );
>> +
>> +    if ( action->free_on_release )
>> +        xvfree(action);
>> +}
>> +
>> +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;
>> +    int ret = -EINVAL;
>> +
>> +    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;
>> +
>> +    /*
>> +     * Live IRQ unrouting from a running domain is not supported: the tear-down
>> +     * drops desc->lock across release_irq()/xvfree() and relies on no
>> +     * concurrent route_irq_to_guest() being issued for this domain. Only permit
>> +     * it for a dying domain, where assignment is frozen and no new routes can
>> +     * appear.
>> +     */
>> +    if ( !d->is_dying )
>> +    {
>> +        ret = -EBUSY;
>> +        goto unlock_err;
>> +    }
>> +
>> +    /*
>> +     * Clear _IRQ_GUEST while still holding the lock so that a concurrent
>> +     * release_guest_irq() for the same IRQ observes it and bails out, rather
>> +     * than capturing the same 'info' and double-freeing it below.
>> +     */
>> +    __clear_bit(_IRQ_GUEST, &desc->status);
>> +
>> +    spin_unlock_irqrestore(&desc->lock, flags);
>> +
>> +    release_irq(desc->irq, info);
>> +    xvfree(info);
> 
> While in the v7 revlog you claim there is no issue here, imo there (still) is.
> You obtain "info" with the lock held, then drop the lock, for release_irq() to
> re-acquire. If a similar pattern was used elsewhere (info obtained under lock,
> lock dropped, then using info), the pointer would go stale the moment you free
> it here. Imo for this to be safe _and_ not setting a bad precendent, you need
> a variant of release_irq() which is passed desc with the lock already held.
> release_irq() itself (if to be called from anywhere else) would then be a thin
> wrapper around it.

I agree that it will be safer in general.

I will introduce:

static void irq_release_action(const struct irq_desc *desc,
                                struct irqaction *action)
{
     /*
      * Wait to make sure it's not being used on another CPU.
      *
      * The read barrier pairs with the spin_unlock() in do_IRQ(): once we
      * observe _IRQ_INPROGRESS cleared, we are guaranteed to also see the
      * writes do_IRQ() made to desc (e.g. desc->action) before 
releasing the
      * lock, so it is safe to free the action below.
      */
     do { smp_rmb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );

     if ( action->free_on_release )
         xvfree(action);
}

+void release_irq(unsigned int irq, const void *dev_id)
+{
+    struct irq_desc *desc = irq_to_desc(irq);
+    struct irqaction *action;
+    unsigned long flags;
+
+    spin_lock_irqsave(&desc->lock, flags);
+    action = irq_detach_action(desc, dev_id);
+    spin_unlock_irqrestore(&desc->lock, flags);
+
+    if ( action )
+        irq_release_action(desc, action);
+}

Where irq_detach_action() will be almost what release_irq() was before:

+static struct irqaction *irq_detach_action(struct irq_desc *desc,
+                                           const void *dev_id)
  {
-    struct irq_desc *desc = irq_to_desc(irq);
-    unsigned long flags;
      struct irqaction *action, **action_ptr = &desc->action;

-    spin_lock_irqsave(&desc->lock, flags);
+    ASSERT(spin_is_locked(&desc->lock));
+
  #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 )
+        if ( !action || (action->dev_id == dev_id) )
              break;

          action_ptr = &action->next;
      }
+#else
+    action = *action_ptr;
+#endif
+
+    if ( !action )
+    {
+        printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n",
+               desc->irq);
+        return NULL;
+    }

      /* Found it - remove it from the action list */
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
      *action_ptr = action->next;
  #else
-    action = *action_ptr;
      *action_ptr = NULL;
  #endif

@@ -298,8 +309,22 @@ void release_irq(unsigned int irq, const void *dev_id)
          __clear_bit(_IRQ_GUEST, &desc->status);
      }

-    spin_unlock_irqrestore(&desc->lock, flags);
+    return action;
+}

and then release_guest_irq() (the end) will be changed in the following way:

....
     /*
      * Detaching the action happens with desc->lock still held, so that a
      * concurrent release_guest_irq() for the same IRQ sees _IRQ_GUEST 
already
      * cleared and bails out, rather than capturing the same 'info' and
      * double-freeing it below.
      */
     action = irq_detach_action(desc, info);

     spin_unlock_irqrestore(&desc->lock, flags);

     if ( action )
         irq_release_action(desc, action);

     xvfree(info);

     return 0;

> 
>> +/* 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 irq_guest *info;
>> +    struct irq_desc *desc;
>> +    unsigned long flags;
>> +    int retval = 0;
>> +
>> +    if ( d->is_dying )
>> +        return -EINVAL;
>> +
>> +    desc = irq_to_desc(irq);
> 
> Imo this either wants to be the initializer of the variable, or (perhaps
> better here) it wants to move immediately ahead of ...
> 
>> +    info = xvzalloc(struct irq_guest);
>> +    if ( !info )
>> +        return -ENOMEM;
>> +
>> +    info->d = d;
>> +    info->virq = virq;
>> +
>> +    info->action.dev_id = info;
>> +    info->action.name = devname;
>> +    /* The action is part of 'info', thus it is freed together with it. */
>> +    info->action.free_on_release = false;
>> +
>> +    spin_lock_irqsave(&desc->lock, flags);
> 
> ... this.

I will move initialization here.

> 
>> +    /*
>> +     * If the IRQ is already used by someone
>> +     *  - If it's the same domain -> Xen doesn't need to update the IRQ desc.
>> +     *  For safety check if we are not trying to assign the IRQ to a
>> +     *  different vIRQ.
>> +     *  - Otherwise -> For now, don't allow the IRQ to be shared between
>> +     *  Xen and domains.
>> +     */
>> +    if ( desc->action != NULL )
>> +    {
>> +        if ( test_bit(_IRQ_GUEST, &desc->status) )
>> +        {
>> +            struct domain *ad = irq_get_guest_info(desc)->d;
>> +
>> +            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;
>> +            }
>> +        }
>> +        else
>> +        {
>> +            printk(XENLOG_G_ERR "IRQ %u is already used by Xen\n", irq);
>> +            retval = -EBUSY;
>> +        }
>> +        goto out;
>> +    }
>> +
>> +    retval = _setup_irq(desc, 0, &info->action);
>> +    if ( retval )
>> +        goto out;
>> +
>> +    retval = intc_route_irq_to_guest(desc, IRQ_NO_PRIORITY);
>> +
>> +    spin_unlock_irqrestore(&desc->lock, flags);
>> +
>> +    if ( retval )
>> +    {
>> +        release_irq(desc->irq, info);
> 
> Like above, I think you want to avoid transiently dropping the lock here.

With suggested above it will look like:

      retval = intc_route_irq_to_guest(desc, IRQ_NO_PRIORITY);
-
-    spin_unlock_irqrestore(&desc->lock, flags);
-
      if ( retval )
      {
-        release_irq(desc->irq, info);
+        struct irqaction *action = irq_detach_action(desc, info);
+
+        spin_unlock_irqrestore(&desc->lock, flags);
+
+        if ( action )
+            irq_release_action(desc, action);
+
          goto free_info;
      }

+    spin_unlock_irqrestore(&desc->lock, flags);
+
      return 0;

   out:

Thanks.

~ Oleksii