[PATCH 0/6] irqchip: GICv2/v3: Add support for irq_vcpu_affinity

[PATCH 0/6] irqchip: GICv2/v3: Add support for irq_vcpu_affinity

[Marc Zyngier]

The GICv2 and GICv3 architectures allow an active physical interrupt
to be forwarded to a guest, and the guest to indirectly perform the
deactivation of the interrupt by performing an EOI on the virtual
interrupt (see for example the GICv2 spec, 3.2.1).

This allows some substantial performance improvement for level
triggered interrupts that otherwise have to be masked/unmasked in
VFIO, not to mention the required trap back to KVM when the guest
performs an EOI.

To enable this, the GICs need to be switched to a different EOImode,
where a taken interrupt can be left "active" (which prevents the same
interrupt from being taken again), while other interrupts are still
being processed normally.

We also use the new irq_set_vcpu_affinity hook that was introduced for
Intel's "Posted Interrupts" to determine whether or not to perform the
deactivation at EOI-time.

As all of this only makes sense when the kernel can behave as a
hypervisor, we only enable this mode on detecting that the kernel was
actually booted in HYP mode, and that the GIC supports this feature.

This series is a complete rework of a RFC I sent over a year ago:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266328.html

Since then, a lot has been either merged (the irqchip_state) or reworked
(my active-timer series: http://www.spinics.net/lists/kvm/msg118768.html),
and this implements the last few bits for Eric Auger's series to
finally make it into the kernel:

https://lkml.org/lkml/2015/7/2/268
https://lkml.org/lkml/2015/7/6/291

With all these patches combined, physical interrupt routing from the
kernel into a VM becomes possible.

This has been tested on Juno (GICv2) and FastModel (GICv3). A branch
is available at:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/gic-irq-vcpu-affinity

Marc Zyngier (6):
  irqchip: GICv3: Convert to EOImode == 1
  irqchip: GIC: Convert to EOImode == 1
  irqchip: GICv3: Skip LPI deactivation
  irqchip: GIC: Use chip_data instead of handler_data for cascaded
    interrupts
  irqchip: GICv3: Don't deactivate interrupts forwarded to a guest
  irqchip: GIC: Don't deactivate interrupts forwarded to a guest

 drivers/irqchip/irq-gic-v3.c       | 57 +++++++++++++++++++++++++++++++--
 drivers/irqchip/irq-gic.c          | 64 +++++++++++++++++++++++++++++++++++---
 include/linux/irqchip/arm-gic-v3.h |  9 ++++++
 include/linux/irqchip/arm-gic.h    |  4 +++
 4 files changed, 126 insertions(+), 8 deletions(-)

-- 
2.1.4

[PATCH 1/6] irqchip: GICv3: Convert to EOImode == 1

[Marc Zyngier]

So far, GICv3 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 ICC_EOIR1_EL1 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 ICC_DIR_EL1 marks the interrupt as inactive, meaning it can
now be taken again.

This patch converts the driver to be able to use this new mode, depending
on whether or not the kernel can behave as a hypervisor. No feature change.

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

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c52f7ba..49768fc 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -30,6 +30,7 @@
 #include <asm/cputype.h>
 #include <asm/exception.h>
 #include <asm/smp_plat.h>
+#include <asm/virt.h>
 
 #include "irq-gic-common.h"
 #include "irqchip.h"
@@ -50,6 +51,7 @@ struct gic_chip_data {
 };
 
 static struct gic_chip_data gic_data __read_mostly;
+static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
 
 #define gic_data_rdist()		(this_cpu_ptr(gic_data.rdists.rdist))
 #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
@@ -293,7 +295,10 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
 
 static void gic_eoi_irq(struct irq_data *d)
 {
-	gic_write_eoir(gic_irq(d));
+	if (static_key_true(&supports_deactivate))
+		gic_write_dir(gic_irq(d));
+	else
+		gic_write_eoir(gic_irq(d));
 }
 
 static int gic_set_type(struct irq_data *d, unsigned int type)
@@ -343,6 +348,10 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
 
 		if (likely(irqnr > 15 && irqnr < 1020) || irqnr >= 8192) {
 			int err;
+
+			if (static_key_true(&supports_deactivate))
+				gic_write_eoir(irqnr);
+
 			err = handle_domain_irq(gic_data.domain, irqnr, regs);
 			if (err) {
 				WARN_ONCE(true, "Unexpected interrupt received!\n");
@@ -352,6 +361,8 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
 		}
 		if (irqnr < 16) {
 			gic_write_eoir(irqnr);
+			if (static_key_true(&supports_deactivate))
+				gic_write_dir(irqnr);
 #ifdef CONFIG_SMP
 			handle_IPI(irqnr, regs);
 #else
@@ -451,8 +462,13 @@ static void gic_cpu_sys_reg_init(void)
 	/* Set priority mask register */
 	gic_write_pmr(DEFAULT_PMR_VALUE);
 
-	/* EOI deactivates interrupt too (mode 0) */
-	gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop_dir);
+	if (static_key_true(&supports_deactivate)) {
+		/* EOI drops priority only (mode 1) */
+		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop);
+	} else {
+		/* EOI deactivates interrupt too (mode 0) */
+		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop_dir);
+	}
 
 	/* ... and let's hit the road... */
 	gic_write_grpen1(1);
@@ -820,6 +836,12 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
 	if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
 		redist_stride = 0;
 
+	if (!is_hyp_mode_available())
+		static_key_slow_dec(&supports_deactivate);
+
+	pr_info ("GIC: Using EOImode == %d\n",
+		 static_key_true(&supports_deactivate));
+
 	gic_data.dist_base = dist_base;
 	gic_data.redist_regions = rdist_regs;
 	gic_data.nr_redist_regions = nr_redist_regions;
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index ffbc034..bc98832 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -104,6 +104,8 @@
 #define GICR_SYNCR			0x00C0
 #define GICR_MOVLPIR			0x0100
 #define GICR_MOVALLR			0x0110
+#define GICR_ISACTIVER			GICD_ISACTIVER
+#define GICR_ICACTIVER			GICD_ICACTIVER
 #define GICR_IDREGS			GICD_IDREGS
 #define GICR_PIDR2			GICD_PIDR2
 
@@ -288,6 +290,7 @@
 #define ICH_VMCR_PMR_MASK		(0xffUL << ICH_VMCR_PMR_SHIFT)
 
 #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
+#define ICC_DIR_EL1			sys_reg(3, 0, 12, 11, 1)
 #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
 #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
 #define ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
@@ -384,6 +387,12 @@ static inline void gic_write_eoir(u64 irq)
 	isb();
 }
 
+static inline void gic_write_dir(u64 irq)
+{
+	asm volatile("msr_s " __stringify(ICC_DIR_EL1) ", %0" : : "r" (irq));
+	isb();
+}
+
 struct irq_domain;
 int its_cpu_init(void);
 int its_init(struct device_node *node, struct rdists *rdists,
-- 
2.1.4

[PATCH 1/6] irqchip: GICv3: Convert to EOImode == 1

[Eric Auger]

Hi Marc,
On 07/09/2015 03:19 PM, Marc Zyngier wrote:
> So far, GICv3 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 ICC_EOIR1_EL1 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 ICC_DIR_EL1 marks the interrupt as inactive, meaning it can
> now be taken again.
> 
> This patch converts the driver to be able to use this new mode, depending
> on whether or not the kernel can behave as a hypervisor. No feature change.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic-v3.c       | 28 +++++++++++++++++++++++++---
>  include/linux/irqchip/arm-gic-v3.h |  9 +++++++++
>  2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index c52f7ba..49768fc 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -30,6 +30,7 @@
>  #include <asm/cputype.h>
>  #include <asm/exception.h>
>  #include <asm/smp_plat.h>
> +#include <asm/virt.h>
>  
>  #include "irq-gic-common.h"
>  #include "irqchip.h"
> @@ -50,6 +51,7 @@ struct gic_chip_data {
>  };
>  
>  static struct gic_chip_data gic_data __read_mostly;
> +static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
>  
>  #define gic_data_rdist()		(this_cpu_ptr(gic_data.rdists.rdist))
>  #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
> @@ -293,7 +295,10 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
>  
>  static void gic_eoi_irq(struct irq_data *d)
>  {
> -	gic_write_eoir(gic_irq(d));
> +	if (static_key_true(&supports_deactivate))
> +		gic_write_dir(gic_irq(d));
> +	else
> +		gic_write_eoir(gic_irq(d));
>  }
>  
>  static int gic_set_type(struct irq_data *d, unsigned int type)
> @@ -343,6 +348,10 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>  
>  		if (likely(irqnr > 15 && irqnr < 1020) || irqnr >= 8192) {
>  			int err;
> +
> +			if (static_key_true(&supports_deactivate))
> +				gic_write_eoir(irqnr);
> +
>  			err = handle_domain_irq(gic_data.domain, irqnr, regs);
>  			if (err) {
>  				WARN_ONCE(true, "Unexpected interrupt received!\n");
shouldn't we DIR here as well in case of err (we did EOI before)?
Besides Reviewed-by: Eric Auger <eric.auger@linaro.org> if it can help.

Eric
> @@ -352,6 +361,8 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>  		}
>  		if (irqnr < 16) {
>  			gic_write_eoir(irqnr);
> +			if (static_key_true(&supports_deactivate))
> +				gic_write_dir(irqnr);
>  #ifdef CONFIG_SMP
>  			handle_IPI(irqnr, regs);
>  #else
> @@ -451,8 +462,13 @@ static void gic_cpu_sys_reg_init(void)
>  	/* Set priority mask register */
>  	gic_write_pmr(DEFAULT_PMR_VALUE);
>  
> -	/* EOI deactivates interrupt too (mode 0) */
> -	gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop_dir);
> +	if (static_key_true(&supports_deactivate)) {
> +		/* EOI drops priority only (mode 1) */
> +		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop);
> +	} else {
> +		/* EOI deactivates interrupt too (mode 0) */
> +		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop_dir);
> +	}
>  
>  	/* ... and let's hit the road... */
>  	gic_write_grpen1(1);
> @@ -820,6 +836,12 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>  	if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
>  		redist_stride = 0;
>  
> +	if (!is_hyp_mode_available())
> +		static_key_slow_dec(&supports_deactivate);
> +
> +	pr_info ("GIC: Using EOImode == %d\n",
> +		 static_key_true(&supports_deactivate));
> +
>  	gic_data.dist_base = dist_base;
>  	gic_data.redist_regions = rdist_regs;
>  	gic_data.nr_redist_regions = nr_redist_regions;
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index ffbc034..bc98832 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -104,6 +104,8 @@
>  #define GICR_SYNCR			0x00C0
>  #define GICR_MOVLPIR			0x0100
>  #define GICR_MOVALLR			0x0110
> +#define GICR_ISACTIVER			GICD_ISACTIVER
> +#define GICR_ICACTIVER			GICD_ICACTIVER
>  #define GICR_IDREGS			GICD_IDREGS
>  #define GICR_PIDR2			GICD_PIDR2
>  
> @@ -288,6 +290,7 @@
>  #define ICH_VMCR_PMR_MASK		(0xffUL << ICH_VMCR_PMR_SHIFT)
>  
>  #define ICC_EOIR1_EL1			sys_reg(3, 0, 12, 12, 1)
> +#define ICC_DIR_EL1			sys_reg(3, 0, 12, 11, 1)
>  #define ICC_IAR1_EL1			sys_reg(3, 0, 12, 12, 0)
>  #define ICC_SGI1R_EL1			sys_reg(3, 0, 12, 11, 5)
>  #define ICC_PMR_EL1			sys_reg(3, 0, 4, 6, 0)
> @@ -384,6 +387,12 @@ static inline void gic_write_eoir(u64 irq)
>  	isb();
>  }
>  
> +static inline void gic_write_dir(u64 irq)
> +{
> +	asm volatile("msr_s " __stringify(ICC_DIR_EL1) ", %0" : : "r" (irq));
> +	isb();
> +}
> +
>  struct irq_domain;
>  int its_cpu_init(void);
>  int its_init(struct device_node *node, struct rdists *rdists,
> 

[PATCH 1/6] irqchip: GICv3: Convert to EOImode == 1

[Marc Zyngier]

On 11/08/15 10:14, Eric Auger wrote:
> Hi Marc,
> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>> So far, GICv3 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 ICC_EOIR1_EL1 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 ICC_DIR_EL1 marks the interrupt as inactive, meaning it can
>> now be taken again.
>>
>> This patch converts the driver to be able to use this new mode, depending
>> on whether or not the kernel can behave as a hypervisor. No feature change.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/irqchip/irq-gic-v3.c       | 28 +++++++++++++++++++++++++---
>>  include/linux/irqchip/arm-gic-v3.h |  9 +++++++++
>>  2 files changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index c52f7ba..49768fc 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -30,6 +30,7 @@
>>  #include <asm/cputype.h>
>>  #include <asm/exception.h>
>>  #include <asm/smp_plat.h>
>> +#include <asm/virt.h>
>>  
>>  #include "irq-gic-common.h"
>>  #include "irqchip.h"
>> @@ -50,6 +51,7 @@ struct gic_chip_data {
>>  };
>>  
>>  static struct gic_chip_data gic_data __read_mostly;
>> +static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
>>  
>>  #define gic_data_rdist()		(this_cpu_ptr(gic_data.rdists.rdist))
>>  #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
>> @@ -293,7 +295,10 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
>>  
>>  static void gic_eoi_irq(struct irq_data *d)
>>  {
>> -	gic_write_eoir(gic_irq(d));
>> +	if (static_key_true(&supports_deactivate))
>> +		gic_write_dir(gic_irq(d));
>> +	else
>> +		gic_write_eoir(gic_irq(d));
>>  }
>>  
>>  static int gic_set_type(struct irq_data *d, unsigned int type)
>> @@ -343,6 +348,10 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
>>  
>>  		if (likely(irqnr > 15 && irqnr < 1020) || irqnr >= 8192) {
>>  			int err;
>> +
>> +			if (static_key_true(&supports_deactivate))
>> +				gic_write_eoir(irqnr);
>> +
>>  			err = handle_domain_irq(gic_data.domain, irqnr, regs);
>>  			if (err) {
>>  				WARN_ONCE(true, "Unexpected interrupt received!\n");
> shouldn't we DIR here as well in case of err (we did EOI before)?

Yes, we should, very good point. I'll fix that up.

> Besides Reviewed-by: Eric Auger <eric.auger@linaro.org> if it can help.
> 

Thanks!

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

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

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

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

[Eric Auger]

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
> 

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

[Marc Zyngier]

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

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

[Catalin Marinas]

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.

-- 
Catalin

[PATCH 3/6] irqchip: GICv3: Skip LPI deactivation

[Marc Zyngier]

Contrary to other GICv3 interrupts, LPIs do not have an active state
by virtue of being edge-triggered only (they only have a pending state).

Given this, there is no point trying to deactivate them, and we can
skip the ICC_DIR_EL1 entierely.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 49768fc..e02592b 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -295,10 +295,14 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
 
 static void gic_eoi_irq(struct irq_data *d)
 {
-	if (static_key_true(&supports_deactivate))
+	if (static_key_true(&supports_deactivate)) {
+		/* No need to deactivate an LPI */
+		if (gic_irq(d) >= 8192)
+			return;
 		gic_write_dir(gic_irq(d));
-	else
+	} else {
 		gic_write_eoir(gic_irq(d));
+	}
 }
 
 static int gic_set_type(struct irq_data *d, unsigned int type)
-- 
2.1.4

[PATCH 3/6] irqchip: GICv3: Skip LPI deactivation

[Eric Auger]

On 07/09/2015 03:19 PM, Marc Zyngier wrote:
> Contrary to other GICv3 interrupts, LPIs do not have an active state
> by virtue of being edge-triggered only (they only have a pending state).
> 
> Given this, there is no point trying to deactivate them, and we can
> skip the ICC_DIR_EL1 entierely.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic-v3.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 49768fc..e02592b 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -295,10 +295,14 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
>  
>  static void gic_eoi_irq(struct irq_data *d)
>  {
> -	if (static_key_true(&supports_deactivate))
> +	if (static_key_true(&supports_deactivate)) {
> +		/* No need to deactivate an LPI */
> +		if (gic_irq(d) >= 8192)
In case of EOIMode == 0, we do not call EOI. I can't understand whether
it is an issue.
In 4.8.3 Properties of LPI, in 2d note it is written:

"SW must issue a write to EOI to clear the active priorities register,
hence the CPU interface still requires an active state for LPIs, even
through this is not necessary within the redistributor"

Eric
> +			return;
>  		gic_write_dir(gic_irq(d));
> -	else
> +	} else {
>  		gic_write_eoir(gic_irq(d));
> +	}
>  }
>  
>  static int gic_set_type(struct irq_data *d, unsigned int type)
> 

[PATCH 3/6] irqchip: GICv3: Skip LPI deactivation

[Marc Zyngier]

On 11/08/15 10:42, Eric Auger wrote:
> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>> Contrary to other GICv3 interrupts, LPIs do not have an active state
>> by virtue of being edge-triggered only (they only have a pending state).
>>
>> Given this, there is no point trying to deactivate them, and we can
>> skip the ICC_DIR_EL1 entierely.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/irqchip/irq-gic-v3.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 49768fc..e02592b 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -295,10 +295,14 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
>>  
>>  static void gic_eoi_irq(struct irq_data *d)
>>  {
>> -	if (static_key_true(&supports_deactivate))
>> +	if (static_key_true(&supports_deactivate)) {
>> +		/* No need to deactivate an LPI */
>> +		if (gic_irq(d) >= 8192)
> In case of EOIMode == 0, we do not call EOI. I can't understand whether
> it is an issue.

What do you mean? We definitely perform an EOI in both EOImodes...

> In 4.8.3 Properties of LPI, in 2d note it is written:
> 
> "SW must issue a write to EOI to clear the active priorities register,
> hence the CPU interface still requires an active state for LPIs, even
> through this is not necessary within the redistributor"
> 
> Eric
>> +			return;
>>  		gic_write_dir(gic_irq(d));
>> -	else
>> +	} else {
>>  		gic_write_eoir(gic_irq(d));

... right here.

Of am I missing something completely obvious?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 3/6] irqchip: GICv3: Skip LPI deactivation

[Eric Auger]

On 08/12/2015 03:34 PM, Marc Zyngier wrote:
> On 11/08/15 10:42, Eric Auger wrote:
>> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>>> Contrary to other GICv3 interrupts, LPIs do not have an active state
>>> by virtue of being edge-triggered only (they only have a pending state).
>>>
>>> Given this, there is no point trying to deactivate them, and we can
>>> skip the ICC_DIR_EL1 entierely.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  drivers/irqchip/irq-gic-v3.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index 49768fc..e02592b 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -295,10 +295,14 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
>>>  
>>>  static void gic_eoi_irq(struct irq_data *d)
>>>  {
>>> -	if (static_key_true(&supports_deactivate))
>>> +	if (static_key_true(&supports_deactivate)) {
>>> +		/* No need to deactivate an LPI */
>>> +		if (gic_irq(d) >= 8192)
>> In case of EOIMode == 0, we do not call EOI. I can't understand whether
>> it is an issue.
> 
> What do you mean? We definitely perform an EOI in both EOImodes...
> 
>> In 4.8.3 Properties of LPI, in 2d note it is written:
>>
>> "SW must issue a write to EOI to clear the active priorities register,
>> hence the CPU interface still requires an active state for LPIs, even
>> through this is not necessary within the redistributor"
>>
>> Eric
>>> +			return;
>>>  		gic_write_dir(gic_irq(d));
>>> -	else
>>> +	} else {
>>>  		gic_write_eoir(gic_irq(d));
> 
> ... right here.
> 
> Of am I missing something completely obvious?
yes sorry please forget this. I think I meant EOImode == 1 instead and
anyway the EOI is done in gic_handle_irq.

Apologies

Eric
> 
> Thanks,
> 
> 	M.
> 

[PATCH 4/6] irqchip: GIC: Use chip_data instead of handler_data for cascaded interrupts

[Marc Zyngier]

When used as a primary interrupt controller, the GIC driver uses
irq_data->chip_data to extract controller-specific information.

When used as a secondary interrupt controller, it uses handler_data
instead. As this difference is relatively pointless and only creates
confusion, change the secondary path to match what the rest of the
driver does.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index e264675..3c7f3a4 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
 
 static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 {
-	struct gic_chip_data *chip_data = irq_get_handler_data(irq);
+	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
 	struct irq_chip *chip = irq_get_chip(irq);
 	unsigned int cascade_irq, gic_irq;
 	unsigned long status;
@@ -341,7 +341,7 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 {
 	if (gic_nr >= MAX_GIC_NR)
 		BUG();
-	if (irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
+	if (irq_set_chip_data(irq, &gic_data[gic_nr]) != 0)
 		BUG();
 	irq_set_chained_handler(irq, gic_handle_cascade_irq);
 }
-- 
2.1.4

[PATCH 4/6] irqchip: GIC: Use chip_data instead of handler_data for cascaded interrupts

[Thomas Gleixner]

On Thu, 9 Jul 2015, Marc Zyngier wrote:

> When used as a primary interrupt controller, the GIC driver uses
> irq_data->chip_data to extract controller-specific information.
> 
> When used as a secondary interrupt controller, it uses handler_data
> instead. As this difference is relatively pointless and only creates
> confusion, change the secondary path to match what the rest of the
> driver does.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index e264675..3c7f3a4 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>  
>  static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
>  {
> -	struct gic_chip_data *chip_data = irq_get_handler_data(irq);
> +	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
>  	struct irq_chip *chip = irq_get_chip(irq);

You should make that

    chip_data = irq_desc_get_chip_data(desc);
    chip = irq_desc_get_chip(desc);

That avoids two pointless lookups of irqdesc ....

Thanks,

	tglx

[PATCH 4/6] irqchip: GIC: Use chip_data instead of handler_data for cascaded interrupts

[Marc Zyngier]

On 09/07/15 22:33, Thomas Gleixner wrote:
> On Thu, 9 Jul 2015, Marc Zyngier wrote:
> 
>> When used as a primary interrupt controller, the GIC driver uses
>> irq_data->chip_data to extract controller-specific information.
>>
>> When used as a secondary interrupt controller, it uses handler_data
>> instead. As this difference is relatively pointless and only creates
>> confusion, change the secondary path to match what the rest of the
>> driver does.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/irqchip/irq-gic.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index e264675..3c7f3a4 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>>  
>>  static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
>>  {
>> -	struct gic_chip_data *chip_data = irq_get_handler_data(irq);
>> +	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
>>  	struct irq_chip *chip = irq_get_chip(irq);
> 
> You should make that
> 
>     chip_data = irq_desc_get_chip_data(desc);
>     chip = irq_desc_get_chip(desc);
> 
> That avoids two pointless lookups of irqdesc ....

Ah, very good point.

And it turns out that these constructs (use of irq_get_* when the
irq_desc is readily available) is actually fairly common in a number of
irqchip implementations used as secondary interrupt controllers.

Time for another cleanup series, I believe... ;-)

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/6] irqchip: GIC: Use chip_data instead of handler_data for cascaded interrupts

[Jiang Liu]

On 2015/7/10 15:52, Marc Zyngier wrote:
> On 09/07/15 22:33, Thomas Gleixner wrote:
>> On Thu, 9 Jul 2015, Marc Zyngier wrote:
>>
>>> When used as a primary interrupt controller, the GIC driver uses
>>> irq_data->chip_data to extract controller-specific information.
>>>
>>> When used as a secondary interrupt controller, it uses handler_data
>>> instead. As this difference is relatively pointless and only creates
>>> confusion, change the secondary path to match what the rest of the
>>> driver does.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  drivers/irqchip/irq-gic.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index e264675..3c7f3a4 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>>>  
>>>  static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
>>>  {
>>> -	struct gic_chip_data *chip_data = irq_get_handler_data(irq);
>>> +	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
>>>  	struct irq_chip *chip = irq_get_chip(irq);
>>
>> You should make that
>>
>>     chip_data = irq_desc_get_chip_data(desc);
>>     chip = irq_desc_get_chip(desc);
>>
>> That avoids two pointless lookups of irqdesc ....
> 
> Ah, very good point.
> 
> And it turns out that these constructs (use of irq_get_* when the
> irq_desc is readily available) is actually fairly common in a number of
> irqchip implementations used as secondary interrupt controllers.
> 
> Time for another cleanup series, I believe... ;-)
Hi Marc,
	I'm ahead of your request and expect an reviewed-by from
you then:). Please refer to http://lkml.org/lkml/2015/6/4/743 for
what we have done to clear up this interfaces.
Thanks!
Gerry

[PATCH 4/6] irqchip: GIC: Use chip_data instead of handler_data for cascaded interrupts

[Marc Zyngier]

On 10/07/15 09:17, Jiang Liu wrote:
> On 2015/7/10 15:52, Marc Zyngier wrote:
>> On 09/07/15 22:33, Thomas Gleixner wrote:
>>> On Thu, 9 Jul 2015, Marc Zyngier wrote:
>>>
>>>> When used as a primary interrupt controller, the GIC driver uses
>>>> irq_data->chip_data to extract controller-specific information.
>>>>
>>>> When used as a secondary interrupt controller, it uses handler_data
>>>> instead. As this difference is relatively pointless and only creates
>>>> confusion, change the secondary path to match what the rest of the
>>>> driver does.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  drivers/irqchip/irq-gic.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>>> index e264675..3c7f3a4 100644
>>>> --- a/drivers/irqchip/irq-gic.c
>>>> +++ b/drivers/irqchip/irq-gic.c
>>>> @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>>>>  
>>>>  static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
>>>>  {
>>>> -	struct gic_chip_data *chip_data = irq_get_handler_data(irq);
>>>> +	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
>>>>  	struct irq_chip *chip = irq_get_chip(irq);
>>>
>>> You should make that
>>>
>>>     chip_data = irq_desc_get_chip_data(desc);
>>>     chip = irq_desc_get_chip(desc);
>>>
>>> That avoids two pointless lookups of irqdesc ....
>>
>> Ah, very good point.
>>
>> And it turns out that these constructs (use of irq_get_* when the
>> irq_desc is readily available) is actually fairly common in a number of
>> irqchip implementations used as secondary interrupt controllers.
>>
>> Time for another cleanup series, I believe... ;-)
> Hi Marc,
> 	I'm ahead of your request and expect an reviewed-by from
> you then:). Please refer to http://lkml.org/lkml/2015/6/4/743 for
> what we have done to clear up this interfaces.

Ah, let me dig this now (yeah, I've been slacking off on reviewing
lately...). Looks awesome! :-)

Thanks!

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/6] irqchip: GIC: Use chip_data instead of handler_data for cascaded interrupts

[Eric Auger]

Hi Marc,
On 07/09/2015 03:19 PM, Marc Zyngier wrote:
> When used as a primary interrupt controller, the GIC driver uses
> irq_data->chip_data to extract controller-specific information.
> 
> When used as a secondary interrupt controller, it uses handler_data
> instead. As this difference is relatively pointless and only creates
> confusion, change the secondary path to match what the rest of the
> driver does.
not really related to this series' topic, is it?

Eric
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index e264675..3c7f3a4 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -298,7 +298,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>  
>  static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
>  {
> -	struct gic_chip_data *chip_data = irq_get_handler_data(irq);
> +	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
>  	struct irq_chip *chip = irq_get_chip(irq);
>  	unsigned int cascade_irq, gic_irq;
>  	unsigned long status;
> @@ -341,7 +341,7 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>  {
>  	if (gic_nr >= MAX_GIC_NR)
>  		BUG();
> -	if (irq_set_handler_data(irq, &gic_data[gic_nr]) != 0)
> +	if (irq_set_chip_data(irq, &gic_data[gic_nr]) != 0)
>  		BUG();
>  	irq_set_chained_handler(irq, gic_handle_cascade_irq);
>  }
> 

[PATCH 4/6] irqchip: GIC: Use chip_data instead of handler_data for cascaded interrupts

[Marc Zyngier]

On 11/08/15 10:45, Eric Auger wrote:
> Hi Marc,
> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>> When used as a primary interrupt controller, the GIC driver uses
>> irq_data->chip_data to extract controller-specific information.
>>
>> When used as a secondary interrupt controller, it uses handler_data
>> instead. As this difference is relatively pointless and only creates
>> confusion, change the secondary path to match what the rest of the
>> driver does.
> not really related to this series' topic, is it?

It is. Or was. We use the handler_data field to hold the vcpu
information, but that's also used by the chained interrupt handler to
find out which secondary it is.

But this patch is wrong anyway, as it entirely breaks secondary GICs (I
found out when I merged this with a patch from Jon Hunter fixing some
other related stuff). Took me hours to get an EB up and running!

I have a working version now, which makes this patch irrelevant.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/6] irqchip: GICv3: Don't deactivate interrupts forwarded to a guest

[Marc Zyngier]

Commit 0a4377de3056 ("genirq: Introduce irq_set_vcpu_affinity() to
target an interrupt to a VCPU") added just what we needed at the
lowest level to allow an interrupt to be deactivated by a guest.

When such a request reaches the GIC, it knows it doesn't need to
perform the deactivation anymore, and can safely leave the guest
do its magic. This of course requires additional support in both
VFIO and KVM.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index e02592b..a1ca9e6 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -70,6 +70,11 @@ static inline int gic_irq_in_rdist(struct irq_data *d)
 	return gic_irq(d) < 32;
 }
 
+static inline bool forwarded_irq(struct irq_data *d)
+{
+	return d->handler_data != NULL;
+}
+
 static inline void __iomem *gic_dist_base(struct irq_data *d)
 {
 	if (gic_irq_in_rdist(d))	/* SGI+PPI -> SGI_base for this CPU */
@@ -231,6 +236,12 @@ static void gic_poke_irq(struct irq_data *d, u32 offset)
 static void gic_mask_irq(struct irq_data *d)
 {
 	gic_poke_irq(d, GICD_ICENABLER);
+	/*
+	 * When masking a forwarded interrupt, make sure it is
+	 * deactivated as well.
+	 */
+	if (static_key_true(&supports_deactivate) && forwarded_irq(d))
+		gic_poke_irq(d, GICD_ICACTIVER);
 }
 
 static void gic_unmask_irq(struct irq_data *d)
@@ -296,8 +307,11 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
 static void gic_eoi_irq(struct irq_data *d)
 {
 	if (static_key_true(&supports_deactivate)) {
-		/* No need to deactivate an LPI */
-		if (gic_irq(d) >= 8192)
+		/*
+		 * No need to deactivate an LPI, or an interrupt that
+		 * is is getting forwarded to a vcpu.
+		 */
+		if (gic_irq(d) >= 8192 || forwarded_irq(d))
 			return;
 		gic_write_dir(gic_irq(d));
 	} else {
@@ -331,6 +345,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 	return gic_configure_irq(irq, type, base, rwp_wait);
 }
 
+static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
+{
+	if (static_key_true(&supports_deactivate)) {
+		d->handler_data = vcpu;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static u64 gic_mpidr_to_affinity(u64 mpidr)
 {
 	u64 aff;
@@ -678,6 +702,7 @@ static struct irq_chip gic_chip = {
 	.irq_set_affinity	= gic_set_affinity,
 	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
 	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
+	.irq_set_vcpu_affinity	= gic_irq_set_vcpu_affinity,
 	.flags			= IRQCHIP_SET_TYPE_MASKED,
 };
 
-- 
2.1.4

[PATCH 5/6] irqchip: GICv3: Don't deactivate interrupts forwarded to a guest

[Eric Auger]

On 07/09/2015 03:19 PM, Marc Zyngier wrote:
> Commit 0a4377de3056 ("genirq: Introduce irq_set_vcpu_affinity() to
> target an interrupt to a VCPU") added just what we needed at the
> lowest level to allow an interrupt to be deactivated by a guest.
> 
> When such a request reaches the GIC, it knows it doesn't need to
> perform the deactivation anymore, and can safely leave the guest
> do its magic. This of course requires additional support in both
> VFIO and KVM.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/irq-gic-v3.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index e02592b..a1ca9e6 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -70,6 +70,11 @@ static inline int gic_irq_in_rdist(struct irq_data *d)
>  	return gic_irq(d) < 32;
>  }
>  
> +static inline bool forwarded_irq(struct irq_data *d)
> +{
> +	return d->handler_data != NULL;
> +}
> +
>  static inline void __iomem *gic_dist_base(struct irq_data *d)
>  {
>  	if (gic_irq_in_rdist(d))	/* SGI+PPI -> SGI_base for this CPU */
> @@ -231,6 +236,12 @@ static void gic_poke_irq(struct irq_data *d, u32 offset)
>  static void gic_mask_irq(struct irq_data *d)
>  {
>  	gic_poke_irq(d, GICD_ICENABLER);
> +	/*
> +	 * When masking a forwarded interrupt, make sure it is
> +	 * deactivated as well.
To me it is not straightforward to understand why a forwarded IRQ would
need to be DIR'ed when masked. This is needed because of the disable_irq
optimisation, I would add a related comment.

Eric
> +	 */
> +	if (static_key_true(&supports_deactivate) && forwarded_irq(d))
> +		gic_poke_irq(d, GICD_ICACTIVER);
>  }
>  
>  static void gic_unmask_irq(struct irq_data *d)
> @@ -296,8 +307,11 @@ static int gic_irq_get_irqchip_state(struct irq_data *d,
>  static void gic_eoi_irq(struct irq_data *d)
>  {
>  	if (static_key_true(&supports_deactivate)) {
> -		/* No need to deactivate an LPI */
> -		if (gic_irq(d) >= 8192)
> +		/*
> +		 * No need to deactivate an LPI, or an interrupt that
> +		 * is is getting forwarded to a vcpu.
> +		 */
> +		if (gic_irq(d) >= 8192 || forwarded_irq(d))
>  			return;
>  		gic_write_dir(gic_irq(d));
>  	} else {
> @@ -331,6 +345,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  	return gic_configure_irq(irq, type, base, rwp_wait);
>  }
>  
> +static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
> +{
> +	if (static_key_true(&supports_deactivate)) {
> +		d->handler_data = vcpu;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static u64 gic_mpidr_to_affinity(u64 mpidr)
>  {
>  	u64 aff;
> @@ -678,6 +702,7 @@ static struct irq_chip gic_chip = {
>  	.irq_set_affinity	= gic_set_affinity,
>  	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
>  	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
> +	.irq_set_vcpu_affinity	= gic_irq_set_vcpu_affinity,
>  	.flags			= IRQCHIP_SET_TYPE_MASKED,
>  };
>  
> 

[PATCH 5/6] irqchip: GICv3: Don't deactivate interrupts forwarded to a guest

[Marc Zyngier]

On 11/08/15 11:03, Eric Auger wrote:
> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>> Commit 0a4377de3056 ("genirq: Introduce irq_set_vcpu_affinity() to
>> target an interrupt to a VCPU") added just what we needed at the
>> lowest level to allow an interrupt to be deactivated by a guest.
>>
>> When such a request reaches the GIC, it knows it doesn't need to
>> perform the deactivation anymore, and can safely leave the guest
>> do its magic. This of course requires additional support in both
>> VFIO and KVM.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/irqchip/irq-gic-v3.c | 29 +++++++++++++++++++++++++++--
>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index e02592b..a1ca9e6 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -70,6 +70,11 @@ static inline int gic_irq_in_rdist(struct irq_data *d)
>>  	return gic_irq(d) < 32;
>>  }
>>  
>> +static inline bool forwarded_irq(struct irq_data *d)
>> +{
>> +	return d->handler_data != NULL;
>> +}
>> +
>>  static inline void __iomem *gic_dist_base(struct irq_data *d)
>>  {
>>  	if (gic_irq_in_rdist(d))	/* SGI+PPI -> SGI_base for this CPU */
>> @@ -231,6 +236,12 @@ static void gic_poke_irq(struct irq_data *d, u32 offset)
>>  static void gic_mask_irq(struct irq_data *d)
>>  {
>>  	gic_poke_irq(d, GICD_ICENABLER);
>> +	/*
>> +	 * When masking a forwarded interrupt, make sure it is
>> +	 * deactivated as well.
> To me it is not straightforward to understand why a forwarded IRQ would
> need to be DIR'ed when masked. This is needed because of the disable_irq
> optimisation, I would add a related comment.
> 

The lazy disable_irq is just an optimization.

The real reason is that if we mask an interrupt on the host, it is
because we don't want the guest to process it at all. There is three cases:

1) The interrupt was inactive: no problem
2) The interrupt was active, but not presented to the guest yet: no
problem either. The interrupt will be taken again on unmask.
3) The interrupt was active and presented to the guest: we might get a
double deactivate, which shouldn't be a big deal (but mostly should not
occur at all).

Would something like this make sense?

On a related note, I wonder if we need to mark the interrupt pending if
it is configured as edge. Otherwise, we could loose an interrupt in case
2 (mask, deactivate, unmask, oh look nothing triggers). Thoughts?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/6] irqchip: GICv3: Don't deactivate interrupts forwarded to a guest

[Eric Auger]

On 08/12/2015 04:20 PM, Marc Zyngier wrote:
> On 11/08/15 11:03, Eric Auger wrote:
>> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>>> Commit 0a4377de3056 ("genirq: Introduce irq_set_vcpu_affinity() to
>>> target an interrupt to a VCPU") added just what we needed at the
>>> lowest level to allow an interrupt to be deactivated by a guest.
>>>
>>> When such a request reaches the GIC, it knows it doesn't need to
>>> perform the deactivation anymore, and can safely leave the guest
>>> do its magic. This of course requires additional support in both
>>> VFIO and KVM.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  drivers/irqchip/irq-gic-v3.c | 29 +++++++++++++++++++++++++++--
>>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index e02592b..a1ca9e6 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -70,6 +70,11 @@ static inline int gic_irq_in_rdist(struct irq_data *d)
>>>  	return gic_irq(d) < 32;
>>>  }
>>>  
>>> +static inline bool forwarded_irq(struct irq_data *d)
>>> +{
>>> +	return d->handler_data != NULL;
>>> +}
>>> +
>>>  static inline void __iomem *gic_dist_base(struct irq_data *d)
>>>  {
>>>  	if (gic_irq_in_rdist(d))	/* SGI+PPI -> SGI_base for this CPU */
>>> @@ -231,6 +236,12 @@ static void gic_poke_irq(struct irq_data *d, u32 offset)
>>>  static void gic_mask_irq(struct irq_data *d)
>>>  {
>>>  	gic_poke_irq(d, GICD_ICENABLER);
>>> +	/*
>>> +	 * When masking a forwarded interrupt, make sure it is
>>> +	 * deactivated as well.
>> To me it is not straightforward to understand why a forwarded IRQ would
>> need to be DIR'ed when masked. This is needed because of the disable_irq
>> optimisation, I would add a related comment.
>>
> 
> The lazy disable_irq is just an optimization.
yes that's true but it causes a real problem here since although you
disabled the IRQ, a new one can show up, we do not call the actual
handler (that was supposed to forward it to the guest) but still you
expect the guest to complete it. Practically that's why the host must
take in charge the deactivation in that case, and this happens during
the masking with this implementation.
> 
> The real reason is that if we mask an interrupt on the host, it is
> because we don't want the guest to process it at all. There is three cases:
> 
> 1) The interrupt was inactive: no problem
> 2) The interrupt was active, but not presented to the guest yet: no
> problem either. The interrupt will be taken again on unmask.
> 3) The interrupt was active and presented to the guest: we might get a
> double deactivate, which shouldn't be a big deal (but mostly should not
> occur at all).
> 
> Would something like this make sense?
yes makes sense. The only thing that scares me a bit is 3: when
masking/DIR an edge irq (#n) we can have the same new physical IRQ
showing up when unmasking (#n+1); when guest deactivates the #nth
virtual IRQ it is going to DIR #n+1 physical IRQ.

Also with VGIC state machine, we must be attention not to inject the
second forwarded edge irq while there is one programmed in the LR. We
said "it comes from the HW so it must be true"? Not safe anymore here ...

> 
> On a related note, I wonder if we need to mark the interrupt pending if
> it is configured as edge. Otherwise, we could loose an interrupt in case
> 2 (mask, deactivate, unmask, oh look nothing triggers). Thoughts?
Yes I think this makes sense indeed. Definitively this one will be lost.

Eric
> 
> Thanks,
> 
> 	M.
> 

[PATCH 5/6] irqchip: GICv3: Don't deactivate interrupts forwarded to a guest

[Marc Zyngier]

On 12/08/15 16:09, Eric Auger wrote:
> On 08/12/2015 04:20 PM, Marc Zyngier wrote:
>> On 11/08/15 11:03, Eric Auger wrote:
>>> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>>>> Commit 0a4377de3056 ("genirq: Introduce irq_set_vcpu_affinity() to
>>>> target an interrupt to a VCPU") added just what we needed at the
>>>> lowest level to allow an interrupt to be deactivated by a guest.
>>>>
>>>> When such a request reaches the GIC, it knows it doesn't need to
>>>> perform the deactivation anymore, and can safely leave the guest
>>>> do its magic. This of course requires additional support in both
>>>> VFIO and KVM.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  drivers/irqchip/irq-gic-v3.c | 29 +++++++++++++++++++++++++++--
>>>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>> index e02592b..a1ca9e6 100644
>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>> @@ -70,6 +70,11 @@ static inline int gic_irq_in_rdist(struct irq_data *d)
>>>>  	return gic_irq(d) < 32;
>>>>  }
>>>>  
>>>> +static inline bool forwarded_irq(struct irq_data *d)
>>>> +{
>>>> +	return d->handler_data != NULL;
>>>> +}
>>>> +
>>>>  static inline void __iomem *gic_dist_base(struct irq_data *d)
>>>>  {
>>>>  	if (gic_irq_in_rdist(d))	/* SGI+PPI -> SGI_base for this CPU */
>>>> @@ -231,6 +236,12 @@ static void gic_poke_irq(struct irq_data *d, u32 offset)
>>>>  static void gic_mask_irq(struct irq_data *d)
>>>>  {
>>>>  	gic_poke_irq(d, GICD_ICENABLER);
>>>> +	/*
>>>> +	 * When masking a forwarded interrupt, make sure it is
>>>> +	 * deactivated as well.
>>> To me it is not straightforward to understand why a forwarded IRQ would
>>> need to be DIR'ed when masked. This is needed because of the disable_irq
>>> optimisation, I would add a related comment.
>>>
>>
>> The lazy disable_irq is just an optimization.
> yes that's true but it causes a real problem here since although you
> disabled the IRQ, a new one can show up, we do not call the actual
> handler (that was supposed to forward it to the guest) but still you
> expect the guest to complete it. Practically that's why the host must
> take in charge the deactivation in that case, and this happens during
> the masking with this implementation.

Yeah, I see what you mean. If we end-up here with an active interrupt,
that's because the lazy interrupt masking has been used, and we need to
fix up things.

>>
>> The real reason is that if we mask an interrupt on the host, it is
>> because we don't want the guest to process it at all. There is three cases:
>>
>> 1) The interrupt was inactive: no problem
>> 2) The interrupt was active, but not presented to the guest yet: no
>> problem either. The interrupt will be taken again on unmask.
>> 3) The interrupt was active and presented to the guest: we might get a
>> double deactivate, which shouldn't be a big deal (but mostly should not
>> occur at all).
>>
>> Would something like this make sense?
> yes makes sense. The only thing that scares me a bit is 3: when
> masking/DIR an edge irq (#n) we can have the same new physical IRQ
> showing up when unmasking (#n+1); when guest deactivates the #nth
> virtual IRQ it is going to DIR #n+1 physical IRQ.

That bit is not worrying me too much for a few reasons reasons:
- You normally don't mask a forwarded interrupt. You only do that on
specific events like guest termination. At that point, it doesn't matter
anymore.
- Edge interrupts can always be coalesced. So getting one event instead
of two is not a problem.
- Deactivation (specially on EOI from a guest) is not "refcounted". It
just clears the active bit.

> Also with VGIC state machine, we must be attention not to inject the
> second forwarded edge irq while there is one programmed in the LR. We
> said "it comes from the HW so it must be true"? Not safe anymore here ...

I don't believe this is a problem. You should never end-up masking the
interrupt if you don't intend to tear it down (this is why we have the
active bit - to avoid masking thing in normal operations).

> 
>>
>> On a related note, I wonder if we need to mark the interrupt pending if
>> it is configured as edge. Otherwise, we could loose an interrupt in case
>> 2 (mask, deactivate, unmask, oh look nothing triggers). Thoughts?
> Yes I think this makes sense indeed. Definitively this one will be lost.

Depends. If we are to restore a working interrupt flow, then we need it.
If we just mask to allow an interrupt to be "unforwarded", then we do
not have to care.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/6] irqchip: GICv3: Don't deactivate interrupts forwarded to a guest

[Eric Auger]

On 08/12/2015 05:40 PM, Marc Zyngier wrote:
> On 12/08/15 16:09, Eric Auger wrote:
>> On 08/12/2015 04:20 PM, Marc Zyngier wrote:
>>> On 11/08/15 11:03, Eric Auger wrote:
>>>> On 07/09/2015 03:19 PM, Marc Zyngier wrote:
>>>>> Commit 0a4377de3056 ("genirq: Introduce irq_set_vcpu_affinity() to
>>>>> target an interrupt to a VCPU") added just what we needed at the
>>>>> lowest level to allow an interrupt to be deactivated by a guest.
>>>>>
>>>>> When such a request reaches the GIC, it knows it doesn't need to
>>>>> perform the deactivation anymore, and can safely leave the guest
>>>>> do its magic. This of course requires additional support in both
>>>>> VFIO and KVM.
>>>>>
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> ---
>>>>>  drivers/irqchip/irq-gic-v3.c | 29 +++++++++++++++++++++++++++--
>>>>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>>> index e02592b..a1ca9e6 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>>> @@ -70,6 +70,11 @@ static inline int gic_irq_in_rdist(struct irq_data *d)
>>>>>  	return gic_irq(d) < 32;
>>>>>  }
>>>>>  
>>>>> +static inline bool forwarded_irq(struct irq_data *d)
>>>>> +{
>>>>> +	return d->handler_data != NULL;
>>>>> +}
>>>>> +
>>>>>  static inline void __iomem *gic_dist_base(struct irq_data *d)
>>>>>  {
>>>>>  	if (gic_irq_in_rdist(d))	/* SGI+PPI -> SGI_base for this CPU */
>>>>> @@ -231,6 +236,12 @@ static void gic_poke_irq(struct irq_data *d, u32 offset)
>>>>>  static void gic_mask_irq(struct irq_data *d)
>>>>>  {
>>>>>  	gic_poke_irq(d, GICD_ICENABLER);
>>>>> +	/*
>>>>> +	 * When masking a forwarded interrupt, make sure it is
>>>>> +	 * deactivated as well.
>>>> To me it is not straightforward to understand why a forwarded IRQ would
>>>> need to be DIR'ed when masked. This is needed because of the disable_irq
>>>> optimisation, I would add a related comment.
>>>>
>>>
>>> The lazy disable_irq is just an optimization.
>> yes that's true but it causes a real problem here since although you
>> disabled the IRQ, a new one can show up, we do not call the actual
>> handler (that was supposed to forward it to the guest) but still you
>> expect the guest to complete it. Practically that's why the host must
>> take in charge the deactivation in that case, and this happens during
>> the masking with this implementation.
> 
> Yeah, I see what you mean. If we end-up here with an active interrupt,
> that's because the lazy interrupt masking has been used, and we need to
> fix up things.
> 
>>>
>>> The real reason is that if we mask an interrupt on the host, it is
>>> because we don't want the guest to process it at all. There is three cases:
>>>
>>> 1) The interrupt was inactive: no problem
>>> 2) The interrupt was active, but not presented to the guest yet: no
>>> problem either. The interrupt will be taken again on unmask.
>>> 3) The interrupt was active and presented to the guest: we might get a
>>> double deactivate, which shouldn't be a big deal (but mostly should not
>>> occur at all).
>>>
>>> Would something like this make sense?
>> yes makes sense. The only thing that scares me a bit is 3: when
>> masking/DIR an edge irq (#n) we can have the same new physical IRQ
>> showing up when unmasking (#n+1); when guest deactivates the #nth
>> virtual IRQ it is going to DIR #n+1 physical IRQ.
> 
> That bit is not worrying me too much for a few reasons reasons:
> - You normally don't mask a forwarded interrupt. You only do that on
> specific events like guest termination. At that point, it doesn't matter
> anymore.
> - Edge interrupts can always be coalesced. So getting one event instead
> of two is not a problem.
> - Deactivation (specially on EOI from a guest) is not "refcounted". It
> just clears the active bit.
> 
>> Also with VGIC state machine, we must be attention not to inject the
>> second forwarded edge irq while there is one programmed in the LR. We
>> said "it comes from the HW so it must be true"? Not safe anymore here ...
> 
> I don't believe this is a problem. You should never end-up masking the
> interrupt if you don't intend to tear it down (this is why we have the
> active bit - to avoid masking thing in normal operations).

OK
> 
>>
>>>
>>> On a related note, I wonder if we need to mark the interrupt pending if
>>> it is configured as edge. Otherwise, we could loose an interrupt in case
>>> 2 (mask, deactivate, unmask, oh look nothing triggers). Thoughts?
>> Yes I think this makes sense indeed. Definitively this one will be lost.
> 
> Depends. If we are to restore a working interrupt flow, then we need it.
> If we just mask to allow an interrupt to be "unforwarded", then we do
> not have to care.

yes. I am currently focused on unforwarding and effectively that works
fine since, as you explained before, I am tearing down the system.

Best Regards

Eric
> 
> Thanks,
> 
> 	M.
> 

[PATCH 6/6] irqchip: GIC: Don't deactivate interrupts forwarded to a guest

[Marc Zyngier]

Commit 0a4377de3056 ("genirq: Introduce irq_set_vcpu_affinity() to
target an interrupt to a VCPU") added just what we needed at the
lowest level to allow an interrupt to be deactivated by a guest.

When such a request reaches the GIC, it knows it doesn't need to
perform the deactivation anymore, and can safely leave the guest
do its magic. This of course requires additional support in both
VFIO and KVM.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 3c7f3a4..ca4074a 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -140,6 +140,11 @@ static inline unsigned int gic_irq(struct irq_data *d)
 	return d->hwirq;
 }
 
+static inline bool forwarded_irq(struct irq_data *d)
+{
+	return d->handler_data != NULL;
+}
+
 /*
  * Routines to acknowledge, disable and enable interrupts
  */
@@ -158,6 +163,12 @@ static int gic_peek_irq(struct irq_data *d, u32 offset)
 static void gic_mask_irq(struct irq_data *d)
 {
 	gic_poke_irq(d, GIC_DIST_ENABLE_CLEAR);
+	/*
+	 * When masking a forwarded interrupt, make sure it is
+	 * deactivated as well.
+	 */
+	if (static_key_true(&supports_deactivate) && forwarded_irq(d))
+		gic_poke_irq(d, GIC_DIST_ACTIVE_CLEAR);
 }
 
 static void gic_unmask_irq(struct irq_data *d)
@@ -167,10 +178,14 @@ static void gic_unmask_irq(struct irq_data *d)
 
 static void gic_eoi_irq(struct irq_data *d)
 {
-	if (static_key_true(&supports_deactivate))
+	if (static_key_true(&supports_deactivate)) {
+		/* Do not deactivate an IRQ forwarded to a vcpu. */
+		if (forwarded_irq(d))
+			return;
 		writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
-	else
+	} else {
 		writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
+	}
 }
 
 static int gic_irq_set_irqchip_state(struct irq_data *d,
@@ -239,6 +254,18 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 	return gic_configure_irq(gicirq, type, base, NULL);
 }
 
+static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
+{
+	/* Only interrupts on the primary GIC can be forwarded to a vcpu. */
+	if (static_key_true(&supports_deactivate) &&
+	    irq_data_get_irq_chip_data(d) == &gic_data[0]) {
+		d->handler_data = vcpu;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 #ifdef CONFIG_SMP
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 			    bool force)
@@ -334,6 +361,7 @@ static struct irq_chip gic_chip = {
 #endif
 	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
 	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
+	.irq_set_vcpu_affinity	= gic_irq_set_vcpu_affinity,
 	.flags			= IRQCHIP_SET_TYPE_MASKED,
 };
 
-- 
2.1.4

[PATCH 0/6] irqchip: GICv2/v3: Add support for irq_vcpu_affinity

[Eric Auger]

Hi Marc,

The series works fine with KVM platform device passthrough use case
(Calxeda Midway, GICv2, SPI assignment with VFIO). My integration branch
can be found at
https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.2-rc6-irq-forward-v3
if useful.

Tested-by: Eric Auger <eric.auger@linaro.org>

Best Regards

Eric
On 07/09/2015 03:19 PM, Marc Zyngier wrote:
> The GICv2 and GICv3 architectures allow an active physical interrupt
> to be forwarded to a guest, and the guest to indirectly perform the
> deactivation of the interrupt by performing an EOI on the virtual
> interrupt (see for example the GICv2 spec, 3.2.1).
> 
> This allows some substantial performance improvement for level
> triggered interrupts that otherwise have to be masked/unmasked in
> VFIO, not to mention the required trap back to KVM when the guest
> performs an EOI.
> 
> To enable this, the GICs need to be switched to a different EOImode,
> where a taken interrupt can be left "active" (which prevents the same
> interrupt from being taken again), while other interrupts are still
> being processed normally.
> 
> We also use the new irq_set_vcpu_affinity hook that was introduced for
> Intel's "Posted Interrupts" to determine whether or not to perform the
> deactivation at EOI-time.
> 
> As all of this only makes sense when the kernel can behave as a
> hypervisor, we only enable this mode on detecting that the kernel was
> actually booted in HYP mode, and that the GIC supports this feature.
> 
> This series is a complete rework of a RFC I sent over a year ago:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266328.html
> 
> Since then, a lot has been either merged (the irqchip_state) or reworked
> (my active-timer series: http://www.spinics.net/lists/kvm/msg118768.html),
> and this implements the last few bits for Eric Auger's series to
> finally make it into the kernel:
> 
> https://lkml.org/lkml/2015/7/2/268
> https://lkml.org/lkml/2015/7/6/291
> 
> With all these patches combined, physical interrupt routing from the
> kernel into a VM becomes possible.
> 
> This has been tested on Juno (GICv2) and FastModel (GICv3). A branch
> is available at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/gic-irq-vcpu-affinity
> 
> Marc Zyngier (6):
>   irqchip: GICv3: Convert to EOImode == 1
>   irqchip: GIC: Convert to EOImode == 1
>   irqchip: GICv3: Skip LPI deactivation
>   irqchip: GIC: Use chip_data instead of handler_data for cascaded
>     interrupts
>   irqchip: GICv3: Don't deactivate interrupts forwarded to a guest
>   irqchip: GIC: Don't deactivate interrupts forwarded to a guest
> 
>  drivers/irqchip/irq-gic-v3.c       | 57 +++++++++++++++++++++++++++++++--
>  drivers/irqchip/irq-gic.c          | 64 +++++++++++++++++++++++++++++++++++---
>  include/linux/irqchip/arm-gic-v3.h |  9 ++++++
>  include/linux/irqchip/arm-gic.h    |  4 +++
>  4 files changed, 126 insertions(+), 8 deletions(-)
> 

[PATCH 0/6] irqchip: GICv2/v3: Add support for irq_vcpu_affinity

[Marc Zyngier]

On 11/08/15 11:06, Eric Auger wrote:
> Hi Marc,
> 
> The series works fine with KVM platform device passthrough use case
> (Calxeda Midway, GICv2, SPI assignment with VFIO). My integration branch
> can be found at
> https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.2-rc6-irq-forward-v3
> if useful.
> 
> Tested-by: Eric Auger <eric.auger@linaro.org>

Many thanks for trying it (and for the review). I'll post an updated
version soon.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...