Re: [PATCH v2 2/8] riscv: add support for misaligned trap handling in S-mode

["Clément Léger" <cleger@rivosinc.com>]

On 04/10/2023 19:00, Björn Töpel wrote:
> Clément Léger <cleger@rivosinc.com> writes:
> 
>> Misalignment trap handling is only supported for M-mode and uses direct
>> accesses to user memory. In S-mode, when handling usermode fault, this
>> requires to use the get_user()/put_user() accessors. Implement
>> load_u8(), store_u8() and get_insn() using these accessors for
>> userspace and direct text access for kernel.
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  arch/riscv/Kconfig                    |   8 ++
>>  arch/riscv/include/asm/entry-common.h |  14 +++
>>  arch/riscv/kernel/Makefile            |   2 +-
>>  arch/riscv/kernel/traps.c             |   9 --
>>  arch/riscv/kernel/traps_misaligned.c  | 119 +++++++++++++++++++++++---
>>  5 files changed, 129 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index d607ab0f7c6d..6e167358a897 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -636,6 +636,14 @@ config THREAD_SIZE_ORDER
>>  	  Specify the Pages of thread stack size (from 4KB to 64KB), which also
>>  	  affects irq stack size, which is equal to thread stack size.
>>  
>> +config RISCV_MISALIGNED
>> +	bool "Support misaligned load/store traps for kernel and userspace"
>> +	default y
>> +	help
>> +	  Say Y here if you want the kernel to embed support for misaligned
>> +	  load/store for both kernel and userspace. When disable, misaligned
>> +	  accesses will generate SIGBUS in userspace and panic in kernel.
>> +
>>  endmenu # "Platform type"
>>  
>>  menu "Kernel features"
>> diff --git a/arch/riscv/include/asm/entry-common.h b/arch/riscv/include/asm/entry-common.h
>> index 6e4dee49d84b..7ab5e34318c8 100644
>> --- a/arch/riscv/include/asm/entry-common.h
>> +++ b/arch/riscv/include/asm/entry-common.h
>> @@ -8,4 +8,18 @@
>>  void handle_page_fault(struct pt_regs *regs);
>>  void handle_break(struct pt_regs *regs);
>>  
>> +#ifdef CONFIG_RISCV_MISALIGNED
>> +int handle_misaligned_load(struct pt_regs *regs);
>> +int handle_misaligned_store(struct pt_regs *regs);
>> +#else
>> +static inline int handle_misaligned_load(struct pt_regs *regs)
>> +{
>> +	return -1;
>> +}
>> +static inline int handle_misaligned_store(struct pt_regs *regs)
>> +{
>> +	return -1;
>> +}
>> +#endif
>> +
>>  #endif /* _ASM_RISCV_ENTRY_COMMON_H */
>> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
>> index 95cf25d48405..0d874fb24b51 100644
>> --- a/arch/riscv/kernel/Makefile
>> +++ b/arch/riscv/kernel/Makefile
>> @@ -59,7 +59,7 @@ obj-y	+= patch.o
>>  obj-y	+= probes/
>>  obj-$(CONFIG_MMU) += vdso.o vdso/
>>  
>> -obj-$(CONFIG_RISCV_M_MODE)	+= traps_misaligned.o
>> +obj-$(CONFIG_RISCV_MISALIGNED)	+= traps_misaligned.o
>>  obj-$(CONFIG_FPU)		+= fpu.o
>>  obj-$(CONFIG_RISCV_ISA_V)	+= vector.o
>>  obj-$(CONFIG_SMP)		+= smpboot.o
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index 19807c4d3805..d69779e4b967 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -179,14 +179,6 @@ asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *re
>>  
>>  DO_ERROR_INFO(do_trap_load_fault,
>>  	SIGSEGV, SEGV_ACCERR, "load access fault");
>> -#ifndef CONFIG_RISCV_M_MODE
>> -DO_ERROR_INFO(do_trap_load_misaligned,
>> -	SIGBUS, BUS_ADRALN, "Oops - load address misaligned");
>> -DO_ERROR_INFO(do_trap_store_misaligned,
>> -	SIGBUS, BUS_ADRALN, "Oops - store (or AMO) address misaligned");
>> -#else
>> -int handle_misaligned_load(struct pt_regs *regs);
>> -int handle_misaligned_store(struct pt_regs *regs);
>>  
>>  asmlinkage __visible __trap_section void do_trap_load_misaligned(struct pt_regs *regs)
>>  {
>> @@ -229,7 +221,6 @@ asmlinkage __visible __trap_section void do_trap_store_misaligned(struct pt_regs
>>  		irqentry_nmi_exit(regs, state);
>>  	}
>>  }
>> -#endif
>>  DO_ERROR_INFO(do_trap_store_fault,
>>  	SIGSEGV, SEGV_ACCERR, "store (or AMO) access fault");
>>  DO_ERROR_INFO(do_trap_ecall_s,
>> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
>> index e7bfb33089c1..9daed7d756ae 100644
>> --- a/arch/riscv/kernel/traps_misaligned.c
>> +++ b/arch/riscv/kernel/traps_misaligned.c
>> @@ -12,6 +12,7 @@
>>  #include <asm/processor.h>
>>  #include <asm/ptrace.h>
>>  #include <asm/csr.h>
>> +#include <asm/entry-common.h>
>>  
>>  #define INSN_MATCH_LB			0x3
>>  #define INSN_MASK_LB			0x707f
>> @@ -151,21 +152,25 @@
>>  #define PRECISION_S 0
>>  #define PRECISION_D 1
>>  
>> -static inline u8 load_u8(const u8 *addr)
>> +#ifdef CONFIG_RISCV_M_MODE
>> +static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val)
>>  {
>>  	u8 val;
>>  
>>  	asm volatile("lbu %0, %1" : "=&r" (val) : "m" (*addr));
>> +	*r_val = val;
>>  
>> -	return val;
>> +	return 0;
>>  }
>>  
>> -static inline void store_u8(u8 *addr, u8 val)
>> +static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val)
>>  {
>>  	asm volatile ("sb %0, %1\n" : : "r" (val), "m" (*addr));
>> +
>> +	return 0;
>>  }
>>  
>> -static inline ulong get_insn(ulong mepc)
>> +static inline int get_insn(struct pt_regs *regs, ulong mepc, ulong *r_insn)
>>  {
>>  	register ulong __mepc asm ("a2") = mepc;
>>  	ulong val, rvc_mask = 3, tmp;
>> @@ -194,9 +199,87 @@ static inline ulong get_insn(ulong mepc)
>>  	: [addr] "r" (__mepc), [rvc_mask] "r" (rvc_mask),
>>  	  [xlen_minus_16] "i" (XLEN_MINUS_16));
>>  
>> -	return val;
>> +	*r_insn = val;
>> +
>> +	return 0;
>> +}
>> +#else
>> +static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val)
>> +{
>> +	if (user_mode(regs)) {
>> +		return __get_user(*r_val, addr);
>> +	} else {
>> +		*r_val = *addr;
>> +		return 0;
>> +	}
> 
> One nit (...well two) ;-)
> 
> If you're respinning I'd get rid of the "inlines",and personally I
> think early exit is easier to read. Applies to the whole patch.

Noted, I'll fix that (checkpatch suggested it so that makes sense).

Clément

> 
>   | {
>   | 	if (user_mode(regs))
>   | 		return __get_user(*r_val, addr);
>   | 
>   |         *r_val = *addr;
>   | 	return 0;
>   | }
> 
> 
> Regardless if you change or not,
> 
> Reviewed-by: Björn Töpel <bjorn@rivosinc.com>