Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

arch: riscv: handle interrupt level for CLIC #75581

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+111 −0
Open

arch: riscv: handle interrupt level for CLIC #75581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcf3c28
arch: riscv: handle interrupt level for CLIC
jimmyzhe Jul 3, 2024
8221aac
soc: gd32: gd32vf103: keep the mcause.interrupt by SOC-specific context
jimmyzhe Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions arch/riscv/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,18 @@ config RISCV_HAS_PLIC
config RISCV_HAS_CLIC
bool
depends on RISCV_PRIVILEGED
select RISCV_ALWAYS_SWITCH_THROUGH_ECALL if MULTITHREADING
select CLIC_SUPPORT_INTERRUPT_PREEMPTION if !NRFX_CLIC
help
Does the SOC provide support for a Core-Local Interrupt Controller (CLIC).

config CLIC_SUPPORT_INTERRUPT_PREEMPTION
bool
depends on RISCV_HAS_CLIC
help
This option handles interrupt level in ISR to support interrupt
preemption for nested interrupt.

config RISCV_SOC_EXCEPTION_FROM_IRQ
bool
help
Expand Down
13 changes: 13 additions & 0 deletions arch/riscv/core/isr.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ SECTION_FUNC(exception.entry, _isr_wrapper)
sr s0, __struct_arch_esf_s0_OFFSET(sp)
get_current_cpu s0

#ifdef CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION
/* Save mcause register */
csrr t0, mcause
sr t0, __struct_arch_esf_mcause_OFFSET(sp)
#endif /* CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION */

/* Save MEPC register */
csrr t0, mepc
sr t0, __struct_arch_esf_mepc_OFFSET(sp)
Expand Down Expand Up @@ -721,6 +727,13 @@ fp_trap_exit:
/* Restore MEPC and MSTATUS registers */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for all this masking instead of just restoring the saved mcause value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I mean.

According to CLIC doc, when CLIC mode is enabled, mcause has new fields mcause.mpp and mcause.mpie that mirror mstatus.mpp and mstatus.mpie.
Restoring the entire mcause here pollutes the previous mstatus settings.

Copy link

@tovine tovine Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But mstatus is also restored later (line -689/+707), so it doesn't really matter if you "pollute" it?

Besides, if you end up in a situation where the thread context of mcause and mstatus are inconsistent then I'd say you have bigger problems, as the thread state would essentially be corrupt

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, mcause.mpp and mcause.mpie fields are always overwritten by the later storing to mstatus. The mirror fields in mstatus and mcause always remain the same in the stack frame because they are restored together.

However, the RISC-V PMP stack guard and userspace mechanism are based on mstatus.mpp. I am still figuring out if it is safe when mcause.mpp is restored before access to the stack (line -704/+705).

	lr t0, __struct_arch_esf_mepc_OFFSET(sp)
	lr t2, __struct_arch_esf_mstatus_OFFSET(sp)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, the RISC-V PMP stack guard and userspace mechanism are based on mstatus.mpp

Are you sure this is correct? Sounds a bit strange that they would depends on the previous privilege mode (mpp) instead of the current one.
I don't think mstatus.mpp would take any effect until after executing mret to get back to whatever mode you were in...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstood the userspace. Only the PMP stack guard is based on mstatus.mpp.
According to The RISC-V Instruction Set Manual: Volume II 3.1.6.3. Memory Privilege in mstatus Register, we can use mstatus.mprv and mstatus.mpp to perform load/store operations with MMU translation or PMP protection in M-mode.
The PMP stack guard has used this to detect thread and interrupt stack overflow since PR #44651.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the restoring for mcause. It now saves and restores the entire mcause and avoids storing to memory between restoring mcause and mstatus.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aah, I hadn't seen this one before - sorry about that, you were right about MPP being used in some cases. 🙂

When MPRV=1, load and store memory
addresses are translated and protected, and endianness is applied, as though the current privilege
mode were set to MPP.

I think your updated code looks much better though 👍

lr t0, __struct_arch_esf_mepc_OFFSET(sp)
lr t2, __struct_arch_esf_mstatus_OFFSET(sp)

#ifdef CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION
/* Restore MCAUSE register for previous interrupt level. */
lr t1, __struct_arch_esf_mcause_OFFSET(sp)
csrw mcause, t1
#endif /* CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION */

csrw mepc, t0
csrw mstatus, t2

Expand Down
4 changes: 4 additions & 0 deletions arch/riscv/core/offsets/offsets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ GEN_OFFSET_STRUCT(arch_esf, a7);
GEN_OFFSET_STRUCT(arch_esf, mepc);
GEN_OFFSET_STRUCT(arch_esf, mstatus);

#ifdef CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION
GEN_OFFSET_STRUCT(arch_esf, mcause);
#endif /* CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION */

GEN_OFFSET_STRUCT(arch_esf, s0);

#ifdef CONFIG_USERSPACE
Expand Down
5 changes: 5 additions & 0 deletions arch/riscv/core/thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ void arch_new_thread(struct k_thread *thread, k_thread_stack_t *stack,
stack_init->soc_context = soc_esf_init;
#endif

#ifdef CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION
/* Clear the previous interrupt level. */
stack_init->mcause = 0;
#endif

thread->callee_saved.sp = (unsigned long)stack_init;

/* where to go when returning from z_riscv_switch() */
Expand Down
4 changes: 4 additions & 0 deletions include/zephyr/arch/riscv/exception.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@
unsigned long a7; /* function argument */
#endif /* !CONFIG_RISCV_ISA_RV32E */

#ifdef CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION
unsigned long mcause; /* machine cause register */
#endif /* CONFIG_CLIC_SUPPORT_INTERRUPT_PREEMPTION */

Check notice on line 83 in include/zephyr/arch/riscv/exception.h

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

include/zephyr/arch/riscv/exception.h:83 
-	unsigned long mcause;		/* machine cause register */
+	unsigned long mcause; /* machine cause register */

unsigned long mepc; /* machine exception program counter */
unsigned long mstatus; /* machine status register */

Expand All @@ -99,7 +103,7 @@

#ifdef CONFIG_RISCV_SOC_CONTEXT_SAVE
typedef struct soc_esf soc_esf_t;
#endif

Check notice on line 106 in include/zephyr/arch/riscv/exception.h

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

include/zephyr/arch/riscv/exception.h:106 
-typedef struct soc_esf soc_esf_t;
+	typedef struct soc_esf soc_esf_t;

#ifdef __cplusplus
}
Expand Down
1 change: 1 addition & 0 deletions soc/gd/gd32/gd32vf103/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

zephyr_sources(entry.S)
zephyr_sources(soc.c)
zephyr_sources(soc_irq.S)

zephyr_include_directories(.)

Expand Down
1 change: 1 addition & 0 deletions soc/gd/gd32/gd32vf103/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ config SOC_SERIES_GD32VF103
select RISCV_ISA_EXT_ZICSR
select RISCV_ISA_EXT_ZIFENCEI
select RISCV_HAS_CLIC
select RISCV_SOC_CONTEXT_SAVE
select ATOMIC_OPERATIONS_C
select INCLUDE_RESET_VECTOR
select GD32_HAS_AFIO_PINMUX
Expand Down
18 changes: 18 additions & 0 deletions soc/gd/gd32/gd32vf103/soc_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright (c) 2024 Andes Technology Corporation
*
* SPDX-License-Identifier: Apache-2.0
*/

#ifndef SOC_RISCV_GD32_GD32VF103_SOC_CONTEXT_H
#define SOC_RISCV_GD32_GD32VF103_SOC_CONTEXT_H

#ifdef CONFIG_RISCV_SOC_CONTEXT_SAVE

#define SOC_ESF_MEMBERS

#define SOC_ESF_INIT

#endif /* CONFIG_RISCV_SOC_CONTEXT_SAVE */

#endif /* SOC_RISCV_GD32_GD32VF103_SOC_CONTEXT_H */
40 changes: 40 additions & 0 deletions soc/gd/gd32/gd32vf103/soc_irq.S
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright (c) 2024 Andes Technology Corporation
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/offsets.h>
#include <zephyr/toolchain.h>
#include <zephyr/arch/riscv/irq.h>

/* Exports */
#ifdef CONFIG_RISCV_SOC_CONTEXT_SAVE
GTEXT(__soc_save_context)
GTEXT(__soc_restore_context)
#endif

#ifdef CONFIG_RISCV_SOC_CONTEXT_SAVE

SECTION_FUNC(exception.other, __soc_save_context)

ret

SECTION_FUNC(exception.other, __soc_restore_context)

/*
* For Nuclei ECLIC, the interrupt level (mintstatus.MIL) is restored
* from the previous interrupt level (mcause.MPIL) only if
* mcause.interrupt is set when executing MRET.
* Always set the next context's mcause.interrupt to ensure the
* interrupt level is restored correctly after MRET.
*/
addi a0, a0, -__struct_arch_esf_soc_context_OFFSET
lw t0, __struct_arch_esf_mcause_OFFSET(a0)
li t1, 1 << RISCV_MCAUSE_IRQ_POS
or t0, t0, t1
sw t0, __struct_arch_esf_mcause_OFFSET(a0)

ret

#endif /* CONFIG_RISCV_SOC_CONTEXT_SAVE */
16 changes: 16 additions & 0 deletions soc/gd/gd32/gd32vf103/soc_offsets.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright (c) 2024 Andes Technology Corporation
*
* SPDX-License-Identifier: Apache-2.0
*/

#ifndef SOC_RISCV_GD32_GD32VF103_SOC_OFFSETS_H_
#define SOC_RISCV_GD32_GD32VF103_SOC_OFFSETS_H_

#ifdef CONFIG_RISCV_SOC_OFFSETS

#define GEN_SOC_OFFSET_SYMS()

#endif /* CONFIG_RISCV_SOC_OFFSETS */

#endif /* SOC_RISCV_GD32_GD32VF103_SOC_OFFSETS_H_*/
Loading