[PATCH v3 0/8] ARM: VGIC/GIC separation cleanups

[PATCH v3 0/8] ARM: VGIC/GIC separation cleanups

[Andre Przywara]

Hi,

minor updates to this series. Three patches have been merged already.
Patch 1/8 (was v2 04/10) has been significantly reworked to address
Stefano's comments, see the reply to his v2 comment.
The rest has just been rebased and got tagged, if appropriate.
The last patch is an improvement to the already merged 02/10, as suggested
by Julien.

Please test and apply.

Cheers,
Andre

Changelog:
01: reworked
02: rebased to match staging, added tag
03, 07: added tag
08: new, as suggested by Julien

================
By the original VGIC design, Xen differentiates between the actual VGIC
emulation on one hand and the GIC hardware accesses on the other.
It seems there were some deviations from that scheme (over time?), so at
the moment we end up happily accessing VGIC specific data structures
like struct pending_irq and struct vgic_irq_rank from pure GIC files
like gic.c or even irq.c (try: git grep -l struct\ pending_irq xen/arch/arm).
But any future VGIC rework will depend on a clean separation, so this
series tries to clean this up.
After this series there are no more references to VGIC structures from
GIC files, at least for non-ITS code. The ITS is a beast own its own
(blame the author) and will be addressed later.

Andre Przywara (8):
  ARM: VGIC: drop unneeded gic_restore_pending_irqs()
  ARM: VGIC: split gic.c to observe hardware/virtual GIC separation
  ARM: VGIC: split up gic_dump_info() to cover virtual part separately
  ARM: VGIC: rework events_need_delivery()
  ARM: VGIC: factor out vgic_connect_hw_irq()
  ARM: VGIC: factor out vgic_get_hw_irq_desc()
  ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq
  ARM: make nr_irqs a constant

 xen/arch/arm/Makefile       |   1 +
 xen/arch/arm/domain.c       |   1 +
 xen/arch/arm/gic-v2.c       |  14 +-
 xen/arch/arm/gic-v3.c       |  12 +-
 xen/arch/arm/gic-vgic.c     | 468 ++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic.c          | 421 +--------------------------------------
 xen/arch/arm/irq.c          |   9 +-
 xen/arch/arm/vgic.c         |  11 ++
 xen/include/asm-arm/event.h |  13 +-
 xen/include/asm-arm/gic.h   |   5 +-
 xen/include/asm-arm/irq.h   |   5 +-
 xen/include/asm-arm/vgic.h  |   6 +
 12 files changed, 521 insertions(+), 445 deletions(-)
 create mode 100644 xen/arch/arm/gic-vgic.c

-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 1/8] ARM: VGIC: drop unneeded gic_restore_pending_irqs()

[Andre Przywara]

In gic_restore_pending_irqs() we push our pending virtual IRQs into the
list registers. This function is called once from gic_inject(), just
before we return to the guest, but also in gic_restore_state(), when
we context-switch a VCPU. Having a closer look it turns out that the
later call is not needed, since we will always call gic_inject() anyway.
So remove that call (and the forward declaration) to streamline this
interface and make separating the GIC from the VGIC world later.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/gic.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index bac8ada2bb..721a17a9d7 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -36,8 +36,6 @@
 #include <asm/vgic.h>
 #include <asm/acpi.h>
 
-static void gic_restore_pending_irqs(struct vcpu *v);
-
 static DEFINE_PER_CPU(uint64_t, lr_mask);
 
 #define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->info->nr_lrs) - 1))
@@ -91,8 +89,6 @@ void gic_restore_state(struct vcpu *v)
     gic_hw_ops->restore_state(v);
 
     isb();
-
-    gic_restore_pending_irqs(v);
 }
 
 /* desc->irq needs to be disabled before calling this function */
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 2/8] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation

[Andre Przywara]

Currently gic.c holds code to handle hardware IRQs as well as code to
bridge VGIC requests to the GIC virtualization hardware.
Despite being named gic.c, this file reaches into the VGIC and uses data
structures describing virtual IRQs.
To improve abstraction, move the VGIC functions into a separate file,
so that gic.c does what it says on the tin.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/Makefile   |   1 +
 xen/arch/arm/gic-vgic.c | 410 ++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic.c      | 363 +-----------------------------------------
 3 files changed, 413 insertions(+), 361 deletions(-)
 create mode 100644 xen/arch/arm/gic-vgic.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 30a2a6500a..41d7366527 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -16,6 +16,7 @@ obj-y += domain_build.o
 obj-y += domctl.o
 obj-$(EARLY_PRINTK) += early_printk.o
 obj-y += gic.o
+obj-y += gic-vgic.o
 obj-y += gic-v2.o
 obj-$(CONFIG_HAS_GICV3) += gic-v3.o
 obj-$(CONFIG_HAS_ITS) += gic-v3-its.o
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
new file mode 100644
index 0000000000..98df84e3d5
--- /dev/null
+++ b/xen/arch/arm/gic-vgic.c
@@ -0,0 +1,410 @@
+/*
+ * xen/arch/arm/gic-vgic.c
+ *
+ * ARM Generic Interrupt Controller virtualization support
+ *
+ * Tim Deegan <tim@xen.org>
+ * Copyright (c) 2011 Citrix Systems.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <xen/lib.h>
+#include <xen/init.h>
+#include <xen/mm.h>
+#include <xen/irq.h>
+#include <xen/sched.h>
+#include <xen/errno.h>
+#include <xen/softirq.h>
+#include <xen/list.h>
+#include <xen/device_tree.h>
+#include <xen/acpi.h>
+#include <asm/p2m.h>
+#include <asm/domain.h>
+#include <asm/platform.h>
+#include <asm/device.h>
+#include <asm/io.h>
+#include <asm/gic.h>
+#include <asm/vgic.h>
+#include <asm/acpi.h>
+
+extern uint64_t per_cpu__lr_mask;
+extern const struct gic_hw_operations *gic_hw_ops;
+
+#define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->info->nr_lrs) - 1))
+
+#undef GIC_DEBUG
+
+static void gic_update_one_lr(struct vcpu *v, int i);
+
+static inline void gic_set_lr(int lr, struct pending_irq *p,
+                              unsigned int state)
+{
+    ASSERT(!local_irq_is_enabled());
+
+    clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
+
+    gic_hw_ops->update_lr(lr, p, state);
+
+    set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+    p->lr = lr;
+}
+
+static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
+{
+    struct pending_irq *iter;
+
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    if ( !list_empty(&n->lr_queue) )
+        return;
+
+    list_for_each_entry ( iter, &v->arch.vgic.lr_pending, lr_queue )
+    {
+        if ( iter->priority > n->priority )
+        {
+            list_add_tail(&n->lr_queue, &iter->lr_queue);
+            return;
+        }
+    }
+    list_add_tail(&n->lr_queue, &v->arch.vgic.lr_pending);
+}
+
+void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p)
+{
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    list_del_init(&p->lr_queue);
+}
+
+void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
+{
+    struct pending_irq *n = irq_to_pending(v, virtual_irq);
+
+    /* If an LPI has been removed meanwhile, there is nothing left to raise. */
+    if ( unlikely(!n) )
+        return;
+
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    /* Don't try to update the LR if the interrupt is disabled */
+    if ( !test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
+        return;
+
+    if ( list_empty(&n->lr_queue) )
+    {
+        if ( v == current )
+            gic_update_one_lr(v, n->lr);
+    }
+#ifdef GIC_DEBUG
+    else
+        gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is still lr_pending\n",
+                 virtual_irq, v->domain->domain_id, v->vcpu_id);
+#endif
+}
+
+/*
+ * Find an unused LR to insert an IRQ into, starting with the LR given
+ * by @lr. If this new interrupt is a PRISTINE LPI, scan the other LRs to
+ * avoid inserting the same IRQ twice. This situation can occur when an
+ * event gets discarded while the LPI is in an LR, and a new LPI with the
+ * same number gets mapped quickly afterwards.
+ */
+static unsigned int gic_find_unused_lr(struct vcpu *v,
+                                       struct pending_irq *p,
+                                       unsigned int lr)
+{
+    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
+    unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask);
+    struct gic_lr lr_val;
+
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    if ( unlikely(test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status)) )
+    {
+        unsigned int used_lr;
+
+        for_each_set_bit(used_lr, lr_mask, nr_lrs)
+        {
+            gic_hw_ops->read_lr(used_lr, &lr_val);
+            if ( lr_val.virq == p->irq )
+                return used_lr;
+        }
+    }
+
+    lr = find_next_zero_bit(lr_mask, nr_lrs, lr);
+
+    return lr;
+}
+
+void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
+        unsigned int priority)
+{
+    int i;
+    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
+    struct pending_irq *p = irq_to_pending(v, virtual_irq);
+
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    if ( unlikely(!p) )
+        /* An unmapped LPI does not need to be raised. */
+        return;
+
+    if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
+    {
+        i = gic_find_unused_lr(v, p, 0);
+
+        if (i < nr_lrs) {
+            set_bit(i, &this_cpu(lr_mask));
+            gic_set_lr(i, p, GICH_LR_PENDING);
+            return;
+        }
+    }
+
+    gic_add_to_lr_pending(v, p);
+}
+
+static void gic_update_one_lr(struct vcpu *v, int i)
+{
+    struct pending_irq *p;
+    int irq;
+    struct gic_lr lr_val;
+
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(!local_irq_is_enabled());
+
+    gic_hw_ops->read_lr(i, &lr_val);
+    irq = lr_val.virq;
+    p = irq_to_pending(v, irq);
+    /*
+     * An LPI might have been unmapped, in which case we just clean up here.
+     * If that LPI is marked as PRISTINE, the information in the LR is bogus,
+     * as it belongs to a previous, already unmapped LPI. So we discard it
+     * here as well.
+     */
+    if ( unlikely(!p ||
+                  test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status)) )
+    {
+        ASSERT(is_lpi(irq));
+
+        gic_hw_ops->clear_lr(i);
+        clear_bit(i, &this_cpu(lr_mask));
+
+        return;
+    }
+
+    if ( lr_val.state & GICH_LR_ACTIVE )
+    {
+        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
+        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
+        {
+            if ( p->desc == NULL )
+            {
+                 lr_val.state |= GICH_LR_PENDING;
+                 gic_hw_ops->write_lr(i, &lr_val);
+            }
+            else
+                gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
+                         irq, v->domain->domain_id, v->vcpu_id, i);
+        }
+    }
+    else if ( lr_val.state & GICH_LR_PENDING )
+    {
+        int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+#ifdef GIC_DEBUG
+        if ( q )
+            gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is already pending in LR%d\n",
+                    irq, v->domain->domain_id, v->vcpu_id, i);
+#endif
+    }
+    else
+    {
+        gic_hw_ops->clear_lr(i);
+        clear_bit(i, &this_cpu(lr_mask));
+
+        if ( p->desc != NULL )
+            clear_bit(_IRQ_INPROGRESS, &p->desc->status);
+        clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+        clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
+        p->lr = GIC_INVALID_LR;
+        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+            gic_raise_guest_irq(v, irq, p->priority);
+        else {
+            list_del_init(&p->inflight);
+            /*
+             * Remove from inflight, then change physical affinity. It
+             * makes sure that when a new interrupt is received on the
+             * next pcpu, inflight is already cleared. No concurrent
+             * accesses to inflight.
+             */
+            smp_wmb();
+            if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+            {
+                struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
+                irq_set_affinity(p->desc, cpumask_of(v_target->processor));
+                clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+            }
+        }
+    }
+}
+
+void gic_clear_lrs(struct vcpu *v)
+{
+    int i = 0;
+    unsigned long flags;
+    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
+
+    /* The idle domain has no LRs to be cleared. Since gic_restore_state
+     * doesn't write any LR registers for the idle domain they could be
+     * non-zero. */
+    if ( is_idle_vcpu(v) )
+        return;
+
+    gic_hw_ops->update_hcr_status(GICH_HCR_UIE, false);
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    while ((i = find_next_bit((const unsigned long *) &this_cpu(lr_mask),
+                              nr_lrs, i)) < nr_lrs ) {
+        gic_update_one_lr(v, i);
+        i++;
+    }
+
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+}
+
+static void gic_restore_pending_irqs(struct vcpu *v)
+{
+    int lr = 0;
+    struct pending_irq *p, *t, *p_r;
+    struct list_head *inflight_r;
+    unsigned long flags;
+    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
+    int lrs = nr_lrs;
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    if ( list_empty(&v->arch.vgic.lr_pending) )
+        goto out;
+
+    inflight_r = &v->arch.vgic.inflight_irqs;
+    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
+    {
+        lr = gic_find_unused_lr(v, p, lr);
+        if ( lr >= nr_lrs )
+        {
+            /* No more free LRs: find a lower priority irq to evict */
+            list_for_each_entry_reverse( p_r, inflight_r, inflight )
+            {
+                if ( p_r->priority == p->priority )
+                    goto out;
+                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
+                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
+                    goto found;
+            }
+            /* We didn't find a victim this time, and we won't next
+             * time, so quit */
+            goto out;
+
+found:
+            lr = p_r->lr;
+            p_r->lr = GIC_INVALID_LR;
+            set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
+            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
+            gic_add_to_lr_pending(v, p_r);
+            inflight_r = &p_r->inflight;
+        }
+
+        gic_set_lr(lr, p, GICH_LR_PENDING);
+        list_del_init(&p->lr_queue);
+        set_bit(lr, &this_cpu(lr_mask));
+
+        /* We can only evict nr_lrs entries */
+        lrs--;
+        if ( lrs == 0 )
+            break;
+    }
+
+out:
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+}
+
+void gic_clear_pending_irqs(struct vcpu *v)
+{
+    struct pending_irq *p, *t;
+
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    v->arch.lr_mask = 0;
+    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
+        gic_remove_from_lr_pending(v, p);
+}
+
+int gic_events_need_delivery(void)
+{
+    struct vcpu *v = current;
+    struct pending_irq *p;
+    unsigned long flags;
+    const unsigned long apr = gic_hw_ops->read_apr(0);
+    int mask_priority;
+    int active_priority;
+    int rc = 0;
+
+    mask_priority = gic_hw_ops->read_vmcr_priority();
+    active_priority = find_next_bit(&apr, 32, 0);
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    /* TODO: We order the guest irqs by priority, but we don't change
+     * the priority of host irqs. */
+
+    /* find the first enabled non-active irq, the queue is already
+     * ordered by priority */
+    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
+    {
+        if ( GIC_PRI_TO_GUEST(p->priority) >= mask_priority )
+            goto out;
+        if ( GIC_PRI_TO_GUEST(p->priority) >= active_priority )
+            goto out;
+        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+        {
+            rc = 1;
+            goto out;
+        }
+    }
+
+out:
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+    return rc;
+}
+
+void gic_inject(void)
+{
+    ASSERT(!local_irq_is_enabled());
+
+    gic_restore_pending_irqs(current);
+
+    if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
+        gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 721a17a9d7..04e6d66b69 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -36,15 +36,11 @@
 #include <asm/vgic.h>
 #include <asm/acpi.h>
 
-static DEFINE_PER_CPU(uint64_t, lr_mask);
-
-#define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->info->nr_lrs) - 1))
+DEFINE_PER_CPU(uint64_t, lr_mask);
 
 #undef GIC_DEBUG
 
-static void gic_update_one_lr(struct vcpu *v, int i);
-
-static const struct gic_hw_operations *gic_hw_ops;
+const struct gic_hw_operations *gic_hw_ops;
 
 void register_gic_ops(const struct gic_hw_operations *ops)
 {
@@ -366,361 +362,6 @@ void gic_disable_cpu(void)
     gic_hw_ops->disable_interface();
 }
 
-static inline void gic_set_lr(int lr, struct pending_irq *p,
-                              unsigned int state)
-{
-    ASSERT(!local_irq_is_enabled());
-
-    clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
-
-    gic_hw_ops->update_lr(lr, p, state);
-
-    set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
-    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
-    p->lr = lr;
-}
-
-static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
-{
-    struct pending_irq *iter;
-
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
-
-    if ( !list_empty(&n->lr_queue) )
-        return;
-
-    list_for_each_entry ( iter, &v->arch.vgic.lr_pending, lr_queue )
-    {
-        if ( iter->priority > n->priority )
-        {
-            list_add_tail(&n->lr_queue, &iter->lr_queue);
-            return;
-        }
-    }
-    list_add_tail(&n->lr_queue, &v->arch.vgic.lr_pending);
-}
-
-void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p)
-{
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
-
-    list_del_init(&p->lr_queue);
-}
-
-void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
-{
-    struct pending_irq *n = irq_to_pending(v, virtual_irq);
-
-    /* If an LPI has been removed meanwhile, there is nothing left to raise. */
-    if ( unlikely(!n) )
-        return;
-
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
-
-    /* Don't try to update the LR if the interrupt is disabled */
-    if ( !test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
-        return;
-
-    if ( list_empty(&n->lr_queue) )
-    {
-        if ( v == current )
-            gic_update_one_lr(v, n->lr);
-    }
-#ifdef GIC_DEBUG
-    else
-        gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is still lr_pending\n",
-                 virtual_irq, v->domain->domain_id, v->vcpu_id);
-#endif
-}
-
-/*
- * Find an unused LR to insert an IRQ into, starting with the LR given
- * by @lr. If this new interrupt is a PRISTINE LPI, scan the other LRs to
- * avoid inserting the same IRQ twice. This situation can occur when an
- * event gets discarded while the LPI is in an LR, and a new LPI with the
- * same number gets mapped quickly afterwards.
- */
-static unsigned int gic_find_unused_lr(struct vcpu *v,
-                                       struct pending_irq *p,
-                                       unsigned int lr)
-{
-    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
-    unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask);
-    struct gic_lr lr_val;
-
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
-
-    if ( unlikely(test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status)) )
-    {
-        unsigned int used_lr;
-
-        for_each_set_bit(used_lr, lr_mask, nr_lrs)
-        {
-            gic_hw_ops->read_lr(used_lr, &lr_val);
-            if ( lr_val.virq == p->irq )
-                return used_lr;
-        }
-    }
-
-    lr = find_next_zero_bit(lr_mask, nr_lrs, lr);
-
-    return lr;
-}
-
-void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
-        unsigned int priority)
-{
-    int i;
-    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
-    struct pending_irq *p = irq_to_pending(v, virtual_irq);
-
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
-
-    if ( unlikely(!p) )
-        /* An unmapped LPI does not need to be raised. */
-        return;
-
-    if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
-    {
-        i = gic_find_unused_lr(v, p, 0);
-
-        if (i < nr_lrs) {
-            set_bit(i, &this_cpu(lr_mask));
-            gic_set_lr(i, p, GICH_LR_PENDING);
-            return;
-        }
-    }
-
-    gic_add_to_lr_pending(v, p);
-}
-
-static void gic_update_one_lr(struct vcpu *v, int i)
-{
-    struct pending_irq *p;
-    int irq;
-    struct gic_lr lr_val;
-
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
-    ASSERT(!local_irq_is_enabled());
-
-    gic_hw_ops->read_lr(i, &lr_val);
-    irq = lr_val.virq;
-    p = irq_to_pending(v, irq);
-    /*
-     * An LPI might have been unmapped, in which case we just clean up here.
-     * If that LPI is marked as PRISTINE, the information in the LR is bogus,
-     * as it belongs to a previous, already unmapped LPI. So we discard it
-     * here as well.
-     */
-    if ( unlikely(!p ||
-                  test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status)) )
-    {
-        ASSERT(is_lpi(irq));
-
-        gic_hw_ops->clear_lr(i);
-        clear_bit(i, &this_cpu(lr_mask));
-
-        return;
-    }
-
-    if ( lr_val.state & GICH_LR_ACTIVE )
-    {
-        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
-        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
-             test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
-        {
-            if ( p->desc == NULL )
-            {
-                 lr_val.state |= GICH_LR_PENDING;
-                 gic_hw_ops->write_lr(i, &lr_val);
-            }
-            else
-                gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
-                         irq, v->domain->domain_id, v->vcpu_id, i);
-        }
-    }
-    else if ( lr_val.state & GICH_LR_PENDING )
-    {
-        int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
-#ifdef GIC_DEBUG
-        if ( q )
-            gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is already pending in LR%d\n",
-                    irq, v->domain->domain_id, v->vcpu_id, i);
-#endif
-    }
-    else
-    {
-        gic_hw_ops->clear_lr(i);
-        clear_bit(i, &this_cpu(lr_mask));
-
-        if ( p->desc != NULL )
-            clear_bit(_IRQ_INPROGRESS, &p->desc->status);
-        clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
-        clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
-        p->lr = GIC_INVALID_LR;
-        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
-             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
-             !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
-            gic_raise_guest_irq(v, irq, p->priority);
-        else {
-            list_del_init(&p->inflight);
-            /*
-             * Remove from inflight, then change physical affinity. It
-             * makes sure that when a new interrupt is received on the
-             * next pcpu, inflight is already cleared. No concurrent
-             * accesses to inflight.
-             */
-            smp_wmb();
-            if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
-            {
-                struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
-                irq_set_affinity(p->desc, cpumask_of(v_target->processor));
-                clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
-            }
-        }
-    }
-}
-
-void gic_clear_lrs(struct vcpu *v)
-{
-    int i = 0;
-    unsigned long flags;
-    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
-
-    /* The idle domain has no LRs to be cleared. Since gic_restore_state
-     * doesn't write any LR registers for the idle domain they could be
-     * non-zero. */
-    if ( is_idle_vcpu(v) )
-        return;
-
-    gic_hw_ops->update_hcr_status(GICH_HCR_UIE, false);
-
-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
-
-    while ((i = find_next_bit((const unsigned long *) &this_cpu(lr_mask),
-                              nr_lrs, i)) < nr_lrs ) {
-        gic_update_one_lr(v, i);
-        i++;
-    }
-
-    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
-}
-
-static void gic_restore_pending_irqs(struct vcpu *v)
-{
-    int lr = 0;
-    struct pending_irq *p, *t, *p_r;
-    struct list_head *inflight_r;
-    unsigned long flags;
-    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
-    int lrs = nr_lrs;
-
-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
-
-    if ( list_empty(&v->arch.vgic.lr_pending) )
-        goto out;
-
-    inflight_r = &v->arch.vgic.inflight_irqs;
-    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
-    {
-        lr = gic_find_unused_lr(v, p, lr);
-        if ( lr >= nr_lrs )
-        {
-            /* No more free LRs: find a lower priority irq to evict */
-            list_for_each_entry_reverse( p_r, inflight_r, inflight )
-            {
-                if ( p_r->priority == p->priority )
-                    goto out;
-                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
-                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
-                    goto found;
-            }
-            /* We didn't find a victim this time, and we won't next
-             * time, so quit */
-            goto out;
-
-found:
-            lr = p_r->lr;
-            p_r->lr = GIC_INVALID_LR;
-            set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
-            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
-            gic_add_to_lr_pending(v, p_r);
-            inflight_r = &p_r->inflight;
-        }
-
-        gic_set_lr(lr, p, GICH_LR_PENDING);
-        list_del_init(&p->lr_queue);
-        set_bit(lr, &this_cpu(lr_mask));
-
-        /* We can only evict nr_lrs entries */
-        lrs--;
-        if ( lrs == 0 )
-            break;
-    }
-
-out:
-    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
-}
-
-void gic_clear_pending_irqs(struct vcpu *v)
-{
-    struct pending_irq *p, *t;
-
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
-
-    v->arch.lr_mask = 0;
-    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
-        gic_remove_from_lr_pending(v, p);
-}
-
-int gic_events_need_delivery(void)
-{
-    struct vcpu *v = current;
-    struct pending_irq *p;
-    unsigned long flags;
-    const unsigned long apr = gic_hw_ops->read_apr(0);
-    int mask_priority;
-    int active_priority;
-    int rc = 0;
-
-    mask_priority = gic_hw_ops->read_vmcr_priority();
-    active_priority = find_next_bit(&apr, 32, 0);
-
-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
-
-    /* TODO: We order the guest irqs by priority, but we don't change
-     * the priority of host irqs. */
-
-    /* find the first enabled non-active irq, the queue is already
-     * ordered by priority */
-    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
-    {
-        if ( GIC_PRI_TO_GUEST(p->priority) >= mask_priority )
-            goto out;
-        if ( GIC_PRI_TO_GUEST(p->priority) >= active_priority )
-            goto out;
-        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
-        {
-            rc = 1;
-            goto out;
-        }
-    }
-
-out:
-    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
-    return rc;
-}
-
-void gic_inject(void)
-{
-    ASSERT(!local_irq_is_enabled());
-
-    gic_restore_pending_irqs(current);
-
-    if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
-        gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
-}
-
 static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
 {
     /* Lower the priority */
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 3/8] ARM: VGIC: split up gic_dump_info() to cover virtual part separately

[Andre Przywara]

Currently gic_dump_info() not only dumps the hardware state of the GIC,
but also the VGIC internal virtual IRQ lists.
Split the latter off and move it into gic-vgic.c to observe the abstraction.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/domain.c     |  1 +
 xen/arch/arm/gic-vgic.c   | 11 +++++++++++
 xen/arch/arm/gic.c        | 12 ------------
 xen/include/asm-arm/gic.h |  1 +
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index a74ff1c07c..8d7f1b7138 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -941,6 +941,7 @@ long arch_do_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
 void arch_dump_vcpu_info(struct vcpu *v)
 {
     gic_dump_info(v);
+    gic_dump_vgic_info(v);
 }
 
 void vcpu_mark_events_pending(struct vcpu *v)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 98df84e3d5..0a2958c5db 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -400,6 +400,17 @@ void gic_inject(void)
         gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
 }
 
+void gic_dump_vgic_info(struct vcpu *v)
+{
+    struct pending_irq *p;
+
+    list_for_each_entry ( p, &v->arch.vgic.inflight_irqs, inflight )
+        printk("Inflight irq=%u lr=%u\n", p->irq, p->lr);
+
+    list_for_each_entry( p, &v->arch.vgic.lr_pending, lr_queue )
+        printk("Pending irq=%d\n", p->irq);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 04e6d66b69..4cb74d449e 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -443,20 +443,8 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
 
 void gic_dump_info(struct vcpu *v)
 {
-    struct pending_irq *p;
-
     printk("GICH_LRs (vcpu %d) mask=%"PRIx64"\n", v->vcpu_id, v->arch.lr_mask);
     gic_hw_ops->dump_state(v);
-
-    list_for_each_entry ( p, &v->arch.vgic.inflight_irqs, inflight )
-    {
-        printk("Inflight irq=%u lr=%u\n", p->irq, p->lr);
-    }
-
-    list_for_each_entry( p, &v->arch.vgic.lr_pending, lr_queue )
-    {
-        printk("Pending irq=%d\n", p->irq);
-    }
 }
 
 void init_maintenance_interrupt(void)
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 587a14f8b9..b51b485c20 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -285,6 +285,7 @@ extern void send_SGI_allbutself(enum gic_sgi sgi);
 
 /* print useful debug info */
 extern void gic_dump_info(struct vcpu *v);
+extern void gic_dump_vgic_info(struct vcpu *v);
 
 /* Number of interrupt lines */
 extern unsigned int gic_number_lines(void);
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 4/8] ARM: VGIC: rework events_need_delivery()

[Andre Przywara]

In event.h we very deeply dive into the VGIC to learn if an event for
a guest is pending.
Rework that function to abstract the VGIC specific part out. Also
reorder the queries there, as we only actually need to check for the
event channel if there are no other pending IRQs.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/vgic.c         | 11 +++++++++++
 xen/include/asm-arm/event.h | 13 +++----------
 xen/include/asm-arm/vgic.h  |  2 ++
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 6e933a86d3..9921769b15 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -593,6 +593,17 @@ void arch_evtchn_inject(struct vcpu *v)
     vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
 }
 
+bool vgic_evtchn_irq_pending(struct vcpu *v)
+{
+    struct pending_irq *p;
+
+    p = irq_to_pending(v, v->domain->arch.evtchn_irq);
+    /* Does not work for LPIs. */
+    ASSERT(!is_lpi(v->domain->arch.evtchn_irq));
+
+    return list_empty(&p->inflight);
+}
+
 bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
 {
     struct vcpu *v = current;
diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
index caefa506a9..67684e9763 100644
--- a/xen/include/asm-arm/event.h
+++ b/xen/include/asm-arm/event.h
@@ -16,12 +16,6 @@ static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
 
 static inline int local_events_need_delivery_nomask(void)
 {
-    struct pending_irq *p = irq_to_pending(current,
-                                           current->domain->arch.evtchn_irq);
-
-    /* Does not work for LPIs. */
-    ASSERT(!is_lpi(current->domain->arch.evtchn_irq));
-
     /* XXX: if the first interrupt has already been delivered, we should
      * check whether any other interrupts with priority higher than the
      * one in GICV_IAR are in the lr_pending queue or in the LR
@@ -33,11 +27,10 @@ static inline int local_events_need_delivery_nomask(void)
     if ( gic_events_need_delivery() )
         return 1;
 
-    if ( vcpu_info(current, evtchn_upcall_pending) &&
-        list_empty(&p->inflight) )
-        return 1;
+    if ( !vcpu_info(current, evtchn_upcall_pending) )
+        return 0;
 
-    return 0;
+    return vgic_evtchn_irq_pending(current);
 }
 
 static inline int local_events_need_delivery(void)
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 2a93a7bef9..22c8502c95 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -218,6 +218,8 @@ extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
 int vgic_v2_init(struct domain *d, int *mmio_count);
 int vgic_v3_init(struct domain *d, int *mmio_count);
 
+bool vgic_evtchn_irq_pending(struct vcpu *v);
+
 extern int domain_vgic_register(struct domain *d, int *mmio_count);
 extern int vcpu_vgic_free(struct vcpu *v);
 extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 5/8] ARM: VGIC: factor out vgic_connect_hw_irq()

[Andre Przywara]

At the moment we happily access VGIC internal data structures like
the rank and struct pending_irq in gic.c, which should be VGIC agnostic.

Factor out a new function vgic_connect_hw_irq(), which allows a virtual
IRQ to be connected to a hardware IRQ (using the hw bit in the LR).

This removes said accesses to VGIC data structures and improves abstraction.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/gic-vgic.c    | 31 +++++++++++++++++++++++++++++++
 xen/arch/arm/gic.c         | 42 ++++++------------------------------------
 xen/include/asm-arm/vgic.h |  2 ++
 3 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 0a2958c5db..d44e4dacd3 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -411,6 +411,37 @@ void gic_dump_vgic_info(struct vcpu *v)
         printk("Pending irq=%d\n", p->irq);
 }
 
+int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
+                        struct irq_desc *desc)
+{
+    unsigned long flags;
+    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
+     * route SPIs to guests, it doesn't make any difference. */
+    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
+    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
+    struct pending_irq *p = irq_to_pending(v_target, virq);
+    int ret = 0;
+
+    /* We are taking to rank lock to prevent parallel connections. */
+    vgic_lock_rank(v_target, rank, flags);
+
+    if ( desc )
+    {
+        /* The VIRQ should not be already enabled by the guest */
+        if ( !p->desc &&
+             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+            p->desc = desc;
+        else
+            ret = -EBUSY;
+    }
+    else
+        p->desc = NULL;
+
+    vgic_unlock_rank(v_target, rank, flags);
+
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 4cb74d449e..d46a6d54b3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -128,27 +128,12 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
 int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
                            struct irq_desc *desc, unsigned int priority)
 {
-    unsigned long flags;
-    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
-     * route SPIs to guests, it doesn't make any difference. */
-    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
-    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
-    struct pending_irq *p = irq_to_pending(v_target, virq);
-    int res = -EBUSY;
-
     ASSERT(spin_is_locked(&desc->lock));
     /* Caller has already checked that the IRQ is an SPI */
     ASSERT(virq >= 32);
     ASSERT(virq < vgic_num_irqs(d));
     ASSERT(!is_lpi(virq));
 
-    vgic_lock_rank(v_target, rank, flags);
-
-    if ( p->desc ||
-         /* The VIRQ should not be already enabled by the guest */
-         test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
-        goto out;
-
     desc->handler = gic_hw_ops->gic_guest_irq_type;
     set_bit(_IRQ_GUEST, &desc->status);
 
@@ -156,31 +141,19 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
         gic_set_irq_type(desc, desc->arch.type);
     gic_set_irq_priority(desc, priority);
 
-    p->desc = desc;
-    res = 0;
-
-out:
-    vgic_unlock_rank(v_target, rank, flags);
-
-    return res;
+    return vgic_connect_hw_irq(d, NULL, virq, desc);
 }
 
 /* This function only works with SPIs for now */
 int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
                               struct irq_desc *desc)
 {
-    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
-    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
-    struct pending_irq *p = irq_to_pending(v_target, virq);
-    unsigned long flags;
+    int ret;
 
     ASSERT(spin_is_locked(&desc->lock));
     ASSERT(test_bit(_IRQ_GUEST, &desc->status));
-    ASSERT(p->desc == desc);
     ASSERT(!is_lpi(virq));
 
-    vgic_lock_rank(v_target, rank, flags);
-
     if ( d->is_dying )
     {
         desc->handler->shutdown(desc);
@@ -198,19 +171,16 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
          */
         if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
              !test_bit(_IRQ_DISABLED, &desc->status) )
-        {
-            vgic_unlock_rank(v_target, rank, flags);
             return -EBUSY;
-        }
     }
 
+    ret = vgic_connect_hw_irq(d, NULL, virq, NULL);
+    if ( ret )
+        return ret;
+
     clear_bit(_IRQ_GUEST, &desc->status);
     desc->handler = &no_irq_type;
 
-    p->desc = NULL;
-
-    vgic_unlock_rank(v_target, rank, flags);
-
     return 0;
 }
 
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 22c8502c95..f4240df371 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -219,6 +219,8 @@ int vgic_v2_init(struct domain *d, int *mmio_count);
 int vgic_v3_init(struct domain *d, int *mmio_count);
 
 bool vgic_evtchn_irq_pending(struct vcpu *v);
+int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
+                        struct irq_desc *desc);
 
 extern int domain_vgic_register(struct domain *d, int *mmio_count);
 extern int vcpu_vgic_free(struct vcpu *v);
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 6/8] ARM: VGIC: factor out vgic_get_hw_irq_desc()

[Andre Przywara]

At the moment we happily access the VGIC internal struct pending_irq
(which describes a virtual IRQ) in irq.c.
Factor out the actually needed functionality to learn the associated
hardware IRQ and move that into gic-vgic.c to improve abstraction.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/gic-vgic.c    | 15 +++++++++++++++
 xen/arch/arm/irq.c         |  7 ++-----
 xen/include/asm-arm/vgic.h |  2 ++
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index d44e4dacd3..3ad98dcd3a 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -411,6 +411,21 @@ void gic_dump_vgic_info(struct vcpu *v)
         printk("Pending irq=%d\n", p->irq);
 }
 
+struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
+                                      unsigned int virq)
+{
+    struct pending_irq *p;
+
+    if ( !v )
+        v = d->vcpu[0];
+
+    p = irq_to_pending(v, virq);
+    if ( !p )
+        return NULL;
+
+    return p->desc;
+}
+
 int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
                         struct irq_desc *desc)
 {
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 7f133de549..62103a20e3 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -534,19 +534,16 @@ int release_guest_irq(struct domain *d, unsigned int virq)
     struct irq_desc *desc;
     struct irq_guest *info;
     unsigned long flags;
-    struct pending_irq *p;
     int ret;
 
     /* Only SPIs are supported */
     if ( virq < NR_LOCAL_IRQS || virq >= vgic_num_irqs(d) )
         return -EINVAL;
 
-    p = spi_to_pending(d, virq);
-    if ( !p->desc )
+    desc = vgic_get_hw_irq_desc(d, NULL, virq);
+    if ( !desc )
         return -EINVAL;
 
-    desc = p->desc;
-
     spin_lock_irqsave(&desc->lock, flags);
 
     ret = -EINVAL;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index f4240df371..ebc0cfaee8 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -219,6 +219,8 @@ int vgic_v2_init(struct domain *d, int *mmio_count);
 int vgic_v3_init(struct domain *d, int *mmio_count);
 
 bool vgic_evtchn_irq_pending(struct vcpu *v);
+struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
+                                      unsigned int virq);
 int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
                         struct irq_desc *desc);
 
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 7/8] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

[Andre Przywara]

The functions to actually populate a list register were accessing
the VGIC internal pending_irq struct, although they should be abstracting
from that.
Break the needed information down to remove the reference to pending_irq
from gic-v[23].c.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/gic-v2.c     | 14 +++++++-------
 xen/arch/arm/gic-v3.c     | 12 ++++++------
 xen/arch/arm/gic-vgic.c   |  3 ++-
 xen/include/asm-arm/gic.h |  4 ++--
 xen/include/asm-arm/irq.h |  3 +++
 5 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 511c8d7294..2b271ba322 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -428,8 +428,8 @@ static void gicv2_disable_interface(void)
     spin_unlock(&gicv2.lock);
 }
 
-static void gicv2_update_lr(int lr, const struct pending_irq *p,
-                            unsigned int state)
+static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority,
+                            unsigned int hw_irq, unsigned int state)
 {
     uint32_t lr_reg;
 
@@ -437,12 +437,12 @@ static void gicv2_update_lr(int lr, const struct pending_irq *p,
     BUG_ON(lr < 0);
 
     lr_reg = (((state & GICH_V2_LR_STATE_MASK) << GICH_V2_LR_STATE_SHIFT)  |
-              ((GIC_PRI_TO_GUEST(p->priority) & GICH_V2_LR_PRIORITY_MASK)
-                                             << GICH_V2_LR_PRIORITY_SHIFT) |
-              ((p->irq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT));
+              ((GIC_PRI_TO_GUEST(priority) & GICH_V2_LR_PRIORITY_MASK)
+                                          << GICH_V2_LR_PRIORITY_SHIFT) |
+              ((virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT));
 
-    if ( p->desc != NULL )
-        lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK )
+    if ( hw_irq != INVALID_IRQ )
+        lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK )
                                    << GICH_V2_LR_PHYSICAL_SHIFT);
 
     writel_gich(lr_reg, GICH_LR + lr * 4);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index a0d290b55c..9ecb62b113 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -966,8 +966,8 @@ static void gicv3_disable_interface(void)
     spin_unlock(&gicv3.lock);
 }
 
-static void gicv3_update_lr(int lr, const struct pending_irq *p,
-                            unsigned int state)
+static void gicv3_update_lr(int lr, unsigned int virq, uint8_t priority,
+                            unsigned int hw_irq, unsigned int state)
 {
     uint64_t val = 0;
 
@@ -983,11 +983,11 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p,
     if ( current->domain->arch.vgic.version == GIC_V3 )
         val |= GICH_LR_GRP1;
 
-    val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
-    val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
+    val |= (uint64_t)priority << GICH_LR_PRIORITY_SHIFT;
+    val |= ((uint64_t)virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
 
-   if ( p->desc != NULL )
-       val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK)
+   if ( hw_irq != INVALID_IRQ )
+       val |= GICH_LR_HW | (((uint64_t)hw_irq & GICH_LR_PHYSICAL_MASK)
                            << GICH_LR_PHYSICAL_SHIFT);
 
     gicv3_ich_write_lr(lr, val);
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 3ad98dcd3a..319617babe 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -52,7 +52,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
 
     clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
 
-    gic_hw_ops->update_lr(lr, p, state);
+    gic_hw_ops->update_lr(lr, p->irq, p->priority,
+                          p->desc ? p->desc->irq : INVALID_IRQ, state);
 
     set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
     clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index b51b485c20..30e62c37ec 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -342,8 +342,8 @@ struct gic_hw_operations {
     /* Disable CPU physical and virtual interfaces */
     void (*disable_interface)(void);
     /* Update LR register with state and priority */
-    void (*update_lr)(int lr, const struct pending_irq *pending_irq,
-                      unsigned int state);
+    void (*update_lr)(int lr, unsigned int virq, uint8_t priority,
+                      unsigned int hw_irq, unsigned int state);
     /* Update HCR status register */
     void (*update_hcr_status)(uint32_t flag, bool set);
     /* Clear LR register */
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index abc8f06a13..0d110ecb08 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -31,6 +31,9 @@ struct arch_irq_desc {
 /* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
 #define INVALID_LPI     0
 
+/* This is a spurious interrupt ID which never makes it into the GIC code. */
+#define INVALID_IRQ     1023
+
 extern unsigned int nr_irqs;
 #define nr_static_irqs NR_IRQS
 #define arch_hwdom_irqs(domid) NR_IRQS
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 8/8] ARM: make nr_irqs a constant

[Andre Przywara]

On ARM the maximum number of IRQs is a constant, but we share it being
a variable to match x86. Since we are not supposed to alter it, let's
mark it as "const" to avoid accidental change.

Suggested-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/irq.c        | 2 +-
 xen/include/asm-arm/irq.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 62103a20e3..d229cb6871 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -27,7 +27,7 @@
 #include <asm/gic.h>
 #include <asm/vgic.h>
 
-unsigned int __read_mostly nr_irqs = NR_IRQS;
+const unsigned int __read_mostly nr_irqs = NR_IRQS;
 
 static unsigned int local_irqs_type[NR_LOCAL_IRQS];
 static DEFINE_SPINLOCK(local_irqs_type_lock);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 0d110ecb08..9d55e9b122 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -34,7 +34,7 @@ struct arch_irq_desc {
 /* This is a spurious interrupt ID which never makes it into the GIC code. */
 #define INVALID_IRQ     1023
 
-extern unsigned int nr_irqs;
+extern const unsigned int nr_irqs;
 #define nr_static_irqs NR_IRQS
 #define arch_hwdom_irqs(domid) NR_IRQS
 
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/8] ARM: VGIC: drop unneeded gic_restore_pending_irqs()

[Julien Grall]

Hi Andre,

On 24/01/18 18:10, Andre Przywara wrote:
> In gic_restore_pending_irqs() we push our pending virtual IRQs into the
> list registers. This function is called once from gic_inject(), just
> before we return to the guest, but also in gic_restore_state(), when
> we context-switch a VCPU. Having a closer look it turns out that the
> later call is not needed, since we will always call gic_inject() anyway.
> So remove that call (and the forward declaration) to streamline this
> interface and make separating the GIC from the VGIC world later.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
>   xen/arch/arm/gic.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index bac8ada2bb..721a17a9d7 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -36,8 +36,6 @@
>   #include <asm/vgic.h>
>   #include <asm/acpi.h>
>   
> -static void gic_restore_pending_irqs(struct vcpu *v);
> -
>   static DEFINE_PER_CPU(uint64_t, lr_mask);
>   
>   #define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->info->nr_lrs) - 1))
> @@ -91,8 +89,6 @@ void gic_restore_state(struct vcpu *v)
>       gic_hw_ops->restore_state(v);
>   
>       isb();
> -
> -    gic_restore_pending_irqs(v);
>   }
>   
>   /* desc->irq needs to be disabled before calling this function */
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/8] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation

[Julien Grall]

Hi Andre,

On 24/01/18 18:10, Andre Przywara wrote:
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> new file mode 100644
> index 0000000000..98df84e3d5
> --- /dev/null
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -0,0 +1,410 @@
> +/*
> + * xen/arch/arm/gic-vgic.c
> + *
> + * ARM Generic Interrupt Controller virtualization support
> + *
> + * Tim Deegan <tim@xen.org>
> + * Copyright (c) 2011 Citrix Systems.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/lib.h>
> +#include <xen/init.h>
> +#include <xen/mm.h>
> +#include <xen/irq.h>
> +#include <xen/sched.h>
> +#include <xen/errno.h>
> +#include <xen/softirq.h>
> +#include <xen/list.h>
> +#include <xen/device_tree.h>
> +#include <xen/acpi.h>
> +#include <asm/p2m.h>
> +#include <asm/domain.h>
> +#include <asm/platform.h>
> +#include <asm/device.h>
> +#include <asm/io.h>
> +#include <asm/gic.h>
> +#include <asm/vgic.h>
> +#include <asm/acpi.h>

Do we need all those includes? Likely none of this code cares about DT 
or ACPI.

> +
> +extern uint64_t per_cpu__lr_mask;

Please use DECLARE_PER_CPU(lr_mask). But that should really be defined 
in an header and not an extern in C file.

> +extern const struct gic_hw_operations *gic_hw_ops;

Ditto for the extern.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 5/8] ARM: VGIC: factor out vgic_connect_hw_irq()

[Julien Grall]

Hi Andre,

On 24/01/18 18:10, Andre Przywara wrote:
> At the moment we happily access VGIC internal data structures like
> the rank and struct pending_irq in gic.c, which should be VGIC agnostic.
> 
> Factor out a new function vgic_connect_hw_irq(), which allows a virtual
> IRQ to be connected to a hardware IRQ (using the hw bit in the LR).
> 
> This removes said accesses to VGIC data structures and improves abstraction.

You are modifying the locking of the 2 functions. But I don't see how 
this is safe. Can you explain it?

> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>   xen/arch/arm/gic-vgic.c    | 31 +++++++++++++++++++++++++++++++
>   xen/arch/arm/gic.c         | 42 ++++++------------------------------------
>   xen/include/asm-arm/vgic.h |  2 ++
>   3 files changed, 39 insertions(+), 36 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 0a2958c5db..d44e4dacd3 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -411,6 +411,37 @@ void gic_dump_vgic_info(struct vcpu *v)
>           printk("Pending irq=%d\n", p->irq);
>   }
>   
> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
> +                        struct irq_desc *desc)
> +{
> +    unsigned long flags;
> +    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> +     * route SPIs to guests, it doesn't make any difference. */

Can you fix the coding style of the comment?

> +    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> +    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> +    struct pending_irq *p = irq_to_pending(v_target, virq);
> +    int ret = 0;
> +
> +    /* We are taking to rank lock to prevent parallel connections. */
> +    vgic_lock_rank(v_target, rank, flags);
> +
> +    if ( desc )
> +    {
> +        /* The VIRQ should not be already enabled by the guest */
> +        if ( !p->desc &&
> +             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> +            p->desc = desc;
> +        else
> +            ret = -EBUSY;
> +    }
> +    else
> +        p->desc = NULL;
> +
> +    vgic_unlock_rank(v_target, rank, flags);
> +
> +    return ret;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 4cb74d449e..d46a6d54b3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -128,27 +128,12 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
>   int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>                              struct irq_desc *desc, unsigned int priority)
>   {
> -    unsigned long flags;
> -    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> -     * route SPIs to guests, it doesn't make any difference. */
> -    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> -    struct pending_irq *p = irq_to_pending(v_target, virq);
> -    int res = -EBUSY;
> -
>       ASSERT(spin_is_locked(&desc->lock));
>       /* Caller has already checked that the IRQ is an SPI */
>       ASSERT(virq >= 32);
>       ASSERT(virq < vgic_num_irqs(d));
>       ASSERT(!is_lpi(virq));
>   
> -    vgic_lock_rank(v_target, rank, flags);
> -
> -    if ( p->desc ||
> -         /* The VIRQ should not be already enabled by the guest */
> -         test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> -        goto out;
> -
>       desc->handler = gic_hw_ops->gic_guest_irq_type;
>       set_bit(_IRQ_GUEST, &desc->status);

This looks wrong to me. You don't want to execute any of the code below 
if you are not able to route the pIRQ. For instance because the vIRQ has 
already a desc assigned.

>   
> @@ -156,31 +141,19 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>           gic_set_irq_type(desc, desc->arch.type);
>       gic_set_irq_priority(desc, priority);
>   
> -    p->desc = desc;
> -    res = 0;
> -
> -out:
> -    vgic_unlock_rank(v_target, rank, flags);
> -
> -    return res;
> +    return vgic_connect_hw_irq(d, NULL, virq, desc);
>   }
>   
>   /* This function only works with SPIs for now */
>   int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>                                 struct irq_desc *desc)
>   {
> -    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> -    struct pending_irq *p = irq_to_pending(v_target, virq);
> -    unsigned long flags;
> +    int ret;
>   
>       ASSERT(spin_is_locked(&desc->lock));
>       ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> -    ASSERT(p->desc == desc);

You dropped this assert but I don't see any replacement in the new code? 
We really want to make sure the caller will not do something dumb here 
(like passing a different desc).

>       ASSERT(!is_lpi(virq));
>   
> -    vgic_lock_rank(v_target, rank, flags);
> -
>       if ( d->is_dying )
>       {
>           desc->handler->shutdown(desc);
> @@ -198,19 +171,16 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>            */
>           if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
>                !test_bit(_IRQ_DISABLED, &desc->status) )
> -        {
> -            vgic_unlock_rank(v_target, rank, flags);
>               return -EBUSY;
> -        }
>       }
>   
> +    ret = vgic_connect_hw_irq(d, NULL, virq, NULL);
> +    if ( ret )
> +        return ret;
> +
>       clear_bit(_IRQ_GUEST, &desc->status);
>       desc->handler = &no_irq_type;
>   
> -    p->desc = NULL;
> -
> -    vgic_unlock_rank(v_target, rank, flags);
> -
>       return 0;
>   }
>   
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 22c8502c95..f4240df371 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -219,6 +219,8 @@ int vgic_v2_init(struct domain *d, int *mmio_count);
>   int vgic_v3_init(struct domain *d, int *mmio_count);
>   
>   bool vgic_evtchn_irq_pending(struct vcpu *v);
> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
> +                        struct irq_desc *desc);
>   
>   extern int domain_vgic_register(struct domain *d, int *mmio_count);
>   extern int vcpu_vgic_free(struct vcpu *v);
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Julien Grall]

Hi Andre,

On 24/01/18 18:10, Andre Przywara wrote:
> On ARM the maximum number of IRQs is a constant, but we share it being
> a variable to match x86. Since we are not supposed to alter it, let's
> mark it as "const" to avoid accidental change.
> 
> Suggested-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
>   xen/arch/arm/irq.c        | 2 +-
>   xen/include/asm-arm/irq.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 62103a20e3..d229cb6871 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -27,7 +27,7 @@
>   #include <asm/gic.h>
>   #include <asm/vgic.h>
>   
> -unsigned int __read_mostly nr_irqs = NR_IRQS;
> +const unsigned int __read_mostly nr_irqs = NR_IRQS;
>   
>   static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>   static DEFINE_SPINLOCK(local_irqs_type_lock);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 0d110ecb08..9d55e9b122 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -34,7 +34,7 @@ struct arch_irq_desc {
>   /* This is a spurious interrupt ID which never makes it into the GIC code. */
>   #define INVALID_IRQ     1023
>   
> -extern unsigned int nr_irqs;
> +extern const unsigned int nr_irqs;
>   #define nr_static_irqs NR_IRQS
>   #define arch_hwdom_irqs(domid) NR_IRQS
>   
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Roger Pau Monné]

On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote:
> On ARM the maximum number of IRQs is a constant, but we share it being
> a variable to match x86. Since we are not supposed to alter it, let's
> mark it as "const" to avoid accidental change.
> 
> Suggested-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  xen/arch/arm/irq.c        | 2 +-
>  xen/include/asm-arm/irq.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 62103a20e3..d229cb6871 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -27,7 +27,7 @@
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> -unsigned int __read_mostly nr_irqs = NR_IRQS;
> +const unsigned int __read_mostly nr_irqs = NR_IRQS;

Shouldn't you remove the __read_mostly attribute, so the symbol it's
placed at the .rodata section by the compiler?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/8] ARM: VGIC: drop unneeded gic_restore_pending_irqs()

[Stefano Stabellini]

On Wed, 24 Jan 2018, Andre Przywara wrote:
> In gic_restore_pending_irqs() we push our pending virtual IRQs into the
> list registers. This function is called once from gic_inject(), just
> before we return to the guest, but also in gic_restore_state(), when
> we context-switch a VCPU. Having a closer look it turns out that the
> later call is not needed, since we will always call gic_inject() anyway.
> So remove that call (and the forward declaration) to streamline this
> interface and make separating the GIC from the VGIC world later.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/gic.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index bac8ada2bb..721a17a9d7 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -36,8 +36,6 @@
>  #include <asm/vgic.h>
>  #include <asm/acpi.h>
>  
> -static void gic_restore_pending_irqs(struct vcpu *v);
> -
>  static DEFINE_PER_CPU(uint64_t, lr_mask);
>  
>  #define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->info->nr_lrs) - 1))
> @@ -91,8 +89,6 @@ void gic_restore_state(struct vcpu *v)
>      gic_hw_ops->restore_state(v);
>  
>      isb();
> -
> -    gic_restore_pending_irqs(v);
>  }
>  
>  /* desc->irq needs to be disabled before calling this function */
> -- 
> 2.14.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 5/8] ARM: VGIC: factor out vgic_connect_hw_irq()

[Andre Przywara]

Hi,

Yeah! Locking discussions! Have fun below ;-)

On 30/01/18 13:19, Julien Grall wrote:
> Hi Andre,
> 
> On 24/01/18 18:10, Andre Przywara wrote:
>> At the moment we happily access VGIC internal data structures like
>> the rank and struct pending_irq in gic.c, which should be VGIC agnostic.
>>
>> Factor out a new function vgic_connect_hw_irq(), which allows a virtual
>> IRQ to be connected to a hardware IRQ (using the hw bit in the LR).
>>
>> This removes said accesses to VGIC data structures and improves
>> abstraction.
> 
> You are modifying the locking of the 2 functions. But I don't see how
> this is safe. Can you explain it?

Are you worried about anything particular? I will explain my reasoning
below, but feel free to point me to the cause of your gripes.

>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>> ---
>>   xen/arch/arm/gic-vgic.c    | 31 +++++++++++++++++++++++++++++++
>>   xen/arch/arm/gic.c         | 42
>> ++++++------------------------------------
>>   xen/include/asm-arm/vgic.h |  2 ++
>>   3 files changed, 39 insertions(+), 36 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index 0a2958c5db..d44e4dacd3 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -411,6 +411,37 @@ void gic_dump_vgic_info(struct vcpu *v)
>>           printk("Pending irq=%d\n", p->irq);
>>   }
>>   +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned
>> int virq,
>> +                        struct irq_desc *desc)
>> +{
>> +    unsigned long flags;
>> +    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
>> +     * route SPIs to guests, it doesn't make any difference. */
> 
> Can you fix the coding style of the comment?

Sure, I wasn't sure if the original style was on purpose.

>> +    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>> +    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
>> +    struct pending_irq *p = irq_to_pending(v_target, virq);
>> +    int ret = 0;
>> +
>> +    /* We are taking to rank lock to prevent parallel connections. */
>> +    vgic_lock_rank(v_target, rank, flags);

So the rank lock here protects against assigning different (or the
same?) hardware IRQs to some particular virtual IRQ. The rank lock is
not strictly correct, but a not-too-bad replacement for the missing
per-IRQ lock. To this degree it protects reading and assigning p->desc
in struct pending_irq.

>> +
>> +    if ( desc )
>> +    {
>> +        /* The VIRQ should not be already enabled by the guest */
>> +        if ( !p->desc &&
>> +             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>> +            p->desc = desc;
>> +        else
>> +            ret = -EBUSY;
>> +    }
>> +    else
>> +        p->desc = NULL;
>> +
>> +    vgic_unlock_rank(v_target, rank, flags);
>> +
>> +    return ret;
>> +}
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 4cb74d449e..d46a6d54b3 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -128,27 +128,12 @@ void gic_route_irq_to_xen(struct irq_desc *desc,
>> unsigned int priority)
>>   int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>>                              struct irq_desc *desc, unsigned int
>> priority)
>>   {
>> -    unsigned long flags;
>> -    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
>> -     * route SPIs to guests, it doesn't make any difference. */
>> -    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>> -    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
>> -    struct pending_irq *p = irq_to_pending(v_target, virq);
>> -    int res = -EBUSY;
>> -
>>       ASSERT(spin_is_locked(&desc->lock));
>>       /* Caller has already checked that the IRQ is an SPI */
>>       ASSERT(virq >= 32);
>>       ASSERT(virq < vgic_num_irqs(d));
>>       ASSERT(!is_lpi(virq));
>>   -    vgic_lock_rank(v_target, rank, flags);
>> -
>> -    if ( p->desc ||
>> -         /* The VIRQ should not be already enabled by the guest */
>> -         test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>> -        goto out;
>> -
>>       desc->handler = gic_hw_ops->gic_guest_irq_type;

Assigning desc->handler is protected by the desc lock, which we check is
held above. This is independent from the virtual IRQ locking. This is
only leaving aside the issue you correctly point out below ...

>>       set_bit(_IRQ_GUEST, &desc->status);
> 
> This looks wrong to me. You don't want to execute any of the code below
> if you are not able to route the pIRQ. For instance because the vIRQ has
> already a desc assigned.

Ah, good point. Indeed I didn't consider the failure path. Should be
easily fixed, though. Thanks for catching this.

>>   @@ -156,31 +141,19 @@ int gic_route_irq_to_guest(struct domain *d,
>> unsigned int virq,
>>           gic_set_irq_type(desc, desc->arch.type);
>>       gic_set_irq_priority(desc, priority);
>>   -    p->desc = desc;
>> -    res = 0;
>> -
>> -out:
>> -    vgic_unlock_rank(v_target, rank, flags);
>> -
>> -    return res;
>> +    return vgic_connect_hw_irq(d, NULL, virq, desc);
>>   }
>>     /* This function only works with SPIs for now */
>>   int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>>                                 struct irq_desc *desc)
>>   {
>> -    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>> -    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
>> -    struct pending_irq *p = irq_to_pending(v_target, virq);
>> -    unsigned long flags;
>> +    int ret;
>>         ASSERT(spin_is_locked(&desc->lock));
>>       ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>> -    ASSERT(p->desc == desc);
> 
> You dropped this assert but I don't see any replacement in the new code?
> We really want to make sure the caller will not do something dumb here
> (like passing a different desc).

So the first thing here is that I can't have anything dereferencing
struct pending_irq here. Secondly the rank lock (protecting the p->
structure) is only taken below, so nothing prevents this from changing
between the ASSERT and the lock, AFAICS.
And to be honest, I don't really get the purpose of this ASSERT: the
desc pointer is taken from the pending_irq in the caller, but without
any locks. So if I am not mistaken, it could race with a
gic_route_irq_to_xen(), and that would lead to the ASSERT triggering,
just because of this race and not because of the code being broken
ultimately.
I *could* get the irq_desc by calling the new vgic_get_hw_irq_desc() -
again. Not sure if that is useful, though.
Another possibility would be to rethink this whole functionality:
The only caller (release_guest_irq() in irq.c) gets a virtual IRQ
number, then finds the associated irq_desc, only to lock it. Then it
passes both the virtual IRQ number and the irq_desc to this function,
where both are rechecked. The reason for this redundancy seems to be
some locking order (irq_desc first?), however I can't find any
documentation about this.
So I wonder if we could just pass on only the virtual IRQ number, and
let it up to this function here to safely retrieve the right irq_desc.

>>       ASSERT(!is_lpi(virq));
>>   -    vgic_lock_rank(v_target, rank, flags);

I couldn't find what this lock protects here, so early at least. Until
the actual "p->desc = NULL;" line below nothing needs to be protected by
this lock, it's all already covered by the desc lock.
We only need the lock to eventually atomically remove the connection
between the h/w and the virtual IRQ, which is done in
vgic_connect_hw_irq() now.

Cheers,
Andre.

>> -
>>       if ( d->is_dying )
>>       {
>>           desc->handler->shutdown(desc);
>> @@ -198,19 +171,16 @@ int gic_remove_irq_from_guest(struct domain *d,
>> unsigned int virq,
>>            */
>>           if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
>>                !test_bit(_IRQ_DISABLED, &desc->status) )
>> -        {
>> -            vgic_unlock_rank(v_target, rank, flags);
>>               return -EBUSY;
>> -        }
>>       }
>>   +    ret = vgic_connect_hw_irq(d, NULL, virq, NULL);
>> +    if ( ret )
>> +        return ret;
>> +
>>       clear_bit(_IRQ_GUEST, &desc->status);
>>       desc->handler = &no_irq_type;
>>   -    p->desc = NULL;
>> -
>> -    vgic_unlock_rank(v_target, rank, flags);
>> -
>>       return 0;
>>   }
>>   diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index 22c8502c95..f4240df371 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -219,6 +219,8 @@ int vgic_v2_init(struct domain *d, int *mmio_count);
>>   int vgic_v3_init(struct domain *d, int *mmio_count);
>>     bool vgic_evtchn_irq_pending(struct vcpu *v);
>> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned
>> int virq,
>> +                        struct irq_desc *desc);
>>     extern int domain_vgic_register(struct domain *d, int *mmio_count);
>>   extern int vcpu_vgic_free(struct vcpu *v);
>>
> 
> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 6/8] ARM: VGIC: factor out vgic_get_hw_irq_desc()

[Julien Grall]

Hi,

On 24/01/18 18:10, Andre Przywara wrote:
> At the moment we happily access the VGIC internal struct pending_irq
> (which describes a virtual IRQ) in irq.c.
> Factor out the actually needed functionality to learn the associated
> hardware IRQ and move that into gic-vgic.c to improve abstraction.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>   xen/arch/arm/gic-vgic.c    | 15 +++++++++++++++
>   xen/arch/arm/irq.c         |  7 ++-----
>   xen/include/asm-arm/vgic.h |  2 ++
>   3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index d44e4dacd3..3ad98dcd3a 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -411,6 +411,21 @@ void gic_dump_vgic_info(struct vcpu *v)
>           printk("Pending irq=%d\n", p->irq);
>   }
>   
> +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v,
> +                                      unsigned int virq)
> +{
> +    struct pending_irq *p;
> +
> +    if ( !v )
> +        v = d->vcpu[0];

I dislike this new function in the current state. Let's imagine someone 
decide to pass a PPI but no vCPU. The vCPU would now be default to vCPU0 
and potentially returning the wrong irq_desc (imagine PPI passthrough 
such as for the vtimer).

So the code needs at least checking the vCPU is passed in the case of a 
PPI. I would be happy with an ASSERT().

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 6/8] ARM: VGIC: factor out vgic_get_hw_irq_desc()

[Andre Przywara]

Hi,

On 31/01/18 16:16, Julien Grall wrote:
> Hi,
> 
> On 24/01/18 18:10, Andre Przywara wrote:
>> At the moment we happily access the VGIC internal struct pending_irq
>> (which describes a virtual IRQ) in irq.c.
>> Factor out the actually needed functionality to learn the associated
>> hardware IRQ and move that into gic-vgic.c to improve abstraction.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>> ---
>>   xen/arch/arm/gic-vgic.c    | 15 +++++++++++++++
>>   xen/arch/arm/irq.c         |  7 ++-----
>>   xen/include/asm-arm/vgic.h |  2 ++
>>   3 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index d44e4dacd3..3ad98dcd3a 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -411,6 +411,21 @@ void gic_dump_vgic_info(struct vcpu *v)
>>           printk("Pending irq=%d\n", p->irq);
>>   }
>>   +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu
>> *v,
>> +                                      unsigned int virq)
>> +{
>> +    struct pending_irq *p;
>> +
>> +    if ( !v )
>> +        v = d->vcpu[0];
> 
> I dislike this new function in the current state. Let's imagine someone
> decide to pass a PPI but no vCPU. The vCPU would now be default to vCPU0
> and potentially returning the wrong irq_desc (imagine PPI passthrough
> such as for the vtimer).
> 
> So the code needs at least checking the vCPU is passed in the case of a
> PPI. I would be happy with an ASSERT().

Yeah, good point. Something like ASSERT(!v && virq >= 32), I guess?

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 6/8] ARM: VGIC: factor out vgic_get_hw_irq_desc()

[Julien Grall]

On 31/01/18 16:24, Andre Przywara wrote:
> Hi,
> 
> On 31/01/18 16:16, Julien Grall wrote:
>> Hi,
>>
>> On 24/01/18 18:10, Andre Przywara wrote:
>>> At the moment we happily access the VGIC internal struct pending_irq
>>> (which describes a virtual IRQ) in irq.c.
>>> Factor out the actually needed functionality to learn the associated
>>> hardware IRQ and move that into gic-vgic.c to improve abstraction.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>> ---
>>>    xen/arch/arm/gic-vgic.c    | 15 +++++++++++++++
>>>    xen/arch/arm/irq.c         |  7 ++-----
>>>    xen/include/asm-arm/vgic.h |  2 ++
>>>    3 files changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>>> index d44e4dacd3..3ad98dcd3a 100644
>>> --- a/xen/arch/arm/gic-vgic.c
>>> +++ b/xen/arch/arm/gic-vgic.c
>>> @@ -411,6 +411,21 @@ void gic_dump_vgic_info(struct vcpu *v)
>>>            printk("Pending irq=%d\n", p->irq);
>>>    }
>>>    +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu
>>> *v,
>>> +                                      unsigned int virq)
>>> +{
>>> +    struct pending_irq *p;
>>> +
>>> +    if ( !v )
>>> +        v = d->vcpu[0];
>>
>> I dislike this new function in the current state. Let's imagine someone
>> decide to pass a PPI but no vCPU. The vCPU would now be default to vCPU0
>> and potentially returning the wrong irq_desc (imagine PPI passthrough
>> such as for the vtimer).
>>
>> So the code needs at least checking the vCPU is passed in the case of a
>> PPI. I would be happy with an ASSERT().
> 
> Yeah, good point. Something like ASSERT(!v && virq >= 32), I guess?

Yes. It should be enough.

Cheers,


> Cheers,
> Andre.
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 5/8] ARM: VGIC: factor out vgic_connect_hw_irq()

[Julien Grall]

On 31/01/18 15:54, Andre Przywara wrote:
> Hi,
> 
> Yeah! Locking discussions! Have fun below ;-)
> 
> On 30/01/18 13:19, Julien Grall wrote:
>> Hi Andre,
>>
>> On 24/01/18 18:10, Andre Przywara wrote:
>>> At the moment we happily access VGIC internal data structures like
>>> the rank and struct pending_irq in gic.c, which should be VGIC agnostic.
>>>
>>> Factor out a new function vgic_connect_hw_irq(), which allows a virtual
>>> IRQ to be connected to a hardware IRQ (using the hw bit in the LR).
>>>
>>> This removes said accesses to VGIC data structures and improves
>>> abstraction.
>>
>> You are modifying the locking of the 2 functions. But I don't see how
>> this is safe. Can you explain it?
> 
> Are you worried about anything particular? I will explain my reasoning
> below, but feel free to point me to the cause of your gripes.

In general, it is quite nice to explain roughly in the commit message 
why the new locking order is ok. It would avoid reviewers to spend time 
guessing why it is fine.

In that particular case I am concerned about any potential concurrent 
access on anything related to a vIRQ.

[...]

>>>        set_bit(_IRQ_GUEST, &desc->status);
>>
>> This looks wrong to me. You don't want to execute any of the code below
>> if you are not able to route the pIRQ. For instance because the vIRQ has
>> already a desc assigned.
> 
> Ah, good point. Indeed I didn't consider the failure path. Should be
> easily fixed, though. Thanks for catching this.
> 
>>>    @@ -156,31 +141,19 @@ int gic_route_irq_to_guest(struct domain *d,
>>> unsigned int virq,
>>>            gic_set_irq_type(desc, desc->arch.type);
>>>        gic_set_irq_priority(desc, priority);
>>>    -    p->desc = desc;
>>> -    res = 0;
>>> -
>>> -out:
>>> -    vgic_unlock_rank(v_target, rank, flags);
>>> -
>>> -    return res;
>>> +    return vgic_connect_hw_irq(d, NULL, virq, desc);
>>>    }
>>>      /* This function only works with SPIs for now */
>>>    int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>>>                                  struct irq_desc *desc)
>>>    {
>>> -    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>>> -    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
>>> -    struct pending_irq *p = irq_to_pending(v_target, virq);
>>> -    unsigned long flags;
>>> +    int ret;
>>>          ASSERT(spin_is_locked(&desc->lock));
>>>        ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>>> -    ASSERT(p->desc == desc);
>>
>> You dropped this assert but I don't see any replacement in the new code?
>> We really want to make sure the caller will not do something dumb here
>> (like passing a different desc).
> 
> So the first thing here is that I can't have anything dereferencing
> struct pending_irq here. Secondly the rank lock (protecting the p->
> structure) is only taken below, so nothing prevents this from changing
> between the ASSERT and the lock, AFAICS.

You could move the ASSERT within the lock, right?

> And to be honest, I don't really get the purpose of this ASSERT: the
> desc pointer is taken from the pending_irq in the caller, but without
> any locks. So if I am not mistaken, it could race with a
> gic_route_irq_to_xen(), and that would lead to the ASSERT triggering,
> just because of this race and not because of the code being broken
> ultimately.
> I *could* get the irq_desc by calling the new vgic_get_hw_irq_desc() -
> again. Not sure if that is useful, though.
> Another possibility would be to rethink this whole functionality:
> The only caller (release_guest_irq() in irq.c) gets a virtual IRQ
> number, then finds the associated irq_desc, only to lock it. Then it
> passes both the virtual IRQ number and the irq_desc to this function,
> where both are rechecked. The reason for this redundancy seems to be
> some locking order (irq_desc first?), however I can't find any
> documentation about this.
> So I wonder if we could just pass on only the virtual IRQ number, and
> let it up to this function here to safely retrieve the right irq_desc.

While I agree that the ASSERT without any lock is dangerous, it could at 
least catch someone passing the wrong irq_desc. Something will really go 
wrong if you disable pIRQ A but the irq_desc was belonging to pIRQ B.

And I agree that the code does not prevent that today. But it at least 
limit the scope of the problem.

So I think the code should be:

gic_remove_irq_from_guest(.....)
{

    vgic_lock_rank(...)
    if ( p->desc != desc )
    {
	vgic_unlock_rank(...)
         return -1;
    }

    do the vGIC removal

    vgic_unlock_rank(...)

    return 0;
}

> 
>>>        ASSERT(!is_lpi(virq));
>>>    -    vgic_lock_rank(v_target, rank, flags);
> 
> I couldn't find what this lock protects here, so early at least. Until
> the actual "p->desc = NULL;" line below nothing needs to be protected by
> this lock, it's all already covered by the desc lock.
> We only need the lock to eventually atomically remove the connection
> between the h/w and the virtual IRQ, which is done in
> vgic_connect_hw_irq() now.

See above.

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 5/8] ARM: VGIC: factor out vgic_connect_hw_irq()

[Andre Przywara]

Hi,

On 31/01/18 16:30, Julien Grall wrote:
> 
> 
> On 31/01/18 15:54, Andre Przywara wrote:
>> Hi,
>>
>> Yeah! Locking discussions! Have fun below ;-)
>>
>> On 30/01/18 13:19, Julien Grall wrote:
>>> Hi Andre,
>>>
>>> On 24/01/18 18:10, Andre Przywara wrote:
>>>> At the moment we happily access VGIC internal data structures like
>>>> the rank and struct pending_irq in gic.c, which should be VGIC
>>>> agnostic.
>>>>
>>>> Factor out a new function vgic_connect_hw_irq(), which allows a virtual
>>>> IRQ to be connected to a hardware IRQ (using the hw bit in the LR).
>>>>
>>>> This removes said accesses to VGIC data structures and improves
>>>> abstraction.
>>>
>>> You are modifying the locking of the 2 functions. But I don't see how
>>> this is safe. Can you explain it?
>>
>> Are you worried about anything particular? I will explain my reasoning
>> below, but feel free to point me to the cause of your gripes.
> 
> In general, it is quite nice to explain roughly in the commit message
> why the new locking order is ok. It would avoid reviewers to spend time
> guessing why it is fine.
> 
> In that particular case I am concerned about any potential concurrent
> access on anything related to a vIRQ.
> 
> [...]
> 
>>>>        set_bit(_IRQ_GUEST, &desc->status);
>>>
>>> This looks wrong to me. You don't want to execute any of the code below
>>> if you are not able to route the pIRQ. For instance because the vIRQ has
>>> already a desc assigned.
>>
>> Ah, good point. Indeed I didn't consider the failure path. Should be
>> easily fixed, though. Thanks for catching this.
>>
>>>>    @@ -156,31 +141,19 @@ int gic_route_irq_to_guest(struct domain *d,
>>>> unsigned int virq,
>>>>            gic_set_irq_type(desc, desc->arch.type);
>>>>        gic_set_irq_priority(desc, priority);
>>>>    -    p->desc = desc;
>>>> -    res = 0;
>>>> -
>>>> -out:
>>>> -    vgic_unlock_rank(v_target, rank, flags);
>>>> -
>>>> -    return res;
>>>> +    return vgic_connect_hw_irq(d, NULL, virq, desc);
>>>>    }
>>>>      /* This function only works with SPIs for now */
>>>>    int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>>>>                                  struct irq_desc *desc)
>>>>    {
>>>> -    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>>>> -    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
>>>> -    struct pending_irq *p = irq_to_pending(v_target, virq);
>>>> -    unsigned long flags;
>>>> +    int ret;
>>>>          ASSERT(spin_is_locked(&desc->lock));
>>>>        ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>>>> -    ASSERT(p->desc == desc);
>>>
>>> You dropped this assert but I don't see any replacement in the new code?
>>> We really want to make sure the caller will not do something dumb here
>>> (like passing a different desc).
>>
>> So the first thing here is that I can't have anything dereferencing
>> struct pending_irq here. Secondly the rank lock (protecting the p->
>> structure) is only taken below, so nothing prevents this from changing
>> between the ASSERT and the lock, AFAICS.
> 
> You could move the ASSERT within the lock, right?

Move: yes, ASSERT: no.
My understanding is that an ASSERT detects logical coding errors, where
a functions passes an invalid argument. Hopefully we trigger this code
path while running a debug build during testing, and can fix this. The
ASSERT stays in place to prevent new users of this function to do wrong
things and to spot regressions.

Now in this particular case I sense a *race* that is not properly
protected, I guess due to locking order. My understanding of the code
flow (please correct me if I am wrong here):
- Someone calls release_guest_irq(), passing in a domain and a *virtual*
IRQ number.
- release_guest_irq() looks up the associated irq_desc (no vIRQ locks
involved!), does some sanity checks, locks that *desc* structure and
calls  gic_remove_irq_from_guest().
- gic_remove_irq_from_guest() now locks that virtual IRQ (somewhat)
using the rank lock. That should have happened before, but I _guess_
that would violate the locking order (desc first, then rank).
- Now it checks whether the virtual IRQ and the passed irq_desc (still)
match. However an ASSERT is not the right way of doing this, since the
caller logic is correct. BUT there could be a race, because the virtual
IRQ could have changed between the original irq_desc lookup in the
caller and the rank lock being taken here. Maybe not in practise due to
subtle (and fragile?) other conditions, but by just looking at the
functions.

>> And to be honest, I don't really get the purpose of this ASSERT: the
>> desc pointer is taken from the pending_irq in the caller, but without
>> any locks. So if I am not mistaken, it could race with a
>> gic_route_irq_to_xen(), and that would lead to the ASSERT triggering,
>> just because of this race and not because of the code being broken
>> ultimately.
>> I *could* get the irq_desc by calling the new vgic_get_hw_irq_desc() -
>> again. Not sure if that is useful, though.
>> Another possibility would be to rethink this whole functionality:
>> The only caller (release_guest_irq() in irq.c) gets a virtual IRQ
>> number, then finds the associated irq_desc, only to lock it. Then it
>> passes both the virtual IRQ number and the irq_desc to this function,
>> where both are rechecked. The reason for this redundancy seems to be
>> some locking order (irq_desc first?), however I can't find any
>> documentation about this.
>> So I wonder if we could just pass on only the virtual IRQ number, and
>> let it up to this function here to safely retrieve the right irq_desc.
> 
> While I agree that the ASSERT without any lock is dangerous, it could at
> least catch someone passing the wrong irq_desc. Something will really go
> wrong if you disable pIRQ A but the irq_desc was belonging to pIRQ B.
> 
> And I agree that the code does not prevent that today. But it at least
> limit the scope of the problem.
> 
> So I think the code should be:
> 
> gic_remove_irq_from_guest(.....)
> {
> 
>    vgic_lock_rank(...)
>    if ( p->desc != desc )
>    {
>     vgic_unlock_rank(...)
>         return -1;
>    }
> 
>    do the vGIC removal
> 
>    vgic_unlock_rank(...)
> 
>    return 0;
> }

Yeah, that makes some sense. Only problem is that series tries to remove
any dependency on the VGIC, namely p->desc and the rank lock, here.
So p->desc would translate into vgic_get_hw_irq_desc(), but the lock
can't be used anymore, at least not for whole of this function.
I need to think about a solution for this.

Cheers,
Andre.

>>>>        ASSERT(!is_lpi(virq));
>>>>    -    vgic_lock_rank(v_target, rank, flags);
>>
>> I couldn't find what this lock protects here, so early at least. Until
>> the actual "p->desc = NULL;" line below nothing needs to be protected by
>> this lock, it's all already covered by the desc lock.
>> We only need the lock to eventually atomically remove the connection
>> between the h/w and the virtual IRQ, which is done in
>> vgic_connect_hw_irq() now.
> 
> See above.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Andre Przywara]

Hi,

On 30/01/18 14:36, Roger Pau Monné wrote:
> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote:
>> On ARM the maximum number of IRQs is a constant, but we share it being
>> a variable to match x86. Since we are not supposed to alter it, let's
>> mark it as "const" to avoid accidental change.
>>
>> Suggested-by: Julien Grall <julien.grall@linaro.org>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>  xen/arch/arm/irq.c        | 2 +-
>>  xen/include/asm-arm/irq.h | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index 62103a20e3..d229cb6871 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -27,7 +27,7 @@
>>  #include <asm/gic.h>
>>  #include <asm/vgic.h>
>>  
>> -unsigned int __read_mostly nr_irqs = NR_IRQS;
>> +const unsigned int __read_mostly nr_irqs = NR_IRQS;
> 
> Shouldn't you remove the __read_mostly attribute, so the symbol it's
> placed at the .rodata section by the compiler?

Yes, makes sense, thanks for pointing this out!
const ... __read_mostly sounds somewhat redundant.

It looks like the compiler does the right thing anyway, as I can't find
nr_irqs in the ELF in any case. Both with and without __read_mostly it
results into the very same binary, actually even without the const.
But I will include the change anyway.

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Julien Grall]

Hi Andre,

On 01/02/18 13:43, Andre Przywara wrote:
> On 30/01/18 14:36, Roger Pau Monné wrote:
>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote:
>>> On ARM the maximum number of IRQs is a constant, but we share it being
>>> a variable to match x86. Since we are not supposed to alter it, let's
>>> mark it as "const" to avoid accidental change.
>>>
>>> Suggested-by: Julien Grall <julien.grall@linaro.org>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> ---
>>>   xen/arch/arm/irq.c        | 2 +-
>>>   xen/include/asm-arm/irq.h | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>> index 62103a20e3..d229cb6871 100644
>>> --- a/xen/arch/arm/irq.c
>>> +++ b/xen/arch/arm/irq.c
>>> @@ -27,7 +27,7 @@
>>>   #include <asm/gic.h>
>>>   #include <asm/vgic.h>
>>>   
>>> -unsigned int __read_mostly nr_irqs = NR_IRQS;
>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS;
>>
>> Shouldn't you remove the __read_mostly attribute, so the symbol it's
>> placed at the .rodata section by the compiler?
> 
> Yes, makes sense, thanks for pointing this out!
> const ... __read_mostly sounds somewhat redundant.
> 
> It looks like the compiler does the right thing anyway, as I can't find
> nr_irqs in the ELF in any case. Both with and without __read_mostly it
> results into the very same binary, actually even without the const.

Really? I was expecting const data to be in the section.rodata. Are you 
suggesting this is landing in the section .data instead?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Roger Pau Monné]

On Thu, Feb 01, 2018 at 01:43:09PM +0000, Andre Przywara wrote:
> Hi,
> 
> On 30/01/18 14:36, Roger Pau Monné wrote:
> > On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote:
> >> On ARM the maximum number of IRQs is a constant, but we share it being
> >> a variable to match x86. Since we are not supposed to alter it, let's
> >> mark it as "const" to avoid accidental change.
> >>
> >> Suggested-by: Julien Grall <julien.grall@linaro.org>
> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> >> ---
> >>  xen/arch/arm/irq.c        | 2 +-
> >>  xen/include/asm-arm/irq.h | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> >> index 62103a20e3..d229cb6871 100644
> >> --- a/xen/arch/arm/irq.c
> >> +++ b/xen/arch/arm/irq.c
> >> @@ -27,7 +27,7 @@
> >>  #include <asm/gic.h>
> >>  #include <asm/vgic.h>
> >>  
> >> -unsigned int __read_mostly nr_irqs = NR_IRQS;
> >> +const unsigned int __read_mostly nr_irqs = NR_IRQS;
> > 
> > Shouldn't you remove the __read_mostly attribute, so the symbol it's
> > placed at the .rodata section by the compiler?
> 
> Yes, makes sense, thanks for pointing this out!
> const ... __read_mostly sounds somewhat redundant.
> 
> It looks like the compiler does the right thing anyway, as I can't find
> nr_irqs in the ELF in any case. Both with and without __read_mostly it
> results into the very same binary, actually even without the const.
> But I will include the change anyway.

Hm, that's kind of weird. nr_irqs seems to be used in ARM code. How
did you assert that the symbol is not there?

This is what I do on x86:

# nm xen/xen-syms | grep ' nr_irqs$'
ffff82d0804324f0 D nr_irqs

Which matches what I would expect from the x86 build.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Andre Przywara]

Hi,

On 01/02/18 13:47, Julien Grall wrote:
> Hi Andre,
> 
> On 01/02/18 13:43, Andre Przywara wrote:
>> On 30/01/18 14:36, Roger Pau Monné wrote:
>>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote:
>>>> On ARM the maximum number of IRQs is a constant, but we share it being
>>>> a variable to match x86. Since we are not supposed to alter it, let's
>>>> mark it as "const" to avoid accidental change.
>>>>
>>>> Suggested-by: Julien Grall <julien.grall@linaro.org>
>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>>> ---
>>>>   xen/arch/arm/irq.c        | 2 +-
>>>>   xen/include/asm-arm/irq.h | 2 +-
>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>>> index 62103a20e3..d229cb6871 100644
>>>> --- a/xen/arch/arm/irq.c
>>>> +++ b/xen/arch/arm/irq.c
>>>> @@ -27,7 +27,7 @@
>>>>   #include <asm/gic.h>
>>>>   #include <asm/vgic.h>
>>>>   -unsigned int __read_mostly nr_irqs = NR_IRQS;
>>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS;
>>>
>>> Shouldn't you remove the __read_mostly attribute, so the symbol it's
>>> placed at the .rodata section by the compiler?
>>
>> Yes, makes sense, thanks for pointing this out!
>> const ... __read_mostly sounds somewhat redundant.
>>
>> It looks like the compiler does the right thing anyway, as I can't find
>> nr_irqs in the ELF in any case. Both with and without __read_mostly it
>> results into the very same binary, actually even without the const.
> 
> Really? I was expecting const data to be in the section.rodata. Are you
> suggesting this is landing in the section .data instead?

Well, for the local case (arm/irq.c) the compiler just does the right
thing and eliminates the variable completely :

00000000000005dc <request_irq>:
 5dc:   eb1f005f        cmp     x2, xzr
 5e0:   52807fe5        mov     w5, #0x3ff                // #1023
 5e4:   7a451002        ccmp    w0, w5, #0x2, ne
 5e8:   540003e8        b.hi    664 <request_irq+0x88>    // EINVAL

For common/domain.o it is as you guessed: in .data.read_mostly, with or
without this (original) patch. So __read_mostly trumps const.
Removing __read_mostly indeed puts it in .rodata.

Don't know why the resulting xen/xen.axf don't show any difference, but
the respective object files do.

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Julien Grall]

On 01/02/18 14:34, Andre Przywara wrote:
> Hi,

Hi,

> On 01/02/18 13:47, Julien Grall wrote:
>> Hi Andre,
>>
>> On 01/02/18 13:43, Andre Przywara wrote:
>>> On 30/01/18 14:36, Roger Pau Monné wrote:
>>>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote:
>>>>> On ARM the maximum number of IRQs is a constant, but we share it being
>>>>> a variable to match x86. Since we are not supposed to alter it, let's
>>>>> mark it as "const" to avoid accidental change.
>>>>>
>>>>> Suggested-by: Julien Grall <julien.grall@linaro.org>
>>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>>>> ---
>>>>>    xen/arch/arm/irq.c        | 2 +-
>>>>>    xen/include/asm-arm/irq.h | 2 +-
>>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>>>> index 62103a20e3..d229cb6871 100644
>>>>> --- a/xen/arch/arm/irq.c
>>>>> +++ b/xen/arch/arm/irq.c
>>>>> @@ -27,7 +27,7 @@
>>>>>    #include <asm/gic.h>
>>>>>    #include <asm/vgic.h>
>>>>>    -unsigned int __read_mostly nr_irqs = NR_IRQS;
>>>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS;
>>>>
>>>> Shouldn't you remove the __read_mostly attribute, so the symbol it's
>>>> placed at the .rodata section by the compiler?
>>>
>>> Yes, makes sense, thanks for pointing this out!
>>> const ... __read_mostly sounds somewhat redundant.
>>>
>>> It looks like the compiler does the right thing anyway, as I can't find
>>> nr_irqs in the ELF in any case. Both with and without __read_mostly it
>>> results into the very same binary, actually even without the const.
>>
>> Really? I was expecting const data to be in the section.rodata. Are you
>> suggesting this is landing in the section .data instead?
> 
> Well, for the local case (arm/irq.c) the compiler just does the right
> thing and eliminates the variable completely :
> 
> 00000000000005dc <request_irq>:
>   5dc:   eb1f005f        cmp     x2, xzr
>   5e0:   52807fe5        mov     w5, #0x3ff                // #1023
>   5e4:   7a451002        ccmp    w0, w5, #0x2, ne
>   5e8:   540003e8        b.hi    664 <request_irq+0x88>    // EINVAL
> 
> For common/domain.o it is as you guessed: in .data.read_mostly, with or
> without this (original) patch. So __read_mostly trumps const.
> Removing __read_mostly indeed puts it in .rodata.
> 
> Don't know why the resulting xen/xen.axf don't show any difference, but
> the respective object files do.
xen-syms is an ELF binary.
xen is not an ELF.
xen.axf is not built anymore since Xen 4.9.

That might explain why you are not able to find it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Andre Przywara]

Hi,

On 01/02/18 13:57, Roger Pau Monné wrote:
> On Thu, Feb 01, 2018 at 01:43:09PM +0000, Andre Przywara wrote:
>> Hi,
>>
>> On 30/01/18 14:36, Roger Pau Monné wrote:
>>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote:
>>>> On ARM the maximum number of IRQs is a constant, but we share it being
>>>> a variable to match x86. Since we are not supposed to alter it, let's
>>>> mark it as "const" to avoid accidental change.
>>>>
>>>> Suggested-by: Julien Grall <julien.grall@linaro.org>
>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>>> ---
>>>>  xen/arch/arm/irq.c        | 2 +-
>>>>  xen/include/asm-arm/irq.h | 2 +-
>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>>> index 62103a20e3..d229cb6871 100644
>>>> --- a/xen/arch/arm/irq.c
>>>> +++ b/xen/arch/arm/irq.c
>>>> @@ -27,7 +27,7 @@
>>>>  #include <asm/gic.h>
>>>>  #include <asm/vgic.h>
>>>>  
>>>> -unsigned int __read_mostly nr_irqs = NR_IRQS;
>>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS;
>>>
>>> Shouldn't you remove the __read_mostly attribute, so the symbol it's
>>> placed at the .rodata section by the compiler?
>>
>> Yes, makes sense, thanks for pointing this out!
>> const ... __read_mostly sounds somewhat redundant.
>>
>> It looks like the compiler does the right thing anyway, as I can't find
>> nr_irqs in the ELF in any case. Both with and without __read_mostly it
>> results into the very same binary, actually even without the const.
>> But I will include the change anyway.
> 
> Hm, that's kind of weird. nr_irqs seems to be used in ARM code. How
> did you assert that the symbol is not there?

Well, I was looking at xen/xen.axf, which is obviously an artefact from
some experiments last summer, as it didn't get rebuild ;-)
So ignore me, the differences are there in xen-syms. And __read_mostly
overrides const, so it *is* helpful to remove it.

Thanks!
Andre.

> 
> This is what I do on x86:
> 
> # nm xen/xen-syms | grep ' nr_irqs$'
> ffff82d0804324f0 D nr_irqs
> 
> Which matches what I would expect from the x86 build.
> 
> Roger.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 8/8] ARM: make nr_irqs a constant

[Andre Przywara]

Hi,

On 01/02/18 14:39, Julien Grall wrote:
> 
> 
> On 01/02/18 14:34, Andre Przywara wrote:
>> Hi,
> 
> Hi,
> 
>> On 01/02/18 13:47, Julien Grall wrote:
>>> Hi Andre,
>>>
>>> On 01/02/18 13:43, Andre Przywara wrote:
>>>> On 30/01/18 14:36, Roger Pau Monné wrote:
>>>>> On Wed, Jan 24, 2018 at 06:10:58PM +0000, Andre Przywara wrote:
>>>>>> On ARM the maximum number of IRQs is a constant, but we share it
>>>>>> being
>>>>>> a variable to match x86. Since we are not supposed to alter it, let's
>>>>>> mark it as "const" to avoid accidental change.
>>>>>>
>>>>>> Suggested-by: Julien Grall <julien.grall@linaro.org>
>>>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>>>>> ---
>>>>>>    xen/arch/arm/irq.c        | 2 +-
>>>>>>    xen/include/asm-arm/irq.h | 2 +-
>>>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>>>>> index 62103a20e3..d229cb6871 100644
>>>>>> --- a/xen/arch/arm/irq.c
>>>>>> +++ b/xen/arch/arm/irq.c
>>>>>> @@ -27,7 +27,7 @@
>>>>>>    #include <asm/gic.h>
>>>>>>    #include <asm/vgic.h>
>>>>>>    -unsigned int __read_mostly nr_irqs = NR_IRQS;
>>>>>> +const unsigned int __read_mostly nr_irqs = NR_IRQS;
>>>>>
>>>>> Shouldn't you remove the __read_mostly attribute, so the symbol it's
>>>>> placed at the .rodata section by the compiler?
>>>>
>>>> Yes, makes sense, thanks for pointing this out!
>>>> const ... __read_mostly sounds somewhat redundant.
>>>>
>>>> It looks like the compiler does the right thing anyway, as I can't find
>>>> nr_irqs in the ELF in any case. Both with and without __read_mostly it
>>>> results into the very same binary, actually even without the const.
>>>
>>> Really? I was expecting const data to be in the section.rodata. Are you
>>> suggesting this is landing in the section .data instead?
>>
>> Well, for the local case (arm/irq.c) the compiler just does the right
>> thing and eliminates the variable completely :
>>
>> 00000000000005dc <request_irq>:
>>   5dc:   eb1f005f        cmp     x2, xzr
>>   5e0:   52807fe5        mov     w5, #0x3ff                // #1023
>>   5e4:   7a451002        ccmp    w0, w5, #0x2, ne
>>   5e8:   540003e8        b.hi    664 <request_irq+0x88>    // EINVAL
>>
>> For common/domain.o it is as you guessed: in .data.read_mostly, with or
>> without this (original) patch. So __read_mostly trumps const.
>> Removing __read_mostly indeed puts it in .rodata.
>>
>> Don't know why the resulting xen/xen.axf don't show any difference, but
>> the respective object files do.
> xen-syms is an ELF binary.
> xen is not an ELF.
> xen.axf is not built anymore since Xen 4.9.

Indeed, I missed that part. The date stayed always at "Jun  7  2017" ;-)
So on real surprise that it didn't change.

Cheers,
Andre.

> That might explain why you are not able to find it.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel