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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, Nov 30, 2018 at 1:32 PM Anup Patel <[email protected]> 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
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.
> -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.
> +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.
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 <[email protected]>
> ---
> 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 <[email protected]>
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?
On Mon, Dec 17, 2018 at 11:54 PM Christoph Hellwig <[email protected]> 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
On Mon, Dec 17, 2018 at 11:55 PM Christoph Hellwig <[email protected]> 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
On Mon, Dec 17, 2018 at 11:58 PM Christoph Hellwig <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Regards,
Anup
On Mon, Dec 17, 2018 at 11:57 PM Christoph Hellwig <[email protected]> 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
On Tue, Dec 18, 2018 at 12:02 AM Christoph Hellwig <[email protected]> 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
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..
On Mon, 17 Dec 2018 01:37:58 PST (-0800), [email protected] wrote:
> On Fri, Nov 30, 2018 at 1:32 PM Anup Patel <[email protected]> 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.
On Wed, Dec 19, 2018 at 9:58 PM Christoph Hellwig <[email protected]> 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