[XEN,v3,03/11] xen: arm: Refactor route_irq_to_guest

Commit Message

Stewart Hildebrand Nov. 15, 2019, 8:01 p.m. UTC

From: Ian Campbell <[e-mail]>

Split out the bit which allocates the struct irqaction and calls
__setup_irq into a new function (setup_guest_irq). I'm going to want
to call this a second time in a subsequent patch.

Note that the action is now allocated and initialised with the desc
lock held (since it is taken by the caller). I don't think this is an
issue (and avoiding this would make things more complex)

Signed-off-by: Ian Campbell <[e-mail]>
Signed-off-by: Stewart Hildebrand <[e-mail]>
---
v2: New patch (maybe, it's been a while...)

v3: Rebase + trivial fixups

---
Note: I have not given much thought regarding Julien's comment in [1]

[1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01041.html
---
 xen/arch/arm/irq.c | 108 +++++++++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 44 deletions(-)

Comments

Julien Grall Nov. 23, 2019, 7:21 p.m. UTC | #1

Hi,

On 15/11/2019 20:01, Stewart Hildebrand wrote:
> From: Ian Campbell <[e-mail]>
> 
> Split out the bit which allocates the struct irqaction and calls
> __setup_irq into a new function (setup_guest_irq). I'm going to want
> to call this a second time in a subsequent patch.
> 
> Note that the action is now allocated and initialised with the desc
> lock held (since it is taken by the caller). I don't think this is an
> issue (and avoiding this would make things more complex)
> 
> Signed-off-by: Ian Campbell <[e-mail]>
> Signed-off-by: Stewart Hildebrand <[e-mail]>
> ---
> v2: New patch (maybe, it's been a while...)
> 
> v3: Rebase + trivial fixups
> 
> ---
> Note: I have not given much thought regarding Julien's comment in [1]
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg01041.html

I would rather avoid to do memory allocation with interrupts disabled.

You may need to introduce a new function that will allocate/setup the 
action, but I think this is worth the trouble. Note that the new 
function could likely be re-used in request_irq() as well.

Cheers,

Patch

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 3877657a52..9cc0a54867 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -407,61 +407,25 @@  bool irq_type_set_by_domain(const struct domain *d)
     return (d == hardware_domain);
 }
 
-/*
- * Route an IRQ to a specific guest.
- * For now only SPIs are assignable to the guest.
- */
-int route_irq_to_guest(struct domain *d, unsigned int virq,
-                       unsigned int irq, const char * devname)
+static int setup_guest_irq(struct irq_desc *desc, unsigned int virq,
+                           unsigned int irqflags,
+                           struct irq_guest *info, const char *devname)
 {
+    const unsigned irq = desc->irq;
     struct irqaction *action;
-    struct irq_guest *info;
-    struct irq_desc *desc;
-    unsigned long flags;
-    int retval = 0;
-
-    if ( virq >= vgic_num_irqs(d) )
-    {
-        printk(XENLOG_G_ERR
-               "the vIRQ number %u is too high for domain %u (max = %u)\n",
-               irq, d->domain_id, vgic_num_irqs(d));
-        return -EINVAL;
-    }
-
-    /* Only routing to virtual SPIs is supported */
-    if ( virq < NR_LOCAL_IRQS )
-    {
-        printk(XENLOG_G_ERR "IRQ can only be routed to an SPI\n");
-        return -EINVAL;
-    }
+    int retval;
+    struct domain *d = info->d;
 
-    if ( !is_assignable_irq(irq) )
-    {
-        printk(XENLOG_G_ERR "the IRQ%u is not routable\n", irq);
-        return -EINVAL;
-    }
-    desc = irq_to_desc(irq);
+    ASSERT(spin_is_locked(&desc->lock));
 
     action = xmalloc(struct irqaction);
     if ( !action )
         return -ENOMEM;
 
-    info = xmalloc(struct irq_guest);
-    if ( !info )
-    {
-        xfree(action);
-        return -ENOMEM;
-    }
-
-    info->d = d;
-    info->virq = virq;
-
     action->dev_id = info;
     action->name = devname;
     action->free_on_release = 1;
 
-    spin_lock_irqsave(&desc->lock, flags);
-
     if ( !irq_type_set_by_domain(d) && desc->arch.type == IRQ_TYPE_INVALID )
     {
         printk(XENLOG_G_ERR "IRQ %u has not been configured\n", irq);
@@ -496,6 +460,8 @@  int route_irq_to_guest(struct domain *d, unsigned int virq,
                        d->domain_id, irq, irq_get_guest_info(desc)->virq);
                 retval = -EBUSY;
             }
+            else
+                retval = 0;
         }
         else
         {
@@ -509,6 +475,61 @@  int route_irq_to_guest(struct domain *d, unsigned int virq,
     if ( retval )
         goto out;
 
+    return 0;
+
+out:
+    xfree(action);
+    return retval;
+}
+
+/*
+ * Route an IRQ to a specific guest.
+ * For now only SPIs are assignable to the 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;
+
+    if ( virq >= vgic_num_irqs(d) )
+    {
+        printk(XENLOG_G_ERR
+               "the vIRQ number %u is too high for domain %u (max = %u)\n",
+               irq, d->domain_id, vgic_num_irqs(d));
+        return -EINVAL;
+    }
+
+    /* Only routing to virtual SPIs is supported */
+    if ( virq < NR_LOCAL_IRQS )
+    {
+        printk(XENLOG_G_ERR "IRQ can only be routed to an SPI\n");
+        return -EINVAL;
+    }
+
+    if ( !is_assignable_irq(irq) )
+    {
+        printk(XENLOG_G_ERR "the IRQ%u is not routable\n", irq);
+        return -EINVAL;
+    }
+
+    desc = irq_to_desc(irq);
+
+    info = xmalloc(struct irq_guest);
+    if ( !info )
+        return -ENOMEM;
+
+    info->d = d;
+    info->virq = virq;
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    retval = setup_guest_irq(desc, virq, flags, info, devname);
+    if ( retval )
+        goto out;
+
     retval = gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
 
     spin_unlock_irqrestore(&desc->lock, flags);
@@ -523,7 +544,6 @@  int route_irq_to_guest(struct domain *d, unsigned int virq,
 
 out:
     spin_unlock_irqrestore(&desc->lock, flags);
-    xfree(action);
 free_info:
     xfree(info);