[PATCH 0/2] sifive-plic minor improvements

[PATCH 0/2] sifive-plic minor improvements

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Hello there,

This small series has two small improvements for the sifive-plic
interrupt-controller driver.

Thankful for reviews.

Kind regards,
Niklas


Niklas Cassel (2):
  irqchip/sifive-plic: Improve naming scheme for per context offsets
  irqchip/sifive-plic: Disable S-mode IRQs if running in M-mode

 drivers/irqchip/irq-sifive-plic.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

-- 
2.35.1

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

[PATCH 1/2] irqchip/sifive-plic: Improve naming scheme for per context offsets

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

A hart context is a given privilege mode on a given hart.
The PLIC supports a fixed number of hart contexts (15872).
Each hart context has fixed register offsets in PLIC.

The number of hart contexts for each hart depends on the privilege modes
supported by each hart. Therefore, this mapping between hart context to
hart id is platform specific, and is currently supplied via device tree.

For example, canaan,k210 has the following mapping:
Context0: hart0 M-mode
Context1: hart0 S-mode
Context2: hart1 M-mode
Context3: hart1 S-mode

While sifive,fu540 has the following mapping:
Context0: hart0 M-mode
Context1: hart1 M-mode
Context2: hart1 S-mode

Because the number of hart contexts per hart is not fixed, the names
ENABLE_PER_HART and CONTEXT_PER_HART for the register offsets are quite
confusing and might mislead the reader to think that these are fixed
register offsets per hart.

Rename the offsets to more clearly highlight that they are per hart
_context_ and not per hart.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/irqchip/irq-sifive-plic.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 09cc98266d30..211bcb10aa93 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -41,19 +41,21 @@
 #define     PRIORITY_PER_ID		4
 
 /*
+ * A hart context is a given privilege mode on a given hart.
  * Each hart context has a vector of interrupt enable bits associated with it.
  * There's one bit for each interrupt source.
  */
 #define ENABLE_BASE			0x2000
-#define     ENABLE_PER_HART		0x80
+#define     ENABLE_PER_HART_CTX		0x80
 
 /*
+ * A hart context is a given privilege mode on a given hart.
  * Each hart context has a set of control registers associated with it.  Right
  * now there's only two: a source priority threshold over which the hart will
  * take an interrupt, and a register to claim interrupts.
  */
 #define CONTEXT_BASE			0x200000
-#define     CONTEXT_PER_HART		0x1000
+#define     CONTEXT_PER_HART_CTX	0x1000
 #define     CONTEXT_THRESHOLD		0x00
 #define     CONTEXT_CLAIM		0x04
 
@@ -362,10 +364,10 @@ static int __init plic_init(struct device_node *node,
 		cpumask_set_cpu(cpu, &priv->lmask);
 		handler->present = true;
 		handler->hart_base =
-			priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
+			priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART_CTX;
 		raw_spin_lock_init(&handler->enable_lock);
 		handler->enable_base =
-			priv->regs + ENABLE_BASE + i * ENABLE_PER_HART;
+			priv->regs + ENABLE_BASE + i * ENABLE_PER_HART_CTX;
 		handler->priv = priv;
 done:
 		for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
-- 
2.35.1

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

Re: [PATCH 1/2] irqchip/sifive-plic: Improve naming scheme for per context offsets

[Anup Patel]

On Tue, Mar 1, 2022 at 6:22 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:
>
> From: Niklas Cassel <niklas.cassel@wdc.com>
>
> A hart context is a given privilege mode on a given hart.
> The PLIC supports a fixed number of hart contexts (15872).
> Each hart context has fixed register offsets in PLIC.
>
> The number of hart contexts for each hart depends on the privilege modes
> supported by each hart. Therefore, this mapping between hart context to
> hart id is platform specific, and is currently supplied via device tree.
>
> For example, canaan,k210 has the following mapping:
> Context0: hart0 M-mode
> Context1: hart0 S-mode
> Context2: hart1 M-mode
> Context3: hart1 S-mode
>
> While sifive,fu540 has the following mapping:
> Context0: hart0 M-mode
> Context1: hart1 M-mode
> Context2: hart1 S-mode
>
> Because the number of hart contexts per hart is not fixed, the names
> ENABLE_PER_HART and CONTEXT_PER_HART for the register offsets are quite
> confusing and might mislead the reader to think that these are fixed
> register offsets per hart.
>
> Rename the offsets to more clearly highlight that they are per hart
> _context_ and not per hart.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 09cc98266d30..211bcb10aa93 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -41,19 +41,21 @@
>  #define     PRIORITY_PER_ID            4
>
>  /*
> + * A hart context is a given privilege mode on a given hart.
>   * Each hart context has a vector of interrupt enable bits associated with it.
>   * There's one bit for each interrupt source.
>   */
>  #define ENABLE_BASE                    0x2000
> -#define     ENABLE_PER_HART            0x80
> +#define     ENABLE_PER_HART_CTX                0x80

These are enable registers for each plic-context and we have multiple
plic-context associated with each HART.
(Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc)

Correct name would be ENABLE_PER_CONTEXT instead of
ENABLE_PER_HART_CTX.

Regards,
Anup

>
>  /*
> + * A hart context is a given privilege mode on a given hart.
>   * Each hart context has a set of control registers associated with it.  Right
>   * now there's only two: a source priority threshold over which the hart will
>   * take an interrupt, and a register to claim interrupts.
>   */
>  #define CONTEXT_BASE                   0x200000
> -#define     CONTEXT_PER_HART           0x1000
> +#define     CONTEXT_PER_HART_CTX       0x1000
>  #define     CONTEXT_THRESHOLD          0x00
>  #define     CONTEXT_CLAIM              0x04
>
> @@ -362,10 +364,10 @@ static int __init plic_init(struct device_node *node,
>                 cpumask_set_cpu(cpu, &priv->lmask);
>                 handler->present = true;
>                 handler->hart_base =
> -                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
> +                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART_CTX;
>                 raw_spin_lock_init(&handler->enable_lock);
>                 handler->enable_base =
> -                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART;
> +                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART_CTX;
>                 handler->priv = priv;
>  done:
>                 for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
> --
> 2.35.1
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

Re: [PATCH 1/2] irqchip/sifive-plic: Improve naming scheme for per context offsets

[Niklas Cassel]

On Tue, Mar 01, 2022 at 09:42:46AM +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>
> >
> > A hart context is a given privilege mode on a given hart.
> > The PLIC supports a fixed number of hart contexts (15872).
> > Each hart context has fixed register offsets in PLIC.
> >
> > The number of hart contexts for each hart depends on the privilege modes
> > supported by each hart. Therefore, this mapping between hart context to
> > hart id is platform specific, and is currently supplied via device tree.
> >
> > For example, canaan,k210 has the following mapping:
> > Context0: hart0 M-mode
> > Context1: hart0 S-mode
> > Context2: hart1 M-mode
> > Context3: hart1 S-mode
> >
> > While sifive,fu540 has the following mapping:
> > Context0: hart0 M-mode
> > Context1: hart1 M-mode
> > Context2: hart1 S-mode
> >
> > Because the number of hart contexts per hart is not fixed, the names
> > ENABLE_PER_HART and CONTEXT_PER_HART for the register offsets are quite
> > confusing and might mislead the reader to think that these are fixed
> > register offsets per hart.
> >
> > Rename the offsets to more clearly highlight that they are per hart
> > _context_ and not per hart.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 09cc98266d30..211bcb10aa93 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -41,19 +41,21 @@
> >  #define     PRIORITY_PER_ID            4
> >
> >  /*
> > + * A hart context is a given privilege mode on a given hart.
> >   * Each hart context has a vector of interrupt enable bits associated with it.
> >   * There's one bit for each interrupt source.
> >   */
> >  #define ENABLE_BASE                    0x2000
> > -#define     ENABLE_PER_HART            0x80
> > +#define     ENABLE_PER_HART_CTX                0x80
> 
> These are enable registers for each plic-context and we have multiple
> plic-context associated with each HART.
> (Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc)
> 
> Correct name would be ENABLE_PER_CONTEXT instead of
> ENABLE_PER_HART_CTX.

Hello Anup,

If you look at the RISC-V Privileged Spec v1.11-draft:
https://github.com/riscv/riscv-isa-manual/releases/download/draft-20181201-5449851/riscv-privileged.pdf

They do use the wording "hart context" all the time.

See e.g. 7.3 Interrupt Targets and Hart Contexts

"Interrupt targets are usually hart contexts, where a hart context is a given privilege mode on a
given hart (though there are other possible interrupt targets, such as DMA engines). Not all hart
contexts need be interrupt targets, in particular, if a processor core does not support delegating
external interrupts to lower-privilege modes, then the lower-privilege hart contexts will not be
interrupt targets."


Also see the DT binding, which also uses the term hart context:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml?h=v5.17-rc6


And the existing comments in the driver also uses hart context:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sifive-plic.c?h=v5.17-rc6#n44


I'm not sure why:
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc
Seems to have done search/replace on "hart context" with "context"
in all places except one. Almost looks like some kind of revisionism ;)


I would say that "hart context" seems to be slightly more correct,
and it is already used in many places, dt binding, comments, etc.


However, if you feel that I should search/replace "hart context" with
"context" inside the PLIC driver, to better match the current github spec,
I can do that. It is only used in two places.
(I don't think we should touch the DT binding though. It defines "hart
context", then uses "context".)


If we use ENABLE_PER_CONTEXT, like you suggest, do you have a better
suggestion for CONTEXT_PER_HART_CTX as well?
I don't think we can keep the CONTEXT_ prefix.
And in that case, we probably shouldn't keep the ENABLE_ prefix either.

How about:
PER_CONTEXT_ENABLE_OFFSET and PER_CONTEXT_CTRL_OFFSET?


Kind regards,
Niklas


> 
> Regards,
> Anup
> 
> >
> >  /*
> > + * A hart context is a given privilege mode on a given hart.
> >   * Each hart context has a set of control registers associated with it.  Right
> >   * now there's only two: a source priority threshold over which the hart will
> >   * take an interrupt, and a register to claim interrupts.
> >   */
> >  #define CONTEXT_BASE                   0x200000
> > -#define     CONTEXT_PER_HART           0x1000
> > +#define     CONTEXT_PER_HART_CTX       0x1000
> >  #define     CONTEXT_THRESHOLD          0x00
> >  #define     CONTEXT_CLAIM              0x04
> >
> > @@ -362,10 +364,10 @@ static int __init plic_init(struct device_node *node,
> >                 cpumask_set_cpu(cpu, &priv->lmask);
> >                 handler->present = true;
> >                 handler->hart_base =
> > -                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
> > +                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART_CTX;
> >                 raw_spin_lock_init(&handler->enable_lock);
> >                 handler->enable_base =
> > -                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART;
> > +                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART_CTX;
> >                 handler->priv = priv;
> >  done:
> >                 for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
> > --
> > 2.35.1
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] irqchip/sifive-plic: Improve naming scheme for per context offsets

[Anup Patel]

On Tue, Mar 1, 2022 at 2:36 PM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:
>
> On Tue, Mar 01, 2022 at 09:42:46AM +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>
> > >
> > > A hart context is a given privilege mode on a given hart.
> > > The PLIC supports a fixed number of hart contexts (15872).
> > > Each hart context has fixed register offsets in PLIC.
> > >
> > > The number of hart contexts for each hart depends on the privilege modes
> > > supported by each hart. Therefore, this mapping between hart context to
> > > hart id is platform specific, and is currently supplied via device tree.
> > >
> > > For example, canaan,k210 has the following mapping:
> > > Context0: hart0 M-mode
> > > Context1: hart0 S-mode
> > > Context2: hart1 M-mode
> > > Context3: hart1 S-mode
> > >
> > > While sifive,fu540 has the following mapping:
> > > Context0: hart0 M-mode
> > > Context1: hart1 M-mode
> > > Context2: hart1 S-mode
> > >
> > > Because the number of hart contexts per hart is not fixed, the names
> > > ENABLE_PER_HART and CONTEXT_PER_HART for the register offsets are quite
> > > confusing and might mislead the reader to think that these are fixed
> > > register offsets per hart.
> > >
> > > Rename the offsets to more clearly highlight that they are per hart
> > > _context_ and not per hart.
> > >
> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > ---
> > >  drivers/irqchip/irq-sifive-plic.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > index 09cc98266d30..211bcb10aa93 100644
> > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > @@ -41,19 +41,21 @@
> > >  #define     PRIORITY_PER_ID            4
> > >
> > >  /*
> > > + * A hart context is a given privilege mode on a given hart.
> > >   * Each hart context has a vector of interrupt enable bits associated with it.
> > >   * There's one bit for each interrupt source.
> > >   */
> > >  #define ENABLE_BASE                    0x2000
> > > -#define     ENABLE_PER_HART            0x80
> > > +#define     ENABLE_PER_HART_CTX                0x80
> >
> > These are enable registers for each plic-context and we have multiple
> > plic-context associated with each HART.
> > (Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc)
> >
> > Correct name would be ENABLE_PER_CONTEXT instead of
> > ENABLE_PER_HART_CTX.
>
> Hello Anup,
>
> If you look at the RISC-V Privileged Spec v1.11-draft:
> https://github.com/riscv/riscv-isa-manual/releases/download/draft-20181201-5449851/riscv-privileged.pdf

Clearly, you are referring to a very old draft version of the specification.

>
> They do use the wording "hart context" all the time.
>
> See e.g. 7.3 Interrupt Targets and Hart Contexts
>
> "Interrupt targets are usually hart contexts, where a hart context is a given privilege mode on a
> given hart (though there are other possible interrupt targets, such as DMA engines). Not all hart
> contexts need be interrupt targets, in particular, if a processor core does not support delegating
> external interrupts to lower-privilege modes, then the lower-privilege hart contexts will not be
> interrupt targets."

The latest ratified RISC-V Privileged Spec v1.12 has no such section.

>
>
> Also see the DT binding, which also uses the term hart context:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml?h=v5.17-rc6

I am referring to the RISC-V PLIC spec which simply names these
registers as "interrupt enable" registers.

>
>
> And the existing comments in the driver also uses hart context:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sifive-plic.c?h=v5.17-rc6#n44
>
>
> I'm not sure why:
> https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc
> Seems to have done search/replace on "hart context" with "context"
> in all places except one. Almost looks like some kind of revisionism ;)

The PLIC context output connects to the external interrupt of a specific
privilege mode in HART. If a HART supports both M and S modes then
we will have separate PLIC contexts for HART M-mode external interrupt
and HART S-mode external interrupt.

Clearly, the term "context" is defined by PLIC spec and not the RISC-V
privilege spec.

We also have a new RISC-V AIA specification which has totally different
terminology.

>
>
> I would say that "hart context" seems to be slightly more correct,
> and it is already used in many places, dt binding, comments, etc.
>
>
> However, if you feel that I should search/replace "hart context" with
> "context" inside the PLIC driver, to better match the current github spec,
> I can do that. It is only used in two places.
> (I don't think we should touch the DT binding though. It defines "hart
> context", then uses "context".)
>
>
> If we use ENABLE_PER_CONTEXT, like you suggest, do you have a better
> suggestion for CONTEXT_PER_HART_CTX as well?
> I don't think we can keep the CONTEXT_ prefix.
> And in that case, we probably shouldn't keep the ENABLE_ prefix either.
>
> How about:
> PER_CONTEXT_ENABLE_OFFSET and PER_CONTEXT_CTRL_OFFSET?

We just need three renaming:
1) Rename ENABLE_BASE to CONTEXT_ENABLE_BASE
2) Rename ENABLE_PER_HART to CONTEXT_ENABLE_SIZE
3) Rename CONTEXT_PER_HART to CONTEXT_SIZE

Regards,
Anup

>
>
> Kind regards,
> Niklas
>
>
> >
> > Regards,
> > Anup
> >
> > >
> > >  /*
> > > + * A hart context is a given privilege mode on a given hart.
> > >   * Each hart context has a set of control registers associated with it.  Right
> > >   * now there's only two: a source priority threshold over which the hart will
> > >   * take an interrupt, and a register to claim interrupts.
> > >   */
> > >  #define CONTEXT_BASE                   0x200000
> > > -#define     CONTEXT_PER_HART           0x1000
> > > +#define     CONTEXT_PER_HART_CTX       0x1000
> > >  #define     CONTEXT_THRESHOLD          0x00
> > >  #define     CONTEXT_CLAIM              0x04
> > >
> > > @@ -362,10 +364,10 @@ static int __init plic_init(struct device_node *node,
> > >                 cpumask_set_cpu(cpu, &priv->lmask);
> > >                 handler->present = true;
> > >                 handler->hart_base =
> > > -                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
> > > +                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART_CTX;
> > >                 raw_spin_lock_init(&handler->enable_lock);
> > >                 handler->enable_base =
> > > -                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART;
> > > +                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART_CTX;
> > >                 handler->priv = priv;
> > >  done:
> > >                 for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
> > > --
> > > 2.35.1
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

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

[Niklas Cassel]

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);
+				tmp_handler.enable_base = priv->regs +
+					ENABLE_BASE + i * ENABLE_PER_HART_CTX;
+				for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
+					plic_toggle(&tmp_handler, 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

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

[Anup Patel]

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.

Regards,
Anup

> +                               tmp_handler.enable_base = priv->regs +
> +                                       ENABLE_BASE + i * ENABLE_PER_HART_CTX;
> +                               for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
> +                                       plic_toggle(&tmp_handler, 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

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

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

[Niklas Cassel]

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