All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups
@ 2017-12-07 16:14 Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 01/10] ARM: remove unneeded gic.h inclusions Andre Przywara
                   ` (9 more replies)
  0 siblings, 10 replies; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

Hi,

a reworked version of this series, addressing Stefano's comments.
Although I don't fully agree, I decided to drop the patches that Stefano
didn't like (3/12, 5/12, 8/12), instead moved the functions in question
into the new gic-vgic.c file, where they are out of the way for any new
VGIC as well. I also fixed the smaller comments and added the tags so
far.

Let me know how this looks!

Cheers,
Andre

Changelog:
01, 02, 03: add tags
04: unchanged
05: update to include non-moved functions (caused by the dropped patches)
06: rework to keep vgic info dump in gic-vgic.c
07, 08, 09: add tags
10: use newly introduced INVALID_IRQ

================
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.
It starts with some rather innocent patches, reaches its peak with the
heavy patch 5/10, and calms down in the rest of the series again.
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 (10):
  ARM: remove unneeded gic.h inclusions
  ARM: vGIC: fix nr_irq definition
  ARM: VGIC: move gic_remove_irq_from_queues()
  ARM: VGIC: streamline 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

 xen/arch/arm/Makefile                |   1 +
 xen/arch/arm/domain.c                |   2 +
 xen/arch/arm/domain_build.c          |   1 -
 xen/arch/arm/gic-v2.c                |  14 +-
 xen/arch/arm/gic-v3.c                |  12 +-
 xen/arch/arm/gic-vgic.c              | 471 +++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic.c                   | 430 +-------------------------------
 xen/arch/arm/irq.c                   |   9 +-
 xen/arch/arm/p2m.c                   |   1 -
 xen/arch/arm/platforms/vexpress.c    |   1 -
 xen/arch/arm/platforms/xgene-storm.c |   1 -
 xen/arch/arm/time.c                  |   1 -
 xen/arch/arm/traps.c                 |   3 +-
 xen/arch/arm/vgic-v3-its.c           |   4 +-
 xen/arch/arm/vgic.c                  |  22 +-
 xen/arch/arm/vpsci.c                 |   1 -
 xen/arch/arm/vtimer.c                |   1 -
 xen/include/asm-arm/event.h          |  13 +-
 xen/include/asm-arm/gic.h            |   8 +-
 xen/include/asm-arm/irq.h            |   5 +-
 xen/include/asm-arm/vgic.h           |   7 +
 21 files changed, 541 insertions(+), 467 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 01/10] ARM: remove unneeded gic.h inclusions
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 02/10] ARM: vGIC: fix nr_irq definition Andre Przywara
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

gic.h is supposed to hold defines and prototypes for the hardware side
of the GIC interrupt controller. A lot of parts in Xen should not be
bothered with that, as they either only care about the VGIC or use
more generic interfaces.
Remove unneeded inclusions of gic.h from files where they are actually
not needed.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/domain_build.c          | 1 -
 xen/arch/arm/p2m.c                   | 1 -
 xen/arch/arm/platforms/vexpress.c    | 1 -
 xen/arch/arm/platforms/xgene-storm.c | 1 -
 xen/arch/arm/time.c                  | 1 -
 xen/arch/arm/traps.c                 | 1 -
 xen/arch/arm/vpsci.c                 | 1 -
 xen/arch/arm/vtimer.c                | 1 -
 8 files changed, 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c74f4dd69d..51e5c8d9f4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -21,7 +21,6 @@
 #include <asm/setup.h>
 #include <asm/cpufeature.h>
 
-#include <asm/gic.h>
 #include <xen/irq.h>
 #include <xen/grant_table.h>
 #include "kernel.h"
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 417609ede2..7bf34aaa8c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -10,7 +10,6 @@
 #include <xen/xmalloc.h>
 #include <public/vm_event.h>
 #include <asm/flushtlb.h>
-#include <asm/gic.h>
 #include <asm/event.h>
 #include <asm/hardirq.h>
 #include <asm/page.h>
diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c
index 39b6bcc70e..70839d676f 100644
--- a/xen/arch/arm/platforms/vexpress.c
+++ b/xen/arch/arm/platforms/vexpress.c
@@ -22,7 +22,6 @@
 #include <xen/mm.h>
 #include <xen/vmap.h>
 #include <asm/io.h>
-#include <asm/gic.h>
 
 #define DCC_SHIFT      26
 #define FUNCTION_SHIFT 20
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 3b007fe5ed..deb8479a49 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -22,7 +22,6 @@
 #include <xen/vmap.h>
 #include <xen/device_tree.h>
 #include <asm/io.h>
-#include <asm/gic.h>
 
 /* XGENE RESET Specific defines */
 #define XGENE_RESET_ADDR        0x17000014UL
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 105c7410c7..36f640f0c1 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -31,7 +31,6 @@
 #include <xen/acpi.h>
 #include <asm/system.h>
 #include <asm/time.h>
-#include <asm/gic.h>
 #include <asm/vgic.h>
 #include <asm/cpufeature.h>
 #include <asm/platform.h>
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..ff3d6ff2aa 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -43,7 +43,6 @@
 #include <asm/debugger.h>
 #include <asm/event.h>
 #include <asm/flushtlb.h>
-#include <asm/gic.h>
 #include <asm/mmio.h>
 #include <asm/monitor.h>
 #include <asm/psci.h>
diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index 0e024f7578..cd724904ef 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -15,7 +15,6 @@
 #include <xen/types.h>
 
 #include <asm/current.h>
-#include <asm/gic.h>
 #include <asm/vgic.h>
 #include <asm/psci.h>
 #include <asm/event.h>
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 3f84893a74..f52a723a5f 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -24,7 +24,6 @@
 
 #include <asm/cpregs.h>
 #include <asm/div64.h>
-#include <asm/gic.h>
 #include <asm/irq.h>
 #include <asm/regs.h>
 #include <asm/time.h>
-- 
2.14.1


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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 02/10] ARM: vGIC: fix nr_irq definition
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 01/10] ARM: remove unneeded gic.h inclusions Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-08 21:42   ` Julien Grall
  2017-12-07 16:14 ` [PATCH v2 03/10] ARM: VGIC: move gic_remove_irq_from_queues() Andre Przywara
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

The global variable "nr_irqs" is used for x86 and some common Xen code.
To make the latter work easily for ARM, it was #defined to NR_IRQS.
This not only violated the common habit of capitalizing macros, but
also caused issues if one wanted to use a rather innocent "nr_irqs" as
a local variable name or as a function parameter.
Drop the optimization and make nr_irqs a normal variable for ARM also.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/irq.c        | 2 ++
 xen/include/asm-arm/irq.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index cbc7e6ebb8..7f133de549 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -27,6 +27,8 @@
 #include <asm/gic.h>
 #include <asm/vgic.h>
 
+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 2de76d0f56..abc8f06a13 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -31,7 +31,7 @@ struct arch_irq_desc {
 /* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
 #define INVALID_LPI     0
 
-#define nr_irqs NR_IRQS
+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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 03/10] ARM: VGIC: move gic_remove_irq_from_queues()
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 01/10] ARM: remove unneeded gic.h inclusions Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 02/10] ARM: vGIC: fix nr_irq definition Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-08 21:27   ` Stefano Stabellini
  2017-12-07 16:14 ` [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs() Andre Przywara
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

gic_remove_irq_from_queues() was not only misnamed, it also has the wrong
abstraction, as it should not live in gic.c.
Move it into vgic.c and vgic.h, where it belongs to, and rename it on
the way.

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ed363f6c37..bac8ada2bb 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -411,15 +411,6 @@ void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p)
     list_del_init(&p->lr_queue);
 }
 
-void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
-{
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
-
-    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
-    list_del_init(&p->inflight);
-    gic_remove_from_lr_pending(v, p);
-}
-
 void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
 {
     struct pending_irq *n = irq_to_pending(v, virtual_irq);
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 72a5c70656..d8fa44258d 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -381,7 +381,7 @@ static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr)
      * have no active state, we don't need to care about this here.
      */
     if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-        gic_remove_irq_from_queues(vcpu, p);
+        vgic_remove_irq_from_queues(vcpu, p);
 
     spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
     ret = 0;
@@ -619,7 +619,7 @@ static int its_discard_event(struct virt_its *its,
     }
 
     /* Cleanup the pending_irq and disconnect it from the LPI. */
-    gic_remove_irq_from_queues(vcpu, p);
+    vgic_remove_irq_from_queues(vcpu, p);
     vgic_init_pending_irq(p, INVALID_LPI);
 
     spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index d8acbbeaaa..6e933a86d3 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -281,7 +281,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
     if ( !list_empty(&p->lr_queue) )
     {
-        gic_remove_irq_from_queues(old, p);
+        vgic_remove_irq_from_queues(old, p);
         irq_set_affinity(p->desc, cpumask_of(new->processor));
         spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
         vgic_vcpu_inject_irq(new, irq);
@@ -508,6 +508,15 @@ void vgic_clear_pending_irqs(struct vcpu *v)
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
+void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
+{
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+    list_del_init(&p->inflight);
+    gic_remove_from_lr_pending(v, p);
+}
+
 void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
 {
     uint8_t priority;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index d3d7bda50d..587a14f8b9 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -244,7 +244,6 @@ extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
         unsigned int priority);
 extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
 extern void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p);
-extern void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
 
 /* Accept an interrupt from the GIC and dispatch its handler */
 extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index e489d0bf21..2a93a7bef9 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -204,6 +204,7 @@ extern int vcpu_vgic_init(struct vcpu *v);
 extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
 extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
 extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
+extern void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
-- 
2.14.1


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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs()
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
                   ` (2 preceding siblings ...)
  2017-12-07 16:14 ` [PATCH v2 03/10] ARM: VGIC: move gic_remove_irq_from_queues() Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-08 21:40   ` Stefano Stabellini
  2017-12-07 16:14 ` [PATCH v2 05/10] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation Andre Przywara
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

In gic_restore_pending_irqs() we push our pending virtual IRQs into the
list registers. This function is called once from a GIC context and once
from a VGIC context. Refactor the calls so that we have only one callsite
from the VGIC context. This will help separating the two worlds later.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/domain.c     |  1 +
 xen/arch/arm/gic.c        | 11 +++++------
 xen/arch/arm/traps.c      |  2 +-
 xen/include/asm-arm/gic.h |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index a74ff1c07c..73f4d4b2b2 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -185,6 +185,7 @@ static void ctxt_switch_to(struct vcpu *n)
 
     /* VGIC */
     gic_restore_state(n);
+    gic_inject(n);
 
     /* VFP */
     vfp_restore_state(n);
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index bac8ada2bb..1f00654ef5 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 */
@@ -715,11 +711,14 @@ out:
     return rc;
 }
 
-void gic_inject(void)
+void gic_inject(struct vcpu *v)
 {
     ASSERT(!local_irq_is_enabled());
 
-    gic_restore_pending_irqs(current);
+    gic_restore_pending_irqs(v);
+
+    if ( v != current )
+        return;
 
     if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
         gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ff3d6ff2aa..7fd676ed9d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2298,7 +2298,7 @@ void leave_hypervisor_tail(void)
     {
         local_irq_disable();
         if (!softirq_pending(smp_processor_id())) {
-            gic_inject();
+            gic_inject(current);
 
             /*
              * If the SErrors handle option is "DIVERSE", we have to prevent
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 587a14f8b9..28cf16654a 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -235,7 +235,7 @@ extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
 int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
                               struct irq_desc *desc);
 
-extern void gic_inject(void);
+extern void gic_inject(struct vcpu *v);
 extern void gic_clear_pending_irqs(struct vcpu *v);
 extern int gic_events_need_delivery(void);
 
-- 
2.14.1


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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 05/10] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
                   ` (3 preceding siblings ...)
  2017-12-07 16:14 ` [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs() Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-08 21:57   ` Stefano Stabellini
  2017-12-07 16:14 ` [PATCH v2 06/10] ARM: VGIC: split up gic_dump_info() to cover virtual part separately Andre Przywara
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

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 is says on the tin.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/Makefile   |   1 +
 xen/arch/arm/gic-vgic.c | 413 ++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic.c      | 366 +-----------------------------------------
 3 files changed, 416 insertions(+), 364 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..971b3bfe37
--- /dev/null
+++ b/xen/arch/arm/gic-vgic.c
@@ -0,0 +1,413 @@
+/*
+ * 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, 0);
+
+    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(struct vcpu *v)
+{
+    ASSERT(!local_irq_is_enabled());
+
+    gic_restore_pending_irqs(v);
+
+    if ( v != current )
+        return;
+
+    if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
+        gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
+}
+
+/*
+ * 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 1f00654ef5..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,364 +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(struct vcpu *v)
-{
-    ASSERT(!local_irq_is_enabled());
-
-    gic_restore_pending_irqs(v);
-
-    if ( v != current )
-        return;
-
-    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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 06/10] ARM: VGIC: split up gic_dump_info() to cover virtual part separately
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
                   ` (4 preceding siblings ...)
  2017-12-07 16:14 ` [PATCH v2 05/10] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-08 22:03   ` Stefano Stabellini
  2017-12-07 16:14 ` [PATCH v2 07/10] ARM: VGIC: rework events_need_delivery() Andre Przywara
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

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>
---
 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 73f4d4b2b2..5d2943b800 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -942,6 +942,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 971b3bfe37..90b827c574 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -403,6 +403,17 @@ void gic_inject(struct vcpu *v)
         gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
 }
 
+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 28cf16654a..4f4fd555c1 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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 07/10] ARM: VGIC: rework events_need_delivery()
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
                   ` (5 preceding siblings ...)
  2017-12-07 16:14 ` [PATCH v2 06/10] ARM: VGIC: split up gic_dump_info() to cover virtual part separately Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 08/10] ARM: VGIC: factor out vgic_connect_hw_irq() Andre Przywara
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 08/10] ARM: VGIC: factor out vgic_connect_hw_irq()
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
                   ` (6 preceding siblings ...)
  2017-12-07 16:14 ` [PATCH v2 07/10] ARM: VGIC: rework events_need_delivery() Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 09/10] ARM: VGIC: factor out vgic_get_hw_irq_desc() Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 10/10] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq Andre Przywara
  9 siblings, 0 replies; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

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 90b827c574..37f005d99c 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -414,6 +414,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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 09/10] ARM: VGIC: factor out vgic_get_hw_irq_desc()
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
                   ` (7 preceding siblings ...)
  2017-12-07 16:14 ` [PATCH v2 08/10] ARM: VGIC: factor out vgic_connect_hw_irq() Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-07 16:14 ` [PATCH v2 10/10] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq Andre Przywara
  9 siblings, 0 replies; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

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 37f005d99c..8d43a6ba76 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -414,6 +414,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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 10/10] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq
  2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
                   ` (8 preceding siblings ...)
  2017-12-07 16:14 ` [PATCH v2 09/10] ARM: VGIC: factor out vgic_get_hw_irq_desc() Andre Przywara
@ 2017-12-07 16:14 ` Andre Przywara
  2017-12-08 22:05   ` Stefano Stabellini
  9 siblings, 1 reply; 18+ messages in thread
From: Andre Przywara @ 2017-12-07 16:14 UTC (permalink / raw)
  To: Stefano Stabellini, Julien Grall; +Cc: xen-devel

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>
---
 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 473e26111f..ce1e5cad25 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -962,8 +962,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;
 
@@ -979,11 +979,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 8d43a6ba76..60f6498092 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 4f4fd555c1..ce9d1d058a 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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 03/10] ARM: VGIC: move gic_remove_irq_from_queues()
  2017-12-07 16:14 ` [PATCH v2 03/10] ARM: VGIC: move gic_remove_irq_from_queues() Andre Przywara
@ 2017-12-08 21:27   ` Stefano Stabellini
  0 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2017-12-08 21:27 UTC (permalink / raw)
  To: Andre Przywara; +Cc: xen-devel, Stefano Stabellini, Julien Grall

On Thu, 7 Dec 2017, Andre Przywara wrote:
> gic_remove_irq_from_queues() was not only misnamed, it also has the wrong
> abstraction, as it should not live in gic.c.
> Move it into vgic.c and vgic.h, where it belongs to, and rename it on
> the way.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

I committed the first three patches


> ---
>  xen/arch/arm/gic.c         |  9 ---------
>  xen/arch/arm/vgic-v3-its.c |  4 ++--
>  xen/arch/arm/vgic.c        | 11 ++++++++++-
>  xen/include/asm-arm/gic.h  |  1 -
>  xen/include/asm-arm/vgic.h |  1 +
>  5 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ed363f6c37..bac8ada2bb 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -411,15 +411,6 @@ void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p)
>      list_del_init(&p->lr_queue);
>  }
>  
> -void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
> -{
> -    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> -
> -    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> -    list_del_init(&p->inflight);
> -    gic_remove_from_lr_pending(v, p);
> -}
> -
>  void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
>  {
>      struct pending_irq *n = irq_to_pending(v, virtual_irq);
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 72a5c70656..d8fa44258d 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -381,7 +381,7 @@ static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr)
>       * have no active state, we don't need to care about this here.
>       */
>      if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> -        gic_remove_irq_from_queues(vcpu, p);
> +        vgic_remove_irq_from_queues(vcpu, p);
>  
>      spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
>      ret = 0;
> @@ -619,7 +619,7 @@ static int its_discard_event(struct virt_its *its,
>      }
>  
>      /* Cleanup the pending_irq and disconnect it from the LPI. */
> -    gic_remove_irq_from_queues(vcpu, p);
> +    vgic_remove_irq_from_queues(vcpu, p);
>      vgic_init_pending_irq(p, INVALID_LPI);
>  
>      spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index d8acbbeaaa..6e933a86d3 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -281,7 +281,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>      /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
>      if ( !list_empty(&p->lr_queue) )
>      {
> -        gic_remove_irq_from_queues(old, p);
> +        vgic_remove_irq_from_queues(old, p);
>          irq_set_affinity(p->desc, cpumask_of(new->processor));
>          spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>          vgic_vcpu_inject_irq(new, irq);
> @@ -508,6 +508,15 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>  }
>  
> +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
> +{
> +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> +
> +    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> +    list_del_init(&p->inflight);
> +    gic_remove_from_lr_pending(v, p);
> +}
> +
>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>  {
>      uint8_t priority;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index d3d7bda50d..587a14f8b9 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -244,7 +244,6 @@ extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
>          unsigned int priority);
>  extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
>  extern void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p);
> -extern void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
>  
>  /* Accept an interrupt from the GIC and dispatch its handler */
>  extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index e489d0bf21..2a93a7bef9 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -204,6 +204,7 @@ extern int vcpu_vgic_init(struct vcpu *v);
>  extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
>  extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
>  extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
> +extern void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
>  extern void vgic_clear_pending_irqs(struct vcpu *v);
>  extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq);
>  extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
> -- 
> 2.14.1
> 

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs()
  2017-12-07 16:14 ` [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs() Andre Przywara
@ 2017-12-08 21:40   ` Stefano Stabellini
  2018-01-24 17:43     ` Andre Przywara
  0 siblings, 1 reply; 18+ messages in thread
From: Stefano Stabellini @ 2017-12-08 21:40 UTC (permalink / raw)
  To: Andre Przywara; +Cc: xen-devel, Stefano Stabellini, Julien Grall

On Thu, 7 Dec 2017, Andre Przywara wrote:
> In gic_restore_pending_irqs() we push our pending virtual IRQs into the
> list registers. This function is called once from a GIC context and once
> from a VGIC context. Refactor the calls so that we have only one callsite
> from the VGIC context. This will help separating the two worlds later.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  xen/arch/arm/domain.c     |  1 +
>  xen/arch/arm/gic.c        | 11 +++++------
>  xen/arch/arm/traps.c      |  2 +-
>  xen/include/asm-arm/gic.h |  2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index a74ff1c07c..73f4d4b2b2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -185,6 +185,7 @@ static void ctxt_switch_to(struct vcpu *n)
>  
>      /* VGIC */
>      gic_restore_state(n);
> +    gic_inject(n);
>  
>      /* VFP */
>      vfp_restore_state(n);
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index bac8ada2bb..1f00654ef5 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 */
> @@ -715,11 +711,14 @@ out:
>      return rc;
>  }
>  
> -void gic_inject(void)
> +void gic_inject(struct vcpu *v)
>  {
>      ASSERT(!local_irq_is_enabled());
>  
> -    gic_restore_pending_irqs(current);
> +    gic_restore_pending_irqs(v);

This looks suspicious to me: gic_restore_pending_irqs calls gic_set_lr.
It doesn't look like gic_restore_pending_irqs can actually be called for
v != current.

However, I think that we could remove the call to
gic_restore_pending_irqs from gic_restore_state safely, because we can
still rely on gic_inject being called before entering the guest.

There is no need to add a call to gic_inject in ctxt_switch_to, I think.


> +    if ( v != current )
> +        return;
>  
>      if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
>          gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ff3d6ff2aa..7fd676ed9d 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2298,7 +2298,7 @@ void leave_hypervisor_tail(void)
>      {
>          local_irq_disable();
>          if (!softirq_pending(smp_processor_id())) {
> -            gic_inject();
> +            gic_inject(current);
>  
>              /*
>               * If the SErrors handle option is "DIVERSE", we have to prevent
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 587a14f8b9..28cf16654a 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -235,7 +235,7 @@ extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
>  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>                                struct irq_desc *desc);
>  
> -extern void gic_inject(void);
> +extern void gic_inject(struct vcpu *v);
>  extern void gic_clear_pending_irqs(struct vcpu *v);
>  extern int gic_events_need_delivery(void);

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 02/10] ARM: vGIC: fix nr_irq definition
  2017-12-07 16:14 ` [PATCH v2 02/10] ARM: vGIC: fix nr_irq definition Andre Przywara
@ 2017-12-08 21:42   ` Julien Grall
  0 siblings, 0 replies; 18+ messages in thread
From: Julien Grall @ 2017-12-08 21:42 UTC (permalink / raw)
  To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel

Hi,

On 12/07/2017 04:14 PM, Andre Przywara wrote:
> The global variable "nr_irqs" is used for x86 and some common Xen code.
> To make the latter work easily for ARM, it was #defined to NR_IRQS.
> This not only violated the common habit of capitalizing macros, but
> also caused issues if one wanted to use a rather innocent "nr_irqs" as
> a local variable name or as a function parameter.
> Drop the optimization and make nr_irqs a normal variable for ARM also.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Surprisingly the reviewed-by was not carried in the merge. Anyway...

> ---
>   xen/arch/arm/irq.c        | 2 ++
>   xen/include/asm-arm/irq.h | 2 +-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index cbc7e6ebb8..7f133de549 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -27,6 +27,8 @@
>   #include <asm/gic.h>
>   #include <asm/vgic.h>
>   
> +unsigned int __read_mostly nr_irqs = NR_IRQS;

I am ok with turning this to a variable. But this should really be const 
and not let a chance of someone to override it by mistake.

Can you please send a follow-up to fix that.

> +
>   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 2de76d0f56..abc8f06a13 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -31,7 +31,7 @@ struct arch_irq_desc {
>   /* LPIs are always numbered starting at 8192, so 0 is a good invalid case. */
>   #define INVALID_LPI     0
>   
> -#define nr_irqs NR_IRQS
> +extern unsigned int nr_irqs;
>   #define nr_static_irqs NR_IRQS
>   #define arch_hwdom_irqs(domid) NR_IRQS
>   
> 

Cheers

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 05/10] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation
  2017-12-07 16:14 ` [PATCH v2 05/10] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation Andre Przywara
@ 2017-12-08 21:57   ` Stefano Stabellini
  0 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2017-12-08 21:57 UTC (permalink / raw)
  To: Andre Przywara; +Cc: xen-devel, Stefano Stabellini, Julien Grall

On Thu, 7 Dec 2017, Andre Przywara wrote:
> 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 is 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 | 413 ++++++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic.c      | 366 +-----------------------------------------
>  3 files changed, 416 insertions(+), 364 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..971b3bfe37
> --- /dev/null
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -0,0 +1,413 @@
> +/*
> + * 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, 0);
> +
> +    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(struct vcpu *v)
> +{
> +    ASSERT(!local_irq_is_enabled());
> +
> +    gic_restore_pending_irqs(v);
> +
> +    if ( v != current )
> +        return;
> +
> +    if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
> +        gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
> +}
> +
> +/*
> + * 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 1f00654ef5..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,364 +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(struct vcpu *v)
> -{
> -    ASSERT(!local_irq_is_enabled());
> -
> -    gic_restore_pending_irqs(v);
> -
> -    if ( v != current )
> -        return;
> -
> -    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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 06/10] ARM: VGIC: split up gic_dump_info() to cover virtual part separately
  2017-12-07 16:14 ` [PATCH v2 06/10] ARM: VGIC: split up gic_dump_info() to cover virtual part separately Andre Przywara
@ 2017-12-08 22:03   ` Stefano Stabellini
  0 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2017-12-08 22:03 UTC (permalink / raw)
  To: Andre Przywara; +Cc: xen-devel, Stefano Stabellini, Julien Grall

On Thu, 7 Dec 2017, Andre Przywara wrote:
> 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 73f4d4b2b2..5d2943b800 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -942,6 +942,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 971b3bfe37..90b827c574 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -403,6 +403,17 @@ void gic_inject(struct vcpu *v)
>          gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
>  }
>  
> +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 28cf16654a..4f4fd555c1 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 10/10] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq
  2017-12-07 16:14 ` [PATCH v2 10/10] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq Andre Przywara
@ 2017-12-08 22:05   ` Stefano Stabellini
  0 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2017-12-08 22:05 UTC (permalink / raw)
  To: Andre Przywara; +Cc: xen-devel, Stefano Stabellini, Julien Grall

On Thu, 7 Dec 2017, Andre Przywara wrote:
> 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 473e26111f..ce1e5cad25 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -962,8 +962,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;
>  
> @@ -979,11 +979,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 8d43a6ba76..60f6498092 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 4f4fd555c1..ce9d1d058a 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs()
  2017-12-08 21:40   ` Stefano Stabellini
@ 2018-01-24 17:43     ` Andre Przywara
  0 siblings, 0 replies; 18+ messages in thread
From: Andre Przywara @ 2018-01-24 17:43 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Julien Grall

Hi,

sorry for the delay on this, I just found this in preparation for a repost.

On 08/12/17 21:40, Stefano Stabellini wrote:
> On Thu, 7 Dec 2017, Andre Przywara wrote:
>> In gic_restore_pending_irqs() we push our pending virtual IRQs into the
>> list registers. This function is called once from a GIC context and once
>> from a VGIC context. Refactor the calls so that we have only one callsite
>> from the VGIC context. This will help separating the two worlds later.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>  xen/arch/arm/domain.c     |  1 +
>>  xen/arch/arm/gic.c        | 11 +++++------
>>  xen/arch/arm/traps.c      |  2 +-
>>  xen/include/asm-arm/gic.h |  2 +-
>>  4 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index a74ff1c07c..73f4d4b2b2 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -185,6 +185,7 @@ static void ctxt_switch_to(struct vcpu *n)
>>  
>>      /* VGIC */
>>      gic_restore_state(n);
>> +    gic_inject(n);
>>  
>>      /* VFP */
>>      vfp_restore_state(n);
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index bac8ada2bb..1f00654ef5 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 */
>> @@ -715,11 +711,14 @@ out:
>>      return rc;
>>  }
>>  
>> -void gic_inject(void)
>> +void gic_inject(struct vcpu *v)
>>  {
>>      ASSERT(!local_irq_is_enabled());
>>  
>> -    gic_restore_pending_irqs(current);
>> +    gic_restore_pending_irqs(v);
> 
> This looks suspicious to me: gic_restore_pending_irqs calls gic_set_lr.
> It doesn't look like gic_restore_pending_irqs can actually be called for
> v != current.

Good point!

> However, I think that we could remove the call to
> gic_restore_pending_irqs from gic_restore_state safely, because we can
> still rely on gic_inject being called before entering the guest.

Yes, I believe so as well, actually found this when debugging the new
VGIC. Debug printks showed LR injections happening twice, which is
actually harmful with the new VGIC (because it sometimes changes the
state of the virtual IRQ).

> There is no need to add a call to gic_inject in ctxt_switch_to, I think.

Yes, I agree.

So this simplifies this patch quite a lot, actually to just removing the
call of gic_restore_pending_irqs(), and the forward declaration.
But in contrast to this v2 patch here (which was really just
refactoring) this changes behaviour now, so needs some testing (works
for me (tm), though)

Cheers,
Andre.

>> +    if ( v != current )
>> +        return;
>>  
>>      if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
>>          gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index ff3d6ff2aa..7fd676ed9d 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2298,7 +2298,7 @@ void leave_hypervisor_tail(void)
>>      {
>>          local_irq_disable();
>>          if (!softirq_pending(smp_processor_id())) {
>> -            gic_inject();
>> +            gic_inject(current);
>>  
>>              /*
>>               * If the SErrors handle option is "DIVERSE", we have to prevent
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 587a14f8b9..28cf16654a 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -235,7 +235,7 @@ extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
>>  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>>                                struct irq_desc *desc);
>>  
>> -extern void gic_inject(void);
>> +extern void gic_inject(struct vcpu *v);
>>  extern void gic_clear_pending_irqs(struct vcpu *v);
>>  extern int gic_events_need_delivery(void);

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-01-24 17:43 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-07 16:14 [PATCH v2 00/10] ARM: VGIC/GIC separation cleanups Andre Przywara
2017-12-07 16:14 ` [PATCH v2 01/10] ARM: remove unneeded gic.h inclusions Andre Przywara
2017-12-07 16:14 ` [PATCH v2 02/10] ARM: vGIC: fix nr_irq definition Andre Przywara
2017-12-08 21:42   ` Julien Grall
2017-12-07 16:14 ` [PATCH v2 03/10] ARM: VGIC: move gic_remove_irq_from_queues() Andre Przywara
2017-12-08 21:27   ` Stefano Stabellini
2017-12-07 16:14 ` [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs() Andre Przywara
2017-12-08 21:40   ` Stefano Stabellini
2018-01-24 17:43     ` Andre Przywara
2017-12-07 16:14 ` [PATCH v2 05/10] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation Andre Przywara
2017-12-08 21:57   ` Stefano Stabellini
2017-12-07 16:14 ` [PATCH v2 06/10] ARM: VGIC: split up gic_dump_info() to cover virtual part separately Andre Przywara
2017-12-08 22:03   ` Stefano Stabellini
2017-12-07 16:14 ` [PATCH v2 07/10] ARM: VGIC: rework events_need_delivery() Andre Przywara
2017-12-07 16:14 ` [PATCH v2 08/10] ARM: VGIC: factor out vgic_connect_hw_irq() Andre Przywara
2017-12-07 16:14 ` [PATCH v2 09/10] ARM: VGIC: factor out vgic_get_hw_irq_desc() Andre Przywara
2017-12-07 16:14 ` [PATCH v2 10/10] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq Andre Przywara
2017-12-08 22:05   ` Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.