Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Sat, Sep 29, 2018 at 7:15 AM Palmer Dabbelt wrote: > > On Mon, 10 Sep 2018 06:34:18 PDT (-0700), Christoph Hellwig wrote: > > On Thu, Sep 06, 2018 at 04:15:14PM +0530, Anup Patel wrote: > >> This patch is doing two things: > >> 1. Allow IRQCHIP driver to provide IPI trigger mechanism > > > > And the big questions is why do we want that? The last thing we > > want is for people to "innovate" on how they deliver IPIs. RISC-V > > has defined an SBI interface for it to hide all the details, and > > we should not try to handle systems that are not SBI compliant. > > > > Eventuall we might want to revisit the SBI to improve on shortcomings > > if there are any, but we should not allow random irqchip drivers to > > override this. > > I agree. The whole point of the SBI is to provide an interface that everyone > uses so we can the go figure out how to make this fast later. If each > platform > has their own magic IPI hooks then this will end up being a mess. > > We've got some schemes floating around to make the SBI fast (essentially an > SBI > VDSO), I'd prefer to push on that rather than adding a bunch of complexity > here. Yes, I have already removed the IPI triggering part from this patchset. Regards, Anup
Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Mon, 10 Sep 2018 06:34:18 PDT (-0700), Christoph Hellwig wrote: On Thu, Sep 06, 2018 at 04:15:14PM +0530, Anup Patel wrote: This patch is doing two things: 1. Allow IRQCHIP driver to provide IPI trigger mechanism And the big questions is why do we want that? The last thing we want is for people to "innovate" on how they deliver IPIs. RISC-V has defined an SBI interface for it to hide all the details, and we should not try to handle systems that are not SBI compliant. Eventuall we might want to revisit the SBI to improve on shortcomings if there are any, but we should not allow random irqchip drivers to override this. I agree. The whole point of the SBI is to provide an interface that everyone uses so we can the go figure out how to make this fast later. If each platform has their own magic IPI hooks then this will end up being a mess. We've got some schemes floating around to make the SBI fast (essentially an SBI VDSO), I'd prefer to push on that rather than adding a bunch of complexity here. 2. Have more generic IPI handler in arch/riscv so that IRQCHIP driver can call it And that is rather irrelevant without 1) above.
Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Mon, Sep 10, 2018 at 7:04 PM, Christoph Hellwig wrote: > On Thu, Sep 06, 2018 at 04:15:14PM +0530, Anup Patel wrote: >> This patch is doing two things: >> 1. Allow IRQCHIP driver to provide IPI trigger mechanism > > And the big questions is why do we want that? The last thing we > want is for people to "innovate" on how they deliver IPIs. RISC-V > has defined an SBI interface for it to hide all the details, and > we should not try to handle systems that are not SBI compliant. > > Eventuall we might want to revisit the SBI to improve on shortcomings > if there are any, but we should not allow random irqchip drivers to > override this. I have already dropped this part from the PATCH v2. > >> 2. Have more generic IPI handler in arch/riscv so that IRQCHIP driver >> can call it > > And that is rather irrelevant without 1) above. Nopes, this is required for the RISC-V INTC driver. Regards, Anup
Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Thu, Sep 06, 2018 at 04:15:14PM +0530, Anup Patel wrote: > This patch is doing two things: > 1. Allow IRQCHIP driver to provide IPI trigger mechanism And the big questions is why do we want that? The last thing we want is for people to "innovate" on how they deliver IPIs. RISC-V has defined an SBI interface for it to hide all the details, and we should not try to handle systems that are not SBI compliant. Eventuall we might want to revisit the SBI to improve on shortcomings if there are any, but we should not allow random irqchip drivers to override this. > 2. Have more generic IPI handler in arch/riscv so that IRQCHIP driver > can call it And that is rather irrelevant without 1) above.
Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Thu, Sep 6, 2018 at 3:15 PM, Palmer Dabbelt wrote: > On Tue, 04 Sep 2018 11:50:02 PDT (-0700), Christoph Hellwig wrote: >> >> On Tue, Sep 04, 2018 at 06:15:10PM +0530, Anup Patel wrote: >>> >>> The mechanism to trigger IPI is generally part of interrupt-controller >>> driver for various architectures. On RISC-V, we have an option to trigger >>> IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be >>> triggered using SOC interrupt-controller (e.g. custom PLIC). >> >> >> Which is exactly what we want to avoid, and should not make it easy. >> >> The last thing we need is non-standard whacky IPI mechanisms, and >> that is why we habe SBI calls for it. I think we should simply >> stat that if an RISC-V cpu design bypasse the SBI for no good reason >> we'll simply not support it. > > > I agree. Hiding this sort of stuff is the whole point of the SBI. > > Anup: do you have some concrete reason for trying to avoid the SBI? If it's > just to add non-standard interrupt controllers then I don't think that's a > sufficient reason, as you can just add support for whatever the non-standard > interrupt mechanism is in the SBI implementation -- that's what we're doing > with BBL's CLINT driver, though there's not a whole lot of wackiness there > so at least the SBI implementation is pretty small. > >> So NAK for this patch. > > > Certainly without a compelling reason, and even then I'd only want to take > some standard interrupt controller -- for example, the CLIC (or whatever the > result of the fast interrupts task group is called) could be a viable > option. Even with a standard interrupt controller, we'd need a really > compelling reason to do so. This patch is doing two things: 1. Allow IRQCHIP driver to provide IPI trigger mechanism 2. Have more generic IPI handler in arch/riscv so that IRQCHIP driver can call it The main intention behind point1 was to allow interrupt-controller specific IPI triggering mechanism. I am totally fine in dropping changes related to point1. May be we can revisit this if we find compelling use-case. I will revise this patch to have changes related to point2 only. These changes are required for the new RISCV local interrupt controller driver introduced by this patchset. Regards, Anup
Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Tue, 04 Sep 2018 11:50:02 PDT (-0700), Christoph Hellwig wrote: On Tue, Sep 04, 2018 at 06:15:10PM +0530, Anup Patel wrote: The mechanism to trigger IPI is generally part of interrupt-controller driver for various architectures. On RISC-V, we have an option to trigger IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be triggered using SOC interrupt-controller (e.g. custom PLIC). Which is exactly what we want to avoid, and should not make it easy. The last thing we need is non-standard whacky IPI mechanisms, and that is why we habe SBI calls for it. I think we should simply stat that if an RISC-V cpu design bypasse the SBI for no good reason we'll simply not support it. I agree. Hiding this sort of stuff is the whole point of the SBI. Anup: do you have some concrete reason for trying to avoid the SBI? If it's just to add non-standard interrupt controllers then I don't think that's a sufficient reason, as you can just add support for whatever the non-standard interrupt mechanism is in the SBI implementation -- that's what we're doing with BBL's CLINT driver, though there's not a whole lot of wackiness there so at least the SBI implementation is pretty small. So NAK for this patch. Certainly without a compelling reason, and even then I'd only want to take some standard interrupt controller -- for example, the CLIC (or whatever the result of the fast interrupts task group is called) could be a viable option. Even with a standard interrupt controller, we'd need a really compelling reason to do so.
Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Wed, Sep 05, 2018 at 10:06:24AM +0530, Anup Patel wrote: > It's outrageous to call IPI mechanisms using interrupt-controller as "wacky". I call pluggable IPI whacky, and it's not outragous. What is outragous is your bullshit architecture astronaut patches. There might be a nned to abstract IPI details even more in the future, but the way to do it is either architectureally in the RISC-V privileged spec, or in the SBI spec once we actually have one. Having host OSes go through hoops to provide pluggable IPI implementations is complete bullshit, and the argument that we already have this in some architectures is not a good reason to repeat that mistake.
Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Wed, Sep 5, 2018 at 12:20 AM, Christoph Hellwig wrote: > On Tue, Sep 04, 2018 at 06:15:10PM +0530, Anup Patel wrote: >> The mechanism to trigger IPI is generally part of interrupt-controller >> driver for various architectures. On RISC-V, we have an option to trigger >> IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be >> triggered using SOC interrupt-controller (e.g. custom PLIC). > > Which is exactly what we want to avoid, and should not make it easy. > > The last thing we need is non-standard whacky IPI mechanisms, and > that is why we habe SBI calls for it. I think we should simply > stat that if an RISC-V cpu design bypasse the SBI for no good reason > we'll simply not support it. It's outrageous to call IPI mechanisms using interrupt-controller as "wacky". Lot of architectures have well thought-out interrupt-controller designs with IPI support. In fact having IPIs through interrupt-controller drivers is much faster because SBI call will have it's own overhead and M-mode code with eventually write to some platform-specific/interrupt-controller register. The SBI call only makes sense for very simple interrupt-controller (such as PLIC) which do not provide IPI mechanism. It totally seems like SBI call for triggering IPIs was added as workaround to address limitations of current PLIC. RISC-V systems require a more mature and feature complete interrupt-controllers which supports IPIs, PCI MSI, and Virtualization Extensions. I am sure will see a much better interrupt controller (PLIC++ or something else) soon. > > So NAK for this patch. I think you jumped the gun to quickly here. This patch does two things: 1. Adds a pluggable IPI triggering mechanism 2. Make IPI handling mechanism more generic so that we can call IPI handler from interrupt-controller driver. Your primary objection seems to be for point1 above. I will drop that part only keep changes related to point2 above. Regards, Anup
Re: [RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
On Tue, Sep 04, 2018 at 06:15:10PM +0530, Anup Patel wrote: > The mechanism to trigger IPI is generally part of interrupt-controller > driver for various architectures. On RISC-V, we have an option to trigger > IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be > triggered using SOC interrupt-controller (e.g. custom PLIC). Which is exactly what we want to avoid, and should not make it easy. The last thing we need is non-standard whacky IPI mechanisms, and that is why we habe SBI calls for it. I think we should simply stat that if an RISC-V cpu design bypasse the SBI for no good reason we'll simply not support it. So NAK for this patch.
[RFC PATCH 1/5] RISC-V: Make IPI triggering flexible
The mechanism to trigger IPI is generally part of interrupt-controller
driver for various architectures. On RISC-V, we have an option to trigger
IPI using SBI or SOC vendor can implement RISC-V CPU where IPI will be
triggered using SOC interrupt-controller (e.g. custom PLIC).
This patch makes IPI triggering flexible by providing a mechanism for
interrupt-controller driver to register IPI trigger function using
set_smp_ipi_trigger() function.
Signed-off-by: Anup Patel
---
arch/riscv/include/asm/irq.h | 1 -
arch/riscv/include/asm/smp.h | 10 ++
arch/riscv/kernel/irq.c | 26 +-
arch/riscv/kernel/smp.c | 23 +++
4 files changed, 50 insertions(+), 10 deletions(-)
diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index 996b6fbe17a6..93eb75eac4ff 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -18,7 +18,6 @@
#define NR_IRQS 0
void riscv_timer_interrupt(void);
-void riscv_software_interrupt(void);
#include
diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 36016845461d..72671580e1d4 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -27,6 +27,16 @@
/* SMP initialization hook for setup_arch */
void __init setup_smp(void);
+/*
+ * Called from C code, this handles an IPI.
+ */
+void handle_IPI(struct pt_regs *regs);
+
+/*
+ * Provide a function to raise an IPI on CPUs in callmap.
+ */
+void __init set_smp_ipi_trigger(void (*fn)(const struct cpumask *));
+
/* Hook for the generic smp_call_function_many() routine. */
void arch_send_call_function_ipi_mask(struct cpumask *mask);
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index 0cfac48a1272..5532e7cedaec 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -9,6 +9,8 @@
#include
#include
+#include
+
/*
* Possible interrupt causes:
*/
@@ -26,12 +28,15 @@
asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
{
- struct pt_regs *old_regs = set_irq_regs(regs);
+ struct pt_regs *old_regs;
- irq_enter();
switch (cause & ~INTERRUPT_CAUSE_FLAG) {
case INTERRUPT_CAUSE_TIMER:
+ old_regs = set_irq_regs(regs);
+ irq_enter();
riscv_timer_interrupt();
+ irq_exit();
+ set_irq_regs(old_regs);
break;
#ifdef CONFIG_SMP
case INTERRUPT_CAUSE_SOFTWARE:
@@ -39,21 +44,32 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs,
unsigned long cause)
* We only use software interrupts to pass IPIs, so if a non-SMP
* system gets one, then we don't know what to do.
*/
- riscv_software_interrupt();
+ handle_IPI(regs);
break;
#endif
case INTERRUPT_CAUSE_EXTERNAL:
+ old_regs = set_irq_regs(regs);
+ irq_enter();
handle_arch_irq(regs);
+ irq_exit();
+ set_irq_regs(old_regs);
break;
default:
panic("unexpected interrupt cause");
}
- irq_exit();
+}
- set_irq_regs(old_regs);
+#ifdef CONFIG_SMP
+static void smp_ipi_trigger_sbi(const struct cpumask *to_whom)
+{
+ sbi_send_ipi(cpumask_bits(to_whom));
}
+#endif
void __init init_IRQ(void)
{
irqchip_init();
+#ifdef CONFIG_SMP
+ set_smp_ipi_trigger(smp_ipi_trigger_sbi);
+#endif
}
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 906fe21ea21b..04571d77eceb 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -38,17 +38,19 @@ enum ipi_message_type {
IPI_MAX
};
-
/* Unsupported */
int setup_profiling_timer(unsigned int multiplier)
{
return -EINVAL;
}
-void riscv_software_interrupt(void)
+void handle_IPI(struct pt_regs *regs)
{
+ struct pt_regs *old_regs = set_irq_regs(regs);
unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits;
+ irq_enter();
+
/* Clear pending IPI */
csr_clear(sip, SIE_SSIE);
@@ -60,7 +62,7 @@ void riscv_software_interrupt(void)
ops = xchg(pending_ipis, 0);
if (ops == 0)
- return;
+ goto done;
if (ops & (1 << IPI_RESCHEDULE))
scheduler_ipi();
@@ -73,6 +75,17 @@ void riscv_software_interrupt(void)
/* Order data access and bit testing. */
mb();
}
+
+done:
+ irq_exit();
+ set_irq_regs(old_regs);
+}
+
+static void (*__smp_ipi_trigger)(const struct cpumask *);
+
+void __init set_smp_ipi_trigger(void (*fn)(const struct cpumask *))
+{
+ __smp_ipi_trigger = fn;
}
static void
@@ -85,7 +98,9 @@ send_ipi_message(const struct cpumask *to_whom, enum
ipi_message_type operation)
set_bit(o
