Re: [PATCH 2/2] irqchip/sifive-plic: Disable S-mode IRQs if running in M-mode

From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Anup Patel <anup@brainfault.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Marc Zyngier <maz@kernel.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>
Subject: Re: [PATCH 2/2] irqchip/sifive-plic: Disable S-mode IRQs if running in M-mode
Date: Tue, 1 Mar 2022 08:30:06 +0000	[thread overview]
Message-ID: <Yh3ZjqTbdDK0dQSt@x1-carbon> (raw)
In-Reply-To: <CAAhSdy0c4Sn=44mwpghpj54jmdHPePXynWQqQDtGM4p0XLj19w@mail.gmail.com>

On Tue, Mar 01, 2022 at 09:48:20AM +0530, Anup Patel wrote:
> On Tue, Mar 1, 2022 at 6:22 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:
> >
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > When detecting a hart context for a privilege mode different from the
> > current running privilege mode, we simply skip to the next hart 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 doesn't seem like a good default when CONFIG_RISCV_M_MODE is set,
> > since in that case we will never enter 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 | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 211bcb10aa93..46caeb11a114 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -326,8 +326,19 @@ 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)) {
> > +                               struct plic_handler tmp_handler = {};
> > +
> > +                               raw_spin_lock_init(&tmp_handler.enable_lock);
> 
> Creating a dummy plic_handler over here is a strange work-around.
> 
> Please define and use
> "void __plic_toggle(void __iomem *enable_base, int hwirq, int enable)"
> over here and in the plic_toggle() function.

Hello Anup,

Your suggestion sounds like a cleaner solution, will fix in V2.

Kind regards,
Niklas
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv