[2/6] irqchip: GIC: Convert to EOImode == 1
diff mbox

Message ID 1436447951-22357-3-git-send-email-marc.zyngier@arm.com
State New
Headers show

Commit Message

Marc Zyngier July 9, 2015, 1:19 p.m. UTC
So far, GICv2 has been used in with EOImode == 0. The effect of this
mode is to perform the priority drop and the deactivation of the
interrupt at the same time.

While this works perfectly for Linux (we only have a single priority),
it causes issues when an interrupt is forwarded to a guest, and when
we want the guest to perform the EOI itself.

For this case, the GIC architecture provides EOImode == 1, where:
- A write to the EOI register drops the priority of the interrupt and leaves
it active. Other interrupts at the same priority level can now be taken,
but the active interrupt cannot be taken again
- A write to the DIR marks the interrupt as inactive, meaning it can
now be taken again.

We only enable this feature when booted in HYP mode and that
the device-tree reporte a suitable CPU interface. Observable behaviour
should remain unchanged.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c       | 32 +++++++++++++++++++++++++++++---
 include/linux/irqchip/arm-gic.h |  4 ++++
 2 files changed, 33 insertions(+), 3 deletions(-)

Comments

Eric Auger Aug. 11, 2015, 9:15 a.m. UTC | #1
Hi Marc,
On 07/09/2015 03:19 PM, Marc Zyngier wrote:
> So far, GICv2 has been used in with EOImode == 0. The effect of this
> mode is to perform the priority drop and the deactivation of the
> interrupt at the same time.
> 
> While this works perfectly for Linux (we only have a single priority),
> it causes issues when an interrupt is forwarded to a guest, and when
> we want the guest to perform the EOI itself.
> 
> For this case, the GIC architecture provides EOImode == 1, where:
> - A write to the EOI register drops the priority of the interrupt and leaves
> it active. Other interrupts at the same priority level can now be taken,
> but the active interrupt cannot be taken again
> - A write to the DIR marks the interrupt as inactive, meaning it can
> now be taken again.
> 
> We only enable this feature when booted in HYP mode and that
> the device-tree reporte
reported
 a suitable CPU interface. Observable behaviour
> should remain unchanged.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic.c       | 32 +++++++++++++++++++++++++++++---
>  include/linux/irqchip/arm-gic.h |  4 ++++
>  2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 8d7e1c8..e264675 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -46,6 +46,7 @@
>  #include <asm/irq.h>
>  #include <asm/exception.h>
>  #include <asm/smp_plat.h>
> +#include <asm/virt.h>
>  
>  #include "irq-gic-common.h"
>  #include "irqchip.h"
> @@ -82,6 +83,8 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>  #define NR_GIC_CPU_IF 8
>  static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
>  
> +static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
> +
>  #ifndef MAX_GIC_NR
>  #define MAX_GIC_NR	1
>  #endif
> @@ -164,7 +167,10 @@ static void gic_unmask_irq(struct irq_data *d)
>  
>  static void gic_eoi_irq(struct irq_data *d)
>  {
> -	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
> +	if (static_key_true(&supports_deactivate))
> +		writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
> +	else
> +		writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>  }
>  
>  static int gic_irq_set_irqchip_state(struct irq_data *d,
> @@ -272,11 +278,15 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>  		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
>  
>  		if (likely(irqnr > 15 && irqnr < 1021)) {
shouldn't we have < 1020?
> +			if (static_key_true(&supports_deactivate))
> +				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
>  			handle_domain_irq(gic->domain, irqnr, regs);
why don't we handle the returned value of handle_domain_irq as we do in
GICv3?
>  			continue;
>  		}
>  		if (irqnr < 16) {
>  			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
> +			if (static_key_true(&supports_deactivate))
> +				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
>  #ifdef CONFIG_SMP
>  			handle_IPI(irqnr, regs);
>  #endif
> @@ -358,7 +368,11 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
>  static void gic_cpu_if_up(void)
>  {
>  	void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
> -	u32 bypass = 0;
> +	u32 bypass;
> +	u32 mode = 0;
> +
> +	if (static_key_true(&supports_deactivate))
> +		mode = GIC_CPU_CTRL_EOImodeNS;
>  
>  	/*
>  	* Preserve bypass disable bits to be written back later
> @@ -366,7 +380,7 @@ static void gic_cpu_if_up(void)
>  	bypass = readl(cpu_base + GIC_CPU_CTRL);
>  	bypass &= GICC_DIS_BYPASS_MASK;
>  
> -	writel_relaxed(bypass | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
> +	writel_relaxed(bypass | mode | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
>  }
>  
>  
> @@ -986,6 +1000,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  		register_cpu_notifier(&gic_cpu_notifier);
>  #endif
>  		set_handle_irq(gic_handle_irq);
> +		pr_info ("GIC: Using EOImode == %d\n",
> +			 static_key_true(&supports_deactivate));
> +
>  	}
>  
>  	gic_dist_init(gic);
> @@ -1001,6 +1018,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>  {
>  	void __iomem *cpu_base;
>  	void __iomem *dist_base;
> +	struct resource cpu_res;
>  	u32 percpu_offset;
>  	int irq;
>  
> @@ -1013,6 +1031,11 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>  	cpu_base = of_iomap(node, 1);
>  	WARN(!cpu_base, "unable to map gic cpu registers\n");
>  
> +	of_address_to_resource(node, 1, &cpu_res);
> +	if (!gic_cnt &&
> +	    (!is_hyp_mode_available() || resource_size(&cpu_res) < SZ_8K))
I don't understand why we check this size?
in GICv1 EOIMode necessarily is 0, right? is it related?

Best Regards

Eric
> +		static_key_slow_dec(&supports_deactivate);
> +
>  	if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
>  		percpu_offset = 0;
>  
> @@ -1131,6 +1154,9 @@ gic_v2_acpi_init(struct acpi_table_header *table)
>  		return -ENOMEM;
>  	}
>  
> +	if (!is_hyp_mode_available())
> +		static_key_slow_dec(&supports_deactivate);
> +
>  	/*
>  	 * Initialize zero GIC instance (no multi-GIC support). Also, set GIC
>  	 * as default IRQ domain to allow for GSI registration and GSI to IRQ
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index 9de976b..b1533c0 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -20,9 +20,13 @@
>  #define GIC_CPU_ALIAS_BINPOINT		0x1c
>  #define GIC_CPU_ACTIVEPRIO		0xd0
>  #define GIC_CPU_IDENT			0xfc
> +#define GIC_CPU_DEACTIVATE		0x1000
>  
>  #define GICC_ENABLE			0x1
>  #define GICC_INT_PRI_THRESHOLD		0xf0
> +
> +#define GIC_CPU_CTRL_EOImodeNS		(1 << 9)
> +
>  #define GICC_IAR_INT_ID_MASK		0x3ff
>  #define GICC_INT_SPURIOUS		1023
>  #define GICC_DIS_BYPASS_MASK		0x1e0
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier Aug. 12, 2015, 1:31 p.m. UTC | #2
Hi Eric,

On 11/08/15 10:15, Eric Auger wrote:
> Hi Marc,
> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>> So far, GICv2 has been used in with EOImode == 0. The effect of this
>> mode is to perform the priority drop and the deactivation of the
>> interrupt at the same time.
>>
>> While this works perfectly for Linux (we only have a single priority),
>> it causes issues when an interrupt is forwarded to a guest, and when
>> we want the guest to perform the EOI itself.
>>
>> For this case, the GIC architecture provides EOImode == 1, where:
>> - A write to the EOI register drops the priority of the interrupt and leaves
>> it active. Other interrupts at the same priority level can now be taken,
>> but the active interrupt cannot be taken again
>> - A write to the DIR marks the interrupt as inactive, meaning it can
>> now be taken again.
>>
>> We only enable this feature when booted in HYP mode and that
>> the device-tree reporte
> reported
>  a suitable CPU interface. Observable behaviour
>> should remain unchanged.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/irqchip/irq-gic.c       | 32 +++++++++++++++++++++++++++++---
>>  include/linux/irqchip/arm-gic.h |  4 ++++
>>  2 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 8d7e1c8..e264675 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -46,6 +46,7 @@
>>  #include <asm/irq.h>
>>  #include <asm/exception.h>
>>  #include <asm/smp_plat.h>
>> +#include <asm/virt.h>
>>  
>>  #include "irq-gic-common.h"
>>  #include "irqchip.h"
>> @@ -82,6 +83,8 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>>  #define NR_GIC_CPU_IF 8
>>  static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
>>  
>> +static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
>> +
>>  #ifndef MAX_GIC_NR
>>  #define MAX_GIC_NR	1
>>  #endif
>> @@ -164,7 +167,10 @@ static void gic_unmask_irq(struct irq_data *d)
>>  
>>  static void gic_eoi_irq(struct irq_data *d)
>>  {
>> -	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>> +	if (static_key_true(&supports_deactivate))
>> +		writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
>> +	else
>> +		writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
>>  }
>>  
>>  static int gic_irq_set_irqchip_state(struct irq_data *d,
>> @@ -272,11 +278,15 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>>  		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
>>  
>>  		if (likely(irqnr > 15 && irqnr < 1021)) {
> shouldn't we have < 1020?

Looks like you have unearthed a very long standing (though not fatal)
bug - I can trace it back to 2005 and the inclusion of the Realview
support (see include/asm-arm/arch-realview/entry-macro.S in 8ad68bbf for
the details).

It may be that the original GIC didn't make number 1020 a special one,
though the earliest spec I have access to (GICv1) is already making 1020
a reserved interrupt number. And looking at the pre-existing code
(arch/arm/common/gic.c), 1020 seems to already be considered an invalid
number.

CC-ing Catalin, as he was the one who introduced it... ;-) Unless he
says otherwise, I'll cook a patch for that.

>> +			if (static_key_true(&supports_deactivate))
>> +				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
>>  			handle_domain_irq(gic->domain, irqnr, regs);
> why don't we handle the returned value of handle_domain_irq as we do in
> GICv3?

Because I wrote the GICv3 code with my paranoia hat on... This really
should never fail.

>>  			continue;
>>  		}
>>  		if (irqnr < 16) {
>>  			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
>> +			if (static_key_true(&supports_deactivate))
>> +				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
>>  #ifdef CONFIG_SMP
>>  			handle_IPI(irqnr, regs);
>>  #endif
>> @@ -358,7 +368,11 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
>>  static void gic_cpu_if_up(void)
>>  {
>>  	void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
>> -	u32 bypass = 0;
>> +	u32 bypass;
>> +	u32 mode = 0;
>> +
>> +	if (static_key_true(&supports_deactivate))
>> +		mode = GIC_CPU_CTRL_EOImodeNS;
>>  
>>  	/*
>>  	* Preserve bypass disable bits to be written back later
>> @@ -366,7 +380,7 @@ static void gic_cpu_if_up(void)
>>  	bypass = readl(cpu_base + GIC_CPU_CTRL);
>>  	bypass &= GICC_DIS_BYPASS_MASK;
>>  
>> -	writel_relaxed(bypass | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
>> +	writel_relaxed(bypass | mode | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
>>  }
>>  
>>  
>> @@ -986,6 +1000,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>  		register_cpu_notifier(&gic_cpu_notifier);
>>  #endif
>>  		set_handle_irq(gic_handle_irq);
>> +		pr_info ("GIC: Using EOImode == %d\n",
>> +			 static_key_true(&supports_deactivate));
>> +
>>  	}
>>  
>>  	gic_dist_init(gic);
>> @@ -1001,6 +1018,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>>  {
>>  	void __iomem *cpu_base;
>>  	void __iomem *dist_base;
>> +	struct resource cpu_res;
>>  	u32 percpu_offset;
>>  	int irq;
>>  
>> @@ -1013,6 +1031,11 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>>  	cpu_base = of_iomap(node, 1);
>>  	WARN(!cpu_base, "unable to map gic cpu registers\n");
>>  
>> +	of_address_to_resource(node, 1, &cpu_res);
>> +	if (!gic_cnt &&
>> +	    (!is_hyp_mode_available() || resource_size(&cpu_res) < SZ_8K))
> I don't understand why we check this size?
> in GICv1 EOIMode necessarily is 0, right? is it related?

This is indeed related. For a GIC to support EOIMode==1, you need to
have access to the DIR register, which has the good idea of being
located on a separate 4k page, making the size of the CPU interface 8k
in total. Anything smaller means you don't have a DIR register.

It is worth noticing that most of the DTS files containing a reference
to GICv2 are wrong, and are only exposing a 4k CPU interface. These
platforms won't be able to use this feature until they are fixed.
Basically anything with an A7/A15/A53/A57/X-Gene.

Thanks,

	M.
Catalin Marinas Aug. 12, 2015, 5:40 p.m. UTC | #3
On Wed, Aug 12, 2015 at 02:31:47PM +0100, Marc Zyngier wrote:
> On 11/08/15 10:15, Eric Auger wrote:
> > On 07/09/2015 03:19 PM, Marc Zyngier wrote:
> >>  static int gic_irq_set_irqchip_state(struct irq_data *d,
> >> @@ -272,11 +278,15 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
> >>  		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> >>  
> >>  		if (likely(irqnr > 15 && irqnr < 1021)) {
> > shouldn't we have < 1020?
> 
> Looks like you have unearthed a very long standing (though not fatal)
> bug - I can trace it back to 2005 and the inclusion of the Realview
> support (see include/asm-arm/arch-realview/entry-macro.S in 8ad68bbf for
> the details).
> 
> It may be that the original GIC didn't make number 1020 a special one,
> though the earliest spec I have access to (GICv1) is already making 1020
> a reserved interrupt number. And looking at the pre-existing code
> (arch/arm/common/gic.c), 1020 seems to already be considered an invalid
> number.
> 
> CC-ing Catalin, as he was the one who introduced it... ;-) Unless he
> says otherwise, I'll cook a patch for that.

I really have no idea where it came from. The code probably pre-dates
the existence of a GIC architecture spec (the GIC spec used to be part
of the board or CPU TRM).

I don't see any problem with using 1020 here.

Patch
diff mbox

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 8d7e1c8..e264675 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -46,6 +46,7 @@ 
 #include <asm/irq.h>
 #include <asm/exception.h>
 #include <asm/smp_plat.h>
+#include <asm/virt.h>
 
 #include "irq-gic-common.h"
 #include "irqchip.h"
@@ -82,6 +83,8 @@  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 #define NR_GIC_CPU_IF 8
 static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
 
+static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
+
 #ifndef MAX_GIC_NR
 #define MAX_GIC_NR	1
 #endif
@@ -164,7 +167,10 @@  static void gic_unmask_irq(struct irq_data *d)
 
 static void gic_eoi_irq(struct irq_data *d)
 {
-	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+	if (static_key_true(&supports_deactivate))
+		writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
+	else
+		writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
 }
 
 static int gic_irq_set_irqchip_state(struct irq_data *d,
@@ -272,11 +278,15 @@  static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
 		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
 
 		if (likely(irqnr > 15 && irqnr < 1021)) {
+			if (static_key_true(&supports_deactivate))
+				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
 			handle_domain_irq(gic->domain, irqnr, regs);
 			continue;
 		}
 		if (irqnr < 16) {
 			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
+			if (static_key_true(&supports_deactivate))
+				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
 #ifdef CONFIG_SMP
 			handle_IPI(irqnr, regs);
 #endif
@@ -358,7 +368,11 @@  static u8 gic_get_cpumask(struct gic_chip_data *gic)
 static void gic_cpu_if_up(void)
 {
 	void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
-	u32 bypass = 0;
+	u32 bypass;
+	u32 mode = 0;
+
+	if (static_key_true(&supports_deactivate))
+		mode = GIC_CPU_CTRL_EOImodeNS;
 
 	/*
 	* Preserve bypass disable bits to be written back later
@@ -366,7 +380,7 @@  static void gic_cpu_if_up(void)
 	bypass = readl(cpu_base + GIC_CPU_CTRL);
 	bypass &= GICC_DIS_BYPASS_MASK;
 
-	writel_relaxed(bypass | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
+	writel_relaxed(bypass | mode | GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
 }
 
 
@@ -986,6 +1000,9 @@  void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		register_cpu_notifier(&gic_cpu_notifier);
 #endif
 		set_handle_irq(gic_handle_irq);
+		pr_info ("GIC: Using EOImode == %d\n",
+			 static_key_true(&supports_deactivate));
+
 	}
 
 	gic_dist_init(gic);
@@ -1001,6 +1018,7 @@  gic_of_init(struct device_node *node, struct device_node *parent)
 {
 	void __iomem *cpu_base;
 	void __iomem *dist_base;
+	struct resource cpu_res;
 	u32 percpu_offset;
 	int irq;
 
@@ -1013,6 +1031,11 @@  gic_of_init(struct device_node *node, struct device_node *parent)
 	cpu_base = of_iomap(node, 1);
 	WARN(!cpu_base, "unable to map gic cpu registers\n");
 
+	of_address_to_resource(node, 1, &cpu_res);
+	if (!gic_cnt &&
+	    (!is_hyp_mode_available() || resource_size(&cpu_res) < SZ_8K))
+		static_key_slow_dec(&supports_deactivate);
+
 	if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;
 
@@ -1131,6 +1154,9 @@  gic_v2_acpi_init(struct acpi_table_header *table)
 		return -ENOMEM;
 	}
 
+	if (!is_hyp_mode_available())
+		static_key_slow_dec(&supports_deactivate);
+
 	/*
 	 * Initialize zero GIC instance (no multi-GIC support). Also, set GIC
 	 * as default IRQ domain to allow for GSI registration and GSI to IRQ
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 9de976b..b1533c0 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -20,9 +20,13 @@ 
 #define GIC_CPU_ALIAS_BINPOINT		0x1c
 #define GIC_CPU_ACTIVEPRIO		0xd0
 #define GIC_CPU_IDENT			0xfc
+#define GIC_CPU_DEACTIVATE		0x1000
 
 #define GICC_ENABLE			0x1
 #define GICC_INT_PRI_THRESHOLD		0xf0
+
+#define GIC_CPU_CTRL_EOImodeNS		(1 << 9)
+
 #define GICC_IAR_INT_ID_MASK		0x3ff
 #define GICC_INT_SPURIOUS		1023
 #define GICC_DIS_BYPASS_MASK		0x1e0