[PATCH v3 00/16] arm/arm64: Turning IPIs into normal interrupts

[PATCH v3 00/16] arm/arm64: Turning IPIs into normal interrupts

[Marc Zyngier]

For as long as SMP ARM has existed, IPIs have been handled as
something special. The arch code and the interrupt controller exchange
a couple of hooks (one to generate an IPI, another to handle it).

Although this is perfectly manageable, it prevents the use of features
that we could use if IPIs were Linux IRQs (such as pseudo-NMIs). It
also means that each interrupt controller driver has to follow an
architecture-specific interface instead of just implementing the base
irqchip functionalities. The arch code also duplicates a number of
things that the core irq code already does (such as calling
set_irq_regs(), irq_enter()...).

This series tries to remedy this on arm/arm64 by offering a new
registration interface where the irqchip gives the arch code a range
of interrupts to use for IPIs. The arch code requests these as normal
per-cpu interrupts.

The bulk of the work is at the interrupt controller level, where all 5
irqchips used on arm+SMP/arm64 get converted.

Finally, we drop the legacy registration interface as well as the
custom statistics accounting.

Note that I have had a look at providing a "generic" interface by
expanding the kernel/irq/ipi.c bag of helpers, but so far all
irqchips have very different requirements, so there is hardly anything
to consolidate for now. Maybe some as hip04 and the Marvell horror get
cleaned up (the latter certainly could do with a good dusting).

This has been tested on a bunch of 32 and 64bit guests (GICv2, GICv3),
as well as 64bit bare metal (GICv3). The RPi part has only been tested
in QEMU as a 64bit guest, while the HiSi and Marvell parts have only
been compile-tested.

I'm aiming for 5.10 for this, so any comment would be appreciated.

* From v2:
  - Tidied up the arch code (__read_mostly for the IPI handling data,
    WARN_ON_ONCE on setup and teardown), dropped spurious prototype
    on 32bit
  - Prevent IPIs from being forwarded to VCPUs
  - Merged the GIC affinity fix, hence one less patch
  - Added RBs from Valentin

* From v1:
  - Clarified the effect of nesting irq_enter/exit (Russell)
  - Changed the point where we tear IPIs down on (Valentin)
  - IPIs are no longer accessible from DT
  - HIP04 and Armada 370-XP have been converted, but are untested
  - arch-specific kstat accounting is removed
  - ARM's legacy interface is dropped

Marc Zyngier (16):
  genirq: Add fasteoi IPI flow
  genirq: Allow interrupts to be excluded from /proc/interrupts
  arm64: Allow IPIs to be handled as normal interrupts
  ARM: Allow IPIs to be handled as normal interrupts
  irqchip/gic-v3: Describe the SGI range
  irqchip/gic-v3: Configure SGIs as standard interrupts
  irqchip/gic: Refactor SMP configuration
  irqchip/gic: Configure SGIs as standard interrupts
  irqchip/gic-common: Don't enable SGIs by default
  irqchip/bcm2836: Configure mailbox interrupts as standard interrupts
  irqchip/hip04: Configure IPIs as standard interrupts
  irqchip/armada-370-xp: Configure IPIs as standard interrupts
  arm64: Kill __smp_cross_call and co
  arm64: Remove custom IRQ stat accounting
  ARM: Kill __smp_cross_call and co
  ARM: Remove custom IRQ stat accounting

 arch/arm/Kconfig                    |   1 +
 arch/arm/include/asm/hardirq.h      |  17 --
 arch/arm/include/asm/smp.h          |   5 +-
 arch/arm/kernel/smp.c               | 135 +++++++++-----
 arch/arm64/Kconfig                  |   1 +
 arch/arm64/include/asm/hardirq.h    |   9 -
 arch/arm64/include/asm/irq_work.h   |   4 +-
 arch/arm64/include/asm/smp.h        |   6 +-
 arch/arm64/kernel/smp.c             | 121 ++++++++-----
 drivers/irqchip/irq-armada-370-xp.c | 262 +++++++++++++++++++---------
 drivers/irqchip/irq-bcm2836.c       | 151 +++++++++++++---
 drivers/irqchip/irq-gic-common.c    |   3 -
 drivers/irqchip/irq-gic-v3.c        | 104 ++++++-----
 drivers/irqchip/irq-gic.c           | 177 +++++++++++--------
 drivers/irqchip/irq-hip04.c         |  89 +++++-----
 include/linux/irq.h                 |   5 +-
 kernel/irq/chip.c                   |  27 +++
 kernel/irq/debugfs.c                |   1 +
 kernel/irq/proc.c                   |   2 +-
 kernel/irq/settings.h               |   7 +
 20 files changed, 720 insertions(+), 407 deletions(-)

-- 
2.27.0

[PATCH v3 01/16] genirq: Add fasteoi IPI flow

[Marc Zyngier]

For irqchips using the fasteoi flow, IPIs are a bit special.
They need to be EOI'd early (before calling the handler), as
funny things may happen in the handler (they do not necessarily
behave like a normal interrupt).

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/linux/irq.h |  1 +
 kernel/irq/chip.c   | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 1b7f4dfee35b..57205bbf46bf 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -634,6 +634,7 @@ static inline int irq_set_parent(int irq, int parent_irq)
  */
 extern void handle_level_irq(struct irq_desc *desc);
 extern void handle_fasteoi_irq(struct irq_desc *desc);
+extern void handle_percpu_devid_fasteoi_ipi(struct irq_desc *desc);
 extern void handle_edge_irq(struct irq_desc *desc);
 extern void handle_edge_eoi_irq(struct irq_desc *desc);
 extern void handle_simple_irq(struct irq_desc *desc);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 857f5f4c8098..bed517d5aa4d 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -944,6 +944,33 @@ void handle_percpu_devid_irq(struct irq_desc *desc)
 		chip->irq_eoi(&desc->irq_data);
 }
 
+/**
+ * handle_percpu_devid_fasteoi_ipi - Per CPU local IPI handler with per cpu
+ *				     dev ids
+ * @desc:	the interrupt description structure for this irq
+ *
+ * The biggest difference with the IRQ version is that the interrupt is
+ * EOIed early, as the IPI could result in a context switch, and we need to
+ * make sure the IPI can fire again. We also assume that the arch code has
+ * registered an action. If not, we are positively doomed.
+ */
+void handle_percpu_devid_fasteoi_ipi(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct irqaction *action = desc->action;
+	unsigned int irq = irq_desc_get_irq(desc);
+	irqreturn_t res;
+
+	__kstat_incr_irqs_this_cpu(desc);
+
+	if (chip->irq_eoi)
+		chip->irq_eoi(&desc->irq_data);
+
+	trace_irq_handler_entry(irq, action);
+	res = action->handler(irq, raw_cpu_ptr(action->percpu_dev_id));
+	trace_irq_handler_exit(irq, action, res);
+}
+
 /**
  * handle_percpu_devid_fasteoi_nmi - Per CPU local NMI handler with per cpu
  *				     dev ids
-- 
2.27.0

[PATCH v3 02/16] genirq: Allow interrupts to be excluded from /proc/interrupts

[Marc Zyngier]

A number of architectures implement IPI statistics directly,
duplicating the core kstat_irqs accounting. As we move IPIs to
being actual IRQs, we would end-up with a confusing display
in /proc/interrupts (where the IPIs would appear twice).

In order to solve this, allow interrupts to be flagged as
"hidden", which excludes them from /proc/interrupts.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/linux/irq.h   | 4 +++-
 kernel/irq/debugfs.c  | 1 +
 kernel/irq/proc.c     | 2 +-
 kernel/irq/settings.h | 7 +++++++
 4 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 57205bbf46bf..63b9d962ee67 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -71,6 +71,7 @@ enum irqchip_irq_state;
  *				  it from the spurious interrupt detection
  *				  mechanism and from core side polling.
  * IRQ_DISABLE_UNLAZY		- Disable lazy irq disable
+ * IRQ_HIDDEN			- Don't show up in /proc/interrupts
  */
 enum {
 	IRQ_TYPE_NONE		= 0x00000000,
@@ -97,13 +98,14 @@ enum {
 	IRQ_PER_CPU_DEVID	= (1 << 17),
 	IRQ_IS_POLLED		= (1 << 18),
 	IRQ_DISABLE_UNLAZY	= (1 << 19),
+	IRQ_HIDDEN		= (1 << 20),
 };
 
 #define IRQF_MODIFY_MASK	\
 	(IRQ_TYPE_SENSE_MASK | IRQ_NOPROBE | IRQ_NOREQUEST | \
 	 IRQ_NOAUTOEN | IRQ_MOVE_PCNTXT | IRQ_LEVEL | IRQ_NO_BALANCING | \
 	 IRQ_PER_CPU | IRQ_NESTED_THREAD | IRQ_NOTHREAD | IRQ_PER_CPU_DEVID | \
-	 IRQ_IS_POLLED | IRQ_DISABLE_UNLAZY)
+	 IRQ_IS_POLLED | IRQ_DISABLE_UNLAZY | IRQ_HIDDEN)
 
 #define IRQ_NO_BALANCING_MASK	(IRQ_PER_CPU | IRQ_NO_BALANCING)
 
diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index b95ff5d5f4bd..acabc0c0e46b 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -136,6 +136,7 @@ static const struct irq_bit_descr irqdesc_states[] = {
 	BIT_MASK_DESCR(_IRQ_PER_CPU_DEVID),
 	BIT_MASK_DESCR(_IRQ_IS_POLLED),
 	BIT_MASK_DESCR(_IRQ_DISABLE_UNLAZY),
+	BIT_MASK_DESCR(_IRQ_HIDDEN),
 };
 
 static const struct irq_bit_descr irqdesc_istates[] = {
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 32c071d7bc03..72513ed2a5fc 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -485,7 +485,7 @@ int show_interrupts(struct seq_file *p, void *v)
 
 	rcu_read_lock();
 	desc = irq_to_desc(i);
-	if (!desc)
+	if (!desc || irq_settings_is_hidden(desc))
 		goto outsparse;
 
 	if (desc->kstat_irqs)
diff --git a/kernel/irq/settings.h b/kernel/irq/settings.h
index e43795cd2ccf..403378b9947b 100644
--- a/kernel/irq/settings.h
+++ b/kernel/irq/settings.h
@@ -17,6 +17,7 @@ enum {
 	_IRQ_PER_CPU_DEVID	= IRQ_PER_CPU_DEVID,
 	_IRQ_IS_POLLED		= IRQ_IS_POLLED,
 	_IRQ_DISABLE_UNLAZY	= IRQ_DISABLE_UNLAZY,
+	_IRQ_HIDDEN		= IRQ_HIDDEN,
 	_IRQF_MODIFY_MASK	= IRQF_MODIFY_MASK,
 };
 
@@ -31,6 +32,7 @@ enum {
 #define IRQ_PER_CPU_DEVID	GOT_YOU_MORON
 #define IRQ_IS_POLLED		GOT_YOU_MORON
 #define IRQ_DISABLE_UNLAZY	GOT_YOU_MORON
+#define IRQ_HIDDEN		GOT_YOU_MORON
 #undef IRQF_MODIFY_MASK
 #define IRQF_MODIFY_MASK	GOT_YOU_MORON
 
@@ -167,3 +169,8 @@ static inline void irq_settings_clr_disable_unlazy(struct irq_desc *desc)
 {
 	desc->status_use_accessors &= ~_IRQ_DISABLE_UNLAZY;
 }
+
+static inline bool irq_settings_is_hidden(struct irq_desc *desc)
+{
+	return desc->status_use_accessors & _IRQ_HIDDEN;
+}
-- 
2.27.0

[PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Marc Zyngier]

In order to deal with IPIs as normal interrupts, let's add
a new way to register them with the architecture code.

set_smp_ipi_range() takes a range of interrupts, and allows
the arch code to request them as if the were normal interrupts.
A standard handler is then called by the core IRQ code to deal
with the IPI.

This means that we don't need to call irq_enter/irq_exit, and
that we don't need to deal with set_irq_regs either. So let's
move the dispatcher into its own function, and leave handle_IPI()
as a compatibility function.

On the sending side, let's make use of ipi_send_mask, which
already exists for this purpose.

One of the major difference is that we end up, in some cases
(such as when performing IRQ time accounting on the scheduler
IPI), end up with nested irq_enter()/irq_exit() pairs.
Other than the (relatively small) overhead, there should be
no consequences to it (these pairs are designed to nest
correctly, and the accounting shouldn't be off).

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/Kconfig           |  1 +
 arch/arm64/include/asm/smp.h |  5 ++
 arch/arm64/kernel/smp.c      | 93 +++++++++++++++++++++++++++++++-----
 3 files changed, 87 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6d232837cbee..d0fdbe5fb32f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -106,6 +106,7 @@ config ARM64
 	select GENERIC_CPU_VULNERABILITIES
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_IDLE_POLL_SETUP
+	select GENERIC_IRQ_IPI
 	select GENERIC_IRQ_MULTI_HANDLER
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index 0eadbf933e35..57c5db15f6b7 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -78,6 +78,11 @@ extern void set_smp_cross_call(void (*)(const struct cpumask *, unsigned int));
 
 extern void (*__smp_cross_call)(const struct cpumask *, unsigned int);
 
+/*
+ * Register IPI interrupts with the arch SMP code
+ */
+extern void set_smp_ipi_range(int ipi_base, int nr_ipi);
+
 /*
  * Called from the secondary holding pen, this is the secondary CPU entry point.
  */
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 355ee9eed4dd..00c9db1b61b5 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -75,6 +75,13 @@ enum ipi_msg_type {
 	IPI_WAKEUP
 };
 
+static int ipi_irq_base __read_mostly;
+static int nr_ipi __read_mostly = NR_IPI;
+static struct irq_desc *ipi_desc[NR_IPI] __read_mostly;
+
+static void ipi_setup(int cpu);
+static void ipi_teardown(int cpu);
+
 #ifdef CONFIG_HOTPLUG_CPU
 static int op_cpu_kill(unsigned int cpu);
 #else
@@ -237,6 +244,8 @@ asmlinkage notrace void secondary_start_kernel(void)
 	 */
 	notify_cpu_starting(cpu);
 
+	ipi_setup(cpu);
+
 	store_cpu_topology(cpu);
 	numa_add_cpu(cpu);
 
@@ -302,6 +311,7 @@ int __cpu_disable(void)
 	 * and we must not schedule until we're ready to give up the cpu.
 	 */
 	set_cpu_online(cpu, false);
+	ipi_teardown(cpu);
 
 	/*
 	 * OK - migrate IRQs away from this CPU
@@ -890,10 +900,9 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
 /*
  * Main handler for inter-processor interrupts
  */
-void handle_IPI(int ipinr, struct pt_regs *regs)
+static void do_handle_IPI(int ipinr)
 {
 	unsigned int cpu = smp_processor_id();
-	struct pt_regs *old_regs = set_irq_regs(regs);
 
 	if ((unsigned)ipinr < NR_IPI) {
 		trace_ipi_entry_rcuidle(ipi_types[ipinr]);
@@ -906,21 +915,16 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 		break;
 
 	case IPI_CALL_FUNC:
-		irq_enter();
 		generic_smp_call_function_interrupt();
-		irq_exit();
 		break;
 
 	case IPI_CPU_STOP:
-		irq_enter();
 		local_cpu_stop();
-		irq_exit();
 		break;
 
 	case IPI_CPU_CRASH_STOP:
 		if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
-			irq_enter();
-			ipi_cpu_crash_stop(cpu, regs);
+			ipi_cpu_crash_stop(cpu, get_irq_regs());
 
 			unreachable();
 		}
@@ -928,17 +932,13 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 	case IPI_TIMER:
-		irq_enter();
 		tick_receive_broadcast();
-		irq_exit();
 		break;
 #endif
 
 #ifdef CONFIG_IRQ_WORK
 	case IPI_IRQ_WORK:
-		irq_enter();
 		irq_work_run();
-		irq_exit();
 		break;
 #endif
 
@@ -957,9 +957,78 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 
 	if ((unsigned)ipinr < NR_IPI)
 		trace_ipi_exit_rcuidle(ipi_types[ipinr]);
+}
+
+/* Legacy version, should go away once all irqchips have been converted */
+void handle_IPI(int ipinr, struct pt_regs *regs)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	irq_enter();
+	do_handle_IPI(ipinr);
+	irq_exit();
+
 	set_irq_regs(old_regs);
 }
 
+static irqreturn_t ipi_handler(int irq, void *data)
+{
+	do_handle_IPI(irq - ipi_irq_base);
+	return IRQ_HANDLED;
+}
+
+static void ipi_send(const struct cpumask *target, unsigned int ipi)
+{
+	__ipi_send_mask(ipi_desc[ipi], target);
+}
+
+static void ipi_setup(int cpu)
+{
+	int i;
+
+	if (!ipi_irq_base)
+		return;
+
+	for (i = 0; i < nr_ipi; i++)
+		enable_percpu_irq(ipi_irq_base + i, 0);
+}
+
+static void ipi_teardown(int cpu)
+{
+	int i;
+
+	if (!ipi_irq_base)
+		return;
+
+	for (i = 0; i < nr_ipi; i++)
+		disable_percpu_irq(ipi_irq_base + i);
+}
+
+void __init set_smp_ipi_range(int ipi_base, int n)
+{
+	int i;
+
+	WARN_ON(n < NR_IPI);
+	nr_ipi = min(n, NR_IPI);
+
+	for (i = 0; i < nr_ipi; i++) {
+		int err;
+
+		err = request_percpu_irq(ipi_base + i, ipi_handler,
+					 "IPI", &irq_stat);
+		WARN_ON(err);
+
+		ipi_desc[i] = irq_to_desc(ipi_base + i);
+		irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);
+	}
+
+	ipi_irq_base = ipi_base;
+	__smp_cross_call = ipi_send;
+
+	/* Setup the boot CPU immediately */
+	ipi_setup(smp_processor_id());
+}
+
 void smp_send_reschedule(int cpu)
 {
 	smp_cross_call(cpumask_of(cpu), IPI_RESCHEDULE);
-- 
2.27.0

[PATCH v3 04/16] ARM: Allow IPIs to be handled as normal interrupts

[Marc Zyngier]

In order to deal with IPIs as normal interrupts, let's add
a new way to register them with the architecture code.

set_smp_ipi_range() takes a range of interrupts, and allows
the arch code to request them as if the were normal interrupts.
A standard handler is then called by the core IRQ code to deal
with the IPI.

This means that we don't need to call irq_enter/irq_exit, and
that we don't need to deal with set_irq_regs either. So let's
move the dispatcher into its own function, and leave handle_IPI()
as a compatibility function.

On the sending side, let's make use of ipi_send_mask, which
already exists for this purpose.

One of the major difference is that we end up, in some cases
(such as when performing IRQ time accounting on the scheduler
IPI), end up with nested irq_enter()/irq_exit() pairs.
Other than the (relatively small) overhead, there should be
no consequences to it (these pairs are designed to nest
correctly, and the accounting shouldn't be off).

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm/Kconfig           |  1 +
 arch/arm/include/asm/smp.h |  5 ++
 arch/arm/kernel/smp.c      | 99 ++++++++++++++++++++++++++++++++------
 3 files changed, 89 insertions(+), 16 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index e00d94b16658..e67ef15c800f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -49,6 +49,7 @@ config ARM
 	select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
 	select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
 	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
+	select GENERIC_IRQ_IPI if SMP
 	select GENERIC_CPU_AUTOPROBE
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_IDLE_POLL_SETUP
diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index a91f21e3c5b5..0e29730295ca 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -45,6 +45,11 @@ extern void smp_init_cpus(void);
  */
 extern void set_smp_cross_call(void (*)(const struct cpumask *, unsigned int));
 
+/*
+ * Register IPI interrupts with the arch SMP code
+ */
+extern void set_smp_ipi_range(int ipi_base, int nr_ipi);
+
 /*
  * Called from platform specific assembly code, this is the
  * secondary CPU entry point.
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 5d9da61eff62..f21f78483353 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -69,14 +69,22 @@ enum ipi_msg_type {
 	 * CPU_BACKTRACE is special and not included in NR_IPI
 	 * or tracable with trace_ipi_*
 	 */
-	IPI_CPU_BACKTRACE,
+	IPI_CPU_BACKTRACE = NR_IPI,
 	/*
 	 * SGI8-15 can be reserved by secure firmware, and thus may
 	 * not be usable by the kernel. Please keep the above limited
 	 * to at most 8 entries.
 	 */
+	MAX_IPI
 };
 
+static int ipi_irq_base __read_mostly;
+static int nr_ipi __read_mostly = NR_IPI;
+static struct irq_desc *ipi_desc[MAX_IPI] __read_mostly;
+
+static void ipi_setup(int cpu);
+static void ipi_teardown(int cpu);
+
 static DECLARE_COMPLETION(cpu_running);
 
 static struct smp_operations smp_ops __ro_after_init;
@@ -247,6 +255,7 @@ int __cpu_disable(void)
 	 * and we must not schedule until we're ready to give up the cpu.
 	 */
 	set_cpu_online(cpu, false);
+	ipi_teardown(cpu);
 
 	/*
 	 * OK - migrate IRQs away from this CPU
@@ -422,6 +431,8 @@ asmlinkage void secondary_start_kernel(void)
 
 	notify_cpu_starting(cpu);
 
+	ipi_setup(cpu);
+
 	calibrate_delay();
 
 	smp_store_cpu_info(cpu);
@@ -627,10 +638,9 @@ asmlinkage void __exception_irq_entry do_IPI(int ipinr, struct pt_regs *regs)
 	handle_IPI(ipinr, regs);
 }
 
-void handle_IPI(int ipinr, struct pt_regs *regs)
+static void do_handle_IPI(int ipinr)
 {
 	unsigned int cpu = smp_processor_id();
-	struct pt_regs *old_regs = set_irq_regs(regs);
 
 	if ((unsigned)ipinr < NR_IPI) {
 		trace_ipi_entry_rcuidle(ipi_types[ipinr]);
@@ -643,9 +653,7 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 	case IPI_TIMER:
-		irq_enter();
 		tick_receive_broadcast();
-		irq_exit();
 		break;
 #endif
 
@@ -654,36 +662,26 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 		break;
 
 	case IPI_CALL_FUNC:
-		irq_enter();
 		generic_smp_call_function_interrupt();
-		irq_exit();
 		break;
 
 	case IPI_CPU_STOP:
-		irq_enter();
 		ipi_cpu_stop(cpu);
-		irq_exit();
 		break;
 
 #ifdef CONFIG_IRQ_WORK
 	case IPI_IRQ_WORK:
-		irq_enter();
 		irq_work_run();
-		irq_exit();
 		break;
 #endif
 
 	case IPI_COMPLETION:
-		irq_enter();
 		ipi_complete(cpu);
-		irq_exit();
 		break;
 
 	case IPI_CPU_BACKTRACE:
 		printk_nmi_enter();
-		irq_enter();
-		nmi_cpu_backtrace(regs);
-		irq_exit();
+		nmi_cpu_backtrace(get_irq_regs());
 		printk_nmi_exit();
 		break;
 
@@ -695,9 +693,78 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 
 	if ((unsigned)ipinr < NR_IPI)
 		trace_ipi_exit_rcuidle(ipi_types[ipinr]);
+}
+
+/* Legacy version, should go away once all irqchips have been converted */
+void handle_IPI(int ipinr, struct pt_regs *regs)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	irq_enter();
+	do_handle_IPI(ipinr);
+	irq_exit();
+
 	set_irq_regs(old_regs);
 }
 
+static irqreturn_t ipi_handler(int irq, void *data)
+{
+	do_handle_IPI(irq - ipi_irq_base);
+	return IRQ_HANDLED;
+}
+
+static void ipi_send(const struct cpumask *target, unsigned int ipi)
+{
+	__ipi_send_mask(ipi_desc[ipi], target);
+}
+
+static void ipi_setup(int cpu)
+{
+	int i;
+
+	if (!ipi_irq_base)
+		return;
+
+	for (i = 0; i < nr_ipi; i++)
+		enable_percpu_irq(ipi_irq_base + i, 0);
+}
+
+static void ipi_teardown(int cpu)
+{
+	int i;
+
+	if (!ipi_irq_base)
+		return;
+
+	for (i = 0; i < nr_ipi; i++)
+		disable_percpu_irq(ipi_irq_base + i);
+}
+
+void __init set_smp_ipi_range(int ipi_base, int n)
+{
+	int i;
+
+	WARN_ON(n < MAX_IPI);
+	nr_ipi = min(n, MAX_IPI);
+
+	for (i = 0; i < nr_ipi; i++) {
+		int err;
+
+		err = request_percpu_irq(ipi_base + i, ipi_handler,
+					 "IPI", &irq_stat);
+		WARN_ON(err);
+
+		ipi_desc[i] = irq_to_desc(ipi_base + i);
+		irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);
+	}
+
+	ipi_irq_base = ipi_base;
+	set_smp_cross_call(ipi_send);
+
+	/* Setup the boot CPU immediately */
+	ipi_setup(smp_processor_id());
+}
+
 void smp_send_reschedule(int cpu)
 {
 	smp_cross_call(cpumask_of(cpu), IPI_RESCHEDULE);
-- 
2.27.0

[PATCH v3 05/16] irqchip/gic-v3: Describe the SGI range

[Marc Zyngier]

As we are about to start making use of SGIs in a more conventional
way, let's describe it is the GICv3 list of interrupt types.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 850842f27bee..f7c99a302d01 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -112,6 +112,7 @@ static DEFINE_PER_CPU(bool, has_rss);
 #define DEFAULT_PMR_VALUE	0xf0
 
 enum gic_intid_range {
+	SGI_RANGE,
 	PPI_RANGE,
 	SPI_RANGE,
 	EPPI_RANGE,
@@ -123,6 +124,8 @@ enum gic_intid_range {
 static enum gic_intid_range __get_intid_range(irq_hw_number_t hwirq)
 {
 	switch (hwirq) {
+	case 0 ... 15:
+		return SGI_RANGE;
 	case 16 ... 31:
 		return PPI_RANGE;
 	case 32 ... 1019:
@@ -148,15 +151,22 @@ static inline unsigned int gic_irq(struct irq_data *d)
 	return d->hwirq;
 }
 
-static inline int gic_irq_in_rdist(struct irq_data *d)
+static inline bool gic_irq_in_rdist(struct irq_data *d)
 {
-	enum gic_intid_range range = get_intid_range(d);
-	return range == PPI_RANGE || range == EPPI_RANGE;
+	switch (get_intid_range(d)) {
+	case SGI_RANGE:
+	case PPI_RANGE:
+	case EPPI_RANGE:
+		return true;
+	default:
+		return false;
+	}
 }
 
 static inline void __iomem *gic_dist_base(struct irq_data *d)
 {
 	switch (get_intid_range(d)) {
+	case SGI_RANGE:
 	case PPI_RANGE:
 	case EPPI_RANGE:
 		/* SGI+PPI -> SGI_base for this CPU */
@@ -253,6 +263,7 @@ static void gic_enable_redist(bool enable)
 static u32 convert_offset_index(struct irq_data *d, u32 offset, u32 *index)
 {
 	switch (get_intid_range(d)) {
+	case SGI_RANGE:
 	case PPI_RANGE:
 	case SPI_RANGE:
 		*index = d->hwirq;
@@ -1277,6 +1288,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 		chip = &gic_eoimode1_chip;
 
 	switch (__get_intid_range(hw)) {
+	case SGI_RANGE:
 	case PPI_RANGE:
 	case EPPI_RANGE:
 		irq_set_percpu_devid(irq);
-- 
2.27.0

[PATCH v3 06/16] irqchip/gic-v3: Configure SGIs as standard interrupts

[Marc Zyngier]

Change the way we deal with GICv3 SGIs by turning them into proper
IRQs, and calling into the arch code to register the interrupt range
instead of a callback.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3.c | 86 ++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 39 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index f7c99a302d01..717064588299 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -36,6 +36,8 @@
 #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996	(1ULL << 0)
 #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539	(1ULL << 1)
 
+#define GIC_IRQ_TYPE_PARTITION	(GIC_IRQ_TYPE_LPI + 1)
+
 struct redist_region {
 	void __iomem		*redist_base;
 	phys_addr_t		phys_base;
@@ -383,7 +385,7 @@ static int gic_irq_set_irqchip_state(struct irq_data *d,
 {
 	u32 reg;
 
-	if (d->hwirq >= 8192) /* PPI/SPI only */
+	if (d->hwirq >= 8192) /* SGI/PPI/SPI only */
 		return -EINVAL;
 
 	switch (which) {
@@ -583,6 +585,9 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 
 static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
 {
+	if (get_intid_range(d) == SGI_RANGE)
+		return -EINVAL;
+
 	if (vcpu)
 		irqd_set_forwarded_to_vcpu(d);
 	else
@@ -657,38 +662,14 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
 	if ((irqnr >= 1020 && irqnr <= 1023))
 		return;
 
-	/* Treat anything but SGIs in a uniform way */
-	if (likely(irqnr > 15)) {
-		int err;
-
-		if (static_branch_likely(&supports_deactivate_key))
-			gic_write_eoir(irqnr);
-		else
-			isb();
-
-		err = handle_domain_irq(gic_data.domain, irqnr, regs);
-		if (err) {
-			WARN_ONCE(true, "Unexpected interrupt received!\n");
-			gic_deactivate_unhandled(irqnr);
-		}
-		return;
-	}
-	if (irqnr < 16) {
+	if (static_branch_likely(&supports_deactivate_key))
 		gic_write_eoir(irqnr);
-		if (static_branch_likely(&supports_deactivate_key))
-			gic_write_dir(irqnr);
-#ifdef CONFIG_SMP
-		/*
-		 * Unlike GICv2, we don't need an smp_rmb() here.
-		 * The control dependency from gic_read_iar to
-		 * the ISB in gic_write_eoir is enough to ensure
-		 * that any shared data read by handle_IPI will
-		 * be read after the ACK.
-		 */
-		handle_IPI(irqnr, regs);
-#else
-		WARN_ONCE(true, "Unexpected SGI received!\n");
-#endif
+	else
+		isb();
+
+	if (handle_domain_irq(gic_data.domain, irqnr, regs)) {
+		WARN_ONCE(true, "Unexpected interrupt received!\n");
+		gic_deactivate_unhandled(irqnr);
 	}
 }
 
@@ -1136,11 +1117,11 @@ static void gic_send_sgi(u64 cluster_id, u16 tlist, unsigned int irq)
 	gic_write_sgi1r(val);
 }
 
-static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
+static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
 {
 	int cpu;
 
-	if (WARN_ON(irq >= 16))
+	if (WARN_ON(d->hwirq >= 16))
 		return;
 
 	/*
@@ -1154,7 +1135,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 		u16 tlist;
 
 		tlist = gic_compute_target_list(&cpu, mask, cluster_id);
-		gic_send_sgi(cluster_id, tlist, irq);
+		gic_send_sgi(cluster_id, tlist, d->hwirq);
 	}
 
 	/* Force the above writes to ICC_SGI1R_EL1 to be executed */
@@ -1163,10 +1144,24 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 
 static void __init gic_smp_init(void)
 {
-	set_smp_cross_call(gic_raise_softirq);
+	struct irq_fwspec sgi_fwspec = {
+		.fwnode		= gic_data.fwnode,
+		.param_count	= 1,
+	};
+	int base_sgi;
+
 	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
 				  "irqchip/arm/gicv3:starting",
 				  gic_starting_cpu, NULL);
+
+	/* Register all 8 non-secure SGIs */
+	base_sgi = __irq_domain_alloc_irqs(gic_data.domain, -1, 8,
+					   NUMA_NO_NODE, &sgi_fwspec,
+					   false, NULL);
+	if (WARN_ON(base_sgi <= 0))
+		return;
+
+	set_smp_ipi_range(base_sgi, 8);
 }
 
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
@@ -1215,6 +1210,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 }
 #else
 #define gic_set_affinity	NULL
+#define gic_ipi_send_mask	NULL
 #define gic_smp_init()		do { } while(0)
 #endif
 
@@ -1257,6 +1253,7 @@ static struct irq_chip gic_chip = {
 	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 	.irq_nmi_setup		= gic_irq_nmi_setup,
 	.irq_nmi_teardown	= gic_irq_nmi_teardown,
+	.ipi_send_mask		= gic_ipi_send_mask,
 	.flags			= IRQCHIP_SET_TYPE_MASKED |
 				  IRQCHIP_SKIP_SET_WAKE |
 				  IRQCHIP_MASK_ON_SUSPEND,
@@ -1274,6 +1271,7 @@ static struct irq_chip gic_eoimode1_chip = {
 	.irq_set_vcpu_affinity	= gic_irq_set_vcpu_affinity,
 	.irq_nmi_setup		= gic_irq_nmi_setup,
 	.irq_nmi_teardown	= gic_irq_nmi_teardown,
+	.ipi_send_mask		= gic_ipi_send_mask,
 	.flags			= IRQCHIP_SET_TYPE_MASKED |
 				  IRQCHIP_SKIP_SET_WAKE |
 				  IRQCHIP_MASK_ON_SUSPEND,
@@ -1289,6 +1287,12 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 
 	switch (__get_intid_range(hw)) {
 	case SGI_RANGE:
+		irq_set_percpu_devid(irq);
+		irq_domain_set_info(d, irq, hw, chip, d->host_data,
+				    handle_percpu_devid_fasteoi_ipi,
+				    NULL, NULL);
+		break;
+
 	case PPI_RANGE:
 	case EPPI_RANGE:
 		irq_set_percpu_devid(irq);
@@ -1318,13 +1322,17 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 	return 0;
 }
 
-#define GIC_IRQ_TYPE_PARTITION	(GIC_IRQ_TYPE_LPI + 1)
-
 static int gic_irq_domain_translate(struct irq_domain *d,
 				    struct irq_fwspec *fwspec,
 				    unsigned long *hwirq,
 				    unsigned int *type)
 {
+	if (fwspec->param_count == 1 && fwspec->param[0] < 16) {
+		*hwirq = fwspec->param[0];
+		*type = IRQ_TYPE_EDGE_RISING;
+		return 0;
+	}
+
 	if (is_of_node(fwspec->fwnode)) {
 		if (fwspec->param_count < 3)
 			return -EINVAL;
@@ -1656,9 +1664,9 @@ static int __init gic_init_bases(void __iomem *dist_base,
 
 	gic_update_rdist_properties();
 
-	gic_smp_init();
 	gic_dist_init();
 	gic_cpu_init();
+	gic_smp_init();
 	gic_cpu_pm_init();
 
 	if (gic_dist_supports_lpis()) {
-- 
2.27.0

[PATCH v3 07/16] irqchip/gic: Refactor SMP configuration

[Marc Zyngier]

As we are about to change quite a lot of the SMP support code,
let's start by moving it around so that it minimizes the amount
of #ifdefery.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic.c | 76 ++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 36 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a27ba2cc1dce..4ffd62af888f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -325,28 +325,6 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
 	return 0;
 }
 
-#ifdef CONFIG_SMP
-static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
-			    bool force)
-{
-	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d);
-	unsigned int cpu;
-
-	if (!force)
-		cpu = cpumask_any_and(mask_val, cpu_online_mask);
-	else
-		cpu = cpumask_first(mask_val);
-
-	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
-		return -EINVAL;
-
-	writeb_relaxed(gic_cpu_map[cpu], reg);
-	irq_data_update_effective_affinity(d, cpumask_of(cpu));
-
-	return IRQ_SET_MASK_OK_DONE;
-}
-#endif
-
 static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
 {
 	u32 irqstat, irqnr;
@@ -795,6 +773,26 @@ static int gic_pm_init(struct gic_chip_data *gic)
 #endif
 
 #ifdef CONFIG_SMP
+static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
+			    bool force)
+{
+	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d);
+	unsigned int cpu;
+
+	if (!force)
+		cpu = cpumask_any_and(mask_val, cpu_online_mask);
+	else
+		cpu = cpumask_first(mask_val);
+
+	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
+		return -EINVAL;
+
+	writeb_relaxed(gic_cpu_map[cpu], reg);
+	irq_data_update_effective_affinity(d, cpumask_of(cpu));
+
+	return IRQ_SET_MASK_OK_DONE;
+}
+
 static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 {
 	int cpu;
@@ -824,6 +822,23 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 
 	gic_unlock_irqrestore(flags);
 }
+
+static int gic_starting_cpu(unsigned int cpu)
+{
+	gic_cpu_init(&gic_data[0]);
+	return 0;
+}
+
+static __init void gic_smp_init(void)
+{
+	set_smp_cross_call(gic_raise_softirq);
+	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
+				  "irqchip/arm/gic:starting",
+				  gic_starting_cpu, NULL);
+}
+#else
+#define gic_smp_init()		do { } while(0)
+#define gic_set_affinity	NULL
 #endif
 
 #ifdef CONFIG_BL_SWITCHER
@@ -1027,12 +1042,6 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 	return -EINVAL;
 }
 
-static int gic_starting_cpu(unsigned int cpu)
-{
-	gic_cpu_init(&gic_data[0]);
-	return 0;
-}
-
 static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 				unsigned int nr_irqs, void *arg)
 {
@@ -1079,10 +1088,8 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev,
 		gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
 	}
 
-#ifdef CONFIG_SMP
 	if (gic == &gic_data[0])
 		gic->chip.irq_set_affinity = gic_set_affinity;
-#endif
 }
 
 static int gic_init_bases(struct gic_chip_data *gic,
@@ -1199,12 +1206,7 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
 		 */
 		for (i = 0; i < NR_GIC_CPU_IF; i++)
 			gic_cpu_map[i] = 0xff;
-#ifdef CONFIG_SMP
-		set_smp_cross_call(gic_raise_softirq);
-#endif
-		cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
-					  "irqchip/arm/gic:starting",
-					  gic_starting_cpu, NULL);
+
 		set_handle_irq(gic_handle_irq);
 		if (static_branch_likely(&supports_deactivate_key))
 			pr_info("GIC: Using split EOI/Deactivate mode\n");
@@ -1221,6 +1223,8 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,
 	ret = gic_init_bases(gic, handle);
 	if (ret)
 		kfree(name);
+	else if (gic == &gic_data[0])
+		gic_smp_init();
 
 	return ret;
 }
-- 
2.27.0

[PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Change the way we deal with GIC SGIs by turning them into proper
IRQs, and calling into the arch code to register the interrupt range
instead of a callback.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic.c | 103 +++++++++++++++++++++++---------------
 1 file changed, 63 insertions(+), 40 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4ffd62af888f..4be2b62f816f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -315,7 +315,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
 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 (cascading_gic_irq(d))
+	if (cascading_gic_irq(d) || gic_irq(d) < 16)
 		return -EINVAL;
 
 	if (vcpu)
@@ -335,31 +335,22 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
 		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
 		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
 
-		if (likely(irqnr > 15 && irqnr < 1020)) {
-			if (static_branch_likely(&supports_deactivate_key))
-				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
-			isb();
-			handle_domain_irq(gic->domain, irqnr, regs);
-			continue;
-		}
-		if (irqnr < 16) {
+		if (unlikely(irqnr >= 1020))
+			break;
+
+		if (static_branch_likely(&supports_deactivate_key))
 			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
-			if (static_branch_likely(&supports_deactivate_key))
-				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
-#ifdef CONFIG_SMP
-			/*
-			 * Ensure any shared data written by the CPU sending
-			 * the IPI is read after we've read the ACK register
-			 * on the GIC.
-			 *
-			 * Pairs with the write barrier in gic_raise_softirq
-			 */
+		isb();
+
+		/*
+		 * Ensure any shared data written by the CPU sending the IPI
+		 * is read after we've read the ACK register on the GIC.
+		 *
+		 * Pairs with the write barrier in gic_ipi_send_mask
+		 */
+		if (irqnr <= 15)
 			smp_rmb();
-			handle_IPI(irqnr, regs);
-#endif
-			continue;
-		}
-		break;
+		handle_domain_irq(gic->domain, irqnr, regs);
 	} while (1);
 }
 
@@ -793,14 +784,14 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 	return IRQ_SET_MASK_OK_DONE;
 }
 
-static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
+static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
 {
 	int cpu;
 	unsigned long flags, map = 0;
 
 	if (unlikely(nr_cpu_ids == 1)) {
 		/* Only one CPU? let's do a self-IPI... */
-		writel_relaxed(2 << 24 | irq,
+		writel_relaxed(2 << 24 | d->hwirq,
 			       gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 		return;
 	}
@@ -818,7 +809,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	dmb(ishst);
 
 	/* this always happens on GIC0 */
-	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
+	writel_relaxed(map << 16 | d->hwirq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 
 	gic_unlock_irqrestore(flags);
 }
@@ -831,14 +822,28 @@ static int gic_starting_cpu(unsigned int cpu)
 
 static __init void gic_smp_init(void)
 {
-	set_smp_cross_call(gic_raise_softirq);
+	struct irq_fwspec sgi_fwspec = {
+		.fwnode		= gic_data[0].domain->fwnode,
+		.param_count	= 1,
+	};
+	int base_sgi;
+
 	cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_GIC_STARTING,
 				  "irqchip/arm/gic:starting",
 				  gic_starting_cpu, NULL);
+
+	base_sgi = __irq_domain_alloc_irqs(gic_data[0].domain, -1, 8,
+					   NUMA_NO_NODE, &sgi_fwspec,
+					   false, NULL);
+	if (WARN_ON(base_sgi <= 0))
+		return;
+
+	set_smp_ipi_range(base_sgi, 8);
 }
 #else
 #define gic_smp_init()		do { } while(0)
 #define gic_set_affinity	NULL
+#define gic_ipi_send_mask	NULL
 #endif
 
 #ifdef CONFIG_BL_SWITCHER
@@ -985,15 +990,24 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 {
 	struct gic_chip_data *gic = d->host_data;
 
-	if (hw < 32) {
+	switch (hw) {
+	case 0 ... 15:
+		irq_set_percpu_devid(irq);
+		irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
+				    handle_percpu_devid_fasteoi_ipi,
+				    NULL, NULL);
+		break;
+	case 16 ... 31:
 		irq_set_percpu_devid(irq);
 		irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
 				    handle_percpu_devid_irq, NULL, NULL);
-	} else {
+		break;
+	default:
 		irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
 				    handle_fasteoi_irq, NULL, NULL);
 		irq_set_probe(irq);
 		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(irq)));
+		break;
 	}
 	return 0;
 }
@@ -1007,19 +1021,26 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 				    unsigned long *hwirq,
 				    unsigned int *type)
 {
+	if (fwspec->param_count == 1 && fwspec->param[0] < 16) {
+		*hwirq = fwspec->param[0];
+		*type = IRQ_TYPE_EDGE_RISING;
+		return 0;
+	}
+
 	if (is_of_node(fwspec->fwnode)) {
 		if (fwspec->param_count < 3)
 			return -EINVAL;
 
-		/* Get the interrupt number and add 16 to skip over SGIs */
-		*hwirq = fwspec->param[1] + 16;
-
-		/*
-		 * For SPIs, we need to add 16 more to get the GIC irq
-		 * ID number
-		 */
-		if (!fwspec->param[0])
-			*hwirq += 16;
+		switch (fwspec->param[0]) {
+		case 0:			/* SPI */
+			*hwirq = fwspec->param[1] + 32;
+			break;
+		case 1:			/* PPI */
+			*hwirq = fwspec->param[1] + 16;
+			break;
+		default:
+			return -EINVAL;
+		}
 
 		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
 
@@ -1088,8 +1109,10 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev,
 		gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
 	}
 
-	if (gic == &gic_data[0])
+	if (gic == &gic_data[0]) {
 		gic->chip.irq_set_affinity = gic_set_affinity;
+		gic->chip.ipi_send_mask = gic_ipi_send_mask;
+	}
 }
 
 static int gic_init_bases(struct gic_chip_data *gic,
-- 
2.27.0

[PATCH v3 09/16] irqchip/gic-common: Don't enable SGIs by default

[Marc Zyngier]

The architecture code now enables the IPIs as required, so no
need to enable SGIs by default in the GIC code.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-common.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index 82520006195d..f47b41dfd023 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -152,9 +152,6 @@ void gic_cpu_config(void __iomem *base, int nr, void (*sync_access)(void))
 		writel_relaxed(GICD_INT_DEF_PRI_X4,
 					base + GIC_DIST_PRI + i * 4 / 4);
 
-	/* Ensure all SGI interrupts are now enabled */
-	writel_relaxed(GICD_INT_EN_SET_SGI, base + GIC_DIST_ENABLE_SET);
-
 	if (sync_access)
 		sync_access();
 }
-- 
2.27.0

[PATCH v3 10/16] irqchip/bcm2836: Configure mailbox interrupts as standard interrupts

[Marc Zyngier]

In order to switch the bcm2836 driver to privide standard interrupts
for IPIs, it first needs to stop lying about the way things work.

The mailbox interrupt is actually a multiplexer, with enough
bits to store 32 pending interrupts per CPU. So let's turn it
into a chained irqchip.

Once this is done, we can instanciate the corresponding IPIs,
and pass them to the architecture code.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-bcm2836.c | 151 ++++++++++++++++++++++++++++------
 1 file changed, 125 insertions(+), 26 deletions(-)

diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
index 2038693f074c..85df6ddad9be 100644
--- a/drivers/irqchip/irq-bcm2836.c
+++ b/drivers/irqchip/irq-bcm2836.c
@@ -10,6 +10,7 @@
 #include <linux/of_irq.h>
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>
 #include <linux/irqchip/irq-bcm2836.h>
 
 #include <asm/exception.h>
@@ -89,12 +90,24 @@ static struct irq_chip bcm2836_arm_irqchip_gpu = {
 	.irq_unmask	= bcm2836_arm_irqchip_unmask_gpu_irq,
 };
 
+static void bcm2836_arm_irqchip_dummy_op(struct irq_data *d)
+{
+}
+
+static struct irq_chip bcm2836_arm_irqchip_dummy = {
+	.name		= "bcm2836-dummy",
+	.irq_eoi	= bcm2836_arm_irqchip_dummy_op,
+};
+
 static int bcm2836_map(struct irq_domain *d, unsigned int irq,
 		       irq_hw_number_t hw)
 {
 	struct irq_chip *chip;
 
 	switch (hw) {
+	case LOCAL_IRQ_MAILBOX0:
+		chip = &bcm2836_arm_irqchip_dummy;
+		break;
 	case LOCAL_IRQ_CNTPSIRQ:
 	case LOCAL_IRQ_CNTPNSIRQ:
 	case LOCAL_IRQ_CNTHPIRQ:
@@ -127,17 +140,7 @@ __exception_irq_entry bcm2836_arm_irqchip_handle_irq(struct pt_regs *regs)
 	u32 stat;
 
 	stat = readl_relaxed(intc.base + LOCAL_IRQ_PENDING0 + 4 * cpu);
-	if (stat & BIT(LOCAL_IRQ_MAILBOX0)) {
-#ifdef CONFIG_SMP
-		void __iomem *mailbox0 = (intc.base +
-					  LOCAL_MAILBOX0_CLR0 + 16 * cpu);
-		u32 mbox_val = readl(mailbox0);
-		u32 ipi = ffs(mbox_val) - 1;
-
-		writel(1 << ipi, mailbox0);
-		handle_IPI(ipi, regs);
-#endif
-	} else if (stat) {
+	if (stat) {
 		u32 hwirq = ffs(stat) - 1;
 
 		handle_domain_irq(intc.domain, hwirq, regs);
@@ -145,8 +148,35 @@ __exception_irq_entry bcm2836_arm_irqchip_handle_irq(struct pt_regs *regs)
 }
 
 #ifdef CONFIG_SMP
-static void bcm2836_arm_irqchip_send_ipi(const struct cpumask *mask,
-					 unsigned int ipi)
+static struct irq_domain *ipi_domain;
+
+static void bcm2836_arm_irqchip_handle_ipi(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int cpu = smp_processor_id();
+	u32 mbox_val;
+
+	chained_irq_enter(chip, desc);
+
+	mbox_val = readl_relaxed(intc.base + LOCAL_MAILBOX0_CLR0 + 16 * cpu);
+	if (mbox_val) {
+		int hwirq = ffs(mbox_val) - 1;
+		generic_handle_irq(irq_find_mapping(ipi_domain, hwirq));
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void bcm2836_arm_irqchip_ipi_eoi(struct irq_data *d)
+{
+	int cpu = smp_processor_id();
+
+	writel_relaxed(BIT(d->hwirq),
+		       intc.base + LOCAL_MAILBOX0_CLR0 + 16 * cpu);
+}
+
+static void bcm2836_arm_irqchip_ipi_send_mask(struct irq_data *d,
+					      const struct cpumask *mask)
 {
 	int cpu;
 	void __iomem *mailbox0_base = intc.base + LOCAL_MAILBOX0_SET0;
@@ -157,11 +187,45 @@ static void bcm2836_arm_irqchip_send_ipi(const struct cpumask *mask,
 	 */
 	smp_wmb();
 
-	for_each_cpu(cpu, mask)	{
-		writel(1 << ipi, mailbox0_base + 16 * cpu);
+	for_each_cpu(cpu, mask)
+		writel_relaxed(BIT(d->hwirq), mailbox0_base + 16 * cpu);
+}
+
+static struct irq_chip bcm2836_arm_irqchip_ipi = {
+	.name		= "IPI",
+	.irq_eoi	= bcm2836_arm_irqchip_ipi_eoi,
+	.ipi_send_mask	= bcm2836_arm_irqchip_ipi_send_mask,
+};
+
+static int bcm2836_arm_irqchip_ipi_alloc(struct irq_domain *d,
+					 unsigned int virq,
+					 unsigned int nr_irqs, void *args)
+{
+	int i;
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_set_percpu_devid(virq + i);
+		irq_domain_set_info(d, virq + i, i, &bcm2836_arm_irqchip_ipi,
+				    d->host_data,
+				    handle_percpu_devid_fasteoi_ipi,
+				    NULL, NULL);
 	}
+
+	return 0;
 }
 
+static void bcm2836_arm_irqchip_ipi_free(struct irq_domain *d,
+					 unsigned int virq,
+					 unsigned int nr_irqs)
+{
+	/* Not freeing IPIs */
+}
+
+static const struct irq_domain_ops ipi_domain_ops = {
+	.alloc	= bcm2836_arm_irqchip_ipi_alloc,
+	.free	= bcm2836_arm_irqchip_ipi_free,
+};
+
 static int bcm2836_cpu_starting(unsigned int cpu)
 {
 	bcm2836_arm_irqchip_unmask_per_cpu_irq(LOCAL_MAILBOX_INT_CONTROL0, 0,
@@ -175,25 +239,58 @@ static int bcm2836_cpu_dying(unsigned int cpu)
 					     cpu);
 	return 0;
 }
-#endif
 
-static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = {
-	.xlate = irq_domain_xlate_onetwocell,
-	.map = bcm2836_map,
-};
+#define BITS_PER_MBOX	32
 
-static void
-bcm2836_arm_irqchip_smp_init(void)
+static void bcm2836_arm_irqchip_smp_init(void)
 {
-#ifdef CONFIG_SMP
+	struct irq_fwspec ipi_fwspec = {
+		.fwnode		= intc.domain->fwnode,
+		.param_count	= 1,
+		.param		= {
+			[0]	= LOCAL_IRQ_MAILBOX0,
+		},
+	};
+	int base_ipi, mux_irq;
+
+	mux_irq = irq_create_fwspec_mapping(&ipi_fwspec);
+	if (WARN_ON(mux_irq <= 0))
+		return;
+
+	ipi_domain = irq_domain_create_linear(intc.domain->fwnode,
+					      BITS_PER_MBOX, &ipi_domain_ops,
+					      NULL);
+	if (WARN_ON(!ipi_domain))
+		return;
+
+	ipi_domain->flags |= IRQ_DOMAIN_FLAG_IPI_SINGLE;
+	irq_domain_update_bus_token(ipi_domain, DOMAIN_BUS_IPI);
+
+	base_ipi = __irq_domain_alloc_irqs(ipi_domain, -1, BITS_PER_MBOX,
+					   NUMA_NO_NODE, NULL,
+					   false, NULL);
+
+	if (WARN_ON(!base_ipi))
+		return;
+
+	set_smp_ipi_range(base_ipi, BITS_PER_MBOX);
+
+	irq_set_chained_handler_and_data(mux_irq,
+					 bcm2836_arm_irqchip_handle_ipi, NULL);
+
 	/* Unmask IPIs to the boot CPU. */
 	cpuhp_setup_state(CPUHP_AP_IRQ_BCM2836_STARTING,
 			  "irqchip/bcm2836:starting", bcm2836_cpu_starting,
 			  bcm2836_cpu_dying);
-
-	set_smp_cross_call(bcm2836_arm_irqchip_send_ipi);
-#endif
 }
+#else
+#define bcm2836_arm_irqchip_smp_init()	do { } while(0)
+#endif
+
+static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = {
+	.xlate = irq_domain_xlate_onetwocell,
+	.map = bcm2836_map,
+};
 
 /*
  * The LOCAL_IRQ_CNT* timer firings are based off of the external
@@ -232,6 +329,8 @@ static int __init bcm2836_arm_irqchip_l1_intc_of_init(struct device_node *node,
 	if (!intc.domain)
 		panic("%pOF: unable to create IRQ domain\n", node);
 
+	irq_domain_update_bus_token(intc.domain, DOMAIN_BUS_WIRED);
+
 	bcm2836_arm_irqchip_smp_init();
 
 	set_handle_irq(bcm2836_arm_irqchip_handle_irq);
-- 
2.27.0

[PATCH v3 11/16] irqchip/hip04: Configure IPIs as standard interrupts

[Marc Zyngier]

In order to switch the hip04 driver to provide standard interrupts
for IPIs, rework the way interrupts are allocated, making sure
the irqdomain covers the SGIs as well as the rest of the interrupt
range.

The driver is otherwise so old-school that it creates all interrupts
upfront (duh!), so there is hardly anything else to change, apart
from communicating the IPIs to the arch code.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-hip04.c | 89 +++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 49 deletions(-)

diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c
index 130caa1c9d93..9b73dcfaf48d 100644
--- a/drivers/irqchip/irq-hip04.c
+++ b/drivers/irqchip/irq-hip04.c
@@ -171,6 +171,29 @@ static int hip04_irq_set_affinity(struct irq_data *d,
 
 	return IRQ_SET_MASK_OK;
 }
+
+static void hip04_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
+{
+	int cpu;
+	unsigned long flags, map = 0;
+
+	raw_spin_lock_irqsave(&irq_controller_lock, flags);
+
+	/* Convert our logical CPU mask into a physical one. */
+	for_each_cpu(cpu, mask)
+		map |= hip04_cpu_map[cpu];
+
+	/*
+	 * Ensure that stores to Normal memory are visible to the
+	 * other CPUs before they observe us issuing the IPI.
+	 */
+	dmb(ishst);
+
+	/* this always happens on GIC0 */
+	writel_relaxed(map << 8 | d->hwirq, hip04_data.dist_base + GIC_DIST_SOFTINT);
+
+	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
+}
 #endif
 
 static void __exception_irq_entry hip04_handle_irq(struct pt_regs *regs)
@@ -182,19 +205,9 @@ static void __exception_irq_entry hip04_handle_irq(struct pt_regs *regs)
 		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
 		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
 
-		if (likely(irqnr > 15 && irqnr <= HIP04_MAX_IRQS)) {
+		if (irqnr <= HIP04_MAX_IRQS)
 			handle_domain_irq(hip04_data.domain, irqnr, regs);
-			continue;
-		}
-		if (irqnr < 16) {
-			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
-#ifdef CONFIG_SMP
-			handle_IPI(irqnr, regs);
-#endif
-			continue;
-		}
-		break;
-	} while (1);
+	} while (irqnr > HIP04_MAX_IRQS);
 }
 
 static struct irq_chip hip04_irq_chip = {
@@ -205,6 +218,7 @@ static struct irq_chip hip04_irq_chip = {
 	.irq_set_type		= hip04_irq_set_type,
 #ifdef CONFIG_SMP
 	.irq_set_affinity	= hip04_irq_set_affinity,
+	.ipi_send_mask		= hip04_ipi_send_mask,
 #endif
 	.flags			= IRQCHIP_SET_TYPE_MASKED |
 				  IRQCHIP_SKIP_SET_WAKE |
@@ -279,39 +293,17 @@ static void hip04_irq_cpu_init(struct hip04_irq_data *intc)
 	writel_relaxed(1, base + GIC_CPU_CTRL);
 }
 
-#ifdef CONFIG_SMP
-static void hip04_raise_softirq(const struct cpumask *mask, unsigned int irq)
-{
-	int cpu;
-	unsigned long flags, map = 0;
-
-	raw_spin_lock_irqsave(&irq_controller_lock, flags);
-
-	/* Convert our logical CPU mask into a physical one. */
-	for_each_cpu(cpu, mask)
-		map |= hip04_cpu_map[cpu];
-
-	/*
-	 * Ensure that stores to Normal memory are visible to the
-	 * other CPUs before they observe us issuing the IPI.
-	 */
-	dmb(ishst);
-
-	/* this always happens on GIC0 */
-	writel_relaxed(map << 8 | irq, hip04_data.dist_base + GIC_DIST_SOFTINT);
-
-	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
-}
-#endif
-
 static int hip04_irq_domain_map(struct irq_domain *d, unsigned int irq,
 				irq_hw_number_t hw)
 {
-	if (hw < 32) {
+	if (hw < 16) {
+		irq_set_percpu_devid(irq);
+		irq_set_chip_and_handler(irq, &hip04_irq_chip,
+					 handle_percpu_devid_fasteoi_ipi);
+	} else if (hw < 32) {
 		irq_set_percpu_devid(irq);
 		irq_set_chip_and_handler(irq, &hip04_irq_chip,
 					 handle_percpu_devid_irq);
-		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 	} else {
 		irq_set_chip_and_handler(irq, &hip04_irq_chip,
 					 handle_fasteoi_irq);
@@ -328,10 +320,13 @@ static int hip04_irq_domain_xlate(struct irq_domain *d,
 				  unsigned long *out_hwirq,
 				  unsigned int *out_type)
 {
-	unsigned long ret = 0;
-
 	if (irq_domain_get_of_node(d) != controller)
 		return -EINVAL;
+	if (intsize == 1 && intspec[0] < 16) {
+		*out_hwirq = intspec[0];
+		*out_type = IRQ_TYPE_EDGE_RISING;
+		return 0;
+	}
 	if (intsize < 3)
 		return -EINVAL;
 
@@ -344,7 +339,7 @@ static int hip04_irq_domain_xlate(struct irq_domain *d,
 
 	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
 
-	return ret;
+	return 0;
 }
 
 static int hip04_irq_starting_cpu(unsigned int cpu)
@@ -361,7 +356,6 @@ static const struct irq_domain_ops hip04_irq_domain_ops = {
 static int __init
 hip04_of_init(struct device_node *node, struct device_node *parent)
 {
-	irq_hw_number_t hwirq_base = 16;
 	int nr_irqs, irq_base, i;
 
 	if (WARN_ON(!node))
@@ -390,24 +384,21 @@ hip04_of_init(struct device_node *node, struct device_node *parent)
 		nr_irqs = HIP04_MAX_IRQS;
 	hip04_data.nr_irqs = nr_irqs;
 
-	nr_irqs -= hwirq_base; /* calculate # of irqs to allocate */
-
-	irq_base = irq_alloc_descs(-1, hwirq_base, nr_irqs, numa_node_id());
+	irq_base = irq_alloc_descs(-1, 0, nr_irqs, numa_node_id());
 	if (irq_base < 0) {
 		pr_err("failed to allocate IRQ numbers\n");
 		return -EINVAL;
 	}
 
 	hip04_data.domain = irq_domain_add_legacy(node, nr_irqs, irq_base,
-						  hwirq_base,
+						  0,
 						  &hip04_irq_domain_ops,
 						  &hip04_data);
-
 	if (WARN_ON(!hip04_data.domain))
 		return -EINVAL;
 
 #ifdef CONFIG_SMP
-	set_smp_cross_call(hip04_raise_softirq);
+	set_smp_ipi_range(irq_base, 16);
 #endif
 	set_handle_irq(hip04_handle_irq);
 
-- 
2.27.0

[PATCH v3 12/16] irqchip/armada-370-xp: Configure IPIs as standard interrupts

[Marc Zyngier]

To introduce IPIs as standard interrupts to the Armada 370-XP
driver, let's allocate a completely separate irqdomain and
irqchip combo that lives parallel to the "standard" one.

This effectively should be modelled as a chained interrupt
controller, but the code is in such a state that it is
pretty hard to shoehorn, as it would require the rewrite
of the MSI layer as well.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-armada-370-xp.c | 262 +++++++++++++++++++---------
 1 file changed, 178 insertions(+), 84 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index c9bdc5221b82..d7eb2e93db8f 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -310,7 +310,134 @@ static inline int armada_370_xp_msi_init(struct device_node *node,
 }
 #endif
 
+static void armada_xp_mpic_perf_init(void)
+{
+	unsigned long cpuid = cpu_logical_map(smp_processor_id());
+
+	/* Enable Performance Counter Overflow interrupts */
+	writel(ARMADA_370_XP_INT_CAUSE_PERF(cpuid),
+	       per_cpu_int_base + ARMADA_370_XP_INT_FABRIC_MASK_OFFS);
+}
+
 #ifdef CONFIG_SMP
+static struct irq_domain *ipi_domain;
+
+static void armada_370_xp_ipi_mask(struct irq_data *d)
+{
+	u32 reg;
+	reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
+	reg &= ~BIT(d->hwirq);
+	writel(reg, per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
+}
+
+static void armada_370_xp_ipi_unmask(struct irq_data *d)
+{
+	u32 reg;
+	reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
+	reg |= BIT(d->hwirq);
+	writel(reg, per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
+}
+
+static void armada_370_xp_ipi_send_mask(struct irq_data *d,
+					const struct cpumask *mask)
+{
+	unsigned long map = 0;
+	int cpu;
+
+	/* Convert our logical CPU mask into a physical one. */
+	for_each_cpu(cpu, mask)
+		map |= 1 << cpu_logical_map(cpu);
+
+	/*
+	 * Ensure that stores to Normal memory are visible to the
+	 * other CPUs before issuing the IPI.
+	 */
+	dsb();
+
+	/* submit softirq */
+	writel((map << 8) | d->hwirq, main_int_base +
+		ARMADA_370_XP_SW_TRIG_INT_OFFS);
+}
+
+static void armada_370_xp_ipi_eoi(struct irq_data *d)
+{
+	writel(~BIT(d->hwirq), per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);
+}
+
+static struct irq_chip ipi_irqchip = {
+	.name		= "IPI",
+	.irq_mask	= armada_370_xp_ipi_mask,
+	.irq_unmask	= armada_370_xp_ipi_unmask,
+	.irq_eoi	= armada_370_xp_ipi_eoi,
+	.ipi_send_mask	= armada_370_xp_ipi_send_mask,
+};
+
+static int armada_370_xp_ipi_alloc(struct irq_domain *d,
+					 unsigned int virq,
+					 unsigned int nr_irqs, void *args)
+{
+	int i;
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_set_percpu_devid(virq + i);
+		irq_domain_set_info(d, virq + i, i, &ipi_irqchip,
+				    d->host_data,
+				    handle_percpu_devid_fasteoi_ipi,
+				    NULL, NULL);
+	}
+
+	return 0;
+}
+
+static void armada_370_xp_ipi_free(struct irq_domain *d,
+					 unsigned int virq,
+					 unsigned int nr_irqs)
+{
+	/* Not freeing IPIs */
+}
+
+static const struct irq_domain_ops ipi_domain_ops = {
+	.alloc	= armada_370_xp_ipi_alloc,
+	.free	= armada_370_xp_ipi_free,
+};
+
+static void ipi_resume(void)
+{
+	int i;
+
+	for (i = 0; i < IPI_DOORBELL_END; i++) {
+		int irq;
+
+		irq = irq_find_mapping(ipi_domain, i);
+		if (irq <= 0)
+			continue;
+		if (irq_percpu_is_enabled(irq)) {
+			struct irq_data *d;
+			d = irq_domain_get_irq_data(ipi_domain, irq);
+			armada_370_xp_ipi_unmask(d);
+		}
+	}
+}
+
+static __init void armada_xp_ipi_init(struct device_node *node)
+{
+	int base_ipi;
+
+	ipi_domain = irq_domain_create_linear(of_node_to_fwnode(node),
+					      IPI_DOORBELL_END,
+					      &ipi_domain_ops, NULL);
+	if (WARN_ON(!ipi_domain))
+		return;
+
+	irq_domain_update_bus_token(ipi_domain, DOMAIN_BUS_IPI);
+	base_ipi = __irq_domain_alloc_irqs(ipi_domain, -1, IPI_DOORBELL_END,
+					   NUMA_NO_NODE, NULL, false, NULL);
+	if (WARN_ON(!base_ipi))
+		return;
+
+	set_smp_ipi_range(base_ipi, IPI_DOORBELL_END);
+}
+
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 
 static int armada_xp_set_affinity(struct irq_data *d,
@@ -334,43 +461,6 @@ static int armada_xp_set_affinity(struct irq_data *d,
 
 	return IRQ_SET_MASK_OK;
 }
-#endif
-
-static struct irq_chip armada_370_xp_irq_chip = {
-	.name		= "MPIC",
-	.irq_mask       = armada_370_xp_irq_mask,
-	.irq_mask_ack   = armada_370_xp_irq_mask,
-	.irq_unmask     = armada_370_xp_irq_unmask,
-#ifdef CONFIG_SMP
-	.irq_set_affinity = armada_xp_set_affinity,
-#endif
-	.flags		= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
-};
-
-static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
-				      unsigned int virq, irq_hw_number_t hw)
-{
-	armada_370_xp_irq_mask(irq_get_irq_data(virq));
-	if (!is_percpu_irq(hw))
-		writel(hw, per_cpu_int_base +
-			ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
-	else
-		writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS);
-	irq_set_status_flags(virq, IRQ_LEVEL);
-
-	if (is_percpu_irq(hw)) {
-		irq_set_percpu_devid(virq);
-		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
-					handle_percpu_devid_irq);
-	} else {
-		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
-					handle_level_irq);
-		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq)));
-	}
-	irq_set_probe(virq);
-
-	return 0;
-}
 
 static void armada_xp_mpic_smp_cpu_init(void)
 {
@@ -383,48 +473,16 @@ static void armada_xp_mpic_smp_cpu_init(void)
 	for (i = 0; i < nr_irqs; i++)
 		writel(i, per_cpu_int_base + ARMADA_370_XP_INT_SET_MASK_OFFS);
 
+	/* Disable all IPIs */
+	writel(0, per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
+
 	/* Clear pending IPIs */
 	writel(0, per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);
 
-	/* Enable first 8 IPIs */
-	writel(IPI_DOORBELL_MASK, per_cpu_int_base +
-		ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
-
 	/* Unmask IPI interrupt */
 	writel(0, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
 }
 
-static void armada_xp_mpic_perf_init(void)
-{
-	unsigned long cpuid = cpu_logical_map(smp_processor_id());
-
-	/* Enable Performance Counter Overflow interrupts */
-	writel(ARMADA_370_XP_INT_CAUSE_PERF(cpuid),
-	       per_cpu_int_base + ARMADA_370_XP_INT_FABRIC_MASK_OFFS);
-}
-
-#ifdef CONFIG_SMP
-static void armada_mpic_send_doorbell(const struct cpumask *mask,
-				      unsigned int irq)
-{
-	int cpu;
-	unsigned long map = 0;
-
-	/* Convert our logical CPU mask into a physical one. */
-	for_each_cpu(cpu, mask)
-		map |= 1 << cpu_logical_map(cpu);
-
-	/*
-	 * Ensure that stores to Normal memory are visible to the
-	 * other CPUs before issuing the IPI.
-	 */
-	dsb();
-
-	/* submit softirq */
-	writel((map << 8) | irq, main_int_base +
-		ARMADA_370_XP_SW_TRIG_INT_OFFS);
-}
-
 static void armada_xp_mpic_reenable_percpu(void)
 {
 	unsigned int irq;
@@ -445,6 +503,8 @@ static void armada_xp_mpic_reenable_percpu(void)
 
 		armada_370_xp_irq_unmask(data);
 	}
+
+	ipi_resume();
 }
 
 static int armada_xp_mpic_starting_cpu(unsigned int cpu)
@@ -462,7 +522,46 @@ static int mpic_cascaded_starting_cpu(unsigned int cpu)
 	enable_percpu_irq(parent_irq, IRQ_TYPE_NONE);
 	return 0;
 }
+#else
+static void armada_xp_mpic_smp_cpu_init(void) {}
+static void ipi_resume(void) {}
+#endif
+
+static struct irq_chip armada_370_xp_irq_chip = {
+	.name		= "MPIC",
+	.irq_mask       = armada_370_xp_irq_mask,
+	.irq_mask_ack   = armada_370_xp_irq_mask,
+	.irq_unmask     = armada_370_xp_irq_unmask,
+#ifdef CONFIG_SMP
+	.irq_set_affinity = armada_xp_set_affinity,
 #endif
+	.flags		= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
+				      unsigned int virq, irq_hw_number_t hw)
+{
+	armada_370_xp_irq_mask(irq_get_irq_data(virq));
+	if (!is_percpu_irq(hw))
+		writel(hw, per_cpu_int_base +
+			ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
+	else
+		writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS);
+	irq_set_status_flags(virq, IRQ_LEVEL);
+
+	if (is_percpu_irq(hw)) {
+		irq_set_percpu_devid(virq);
+		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
+					handle_percpu_devid_irq);
+	} else {
+		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
+					handle_level_irq);
+		irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq)));
+	}
+	irq_set_probe(virq);
+
+	return 0;
+}
 
 static const struct irq_domain_ops armada_370_xp_mpic_irq_ops = {
 	.map = armada_370_xp_mpic_irq_map,
@@ -562,22 +661,15 @@ armada_370_xp_handle_irq(struct pt_regs *regs)
 #ifdef CONFIG_SMP
 		/* IPI Handling */
 		if (irqnr == 0) {
-			u32 ipimask, ipinr;
+			unsigned long ipimask;
+			int ipi;
 
 			ipimask = readl_relaxed(per_cpu_int_base +
 						ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
 				& IPI_DOORBELL_MASK;
 
-			writel(~ipimask, per_cpu_int_base +
-				ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);
-
-			/* Handle all pending doorbells */
-			for (ipinr = IPI_DOORBELL_START;
-			     ipinr < IPI_DOORBELL_END; ipinr++) {
-				if (ipimask & (0x1 << ipinr))
-					handle_IPI(ipinr, regs);
-			}
-			continue;
+			for_each_set_bit(ipi, &ipimask, IPI_DOORBELL_END)
+				handle_domain_irq(ipi_domain, ipi, regs);
 		}
 #endif
 
@@ -636,6 +728,8 @@ static void armada_370_xp_mpic_resume(void)
 		writel(0, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
 	if (doorbell_mask_reg & PCI_MSI_DOORBELL_MASK)
 		writel(1, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
+
+	ipi_resume();
 }
 
 static struct syscore_ops armada_370_xp_mpic_syscore_ops = {
@@ -691,7 +785,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 		irq_set_default_host(armada_370_xp_mpic_domain);
 		set_handle_irq(armada_370_xp_handle_irq);
 #ifdef CONFIG_SMP
-		set_smp_cross_call(armada_mpic_send_doorbell);
+		armada_xp_ipi_init(node);
 		cpuhp_setup_state_nocalls(CPUHP_AP_IRQ_ARMADA_XP_STARTING,
 					  "irqchip/armada/ipi:starting",
 					  armada_xp_mpic_starting_cpu, NULL);
-- 
2.27.0

[PATCH v3 13/16] arm64: Kill __smp_cross_call and co

[Marc Zyngier]

The old IPI registration interface is now unused on arm64, so let's
get rid of it.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/irq_work.h |  4 +---
 arch/arm64/include/asm/smp.h      |  7 ------
 arch/arm64/kernel/smp.c           | 38 ++++++-------------------------
 3 files changed, 8 insertions(+), 41 deletions(-)

diff --git a/arch/arm64/include/asm/irq_work.h b/arch/arm64/include/asm/irq_work.h
index 8a1ef1907760..a1020285ea75 100644
--- a/arch/arm64/include/asm/irq_work.h
+++ b/arch/arm64/include/asm/irq_work.h
@@ -2,11 +2,9 @@
 #ifndef __ASM_IRQ_WORK_H
 #define __ASM_IRQ_WORK_H
 
-#include <asm/smp.h>
-
 static inline bool arch_irq_work_has_interrupt(void)
 {
-	return !!__smp_cross_call;
+	return true;
 }
 
 #endif /* __ASM_IRQ_WORK_H */
diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index 57c5db15f6b7..06bc8684f70c 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -71,13 +71,6 @@ extern void handle_IPI(int ipinr, struct pt_regs *regs);
  */
 extern void smp_init_cpus(void);
 
-/*
- * Provide a function to raise an IPI cross call on CPUs in callmap.
- */
-extern void set_smp_cross_call(void (*)(const struct cpumask *, unsigned int));
-
-extern void (*__smp_cross_call)(const struct cpumask *, unsigned int);
-
 /*
  * Register IPI interrupts with the arch SMP code
  */
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 00c9db1b61b5..58fb155fb0ab 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -782,13 +782,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	}
 }
 
-void (*__smp_cross_call)(const struct cpumask *, unsigned int);
-
-void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
-{
-	__smp_cross_call = fn;
-}
-
 static const char *ipi_types[NR_IPI] __tracepoint_string = {
 #define S(x,s)	[x] = s
 	S(IPI_RESCHEDULE, "Rescheduling interrupts"),
@@ -800,11 +793,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
 	S(IPI_WAKEUP, "CPU wake-up interrupts"),
 };
 
-static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
-{
-	trace_ipi_raise(target, ipi_types[ipinr]);
-	__smp_cross_call(target, ipinr);
-}
+static void smp_cross_call(const struct cpumask *target, unsigned int ipinr);
 
 void show_ipi_list(struct seq_file *p, int prec)
 {
@@ -851,8 +840,7 @@ void arch_send_wakeup_ipi_mask(const struct cpumask *mask)
 #ifdef CONFIG_IRQ_WORK
 void arch_irq_work_raise(void)
 {
-	if (__smp_cross_call)
-		smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
+	smp_cross_call(cpumask_of(smp_processor_id()), IPI_IRQ_WORK);
 }
 #endif
 
@@ -959,34 +947,23 @@ static void do_handle_IPI(int ipinr)
 		trace_ipi_exit_rcuidle(ipi_types[ipinr]);
 }
 
-/* Legacy version, should go away once all irqchips have been converted */
-void handle_IPI(int ipinr, struct pt_regs *regs)
-{
-	struct pt_regs *old_regs = set_irq_regs(regs);
-
-	irq_enter();
-	do_handle_IPI(ipinr);
-	irq_exit();
-
-	set_irq_regs(old_regs);
-}
-
 static irqreturn_t ipi_handler(int irq, void *data)
 {
 	do_handle_IPI(irq - ipi_irq_base);
 	return IRQ_HANDLED;
 }
 
-static void ipi_send(const struct cpumask *target, unsigned int ipi)
+static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
 {
-	__ipi_send_mask(ipi_desc[ipi], target);
+	trace_ipi_raise(target, ipi_types[ipinr]);
+	__ipi_send_mask(ipi_desc[ipinr], target);
 }
 
 static void ipi_setup(int cpu)
 {
 	int i;
 
-	if (!ipi_irq_base)
+	if (WARN_ON_ONCE(!ipi_irq_base))
 		return;
 
 	for (i = 0; i < nr_ipi; i++)
@@ -997,7 +974,7 @@ static void ipi_teardown(int cpu)
 {
 	int i;
 
-	if (!ipi_irq_base)
+	if (WARN_ON_ONCE(!ipi_irq_base))
 		return;
 
 	for (i = 0; i < nr_ipi; i++)
@@ -1023,7 +1000,6 @@ void __init set_smp_ipi_range(int ipi_base, int n)
 	}
 
 	ipi_irq_base = ipi_base;
-	__smp_cross_call = ipi_send;
 
 	/* Setup the boot CPU immediately */
 	ipi_setup(smp_processor_id());
-- 
2.27.0

[PATCH v3 14/16] arm64: Remove custom IRQ stat accounting

[Marc Zyngier]

Let's switch the arm64 code to the core accounting, which already
does everything we need.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/hardirq.h |  9 ---------
 arch/arm64/kernel/smp.c          | 24 ++++++------------------
 2 files changed, 6 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
index 985493af704b..5ffa4bacdad3 100644
--- a/arch/arm64/include/asm/hardirq.h
+++ b/arch/arm64/include/asm/hardirq.h
@@ -13,21 +13,12 @@
 #include <asm/kvm_arm.h>
 #include <asm/sysreg.h>
 
-#define NR_IPI	7
-
 typedef struct {
 	unsigned int __softirq_pending;
-	unsigned int ipi_irqs[NR_IPI];
 } ____cacheline_aligned irq_cpustat_t;
 
 #include <linux/irq_cpustat.h>	/* Standard mappings for irq_cpustat_t above */
 
-#define __inc_irq_stat(cpu, member)	__IRQ_STAT(cpu, member)++
-#define __get_irq_stat(cpu, member)	__IRQ_STAT(cpu, member)
-
-u64 smp_irq_stat_cpu(unsigned int cpu);
-#define arch_irq_stat_cpu	smp_irq_stat_cpu
-
 #define __ARCH_IRQ_EXIT_IRQS_DISABLED	1
 
 struct nmi_ctx {
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 58fb155fb0ab..07b45466e0bb 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -72,7 +72,8 @@ enum ipi_msg_type {
 	IPI_CPU_CRASH_STOP,
 	IPI_TIMER,
 	IPI_IRQ_WORK,
-	IPI_WAKEUP
+	IPI_WAKEUP,
+	NR_IPI
 };
 
 static int ipi_irq_base __read_mostly;
@@ -800,26 +801,15 @@ void show_ipi_list(struct seq_file *p, int prec)
 	unsigned int cpu, i;
 
 	for (i = 0; i < NR_IPI; i++) {
+		unsigned int irq = irq_desc_get_irq(ipi_desc[i]);
 		seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i,
 			   prec >= 4 ? " " : "");
 		for_each_online_cpu(cpu)
-			seq_printf(p, "%10u ",
-				   __get_irq_stat(cpu, ipi_irqs[i]));
+			seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
 		seq_printf(p, "      %s\n", ipi_types[i]);
 	}
 }
 
-u64 smp_irq_stat_cpu(unsigned int cpu)
-{
-	u64 sum = 0;
-	int i;
-
-	for (i = 0; i < NR_IPI; i++)
-		sum += __get_irq_stat(cpu, ipi_irqs[i]);
-
-	return sum;
-}
-
 void arch_send_call_function_ipi_mask(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_CALL_FUNC);
@@ -892,10 +882,8 @@ static void do_handle_IPI(int ipinr)
 {
 	unsigned int cpu = smp_processor_id();
 
-	if ((unsigned)ipinr < NR_IPI) {
+	if ((unsigned)ipinr < NR_IPI)
 		trace_ipi_entry_rcuidle(ipi_types[ipinr]);
-		__inc_irq_stat(cpu, ipi_irqs[ipinr]);
-	}
 
 	switch (ipinr) {
 	case IPI_RESCHEDULE:
@@ -992,7 +980,7 @@ void __init set_smp_ipi_range(int ipi_base, int n)
 		int err;
 
 		err = request_percpu_irq(ipi_base + i, ipi_handler,
-					 "IPI", &irq_stat);
+					 "IPI", &cpu_number);
 		WARN_ON(err);
 
 		ipi_desc[i] = irq_to_desc(ipi_base + i);
-- 
2.27.0

[PATCH v3 15/16] ARM: Kill __smp_cross_call and co

[Marc Zyngier]

The old IPI registration interface is now unused on arm, so let's
get rid of it.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm/include/asm/smp.h |  6 ------
 arch/arm/kernel/smp.c      | 26 +++++++-------------------
 2 files changed, 7 insertions(+), 25 deletions(-)

diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 0e29730295ca..0ca55a607d0a 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -39,12 +39,6 @@ void handle_IPI(int ipinr, struct pt_regs *regs);
  */
 extern void smp_init_cpus(void);
 
-
-/*
- * Provide a function to raise an IPI cross call on CPUs in callmap.
- */
-extern void set_smp_cross_call(void (*)(const struct cpumask *, unsigned int));
-
 /*
  * Register IPI interrupts with the arch SMP code
  */
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index f21f78483353..d51e64955a26 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -511,14 +511,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	}
 }
 
-static void (*__smp_cross_call)(const struct cpumask *, unsigned int);
-
-void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
-{
-	if (!__smp_cross_call)
-		__smp_cross_call = fn;
-}
-
 static const char *ipi_types[NR_IPI] __tracepoint_string = {
 #define S(x,s)	[x] = s
 	S(IPI_WAKEUP, "CPU wakeup interrupts"),
@@ -530,11 +522,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
 	S(IPI_COMPLETION, "completion interrupts"),
 };
 
-static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
-{
-	trace_ipi_raise_rcuidle(target, ipi_types[ipinr]);
-	__smp_cross_call(target, ipinr);
-}
+static void smp_cross_call(const struct cpumask *target, unsigned int ipinr);
 
 void show_ipi_list(struct seq_file *p, int prec)
 {
@@ -713,16 +701,17 @@ static irqreturn_t ipi_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void ipi_send(const struct cpumask *target, unsigned int ipi)
+static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
 {
-	__ipi_send_mask(ipi_desc[ipi], target);
+	trace_ipi_raise_rcuidle(target, ipi_types[ipinr]);
+	__ipi_send_mask(ipi_desc[ipinr], target);
 }
 
 static void ipi_setup(int cpu)
 {
 	int i;
 
-	if (!ipi_irq_base)
+	if (WARN_ON_ONCE(!ipi_irq_base))
 		return;
 
 	for (i = 0; i < nr_ipi; i++)
@@ -733,7 +722,7 @@ static void ipi_teardown(int cpu)
 {
 	int i;
 
-	if (!ipi_irq_base)
+	if (WARN_ON_ONCE(!ipi_irq_base))
 		return;
 
 	for (i = 0; i < nr_ipi; i++)
@@ -759,7 +748,6 @@ void __init set_smp_ipi_range(int ipi_base, int n)
 	}
 
 	ipi_irq_base = ipi_base;
-	set_smp_cross_call(ipi_send);
 
 	/* Setup the boot CPU immediately */
 	ipi_setup(smp_processor_id());
@@ -872,7 +860,7 @@ core_initcall(register_cpufreq_notifier);
 
 static void raise_nmi(cpumask_t *mask)
 {
-	__smp_cross_call(mask, IPI_CPU_BACKTRACE);
+	__ipi_send_mask(ipi_desc[IPI_CPU_BACKTRACE], mask);
 }
 
 void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
-- 
2.27.0

[PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[Marc Zyngier]

Let's switch the arm code to the core accounting, which already
does everything we need.

Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm/include/asm/hardirq.h | 17 -----------------
 arch/arm/kernel/smp.c          | 20 ++++----------------
 2 files changed, 4 insertions(+), 33 deletions(-)

diff --git a/arch/arm/include/asm/hardirq.h b/arch/arm/include/asm/hardirq.h
index 7a88f160b1fb..b95848ed2bc7 100644
--- a/arch/arm/include/asm/hardirq.h
+++ b/arch/arm/include/asm/hardirq.h
@@ -6,29 +6,12 @@
 #include <linux/threads.h>
 #include <asm/irq.h>
 
-/* number of IPIS _not_ including IPI_CPU_BACKTRACE */
-#define NR_IPI	7
-
 typedef struct {
 	unsigned int __softirq_pending;
-#ifdef CONFIG_SMP
-	unsigned int ipi_irqs[NR_IPI];
-#endif
 } ____cacheline_aligned irq_cpustat_t;
 
 #include <linux/irq_cpustat.h>	/* Standard mappings for irq_cpustat_t above */
 
-#define __inc_irq_stat(cpu, member)	__IRQ_STAT(cpu, member)++
-#define __get_irq_stat(cpu, member)	__IRQ_STAT(cpu, member)
-
-#ifdef CONFIG_SMP
-u64 smp_irq_stat_cpu(unsigned int cpu);
-#else
-#define smp_irq_stat_cpu(cpu)	0
-#endif
-
-#define arch_irq_stat_cpu	smp_irq_stat_cpu
-
 #define __ARCH_IRQ_EXIT_IRQS_DISABLED	1
 
 #endif /* __ASM_HARDIRQ_H */
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index d51e64955a26..aead847ac8b9 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -65,6 +65,7 @@ enum ipi_msg_type {
 	IPI_CPU_STOP,
 	IPI_IRQ_WORK,
 	IPI_COMPLETION,
+	NR_IPI,
 	/*
 	 * CPU_BACKTRACE is special and not included in NR_IPI
 	 * or tracable with trace_ipi_*
@@ -529,27 +530,16 @@ void show_ipi_list(struct seq_file *p, int prec)
 	unsigned int cpu, i;
 
 	for (i = 0; i < NR_IPI; i++) {
+		unsigned int irq = irq_desc_get_irq(ipi_desc[i]);
 		seq_printf(p, "%*s%u: ", prec - 1, "IPI", i);
 
 		for_each_online_cpu(cpu)
-			seq_printf(p, "%10u ",
-				   __get_irq_stat(cpu, ipi_irqs[i]));
+			seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
 
 		seq_printf(p, " %s\n", ipi_types[i]);
 	}
 }
 
-u64 smp_irq_stat_cpu(unsigned int cpu)
-{
-	u64 sum = 0;
-	int i;
-
-	for (i = 0; i < NR_IPI; i++)
-		sum += __get_irq_stat(cpu, ipi_irqs[i]);
-
-	return sum;
-}
-
 void arch_send_call_function_ipi_mask(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_CALL_FUNC);
@@ -630,10 +620,8 @@ static void do_handle_IPI(int ipinr)
 {
 	unsigned int cpu = smp_processor_id();
 
-	if ((unsigned)ipinr < NR_IPI) {
+	if ((unsigned)ipinr < NR_IPI)
 		trace_ipi_entry_rcuidle(ipi_types[ipinr]);
-		__inc_irq_stat(cpu, ipi_irqs[ipinr]);
-	}
 
 	switch (ipinr) {
 	case IPI_WAKEUP:
-- 
2.27.0

Re: [PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3048 bytes --]

Hi Marc,

I love your patch! Yet something to improve:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on arm/for-next v5.9-rc3 next-20200828]
[cannot apply to tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Marc-Zyngier/arm-arm64-Turning-IPIs-into-normal-interrupts/20200901-225407
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm-realview_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/arm/kernel/smp.c: In function 'show_ipi_list':
>> arch/arm/kernel/smp.c:537:27: error: implicit declaration of function 'kstat_irqs_cpu' [-Werror=implicit-function-declaration]
     537 |    seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
         |                           ^~~~~~~~~~~~~~
   arch/arm/kernel/smp.c: At top level:
   arch/arm/kernel/smp.c:559:6: warning: no previous prototype for 'arch_irq_work_raise' [-Wmissing-prototypes]
     559 | void arch_irq_work_raise(void)
         |      ^~~~~~~~~~~~~~~~~~~
   arch/arm/kernel/smp.c:774:6: warning: no previous prototype for 'panic_smp_self_stop' [-Wmissing-prototypes]
     774 | void panic_smp_self_stop(void)
         |      ^~~~~~~~~~~~~~~~~~~
   arch/arm/kernel/smp.c:786:5: warning: no previous prototype for 'setup_profiling_timer' [-Wmissing-prototypes]
     786 | int setup_profiling_timer(unsigned int multiplier)
         |     ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

# https://github.com/0day-ci/linux/commit/780a93ca7a729bf2414f45d5322cc62f43ae4070
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Marc-Zyngier/arm-arm64-Turning-IPIs-into-normal-interrupts/20200901-225407
git checkout 780a93ca7a729bf2414f45d5322cc62f43ae4070
vim +/kstat_irqs_cpu +537 arch/arm/kernel/smp.c

   527	
   528	void show_ipi_list(struct seq_file *p, int prec)
   529	{
   530		unsigned int cpu, i;
   531	
   532		for (i = 0; i < NR_IPI; i++) {
   533			unsigned int irq = irq_desc_get_irq(ipi_desc[i]);
   534			seq_printf(p, "%*s%u: ", prec - 1, "IPI", i);
   535	
   536			for_each_online_cpu(cpu)
 > 537				seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
   538	
   539			seq_printf(p, " %s\n", ipi_types[i]);
   540		}
   541	}
   542	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21031 bytes --]

Re: [PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[Marc Zyngier]

On Wed, 02 Sep 2020 08:41:31 +0100,
kernel test robot <lkp@intel.com> wrote:
> 
> Hi Marc,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on arm64/for-next/core]
> [also build test ERROR on arm/for-next v5.9-rc3 next-20200828]
> [cannot apply to tip/irq/core]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Marc-Zyngier/arm-arm64-Turning-IPIs-into-normal-interrupts/20200901-225407
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> config: arm-realview_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    arch/arm/kernel/smp.c: In function 'show_ipi_list':
> >> arch/arm/kernel/smp.c:537:27: error: implicit declaration of function 'kstat_irqs_cpu' [-Werror=implicit-function-declaration]
>      537 |    seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
>          |                           ^~~~~~~~~~~~~~

Now fixed. Thanks for the heads up.

	M.

-- 
Without deviation from the norm, progress is not possible.

RE: [PATCH v3 00/16] arm/arm64: Turning IPIs into normal interrupts

[hasegawa-hitomi]

Hi Marc.

I am interested in this feature.
I have confirmed this patch works fine on Fujitsu FX1000. Thanks for creating it.
I look forward to the day when this patch is merged.
We plan to post a patch that implements IPI for CPU stop Interrupt (for crash dump) with pseudo NMI.

Tested-by: Hitomi Hasegawa <hasegwa-hitomi@fujitsu.com>
Thanks.

-----Original Message-----
From: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> On Behalf Of Marc Zyngier
Sent: Tuesday, September 1, 2020 11:43 PM
To: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Cc: Sumit Garg <sumit.garg@linaro.org>; kernel-team@android.com; Florian Fainelli <f.fainelli@gmail.com>; Russell King <linux@arm.linux.org.uk>; Jason Cooper <jason@lakedaemon.net>; Saravana Kannan <saravanak@google.com>; Andrew Lunn <andrew@lunn.ch>; Catalin Marinas <catalin.marinas@arm.com>; Gregory Clement <gregory.clement@bootlin.com>; Thomas Gleixner <tglx@linutronix.de>; Will Deacon <will@kernel.org>; Valentin Schneider <Valentin.Schneider@arm.com>
Subject: [PATCH v3 00/16] arm/arm64: Turning IPIs into normal interrupts

For as long as SMP ARM has existed, IPIs have been handled as something special. The arch code and the interrupt controller exchange a couple of hooks (one to generate an IPI, another to handle it).

Although this is perfectly manageable, it prevents the use of features that we could use if IPIs were Linux IRQs (such as pseudo-NMIs). It also means that each interrupt controller driver has to follow an architecture-specific interface instead of just implementing the base irqchip functionalities. The arch code also duplicates a number of things that the core irq code already does (such as calling set_irq_regs(), irq_enter()...).

This series tries to remedy this on arm/arm64 by offering a new registration interface where the irqchip gives the arch code a range of interrupts to use for IPIs. The arch code requests these as normal per-cpu interrupts.

The bulk of the work is at the interrupt controller level, where all 5 irqchips used on arm+SMP/arm64 get converted.

Finally, we drop the legacy registration interface as well as the custom statistics accounting.

Note that I have had a look at providing a "generic" interface by expanding the kernel/irq/ipi.c bag of helpers, but so far all irqchips have very different requirements, so there is hardly anything to consolidate for now. Maybe some as hip04 and the Marvell horror get cleaned up (the latter certainly could do with a good dusting).

This has been tested on a bunch of 32 and 64bit guests (GICv2, GICv3), as well as 64bit bare metal (GICv3). The RPi part has only been tested in QEMU as a 64bit guest, while the HiSi and Marvell parts have only been compile-tested.

I'm aiming for 5.10 for this, so any comment would be appreciated.

* From v2:
  - Tidied up the arch code (__read_mostly for the IPI handling data,
    WARN_ON_ONCE on setup and teardown), dropped spurious prototype
    on 32bit
  - Prevent IPIs from being forwarded to VCPUs
  - Merged the GIC affinity fix, hence one less patch
  - Added RBs from Valentin

* From v1:
  - Clarified the effect of nesting irq_enter/exit (Russell)
  - Changed the point where we tear IPIs down on (Valentin)
  - IPIs are no longer accessible from DT
  - HIP04 and Armada 370-XP have been converted, but are untested
  - arch-specific kstat accounting is removed
  - ARM's legacy interface is dropped

Marc Zyngier (16):
  genirq: Add fasteoi IPI flow
  genirq: Allow interrupts to be excluded from /proc/interrupts
  arm64: Allow IPIs to be handled as normal interrupts
  ARM: Allow IPIs to be handled as normal interrupts
  irqchip/gic-v3: Describe the SGI range
  irqchip/gic-v3: Configure SGIs as standard interrupts
  irqchip/gic: Refactor SMP configuration
  irqchip/gic: Configure SGIs as standard interrupts
  irqchip/gic-common: Don't enable SGIs by default
  irqchip/bcm2836: Configure mailbox interrupts as standard interrupts
  irqchip/hip04: Configure IPIs as standard interrupts
  irqchip/armada-370-xp: Configure IPIs as standard interrupts
  arm64: Kill __smp_cross_call and co
  arm64: Remove custom IRQ stat accounting
  ARM: Kill __smp_cross_call and co
  ARM: Remove custom IRQ stat accounting

 arch/arm/Kconfig                    |   1 +
 arch/arm/include/asm/hardirq.h      |  17 --
 arch/arm/include/asm/smp.h          |   5 +-
 arch/arm/kernel/smp.c               | 135 +++++++++-----
 arch/arm64/Kconfig                  |   1 +
 arch/arm64/include/asm/hardirq.h    |   9 -
 arch/arm64/include/asm/irq_work.h   |   4 +-
 arch/arm64/include/asm/smp.h        |   6 +-
 arch/arm64/kernel/smp.c             | 121 ++++++++-----
 drivers/irqchip/irq-armada-370-xp.c | 262 +++++++++++++++++++---------
 drivers/irqchip/irq-bcm2836.c       | 151 +++++++++++++---
 drivers/irqchip/irq-gic-common.c    |   3 -
 drivers/irqchip/irq-gic-v3.c        | 104 ++++++-----
 drivers/irqchip/irq-gic.c           | 177 +++++++++++--------
 drivers/irqchip/irq-hip04.c         |  89 +++++-----
 include/linux/irq.h                 |   5 +-
 kernel/irq/chip.c                   |  27 +++
 kernel/irq/debugfs.c                |   1 +
 kernel/irq/proc.c                   |   2 +-
 kernel/irq/settings.h               |   7 +
 20 files changed, 720 insertions(+), 407 deletions(-)

--
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Catalin Marinas]

On Tue, Sep 01, 2020 at 03:43:11PM +0100, Marc Zyngier wrote:
> In order to deal with IPIs as normal interrupts, let's add
> a new way to register them with the architecture code.
> 
> set_smp_ipi_range() takes a range of interrupts, and allows
> the arch code to request them as if the were normal interrupts.
> A standard handler is then called by the core IRQ code to deal
> with the IPI.
> 
> This means that we don't need to call irq_enter/irq_exit, and
> that we don't need to deal with set_irq_regs either. So let's
> move the dispatcher into its own function, and leave handle_IPI()
> as a compatibility function.
> 
> On the sending side, let's make use of ipi_send_mask, which
> already exists for this purpose.
> 
> One of the major difference is that we end up, in some cases
> (such as when performing IRQ time accounting on the scheduler
> IPI), end up with nested irq_enter()/irq_exit() pairs.
> Other than the (relatively small) overhead, there should be
> no consequences to it (these pairs are designed to nest
> correctly, and the accounting shouldn't be off).
> 
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

In case you need an ack for the arm64 part:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v3 13/16] arm64: Kill __smp_cross_call and co

[Catalin Marinas]

On Tue, Sep 01, 2020 at 03:43:21PM +0100, Marc Zyngier wrote:
> The old IPI registration interface is now unused on arm64, so let's
> get rid of it.
> 
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v3 14/16] arm64: Remove custom IRQ stat accounting

[Catalin Marinas]

On Tue, Sep 01, 2020 at 03:43:22PM +0100, Marc Zyngier wrote:
> Let's switch the arm64 code to the core accounting, which already
> does everything we need.
> 
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marek Szyprowski]

Hi Marc,

On 01.09.2020 16:43, Marc Zyngier wrote:
> Change the way we deal with GIC SGIs by turning them into proper
> IRQs, and calling into the arch code to register the interrupt range
> instead of a callback.
>
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
This patch landed in linux next-20200914 as commit ac063232d4b0 
("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it breaks 
booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM 
Cortex A9 based). Here are the last lines from the bootlog:

[    0.106322] CPU: Testing write buffer coherency: ok
[    0.109895] CPU0: Spectre v2: using BPIALL workaround
[    0.116057] CPU0: thread -1, cpu 0, socket 9, mpidr 80000900
[    0.123885] Setting up static identity map for 0x40100000 - 0x40100060
[    0.130191] rcu: Hierarchical SRCU implementation.
[    0.137195] soc soc0: Exynos: CPU[EXYNOS4210] PRO_ID[0x43210211] 
REV[0x11] Detected
[    0.145129] smp: Bringing up secondary CPUs ...
[    0.156279] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
[    0.156291] CPU1: Spectre v2: using BPIALL workaround
[    2.716379] random: fast init done

> ---
>   drivers/irqchip/irq-gic.c | 103 +++++++++++++++++++++++---------------
>   1 file changed, 63 insertions(+), 40 deletions(-)
>
> ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Hi Marek,

On 2020-09-14 14:06, Marek Szyprowski wrote:
> Hi Marc,
> 
> On 01.09.2020 16:43, Marc Zyngier wrote:
>> Change the way we deal with GIC SGIs by turning them into proper
>> IRQs, and calling into the arch code to register the interrupt range
>> instead of a callback.
>> 
>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> This patch landed in linux next-20200914 as commit ac063232d4b0
> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it breaks
> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
> Cortex A9 based). Here are the last lines from the bootlog:
> 
> [    0.106322] CPU: Testing write buffer coherency: ok
> [    0.109895] CPU0: Spectre v2: using BPIALL workaround
> [    0.116057] CPU0: thread -1, cpu 0, socket 9, mpidr 80000900
> [    0.123885] Setting up static identity map for 0x40100000 - 
> 0x40100060
> [    0.130191] rcu: Hierarchical SRCU implementation.
> [    0.137195] soc soc0: Exynos: CPU[EXYNOS4210] PRO_ID[0x43210211]
> REV[0x11] Detected
> [    0.145129] smp: Bringing up secondary CPUs ...
> [    0.156279] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
> [    0.156291] CPU1: Spectre v2: using BPIALL workaround
> [    2.716379] random: fast init done

Thanks for the report. Is this the funky non-banked GIC?

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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marek Szyprowski]

Hi Marc,

On 14.09.2020 15:13, Marc Zyngier wrote:
> On 2020-09-14 14:06, Marek Szyprowski wrote:
>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>> Change the way we deal with GIC SGIs by turning them into proper
>>> IRQs, and calling into the arch code to register the interrupt range
>>> instead of a callback.
>>>
>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> This patch landed in linux next-20200914 as commit ac063232d4b0
>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it breaks
>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>> Cortex A9 based). Here are the last lines from the bootlog:
>>
>> [    0.106322] CPU: Testing write buffer coherency: ok
>> [    0.109895] CPU0: Spectre v2: using BPIALL workaround
>> [    0.116057] CPU0: thread -1, cpu 0, socket 9, mpidr 80000900
>> [    0.123885] Setting up static identity map for 0x40100000 - 
>> 0x40100060
>> [    0.130191] rcu: Hierarchical SRCU implementation.
>> [    0.137195] soc soc0: Exynos: CPU[EXYNOS4210] PRO_ID[0x43210211]
>> REV[0x11] Detected
>> [    0.145129] smp: Bringing up secondary CPUs ...
>> [    0.156279] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
>> [    0.156291] CPU1: Spectre v2: using BPIALL workaround
>> [    2.716379] random: fast init done
>
> Thanks for the report. Is this the funky non-banked GIC?

Both Exynos 4210 and 4412 use non-zero cpu-offset in GIC node in 
device-tree: arch/arm/boot/dts/exynos{4210,4412}.dtsi, so I assume that 
the GIC registers are not banked.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 10/16] irqchip/bcm2836: Configure mailbox interrupts as standard interrupts

[Marek Szyprowski]

Hi Marc,

On 01.09.2020 16:43, Marc Zyngier wrote:
> In order to switch the bcm2836 driver to privide standard interrupts
> for IPIs, it first needs to stop lying about the way things work.
>
> The mailbox interrupt is actually a multiplexer, with enough
> bits to store 32 pending interrupts per CPU. So let's turn it
> into a chained irqchip.
>
> Once this is done, we can instanciate the corresponding IPIs,
> and pass them to the architecture code.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

This one also fails. It breaks booting of Raspberry Pi 3b boards (both 
in ARM and ARM64 mode):

NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = (ptrval)
[00000000] *pgd=80000000004003, *pmd=00000000
Internal error: Oops: 80000206 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc4+ #9166
Hardware name: BCM2835
PC is at 0x0
LR is at irq_percpu_enable+0x40/0x50
pc : [<00000000>]    lr : [<c0274638>]    psr: 600000d3
sp : c1201e00  ip : c1201e18  fp : c1201e14
r10: c0ef23dc  r9 : c120583c  r8 : 00000000
r7 : 00000011  r6 : eb032d00  r5 : 00000000  r4 : eb032d00
r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : eb032d18
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment user
Control: 30c5383d  Table: 00003000  DAC: fffffffd
Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
Stack: (0xc1201e00 to 0xc1202000)
...
Backtrace:
[<c02745f8>] (irq_percpu_enable) from [<c0272498>] 
(enable_percpu_irq+0xa4/0xd8)
  r5:c1204ec8 r4:00000000
[<c02723f4>] (enable_percpu_irq) from [<c020ded0>] 
(ipi_setup.part.0+0x3c/0x48)
  r8:c107ba80 r7:00000018 r6:00000011 r5:c1204ee0 r4:00000001
[<c020de94>] (ipi_setup.part.0) from [<c1005684>] 
(set_smp_ipi_range+0xd8/0xf8)
  r5:c1204ee0 r4:00000008
[<c10055ac>] (set_smp_ipi_range) from [<c1023388>] 
(bcm2836_arm_irqchip_l1_intc_of_init+0x1c0/0x22c)
  r8:c1366820 r7:c1204ec8 r6:00000010 r5:00000001 r4:00000000
[<c10231c8>] (bcm2836_arm_irqchip_l1_intc_of_init) from [<c102f28c>] 
(of_irq_init+0x18c/0x2dc)
  r9:00000000 r8:c1201f34 r7:c1204ec8 r6:c1201f2c r5:c1201f2c r4:eb004880
[<c102f100>] (of_irq_init) from [<c1022f4c>] (irqchip_init+0x1c/0x24)
  r10:0000006c r9:00000000 r8:00000000 r7:ffffffff r6:cccccccd r5:c0eb7abe
  r4:c103ba30
[<c1022f30>] (irqchip_init) from [<c1003960>] (init_IRQ+0x30/0x98)
[<c1003930>] (init_IRQ) from [<c1000ebc>] (start_kernel+0x3b4/0x628)
  r5:c0eb7abe r4:c12c1000
[<c1000b08>] (start_kernel) from [<00000000>] (0x0)
  r10:30c5387d r9:410fd034 r8:02600000 r7:00000000 r6:30c0387d r5:00000000
  r4:c1000330
Code: bad PC value
random: get_random_bytes called from init_oops_id+0x30/0x4c with crng_init=0
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Attempted to kill the idle task!
---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---


> ---
>   drivers/irqchip/irq-bcm2836.c | 151 ++++++++++++++++++++++++++++------
>   1 file changed, 125 insertions(+), 26 deletions(-)
>
> ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Marek,

On 2020-09-14 14:26, Marek Szyprowski wrote:
> Hi Marc,
> 
> On 14.09.2020 15:13, Marc Zyngier wrote:
>> On 2020-09-14 14:06, Marek Szyprowski wrote:
>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>> IRQs, and calling into the arch code to register the interrupt range
>>>> instead of a callback.
>>>> 
>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it 
>>> breaks
>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>>> Cortex A9 based). Here are the last lines from the bootlog:
>>> 
>>> [    0.106322] CPU: Testing write buffer coherency: ok
>>> [    0.109895] CPU0: Spectre v2: using BPIALL workaround
>>> [    0.116057] CPU0: thread -1, cpu 0, socket 9, mpidr 80000900
>>> [    0.123885] Setting up static identity map for 0x40100000 -
>>> 0x40100060
>>> [    0.130191] rcu: Hierarchical SRCU implementation.
>>> [    0.137195] soc soc0: Exynos: CPU[EXYNOS4210] PRO_ID[0x43210211]
>>> REV[0x11] Detected
>>> [    0.145129] smp: Bringing up secondary CPUs ...
>>> [    0.156279] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
>>> [    0.156291] CPU1: Spectre v2: using BPIALL workaround
>>> [    2.716379] random: fast init done
>> 
>> Thanks for the report. Is this the funky non-banked GIC?
> 
> Both Exynos 4210 and 4412 use non-zero cpu-offset in GIC node in
> device-tree: arch/arm/boot/dts/exynos{4210,4412}.dtsi, so I assume that
> the GIC registers are not banked.

Annoyingly, it seems to work correctly in QEMU:

root@unassigned-hostname:~# cat /proc/interrupts
            CPU0       CPU1
  40:          0          0     GIC-0  89 Level     mct_comp_irq
  41:      16144          0     GIC-0  74 Level     mct_tick0
  42:          0      15205     GIC-0  80 Level     mct_tick1
  43:          0          0  COMBINER  18 Edge      arm-pmu
  44:          0          0  COMBINER  26 Edge      arm-pmu
  46:       2270          0     GIC-0 107 Level     mmc0
  48:        878          0     GIC-0  84 Level     13800000.serial
  52:          0          0     GIC-0  90 Level     13860000.i2c
  54:          0          0     GIC-0  67 Level     12680000.pdma
  55:          0          0     GIC-0  68 Level     12690000.pdma
  56:          0          0     GIC-0  66 Level     12850000.mdma
  59:          0          0  COMBINER  45 Edge      13620000.sysmmu
  60:          0          0  COMBINER  46 Edge      13630000.sysmmu
  61:          0          0  COMBINER  44 Edge      12e20000.sysmmu
  62:          0          0  COMBINER  34 Edge      11a20000.sysmmu
  63:          0          0  COMBINER  35 Edge      11a30000.sysmmu
  64:          0          0  COMBINER  36 Edge      11a40000.sysmmu
  65:          0          0  COMBINER  37 Edge      11a50000.sysmmu
  66:          0          0  COMBINER  38 Edge      11a60000.sysmmu
  67:          0          0  COMBINER  40 Edge      12a30000.sysmmu
  68:          0          0  COMBINER  42 Edge      11e20000.sysmmu
  74:          0          0     GIC-0  79 Level     11400000.pinctrl
  75:          0          0     GIC-0  78 Level     11000000.pinctrl
  77:          0          0  COMBINER  39 Edge      12a20000.sysmmu
  78:          0          0  COMBINER  43 Edge      12220000.sysmmu
IPI0:          0          1  CPU wakeup interrupts
IPI1:          0          0  Timer broadcast interrupts
IPI2:         32         63  Rescheduling interrupts
IPI3:       3925       5381  Function call interrupts
IPI4:          0          0  CPU stop interrupts
IPI5:       4375       3778  IRQ work interrupts
IPI6:          0          0  completion interrupts
Err:          0

Do you happen to know whether the QEMU emulation is trustworthy?

Thanks,

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

Re: [PATCH v3 10/16] irqchip/bcm2836: Configure mailbox interrupts as standard interrupts

[Marc Zyngier]

Hi Marek,

On 2020-09-14 15:32, Marek Szyprowski wrote:
> Hi Marc,
> 
> On 01.09.2020 16:43, Marc Zyngier wrote:
>> In order to switch the bcm2836 driver to privide standard interrupts
>> for IPIs, it first needs to stop lying about the way things work.
>> 
>> The mailbox interrupt is actually a multiplexer, with enough
>> bits to store 32 pending interrupts per CPU. So let's turn it
>> into a chained irqchip.
>> 
>> Once this is done, we can instanciate the corresponding IPIs,
>> and pass them to the architecture code.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> This one also fails. It breaks booting of Raspberry Pi 3b boards (both
> in ARM and ARM64 mode):

Damn. This used to work. Looks like I was eager to delete stuff at
some point. Can you give this a go and let me know if that works
for you (only tested in QEMU with the raspi2 model):

diff --git a/drivers/irqchip/irq-bcm2836.c 
b/drivers/irqchip/irq-bcm2836.c
index 85df6ddad9be..97838eb705f9 100644
--- a/drivers/irqchip/irq-bcm2836.c
+++ b/drivers/irqchip/irq-bcm2836.c
@@ -193,6 +193,8 @@ static void bcm2836_arm_irqchip_ipi_send_mask(struct 
irq_data *d,

  static struct irq_chip bcm2836_arm_irqchip_ipi = {
  	.name		= "IPI",
+	.irq_mask	= bcm2836_arm_irqchip_dummy_op,
+	.irq_unmask	= bcm2836_arm_irqchip_dummy_op,
  	.irq_eoi	= bcm2836_arm_irqchip_ipi_eoi,
  	.ipi_send_mask	= bcm2836_arm_irqchip_ipi_send_mask,
  };


Thanks again,

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

Re: [PATCH v3 10/16] irqchip/bcm2836: Configure mailbox interrupts as standard interrupts

[Marek Szyprowski]

Hi Marc,

On 14.09.2020 18:10, Marc Zyngier wrote:
> On 2020-09-14 15:32, Marek Szyprowski wrote:
>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>> In order to switch the bcm2836 driver to privide standard interrupts
>>> for IPIs, it first needs to stop lying about the way things work.
>>>
>>> The mailbox interrupt is actually a multiplexer, with enough
>>> bits to store 32 pending interrupts per CPU. So let's turn it
>>> into a chained irqchip.
>>>
>>> Once this is done, we can instanciate the corresponding IPIs,
>>> and pass them to the architecture code.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>
>> This one also fails. It breaks booting of Raspberry Pi 3b boards (both
>> in ARM and ARM64 mode):
>
> Damn. This used to work. Looks like I was eager to delete stuff at
> some point. Can you give this a go and let me know if that works
> for you (only tested in QEMU with the raspi2 model):
>
> diff --git a/drivers/irqchip/irq-bcm2836.c 
> b/drivers/irqchip/irq-bcm2836.c
> index 85df6ddad9be..97838eb705f9 100644
> --- a/drivers/irqchip/irq-bcm2836.c
> +++ b/drivers/irqchip/irq-bcm2836.c
> @@ -193,6 +193,8 @@ static void 
> bcm2836_arm_irqchip_ipi_send_mask(struct irq_data *d,
>
>  static struct irq_chip bcm2836_arm_irqchip_ipi = {
>      .name        = "IPI",
> +    .irq_mask    = bcm2836_arm_irqchip_dummy_op,
> +    .irq_unmask    = bcm2836_arm_irqchip_dummy_op,
>      .irq_eoi    = bcm2836_arm_irqchip_ipi_eoi,
>      .ipi_send_mask    = bcm2836_arm_irqchip_ipi_send_mask,
>  };
>
>
> Thanks again,

This fixes boot on my RPi3b. Thanks!

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marek Szyprowski]

Hi Marc,

On 14.09.2020 17:09, Marc Zyngier wrote:
> On 2020-09-14 14:26, Marek Szyprowski wrote:
>> On 14.09.2020 15:13, Marc Zyngier wrote:
>>> On 2020-09-14 14:06, Marek Szyprowski wrote:
>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>> IRQs, and calling into the arch code to register the interrupt range
>>>>> instead of a callback.
>>>>>
>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it 
>>>> breaks
>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>>>
>>>> [    0.106322] CPU: Testing write buffer coherency: ok
>>>> [    0.109895] CPU0: Spectre v2: using BPIALL workaround
>>>> [    0.116057] CPU0: thread -1, cpu 0, socket 9, mpidr 80000900
>>>> [    0.123885] Setting up static identity map for 0x40100000 -
>>>> 0x40100060
>>>> [    0.130191] rcu: Hierarchical SRCU implementation.
>>>> [    0.137195] soc soc0: Exynos: CPU[EXYNOS4210] PRO_ID[0x43210211]
>>>> REV[0x11] Detected
>>>> [    0.145129] smp: Bringing up secondary CPUs ...
>>>> [    0.156279] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
>>>> [    0.156291] CPU1: Spectre v2: using BPIALL workaround
>>>> [    2.716379] random: fast init done
>>>
>>> Thanks for the report. Is this the funky non-banked GIC?
>>
>> Both Exynos 4210 and 4412 use non-zero cpu-offset in GIC node in
>> device-tree: arch/arm/boot/dts/exynos{4210,4412}.dtsi, so I assume that
>> the GIC registers are not banked.
>
> Annoyingly, it seems to work correctly in QEMU:
>
> root@unassigned-hostname:~# cat /proc/interrupts
>            CPU0       CPU1
>  40:          0          0     GIC-0  89 Level     mct_comp_irq
>  41:      16144          0     GIC-0  74 Level     mct_tick0
>  42:          0      15205     GIC-0  80 Level     mct_tick1
>  43:          0          0  COMBINER  18 Edge      arm-pmu
>  44:          0          0  COMBINER  26 Edge      arm-pmu
>  46:       2270          0     GIC-0 107 Level     mmc0
>  48:        878          0     GIC-0  84 Level     13800000.serial
>  52:          0          0     GIC-0  90 Level     13860000.i2c
>  54:          0          0     GIC-0  67 Level     12680000.pdma
>  55:          0          0     GIC-0  68 Level     12690000.pdma
>  56:          0          0     GIC-0  66 Level     12850000.mdma
>  59:          0          0  COMBINER  45 Edge      13620000.sysmmu
>  60:          0          0  COMBINER  46 Edge      13630000.sysmmu
>  61:          0          0  COMBINER  44 Edge      12e20000.sysmmu
>  62:          0          0  COMBINER  34 Edge      11a20000.sysmmu
>  63:          0          0  COMBINER  35 Edge      11a30000.sysmmu
>  64:          0          0  COMBINER  36 Edge      11a40000.sysmmu
>  65:          0          0  COMBINER  37 Edge      11a50000.sysmmu
>  66:          0          0  COMBINER  38 Edge      11a60000.sysmmu
>  67:          0          0  COMBINER  40 Edge      12a30000.sysmmu
>  68:          0          0  COMBINER  42 Edge      11e20000.sysmmu
>  74:          0          0     GIC-0  79 Level 11400000.pinctrl
>  75:          0          0     GIC-0  78 Level 11000000.pinctrl
>  77:          0          0  COMBINER  39 Edge      12a20000.sysmmu
>  78:          0          0  COMBINER  43 Edge      12220000.sysmmu
> IPI0:          0          1  CPU wakeup interrupts
> IPI1:          0          0  Timer broadcast interrupts
> IPI2:         32         63  Rescheduling interrupts
> IPI3:       3925       5381  Function call interrupts
> IPI4:          0          0  CPU stop interrupts
> IPI5:       4375       3778  IRQ work interrupts
> IPI6:          0          0  completion interrupts
> Err:          0
>
> Do you happen to know whether the QEMU emulation is trustworthy?

I didn't play much with Exynos emulation on QEMU. All I know is that 
this patch simply doesn't work on the real hw.

If there is anything to check or test, let me know. I will try to help 
as much as possible.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Hi Marek,

On 2020-09-15 07:48, Marek Szyprowski wrote:
> Hi Marc,
> 
>>> Both Exynos 4210 and 4412 use non-zero cpu-offset in GIC node in
>>> device-tree: arch/arm/boot/dts/exynos{4210,4412}.dtsi, so I assume 
>>> that
>>> the GIC registers are not banked.
>> 
>> Annoyingly, it seems to work correctly in QEMU:

[...]

>> Do you happen to know whether the QEMU emulation is trustworthy?
> 
> I didn't play much with Exynos emulation on QEMU. All I know is that
> this patch simply doesn't work on the real hw.

I don't doubt it. The question was more whether we could trust QEMU
to be reliable, in which case the issue would be around a kernel
configuration problem. Could you stash your kernel config somewhere?

> If there is anything to check or test, let me know. I will try to help
> as much as possible.

It would be interesting to see whether the CPUs are getting any IPI.
Can you try the following patch, and send the results back?

Thanks,

         M.

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 00327fa74b01..5b01d53de9af 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -420,7 +420,7 @@ asmlinkage void secondary_start_kernel(void)
  #ifndef CONFIG_MMU
  	setup_vectors_base();
  #endif
-	pr_debug("CPU%u: Booted secondary processor\n", cpu);
+	pr_err("CPU%u: Booted secondary processor\n", cpu);

  	preempt_disable();
  	trace_hardirqs_off();
@@ -621,6 +621,8 @@ static void do_handle_IPI(int ipinr)
  {
  	unsigned int cpu = smp_processor_id();

+	pr_info("CPU%d IPI%d received\n", cpu, ipinr);
+
  	if ((unsigned)ipinr < NR_IPI)
  		trace_ipi_entry_rcuidle(ipi_types[ipinr]);

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index d7321ccf730f..7723cad6e406 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -190,6 +190,8 @@ static inline bool cascading_gic_irq(struct irq_data 
*d)
  static void gic_poke_irq(struct irq_data *d, u32 offset)
  {
  	u32 mask = 1 << (gic_irq(d) % 32);
+	if (gic_irq(d) < 16)
+		pr_info("CPU%d IPI%lu base = %lx\n", smp_processor_id(), d->hwirq, 
(unsigned long)gic_dist_base(d));
  	writel_relaxed(mask, gic_dist_base(d) + offset + (gic_irq(d) / 32) * 
4);
  }

@@ -814,6 +816,7 @@ static void gic_ipi_send_mask(struct irq_data *d, 
const struct cpumask *mask)
  	 */
  	dmb(ishst);

+	pr_info("CPU%d send IPI%lu base = %lx\n", smp_processor_id(), 
d->hwirq, (unsigned long)gic_data_dist_base(&gic_data[0]));
  	/* this always happens on GIC0 */
  	writel_relaxed(map << 16 | d->hwirq, gic_data_dist_base(&gic_data[0]) 
+ GIC_DIST_SOFTINT);


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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marek Szyprowski]

Hi Marc,

On 15.09.2020 10:07, Marc Zyngier wrote:
> On 2020-09-15 07:48, Marek Szyprowski wrote:
>>>> Both Exynos 4210 and 4412 use non-zero cpu-offset in GIC node in
>>>> device-tree: arch/arm/boot/dts/exynos{4210,4412}.dtsi, so I assume 
>>>> that
>>>> the GIC registers are not banked.
>>>
>>> Annoyingly, it seems to work correctly in QEMU:
>
> [...]
>
>>> Do you happen to know whether the QEMU emulation is trustworthy?
>>
>> I didn't play much with Exynos emulation on QEMU. All I know is that
>> this patch simply doesn't work on the real hw.
>
> I don't doubt it. The question was more whether we could trust QEMU
> to be reliable, in which case the issue would be around a kernel
> configuration problem. Could you stash your kernel config somewhere?

I just use the vanilla exynos_defconfig for my tests.

>> If there is anything to check or test, let me know. I will try to help
>> as much as possible.
>
> It would be interesting to see whether the CPUs are getting any IPI.
> Can you try the following patch, and send the results back?

Starting kernel ...

[    0.000000] Booting Linux on physical CPU 0x900
[    0.000000] Linux version 5.9.0-rc4-00008-gac063232d4b0-dirty 
(mszyprow@AMDC2765) (arm-linux-gnueabi-gcc (Linaro GCC 4.9-2017.01) 
4.9.4, GNU ld (Linaro_Binutils-2017.01) 2.24.0.20141017 Linaro 
2014_11-3-git) #9174 SMP PREEMPT Tue Sep 15 10:30:46 CEST 2020
[    0.000000] CPU: ARMv7 Processor [412fc091] revision 1 (ARMv7), 
cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing 
instruction cache
[    0.000000] OF: fdt: Machine model: Samsung Trats based on Exynos4210
[    0.000000] earlycon: exynos4210 at MMIO 0x13820000 (options '115200n8')
[    0.000000] printk: bootconsole [exynos4210] enabled
[    0.000000] Memory policy: Data cache writealloc
[    0.000000] cma: Reserved 96 MiB at 0x7a000000
[    0.000000] Samsung CPU ID: 0x43210211
[    0.000000] Zone ranges:
[    0.000000]   Normal   [mem 0x0000000040000000-0x000000006fffffff]
[    0.000000]   HighMem  [mem 0x0000000070000000-0x000000007fffffff]
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000040000000-0x000000007fffffff]
[    0.000000] Initmem setup node 0 [mem 
0x0000000040000000-0x000000007fffffff]
[    0.000000] percpu: Embedded 20 pages/cpu s51904 r8192 d21824 u81920
[    0.000000] Built 1 zonelists, mobility grouping on.  Total pages: 260608
[    0.000000] Kernel command line: root=PARTLABEL=data rootwait 
console=tty1 console=ttySAC2,115200n8 earlycon
[    0.000000] Dentry cache hash table entries: 131072 (order: 7, 524288 
bytes, linear)
[    0.000000] Inode-cache hash table entries: 65536 (order: 6, 262144 
bytes, linear)
[    0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[    0.000000] Memory: 917072K/1048576K available (10240K kernel code, 
958K rwdata, 4000K rodata, 1024K init, 6487K bss, 33200K reserved, 
98304K cma-reserved, 163840K highmem)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
[    0.000000] Running RCU self tests
[    0.000000] rcu: Preemptible hierarchical RCU implementation.
[    0.000000] rcu:     RCU event tracing is enabled.
[    0.000000] rcu:     RCU lockdep checking is enabled.
[    0.000000] rcu:     RCU restricting CPUs from NR_CPUS=8 to nr_cpu_ids=2.
[    0.000000]  Trampoline variant of Tasks RCU enabled.
[    0.000000] rcu: RCU calculated value of scheduler-enlistment delay 
is 10 jiffies.
[    0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=2
[    0.000000] NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
[    0.000000] CPU0 IPI0 base = f0800000
[    0.000000] CPU0 IPI1 base = f0800000
[    0.000000] CPU0 IPI2 base = f0800000
[    0.000000] CPU0 IPI3 base = f0800000
[    0.000000] CPU0 IPI4 base = f0800000
[    0.000000] CPU0 IPI5 base = f0800000
[    0.000000] CPU0 IPI6 base = f0800000
[    0.000000] CPU0 IPI7 base = f0800000
[    0.000000] L2C: platform modifies aux control register: 0x02070000 
-> 0x3e470000
[    0.000000] L2C: DT/platform modifies aux control register: 
0x02070000 -> 0x3e470000
[    0.000000] L2C-310 enabling early BRESP for Cortex-A9
[    0.000000] L2C-310 full line of zeros enabled for Cortex-A9
[    0.000000] L2C-310 dynamic clock gating enabled, standby mode enabled
[    0.000000] L2C-310 cache controller enabled, 16 ways, 1024 kB
[    0.000000] L2C-310: CACHE_ID 0x4100c4c5, AUX_CTRL 0x4e470001
[    0.000000] random: get_random_bytes called from 
start_kernel+0x4c0/0x67c with crng_init=0
[    0.000000] Exynos4210 clocks: sclk_apll = 800000000, sclk_mpll = 
800000000
[    0.000000]  sclk_epll = 96000000, sclk_vpll = 108000000, arm_clk = 
800000000
[    0.000000] Switching to timer-based delay loop, resolution 41ns
[    0.000000] clocksource: mct-frc: mask: 0xffffffff max_cycles: 
0xffffffff, max_idle_ns: 79635851949 ns
[    0.000007] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps 
every 89478484971ns
[    0.008850] Console: colour dummy device 80x30
[    0.017306] printk: console [tty1] enabled
[    0.019992] Lock dependency validator: Copyright (c) 2006 Red Hat, 
Inc., Ingo Molnar
[    0.027791] ... MAX_LOCKDEP_SUBCLASSES:  8
[    0.031793] ... MAX_LOCK_DEPTH:          48
[    0.035959] ... MAX_LOCKDEP_KEYS:        8192
[    0.040327] ... CLASSHASH_SIZE:          4096
[    0.044643] ... MAX_LOCKDEP_ENTRIES:     32768
[    0.049096] ... MAX_LOCKDEP_CHAINS:      65536
[    0.053497] ... CHAINHASH_SIZE:          32768
[    0.057948]  memory used by lock dependency info: 4029 kB
[    0.063318]  memory used for stack traces: 2112 kB
[    0.068110]  per task-struct memory footprint: 1536 bytes
[    0.073536] Calibrating delay loop (skipped), value calculated using 
timer frequency.. 48.00 BogoMIPS (lpj=240000)
[    0.083901] pid_max: default: 32768 minimum: 301
[    0.088789] Mount-cache hash table entries: 2048 (order: 1, 8192 
bytes, linear)
[    0.095733] Mountpoint-cache hash table entries: 2048 (order: 1, 8192 
bytes, linear)
[    0.106706] CPU: Testing write buffer coherency: ok
[    0.110276] CPU0: Spectre v2: using BPIALL workaround
[    0.116474] CPU0: thread -1, cpu 0, socket 9, mpidr 80000900
[    0.124225] Setting up static identity map for 0x40100000 - 0x40100060
[    0.130539] rcu: Hierarchical SRCU implementation.
[    0.137562] soc soc0: Exynos: CPU[EXYNOS4210] PRO_ID[0x43210211] 
REV[0x11] Detected
[    0.145493] smp: Bringing up secondary CPUs ...
[    0.152740] CPU0 send IPI0 base = f0800000
[    0.152786] CPU1: Booted secondary processor
[    0.155582] CPU0 send IPI0 base = f0800000
[    0.163945] CPU1 IPI0 base = f0808000
[    0.163956] CPU1 IPI1 base = f0808000
[    0.163966] CPU1 IPI2 base = f0808000
[    0.163976] CPU1 IPI3 base = f0808000
[    0.163986] CPU1 IPI4 base = f0808000
[    0.163995] CPU1 IPI5 base = f0808000
[    0.164004] CPU1 IPI6 base = f0808000
[    0.164014] CPU1 IPI7 base = f0808000
[    0.164025] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
[    0.164035] CPU1: Spectre v2: using BPIALL workaround
[    0.203803] CPU1 send IPI2 base = f0808000
[    0.207834] CPU1 IPI0 received
[    0.207839] CPU0 IPI2 received
[    0.214052] CPU0 send IPI2 base = f0800000
[    0.217990] CPU1 IPI2 received
[    0.222188] CPU1 send IPI2 base = f0808000
[    2.754062] random: fast init done

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-15 09:35, Marek Szyprowski wrote:
> Hi Marc,
> 
> On 15.09.2020 10:07, Marc Zyngier wrote:
>> On 2020-09-15 07:48, Marek Szyprowski wrote:
>>>>> Both Exynos 4210 and 4412 use non-zero cpu-offset in GIC node in
>>>>> device-tree: arch/arm/boot/dts/exynos{4210,4412}.dtsi, so I assume
>>>>> that
>>>>> the GIC registers are not banked.
>>>> 
>>>> Annoyingly, it seems to work correctly in QEMU:
>> 
>> [...]
>> 
>>>> Do you happen to know whether the QEMU emulation is trustworthy?
>>> 
>>> I didn't play much with Exynos emulation on QEMU. All I know is that
>>> this patch simply doesn't work on the real hw.
>> 
>> I don't doubt it. The question was more whether we could trust QEMU
>> to be reliable, in which case the issue would be around a kernel
>> configuration problem. Could you stash your kernel config somewhere?
> 
> I just use the vanilla exynos_defconfig for my tests.

Tried that with QEMU, same result. It keeps working. Oh well.

> 
>>> If there is anything to check or test, let me know. I will try to 
>>> help
>>> as much as possible.
>> 
>> It would be interesting to see whether the CPUs are getting any IPI.
>> Can you try the following patch, and send the results back?

[...]

> [    0.145493] smp: Bringing up secondary CPUs ...
> [    0.152740] CPU0 send IPI0 base = f0800000
> [    0.152786] CPU1: Booted secondary processor
> [    0.155582] CPU0 send IPI0 base = f0800000
> [    0.163945] CPU1 IPI0 base = f0808000
> [    0.163956] CPU1 IPI1 base = f0808000
> [    0.163966] CPU1 IPI2 base = f0808000
> [    0.163976] CPU1 IPI3 base = f0808000
> [    0.163986] CPU1 IPI4 base = f0808000
> [    0.163995] CPU1 IPI5 base = f0808000
> [    0.164004] CPU1 IPI6 base = f0808000
> [    0.164014] CPU1 IPI7 base = f0808000
> [    0.164025] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
> [    0.164035] CPU1: Spectre v2: using BPIALL workaround
> [    0.203803] CPU1 send IPI2 base = f0808000
> [    0.207834] CPU1 IPI0 received
> [    0.207839] CPU0 IPI2 received
> [    0.214052] CPU0 send IPI2 base = f0800000
> [    0.217990] CPU1 IPI2 received
> [    0.222188] CPU1 send IPI2 base = f0808000
> [    2.754062] random: fast init done

So IPIs *do work* for some time, but CPU0 ends up not seeing IPI2.
I see a slightly different behaviour in QEMU:

[    0.555590] smp: Bringing up secondary CPUs ...
[    0.606032] CPU0 send IPI0 base = f0800000
[    0.609149] CPU0 send IPI0 base = f0800000
[    0.610329] CPU0 send IPI0 base = f0800000
[    0.611445] CPU0 send IPI0 base = f0800000
[    0.611588] CPU1: Booted secondary processor
[    0.613579] CPU0 send IPI0 base = f0800000
[    0.616180] CPU1 IPI0 base = f0808000
[    0.616470] CPU1 IPI1 base = f0808000
[    0.616634] CPU1 IPI2 base = f0808000
[    0.616781] CPU1 IPI3 base = f0808000
[    0.616931] CPU1 IPI4 base = f0808000
[    0.617074] CPU1 IPI5 base = f0808000
[    0.617220] CPU1 IPI6 base = f0808000
[    0.617366] CPU1 IPI7 base = f0808000
[    0.617824] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
[    0.618115] CPU1: Spectre v2: using BPIALL workaround
[    0.627969] CPU1 send IPI3 base = f0808000
[    0.631301] CPU0 IPI3 received
[    0.631389] CPU1 IPI0 received
[    0.639726] CPU0 send IPI2 base = f0800000
[    0.641632] CPU1 IPI2 received
[    0.664666] CPU1 send IPI2 base = f0808000
[    0.665987] CPU0 IPI2 received
[    0.670718] smp: Brought up 1 node, 2 CPUs
[    0.672175] SMP: Total of 2 processors activated (48.00 BogoMIPS).
[    0.674071] CPU: All CPU(s) started in SVC mode.

where the secondary starts by sending IPI3 (IPI_CALL_FUNC). Not sure it
matters.

The fact that CPU0 doesn't process the second IPI2 makes me wonder
if there is something flawed in the EOI logic.

Can you try applying this patch, which reverts that particular logic?
If that happens to work, we'll have to investigate what comes out
of the IAR register...

Otherwise, we'll keep reverting bits of the patch until we nail it...

Thanks,

         M.

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4be2b62f816f..6daf2de7233a 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -335,22 +335,31 @@ static void __exception_irq_entry 
gic_handle_irq(struct pt_regs *regs)
  		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
  		irqnr = irqstat & GICC_IAR_INT_ID_MASK;

-		if (unlikely(irqnr >= 1020))
-			break;
-
-		if (static_branch_likely(&supports_deactivate_key))
+		if (likely(irqnr > 15 && irqnr < 1020)) {
+			if (static_branch_likely(&supports_deactivate_key))
+				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
+			isb();
+			handle_domain_irq(gic->domain, irqnr, regs);
+			continue;
+		}
+		if (irqnr < 16) {
  			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
-		isb();
-
-		/*
-		 * Ensure any shared data written by the CPU sending the IPI
-		 * is read after we've read the ACK register on the GIC.
-		 *
-		 * Pairs with the write barrier in gic_ipi_send_mask
-		 */
-		if (irqnr <= 15)
+			if (static_branch_likely(&supports_deactivate_key))
+				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
+#ifdef CONFIG_SMP
+			/*
+			 * Ensure any shared data written by the CPU sending
+			 * the IPI is read after we've read the ACK register
+			 * on the GIC.
+			 *
+			 * Pairs with the write barrier in gic_raise_softirq
+			 */
  			smp_rmb();
-		handle_domain_irq(gic->domain, irqnr, regs);
+			handle_IPI(irqnr, regs);
+#endif
+			continue;
+		}
+		break;
  	} while (1);
  }


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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Linus Walleij]

On Tue, Sep 1, 2020 at 4:44 PM Marc Zyngier <maz@kernel.org> wrote:

> Change the way we deal with GIC SGIs by turning them into proper
> IRQs, and calling into the arch code to register the interrupt range
> instead of a callback.
>
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Hmmm apart from Exynos this crashes the Ux500 too... I don't
even get the crash dumpon a LL UART, it hard hangs.

Yours,
Linus Walleij

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Hi Linus,

On 2020-09-16 15:03, Linus Walleij wrote:
> On Tue, Sep 1, 2020 at 4:44 PM Marc Zyngier <maz@kernel.org> wrote:
> 
>> Change the way we deal with GIC SGIs by turning them into proper
>> IRQs, and calling into the arch code to register the interrupt range
>> instead of a callback.
>> 
>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> Hmmm apart from Exynos this crashes the Ux500 too... I don't
> even get the crash dumpon a LL UART, it hard hangs.

No output whatsoever? That's really odd... Or do you see the kernel
booting and locking up when starting secondaries? If the latter, it
would indicate that we haven't properly deactivated the SGI...

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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

Hi Marc,

On 14/09/2020 14:06, Marek Szyprowski wrote:
> Hi Marc,
> 
> On 01.09.2020 16:43, Marc Zyngier wrote:
>> Change the way we deal with GIC SGIs by turning them into proper
>> IRQs, and calling into the arch code to register the interrupt range
>> instead of a callback.
>>
>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> This patch landed in linux next-20200914 as commit ac063232d4b0 
> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it breaks 
> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM 
> Cortex A9 based). Here are the last lines from the bootlog:

I am observing the same thing on several Tegra boards (both arm and
arm64). Bisect is pointing to this commit. Reverting this alone does not
appear to be enough to fix the issue.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Hi Jon,

+Linus, who is facing a similar issue.

On 2020-09-16 15:16, Jon Hunter wrote:
> Hi Marc,
> 
> On 14/09/2020 14:06, Marek Szyprowski wrote:
>> Hi Marc,
>> 
>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>> Change the way we deal with GIC SGIs by turning them into proper
>>> IRQs, and calling into the arch code to register the interrupt range
>>> instead of a callback.
>>> 
>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> This patch landed in linux next-20200914 as commit ac063232d4b0
>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it 
>> breaks
>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>> Cortex A9 based). Here are the last lines from the bootlog:
> 
> I am observing the same thing on several Tegra boards (both arm and
> arm64). Bisect is pointing to this commit. Reverting this alone does 
> not
> appear to be enough to fix the issue.

Right, I am just massively by the GICv3 spec, and failed to remember
that ye olde GIC exposes the source CPU in AIR *and* wants it back, 
while
newer GICs deal with that transparently.

Can you try the patch below and let me know?

         M.

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 98743afdaea6..56492bf8b6f9 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -121,9 +121,10 @@ static struct gic_chip_data 
gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;

  static struct gic_kvm_info gic_v2_kvm_info;

+static DEFINE_PER_CPU(u32, sgi_intid);
+
  #ifdef CONFIG_GIC_NON_BANKED
  static DEFINE_STATIC_KEY_FALSE(frankengic_key);
-static DEFINE_PER_CPU(u32, sgi_intid);

  static void enable_frankengic(void)
  {
@@ -135,16 +136,6 @@ static inline bool is_frankengic(void)
  	return static_branch_unlikely(&frankengic_key);
  }

-static inline void set_sgi_intid(u32 intid)
-{
-	this_cpu_write(sgi_intid, intid);
-}
-
-static inline u32 get_sgi_intid(void)
-{
-	return this_cpu_read(sgi_intid);
-}
-
  static inline void __iomem *__get_base(union gic_base *base)
  {
  	if (is_frankengic())
@@ -160,8 +151,6 @@ static inline void __iomem *__get_base(union 
gic_base *base)
  #define gic_data_cpu_base(d)	((d)->cpu_base.common_base)
  #define enable_frankengic()	do { } while(0)
  #define is_frankengic()		false
-#define set_sgi_intid(i)	do { } while(0)
-#define get_sgi_intid()		0
  #endif

  static inline void __iomem *gic_dist_base(struct irq_data *d)
@@ -236,8 +225,8 @@ static void gic_eoi_irq(struct irq_data *d)
  {
  	u32 hwirq = gic_irq(d);

-	if (is_frankengic() && hwirq < 16)
-		hwirq = get_sgi_intid();
+	if (hwirq < 16)
+		hwirq = this_cpu_read(sgi_intid);

  	writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_EOI);
  }
@@ -365,14 +354,13 @@ static void __exception_irq_entry 
gic_handle_irq(struct pt_regs *regs)
  			smp_rmb();

  			/*
-			 * Samsung's funky GIC encodes the source CPU in
-			 * GICC_IAR, leading to the deactivation to fail if
-			 * not written back as is to GICC_EOI.  Stash the
-			 * INTID away for gic_eoi_irq() to write back.
-			 * This only works because we don't nest SGIs...
+			 * The GIC encodes the source CPU in GICC_IAR,
+			 * leading to the deactivation to fail if not
+			 * written back as is to GICC_EOI.  Stash the INTID
+			 * away for gic_eoi_irq() to write back.  This only
+			 * works because we don't nest SGIs...
  			 */
-			if (is_frankengic())
-				set_sgi_intid(irqstat);
+			this_cpu_write(sgi_intid, intid);
  		}

  		handle_domain_irq(gic->domain, irqnr, regs);

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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 16:10, Marc Zyngier wrote:
> Hi Jon,
> 
> +Linus, who is facing a similar issue.
> 
> On 2020-09-16 15:16, Jon Hunter wrote:
>> Hi Marc,
>>
>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>> Hi Marc,
>>>
>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>> IRQs, and calling into the arch code to register the interrupt range
>>>> instead of a callback.
>>>>
>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it breaks
>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>>> Cortex A9 based). Here are the last lines from the bootlog:
>>
>> I am observing the same thing on several Tegra boards (both arm and
>> arm64). Bisect is pointing to this commit. Reverting this alone does not
>> appear to be enough to fix the issue.
> 
> Right, I am just massively by the GICv3 spec, and failed to remember
> that ye olde GIC exposes the source CPU in AIR *and* wants it back, while
> newer GICs deal with that transparently.
> 
> Can you try the patch below and let me know?

Yes will do.

> @@ -365,14 +354,13 @@ static void __exception_irq_entry
> gic_handle_irq(struct pt_regs *regs)
>              smp_rmb();
> 
>              /*
> -             * Samsung's funky GIC encodes the source CPU in
> -             * GICC_IAR, leading to the deactivation to fail if
> -             * not written back as is to GICC_EOI.  Stash the
> -             * INTID away for gic_eoi_irq() to write back.
> -             * This only works because we don't nest SGIs...
> +             * The GIC encodes the source CPU in GICC_IAR,
> +             * leading to the deactivation to fail if not
> +             * written back as is to GICC_EOI.  Stash the INTID
> +             * away for gic_eoi_irq() to write back.  This only
> +             * works because we don't nest SGIs...
>               */
> -            if (is_frankengic())
> -                set_sgi_intid(irqstat);
> +            this_cpu_write(sgi_intid, intid);

I assume that it should be irqstat here and not intid?

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-16 16:46, Jon Hunter wrote:
> On 16/09/2020 16:10, Marc Zyngier wrote:
>> Hi Jon,
>> 
>> +Linus, who is facing a similar issue.
>> 
>> On 2020-09-16 15:16, Jon Hunter wrote:
>>> Hi Marc,
>>> 
>>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>>> Hi Marc,
>>>> 
>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>> IRQs, and calling into the arch code to register the interrupt 
>>>>> range
>>>>> instead of a callback.
>>>>> 
>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it 
>>>> breaks
>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>> 
>>> I am observing the same thing on several Tegra boards (both arm and
>>> arm64). Bisect is pointing to this commit. Reverting this alone does 
>>> not
>>> appear to be enough to fix the issue.
>> 
>> Right, I am just massively by the GICv3 spec, and failed to remember
>> that ye olde GIC exposes the source CPU in AIR *and* wants it back, 
>> while
>> newer GICs deal with that transparently.
>> 
>> Can you try the patch below and let me know?
> 
> Yes will do.
> 
>> @@ -365,14 +354,13 @@ static void __exception_irq_entry
>> gic_handle_irq(struct pt_regs *regs)
>>              smp_rmb();
>> 
>>              /*
>> -             * Samsung's funky GIC encodes the source CPU in
>> -             * GICC_IAR, leading to the deactivation to fail if
>> -             * not written back as is to GICC_EOI.  Stash the
>> -             * INTID away for gic_eoi_irq() to write back.
>> -             * This only works because we don't nest SGIs...
>> +             * The GIC encodes the source CPU in GICC_IAR,
>> +             * leading to the deactivation to fail if not
>> +             * written back as is to GICC_EOI.  Stash the INTID
>> +             * away for gic_eoi_irq() to write back.  This only
>> +             * works because we don't nest SGIs...
>>               */
>> -            if (is_frankengic())
>> -                set_sgi_intid(irqstat);
>> +            this_cpu_write(sgi_intid, intid);
> 
> I assume that it should be irqstat here and not intid?

Indeed. As you can tell, I haven't even tried to compile it, sorry about 
that.

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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 16:55, Marc Zyngier wrote:
> On 2020-09-16 16:46, Jon Hunter wrote:
>> On 16/09/2020 16:10, Marc Zyngier wrote:
>>> Hi Jon,
>>>
>>> +Linus, who is facing a similar issue.
>>>
>>> On 2020-09-16 15:16, Jon Hunter wrote:
>>>> Hi Marc,
>>>>
>>>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>>>> Hi Marc,
>>>>>
>>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>>> IRQs, and calling into the arch code to register the interrupt range
>>>>>> instead of a callback.
>>>>>>
>>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it
>>>>> breaks
>>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad ARM
>>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>>>
>>>> I am observing the same thing on several Tegra boards (both arm and
>>>> arm64). Bisect is pointing to this commit. Reverting this alone does
>>>> not
>>>> appear to be enough to fix the issue.
>>>
>>> Right, I am just massively by the GICv3 spec, and failed to remember
>>> that ye olde GIC exposes the source CPU in AIR *and* wants it back,
>>> while
>>> newer GICs deal with that transparently.
>>>
>>> Can you try the patch below and let me know?
>>
>> Yes will do.
>>
>>> @@ -365,14 +354,13 @@ static void __exception_irq_entry
>>> gic_handle_irq(struct pt_regs *regs)
>>>              smp_rmb();
>>>
>>>              /*
>>> -             * Samsung's funky GIC encodes the source CPU in
>>> -             * GICC_IAR, leading to the deactivation to fail if
>>> -             * not written back as is to GICC_EOI.  Stash the
>>> -             * INTID away for gic_eoi_irq() to write back.
>>> -             * This only works because we don't nest SGIs...
>>> +             * The GIC encodes the source CPU in GICC_IAR,
>>> +             * leading to the deactivation to fail if not
>>> +             * written back as is to GICC_EOI.  Stash the INTID
>>> +             * away for gic_eoi_irq() to write back.  This only
>>> +             * works because we don't nest SGIs...
>>>               */
>>> -            if (is_frankengic())
>>> -                set_sgi_intid(irqstat);
>>> +            this_cpu_write(sgi_intid, intid);
>>
>> I assume that it should be irqstat here and not intid?
> 
> Indeed. As you can tell, I haven't even tried to compile it, sorry about
> that.

No worries, I got the gist. However, even with this change, it still
does not boot :-(

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-16 16:58, Jon Hunter wrote:
> On 16/09/2020 16:55, Marc Zyngier wrote:
>> On 2020-09-16 16:46, Jon Hunter wrote:
>>> On 16/09/2020 16:10, Marc Zyngier wrote:
>>>> Hi Jon,
>>>> 
>>>> +Linus, who is facing a similar issue.
>>>> 
>>>> On 2020-09-16 15:16, Jon Hunter wrote:
>>>>> Hi Marc,
>>>>> 
>>>>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>>>>> Hi Marc,
>>>>>> 
>>>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>>>> IRQs, and calling into the arch code to register the interrupt 
>>>>>>> range
>>>>>>> instead of a callback.
>>>>>>> 
>>>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it
>>>>>> breaks
>>>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad 
>>>>>> ARM
>>>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>>>> 
>>>>> I am observing the same thing on several Tegra boards (both arm and
>>>>> arm64). Bisect is pointing to this commit. Reverting this alone 
>>>>> does
>>>>> not
>>>>> appear to be enough to fix the issue.
>>>> 
>>>> Right, I am just massively by the GICv3 spec, and failed to remember
>>>> that ye olde GIC exposes the source CPU in AIR *and* wants it back,
>>>> while
>>>> newer GICs deal with that transparently.
>>>> 
>>>> Can you try the patch below and let me know?
>>> 
>>> Yes will do.
>>> 
>>>> @@ -365,14 +354,13 @@ static void __exception_irq_entry
>>>> gic_handle_irq(struct pt_regs *regs)
>>>>              smp_rmb();
>>>> 
>>>>              /*
>>>> -             * Samsung's funky GIC encodes the source CPU in
>>>> -             * GICC_IAR, leading to the deactivation to fail if
>>>> -             * not written back as is to GICC_EOI.  Stash the
>>>> -             * INTID away for gic_eoi_irq() to write back.
>>>> -             * This only works because we don't nest SGIs...
>>>> +             * The GIC encodes the source CPU in GICC_IAR,
>>>> +             * leading to the deactivation to fail if not
>>>> +             * written back as is to GICC_EOI.  Stash the INTID
>>>> +             * away for gic_eoi_irq() to write back.  This only
>>>> +             * works because we don't nest SGIs...
>>>>               */
>>>> -            if (is_frankengic())
>>>> -                set_sgi_intid(irqstat);
>>>> +            this_cpu_write(sgi_intid, intid);
>>> 
>>> I assume that it should be irqstat here and not intid?
>> 
>> Indeed. As you can tell, I haven't even tried to compile it, sorry 
>> about
>> that.
> 
> No worries, I got the gist. However, even with this change, it still
> does not boot :-(

Do you boot form EL2? If so, you'd also need this:

  static void gic_eoimode1_eoi_irq(struct irq_data *d)
  {
+	u32 hwirq = gic_irq(d);
+
  	/* Do not deactivate an IRQ forwarded to a vcpu. */
  	if (irqd_is_forwarded_to_vcpu(d))
  		return;

+	if (hwirq < 16)
+		hwirq = this_cpu_read(sgi_intid);
+
  	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
  }

If none of that works, we'll need some additional traces. On the other
hand, I just booted this on a GICv2-based system, and it worked fine...

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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-16 17:22, Marc Zyngier wrote:
> On 2020-09-16 16:58, Jon Hunter wrote:
>> On 16/09/2020 16:55, Marc Zyngier wrote:
>>> On 2020-09-16 16:46, Jon Hunter wrote:
>>>> On 16/09/2020 16:10, Marc Zyngier wrote:
>>>>> Hi Jon,
>>>>> 
>>>>> +Linus, who is facing a similar issue.
>>>>> 
>>>>> On 2020-09-16 15:16, Jon Hunter wrote:
>>>>>> Hi Marc,
>>>>>> 
>>>>>> On 14/09/2020 14:06, Marek Szyprowski wrote:
>>>>>>> Hi Marc,
>>>>>>> 
>>>>>>> On 01.09.2020 16:43, Marc Zyngier wrote:
>>>>>>>> Change the way we deal with GIC SGIs by turning them into proper
>>>>>>>> IRQs, and calling into the arch code to register the interrupt 
>>>>>>>> range
>>>>>>>> instead of a callback.
>>>>>>>> 
>>>>>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>>>> This patch landed in linux next-20200914 as commit ac063232d4b0
>>>>>>> ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it
>>>>>>> breaks
>>>>>>> booting of all Samsung Exynos 4210/4412 based boards (dual/quad 
>>>>>>> ARM
>>>>>>> Cortex A9 based). Here are the last lines from the bootlog:
>>>>>> 
>>>>>> I am observing the same thing on several Tegra boards (both arm 
>>>>>> and
>>>>>> arm64). Bisect is pointing to this commit. Reverting this alone 
>>>>>> does
>>>>>> not
>>>>>> appear to be enough to fix the issue.
>>>>> 
>>>>> Right, I am just massively by the GICv3 spec, and failed to 
>>>>> remember
>>>>> that ye olde GIC exposes the source CPU in AIR *and* wants it back,
>>>>> while
>>>>> newer GICs deal with that transparently.
>>>>> 
>>>>> Can you try the patch below and let me know?
>>>> 
>>>> Yes will do.
>>>> 
>>>>> @@ -365,14 +354,13 @@ static void __exception_irq_entry
>>>>> gic_handle_irq(struct pt_regs *regs)
>>>>>              smp_rmb();
>>>>> 
>>>>>              /*
>>>>> -             * Samsung's funky GIC encodes the source CPU in
>>>>> -             * GICC_IAR, leading to the deactivation to fail if
>>>>> -             * not written back as is to GICC_EOI.  Stash the
>>>>> -             * INTID away for gic_eoi_irq() to write back.
>>>>> -             * This only works because we don't nest SGIs...
>>>>> +             * The GIC encodes the source CPU in GICC_IAR,
>>>>> +             * leading to the deactivation to fail if not
>>>>> +             * written back as is to GICC_EOI.  Stash the INTID
>>>>> +             * away for gic_eoi_irq() to write back.  This only
>>>>> +             * works because we don't nest SGIs...
>>>>>               */
>>>>> -            if (is_frankengic())
>>>>> -                set_sgi_intid(irqstat);
>>>>> +            this_cpu_write(sgi_intid, intid);
>>>> 
>>>> I assume that it should be irqstat here and not intid?
>>> 
>>> Indeed. As you can tell, I haven't even tried to compile it, sorry 
>>> about
>>> that.
>> 
>> No worries, I got the gist. However, even with this change, it still
>> does not boot :-(
> 
> Do you boot form EL2? If so, you'd also need this:
> 
>  static void gic_eoimode1_eoi_irq(struct irq_data *d)
>  {
> +	u32 hwirq = gic_irq(d);
> +
>  	/* Do not deactivate an IRQ forwarded to a vcpu. */
>  	if (irqd_is_forwarded_to_vcpu(d))
>  		return;
> 
> +	if (hwirq < 16)
> +		hwirq = this_cpu_read(sgi_intid);
> +
>  	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
>  }

Make it that instead:

  static void gic_eoimode1_eoi_irq(struct irq_data *d)
  {
+	u32 hwirq = gic_irq(d);
+
  	/* Do not deactivate an IRQ forwarded to a vcpu. */
  	if (irqd_is_forwarded_to_vcpu(d))
  		return;

-	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
+	if (hwirq < 16)
+		hwirq = this_cpu_read(sgi_intid);
+
+	writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
  }


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

Re: [PATCH v3 00/16] arm/arm64: Turning IPIs into normal interrupts

[Florian Fainelli]

On 9/1/2020 7:43 AM, Marc Zyngier wrote:
> For as long as SMP ARM has existed, IPIs have been handled as
> something special. The arch code and the interrupt controller exchange
> a couple of hooks (one to generate an IPI, another to handle it).
> 
> Although this is perfectly manageable, it prevents the use of features
> that we could use if IPIs were Linux IRQs (such as pseudo-NMIs). It
> also means that each interrupt controller driver has to follow an
> architecture-specific interface instead of just implementing the base
> irqchip functionalities. The arch code also duplicates a number of
> things that the core irq code already does (such as calling
> set_irq_regs(), irq_enter()...).
> 
> This series tries to remedy this on arm/arm64 by offering a new
> registration interface where the irqchip gives the arch code a range
> of interrupts to use for IPIs. The arch code requests these as normal
> per-cpu interrupts.
> 
> The bulk of the work is at the interrupt controller level, where all 5
> irqchips used on arm+SMP/arm64 get converted.
> 
> Finally, we drop the legacy registration interface as well as the
> custom statistics accounting.
> 
> Note that I have had a look at providing a "generic" interface by
> expanding the kernel/irq/ipi.c bag of helpers, but so far all
> irqchips have very different requirements, so there is hardly anything
> to consolidate for now. Maybe some as hip04 and the Marvell horror get
> cleaned up (the latter certainly could do with a good dusting).
> 
> This has been tested on a bunch of 32 and 64bit guests (GICv2, GICv3),
> as well as 64bit bare metal (GICv3). The RPi part has only been tested
> in QEMU as a 64bit guest, while the HiSi and Marvell parts have only
> been compile-tested.
> 
> I'm aiming for 5.10 for this, so any comment would be appreciated.

FWIW, I boot tested this on a Brahma-B53 device (GIC-400) in 32-bit and 
64-bit mode and on a Brahma-B15 device (GIC-400 as well) and both 
devices worked in all 3 configurations:

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

All cores were brought up successfully. All of these devices use 
PSCI/ATF FWIW.
-- 
Florian

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 17:22, Marc Zyngier wrote:
> Do you boot form EL2? 

Not that I am aware of. There is no hypervisor that we are using.

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 17:28, Marc Zyngier wrote:

...

> Make it that instead:
> 
>  static void gic_eoimode1_eoi_irq(struct irq_data *d)
>  {
> +    u32 hwirq = gic_irq(d);
> +
>      /* Do not deactivate an IRQ forwarded to a vcpu. */
>      if (irqd_is_forwarded_to_vcpu(d))
>          return;
> 
> -    writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
> +    if (hwirq < 16)
> +        hwirq = this_cpu_read(sgi_intid);
> +
> +    writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
>  }


Unfortunately, still does not boot :-(

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Mikko Perttunen]

Not sure which boards this issue is happening on, but looking at my 
hobby kernel's git history (from a couple of years ago, memory is a bit 
hazy), the commit labeled "Add support for TX2" adds code to drop from 
EL2 to EL1 at boot.

Mikko

On 9/16/20 10:06 PM, Jon Hunter wrote:
> On 16/09/2020 17:22, Marc Zyngier wrote:
>> Do you boot form EL2?
> 
> Not that I am aware of. There is no hypervisor that we are using.
> 
> Jon
> 

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 16/09/2020 20:26, Mikko Perttunen wrote:
> Not sure which boards this issue is happening on, but looking at my
> hobby kernel's git history (from a couple of years ago, memory is a bit
> hazy), the commit labeled "Add support for TX2" adds code to drop from
> EL2 to EL1 at boot.

I am seeing boot issues on Tegra20, Tegra30, Tegra186 and Tegra194.
Interestingly, Tegra124 and Tegra210 are booting OK.

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Linus Walleij]

On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:

> Can you try the patch below and let me know?

I tried this patch and now Ux500 WORKS. So this patch is definitely
something you should apply.

> -                       if (is_frankengic())
> -                               set_sgi_intid(irqstat);
> +                       this_cpu_write(sgi_intid, intid);

This needs changing to irqstat to compile as pointed out by Jon.

With that:
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Hi Linus,

On 2020-09-17 08:40, Linus Walleij wrote:
> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
> 
>> Can you try the patch below and let me know?
> 
> I tried this patch and now Ux500 WORKS. So this patch is definitely
> something you should apply.
> 
>> -                       if (is_frankengic())
>> -                               set_sgi_intid(irqstat);
>> +                       this_cpu_write(sgi_intid, intid);
> 
> This needs changing to irqstat to compile as pointed out by Jon.
> 
> With that:
> Tested-by: Linus Walleij <linus.walleij@linaro.org>

Thanks a lot for that.

Still need to understand why some of Jon's systems are left unbootable,
despite having similar GIC implementations (Tegra194 and Tegra210 use
the same GIC-400, and yet only one of the two boots correctly...).

Thanks again,

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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 08:50, Marc Zyngier wrote:
> Hi Linus,
> 
> On 2020-09-17 08:40, Linus Walleij wrote:
>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>
>>> Can you try the patch below and let me know?
>>
>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>> something you should apply.
>>
>>> -                       if (is_frankengic())
>>> -                               set_sgi_intid(irqstat);
>>> +                       this_cpu_write(sgi_intid, intid);
>>
>> This needs changing to irqstat to compile as pointed out by Jon.
>>
>> With that:
>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Thanks a lot for that.
> 
> Still need to understand why some of Jon's systems are left unbootable,
> despite having similar GIC implementations (Tegra194 and Tegra210 use
> the same GIC-400, and yet only one of the two boots correctly...).

So far, I have only tested this patch on Tegra20. Let me try the other
failing boards this morning and see if those still fail.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-17 08:54, Jon Hunter wrote:
> On 17/09/2020 08:50, Marc Zyngier wrote:
>> Hi Linus,
>> 
>> On 2020-09-17 08:40, Linus Walleij wrote:
>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>> 
>>>> Can you try the patch below and let me know?
>>> 
>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>> something you should apply.
>>> 
>>>> -                       if (is_frankengic())
>>>> -                               set_sgi_intid(irqstat);
>>>> +                       this_cpu_write(sgi_intid, intid);
>>> 
>>> This needs changing to irqstat to compile as pointed out by Jon.
>>> 
>>> With that:
>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>> 
>> Thanks a lot for that.
>> 
>> Still need to understand why some of Jon's systems are left 
>> unbootable,
>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>> the same GIC-400, and yet only one of the two boots correctly...).
> 
> So far, I have only tested this patch on Tegra20. Let me try the other
> failing boards this morning and see if those still fail.

Tegra20 (if I remember well) is a dual A9 with the same GIC 
implementation
as Ux500, hence requiring the source CPU bits to be written back. So 
this
patch should have cured it, but didn't...

/me puzzled.

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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 09:45, Marc Zyngier wrote:
> On 2020-09-17 08:54, Jon Hunter wrote:
>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>> Hi Linus,
>>>
>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>
>>>>> Can you try the patch below and let me know?
>>>>
>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>> something you should apply.
>>>>
>>>>> -                       if (is_frankengic())
>>>>> -                               set_sgi_intid(irqstat);
>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>
>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>
>>>> With that:
>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> Thanks a lot for that.
>>>
>>> Still need to understand why some of Jon's systems are left unbootable,
>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>> the same GIC-400, and yet only one of the two boots correctly...).
>>
>> So far, I have only tested this patch on Tegra20. Let me try the other
>> failing boards this morning and see if those still fail.
> 
> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
> as Ux500, hence requiring the source CPU bits to be written back. So this
> patch should have cured it, but didn't...
> 
> /me puzzled.

Me too. Maybe there just happens to be something else also going wrong
in next. I am doing a bit more testing to see if applying the fix
directly on top of this change fixes it to try and eliminate anything
else in -next.

Linus, what -next are you testing on? I am using next-20200916.

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marek Szyprowski]

Hi Jon,

On 17.09.2020 10:49, Jon Hunter wrote:
> On 17/09/2020 09:45, Marc Zyngier wrote:
>> On 2020-09-17 08:54, Jon Hunter wrote:
>>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>>
>>>>>> Can you try the patch below and let me know?
>>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>>> something you should apply.
>>>>>
>>>>>> -                       if (is_frankengic())
>>>>>> -                               set_sgi_intid(irqstat);
>>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>>
>>>>> With that:
>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>> Thanks a lot for that.
>>>>
>>>> Still need to understand why some of Jon's systems are left unbootable,
>>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>>> the same GIC-400, and yet only one of the two boots correctly...).
>>> So far, I have only tested this patch on Tegra20. Let me try the other
>>> failing boards this morning and see if those still fail.
>> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
>> as Ux500, hence requiring the source CPU bits to be written back. So this
>> patch should have cured it, but didn't...
>>
>> /me puzzled.
> Me too. Maybe there just happens to be something else also going wrong
> in next. I am doing a bit more testing to see if applying the fix
> directly on top of this change fixes it to try and eliminate anything
> else in -next.
>
> Linus, what -next are you testing on? I am using next-20200916.

next-20200916 completely broken on ARM and ARM64. Please check 
next-20200915 + the mentioned fix or just check 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-17 09:49, Jon Hunter wrote:
> On 17/09/2020 09:45, Marc Zyngier wrote:
>> On 2020-09-17 08:54, Jon Hunter wrote:

>>> So far, I have only tested this patch on Tegra20. Let me try the 
>>> other
>>> failing boards this morning and see if those still fail.
>> 
>> Tegra20 (if I remember well) is a dual A9 with the same GIC 
>> implementation
>> as Ux500, hence requiring the source CPU bits to be written back. So 
>> this
>> patch should have cured it, but didn't...
>> 
>> /me puzzled.
> 
> Me too. Maybe there just happens to be something else also going wrong
> in next. I am doing a bit more testing to see if applying the fix
> directly on top of this change fixes it to try and eliminate anything
> else in -next.
> 
> Linus, what -next are you testing on? I am using next-20200916.

You can directly try [1], which has all the queued fixes (and only 
that).

         M.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 09:54, Marek Szyprowski wrote:
> Hi Jon,
> 
> On 17.09.2020 10:49, Jon Hunter wrote:
>> On 17/09/2020 09:45, Marc Zyngier wrote:
>>> On 2020-09-17 08:54, Jon Hunter wrote:
>>>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>
>>>>>>> Can you try the patch below and let me know?
>>>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>>>> something you should apply.
>>>>>>
>>>>>>> -                       if (is_frankengic())
>>>>>>> -                               set_sgi_intid(irqstat);
>>>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>>>
>>>>>> With that:
>>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>>> Thanks a lot for that.
>>>>>
>>>>> Still need to understand why some of Jon's systems are left unbootable,
>>>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>>>> the same GIC-400, and yet only one of the two boots correctly...).
>>>> So far, I have only tested this patch on Tegra20. Let me try the other
>>>> failing boards this morning and see if those still fail.
>>> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
>>> as Ux500, hence requiring the source CPU bits to be written back. So this
>>> patch should have cured it, but didn't...
>>>
>>> /me puzzled.
>> Me too. Maybe there just happens to be something else also going wrong
>> in next. I am doing a bit more testing to see if applying the fix
>> directly on top of this change fixes it to try and eliminate anything
>> else in -next.
>>
>> Linus, what -next are you testing on? I am using next-20200916.
> 
> next-20200916 completely broken on ARM and ARM64. Please check 
> next-20200915 + the mentioned fix or just check 
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq

Ah thanks! Any idea what is causing the other failure on next-20200916?

Yes we have noticed that now everything fails next-20200916 so not just
this issue.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marek Szyprowski]

Hi Jon,

On 17.09.2020 11:09, Jon Hunter wrote:
> On 17/09/2020 09:54, Marek Szyprowski wrote:
>> On 17.09.2020 10:49, Jon Hunter wrote:
>>> On 17/09/2020 09:45, Marc Zyngier wrote:
>>>> On 2020-09-17 08:54, Jon Hunter wrote:
>>>>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>>>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>>
>>>>>>>> Can you try the patch below and let me know?
>>>>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>>>>> something you should apply.
>>>>>>>
>>>>>>>> -                       if (is_frankengic())
>>>>>>>> -                               set_sgi_intid(irqstat);
>>>>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>>>>
>>>>>>> With that:
>>>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>> Thanks a lot for that.
>>>>>>
>>>>>> Still need to understand why some of Jon's systems are left unbootable,
>>>>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>>>>> the same GIC-400, and yet only one of the two boots correctly...).
>>>>> So far, I have only tested this patch on Tegra20. Let me try the other
>>>>> failing boards this morning and see if those still fail.
>>>> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
>>>> as Ux500, hence requiring the source CPU bits to be written back. So this
>>>> patch should have cured it, but didn't...
>>>>
>>>> /me puzzled.
>>> Me too. Maybe there just happens to be something else also going wrong
>>> in next. I am doing a bit more testing to see if applying the fix
>>> directly on top of this change fixes it to try and eliminate anything
>>> else in -next.
>>>
>>> Linus, what -next are you testing on? I am using next-20200916.
>> next-20200916 completely broken on ARM and ARM64. Please check
>> next-20200915 + the mentioned fix or just check
>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
> Ah thanks! Any idea what is causing the other failure on next-20200916?
>
> Yes we have noticed that now everything fails next-20200916 so not just
> this issue.

The issue is caused by commit c999bd436fe9 ("mm/cma: make number of CMA 
areas dynamic, remove CONFIG_CMA_AREAS")

https://lore.kernel.org/linux-arm-kernel/20200915205703.34572-1-mike.kravetz@oracle.com/

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-17 10:13, Marek Szyprowski wrote:

[...]

>>>> Linus, what -next are you testing on? I am using next-20200916.
>>> next-20200916 completely broken on ARM and ARM64. Please check
>>> next-20200915 + the mentioned fix or just check
>>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
>> Ah thanks! Any idea what is causing the other failure on 
>> next-20200916?
>> 
>> Yes we have noticed that now everything fails next-20200916 so not 
>> just
>> this issue.
> 
> The issue is caused by commit c999bd436fe9 ("mm/cma: make number of CMA
> areas dynamic, remove CONFIG_CMA_AREAS")
> 
> https://lore.kernel.org/linux-arm-kernel/20200915205703.34572-1-mike.kravetz@oracle.com/

There is a workaround here[1] for arm64, but I doubt that's the end of
it (32bit is still dead).

         M.

[1] 
https://lore.kernel.org/linux-arm-kernel/20200916085933.25220-1-song.bao.hua@hisilicon.com/
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Linus Walleij]

On Thu, Sep 17, 2020 at 10:49 AM Jon Hunter <jonathanh@nvidia.com> wrote:

> Linus, what -next are you testing on? I am using next-20200916.

That's what I use. But the Ux500 graphics are simple and does not
use CMA and that is why I don't see this crash (I assume).

Yours,
Linus Walleij

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 09:54, Marek Szyprowski wrote:
> Hi Jon,
> 
> On 17.09.2020 10:49, Jon Hunter wrote:
>> On 17/09/2020 09:45, Marc Zyngier wrote:
>>> On 2020-09-17 08:54, Jon Hunter wrote:
>>>> On 17/09/2020 08:50, Marc Zyngier wrote:
>>>>> On 2020-09-17 08:40, Linus Walleij wrote:
>>>>>> On Wed, Sep 16, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote:
>>>>>>
>>>>>>> Can you try the patch below and let me know?
>>>>>> I tried this patch and now Ux500 WORKS. So this patch is definitely
>>>>>> something you should apply.
>>>>>>
>>>>>>> -                       if (is_frankengic())
>>>>>>> -                               set_sgi_intid(irqstat);
>>>>>>> +                       this_cpu_write(sgi_intid, intid);
>>>>>> This needs changing to irqstat to compile as pointed out by Jon.
>>>>>>
>>>>>> With that:
>>>>>> Tested-by: Linus Walleij <linus.walleij@linaro.org>
>>>>> Thanks a lot for that.
>>>>>
>>>>> Still need to understand why some of Jon's systems are left unbootable,
>>>>> despite having similar GIC implementations (Tegra194 and Tegra210 use
>>>>> the same GIC-400, and yet only one of the two boots correctly...).
>>>> So far, I have only tested this patch on Tegra20. Let me try the other
>>>> failing boards this morning and see if those still fail.
>>> Tegra20 (if I remember well) is a dual A9 with the same GIC implementation
>>> as Ux500, hence requiring the source CPU bits to be written back. So this
>>> patch should have cured it, but didn't...
>>>
>>> /me puzzled.
>> Me too. Maybe there just happens to be something else also going wrong
>> in next. I am doing a bit more testing to see if applying the fix
>> directly on top of this change fixes it to try and eliminate anything
>> else in -next.
>>
>> Linus, what -next are you testing on? I am using next-20200916.
> 
> next-20200916 completely broken on ARM and ARM64. Please check 
> next-20200915 + the mentioned fix or just check 
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq

OK, I have confirmed that on Tegra20 and Tegra30, that next-20200915 +
Marc's fix boots fine.

Tegra186 and Tegra194 are not booting, but I am wondering if this is yet
another issue that is not related. I have not actually bisected on these
boards, but I am now bisecting on Tegra186 to confirm.

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Jon Hunter]

On 17/09/2020 15:53, Jon Hunter wrote:

...

>> next-20200916 completely broken on ARM and ARM64. Please check 
>> next-20200915 + the mentioned fix or just check 
>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
> 
> OK, I have confirmed that on Tegra20 and Tegra30, that next-20200915 +
> Marc's fix boots fine.
> 
> Tegra186 and Tegra194 are not booting, but I am wondering if this is yet
> another issue that is not related. I have not actually bisected on these
> boards, but I am now bisecting on Tegra186 to confirm.

Hmm ... well bisect on Tegra186 is also pointing to this commit, but the
fix is not working there. Wonder what's going on with this gic-400?

Jon

-- 
nvpublic

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-17 19:24, Jon Hunter wrote:
> On 17/09/2020 15:53, Jon Hunter wrote:
> 
> ...
> 
>>> next-20200916 completely broken on ARM and ARM64. Please check
>>> next-20200915 + the mentioned fix or just check
>>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-as-irq
>> 
>> OK, I have confirmed that on Tegra20 and Tegra30, that next-20200915 +
>> Marc's fix boots fine.
>> 
>> Tegra186 and Tegra194 are not booting, but I am wondering if this is 
>> yet
>> another issue that is not related. I have not actually bisected on 
>> these
>> boards, but I am now bisecting on Tegra186 to confirm.
> 
> Hmm ... well bisect on Tegra186 is also pointing to this commit, but 
> the
> fix is not working there. Wonder what's going on with this gic-400?

It's not GIC400. I have tons of machines with GIC400 around me, and they
don't even need the fix. What happens if you only boot the non-Denver 
CPUs?

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

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[James Morse]

Hi Marc,

(CC: +Jon)

On 01/09/2020 15:43, Marc Zyngier wrote:
> Change the way we deal with GIC SGIs by turning them into proper
> IRQs, and calling into the arch code to register the interrupt range
> instead of a callback.

Your comment "This only works because we don't nest SGIs..." on this thread tripped some
bad memories from adding the irq-stack. Softirq causes us to nest irqs, but only once.


(I've messed with the below diff to remove the added stuff:)

> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 4ffd62af888f..4be2b62f816f 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -335,31 +335,22 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>  		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>  		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
>  
> -		if (likely(irqnr > 15 && irqnr < 1020)) {
> -			if (static_branch_likely(&supports_deactivate_key))
> -				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
> -			isb();
> -			handle_domain_irq(gic->domain, irqnr, regs);
> -			continue;
> -		}
> -		if (irqnr < 16) {
>  			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
> -			if (static_branch_likely(&supports_deactivate_key))
> -				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
> -#ifdef CONFIG_SMP
> -			/*
> -			 * Ensure any shared data written by the CPU sending
> -			 * the IPI is read after we've read the ACK register
> -			 * on the GIC.
> -			 *
> -			 * Pairs with the write barrier in gic_raise_softirq
> -			 */
>  			smp_rmb();
> -			handle_IPI(irqnr, regs);

If I read this right, previously we would EOI the interrupt before calling handle_IPI().
Where as now with the version of this series in your tree, we stuff the to-be-EOId value
in a percpu variable, which is only safe if these don't nest.

Hidden in irq_exit(), kernel/softirq.c::__irq_exit_rcu() has this:
|	preempt_count_sub(HARDIRQ_OFFSET);
|	if (!in_interrupt() && local_softirq_pending())
|		invoke_softirq();

The arch code doesn't raise the preempt counter by HARDIRQ, so once __irq_exit_rcu() has
dropped it, in_interrupt() returns false, and we invoke_softirq().

invoke_softirq() -> __do_softirq() -> local_irq_enable()!

Fortunately, __do_softirq() raises the softirq count first using __local_bh_disable_ip(),
which in-interrupt() checks too, so this can only happen once per IRQ.

Now the irq_exit() has moved from handle_IPI(), which ran after EOI, into
handle_domain_irq(), which runs before. I think its possible SGIs nest, and the new percpu
variable becomes corrupted.

Presumably this isn't a problem for regular IRQ, as they don't need the sending-CPU in
order to EOI, which is why it wasn't a problem before.

Adding anything to preempt-count around the whole thing upsets RCU, and softirq seems to
expect this nesting, but evidently the gic does not. I'm not sure what the right thing to
do would be. A dirty hack like [0] would confirm the theory.

/me runs

Thanks,

James



[0] A dirty hack
-----------%<-----------
diff --git a/kernel/softirq.c b/kernel/softirq.c
index bf88d7f62433..50e14d8cbec3 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -376,7 +376,7 @@ static inline void invoke_softirq(void)
        if (ksoftirqd_running(local_softirq_pending()))
                return;

-       if (!force_irqthreads) {
+       if (false) {
 #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK
                /*
                 * We can safely execute softirq on the current stack if
@@ -393,6 +393,7 @@ static inline void invoke_softirq(void)
                do_softirq_own_stack();
 #endif
        } else {
+               /* hack: force this */
                wakeup_softirqd();
        }
 }
-----------%<-----------

Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

Hi James,

On Fri, 18 Sep 2020 10:58:45 +0100,
James Morse <james.morse@arm.com> wrote:
> 
> Hi Marc,
> 
> (CC: +Jon)
> 
> On 01/09/2020 15:43, Marc Zyngier wrote:
> > Change the way we deal with GIC SGIs by turning them into proper
> > IRQs, and calling into the arch code to register the interrupt range
> > instead of a callback.
> 
> Your comment "This only works because we don't nest SGIs..." on this
> thread tripped some bad memories from adding the irq-stack. Softirq
> causes us to nest irqs, but only once.
> 
> 
> (I've messed with the below diff to remove the added stuff:)
> 
> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index 4ffd62af888f..4be2b62f816f 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -335,31 +335,22 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
> >  		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
> >  		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> >  
> > -		if (likely(irqnr > 15 && irqnr < 1020)) {
> > -			if (static_branch_likely(&supports_deactivate_key))
> > -				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
> > -			isb();
> > -			handle_domain_irq(gic->domain, irqnr, regs);
> > -			continue;
> > -		}
> > -		if (irqnr < 16) {
> >  			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
> > -			if (static_branch_likely(&supports_deactivate_key))
> > -				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
> > -#ifdef CONFIG_SMP
> > -			/*
> > -			 * Ensure any shared data written by the CPU sending
> > -			 * the IPI is read after we've read the ACK register
> > -			 * on the GIC.
> > -			 *
> > -			 * Pairs with the write barrier in gic_raise_softirq
> > -			 */
> >  			smp_rmb();
> > -			handle_IPI(irqnr, regs);
> 
> If I read this right, previously we would EOI the interrupt before
> calling handle_IPI().  Where as now with the version of this series
> in your tree, we stuff the to-be-EOId value in a percpu variable,
> which is only safe if these don't nest.
> 
> Hidden in irq_exit(), kernel/softirq.c::__irq_exit_rcu() has this:
> |	preempt_count_sub(HARDIRQ_OFFSET);
> |	if (!in_interrupt() && local_softirq_pending())
> |		invoke_softirq();
> 
> The arch code doesn't raise the preempt counter by HARDIRQ, so once
> __irq_exit_rcu() has dropped it, in_interrupt() returns false, and
> we invoke_softirq().
> 
> invoke_softirq() -> __do_softirq() -> local_irq_enable()!
> 
> Fortunately, __do_softirq() raises the softirq count first using
> __local_bh_disable_ip(), which in-interrupt() checks too, so this
> can only happen once per IRQ.
> 
> Now the irq_exit() has moved from handle_IPI(), which ran after EOI,
> into handle_domain_irq(), which runs before. I think its possible
> SGIs nest, and the new percpu variable becomes corrupted.

I can't see how. The interrupt is active until we EOI/deactivate it,
and thus cannot be observed again by the CPU interface until this
happens.

Furthermore, irq_exit() in __handle_domain_irq() is *after* the EOI
anyway (generic_handle_irq_() directly calls the flow, which
immediately EOIs the interrupt). The only material change is that
irq_enter() happens before EOI. Is that what you are referring to?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[Guillaume Tucker]

Hi Marc,

On 01/09/2020 15:43, Marc Zyngier wrote:
> Let's switch the arm code to the core accounting, which already
> does everything we need.
> 
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm/include/asm/hardirq.h | 17 -----------------
>  arch/arm/kernel/smp.c          | 20 ++++----------------
>  2 files changed, 4 insertions(+), 33 deletions(-)

This appears to be causing a NULL pointer dereference on
beaglebone-black, it got bisected automatically several times.
None of the other platforms in the KernelCI labs appears to be
affected.

Here's the error in the full job log, with next-20200923:

  https://storage.staging.kernelci.org/kernelci/staging.kernelci.org/staging-20200924.0/arm/multi_v7_defconfig/gcc-8/lab-baylibre/baseline-beaglebone-black.html#L460

and some meta-data:

  https://staging.kernelci.org/test/case/id/5f6bea67f724eb1b34dce584/

The full bisection report is available here:

  https://groups.io/g/kernelci-results-staging/message/2094

I've also run it again with a debug build to locate the problem,
see below.


> diff --git a/arch/arm/include/asm/hardirq.h b/arch/arm/include/asm/hardirq.h
> index 7a88f160b1fb..b95848ed2bc7 100644
> --- a/arch/arm/include/asm/hardirq.h
> +++ b/arch/arm/include/asm/hardirq.h
> @@ -6,29 +6,12 @@
>  #include <linux/threads.h>
>  #include <asm/irq.h>
>  
> -/* number of IPIS _not_ including IPI_CPU_BACKTRACE */
> -#define NR_IPI	7
> -
>  typedef struct {
>  	unsigned int __softirq_pending;
> -#ifdef CONFIG_SMP
> -	unsigned int ipi_irqs[NR_IPI];
> -#endif
>  } ____cacheline_aligned irq_cpustat_t;
>  
>  #include <linux/irq_cpustat.h>	/* Standard mappings for irq_cpustat_t above */
>  
> -#define __inc_irq_stat(cpu, member)	__IRQ_STAT(cpu, member)++
> -#define __get_irq_stat(cpu, member)	__IRQ_STAT(cpu, member)
> -
> -#ifdef CONFIG_SMP
> -u64 smp_irq_stat_cpu(unsigned int cpu);
> -#else
> -#define smp_irq_stat_cpu(cpu)	0
> -#endif
> -
> -#define arch_irq_stat_cpu	smp_irq_stat_cpu
> -
>  #define __ARCH_IRQ_EXIT_IRQS_DISABLED	1
>  
>  #endif /* __ASM_HARDIRQ_H */
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index d51e64955a26..aead847ac8b9 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -65,6 +65,7 @@ enum ipi_msg_type {
>  	IPI_CPU_STOP,
>  	IPI_IRQ_WORK,
>  	IPI_COMPLETION,
> +	NR_IPI,
>  	/*
>  	 * CPU_BACKTRACE is special and not included in NR_IPI
>  	 * or tracable with trace_ipi_*
> @@ -529,27 +530,16 @@ void show_ipi_list(struct seq_file *p, int prec)
>  	unsigned int cpu, i;
>  
>  	for (i = 0; i < NR_IPI; i++) {
> +		unsigned int irq = irq_desc_get_irq(ipi_desc[i]);

It looks like irq_desc_get_irq() gets called with a NULL
pointer (well, 0x0000001c):

(gdb) l *0xc030ef38
0xc030ef38 is in show_ipi_list (../include/linux/irqdesc.h:123).
118		return container_of(data->common, struct irq_desc, irq_common_data);
119	}
120	
121	static inline unsigned int irq_desc_get_irq(struct irq_desc *desc)
122	{
123		return desc->irq_data.irq;
124	}
125	
126	static inline struct irq_data *irq_desc_get_irq_data(struct irq_desc *desc)
127	{

Full job log: https://lava.baylibre.com/scheduler/job/142375#L727

I haven't looked any further but hopefully this should be a good
enough clue to find the root cause.  I don't know if you have a
platform at hand to reproduce the issue, please let me know if
you need some help with debugging or testing a fix.

Hope this helps,
Guillaume


>  		seq_printf(p, "%*s%u: ", prec - 1, "IPI", i);
>  
>  		for_each_online_cpu(cpu)
> -			seq_printf(p, "%10u ",
> -				   __get_irq_stat(cpu, ipi_irqs[i]));
> +			seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu));
>  
>  		seq_printf(p, " %s\n", ipi_types[i]);
>  	}
>  }
>  
> -u64 smp_irq_stat_cpu(unsigned int cpu)
> -{
> -	u64 sum = 0;
> -	int i;
> -
> -	for (i = 0; i < NR_IPI; i++)
> -		sum += __get_irq_stat(cpu, ipi_irqs[i]);
> -
> -	return sum;
> -}
> -
>  void arch_send_call_function_ipi_mask(const struct cpumask *mask)
>  {
>  	smp_cross_call(mask, IPI_CALL_FUNC);
> @@ -630,10 +620,8 @@ static void do_handle_IPI(int ipinr)
>  {
>  	unsigned int cpu = smp_processor_id();
>  
> -	if ((unsigned)ipinr < NR_IPI) {
> +	if ((unsigned)ipinr < NR_IPI)
>  		trace_ipi_entry_rcuidle(ipi_types[ipinr]);
> -		__inc_irq_stat(cpu, ipi_irqs[ipinr]);
> -	}
>  
>  	switch (ipinr) {
>  	case IPI_WAKEUP:
> 

Re: [PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[Marc Zyngier]

Hi Guillaume,

On Thu, 24 Sep 2020 10:00:09 +0100,
Guillaume Tucker <guillaume.tucker@collabora.com> wrote:
> 
> Hi Marc,
> 
> On 01/09/2020 15:43, Marc Zyngier wrote:
> > Let's switch the arm code to the core accounting, which already
> > does everything we need.
> > 
> > Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm/include/asm/hardirq.h | 17 -----------------
> >  arch/arm/kernel/smp.c          | 20 ++++----------------
> >  2 files changed, 4 insertions(+), 33 deletions(-)
> 
> This appears to be causing a NULL pointer dereference on
> beaglebone-black, it got bisected automatically several times.
> None of the other platforms in the KernelCI labs appears to be
> affected.

Hmm. My bet is that because this is a UP machine running an SMP
kernel, and I fell into the trap of forgetting about this 32bit
configuration.

I expect the following patch to fix it. Please give it a go if you can
(I'm away at the moment and can't test much, and do not have any
physical 32bit machine to test this on).

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 00327fa74b01..b4e3d336dc33 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -531,7 +531,12 @@ void show_ipi_list(struct seq_file *p, int prec)
 	unsigned int cpu, i;
 
 	for (i = 0; i < NR_IPI; i++) {
-		unsigned int irq = irq_desc_get_irq(ipi_desc[i]);
+		unsigned int irq;
+
+		if (!ipi_desc[i])
+			continue;
+
+		irq = irq_desc_get_irq(ipi_desc[i]);
 		seq_printf(p, "%*s%u: ", prec - 1, "IPI", i);
 
 		for_each_online_cpu(cpu)

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[Guillaume Tucker]

On 24/09/2020 10:29, Marc Zyngier wrote:
> Hi Guillaume,
> 
> On Thu, 24 Sep 2020 10:00:09 +0100,
> Guillaume Tucker <guillaume.tucker@collabora.com> wrote:
>>
>> Hi Marc,
>>
>> On 01/09/2020 15:43, Marc Zyngier wrote:
>>> Let's switch the arm code to the core accounting, which already
>>> does everything we need.
>>>
>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm/include/asm/hardirq.h | 17 -----------------
>>>  arch/arm/kernel/smp.c          | 20 ++++----------------
>>>  2 files changed, 4 insertions(+), 33 deletions(-)
>>
>> This appears to be causing a NULL pointer dereference on
>> beaglebone-black, it got bisected automatically several times.
>> None of the other platforms in the KernelCI labs appears to be
>> affected.
> 
> Hmm. My bet is that because this is a UP machine running an SMP
> kernel, and I fell into the trap of forgetting about this 32bit
> configuration.
> 
> I expect the following patch to fix it. Please give it a go if you can
> (I'm away at the moment and can't test much, and do not have any
> physical 32bit machine to test this on).

OK thanks, that worked:

  https://lava.baylibre.com/scheduler/job/143170

I've added this fix to the kernel branch used on
staging.kernelci.org which is based on linux-next, so it will get
fully verified a bit later today.

Guillaume


> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 00327fa74b01..b4e3d336dc33 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -531,7 +531,12 @@ void show_ipi_list(struct seq_file *p, int prec)
>  	unsigned int cpu, i;
>  
>  	for (i = 0; i < NR_IPI; i++) {
> -		unsigned int irq = irq_desc_get_irq(ipi_desc[i]);
> +		unsigned int irq;
> +
> +		if (!ipi_desc[i])
> +			continue;
> +
> +		irq = irq_desc_get_irq(ipi_desc[i]);
>  		seq_printf(p, "%*s%u: ", prec - 1, "IPI", i);
>  
>  		for_each_online_cpu(cpu)
> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[Fabio Estevam]

Hi Guillaume,

On Thu, Sep 24, 2020 at 6:01 AM Guillaume Tucker
<guillaume.tucker@collabora.com> wrote:

> This appears to be causing a NULL pointer dereference on
> beaglebone-black, it got bisected automatically several times.
> None of the other platforms in the KernelCI labs appears to be
> affected.

Actually imx53-qsb is also affected:
https://storage.kernelci.org/next/master/next-20200924/arm/imx_v6_v7_defconfig/gcc-8/lab-pengutronix/baseline-imx53-qsrb.html

kernelci marks it Boot result: PASS though.

Shouldn't kernelci flag a warning or error instead?

Thanks

Re: [PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[Guillaume Tucker]

On 24/09/2020 14:34, Fabio Estevam wrote:
> Hi Guillaume,
> 
> On Thu, Sep 24, 2020 at 6:01 AM Guillaume Tucker
> <guillaume.tucker@collabora.com> wrote:
> 
>> This appears to be causing a NULL pointer dereference on
>> beaglebone-black, it got bisected automatically several times.
>> None of the other platforms in the KernelCI labs appears to be
>> affected.
> 
> Actually imx53-qsb is also affected:
> https://storage.kernelci.org/next/master/next-20200924/arm/imx_v6_v7_defconfig/gcc-8/lab-pengutronix/baseline-imx53-qsrb.html
> 
> kernelci marks it Boot result: PASS though.
> 
> Shouldn't kernelci flag a warning or error instead?

Thanks for bringing this up.  The status in the HTML log file is
a very coarse one, in this case the board booted "fine" since it
reached a login prompt.  The issue was detected later when
checking for errors in the kernel log.

But yes you're right, the issue is also impacting imx53-qsrb
indeed.  I didn't spot that because it was only reported as a
regression on staging.kernelci.org, whereas imx53-qsrb is in the
Pengutronix lab which is not sending results there at the moment.

The failures can be found on the production web dashboard though,
but not as regressions:

beaglebone-black:

  https://kernelci.org/test/case/id/5f6c7f1ab7c8c5472cbf9de9/

imx53-qsrb:

  https://kernelci.org/test/case/id/5f6c7ea6f89a9d0f4dbf9ddf/


I need to investigate why that is the case, knowing that the
regression was detected correctly on staging which is the
development KernelCI instance:

  https://staging.kernelci.org/test/plan/id/5f6bea67f724eb1b34dce581/


Thanks,
Guillaume

Re: [PATCH v3 16/16] ARM: Remove custom IRQ stat accounting

[Guillaume Tucker]

Hi Marc,

On 24/09/2020 14:09, Guillaume Tucker wrote:
> On 24/09/2020 10:29, Marc Zyngier wrote:
>> Hi Guillaume,
>>
>> On Thu, 24 Sep 2020 10:00:09 +0100,
>> Guillaume Tucker <guillaume.tucker@collabora.com> wrote:
>>>
>>> Hi Marc,
>>>
>>> On 01/09/2020 15:43, Marc Zyngier wrote:
>>>> Let's switch the arm code to the core accounting, which already
>>>> does everything we need.
>>>>
>>>> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> ---
>>>>  arch/arm/include/asm/hardirq.h | 17 -----------------
>>>>  arch/arm/kernel/smp.c          | 20 ++++----------------
>>>>  2 files changed, 4 insertions(+), 33 deletions(-)
>>>
>>> This appears to be causing a NULL pointer dereference on
>>> beaglebone-black, it got bisected automatically several times.
>>> None of the other platforms in the KernelCI labs appears to be
>>> affected.
>>
>> Hmm. My bet is that because this is a UP machine running an SMP
>> kernel, and I fell into the trap of forgetting about this 32bit
>> configuration.
>>
>> I expect the following patch to fix it. Please give it a go if you can
>> (I'm away at the moment and can't test much, and do not have any
>> physical 32bit machine to test this on).
> 
> OK thanks, that worked:
> 
>   https://lava.baylibre.com/scheduler/job/143170
> 
> I've added this fix to the kernel branch used on
> staging.kernelci.org which is based on linux-next, so it will get
> fully verified a bit later today.
> 
> Guillaume
> 
> 
>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>> index 00327fa74b01..b4e3d336dc33 100644
>> --- a/arch/arm/kernel/smp.c
>> +++ b/arch/arm/kernel/smp.c
>> @@ -531,7 +531,12 @@ void show_ipi_list(struct seq_file *p, int prec)
>>  	unsigned int cpu, i;
>>  
>>  	for (i = 0; i < NR_IPI; i++) {
>> -		unsigned int irq = irq_desc_get_irq(ipi_desc[i]);
>> +		unsigned int irq;
>> +
>> +		if (!ipi_desc[i])
>> +			continue;
>> +
>> +		irq = irq_desc_get_irq(ipi_desc[i]);
>>  		seq_printf(p, "%*s%u: ", prec - 1, "IPI", i);
>>  
>>  		for_each_online_cpu(cpu)

This fix has been all tested now, with no visible side effects:

  https://staging.kernelci.org/test/job/kernelci/branch/staging.kernelci.org/kernel/staging-20200928.1/plan/baseline/

In the meantime, the same issue was detected (without the fix)
and bisected on sun5i-a13-olinuxino-micro and landed on the same
commit.  A few more platforms are also impacted such as imx53-qsb
as mentioned by Fabio.

The commit is in your irqchip tree so I guess we should wait for
you to apply the fix.  If you do make a separate commit to fix
the issue, please add:

  Reported-by: kernelci.org bot <bot@kernelci.org>

and also:

  Tested-by: Guillaume Tucker <guillaume.tucker@collabora.com>

Thanks,
Guillaume

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Vincent Guittot]

Hi Marc,

On Tue, 1 Sep 2020 at 16:44, Marc Zyngier <maz@kernel.org> wrote:
>
> In order to deal with IPIs as normal interrupts, let's add
> a new way to register them with the architecture code.
>
> set_smp_ipi_range() takes a range of interrupts, and allows
> the arch code to request them as if the were normal interrupts.
> A standard handler is then called by the core IRQ code to deal
> with the IPI.
>
> This means that we don't need to call irq_enter/irq_exit, and
> that we don't need to deal with set_irq_regs either. So let's
> move the dispatcher into its own function, and leave handle_IPI()
> as a compatibility function.
>
> On the sending side, let's make use of ipi_send_mask, which
> already exists for this purpose.
>
> One of the major difference is that we end up, in some cases
> (such as when performing IRQ time accounting on the scheduler
> IPI), end up with nested irq_enter()/irq_exit() pairs.
> Other than the (relatively small) overhead, there should be
> no consequences to it (these pairs are designed to nest
> correctly, and the accounting shouldn't be off).

While rebasing on mainline, I have faced a performance regression for
the benchmark:
perf bench sched pipe
on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)

The regression comes from:
commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal interrupts")

          v5.9              + this patch
hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%

By + this patch,  I mean merging branch from this patch. Whereas
merging the previous:
commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
/proc/interrupts")
 It doesn't show any regression

Vincent

>
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/Kconfig           |  1 +
>  arch/arm64/include/asm/smp.h |  5 ++
>  arch/arm64/kernel/smp.c      | 93 +++++++++++++++++++++++++++++++-----
>  3 files changed, 87 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 6d232837cbee..d0fdbe5fb32f 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -106,6 +106,7 @@ config ARM64
>         select GENERIC_CPU_VULNERABILITIES
>         select GENERIC_EARLY_IOREMAP
>         select GENERIC_IDLE_POLL_SETUP
> +       select GENERIC_IRQ_IPI
>         select GENERIC_IRQ_MULTI_HANDLER
>         select GENERIC_IRQ_PROBE
>         select GENERIC_IRQ_SHOW
> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
> index 0eadbf933e35..57c5db15f6b7 100644
> --- a/arch/arm64/include/asm/smp.h
> +++ b/arch/arm64/include/asm/smp.h
> @@ -78,6 +78,11 @@ extern void set_smp_cross_call(void (*)(const struct cpumask *, unsigned int));
>
>  extern void (*__smp_cross_call)(const struct cpumask *, unsigned int);
>
> +/*
> + * Register IPI interrupts with the arch SMP code
> + */
> +extern void set_smp_ipi_range(int ipi_base, int nr_ipi);
> +
>  /*
>   * Called from the secondary holding pen, this is the secondary CPU entry point.
>   */
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 355ee9eed4dd..00c9db1b61b5 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -75,6 +75,13 @@ enum ipi_msg_type {
>         IPI_WAKEUP
>  };
>
> +static int ipi_irq_base __read_mostly;
> +static int nr_ipi __read_mostly = NR_IPI;
> +static struct irq_desc *ipi_desc[NR_IPI] __read_mostly;
> +
> +static void ipi_setup(int cpu);
> +static void ipi_teardown(int cpu);
> +
>  #ifdef CONFIG_HOTPLUG_CPU
>  static int op_cpu_kill(unsigned int cpu);
>  #else
> @@ -237,6 +244,8 @@ asmlinkage notrace void secondary_start_kernel(void)
>          */
>         notify_cpu_starting(cpu);
>
> +       ipi_setup(cpu);
> +
>         store_cpu_topology(cpu);
>         numa_add_cpu(cpu);
>
> @@ -302,6 +311,7 @@ int __cpu_disable(void)
>          * and we must not schedule until we're ready to give up the cpu.
>          */
>         set_cpu_online(cpu, false);
> +       ipi_teardown(cpu);
>
>         /*
>          * OK - migrate IRQs away from this CPU
> @@ -890,10 +900,9 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
>  /*
>   * Main handler for inter-processor interrupts
>   */
> -void handle_IPI(int ipinr, struct pt_regs *regs)
> +static void do_handle_IPI(int ipinr)
>  {
>         unsigned int cpu = smp_processor_id();
> -       struct pt_regs *old_regs = set_irq_regs(regs);
>
>         if ((unsigned)ipinr < NR_IPI) {
>                 trace_ipi_entry_rcuidle(ipi_types[ipinr]);
> @@ -906,21 +915,16 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>                 break;
>
>         case IPI_CALL_FUNC:
> -               irq_enter();
>                 generic_smp_call_function_interrupt();
> -               irq_exit();
>                 break;
>
>         case IPI_CPU_STOP:
> -               irq_enter();
>                 local_cpu_stop();
> -               irq_exit();
>                 break;
>
>         case IPI_CPU_CRASH_STOP:
>                 if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
> -                       irq_enter();
> -                       ipi_cpu_crash_stop(cpu, regs);
> +                       ipi_cpu_crash_stop(cpu, get_irq_regs());
>
>                         unreachable();
>                 }
> @@ -928,17 +932,13 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>
>  #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
>         case IPI_TIMER:
> -               irq_enter();
>                 tick_receive_broadcast();
> -               irq_exit();
>                 break;
>  #endif
>
>  #ifdef CONFIG_IRQ_WORK
>         case IPI_IRQ_WORK:
> -               irq_enter();
>                 irq_work_run();
> -               irq_exit();
>                 break;
>  #endif
>
> @@ -957,9 +957,78 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>
>         if ((unsigned)ipinr < NR_IPI)
>                 trace_ipi_exit_rcuidle(ipi_types[ipinr]);
> +}
> +
> +/* Legacy version, should go away once all irqchips have been converted */
> +void handle_IPI(int ipinr, struct pt_regs *regs)
> +{
> +       struct pt_regs *old_regs = set_irq_regs(regs);
> +
> +       irq_enter();
> +       do_handle_IPI(ipinr);
> +       irq_exit();
> +
>         set_irq_regs(old_regs);
>  }
>
> +static irqreturn_t ipi_handler(int irq, void *data)
> +{
> +       do_handle_IPI(irq - ipi_irq_base);
> +       return IRQ_HANDLED;
> +}
> +
> +static void ipi_send(const struct cpumask *target, unsigned int ipi)
> +{
> +       __ipi_send_mask(ipi_desc[ipi], target);
> +}
> +
> +static void ipi_setup(int cpu)
> +{
> +       int i;
> +
> +       if (!ipi_irq_base)
> +               return;
> +
> +       for (i = 0; i < nr_ipi; i++)
> +               enable_percpu_irq(ipi_irq_base + i, 0);
> +}
> +
> +static void ipi_teardown(int cpu)
> +{
> +       int i;
> +
> +       if (!ipi_irq_base)
> +               return;
> +
> +       for (i = 0; i < nr_ipi; i++)
> +               disable_percpu_irq(ipi_irq_base + i);
> +}
> +
> +void __init set_smp_ipi_range(int ipi_base, int n)
> +{
> +       int i;
> +
> +       WARN_ON(n < NR_IPI);
> +       nr_ipi = min(n, NR_IPI);
> +
> +       for (i = 0; i < nr_ipi; i++) {
> +               int err;
> +
> +               err = request_percpu_irq(ipi_base + i, ipi_handler,
> +                                        "IPI", &irq_stat);
> +               WARN_ON(err);
> +
> +               ipi_desc[i] = irq_to_desc(ipi_base + i);
> +               irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);
> +       }
> +
> +       ipi_irq_base = ipi_base;
> +       __smp_cross_call = ipi_send;
> +
> +       /* Setup the boot CPU immediately */
> +       ipi_setup(smp_processor_id());
> +}
> +
>  void smp_send_reschedule(int cpu)
>  {
>         smp_cross_call(cpumask_of(cpu), IPI_RESCHEDULE);
> --
> 2.27.0
>

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Marc Zyngier]

Hi Vincent,

On 2020-10-19 13:42, Vincent Guittot wrote:
> Hi Marc,
> 
> On Tue, 1 Sep 2020 at 16:44, Marc Zyngier <maz@kernel.org> wrote:
>> 
>> In order to deal with IPIs as normal interrupts, let's add
>> a new way to register them with the architecture code.
>> 
>> set_smp_ipi_range() takes a range of interrupts, and allows
>> the arch code to request them as if the were normal interrupts.
>> A standard handler is then called by the core IRQ code to deal
>> with the IPI.
>> 
>> This means that we don't need to call irq_enter/irq_exit, and
>> that we don't need to deal with set_irq_regs either. So let's
>> move the dispatcher into its own function, and leave handle_IPI()
>> as a compatibility function.
>> 
>> On the sending side, let's make use of ipi_send_mask, which
>> already exists for this purpose.
>> 
>> One of the major difference is that we end up, in some cases
>> (such as when performing IRQ time accounting on the scheduler
>> IPI), end up with nested irq_enter()/irq_exit() pairs.
>> Other than the (relatively small) overhead, there should be
>> no consequences to it (these pairs are designed to nest
>> correctly, and the accounting shouldn't be off).
> 
> While rebasing on mainline, I have faced a performance regression for
> the benchmark:
> perf bench sched pipe
> on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)
> 
> The regression comes from:
> commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal 
> interrupts")

That's interesting, as this patch doesn't really change anything (most
of the potential overhead comes in later). The only potential overhead
I can see is that the scheduler_ipi() call is now wrapped around
irq_enter()/irq_exit().

> 
>           v5.9              + this patch
> hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
> thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%
> 
> By + this patch,  I mean merging branch from this patch. Whereas
> merging the previous:
> commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
> /proc/interrupts")
>  It doesn't show any regression

Since you are running perf, can you spot where the overhead occurs?

Thanks,

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

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Vincent Guittot]

On Mon, 19 Oct 2020 at 15:04, Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Vincent,
>
> On 2020-10-19 13:42, Vincent Guittot wrote:
> > Hi Marc,
> >
> > On Tue, 1 Sep 2020 at 16:44, Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> In order to deal with IPIs as normal interrupts, let's add
> >> a new way to register them with the architecture code.
> >>
> >> set_smp_ipi_range() takes a range of interrupts, and allows
> >> the arch code to request them as if the were normal interrupts.
> >> A standard handler is then called by the core IRQ code to deal
> >> with the IPI.
> >>
> >> This means that we don't need to call irq_enter/irq_exit, and
> >> that we don't need to deal with set_irq_regs either. So let's
> >> move the dispatcher into its own function, and leave handle_IPI()
> >> as a compatibility function.
> >>
> >> On the sending side, let's make use of ipi_send_mask, which
> >> already exists for this purpose.
> >>
> >> One of the major difference is that we end up, in some cases
> >> (such as when performing IRQ time accounting on the scheduler
> >> IPI), end up with nested irq_enter()/irq_exit() pairs.
> >> Other than the (relatively small) overhead, there should be
> >> no consequences to it (these pairs are designed to nest
> >> correctly, and the accounting shouldn't be off).
> >
> > While rebasing on mainline, I have faced a performance regression for
> > the benchmark:
> > perf bench sched pipe
> > on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)
> >
> > The regression comes from:
> > commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal
> > interrupts")
>
> That's interesting, as this patch doesn't really change anything (most
> of the potential overhead comes in later). The only potential overhead
> I can see is that the scheduler_ipi() call is now wrapped around
> irq_enter()/irq_exit().
>
> >
> >           v5.9              + this patch
> > hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
> > thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%
> >
> > By + this patch,  I mean merging branch from this patch. Whereas
> > merging the previous:
> > commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
> > /proc/interrupts")
> >  It doesn't show any regression
>
> Since you are running perf, can you spot where the overhead occurs?

hmm... Difficult to say because tracing the bench decreases a lot the
result. I have pasted the perf reports.

With this patch :

# Samples: 634  of event 'cpu-clock'
# Event count (approx.): 158500000
#
# Overhead  Command     Shared Object       Symbol
# ........  ..........  ..................  ..................................
#
    31.86%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_unlock_irqrestore
     8.68%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_unlock_irq
     6.31%  sched-pipe  [kernel.kallsyms]   [k] __schedule
     5.21%  sched-pipe  [kernel.kallsyms]   [k] schedule
     4.73%  sched-pipe  [kernel.kallsyms]   [k] pipe_read
     3.31%  sched-pipe  [kernel.kallsyms]   [k] el0_svc_common.constprop.3
     2.84%  sched-pipe  [kernel.kallsyms]   [k] ww_mutex_lock_interruptible
     2.52%  sched-pipe  [kernel.kallsyms]   [k] init_wait_entry
     2.37%  sched-pipe  [kernel.kallsyms]   [k] mutex_unlock
     2.21%  sched-pipe  [kernel.kallsyms]   [k] new_sync_read
     1.89%  sched-pipe  [kernel.kallsyms]   [k] new_sync_write
     1.74%  sched-pipe  [kernel.kallsyms]   [k] security_file_permission
     1.74%  sched-pipe  [kernel.kallsyms]   [k] vfs_read
     1.58%  sched-pipe  [kernel.kallsyms]   [k] __my_cpu_offset
     1.26%  sched-pipe  libpthread-2.24.so  [.] 0x0000000000010a2c
     1.10%  sched-pipe  [kernel.kallsyms]   [k] mutex_lock
     1.10%  sched-pipe  [kernel.kallsyms]   [k] vfs_write

After reverting this patch which gives a result similar to v5.9:

# Samples: 659  of event 'cpu-clock'
# Event count (approx.): 164750000
#
# Overhead  Command     Shared Object       Symbol
# ........  ..........  ..................  ...............................
#
    29.29%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_unlock_irqrestore
    21.40%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_unlock_irq
     4.86%  sched-pipe  [kernel.kallsyms]   [k] pipe_read
     4.55%  sched-pipe  [kernel.kallsyms]   [k] ww_mutex_lock_interruptible
     2.88%  sched-pipe  [kernel.kallsyms]   [k] __schedule
     2.88%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_lock_irqsave
     2.88%  sched-pipe  [kernel.kallsyms]   [k] schedule
     2.12%  sched-pipe  [kernel.kallsyms]   [k] new_sync_read
     1.82%  sched-pipe  [kernel.kallsyms]   [k] mutex_lock
     1.67%  sched-pipe  [kernel.kallsyms]   [k] el0_svc_common.constprop.3
     1.67%  sched-pipe  [kernel.kallsyms]   [k] pipe_write
     1.21%  sched-pipe  [kernel.kallsyms]   [k] rw_verify_area
     1.21%  sched-pipe  [kernel.kallsyms]   [k] security_file_permission
     1.06%  sched-pipe  [kernel.kallsyms]   [k] fsnotify

I have only put symbol with overhead above 1%

so _raw_spin_unlock_irq, schedule and __schedule seem the most
impacted but i can't get any conclusion

I can sent you perf.data files if you want


>
> Thanks,
>
>          M.
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Valentin Schneider]

Hi,

On 19/10/20 16:43, Vincent Guittot wrote:
> On Mon, 19 Oct 2020 at 15:04, Marc Zyngier <maz@kernel.org> wrote:
>> Since you are running perf, can you spot where the overhead occurs?
>
> hmm... Difficult to say because tracing the bench decreases a lot the
> result. I have pasted the perf reports.
>

<snip>

> I have only put symbol with overhead above 1%
>
> so _raw_spin_unlock_irq, schedule and __schedule seem the most
> impacted but i can't get any conclusion
>

AFAICT on TX2 you should be able to run these and get some more details
within IRQ-disabled regions:

https://lore.kernel.org/linux-arm-kernel/20200924110706.254996-1-alexandru.elisei@arm.com/

(they should be on linux-next)

> I can sent you perf.data files if you want
>
>
>>
>> Thanks,
>>
>>          M.
>> --
>> Jazz is not dead. It just smells funny...

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Vincent Guittot]

HI Marc,

On Mon, 19 Oct 2020 at 17:43, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Mon, 19 Oct 2020 at 15:04, Marc Zyngier <maz@kernel.org> wrote:
> >

...

> > >>
> > >> One of the major difference is that we end up, in some cases
> > >> (such as when performing IRQ time accounting on the scheduler
> > >> IPI), end up with nested irq_enter()/irq_exit() pairs.
> > >> Other than the (relatively small) overhead, there should be
> > >> no consequences to it (these pairs are designed to nest
> > >> correctly, and the accounting shouldn't be off).
> > >
> > > While rebasing on mainline, I have faced a performance regression for
> > > the benchmark:
> > > perf bench sched pipe
> > > on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)
> > >
> > > The regression comes from:
> > > commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal
> > > interrupts")
> >
> > That's interesting, as this patch doesn't really change anything (most
> > of the potential overhead comes in later). The only potential overhead
> > I can see is that the scheduler_ipi() call is now wrapped around
> > irq_enter()/irq_exit().
> >
> > >
> > >           v5.9              + this patch
> > > hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
> > > thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%
> > >
> > > By + this patch,  I mean merging branch from this patch. Whereas
> > > merging the previous:
> > > commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
> > > /proc/interrupts")
> > >  It doesn't show any regression
> >
> > Since you are running perf, can you spot where the overhead occurs?

Any idea about the root cause of the regression ?
I have faced it on more arm64 platforms in the meantime

>
> hmm... Difficult to say because tracing the bench decreases a lot the
> result. I have pasted the perf reports.
>
> With this patch :
>
> # Samples: 634  of event 'cpu-clock'
> # Event count (approx.): 158500000
> #
> # Overhead  Command     Shared Object       Symbol
> # ........  ..........  ..................  ..................................
> #
>     31.86%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_unlock_irqrestore
>      8.68%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_unlock_irq
>      6.31%  sched-pipe  [kernel.kallsyms]   [k] __schedule
>      5.21%  sched-pipe  [kernel.kallsyms]   [k] schedule
>      4.73%  sched-pipe  [kernel.kallsyms]   [k] pipe_read
>      3.31%  sched-pipe  [kernel.kallsyms]   [k] el0_svc_common.constprop.3
>      2.84%  sched-pipe  [kernel.kallsyms]   [k] ww_mutex_lock_interruptible
>      2.52%  sched-pipe  [kernel.kallsyms]   [k] init_wait_entry
>      2.37%  sched-pipe  [kernel.kallsyms]   [k] mutex_unlock
>      2.21%  sched-pipe  [kernel.kallsyms]   [k] new_sync_read
>      1.89%  sched-pipe  [kernel.kallsyms]   [k] new_sync_write
>      1.74%  sched-pipe  [kernel.kallsyms]   [k] security_file_permission
>      1.74%  sched-pipe  [kernel.kallsyms]   [k] vfs_read
>      1.58%  sched-pipe  [kernel.kallsyms]   [k] __my_cpu_offset
>      1.26%  sched-pipe  libpthread-2.24.so  [.] 0x0000000000010a2c
>      1.10%  sched-pipe  [kernel.kallsyms]   [k] mutex_lock
>      1.10%  sched-pipe  [kernel.kallsyms]   [k] vfs_write
>
> After reverting this patch which gives a result similar to v5.9:
>
> # Samples: 659  of event 'cpu-clock'
> # Event count (approx.): 164750000
> #
> # Overhead  Command     Shared Object       Symbol
> # ........  ..........  ..................  ...............................
> #
>     29.29%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_unlock_irqrestore
>     21.40%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_unlock_irq
>      4.86%  sched-pipe  [kernel.kallsyms]   [k] pipe_read
>      4.55%  sched-pipe  [kernel.kallsyms]   [k] ww_mutex_lock_interruptible
>      2.88%  sched-pipe  [kernel.kallsyms]   [k] __schedule
>      2.88%  sched-pipe  [kernel.kallsyms]   [k] _raw_spin_lock_irqsave
>      2.88%  sched-pipe  [kernel.kallsyms]   [k] schedule
>      2.12%  sched-pipe  [kernel.kallsyms]   [k] new_sync_read
>      1.82%  sched-pipe  [kernel.kallsyms]   [k] mutex_lock
>      1.67%  sched-pipe  [kernel.kallsyms]   [k] el0_svc_common.constprop.3
>      1.67%  sched-pipe  [kernel.kallsyms]   [k] pipe_write
>      1.21%  sched-pipe  [kernel.kallsyms]   [k] rw_verify_area
>      1.21%  sched-pipe  [kernel.kallsyms]   [k] security_file_permission
>      1.06%  sched-pipe  [kernel.kallsyms]   [k] fsnotify
>
> I have only put symbol with overhead above 1%
>
> so _raw_spin_unlock_irq, schedule and __schedule seem the most
> impacted but i can't get any conclusion
>
> I can sent you perf.data files if you want
>
>
> >
> > Thanks,
> >
> >          M.
> > --
> > Jazz is not dead. It just smells funny...

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Marc Zyngier]

On 2020-10-27 10:12, Vincent Guittot wrote:
> HI Marc,
> 
> On Mon, 19 Oct 2020 at 17:43, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
>> 
>> On Mon, 19 Oct 2020 at 15:04, Marc Zyngier <maz@kernel.org> wrote:
>> >
> 
> ...
> 
>> > >>
>> > >> One of the major difference is that we end up, in some cases
>> > >> (such as when performing IRQ time accounting on the scheduler
>> > >> IPI), end up with nested irq_enter()/irq_exit() pairs.
>> > >> Other than the (relatively small) overhead, there should be
>> > >> no consequences to it (these pairs are designed to nest
>> > >> correctly, and the accounting shouldn't be off).
>> > >
>> > > While rebasing on mainline, I have faced a performance regression for
>> > > the benchmark:
>> > > perf bench sched pipe
>> > > on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)
>> > >
>> > > The regression comes from:
>> > > commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal
>> > > interrupts")
>> >
>> > That's interesting, as this patch doesn't really change anything (most
>> > of the potential overhead comes in later). The only potential overhead
>> > I can see is that the scheduler_ipi() call is now wrapped around
>> > irq_enter()/irq_exit().
>> >
>> > >
>> > >           v5.9              + this patch
>> > > hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
>> > > thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%
>> > >
>> > > By + this patch,  I mean merging branch from this patch. Whereas
>> > > merging the previous:
>> > > commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
>> > > /proc/interrupts")
>> > >  It doesn't show any regression
>> >
>> > Since you are running perf, can you spot where the overhead occurs?
> 
> Any idea about the root cause of the regression ?
> I have faced it on more arm64 platforms in the meantime

two possible causes:

(1) irq_enter/exit on the rescheduling IPI means we reschedule much more 
often
(2) irq_domain lookups add some overhead.

For (1), I have this series[1] which is ugly as sin and needs much more 
testing.

For (2), I have some ideas which need more work (let the irq domain 
resolve to
an irq_desc instead of an interrupt number, avoiding another radix-tree 
lookup).

         M.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-fixes
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Vincent Guittot]

On Tue, 27 Oct 2020 at 11:37, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-10-27 10:12, Vincent Guittot wrote:
> > HI Marc,
> >
> > On Mon, 19 Oct 2020 at 17:43, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> >>
> >> On Mon, 19 Oct 2020 at 15:04, Marc Zyngier <maz@kernel.org> wrote:
> >> >
> >
> > ...
> >
> >> > >>
> >> > >> One of the major difference is that we end up, in some cases
> >> > >> (such as when performing IRQ time accounting on the scheduler
> >> > >> IPI), end up with nested irq_enter()/irq_exit() pairs.
> >> > >> Other than the (relatively small) overhead, there should be
> >> > >> no consequences to it (these pairs are designed to nest
> >> > >> correctly, and the accounting shouldn't be off).
> >> > >
> >> > > While rebasing on mainline, I have faced a performance regression for
> >> > > the benchmark:
> >> > > perf bench sched pipe
> >> > > on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)
> >> > >
> >> > > The regression comes from:
> >> > > commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal
> >> > > interrupts")
> >> >
> >> > That's interesting, as this patch doesn't really change anything (most
> >> > of the potential overhead comes in later). The only potential overhead
> >> > I can see is that the scheduler_ipi() call is now wrapped around
> >> > irq_enter()/irq_exit().
> >> >
> >> > >
> >> > >           v5.9              + this patch
> >> > > hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
> >> > > thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%
> >> > >
> >> > > By + this patch,  I mean merging branch from this patch. Whereas
> >> > > merging the previous:
> >> > > commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
> >> > > /proc/interrupts")
> >> > >  It doesn't show any regression
> >> >
> >> > Since you are running perf, can you spot where the overhead occurs?
> >
> > Any idea about the root cause of the regression ?
> > I have faced it on more arm64 platforms in the meantime
>
> two possible causes:
>
> (1) irq_enter/exit on the rescheduling IPI means we reschedule much more
> often
> (2) irq_domain lookups add some overhead.
>
> For (1), I have this series[1] which is ugly as sin and needs much more
> testing.

Ok, I'm going to test this series to see if it fixes the perf regression

>
> For (2), I have some ideas which need more work (let the irq domain
> resolve to
> an irq_desc instead of an interrupt number, avoiding another radix-tree
> lookup).
>
>          M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-fixes
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Vincent Guittot]

On Tue, 27 Oct 2020 at 11:50, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Tue, 27 Oct 2020 at 11:37, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On 2020-10-27 10:12, Vincent Guittot wrote:
> > > HI Marc,
> > >
> > > On Mon, 19 Oct 2020 at 17:43, Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > >>
> > >> On Mon, 19 Oct 2020 at 15:04, Marc Zyngier <maz@kernel.org> wrote:
> > >> >
> > >
> > > ...
> > >
> > >> > >>
> > >> > >> One of the major difference is that we end up, in some cases
> > >> > >> (such as when performing IRQ time accounting on the scheduler
> > >> > >> IPI), end up with nested irq_enter()/irq_exit() pairs.
> > >> > >> Other than the (relatively small) overhead, there should be
> > >> > >> no consequences to it (these pairs are designed to nest
> > >> > >> correctly, and the accounting shouldn't be off).
> > >> > >
> > >> > > While rebasing on mainline, I have faced a performance regression for
> > >> > > the benchmark:
> > >> > > perf bench sched pipe
> > >> > > on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)
> > >> > >
> > >> > > The regression comes from:
> > >> > > commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal
> > >> > > interrupts")
> > >> >
> > >> > That's interesting, as this patch doesn't really change anything (most
> > >> > of the potential overhead comes in later). The only potential overhead
> > >> > I can see is that the scheduler_ipi() call is now wrapped around
> > >> > irq_enter()/irq_exit().
> > >> >
> > >> > >
> > >> > >           v5.9              + this patch
> > >> > > hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
> > >> > > thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%
> > >> > >
> > >> > > By + this patch,  I mean merging branch from this patch. Whereas
> > >> > > merging the previous:
> > >> > > commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
> > >> > > /proc/interrupts")
> > >> > >  It doesn't show any regression
> > >> >
> > >> > Since you are running perf, can you spot where the overhead occurs?
> > >
> > > Any idea about the root cause of the regression ?
> > > I have faced it on more arm64 platforms in the meantime
> >
> > two possible causes:
> >
> > (1) irq_enter/exit on the rescheduling IPI means we reschedule much more
> > often
> > (2) irq_domain lookups add some overhead.
> >
> > For (1), I have this series[1] which is ugly as sin and needs much more
> > testing.
>
> Ok, I'm going to test this series to see if it fixes the perf regression

You have spotted the root cause of the regression. We are back to ~1%
performance diff on the hikey

>
> >
> > For (2), I have some ideas which need more work (let the irq domain
> > resolve to
> > an irq_desc instead of an interrupt number, avoiding another radix-tree
> > lookup).
> >
> >          M.
> >
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/ipi-fixes
> > --
> > Jazz is not dead. It just smells funny...

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Marc Zyngier]

On 2020-10-27 11:21, Vincent Guittot wrote:
> On Tue, 27 Oct 2020 at 11:50, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
>> 
>> On Tue, 27 Oct 2020 at 11:37, Marc Zyngier <maz@kernel.org> wrote:
>> >
>> > On 2020-10-27 10:12, Vincent Guittot wrote:
>> > > HI Marc,
>> > >
>> > > On Mon, 19 Oct 2020 at 17:43, Vincent Guittot
>> > > <vincent.guittot@linaro.org> wrote:
>> > >>
>> > >> On Mon, 19 Oct 2020 at 15:04, Marc Zyngier <maz@kernel.org> wrote:
>> > >> >
>> > >
>> > > ...
>> > >
>> > >> > >>
>> > >> > >> One of the major difference is that we end up, in some cases
>> > >> > >> (such as when performing IRQ time accounting on the scheduler
>> > >> > >> IPI), end up with nested irq_enter()/irq_exit() pairs.
>> > >> > >> Other than the (relatively small) overhead, there should be
>> > >> > >> no consequences to it (these pairs are designed to nest
>> > >> > >> correctly, and the accounting shouldn't be off).
>> > >> > >
>> > >> > > While rebasing on mainline, I have faced a performance regression for
>> > >> > > the benchmark:
>> > >> > > perf bench sched pipe
>> > >> > > on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)
>> > >> > >
>> > >> > > The regression comes from:
>> > >> > > commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal
>> > >> > > interrupts")
>> > >> >
>> > >> > That's interesting, as this patch doesn't really change anything (most
>> > >> > of the potential overhead comes in later). The only potential overhead
>> > >> > I can see is that the scheduler_ipi() call is now wrapped around
>> > >> > irq_enter()/irq_exit().
>> > >> >
>> > >> > >
>> > >> > >           v5.9              + this patch
>> > >> > > hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
>> > >> > > thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%
>> > >> > >
>> > >> > > By + this patch,  I mean merging branch from this patch. Whereas
>> > >> > > merging the previous:
>> > >> > > commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
>> > >> > > /proc/interrupts")
>> > >> > >  It doesn't show any regression
>> > >> >
>> > >> > Since you are running perf, can you spot where the overhead occurs?
>> > >
>> > > Any idea about the root cause of the regression ?
>> > > I have faced it on more arm64 platforms in the meantime
>> >
>> > two possible causes:
>> >
>> > (1) irq_enter/exit on the rescheduling IPI means we reschedule much more
>> > often
>> > (2) irq_domain lookups add some overhead.
>> >
>> > For (1), I have this series[1] which is ugly as sin and needs much more
>> > testing.
>> 
>> Ok, I'm going to test this series to see if it fixes the perf 
>> regression
> 
> You have spotted the root cause of the regression. We are back to ~1%
> performance diff on the hikey

Yeah. Only thing is that I can't look at this hack without vomiting...

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

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Vincent Guittot]

On Tue, 27 Oct 2020 at 13:06, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-10-27 11:21, Vincent Guittot wrote:
> > On Tue, 27 Oct 2020 at 11:50, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> >>
> >> On Tue, 27 Oct 2020 at 11:37, Marc Zyngier <maz@kernel.org> wrote:
> >> >
> >> > On 2020-10-27 10:12, Vincent Guittot wrote:
> >> > > HI Marc,
> >> > >
> >> > > On Mon, 19 Oct 2020 at 17:43, Vincent Guittot
> >> > > <vincent.guittot@linaro.org> wrote:
> >> > >>
> >> > >> On Mon, 19 Oct 2020 at 15:04, Marc Zyngier <maz@kernel.org> wrote:
> >> > >> >
> >> > >
> >> > > ...
> >> > >
> >> > >> > >>
> >> > >> > >> One of the major difference is that we end up, in some cases
> >> > >> > >> (such as when performing IRQ time accounting on the scheduler
> >> > >> > >> IPI), end up with nested irq_enter()/irq_exit() pairs.
> >> > >> > >> Other than the (relatively small) overhead, there should be
> >> > >> > >> no consequences to it (these pairs are designed to nest
> >> > >> > >> correctly, and the accounting shouldn't be off).
> >> > >> > >
> >> > >> > > While rebasing on mainline, I have faced a performance regression for
> >> > >> > > the benchmark:
> >> > >> > > perf bench sched pipe
> >> > >> > > on my arm64 dual quad core (hikey) and my 2 nodes x 112 CPUS (thx2)
> >> > >> > >
> >> > >> > > The regression comes from:
> >> > >> > > commit: d3afc7f12987 ("arm64: Allow IPIs to be handled as normal
> >> > >> > > interrupts")
> >> > >> >
> >> > >> > That's interesting, as this patch doesn't really change anything (most
> >> > >> > of the potential overhead comes in later). The only potential overhead
> >> > >> > I can see is that the scheduler_ipi() call is now wrapped around
> >> > >> > irq_enter()/irq_exit().
> >> > >> >
> >> > >> > >
> >> > >> > >           v5.9              + this patch
> >> > >> > > hikey :   48818(+/- 0.31)   37503(+/- 0.15%)  -23.2%
> >> > >> > > thx2  :  132410(+/- 1.72)  122646(+/- 1.92%)   -7.4%
> >> > >> > >
> >> > >> > > By + this patch,  I mean merging branch from this patch. Whereas
> >> > >> > > merging the previous:
> >> > >> > > commit: 83cfac95c018 ("genirq: Allow interrupts to be excluded from
> >> > >> > > /proc/interrupts")
> >> > >> > >  It doesn't show any regression
> >> > >> >
> >> > >> > Since you are running perf, can you spot where the overhead occurs?
> >> > >
> >> > > Any idea about the root cause of the regression ?
> >> > > I have faced it on more arm64 platforms in the meantime
> >> >
> >> > two possible causes:
> >> >
> >> > (1) irq_enter/exit on the rescheduling IPI means we reschedule much more
> >> > often
> >> > (2) irq_domain lookups add some overhead.
> >> >
> >> > For (1), I have this series[1] which is ugly as sin and needs much more
> >> > testing.
> >>
> >> Ok, I'm going to test this series to see if it fixes the perf
> >> regression
> >
> > You have spotted the root cause of the regression. We are back to ~1%
> > performance diff on the hikey
>
> Yeah. Only thing is that I can't look at this hack without vomiting...

At least, we know the root cause and the impact of irq_enter/exit
>
>          M.
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Marc Zyngier]

On Thu, 06 May 2021 08:50:42 +0100,
He Ying <heying24@huawei.com> wrote:
> 
> Hello Marc,
> 
> We have faced a performance regression for handling ipis since this
> commit. I think it's the same issue reported by Vincent.

Can you share more details on what regression you have observed?
What's the workload, the system, the performance drop?

> I found you pointed out the possible two causes:
> 
> (1) irq_enter/exit on the rescheduling IPI means we reschedule much
> more often.

It turned out to be a red herring. We don't reschedule more often, but
we instead suffer from the overhead of irq_enter()/irq_exit().
However, this only matters for silly benchmarks, and no real-life
workload showed any significant regression. Have you identified such
realistic workload?

> (2) irq_domain lookups add some overhead.

While this is also a potential source of overhead, it turned out not
to be the case.

> But I don't see any following patches in mainline. So, are you still
> working on this issue?  Looking forward to your reply.

See [1]. However, there is probably better things to do than this
low-level specialisation of IPIs, and Thomas outlined what needs to be
done (see v1 of the patch series).

Thanks,

	M.

[1] https://lore.kernel.org/lkml/20201124141449.572446-1-maz@kernel.org/

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[He Ying]

在 2021/5/6 19:44, Marc Zyngier 写道:
> On Thu, 06 May 2021 08:50:42 +0100,
> He Ying <heying24@huawei.com> wrote:
>> Hello Marc,
>>
>> We have faced a performance regression for handling ipis since this
>> commit. I think it's the same issue reported by Vincent.
> Can you share more details on what regression you have observed?
> What's the workload, the system, the performance drop?

OK. We have just calculated the pmu cycles from the entry of gic_handle_irq

to the entry of do_handle_ipi. Here is some more information about our test:

CPU: Hisilicon hip05-d02

Applying the patch series: 1115 cycles

Reverting the patch series: 599 cycles

>
>> I found you pointed out the possible two causes:
>>
>> (1) irq_enter/exit on the rescheduling IPI means we reschedule much
>> more often.
> It turned out to be a red herring. We don't reschedule more often, but
> we instead suffer from the overhead of irq_enter()/irq_exit().
> However, this only matters for silly benchmarks, and no real-life
> workload showed any significant regression. Have you identified such
> realistic workload?

I'm afraid not. We just run some benchmarks and calculated pmu cycle 
counters.

But we have observed running time from the entry of gic_handle_irq to 
the entry

of do_handle_ipi almost doubles. Doesn't it affect realistic workload?

>
>> (2) irq_domain lookups add some overhead.
> While this is also a potential source of overhead, it turned out not
> to be the case.
OK.
>
>> But I don't see any following patches in mainline. So, are you still
>> working on this issue?  Looking forward to your reply.
> See [1]. However, there is probably better things to do than this
> low-level specialisation of IPIs, and Thomas outlined what needs to be
> done (see v1 of the patch series).

OK. I see the patch series. Would it be applied to the mainline someday? 
I notice

that more than 5 months have passed since you sent the patch series.


Thanks.

>
> Thanks,
>
> 	M.
>
> [1] https://lore.kernel.org/lkml/20201124141449.572446-1-maz@kernel.org/
>

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[Marc Zyngier]

On Fri, 07 May 2021 08:30:06 +0100,
He Ying <heying24@huawei.com> wrote:
> 
> 
> 在 2021/5/6 19:44, Marc Zyngier 写道:
> > On Thu, 06 May 2021 08:50:42 +0100,
> > He Ying <heying24@huawei.com> wrote:
> >> Hello Marc,
> >> 
> >> We have faced a performance regression for handling ipis since this
> >> commit. I think it's the same issue reported by Vincent.
> > Can you share more details on what regression you have observed?
> > What's the workload, the system, the performance drop?
> 
> OK. We have just calculated the pmu cycles from the entry of gic_handle_irq
> to the entry of do_handle_ipi. Here is some more information about our test:
> 
> CPU: Hisilicon hip05-d02
> 
> Applying the patch series: 1115 cycles
> Reverting the patch series: 599 cycles

And? How is that meaningful? Interrupts are pretty rare compared to
everything that happens in the system. How does it affect the
behaviour of the system as a whole?

> 
> > 
> >> I found you pointed out the possible two causes:
> >> 
> >> (1) irq_enter/exit on the rescheduling IPI means we reschedule much
> >> more often.
> > It turned out to be a red herring. We don't reschedule more often, but
> > we instead suffer from the overhead of irq_enter()/irq_exit().
> > However, this only matters for silly benchmarks, and no real-life
> > workload showed any significant regression. Have you identified such
> > realistic workload?
> 
> I'm afraid not. We just run some benchmarks and calculated pmu cycle
> counters.  But we have observed running time from the entry of
> gic_handle_irq to the entry of do_handle_ipi almost doubles. Doesn't
> it affect realistic workload?

Then I'm not that interested. Show me an actual regression in a real
workload that affects people, and I'll be a bit more sympathetic to
your complain. But quoting raw numbers do not help.

There is a number of advantages to having IPI as IRQs, as it allows us
to deal with proper allocation (other subsystem want to use IPIs), and
eventually NMIs. There is a trade-off, and if that means wasting a few
cycles, so be it.

> >> (2) irq_domain lookups add some overhead.
> > While this is also a potential source of overhead, it turned out not
> > to be the case.
> OK.
> > 
> >> But I don't see any following patches in mainline. So, are you still
> >> working on this issue?  Looking forward to your reply.
> > See [1]. However, there is probably better things to do than this
> > low-level specialisation of IPIs, and Thomas outlined what needs to be
> > done (see v1 of the patch series).
> 
> OK. I see the patch series. Would it be applied to the mainline
> someday? I notice that more than 5 months have passed since you sent
> the patch series.

I have no plan to merge these patches any time soon, given that nobody
has shown a measurable regression using something other than a trivial
benchmark. If you come up with such an example, I will of course
reconsider this position.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 03/16] arm64: Allow IPIs to be handled as normal interrupts

[He Ying]

在 2021/5/7 16:56, Marc Zyngier 写道:
> On Fri, 07 May 2021 08:30:06 +0100,
> He Ying <heying24@huawei.com> wrote:
>>
>> 在 2021/5/6 19:44, Marc Zyngier 写道:
>>> On Thu, 06 May 2021 08:50:42 +0100,
>>> He Ying <heying24@huawei.com> wrote:
>>>> Hello Marc,
>>>>
>>>> We have faced a performance regression for handling ipis since this
>>>> commit. I think it's the same issue reported by Vincent.
>>> Can you share more details on what regression you have observed?
>>> What's the workload, the system, the performance drop?
>> OK. We have just calculated the pmu cycles from the entry of gic_handle_irq
>> to the entry of do_handle_ipi. Here is some more information about our test:
>>
>> CPU: Hisilicon hip05-d02
>>
>> Applying the patch series: 1115 cycles
>> Reverting the patch series: 599 cycles
> And? How is that meaningful? Interrupts are pretty rare compared to
> everything that happens in the system. How does it affect the
> behaviour of the system as a whole?
OK.
>
>>>> I found you pointed out the possible two causes:
>>>>
>>>> (1) irq_enter/exit on the rescheduling IPI means we reschedule much
>>>> more often.
>>> It turned out to be a red herring. We don't reschedule more often, but
>>> we instead suffer from the overhead of irq_enter()/irq_exit().
>>> However, this only matters for silly benchmarks, and no real-life
>>> workload showed any significant regression. Have you identified such
>>> realistic workload?
>> I'm afraid not. We just run some benchmarks and calculated pmu cycle
>> counters.  But we have observed running time from the entry of
>> gic_handle_irq to the entry of do_handle_ipi almost doubles. Doesn't
>> it affect realistic workload?
> Then I'm not that interested. Show me an actual regression in a real
> workload that affects people, and I'll be a bit more sympathetic to
> your complain. But quoting raw numbers do not help.
>
> There is a number of advantages to having IPI as IRQs, as it allows us
> to deal with proper allocation (other subsystem want to use IPIs), and
> eventually NMIs. There is a trade-off, and if that means wasting a few
> cycles, so be it.
OK. I see.
>
>>>> (2) irq_domain lookups add some overhead.
>>> While this is also a potential source of overhead, it turned out not
>>> to be the case.
>> OK.
>>>> But I don't see any following patches in mainline. So, are you still
>>>> working on this issue?  Looking forward to your reply.
>>> See [1]. However, there is probably better things to do than this
>>> low-level specialisation of IPIs, and Thomas outlined what needs to be
>>> done (see v1 of the patch series).
>> OK. I see the patch series. Would it be applied to the mainline
>> someday? I notice that more than 5 months have passed since you sent
>> the patch series.
> I have no plan to merge these patches any time soon, given that nobody
> has shown a measurable regression using something other than a trivial
> benchmark. If you come up with such an example, I will of course
> reconsider this position.

OK. Thanks a lot for all your reply. If I come up with a measurable 
regression

with a realistic workload, I'll contact you again.


Thanks.

>
> Thanks,
>
> 	M.
>