| Message ID | 20210821195958.41312-5-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Fix insn exception priorities | expand |
On Sat, 21 Aug 2021 at 21:03, Richard Henderson <richard.henderson@linaro.org> wrote: > > Pull the fault information from where we placed it, in > arm_cpu_tlb_fill and arm_cpu_do_unaligned_access. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > Pulled out from the larger unaligned data patch set. > For short-form FSC, pc misalignment is reported in the same way. > --- > linux-user/arm/cpu_loop.c | 39 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c > + case 0x3: /* Access flag fault, level 1 */ > + case 0x6: /* Access flag fault, level 2 */ > + case 0x9: /* Domain fault, level 1 */ > + case 0xb: /* Domain fault, level 2 */ > + case 0xd: /* Permision fault, level 1 */ > + case 0xf: /* Permision fault, level 2 */ "Permission" > + si_signo = TARGET_SIGSEGV; > + si_code = TARGET_SEGV_ACCERR; > + break; > + case 0x5: /* Translation fault, level 1 */ > + case 0x7: /* Translation fault, level 2 */ > + si_signo = TARGET_SIGSEGV; > + si_code = TARGET_SEGV_MAPERR; > + break; Side note: for cases like this where we can tell MAPERR from ACCERR based on info the exception handler passes to us, should we prefer that or the "check the page flags" approach that force_sigsegv_for_addr() takes ? I feel like the former is nicer, because in a multithreaded program some other thread might have changed whether the page is mapped between our taking the fault and getting here. But maybe that's always racy... Anyway, other than the typo, Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 8/26/21 3:31 PM, Peter Maydell wrote: > Side note: for cases like this where we can tell MAPERR from > ACCERR based on info the exception handler passes to us, should > we prefer that or the "check the page flags" approach that > force_sigsegv_for_addr() takes ? I feel like the former is > nicer, because in a multithreaded program some other thread > might have changed whether the page is mapped between our taking > the fault and getting here. But maybe that's always racy... Both ways are racy. After having played with SIGBUS, what I believe should happen is that we clean up the signal handling such that we can pass through the host MAPERR/ACCERR, remapping any fault address, after filtering the write-protect case that we care about. I'm not sure how much effort it would be to do that. Certainly the test matrix is pretty darn large. But perhaps it would simplify the huge SIGBUS patch set, and thus make it all worthwhile. r~
On 8/26/21 6:31 AM, Peter Maydell wrote: >> + si_signo = TARGET_SIGSEGV; >> + si_code = TARGET_SEGV_ACCERR; >> + break; >> + case 0x5: /* Translation fault, level 1 */ >> + case 0x7: /* Translation fault, level 2 */ >> + si_signo = TARGET_SIGSEGV; >> + si_code = TARGET_SEGV_MAPERR; >> + break; > > Side note: for cases like this where we can tell MAPERR from > ACCERR based on info the exception handler passes to us, should > we prefer that or the "check the page flags" approach that > force_sigsegv_for_addr() takes ? FYI, the v3 version of the sigsegv+siginfo patch set makes is vastly easier on the target code. For the most part the target code goes away entirely. For the specific case of Arm (both a32 and a64), we retain it because we are supposed to report the ESR and FAR as part of the signal frame. I'll note that a64 isn't filling in the esr_context and far_context structures. The latter was invented for MTE, I believe, where the normal si_addr is untagged. I should have a double-check around those at some point... r~
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c index d4b4f0c71f..5731d3c937 100644 --- a/linux-user/arm/cpu_loop.c +++ b/linux-user/arm/cpu_loop.c @@ -24,6 +24,7 @@ #include "cpu_loop-common.h" #include "signal-common.h" #include "semihosting/common-semi.h" +#include "target/arm/syndrome.h" #define get_user_code_u32(x, gaddr, env) \ ({ abi_long __r = get_user_u32((x), (gaddr)); \ @@ -279,8 +280,8 @@ static bool emulate_arm_fpa11(CPUARMState *env, uint32_t opcode) void cpu_loop(CPUARMState *env) { CPUState *cs = env_cpu(env); - int trapnr; - unsigned int n, insn; + int trapnr, si_signo, si_code; + unsigned int n, insn, ec, fsc; abi_ulong ret; for(;;) { @@ -422,9 +423,37 @@ void cpu_loop(CPUARMState *env) break; case EXCP_PREFETCH_ABORT: case EXCP_DATA_ABORT: - /* XXX: check env->error_code */ - force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MAPERR, - env->exception.vaddress); + /* + * For user-only we don't set TTBCR_EAE, so we always get + * short-form FSC, which then tells us to look at the FSR. + */ + ec = syn_get_ec(env->exception.syndrome); + assert(ec == EC_DATAABORT || ec == EC_INSNABORT); + fsc = extract32(env->exception.syndrome, 0, 6); + assert(fsc == 0x3f); + switch (env->exception.fsr & 0x1f) { + case 0x1: /* Alignment */ + si_signo = TARGET_SIGBUS; + si_code = TARGET_BUS_ADRALN; + break; + case 0x3: /* Access flag fault, level 1 */ + case 0x6: /* Access flag fault, level 2 */ + case 0x9: /* Domain fault, level 1 */ + case 0xb: /* Domain fault, level 2 */ + case 0xd: /* Permision fault, level 1 */ + case 0xf: /* Permision fault, level 2 */ + si_signo = TARGET_SIGSEGV; + si_code = TARGET_SEGV_ACCERR; + break; + case 0x5: /* Translation fault, level 1 */ + case 0x7: /* Translation fault, level 2 */ + si_signo = TARGET_SIGSEGV; + si_code = TARGET_SEGV_MAPERR; + break; + default: + g_assert_not_reached(); + } + force_sig_fault(si_signo, si_code, env->exception.vaddress); break; case EXCP_DEBUG: case EXCP_BKPT:
Pull the fault information from where we placed it, in arm_cpu_tlb_fill and arm_cpu_do_unaligned_access. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- Pulled out from the larger unaligned data patch set. For short-form FSC, pc misalignment is reported in the same way. --- linux-user/arm/cpu_loop.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)