| Message ID | 20220301171317.3109161-3-Niklas.Cassel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sifive-plic minor improvements | expand |
On Tue, Mar 1, 2022 at 10:43 PM Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > > From: Niklas Cassel <niklas.cassel@wdc.com> > > When detecting a context for a privilege mode different from the current > running privilege mode, we simply skip to the next context register. > > This means that we never clear the S-mode enable bits when running in > M-mode. > > On canaan k210, a bunch of S-mode interrupts are enabled by the bootrom. > These S-mode specific interrupts should never trigger, since we never set > the mie.SEIE bit in the parent interrupt controller (riscv-intc). > > However, we will be able to see the mip.SEIE bit set as pending. > > This isn't a good default when CONFIG_RISCV_M_MODE is set, since in that > case we will never enter a lower privilege mode (e.g. S-mode). > > Let's clear the S-mode enable bits when running the kernel in M-mode, such > that we won't have a interrupt pending bit set, which we will never clear. > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > drivers/irqchip/irq-sifive-plic.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c > index fc9da94eb816..e6193d66c0ae 100644 > --- a/drivers/irqchip/irq-sifive-plic.c > +++ b/drivers/irqchip/irq-sifive-plic.c > @@ -81,17 +81,23 @@ static int plic_parent_irq __ro_after_init; > static bool plic_cpuhp_setup_done __ro_after_init; > static DEFINE_PER_CPU(struct plic_handler, plic_handlers); > > -static inline void plic_toggle(struct plic_handler *handler, > - int hwirq, int enable) > +static inline void __plic_toggle(void __iomem *enable_base, > + int hwirq, int enable) > { > - u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32); > + u32 __iomem *reg = enable_base + (hwirq / 32) * sizeof(u32); > u32 hwirq_mask = 1 << (hwirq % 32); > > - raw_spin_lock(&handler->enable_lock); > if (enable) > writel(readl(reg) | hwirq_mask, reg); > else > writel(readl(reg) & ~hwirq_mask, reg); > +} > + > +static inline void plic_toggle(struct plic_handler *handler, > + int hwirq, int enable) > +{ > + raw_spin_lock(&handler->enable_lock); > + __plic_toggle(handler->enable_base, hwirq, enable); > raw_spin_unlock(&handler->enable_lock); > } > > @@ -324,8 +330,18 @@ static int __init plic_init(struct device_node *node, > * Skip contexts other than external interrupts for our > * privilege level. > */ > - if (parent.args[0] != RV_IRQ_EXT) > + if (parent.args[0] != RV_IRQ_EXT) { > + /* Disable S-mode enable bits if running in M-mode. */ > + if (IS_ENABLED(CONFIG_RISCV_M_MODE)) { > + void __iomem *enable_base = priv->regs + > + CONTEXT_ENABLE_BASE + > + i * CONTEXT_ENABLE_SIZE; > + > + for (hwirq = 1; hwirq <= nr_irqs; hwirq++) > + __plic_toggle(enable_base, hwirq, 0); > + } > continue; > + } > > hartid = riscv_of_parent_hartid(parent.np); > if (hartid < 0) { > -- > 2.35.1 > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 2022-03-01 17:13, Niklas Cassel wrote: > From: Niklas Cassel <niklas.cassel@wdc.com> > > When detecting a context for a privilege mode different from the > current > running privilege mode, we simply skip to the next context register. > > This means that we never clear the S-mode enable bits when running in > M-mode. > > On canaan k210, a bunch of S-mode interrupts are enabled by the > bootrom. > These S-mode specific interrupts should never trigger, since we never > set > the mie.SEIE bit in the parent interrupt controller (riscv-intc). > > However, we will be able to see the mip.SEIE bit set as pending. > > This isn't a good default when CONFIG_RISCV_M_MODE is set, since in > that > case we will never enter a lower privilege mode (e.g. S-mode). > > Let's clear the S-mode enable bits when running the kernel in M-mode, > such > that we won't have a interrupt pending bit set, which we will never > clear. > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > drivers/irqchip/irq-sifive-plic.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/irqchip/irq-sifive-plic.c > b/drivers/irqchip/irq-sifive-plic.c > index fc9da94eb816..e6193d66c0ae 100644 > --- a/drivers/irqchip/irq-sifive-plic.c > +++ b/drivers/irqchip/irq-sifive-plic.c > @@ -81,17 +81,23 @@ static int plic_parent_irq __ro_after_init; > static bool plic_cpuhp_setup_done __ro_after_init; > static DEFINE_PER_CPU(struct plic_handler, plic_handlers); > > -static inline void plic_toggle(struct plic_handler *handler, > - int hwirq, int enable) > +static inline void __plic_toggle(void __iomem *enable_base, > + int hwirq, int enable) While you're at it, please drop the inline attributes. They really serve no purpose, as that's the job of the compiler. Thanks, M.
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index fc9da94eb816..e6193d66c0ae 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -81,17 +81,23 @@ static int plic_parent_irq __ro_after_init; static bool plic_cpuhp_setup_done __ro_after_init; static DEFINE_PER_CPU(struct plic_handler, plic_handlers); -static inline void plic_toggle(struct plic_handler *handler, - int hwirq, int enable) +static inline void __plic_toggle(void __iomem *enable_base, + int hwirq, int enable) { - u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32); + u32 __iomem *reg = enable_base + (hwirq / 32) * sizeof(u32); u32 hwirq_mask = 1 << (hwirq % 32); - raw_spin_lock(&handler->enable_lock); if (enable) writel(readl(reg) | hwirq_mask, reg); else writel(readl(reg) & ~hwirq_mask, reg); +} + +static inline void plic_toggle(struct plic_handler *handler, + int hwirq, int enable) +{ + raw_spin_lock(&handler->enable_lock); + __plic_toggle(handler->enable_base, hwirq, enable); raw_spin_unlock(&handler->enable_lock); } @@ -324,8 +330,18 @@ static int __init plic_init(struct device_node *node, * Skip contexts other than external interrupts for our * privilege level. */ - if (parent.args[0] != RV_IRQ_EXT) + if (parent.args[0] != RV_IRQ_EXT) { + /* Disable S-mode enable bits if running in M-mode. */ + if (IS_ENABLED(CONFIG_RISCV_M_MODE)) { + void __iomem *enable_base = priv->regs + + CONTEXT_ENABLE_BASE + + i * CONTEXT_ENABLE_SIZE; + + for (hwirq = 1; hwirq <= nr_irqs; hwirq++) + __plic_toggle(enable_base, hwirq, 0); + } continue; + } hartid = riscv_of_parent_hartid(parent.np); if (hartid < 0) {