Date: Mon, 19 Nov 2018 16:03:40 -0800 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Justin Hibbits <jhibbits@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r340653 - in head/sys/powerpc: fpu include powerpc Message-ID: <5bb6d4a3-57e0-6008-3e6a-4bad77fd3108@freebsd.org> In-Reply-To: <201811192354.wAJNsnUu065339@repo.freebsd.org> References: <201811192354.wAJNsnUu065339@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Is this reasonable? What if the junk in the cache happened to be a *valid* instruction? Won't this approach result in silent corruption and later failure? -Nathan On 11/19/18 3:54 PM, Justin Hibbits wrote: > Author: jhibbits > Date: Mon Nov 19 23:54:49 2018 > New Revision: 340653 > URL: https://svnweb.freebsd.org/changeset/base/340653 > > Log: > powerpc: Sync icache on SIGILL, in case of cache issues > > The update of jemalloc to 5.1.0 exposed a cache syncing issue on a Freescale > e500 base system. There was already code in the FPU emulator to address > this, but it was limited to a single static variable, and did not attempt to > sync the cache. This pulls that out to the higher level program exception > handler, and syncs the cache. > > If a SIGILL is hit a second time at the same address, it will be treated as > a real illegal instruction, and handled accordingly. > > Modified: > head/sys/powerpc/fpu/fpu_emu.c > head/sys/powerpc/include/pcb.h > head/sys/powerpc/powerpc/exec_machdep.c > > Modified: head/sys/powerpc/fpu/fpu_emu.c > ============================================================================== > --- head/sys/powerpc/fpu/fpu_emu.c Mon Nov 19 22:18:18 2018 (r340652) > +++ head/sys/powerpc/fpu/fpu_emu.c Mon Nov 19 23:54:49 2018 (r340653) > @@ -189,7 +189,6 @@ fpu_emulate(struct trapframe *frame, struct fpu *fpf) > { > union instr insn; > struct fpemu fe; > - static int lastill = 0; > int sig; > > /* initialize insn.is_datasize to tell it is *not* initialized */ > @@ -243,17 +242,11 @@ fpu_emulate(struct trapframe *frame, struct fpu *fpf) > opc_disasm(frame->srr0, insn.i_int); > } > #endif > - /* > - * XXXX retry an illegal insn once due to cache issues. > - */ > - if (lastill == frame->srr0) { > - sig = SIGILL; > + sig = SIGILL; > #ifdef DEBUG > - if (fpe_debug & FPE_EX) > - kdb_enter(KDB_WHY_UNSET, "illegal instruction"); > + if (fpe_debug & FPE_EX) > + kdb_enter(KDB_WHY_UNSET, "illegal instruction"); > #endif > - } > - lastill = frame->srr0; > break; > } > > > Modified: head/sys/powerpc/include/pcb.h > ============================================================================== > --- head/sys/powerpc/include/pcb.h Mon Nov 19 22:18:18 2018 (r340652) > +++ head/sys/powerpc/include/pcb.h Mon Nov 19 23:54:49 2018 (r340653) > @@ -89,6 +89,7 @@ struct pcb { > register_t dbcr0; > } booke; > } pcb_cpu; > + vm_offset_t pcb_lastill; /* Last illegal instruction */ > }; > #endif > > > Modified: head/sys/powerpc/powerpc/exec_machdep.c > ============================================================================== > --- head/sys/powerpc/powerpc/exec_machdep.c Mon Nov 19 22:18:18 2018 (r340652) > +++ head/sys/powerpc/powerpc/exec_machdep.c Mon Nov 19 23:54:49 2018 (r340653) > @@ -94,6 +94,8 @@ __FBSDID("$FreeBSD$"); > #include <machine/trap.h> > #include <machine/vmparam.h> > > +#include <vm/pmap.h> > + > #ifdef FPU_EMU > #include <powerpc/fpu/fpu_extern.h> > #endif > @@ -1099,6 +1101,14 @@ ppc_instr_emulate(struct trapframe *frame, struct pcb > } > sig = fpu_emulate(frame, &pcb->pcb_fpu); > #endif > + if (sig == SIGILL) { > + if (pcb->pcb_lastill != frame->srr0) { > + /* Allow a second chance, in case of cache sync issues. */ > + sig = 0; > + pmap_sync_icache(PCPU_GET(curpmap), frame->srr0, 4); > + pcb->pcb_lastill = frame->srr0; > + } > + } > > return (sig); > } >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5bb6d4a3-57e0-6008-3e6a-4bad77fd3108>