Re: [PATCH 2/6] irqchip: GIC: Convert to EOImode == 1

[Marc Zyngier <marc.zyngier@arm.com>]

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.
-- 
Jazz is not dead. It just smells funny...