Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion arch/riscv/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,6 @@ endif #RISCV_PMP

config PMP_STACK_GUARD
def_bool y
depends on MULTITHREADING
depends on HW_STACK_PROTECTION

config PMP_STACK_GUARD_MIN_SIZE
Expand Down
10 changes: 10 additions & 0 deletions arch/riscv/core/fatal.c
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,21 @@ static bool bad_stack_pointer(struct arch_esf *esf)
}
#endif /* CONFIG_USERSPACE */

#if CONFIG_MULTITHREADING
if (sp >= _current->stack_info.start - K_KERNEL_STACK_RESERVED &&
sp < _current->stack_info.start - K_KERNEL_STACK_RESERVED
+ Z_RISCV_STACK_GUARD_SIZE) {
return true;
}
#else
uintptr_t isr_stack = (uintptr_t)z_interrupt_stacks;
uintptr_t main_stack = (uintptr_t)z_main_stack;

if ((sp >= isr_stack && sp < isr_stack + Z_RISCV_STACK_GUARD_SIZE) ||
(sp >= main_stack && sp < main_stack + Z_RISCV_STACK_GUARD_SIZE)) {
return true;
}
#endif /* CONFIG_MULTITHREADING */
#endif /* CONFIG_PMP_STACK_GUARD */

#ifdef CONFIG_USERSPACE
Expand Down
24 changes: 21 additions & 3 deletions arch/riscv/core/pmp.c
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,8 @@
*/
void z_riscv_pmp_init(void)
{
unsigned long pmp_addr[5];
unsigned long pmp_cfg[2];
unsigned long pmp_addr[CONFIG_PMP_SLOTS];
unsigned long pmp_cfg[CONFIG_PMP_SLOTS / PMPCFG_STRIDE];
unsigned int index = 0;

/* The read-only area is always there for every mode */
Expand All @@ -370,6 +370,7 @@
#endif

#ifdef CONFIG_PMP_STACK_GUARD
#ifdef CONFIG_MULTITHREADING
/*
* Set the stack guard for this CPU's IRQ stack by making the bottom
* addresses inaccessible. This will never change so we do it here
Expand All @@ -396,6 +397,21 @@

/* And forget about that last entry as we won't need it later */
index--;
#else
/* Without multithreading setup stack guards for IRQ and main stacks */
set_pmp_entry(&index, PMP_NONE | PMP_L,
(uintptr_t)z_interrupt_stacks,
Z_RISCV_STACK_GUARD_SIZE,
pmp_addr, pmp_cfg, ARRAY_SIZE(pmp_addr));

set_pmp_entry(&index, PMP_NONE | PMP_L,
(uintptr_t)z_main_stack,
Z_RISCV_STACK_GUARD_SIZE,
pmp_addr, pmp_cfg, ARRAY_SIZE(pmp_addr));

Check notice on line 410 in arch/riscv/core/pmp.c

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

arch/riscv/core/pmp.c:410 - set_pmp_entry(&index, PMP_NONE | PMP_L, - (uintptr_t)z_interrupt_stacks, - Z_RISCV_STACK_GUARD_SIZE, - pmp_addr, pmp_cfg, ARRAY_SIZE(pmp_addr)); - - set_pmp_entry(&index, PMP_NONE | PMP_L, - (uintptr_t)z_main_stack, - Z_RISCV_STACK_GUARD_SIZE, + set_pmp_entry(&index, PMP_NONE | PMP_L, (uintptr_t)z_interrupt_stacks, + Z_RISCV_STACK_GUARD_SIZE, pmp_addr, pmp_cfg, ARRAY_SIZE(pmp_addr)); + + set_pmp_entry(&index, PMP_NONE | PMP_L, (uintptr_t)z_main_stack, Z_RISCV_STACK_GUARD_SIZE,

/* Write those entries to PMP regs. */
write_pmp_entries(0, index, true, pmp_addr, pmp_cfg, ARRAY_SIZE(pmp_addr));
#endif /* CONFIG_MULTITHREADING */
#else
/* Write those entries to PMP regs. */
write_pmp_entries(0, index, true, pmp_addr, pmp_cfg, ARRAY_SIZE(pmp_addr));
Expand All @@ -419,7 +435,6 @@
}
#endif

__ASSERT(index <= PMPCFG_STRIDE, "provision for one global word only");
global_pmp_cfg[0] = pmp_cfg[0];
global_pmp_last_addr = pmp_addr[index - 1];
global_pmp_end_index = index;
Expand Down Expand Up @@ -454,6 +469,7 @@

#ifdef CONFIG_PMP_STACK_GUARD

#ifdef CONFIG_MULTITHREADING
/**
* @brief Prepare the PMP stackguard content for given thread.
*
Expand Down Expand Up @@ -511,6 +527,8 @@
csr_set(mstatus, MSTATUS_MPRV);
}

#endif /* CONFIG_MULTITHREADING */

/**
* @brief Remove PMP stackguard content to actual PMP registers
*/
Expand Down
1 change: 1 addition & 0 deletions kernel/include/kernel_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ extern struct k_thread z_idle_threads[CONFIG_MP_MAX_NUM_CPUS];
#endif /* CONFIG_MULTITHREADING */
K_KERNEL_PINNED_STACK_ARRAY_DECLARE(z_interrupt_stacks, CONFIG_MP_MAX_NUM_CPUS,
CONFIG_ISR_STACK_SIZE);
K_THREAD_STACK_DECLARE(z_main_stack, CONFIG_MAIN_STACK_SIZE);

#ifdef CONFIG_GEN_PRIV_STACKS
extern uint8_t *z_priv_stack_find(k_thread_stack_t *stack);
Expand Down
13 changes: 13 additions & 0 deletions tests/arch/riscv/pmp/no-multithreading/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.20.0)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(riscv_pmp)

FILE(GLOB app_sources src/*.c)
target_sources(app PRIVATE ${app_sources})

target_include_directories(app PRIVATE
${ZEPHYR_BASE}/kernel/include
${ZEPHYR_BASE}/arch/${ARCH}/include
)
2 changes: 2 additions & 0 deletions tests/arch/riscv/pmp/no-multithreading/prj.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CONFIG_ZTEST=y
CONFIG_MULTITHREADING=n
79 changes: 79 additions & 0 deletions tests/arch/riscv/pmp/no-multithreading/src/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright (c) 2024 Marvell.
* SPDX-License-Identifier: Apache-2.0
*/

#include <kernel_internal.h>
#include <zephyr/tc_util.h>
#include <zephyr/ztest.h>

static volatile ZTEST_BMEM bool valid_fault;

void k_sys_fatal_error_handler(unsigned int reason, const struct arch_esf *pEsf)
{
int rv = TC_PASS;

TC_PRINT("Caught system error -- reason %d %d\n", reason, valid_fault);
if (!valid_fault) {
TC_PRINT("Fatal error was unexpected, aborting...\n");
rv = TC_FAIL;
}
TC_END_RESULT_CUSTOM(rv, "test_pmp");
TC_END_REPORT(rv);
arch_system_halt(reason);
}

#ifdef CONFIG_PMP_STACK_GUARD
static void check_isr_stack_guard(void)
{
char *isr_stack = (char *)z_interrupt_stacks;

valid_fault = true;
*isr_stack = 42;
}

static void check_main_stack_guard(void)
{
char *main_stack = (char *)z_main_stack;

valid_fault = true;
*main_stack = 42;
}

#else

static void check_isr_stack_guard(void)
{
ztest_test_skip();
}

static void check_main_stack_guard(void)
{
ztest_test_skip();
}

#endif /* CONFIG_PMP_STACK_GUARD */

typedef void (*pmp_test_func_t)(void);

static const pmp_test_func_t pmp_test_func[] = {
check_isr_stack_guard,
check_main_stack_guard,
};

/**
* @brief Verify RISC-V specific PMP stack guard regions.
* @details Manually write to the protected stack region to trigger fatal error.
*/
ZTEST(riscv_pmp_no_mt, test_pmp)
{
#ifndef PMP_TEST_FUNC_IDX
#define PMP_TEST_FUNC_IDX 0
#endif
pmp_test_func[PMP_TEST_FUNC_IDX]();

zassert_unreachable("Write to stack guard did not fault");
TC_END_REPORT(TC_FAIL);
}

ZTEST_SUITE(riscv_pmp_no_mt, NULL, NULL, NULL, NULL, NULL);
13 changes: 13 additions & 0 deletions tests/arch/riscv/pmp/no-multithreading/testcase.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
common:
platform_allow:
- qemu_riscv32
- qemu_riscv32e
- qemu_riscv64
filter: CONFIG_RISCV_PMP
ignore_faults: true

tests:
arch.riscv.pmp.no-mt.isr-stack-guard:
extra_args: EXTRA_CFLAGS=-DPMP_TEST_FUNC_IDX=0
Copy link
Member

@ycsin ycsin Nov 19, 2024

Choose a reason for hiding this comment

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

could have used Kconfig instead? i.e. CONFIG_TEST_ISR_STACK_GUARD / CONFIG_TEST_MAIN_STACK_GUARD, and enable it here

Copy link
Author

Choose a reason for hiding this comment

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

I think Kconfig option usually meant to allow user to tweak something.
Here it's already possible to select different tests by name, so I don't see benefits in using Kconfig.

arch.riscv.pmp.no-mt.main-stack-guard:
extra_args: EXTRA_CFLAGS=-DPMP_TEST_FUNC_IDX=1
Loading