> From: Jeremie Courreges-Anglas <[email protected]> > Date: Mon, 13 Sep 2021 21:35:33 +0200 > > 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.
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 > -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 >
