diff mbox series

[v2,4/8] linux-user/arm: Report SIGBUS and SIGSEGV correctly

Message ID 20210821195958.41312-5-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: Fix insn exception priorities | expand

Commit Message

Richard Henderson Aug. 21, 2021, 7:59 p.m. UTC
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(-)

-- 
2.25.1

Comments

Peter Maydell Aug. 26, 2021, 1:31 p.m. UTC | #1
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
Richard Henderson Sept. 8, 2021, 9:19 a.m. UTC | #2
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~
Richard Henderson Sept. 19, 2021, 10:23 p.m. UTC | #3
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 mbox series

Patch

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: