Skip to contentSkip to conversation

arm/arm64: Turning IPIs into normal interrupts

PATCH Applied Marc Zyngier 8 participants 50 messages Jun 24, 2020 – Sep 22, 2021

Subsystems (9)

v2 of 3 Applied Latest is v3 →

lore
Share & tools
lore

Marc Zyngier

author maintainer
#0 lore raw

v2 cover 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.

* 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 (17):
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: Atomically update affinity
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             | 119 ++++++++-----
 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        |  99 ++++++-----
 drivers/irqchip/irq-gic.c           | 183 ++++++++++---------
 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, 713 insertions(+), 413 deletions(-)
Signature · 1 line
2.27.0
author maintainer
#1 lore raw

PATCH v2 1/17 genirq: Add fasteoi IPI flow

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

---
 include/linux/irq.h |  1 +
 kernel/irq/chip.c   | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)
include/linux/irq.h +1 -0
index 8d5bc2c237d7..726f94d8b8cc 100644
@@ -621,6 +621,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);
kernel/irq/chip.c +27 -0
index 41e7e37a0928..b9c49a61f8bb 100644
@@ -955,6 +955,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
Signature · 1 line
2.27.0
author maintainer
#2 lore raw

PATCH v2 2/17 genirq: Allow interrupts to be excluded from /proc/interrupts

In reply to 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.

---
 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(-)
include/linux/irq.h +3 -1
index 726f94d8b8cc..48f23533aec8 100644
@@ -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)
 
kernel/irq/debugfs.c +1 -0
index 4f9f844074db..ce93626470c3 100644
@@ -131,6 +131,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[] = {
kernel/irq/proc.c +1 -1
index 32c071d7bc03..72513ed2a5fc 100644
@@ -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)
kernel/irq/settings.h +7 -0
index e43795cd2ccf..403378b9947b 100644
@@ -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;
+}
Signature · 1 line
2.27.0
author maintainer
#3 lore raw

PATCH v2 3/17 arm64: Allow IPIs to be handled as normal interrupts

In reply to 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).

---
 arch/arm64/Kconfig           |  1 +
 arch/arm64/include/asm/smp.h |  5 ++
 arch/arm64/kernel/smp.c      | 91 +++++++++++++++++++++++++++++++-----
 3 files changed, 85 insertions(+), 12 deletions(-)
arch/arm64/Kconfig +1 -0
index a4a094bedcb2..fe88d28bb985 100644
@@ -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
arch/arm64/include/asm/smp.h +5 -0
index ea268d88b6f7..4537a4620e3d 100644
@@ -74,6 +74,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.
  */
arch/arm64/kernel/smp.c +79 -12

154 lines not on this page · raw message

Signature · 1 line
2.27.0
#4 lore raw

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

Quoted 28 lines · Marc Zyngier
arch/arm64/kernel/smp.c +21 -0
@@ -958,9 +958,76 @@ 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);
+}

arch/arm64/kernel/smp.c void handle_IPI(int ipinr, struct pt_regs *regs) · line 994

Quoted 10 lines
arch/arm64/kernel/smp.c +10 -0
+
+static void ipi_setup(int cpu)
+{
+	if (ipi_irq_base) {
+		int i;
+
+		for (i = 0; i < nr_ipi; i++)
+			enable_percpu_irq(ipi_irq_base + i, 0);
+	}
+}

Nit: Once we have the irqchip changes in, should we warn & bail out when !ipi_irq_base? Ditto for the teardown

Quoted 10 lines
arch/arm64/kernel/smp.c +10 -0
+
+static void ipi_teardown(int cpu)
+{
+	if (ipi_irq_base) {
+		int i;
+
+		for (i = 0; i < nr_ipi; i++)
+			disable_percpu_irq(ipi_irq_base + i);
+	}
+}
#5 lore raw

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

In reply to Marc Zyngier

Itsy bitsy nit below from re-reading this.

arch/arm64/kernel/smp.c enum ipi_msg_type { · line 80

Quoted 10 lines · Marc Zyngier
arch/arm64/kernel/smp.c +2 -0
index e43a8ff19f0f..c08950cc09cc 100644
@@ -76,6 +76,13 @@ enum ipi_msg_type {
      IPI_WAKEUP
 };
 
+static int ipi_irq_base;
+static int nr_ipi = NR_IPI;

If we really care, these two could be marked as __read_mostly.

Quoted 8 lines
arch/arm64/kernel/smp.c +5 -0
+static struct irq_desc *ipi_desc[NR_IPI];
+
+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
author maintainer
#6 lore raw

PATCH v2 4/17 ARM: Allow IPIs to be handled as normal interrupts

In reply to 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).

---
 arch/arm/Kconfig           |  1 +
 arch/arm/include/asm/smp.h |  5 ++
 arch/arm/kernel/smp.c      | 99 ++++++++++++++++++++++++++++++++------
 3 files changed, 89 insertions(+), 16 deletions(-)
arch/arm/Kconfig +1 -0
index 2ac74904a3ce..27c26d3c06f2 100644
@@ -48,6 +48,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
arch/arm/include/asm/smp.h +5 -0
index a91f21e3c5b5..0e29730295ca 100644
@@ -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.
arch/arm/kernel/smp.c +83 -16

177 lines not on this page · raw message

Signature · 1 line
2.27.0
#7 lore raw

Re: [PATCH v2 04/17] ARM: Allow IPIs to be handled as normal interrupts

Quoted 45 lines · Marc Zyngier
arch/arm/kernel/smp.c +38 -0
@@ -696,9 +696,76 @@ 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)
+{
+	if (ipi_irq_base) {
+		int i;
+
+		for (i = 0; i < nr_ipi; i++)
+			enable_percpu_irq(ipi_irq_base + i, 0);
+	}
+}
+
+static void ipi_teardown(int cpu)
+{
+	if (ipi_irq_base) {
+		int i;
+
+		for (i = 0; i < nr_ipi; i++)

arch/arm/kernel/smp.c void handle_IPI(int ipinr, struct pt_regs *regs) · line 749

Quoted 10 lines
arch/arm/kernel/smp.c +10 -0
+			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);

I got confused by that backtrace thing and NR_IPI vs MAX_IPI. I think I got it now : we don't want to call trace_ipi_raise() for IPI_CPU_BACKTRACE *but* we still need to alloc the desc and route it through the generic IPI layers.

The only difference I can tell is that now we will get some trace events for it via the handler entry/exit tracepoints - that shouldn't cause any issue.

Quoted 22 lines
arch/arm/kernel/smp.c +19 -0
+
+	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);
author maintainer
#8 lore raw

Re: [PATCH v2 04/17] ARM: Allow IPIs to be handled as normal interrupts

Quoted 55 lines · Valentin Schneider
Quoted 54 lines · Marc Zyngier
arch/arm/kernel/smp.c +3 -0
@@ -696,9 +696,76 @@ 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 */

arch/arm/kernel/smp.c +43 -0
+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)
+{
+	if (ipi_irq_base) {
+		int i;
+
+		for (i = 0; i < nr_ipi; i++)
+			enable_percpu_irq(ipi_irq_base + i, 0);
+	}
+}
+
+static void ipi_teardown(int cpu)
+{
+	if (ipi_irq_base) {
+		int i;
+
+		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;
+
Quoted 8 lines

arch/arm/kernel/smp.c void handle_IPI(int ipinr, struct pt_regs *regs) · line 749

Quoted 2 lines
arch/arm/kernel/smp.c +2 -0
+	WARN_ON(n < MAX_IPI);
+	nr_ipi = min(n, MAX_IPI);

I got confused by that backtrace thing and NR_IPI vs MAX_IPI. I think I got it now : we don't want to call trace_ipi_raise() for IPI_CPU_BACKTRACE *but* we still need to alloc the desc and route it through the generic IPI layers.

Indeed, and I didn't want to have a bizarre "+ 1" hanging about.

Quoted 3 lines

The only difference I can tell is that now we will get some trace events for it via the handler entry/exit tracepoints - that shouldn't cause any issue.

I hope so. I don't see how you can avoid all tracepoints anyway (if that was the intention).

Thanks,

        M.
Signature · 6 lines
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author maintainer
#9 lore raw

PATCH v2 5/17 irqchip/gic-v3: Describe the SGI range

In reply to 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.

---
 drivers/irqchip/irq-gic-v3.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)
drivers/irqchip/irq-gic-v3.c +15 -3
index cc46bc2d634b..19b294ed48ba 100644
@@ -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);
Signature · 1 line
2.27.0
author maintainer
#10 lore raw

PATCH v2 6/17 irqchip/gic-v3: Configure SGIs as standard interrupts

In reply to 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.

---
 drivers/irqchip/irq-gic-v3.c | 81 +++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 38 deletions(-)
drivers/irqchip/irq-gic-v3.c +43 -38
index 19b294ed48ba..d275e9b9533d 100644
@@ -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;
@@ -657,38 +659,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 +1114,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 +1132,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 +1141,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 +1207,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 +1250,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 +1268,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 +1284,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 +1319,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 +1661,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()) {
Signature · 1 line
2.27.0
#11 lore raw

Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts

Quoted 14 lines · 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.

---
 drivers/irqchip/irq-gic-v3.c | 81 +++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 38 deletions(-)
drivers/irqchip/irq-gic-v3.c +0 -0
index 19b294ed48ba..d275e9b9533d 100644

drivers/irqchip/irq-gic-v3.c line 40

Quoted 5 lines
drivers/irqchip/irq-gic-v3.c +2 -0
@@ -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)
+

Nit: this piqued my interest but ended up being just a define shuffle; As a member of the git speleologists' guild, I'd be overjoyed with having a small notion of that in the changelog.

Quoted 26 lines
drivers/irqchip/irq-gic-v3.c +1 -17
 struct redist_region {
      void __iomem		*redist_base;
      phys_addr_t		phys_base;
@@ -657,38 +659,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);

drivers/irqchip/irq-gic-v3.c static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs · line 687

Quoted 10 lines
drivers/irqchip/irq-gic-v3.c +0 -10
-		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.
-		 */

Isn't that still relevant?

Also, while staring at this it dawned on me that IPI's don't need the eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll get a gic_eoi_irq() just before calling into the irqaction. Dunno how much we care about it.

Quoted 13 lines
drivers/irqchip/irq-gic-v3.c +6 -4
-		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);
      }
 }
 
author maintainer
#12 lore raw

Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts

Quoted 19 lines · Valentin Schneider
Quoted 18 lines · 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.

---
 drivers/irqchip/irq-gic-v3.c | 81 
+++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 38 deletions(-)
drivers/irqchip/irq-gic-v3.c +0 -0
index 19b294ed48ba..d275e9b9533d 100644
@@ -36,6 +36,8 @@
 #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996	(1ULL << 0)
 #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539	(1ULL << 1)
 
Quoted 6 lines

drivers/irqchip/irq-gic-v3.c line 40

Quoted 2 lines
drivers/irqchip/irq-gic-v3.c +2 -0
+#define GIC_IRQ_TYPE_PARTITION	(GIC_IRQ_TYPE_LPI + 1)
+

Nit: this piqued my interest but ended up being just a define shuffle; As a member of the git speleologists' guild, I'd be overjoyed with having a small notion of that in the changelog.

Fair enough.

Quoted 32 lines
Quoted 32 lines
drivers/irqchip/irq-gic-v3.c +0 -0
 struct redist_region {
      void __iomem		*redist_base;
      phys_addr_t		phys_base;
@@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entry 

gic_handle_irq(struct pt_regs *regs

      if ((irqnr >= 1020 && irqnr <= 1023))
              return;
drivers/irqchip/irq-gic-v3.c +1 -21
-	/* 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
-		/*
Quoted 8 lines

drivers/irqchip/irq-gic-v3.c static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs · line 687

Quoted 6 lines
drivers/irqchip/irq-gic-v3.c +0 -6
-		 * 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.
-		 */

Isn't that still relevant?

It is. It is just that there is no really good place to put it. I may end-up just leaving it where it is.

Quoted 4 lines

Also, while staring at this it dawned on me that IPI's don't need the eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll get a gic_eoi_irq() just before calling into the irqaction. Dunno how much we care about it.

That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR.

Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though.

Thanks,

        M.
Signature · 1 line
Jazz is not dead. It just smells funny...
#13 lore raw

Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts

Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Valentin Schneider

Also, while staring at this it dawned on me that IPI's don't need the eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll get a
gic_eoi_irq() just before calling into the irqaction. Dunno how much we care about it.

Quoted 9 lines

That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR.

Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though.

If I got this one right:

39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and ->handle_irq")

you're describing case 2, which is indeed gone on gic-v3. However IIUC we also want an ISB between poking IAR and calling into the irqaction (case 1)
- we get just that with IPIs due to the early gic_eoi_irq(), but we don't for the other flows.

Quoted 3 lines

Thanks,

         M.
author maintainer
#14 lore raw

Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts

Quoted 20 lines · Valentin Schneider
Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Valentin Schneider

Also, while staring at this it dawned on me that IPI's don't need the eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll get a
gic_eoi_irq() just before calling into the irqaction. Dunno how much we care about it.

Quoted 9 lines

That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR.

Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though.

If I got this one right:

Quoted 8 lines

39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and
->handle_irq")

you're describing case 2, which is indeed gone on gic-v3. However IIUC we also want an ISB between poking IAR and calling into the irqaction (case 1)
- we get just that with IPIs due to the early gic_eoi_irq(), but we don't
for the other flows.

You just made me realise something amazing: I've started to forget about all this crap. Which is wonderful! ;-)

More seriously, you are absolutely right. If we wanted to address this, we'd probably have to give IPIs their own irqchip so that they get their own eoi callback. Not sure that's worth it.

        M.
Signature · 1 line
Jazz is not dead. It just smells funny...
#15 lore raw

Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts

Quoted 33 lines · Marc Zyngier
Quoted 24 lines · Valentin Schneider
Quoted 8 lines · Marc Zyngier
Quoted 6 lines · Valentin Schneider

Also, while staring at this it dawned on me that IPI's don't need the eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll get a
gic_eoi_irq() just before calling into the irqaction. Dunno how much we
care about it.

Quoted 9 lines

That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR.

Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though.

If I got this one right:

39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and
->handle_irq")

Quoted 7 lines

you're describing case 2, which is indeed gone on gic-v3. However IIUC we
also want an ISB between poking IAR and calling into the irqaction (case 1)
- we get just that with IPIs due to the early gic_eoi_irq(), but we don't
for the other flows.

Quoted 3 lines

You just made me realise something amazing: I've started to forget about all this crap. Which is wonderful! ;-)

:)

Quoted 4 lines

More seriously, you are absolutely right. If we wanted to address this, we'd probably have to give IPIs their own irqchip so that they get their own eoi callback. Not sure that's worth it.

I was initially thinking of something like

        if (static_branch_likely(&supports_deactivate_key))
                gic_write_eoir(irqnr);
        else if (__get_intid_range(irqnr) != SGI_RANGE)
                isb();

which is not particularly pretty, so maybe we should just slap this to the isb():

                /* Superfluous for SGIs, but who cares */
Quoted 1 line
         M.
author maintainer
#16 lore raw

PATCH v2 7/17 irqchip/gic: Atomically update affinity

In reply to Marc Zyngier

The GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.

But these sequences only expend into anything meaningful if the BL_SWITCHER option is selected, which almost never happens.

It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the GICD_TARGETRn registers, which when used make the update atomic by definition.

Drop the terminally broken code and replace it by a byte write.

Fixes 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")
---
 drivers/irqchip/irq-gic.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/irqchip/irq-gic.c +3 -11
index 00de05abd3c3..c17fabd6741e 100644
@@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
 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) & ~3);
-	unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
-	u32 val, mask, bit;
-	unsigned long flags;
+	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);
@@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	gic_lock_irqsave(flags);
-	mask = 0xff << shift;
-	bit = gic_cpu_map[cpu] << shift;
-	val = readl_relaxed(reg) & ~mask;
-	writel_relaxed(val | bit, reg);
-	gic_unlock_irqrestore(flags);
-
+	writeb_relaxed(gic_cpu_map[cpu], reg);
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
 	return IRQ_SET_MASK_OK_DONE;
Signature · 1 line
2.27.0
maintainer
#17 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag fixing commit: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature").

The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228.

v5.7.6: Build OK!
v5.4.49: Build OK!
v4.19.130: Build OK!
v4.14.186: Build OK!
v4.9.228: Failed to apply! Possible dependencies:
0c9e498286ef9 ("irqchip/gic: Report that effective affinity is a single target")

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

Signature · 2 lines
Thanks
Sasha
maintainer
#18 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

In reply to Marc Zyngier

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag fixing commit: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature").

The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228.

v5.7.6: Build OK!
v5.4.49: Build OK!
v4.19.130: Build OK!
v4.14.186: Build OK!
v4.9.228: Failed to apply! Possible dependencies:
0c9e498286ef9 ("irqchip/gic: Report that effective affinity is a single target")

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

Signature · 2 lines
Thanks
Sasha
#19 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

In reply to Marc Zyngier

Hi Marc, Russell,

Quoted 11 lines · Marc Zyngier

The GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.

But these sequences only expend into anything meaningful if the BL_SWITCHER option is selected, which almost never happens.

It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the GICD_TARGETRn registers, which when used make the update atomic by definition.

Quoted 5 lines

Drop the terminally broken code and replace it by a byte write.

Fixes 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")

Thanks for your patch, which is now commit 005c34ae4b44f085 ("irqchip/gic: Atomically update affinity"), to which I bisected a hard lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual board, which has a dual Cortex-A9 with PL390.

Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.

Unfortunately I only have remote access to the board showing the issue. I did check that adding the writeb_relaxed() before the writel_relaxed() that was used before also causes a lock-up, so the issue is not an endian mismatch.
Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?

Since you said the locking was bogus before, due to the reliance on the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?

Thanks for your comments!

Quoted 20 lines
drivers/irqchip/irq-gic.c +2 -5
@@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
 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) & ~3);
-       unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
-       u32 val, mask, bit;
-       unsigned long flags;
+       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);
@@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
        if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
                return -EINVAL;
 
-       gic_lock_irqsave(flags);

drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 346

Quoted 10 lines
drivers/irqchip/irq-gic.c +1 -6
-       mask = 0xff << shift;
-       bit = gic_cpu_map[cpu] << shift;
-       val = readl_relaxed(reg) & ~mask;
-       writel_relaxed(val | bit, reg);
-       gic_unlock_irqrestore(flags);
-
+       writeb_relaxed(gic_cpu_map[cpu], reg);
        irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
        return IRQ_SET_MASK_OK_DONE;

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#20 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Quoted 5 lines · Geert Uytterhoeven

Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.

Fun. Seems someone can't read ARMs documentation. Even the old ARM IHI 0048B.b document I have for the GIC from 2013 states "In addition, the GICD_IPRIORITYRn, GICD_ITARGETSRn, GICD_CPENDSGIRn, and GICD_SPENDSGIRn registers support byte accesses."

However, this kind of thing is sadly not uncommon. There's been a similar issue with the PL011 UART driver as well - some platforms require 16-bit accesses instead of normal 32-bit accesses.

Quoted 9 lines

Unfortunately I only have remote access to the board showing the issue. I did check that adding the writeb_relaxed() before the writel_relaxed() that was used before also causes a lock-up, so the issue is not an endian mismatch.
Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?

Quoted 3 lines

Since you said the locking was bogus before, due to the reliance on the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?

If byte accesses are not going to be workable, then the only answer _is_ a read-modify-write with working locking.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
author maintainer
#21 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

In reply to Geert Uytterhoeven

Hi Geert,

Quoted 26 lines · Geert Uytterhoeven

Hi Marc, Russell,

Quoted 11 lines · Marc Zyngier

The GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.

But these sequences only expend into anything meaningful if the BL_SWITCHER option is selected, which almost never happens.

It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the GICD_TARGETRn registers, which when used make the update atomic by definition.

Quoted 5 lines

Drop the terminally broken code and replace it by a byte write.

Fixes 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")

Thanks for your patch, which is now commit 005c34ae4b44f085 ("irqchip/gic: Atomically update affinity"), to which I bisected a hard lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual board, which has a dual Cortex-A9 with PL390.

Quoted 5 lines

Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.

Urgh. That is definitely a pretty poor integration. How about the priority registers? I guess they suffer from the same issue...

Quoted 8 lines

Unfortunately I only have remote access to the board showing the issue. I did check that adding the writeb_relaxed() before the writel_relaxed() that was used before also causes a lock-up, so the issue is not an endian mismatch.
Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?

Not that I know. A lock was probably fine on a two CPU system. Less so on a busy 8 CPU machine where interrupts are often migrated. The GIC architecture makes a point in not requiring locking for most of the registers that can be accessed concurrently.

Quoted 3 lines

Since you said the locking was bogus before, due to the reliance on the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?

There isn't much we can do aside from reintroducing the RMW+spinlock approach, and for real this time. It would have to be handled as a quirk though, as I'm not keen on reintroducing this for all systems.

I wrote the patchlet below, which is totally untested. Please give it a go and let me know if it helps.

	M.
drivers/irqchip/irq-gic.c +50 -1
index d329ec3d64d8..dca40a974b7a 100644
@@ -107,6 +107,8 @@ static DEFINE_RAW_SPINLOCK(cpu_map_lock);
 
 #endif
 
+static DEFINE_STATIC_KEY_FALSE(needs_rmw_access);
+
 /*
  * The GIC mapping of CPU interfaces does not necessarily match
  * the logical CPU numbering.  Let's use a mapping as returned
@@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)
 #endif
 
 #ifdef CONFIG_SMP
+static void rmw_writeb(u8 bval, void __iomem *addr)
+{
+	static DEFINE_RAW_SPINLOCK(rmw_lock);
+	unsigned long offset = (unsigned long)addr & ~3UL;
+	unsigned long shift = offset * 8;
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&rmw_lock, flags);
+
+	addr -= offset;
+	val = readl_relaxed(addr);
+	val &= ~(0xffUL << shift);
+	val |= (u32)bval << shift;
+	writel_relaxed(val, addr);
+
+	raw_spin_unlock_irqrestore(&rmw_lock, flags);
+}
+
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 			    bool force)
 {
@@ -788,7 +809,10 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	writeb_relaxed(gic_cpu_map[cpu], reg);
+	if (static_branch_unlikely(&needs_rmw_access))
+		rmw_writeb(gic_cpu_map[cpu], reg);
+	else
+		writeb_relaxed(gic_cpu_map[cpu], reg);
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
 	return IRQ_SET_MASK_OK_DONE;
@@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
+static bool gic_enable_rmw_access(void *data)
+{
+	/*
+	 * The EMEV2 class of machines has a broken interconnect, and
+	 * locks up on accesses that are less than 32bit. So far, only
+	 * the affinity setting requires it.
+	 */
+	if (of_machine_is_compatible("renesas,emev2")) {
+		static_branch_enable(&needs_rmw_access);
+		return true;
+	}
+
+	return false;
+}
+
+static const struct gic_quirk gic_quirks[] = {
+	{
+		.desc		= "Implementation with broken byte access",
+		.compatible	= "arm,pl390",
+		.init		= gic_enable_rmw_access,
+	},
+};
+
 static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
 {
 	if (!gic || !node)
@@ -1391,6 +1438,8 @@ static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
 	if (of_property_read_u32(node, "cpu-offset", &gic->percpu_offset))
 		gic->percpu_offset = 0;
 
+	gic_enable_of_quirks(node, gic_quirks, gic);
+
 	return 0;
 
 error:
Signature · 1 line
Without deviation from the norm, progress is not possible.
#22 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Hi Marc,

Quoted 25 lines · Marc Zyngier
Quoted 23 lines · Geert Uytterhoeven
Quoted 11 lines · Marc Zyngier

The GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.

But these sequences only expend into anything meaningful if the BL_SWITCHER option is selected, which almost never happens.

It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the GICD_TARGETRn registers, which when used make the update atomic by definition.

Quoted 5 lines

Drop the terminally broken code and replace it by a byte write.

Fixes 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")

Thanks for your patch, which is now commit 005c34ae4b44f085 ("irqchip/gic: Atomically update affinity"), to which I bisected a hard lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual board, which has a dual Cortex-A9 with PL390.

Quoted 8 lines
Quoted 5 lines

Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.

Urgh. That is definitely a pretty poor integration. How about the priority registers? I guess they suffer from the same issue...

Yes, they do.

Quoted 18 lines
Quoted 8 lines

Unfortunately I only have remote access to the board showing the issue. I did check that adding the writeb_relaxed() before the writel_relaxed() that was used before also causes a lock-up, so the issue is not an endian mismatch.
Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?

Not that I know. A lock was probably fine on a two CPU system. Less so on a busy 8 CPU machine where interrupts are often migrated. The GIC architecture makes a point in not requiring locking for most of the registers that can be accessed concurrently.

Quoted 3 lines

Since you said the locking was bogus before, due to the reliance on the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?

Quoted 6 lines

There isn't much we can do aside from reintroducing the RMW+spinlock approach, and for real this time. It would have to be handled as a quirk though, as I'm not keen on reintroducing this for all systems.

I wrote the patchlet below, which is totally untested. Please give it a go and let me know if it helps.

Thanks for your quick response!
Your solution works, after making a few small modifications.

drivers/irqchip/irq-gic.c static int gic_pm_init(struct gic_chip_data *gic) · line 782

Quoted 10 lines
drivers/irqchip/irq-gic.c +4 -0
@@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)
 #endif
 
 #ifdef CONFIG_SMP
+static void rmw_writeb(u8 bval, void __iomem *addr)
+{
+       static DEFINE_RAW_SPINLOCK(rmw_lock);
+       unsigned long offset = (unsigned long)addr & ~3UL;

Please drop the tilde.

drivers/irqchip/irq-gic.c static int gic_pm_init(struct gic_chip_data *gic) · line 783

Quoted 1 line
drivers/irqchip/irq-gic.c +1 -0
+       unsigned long shift = offset * 8;

"unsigned int" is sufficient for offset and size.

drivers/irqchip/irq-gic.c static int gic_pm_init(struct gic_chip_data *gic) · line 791

Quoted 8 lines
drivers/irqchip/irq-gic.c +8 -0
+       unsigned long flags;
+       u32 val;
+
+       raw_spin_lock_irqsave(&rmw_lock, flags);
+
+       addr -= offset;
+       val = readl_relaxed(addr);
+       val &= ~(0xffUL << shift);

No need for the UL suffix.

drivers/irqchip/irq-gic.c static int gic_pm_init(struct gic_chip_data *gic) · line 792

Quoted 1 line
drivers/irqchip/irq-gic.c +1 -0
+       val |= (u32)bval << shift;

No need for the cast.

Quoted 21 lines
drivers/irqchip/irq-gic.c +13 -0
+       writel_relaxed(val, addr);
+
+       raw_spin_unlock_irqrestore(&rmw_lock, flags);
+}
+
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
                            bool force)
 {
@@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
        return true;
 }
 
+static bool gic_enable_rmw_access(void *data)
+{
+       /*
+        * The EMEV2 class of machines has a broken interconnect, and
+        * locks up on accesses that are less than 32bit. So far, only
+        * the affinity setting requires it.
+        */
+       if (of_machine_is_compatible("renesas,emev2")) {

drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1419

Quoted 10 lines
drivers/irqchip/irq-gic.c +10 -0
+               static_branch_enable(&needs_rmw_access);
+               return true;
+       }
+
+       return false;
+}
+
+static const struct gic_quirk gic_quirks[] = {
+       {
+               .desc           = "Implementation with broken byte access",

The output would look better without capitalizing the first word. I think you can drop the first two words, saving some space:

GIC: enabling workaround for broken byte access

drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1422

Quoted 3 lines
drivers/irqchip/irq-gic.c +3 -0
+               .compatible     = "arm,pl390",
+               .init           = gic_enable_rmw_access,
+       },

Missing "{ /* sentinel */ }".

drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1427

Quoted 5 lines
drivers/irqchip/irq-gic.c +2 -0
+};
+
 static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
 {
        if (!gic || !node)

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#23 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Hi Geert, Mark, RMK, everyone,

Thanks for your efforts. Let me just chime in with a few details and a question.

Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier

On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:

GIC: enabling workaround for broken byte access

Indeed, byte access is unsupported according to the EMEV2 documentation.

The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.

From my basic research, "ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" is documented in ARM IHI 0048A from 2008 (Non-Confidential) which contains:
"All GIC registers are 32-bit wide." and "All registers support 32-bit word access..."
"In addition, the following registers support byte accesses:" "ICDIPR"
"ICDIPTR"

So the GICv1 documentation says byte access is partially supported however EMEV2 documentation says 32-bit access is required.

Quoted 3 lines

drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1422

Quoted 3 lines
drivers/irqchip/irq-gic.c +3 -0
+               .compatible     = "arm,pl390",
+               .init           = gic_enable_rmw_access,
+       },

May I ask about a clarification about the EMEV2 DTS and DT binding documentation in:
arch/arm/boot/dts/emev2.dts Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml

On EMEV2 the DT compatible string currently seems to be the rather generic "arm,pl390". In the DT binding documentation GICv1 is listed in an example as "arm,cortex-a9-gic". Is there any reason for not using the GICv1 compatible string (and 32-bit access) for EMEV2? Just curious.

Cheers,

/ magnus

author maintainer
#24 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Hi Magnus,

Quoted 14 lines · Magnus Damm

Hi Geert, Mark, RMK, everyone,

Thanks for your efforts. Let me just chime in with a few details and a question.

Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier

On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:

GIC: enabling workaround for broken byte access

Indeed, byte access is unsupported according to the EMEV2 documentation.

Quoted 7 lines

The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.

This is as bad as it gets. Do you know if any other Renesas platform is affected by the same issue?

Quoted 8 lines

From my basic research, "ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" is documented in ARM IHI 0048A from 2008 (Non-Confidential) which contains:
"All GIC registers are 32-bit wide." and "All registers support 32-bit word access..."
"In addition, the following registers support byte accesses:" "ICDIPR"

Renamed to GICD_IPRIORITYRn in IHI0048B.

Quoted 1 line

"ICDIPTR"

Renamed to GICD_ITARGETRn in IHI0048B.

See IHI0048B_b ("B.1 Alternative register names" and specifically table B-1) for the translation table between GICv1 and GICv2 names.

Quoted 2 lines

So the GICv1 documentation says byte access is partially supported however EMEV2 documentation says 32-bit access is required.

Which is definitely an integration bug. Both set of registers *must* support byte accesses. This isn't optional and left to the appreciation of the integrator. This breaks the programming model badly, and prevents standard software from running unmodified.

One of the few things the GIC architecture got right is the absence of locking requirements, as all the registers can be accessed concurrently by multiple CPUs as long as they operate on distinct interrupts. This is why the enable and pending registers have both set and clear accessors, that the priority and target registers are byte accessible, and that everything else happens in CPU-private registers (the CPU interface).

This requirement has been there from day-1. Even the good old DIC (the GIC's ancestor) that was included with the 11MP-Core says: "All Interrupt Distributor Registers are byte accessible.", which is more than actually necessary for the GIC. See DDI 0360F for details. And yes, SW written for the GIC does work on the DIC.

Quoted 10 lines
Quoted 4 lines

drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1422

Quoted 4 lines
drivers/irqchip/irq-gic.c +3 -0
+               .compatible     = "arm,pl390",
+               .init           = gic_enable_rmw_access,
+       },

May I ask about a clarification about the EMEV2 DTS and DT binding documentation in:
arch/arm/boot/dts/emev2.dts Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml

Quoted 5 lines

On EMEV2 the DT compatible string currently seems to be the rather generic "arm,pl390". In the DT binding documentation GICv1 is listed in an example as "arm,cortex-a9-gic". Is there any reason for not using the GICv1 compatible string (and 32-bit access) for EMEV2? Just curious.

GICv1 is an architecture specification. PL390 is an implementation of GICv1. The so called "Cortex-A9 GIC" doesn't really exist. It is simply the amalgamation of the CPU interface implemented by the A9 (with the prototype of the GICv2 virtualisation extensions) with a distributor (usually a PL390, but not necessarily). All of them require that the priority and target registers are byte accessible.

As for changing the compatibility string, I don't see the point. This will break existing setups, and doesn't change the core of the issue. As far as I can see, the EMEV2 DT is correct in the sense that it describes the actual implementation of the GIC used.

Thanks,

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
#25 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Hi Marc,

Quoted 19 lines · Marc Zyngier
Quoted 9 lines · Magnus Damm
Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier

On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:

GIC: enabling workaround for broken byte access

Indeed, byte access is unsupported according to the EMEV2 documentation.

Quoted 7 lines

The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.

Quoted 2 lines

This is as bad as it gets. Do you know if any other Renesas platform is affected by the same issue?

Next time we have a beer together I would be happy to show you some legacy interrupt controller code. =)

EMEV2 and the Emma Mobile product line came from the NEC Electronics side that got merged into Renesas Electronics in 2010. Historically NEC Electronics mainly used MIPS I've been told, and the Emma Mobile SoCs were one of the earlier Cortex-A9 adopters. That might have something to do with the rather loose interpretation of the spec.

Renesas SoCs from a similar era:
AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)
R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS) R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC)
R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC)
Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC)
SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)

The INTCA/INTCS interrupt controllers came from the SH architecture but were phased out once SMP became the norm. I've got the majority of the boards above hooked up for remote access if anyone wants to test something.

Quoted 8 lines
Quoted 7 lines

From my basic research, "ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" is documented in ARM IHI 0048A from 2008 (Non-Confidential) which contains:
"All GIC registers are 32-bit wide." and "All registers support 32-bit word access..."
"In addition, the following registers support byte accesses:" "ICDIPR"

Quoted 8 lines

Renamed to GICD_IPRIORITYRn in IHI0048B.

Quoted 1 line

"ICDIPTR"

Renamed to GICD_ITARGETRn in IHI0048B.

See IHI0048B_b ("B.1 Alternative register names" and specifically table B-1) for the translation table between GICv1 and GICv2 names.

Thanks.

Quoted 7 lines
Quoted 2 lines

So the GICv1 documentation says byte access is partially supported however EMEV2 documentation says 32-bit access is required.

Which is definitely an integration bug. Both set of registers *must* support byte accesses. This isn't optional and left to the appreciation of the integrator. This breaks the programming model badly, and prevents standard software from running unmodified.

This reminds me that on SH we used to fix up I/O access alignment for certain on-chip devices by trapping. The fast path worked well and the special case worked but was slow.

Quoted 7 lines

One of the few things the GIC architecture got right is the absence of locking requirements, as all the registers can be accessed concurrently by multiple CPUs as long as they operate on distinct interrupts. This is why the enable and pending registers have both set and clear accessors, that the priority and target registers are byte accessible, and that everything else happens in CPU-private registers (the CPU interface).

Yeah the GIC is quite nice IMO. The legacy INTC hardware often had separate registers for setting and clearing, however priority probably required read-modify-write. SMP wasn't an issue. =)

Quoted 5 lines

This requirement has been there from day-1. Even the good old DIC (the GIC's ancestor) that was included with the 11MP-Core says: "All Interrupt Distributor Registers are byte accessible.", which is more than actually necessary for the GIC. See DDI 0360F for details. And yes, SW written for the GIC does work on the DIC.

Interesting. For some not so big reason this makes me think of Monty Python. =)

Quoted 16 lines
Quoted 10 lines
Quoted 4 lines

drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1422

Quoted 4 lines
drivers/irqchip/irq-gic.c +3 -0
+               .compatible     = "arm,pl390",
+               .init           = gic_enable_rmw_access,
+       },

May I ask about a clarification about the EMEV2 DTS and DT binding documentation in:
arch/arm/boot/dts/emev2.dts Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml

Quoted 5 lines

On EMEV2 the DT compatible string currently seems to be the rather generic "arm,pl390". In the DT binding documentation GICv1 is listed in an example as "arm,cortex-a9-gic". Is there any reason for not using the GICv1 compatible string (and 32-bit access) for EMEV2? Just curious.

Quoted 6 lines

GICv1 is an architecture specification. PL390 is an implementation of GICv1. The so called "Cortex-A9 GIC" doesn't really exist. It is simply the amalgamation of the CPU interface implemented by the A9 (with the prototype of the GICv2 virtualisation extensions) with a distributor (usually a PL390, but not necessarily). All of them require that the priority and target registers are byte accessible.

Makes sense.

Quoted 4 lines

As for changing the compatibility string, I don't see the point. This will break existing setups, and doesn't change the core of the issue. As far as I can see, the EMEV2 DT is correct in the sense that it describes the actual implementation of the GIC used.

I'm all for not breaking existing setups, but EMEV2 is a pretty rare case. So my line of thinking was that instead of punishing all GIC platforms with this EMEV2-specific workaround it is completely fine from my side to to special case it in DT if it makes the rest of the code any cleaner. But it is really up to you guys.

Thanks,

/ magnus

#26 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Hi Magnus,

Quoted 23 lines · Magnus Damm
Quoted 19 lines · Marc Zyngier
Quoted 9 lines · Magnus Damm
Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier

On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:

GIC: enabling workaround for broken byte access

Indeed, byte access is unsupported according to the EMEV2 documentation.

Quoted 7 lines

The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.

Quoted 2 lines

This is as bad as it gets. Do you know if any other Renesas platform is affected by the same issue?

Quoted 8 lines

Next time we have a beer together I would be happy to show you some legacy interrupt controller code. =)

EMEV2 and the Emma Mobile product line came from the NEC Electronics side that got merged into Renesas Electronics in 2010. Historically NEC Electronics mainly used MIPS I've been told, and the Emma Mobile SoCs were one of the earlier Cortex-A9 adopters. That might have something to do with the rather loose interpretation of the spec.

Indeed. I used to work on products using EMMA1 and EMMA2, and they were MIPS-based (vr4120A for EMMA2, IIRC). Later variants (EMMA2H and EMMA3?) did include a small ARM core for standby control.

Quoted 2 lines

Renesas SoCs from a similar era:
AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)

This is no longer supported upstream (and not affected, as no GIC).

Quoted 1 line

R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS)

R-Mobile A1 has GIC (PL390), too, and is not affected.

Quoted 4 lines

R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC) R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC) Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC) SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)

All of these (except for EMEV2) are fine, too.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#27 lore raw

Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

Hi Geert, everyone,

Quoted 36 lines · Geert Uytterhoeven

Hi Magnus,

Quoted 23 lines · Magnus Damm
Quoted 19 lines · Marc Zyngier
Quoted 9 lines · Magnus Damm
Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier

On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:

GIC: enabling workaround for broken byte access

Indeed, byte access is unsupported according to the EMEV2 documentation.

Quoted 7 lines

The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.

Quoted 2 lines

This is as bad as it gets. Do you know if any other Renesas platform is affected by the same issue?

Quoted 8 lines

Next time we have a beer together I would be happy to show you some legacy interrupt controller code. =)

EMEV2 and the Emma Mobile product line came from the NEC Electronics side that got merged into Renesas Electronics in 2010. Historically NEC Electronics mainly used MIPS I've been told, and the Emma Mobile SoCs were one of the earlier Cortex-A9 adopters. That might have something to do with the rather loose interpretation of the spec.

Quoted 3 lines

Indeed. I used to work on products using EMMA1 and EMMA2, and they were MIPS-based (vr4120A for EMMA2, IIRC). Later variants (EMMA2H and EMMA3?) did include a small ARM core for standby control.

Thanks for sharing some more background!

Quoted 4 lines
Quoted 2 lines

Renesas SoCs from a similar era:
AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)

This is no longer supported upstream (and not affected, as no GIC).

Right. I might mix it up with the AP4.5 chip that I used for SMP prototyping back then. It had 4 x CA9 and obviously a GIC.

Quoted 10 lines
Quoted 1 line

R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS)

R-Mobile A1 has GIC (PL390), too, and is not affected.

Quoted 4 lines

R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC) R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC) Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC) SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)

All of these (except for EMEV2) are fine, too.

Thanks for checking!

Cheers,

/ magnus

#28 lore raw

[irqchip: irq/irqchip-fixes] irqchip/gic: Work around broken Renesas integration

In reply to Geert Uytterhoeven
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID:     b78f26926b17cc289e4f16b63363abe0aa2e8efc
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/b78f26926b17cc289e4f16b63363abe0aa2e8efc
Author:        Marc Zyngier <[email protected]>
AuthorDate:    Fri, 10 Sep 2021 18:29:25 +01:00
Committer:     Marc Zyngier <[email protected]>
CommitterDate: Wed, 22 Sep 2021 14:44:25 +01:00

irqchip/gic: Work around broken Renesas integration

Geert reported that the GIC driver locks up on a Renesas system since 005c34ae4b44f085 ("irqchip/gic: Atomically update affinity") fixed the driver to use writeb_relaxed() instead of writel_relaxed().

As it turns out, the interconnect used on this system mandates 32bit wide accesses for all MMIO transactions, even if the GIC architecture specifically mandates for some registers to be byte accessible. Gahhh...

Work around the issue by crudly detecting the offending system, and falling back to an inefficient RMW+lock implementation.

---
 drivers/irqchip/irq-gic.c | 52 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)
drivers/irqchip/irq-gic.c +51 -1
index d329ec3..5f22c9d 100644
@@ -107,6 +107,8 @@ static DEFINE_RAW_SPINLOCK(cpu_map_lock);
 
 #endif
 
+static DEFINE_STATIC_KEY_FALSE(needs_rmw_access);
+
 /*
  * The GIC mapping of CPU interfaces does not necessarily match
  * the logical CPU numbering.  Let's use a mapping as returned
@@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)
 #endif
 
 #ifdef CONFIG_SMP
+static void rmw_writeb(u8 bval, void __iomem *addr)
+{
+	static DEFINE_RAW_SPINLOCK(rmw_lock);
+	unsigned long offset = (unsigned long)addr & 3UL;
+	unsigned long shift = offset * 8;
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&rmw_lock, flags);
+
+	addr -= offset;
+	val = readl_relaxed(addr);
+	val &= ~GENMASK(shift + 7, shift);
+	val |= bval << shift;
+	writel_relaxed(val, addr);
+
+	raw_spin_unlock_irqrestore(&rmw_lock, flags);
+}
+
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 			    bool force)
 {
@@ -788,7 +809,10 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-	writeb_relaxed(gic_cpu_map[cpu], reg);
+	if (static_branch_unlikely(&needs_rmw_access))
+		rmw_writeb(gic_cpu_map[cpu], reg);
+	else
+		writeb_relaxed(gic_cpu_map[cpu], reg);
 	irq_data_update_effective_affinity(d, cpumask_of(cpu));
 
 	return IRQ_SET_MASK_OK_DONE;
@@ -1375,6 +1399,30 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
+static bool gic_enable_rmw_access(void *data)
+{
+	/*
+	 * The EMEV2 class of machines has a broken interconnect, and
+	 * locks up on accesses that are less than 32bit. So far, only
+	 * the affinity setting requires it.
+	 */
+	if (of_machine_is_compatible("renesas,emev2")) {
+		static_branch_enable(&needs_rmw_access);
+		return true;
+	}
+
+	return false;
+}
+
+static const struct gic_quirk gic_quirks[] = {
+	{
+		.desc		= "broken byte access",
+		.compatible	= "arm,pl390",
+		.init		= gic_enable_rmw_access,
+	},
+	{ },
+};
+
 static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
 {
 	if (!gic || !node)
@@ -1391,6 +1439,8 @@ static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
 	if (of_property_read_u32(node, "cpu-offset", &gic->percpu_offset))
 		gic->percpu_offset = 0;
 
+	gic_enable_of_quirks(node, gic_quirks, gic);
+
 	return 0;
 
 error:
author maintainer
#29 lore raw

PATCH v2 8/17 irqchip/gic: Refactor SMP configuration

In reply to 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.

---
 drivers/irqchip/irq-gic.c | 76 ++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 36 deletions(-)
drivers/irqchip/irq-gic.c +40 -36
index c17fabd6741e..801a5a5004d6 100644
@@ -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;
 }
Signature · 1 line
2.27.0
author maintainer
#30 lore raw

PATCH v2 9/17 irqchip/gic: Configure SGIs as standard interrupts

In reply to 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.

---
 drivers/irqchip/irq-gic.c | 101 +++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 39 deletions(-)
drivers/irqchip/irq-gic.c +62 -39
index 801a5a5004d6..86699761e19f 100644
@@ -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,
Signature · 1 line
2.27.0
author maintainer
#31 lore raw

PATCH v2 10/17 irqchip/gic-common: Don't enable SGIs by default

In reply to Marc Zyngier

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

---
 drivers/irqchip/irq-gic-common.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/irqchip/irq-gic-common.c +0 -3
index 82520006195d..f47b41dfd023 100644
@@ -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();
 }
Signature · 1 line
2.27.0
author maintainer
#32 lore raw

PATCH v2 11/17 irqchip/bcm2836: Configure mailbox interrupts as standard interrupts

In reply to 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.

---
 drivers/irqchip/irq-bcm2836.c | 151 ++++++++++++++++++++++++++++------
 1 file changed, 125 insertions(+), 26 deletions(-)
drivers/irqchip/irq-bcm2836.c +125 -26
index 2038693f074c..85df6ddad9be 100644
@@ -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);
Signature · 1 line
2.27.0
author maintainer
#33 lore raw

PATCH v2 12/17 irqchip/hip04: Configure IPIs as standard interrupts

In reply to 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.

---
 drivers/irqchip/irq-hip04.c | 89 +++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 49 deletions(-)
drivers/irqchip/irq-hip04.c +40 -49
index 130caa1c9d93..9b73dcfaf48d 100644
@@ -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);
 
Signature · 1 line
2.27.0
author maintainer
#34 lore raw

PATCH v2 13/17 irqchip/armada-370-xp: Configure IPIs as standard interrupts

In reply to 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.

---
 drivers/irqchip/irq-armada-370-xp.c | 262 +++++++++++++++++++---------
 1 file changed, 178 insertions(+), 84 deletions(-)
drivers/irqchip/irq-armada-370-xp.c +178 -84
index c9bdc5221b82..d7eb2e93db8f 100644
@@ -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);
Signature · 1 line
2.27.0
author maintainer
#35 lore raw

PATCH v2 14/17 arm64: Kill __smp_cross_call and co

In reply to Marc Zyngier

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

---
 arch/arm64/include/asm/irq_work.h |  4 +---
 arch/arm64/include/asm/smp.h      |  7 -------
 arch/arm64/kernel/smp.c           | 34 +++++--------------------------
 3 files changed, 6 insertions(+), 39 deletions(-)
arch/arm64/include/asm/irq_work.h +1 -3
index 8a1ef1907760..a1020285ea75 100644
@@ -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 */
arch/arm64/include/asm/smp.h +0 -7
index 4537a4620e3d..dd3f68ed9ac9 100644
@@ -67,13 +67,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
  */
arch/arm64/kernel/smp.c +5 -29
index c08950cc09cc..ed8a8184e3b6 100644
@@ -783,13 +783,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"),
@@ -801,11 +794,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)
 {
@@ -852,8 +841,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
 
@@ -960,27 +948,16 @@ 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)
@@ -1022,7 +999,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());
Signature · 1 line
2.27.0
#36 lore raw

Re: [PATCH v2 14/17] arm64: Kill __smp_cross_call and co

arch/arm64/kernel/smp.c void arch_send_wakeup_ipi_mask(const struct cpumask *mask) · line 844

Quoted 7 lines · Marc Zyngier
arch/arm64/kernel/smp.c +1 -2
@@ -852,8 +841,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);

AIU the following commit:

eb631bb5bf5b ("arm64: Support arch_irq_work_raise() via self IPIs")

It seems arm64 hasn't needed that check since

4b3dc9679cf7 ("arm64: force CONFIG_SMP=y and remove redundant #ifdefs")

Did I get that right?

Quoted 3 lines
(quoted diff) +0 -0
 }
 #endif
 
author maintainer
#37 lore raw

Re: [PATCH v2 14/17] arm64: Kill __smp_cross_call and co

Quoted 12 lines · Valentin Schneider

arch/arm64/kernel/smp.c void arch_send_wakeup_ipi_mask(const struct · line 845

Quoted 8 lines · Marc Zyngier
arch/arm64/kernel/smp.c +1 -2
@@ -852,8 +841,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);

AIU the following commit:

Quoted 8 lines

eb631bb5bf5b ("arm64: Support arch_irq_work_raise() via self IPIs")

It seems arm64 hasn't needed that check since

4b3dc9679cf7 ("arm64: force CONFIG_SMP=y and remove redundant
#ifdefs")

Did I get that right?

Indeed. SMP implies being able to IPI, and thus the check has been irrelevant for some time now.

Thanks,

        M.
Signature · 1 line
Jazz is not dead. It just smells funny...
author maintainer
#38 lore raw

PATCH v2 15/17 arm64: Remove custom IRQ stat accounting

In reply to Marc Zyngier

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

---
 arch/arm64/include/asm/hardirq.h |  9 ---------
 arch/arm64/kernel/smp.c          | 24 ++++++------------------
 2 files changed, 6 insertions(+), 27 deletions(-)
arch/arm64/include/asm/hardirq.h +0 -9
index 985493af704b..5ffa4bacdad3 100644
@@ -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 {
arch/arm64/kernel/smp.c +6 -18
index ed8a8184e3b6..8b903ceef2be 100644
@@ -73,7 +73,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;
@@ -801,26 +802,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);
@@ -893,10 +883,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:
@@ -991,7 +979,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);
Signature · 1 line
2.27.0
#39 lore raw

Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting

arch/arm64/kernel/smp.c void show_ipi_list(struct seq_file *p, int prec) · line 810

Quoted 12 lines · Marc Zyngier
arch/arm64/kernel/smp.c +2 -2
@@ -801,26 +802,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]);

How attached are we to that custom IPI printout? AIUI we *could* give them a "prettier" name in request_percpu_irq() and let the standard procfs printout take the wheel.

author maintainer
#40 lore raw

Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting

Quoted 4 lines · Valentin Schneider
Quoted 3 lines · Marc Zyngier
arch/arm64/kernel/smp.c +0 -0
@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec)
      unsigned int cpu, i;
 
Quoted 13 lines

arch/arm64/kernel/smp.c void show_ipi_list(struct seq_file *p, int prec) · line 810

Quoted 9 lines
arch/arm64/kernel/smp.c +2 -2
      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]);

How attached are we to that custom IPI printout? AIUI we *could* give them a "prettier" name in request_percpu_irq() and let the standard procfs printout take the wheel.

I wish. Unfortunately, /proc/interrupts is likely to be considered ABI, and I'm really worried to change this (see what happened last time we tried to update /proc/cpuinfo to be less braindead...).

        M.
Signature · 1 line
Jazz is not dead. It just smells funny...
#41 lore raw

Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting

Quoted 20 lines · Marc Zyngier
Quoted 4 lines · Valentin Schneider
Quoted 3 lines · Marc Zyngier
arch/arm64/kernel/smp.c +0 -0
@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec)
      unsigned int cpu, i;
 
Quoted 14 lines

arch/arm64/kernel/smp.c void show_ipi_list(struct seq_file *p, int prec) · line 810

Quoted 9 lines
arch/arm64/kernel/smp.c +2 -2
      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]);

How attached are we to that custom IPI printout? AIUI we *could* give them
a "prettier" name in request_percpu_irq() and let the standard procfs printout take the wheel.

Quoted 4 lines

I wish. Unfortunately, /proc/interrupts is likely to be considered ABI, and I'm really worried to change this (see what happened last time we tried to update /proc/cpuinfo to be less braindead...).

Hmph, it's borderline here I think: we're not introducing a new field or format in the file, so any tool that can parse /proc/interrupts can parse the IPIs if they are formatted like the other "regular" interrupts. But then said tool could die in flames when not seeing the special IPI fields because sturdiness is overrated :(

Oh well.

Quoted 1 line
         M.
author maintainer
#42 lore raw

Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting

Quoted 27 lines · Valentin Schneider
Quoted 21 lines · Marc Zyngier
Quoted 5 lines · Valentin Schneider
Quoted 4 lines · Marc Zyngier
arch/arm64/kernel/smp.c +0 -0
@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int 
 prec)
      unsigned int cpu, i;
 
Quoted 14 lines

arch/arm64/kernel/smp.c void show_ipi_list(struct seq_file *p, int prec) · line 810

Quoted 9 lines
arch/arm64/kernel/smp.c +2 -2
      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]);

How attached are we to that custom IPI printout? AIUI we *could* give them
a "prettier" name in request_percpu_irq() and let the standard procfs printout take the wheel.

Quoted 4 lines

I wish. Unfortunately, /proc/interrupts is likely to be considered ABI, and I'm really worried to change this (see what happened last time we tried to update /proc/cpuinfo to be less braindead...).

Quoted 5 lines

Hmph, it's borderline here I think: we're not introducing a new field or format in the file, so any tool that can parse /proc/interrupts can parse the IPIs if they are formatted like the other "regular" interrupts. But then said tool could die in flames when not seeing the special IPI fields because sturdiness is overrated :(

Which is exactly what I'm worried about. People do stupid things, and stupidity becomes ABI. I hate luserspace.

         M.
Signature · 1 line
Jazz is not dead. It just smells funny...
#43 lore raw

Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting

In reply to Marc Zyngier
Quoted 13 lines · Marc Zyngier

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

---
 arch/arm64/include/asm/hardirq.h |  9 ---------
 arch/arm64/kernel/smp.c          | 24 ++++++------------------
 2 files changed, 6 insertions(+), 27 deletions(-)
arch/arm64/kernel/smp.c +0 -0
index ed8a8184e3b6..8b903ceef2be 100644

arch/arm64/kernel/smp.c void __init set_smp_ipi_range(int ipi_base, int n) · line 982

Quoted 5 lines
arch/arm64/kernel/smp.c +1 -1
@@ -991,7 +979,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);

That pointer remains unused in ipi_handler(); I suppose it's just a matter of cosmetics? I see that we leave it as irq_stat in arm, I suppose simply because we don't have a cpu_number equivalent there and need *some* percpu variable for the request to happen.

Quoted 3 lines
arch/arm64/kernel/smp.c +0 -0
              WARN_ON(err);
 
              ipi_desc[i] = irq_to_desc(ipi_base + i);
author maintainer
#44 lore raw

PATCH v2 16/17 ARM: Kill __smp_cross_call and co

In reply to Marc Zyngier

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

---
 arch/arm/include/asm/smp.h |  6 ------
 arch/arm/kernel/smp.c      | 22 +++++-----------------
 2 files changed, 5 insertions(+), 23 deletions(-)
arch/arm/include/asm/smp.h +0 -6
index 0e29730295ca..0ca55a607d0a 100644
@@ -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
  */
arch/arm/kernel/smp.c +5 -17
index 269639b14259..285980c2a437 100644
@@ -514,14 +514,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"),
@@ -533,11 +525,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)
 {
@@ -716,9 +704,10 @@ 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)
@@ -760,7 +749,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());
@@ -873,7 +861,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)
Signature · 1 line
2.27.0
author maintainer
#45 lore raw

PATCH v2 17/17 ARM: Remove custom IRQ stat accounting

In reply to Marc Zyngier

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

---
 arch/arm/include/asm/hardirq.h | 17 -----------------
 arch/arm/kernel/smp.c          | 20 ++++----------------
 2 files changed, 4 insertions(+), 33 deletions(-)
arch/arm/include/asm/hardirq.h +0 -17
index 7a88f160b1fb..b95848ed2bc7 100644
@@ -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 */
arch/arm/kernel/smp.c +4 -16
index 285980c2a437..249fac8b0a8e 100644
@@ -66,6 +66,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_*
@@ -532,27 +533,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);
@@ -633,10 +623,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:
Signature · 1 line
2.27.0
Reviewed-by on the v2 series
#46 lore raw
In reply to Marc Zyngier

Hi Marc,

Quoted 29 lines · 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).

Quoted 5 lines

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 gave that a spin on Juno r0 and HiKey960 (both GICv2), all good! I also wanted to try it out on my eMAG (to get some GICv3 airtime) but ran into "technical difficulties". I think I'll need to get someone to go poke it (most likely next week). I'm pretty sure I'm the one who should be asking you for hardware, but if there's anything specific you need me to test, please shout.

I have a few extra nits/comments in some patches, but it's all fairly minor so FWIW you can also add, for patches [01-10, 14-15]:

I haven't really looked at those other irqchips, but I can give it a shot if no one else shows up. Also I'll most likely look at the arm side, but I'm afraid I'm too well-done right now to pay much more attention to details.

#47 lore raw
Quoted 9 lines · Valentin Schneider

I have a few extra nits/comments in some patches, but it's all fairly minor so FWIW you can also add, for patches [01-10, 14-15]:

I haven't really looked at those other irqchips, but I can give it a shot if no one else shows up. Also I'll most likely look at the arm side, but I'm afraid I'm too well-done right now to pay much more attention to details.

Had a look at the arm side (and a took a refresher on the arm64 side); LGTM, so you can extend that to: [01-10, 14-17].

#48 lore raw
In reply to Marc Zyngier

Hi Marc,

Quoted 30 lines · 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).

Quoted 4 lines

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.

This series works perfectly fine on Developerbox.

I just want to follow-up regarding when you are planning to push this series upstream? Are you waiting for other irqchips (apart from GIC) to be reviewed?

Actually mine work to turn IPI as a pseudo NMI [1] is dependent on this patch-set.

[1] https://lkml.org/lkml/2020/5/20/488

-Sumit

Quoted 53 lines

* 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 (17):
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: Atomically update affinity
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             | 119 ++++++++-----
 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        |  99 ++++++-----
 drivers/irqchip/irq-gic.c           | 183 ++++++++++---------
 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, 713 insertions(+), 413 deletions(-)

--
2.27.0

author maintainer
#49 lore raw

Hi Sumit,

Quoted 38 lines · Sumit Garg

Hi Marc,

Quoted 30 lines · 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).

Quoted 4 lines

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.

Quoted 5 lines

This series works perfectly fine on Developerbox.

I just want to follow-up regarding when you are planning to push this series upstream? Are you waiting for other irqchips (apart from GIC) to be reviewed?

I'd certainly like people to review (and maybe test if they have the HW at hand) the rest of the interrupt controller changes.

I'll probably repost the series around -rc1.

Quoted 4 lines

Actually mine work to turn IPI as a pseudo NMI [1] is dependent on this patch-set.

[1] https://lkml.org/lkml/2020/5/20/488

I'm aware of this.

Thanks,

        M.
Signature · 1 line
Jazz is not dead. It just smells funny...