[PATCH v3 0/6] IRQ affinity support in PLIC driver

[PATCH v3 0/6] IRQ affinity support in PLIC driver

[Anup Patel]

This patchset primarily adds IRQ affinity support in PLIC driver and
other improvements.

The patchset gives mechanism for explicitly routing external interrupts to
particular CPUs using smp_affinity attribute of each Linux IRQs. Also, we
can now use IRQ balancer from kernel-space or user-space.

The patchset is tested on QEMU virt machine. It is based on Linux-4.20-rc4
and can be found at riscv_plic_irq_affinity_v3 branch of:
https://github.com/avpatel/linux.git

Changes since v2:
 - Fixed incorrect address of enable registers using sizeof(u32) in PATCH1
 - Retained comment about need for locking in PATCH1
 - Split PATCH2 into two patches
 - Split PATCH3 into two patches
 - Minor fix in commit description of PATCH4

Changes since v1:
 - Removed few whitspace changes from PATCH1
 - Keep use of DEFINE_PER_CPU() as it is

Anup Patel (6):
  irqchip: sifive-plic: Pre-compute context hart base and enable base
  irqchip: sifive-plic: Add struct plic_hw for global PLIC HW details
  irqchip: sifive-plic: More flexible plic_irq_toggle()
  irqchip: sifive-plic: Add warning in plic_init() if handler already
    present
  irqchip: sifive-plic: Differentiate between PLIC handler and context
  irqchip: sifive-plic: Implement irq_set_affinity() for SMP host

 drivers/irqchip/irq-sifive-plic.c | 143 +++++++++++++++++++-----------
 1 file changed, 90 insertions(+), 53 deletions(-)

-- 
2.17.1

[PATCH v3 1/6] irqchip: sifive-plic: Pre-compute context hart base and enable base

[Anup Patel]

This patch does following optimizations:
1. Pre-compute hart base for each context handler
2. Pre-compute enable base for each context handler
3. Have enable lock for each context handler instead
of global plic_toggle_lock

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 drivers/irqchip/irq-sifive-plic.c | 47 ++++++++++++++-----------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 357e9daf94ae..c23a293a2aae 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -59,37 +59,28 @@ static void __iomem *plic_regs;
 
 struct plic_handler {
 	bool			present;
-	int			ctxid;
+	void __iomem		*hart_base;
+	/*
+	 * Protect mask operations on the registers given that we can't
+	 * assume atomic memory operations work on them.
+	 */
+	raw_spinlock_t		enable_lock;
+	void __iomem		*enable_base;
 };
 static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
 
-static inline void __iomem *plic_hart_offset(int ctxid)
-{
-	return plic_regs + CONTEXT_BASE + ctxid * CONTEXT_PER_HART;
-}
-
-static inline u32 __iomem *plic_enable_base(int ctxid)
-{
-	return plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART;
-}
-
-/*
- * Protect mask operations on the registers given that we can't assume that
- * atomic memory operations work on them.
- */
-static DEFINE_RAW_SPINLOCK(plic_toggle_lock);
-
-static inline void plic_toggle(int ctxid, int hwirq, int enable)
+static inline void plic_toggle(struct plic_handler *handler,
+				int hwirq, int enable)
 {
-	u32 __iomem *reg = plic_enable_base(ctxid) + (hwirq / 32);
+	u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
 	u32 hwirq_mask = 1 << (hwirq % 32);
 
-	raw_spin_lock(&plic_toggle_lock);
+	raw_spin_lock(&handler->enable_lock);
 	if (enable)
 		writel(readl(reg) | hwirq_mask, reg);
 	else
 		writel(readl(reg) & ~hwirq_mask, reg);
-	raw_spin_unlock(&plic_toggle_lock);
+	raw_spin_unlock(&handler->enable_lock);
 }
 
 static inline void plic_irq_toggle(struct irq_data *d, int enable)
@@ -101,7 +92,7 @@ static inline void plic_irq_toggle(struct irq_data *d, int enable)
 		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
 
 		if (handler->present)
-			plic_toggle(handler->ctxid, d->hwirq, enable);
+			plic_toggle(handler, d->hwirq, enable);
 	}
 }
 
@@ -150,7 +141,7 @@ static struct irq_domain *plic_irqdomain;
 static void plic_handle_irq(struct pt_regs *regs)
 {
 	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
-	void __iomem *claim = plic_hart_offset(handler->ctxid) + CONTEXT_CLAIM;
+	void __iomem *claim = handler->hart_base + CONTEXT_CLAIM;
 	irq_hw_number_t hwirq;
 
 	WARN_ON_ONCE(!handler->present);
@@ -239,12 +230,16 @@ static int __init plic_init(struct device_node *node,
 		cpu = riscv_hartid_to_cpuid(hartid);
 		handler = per_cpu_ptr(&plic_handlers, cpu);
 		handler->present = true;
-		handler->ctxid = i;
+		handler->hart_base =
+			plic_regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
+		raw_spin_lock_init(&handler->enable_lock);
+		handler->enable_base =
+			plic_regs + ENABLE_BASE + i * ENABLE_PER_HART;
 
 		/* priority must be > threshold to trigger an interrupt */
-		writel(0, plic_hart_offset(i) + CONTEXT_THRESHOLD);
+		writel(0, handler->hart_base + CONTEXT_THRESHOLD);
 		for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
-			plic_toggle(i, hwirq, 0);
+			plic_toggle(handler, hwirq, 0);
 		nr_mapped++;
 	}
 
-- 
2.17.1

[PATCH v3 2/6] irqchip: sifive-plic: Add struct plic_hw for global PLIC HW details

[Anup Patel]

This patch adds Add struct plic_hw to represent global PLIC HW details.

Currently, these details are only used in plic_init() but in-future
these will be useful in implementing PM suspend and resume callbacks.

Further, these global details are good debug info about HW so let's
not throw them away after use in plic_init().

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 drivers/irqchip/irq-sifive-plic.c | 59 +++++++++++++++++--------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index c23a293a2aae..48bee877e0f1 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -55,8 +55,6 @@
 #define     CONTEXT_THRESHOLD		0x00
 #define     CONTEXT_CLAIM		0x04
 
-static void __iomem *plic_regs;
-
 struct plic_handler {
 	bool			present;
 	void __iomem		*hart_base;
@@ -67,8 +65,19 @@ struct plic_handler {
 	raw_spinlock_t		enable_lock;
 	void __iomem		*enable_base;
 };
+
 static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
 
+struct plic_hw {
+	u32			nr_irqs;
+	u32			nr_handlers;
+	u32			nr_mapped;
+	void __iomem		*regs;
+	struct irq_domain	*irqdomain;
+};
+
+static struct plic_hw plic;
+
 static inline void plic_toggle(struct plic_handler *handler,
 				int hwirq, int enable)
 {
@@ -87,7 +96,7 @@ static inline void plic_irq_toggle(struct irq_data *d, int enable)
 {
 	int cpu;
 
-	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
+	writel(enable, plic.regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
 	for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
 		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
 
@@ -130,8 +139,6 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
 	.xlate		= irq_domain_xlate_onecell,
 };
 
-static struct irq_domain *plic_irqdomain;
-
 /*
  * Handling an interrupt is a two-step process: first you claim the interrupt
  * by reading the claim register, then you complete the interrupt by writing
@@ -148,7 +155,7 @@ static void plic_handle_irq(struct pt_regs *regs)
 
 	csr_clear(sie, SIE_SEIE);
 	while ((hwirq = readl(claim))) {
-		int irq = irq_find_mapping(plic_irqdomain, hwirq);
+		int irq = irq_find_mapping(plic.irqdomain, hwirq);
 
 		if (unlikely(irq <= 0))
 			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
@@ -177,36 +184,35 @@ static int plic_find_hart_id(struct device_node *node)
 static int __init plic_init(struct device_node *node,
 		struct device_node *parent)
 {
-	int error = 0, nr_handlers, nr_mapped = 0, i;
-	u32 nr_irqs;
+	int error = 0, i;
 
-	if (plic_regs) {
+	if (plic.regs) {
 		pr_warn("PLIC already present.\n");
 		return -ENXIO;
 	}
 
-	plic_regs = of_iomap(node, 0);
-	if (WARN_ON(!plic_regs))
+	plic.regs = of_iomap(node, 0);
+	if (WARN_ON(!plic.regs))
 		return -EIO;
 
 	error = -EINVAL;
-	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
-	if (WARN_ON(!nr_irqs))
+	of_property_read_u32(node, "riscv,ndev", &plic.nr_irqs);
+	if (WARN_ON(!plic.nr_irqs))
 		goto out_iounmap;
 
-	nr_handlers = of_irq_count(node);
-	if (WARN_ON(!nr_handlers))
+	plic.nr_handlers = of_irq_count(node);
+	if (WARN_ON(!plic.nr_handlers))
 		goto out_iounmap;
-	if (WARN_ON(nr_handlers < num_possible_cpus()))
+	if (WARN_ON(plic.nr_handlers < num_possible_cpus()))
 		goto out_iounmap;
 
 	error = -ENOMEM;
-	plic_irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
-			&plic_irqdomain_ops, NULL);
-	if (WARN_ON(!plic_irqdomain))
+	plic.irqdomain = irq_domain_add_linear(node, plic.nr_irqs + 1,
+						&plic_irqdomain_ops, NULL);
+	if (WARN_ON(!plic.irqdomain))
 		goto out_iounmap;
 
-	for (i = 0; i < nr_handlers; i++) {
+	for (i = 0; i < plic.nr_handlers; i++) {
 		struct of_phandle_args parent;
 		struct plic_handler *handler;
 		irq_hw_number_t hwirq;
@@ -231,25 +237,26 @@ static int __init plic_init(struct device_node *node,
 		handler = per_cpu_ptr(&plic_handlers, cpu);
 		handler->present = true;
 		handler->hart_base =
-			plic_regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
+			plic.regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
 		raw_spin_lock_init(&handler->enable_lock);
 		handler->enable_base =
-			plic_regs + ENABLE_BASE + i * ENABLE_PER_HART;
+			plic.regs + ENABLE_BASE + i * ENABLE_PER_HART;
 
 		/* priority must be > threshold to trigger an interrupt */
 		writel(0, handler->hart_base + CONTEXT_THRESHOLD);
-		for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
+		for (hwirq = 1; hwirq <= plic.nr_irqs; hwirq++)
 			plic_toggle(handler, hwirq, 0);
-		nr_mapped++;
+
+		plic.nr_mapped++;
 	}
 
 	pr_info("mapped %d interrupts to %d (out of %d) handlers.\n",
-		nr_irqs, nr_mapped, nr_handlers);
+		plic.nr_irqs, plic.nr_mapped, plic.nr_handlers);
 	set_handle_irq(plic_handle_irq);
 	return 0;
 
 out_iounmap:
-	iounmap(plic_regs);
+	iounmap(plic.regs);
 	return error;
 }
 
-- 
2.17.1

[PATCH v3 3/6] irqchip: sifive-plic: More flexible plic_irq_toggle()

[Anup Patel]

We make plic_irq_toggle() more generic so that we can enable/disable
hwirq for given cpumask. This generic plic_irq_toggle() will be
eventually used to implement set_affinity for PLIC driver.

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 drivers/irqchip/irq-sifive-plic.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 48bee877e0f1..d4433399eb89 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -78,8 +78,7 @@ struct plic_hw {
 
 static struct plic_hw plic;
 
-static inline void plic_toggle(struct plic_handler *handler,
-				int hwirq, int enable)
+static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
 {
 	u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
 	u32 hwirq_mask = 1 << (hwirq % 32);
@@ -92,27 +91,27 @@ static inline void plic_toggle(struct plic_handler *handler,
 	raw_spin_unlock(&handler->enable_lock);
 }
 
-static inline void plic_irq_toggle(struct irq_data *d, int enable)
+static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)
 {
 	int cpu;
 
-	writel(enable, plic.regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
-	for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
+	writel(enable, plic.regs + PRIORITY_BASE + hwirq * PRIORITY_PER_ID);
+	for_each_cpu(cpu, mask) {
 		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
 
 		if (handler->present)
-			plic_toggle(handler, d->hwirq, enable);
+			plic_toggle(handler, hwirq, enable);
 	}
 }
 
 static void plic_irq_enable(struct irq_data *d)
 {
-	plic_irq_toggle(d, 1);
+	plic_irq_toggle(irq_data_get_affinity_mask(d), d->hwirq, 1);
 }
 
 static void plic_irq_disable(struct irq_data *d)
 {
-	plic_irq_toggle(d, 0);
+	plic_irq_toggle(irq_data_get_affinity_mask(d), d->hwirq, 0);
 }
 
 static struct irq_chip plic_chip = {
-- 
2.17.1

[PATCH v3 4/6] irqchip: sifive-plic: Add warning in plic_init() if handler already present

[Anup Patel]

We have two enteries (one for M-mode and another for S-mode) in the
interrupts-extended DT property of PLIC DT node for each HART. It is
expected that firmware/bootloader will set M-mode HWIRQ line of each
HART to 0xffffffff (i.e. -1) in interrupts-extended DT property
because Linux runs in S-mode only.

If firmware/bootloader is buggy then it will not correctly update
interrupts-extended DT property which might result in a plic_handler
configured twice. This patch adds a warning in plic_init() if a
plic_handler is already marked present. This warning provides us
a hint about incorrectly updated interrupts-extended DT property.

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 drivers/irqchip/irq-sifive-plic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index d4433399eb89..3d4f205f8abe 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -234,6 +234,11 @@ static int __init plic_init(struct device_node *node,
 
 		cpu = riscv_hartid_to_cpuid(hartid);
 		handler = per_cpu_ptr(&plic_handlers, cpu);
+		if (handler->present) {
+			pr_warn("handler not available for context %d.\n", i);
+			continue;
+		}
+
 		handler->present = true;
 		handler->hart_base =
 			plic.regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
-- 
2.17.1

[PATCH v3 5/6] irqchip: sifive-plic: Differentiate between PLIC handler and context

[Anup Patel]

We explicitly differentiate between PLIC handler and context because
PLIC context is for given mode of HART whereas PLIC handler is per-CPU
software construct meant for handling interrupts from a particular
PLIC context.

To achieve this differentiation, we rename "nr_handlers" to "nr_contexts"
and "nr_mapped" to "nr_handlers" in struct plic_hw.

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 drivers/irqchip/irq-sifive-plic.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 3d4f205f8abe..17269622be21 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -70,8 +70,8 @@ static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
 
 struct plic_hw {
 	u32			nr_irqs;
+	u32			nr_contexts;
 	u32			nr_handlers;
-	u32			nr_mapped;
 	void __iomem		*regs;
 	struct irq_domain	*irqdomain;
 };
@@ -199,10 +199,10 @@ static int __init plic_init(struct device_node *node,
 	if (WARN_ON(!plic.nr_irqs))
 		goto out_iounmap;
 
-	plic.nr_handlers = of_irq_count(node);
-	if (WARN_ON(!plic.nr_handlers))
+	plic.nr_contexts = of_irq_count(node);
+	if (WARN_ON(!plic.nr_contexts))
 		goto out_iounmap;
-	if (WARN_ON(plic.nr_handlers < num_possible_cpus()))
+	if (WARN_ON(plic.nr_contexts < num_possible_cpus()))
 		goto out_iounmap;
 
 	error = -ENOMEM;
@@ -211,7 +211,7 @@ static int __init plic_init(struct device_node *node,
 	if (WARN_ON(!plic.irqdomain))
 		goto out_iounmap;
 
-	for (i = 0; i < plic.nr_handlers; i++) {
+	for (i = 0; i < plic.nr_contexts; i++) {
 		struct of_phandle_args parent;
 		struct plic_handler *handler;
 		irq_hw_number_t hwirq;
@@ -251,11 +251,11 @@ static int __init plic_init(struct device_node *node,
 		for (hwirq = 1; hwirq <= plic.nr_irqs; hwirq++)
 			plic_toggle(handler, hwirq, 0);
 
-		plic.nr_mapped++;
+		plic.nr_handlers++;
 	}
 
-	pr_info("mapped %d interrupts to %d (out of %d) handlers.\n",
-		plic.nr_irqs, plic.nr_mapped, plic.nr_handlers);
+	pr_info("mapped %d interrupts with %d handlers for %d contexts.\n",
+		plic.nr_irqs, plic.nr_handlers, plic.nr_contexts);
 	set_handle_irq(plic_handle_irq);
 	return 0;
 
-- 
2.17.1

[PATCH v3 6/6] irqchip: sifive-plic: Implement irq_set_affinity() for SMP host

[Anup Patel]

Currently on SMP host, all CPUs take external interrupts routed via
PLIC. All CPUs will try to claim a given external interrupt but only
one of them will succeed while other CPUs would simply resume whatever
they were doing before. This means if we have N CPUs then for every
external interrupt N-1 CPUs will always fail to claim it and waste
their CPU time.

Instead of above, external interrupts should be taken by only one CPU
and we should have provision to explicitly specify IRQ affinity from
kernel-space or user-space.

This patch provides irq_set_affinity() implementation for PLIC driver.
It also updates irq_enable() such that PLIC interrupts are only enabled
for one of CPUs specified in IRQ affinity mask.

With this patch in-place, we can change IRQ affinity at any-time from
user-space using procfs.

Example:

/ # cat /proc/interrupts
           CPU0       CPU1       CPU2       CPU3
  8:         44          0          0          0  SiFive PLIC   8  virtio0
 10:         48          0          0          0  SiFive PLIC  10  ttyS0
IPI0:        55        663         58        363  Rescheduling interrupts
IPI1:         0          1          3         16  Function call interrupts
/ #
/ #
/ # echo 4 > /proc/irq/10/smp_affinity
/ #
/ # cat /proc/interrupts
           CPU0       CPU1       CPU2       CPU3
  8:         45          0          0          0  SiFive PLIC   8  virtio0
 10:        160          0         17          0  SiFive PLIC  10  ttyS0
IPI0:        68        693         77        410  Rescheduling interrupts
IPI1:         0          2          3         16  Function call interrupts

Signed-off-by: Anup Patel <anup@brainfault.org>
---
 drivers/irqchip/irq-sifive-plic.c | 35 +++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 17269622be21..c5bbb12e74e0 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -106,14 +106,42 @@ static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)
 
 static void plic_irq_enable(struct irq_data *d)
 {
-	plic_irq_toggle(irq_data_get_affinity_mask(d), d->hwirq, 1);
+	unsigned int cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
+					   cpu_online_mask);
+	WARN_ON(cpu >= nr_cpu_ids);
+	plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
 }
 
 static void plic_irq_disable(struct irq_data *d)
 {
-	plic_irq_toggle(irq_data_get_affinity_mask(d), d->hwirq, 0);
+	plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
 }
 
+#ifdef CONFIG_SMP
+static int plic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
+			    bool force)
+{
+	unsigned int cpu;
+
+	if (!force)
+		cpu = cpumask_any_and(mask_val, cpu_online_mask);
+	else
+		cpu = cpumask_first(mask_val);
+
+	if (cpu >= nr_cpu_ids)
+		return -EINVAL;
+
+	if (!irqd_irq_disabled(d)) {
+		plic_irq_toggle(cpu_possible_mask, d->hwirq, 0);
+		plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1);
+	}
+
+	irq_data_update_effective_affinity(d, cpumask_of(cpu));
+
+	return IRQ_SET_MASK_OK_DONE;
+}
+#endif
+
 static struct irq_chip plic_chip = {
 	.name		= "SiFive PLIC",
 	/*
@@ -122,6 +150,9 @@ static struct irq_chip plic_chip = {
 	 */
 	.irq_enable	= plic_irq_enable,
 	.irq_disable	= plic_irq_disable,
+#ifdef CONFIG_SMP
+	.irq_set_affinity = plic_set_affinity,
+#endif
 };
 
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
-- 
2.17.1

Re: [PATCH v3 0/6] IRQ affinity support in PLIC driver

[Anup Patel]

On Fri, Nov 30, 2018 at 1:32 PM Anup Patel <anup@brainfault.org> wrote:
>
> This patchset primarily adds IRQ affinity support in PLIC driver and
> other improvements.
>
> The patchset gives mechanism for explicitly routing external interrupts to
> particular CPUs using smp_affinity attribute of each Linux IRQs. Also, we
> can now use IRQ balancer from kernel-space or user-space.
>
> The patchset is tested on QEMU virt machine. It is based on Linux-4.20-rc4
> and can be found at riscv_plic_irq_affinity_v3 branch of:
> https://github.com/avpatel/linux.git
>
> Changes since v2:
>  - Fixed incorrect address of enable registers using sizeof(u32) in PATCH1
>  - Retained comment about need for locking in PATCH1
>  - Split PATCH2 into two patches
>  - Split PATCH3 into two patches
>  - Minor fix in commit description of PATCH4
>
> Changes since v1:
>  - Removed few whitspace changes from PATCH1
>  - Keep use of DEFINE_PER_CPU() as it is
>
> Anup Patel (6):
>   irqchip: sifive-plic: Pre-compute context hart base and enable base
>   irqchip: sifive-plic: Add struct plic_hw for global PLIC HW details
>   irqchip: sifive-plic: More flexible plic_irq_toggle()
>   irqchip: sifive-plic: Add warning in plic_init() if handler already
>     present
>   irqchip: sifive-plic: Differentiate between PLIC handler and context
>   irqchip: sifive-plic: Implement irq_set_affinity() for SMP host
>
>  drivers/irqchip/irq-sifive-plic.c | 143 +++++++++++++++++++-----------
>  1 file changed, 90 insertions(+), 53 deletions(-)
>
> --
> 2.17.1
>

Hi All,

Any comments on this series?

Regards,
Anup

Re: [PATCH v3 2/6] irqchip: sifive-plic: Add struct plic_hw for global PLIC HW details

[Christoph Hellwig]

> +struct plic_hw {
> +	u32			nr_irqs;
> +	u32			nr_handlers;
> +	u32			nr_mapped;
> +	void __iomem		*regs;
> +	struct irq_domain	*irqdomain;
> +};
> +
> +static struct plic_hw plic;

Please use local variables instead of a single instance struct.
And only add these variables in the patches where you actually need
them.

Re: [PATCH v3 1/6] irqchip: sifive-plic: Pre-compute context hart base and enable base

[Christoph Hellwig]

On Fri, Nov 30, 2018 at 01:32:02PM +0530, Anup Patel wrote:
> This patch does following optimizations:
> 1. Pre-compute hart base for each context handler
> 2. Pre-compute enable base for each context handler
> 3. Have enable lock for each context handler instead
> of global plic_toggle_lock

All of which is pretty obvious from reading the patch.  The big question
that needs to be answered in the changelog is why you do that.

Re: [PATCH v3 3/6] irqchip: sifive-plic: More flexible plic_irq_toggle()

[Christoph Hellwig]

> -static inline void plic_toggle(struct plic_handler *handler,
> -				int hwirq, int enable)
> +static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
>  {
>  	u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
>  	u32 hwirq_mask = 1 << (hwirq % 32);
> @@ -92,27 +91,27 @@ static inline void plic_toggle(struct plic_handler *handler,
>  	raw_spin_unlock(&handler->enable_lock);
>  }
>  
> -static inline void plic_irq_toggle(struct irq_data *d, int enable)
> +static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)

It also removes inline statements which seems rather unrelated to
the patch description.

Also the actual addintion of the single cpumask argument is simple
enough that it should probably go into the patch that makes use of it.

Re: [PATCH v3 4/6] irqchip: sifive-plic: Add warning in plic_init() if handler already present

[Christoph Hellwig]

On Fri, Nov 30, 2018 at 01:32:05PM +0530, Anup Patel wrote:
> We have two enteries (one for M-mode and another for S-mode) in the
> interrupts-extended DT property of PLIC DT node for each HART. It is
> expected that firmware/bootloader will set M-mode HWIRQ line of each
> HART to 0xffffffff (i.e. -1) in interrupts-extended DT property
> because Linux runs in S-mode only.
> 
> If firmware/bootloader is buggy then it will not correctly update
> interrupts-extended DT property which might result in a plic_handler
> configured twice. This patch adds a warning in plic_init() if a
> plic_handler is already marked present. This warning provides us
> a hint about incorrectly updated interrupts-extended DT property.
> 
> Signed-off-by: Anup Patel <anup@brainfault.org>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index d4433399eb89..3d4f205f8abe 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -234,6 +234,11 @@ static int __init plic_init(struct device_node *node,
>  
>  		cpu = riscv_hartid_to_cpuid(hartid);
>  		handler = per_cpu_ptr(&plic_handlers, cpu);
> +		if (handler->present) {
> +			pr_warn("handler not available for context %d.\n", i);
> +			continue;
> +		}

Shouldn't this be something like "handler already present.."

Otherwise this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3 6/6] irqchip: sifive-plic: Implement irq_set_affinity() for SMP host

[Christoph Hellwig]

On Fri, Nov 30, 2018 at 01:32:07PM +0530, Anup Patel wrote:
> This patch provides irq_set_affinity() implementation for PLIC driver.
> It also updates irq_enable() such that PLIC interrupts are only enabled
> for one of CPUs specified in IRQ affinity mask.

But normally our affinity masks are that - masks of CPUs that can take
it.  It seems a bit odd to then just pick the first one, as this means
with default all-CPU masks we'll have all interrupts handled by the
first CPU only.


> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -106,14 +106,42 @@ static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)
>  
>  static void plic_irq_enable(struct irq_data *d)
>  {
> -	plic_irq_toggle(irq_data_get_affinity_mask(d), d->hwirq, 1);
> +	unsigned int cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> +					   cpu_online_mask);
> +	WARN_ON(cpu >= nr_cpu_ids);

I think this should be WARN_ON_ONCE and actually return instead of then
proceeding using the invalid cpu index. 

> +#ifdef CONFIG_SMP
static int plic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> +			    bool force)
> +{
> +	unsigned int cpu;
> +
> +	if (!force)
> +		cpu = cpumask_any_and(mask_val, cpu_online_mask);
> +	else
> +		cpu = cpumask_first(mask_val);

maybe swap the two branches around to avoid the inversion of the force
flag?

Re: [PATCH v3 2/6] irqchip: sifive-plic: Add struct plic_hw for global PLIC HW details

[Anup Patel]

On Mon, Dec 17, 2018 at 11:54 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +struct plic_hw {
> > +     u32                     nr_irqs;
> > +     u32                     nr_handlers;
> > +     u32                     nr_mapped;
> > +     void __iomem            *regs;
> > +     struct irq_domain       *irqdomain;
> > +};
> > +
> > +static struct plic_hw plic;
>
> Please use local variables instead of a single instance struct.
> And only add these variables in the patches where you actually need
> them.

I am sure it will be useful in-future but for now I will
remove the struct instance.

Regards,
Anup

Re: [PATCH v3 1/6] irqchip: sifive-plic: Pre-compute context hart base and enable base

[Anup Patel]

On Mon, Dec 17, 2018 at 11:55 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Nov 30, 2018 at 01:32:02PM +0530, Anup Patel wrote:
> > This patch does following optimizations:
> > 1. Pre-compute hart base for each context handler
> > 2. Pre-compute enable base for each context handler
> > 3. Have enable lock for each context handler instead
> > of global plic_toggle_lock
>
> All of which is pretty obvious from reading the patch.  The big question
> that needs to be answered in the changelog is why you do that.

To compute enable_base and hart_base is two integer additions and
one integer multiplication. This micro-optimization simply avoids this
repeated operations.

Regards,
Anup

Re: [PATCH v3 4/6] irqchip: sifive-plic: Add warning in plic_init() if handler already present

[Anup Patel]

On Mon, Dec 17, 2018 at 11:58 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Nov 30, 2018 at 01:32:05PM +0530, Anup Patel wrote:
> > We have two enteries (one for M-mode and another for S-mode) in the
> > interrupts-extended DT property of PLIC DT node for each HART. It is
> > expected that firmware/bootloader will set M-mode HWIRQ line of each
> > HART to 0xffffffff (i.e. -1) in interrupts-extended DT property
> > because Linux runs in S-mode only.
> >
> > If firmware/bootloader is buggy then it will not correctly update
> > interrupts-extended DT property which might result in a plic_handler
> > configured twice. This patch adds a warning in plic_init() if a
> > plic_handler is already marked present. This warning provides us
> > a hint about incorrectly updated interrupts-extended DT property.
> >
> > Signed-off-by: Anup Patel <anup@brainfault.org>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index d4433399eb89..3d4f205f8abe 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -234,6 +234,11 @@ static int __init plic_init(struct device_node *node,
> >
> >               cpu = riscv_hartid_to_cpuid(hartid);
> >               handler = per_cpu_ptr(&plic_handlers, cpu);
> > +             if (handler->present) {
> > +                     pr_warn("handler not available for context %d.\n", i);
> > +                     continue;
> > +             }
>
> Shouldn't this be something like "handler already present.."

OK, I will re-phrase it.

>
> Otherwise this looks fine:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Regards,
Anup

Re: [PATCH v3 3/6] irqchip: sifive-plic: More flexible plic_irq_toggle()

[Anup Patel]

On Mon, Dec 17, 2018 at 11:57 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> > -static inline void plic_toggle(struct plic_handler *handler,
> > -                             int hwirq, int enable)
> > +static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
> >  {
> >       u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
> >       u32 hwirq_mask = 1 << (hwirq % 32);
> > @@ -92,27 +91,27 @@ static inline void plic_toggle(struct plic_handler *handler,
> >       raw_spin_unlock(&handler->enable_lock);
> >  }
> >
> > -static inline void plic_irq_toggle(struct irq_data *d, int enable)
> > +static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)
>
> It also removes inline statements which seems rather unrelated to
> the patch description.

Actually these functions should not be inline because plic_toggle() uses
raw_spin_lock() and plic_irq_toggle() uses for-loop.

>
> Also the actual addintion of the single cpumask argument is simple
> enough that it should probably go into the patch that makes use of it.

OK, I will have separate patch for removing "inline" and move addition
of cpumask argument to last patch.

Regards,
Anup

Re: [PATCH v3 6/6] irqchip: sifive-plic: Implement irq_set_affinity() for SMP host

[Anup Patel]

On Tue, Dec 18, 2018 at 12:02 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Nov 30, 2018 at 01:32:07PM +0530, Anup Patel wrote:
> > This patch provides irq_set_affinity() implementation for PLIC driver.
> > It also updates irq_enable() such that PLIC interrupts are only enabled
> > for one of CPUs specified in IRQ affinity mask.
>
> But normally our affinity masks are that - masks of CPUs that can take
> it.  It seems a bit odd to then just pick the first one, as this means
> with default all-CPU masks we'll have all interrupts handled by the
> first CPU only.

Yes, affinity mask are CPUs which can take but there is also effective
affinity mask which represent CPUs which will actually receive IRQ.

Interrupt controllers (unlike PLIC) can support hardware IRQ balancing.
For such interrupt controllers, we inform all CPUs that can take IRQ but
interrupt controller will only deliver IRQ to only one of the CPUs.

There are quite a few interrupt controllers which only allow IRQ to
be taken by exactly one CPU. For such interrupt controllers, the
interrupt controller driver has to to pick one CPU out of CPUs which
can take IRQ (Example GICv2, GICv3, etc).

>
>
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -106,14 +106,42 @@ static void plic_irq_toggle(const struct cpumask *mask, int hwirq, int enable)
> >
> >  static void plic_irq_enable(struct irq_data *d)
> >  {
> > -     plic_irq_toggle(irq_data_get_affinity_mask(d), d->hwirq, 1);
> > +     unsigned int cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > +                                        cpu_online_mask);
> > +     WARN_ON(cpu >= nr_cpu_ids);
>
> I think this should be WARN_ON_ONCE and actually return instead of then
> proceeding using the invalid cpu index.

Sure, will update.

>
> > +#ifdef CONFIG_SMP
> static int plic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> > +                         bool force)
> > +{
> > +     unsigned int cpu;
> > +
> > +     if (!force)
> > +             cpu = cpumask_any_and(mask_val, cpu_online_mask);
> > +     else
> > +             cpu = cpumask_first(mask_val);
>
> maybe swap the two branches around to avoid the inversion of the force
> flag?

Sure, will update.

Regards,
Anup

Re: [PATCH v3 3/6] irqchip: sifive-plic: More flexible plic_irq_toggle()

[Christoph Hellwig]

On Tue, Dec 18, 2018 at 02:20:10PM +0530, Anup Patel wrote:
> Actually these functions should not be inline because plic_toggle() uses
> raw_spin_lock() and plic_irq_toggle() uses for-loop.

So?  It still inlines the all of two instances into each caller
for slightly different but related work.  Not sure it is 100% worth
it, but probably more than the one to move the calculations to init
time..

Re: [PATCH v3 0/6] IRQ affinity support in PLIC driver

[Palmer Dabbelt]

On Mon, 17 Dec 2018 01:37:58 PST (-0800), anup@brainfault.org wrote:
> On Fri, Nov 30, 2018 at 1:32 PM Anup Patel <anup@brainfault.org> wrote:
>>
>> This patchset primarily adds IRQ affinity support in PLIC driver and
>> other improvements.
>>
>> The patchset gives mechanism for explicitly routing external interrupts to
>> particular CPUs using smp_affinity attribute of each Linux IRQs. Also, we
>> can now use IRQ balancer from kernel-space or user-space.
>>
>> The patchset is tested on QEMU virt machine. It is based on Linux-4.20-rc4
>> and can be found at riscv_plic_irq_affinity_v3 branch of:
>> https://github.com/avpatel/linux.git
>>
>> Changes since v2:
>>  - Fixed incorrect address of enable registers using sizeof(u32) in PATCH1
>>  - Retained comment about need for locking in PATCH1
>>  - Split PATCH2 into two patches
>>  - Split PATCH3 into two patches
>>  - Minor fix in commit description of PATCH4
>>
>> Changes since v1:
>>  - Removed few whitspace changes from PATCH1
>>  - Keep use of DEFINE_PER_CPU() as it is
>>
>> Anup Patel (6):
>>   irqchip: sifive-plic: Pre-compute context hart base and enable base
>>   irqchip: sifive-plic: Add struct plic_hw for global PLIC HW details
>>   irqchip: sifive-plic: More flexible plic_irq_toggle()
>>   irqchip: sifive-plic: Add warning in plic_init() if handler already
>>     present
>>   irqchip: sifive-plic: Differentiate between PLIC handler and context
>>   irqchip: sifive-plic: Implement irq_set_affinity() for SMP host
>>
>>  drivers/irqchip/irq-sifive-plic.c | 143 +++++++++++++++++++-----------
>>  1 file changed, 90 insertions(+), 53 deletions(-)
>>
>> --
>> 2.17.1
>>
>
> Any comments on this series?

I also haven't had a chance to look at these yet.

Re: [PATCH v3 3/6] irqchip: sifive-plic: More flexible plic_irq_toggle()

[Anup Patel]

On Wed, Dec 19, 2018 at 9:58 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Dec 18, 2018 at 02:20:10PM +0530, Anup Patel wrote:
> > Actually these functions should not be inline because plic_toggle() uses
> > raw_spin_lock() and plic_irq_toggle() uses for-loop.
>
> So?  It still inlines the all of two instances into each caller
> for slightly different but related work.  Not sure it is 100% worth
> it, but probably more than the one to move the calculations to init
> time..

Not just at init time but these functions will also be used when
irq_affinity is changed by IRQ balancer at runtime.

Regards,
Anup