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_v2 branch of:
https://github.com/avpatel/linux.git
Changes since v1:
- Removed few whitspace changes from PATCH1
- Keep use of DEFINE_PER_CPU() as it is
Anup Patel (4):
irqchip: sifive-plic: Pre-compute context hart base and enable base
irqchip: sifive-plic: More flexible plic_irq_toggle()
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 | 144 ++++++++++++++++++------------
1 file changed, 86 insertions(+), 58 deletions(-)
--
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 | 41 +++++++++++++------------------
1 file changed, 17 insertions(+), 24 deletions(-)
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 357e9daf94ae..56fce648a901 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -60,36 +60,24 @@ static void __iomem *plic_regs;
struct plic_handler {
bool present;
int ctxid;
+ void __iomem *hart_base;
+ 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)
+static inline void plic_toggle(struct plic_handler *handler,
+ int hwirq, int enable)
{
- 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)
-{
- u32 __iomem *reg = plic_enable_base(ctxid) + (hwirq / 32);
+ u32 __iomem *reg = handler->enable_base + (hwirq / 32);
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 +89,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 +138,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);
@@ -240,11 +228,16 @@ static int __init plic_init(struct device_node *node,
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
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 | 79 +++++++++++++++----------------
1 file changed, 39 insertions(+), 40 deletions(-)
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 56fce648a901..95b4b92ca9b8 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -55,19 +55,26 @@
#define CONTEXT_THRESHOLD 0x00
#define CONTEXT_CLAIM 0x04
-static void __iomem *plic_regs;
-
struct plic_handler {
bool present;
- int ctxid;
void __iomem *hart_base;
raw_spinlock_t enable_lock;
void __iomem *enable_base;
};
+
static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
-static inline void plic_toggle(struct plic_handler *handler,
- int hwirq, int enable)
+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 void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
{
u32 __iomem *reg = handler->enable_base + (hwirq / 32);
u32 hwirq_mask = 1 << (hwirq % 32);
@@ -80,27 +87,23 @@ 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)) {
- struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
-
- if (handler->present)
- plic_toggle(handler, d->hwirq, enable);
- }
+ writel(enable, plic.regs + PRIORITY_BASE + hwirq * PRIORITY_PER_ID);
+ for_each_cpu(cpu, mask)
+ plic_toggle(per_cpu_ptr(&plic_handlers, cpu), 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 = {
@@ -127,8 +130,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
@@ -145,7 +146,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",
@@ -174,36 +175,34 @@ 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;
@@ -227,27 +226,27 @@ 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;
+ 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
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.
Signed-off-by: Anup Patel <[email protected]>
---
drivers/irqchip/irq-sifive-plic.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 95b4b92ca9b8..ffd4deaca057 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -66,8 +66,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;
};
@@ -191,10 +191,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;
plic.irqdomain = irq_domain_add_linear(node, plic.nr_irqs + 1,
@@ -202,7 +202,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;
@@ -225,6 +225,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;
@@ -237,11 +242,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 explicity 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 ffd4deaca057..fec7da3797fa 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -98,14 +98,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",
/*
@@ -114,6 +142,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
On 11/27/18 2:03 AM, 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
>
> Signed-off-by: Anup Patel <[email protected]>
> ---
> drivers/irqchip/irq-sifive-plic.c | 41 +++++++++++++------------------
> 1 file changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 357e9daf94ae..56fce648a901 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -60,36 +60,24 @@ static void __iomem *plic_regs;
> struct plic_handler {
> bool present;
> int ctxid;
> + void __iomem *hart_base;
> + raw_spinlock_t enable_lock;
> + void __iomem *enable_base;
It should be u32. Otherwise, plic_toggle calculates incorrect address
and it does not boot on Unlheased.
> };
> static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
>
> -static inline void __iomem *plic_hart_offset(int ctxid)
> +static inline void plic_toggle(struct plic_handler *handler,
> + int hwirq, int enable)
> {
> - 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.
> - */
Should we keep the comment for enable_lock ?
> -static DEFINE_RAW_SPINLOCK(plic_toggle_lock);
> -
> -static inline void plic_toggle(int ctxid, int hwirq, int enable)
> -{
> - u32 __iomem *reg = plic_enable_base(ctxid) + (hwirq / 32);
> + u32 __iomem *reg = handler->enable_base + (hwirq / 32);
> 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 +89,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 +138,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);
> @@ -240,11 +228,16 @@ static int __init plic_init(struct device_node *node,
> 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++;
> }
>
>
On 11/27/18 2:03 AM, Anup Patel wrote:
> 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 | 79 +++++++++++++++----------------
> 1 file changed, 39 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 56fce648a901..95b4b92ca9b8 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -55,19 +55,26 @@
> #define CONTEXT_THRESHOLD 0x00
> #define CONTEXT_CLAIM 0x04
>
> -static void __iomem *plic_regs;
> -
> struct plic_handler {
> bool present;
> - int ctxid;
> void __iomem *hart_base;
> raw_spinlock_t enable_lock;
> void __iomem *enable_base;
> };
> +
> static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
>
> -static inline void plic_toggle(struct plic_handler *handler,
> - int hwirq, int enable)
> +struct plic_hw {
> + u32 nr_irqs;
> + u32 nr_handlers;
> + u32 nr_mapped;
Why these three are moved inside a structure? I don't see them being
used outside plic_init. Am I missing something ?
> + void __iomem *regs;
> + struct irq_domain *irqdomain;
> +};
> +
> +static struct plic_hw plic;
> +
> +static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
> {
> u32 __iomem *reg = handler->enable_base + (hwirq / 32);
> u32 hwirq_mask = 1 << (hwirq % 32);
> @@ -80,27 +87,23 @@ 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)) {
> - struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
> -
> - if (handler->present)
> - plic_toggle(handler, d->hwirq, enable);
> - }
> + writel(enable, plic.regs + PRIORITY_BASE + hwirq * PRIORITY_PER_ID);
> + for_each_cpu(cpu, mask)
> + plic_toggle(per_cpu_ptr(&plic_handlers, cpu), hwirq, enable);
Any specific reason to remove the handler->present check.
Moreover, only this part matches commit text. Most of the other changes
looks like cosmetic cleanup because of variable is moved to a structure.
May be separate patch for those changes if they are are required at all.
> }
>
> 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 = {
> @@ -127,8 +130,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
> @@ -145,7 +146,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",
> @@ -174,36 +175,34 @@ 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;
>
Should we return EINVAL if irq_domain_add_linear fails ? Earlier, it was
returning ENOMEM.
> - 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;
> @@ -227,27 +226,27 @@ 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;
The previous patch removed all the usage of ctxid. So this line also can
be included in that patch as well to make it more coherent.
Regards,
Atish
> 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;
> }
>
>
On 11/27/18 2:04 AM, Anup Patel wrote:
> 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.
>
> Signed-off-by: Anup Patel <[email protected]>
> ---
> drivers/irqchip/irq-sifive-plic.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 95b4b92ca9b8..ffd4deaca057 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -66,8 +66,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;
> };
> @@ -191,10 +191,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;
>
> plic.irqdomain = irq_domain_add_linear(node, plic.nr_irqs + 1,
> @@ -202,7 +202,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;
> @@ -225,6 +225,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;
> + }
> +
Ahh you have the handler->present check here in this patch. This should
be in the 2nd patch. This change doesn't match the commit text anyways.
Everything else just variable renaming which can be separated.
nr_handlers->nr_contexts
nr_mapped->nr_handlers
Regards,
Atish
> handler->present = true;
> handler->hart_base =
> plic.regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
> @@ -237,11 +242,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;
>
>
On Fri, Nov 30, 2018 at 6:05 AM Atish Patra <[email protected]> wrote:
>
> On 11/27/18 2:03 AM, 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
> >
> > Signed-off-by: Anup Patel <[email protected]>
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 41 +++++++++++++------------------
> > 1 file changed, 17 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 357e9daf94ae..56fce648a901 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -60,36 +60,24 @@ static void __iomem *plic_regs;
> > struct plic_handler {
> > bool present;
> > int ctxid;
> > + void __iomem *hart_base;
> > + raw_spinlock_t enable_lock;
> > + void __iomem *enable_base;
>
> It should be u32. Otherwise, plic_toggle calculates incorrect address
> and it does not boot on Unlheased.
Good catch. I did not see this issue on QEMU because we have
very IRQs over there.
>
> > };
> > static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> >
> > -static inline void __iomem *plic_hart_offset(int ctxid)
> > +static inline void plic_toggle(struct plic_handler *handler,
> > + int hwirq, int enable)
> > {
> > - 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.
> > - */
>
> Should we keep the comment for enable_lock ?
Sure, I will retain the comment for enable_lock.
>
> > -static DEFINE_RAW_SPINLOCK(plic_toggle_lock);
> > -
> > -static inline void plic_toggle(int ctxid, int hwirq, int enable)
> > -{
> > - u32 __iomem *reg = plic_enable_base(ctxid) + (hwirq / 32);
> > + u32 __iomem *reg = handler->enable_base + (hwirq / 32);
> > 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 +89,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 +138,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);
> > @@ -240,11 +228,16 @@ static int __init plic_init(struct device_node *node,
> > 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++;
> > }
> >
> >
>
--
Anup
On Fri, Nov 30, 2018 at 7:09 AM Atish Patra <[email protected]> wrote:
>
> On 11/27/18 2:03 AM, Anup Patel wrote:
> > 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 | 79 +++++++++++++++----------------
> > 1 file changed, 39 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 56fce648a901..95b4b92ca9b8 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -55,19 +55,26 @@
> > #define CONTEXT_THRESHOLD 0x00
> > #define CONTEXT_CLAIM 0x04
> >
> > -static void __iomem *plic_regs;
> > -
> > struct plic_handler {
> > bool present;
> > - int ctxid;
> > void __iomem *hart_base;
> > raw_spinlock_t enable_lock;
> > void __iomem *enable_base;
> > };
> > +
> > static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> >
> > -static inline void plic_toggle(struct plic_handler *handler,
> > - int hwirq, int enable)
> > +struct plic_hw {
> > + u32 nr_irqs;
> > + u32 nr_handlers;
> > + u32 nr_mapped;
>
> Why these three are moved inside a structure? I don't see them being
> used outside plic_init. Am I missing something ?
Yes, these are not used outside plic_init at the moment but these will be
eventually used to implement pm_suspend() and pm_resume() callbacks.
In general, these details can be used for debug sanity checks as well
since these are critical details about PLIC HW.
>
> > + void __iomem *regs;
> > + struct irq_domain *irqdomain;
> > +};
> > +
> > +static struct plic_hw plic;
> > +
> > +static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
> > {
> > u32 __iomem *reg = handler->enable_base + (hwirq / 32);
> > u32 hwirq_mask = 1 << (hwirq % 32);
> > @@ -80,27 +87,23 @@ 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)) {
> > - struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
> > -
> > - if (handler->present)
> > - plic_toggle(handler, d->hwirq, enable);
> > - }
> > + writel(enable, plic.regs + PRIORITY_BASE + hwirq * PRIORITY_PER_ID);
> > + for_each_cpu(cpu, mask)
> > + plic_toggle(per_cpu_ptr(&plic_handlers, cpu), hwirq, enable);
>
> Any specific reason to remove the handler->present check.
>
> Moreover, only this part matches commit text. Most of the other changes
> looks like cosmetic cleanup because of variable is moved to a structure.
> May be separate patch for those changes if they are are required at all.
Actually, these are two changes:
1. Making plic_irq_toggle() flexible
2. Add struct plic_hw to represent global PLIC HW details
I agree this patch is still a mess. I had broken down one big patch into
a patch series at time of sending to LKML but it seems I did a bad job
of breaking into granular patches.
>
> > }
> >
> > 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 = {
> > @@ -127,8 +130,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
> > @@ -145,7 +146,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",
> > @@ -174,36 +175,34 @@ 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;
> >
>
> Should we return EINVAL if irq_domain_add_linear fails ? Earlier, it was
> returning ENOMEM.
Sure, I will update this.
>
> > - 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;
> > @@ -227,27 +226,27 @@ 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;
>
>
> The previous patch removed all the usage of ctxid. So this line also can
> be included in that patch as well to make it more coherent.
Sure, I will move it to previous patch.
Thanks for the detailed review.
Regards,
Anup
On Fri, Nov 30, 2018 at 7:27 AM Atish Patra <[email protected]> wrote:
>
> On 11/27/18 2:04 AM, Anup Patel wrote:
> > 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.
> >
> > Signed-off-by: Anup Patel <[email protected]>
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 21 +++++++++++++--------
> > 1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 95b4b92ca9b8..ffd4deaca057 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -66,8 +66,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;
> > };
> > @@ -191,10 +191,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;
> >
> > plic.irqdomain = irq_domain_add_linear(node, plic.nr_irqs + 1,
> > @@ -202,7 +202,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;
> > @@ -225,6 +225,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;
> > + }
> > +
>
> Ahh you have the handler->present check here in this patch. This should
> be in the 2nd patch. This change doesn't match the commit text anyways.
Sure, will do.
>
> Everything else just variable renaming which can be separated.
> nr_handlers->nr_contexts
> nr_mapped->nr_handlers
>
Sure, will update commit text.
Thanks,
Anup
On 11/27/18 2:04 AM, Anup Patel wrote:
> 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 explicity specify IRQ affinity from
s/explicity/explicitly
> 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 ffd4deaca057..fec7da3797fa 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -98,14 +98,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 is disabled for a fraction of time for cpu as well.
You can use cpumask_andnot to avoid that.
Moreover, something is weird here. I tested the patch in Unleashed with
a debug statement.
Here are the cpumask plic_set_affinity receives.
# echo 0 > /proc[ 280.810000] plic: plic_set_affinity: set affinity [0-1]
[ 280.810000] plic: plic_set_affinity: cpu = [0] irq = 4
# echo 1 > /proc[ 286.290000] plic: plic_set_affinity: set affinity [0]
[ 286.290000] plic: plic_set_affinity: cpu = [0] irq = 4
# echo 2 > /proc[ 292.130000] plic: plic_set_affinity: set affinity [1]
[ 292.130000] plic: plic_set_affinity: cpu = [1] irq = 4
# echo 3 > /proc[ 297.750000] plic: plic_set_affinity: set affinity [0-1]
[ 297.750000] plic: plic_set_affinity: cpu = [0] irq = 4
# echo 2 > /proc/irq/4/smp_affinity
[ 322.850000] plic: plic_set_affinity: set affinity [1]
[ 322.850000] plic: plic_set_affinity: cpu = [1] irq = 4
I have not figured out why it receive cpu mask for 0 & 3.
Not sure if logical cpu id to hart id mapping is responsible for other
two case. I will continue to test tomorrow.
Regards,
Atish
> + }
> +
> + 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",
> /*
> @@ -114,6 +142,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,
>
On Fri, Nov 30, 2018 at 11:29 AM Atish Patra <[email protected]> wrote:
>
> On 11/27/18 2:04 AM, Anup Patel wrote:
> > 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 explicity specify IRQ affinity from
> s/explicity/explicitly
Sure, I will update it.
>
> > 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 ffd4deaca057..fec7da3797fa 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -98,14 +98,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 is disabled for a fraction of time for cpu as well.
> You can use cpumask_andnot to avoid that.
>
>
> Moreover, something is weird here. I tested the patch in Unleashed with
> a debug statement.
>
> Here are the cpumask plic_set_affinity receives.
The smp_affinity in procfs takes hex values as input.
1 = CPU0
2 = CPU1
3 = CPU0-1
4 = CPU2
... and so on ...
>
> # echo 0 > /proc[ 280.810000] plic: plic_set_affinity: set affinity [0-1]
> [ 280.810000] plic: plic_set_affinity: cpu = [0] irq = 4
OK, this is strange.
> # echo 1 > /proc[ 286.290000] plic: plic_set_affinity: set affinity [0]
> [ 286.290000] plic: plic_set_affinity: cpu = [0] irq = 4
This is correct.
> # echo 2 > /proc[ 292.130000] plic: plic_set_affinity: set affinity [1]
> [ 292.130000] plic: plic_set_affinity: cpu = [1] irq = 4
This is correct.
> # echo 3 > /proc[ 297.750000] plic: plic_set_affinity: set affinity [0-1]
> [ 297.750000] plic: plic_set_affinity: cpu = [0] irq = 4
This is correct.
>
> # echo 2 > /proc/irq/4/smp_affinity
> [ 322.850000] plic: plic_set_affinity: set affinity [1]
> [ 322.850000] plic: plic_set_affinity: cpu = [1] irq = 4
This is correct.
>
> I have not figured out why it receive cpu mask for 0 & 3.
> Not sure if logical cpu id to hart id mapping is responsible for other
> two case. I will continue to test tomorrow.
Except value '0', all cases are correct.
Regards,
Anup
On 11/29/18 9:59 PM, Atish Patra wrote:
> On 11/27/18 2:04 AM, Anup Patel wrote:
>> 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 explicity specify IRQ affinity from
> s/explicity/explicitly
>
>> 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 ffd4deaca057..fec7da3797fa 100644
>> --- a/drivers/irqchip/irq-sifive-plic.c
>> +++ b/drivers/irqchip/irq-sifive-plic.c
>> @@ -98,14 +98,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 is disabled for a fraction of time for cpu as well.
> You can use cpumask_andnot to avoid that.
>
>
> Moreover, something is weird here. I tested the patch in Unleashed with
> a debug statement.
>
> Here are the cpumask plic_set_affinity receives.
>
> # echo 0 > /proc[ 280.810000] plic: plic_set_affinity: set affinity [0-1]
> [ 280.810000] plic: plic_set_affinity: cpu = [0] irq = 4
> # echo 1 > /proc[ 286.290000] plic: plic_set_affinity: set affinity [0]
> [ 286.290000] plic: plic_set_affinity: cpu = [0] irq = 4
> # echo 2 > /proc[ 292.130000] plic: plic_set_affinity: set affinity [1]
> [ 292.130000] plic: plic_set_affinity: cpu = [1] irq = 4
> # echo 3 > /proc[ 297.750000] plic: plic_set_affinity: set affinity [0-1]
> [ 297.750000] plic: plic_set_affinity: cpu = [0] irq = 4
>
> # echo 2 > /proc/irq/4/smp_affinity
> [ 322.850000] plic: plic_set_affinity: set affinity [1]
> [ 322.850000] plic: plic_set_affinity: cpu = [1] irq = 4
>
> I have not figured out why it receive cpu mask for 0 & 3.
> Not sure if logical cpu id to hart id mapping is responsible for other
> two case. I will continue to test tomorrow.
>
Never mind.
The input is in hex which explains the cpumask which explains all three
cases except 0.
If we pass zero, it just passing the previous affinity mask.
Regards,
Atish
> Regards,
> Atish
>> + }
>> +
>
>> + 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",
>> /*
>> @@ -114,6 +142,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,
>>
>
>