[PATCH 0/7] Add support to handle misaligned accesses in S-mode

[PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Clément Léger]

Since commit 61cadb9 ("Provide new description of misaligned load/store
behavior compatible with privileged architecture.") in the RISC-V ISA
manual, it is stated that misaligned load/store might not be supported.
However, the RISC-V kernel uABI describes that misaligned accesses are
supported. In order to support that, this series adds support for S-mode
handling of misaligned accesses as well support for prctl(PR_UNALIGN).

Handling misaligned access in kernel allows for a finer grain control
of the misaligned accesses behavior, and thanks to the prctl call, can
allow disabling misaligned access emulation to generate SIGBUS. User
space can then optimize its software by removing such access based on
SIGBUS generation.

Currently, this series is useful for people that uses a SBI that does
not handled misaligned traps. In a near future, this series will make
use a SBI extension [1] allowing to request delegation of the
misaligned load/store traps to the S-mode software. This extension has
been submitted for review to the riscv tech-prs group. An OpenSBI
implementation for this spec is available at [2].

This series can be tested using the spike simulator [3] and an openSBI
version [4] which allows to always delegate misaligned load/store to
S-mode.

[1] https://lists.riscv.org/g/tech-prs/message/540
[2] https://github.com/rivosinc/opensbi/tree/dev/cleger/fw_feature_upstream
[3] https://github.com/riscv-software-src/riscv-isa-sim
[4] https://github.com/rivosinc/opensbi/tree/dev/cleger/no_misaligned

Clément Léger (7):
  riscv: remove unused functions in traps_misaligned.c
  riscv: add support for misaligned handling in S-mode
  riscv: report perf event for misaligned fault
  riscv: add floating point insn support to misaligned access emulation
  riscv: add support for sysctl unaligned_enabled control
  riscv: report misaligned accesses emulation to hwprobe
  riscv: add support for PR_SET_UNALIGN and PR_GET_UNALIGN

 arch/riscv/Kconfig                    |   1 +
 arch/riscv/include/asm/cpufeature.h   |   6 +
 arch/riscv/include/asm/entry-common.h |   3 +
 arch/riscv/include/asm/processor.h    |   9 +
 arch/riscv/kernel/Makefile            |   2 +-
 arch/riscv/kernel/cpufeature.c        |   6 +-
 arch/riscv/kernel/fpu.S               | 117 ++++++++
 arch/riscv/kernel/process.c           |  18 ++
 arch/riscv/kernel/setup.c             |   1 +
 arch/riscv/kernel/traps.c             |   9 -
 arch/riscv/kernel/traps_misaligned.c  | 374 ++++++++++++++++++++++----
 11 files changed, 488 insertions(+), 58 deletions(-)

-- 
2.40.1


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

[PATCH 1/7] riscv: remove unused functions in traps_misaligned.c

[Clément Léger]

Replace macros by the only two function calls that are done from this
file, store_u8() and load_u8().

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/kernel/traps_misaligned.c | 46 +++++-----------------------
 1 file changed, 7 insertions(+), 39 deletions(-)

diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
index 378f5b151443..e7bfb33089c1 100644
--- a/arch/riscv/kernel/traps_misaligned.c
+++ b/arch/riscv/kernel/traps_misaligned.c
@@ -151,51 +151,19 @@
 #define PRECISION_S 0
 #define PRECISION_D 1
 
-#define DECLARE_UNPRIVILEGED_LOAD_FUNCTION(type, insn)			\
-static inline type load_##type(const type *addr)			\
-{									\
-	type val;							\
-	asm (#insn " %0, %1"						\
-	: "=&r" (val) : "m" (*addr));					\
-	return val;							\
-}
+static inline u8 load_u8(const u8 *addr)
+{
+	u8 val;
 
-#define DECLARE_UNPRIVILEGED_STORE_FUNCTION(type, insn)			\
-static inline void store_##type(type *addr, type val)			\
-{									\
-	asm volatile (#insn " %0, %1\n"					\
-	: : "r" (val), "m" (*addr));					\
-}
+	asm volatile("lbu %0, %1" : "=&r" (val) : "m" (*addr));
 
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(u8, lbu)
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(u16, lhu)
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(s8, lb)
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(s16, lh)
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(s32, lw)
-DECLARE_UNPRIVILEGED_STORE_FUNCTION(u8, sb)
-DECLARE_UNPRIVILEGED_STORE_FUNCTION(u16, sh)
-DECLARE_UNPRIVILEGED_STORE_FUNCTION(u32, sw)
-#if defined(CONFIG_64BIT)
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(u32, lwu)
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(u64, ld)
-DECLARE_UNPRIVILEGED_STORE_FUNCTION(u64, sd)
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(ulong, ld)
-#else
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(u32, lw)
-DECLARE_UNPRIVILEGED_LOAD_FUNCTION(ulong, lw)
-
-static inline u64 load_u64(const u64 *addr)
-{
-	return load_u32((u32 *)addr)
-		+ ((u64)load_u32((u32 *)addr + 1) << 32);
+	return val;
 }
 
-static inline void store_u64(u64 *addr, u64 val)
+static inline void store_u8(u8 *addr, u8 val)
 {
-	store_u32((u32 *)addr, val);
-	store_u32((u32 *)addr + 1, val >> 32);
+	asm volatile ("sb %0, %1\n" : : "r" (val), "m" (*addr));
 }
-#endif
 
 static inline ulong get_insn(ulong mepc)
 {
-- 
2.40.1


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

[PATCH 2/7] riscv: add support for misaligned handling in S-mode

[Clément Léger]

Misalignment handling is only supported for M-mode and uses direct
accesses to user memory. In S-mode, when handlnig usermode fault,
this requires to use the get_user()/put_user() accessors. Implement
load_u8(), store_u8() and get_insn() using these accessors.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/include/asm/entry-common.h |   3 +
 arch/riscv/kernel/Makefile            |   2 +-
 arch/riscv/kernel/traps.c             |   9 --
 arch/riscv/kernel/traps_misaligned.c  | 119 +++++++++++++++++++++++---
 4 files changed, 110 insertions(+), 23 deletions(-)

diff --git a/arch/riscv/include/asm/entry-common.h b/arch/riscv/include/asm/entry-common.h
index 6e4dee49d84b..58e9e2976e1b 100644
--- a/arch/riscv/include/asm/entry-common.h
+++ b/arch/riscv/include/asm/entry-common.h
@@ -8,4 +8,7 @@
 void handle_page_fault(struct pt_regs *regs);
 void handle_break(struct pt_regs *regs);
 
+int handle_misaligned_load(struct pt_regs *regs);
+int handle_misaligned_store(struct pt_regs *regs);
+
 #endif /* _ASM_RISCV_ENTRY_COMMON_H */
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index 95cf25d48405..ccdfd029c511 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -56,10 +56,10 @@ obj-y	+= riscv_ksyms.o
 obj-y	+= stacktrace.o
 obj-y	+= cacheinfo.o
 obj-y	+= patch.o
+obj-y	+= traps_misaligned.o
 obj-y	+= probes/
 obj-$(CONFIG_MMU) += vdso.o vdso/
 
-obj-$(CONFIG_RISCV_M_MODE)	+= 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;
+	}
 }
 
+static inline int store_u8(struct pt_regs *regs, u8 *addr, u8 val)
+{
+	if (user_mode(regs)) {
+		return __put_user(val, addr);
+	} else {
+		*addr = val;
+		return 0;
+	}
+}
+
+#define __read_insn(regs, insn, insn_addr)		\
+({							\
+	int __ret;					\
+							\
+	if (user_mode(regs)) {				\
+		__ret = __get_user(insn, insn_addr);	\
+	} else {					\
+		insn = *insn_addr;			\
+		__ret = 0;				\
+	}						\
+							\
+	__ret;						\
+})
+
+static inline int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn)
+{
+	ulong insn = 0;
+
+	if (epc & 0x2) {
+		ulong tmp = 0;
+		u16 __user *insn_addr = (u16 __user *)epc;
+
+		if (__read_insn(regs, insn, insn_addr))
+			return -EFAULT;
+		/* __get_user() uses regular "lw" which sign extend the loaded
+		 * value make sure to clear higher order bits in case we "or" it
+		 * below with the upper 16 bits half.
+		 */
+		insn &= GENMASK(15, 0);
+		if ((insn & __INSN_LENGTH_MASK) != __INSN_LENGTH_32) {
+			*r_insn = insn;
+			return 0;
+		}
+		insn_addr++;
+		if (__read_insn(regs, tmp, insn_addr))
+			return -EFAULT;
+		*r_insn = (tmp << 16) | insn;
+
+		return 0;
+	} else {
+		u32 __user *insn_addr = (u32 __user *)epc;
+
+		if (__read_insn(regs, insn, insn_addr))
+			return -EFAULT;
+		if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) {
+			*r_insn = insn;
+			return 0;
+		}
+		insn &= GENMASK(15, 0);
+		*r_insn = insn;
+
+		return 0;
+	}
+}
+#endif
+
 union reg_data {
 	u8 data_bytes[8];
 	ulong data_ulong;
@@ -207,10 +290,13 @@ int handle_misaligned_load(struct pt_regs *regs)
 {
 	union reg_data val;
 	unsigned long epc = regs->epc;
-	unsigned long insn = get_insn(epc);
-	unsigned long addr = csr_read(mtval);
+	unsigned long insn;
+	unsigned long addr = regs->badaddr;
 	int i, fp = 0, shift = 0, len = 0;
 
+	if (get_insn(regs, epc, &insn))
+		return -1;
+
 	regs->epc = 0;
 
 	if ((insn & INSN_MASK_LW) == INSN_MATCH_LW) {
@@ -274,8 +360,10 @@ int handle_misaligned_load(struct pt_regs *regs)
 	}
 
 	val.data_u64 = 0;
-	for (i = 0; i < len; i++)
-		val.data_bytes[i] = load_u8((void *)(addr + i));
+	for (i = 0; i < len; i++) {
+		if (load_u8(regs, (void *)(addr + i), &val.data_bytes[i]))
+			return -1;
+	}
 
 	if (fp)
 		return -1;
@@ -290,10 +378,13 @@ int handle_misaligned_store(struct pt_regs *regs)
 {
 	union reg_data val;
 	unsigned long epc = regs->epc;
-	unsigned long insn = get_insn(epc);
-	unsigned long addr = csr_read(mtval);
+	unsigned long insn;
+	unsigned long addr = regs->badaddr;
 	int i, len = 0;
 
+	if (get_insn(regs, epc, &insn))
+		return -1;
+
 	regs->epc = 0;
 
 	val.data_ulong = GET_RS2(insn, regs);
@@ -327,8 +418,10 @@ int handle_misaligned_store(struct pt_regs *regs)
 		return -1;
 	}
 
-	for (i = 0; i < len; i++)
-		store_u8((void *)(addr + i), val.data_bytes[i]);
+	for (i = 0; i < len; i++) {
+		if (store_u8(regs, (void *)(addr + i), val.data_bytes[i]))
+			return -1;
+	}
 
 	regs->epc = epc + INSN_LEN(insn);
 
-- 
2.40.1


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

[PATCH 3/7] riscv: report perf event for misaligned fault

[Clément Léger]

Add missing calls to account for misaligned fault event using
perf_sw_event().

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/kernel/traps_misaligned.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
index 9daed7d756ae..804f6c5e0e44 100644
--- a/arch/riscv/kernel/traps_misaligned.c
+++ b/arch/riscv/kernel/traps_misaligned.c
@@ -6,6 +6,7 @@
 #include <linux/init.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/perf_event.h>
 #include <linux/irq.h>
 #include <linux/stringify.h>
 
@@ -294,6 +295,8 @@ int handle_misaligned_load(struct pt_regs *regs)
 	unsigned long addr = regs->badaddr;
 	int i, fp = 0, shift = 0, len = 0;
 
+	perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS, 1, regs, addr);
+
 	if (get_insn(regs, epc, &insn))
 		return -1;
 
@@ -382,6 +385,8 @@ int handle_misaligned_store(struct pt_regs *regs)
 	unsigned long addr = regs->badaddr;
 	int i, len = 0;
 
+	perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS, 1, regs, addr);
+
 	if (get_insn(regs, epc, &insn))
 		return -1;
 
-- 
2.40.1


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

[PATCH 4/7] riscv: add floating point insn support to misaligned access emulation

[Clément Léger]

This support is partially based of openSBI misaligned emulation floating
point instruction support. It provides support for the existing
floating point instructions (both for 32/64 bits as well as compressed
ones). Since floating point registers are not part of the pt_regs
struct, we need to modify them directly using some assembly. We also
dirty the pt_regs status in case we modify them to be sure context
switch will save FP state. With this support, Linux is on par with
openSBI support.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/kernel/fpu.S              | 117 +++++++++++++++++++++
 arch/riscv/kernel/traps_misaligned.c | 152 ++++++++++++++++++++++++++-
 2 files changed, 265 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/kernel/fpu.S b/arch/riscv/kernel/fpu.S
index dd2205473de7..2785badb247c 100644
--- a/arch/riscv/kernel/fpu.S
+++ b/arch/riscv/kernel/fpu.S
@@ -104,3 +104,120 @@ ENTRY(__fstate_restore)
 	csrc CSR_STATUS, t1
 	ret
 ENDPROC(__fstate_restore)
+
+#define get_f32(which) fmv.x.s a0, which; j 2f
+#define put_f32(which) fmv.s.x which, a1; j 2f
+#if __riscv_xlen == 64
+# define get_f64(which) fmv.x.d a0, which; j 2f
+# define put_f64(which) fmv.d.x which, a1; j 2f
+#else
+# define get_f64(which) fsd which, 0(a1); j 2f
+# define put_f64(which) fld which, 0(a1); j 2f
+#endif
+
+.macro fp_access_prologue
+	/*
+	 * Compute jump offset to store the correct FP register since we don't
+	 * have indirect FP register access
+	 */
+	sll t0, a0, 3
+	la t2, 1f
+	add t0, t0, t2
+	li t1, SR_FS
+	csrs CSR_STATUS, t1
+	jr t0
+1:
+.endm
+
+.macro fp_access_epilogue
+2:
+	csrc CSR_STATUS, t1
+	ret
+.endm
+
+#define fp_access_body(__access_func) \
+	__access_func(f0); \
+	__access_func(f1); \
+	__access_func(f2); \
+	__access_func(f3); \
+	__access_func(f4); \
+	__access_func(f5); \
+	__access_func(f6); \
+	__access_func(f7); \
+	__access_func(f8); \
+	__access_func(f9); \
+	__access_func(f10); \
+	__access_func(f11); \
+	__access_func(f12); \
+	__access_func(f13); \
+	__access_func(f14); \
+	__access_func(f15); \
+	__access_func(f16); \
+	__access_func(f17); \
+	__access_func(f18); \
+	__access_func(f19); \
+	__access_func(f20); \
+	__access_func(f21); \
+	__access_func(f22); \
+	__access_func(f23); \
+	__access_func(f24); \
+	__access_func(f25); \
+	__access_func(f26); \
+	__access_func(f27); \
+	__access_func(f28); \
+	__access_func(f29); \
+	__access_func(f30); \
+	__access_func(f31)
+
+
+/*
+ * Disable compressed instructions set to keep a constant offset between FP
+ * load/store/move instructions
+ */
+.option norvc
+/*
+ * put_f32_reg - Set a FP register from a register containing the value
+ * a0 = FP register index to be set
+ * a1 = value to be loaded in the FP register
+ */
+SYM_FUNC_START(put_f32_reg)
+	fp_access_prologue
+	fp_access_body(put_f32)
+	fp_access_epilogue
+SYM_FUNC_END(put_f32_reg)
+
+/*
+ * get_f32_reg - Get a FP register value and return it
+ * a0 = FP register index to be retrieved
+ */
+SYM_FUNC_START(get_f32_reg)
+	fp_access_prologue
+	fp_access_body(get_f32)
+	fp_access_epilogue
+SYM_FUNC_END(put_f32_reg)
+
+/*
+ * put_f64_reg - Set a 64 bits FP register from a value or a pointer.
+ * a0 = FP register index to be set
+ * a1 = value/pointer to be loaded in the FP register (when xlen == 32 bits, we
+ * load the value to a pointer).
+ */
+SYM_FUNC_START(put_f64_reg)
+	fp_access_prologue
+	fp_access_body(put_f64)
+	fp_access_epilogue
+SYM_FUNC_END(put_f64_reg)
+
+/*
+ * put_f64_reg - Get a 64 bits FP register value and returned it or store it to
+ *	 	 a pointer.
+ * a0 = FP register index to be retrieved
+ * a1 = If xlen == 32, pointer which should be loaded with the FP register value
+ *	or unused if xlen == 64. In which case the FP register value is returned
+ *	through a0
+ */
+SYM_FUNC_START(get_f64_reg)
+	fp_access_prologue
+	fp_access_body(get_f64)
+	fp_access_epilogue
+SYM_FUNC_END(get_f64_reg)
diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
index 804f6c5e0e44..041fd2dbd955 100644
--- a/arch/riscv/kernel/traps_misaligned.c
+++ b/arch/riscv/kernel/traps_misaligned.c
@@ -153,6 +153,115 @@
 #define PRECISION_S 0
 #define PRECISION_D 1
 
+#ifdef CONFIG_FPU
+
+#define FP_GET_RD(insn)		(insn >> 7 & 0x1F)
+
+extern void put_f32_reg(unsigned long fp_reg, unsigned long value);
+
+static int set_f32_rd(unsigned long insn, struct pt_regs *regs,
+		      unsigned long val)
+{
+	unsigned long fp_reg = FP_GET_RD(insn);
+
+	put_f32_reg(fp_reg, val);
+	regs->status |= SR_FS_DIRTY;
+
+	return 0;
+}
+
+extern void put_f64_reg(unsigned long fp_reg, unsigned long value);
+
+static int set_f64_rd(unsigned long insn, struct pt_regs *regs, u64 val)
+{
+	unsigned long fp_reg = FP_GET_RD(insn);
+	unsigned long value;
+
+#if __riscv_xlen == 32
+	value = (unsigned long) &val;
+#else
+	value = val;
+#endif
+	put_f64_reg(fp_reg, value);
+	regs->status |= SR_FS_DIRTY;
+
+	return 0;
+}
+
+#if __riscv_xlen == 32
+extern void get_f64_reg(unsigned long fp_reg, u64 *value);
+
+static u64 get_f64_rs(unsigned long insn, u8 fp_reg_offset,
+		      struct pt_regs *regs)
+{
+	unsigned long fp_reg = (insn >> fp_reg_offset) & 0x1F;
+	u64 val;
+
+	get_f64_reg(fp_reg, &val);
+	regs->status |= SR_FS_DIRTY;
+
+	return val;
+}
+#else
+
+extern unsigned long get_f64_reg(unsigned long fp_reg);
+
+static unsigned long get_f64_rs(unsigned long insn, u8 fp_reg_offset,
+				struct pt_regs *regs)
+{
+	unsigned long fp_reg = (insn >> fp_reg_offset) & 0x1F;
+	unsigned long val;
+
+	val = get_f64_reg(fp_reg);
+	regs->status |= SR_FS_DIRTY;
+
+	return val;
+}
+
+#endif
+
+extern unsigned long get_f32_reg(unsigned long fp_reg);
+
+static unsigned long get_f32_rs(unsigned long insn, u8 fp_reg_offset,
+				struct pt_regs *regs)
+{
+	unsigned long fp_reg = (insn >> fp_reg_offset) & 0x1F;
+	unsigned long val;
+
+	val = get_f32_reg(fp_reg);
+	regs->status |= SR_FS_DIRTY;
+
+	return val;
+}
+
+#else /* CONFIG_FPU */
+static void set_f32_rd(unsigned long insn, struct pt_regs *regs,
+		       unsigned long val) {}
+
+static void set_f64_rd(unsigned long insn, struct pt_regs *regs, u64 val) {}
+
+static unsigned long get_f64_rs(unsigned long insn, u8 fp_reg_offset,
+				struct pt_regs *regs)
+{
+	return 0;
+}
+
+static unsigned long get_f32_rs(unsigned long insn, u8 fp_reg_offset,
+				struct pt_regs *regs)
+{
+	return 0;
+}
+
+#endif
+
+#define GET_F64_RS2(insn, regs) (get_f64_rs(insn, 20, regs))
+#define GET_F64_RS2C(insn, regs) (get_f64_rs(insn, 2, regs))
+#define GET_F64_RS2S(insn, regs) (get_f64_rs(RVC_RS2S(insn), 0, regs))
+
+#define GET_F32_RS2(insn, regs) (get_f32_rs(insn, 20, regs))
+#define GET_F32_RS2C(insn, regs) (get_f32_rs(insn, 2, regs))
+#define GET_F32_RS2S(insn, regs) (get_f32_rs(RVC_RS2S(insn), 0, regs))
+
 #ifdef CONFIG_RISCV_M_MODE
 static inline int load_u8(struct pt_regs *regs, const u8 *addr, u8 *r_val)
 {
@@ -362,15 +471,21 @@ int handle_misaligned_load(struct pt_regs *regs)
 		return -1;
 	}
 
+	if (!IS_ENABLED(CONFIG_FPU) && fp)
+		return -EOPNOTSUPP;
+
 	val.data_u64 = 0;
 	for (i = 0; i < len; i++) {
 		if (load_u8(regs, (void *)(addr + i), &val.data_bytes[i]))
 			return -1;
 	}
 
-	if (fp)
-		return -1;
-	SET_RD(insn, regs, val.data_ulong << shift >> shift);
+	if (!fp)
+		SET_RD(insn, regs, val.data_ulong << shift >> shift);
+	else if (len == 8)
+		set_f64_rd(insn, regs, val.data_u64);
+	else
+		set_f32_rd(insn, regs, val.data_ulong);
 
 	regs->epc = epc + INSN_LEN(insn);
 
@@ -383,7 +498,7 @@ int handle_misaligned_store(struct pt_regs *regs)
 	unsigned long epc = regs->epc;
 	unsigned long insn;
 	unsigned long addr = regs->badaddr;
-	int i, len = 0;
+	int i, len = 0, fp = 0;
 
 	perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS, 1, regs, addr);
 
@@ -400,6 +515,14 @@ int handle_misaligned_store(struct pt_regs *regs)
 	} else if ((insn & INSN_MASK_SD) == INSN_MATCH_SD) {
 		len = 8;
 #endif
+	} else if ((insn & INSN_MASK_FSD) == INSN_MATCH_FSD) {
+		fp = 1;
+		len = 8;
+		val.data_u64 = GET_F64_RS2(insn, regs);
+	} else if ((insn & INSN_MASK_FSW) == INSN_MATCH_FSW) {
+		fp = 1;
+		len = 4;
+		val.data_ulong = GET_F32_RS2(insn, regs);
 	} else if ((insn & INSN_MASK_SH) == INSN_MATCH_SH) {
 		len = 2;
 #if defined(CONFIG_64BIT)
@@ -418,11 +541,32 @@ int handle_misaligned_store(struct pt_regs *regs)
 		   ((insn >> SH_RD) & 0x1f)) {
 		len = 4;
 		val.data_ulong = GET_RS2C(insn, regs);
+	} else if ((insn & INSN_MASK_C_FSD) == INSN_MATCH_C_FSD) {
+		fp = 1;
+		len = 8;
+		val.data_u64 = GET_F64_RS2S(insn, regs);
+	} else if ((insn & INSN_MASK_C_FSDSP) == INSN_MATCH_C_FSDSP) {
+		fp = 1;
+		len = 8;
+		val.data_u64 = GET_F64_RS2C(insn, regs);
+#if !defined(CONFIG_64BIT)
+	} else if ((insn & INSN_MASK_C_FSW) == INSN_MATCH_C_FSW) {
+		fp = 1;
+		len = 4;
+		val.data_ulong = GET_F32_RS2S(insn, regs);
+	} else if ((insn & INSN_MASK_C_FSWSP) == INSN_MATCH_C_FSWSP) {
+		fp = 1;
+		len = 4;
+		val.data_ulong = GET_F32_RS2C(insn, regs);
+#endif
 	} else {
 		regs->epc = epc;
 		return -1;
 	}
 
+	if (!IS_ENABLED(CONFIG_FPU) && fp)
+		return -EOPNOTSUPP;
+
 	for (i = 0; i < len; i++) {
 		if (store_u8(regs, (void *)(addr + i), val.data_bytes[i]))
 			return -1;
-- 
2.40.1


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

[PATCH 5/7] riscv: add support for sysctl unaligned_enabled control

[Clément Léger]

This sysctl tuning option allows the user to disable misaligned access
handling globally on the system. This will also be used by misaligned
detection code to temporarily disable misaligned access handling.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/Kconfig                   | 1 +
 arch/riscv/kernel/traps_misaligned.c | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d607ab0f7c6d..3515510fe418 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -157,6 +157,7 @@ config RISCV
 	select RISCV_TIMER if RISCV_SBI
 	select SIFIVE_PLIC
 	select SPARSE_IRQ
+	select SYSCTL_ARCH_UNALIGN_ALLOW
 	select SYSCTL_EXCEPTION_TRACE
 	select THREAD_INFO_IN_TASK
 	select TRACE_IRQFLAGS_SUPPORT
diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
index 041fd2dbd955..b5fb1ff078e3 100644
--- a/arch/riscv/kernel/traps_misaligned.c
+++ b/arch/riscv/kernel/traps_misaligned.c
@@ -396,6 +396,9 @@ union reg_data {
 	u64 data_u64;
 };
 
+/* sysctl hooks */
+int unaligned_enabled __read_mostly = 1;	/* Enabled by default */
+
 int handle_misaligned_load(struct pt_regs *regs)
 {
 	union reg_data val;
@@ -406,6 +409,9 @@ int handle_misaligned_load(struct pt_regs *regs)
 
 	perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS, 1, regs, addr);
 
+	if (!unaligned_enabled)
+		return -1;
+
 	if (get_insn(regs, epc, &insn))
 		return -1;
 
@@ -502,6 +508,9 @@ int handle_misaligned_store(struct pt_regs *regs)
 
 	perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS, 1, regs, addr);
 
+	if (!unaligned_enabled)
+		return -1;
+
 	if (get_insn(regs, epc, &insn))
 		return -1;
 
-- 
2.40.1


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

[PATCH 6/7] riscv: report misaligned accesses emulation to hwprobe

[Clément Léger]

hwprobe provides a way to report if misaligned access are emulated. In
order to correctly populate that feature, we can check if it actually
traps when doing a misaligned access. This can be checked using an
exception table entry which will actually be used when a misaligned
access is done from kernel mode.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/include/asm/cpufeature.h  |  6 +++
 arch/riscv/kernel/cpufeature.c       |  6 ++-
 arch/riscv/kernel/setup.c            |  1 +
 arch/riscv/kernel/traps_misaligned.c | 63 +++++++++++++++++++++++++++-
 4 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
index d0345bd659c9..c1f0ef02cd7d 100644
--- a/arch/riscv/include/asm/cpufeature.h
+++ b/arch/riscv/include/asm/cpufeature.h
@@ -8,6 +8,7 @@
 
 #include <linux/bitmap.h>
 #include <asm/hwcap.h>
+#include <asm/hwprobe.h>
 
 /*
  * These are probed via a device_initcall(), via either the SBI or directly
@@ -32,4 +33,9 @@ extern struct riscv_isainfo hart_isa[NR_CPUS];
 
 void check_unaligned_access(int cpu);
 
+bool unaligned_ctl_available(void);
+
+bool check_unaligned_access_emulated(int cpu);
+void unaligned_emulation_finish(void);
+
 #endif
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 1cfbba65d11a..fbbde800bc21 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -568,6 +568,9 @@ void check_unaligned_access(int cpu)
 	void *src;
 	long speed = RISCV_HWPROBE_MISALIGNED_SLOW;
 
+	if (check_unaligned_access_emulated(cpu))
+		return;
+
 	page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));
 	if (!page) {
 		pr_warn("Can't alloc pages to measure memcpy performance");
@@ -645,9 +648,10 @@ void check_unaligned_access(int cpu)
 	__free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
 }
 
-static int check_unaligned_access_boot_cpu(void)
+static int __init check_unaligned_access_boot_cpu(void)
 {
 	check_unaligned_access(0);
+	unaligned_emulation_finish();
 	return 0;
 }
 
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index e600aab116a4..3af6ad4df7cf 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -26,6 +26,7 @@
 #include <asm/acpi.h>
 #include <asm/alternative.h>
 #include <asm/cacheflush.h>
+#include <asm/cpufeature.h>
 #include <asm/cpu_ops.h>
 #include <asm/early_ioremap.h>
 #include <asm/pgtable.h>
diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
index b5fb1ff078e3..fa81f6952fa4 100644
--- a/arch/riscv/kernel/traps_misaligned.c
+++ b/arch/riscv/kernel/traps_misaligned.c
@@ -9,11 +9,14 @@
 #include <linux/perf_event.h>
 #include <linux/irq.h>
 #include <linux/stringify.h>
+#include <linux/prctl.h>
 
 #include <asm/processor.h>
 #include <asm/ptrace.h>
 #include <asm/csr.h>
 #include <asm/entry-common.h>
+#include <asm/hwprobe.h>
+#include <asm/cpufeature.h>
 
 #define INSN_MATCH_LB			0x3
 #define INSN_MASK_LB			0x707f
@@ -396,8 +399,10 @@ union reg_data {
 	u64 data_u64;
 };
 
+static bool unaligned_ctl __read_mostly;
+
 /* sysctl hooks */
-int unaligned_enabled __read_mostly = 1;	/* Enabled by default */
+int unaligned_enabled __read_mostly;
 
 int handle_misaligned_load(struct pt_regs *regs)
 {
@@ -412,6 +417,9 @@ int handle_misaligned_load(struct pt_regs *regs)
 	if (!unaligned_enabled)
 		return -1;
 
+	if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
+		return -1;
+
 	if (get_insn(regs, epc, &insn))
 		return -1;
 
@@ -511,6 +519,9 @@ int handle_misaligned_store(struct pt_regs *regs)
 	if (!unaligned_enabled)
 		return -1;
 
+	if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
+		return -1;
+
 	if (get_insn(regs, epc, &insn))
 		return -1;
 
@@ -585,3 +596,53 @@ int handle_misaligned_store(struct pt_regs *regs)
 
 	return 0;
 }
+
+bool check_unaligned_access_emulated(int cpu)
+{
+	unsigned long emulated = 1, tmp_var;
+
+	/* Use a fixup to detect if misaligned access triggered an exception */
+	__asm__ __volatile__ (
+		"1:\n"
+		"	"REG_L" %[tmp], 1(%[ptr])\n"
+		"	li %[emulated], 0\n"
+		"2:\n"
+		_ASM_EXTABLE(1b, 2b)
+	: [emulated] "+r" (emulated), [tmp] "=r" (tmp_var)
+	: [ptr] "r" (&tmp_var)
+	: "memory");
+
+	if (!emulated)
+		return false;
+
+	per_cpu(misaligned_access_speed, cpu) =
+		RISCV_HWPROBE_MISALIGNED_EMULATED;
+
+	return true;
+}
+
+void __init unaligned_emulation_finish(void)
+{
+	int cpu;
+
+	/*
+	 * We can only support PR_UNALIGN controls if all CPUs have misaligned
+	 * accesses emulated since tasks requesting such control can run on any
+	 * CPU.
+	 */
+	for_each_possible_cpu(cpu) {
+		if (per_cpu(misaligned_access_speed, cpu) !=
+		    RISCV_HWPROBE_MISALIGNED_EMULATED) {
+			goto out;
+		}
+	}
+	unaligned_ctl = true;
+
+out:
+	unaligned_enabled = 1;
+}
+
+bool unaligned_ctl_available(void)
+{
+	return unaligned_ctl;
+}
-- 
2.40.1


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

Re: [PATCH 6/7] riscv: report misaligned accesses emulation to hwprobe

[Evan Green]

On Tue, Sep 26, 2023 at 8:03 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> hwprobe provides a way to report if misaligned access are emulated. In
> order to correctly populate that feature, we can check if it actually
> traps when doing a misaligned access. This can be checked using an
> exception table entry which will actually be used when a misaligned
> access is done from kernel mode.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  arch/riscv/include/asm/cpufeature.h  |  6 +++
>  arch/riscv/kernel/cpufeature.c       |  6 ++-
>  arch/riscv/kernel/setup.c            |  1 +
>  arch/riscv/kernel/traps_misaligned.c | 63 +++++++++++++++++++++++++++-
>  4 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index d0345bd659c9..c1f0ef02cd7d 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -8,6 +8,7 @@
>
>  #include <linux/bitmap.h>
>  #include <asm/hwcap.h>
> +#include <asm/hwprobe.h>
>
>  /*
>   * These are probed via a device_initcall(), via either the SBI or directly
> @@ -32,4 +33,9 @@ extern struct riscv_isainfo hart_isa[NR_CPUS];
>
>  void check_unaligned_access(int cpu);
>
> +bool unaligned_ctl_available(void);
> +
> +bool check_unaligned_access_emulated(int cpu);
> +void unaligned_emulation_finish(void);
> +
>  #endif
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 1cfbba65d11a..fbbde800bc21 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -568,6 +568,9 @@ void check_unaligned_access(int cpu)
>         void *src;
>         long speed = RISCV_HWPROBE_MISALIGNED_SLOW;
>
> +       if (check_unaligned_access_emulated(cpu))

This spot (referenced below).

> +               return;
> +
>         page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));
>         if (!page) {
>                 pr_warn("Can't alloc pages to measure memcpy performance");
> @@ -645,9 +648,10 @@ void check_unaligned_access(int cpu)
>         __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
>  }
>
> -static int check_unaligned_access_boot_cpu(void)
> +static int __init check_unaligned_access_boot_cpu(void)
>  {
>         check_unaligned_access(0);
> +       unaligned_emulation_finish();
>         return 0;
>  }
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index e600aab116a4..3af6ad4df7cf 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -26,6 +26,7 @@
>  #include <asm/acpi.h>
>  #include <asm/alternative.h>
>  #include <asm/cacheflush.h>
> +#include <asm/cpufeature.h>
>  #include <asm/cpu_ops.h>
>  #include <asm/early_ioremap.h>
>  #include <asm/pgtable.h>
> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index b5fb1ff078e3..fa81f6952fa4 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -9,11 +9,14 @@
>  #include <linux/perf_event.h>
>  #include <linux/irq.h>
>  #include <linux/stringify.h>
> +#include <linux/prctl.h>
>
>  #include <asm/processor.h>
>  #include <asm/ptrace.h>
>  #include <asm/csr.h>
>  #include <asm/entry-common.h>
> +#include <asm/hwprobe.h>
> +#include <asm/cpufeature.h>
>
>  #define INSN_MATCH_LB                  0x3
>  #define INSN_MASK_LB                   0x707f
> @@ -396,8 +399,10 @@ union reg_data {
>         u64 data_u64;
>  };
>
> +static bool unaligned_ctl __read_mostly;
> +
>  /* sysctl hooks */
> -int unaligned_enabled __read_mostly = 1;       /* Enabled by default */
> +int unaligned_enabled __read_mostly;
>
>  int handle_misaligned_load(struct pt_regs *regs)
>  {
> @@ -412,6 +417,9 @@ int handle_misaligned_load(struct pt_regs *regs)
>         if (!unaligned_enabled)
>                 return -1;
>
> +       if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
> +               return -1;
> +
>         if (get_insn(regs, epc, &insn))
>                 return -1;
>
> @@ -511,6 +519,9 @@ int handle_misaligned_store(struct pt_regs *regs)
>         if (!unaligned_enabled)
>                 return -1;
>
> +       if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
> +               return -1;
> +
>         if (get_insn(regs, epc, &insn))
>                 return -1;
>
> @@ -585,3 +596,53 @@ int handle_misaligned_store(struct pt_regs *regs)
>
>         return 0;
>  }
> +
> +bool check_unaligned_access_emulated(int cpu)
> +{
> +       unsigned long emulated = 1, tmp_var;
> +
> +       /* Use a fixup to detect if misaligned access triggered an exception */
> +       __asm__ __volatile__ (
> +               "1:\n"
> +               "       "REG_L" %[tmp], 1(%[ptr])\n"
> +               "       li %[emulated], 0\n"
> +               "2:\n"
> +               _ASM_EXTABLE(1b, 2b)
> +       : [emulated] "+r" (emulated), [tmp] "=r" (tmp_var)
> +       : [ptr] "r" (&tmp_var)
> +       : "memory");
> +
> +       if (!emulated)
> +               return false;
> +
> +       per_cpu(misaligned_access_speed, cpu) =
> +               RISCV_HWPROBE_MISALIGNED_EMULATED;

For tidiness, can we move the assignment of this per-cpu variable into
check_unaligned_access(), at the spot I referenced above. That way
people looking to see how this variable is set don't have to hunt
through multiple locations.

> +
> +       return true;
> +}
> +
> +void __init unaligned_emulation_finish(void)
> +{
> +       int cpu;
> +
> +       /*
> +        * We can only support PR_UNALIGN controls if all CPUs have misaligned
> +        * accesses emulated since tasks requesting such control can run on any
> +        * CPU.
> +        */
> +       for_each_possible_cpu(cpu) {
> +               if (per_cpu(misaligned_access_speed, cpu) !=
> +                   RISCV_HWPROBE_MISALIGNED_EMULATED) {
> +                       goto out;
> +               }
> +       }
> +       unaligned_ctl = true;

This doesn't handle the case where a CPU is hotplugged later that
doesn't match with the others. You may want to add a patch that fails
the onlining of that new CPU if unaligned_ctl is true and
new_cpu.misaligned_access_speed != EMULATED.

-Evan

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

Re: [PATCH 6/7] riscv: report misaligned accesses emulation to hwprobe

[Clément Léger]

On 26/09/2023 23:57, Evan Green wrote:
> On Tue, Sep 26, 2023 at 8:03 AM Clément Léger <cleger@rivosinc.com> wrote:
>>
>> hwprobe provides a way to report if misaligned access are emulated. In
>> order to correctly populate that feature, we can check if it actually
>> traps when doing a misaligned access. This can be checked using an
>> exception table entry which will actually be used when a misaligned
>> access is done from kernel mode.
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  arch/riscv/include/asm/cpufeature.h  |  6 +++
>>  arch/riscv/kernel/cpufeature.c       |  6 ++-
>>  arch/riscv/kernel/setup.c            |  1 +
>>  arch/riscv/kernel/traps_misaligned.c | 63 +++++++++++++++++++++++++++-
>>  4 files changed, 74 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>> index d0345bd659c9..c1f0ef02cd7d 100644
>> --- a/arch/riscv/include/asm/cpufeature.h
>> +++ b/arch/riscv/include/asm/cpufeature.h
>> @@ -8,6 +8,7 @@
>>
>>  #include <linux/bitmap.h>
>>  #include <asm/hwcap.h>
>> +#include <asm/hwprobe.h>
>>
>>  /*
>>   * These are probed via a device_initcall(), via either the SBI or directly
>> @@ -32,4 +33,9 @@ extern struct riscv_isainfo hart_isa[NR_CPUS];
>>
>>  void check_unaligned_access(int cpu);
>>
>> +bool unaligned_ctl_available(void);
>> +
>> +bool check_unaligned_access_emulated(int cpu);
>> +void unaligned_emulation_finish(void);
>> +
>>  #endif
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 1cfbba65d11a..fbbde800bc21 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -568,6 +568,9 @@ void check_unaligned_access(int cpu)
>>         void *src;
>>         long speed = RISCV_HWPROBE_MISALIGNED_SLOW;
>>
>> +       if (check_unaligned_access_emulated(cpu))
> 
> This spot (referenced below).
> 
>> +               return;
>> +
>>         page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));
>>         if (!page) {
>>                 pr_warn("Can't alloc pages to measure memcpy performance");
>> @@ -645,9 +648,10 @@ void check_unaligned_access(int cpu)
>>         __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
>>  }
>>
>> -static int check_unaligned_access_boot_cpu(void)
>> +static int __init check_unaligned_access_boot_cpu(void)
>>  {
>>         check_unaligned_access(0);
>> +       unaligned_emulation_finish();
>>         return 0;
>>  }
>>
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index e600aab116a4..3af6ad4df7cf 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -26,6 +26,7 @@
>>  #include <asm/acpi.h>
>>  #include <asm/alternative.h>
>>  #include <asm/cacheflush.h>
>> +#include <asm/cpufeature.h>
>>  #include <asm/cpu_ops.h>
>>  #include <asm/early_ioremap.h>
>>  #include <asm/pgtable.h>
>> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
>> index b5fb1ff078e3..fa81f6952fa4 100644
>> --- a/arch/riscv/kernel/traps_misaligned.c
>> +++ b/arch/riscv/kernel/traps_misaligned.c
>> @@ -9,11 +9,14 @@
>>  #include <linux/perf_event.h>
>>  #include <linux/irq.h>
>>  #include <linux/stringify.h>
>> +#include <linux/prctl.h>
>>
>>  #include <asm/processor.h>
>>  #include <asm/ptrace.h>
>>  #include <asm/csr.h>
>>  #include <asm/entry-common.h>
>> +#include <asm/hwprobe.h>
>> +#include <asm/cpufeature.h>
>>
>>  #define INSN_MATCH_LB                  0x3
>>  #define INSN_MASK_LB                   0x707f
>> @@ -396,8 +399,10 @@ union reg_data {
>>         u64 data_u64;
>>  };
>>
>> +static bool unaligned_ctl __read_mostly;
>> +
>>  /* sysctl hooks */
>> -int unaligned_enabled __read_mostly = 1;       /* Enabled by default */
>> +int unaligned_enabled __read_mostly;
>>
>>  int handle_misaligned_load(struct pt_regs *regs)
>>  {
>> @@ -412,6 +417,9 @@ int handle_misaligned_load(struct pt_regs *regs)
>>         if (!unaligned_enabled)
>>                 return -1;
>>
>> +       if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
>> +               return -1;
>> +
>>         if (get_insn(regs, epc, &insn))
>>                 return -1;
>>
>> @@ -511,6 +519,9 @@ int handle_misaligned_store(struct pt_regs *regs)
>>         if (!unaligned_enabled)
>>                 return -1;
>>
>> +       if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
>> +               return -1;
>> +
>>         if (get_insn(regs, epc, &insn))
>>                 return -1;
>>
>> @@ -585,3 +596,53 @@ int handle_misaligned_store(struct pt_regs *regs)
>>
>>         return 0;
>>  }
>> +
>> +bool check_unaligned_access_emulated(int cpu)
>> +{
>> +       unsigned long emulated = 1, tmp_var;
>> +
>> +       /* Use a fixup to detect if misaligned access triggered an exception */
>> +       __asm__ __volatile__ (
>> +               "1:\n"
>> +               "       "REG_L" %[tmp], 1(%[ptr])\n"
>> +               "       li %[emulated], 0\n"
>> +               "2:\n"
>> +               _ASM_EXTABLE(1b, 2b)
>> +       : [emulated] "+r" (emulated), [tmp] "=r" (tmp_var)
>> +       : [ptr] "r" (&tmp_var)
>> +       : "memory");
>> +
>> +       if (!emulated)
>> +               return false;
>> +
>> +       per_cpu(misaligned_access_speed, cpu) =
>> +               RISCV_HWPROBE_MISALIGNED_EMULATED;
> 
> For tidiness, can we move the assignment of this per-cpu variable into
> check_unaligned_access(), at the spot I referenced above. That way
> people looking to see how this variable is set don't have to hunt
> through multiple locations.

Agreed, that seems better.

> 
>> +
>> +       return true;
>> +}
>> +
>> +void __init unaligned_emulation_finish(void)
>> +{
>> +       int cpu;
>> +
>> +       /*
>> +        * We can only support PR_UNALIGN controls if all CPUs have misaligned
>> +        * accesses emulated since tasks requesting such control can run on any
>> +        * CPU.
>> +        */
>> +       for_each_possible_cpu(cpu) {
>> +               if (per_cpu(misaligned_access_speed, cpu) !=
>> +                   RISCV_HWPROBE_MISALIGNED_EMULATED) {
>> +                       goto out;
>> +               }
>> +       }
>> +       unaligned_ctl = true;
> 
> This doesn't handle the case where a CPU is hotplugged later that
> doesn't match with the others. You may want to add a patch that fails
> the onlining of that new CPU if unaligned_ctl is true and
> new_cpu.misaligned_access_speed != EMULATED.

So actually, this will require a bit more plumbing as I realize the
switch to disable misalignment support is global. This switch should
only be disabled at boot which means I won't be able to disable it at
runtime (while hiotplugging a CPU) for CPU detection. There are multiple
ways to handle that:

1- Have a per-cpu switch for misalignment handling which would be
disabled only when detection is needed.

2- Assume that once detected at boot-time, emulation will not change.

Not sure which one is better though. Advice are welcomed.

Clément

> 
> -Evan

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

Re: [PATCH 6/7] riscv: report misaligned accesses emulation to hwprobe

[Evan Green]

On Thu, Sep 28, 2023 at 12:46 AM Clément Léger <cleger@rivosinc.com> wrote:
>
>
>
> On 26/09/2023 23:57, Evan Green wrote:
> > On Tue, Sep 26, 2023 at 8:03 AM Clément Léger <cleger@rivosinc.com> wrote:
> >>
> >> hwprobe provides a way to report if misaligned access are emulated. In
> >> order to correctly populate that feature, we can check if it actually
> >> traps when doing a misaligned access. This can be checked using an
> >> exception table entry which will actually be used when a misaligned
> >> access is done from kernel mode.
> >>
> >> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> >> ---
> >>  arch/riscv/include/asm/cpufeature.h  |  6 +++
> >>  arch/riscv/kernel/cpufeature.c       |  6 ++-
> >>  arch/riscv/kernel/setup.c            |  1 +
> >>  arch/riscv/kernel/traps_misaligned.c | 63 +++++++++++++++++++++++++++-
> >>  4 files changed, 74 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> >> index d0345bd659c9..c1f0ef02cd7d 100644
> >> --- a/arch/riscv/include/asm/cpufeature.h
> >> +++ b/arch/riscv/include/asm/cpufeature.h
> >> @@ -8,6 +8,7 @@
> >>
> >>  #include <linux/bitmap.h>
> >>  #include <asm/hwcap.h>
> >> +#include <asm/hwprobe.h>
> >>
> >>  /*
> >>   * These are probed via a device_initcall(), via either the SBI or directly
> >> @@ -32,4 +33,9 @@ extern struct riscv_isainfo hart_isa[NR_CPUS];
> >>
> >>  void check_unaligned_access(int cpu);
> >>
> >> +bool unaligned_ctl_available(void);
> >> +
> >> +bool check_unaligned_access_emulated(int cpu);
> >> +void unaligned_emulation_finish(void);
> >> +
> >>  #endif
> >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> >> index 1cfbba65d11a..fbbde800bc21 100644
> >> --- a/arch/riscv/kernel/cpufeature.c
> >> +++ b/arch/riscv/kernel/cpufeature.c
> >> @@ -568,6 +568,9 @@ void check_unaligned_access(int cpu)
> >>         void *src;
> >>         long speed = RISCV_HWPROBE_MISALIGNED_SLOW;
> >>
> >> +       if (check_unaligned_access_emulated(cpu))
> >
> > This spot (referenced below).
> >
> >> +               return;
> >> +
> >>         page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));
> >>         if (!page) {
> >>                 pr_warn("Can't alloc pages to measure memcpy performance");
> >> @@ -645,9 +648,10 @@ void check_unaligned_access(int cpu)
> >>         __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
> >>  }
> >>
> >> -static int check_unaligned_access_boot_cpu(void)
> >> +static int __init check_unaligned_access_boot_cpu(void)
> >>  {
> >>         check_unaligned_access(0);
> >> +       unaligned_emulation_finish();
> >>         return 0;
> >>  }
> >>
> >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >> index e600aab116a4..3af6ad4df7cf 100644
> >> --- a/arch/riscv/kernel/setup.c
> >> +++ b/arch/riscv/kernel/setup.c
> >> @@ -26,6 +26,7 @@
> >>  #include <asm/acpi.h>
> >>  #include <asm/alternative.h>
> >>  #include <asm/cacheflush.h>
> >> +#include <asm/cpufeature.h>
> >>  #include <asm/cpu_ops.h>
> >>  #include <asm/early_ioremap.h>
> >>  #include <asm/pgtable.h>
> >> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> >> index b5fb1ff078e3..fa81f6952fa4 100644
> >> --- a/arch/riscv/kernel/traps_misaligned.c
> >> +++ b/arch/riscv/kernel/traps_misaligned.c
> >> @@ -9,11 +9,14 @@
> >>  #include <linux/perf_event.h>
> >>  #include <linux/irq.h>
> >>  #include <linux/stringify.h>
> >> +#include <linux/prctl.h>
> >>
> >>  #include <asm/processor.h>
> >>  #include <asm/ptrace.h>
> >>  #include <asm/csr.h>
> >>  #include <asm/entry-common.h>
> >> +#include <asm/hwprobe.h>
> >> +#include <asm/cpufeature.h>
> >>
> >>  #define INSN_MATCH_LB                  0x3
> >>  #define INSN_MASK_LB                   0x707f
> >> @@ -396,8 +399,10 @@ union reg_data {
> >>         u64 data_u64;
> >>  };
> >>
> >> +static bool unaligned_ctl __read_mostly;
> >> +
> >>  /* sysctl hooks */
> >> -int unaligned_enabled __read_mostly = 1;       /* Enabled by default */
> >> +int unaligned_enabled __read_mostly;
> >>
> >>  int handle_misaligned_load(struct pt_regs *regs)
> >>  {
> >> @@ -412,6 +417,9 @@ int handle_misaligned_load(struct pt_regs *regs)
> >>         if (!unaligned_enabled)
> >>                 return -1;
> >>
> >> +       if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
> >> +               return -1;
> >> +
> >>         if (get_insn(regs, epc, &insn))
> >>                 return -1;
> >>
> >> @@ -511,6 +519,9 @@ int handle_misaligned_store(struct pt_regs *regs)
> >>         if (!unaligned_enabled)
> >>                 return -1;
> >>
> >> +       if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
> >> +               return -1;
> >> +
> >>         if (get_insn(regs, epc, &insn))
> >>                 return -1;
> >>
> >> @@ -585,3 +596,53 @@ int handle_misaligned_store(struct pt_regs *regs)
> >>
> >>         return 0;
> >>  }
> >> +
> >> +bool check_unaligned_access_emulated(int cpu)
> >> +{
> >> +       unsigned long emulated = 1, tmp_var;
> >> +
> >> +       /* Use a fixup to detect if misaligned access triggered an exception */
> >> +       __asm__ __volatile__ (
> >> +               "1:\n"
> >> +               "       "REG_L" %[tmp], 1(%[ptr])\n"
> >> +               "       li %[emulated], 0\n"
> >> +               "2:\n"
> >> +               _ASM_EXTABLE(1b, 2b)
> >> +       : [emulated] "+r" (emulated), [tmp] "=r" (tmp_var)
> >> +       : [ptr] "r" (&tmp_var)
> >> +       : "memory");
> >> +
> >> +       if (!emulated)
> >> +               return false;
> >> +
> >> +       per_cpu(misaligned_access_speed, cpu) =
> >> +               RISCV_HWPROBE_MISALIGNED_EMULATED;
> >
> > For tidiness, can we move the assignment of this per-cpu variable into
> > check_unaligned_access(), at the spot I referenced above. That way
> > people looking to see how this variable is set don't have to hunt
> > through multiple locations.
>
> Agreed, that seems better.
>
> >
> >> +
> >> +       return true;
> >> +}
> >> +
> >> +void __init unaligned_emulation_finish(void)
> >> +{
> >> +       int cpu;
> >> +
> >> +       /*
> >> +        * We can only support PR_UNALIGN controls if all CPUs have misaligned
> >> +        * accesses emulated since tasks requesting such control can run on any
> >> +        * CPU.
> >> +        */
> >> +       for_each_possible_cpu(cpu) {
> >> +               if (per_cpu(misaligned_access_speed, cpu) !=
> >> +                   RISCV_HWPROBE_MISALIGNED_EMULATED) {
> >> +                       goto out;
> >> +               }
> >> +       }
> >> +       unaligned_ctl = true;
> >
> > This doesn't handle the case where a CPU is hotplugged later that
> > doesn't match with the others. You may want to add a patch that fails
> > the onlining of that new CPU if unaligned_ctl is true and
> > new_cpu.misaligned_access_speed != EMULATED.
>
> So actually, this will require a bit more plumbing as I realize the
> switch to disable misalignment support is global. This switch should
> only be disabled at boot which means I won't be able to disable it at
> runtime (while hiotplugging a CPU) for CPU detection. There are multiple
> ways to handle that:
>
> 1- Have a per-cpu switch for misalignment handling which would be
> disabled only when detection is needed.
>
> 2- Assume that once detected at boot-time, emulation will not change.
>
> Not sure which one is better though. Advice are welcomed.

If I gaze into my own crystal ball, my hope is that the Venn diagram
of "systems that support hotplug" and "systems that still use software
assist for misaligned access" is just two circles not touching. If
people agree with that, then the safe thing to do is enforce it, by
failing to online new hotplugged CPUs that don't conform to
misaligned_access_speed == EMULATED if unaligned_ctl is true. We would
sacrifice some future flexibility by making this choice now though, so
it requires buy-in for this particular crystal ball vision.

-Evan

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

Re: [PATCH 6/7] riscv: report misaligned accesses emulation to hwprobe

[Atish Kumar Patra]

On Thu, Sep 28, 2023 at 9:52 AM Evan Green <evan@rivosinc.com> wrote:
>
> On Thu, Sep 28, 2023 at 12:46 AM Clément Léger <cleger@rivosinc.com> wrote:
> >
> >
> >
> > On 26/09/2023 23:57, Evan Green wrote:
> > > On Tue, Sep 26, 2023 at 8:03 AM Clément Léger <cleger@rivosinc.com> wrote:
> > >>
> > >> hwprobe provides a way to report if misaligned access are emulated. In
> > >> order to correctly populate that feature, we can check if it actually
> > >> traps when doing a misaligned access. This can be checked using an
> > >> exception table entry which will actually be used when a misaligned
> > >> access is done from kernel mode.
> > >>
> > >> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> > >> ---
> > >>  arch/riscv/include/asm/cpufeature.h  |  6 +++
> > >>  arch/riscv/kernel/cpufeature.c       |  6 ++-
> > >>  arch/riscv/kernel/setup.c            |  1 +
> > >>  arch/riscv/kernel/traps_misaligned.c | 63 +++++++++++++++++++++++++++-
> > >>  4 files changed, 74 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> > >> index d0345bd659c9..c1f0ef02cd7d 100644
> > >> --- a/arch/riscv/include/asm/cpufeature.h
> > >> +++ b/arch/riscv/include/asm/cpufeature.h
> > >> @@ -8,6 +8,7 @@
> > >>
> > >>  #include <linux/bitmap.h>
> > >>  #include <asm/hwcap.h>
> > >> +#include <asm/hwprobe.h>
> > >>
> > >>  /*
> > >>   * These are probed via a device_initcall(), via either the SBI or directly
> > >> @@ -32,4 +33,9 @@ extern struct riscv_isainfo hart_isa[NR_CPUS];
> > >>
> > >>  void check_unaligned_access(int cpu);
> > >>
> > >> +bool unaligned_ctl_available(void);
> > >> +
> > >> +bool check_unaligned_access_emulated(int cpu);
> > >> +void unaligned_emulation_finish(void);
> > >> +
> > >>  #endif
> > >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > >> index 1cfbba65d11a..fbbde800bc21 100644
> > >> --- a/arch/riscv/kernel/cpufeature.c
> > >> +++ b/arch/riscv/kernel/cpufeature.c
> > >> @@ -568,6 +568,9 @@ void check_unaligned_access(int cpu)
> > >>         void *src;
> > >>         long speed = RISCV_HWPROBE_MISALIGNED_SLOW;
> > >>
> > >> +       if (check_unaligned_access_emulated(cpu))
> > >
> > > This spot (referenced below).
> > >
> > >> +               return;
> > >> +
> > >>         page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));
> > >>         if (!page) {
> > >>                 pr_warn("Can't alloc pages to measure memcpy performance");
> > >> @@ -645,9 +648,10 @@ void check_unaligned_access(int cpu)
> > >>         __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
> > >>  }
> > >>
> > >> -static int check_unaligned_access_boot_cpu(void)
> > >> +static int __init check_unaligned_access_boot_cpu(void)
> > >>  {
> > >>         check_unaligned_access(0);
> > >> +       unaligned_emulation_finish();
> > >>         return 0;
> > >>  }
> > >>
> > >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > >> index e600aab116a4..3af6ad4df7cf 100644
> > >> --- a/arch/riscv/kernel/setup.c
> > >> +++ b/arch/riscv/kernel/setup.c
> > >> @@ -26,6 +26,7 @@
> > >>  #include <asm/acpi.h>
> > >>  #include <asm/alternative.h>
> > >>  #include <asm/cacheflush.h>
> > >> +#include <asm/cpufeature.h>
> > >>  #include <asm/cpu_ops.h>
> > >>  #include <asm/early_ioremap.h>
> > >>  #include <asm/pgtable.h>
> > >> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> > >> index b5fb1ff078e3..fa81f6952fa4 100644
> > >> --- a/arch/riscv/kernel/traps_misaligned.c
> > >> +++ b/arch/riscv/kernel/traps_misaligned.c
> > >> @@ -9,11 +9,14 @@
> > >>  #include <linux/perf_event.h>
> > >>  #include <linux/irq.h>
> > >>  #include <linux/stringify.h>
> > >> +#include <linux/prctl.h>
> > >>
> > >>  #include <asm/processor.h>
> > >>  #include <asm/ptrace.h>
> > >>  #include <asm/csr.h>
> > >>  #include <asm/entry-common.h>
> > >> +#include <asm/hwprobe.h>
> > >> +#include <asm/cpufeature.h>
> > >>
> > >>  #define INSN_MATCH_LB                  0x3
> > >>  #define INSN_MASK_LB                   0x707f
> > >> @@ -396,8 +399,10 @@ union reg_data {
> > >>         u64 data_u64;
> > >>  };
> > >>
> > >> +static bool unaligned_ctl __read_mostly;
> > >> +
> > >>  /* sysctl hooks */
> > >> -int unaligned_enabled __read_mostly = 1;       /* Enabled by default */
> > >> +int unaligned_enabled __read_mostly;
> > >>
> > >>  int handle_misaligned_load(struct pt_regs *regs)
> > >>  {
> > >> @@ -412,6 +417,9 @@ int handle_misaligned_load(struct pt_regs *regs)
> > >>         if (!unaligned_enabled)
> > >>                 return -1;
> > >>
> > >> +       if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
> > >> +               return -1;
> > >> +
> > >>         if (get_insn(regs, epc, &insn))
> > >>                 return -1;
> > >>
> > >> @@ -511,6 +519,9 @@ int handle_misaligned_store(struct pt_regs *regs)
> > >>         if (!unaligned_enabled)
> > >>                 return -1;
> > >>
> > >> +       if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
> > >> +               return -1;
> > >> +
> > >>         if (get_insn(regs, epc, &insn))
> > >>                 return -1;
> > >>
> > >> @@ -585,3 +596,53 @@ int handle_misaligned_store(struct pt_regs *regs)
> > >>
> > >>         return 0;
> > >>  }
> > >> +
> > >> +bool check_unaligned_access_emulated(int cpu)
> > >> +{
> > >> +       unsigned long emulated = 1, tmp_var;
> > >> +
> > >> +       /* Use a fixup to detect if misaligned access triggered an exception */
> > >> +       __asm__ __volatile__ (
> > >> +               "1:\n"
> > >> +               "       "REG_L" %[tmp], 1(%[ptr])\n"
> > >> +               "       li %[emulated], 0\n"
> > >> +               "2:\n"
> > >> +               _ASM_EXTABLE(1b, 2b)
> > >> +       : [emulated] "+r" (emulated), [tmp] "=r" (tmp_var)
> > >> +       : [ptr] "r" (&tmp_var)
> > >> +       : "memory");
> > >> +
> > >> +       if (!emulated)
> > >> +               return false;
> > >> +
> > >> +       per_cpu(misaligned_access_speed, cpu) =
> > >> +               RISCV_HWPROBE_MISALIGNED_EMULATED;
> > >
> > > For tidiness, can we move the assignment of this per-cpu variable into
> > > check_unaligned_access(), at the spot I referenced above. That way
> > > people looking to see how this variable is set don't have to hunt
> > > through multiple locations.
> >
> > Agreed, that seems better.
> >
> > >
> > >> +
> > >> +       return true;
> > >> +}
> > >> +
> > >> +void __init unaligned_emulation_finish(void)
> > >> +{
> > >> +       int cpu;
> > >> +
> > >> +       /*
> > >> +        * We can only support PR_UNALIGN controls if all CPUs have misaligned
> > >> +        * accesses emulated since tasks requesting such control can run on any
> > >> +        * CPU.
> > >> +        */
> > >> +       for_each_possible_cpu(cpu) {
> > >> +               if (per_cpu(misaligned_access_speed, cpu) !=
> > >> +                   RISCV_HWPROBE_MISALIGNED_EMULATED) {
> > >> +                       goto out;
> > >> +               }
> > >> +       }
> > >> +       unaligned_ctl = true;
> > >

Note: You probably want to loop through the present cpu mask instead
of possible cpus.
Possible cpus list will have all the cpus listed in DT/ACPI. However,
all of them may not come up during the boot.
Hardware errata, different kernel configuration, incorrect DT are few
examples where possible may not match present cpumask.

> > > This doesn't handle the case where a CPU is hotplugged later that
> > > doesn't match with the others. You may want to add a patch that fails
> > > the onlining of that new CPU if unaligned_ctl is true and
> > > new_cpu.misaligned_access_speed != EMULATED.
> >
> > So actually, this will require a bit more plumbing as I realize the
> > switch to disable misalignment support is global. This switch should
> > only be disabled at boot which means I won't be able to disable it at
> > runtime (while hiotplugging a CPU) for CPU detection. There are multiple
> > ways to handle that:
> >
> > 1- Have a per-cpu switch for misalignment handling which would be
> > disabled only when detection is needed.
> >
> > 2- Assume that once detected at boot-time, emulation will not change.
> >
> > Not sure which one is better though. Advice are welcomed.
>
> If I gaze into my own crystal ball, my hope is that the Venn diagram
> of "systems that support hotplug" and "systems that still use software
> assist for misaligned access" is just two circles not touching. If
> people agree with that, then the safe thing to do is enforce it, by

In a sane world, this is probably true. But given that errats exists,
who knows what systems
we may end up with.

> failing to online new hotplugged CPUs that don't conform to
> misaligned_access_speed == EMULATED if unaligned_ctl is true. We would
> sacrifice some future flexibility by making this choice now though, so
> it requires buy-in for this particular crystal ball vision.
>
> -Evan

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

Re: [PATCH 6/7] riscv: report misaligned accesses emulation to hwprobe

[kernel test robot]

Hi Clément,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6-rc3 next-20230928]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Cl-ment-L-ger/riscv-remove-unused-functions-in-traps_misaligned-c/20230926-230654
base:   linus/master
patch link:    https://lore.kernel.org/r/20230926150316.1129648-7-cleger%40rivosinc.com
patch subject: [PATCH 6/7] riscv: report misaligned accesses emulation to hwprobe
config: riscv-randconfig-002-20230929 (https://download.01.org/0day-ci/archive/20230929/202309290842.Dk0K2nsp-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230929/202309290842.Dk0K2nsp-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309290842.Dk0K2nsp-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   arch/riscv/kernel/traps_misaligned.c: In function 'handle_misaligned_load':
   arch/riscv/kernel/traps_misaligned.c:420:48: error: 'struct thread_struct' has no member named 'align_ctl'
     420 |         if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
         |                                                ^
   arch/riscv/kernel/traps_misaligned.c: In function 'handle_misaligned_store':
   arch/riscv/kernel/traps_misaligned.c:522:48: error: 'struct thread_struct' has no member named 'align_ctl'
     522 |         if (user_mode(regs) && (current->thread.align_ctl & PR_UNALIGN_SIGBUS))
         |                                                ^
   arch/riscv/kernel/traps_misaligned.c: In function 'check_unaligned_access_emulated':
>> arch/riscv/kernel/traps_misaligned.c:610:17: error: expected ':' or ')' before '_ASM_EXTABLE'
     610 |                 _ASM_EXTABLE(1b, 2b)
         |                 ^~~~~~~~~~~~
>> arch/riscv/kernel/traps_misaligned.c:610:30: error: invalid suffix "b" on integer constant
     610 |                 _ASM_EXTABLE(1b, 2b)
         |                              ^~
   arch/riscv/kernel/traps_misaligned.c:610:34: error: invalid suffix "b" on integer constant
     610 |                 _ASM_EXTABLE(1b, 2b)
         |                                  ^~
>> arch/riscv/kernel/traps_misaligned.c:602:37: warning: unused variable 'tmp_var' [-Wunused-variable]
     602 |         unsigned long emulated = 1, tmp_var;
         |                                     ^~~~~~~


vim +610 arch/riscv/kernel/traps_misaligned.c

   599	
   600	bool check_unaligned_access_emulated(int cpu)
   601	{
 > 602		unsigned long emulated = 1, tmp_var;
   603	
   604		/* Use a fixup to detect if misaligned access triggered an exception */
   605		__asm__ __volatile__ (
   606			"1:\n"
   607			"	"REG_L" %[tmp], 1(%[ptr])\n"
   608			"	li %[emulated], 0\n"
   609			"2:\n"
 > 610			_ASM_EXTABLE(1b, 2b)
   611		: [emulated] "+r" (emulated), [tmp] "=r" (tmp_var)
   612		: [ptr] "r" (&tmp_var)
   613		: "memory");
   614	
   615		if (!emulated)
   616			return false;
   617	
   618		per_cpu(misaligned_access_speed, cpu) =
   619			RISCV_HWPROBE_MISALIGNED_EMULATED;
   620	
   621		return true;
   622	}
   623	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[PATCH 7/7] riscv: add support for PR_SET_UNALIGN and PR_GET_UNALIGN

[Clément Léger]

Now that trap support is ready to handle misalignment errors in S-mode,
allow the user to control the behavior of misaligned accesses using
prctl(PR_SET_UNALIGN). Add an align_ctl flag in thread_struct which
will be used to determine if we should SIGBUS the process or not on
such fault.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 arch/riscv/include/asm/processor.h |  9 +++++++++
 arch/riscv/kernel/process.c        | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 3e23e1786d05..adbe520d07c5 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -8,6 +8,7 @@
 
 #include <linux/const.h>
 #include <linux/cache.h>
+#include <linux/prctl.h>
 
 #include <vdso/processor.h>
 
@@ -82,6 +83,7 @@ struct thread_struct {
 	unsigned long bad_cause;
 	unsigned long vstate_ctrl;
 	struct __riscv_v_ext_state vstate;
+	unsigned long align_ctl;
 };
 
 /* Whitelist the fstate from the task_struct for hardened usercopy */
@@ -94,6 +96,7 @@ static inline void arch_thread_struct_whitelist(unsigned long *offset,
 
 #define INIT_THREAD {					\
 	.sp = sizeof(init_stack) + (long)&init_stack,	\
+	.align_ctl = PR_UNALIGN_NOPRINT,		\
 }
 
 #define task_pt_regs(tsk)						\
@@ -134,6 +137,12 @@ extern long riscv_v_vstate_ctrl_set_current(unsigned long arg);
 extern long riscv_v_vstate_ctrl_get_current(void);
 #endif /* CONFIG_RISCV_ISA_V */
 
+extern int get_unalign_ctl(struct task_struct *tsk, unsigned long addr);
+extern int set_unalign_ctl(struct task_struct *tsk, unsigned int val);
+
+#define GET_UNALIGN_CTL(tsk, addr)	get_unalign_ctl((tsk), (addr))
+#define SET_UNALIGN_CTL(tsk, val)	set_unalign_ctl((tsk), (val))
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_RISCV_PROCESSOR_H */
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index e32d737e039f..4f21d970a129 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -25,6 +25,7 @@
 #include <asm/thread_info.h>
 #include <asm/cpuidle.h>
 #include <asm/vector.h>
+#include <asm/cpufeature.h>
 
 register unsigned long gp_in_global __asm__("gp");
 
@@ -41,6 +42,23 @@ void arch_cpu_idle(void)
 	cpu_do_idle();
 }
 
+int set_unalign_ctl(struct task_struct *tsk, unsigned int val)
+{
+	if (!unaligned_ctl_available())
+		return -EINVAL;
+
+	tsk->thread.align_ctl = val;
+	return 0;
+}
+
+int get_unalign_ctl(struct task_struct *tsk, unsigned long adr)
+{
+	if (!unaligned_ctl_available())
+		return -EINVAL;
+
+	return put_user(tsk->thread.align_ctl, (unsigned long __user *)adr);
+}
+
 void __show_regs(struct pt_regs *regs)
 {
 	show_regs_print_info(KERN_DEFAULT);
-- 
2.40.1


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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Evan Green]

On Tue, Sep 26, 2023 at 8:03 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> Since commit 61cadb9 ("Provide new description of misaligned load/store
> behavior compatible with privileged architecture.") in the RISC-V ISA
> manual, it is stated that misaligned load/store might not be supported.
> However, the RISC-V kernel uABI describes that misaligned accesses are
> supported. In order to support that, this series adds support for S-mode
> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
>
> Handling misaligned access in kernel allows for a finer grain control
> of the misaligned accesses behavior, and thanks to the prctl call, can
> allow disabling misaligned access emulation to generate SIGBUS. User
> space can then optimize its software by removing such access based on
> SIGBUS generation.
>
> Currently, this series is useful for people that uses a SBI that does
> not handled misaligned traps. In a near future, this series will make
> use a SBI extension [1] allowing to request delegation of the
> misaligned load/store traps to the S-mode software. This extension has
> been submitted for review to the riscv tech-prs group. An OpenSBI
> implementation for this spec is available at [2].

For my own education, how does the new SBI call behave with respect to
multiple harts? Does a call to change a feature perform that change
across all harts, or just the hart the SBI call was made on? If the
answer is "all harts", what if not all harts are exactly the same, and
some can enable the feature switch while others cannot? Also if the
answer is "all harts", does it also apply to hotplugged cpus, which
may not have even existed at boot time?

What happens if a hart goes through a context loss event, like
suspend/resume? Is the setting expected to be sticky, or is the kernel
expected to replay these calls?

-Evan

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Clément Léger]

On 26/09/2023 23:43, Evan Green wrote:
> On Tue, Sep 26, 2023 at 8:03 AM Clément Léger <cleger@rivosinc.com> wrote:
>>
>> Since commit 61cadb9 ("Provide new description of misaligned load/store
>> behavior compatible with privileged architecture.") in the RISC-V ISA
>> manual, it is stated that misaligned load/store might not be supported.
>> However, the RISC-V kernel uABI describes that misaligned accesses are
>> supported. In order to support that, this series adds support for S-mode
>> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
>>
>> Handling misaligned access in kernel allows for a finer grain control
>> of the misaligned accesses behavior, and thanks to the prctl call, can
>> allow disabling misaligned access emulation to generate SIGBUS. User
>> space can then optimize its software by removing such access based on
>> SIGBUS generation.
>>
>> Currently, this series is useful for people that uses a SBI that does
>> not handled misaligned traps. In a near future, this series will make
>> use a SBI extension [1] allowing to request delegation of the
>> misaligned load/store traps to the S-mode software. This extension has
>> been submitted for review to the riscv tech-prs group. An OpenSBI
>> implementation for this spec is available at [2].
> 
> For my own education, how does the new SBI call behave with respect to
> multiple harts? Does a call to change a feature perform that change
> across all harts, or just the hart the SBI call was made on? If the
> answer is "all harts", what if not all harts are exactly the same, and
> some can enable the feature switch while others cannot? Also if the
> answer is "all harts", does it also apply to hotplugged cpus, which
> may not have even existed at boot time?

Depending on the feature, they can be either global (all harts) or
local (calling hart). The medeleg register is per hart and thus
misaligned load/store delegation for S-mode is also per hart.


> 
> What happens if a hart goes through a context loss event, like
> suspend/resume? Is the setting expected to be sticky, or is the kernel
> expected to replay these calls?

That is a good question that we did not actually clarified yet. Thanks
for raising it !

Clément

> 
> -Evan

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Evan Green]

On Thu, Sep 28, 2023 at 12:49 AM Clément Léger <cleger@rivosinc.com> wrote:
>
>
>
> On 26/09/2023 23:43, Evan Green wrote:
> > On Tue, Sep 26, 2023 at 8:03 AM Clément Léger <cleger@rivosinc.com> wrote:
> >>
> >> Since commit 61cadb9 ("Provide new description of misaligned load/store
> >> behavior compatible with privileged architecture.") in the RISC-V ISA
> >> manual, it is stated that misaligned load/store might not be supported.
> >> However, the RISC-V kernel uABI describes that misaligned accesses are
> >> supported. In order to support that, this series adds support for S-mode
> >> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
> >>
> >> Handling misaligned access in kernel allows for a finer grain control
> >> of the misaligned accesses behavior, and thanks to the prctl call, can
> >> allow disabling misaligned access emulation to generate SIGBUS. User
> >> space can then optimize its software by removing such access based on
> >> SIGBUS generation.
> >>
> >> Currently, this series is useful for people that uses a SBI that does
> >> not handled misaligned traps. In a near future, this series will make
> >> use a SBI extension [1] allowing to request delegation of the
> >> misaligned load/store traps to the S-mode software. This extension has
> >> been submitted for review to the riscv tech-prs group. An OpenSBI
> >> implementation for this spec is available at [2].
> >
> > For my own education, how does the new SBI call behave with respect to
> > multiple harts? Does a call to change a feature perform that change
> > across all harts, or just the hart the SBI call was made on? If the
> > answer is "all harts", what if not all harts are exactly the same, and
> > some can enable the feature switch while others cannot? Also if the
> > answer is "all harts", does it also apply to hotplugged cpus, which
> > may not have even existed at boot time?
>
> Depending on the feature, they can be either global (all harts) or
> local (calling hart). The medeleg register is per hart and thus
> misaligned load/store delegation for S-mode is also per hart.

We should probably state this in the spec update then, both generally
and for each specific feature added. Otherwise firmware writers are
left not knowing if they're supposed to spread a feature across to all
cores or not.

>
>
> >
> > What happens if a hart goes through a context loss event, like
> > suspend/resume? Is the setting expected to be sticky, or is the kernel
> > expected to replay these calls?
>
> That is a good question that we did not actually clarified yet. Thanks
> for raising it !

No problem! This may also need to be specified per-feature in the
spec. I have a vague hunch that it's better to ask the kernel to do it
on resume, though ideally we'd have the terminology (and I don't think
we do?) to specify exactly which points constitute a context loss.
Mostly I'm remembering the x86 and ARM transition from S3, where lots
of firmware code ran at resume, to S0ix-like power states, where
things resumed directly into the OS and they had to figure out how to
do it without firmware. The vague hunch is that keeping the laundry
list of things firmware must do on resume low might keep us from
getting in S0ix's way, but it's all so speculative it's hard to know
if it's really a useful hunch or not.

-Evan

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Atish Kumar Patra]

On Thu, Sep 28, 2023 at 9:48 AM Evan Green <evan@rivosinc.com> wrote:
>
> On Thu, Sep 28, 2023 at 12:49 AM Clément Léger <cleger@rivosinc.com> wrote:
> >
> >
> >
> > On 26/09/2023 23:43, Evan Green wrote:
> > > On Tue, Sep 26, 2023 at 8:03 AM Clément Léger <cleger@rivosinc.com> wrote:
> > >>
> > >> Since commit 61cadb9 ("Provide new description of misaligned load/store
> > >> behavior compatible with privileged architecture.") in the RISC-V ISA
> > >> manual, it is stated that misaligned load/store might not be supported.
> > >> However, the RISC-V kernel uABI describes that misaligned accesses are
> > >> supported. In order to support that, this series adds support for S-mode
> > >> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
> > >>
> > >> Handling misaligned access in kernel allows for a finer grain control
> > >> of the misaligned accesses behavior, and thanks to the prctl call, can
> > >> allow disabling misaligned access emulation to generate SIGBUS. User
> > >> space can then optimize its software by removing such access based on
> > >> SIGBUS generation.
> > >>
> > >> Currently, this series is useful for people that uses a SBI that does
> > >> not handled misaligned traps. In a near future, this series will make
> > >> use a SBI extension [1] allowing to request delegation of the
> > >> misaligned load/store traps to the S-mode software. This extension has
> > >> been submitted for review to the riscv tech-prs group. An OpenSBI
> > >> implementation for this spec is available at [2].
> > >
> > > For my own education, how does the new SBI call behave with respect to
> > > multiple harts? Does a call to change a feature perform that change
> > > across all harts, or just the hart the SBI call was made on? If the
> > > answer is "all harts", what if not all harts are exactly the same, and
> > > some can enable the feature switch while others cannot? Also if the
> > > answer is "all harts", does it also apply to hotplugged cpus, which
> > > may not have even existed at boot time?
> >
> > Depending on the feature, they can be either global (all harts) or
> > local (calling hart). The medeleg register is per hart and thus
> > misaligned load/store delegation for S-mode is also per hart.
>
> We should probably state this in the spec update then, both generally
> and for each specific feature added. Otherwise firmware writers are
> left not knowing if they're supposed to spread a feature across to all
> cores or not.
>

If a feature is required to update any CSR, it must be per hart only.
The supervisor software
is aware of the state of each hart and it should invoke it from all
the present harts.

Doing it in M-mode will result in M-mode IPIs and seems racy with what
kernel might be doing at that time.

> >
> >
> > >
> > > What happens if a hart goes through a context loss event, like
> > > suspend/resume? Is the setting expected to be sticky, or is the kernel
> > > expected to replay these calls?
> >
> > That is a good question that we did not actually clarified yet. Thanks
> > for raising it !
>

IMO, it should be sticky until a reset. I would be interested to hear
other thoughts though if there is a non-sticky
use-case.


> No problem! This may also need to be specified per-feature in the
> spec. I have a vague hunch that it's better to ask the kernel to do it
> on resume, though ideally we'd have the terminology (and I don't think
> we do?) to specify exactly which points constitute a context loss.
> Mostly I'm remembering the x86 and ARM transition from S3, where lots
> of firmware code ran at resume, to S0ix-like power states, where
> things resumed directly into the OS and they had to figure out how to
> do it without firmware. The vague hunch is that keeping the laundry
> list of things firmware must do on resume low might keep us from
> getting in S0ix's way, but it's all so speculative it's hard to know
> if it's really a useful hunch or not.
>
> -Evan

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[ron minnich]

On Tue, Oct 3, 2023 at 2:40 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:

> IMO, it should be sticky until a reset.

that's a wonderful idea; perhaps this discussion needs to be held
elsewhere, esp. as it regards possible violation of the spec. Thanks!

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1740 bytes --]

On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
> Since commit 61cadb9 ("Provide new description of misaligned load/store
> behavior compatible with privileged architecture.") in the RISC-V ISA
> manual, it is stated that misaligned load/store might not be supported.
> However, the RISC-V kernel uABI describes that misaligned accesses are
> supported. In order to support that, this series adds support for S-mode
> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
> 
> Handling misaligned access in kernel allows for a finer grain control
> of the misaligned accesses behavior, and thanks to the prctl call, can
> allow disabling misaligned access emulation to generate SIGBUS. User
> space can then optimize its software by removing such access based on
> SIGBUS generation.
> 
> Currently, this series is useful for people that uses a SBI that does
> not handled misaligned traps. In a near future, this series will make
> use a SBI extension [1] allowing to request delegation of the
> misaligned load/store traps to the S-mode software. This extension has
> been submitted for review to the riscv tech-prs group. An OpenSBI
> implementation for this spec is available at [2].
> 
> This series can be tested using the spike simulator [3] and an openSBI
> version [4] which allows to always delegate misaligned load/store to
> S-mode.

Some patches in this series do not build for any configs, some are
broken for clang builds and others are broken for nommu. Please try to
build test this more thoroughly before you submit the next version.

Also, AIUI, this series should be marked RFC since the SBI extension
this relies on has not been frozen.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Clément Léger]

On 30/09/2023 11:23, Conor Dooley wrote:
> On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
>> Since commit 61cadb9 ("Provide new description of misaligned load/store
>> behavior compatible with privileged architecture.") in the RISC-V ISA
>> manual, it is stated that misaligned load/store might not be supported.
>> However, the RISC-V kernel uABI describes that misaligned accesses are
>> supported. In order to support that, this series adds support for S-mode
>> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
>>
>> Handling misaligned access in kernel allows for a finer grain control
>> of the misaligned accesses behavior, and thanks to the prctl call, can
>> allow disabling misaligned access emulation to generate SIGBUS. User
>> space can then optimize its software by removing such access based on
>> SIGBUS generation.
>>
>> Currently, this series is useful for people that uses a SBI that does
>> not handled misaligned traps. In a near future, this series will make
>> use a SBI extension [1] allowing to request delegation of the
>> misaligned load/store traps to the S-mode software. This extension has
>> been submitted for review to the riscv tech-prs group. An OpenSBI
>> implementation for this spec is available at [2].
>>
>> This series can be tested using the spike simulator [3] and an openSBI
>> version [4] which allows to always delegate misaligned load/store to
>> S-mode.
> 
> Some patches in this series do not build for any configs, some are
> broken for clang builds and others are broken for nommu. Please try to> build test this more thoroughly before you submit the next version.

Hi Conor,

Thanks for the feedback, I'll check that.

> 
> Also, AIUI, this series should be marked RFC since the SBI extension
> this relies on has not been frozen.

This series does not actually uses the SBI extension but provides a way
to detect if misaligned accesses are not handled by hardware nor by the
SBI. It has been reported by Ron & Daniel they they have a minimal SBI
implementation that does not handle misaligned accesses and that they
would like to make use of the PR_SET_UNALIGN feature. This is what this
series addresses (and thus does not depend on the mentioned SBI extension).

Thanks,

Clément

> 
> Cheers,
> Conor.

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2500 bytes --]

On Mon, Oct 02, 2023 at 09:40:04AM +0200, Clément Léger wrote:
> 
> 
> On 30/09/2023 11:23, Conor Dooley wrote:
> > On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
> >> Since commit 61cadb9 ("Provide new description of misaligned load/store
> >> behavior compatible with privileged architecture.") in the RISC-V ISA
> >> manual, it is stated that misaligned load/store might not be supported.
> >> However, the RISC-V kernel uABI describes that misaligned accesses are
> >> supported. In order to support that, this series adds support for S-mode
> >> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
> >>
> >> Handling misaligned access in kernel allows for a finer grain control
> >> of the misaligned accesses behavior, and thanks to the prctl call, can
> >> allow disabling misaligned access emulation to generate SIGBUS. User
> >> space can then optimize its software by removing such access based on
> >> SIGBUS generation.
> >>
> >> Currently, this series is useful for people that uses a SBI that does
> >> not handled misaligned traps. In a near future, this series will make
> >> use a SBI extension [1] allowing to request delegation of the
> >> misaligned load/store traps to the S-mode software. This extension has
> >> been submitted for review to the riscv tech-prs group. An OpenSBI
> >> implementation for this spec is available at [2].
> >>
> >> This series can be tested using the spike simulator [3] and an openSBI
> >> version [4] which allows to always delegate misaligned load/store to
> >> S-mode.
> > 
> > Some patches in this series do not build for any configs, some are
> > broken for clang builds and others are broken for nommu. Please try to> build test this more thoroughly before you submit the next version.
> 
> Hi Conor,
> 
> Thanks for the feedback, I'll check that.
> 
> > 
> > Also, AIUI, this series should be marked RFC since the SBI extension
> > this relies on has not been frozen.
> 
> This series does not actually uses the SBI extension but provides a way
> to detect if misaligned accesses are not handled by hardware nor by the
> SBI. It has been reported by Ron & Daniel they they have a minimal SBI
> implementation that does not handle misaligned accesses and that they
> would like to make use of the PR_SET_UNALIGN feature. This is what this
> series addresses (and thus does not depend on the mentioned SBI extension).

Ah, I must have misread then. Apologies.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Clément Léger]

On 02/10/2023 12:49, Conor Dooley wrote:
> On Mon, Oct 02, 2023 at 09:40:04AM +0200, Clément Léger wrote:
>>
>>
>> On 30/09/2023 11:23, Conor Dooley wrote:
>>> On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
>>>> Since commit 61cadb9 ("Provide new description of misaligned load/store
>>>> behavior compatible with privileged architecture.") in the RISC-V ISA
>>>> manual, it is stated that misaligned load/store might not be supported.
>>>> However, the RISC-V kernel uABI describes that misaligned accesses are
>>>> supported. In order to support that, this series adds support for S-mode
>>>> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
>>>>
>>>> Handling misaligned access in kernel allows for a finer grain control
>>>> of the misaligned accesses behavior, and thanks to the prctl call, can
>>>> allow disabling misaligned access emulation to generate SIGBUS. User
>>>> space can then optimize its software by removing such access based on
>>>> SIGBUS generation.
>>>>
>>>> Currently, this series is useful for people that uses a SBI that does
>>>> not handled misaligned traps. In a near future, this series will make
>>>> use a SBI extension [1] allowing to request delegation of the
>>>> misaligned load/store traps to the S-mode software. This extension has
>>>> been submitted for review to the riscv tech-prs group. An OpenSBI
>>>> implementation for this spec is available at [2].
>>>>
>>>> This series can be tested using the spike simulator [3] and an openSBI
>>>> version [4] which allows to always delegate misaligned load/store to
>>>> S-mode.
>>>
>>> Some patches in this series do not build for any configs, some are
>>> broken for clang builds and others are broken for nommu. Please try to> build test this more thoroughly before you submit the next version.
>>
>> Hi Conor,
>>
>> Thanks for the feedback, I'll check that.
>>
>>>
>>> Also, AIUI, this series should be marked RFC since the SBI extension
>>> this relies on has not been frozen.
>>
>> This series does not actually uses the SBI extension but provides a way
>> to detect if misaligned accesses are not handled by hardware nor by the
>> SBI. It has been reported by Ron & Daniel they they have a minimal SBI
>> implementation that does not handle misaligned accesses and that they
>> would like to make use of the PR_SET_UNALIGN feature. This is what this
>> series addresses (and thus does not depend on the mentioned SBI extension).
> 
> Ah, I must have misread then. Apologies.

No worries, maybe I should actually remove this from the cover letter to
avoid any confusion !

Clément

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[ron minnich]

This was a very interesting read. One other thought crossed my mind,
which is that a RISC-V implementation might make the alignment
delegation hard-wired to always delegate to S mode. I.e, the bit might
be WARL and always 1. For what I'm doing, this would actually be
pretty convenient. Just want to make sure this code can accommodate
that -- wdyt?

We have found lots of value in our experiments with delegating
alignment traps to Linux -- not least because they tend to locate
problems in the kernel :-) -- we've found issues in module loading,
early startup (there's a needed .align2 directive for sbi secondary
startup, AFAICT) and the timing code for misaligned load/store
handling.

I don't know how you test this unaligned trap handling, but it might
be worthwhile to work that out. You can test via oreboot and the
visionfive2, save we have not figured out why SMP startup is going
wrong, yet :-), so we're not as feature-complete as needed. But soon.

Thanks!

On Mon, Oct 2, 2023 at 5:19 AM Clément Léger <cleger@rivosinc.com> wrote:
>
>
>
> On 02/10/2023 12:49, Conor Dooley wrote:
> > On Mon, Oct 02, 2023 at 09:40:04AM +0200, Clément Léger wrote:
> >>
> >>
> >> On 30/09/2023 11:23, Conor Dooley wrote:
> >>> On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
> >>>> Since commit 61cadb9 ("Provide new description of misaligned load/store
> >>>> behavior compatible with privileged architecture.") in the RISC-V ISA
> >>>> manual, it is stated that misaligned load/store might not be supported.
> >>>> However, the RISC-V kernel uABI describes that misaligned accesses are
> >>>> supported. In order to support that, this series adds support for S-mode
> >>>> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
> >>>>
> >>>> Handling misaligned access in kernel allows for a finer grain control
> >>>> of the misaligned accesses behavior, and thanks to the prctl call, can
> >>>> allow disabling misaligned access emulation to generate SIGBUS. User
> >>>> space can then optimize its software by removing such access based on
> >>>> SIGBUS generation.
> >>>>
> >>>> Currently, this series is useful for people that uses a SBI that does
> >>>> not handled misaligned traps. In a near future, this series will make
> >>>> use a SBI extension [1] allowing to request delegation of the
> >>>> misaligned load/store traps to the S-mode software. This extension has
> >>>> been submitted for review to the riscv tech-prs group. An OpenSBI
> >>>> implementation for this spec is available at [2].
> >>>>
> >>>> This series can be tested using the spike simulator [3] and an openSBI
> >>>> version [4] which allows to always delegate misaligned load/store to
> >>>> S-mode.
> >>>
> >>> Some patches in this series do not build for any configs, some are
> >>> broken for clang builds and others are broken for nommu. Please try to> build test this more thoroughly before you submit the next version.
> >>
> >> Hi Conor,
> >>
> >> Thanks for the feedback, I'll check that.
> >>
> >>>
> >>> Also, AIUI, this series should be marked RFC since the SBI extension
> >>> this relies on has not been frozen.
> >>
> >> This series does not actually uses the SBI extension but provides a way
> >> to detect if misaligned accesses are not handled by hardware nor by the
> >> SBI. It has been reported by Ron & Daniel they they have a minimal SBI
> >> implementation that does not handle misaligned accesses and that they
> >> would like to make use of the PR_SET_UNALIGN feature. This is what this
> >> series addresses (and thus does not depend on the mentioned SBI extension).
> >
> > Ah, I must have misread then. Apologies.
>
> No worries, maybe I should actually remove this from the cover letter to
> avoid any confusion !
>
> Clément

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

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Jessica Clarke]

On 2 Oct 2023, at 16:32, ron minnich <rminnich@gmail.com> wrote:
> 
> This was a very interesting read. One other thought crossed my mind,
> which is that a RISC-V implementation might make the alignment
> delegation hard-wired to always delegate to S mode. I.e, the bit might
> be WARL and always 1. For what I'm doing, this would actually be
> pretty convenient. Just want to make sure this code can accommodate
> that -- wdyt?

Such an implementation would violate the spec:

  An implementation shall not have any bits of medeleg be read-only
  one, i.e., any synchronous trap that can be delegated must support not
  being delegated.

Supporting that is thus out of scope.

Jess

> We have found lots of value in our experiments with delegating
> alignment traps to Linux -- not least because they tend to locate
> problems in the kernel :-) -- we've found issues in module loading,
> early startup (there's a needed .align2 directive for sbi secondary
> startup, AFAICT) and the timing code for misaligned load/store
> handling.
> 
> I don't know how you test this unaligned trap handling, but it might
> be worthwhile to work that out. You can test via oreboot and the
> visionfive2, save we have not figured out why SMP startup is going
> wrong, yet :-), so we're not as feature-complete as needed. But soon.
> 
> Thanks!
> 
> On Mon, Oct 2, 2023 at 5:19 AM Clément Léger <cleger@rivosinc.com> wrote:
>> 
>> 
>> 
>> On 02/10/2023 12:49, Conor Dooley wrote:
>>> On Mon, Oct 02, 2023 at 09:40:04AM +0200, Clément Léger wrote:
>>>> 
>>>> 
>>>> On 30/09/2023 11:23, Conor Dooley wrote:
>>>>> On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
>>>>>> Since commit 61cadb9 ("Provide new description of misaligned load/store
>>>>>> behavior compatible with privileged architecture.") in the RISC-V ISA
>>>>>> manual, it is stated that misaligned load/store might not be supported.
>>>>>> However, the RISC-V kernel uABI describes that misaligned accesses are
>>>>>> supported. In order to support that, this series adds support for S-mode
>>>>>> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
>>>>>> 
>>>>>> Handling misaligned access in kernel allows for a finer grain control
>>>>>> of the misaligned accesses behavior, and thanks to the prctl call, can
>>>>>> allow disabling misaligned access emulation to generate SIGBUS. User
>>>>>> space can then optimize its software by removing such access based on
>>>>>> SIGBUS generation.
>>>>>> 
>>>>>> Currently, this series is useful for people that uses a SBI that does
>>>>>> not handled misaligned traps. In a near future, this series will make
>>>>>> use a SBI extension [1] allowing to request delegation of the
>>>>>> misaligned load/store traps to the S-mode software. This extension has
>>>>>> been submitted for review to the riscv tech-prs group. An OpenSBI
>>>>>> implementation for this spec is available at [2].
>>>>>> 
>>>>>> This series can be tested using the spike simulator [3] and an openSBI
>>>>>> version [4] which allows to always delegate misaligned load/store to
>>>>>> S-mode.
>>>>> 
>>>>> Some patches in this series do not build for any configs, some are
>>>>> broken for clang builds and others are broken for nommu. Please try to> build test this more thoroughly before you submit the next version.
>>>> 
>>>> Hi Conor,
>>>> 
>>>> Thanks for the feedback, I'll check that.
>>>> 
>>>>> 
>>>>> Also, AIUI, this series should be marked RFC since the SBI extension
>>>>> this relies on has not been frozen.
>>>> 
>>>> This series does not actually uses the SBI extension but provides a way
>>>> to detect if misaligned accesses are not handled by hardware nor by the
>>>> SBI. It has been reported by Ron & Daniel they they have a minimal SBI
>>>> implementation that does not handle misaligned accesses and that they
>>>> would like to make use of the PR_SET_UNALIGN feature. This is what this
>>>> series addresses (and thus does not depend on the mentioned SBI extension).
>>> 
>>> Ah, I must have misread then. Apologies.
>> 
>> No worries, maybe I should actually remove this from the cover letter to
>> avoid any confusion !
>> 
>> Clément
> 
> _______________________________________________
> 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 0/7] Add support to handle misaligned accesses in S-mode

[ron minnich]

While it is true that it violates the spec today, given the fluidity
of the spec of the last 10 years, I'm not sure that matters :-)

Anyway, that's out of scope for this discussion, though I appreciate
your clarification. I'll bring it up elsewhere.

Clement points out that this series would work fine if that bit were
hardwired to 1, which is all I care about.

thanks

On Mon, Oct 2, 2023 at 4:23 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 2 Oct 2023, at 16:32, ron minnich <rminnich@gmail.com> wrote:
> >
> > This was a very interesting read. One other thought crossed my mind,
> > which is that a RISC-V implementation might make the alignment
> > delegation hard-wired to always delegate to S mode. I.e, the bit might
> > be WARL and always 1. For what I'm doing, this would actually be
> > pretty convenient. Just want to make sure this code can accommodate
> > that -- wdyt?
>
> Such an implementation would violate the spec:
>
>   An implementation shall not have any bits of medeleg be read-only
>   one, i.e., any synchronous trap that can be delegated must support not
>   being delegated.
>
> Supporting that is thus out of scope.
>
> Jess
>
> > We have found lots of value in our experiments with delegating
> > alignment traps to Linux -- not least because they tend to locate
> > problems in the kernel :-) -- we've found issues in module loading,
> > early startup (there's a needed .align2 directive for sbi secondary
> > startup, AFAICT) and the timing code for misaligned load/store
> > handling.
> >
> > I don't know how you test this unaligned trap handling, but it might
> > be worthwhile to work that out. You can test via oreboot and the
> > visionfive2, save we have not figured out why SMP startup is going
> > wrong, yet :-), so we're not as feature-complete as needed. But soon.
> >
> > Thanks!
> >
> > On Mon, Oct 2, 2023 at 5:19 AM Clément Léger <cleger@rivosinc.com> wrote:
> >>
> >>
> >>
> >> On 02/10/2023 12:49, Conor Dooley wrote:
> >>> On Mon, Oct 02, 2023 at 09:40:04AM +0200, Clément Léger wrote:
> >>>>
> >>>>
> >>>> On 30/09/2023 11:23, Conor Dooley wrote:
> >>>>> On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
> >>>>>> Since commit 61cadb9 ("Provide new description of misaligned load/store
> >>>>>> behavior compatible with privileged architecture.") in the RISC-V ISA
> >>>>>> manual, it is stated that misaligned load/store might not be supported.
> >>>>>> However, the RISC-V kernel uABI describes that misaligned accesses are
> >>>>>> supported. In order to support that, this series adds support for S-mode
> >>>>>> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
> >>>>>>
> >>>>>> Handling misaligned access in kernel allows for a finer grain control
> >>>>>> of the misaligned accesses behavior, and thanks to the prctl call, can
> >>>>>> allow disabling misaligned access emulation to generate SIGBUS. User
> >>>>>> space can then optimize its software by removing such access based on
> >>>>>> SIGBUS generation.
> >>>>>>
> >>>>>> Currently, this series is useful for people that uses a SBI that does
> >>>>>> not handled misaligned traps. In a near future, this series will make
> >>>>>> use a SBI extension [1] allowing to request delegation of the
> >>>>>> misaligned load/store traps to the S-mode software. This extension has
> >>>>>> been submitted for review to the riscv tech-prs group. An OpenSBI
> >>>>>> implementation for this spec is available at [2].
> >>>>>>
> >>>>>> This series can be tested using the spike simulator [3] and an openSBI
> >>>>>> version [4] which allows to always delegate misaligned load/store to
> >>>>>> S-mode.
> >>>>>
> >>>>> Some patches in this series do not build for any configs, some are
> >>>>> broken for clang builds and others are broken for nommu. Please try to> build test this more thoroughly before you submit the next version.
> >>>>
> >>>> Hi Conor,
> >>>>
> >>>> Thanks for the feedback, I'll check that.
> >>>>
> >>>>>
> >>>>> Also, AIUI, this series should be marked RFC since the SBI extension
> >>>>> this relies on has not been frozen.
> >>>>
> >>>> This series does not actually uses the SBI extension but provides a way
> >>>> to detect if misaligned accesses are not handled by hardware nor by the
> >>>> SBI. It has been reported by Ron & Daniel they they have a minimal SBI
> >>>> implementation that does not handle misaligned accesses and that they
> >>>> would like to make use of the PR_SET_UNALIGN feature. This is what this
> >>>> series addresses (and thus does not depend on the mentioned SBI extension).
> >>>
> >>> Ah, I must have misread then. Apologies.
> >>
> >> No worries, maybe I should actually remove this from the cover letter to
> >> avoid any confusion !
> >>
> >> Clément
> >
> > _______________________________________________
> > 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 0/7] Add support to handle misaligned accesses in S-mode

[Clément Léger]

On 02/10/2023 17:32, ron minnich wrote:
> This was a very interesting read. One other thought crossed my mind,
> which is that a RISC-V implementation might make the alignment
> delegation hard-wired to always delegate to S mode. I.e, the bit might
> be WARL and always 1. For what I'm doing, this would actually be
> pretty convenient. Just want to make sure this code can accommodate
> that -- wdyt?

Hi Ron,

This series does not really care about "how" misaligned load/store are
delegated, it only tries to check if misaligned load/store are handled
by the kernel. So whatever you decide to do to delegate that is a bit
out of the scope of this series.

> 
> We have found lots of value in our experiments with delegating
> alignment traps to Linux -- not least because they tend to locate
> problems in the kernel :-) -- we've found issues in module loading,
> early startup (there's a needed .align2 directive for sbi secondary
> startup, AFAICT) and the timing code for misaligned load/store
> handling.>
> I don't know how you test this unaligned trap handling, but it might
> be worthwhile to work that out. You can test via oreboot and the
> visionfive2, save we have not figured out why SMP startup is going
> wrong, yet :-), so we're not as feature-complete as needed. But soon.

I test that on spike (which does not handle misaligned accesses contrary
to qemu) using a userspace program that actually exercise all kind of
standard load/store instructions as well as FPU ones with different
registers. Regarding the kernel, you are right that I might be lacking a
few tests though. I'll also consider using a visionfive2 board to
validate that on real hardware.

Thanks,

Clément

> 
> Thanks!
> 
> On Mon, Oct 2, 2023 at 5:19 AM Clément Léger <cleger@rivosinc.com> wrote:
>>
>>
>>
>> On 02/10/2023 12:49, Conor Dooley wrote:
>>> On Mon, Oct 02, 2023 at 09:40:04AM +0200, Clément Léger wrote:
>>>>
>>>>
>>>> On 30/09/2023 11:23, Conor Dooley wrote:
>>>>> On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
>>>>>> Since commit 61cadb9 ("Provide new description of misaligned load/store
>>>>>> behavior compatible with privileged architecture.") in the RISC-V ISA
>>>>>> manual, it is stated that misaligned load/store might not be supported.
>>>>>> However, the RISC-V kernel uABI describes that misaligned accesses are
>>>>>> supported. In order to support that, this series adds support for S-mode
>>>>>> handling of misaligned accesses as well support for prctl(PR_UNALIGN).
>>>>>>
>>>>>> Handling misaligned access in kernel allows for a finer grain control
>>>>>> of the misaligned accesses behavior, and thanks to the prctl call, can
>>>>>> allow disabling misaligned access emulation to generate SIGBUS. User
>>>>>> space can then optimize its software by removing such access based on
>>>>>> SIGBUS generation.
>>>>>>
>>>>>> Currently, this series is useful for people that uses a SBI that does
>>>>>> not handled misaligned traps. In a near future, this series will make
>>>>>> use a SBI extension [1] allowing to request delegation of the
>>>>>> misaligned load/store traps to the S-mode software. This extension has
>>>>>> been submitted for review to the riscv tech-prs group. An OpenSBI
>>>>>> implementation for this spec is available at [2].
>>>>>>
>>>>>> This series can be tested using the spike simulator [3] and an openSBI
>>>>>> version [4] which allows to always delegate misaligned load/store to
>>>>>> S-mode.
>>>>>
>>>>> Some patches in this series do not build for any configs, some are
>>>>> broken for clang builds and others are broken for nommu. Please try to> build test this more thoroughly before you submit the next version.
>>>>
>>>> Hi Conor,
>>>>
>>>> Thanks for the feedback, I'll check that.
>>>>
>>>>>
>>>>> Also, AIUI, this series should be marked RFC since the SBI extension
>>>>> this relies on has not been frozen.
>>>>
>>>> This series does not actually uses the SBI extension but provides a way
>>>> to detect if misaligned accesses are not handled by hardware nor by the
>>>> SBI. It has been reported by Ron & Daniel they they have a minimal SBI
>>>> implementation that does not handle misaligned accesses and that they
>>>> would like to make use of the PR_SET_UNALIGN feature. This is what this
>>>> series addresses (and thus does not depend on the mentioned SBI extension).
>>>
>>> Ah, I must have misread then. Apologies.
>>
>> No worries, maybe I should actually remove this from the cover letter to
>> avoid any confusion !
>>
>> Clément

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

RE: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[David Laight]

From: Clément Léger
> Sent: 02 October 2023 08:40
> 
> On 30/09/2023 11:23, Conor Dooley wrote:
> > On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
> >> Since commit 61cadb9 ("Provide new description of misaligned load/store
> >> behavior compatible with privileged architecture.") in the RISC-V ISA
> >> manual, it is stated that misaligned load/store might not be supported.
> >> However, the RISC-V kernel uABI describes that misaligned accesses are
> >> supported.
...

That it just really horrid.
If the cpu is going to trap misaligned accesses then you want
The compiler generated code (ie packed data) not to generate
misaligned accesses.
So you have to change the kernel uABI.

OTOH if you known that such accesses won't fault and will be
not really slower than aligned accesses then optimised versions
of some functions (like memcpy and checksums) can use misaligned
accesses.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[Clément Léger]

On 04/10/2023 10:26, David Laight wrote:
> From: Clément Léger
>> Sent: 02 October 2023 08:40
>>
>> On 30/09/2023 11:23, Conor Dooley wrote:
>>> On Tue, Sep 26, 2023 at 05:03:09PM +0200, Clément Léger wrote:
>>>> Since commit 61cadb9 ("Provide new description of misaligned load/store
>>>> behavior compatible with privileged architecture.") in the RISC-V ISA
>>>> manual, it is stated that misaligned load/store might not be supported.
>>>> However, the RISC-V kernel uABI describes that misaligned accesses are
>>>> supported.
> ...
> 
> That it just really horrid.
> If the cpu is going to trap misaligned accesses then you want
> The compiler generated code (ie packed data) not to generate
> misaligned accesses.

Hi David,

Saying that you support misaligned access does not mean that they are
going to be efficient, just that they are supported (in fact, the uABI
state that they may perform poorly). The compiler is actually not so
stupid and will try to do as much aligned access as possible in what it
generates (unless forced by some assembly, cast or whatever that can
screw up alignment accesses). This is already the case and it will most
probably not change.

> So you have to change the kernel uABI.

I don't think so. Rule N°1 for kernel development is "Don't break the
userspace". So if changing the RISC-V uABI to say "misaligned accesses
are not supported", that is unlikely to happen. We stated that
misaligned access are supported and thus, they will continue to be
supported.

> 
> OTOH if you known that such accesses won't fault and will be
> not really slower than aligned accesses then optimised versions
> of some functions (like memcpy and checksums) can use misaligned
> accesses.

Yes, this is selected by HAVE_EFFICIENT_UNALIGNED_ACCESS. On RISC-V,
since the specification says nothing about the efficiency of such
access, we can't select it like that. Some RISC-V based SoC/CPUs might
want to select it manually in their config. In order to support that
dynamically and in a generic way, some future work could involve using
static keys for such alternatives and enable it based on the speed that
was detected.

Thanks,

Clément

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

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

RE: [PATCH 0/7] Add support to handle misaligned accesses in S-mode

[David Laight]

...
> Saying that you support misaligned access does not mean that they are
> going to be efficient, just that they are supported (in fact, the uABI
> state that they may perform poorly). The compiler is actually not so
> stupid and will try to do as much aligned access as possible in what it
> generates (unless forced by some assembly, cast or whatever that can
> screw up alignment accesses). This is already the case and it will most
> probably not change.

I did a quick check.

https://godbolt.org/z/j3e9drv4e

The code generated by both clang and gcc for misaligned reads is horrid.
Gcc does a better job if the alignment is known but generates much
the same as the clang code when it isn't.

The C code is much shorter.
Even though both gcc and clang add a (different) instruction to it

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv