[PATCH 00/12] ARM: VGIC/GIC separation cleanups

[PATCH 00/12] ARM: VGIC/GIC separation cleanups

[Andre Przywara]

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
ugly patch 5/12 and the heavy 6/12, 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.

This is a first shot, any ideas on improvements are welcome.

Cheers,
Andre.

Andre Przywara (12):
  ARM: remove unneeded gic.h inclusions
  ARM: vGIC: fix nr_irq definition
  ARM: VGIC: remove gic_clear_pending_irqs()
  ARM: VGIC: move gic_remove_irq_from_queues()
  ARM: VGIC: move gic_remove_from_lr_pending()
  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              | 442 +++++++++++++++++++++++++++++++++++
 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           |   6 +-
 xen/arch/arm/vgic.c                  |  46 +++-
 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            |   9 +-
 xen/include/asm-arm/irq.h            |   2 +-
 xen/include/asm-arm/vgic.h           |  10 +
 21 files changed, 534 insertions(+), 472 deletions(-)
 create mode 100644 xen/arch/arm/gic-vgic.c

-- 
2.14.1


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

[PATCH 01/12] ARM: remove unneeded gic.h inclusions

[Andre Przywara]

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@arm.com>
---
 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 bf29299707..e7899fbf19 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 68b488997d..07f5cc4468 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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 01/12] ARM: remove unneeded gic.h inclusions

[Stefano Stabellini]

On Thu, 19 Oct 2017, Andre Przywara wrote:
> 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@arm.com>

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 bf29299707..e7899fbf19 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 68b488997d..07f5cc4468 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.xen.org
https://lists.xen.org/xen-devel

[PATCH 02/12] ARM: vGIC: fix nr_irq definition

[Andre Przywara]

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@arm.com>
---
 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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 02/12] ARM: vGIC: fix nr_irq definition

[Stefano Stabellini]

On Thu, 19 Oct 2017, 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@arm.com>

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.xen.org
https://lists.xen.org/xen-devel

[PATCH 03/12] ARM: VGIC: remove gic_clear_pending_irqs()

[Andre Przywara]

gic_clear_pending_irqs() was not only misnamed, but also misplaced, as
a function solely dealing with the GIC emulation should not live in gic.c.
Move the functionality of this function into its only caller in vgic.c

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 xen/arch/arm/gic.c        | 11 -----------
 xen/arch/arm/vgic.c       |  4 +++-
 xen/include/asm-arm/gic.h |  1 -
 3 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ed363f6c37..75b2e0e0ca 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -675,17 +675,6 @@ 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;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index d8acbbeaaa..451a306a98 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -504,7 +504,9 @@ void vgic_clear_pending_irqs(struct vcpu *v)
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
     list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
         list_del_init(&p->inflight);
-    gic_clear_pending_irqs(v);
+    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
+        gic_remove_from_lr_pending(v, p);
+    v->arch.lr_mask = 0;
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index d3d7bda50d..2f248301ce 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -236,7 +236,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
                               struct irq_desc *desc);
 
 extern void gic_inject(void);
-extern void gic_clear_pending_irqs(struct vcpu *v);
 extern int gic_events_need_delivery(void);
 
 extern void init_maintenance_interrupt(void);
-- 
2.14.1


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

Re: [PATCH 03/12] ARM: VGIC: remove gic_clear_pending_irqs()

[Stefano Stabellini]

On Thu, 19 Oct 2017, Andre Przywara wrote:
> gic_clear_pending_irqs() was not only misnamed, but also misplaced, as
> a function solely dealing with the GIC emulation should not live in gic.c.
> Move the functionality of this function into its only caller in vgic.c
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

The reason why gic_clear_pending_irqs is in gic.c is that lr_mask and
lr_pending are considered part of the gic driver (gic.c). On the other
end, inflight is part of the vgic.

As an example, the idea is that the code outside of gic.c (for example
vgic.c) shouldn't have to know, or have to care, whether a given IRQ is
in the lr_pending queue or actually in a LR register.

lr_mask and lr_pending are only accessed from gic.c. The only exception
is the initialization (INIT_LIST_HEAD(&v->arch.vgic.lr_pending)).


> ---
>  xen/arch/arm/gic.c        | 11 -----------
>  xen/arch/arm/vgic.c       |  4 +++-
>  xen/include/asm-arm/gic.h |  1 -
>  3 files changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ed363f6c37..75b2e0e0ca 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -675,17 +675,6 @@ 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;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index d8acbbeaaa..451a306a98 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -504,7 +504,9 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>      list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
>          list_del_init(&p->inflight);
> -    gic_clear_pending_irqs(v);
> +    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> +        gic_remove_from_lr_pending(v, p);
> +    v->arch.lr_mask = 0;
>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>  }
>  
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index d3d7bda50d..2f248301ce 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -236,7 +236,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>                                struct irq_desc *desc);
>  
>  extern void gic_inject(void);
> -extern void gic_clear_pending_irqs(struct vcpu *v);
>  extern int gic_events_need_delivery(void);
>  
>  extern void init_maintenance_interrupt(void);
> -- 
> 2.14.1
> 

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

Re: [PATCH 03/12] ARM: VGIC: remove gic_clear_pending_irqs()

[Andre Przywara]

Hi,

On 26/10/17 01:14, Stefano Stabellini wrote:
> On Thu, 19 Oct 2017, Andre Przywara wrote:
>> gic_clear_pending_irqs() was not only misnamed, but also misplaced, as
>> a function solely dealing with the GIC emulation should not live in gic.c.
>> Move the functionality of this function into its only caller in vgic.c
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> The reason why gic_clear_pending_irqs is in gic.c is that lr_mask and
> lr_pending are considered part of the gic driver (gic.c). On the other
> end, inflight is part of the vgic.
> 
> As an example, the idea is that the code outside of gic.c (for example
> vgic.c) shouldn't have to know, or have to care, whether a given IRQ is
> in the lr_pending queue or actually in a LR register.

I can understand that the lr_pending queue *should* be logical
continuation of the LR registers, something like spill-over LRs.
Though I wasn't aware of this before ;-)
So I can see that from a *logical* point of view it looks like it
belongs to the hardware part of the GIC (more specifically gic-vgic.c),
which deals with the actual LRs. But I guess this is somewhat of a grey
area.

BUT:
This is a design choice of the VGIC, and one which the KVM VGIC design
for instance does *not* share. Also my earlier Xen VGIC rework patches
got rid of this as well (because dealing with two lists is too complicated).
Also, the name is misleading: gic_clear_pending_irqs() does not hint at
all that this is dealing with the GIC emulation, I think it should read
vgic_vcpu_clear_pending_irqs().
And as it accesses VGIC specific data structures only, I don't think it
belongs to gic.c, really.
So I could live with moving it into the new gic-vgic.c, let me see if
that works.

The need for this patch didn't come out of the blue, I actually need it
to be able to reuse gic.c with *any* other VGIC implementation. And this
applies to both a VGIC rework and the KVM VGIC port.
These lr_queue and lr_pending queues are really an implementation detail
of the existing *VGIC*, and, more importantly: they refer to the struct
pending_irq, which is definitely a VGIC detail.

The rabbit to follow in this series is to strictly split the usage of
struct pending_irq from the hardware GIC driver. The KVM VGIC does not
have a "struct pending_irq", so we can't have anything mentioning that
in code that should survive a KVM VGIC port.
So short of replacing gic.c at all, moving everything mentioning
pending_irq out of gic.c is the only option.

Cheers,
Andre.

> lr_mask and lr_pending are only accessed from gic.c. The only exception
> is the initialization (INIT_LIST_HEAD(&v->arch.vgic.lr_pending)).
> 
> 
>> ---
>>  xen/arch/arm/gic.c        | 11 -----------
>>  xen/arch/arm/vgic.c       |  4 +++-
>>  xen/include/asm-arm/gic.h |  1 -
>>  3 files changed, 3 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index ed363f6c37..75b2e0e0ca 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -675,17 +675,6 @@ 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;
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index d8acbbeaaa..451a306a98 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -504,7 +504,9 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>>      list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
>>          list_del_init(&p->inflight);
>> -    gic_clear_pending_irqs(v);
>> +    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
>> +        gic_remove_from_lr_pending(v, p);
>> +    v->arch.lr_mask = 0;
>>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>>  }
>>  
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index d3d7bda50d..2f248301ce 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -236,7 +236,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>>                                struct irq_desc *desc);
>>  
>>  extern void gic_inject(void);
>> -extern void gic_clear_pending_irqs(struct vcpu *v);
>>  extern int gic_events_need_delivery(void);
>>  
>>  extern void init_maintenance_interrupt(void);
>> -- 
>> 2.14.1
>>

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

Re: [PATCH 03/12] ARM: VGIC: remove gic_clear_pending_irqs()

[Stefano Stabellini]

On Fri, 10 Nov 2017, Andre Przywara wrote:
> Hi,
> 
> On 26/10/17 01:14, Stefano Stabellini wrote:
> > On Thu, 19 Oct 2017, Andre Przywara wrote:
> >> gic_clear_pending_irqs() was not only misnamed, but also misplaced, as
> >> a function solely dealing with the GIC emulation should not live in gic.c.
> >> Move the functionality of this function into its only caller in vgic.c
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > 
> > The reason why gic_clear_pending_irqs is in gic.c is that lr_mask and
> > lr_pending are considered part of the gic driver (gic.c). On the other
> > end, inflight is part of the vgic.
> > 
> > As an example, the idea is that the code outside of gic.c (for example
> > vgic.c) shouldn't have to know, or have to care, whether a given IRQ is
> > in the lr_pending queue or actually in a LR register.
> 
> I can understand that the lr_pending queue *should* be logical
> continuation of the LR registers, something like spill-over LRs.
> Though I wasn't aware of this before ;-)
> So I can see that from a *logical* point of view it looks like it
> belongs to the hardware part of the GIC (more specifically gic-vgic.c),
> which deals with the actual LRs. But I guess this is somewhat of a grey
> area.
> 
> BUT:
> This is a design choice of the VGIC, and one which the KVM VGIC design
> for instance does *not* share. Also my earlier Xen VGIC rework patches
> got rid of this as well (because dealing with two lists is too complicated).
> Also, the name is misleading: gic_clear_pending_irqs() does not hint at
> all that this is dealing with the GIC emulation, I think it should read
> vgic_vcpu_clear_pending_irqs().
> And as it accesses VGIC specific data structures only, I don't think it
> belongs to gic.c, really.
> So I could live with moving it into the new gic-vgic.c, let me see if
> that works.
> 
> The need for this patch didn't come out of the blue, I actually need it
> to be able to reuse gic.c with *any* other VGIC implementation. And this
> applies to both a VGIC rework and the KVM VGIC port.
> These lr_queue and lr_pending queues are really an implementation detail
> of the existing *VGIC*, and, more importantly: they refer to the struct
> pending_irq, which is definitely a VGIC detail.
> 
> The rabbit to follow in this series is to strictly split the usage of
> struct pending_irq from the hardware GIC driver. The KVM VGIC does not
> have a "struct pending_irq", so we can't have anything mentioning that
> in code that should survive a KVM VGIC port.
> So short of replacing gic.c at all, moving everything mentioning
> pending_irq out of gic.c is the only option.

Could you at least retain gic_clear_pending_irqs as a separate function?

pending_irq is clearly separate from anything vgic and doesn't belong
there. Nonetheless, I can live with moving gic_clear_pending_irqs to
vgic.c to make future development easier, but at least let's keep
gic_clear_pending_irqs as is.


> > lr_mask and lr_pending are only accessed from gic.c. The only exception
> > is the initialization (INIT_LIST_HEAD(&v->arch.vgic.lr_pending)).
> > 
> > 
> >> ---
> >>  xen/arch/arm/gic.c        | 11 -----------
> >>  xen/arch/arm/vgic.c       |  4 +++-
> >>  xen/include/asm-arm/gic.h |  1 -
> >>  3 files changed, 3 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index ed363f6c37..75b2e0e0ca 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -675,17 +675,6 @@ 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;
> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> >> index d8acbbeaaa..451a306a98 100644
> >> --- a/xen/arch/arm/vgic.c
> >> +++ b/xen/arch/arm/vgic.c
> >> @@ -504,7 +504,9 @@ void vgic_clear_pending_irqs(struct vcpu *v)
> >>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >>      list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
> >>          list_del_init(&p->inflight);
> >> -    gic_clear_pending_irqs(v);
> >> +    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> >> +        gic_remove_from_lr_pending(v, p);
> >> +    v->arch.lr_mask = 0;
> >>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >>  }
> >>  
> >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> >> index d3d7bda50d..2f248301ce 100644
> >> --- a/xen/include/asm-arm/gic.h
> >> +++ b/xen/include/asm-arm/gic.h
> >> @@ -236,7 +236,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> >>                                struct irq_desc *desc);
> >>  
> >>  extern void gic_inject(void);
> >> -extern void gic_clear_pending_irqs(struct vcpu *v);
> >>  extern int gic_events_need_delivery(void);
> >>  
> >>  extern void init_maintenance_interrupt(void);
> >> -- 
> >> 2.14.1
> >>
> 

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

Re: [PATCH 03/12] ARM: VGIC: remove gic_clear_pending_irqs()

[Julien Grall]

Hi Stefano,

On 16/11/17 01:17, Stefano Stabellini wrote:
> On Fri, 10 Nov 2017, Andre Przywara wrote:
>> Hi,
>>
>> On 26/10/17 01:14, Stefano Stabellini wrote:
>>> On Thu, 19 Oct 2017, Andre Przywara wrote:
>>>> gic_clear_pending_irqs() was not only misnamed, but also misplaced, as
>>>> a function solely dealing with the GIC emulation should not live in gic.c.
>>>> Move the functionality of this function into its only caller in vgic.c
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> The reason why gic_clear_pending_irqs is in gic.c is that lr_mask and
>>> lr_pending are considered part of the gic driver (gic.c). On the other
>>> end, inflight is part of the vgic.
>>>
>>> As an example, the idea is that the code outside of gic.c (for example
>>> vgic.c) shouldn't have to know, or have to care, whether a given IRQ is
>>> in the lr_pending queue or actually in a LR register.
>>
>> I can understand that the lr_pending queue *should* be logical
>> continuation of the LR registers, something like spill-over LRs.
>> Though I wasn't aware of this before ;-)
>> So I can see that from a *logical* point of view it looks like it
>> belongs to the hardware part of the GIC (more specifically gic-vgic.c),
>> which deals with the actual LRs. But I guess this is somewhat of a grey
>> area.
>>
>> BUT:
>> This is a design choice of the VGIC, and one which the KVM VGIC design
>> for instance does *not* share. Also my earlier Xen VGIC rework patches
>> got rid of this as well (because dealing with two lists is too complicated).
>> Also, the name is misleading: gic_clear_pending_irqs() does not hint at
>> all that this is dealing with the GIC emulation, I think it should read
>> vgic_vcpu_clear_pending_irqs().
>> And as it accesses VGIC specific data structures only, I don't think it
>> belongs to gic.c, really.
>> So I could live with moving it into the new gic-vgic.c, let me see if
>> that works.
>>
>> The need for this patch didn't come out of the blue, I actually need it
>> to be able to reuse gic.c with *any* other VGIC implementation. And this
>> applies to both a VGIC rework and the KVM VGIC port.
>> These lr_queue and lr_pending queues are really an implementation detail
>> of the existing *VGIC*, and, more importantly: they refer to the struct
>> pending_irq, which is definitely a VGIC detail.
>>
>> The rabbit to follow in this series is to strictly split the usage of
>> struct pending_irq from the hardware GIC driver. The KVM VGIC does not
>> have a "struct pending_irq", so we can't have anything mentioning that
>> in code that should survive a KVM VGIC port.
>> So short of replacing gic.c at all, moving everything mentioning
>> pending_irq out of gic.c is the only option.
> 
> Could you at least retain gic_clear_pending_irqs as a separate function?
> 
> pending_irq is clearly separate from anything vgic and doesn't belong
> there. Nonetheless, I can live with moving gic_clear_pending_irqs to
> vgic.c to make future development easier, but at least let's keep
> gic_clear_pending_irqs as is.

Why do you think pending_irq does not belong to vgic? pending_irq is 
defined by vgic.h and contain the state of the interrupt for the virtual 
GIC.

However, looking at gic_clear_pending_irqs. It is only called by 
vgic_clear_pending_irqs which is then only called by do_common_cpu_on.

The commit message adding this code (see 803d6c993a "xen/arm: clear 
pending irq queues on do_psci_cpu_on") says: "Also when (re)activating a 
vcpu, clear the vgic and gic irq queues: we don't want to inject any 
irqs that couldn't be handled by the vcpu right before going offline."

Even on real hardware you may have pending interrupt at boot. This is 
taken care by the OS when initializing the GIC CPU interface. So what is 
that function trying to solve?

Cheers,

-- 
Julien Grall

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

Re: [PATCH 03/12] ARM: VGIC: remove gic_clear_pending_irqs()

[Andre Przywara]

Hi,

On 10/11/17 16:42, Andre Przywara wrote:
> Hi,
> 
> On 26/10/17 01:14, Stefano Stabellini wrote:
>> On Thu, 19 Oct 2017, Andre Przywara wrote:
>>> gic_clear_pending_irqs() was not only misnamed, but also misplaced, as
>>> a function solely dealing with the GIC emulation should not live in gic.c.
>>> Move the functionality of this function into its only caller in vgic.c
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>
>> The reason why gic_clear_pending_irqs is in gic.c is that lr_mask and
>> lr_pending are considered part of the gic driver (gic.c). On the other
>> end, inflight is part of the vgic.
>>
>> As an example, the idea is that the code outside of gic.c (for example
>> vgic.c) shouldn't have to know, or have to care, whether a given IRQ is
>> in the lr_pending queue or actually in a LR register.
> 
> I can understand that the lr_pending queue *should* be logical
> continuation of the LR registers, something like spill-over LRs.
> Though I wasn't aware of this before ;-)
> So I can see that from a *logical* point of view it looks like it
> belongs to the hardware part of the GIC (more specifically gic-vgic.c),
> which deals with the actual LRs. But I guess this is somewhat of a grey
> area.
> 
> BUT:
> This is a design choice of the VGIC, and one which the KVM VGIC design
> for instance does *not* share. Also my earlier Xen VGIC rework patches
> got rid of this as well (because dealing with two lists is too complicated).
> Also, the name is misleading: gic_clear_pending_irqs() does not hint at
> all that this is dealing with the GIC emulation, I think it should read
> vgic_vcpu_clear_pending_irqs().
> And as it accesses VGIC specific data structures only, I don't think it
> belongs to gic.c, really.
> So I could live with moving it into the new gic-vgic.c, let me see if
> that works.
> 
> The need for this patch didn't come out of the blue, I actually need it
> to be able to reuse gic.c with *any* other VGIC implementation. And this
> applies to both a VGIC rework and the KVM VGIC port.
> These lr_queue and lr_pending queues are really an implementation detail
> of the existing *VGIC*, and, more importantly: they refer to the struct
> pending_irq, which is definitely a VGIC detail.
> 
> The rabbit to follow in this series is to strictly split the usage of
> struct pending_irq from the hardware GIC driver. The KVM VGIC does not
> have a "struct pending_irq", so we can't have anything mentioning that
> in code that should survive a KVM VGIC port.
> So short of replacing gic.c at all, moving everything mentioning
> pending_irq out of gic.c is the only option.

Having said that I just moved it to gic-vgic.c, where it's also you of
the way. With this code eventually being removed anyway sometimes in the
future, it's not really worth arguing about it.

Cheers,
Andre.

>> lr_mask and lr_pending are only accessed from gic.c. The only exception
>> is the initialization (INIT_LIST_HEAD(&v->arch.vgic.lr_pending)).
>>
>>
>>> ---
>>>  xen/arch/arm/gic.c        | 11 -----------
>>>  xen/arch/arm/vgic.c       |  4 +++-
>>>  xen/include/asm-arm/gic.h |  1 -
>>>  3 files changed, 3 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index ed363f6c37..75b2e0e0ca 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -675,17 +675,6 @@ 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;
>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>> index d8acbbeaaa..451a306a98 100644
>>> --- a/xen/arch/arm/vgic.c
>>> +++ b/xen/arch/arm/vgic.c
>>> @@ -504,7 +504,9 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>>>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>>>      list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
>>>          list_del_init(&p->inflight);
>>> -    gic_clear_pending_irqs(v);
>>> +    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
>>> +        gic_remove_from_lr_pending(v, p);
>>> +    v->arch.lr_mask = 0;
>>>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>>>  }
>>>  
>>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>>> index d3d7bda50d..2f248301ce 100644
>>> --- a/xen/include/asm-arm/gic.h
>>> +++ b/xen/include/asm-arm/gic.h
>>> @@ -236,7 +236,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>>>                                struct irq_desc *desc);
>>>  
>>>  extern void gic_inject(void);
>>> -extern void gic_clear_pending_irqs(struct vcpu *v);
>>>  extern int gic_events_need_delivery(void);
>>>  
>>>  extern void init_maintenance_interrupt(void);
>>> -- 
>>> 2.14.1
>>>

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

[PATCH 04/12] ARM: VGIC: move gic_remove_irq_from_queues()

[Andre Przywara]

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@arm.com>
---
 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 75b2e0e0ca..ef041354ea 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 451a306a98..cd50b90d67 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);
@@ -510,6 +510,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 2f248301ce..030c1d86a7 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -243,7 +243,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..8d0ff65708 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);
+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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 04/12] ARM: VGIC: move gic_remove_irq_from_queues()

[Stefano Stabellini]

On Thu, 19 Oct 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@arm.com>

Yes, gic_remove_irq_from_queues could be in the vgic.

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

One comment about cosmetics below.


> ---
>  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 75b2e0e0ca..ef041354ea 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 451a306a98..cd50b90d67 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);
> @@ -510,6 +510,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 2f248301ce..030c1d86a7 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -243,7 +243,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..8d0ff65708 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);
> +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);

cosmetic: you might as well add an extern


>  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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 04/12] ARM: VGIC: move gic_remove_irq_from_queues()

[Julien Grall]

Hi,

On 10/26/2017 01:19 AM, Stefano Stabellini wrote:
> On Thu, 19 Oct 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@arm.com>
> 
> Yes, gic_remove_irq_from_queues could be in the vgic.
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> One comment about cosmetics below.
> 
> 
>> ---
>>   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 75b2e0e0ca..ef041354ea 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 451a306a98..cd50b90d67 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);
>> @@ -510,6 +510,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 2f248301ce..030c1d86a7 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -243,7 +243,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..8d0ff65708 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);
>> +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
> 
> cosmetic: you might as well add an extern

Or just dropped extern from the others. The keyword is pointless.

> 
> 
>>   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);

-- 
Julien Grall

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

Re: [PATCH 04/12] ARM: VGIC: move gic_remove_irq_from_queues()

[Andre Przywara]

Hi,

...

>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index e489d0bf21..8d0ff65708 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);
>> +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
> 
> cosmetic: you might as well add an extern

I was wondering about that. I think extern in front of a prototype in a
header file is a bit pointless. Linux mostly doesn't use it (apart from
fs/ and some parts of security/).
Though I can of course easily add it.

Cheers,
Andre.

> 
> 
>>  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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 04/12] ARM: VGIC: move gic_remove_irq_from_queues()

[Stefano Stabellini]

On Fri, 10 Nov 2017, Andre Przywara wrote:
> Hi,
> 
> ...
> 
> >> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> >> index e489d0bf21..8d0ff65708 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);
> >> +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
> > 
> > cosmetic: you might as well add an extern
> 
> I was wondering about that. I think extern in front of a prototype in a
> header file is a bit pointless. Linux mostly doesn't use it (apart from
> fs/ and some parts of security/).
> Though I can of course easily add it.

It was just to stay consistent. It's fine also to remove all the useless
extern keywords from this header.

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

[PATCH 05/12] ARM: VGIC: move gic_remove_from_lr_pending()

[Andre Przywara]

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

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 xen/arch/arm/gic.c         |  7 -------
 xen/arch/arm/vgic-v3-its.c |  2 +-
 xen/arch/arm/vgic.c        | 13 ++++++++++---
 xen/include/asm-arm/gic.h  |  1 -
 xen/include/asm-arm/vgic.h |  1 +
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ef041354ea..59dd255c2c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -404,13 +404,6 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
     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);
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index d8fa44258d..5b77594723 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -449,7 +449,7 @@ static void update_lpi_vgic_status(struct vcpu *v, struct pending_irq *p)
             gic_raise_guest_irq(v, p->irq, p->lpi_priority);
     }
     else
-        gic_remove_from_lr_pending(v, p);
+        vgic_remove_from_lr_pending(v, p);
 }
 
 static int its_handle_inv(struct virt_its *its, uint64_t *cmdptr)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index cd50b90d67..2cdaca7480 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -345,7 +345,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
         spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
         p = irq_to_pending(v_target, irq);
         clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        gic_remove_from_lr_pending(v_target, p);
+        vgic_remove_from_lr_pending(v_target, p);
         desc = p->desc;
         spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
 
@@ -505,18 +505,25 @@ void vgic_clear_pending_irqs(struct vcpu *v)
     list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
         list_del_init(&p->inflight);
     list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
-        gic_remove_from_lr_pending(v, p);
+        vgic_remove_from_lr_pending(v, p);
     v->arch.lr_mask = 0;
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
+void vgic_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 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);
+    vgic_remove_from_lr_pending(v, p);
 }
 
 void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 030c1d86a7..4b2a60ee64 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -242,7 +242,6 @@ extern void init_maintenance_interrupt(void);
 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);
 
 /* 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 8d0ff65708..0d3810e6af 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -205,6 +205,7 @@ 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);
 void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
+void vgic_remove_from_lr_pending(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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 05/12] ARM: VGIC: move gic_remove_from_lr_pending()

[Stefano Stabellini]

On Thu, 19 Oct 2017, Andre Przywara wrote:
> gic_remove_from_lr_pending() was not only misnamed, it also had the wrong
> abstraction, as it should not live in gic.c.
> Move it into vgic.c and vgic.h, where it belongs, and rename it on the
> way.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Like gic_clear_pending_irqs, gic_remove_from_lr_pending belongs to
gic.c. However, I agree with you that it is misnamed. I would rename it
to something like gic_clear_one_pending.


> ---
>  xen/arch/arm/gic.c         |  7 -------
>  xen/arch/arm/vgic-v3-its.c |  2 +-
>  xen/arch/arm/vgic.c        | 13 ++++++++++---
>  xen/include/asm-arm/gic.h  |  1 -
>  xen/include/asm-arm/vgic.h |  1 +
>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ef041354ea..59dd255c2c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -404,13 +404,6 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
>      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);
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index d8fa44258d..5b77594723 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -449,7 +449,7 @@ static void update_lpi_vgic_status(struct vcpu *v, struct pending_irq *p)
>              gic_raise_guest_irq(v, p->irq, p->lpi_priority);
>      }
>      else
> -        gic_remove_from_lr_pending(v, p);
> +        vgic_remove_from_lr_pending(v, p);
>  }
>  
>  static int its_handle_inv(struct virt_its *its, uint64_t *cmdptr)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index cd50b90d67..2cdaca7480 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -345,7 +345,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>          spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
>          p = irq_to_pending(v_target, irq);
>          clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> -        gic_remove_from_lr_pending(v_target, p);
> +        vgic_remove_from_lr_pending(v_target, p);
>          desc = p->desc;
>          spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>  
> @@ -505,18 +505,25 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>      list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
>          list_del_init(&p->inflight);
>      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> -        gic_remove_from_lr_pending(v, p);
> +        vgic_remove_from_lr_pending(v, p);
>      v->arch.lr_mask = 0;
>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>  }
>  
> +void vgic_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 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);
> +    vgic_remove_from_lr_pending(v, p);
>  }
>  
>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 030c1d86a7..4b2a60ee64 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -242,7 +242,6 @@ extern void init_maintenance_interrupt(void);
>  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);
>  
>  /* 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 8d0ff65708..0d3810e6af 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -205,6 +205,7 @@ 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);
>  void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
> +void vgic_remove_from_lr_pending(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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 05/12] ARM: VGIC: move gic_remove_from_lr_pending()

[Andre Przywara]

Hi,

On 26/10/17 01:20, Stefano Stabellini wrote:
> On Thu, 19 Oct 2017, Andre Przywara wrote:
>> gic_remove_from_lr_pending() was not only misnamed, it also had the wrong
>> abstraction, as it should not live in gic.c.
>> Move it into vgic.c and vgic.h, where it belongs, and rename it on the
>> way.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Like gic_clear_pending_irqs, gic_remove_from_lr_pending belongs to
> gic.c. However, I agree with you that it is misnamed. I would rename it
> to something like gic_clear_one_pending.

Couldn't be bothered ;-) Instead I will just drop this patch and move
the function to gic-vgic.c, so it's out of the way as well.

Cheers,
Andre.

>> ---
>>  xen/arch/arm/gic.c         |  7 -------
>>  xen/arch/arm/vgic-v3-its.c |  2 +-
>>  xen/arch/arm/vgic.c        | 13 ++++++++++---
>>  xen/include/asm-arm/gic.h  |  1 -
>>  xen/include/asm-arm/vgic.h |  1 +
>>  5 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index ef041354ea..59dd255c2c 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -404,13 +404,6 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
>>      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);
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index d8fa44258d..5b77594723 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -449,7 +449,7 @@ static void update_lpi_vgic_status(struct vcpu *v, struct pending_irq *p)
>>              gic_raise_guest_irq(v, p->irq, p->lpi_priority);
>>      }
>>      else
>> -        gic_remove_from_lr_pending(v, p);
>> +        vgic_remove_from_lr_pending(v, p);
>>  }
>>  
>>  static int its_handle_inv(struct virt_its *its, uint64_t *cmdptr)
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index cd50b90d67..2cdaca7480 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -345,7 +345,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>>          spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
>>          p = irq_to_pending(v_target, irq);
>>          clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>> -        gic_remove_from_lr_pending(v_target, p);
>> +        vgic_remove_from_lr_pending(v_target, p);
>>          desc = p->desc;
>>          spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>>  
>> @@ -505,18 +505,25 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>>      list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
>>          list_del_init(&p->inflight);
>>      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
>> -        gic_remove_from_lr_pending(v, p);
>> +        vgic_remove_from_lr_pending(v, p);
>>      v->arch.lr_mask = 0;
>>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>>  }
>>  
>> +void vgic_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 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);
>> +    vgic_remove_from_lr_pending(v, p);
>>  }
>>  
>>  void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 030c1d86a7..4b2a60ee64 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -242,7 +242,6 @@ extern void init_maintenance_interrupt(void);
>>  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);
>>  
>>  /* 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 8d0ff65708..0d3810e6af 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -205,6 +205,7 @@ 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);
>>  void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
>> +void vgic_remove_from_lr_pending(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

[PATCH 06/12] ARM: VGIC: streamline gic_restore_pending_irqs()

[Andre Przywara]

In gic_restore_pending_irqs() we push our pending virtual IRQs into the
list registers. This function is called once from 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@arm.com>
---
 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 59dd255c2c..58d69955fb 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 */
@@ -697,11 +693,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 4b2a60ee64..fe14094c0f 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 int gic_events_need_delivery(void);
 
 extern void init_maintenance_interrupt(void);
-- 
2.14.1


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

[PATCH 07/12] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation

[Andre Przywara]

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

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 xen/arch/arm/Makefile   |   1 +
 xen/arch/arm/gic-vgic.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic.c      | 348 +-----------------------------------------
 3 files changed, 398 insertions(+), 346 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..66cae21e82
--- /dev/null
+++ b/xen/arch/arm/gic-vgic.c
@@ -0,0 +1,395 @@
+/*
+ * 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_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);
+}
+
+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 58d69955fb..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,346 +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_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);
-}
-
-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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 07/12] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation

[Stefano Stabellini]

On Thu, 19 Oct 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.

That is true, however, I don't necessarely see "the code to bridge VGIC
requests to the GIC virtualization hardware" as belonging to the VGIC. I
think is a good abstraction to place in the GIC. That said, see below.


> 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.

Splitting "the code to bridge VGIC requests to the GIC virtualization
hardware" is harmless, so I am OK with this patch.

One cosmetic comment below.


> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/Makefile   |   1 +
>  xen/arch/arm/gic-vgic.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic.c      | 348 +-----------------------------------------
>  3 files changed, 398 insertions(+), 346 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..66cae21e82
> --- /dev/null
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -0,0 +1,395 @@
> +/*
> + * 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;

This is a bit ugly. Would the macro "get_per_cpu_var" help?


> +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_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);
> +}
> +
> +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 58d69955fb..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);

I didn't look at what's left in gic.c, but would it be possible to move
the definition of lr_mask to gic-vgic.c?


>  #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,346 +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_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);
> -}
> -
> -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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 07/12] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation

[Andre Przywara]

Hi,

On 26/10/17 01:37, Stefano Stabellini wrote:
> On Thu, 19 Oct 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.
> 
> That is true, however, I don't necessarely see "the code to bridge VGIC
> requests to the GIC virtualization hardware" as belonging to the VGIC. I
> think is a good abstraction to place in the GIC. That said, see below.
> 
> 
>> 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.
> 
> Splitting "the code to bridge VGIC requests to the GIC virtualization
> hardware" is harmless, so I am OK with this patch.
> 
> One cosmetic comment below.
> 
> 
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  xen/arch/arm/Makefile   |   1 +
>>  xen/arch/arm/gic-vgic.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/gic.c      | 348 +-----------------------------------------
>>  3 files changed, 398 insertions(+), 346 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..66cae21e82
>> --- /dev/null
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -0,0 +1,395 @@
>> +/*
>> + * 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;
> 
> This is a bit ugly. Would the macro "get_per_cpu_var" help?

So this could become:
extern uint64_t get_per_cpu_var(lr_mask);
though I am not sure if this is really nicer?

As you found out below yourself, this lr_mask is a bit nasty, as it's
used in both gic-vgic.c and gic.c. I don't think there is much we can do
about it, thus just sharing this variable was by far the easiest and
simplest solution.

>> +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))
>> +

...


>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 58d69955fb..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);
> 
> I didn't look at what's left in gic.c, but would it be possible to move
> the definition of lr_mask to gic-vgic.c?

As mentioned above: not really, we need it in both files, unfortunately.

As in the other mail, I guess it's not worth trying to properly fix this
now, unless you have a really good reason.

Cheers,
Andre.

>>  #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;

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

[PATCH 08/12] ARM: VGIC: split up gic_dump_info() to cover virtual part separately

[Andre Przywara]

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

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

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 73f4d4b2b2..5250bc2f88 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);
+    vgic_dump_info(v);
 }
 
 void vcpu_mark_events_pending(struct vcpu *v)
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/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2cdaca7480..37a083e804 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -656,6 +656,17 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
     clear_bit(virq, d->arch.vgic.allocated_irqs);
 }
 
+void vgic_dump_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/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 0d3810e6af..49b8a4bec0 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -226,6 +226,8 @@ extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
                         const struct sgi_target *target);
 extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
 
+void vgic_dump_info(struct vcpu *v);
+
 /* Reserve a specific guest vIRQ */
 extern bool vgic_reserve_virq(struct domain *d, unsigned int virq);
 
-- 
2.14.1


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

Re: [PATCH 08/12] ARM: VGIC: split up gic_dump_info() to cover virtual part separately

[Stefano Stabellini]

On Thu, 19 Oct 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 vgic.c to observe the abstraction.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Same comment on lr_pending belonging to gic.c (or now gic-vgic.c).

vgic.c is not allowed to access lr_pending, but gic.c is allowed to
access inflight, only for reading or removing interrupts at the end of
the cycle (after EOI). vgic.c is expected to manage adding irqs to
inflight and removing them, for any reasons other than "the EOI is
done". I admit it is not clear from the code.


> ---
>  xen/arch/arm/domain.c      |  1 +
>  xen/arch/arm/gic.c         | 12 ------------
>  xen/arch/arm/vgic.c        | 11 +++++++++++
>  xen/include/asm-arm/vgic.h |  2 ++
>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 73f4d4b2b2..5250bc2f88 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);
> +    vgic_dump_info(v);
>  }
>  
>  void vcpu_mark_events_pending(struct vcpu *v)
> 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/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2cdaca7480..37a083e804 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -656,6 +656,17 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
>      clear_bit(virq, d->arch.vgic.allocated_irqs);
>  }
>  
> +void vgic_dump_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/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 0d3810e6af..49b8a4bec0 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -226,6 +226,8 @@ extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
>                          const struct sgi_target *target);
>  extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
>  
> +void vgic_dump_info(struct vcpu *v);
> +
>  /* Reserve a specific guest vIRQ */
>  extern bool vgic_reserve_virq(struct domain *d, unsigned int virq);
>  
> -- 
> 2.14.1
> 

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

[PATCH 09/12] ARM: VGIC: rework events_need_delivery()

[Andre Przywara]

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

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 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 37a083e804..f8d0f46e71 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -602,6 +602,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 49b8a4bec0..dcdb1acaf3 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -219,6 +219,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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 09/12] ARM: VGIC: rework events_need_delivery()

[Stefano Stabellini]

On Thu, 19 Oct 2017, Andre Przywara wrote:
> 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@arm.com>

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 37a083e804..f8d0f46e71 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -602,6 +602,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 49b8a4bec0..dcdb1acaf3 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -219,6 +219,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.xen.org
https://lists.xen.org/xen-devel

[PATCH 10/12] ARM: VGIC: factor out vgic_connect_hw_irq()

[Andre Przywara]

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

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

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

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 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 66cae21e82..bf9455a34e 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -385,6 +385,37 @@ void gic_inject(struct vcpu *v)
         gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
 }
 
+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 dcdb1acaf3..cf02dc6394 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -220,6 +220,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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 10/12] ARM: VGIC: factor out vgic_connect_hw_irq()

[Stefano Stabellini]

On Thu, 19 Oct 2017, Andre Przywara wrote:
> At the moment we happily access VGIC internal data structures like
> the rank and struct pending_irq in gic.c, which should be VGIC agnostic.
> 
> Factor out a new function vgic_connect_hw_irq(), which allows a virtual
> IRQ to be connected to a hardware IRQ (using the hw bit in the LR).
> 
> This removes said accesses to VGIC data structures and improves abstraction.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

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 66cae21e82..bf9455a34e 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -385,6 +385,37 @@ void gic_inject(struct vcpu *v)
>          gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
>  }
>  
> +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 dcdb1acaf3..cf02dc6394 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -220,6 +220,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.xen.org
https://lists.xen.org/xen-devel

[PATCH 11/12] ARM: VGIC: factor out vgic_get_hw_irq_desc()

[Andre Przywara]

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

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 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 bf9455a34e..7765d83432 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -385,6 +385,21 @@ void gic_inject(struct vcpu *v)
         gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
 }
 
+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 cf02dc6394..947950875b 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -220,6 +220,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.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 11/12] ARM: VGIC: factor out vgic_get_hw_irq_desc()

[Stefano Stabellini]

On Thu, 19 Oct 2017, Andre Przywara wrote:
> At the moment we happily access the VGIC internal struct pending_irq
> (which describes a virtual IRQ) in irq.c.
> Factor out the actually needed functionality to learn the associated
> hardware IRQ and move that into gic-vgic.c to improve abstraction.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

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 bf9455a34e..7765d83432 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -385,6 +385,21 @@ void gic_inject(struct vcpu *v)
>          gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
>  }
>  
> +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 cf02dc6394..947950875b 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -220,6 +220,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.xen.org
https://lists.xen.org/xen-devel

[PATCH 12/12] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

[Andre Przywara]

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

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 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 ++--
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 511c8d7294..e5acff8900 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 != -1 )
+        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 74d00e0c54..3dec407a02 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -944,8 +944,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;
 
@@ -961,11 +961,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 != -1 )
+       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 7765d83432..e783f3b54b 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 : -1, 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 fe14094c0f..66f0957fab 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -339,8 +339,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 */
-- 
2.14.1


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

Re: [PATCH 12/12] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

[Stefano Stabellini]

On Thu, 19 Oct 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@arm.com>

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 ++--
>  4 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 511c8d7294..e5acff8900 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 != -1 )
> +        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 74d00e0c54..3dec407a02 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -944,8 +944,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;
>  
> @@ -961,11 +961,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 != -1 )
> +       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 7765d83432..e783f3b54b 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 : -1, 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 fe14094c0f..66f0957fab 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -339,8 +339,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 */
> -- 
> 2.14.1
> 

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

Re: [PATCH 12/12] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

[Julien Grall]

Hi Andre,

On 10/19/2017 01:48 PM, 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@arm.com>
> ---
>   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 ++--
>   4 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 511c8d7294..e5acff8900 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 != -1 )
> +        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 74d00e0c54..3dec407a02 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -944,8 +944,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;
>   
> @@ -961,11 +961,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 != -1 )

hw_irq is unsigned to technically it should be ~0. Also, I would prefer 
if you introduce a define making clear where the -1 comes from.

Lastly, I guess IRQ ~0 will never exist?

Cheers,

-- 
Julien Grall

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

Re: [PATCH 12/12] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

[Andre Przywara]

Hi,

On 26/10/17 09:28, Julien Grall wrote:
> Hi Andre,
> 
> On 10/19/2017 01:48 PM, 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@arm.com>
>> ---
>>   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 ++--
>>   4 files changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index 511c8d7294..e5acff8900 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 != -1 )
>> +        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 74d00e0c54..3dec407a02 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -944,8 +944,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;
>>   @@ -961,11 +961,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 != -1 )
> 
> hw_irq is unsigned to technically it should be ~0. Also, I would prefer
> if you introduce a define making clear where the -1 comes from.

Yeah, good point. Interestingly we don't have an INVALID_IRQ or the like
yet.

> Lastly, I guess IRQ ~0 will never exist?

Well, theoretically LPI IDs are 32 bits long, so you *could* have an LPI
with that IRQ ID. But this is somewhat academic, as we have either a 16
or a 24 bit limit elsewhere (IAR, ICC_CTLR.IDbits). But that smells a
bit like this could be extended in the future.

So maybe some cleaner solution would be to use one of the spurious
interrupt IDs (1020-1023) as a place holder for some INVALID_IRQ. I will
try this instead.

Cheers,
Andre.

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

Re: [PATCH 00/12] ARM: VGIC/GIC separation cleanups

[Andre Przywara]

Hi,

On 19/10/17 13:48, Andre Przywara wrote:
> 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
> ugly patch 5/12 and the heavy 6/12, 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.
> 
> This is a first shot, any ideas on improvements are welcome.

Forgot to mention: This is of course not 4.10 material.

And I tested this is on Midway and Juno, with two guests migrating
interrupts like crazy over night:
           CPU0       CPU1
 18:    8892519    8892530     GIC-0  27 Level     arch_timer
 19:  193048966  192887534     GIC-0  31 Level     events
 20:        366          0   xen-dyn     Edge    -event     xenbus
 21:     180335     183325   xen-dyn     Edge    -event     hvc_console
 22:  112174867   81289537   xen-dyn     Edge    -event     blkif
 23:   80768079  111489990   xen-dyn     Edge    -event     blkif

But please give it a good shake on your setup to spot any regressions.

Cheers,
Andre.

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