* [PATCH] RISC-V: Don't enable all interrupts in trap_init()
@ 2020-02-02 11:02 Anup Patel
2020-02-02 11:48 ` Atish Patra
0 siblings, 1 reply; 5+ messages in thread
From: Anup Patel @ 2020-02-02 11:02 UTC (permalink / raw)
To: Palmer Dabbelt, Paul Walmsley
Cc: Damien Le Moal, Anup Patel, Anup Patel, linux-kernel, stable,
Atish Patra, Alistair Francis, linux-riscv, Christoph Hellwig
Historically, we have been enabling all interrupts for each
HART in trap_init(). Ideally, we should only enable M-mode
interrupts for M-mode kernel and S-mode interrupts for S-mode
kernel in trap_init().
Currently, we get suprious S-mode interrupts on Kendryte K210
board running M-mode NO-MMU kernel because we are enabling all
interrupts in trap_init(). To fix this, we only enable software
and external interrupt in trap_init(). In future, trap_init()
will only enable software interrupt and PLIC driver will enable
external interrupt using CPU notifiers.
Cc: stable@vger.kernel.org
Fixes: 76d2a0493a17 ("RISC-V: Init and Halt Code)
Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
arch/riscv/kernel/traps.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index f4cad5163bf2..ffb3d94bf0cc 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -156,6 +156,6 @@ void __init trap_init(void)
csr_write(CSR_SCRATCH, 0);
/* Set the exception vector address */
csr_write(CSR_TVEC, &handle_exception);
- /* Enable all interrupts */
- csr_write(CSR_IE, -1);
+ /* Enable interrupts */
+ csr_write(CSR_IE, IE_SIE | IE_EIE);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Don't enable all interrupts in trap_init()
2020-02-02 11:02 [PATCH] RISC-V: Don't enable all interrupts in trap_init() Anup Patel
@ 2020-02-02 11:48 ` Atish Patra
2020-02-18 18:36 ` Palmer Dabbelt
0 siblings, 1 reply; 5+ messages in thread
From: Atish Patra @ 2020-02-02 11:48 UTC (permalink / raw)
To: Anup Patel
Cc: Damien Le Moal, Anup Patel, linux-kernel@vger.kernel.org List,
stable, Atish Patra, Palmer Dabbelt, Paul Walmsley,
Alistair Francis, linux-riscv, Christoph Hellwig
On Sun, Feb 2, 2020 at 3:06 AM Anup Patel <anup.patel@wdc.com> wrote:
>
> Historically, we have been enabling all interrupts for each
> HART in trap_init(). Ideally, we should only enable M-mode
> interrupts for M-mode kernel and S-mode interrupts for S-mode
> kernel in trap_init().
>
> Currently, we get suprious S-mode interrupts on Kendryte K210
> board running M-mode NO-MMU kernel because we are enabling all
> interrupts in trap_init(). To fix this, we only enable software
> and external interrupt in trap_init(). In future, trap_init()
> will only enable software interrupt and PLIC driver will enable
> external interrupt using CPU notifiers.
>
> Cc: stable@vger.kernel.org
> Fixes: 76d2a0493a17 ("RISC-V: Init and Halt Code)
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> ---
> arch/riscv/kernel/traps.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index f4cad5163bf2..ffb3d94bf0cc 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -156,6 +156,6 @@ void __init trap_init(void)
> csr_write(CSR_SCRATCH, 0);
> /* Set the exception vector address */
> csr_write(CSR_TVEC, &handle_exception);
> - /* Enable all interrupts */
> - csr_write(CSR_IE, -1);
> + /* Enable interrupts */
> + csr_write(CSR_IE, IE_SIE | IE_EIE);
> }
> --
> 2.17.1
>
>
Looks good.
Reviewed-by: Atish Patra <atish.patra@wdc.com>
--
Regards,
Atish
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Don't enable all interrupts in trap_init()
2020-02-02 11:48 ` Atish Patra
@ 2020-02-18 18:36 ` Palmer Dabbelt
2020-02-19 3:28 ` Anup Patel
0 siblings, 1 reply; 5+ messages in thread
From: Palmer Dabbelt @ 2020-02-18 18:36 UTC (permalink / raw)
To: atishp
Cc: Damien Le Moal, anup, Anup Patel, linux-kernel, stable,
Atish Patra, Alistair Francis, Paul Walmsley, linux-riscv,
Christoph Hellwig
On Sun, 02 Feb 2020 03:48:18 PST (-0800), atishp@atishpatra.org wrote:
> On Sun, Feb 2, 2020 at 3:06 AM Anup Patel <anup.patel@wdc.com> wrote:
>>
>> Historically, we have been enabling all interrupts for each
>> HART in trap_init(). Ideally, we should only enable M-mode
>> interrupts for M-mode kernel and S-mode interrupts for S-mode
>> kernel in trap_init().
>>
>> Currently, we get suprious S-mode interrupts on Kendryte K210
>> board running M-mode NO-MMU kernel because we are enabling all
>> interrupts in trap_init(). To fix this, we only enable software
>> and external interrupt in trap_init(). In future, trap_init()
>> will only enable software interrupt and PLIC driver will enable
>> external interrupt using CPU notifiers.
I think we should add a proper interrupt controller driver for the per-hart
interrupt controllers, as doing this within the other drivers is ugly -- for
example, there's no reason an MMIO timer or interrupt controller driver should
be toggling these bits.
>> Cc: stable@vger.kernel.org
>> Fixes: 76d2a0493a17 ("RISC-V: Init and Halt Code)
I'd argue this actually fixes the M-mode stuff, since that's the first place
this issue shows up. I've queued this with
Fixes: a4c3733d32a7 ("riscv: abstract out CSR names for supervisor vs machine mode")
instead, as that's the first commit that will actually write to MIE and
therefor the first commit that will actually exhibit bad behavior. It also has
the advantage of making the patch apply on older trees, which should make life
easier for the stable folks.
>> Signed-off-by: Anup Patel <anup.patel@wdc.com>
>> ---
>> arch/riscv/kernel/traps.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index f4cad5163bf2..ffb3d94bf0cc 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -156,6 +156,6 @@ void __init trap_init(void)
>> csr_write(CSR_SCRATCH, 0);
>> /* Set the exception vector address */
>> csr_write(CSR_TVEC, &handle_exception);
>> - /* Enable all interrupts */
>> - csr_write(CSR_IE, -1);
>> + /* Enable interrupts */
>> + csr_write(CSR_IE, IE_SIE | IE_EIE);
>> }
>> --
>> 2.17.1
>>
>>
>
> Looks good.
> Reviewed-by: Atish Patra <atish.patra@wdc.com>
Tested-by: Palmer Dabbelt <palmerdabbelt@google.com> [QMEU virt machine with SMP]
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
I consider this a bugfix, so I'm targeting it for RCs. It's on fixes and
should go up this week.
Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Don't enable all interrupts in trap_init()
2020-02-18 18:36 ` Palmer Dabbelt
@ 2020-02-19 3:28 ` Anup Patel
2020-03-05 18:51 ` Palmer Dabbelt
0 siblings, 1 reply; 5+ messages in thread
From: Anup Patel @ 2020-02-19 3:28 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Damien Le Moal, Atish Patra, Anup Patel,
linux-kernel@vger.kernel.org List, stable, Atish Patra,
Alistair Francis, Paul Walmsley, linux-riscv, Christoph Hellwig
On Wed, Feb 19, 2020 at 12:06 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Sun, 02 Feb 2020 03:48:18 PST (-0800), atishp@atishpatra.org wrote:
> > On Sun, Feb 2, 2020 at 3:06 AM Anup Patel <anup.patel@wdc.com> wrote:
> >>
> >> Historically, we have been enabling all interrupts for each
> >> HART in trap_init(). Ideally, we should only enable M-mode
> >> interrupts for M-mode kernel and S-mode interrupts for S-mode
> >> kernel in trap_init().
> >>
> >> Currently, we get suprious S-mode interrupts on Kendryte K210
> >> board running M-mode NO-MMU kernel because we are enabling all
> >> interrupts in trap_init(). To fix this, we only enable software
> >> and external interrupt in trap_init(). In future, trap_init()
> >> will only enable software interrupt and PLIC driver will enable
> >> external interrupt using CPU notifiers.
>
> I think we should add a proper interrupt controller driver for the per-hart
> interrupt controllers, as doing this within the other drivers is ugly -- for
> example, there's no reason an MMIO timer or interrupt controller driver should
> be toggling these bits.
I have always been in support of having per-hart interrupt controller driver.
I will rebase my RISC-V INTC driver upon latest kernel and send it again.
Of course, now the situation has changed the RISC-V INTC driver will
have to consider NOMMU kernel as well.
The last version of RISC-V INTC driver can be found in riscv_intc_v2
branch of https://github.com/avpatel/linux.git
>
> >> Cc: stable@vger.kernel.org
> >> Fixes: 76d2a0493a17 ("RISC-V: Init and Halt Code)
>
> I'd argue this actually fixes the M-mode stuff, since that's the first place
> this issue shows up. I've queued this with
>
> Fixes: a4c3733d32a7 ("riscv: abstract out CSR names for supervisor vs machine mode")
>
> instead, as that's the first commit that will actually write to MIE and
> therefor the first commit that will actually exhibit bad behavior. It also has
> the advantage of making the patch apply on older trees, which should make life
> easier for the stable folks.
Sure, no problem.
>
> >> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> >> ---
> >> arch/riscv/kernel/traps.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> >> index f4cad5163bf2..ffb3d94bf0cc 100644
> >> --- a/arch/riscv/kernel/traps.c
> >> +++ b/arch/riscv/kernel/traps.c
> >> @@ -156,6 +156,6 @@ void __init trap_init(void)
> >> csr_write(CSR_SCRATCH, 0);
> >> /* Set the exception vector address */
> >> csr_write(CSR_TVEC, &handle_exception);
> >> - /* Enable all interrupts */
> >> - csr_write(CSR_IE, -1);
> >> + /* Enable interrupts */
> >> + csr_write(CSR_IE, IE_SIE | IE_EIE);
> >> }
> >> --
> >> 2.17.1
> >>
> >>
> >
> > Looks good.
> > Reviewed-by: Atish Patra <atish.patra@wdc.com>
>
> Tested-by: Palmer Dabbelt <palmerdabbelt@google.com> [QMEU virt machine with SMP]
> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
>
> I consider this a bugfix, so I'm targeting it for RCs. It's on fixes and
> should go up this week.
>
> Thanks!
Thanks,
Anup
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RISC-V: Don't enable all interrupts in trap_init()
2020-02-19 3:28 ` Anup Patel
@ 2020-03-05 18:51 ` Palmer Dabbelt
0 siblings, 0 replies; 5+ messages in thread
From: Palmer Dabbelt @ 2020-03-05 18:51 UTC (permalink / raw)
To: anup
Cc: Damien Le Moal, atishp, Anup Patel, linux-kernel, stable,
Atish Patra, Alistair Francis, Paul Walmsley, linux-riscv,
Christoph Hellwig
On Tue, 18 Feb 2020 19:28:38 PST (-0800), anup@brainfault.org wrote:
> On Wed, Feb 19, 2020 at 12:06 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Sun, 02 Feb 2020 03:48:18 PST (-0800), atishp@atishpatra.org wrote:
>> > On Sun, Feb 2, 2020 at 3:06 AM Anup Patel <anup.patel@wdc.com> wrote:
>> >>
>> >> Historically, we have been enabling all interrupts for each
>> >> HART in trap_init(). Ideally, we should only enable M-mode
>> >> interrupts for M-mode kernel and S-mode interrupts for S-mode
>> >> kernel in trap_init().
>> >>
>> >> Currently, we get suprious S-mode interrupts on Kendryte K210
>> >> board running M-mode NO-MMU kernel because we are enabling all
>> >> interrupts in trap_init(). To fix this, we only enable software
>> >> and external interrupt in trap_init(). In future, trap_init()
>> >> will only enable software interrupt and PLIC driver will enable
>> >> external interrupt using CPU notifiers.
>>
>> I think we should add a proper interrupt controller driver for the per-hart
>> interrupt controllers, as doing this within the other drivers is ugly -- for
>> example, there's no reason an MMIO timer or interrupt controller driver should
>> be toggling these bits.
>
> I have always been in support of having per-hart interrupt controller driver.
>
> I will rebase my RISC-V INTC driver upon latest kernel and send it again.
> Of course, now the situation has changed the RISC-V INTC driver will
> have to consider NOMMU kernel as well.
>
> The last version of RISC-V INTC driver can be found in riscv_intc_v2
> branch of https://github.com/avpatel/linux.git
Thanks. I think I saw some patches go by, so let's talk over there.
>
>>
>> >> Cc: stable@vger.kernel.org
>> >> Fixes: 76d2a0493a17 ("RISC-V: Init and Halt Code)
>>
>> I'd argue this actually fixes the M-mode stuff, since that's the first place
>> this issue shows up. I've queued this with
>>
>> Fixes: a4c3733d32a7 ("riscv: abstract out CSR names for supervisor vs machine mode")
>>
>> instead, as that's the first commit that will actually write to MIE and
>> therefor the first commit that will actually exhibit bad behavior. It also has
>> the advantage of making the patch apply on older trees, which should make life
>> easier for the stable folks.
>
> Sure, no problem.
>
>>
>> >> Signed-off-by: Anup Patel <anup.patel@wdc.com>
>> >> ---
>> >> arch/riscv/kernel/traps.c | 4 ++--
>> >> 1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> >> index f4cad5163bf2..ffb3d94bf0cc 100644
>> >> --- a/arch/riscv/kernel/traps.c
>> >> +++ b/arch/riscv/kernel/traps.c
>> >> @@ -156,6 +156,6 @@ void __init trap_init(void)
>> >> csr_write(CSR_SCRATCH, 0);
>> >> /* Set the exception vector address */
>> >> csr_write(CSR_TVEC, &handle_exception);
>> >> - /* Enable all interrupts */
>> >> - csr_write(CSR_IE, -1);
>> >> + /* Enable interrupts */
>> >> + csr_write(CSR_IE, IE_SIE | IE_EIE);
>> >> }
>> >> --
>> >> 2.17.1
>> >>
>> >>
>> >
>> > Looks good.
>> > Reviewed-by: Atish Patra <atish.patra@wdc.com>
>>
>> Tested-by: Palmer Dabbelt <palmerdabbelt@google.com> [QMEU virt machine with SMP]
>> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
>>
>> I consider this a bugfix, so I'm targeting it for RCs. It's on fixes and
>> should go up this week.
>>
>> Thanks!
>
> Thanks,
> Anup
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-05 18:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-02 11:02 [PATCH] RISC-V: Don't enable all interrupts in trap_init() Anup Patel
2020-02-02 11:48 ` Atish Patra
2020-02-18 18:36 ` Palmer Dabbelt
2020-02-19 3:28 ` Anup Patel
2020-03-05 18:51 ` Palmer Dabbelt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).