On Thu, Sep 09 2021, Mark Kettenis <[email protected]> wrote:
>> From: Jeremie Courreges-Anglas <[email protected]>
>> Date: Sun, 05 Sep 2021 21:41:35 +0200
>>
>> On Sat, Sep 04 2021, Jeremie Courreges-Anglas <[email protected]> wrote:
>> > The first problem I was able to diagnose using egdb on riscv was
>> > lang/python/2.7 using libffi and aborting in libcompiler-rt (the
>> > compilerrt_abort() call below).
>> >
>> > --8<--
>> > #elif defined(__riscv) && defined(__linux__)
>> > #define __NR_riscv_flush_icache (244 + 15)
>> > register void *start_reg __asm("a0") = start;
>> > const register void *end_reg __asm("a1") = end;
>> > const register long flags __asm("a2") = 0;
>> > const register long syscall_nr __asm("a7") = __NR_riscv_flush_icache;
>> > __asm __volatile("ecall"
>> > : "=r"(start_reg)
>> > : "r"(start_reg), "r"(end_reg), "r"(flags),
>> > "r"(syscall_nr));
>> > assert(start_reg == 0 && "Cache flush syscall failed.");
>> > #else
>> > #if __APPLE__
>> > // On Darwin, sys_icache_invalidate() provides this functionality
>> > sys_icache_invalidate(start, end - start);
>> > #else
>> > compilerrt_abort();
>> > #endif
>> > #endif
>> > }
>> > -->8--
>> >
>> > The ususal way we provide this functionality is through sysarch(2).
>> > Since the RISC-V ISA only provides fence.i as an extension, and that
>> > fence.i doesn't support parameters to only act on a specific range,
>> > I figured I would reflect that in the API for the sake of clarity.
>> >
>> > If people expect the spec to evolve and new CPUs to ship with
>> > support for finer-grained invalidation, a more forward-looking approach
>> > would be to mimic ARM_SYNC_ICACHE and struct arm_sync_icache_args, and
>> > let the kernel ignore the parameters if appropriate.
>
> I think people expect the spec to evolve, and that's why fence.i is
> considered to be an extention. The Linux syscal for this does specify
> and address range to flush, so I think it makes sense for us to do the
> same.
100% fine with this.
> Modelling this on ARM_SYNC_ICACHE makes sense to me. Don't
> think we need to do the UVM dance that arm32_sync_icache() does
> though. Keep it simple for now.
At least arm32_sync_icache() does some kind of validation:
revision 1.4
date: 2017/03/21 21:43:11; author: kettenis; state: Exp; lines: +36 -2;
commitid: nGOn4iYognC4A9wJ;
Avoid panic in arm_sync_icache() by only flushing the parts of the address
space for which we have a userland mapping.
>> > In the diff below I'm moving the core of the code to cpu.c since it
>> > doesn't look pmap-specific, but I don't feel strongly about it.
>> > I haven't even built this since I'm still on the way back from k2k21 but
>> > I figured I'd ask for feedback early. Input welcome.
>
> While you're not wrong, pmap.c is the traditional place where we deal
> with dcache to icache incoherency. So I think keeping the code there
> makes sense.
okay. That results in much less churn.
> In fact, we already have a standardized pmap interface
> for this, which is pmap_proc_iflush(). This is the code that gets
> called by ptrace(2) to make sure an inserted breakpoint is visible to
> all CPUs. My suggestion would be to make RISCV64_SYNC_ICACHE call
> this function and keep the code in pmap.c
In sys_process.c:process_domem() the various checks probably prevent
userland from calling pmap_proc_iflush() with a bogus addresse/length.
I fear that we could hit the same kind of panic as on arm if people
later modify riscv64 icache_flush() and pmap_proc_iflush() to actually
implement finer-grained icache invalidation and sysarch() isn't updated
to perform argument validation.
Wouldn't it be clearer to stop marking icache_flush() as inline and
reuse it directly for sysarch() support? (Not implemented in the diff
below.)
Index: gnu/llvm/compiler-rt/lib/builtins/clear_cache.c
===================================================================
RCS file: /cvs/src/gnu/llvm/compiler-rt/lib/builtins/clear_cache.c,v
retrieving revision 1.3
diff -u -p -r1.3 clear_cache.c
--- gnu/llvm/compiler-rt/lib/builtins/clear_cache.c 2 Jan 2021 17:14:13
-0000 1.3
+++ gnu/llvm/compiler-rt/lib/builtins/clear_cache.c 13 Sep 2021 19:33:22
-0000
@@ -33,7 +33,7 @@ uintptr_t GetCurrentProcess(void);
#include <machine/sysarch.h>
#endif
-#if defined(__OpenBSD__) && (defined(__arm__) || defined(__mips__))
+#if defined(__OpenBSD__) && (defined(__arm__) || defined(__mips__) ||
defined(__riscv))
// clang-format off
#include <sys/types.h>
#include <machine/sysarch.h>
@@ -157,6 +157,13 @@ void __clear_cache(void *start, void *en
: "=r"(start_reg)
: "r"(start_reg), "r"(end_reg), "r"(flags),
"r"(syscall_nr));
assert(start_reg == 0 && "Cache flush syscall failed.");
+#elif defined(__riscv) && defined(__OpenBSD__)
+ struct riscv_sync_icache_args arg;
+
+ arg.addr = (uintptr_t)start;
+ arg.len = (uintptr_t)end - (uintptr_t)start;
+
+ sysarch(RISCV_SYNC_ICACHE, &arg);
#else
#if __APPLE__
// On Darwin, sys_icache_invalidate() provides this functionality
Index: sys/arch/riscv64/include/sysarch.h
===================================================================
RCS file: sys/arch/riscv64/include/sysarch.h
diff -N sys/arch/riscv64/include/sysarch.h
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ sys/arch/riscv64/include/sysarch.h 13 Sep 2021 19:33:22 -0000
@@ -0,0 +1,43 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2021 Jeremie Courreges-Anglas <[email protected]>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#ifndef _RISCV64_SYSARCH_H_
+#define _RISCV64_SYSARCH_H_
+
+/*
+ * Architecture specific syscalls (riscv64)
+ */
+
+#define RISCV_SYNC_ICACHE 0
+
+struct riscv_sync_icache_args {
+ u_int64_t addr; /* Virtual start address */
+ size_t len; /* Region size */
+};
+
+#ifndef _KERNEL
+
+#include <sys/cdefs.h>
+
+__BEGIN_DECLS
+int sysarch(int, void *);
+__END_DECLS
+
+#endif /* _KERNEL */
+
+#endif /* _RISCV64_SYSARCH_H_ */
Index: sys/arch/riscv64/riscv64/machdep.c
===================================================================
RCS file: /cvs/src/sys/arch/riscv64/riscv64/machdep.c,v
retrieving revision 1.25
diff -u -p -r1.25 machdep.c
--- sys/arch/riscv64/riscv64/machdep.c 2 Jul 2021 14:50:18 -0000 1.25
+++ sys/arch/riscv64/riscv64/machdep.c 13 Sep 2021 19:33:22 -0000
@@ -44,6 +44,7 @@
#include <machine/bus.h>
#include <machine/riscv64var.h>
#include <machine/sbi.h>
+#include <machine/sysarch.h>
#include <machine/db_machdep.h>
#include <ddb/db_extern.h>
@@ -514,9 +515,20 @@ sys_sysarch(struct proc *p, void *v, reg
syscallarg(int) op;
syscallarg(void *) parms;
} */ *uap = v;
+ struct riscv_sync_icache_args args;
int error = 0;
switch (SCARG(uap, op)) {
+ case RISCV_SYNC_ICACHE:
+ if (SCARG(uap, parms) != NULL)
+ error = copyin(SCARG(uap, parms), &args, sizeof(args));
+ if (error)
+ break;
+ /*
+ * XXX Validate args.addr and args.len before using them.
+ */
+ pmap_proc_iflush(p->p_p, (vaddr_t)0, 0);
+ break;
default:
error = EINVAL;
break;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
