xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [RFC 0/8] xen/arm: acpi: Support SPIs routing
@ 2016-06-07 16:48 Julien Grall
  2016-06-07 16:48 ` [RFC 1/8] xen/arm: gic: Consolidate the IRQ affinity set in a single place Julien Grall
                   ` (8 more replies)
  0 siblings, 9 replies; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

Hi all,

Currently, Xen does not route SPIs to DOM0 when ACPI is inuse after
the functionality has been reverted in Xen 4.7 by commit 909bd14.

In the previous approach, the SPIs was routed when DOM0 was writing into
ISENABLER. However, this has resulted to deadlock (see more details in [1])
as soon as the IRQ was enabled by DOM0.

We have multiple solutions to route the IRQ:
 1) Rework route_irq_to_guest to avoid the deadlock
 2) Route and enable the IRQ outside of the emulation of ISENABLER
 3) Remove the dependency on the IRQ type in the routing function
 and route all the unused IRQs during domain building
 4) Add a new hypercall to let DOM0 routing the IRQ

I think that 1) and 2) are not resilient because route_irq_to_guest may fail
and there is no way to report this error to the guest (except by killing it).

Furthermore, in solution 2) enabling the interrupt would need to be defer
until the routing has been done. This would require a lot of code duplication.

Which leave solution 3) and 4). The solution 4) requires to introduce a new
(or re-use one) stable hypercall. I am not sure why we ruled out this
solution when we reviewed the ACPI design document.

This patch series is implementing the 3rd solution which defer the IRQ
type configuration for DOM0 IRQ when ACPI is inuse. However, this will
slightly increase the memory usage of Xen (54KB).

I am happy to consider any other solutions.

I only tested briefly this patch series, Shanker can you give a try on
your hardware?

A branch with all the patches can be found here:

git://xenbits.xen.org/people/julieng/xen-unstable.git branch irq-routing-acpi-rfc

Yours sincerely,

[1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02633.html

Julien Grall (8):
  xen/arm: gic: Consolidate the IRQ affinity set in a single place
  xen/arm: gic: Do not configure affinity for guest IRQ during routing
  xen/arm: gic: split set_irq_properties
  xen/arm: gic: set_type: Pass the type in parameter rather than in
    desc->arch.type
  xen/arm: gic: Document how gic_set_irq_type should be called
  Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on
    ACPI"
  xen/arm: Allow DOM0 to set the irq type when ACPI is inuse
  xen/arm: acpi: route all unused IRQs to DOM0

 xen/arch/arm/domain_build.c | 19 ++++++++-----------
 xen/arch/arm/gic-v2.c       | 28 +++++++++++++---------------
 xen/arch/arm/gic-v3.c       | 22 ++++++++++------------
 xen/arch/arm/gic.c          | 34 ++++++++++++++++++++++------------
 xen/arch/arm/irq.c          | 14 +++++++++++++-
 xen/arch/arm/vgic.c         | 34 ++++++++++++++++++++--------------
 xen/include/asm-arm/gic.h   | 11 +++++++----
 xen/include/asm-arm/irq.h   |  6 ++++++
 8 files changed, 99 insertions(+), 69 deletions(-)

-- 
1.9.1


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

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

* [RFC 1/8] xen/arm: gic: Consolidate the IRQ affinity set in a single place
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
@ 2016-06-07 16:48 ` Julien Grall
  2016-06-22 10:46   ` Stefano Stabellini
  2016-06-07 16:48 ` [RFC 2/8] xen/arm: gic: Do not configure affinity for guest IRQ during routing Julien Grall
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

The code to set the IRQ affinity is duplicated: once in
gicv{2,3}_set_properties and the other is gicv{2,3}_irq_set_affinity.

Remove the code from gicv{2,3}_set_properties and call directly the
affinity set helper from the common code.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v2.c     | 10 +---------
 xen/arch/arm/gic-v3.c     | 10 ----------
 xen/arch/arm/gic.c        |  3 ++-
 xen/include/asm-arm/gic.h |  1 -
 4 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 450755f..90b07b3 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -200,16 +200,10 @@ static unsigned int gicv2_read_irq(void)
     return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
 }
 
-/*
- * needs to be called with a valid cpu_mask, ie each cpu in the mask has
- * already called gic_cpu_init
- */
 static void gicv2_set_irq_properties(struct irq_desc *desc,
-                                   const cpumask_t *cpu_mask,
-                                   unsigned int priority)
+                                     unsigned int priority)
 {
     uint32_t cfg, actual, edgebit;
-    unsigned int mask = gicv2_cpu_mask(cpu_mask);
     unsigned int irq = desc->irq;
     unsigned int type = desc->arch.type;
 
@@ -240,8 +234,6 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
             IRQ_TYPE_LEVEL_HIGH;
     }
 
-    /* Set target CPU mask (RAZ/WI on uniprocessor) */
-    writeb_gicd(mask, GICD_ITARGETSR + irq);
     /* Set priority */
     writeb_gicd(priority, GICD_IPRIORITYR + irq);
 
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 9910877..c936c8a 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -472,13 +472,10 @@ static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
 }
 
 static void gicv3_set_irq_properties(struct irq_desc *desc,
-                                     const cpumask_t *cpu_mask,
                                      unsigned int priority)
 {
     uint32_t cfg, actual, edgebit;
-    uint64_t affinity;
     void __iomem *base;
-    unsigned int cpu = gicv3_get_cpu_from_mask(cpu_mask);
     unsigned int irq = desc->irq;
     unsigned int type = desc->arch.type;
 
@@ -516,13 +513,6 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
             IRQ_TYPE_LEVEL_HIGH;
     }
 
-    affinity = gicv3_mpidr_to_affinity(cpu);
-    /* Make sure we don't broadcast the interrupt */
-    affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
-
-    if ( irq >= NR_GIC_LOCAL_IRQS )
-        writeq_relaxed(affinity, (GICD + GICD_IROUTER + irq * 8));
-
     /* Set priority */
     if ( irq < NR_GIC_LOCAL_IRQS )
         writeb_relaxed(priority, GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + irq);
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 2bfe4de..8a1087b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -106,7 +106,8 @@ static void gic_set_irq_properties(struct irq_desc *desc,
                                    const cpumask_t *cpu_mask,
                                    unsigned int priority)
 {
-   gic_hw_ops->set_irq_properties(desc, cpu_mask, priority);
+    gic_hw_ops->set_irq_properties(desc, priority);
+    desc->handler->set_affinity(desc, cpu_mask);
 }
 
 /* Program the GIC to route an interrupt to the host (i.e. Xen)
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index cd97bb2..b931f98 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -331,7 +331,6 @@ struct gic_hw_operations {
     unsigned int (*read_irq)(void);
     /* Set IRQ property */
     void (*set_irq_properties)(struct irq_desc *desc,
-                               const cpumask_t *cpu_mask,
                                unsigned int priority);
     /* Send SGI */
     void (*send_SGI)(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
-- 
1.9.1


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

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

* [RFC 2/8] xen/arm: gic: Do not configure affinity for guest IRQ during routing
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
  2016-06-07 16:48 ` [RFC 1/8] xen/arm: gic: Consolidate the IRQ affinity set in a single place Julien Grall
@ 2016-06-07 16:48 ` Julien Grall
  2016-06-22 10:54   ` Stefano Stabellini
  2016-06-07 16:48 ` [RFC 3/8] xen/arm: gic: split set_irq_properties Julien Grall
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

The affinity of a guest IRQ is set every time the guest enable it (see
vgic_enable_irqs).

It is not necessary to set the affinity when the IRQ is routed to the
guest because Xen will never receive the IRQ until it hass been enabled
by the guest.

Signed-off-by: Julien grall <julien.grall@arm.com>
---
 xen/arch/arm/gic.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 8a1087b..f25381f 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -97,17 +97,13 @@ void gic_restore_state(struct vcpu *v)
 }
 
 /*
- * needs to be called with a valid cpu_mask, ie each cpu in the mask has
- * already called gic_cpu_init
  * - desc.lock must be held
  * - arch.type must be valid (i.e != IRQ_TYPE_INVALID)
  */
 static void gic_set_irq_properties(struct irq_desc *desc,
-                                   const cpumask_t *cpu_mask,
                                    unsigned int priority)
 {
     gic_hw_ops->set_irq_properties(desc, priority);
-    desc->handler->set_affinity(desc, cpu_mask);
 }
 
 /* Program the GIC to route an interrupt to the host (i.e. Xen)
@@ -123,7 +119,9 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
 
     desc->handler = gic_hw_ops->gic_host_irq_type;
 
-    gic_set_irq_properties(desc, cpu_mask, priority);
+    desc->handler->set_affinity(desc, cpu_mask);
+
+    gic_set_irq_properties(desc, priority);
 }
 
 /* Program the GIC to route an interrupt to a guest
@@ -155,7 +153,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
     desc->handler = gic_hw_ops->gic_guest_irq_type;
     set_bit(_IRQ_GUEST, &desc->status);
 
-    gic_set_irq_properties(desc, cpumask_of(v_target->processor), priority);
+    gic_set_irq_properties(desc, priority);
 
     p->desc = desc;
     res = 0;
-- 
1.9.1


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

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

* [RFC 3/8] xen/arm: gic: split set_irq_properties
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
  2016-06-07 16:48 ` [RFC 1/8] xen/arm: gic: Consolidate the IRQ affinity set in a single place Julien Grall
  2016-06-07 16:48 ` [RFC 2/8] xen/arm: gic: Do not configure affinity for guest IRQ during routing Julien Grall
@ 2016-06-07 16:48 ` Julien Grall
  2016-06-22 10:58   ` Stefano Stabellini
  2016-06-07 16:48 ` [RFC 4/8] xen/arm: gic: set_type: Pass the type in parameter rather than in desc->arch.type Julien Grall
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

The callback set_irq_properties will configure the GIC for a specific
IRQ with the type and the priority.

In a follow-up patch, Xen will configure the type and the priority at
different stage of the routing. So split it in 2 separate callbacks.

At the same time, move the ASSERT to check the validity of the type and
if the desc->lock is locked in the common code (gic.c). This is because
the constraint are the same between GICv2 and GICv3, however the driver
of the latter did not contain any sanity check.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v2.c     | 19 +++++++++++++------
 xen/arch/arm/gic-v3.c     | 15 ++++++++++++---
 xen/arch/arm/gic.c        | 23 ++++++++++++++---------
 xen/include/asm-arm/gic.h |  7 ++++---
 4 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 90b07b3..fa2c5a5 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -200,16 +200,12 @@ static unsigned int gicv2_read_irq(void)
     return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
 }
 
-static void gicv2_set_irq_properties(struct irq_desc *desc,
-                                     unsigned int priority)
+static void gicv2_set_irq_type(struct irq_desc *desc)
 {
     uint32_t cfg, actual, edgebit;
     unsigned int irq = desc->irq;
     unsigned int type = desc->arch.type;
 
-    ASSERT(type != IRQ_TYPE_INVALID);
-    ASSERT(spin_is_locked(&desc->lock));
-
     spin_lock(&gicv2.lock);
     /* Set edge / level */
     cfg = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
@@ -234,6 +230,16 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
             IRQ_TYPE_LEVEL_HIGH;
     }
 
+    spin_unlock(&gicv2.lock);
+}
+
+static void gicv2_set_irq_priority(struct irq_desc *desc,
+                                   unsigned int priority)
+{
+    unsigned int irq = desc->irq;
+
+    spin_lock(&gicv2.lock);
+
     /* Set priority */
     writeb_gicd(priority, GICD_IPRIORITYR + irq);
 
@@ -916,7 +922,8 @@ const static struct gic_hw_operations gicv2_ops = {
     .eoi_irq             = gicv2_eoi_irq,
     .deactivate_irq      = gicv2_dir_irq,
     .read_irq            = gicv2_read_irq,
-    .set_irq_properties  = gicv2_set_irq_properties,
+    .set_irq_type        = gicv2_set_irq_type,
+    .set_irq_priority    = gicv2_set_irq_priority,
     .send_SGI            = gicv2_send_SGI,
     .disable_interface   = gicv2_disable_interface,
     .update_lr           = gicv2_update_lr,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c936c8a..c25fe50 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -471,8 +471,7 @@ static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
              MPIDR_AFFINITY_LEVEL(mpidr, 0));
 }
 
-static void gicv3_set_irq_properties(struct irq_desc *desc,
-                                     unsigned int priority)
+static void gicv3_set_irq_type(struct irq_desc *desc)
 {
     uint32_t cfg, actual, edgebit;
     void __iomem *base;
@@ -512,6 +511,15 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
             IRQ_TYPE_EDGE_RISING :
             IRQ_TYPE_LEVEL_HIGH;
     }
+    spin_unlock(&gicv3.lock);
+}
+
+static void gicv3_set_irq_priority(struct irq_desc *desc,
+                                   unsigned int priority)
+{
+    unsigned int irq = desc->irq;
+
+    spin_lock(&gicv3.lock);
 
     /* Set priority */
     if ( irq < NR_GIC_LOCAL_IRQS )
@@ -1547,7 +1555,8 @@ static const struct gic_hw_operations gicv3_ops = {
     .eoi_irq             = gicv3_eoi_irq,
     .deactivate_irq      = gicv3_dir_irq,
     .read_irq            = gicv3_read_irq,
-    .set_irq_properties  = gicv3_set_irq_properties,
+    .set_irq_type        = gicv3_set_irq_type,
+    .set_irq_priority    = gicv3_set_irq_priority,
     .send_SGI            = gicv3_send_sgi,
     .disable_interface   = gicv3_disable_interface,
     .update_lr           = gicv3_update_lr,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index f25381f..0fd7e8c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -96,14 +96,17 @@ void gic_restore_state(struct vcpu *v)
     gic_restore_pending_irqs(v);
 }
 
-/*
- * - desc.lock must be held
- * - arch.type must be valid (i.e != IRQ_TYPE_INVALID)
- */
-static void gic_set_irq_properties(struct irq_desc *desc,
-                                   unsigned int priority)
+static void gic_set_irq_type(struct irq_desc *desc)
+{
+    ASSERT(spin_is_locked(&desc->lock));
+    ASSERT(desc->arch.type != IRQ_TYPE_INVALID);
+
+    gic_hw_ops->set_irq_type(desc);
+}
+
+static void gic_set_irq_priority(struct irq_desc *desc, unsigned int priority)
 {
-    gic_hw_ops->set_irq_properties(desc, priority);
+    gic_hw_ops->set_irq_priority(desc, priority);
 }
 
 /* Program the GIC to route an interrupt to the host (i.e. Xen)
@@ -121,7 +124,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
 
     desc->handler->set_affinity(desc, cpu_mask);
 
-    gic_set_irq_properties(desc, priority);
+    gic_set_irq_type(desc);
+    gic_set_irq_priority(desc, priority);
 }
 
 /* Program the GIC to route an interrupt to a guest
@@ -153,7 +157,8 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
     desc->handler = gic_hw_ops->gic_guest_irq_type;
     set_bit(_IRQ_GUEST, &desc->status);
 
-    gic_set_irq_properties(desc, priority);
+    gic_set_irq_type(desc);
+    gic_set_irq_priority(desc, priority);
 
     p->desc = desc;
     res = 0;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index b931f98..ffba469 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -329,9 +329,10 @@ struct gic_hw_operations {
     void (*deactivate_irq)(struct irq_desc *irqd);
     /* Read IRQ id and Ack */
     unsigned int (*read_irq)(void);
-    /* Set IRQ property */
-    void (*set_irq_properties)(struct irq_desc *desc,
-                               unsigned int priority);
+    /* Set IRQ type - type is taken from desc->arch.type */
+    void (*set_irq_type)(struct irq_desc *desc);
+    /* Set IRQ priority */
+    void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
     /* Send SGI */
     void (*send_SGI)(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
                      const cpumask_t *online_mask);
-- 
1.9.1


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

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

* [RFC 4/8] xen/arm: gic: set_type: Pass the type in parameter rather than in desc->arch.type
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
                   ` (2 preceding siblings ...)
  2016-06-07 16:48 ` [RFC 3/8] xen/arm: gic: split set_irq_properties Julien Grall
@ 2016-06-07 16:48 ` Julien Grall
  2016-06-22 11:25   ` Stefano Stabellini
  2016-06-07 16:48 ` [RFC 5/8] xen/arm: gic: Document how gic_set_irq_type should be called Julien Grall
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

A follow-up patch will not store the type in desc->arch.type. Also, the
callback prototype is more logical.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v2.c     | 3 +--
 xen/arch/arm/gic-v3.c     | 3 +--
 xen/arch/arm/gic.c        | 9 ++++-----
 xen/include/asm-arm/gic.h | 4 ++--
 4 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index fa2c5a5..399b869 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -200,11 +200,10 @@ static unsigned int gicv2_read_irq(void)
     return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
 }
 
-static void gicv2_set_irq_type(struct irq_desc *desc)
+static void gicv2_set_irq_type(struct irq_desc *desc, unsigned int type)
 {
     uint32_t cfg, actual, edgebit;
     unsigned int irq = desc->irq;
-    unsigned int type = desc->arch.type;
 
     spin_lock(&gicv2.lock);
     /* Set edge / level */
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c25fe50..d0a7a23 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -471,12 +471,11 @@ static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
              MPIDR_AFFINITY_LEVEL(mpidr, 0));
 }
 
-static void gicv3_set_irq_type(struct irq_desc *desc)
+static void gicv3_set_irq_type(struct irq_desc *desc, unsigned int type)
 {
     uint32_t cfg, actual, edgebit;
     void __iomem *base;
     unsigned int irq = desc->irq;
-    unsigned int type = desc->arch.type;
 
     /* SGI's are always edge-triggered not need to call GICD_ICFGR0 */
     ASSERT(irq >= NR_GIC_SGI);
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 0fd7e8c..8b992d8 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -96,12 +96,12 @@ void gic_restore_state(struct vcpu *v)
     gic_restore_pending_irqs(v);
 }
 
-static void gic_set_irq_type(struct irq_desc *desc)
+static void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
 {
     ASSERT(spin_is_locked(&desc->lock));
-    ASSERT(desc->arch.type != IRQ_TYPE_INVALID);
+    ASSERT(type != IRQ_TYPE_INVALID);
 
-    gic_hw_ops->set_irq_type(desc);
+    gic_hw_ops->set_irq_type(desc, type);
 }
 
 static void gic_set_irq_priority(struct irq_desc *desc, unsigned int priority)
@@ -124,7 +124,6 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
 
     desc->handler->set_affinity(desc, cpu_mask);
 
-    gic_set_irq_type(desc);
     gic_set_irq_priority(desc, priority);
 }
 
@@ -157,7 +156,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
     desc->handler = gic_hw_ops->gic_guest_irq_type;
     set_bit(_IRQ_GUEST, &desc->status);
 
-    gic_set_irq_type(desc);
+    gic_set_irq_type(desc, desc->arch.type);
     gic_set_irq_priority(desc, priority);
 
     p->desc = desc;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ffba469..ddc45a8 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -329,8 +329,8 @@ struct gic_hw_operations {
     void (*deactivate_irq)(struct irq_desc *irqd);
     /* Read IRQ id and Ack */
     unsigned int (*read_irq)(void);
-    /* Set IRQ type - type is taken from desc->arch.type */
-    void (*set_irq_type)(struct irq_desc *desc);
+    /* Set IRQ type */
+    void (*set_irq_type)(struct irq_desc *desc, unsigned int type);
     /* Set IRQ priority */
     void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
     /* Send SGI */
-- 
1.9.1


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

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

* [RFC 5/8] xen/arm: gic: Document how gic_set_irq_type should be called
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
                   ` (3 preceding siblings ...)
  2016-06-07 16:48 ` [RFC 4/8] xen/arm: gic: set_type: Pass the type in parameter rather than in desc->arch.type Julien Grall
@ 2016-06-07 16:48 ` Julien Grall
  2016-06-22 11:00   ` Stefano Stabellini
  2016-06-07 16:48 ` [RFC 6/8] Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on ACPI" Julien Grall
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

Changing the value of Int_config is UNPREDICTABLE when the corresponding
interrupt is not disabled.

The driver is assuming the interrupt will be disabled by the caller of
gic_set_irq_type. Add an ASSERT to ensure it.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 8b992d8..27cd177 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -96,8 +96,14 @@ void gic_restore_state(struct vcpu *v)
     gic_restore_pending_irqs(v);
 }
 
+/* desc->irq needs to be disabled before calling this function */
 static void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
 {
+    /*
+     * IRQ must be disabled before configuring it (see 4.3.13 in ARM IHI
+     * 0048B.b). We rely on the caller to do it.
+     */
+    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
     ASSERT(spin_is_locked(&desc->lock));
     ASSERT(type != IRQ_TYPE_INVALID);
 
-- 
1.9.1


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

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

* [RFC 6/8] Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on ACPI"
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
                   ` (4 preceding siblings ...)
  2016-06-07 16:48 ` [RFC 5/8] xen/arm: gic: Document how gic_set_irq_type should be called Julien Grall
@ 2016-06-07 16:48 ` Julien Grall
  2016-06-22 11:01   ` Stefano Stabellini
  2016-06-07 16:48 ` [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse Julien Grall
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

This reverts commit f91c84edebe67296e4051af055dbf0adafb13a37. SPI
routing for ACPI support will be added in a follow-up patch.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/vgic.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 413ff16..ee35683 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -25,8 +25,6 @@
 #include <xen/irq.h>
 #include <xen/sched.h>
 #include <xen/perfc.h>
-#include <xen/iocap.h>
-#include <xen/acpi.h>
 
 #include <asm/current.h>
 
@@ -344,22 +342,9 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     unsigned long flags;
     int i = 0;
     struct vcpu *v_target;
-    struct domain *d = v->domain;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
-        /* Set the irq type and route it to guest only for SPI and Dom0 */
-        if( irq_access_permitted(d, irq) && is_hardware_domain(d) &&
-            ( irq >= 32 ) && ( !acpi_disabled ) )
-        {
-            static int log_once = 0;
-            if ( !log_once )
-            {
-                gprintk(XENLOG_WARNING, "Routing SPIs to Dom0 on ACPI systems is unimplemented.\n");
-                log_once++;
-            }
-        }
-
         v_target = __vgic_get_target_vcpu(v, irq);
         p = irq_to_pending(v_target, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-- 
1.9.1


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

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

* [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
                   ` (5 preceding siblings ...)
  2016-06-07 16:48 ` [RFC 6/8] Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on ACPI" Julien Grall
@ 2016-06-07 16:48 ` Julien Grall
  2016-06-22 11:23   ` Stefano Stabellini
  2016-06-07 16:48 ` [RFC 8/8] xen/arm: acpi: route all unused IRQs to DOM0 Julien Grall
  2016-06-07 18:50 ` [RFC 0/8] xen/arm: acpi: Support SPIs routing Shanker Donthineni
  8 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

The function route_irq_to_guest mandates the IRQ type, stored in
desc->arch.type, to be valid. However, in case of ACPI, these
information is not part of the static tables. Therefore Xen needs to
rely on DOM0 to provide a valid type based on the firmware tables.

A new helper, irq_type_set_by_domain is provided to check whether a
domain is allowed to set the IRQ type. For now, only DOM0 is allowed to
configure it when ACPI is inuse.

When the helper returns 1, the routing function will not check whether
the IRQ type is correctly set and configure the GIC. Instead, this will
be done when the domain will enable the interrupt.

Note that irq_set_spi_type is not called because it validates the type
and does not allow it the domain to change the type after the first
write. It means that desc->arch.type will never be set, which is fine
because the field is only used to configure the type during the routing.

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

---

I am thinking to at least extend the behavior to DOM0 using DT. This
would make us resilient to a DT not providing the correct type without
having to workaround them in Xen.

Furthermore, it might be possible to let any domain configuring the IRQ
type (could be useful when passthrough an IRQ with ACPI). However, we would
need to consider any potential security impact beforehand.
---
 xen/arch/arm/gic.c        |  5 +++--
 xen/arch/arm/irq.c        | 14 +++++++++++++-
 xen/arch/arm/vgic.c       | 21 +++++++++++++++++++++
 xen/include/asm-arm/gic.h |  3 +++
 xen/include/asm-arm/irq.h |  6 ++++++
 5 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 27cd177..80d93a8 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -97,7 +97,7 @@ void gic_restore_state(struct vcpu *v)
 }
 
 /* desc->irq needs to be disabled before calling this function */
-static void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
+void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
 {
     /*
      * IRQ must be disabled before configuring it (see 4.3.13 in ARM IHI
@@ -162,7 +162,8 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
     desc->handler = gic_hw_ops->gic_guest_irq_type;
     set_bit(_IRQ_GUEST, &desc->status);
 
-    gic_set_irq_type(desc, desc->arch.type);
+    if ( !irq_type_set_by_domain(d) )
+        gic_set_irq_type(desc, desc->arch.type);
     gic_set_irq_priority(desc, priority);
 
     p->desc = desc;
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 2f8af72..b9d7749 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -27,6 +27,7 @@
 
 #include <asm/gic.h>
 #include <asm/vgic.h>
+#include <asm/acpi.h>
 
 static unsigned int local_irqs_type[NR_LOCAL_IRQS];
 static DEFINE_SPINLOCK(local_irqs_type_lock);
@@ -395,6 +396,17 @@ bool_t is_assignable_irq(unsigned int irq)
 }
 
 /*
+ * When ACPI is inuse, only the hardware domain knows the interrupt
+ * type.
+ *
+ * XXX: See whether it is possible to let any domain configure the type.
+ */
+bool_t irq_type_set_by_domain(const struct domain *d)
+{
+    return ((d == hardware_domain) && !acpi_disabled);
+}
+
+/*
  * Route an IRQ to a specific guest.
  * For now only SPIs are assignable to the guest.
  */
@@ -449,7 +461,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
 
     spin_lock_irqsave(&desc->lock, flags);
 
-    if ( desc->arch.type == IRQ_TYPE_INVALID )
+    if ( desc->arch.type == IRQ_TYPE_INVALID && !irq_type_set_by_domain(d) )
     {
         printk(XENLOG_G_ERR "IRQ %u has not been configured\n", irq);
         retval = -EIO;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index ee35683..3e1c572 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -249,6 +249,24 @@ static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
     return priority;
 }
 
+#define NR_CFG_PER_ICFGR 16
+#define NR_BITS_PER_CFG (32U / NR_CFG_PER_ICFGR)
+
+/* The function should be called witht the rank lock taken. */
+static int __vgic_get_virq_type(struct vcpu *v, unsigned int virq)
+{
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
+    uint8_t index = virq & INTERRUPT_RANK_MASK;
+    uint32_t reg = rank->icfg[index / NR_CFG_PER_ICFGR];
+    uint8_t val;
+
+    ASSERT(spin_is_locked(&rank->lock));
+
+    val = reg >> ((index % NR_CFG_PER_ICFGR) * NR_BITS_PER_CFG);
+
+    return (val & 0x2) ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_HIGH;
+}
+
 void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
 {
     unsigned long flags;
@@ -342,6 +360,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     unsigned long flags;
     int i = 0;
     struct vcpu *v_target;
+    struct domain *d = v->domain;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
@@ -356,6 +375,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         {
             irq_set_affinity(p->desc, cpumask_of(v_target->processor));
             spin_lock_irqsave(&p->desc->lock, flags);
+            if ( irq_type_set_by_domain(d) )
+                gic_set_irq_type(p->desc, __vgic_get_virq_type(v, irq));
             p->desc->handler->enable(p->desc);
             spin_unlock_irqrestore(&p->desc->lock, flags);
         }
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ddc45a8..44b9ef6 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -222,6 +222,9 @@ enum gic_version {
 
 extern enum gic_version gic_hw_version(void);
 
+/* Program the IRQ type into the GIC */
+void gic_set_irq_type(struct irq_desc *desc, unsigned int type);
+
 /* Program the GIC to route an interrupt */
 extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
                                  unsigned int priority);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 493773c..8f7a167 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -58,6 +58,12 @@ int platform_get_irq(const struct dt_device_node *device, int index);
 
 void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
 
+/*
+ * Use this helper in places that need to know whether the IRQ type is
+ * set by the domain.
+ */
+bool_t irq_type_set_by_domain(const struct domain *d);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
-- 
1.9.1


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

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

* [RFC 8/8] xen/arm: acpi: route all unused IRQs to DOM0
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
                   ` (6 preceding siblings ...)
  2016-06-07 16:48 ` [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse Julien Grall
@ 2016-06-07 16:48 ` Julien Grall
  2016-06-22 11:44   ` Stefano Stabellini
  2016-06-07 18:50 ` [RFC 0/8] xen/arm: acpi: Support SPIs routing Shanker Donthineni
  8 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-07 16:48 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.chen, steve.capper, Julien Grall, shannon.zhao,
	shankerd

It is not possible to know which IRQs will be used by DOM0 when ACPI is
inuse. The approach implemented by this patch, will route all unused
IRQs to DOM0 before it has booted.

The number of IRQs routed is based on the maximum SPIs supported by the
hardware (up to ~1000). However, some of them might not be wired. So we
would allocate resource for nothing.

For each IRQ routed, Xen is allocating memory for irqaction (40 bytes)
and irq_guest (16 bytes). So in the worst case scenario ~54KB of memory
will be allocated. Given that ACPI will mostly be used by server, I
think it is a small drawback.

map_irq_to_domain is slightly reworked to remove the dependency on
device-tree. So the function can be also be used for ACPI and will
avoid code duplication.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/domain_build.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 00dc07a..76d503d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -953,11 +953,10 @@ static int make_timer_node(const struct domain *d, void *fdt,
     return res;
 }
 
-static int map_irq_to_domain(const struct dt_device_node *dev,
-                             struct domain *d, unsigned int irq)
+static int map_irq_to_domain(struct domain *d, unsigned int irq,
+                             bool_t need_mapping, const char *devname)
 
 {
-    bool_t need_mapping = !dt_device_for_passthrough(dev);
     int res;
 
     res = irq_permit_access(d, irq);
@@ -977,7 +976,7 @@ static int map_irq_to_domain(const struct dt_device_node *dev,
          */
         vgic_reserve_virq(d, irq);
 
-        res = route_irq_to_guest(d, irq, irq, dt_node_name(dev));
+        res = route_irq_to_guest(d, irq, irq, devname);
         if ( res < 0 )
         {
             printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
@@ -997,6 +996,7 @@ static int map_dt_irq_to_domain(const struct dt_device_node *dev,
     struct domain *d = data;
     unsigned int irq = dt_irq->irq;
     int res;
+    bool_t need_mapping = !dt_device_for_passthrough(dev);
 
     if ( irq < NR_LOCAL_IRQS )
     {
@@ -1015,7 +1015,7 @@ static int map_dt_irq_to_domain(const struct dt_device_node *dev,
         return res;
     }
 
-    res = map_irq_to_domain(dev, d, irq);
+    res = map_irq_to_domain(d, irq, need_mapping, dt_node_name(dev));
 
     return 0;
 }
@@ -1152,7 +1152,7 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
             return res;
         }
 
-        res = map_irq_to_domain(dev, d, res);
+        res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
         if ( res )
             return res;
     }
@@ -1411,13 +1411,10 @@ static int acpi_permit_spi_access(struct domain *d)
         if ( desc->action != NULL)
             continue;
 
-        res = irq_permit_access(d, i);
+        /* XXX: Shall we use a proper devname? */
+        res = map_irq_to_domain(d, i, true, "ACPI");
         if ( res )
-        {
-            printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
-                   d->domain_id, i);
             return res;
-        }
     }
 
     return 0;
-- 
1.9.1


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

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

* Re: [RFC 0/8] xen/arm: acpi: Support SPIs routing
  2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
                   ` (7 preceding siblings ...)
  2016-06-07 16:48 ` [RFC 8/8] xen/arm: acpi: route all unused IRQs to DOM0 Julien Grall
@ 2016-06-07 18:50 ` Shanker Donthineni
  2016-06-08 11:48   ` Shanker Donthineni
  8 siblings, 1 reply; 29+ messages in thread
From: Shanker Donthineni @ 2016-06-07 18:50 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: sstabellini, shannon.zhao, wei.chen, steve.capper

Hi Julien,

Sure, I'll test this patchset on Qualcomm Technologies QDF2XXX platform and update results.


On 06/07/2016 11:48 AM, Julien Grall wrote:
> Hi all,
>
> Currently, Xen does not route SPIs to DOM0 when ACPI is inuse after
> the functionality has been reverted in Xen 4.7 by commit 909bd14.
>
> In the previous approach, the SPIs was routed when DOM0 was writing into
> ISENABLER. However, this has resulted to deadlock (see more details in [1])
> as soon as the IRQ was enabled by DOM0.
>
> We have multiple solutions to route the IRQ:
>  1) Rework route_irq_to_guest to avoid the deadlock
>  2) Route and enable the IRQ outside of the emulation of ISENABLER
>  3) Remove the dependency on the IRQ type in the routing function
>  and route all the unused IRQs during domain building
>  4) Add a new hypercall to let DOM0 routing the IRQ
>
> I think that 1) and 2) are not resilient because route_irq_to_guest may fail
> and there is no way to report this error to the guest (except by killing it).
>
> Furthermore, in solution 2) enabling the interrupt would need to be defer
> until the routing has been done. This would require a lot of code duplication.
>
> Which leave solution 3) and 4). The solution 4) requires to introduce a new
> (or re-use one) stable hypercall. I am not sure why we ruled out this
> solution when we reviewed the ACPI design document.
>
> This patch series is implementing the 3rd solution which defer the IRQ
> type configuration for DOM0 IRQ when ACPI is inuse. However, this will
> slightly increase the memory usage of Xen (54KB).
>
> I am happy to consider any other solutions.
>
> I only tested briefly this patch series, Shanker can you give a try on
> your hardware?
>
> A branch with all the patches can be found here:
>
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch irq-routing-acpi-rfc
>
> Yours sincerely,
>
> [1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02633.html
>
> Julien Grall (8):
>   xen/arm: gic: Consolidate the IRQ affinity set in a single place
>   xen/arm: gic: Do not configure affinity for guest IRQ during routing
>   xen/arm: gic: split set_irq_properties
>   xen/arm: gic: set_type: Pass the type in parameter rather than in
>     desc->arch.type
>   xen/arm: gic: Document how gic_set_irq_type should be called
>   Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on
>     ACPI"
>   xen/arm: Allow DOM0 to set the irq type when ACPI is inuse
>   xen/arm: acpi: route all unused IRQs to DOM0
>
>  xen/arch/arm/domain_build.c | 19 ++++++++-----------
>  xen/arch/arm/gic-v2.c       | 28 +++++++++++++---------------
>  xen/arch/arm/gic-v3.c       | 22 ++++++++++------------
>  xen/arch/arm/gic.c          | 34 ++++++++++++++++++++++------------
>  xen/arch/arm/irq.c          | 14 +++++++++++++-
>  xen/arch/arm/vgic.c         | 34 ++++++++++++++++++++--------------
>  xen/include/asm-arm/gic.h   | 11 +++++++----
>  xen/include/asm-arm/irq.h   |  6 ++++++
>  8 files changed, 99 insertions(+), 69 deletions(-)
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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

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

* Re: [RFC 0/8] xen/arm: acpi: Support SPIs routing
  2016-06-07 18:50 ` [RFC 0/8] xen/arm: acpi: Support SPIs routing Shanker Donthineni
@ 2016-06-08 11:48   ` Shanker Donthineni
  2016-06-08 11:49     ` Julien Grall
  0 siblings, 1 reply; 29+ messages in thread
From: Shanker Donthineni @ 2016-06-08 11:48 UTC (permalink / raw)
  To: 'Julien Grall', xen-devel
  Cc: sstabellini, shannon.zhao, wei.chen, steve.capper

Hi Julien,

Tested on our hardware, this patchset fixed the deadlock issue but
some of the SPIs are  not working.

--
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center,
Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project



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

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

* Re: [RFC 0/8] xen/arm: acpi: Support SPIs routing
  2016-06-08 11:48   ` Shanker Donthineni
@ 2016-06-08 11:49     ` Julien Grall
  2016-06-08 12:11       ` Shanker Donthineni
  0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-08 11:49 UTC (permalink / raw)
  To: Shanker Donthineni, xen-devel
  Cc: sstabellini, shannon.zhao, wei.chen, steve.capper



On 08/06/16 12:48, Shanker Donthineni wrote:
> Hi Julien,
>
> Tested on our hardware, this patchset fixed the deadlock issue but
> some of the SPIs are  not working.

Can you give a bit more of details?

Regards,

-- 
Julien Grall

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

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

* Re: [RFC 0/8] xen/arm: acpi: Support SPIs routing
  2016-06-08 11:49     ` Julien Grall
@ 2016-06-08 12:11       ` Shanker Donthineni
  2016-06-08 12:34         ` Julien Grall
  0 siblings, 1 reply; 29+ messages in thread
From: Shanker Donthineni @ 2016-06-08 12:11 UTC (permalink / raw)
  To: 'Julien Grall', xen-devel
  Cc: sstabellini, shannon.zhao, wei.chen, steve.capper

I don't know exactly which of the patch causing the issue. I have
noticed a couple of drivers are not receiving SPI interrupts in DOM0.
I need to digest your patches before tracing/investigate the problem
to get more details.

FYI, our system uses both the EDGE and LEVEL interrupts, any major
change in your patches?
--
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center,
Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project


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

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

* Re: [RFC 0/8] xen/arm: acpi: Support SPIs routing
  2016-06-08 12:11       ` Shanker Donthineni
@ 2016-06-08 12:34         ` Julien Grall
  2016-06-13 11:42           ` Julien Grall
  0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-08 12:34 UTC (permalink / raw)
  To: Shanker Donthineni, xen-devel
  Cc: sstabellini, shannon.zhao, wei.chen, steve.capper

Hello Shanker,

On 08/06/16 13:11, Shanker Donthineni wrote:
> I don't know exactly which of the patch causing the issue. I have
> noticed a couple of drivers are not receiving SPI interrupts in DOM0.
> I need to digest your patches before tracing/investigate the problem
> to get more details.
>
> FYI, our system uses both the EDGE and LEVEL interrupts, any major
> change in your patches?

The routing is done when DOM0 is built and the interrupt configuration 
is deferred.

It might be possible to the wrong type is retrieve by __vgic_get_virq_type.

I would recommend you to add a print in vgic_enable_irqs to check the 
value and if p->desc != NULL for those IRQs.

Regards,

-- 
Julien Grall

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

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

* Re: [RFC 0/8] xen/arm: acpi: Support SPIs routing
  2016-06-08 12:34         ` Julien Grall
@ 2016-06-13 11:42           ` Julien Grall
  2016-06-13 17:19             ` Shanker Donthineni
  0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-13 11:42 UTC (permalink / raw)
  To: Shanker Donthineni, xen-devel
  Cc: sstabellini, shannon.zhao, wei.chen, steve.capper



On 08/06/16 13:34, Julien Grall wrote:
> On 08/06/16 13:11, Shanker Donthineni wrote:
>> I don't know exactly which of the patch causing the issue. I have
>> noticed a couple of drivers are not receiving SPI interrupts in DOM0.
>> I need to digest your patches before tracing/investigate the problem
>> to get more details.
>>
>> FYI, our system uses both the EDGE and LEVEL interrupts, any major
>> change in your patches?
>
> The routing is done when DOM0 is built and the interrupt configuration
> is deferred.
>
> It might be possible to the wrong type is retrieve by __vgic_get_virq_type.

I looked at my code again today and add some printk. The IRQ will always 
be configured correctly with the correct type.

It would be good to know if the problem only happen with edge or level 
interrupts.

Regards,

-- 
Julien Grall

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

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

* Re: [RFC 0/8] xen/arm: acpi: Support SPIs routing
  2016-06-13 11:42           ` Julien Grall
@ 2016-06-13 17:19             ` Shanker Donthineni
  2016-06-13 17:20               ` Julien Grall
  0 siblings, 1 reply; 29+ messages in thread
From: Shanker Donthineni @ 2016-06-13 17:19 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: sstabellini, shannon.zhao, wei.chen, steve.capper

Julien,

The problem that was reported earlier related to our platform drivers in 
domn0 kernel. I have verified your patches and works fine without any 
dead-lock.


On 06/13/2016 06:42 AM, Julien Grall wrote:
>
>
> On 08/06/16 13:34, Julien Grall wrote:
>> On 08/06/16 13:11, Shanker Donthineni wrote:
>>> I don't know exactly which of the patch causing the issue. I have
>>> noticed a couple of drivers are not receiving SPI interrupts in DOM0.
>>> I need to digest your patches before tracing/investigate the problem
>>> to get more details.
>>>
>>> FYI, our system uses both the EDGE and LEVEL interrupts, any major
>>> change in your patches?
>>
>> The routing is done when DOM0 is built and the interrupt configuration
>> is deferred.
>>
>> It might be possible to the wrong type is retrieve by 
>> __vgic_get_virq_type.
>
> I looked at my code again today and add some printk. The IRQ will 
> always be configured correctly with the correct type.
>
> It would be good to know if the problem only happen with edge or level 
> interrupts.
>
> Regards,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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

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

* Re: [RFC 0/8] xen/arm: acpi: Support SPIs routing
  2016-06-13 17:19             ` Shanker Donthineni
@ 2016-06-13 17:20               ` Julien Grall
  0 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2016-06-13 17:20 UTC (permalink / raw)
  To: shankerd, xen-devel; +Cc: sstabellini, shannon.zhao, wei.chen, steve.capper

On 13/06/16 18:19, Shanker Donthineni wrote:
> Julien,

Hello,

Please avoid top-posting.

> The problem that was reported earlier related to our platform drivers in
> domn0 kernel. I have verified your patches and works fine without any
> dead-lock.

Good, thank you!

Regards,

-- 
Julien Grall

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

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

* Re: [RFC 1/8] xen/arm: gic: Consolidate the IRQ affinity set in a single place
  2016-06-07 16:48 ` [RFC 1/8] xen/arm: gic: Consolidate the IRQ affinity set in a single place Julien Grall
@ 2016-06-22 10:46   ` Stefano Stabellini
  0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 10:46 UTC (permalink / raw)
  To: Julien Grall
  Cc: sstabellini, wei.chen, steve.capper, xen-devel, shannon.zhao, shankerd

On Tue, 7 Jun 2016, Julien Grall wrote:
> The code to set the IRQ affinity is duplicated: once in
> gicv{2,3}_set_properties and the other is gicv{2,3}_irq_set_affinity.
> 
> Remove the code from gicv{2,3}_set_properties and call directly the
> affinity set helper from the common code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  xen/arch/arm/gic-v2.c     | 10 +---------
>  xen/arch/arm/gic-v3.c     | 10 ----------
>  xen/arch/arm/gic.c        |  3 ++-
>  xen/include/asm-arm/gic.h |  1 -
>  4 files changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 450755f..90b07b3 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -200,16 +200,10 @@ static unsigned int gicv2_read_irq(void)
>      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
>  }
>  
> -/*
> - * needs to be called with a valid cpu_mask, ie each cpu in the mask has
> - * already called gic_cpu_init
> - */
>  static void gicv2_set_irq_properties(struct irq_desc *desc,
> -                                   const cpumask_t *cpu_mask,
> -                                   unsigned int priority)
> +                                     unsigned int priority)
>  {
>      uint32_t cfg, actual, edgebit;
> -    unsigned int mask = gicv2_cpu_mask(cpu_mask);
>      unsigned int irq = desc->irq;
>      unsigned int type = desc->arch.type;
>  
> @@ -240,8 +234,6 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
>              IRQ_TYPE_LEVEL_HIGH;
>      }
>  
> -    /* Set target CPU mask (RAZ/WI on uniprocessor) */
> -    writeb_gicd(mask, GICD_ITARGETSR + irq);
>      /* Set priority */
>      writeb_gicd(priority, GICD_IPRIORITYR + irq);
>  
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 9910877..c936c8a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -472,13 +472,10 @@ static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
>  }
>  
>  static void gicv3_set_irq_properties(struct irq_desc *desc,
> -                                     const cpumask_t *cpu_mask,
>                                       unsigned int priority)
>  {
>      uint32_t cfg, actual, edgebit;
> -    uint64_t affinity;
>      void __iomem *base;
> -    unsigned int cpu = gicv3_get_cpu_from_mask(cpu_mask);
>      unsigned int irq = desc->irq;
>      unsigned int type = desc->arch.type;
>  
> @@ -516,13 +513,6 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
>              IRQ_TYPE_LEVEL_HIGH;
>      }
>  
> -    affinity = gicv3_mpidr_to_affinity(cpu);
> -    /* Make sure we don't broadcast the interrupt */
> -    affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
> -
> -    if ( irq >= NR_GIC_LOCAL_IRQS )
> -        writeq_relaxed(affinity, (GICD + GICD_IROUTER + irq * 8));
> -
>      /* Set priority */
>      if ( irq < NR_GIC_LOCAL_IRQS )
>          writeb_relaxed(priority, GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + irq);
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 2bfe4de..8a1087b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -106,7 +106,8 @@ static void gic_set_irq_properties(struct irq_desc *desc,
>                                     const cpumask_t *cpu_mask,
>                                     unsigned int priority)
>  {
> -   gic_hw_ops->set_irq_properties(desc, cpu_mask, priority);
> +    gic_hw_ops->set_irq_properties(desc, priority);
> +    desc->handler->set_affinity(desc, cpu_mask);
>  }
>  
>  /* Program the GIC to route an interrupt to the host (i.e. Xen)
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index cd97bb2..b931f98 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -331,7 +331,6 @@ struct gic_hw_operations {
>      unsigned int (*read_irq)(void);
>      /* Set IRQ property */
>      void (*set_irq_properties)(struct irq_desc *desc,
> -                               const cpumask_t *cpu_mask,
>                                 unsigned int priority);
>      /* Send SGI */
>      void (*send_SGI)(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
> -- 
> 1.9.1
> 

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

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

* Re: [RFC 2/8] xen/arm: gic: Do not configure affinity for guest IRQ during routing
  2016-06-07 16:48 ` [RFC 2/8] xen/arm: gic: Do not configure affinity for guest IRQ during routing Julien Grall
@ 2016-06-22 10:54   ` Stefano Stabellini
  2016-06-22 11:19     ` Julien Grall
  0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 10:54 UTC (permalink / raw)
  To: Julien Grall
  Cc: sstabellini, wei.chen, steve.capper, xen-devel, shannon.zhao, shankerd

On Tue, 7 Jun 2016, Julien Grall wrote:
> The affinity of a guest IRQ is set every time the guest enable it (see
> vgic_enable_irqs).
> 
> It is not necessary to set the affinity when the IRQ is routed to the
> guest because Xen will never receive the IRQ until it hass been enabled
> by the guest.
> 
> Signed-off-by: Julien grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/gic.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 8a1087b..f25381f 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -97,17 +97,13 @@ void gic_restore_state(struct vcpu *v)
>  }
>  
>  /*
> - * needs to be called with a valid cpu_mask, ie each cpu in the mask has
> - * already called gic_cpu_init
>   * - desc.lock must be held
>   * - arch.type must be valid (i.e != IRQ_TYPE_INVALID)
>   */
>  static void gic_set_irq_properties(struct irq_desc *desc,
> -                                   const cpumask_t *cpu_mask,
>                                     unsigned int priority)
>  {
>      gic_hw_ops->set_irq_properties(desc, priority);
> -    desc->handler->set_affinity(desc, cpu_mask);
>  }
>  
>  /* Program the GIC to route an interrupt to the host (i.e. Xen)
> @@ -123,7 +119,9 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>  
>      desc->handler = gic_hw_ops->gic_host_irq_type;
>  
> -    gic_set_irq_properties(desc, cpu_mask, priority);
> +    desc->handler->set_affinity(desc, cpu_mask);

You could call irq_set_affinity here, it might make for nicer code.

Actually thinking more about this, I think it would be better to add the
irq_set_affinity call to xen/arch/arm/irq.c:setup_irq, right after the
call to gic_route_irq_to_xen.  That way both gic_route_irq_to_xen and
gic_route_irq_to_guest would behave the same way: just setup the routing
and not the affinity.

What do you think?


> +    gic_set_irq_properties(desc, priority);
>  }
>  
>  /* Program the GIC to route an interrupt to a guest
> @@ -155,7 +153,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>      desc->handler = gic_hw_ops->gic_guest_irq_type;
>      set_bit(_IRQ_GUEST, &desc->status);
>  
> -    gic_set_irq_properties(desc, cpumask_of(v_target->processor), priority);
> +    gic_set_irq_properties(desc, priority);
>  
>      p->desc = desc;
>      res = 0;

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

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

* Re: [RFC 3/8] xen/arm: gic: split set_irq_properties
  2016-06-07 16:48 ` [RFC 3/8] xen/arm: gic: split set_irq_properties Julien Grall
@ 2016-06-22 10:58   ` Stefano Stabellini
  0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 10:58 UTC (permalink / raw)
  To: Julien Grall
  Cc: sstabellini, wei.chen, steve.capper, xen-devel, shannon.zhao, shankerd

On Tue, 7 Jun 2016, Julien Grall wrote:
> The callback set_irq_properties will configure the GIC for a specific
> IRQ with the type and the priority.
> 
> In a follow-up patch, Xen will configure the type and the priority at
> different stage of the routing. So split it in 2 separate callbacks.
> 
> At the same time, move the ASSERT to check the validity of the type and
> if the desc->lock is locked in the common code (gic.c). This is because
> the constraint are the same between GICv2 and GICv3, however the driver
> of the latter did not contain any sanity check.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  xen/arch/arm/gic-v2.c     | 19 +++++++++++++------
>  xen/arch/arm/gic-v3.c     | 15 ++++++++++++---
>  xen/arch/arm/gic.c        | 23 ++++++++++++++---------
>  xen/include/asm-arm/gic.h |  7 ++++---
>  4 files changed, 43 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 90b07b3..fa2c5a5 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -200,16 +200,12 @@ static unsigned int gicv2_read_irq(void)
>      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
>  }
>  
> -static void gicv2_set_irq_properties(struct irq_desc *desc,
> -                                     unsigned int priority)
> +static void gicv2_set_irq_type(struct irq_desc *desc)
>  {
>      uint32_t cfg, actual, edgebit;
>      unsigned int irq = desc->irq;
>      unsigned int type = desc->arch.type;
>  
> -    ASSERT(type != IRQ_TYPE_INVALID);
> -    ASSERT(spin_is_locked(&desc->lock));
> -
>      spin_lock(&gicv2.lock);
>      /* Set edge / level */
>      cfg = readl_gicd(GICD_ICFGR + (irq / 16) * 4);
> @@ -234,6 +230,16 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
>              IRQ_TYPE_LEVEL_HIGH;
>      }
>  
> +    spin_unlock(&gicv2.lock);
> +}
> +
> +static void gicv2_set_irq_priority(struct irq_desc *desc,
> +                                   unsigned int priority)
> +{
> +    unsigned int irq = desc->irq;
> +
> +    spin_lock(&gicv2.lock);
> +
>      /* Set priority */
>      writeb_gicd(priority, GICD_IPRIORITYR + irq);
>  
> @@ -916,7 +922,8 @@ const static struct gic_hw_operations gicv2_ops = {
>      .eoi_irq             = gicv2_eoi_irq,
>      .deactivate_irq      = gicv2_dir_irq,
>      .read_irq            = gicv2_read_irq,
> -    .set_irq_properties  = gicv2_set_irq_properties,
> +    .set_irq_type        = gicv2_set_irq_type,
> +    .set_irq_priority    = gicv2_set_irq_priority,
>      .send_SGI            = gicv2_send_SGI,
>      .disable_interface   = gicv2_disable_interface,
>      .update_lr           = gicv2_update_lr,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c936c8a..c25fe50 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -471,8 +471,7 @@ static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
>               MPIDR_AFFINITY_LEVEL(mpidr, 0));
>  }
>  
> -static void gicv3_set_irq_properties(struct irq_desc *desc,
> -                                     unsigned int priority)
> +static void gicv3_set_irq_type(struct irq_desc *desc)
>  {
>      uint32_t cfg, actual, edgebit;
>      void __iomem *base;
> @@ -512,6 +511,15 @@ static void gicv3_set_irq_properties(struct irq_desc *desc,
>              IRQ_TYPE_EDGE_RISING :
>              IRQ_TYPE_LEVEL_HIGH;
>      }
> +    spin_unlock(&gicv3.lock);
> +}
> +
> +static void gicv3_set_irq_priority(struct irq_desc *desc,
> +                                   unsigned int priority)
> +{
> +    unsigned int irq = desc->irq;
> +
> +    spin_lock(&gicv3.lock);
>  
>      /* Set priority */
>      if ( irq < NR_GIC_LOCAL_IRQS )
> @@ -1547,7 +1555,8 @@ static const struct gic_hw_operations gicv3_ops = {
>      .eoi_irq             = gicv3_eoi_irq,
>      .deactivate_irq      = gicv3_dir_irq,
>      .read_irq            = gicv3_read_irq,
> -    .set_irq_properties  = gicv3_set_irq_properties,
> +    .set_irq_type        = gicv3_set_irq_type,
> +    .set_irq_priority    = gicv3_set_irq_priority,
>      .send_SGI            = gicv3_send_sgi,
>      .disable_interface   = gicv3_disable_interface,
>      .update_lr           = gicv3_update_lr,
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index f25381f..0fd7e8c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -96,14 +96,17 @@ void gic_restore_state(struct vcpu *v)
>      gic_restore_pending_irqs(v);
>  }
>  
> -/*
> - * - desc.lock must be held
> - * - arch.type must be valid (i.e != IRQ_TYPE_INVALID)
> - */
> -static void gic_set_irq_properties(struct irq_desc *desc,
> -                                   unsigned int priority)
> +static void gic_set_irq_type(struct irq_desc *desc)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(desc->arch.type != IRQ_TYPE_INVALID);
> +
> +    gic_hw_ops->set_irq_type(desc);
> +}
> +
> +static void gic_set_irq_priority(struct irq_desc *desc, unsigned int priority)
>  {
> -    gic_hw_ops->set_irq_properties(desc, priority);
> +    gic_hw_ops->set_irq_priority(desc, priority);
>  }
>  
>  /* Program the GIC to route an interrupt to the host (i.e. Xen)
> @@ -121,7 +124,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>  
>      desc->handler->set_affinity(desc, cpu_mask);
>  
> -    gic_set_irq_properties(desc, priority);
> +    gic_set_irq_type(desc);
> +    gic_set_irq_priority(desc, priority);
>  }
>  
>  /* Program the GIC to route an interrupt to a guest
> @@ -153,7 +157,8 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>      desc->handler = gic_hw_ops->gic_guest_irq_type;
>      set_bit(_IRQ_GUEST, &desc->status);
>  
> -    gic_set_irq_properties(desc, priority);
> +    gic_set_irq_type(desc);
> +    gic_set_irq_priority(desc, priority);
>  
>      p->desc = desc;
>      res = 0;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index b931f98..ffba469 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -329,9 +329,10 @@ struct gic_hw_operations {
>      void (*deactivate_irq)(struct irq_desc *irqd);
>      /* Read IRQ id and Ack */
>      unsigned int (*read_irq)(void);
> -    /* Set IRQ property */
> -    void (*set_irq_properties)(struct irq_desc *desc,
> -                               unsigned int priority);
> +    /* Set IRQ type - type is taken from desc->arch.type */
> +    void (*set_irq_type)(struct irq_desc *desc);
> +    /* Set IRQ priority */
> +    void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
>      /* Send SGI */
>      void (*send_SGI)(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
>                       const cpumask_t *online_mask);
> -- 
> 1.9.1
> 

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

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

* Re: [RFC 5/8] xen/arm: gic: Document how gic_set_irq_type should be called
  2016-06-07 16:48 ` [RFC 5/8] xen/arm: gic: Document how gic_set_irq_type should be called Julien Grall
@ 2016-06-22 11:00   ` Stefano Stabellini
  0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 11:00 UTC (permalink / raw)
  To: Julien Grall
  Cc: sstabellini, wei.chen, steve.capper, xen-devel, shannon.zhao, shankerd

On Tue, 7 Jun 2016, Julien Grall wrote:
> Changing the value of Int_config is UNPREDICTABLE when the corresponding
> interrupt is not disabled.
> 
> The driver is assuming the interrupt will be disabled by the caller of
> gic_set_irq_type. Add an ASSERT to ensure it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  xen/arch/arm/gic.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 8b992d8..27cd177 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -96,8 +96,14 @@ void gic_restore_state(struct vcpu *v)
>      gic_restore_pending_irqs(v);
>  }
>  
> +/* desc->irq needs to be disabled before calling this function */
>  static void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
>  {
> +    /*
> +     * IRQ must be disabled before configuring it (see 4.3.13 in ARM IHI
> +     * 0048B.b). We rely on the caller to do it.
> +     */
> +    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>      ASSERT(spin_is_locked(&desc->lock));
>      ASSERT(type != IRQ_TYPE_INVALID);
>  
> -- 
> 1.9.1
> 

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

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

* Re: [RFC 6/8] Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on ACPI"
  2016-06-07 16:48 ` [RFC 6/8] Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on ACPI" Julien Grall
@ 2016-06-22 11:01   ` Stefano Stabellini
  0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 11:01 UTC (permalink / raw)
  To: Julien Grall
  Cc: sstabellini, wei.chen, steve.capper, xen-devel, shannon.zhao, shankerd

On Tue, 7 Jun 2016, Julien Grall wrote:
> This reverts commit f91c84edebe67296e4051af055dbf0adafb13a37. SPI
> routing for ACPI support will be added in a follow-up patch.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  xen/arch/arm/vgic.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 413ff16..ee35683 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -25,8 +25,6 @@
>  #include <xen/irq.h>
>  #include <xen/sched.h>
>  #include <xen/perfc.h>
> -#include <xen/iocap.h>
> -#include <xen/acpi.h>
>  
>  #include <asm/current.h>
>  
> @@ -344,22 +342,9 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>      unsigned long flags;
>      int i = 0;
>      struct vcpu *v_target;
> -    struct domain *d = v->domain;
>  
>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
> -        /* Set the irq type and route it to guest only for SPI and Dom0 */
> -        if( irq_access_permitted(d, irq) && is_hardware_domain(d) &&
> -            ( irq >= 32 ) && ( !acpi_disabled ) )
> -        {
> -            static int log_once = 0;
> -            if ( !log_once )
> -            {
> -                gprintk(XENLOG_WARNING, "Routing SPIs to Dom0 on ACPI systems is unimplemented.\n");
> -                log_once++;
> -            }
> -        }
> -
>          v_target = __vgic_get_target_vcpu(v, irq);
>          p = irq_to_pending(v_target, irq);
>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> -- 
> 1.9.1
> 

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

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

* Re: [RFC 2/8] xen/arm: gic: Do not configure affinity for guest IRQ during routing
  2016-06-22 10:54   ` Stefano Stabellini
@ 2016-06-22 11:19     ` Julien Grall
  0 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2016-06-22 11:19 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: steve.capper, shankerd, shannon.zhao, wei.chen, xen-devel

Hi Stefano,

On 22/06/16 11:54, Stefano Stabellini wrote:
> On Tue, 7 Jun 2016, Julien Grall wrote:
>> The affinity of a guest IRQ is set every time the guest enable it (see
>> vgic_enable_irqs).
>>
>> It is not necessary to set the affinity when the IRQ is routed to the
>> guest because Xen will never receive the IRQ until it hass been enabled
>> by the guest.
>>
>> Signed-off-by: Julien grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/gic.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 8a1087b..f25381f 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -97,17 +97,13 @@ void gic_restore_state(struct vcpu *v)
>>   }
>>
>>   /*
>> - * needs to be called with a valid cpu_mask, ie each cpu in the mask has
>> - * already called gic_cpu_init
>>    * - desc.lock must be held
>>    * - arch.type must be valid (i.e != IRQ_TYPE_INVALID)
>>    */
>>   static void gic_set_irq_properties(struct irq_desc *desc,
>> -                                   const cpumask_t *cpu_mask,
>>                                      unsigned int priority)
>>   {
>>       gic_hw_ops->set_irq_properties(desc, priority);
>> -    desc->handler->set_affinity(desc, cpu_mask);
>>   }
>>
>>   /* Program the GIC to route an interrupt to the host (i.e. Xen)
>> @@ -123,7 +119,9 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>>
>>       desc->handler = gic_hw_ops->gic_host_irq_type;
>>
>> -    gic_set_irq_properties(desc, cpu_mask, priority);
>> +    desc->handler->set_affinity(desc, cpu_mask);
>
> You could call irq_set_affinity here, it might make for nicer code.
>
> Actually thinking more about this, I think it would be better to add the
> irq_set_affinity call to xen/arch/arm/irq.c:setup_irq, right after the
> call to gic_route_irq_to_xen.  That way both gic_route_irq_to_xen and
> gic_route_irq_to_guest would behave the same way: just setup the routing
> and not the affinity.
>
> What do you think?

I am fine to call irq_set_affinity from setup_irq. It makes more sense 
than calling the former from gic_route_irq_to_xen.

I will make the change in the next version.

Cheers,

-- 
Julien Grall

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

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

* Re: [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse
  2016-06-07 16:48 ` [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse Julien Grall
@ 2016-06-22 11:23   ` Stefano Stabellini
  2016-06-22 11:46     ` Julien Grall
  0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 11:23 UTC (permalink / raw)
  To: Julien Grall
  Cc: sstabellini, wei.chen, steve.capper, xen-devel, shannon.zhao, shankerd

On Tue, 7 Jun 2016, Julien Grall wrote:
> The function route_irq_to_guest mandates the IRQ type, stored in
> desc->arch.type, to be valid. However, in case of ACPI, these
> information is not part of the static tables. Therefore Xen needs to
> rely on DOM0 to provide a valid type based on the firmware tables.
> 
> A new helper, irq_type_set_by_domain is provided to check whether a
> domain is allowed to set the IRQ type. For now, only DOM0 is allowed to
> configure it when ACPI is inuse.
> 
> When the helper returns 1, the routing function will not check whether
> the IRQ type is correctly set and configure the GIC. Instead, this will
> be done when the domain will enable the interrupt.
> 
> Note that irq_set_spi_type is not called because it validates the type
> and does not allow it the domain to change the type after the first
> write. It means that desc->arch.type will never be set, which is fine
> because the field is only used to configure the type during the routing.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
> I am thinking to at least extend the behavior to DOM0 using DT. This
> would make us resilient to a DT not providing the correct type without
> having to workaround them in Xen.

I agree with you. It is also better to have only one way of doing things
rather than two.


> Furthermore, it might be possible to let any domain configuring the IRQ
> type (could be useful when passthrough an IRQ with ACPI). However, we would
> need to consider any potential security impact beforehand.
> ---
>  xen/arch/arm/gic.c        |  5 +++--
>  xen/arch/arm/irq.c        | 14 +++++++++++++-
>  xen/arch/arm/vgic.c       | 21 +++++++++++++++++++++
>  xen/include/asm-arm/gic.h |  3 +++
>  xen/include/asm-arm/irq.h |  6 ++++++
>  5 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 27cd177..80d93a8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -97,7 +97,7 @@ void gic_restore_state(struct vcpu *v)
>  }
>  
>  /* desc->irq needs to be disabled before calling this function */
> -static void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
> +void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
>  {
>      /*
>       * IRQ must be disabled before configuring it (see 4.3.13 in ARM IHI
> @@ -162,7 +162,8 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>      desc->handler = gic_hw_ops->gic_guest_irq_type;
>      set_bit(_IRQ_GUEST, &desc->status);
>  
> -    gic_set_irq_type(desc, desc->arch.type);
> +    if ( !irq_type_set_by_domain(d) )
> +        gic_set_irq_type(desc, desc->arch.type);
>      gic_set_irq_priority(desc, priority);
>  
>      p->desc = desc;
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 2f8af72..b9d7749 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -27,6 +27,7 @@
>  
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
> +#include <asm/acpi.h>
>  
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>  static DEFINE_SPINLOCK(local_irqs_type_lock);
> @@ -395,6 +396,17 @@ bool_t is_assignable_irq(unsigned int irq)
>  }
>  
>  /*
> + * When ACPI is inuse, only the hardware domain knows the interrupt
> + * type.
> + *
> + * XXX: See whether it is possible to let any domain configure the type.
> + */
> +bool_t irq_type_set_by_domain(const struct domain *d)
> +{
> +    return ((d == hardware_domain) && !acpi_disabled);
> +}
> +
> +/*
>   * Route an IRQ to a specific guest.
>   * For now only SPIs are assignable to the guest.
>   */
> @@ -449,7 +461,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
>  
>      spin_lock_irqsave(&desc->lock, flags);
>  
> -    if ( desc->arch.type == IRQ_TYPE_INVALID )
> +    if ( desc->arch.type == IRQ_TYPE_INVALID && !irq_type_set_by_domain(d) )
>      {
>          printk(XENLOG_G_ERR "IRQ %u has not been configured\n", irq);
>          retval = -EIO;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index ee35683..3e1c572 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -249,6 +249,24 @@ static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
>      return priority;
>  }
>  
> +#define NR_CFG_PER_ICFGR 16
> +#define NR_BITS_PER_CFG (32U / NR_CFG_PER_ICFGR)
> +
> +/* The function should be called witht the rank lock taken. */
                                      ^ with

> +static int __vgic_get_virq_type(struct vcpu *v, unsigned int virq)
> +{
> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
> +    uint8_t index = virq & INTERRUPT_RANK_MASK;
> +    uint32_t reg = rank->icfg[index / NR_CFG_PER_ICFGR];
> +    uint8_t val;
> +
> +    ASSERT(spin_is_locked(&rank->lock));
> +
> +    val = reg >> ((index % NR_CFG_PER_ICFGR) * NR_BITS_PER_CFG);
> +
> +    return (val & 0x2) ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_HIGH;
> +}

I would stick to the function and definition we already had from "from
Configure SPI interrupt type and route to Dom0 dynamically" by Shannon:

  #define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
  
  static inline unsigned int get_the_irq_type(struct vcpu *v, int n, int index)
  {
      struct vgic_irq_rank *vr = vgic_get_rank(v, n);
      uint32_t tr = vr->icfg[index >> 4];
  
      if ( tr & VGIC_ICFG_MASK(index) )
          return IRQ_TYPE_EDGE_BOTH;
      else
          return IRQ_TYPE_LEVEL_MASK;
  }



>  void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>  {
>      unsigned long flags;
> @@ -342,6 +360,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>      unsigned long flags;
>      int i = 0;
>      struct vcpu *v_target;
> +    struct domain *d = v->domain;
>  
>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
> @@ -356,6 +375,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>          {
>              irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>              spin_lock_irqsave(&p->desc->lock, flags);
> +            if ( irq_type_set_by_domain(d) )
> +                gic_set_irq_type(p->desc, __vgic_get_virq_type(v, irq));
>              p->desc->handler->enable(p->desc);
>              spin_unlock_irqrestore(&p->desc->lock, flags);
>          }

This is OK. But I am wondering if it wouldn't make sense to call
gic_set_irq_type when the guest actually writes to the virtual icfg?


> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ddc45a8..44b9ef6 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -222,6 +222,9 @@ enum gic_version {
>  
>  extern enum gic_version gic_hw_version(void);
>  
> +/* Program the IRQ type into the GIC */
> +void gic_set_irq_type(struct irq_desc *desc, unsigned int type);
> +
>  /* Program the GIC to route an interrupt */
>  extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>                                   unsigned int priority);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 493773c..8f7a167 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -58,6 +58,12 @@ int platform_get_irq(const struct dt_device_node *device, int index);
>  
>  void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
>  
> +/*
> + * Use this helper in places that need to know whether the IRQ type is
> + * set by the domain.
> + */
> +bool_t irq_type_set_by_domain(const struct domain *d);
> +
>  #endif /* _ASM_HW_IRQ_H */
>  /*
>   * Local variables:
> -- 
> 1.9.1
> 

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

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

* Re: [RFC 4/8] xen/arm: gic: set_type: Pass the type in parameter rather than in desc->arch.type
  2016-06-07 16:48 ` [RFC 4/8] xen/arm: gic: set_type: Pass the type in parameter rather than in desc->arch.type Julien Grall
@ 2016-06-22 11:25   ` Stefano Stabellini
  0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 11:25 UTC (permalink / raw)
  To: Julien Grall
  Cc: sstabellini, wei.chen, steve.capper, xen-devel, shannon.zhao, shankerd

On Tue, 7 Jun 2016, Julien Grall wrote:
> A follow-up patch will not store the type in desc->arch.type. Also, the
> callback prototype is more logical.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


>  xen/arch/arm/gic-v2.c     | 3 +--
>  xen/arch/arm/gic-v3.c     | 3 +--
>  xen/arch/arm/gic.c        | 9 ++++-----
>  xen/include/asm-arm/gic.h | 4 ++--
>  4 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index fa2c5a5..399b869 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -200,11 +200,10 @@ static unsigned int gicv2_read_irq(void)
>      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
>  }
>  
> -static void gicv2_set_irq_type(struct irq_desc *desc)
> +static void gicv2_set_irq_type(struct irq_desc *desc, unsigned int type)
>  {
>      uint32_t cfg, actual, edgebit;
>      unsigned int irq = desc->irq;
> -    unsigned int type = desc->arch.type;
>  
>      spin_lock(&gicv2.lock);
>      /* Set edge / level */
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c25fe50..d0a7a23 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -471,12 +471,11 @@ static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
>               MPIDR_AFFINITY_LEVEL(mpidr, 0));
>  }
>  
> -static void gicv3_set_irq_type(struct irq_desc *desc)
> +static void gicv3_set_irq_type(struct irq_desc *desc, unsigned int type)
>  {
>      uint32_t cfg, actual, edgebit;
>      void __iomem *base;
>      unsigned int irq = desc->irq;
> -    unsigned int type = desc->arch.type;
>  
>      /* SGI's are always edge-triggered not need to call GICD_ICFGR0 */
>      ASSERT(irq >= NR_GIC_SGI);
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 0fd7e8c..8b992d8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -96,12 +96,12 @@ void gic_restore_state(struct vcpu *v)
>      gic_restore_pending_irqs(v);
>  }
>  
> -static void gic_set_irq_type(struct irq_desc *desc)
> +static void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
>  {
>      ASSERT(spin_is_locked(&desc->lock));
> -    ASSERT(desc->arch.type != IRQ_TYPE_INVALID);
> +    ASSERT(type != IRQ_TYPE_INVALID);
>  
> -    gic_hw_ops->set_irq_type(desc);
> +    gic_hw_ops->set_irq_type(desc, type);
>  }
>  
>  static void gic_set_irq_priority(struct irq_desc *desc, unsigned int priority)
> @@ -124,7 +124,6 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>  
>      desc->handler->set_affinity(desc, cpu_mask);
>  
> -    gic_set_irq_type(desc);
>      gic_set_irq_priority(desc, priority);
>  }
>  
> @@ -157,7 +156,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>      desc->handler = gic_hw_ops->gic_guest_irq_type;
>      set_bit(_IRQ_GUEST, &desc->status);
>  
> -    gic_set_irq_type(desc);
> +    gic_set_irq_type(desc, desc->arch.type);
>      gic_set_irq_priority(desc, priority);
>  
>      p->desc = desc;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ffba469..ddc45a8 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -329,8 +329,8 @@ struct gic_hw_operations {
>      void (*deactivate_irq)(struct irq_desc *irqd);
>      /* Read IRQ id and Ack */
>      unsigned int (*read_irq)(void);
> -    /* Set IRQ type - type is taken from desc->arch.type */
> -    void (*set_irq_type)(struct irq_desc *desc);
> +    /* Set IRQ type */
> +    void (*set_irq_type)(struct irq_desc *desc, unsigned int type);
>      /* Set IRQ priority */
>      void (*set_irq_priority)(struct irq_desc *desc, unsigned int priority);
>      /* Send SGI */
> -- 
> 1.9.1
> 

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

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

* Re: [RFC 8/8] xen/arm: acpi: route all unused IRQs to DOM0
  2016-06-07 16:48 ` [RFC 8/8] xen/arm: acpi: route all unused IRQs to DOM0 Julien Grall
@ 2016-06-22 11:44   ` Stefano Stabellini
  2016-06-22 12:19     ` Julien Grall
  0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 11:44 UTC (permalink / raw)
  To: Julien Grall
  Cc: sstabellini, wei.chen, steve.capper, xen-devel, shannon.zhao, shankerd

On Tue, 7 Jun 2016, Julien Grall wrote:
> It is not possible to know which IRQs will be used by DOM0 when ACPI is
> inuse. The approach implemented by this patch, will route all unused
> IRQs to DOM0 before it has booted.
> 
> The number of IRQs routed is based on the maximum SPIs supported by the
> hardware (up to ~1000). However, some of them might not be wired. So we
> would allocate resource for nothing.
> 
> For each IRQ routed, Xen is allocating memory for irqaction (40 bytes)
> and irq_guest (16 bytes). So in the worst case scenario ~54KB of memory
> will be allocated. Given that ACPI will mostly be used by server, I
> think it is a small drawback.

Yeah, it's a pity. The patch is certainly an improvement and it would
fix ACPI, which is currently broken, so I think it should go in pretty
much as is. (See only one small comment below.)

But I wonder if we could do something better by the next release. Have
you considered using something like a tasklet to call
route_irq_to_guest? The tasklet would be scheduled only after
vgic_enable_irqs is called.

Something like:

- guest writes to GICD_ISENABLER
- run vgic_enable_irqs
  - enable tasklet
- tasklet run
  - call route_irq_to_guest

?


> map_irq_to_domain is slightly reworked to remove the dependency on
> device-tree. So the function can be also be used for ACPI and will
> avoid code duplication.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/domain_build.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 00dc07a..76d503d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -953,11 +953,10 @@ static int make_timer_node(const struct domain *d, void *fdt,
>      return res;
>  }
>  
> -static int map_irq_to_domain(const struct dt_device_node *dev,
> -                             struct domain *d, unsigned int irq)
> +static int map_irq_to_domain(struct domain *d, unsigned int irq,
> +                             bool_t need_mapping, const char *devname)
>  
>  {
> -    bool_t need_mapping = !dt_device_for_passthrough(dev);
>      int res;
>  
>      res = irq_permit_access(d, irq);
> @@ -977,7 +976,7 @@ static int map_irq_to_domain(const struct dt_device_node *dev,
>           */
>          vgic_reserve_virq(d, irq);
>  
> -        res = route_irq_to_guest(d, irq, irq, dt_node_name(dev));
> +        res = route_irq_to_guest(d, irq, irq, devname);
>          if ( res < 0 )
>          {
>              printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> @@ -997,6 +996,7 @@ static int map_dt_irq_to_domain(const struct dt_device_node *dev,
>      struct domain *d = data;
>      unsigned int irq = dt_irq->irq;
>      int res;
> +    bool_t need_mapping = !dt_device_for_passthrough(dev);
>  
>      if ( irq < NR_LOCAL_IRQS )
>      {
> @@ -1015,7 +1015,7 @@ static int map_dt_irq_to_domain(const struct dt_device_node *dev,
>          return res;
>      }
>  
> -    res = map_irq_to_domain(dev, d, irq);
> +    res = map_irq_to_domain(d, irq, need_mapping, dt_node_name(dev));
>  
>      return 0;
>  }
> @@ -1152,7 +1152,7 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
>              return res;
>          }
>  
> -        res = map_irq_to_domain(dev, d, res);
> +        res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
>          if ( res )
>              return res;
>      }
> @@ -1411,13 +1411,10 @@ static int acpi_permit_spi_access(struct domain *d)

I would rename acpi_permit_spi_access to acpi_route_spis_to_dom0 or
something like that, to reflect the change in behavior of the function.



>          if ( desc->action != NULL)
>              continue;
>  
> -        res = irq_permit_access(d, i);
> +        /* XXX: Shall we use a proper devname? */
> +        res = map_irq_to_domain(d, i, true, "ACPI");
>          if ( res )
> -        {
> -            printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> -                   d->domain_id, i);
>              return res;
> -        }
>      }


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

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

* Re: [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse
  2016-06-22 11:23   ` Stefano Stabellini
@ 2016-06-22 11:46     ` Julien Grall
  2016-06-22 11:49       ` Stefano Stabellini
  0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2016-06-22 11:46 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: steve.capper, shankerd, shannon.zhao, wei.chen, xen-devel

Hi Stefano,

On 22/06/16 12:23, Stefano Stabellini wrote:
> On Tue, 7 Jun 2016, Julien Grall wrote:
>> The function route_irq_to_guest mandates the IRQ type, stored in
>> desc->arch.type, to be valid. However, in case of ACPI, these
>> information is not part of the static tables. Therefore Xen needs to
>> rely on DOM0 to provide a valid type based on the firmware tables.
>>
>> A new helper, irq_type_set_by_domain is provided to check whether a
>> domain is allowed to set the IRQ type. For now, only DOM0 is allowed to
>> configure it when ACPI is inuse.
>>
>> When the helper returns 1, the routing function will not check whether
>> the IRQ type is correctly set and configure the GIC. Instead, this will
>> be done when the domain will enable the interrupt.
>>
>> Note that irq_set_spi_type is not called because it validates the type
>> and does not allow it the domain to change the type after the first
>> write. It means that desc->arch.type will never be set, which is fine
>> because the field is only used to configure the type during the routing.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>> I am thinking to at least extend the behavior to DOM0 using DT. This
>> would make us resilient to a DT not providing the correct type without
>> having to workaround them in Xen.
>
> I agree with you. It is also better to have only one way of doing things
> rather than two.

I will update this patch to allow the hardware domain setting the IRQ type.

[...]

>> +#define NR_CFG_PER_ICFGR 16
>> +#define NR_BITS_PER_CFG (32U / NR_CFG_PER_ICFGR)
>> +
>> +/* The function should be called witht the rank lock taken. */
>                                        ^ with
>
>> +static int __vgic_get_virq_type(struct vcpu *v, unsigned int virq)
>> +{
>> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
>> +    uint8_t index = virq & INTERRUPT_RANK_MASK;
>> +    uint32_t reg = rank->icfg[index / NR_CFG_PER_ICFGR];
>> +    uint8_t val;
>> +
>> +    ASSERT(spin_is_locked(&rank->lock));
>> +
>> +    val = reg >> ((index % NR_CFG_PER_ICFGR) * NR_BITS_PER_CFG);
>> +
>> +    return (val & 0x2) ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_HIGH;
>> +}
>
> I would stick to the function and definition we already had from "from
> Configure SPI interrupt type and route to Dom0 dynamically" by Shannon:

I found Shannon's function more difficult to understand. However the 
code is simpler, so I will use his function for the next version.

>    #define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
>
>    static inline unsigned int get_the_irq_type(struct vcpu *v, int n, int index)
>    {
>        struct vgic_irq_rank *vr = vgic_get_rank(v, n);
>        uint32_t tr = vr->icfg[index >> 4];
>
>        if ( tr & VGIC_ICFG_MASK(index) )
>            return IRQ_TYPE_EDGE_BOTH;
>        else
>            return IRQ_TYPE_LEVEL_MASK;
>    }
>
>
>
>>   void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>>   {
>>       unsigned long flags;
>> @@ -342,6 +360,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>       unsigned long flags;
>>       int i = 0;
>>       struct vcpu *v_target;
>> +    struct domain *d = v->domain;
>>
>>       while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>           irq = i + (32 * n);
>> @@ -356,6 +375,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>>           {
>>               irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>>               spin_lock_irqsave(&p->desc->lock, flags);
>> +            if ( irq_type_set_by_domain(d) )
>> +                gic_set_irq_type(p->desc, __vgic_get_virq_type(v, irq));
>>               p->desc->handler->enable(p->desc);
>>               spin_unlock_irqrestore(&p->desc->lock, flags);
>>           }
>
> This is OK. But I am wondering if it wouldn't make sense to call
> gic_set_irq_type when the guest actually writes to the virtual icfg?

Hmmm it looks like that I forgot to explain in the commit message why I 
chose to call gic_set_irq_type here.

Changing the value of Int_config is UNPREDICTABLE when the corresponding
interrupt is not disabled (see 4.3.13 in ARM IHI 0048B.b).

So calling gic_set_irq_type when the guest actualy writes to the virtual 
cfg would require to disable the IRQ before hand.

Given that the behavior is UNPREDICTABLE, I chose to set the type before 
enabling the IRQ because the resulting code is simpler.

Regards,

-- 
Julien Grall

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

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

* Re: [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse
  2016-06-22 11:46     ` Julien Grall
@ 2016-06-22 11:49       ` Stefano Stabellini
  0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2016-06-22 11:49 UTC (permalink / raw)
  To: Julien Grall
  Cc: Stefano Stabellini, wei.chen, steve.capper, xen-devel,
	shannon.zhao, shankerd

On Wed, 22 Jun 2016, Julien Grall wrote:
> > > @@ -356,6 +375,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
> > > n)
> > >           {
> > >               irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> > >               spin_lock_irqsave(&p->desc->lock, flags);
> > > +            if ( irq_type_set_by_domain(d) )
> > > +                gic_set_irq_type(p->desc, __vgic_get_virq_type(v, irq));
> > >               p->desc->handler->enable(p->desc);
> > >               spin_unlock_irqrestore(&p->desc->lock, flags);
> > >           }
> > 
> > This is OK. But I am wondering if it wouldn't make sense to call
> > gic_set_irq_type when the guest actually writes to the virtual icfg?
> 
> Hmmm it looks like that I forgot to explain in the commit message why I chose
> to call gic_set_irq_type here.
> 
> Changing the value of Int_config is UNPREDICTABLE when the corresponding
> interrupt is not disabled (see 4.3.13 in ARM IHI 0048B.b).
> 
> So calling gic_set_irq_type when the guest actualy writes to the virtual cfg
> would require to disable the IRQ before hand.
> 
> Given that the behavior is UNPREDICTABLE, I chose to set the type before
> enabling the IRQ because the resulting code is simpler.

Ah, I see. Makes sense.

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

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

* Re: [RFC 8/8] xen/arm: acpi: route all unused IRQs to DOM0
  2016-06-22 11:44   ` Stefano Stabellini
@ 2016-06-22 12:19     ` Julien Grall
  0 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2016-06-22 12:19 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: steve.capper, shankerd, shannon.zhao, wei.chen, xen-devel

Hi Stefano,

On 22/06/16 12:44, Stefano Stabellini wrote:
> On Tue, 7 Jun 2016, Julien Grall wrote:
>> It is not possible to know which IRQs will be used by DOM0 when ACPI is
>> inuse. The approach implemented by this patch, will route all unused
>> IRQs to DOM0 before it has booted.
>>
>> The number of IRQs routed is based on the maximum SPIs supported by the
>> hardware (up to ~1000). However, some of them might not be wired. So we
>> would allocate resource for nothing.
>>
>> For each IRQ routed, Xen is allocating memory for irqaction (40 bytes)
>> and irq_guest (16 bytes). So in the worst case scenario ~54KB of memory
>> will be allocated. Given that ACPI will mostly be used by server, I
>> think it is a small drawback.
>
> Yeah, it's a pity. The patch is certainly an improvement and it would
> fix ACPI, which is currently broken, so I think it should go in pretty
> much as is. (See only one small comment below.)
>
> But I wonder if we could do something better by the next release. Have
> you considered using something like a tasklet to call
> route_irq_to_guest? The tasklet would be scheduled only after
> vgic_enable_irqs is called.
>
> Something like:
>
> - guest writes to GICD_ISENABLER
> - run vgic_enable_irqs
>    - enable tasklet
> - tasklet run
>    - call route_irq_to_guest
>
> ?

I remembered we talked about a such solution on IRC.

I have considered it and I have found multiple downsides, what do we do 
if the call to route_irq_to_guest fails? It is easy to make this call 
fail on ARM64 because there is no difference between xenheap and 
domheap. And there is no way to report this failure to the guest except 
crashing it.

Furthermore, we would need to take into account that the IRQ might be 
disabled by another CPU before the tasklet is called.

Finally, we would also need to plumb GICD_CTLR.RWP on GICv3 because the 
effects of the write will be differed. Which lead to the problem, what 
if the tasklet takes more than a 1s to be scheduled/executed?

I agree that the memory usage is "greater" (54KB on server with tens of 
gigabyte of memory), but I think the complexity of the code with tasklet 
outweigh the memory usage of this patch.

>
>
>> map_irq_to_domain is slightly reworked to remove the dependency on
>> device-tree. So the function can be also be used for ACPI and will
>> avoid code duplication.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/domain_build.c | 19 ++++++++-----------
>>   1 file changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 00dc07a..76d503d 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c

[...]

>> @@ -1411,13 +1411,10 @@ static int acpi_permit_spi_access(struct domain *d)
>
> I would rename acpi_permit_spi_access to acpi_route_spis_to_dom0 or
> something like that, to reflect the change in behavior of the function.

Good point. I will do it in the next version.

Regards,

-- 
Julien Grall

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

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

end of thread, other threads:[~2016-06-22 12:19 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
2016-06-07 16:48 ` [RFC 1/8] xen/arm: gic: Consolidate the IRQ affinity set in a single place Julien Grall
2016-06-22 10:46   ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 2/8] xen/arm: gic: Do not configure affinity for guest IRQ during routing Julien Grall
2016-06-22 10:54   ` Stefano Stabellini
2016-06-22 11:19     ` Julien Grall
2016-06-07 16:48 ` [RFC 3/8] xen/arm: gic: split set_irq_properties Julien Grall
2016-06-22 10:58   ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 4/8] xen/arm: gic: set_type: Pass the type in parameter rather than in desc->arch.type Julien Grall
2016-06-22 11:25   ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 5/8] xen/arm: gic: Document how gic_set_irq_type should be called Julien Grall
2016-06-22 11:00   ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 6/8] Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on ACPI" Julien Grall
2016-06-22 11:01   ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse Julien Grall
2016-06-22 11:23   ` Stefano Stabellini
2016-06-22 11:46     ` Julien Grall
2016-06-22 11:49       ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 8/8] xen/arm: acpi: route all unused IRQs to DOM0 Julien Grall
2016-06-22 11:44   ` Stefano Stabellini
2016-06-22 12:19     ` Julien Grall
2016-06-07 18:50 ` [RFC 0/8] xen/arm: acpi: Support SPIs routing Shanker Donthineni
2016-06-08 11:48   ` Shanker Donthineni
2016-06-08 11:49     ` Julien Grall
2016-06-08 12:11       ` Shanker Donthineni
2016-06-08 12:34         ` Julien Grall
2016-06-13 11:42           ` Julien Grall
2016-06-13 17:19             ` Shanker Donthineni
2016-06-13 17:20               ` Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).