Re: riscv64: icache flush using sysarch(2)
Mark Kettenis wrote: > Really depends on how the future riscv64 instructions behave. A valid > concern so the XXX is appropriate. But I would pass the address and > size to pmap_proc_iflush(). Yes, let the pmap decide what it wants to do.
Re: riscv64: icache flush using sysarch(2)
> From: Jeremie Courreges-Anglas
> Date: Mon, 13 Sep 2021 21:35:33 +0200
>
> On Thu, Sep 09 2021, Mark Kettenis wrote:
> >> From: Jeremie Courreges-Anglas
> >> Date: Sun, 05 Sep 2021 21:41:35 +0200
> >>
> >> On Sat, Sep 04 2021, Jeremie Courreges-Anglas 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.
Actually, I'm not so sure about that. The uvm_io() call should only
success if the address is invalid, but whether that is still valid
after the uvmspace_free() call is unclear. The kernel lock probably
saves us here.
> 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.
Really depends on how the future riscv64 instructions behave. A valid
concern so the XXX is appropriate. But I would pass the address and
size to pmap_proc_iflush().
With that change, this is ok kettenis@
You should probably commit the kernel bits first and commit the llvm
bit after a few days.
> 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
> - 1.3
> +++ gnu/llvm/compiler-rt/lib/builtins/clear_cache.c 13 Sep 2021 19:33:22
>
Re: riscv64: icache flush using sysarch(2)
On Thu, Sep 09 2021, Mark Kettenis wrote:
>> From: Jeremie Courreges-Anglas
>> Date: Sun, 05 Sep 2021 21:41:35 +0200
>>
>> On Sat, Sep 04 2021, Jeremie Courreges-Anglas 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
- 1.3
+++ gnu/llvm/compiler-rt/lib/builtins/clear_cache.c 13 Sep 2021 19:33:22
-
@@ -33,7 +33,7 @@ uintptr_t GetCurrentProcess(void);
#include
#endif
-#if defined(__OpenBSD__) && (defined(__arm__) || defined(__mips__))
+#if defined(__OpenBSD__) && (defined(__arm__) || defined(__mips__) ||
defined(__riscv))
// clang-format off
#include
#include
@@ -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, );
#else
#if __APPLE__
// On Darwin, sys_icache_invalidate()
Re: riscv64: icache flush using sysarch(2)
> From: Jeremie Courreges-Anglas
> Date: Sun, 05 Sep 2021 21:41:35 +0200
>
> On Sat, Sep 04 2021, Jeremie Courreges-Anglas 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. 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.
> > 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. 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
> Updated diff that builds (one #include was missing) and that appears to
> do the right thing. The diff also includes the compiler-rt change.
> 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
> - 1.3
> +++ gnu/llvm/compiler-rt/lib/builtins/clear_cache.c 19 Aug 2021 17:28:40
> -
> @@ -33,7 +33,7 @@ uintptr_t GetCurrentProcess(void);
> #include
> #endif
>
> -#if defined(__OpenBSD__) && (defined(__arm__) || defined(__mips__))
> +#if defined(__OpenBSD__) && (defined(__arm__) || defined(__mips__) ||
> defined(__riscv))
> // clang-format off
> #include
> #include
> @@ -157,6 +157,8 @@ 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__)
> + sysarch(RISCV_SYNC_ICACHE_ALL, NULL);
> #else
> #if __APPLE__
>// On Darwin, sys_icache_invalidate() provides this functionality
> Index: sys/arch/riscv64/include/cpufunc.h
> ===
> RCS file: /cvs/src/sys/arch/riscv64/include/cpufunc.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 cpufunc.h
> --- sys/arch/riscv64/include/cpufunc.h18 May 2021 09:14:49 -
> 1.4
> +++ sys/arch/riscv64/include/cpufunc.h18 Aug 2021 23:24:59 -
> @@ -92,6 +92,7 @@ extern int64_t icache_line_size;
> extern void (*cpu_dcache_wbinv_range)(paddr_t, psize_t);
> extern void (*cpu_dcache_inv_range)(paddr_t, psize_t);
> extern void (*cpu_dcache_wb_range)(paddr_t, psize_t);
> +extern void icache_flush(void);
>
> static __inline void
> load_satp(uint64_t val)
> Index: sys/arch/riscv64/include/sysarch.h
> ===
> RCS file: sys/arch/riscv64/include/sysarch.h
> diff -N
Re: riscv64: icache flush using sysarch(2)
On Sat, Sep 04 2021, Jeremie Courreges-Anglas 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.
>
> 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.
Updated diff that builds (one #include was missing) and that appears to
do the right thing. The diff also includes the compiler-rt change.
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
- 1.3
+++ gnu/llvm/compiler-rt/lib/builtins/clear_cache.c 19 Aug 2021 17:28:40
-
@@ -33,7 +33,7 @@ uintptr_t GetCurrentProcess(void);
#include
#endif
-#if defined(__OpenBSD__) && (defined(__arm__) || defined(__mips__))
+#if defined(__OpenBSD__) && (defined(__arm__) || defined(__mips__) ||
defined(__riscv))
// clang-format off
#include
#include
@@ -157,6 +157,8 @@ 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__)
+ sysarch(RISCV_SYNC_ICACHE_ALL, NULL);
#else
#if __APPLE__
// On Darwin, sys_icache_invalidate() provides this functionality
Index: sys/arch/riscv64/include/cpufunc.h
===
RCS file: /cvs/src/sys/arch/riscv64/include/cpufunc.h,v
retrieving revision 1.4
diff -u -p -r1.4 cpufunc.h
--- sys/arch/riscv64/include/cpufunc.h 18 May 2021 09:14:49 - 1.4
+++ sys/arch/riscv64/include/cpufunc.h 18 Aug 2021 23:24:59 -
@@ -92,6 +92,7 @@ extern int64_t icache_line_size;
extern void (*cpu_dcache_wbinv_range)(paddr_t, psize_t);
extern void (*cpu_dcache_inv_range)(paddr_t, psize_t);
extern void (*cpu_dcache_wb_range)(paddr_t, psize_t);
+extern void icache_flush(void);
static __inline void
load_satp(uint64_t val)
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 -
+++ sys/arch/riscv64/include/sysarch.h 18 Aug 2021 23:24:59 -
@@ -0,0 +1,38 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2021 Jeremie Courreges-Anglas
+ *
+ * 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)
+
