[for-6.2,03/43] target/arm: Implement do_unaligned_access for user-only

Message ID 20210729004647.282017-4-richard.henderson@linaro.org
State Superseded
Headers show
Series
  • Unaligned accesses for user-only
Related show

Commit Message

Richard Henderson July 29, 2021, 12:46 a.m.
Cc: qemu-arm@nongnu.org
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 linux-user/aarch64/cpu_loop.c |  4 ++++
 linux-user/arm/cpu_loop.c     | 43 +++++++++++++++++++++++++++--------
 target/arm/cpu.c              |  2 +-
 target/arm/cpu_tcg.c          |  2 +-
 4 files changed, 40 insertions(+), 11 deletions(-)

-- 
2.25.1

Comments

Peter Maydell July 29, 2021, 1:14 p.m. | #1
On Thu, 29 Jul 2021 at 01:47, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Cc: qemu-arm@nongnu.org

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  linux-user/aarch64/cpu_loop.c |  4 ++++

>  linux-user/arm/cpu_loop.c     | 43 +++++++++++++++++++++++++++--------

>  target/arm/cpu.c              |  2 +-

>  target/arm/cpu_tcg.c          |  2 +-

>  4 files changed, 40 insertions(+), 11 deletions(-)

>

> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c

> index ee72a1c20f..998831f87f 100644

> --- a/linux-user/aarch64/cpu_loop.c

> +++ b/linux-user/aarch64/cpu_loop.c

> @@ -137,6 +137,10 @@ void cpu_loop(CPUARMState *env)

>              case 0x11: /* Synchronous Tag Check Fault */

>                  info.si_code = TARGET_SEGV_MTESERR;

>                  break;

> +            case 0x21: /* Alignment fault */

> +                info.si_signo = TARGET_SIGBUS;

> +                info.si_code = TARGET_BUS_ADRALN;

> +                break;

>              default:

>                  g_assert_not_reached();

>              }

> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c

> index 69632d15be..da7da6a0c1 100644

> --- a/linux-user/arm/cpu_loop.c

> +++ b/linux-user/arm/cpu_loop.c

> @@ -23,6 +23,7 @@

>  #include "elf.h"

>  #include "cpu_loop-common.h"

>  #include "semihosting/common-semi.h"

> +#include "target/arm/syndrome.h"


Not a huge fan of linux-user files pulling in target/arm headers, but
I guess we do it already in aarch64/cpu_loop.c. (Though that is afaict
the only other place ATM...)

>

>  #define get_user_code_u32(x, gaddr, env)                \

>      ({ abi_long __r = get_user_u32((x), (gaddr));       \

> @@ -286,9 +287,8 @@ void cpu_loop(CPUARMState *env)

>  {

>      CPUState *cs = env_cpu(env);

>      int trapnr;

> -    unsigned int n, insn;

> +    unsigned int n, insn, ec, fsc;

>      target_siginfo_t info;

> -    uint32_t addr;

>      abi_ulong ret;

>

>      for(;;) {

> @@ -437,15 +437,40 @@ void cpu_loop(CPUARMState *env)

>              break;

>          case EXCP_PREFETCH_ABORT:

>          case EXCP_DATA_ABORT:

> -            addr = env->exception.vaddress;

> -            {

> -                info.si_signo = TARGET_SIGSEGV;

> -                info.si_errno = 0;

> -                /* XXX: check env->error_code */

> +            info.si_signo = TARGET_SIGSEGV;

> +            info.si_errno = 0;

> +            info._sifields._sigfault._addr = env->exception.vaddress;

> +            /*

> +             * We should only arrive here with EC in {DATAABORT, INSNABORT},

> +             * and short-form FSC, which then tells us to look at the FSR.

> +             * ??? arm_cpu_reset never sets TTBCR_EAE, so we always get

> +             * short-form FSC.

> +             */

> +            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 */

> +                info.si_signo = TARGET_SIGBUS;

> +                info.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 */

> +                info.si_code = TARGET_SEGV_ACCERR;

> +                break;

> +            case 0x5: /* Translation fault, level 1 */

> +            case 0x7: /* Translation fault, level 2 */

>                  info.si_code = TARGET_SEGV_MAPERR;

> -                info._sifields._sigfault._addr = addr;

> -                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

> +                break;

> +            default:

> +                g_assert_not_reached();

>              }

> +            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

>              break;


It's slightly sad that we start off with a nicely symbolic
ArmMMUFaultInfo type enum value, carefully encode it into a
numeric value and then have to switch on the numeric value here,
but I can see why we end up this way...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM
Richard Henderson July 29, 2021, 6:51 p.m. | #2
On 7/29/21 3:14 AM, Peter Maydell wrote:
> On Thu, 29 Jul 2021 at 01:47, Richard Henderson

> <richard.henderson@linaro.org> wrote:

>>

>> Cc: qemu-arm@nongnu.org

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> ---

>>   linux-user/aarch64/cpu_loop.c |  4 ++++

>>   linux-user/arm/cpu_loop.c     | 43 +++++++++++++++++++++++++++--------

>>   target/arm/cpu.c              |  2 +-

>>   target/arm/cpu_tcg.c          |  2 +-

>>   4 files changed, 40 insertions(+), 11 deletions(-)

>>

>> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c

>> index ee72a1c20f..998831f87f 100644

>> --- a/linux-user/aarch64/cpu_loop.c

>> +++ b/linux-user/aarch64/cpu_loop.c

>> @@ -137,6 +137,10 @@ void cpu_loop(CPUARMState *env)

>>               case 0x11: /* Synchronous Tag Check Fault */

>>                   info.si_code = TARGET_SEGV_MTESERR;

>>                   break;

>> +            case 0x21: /* Alignment fault */

>> +                info.si_signo = TARGET_SIGBUS;

>> +                info.si_code = TARGET_BUS_ADRALN;

>> +                break;

>>               default:

>>                   g_assert_not_reached();

>>               }

>> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c

>> index 69632d15be..da7da6a0c1 100644

>> --- a/linux-user/arm/cpu_loop.c

>> +++ b/linux-user/arm/cpu_loop.c

>> @@ -23,6 +23,7 @@

>>   #include "elf.h"

>>   #include "cpu_loop-common.h"

>>   #include "semihosting/common-semi.h"

>> +#include "target/arm/syndrome.h"

> 

> Not a huge fan of linux-user files pulling in target/arm headers, but

> I guess we do it already in aarch64/cpu_loop.c. (Though that is afaict

> the only other place ATM...)

> 

>>

>>   #define get_user_code_u32(x, gaddr, env)                \

>>       ({ abi_long __r = get_user_u32((x), (gaddr));       \

>> @@ -286,9 +287,8 @@ void cpu_loop(CPUARMState *env)

>>   {

>>       CPUState *cs = env_cpu(env);

>>       int trapnr;

>> -    unsigned int n, insn;

>> +    unsigned int n, insn, ec, fsc;

>>       target_siginfo_t info;

>> -    uint32_t addr;

>>       abi_ulong ret;

>>

>>       for(;;) {

>> @@ -437,15 +437,40 @@ void cpu_loop(CPUARMState *env)

>>               break;

>>           case EXCP_PREFETCH_ABORT:

>>           case EXCP_DATA_ABORT:

>> -            addr = env->exception.vaddress;

>> -            {

>> -                info.si_signo = TARGET_SIGSEGV;

>> -                info.si_errno = 0;

>> -                /* XXX: check env->error_code */

>> +            info.si_signo = TARGET_SIGSEGV;

>> +            info.si_errno = 0;

>> +            info._sifields._sigfault._addr = env->exception.vaddress;

>> +            /*

>> +             * We should only arrive here with EC in {DATAABORT, INSNABORT},

>> +             * and short-form FSC, which then tells us to look at the FSR.

>> +             * ??? arm_cpu_reset never sets TTBCR_EAE, so we always get

>> +             * short-form FSC.

>> +             */

>> +            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 */

>> +                info.si_signo = TARGET_SIGBUS;

>> +                info.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 */

>> +                info.si_code = TARGET_SEGV_ACCERR;

>> +                break;

>> +            case 0x5: /* Translation fault, level 1 */

>> +            case 0x7: /* Translation fault, level 2 */

>>                   info.si_code = TARGET_SEGV_MAPERR;

>> -                info._sifields._sigfault._addr = addr;

>> -                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

>> +                break;

>> +            default:

>> +                g_assert_not_reached();

>>               }

>> +            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

>>               break;

> 

> It's slightly sad that we start off with a nicely symbolic

> ArmMMUFaultInfo type enum value, carefully encode it into a

> numeric value and then have to switch on the numeric value here,

> but I can see why we end up this way...


We don't have to leave it that way.

We could move the ARMMMUFaultInfo out of internals.h, create special user-only copies of 
arm_cpu_tlb_fill and arm_cpu_do_unaligned_access, create a new function to raise the MTE 
exception, and place the proper enumeraor into env->error_code instead of the hw syndrome.

What we have seemed cleaner on the target/arm/ side at the time.


r~

Patch

diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index ee72a1c20f..998831f87f 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -137,6 +137,10 @@  void cpu_loop(CPUARMState *env)
             case 0x11: /* Synchronous Tag Check Fault */
                 info.si_code = TARGET_SEGV_MTESERR;
                 break;
+            case 0x21: /* Alignment fault */
+                info.si_signo = TARGET_SIGBUS;
+                info.si_code = TARGET_BUS_ADRALN;
+                break;
             default:
                 g_assert_not_reached();
             }
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 69632d15be..da7da6a0c1 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -23,6 +23,7 @@ 
 #include "elf.h"
 #include "cpu_loop-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));       \
@@ -286,9 +287,8 @@  void cpu_loop(CPUARMState *env)
 {
     CPUState *cs = env_cpu(env);
     int trapnr;
-    unsigned int n, insn;
+    unsigned int n, insn, ec, fsc;
     target_siginfo_t info;
-    uint32_t addr;
     abi_ulong ret;
 
     for(;;) {
@@ -437,15 +437,40 @@  void cpu_loop(CPUARMState *env)
             break;
         case EXCP_PREFETCH_ABORT:
         case EXCP_DATA_ABORT:
-            addr = env->exception.vaddress;
-            {
-                info.si_signo = TARGET_SIGSEGV;
-                info.si_errno = 0;
-                /* XXX: check env->error_code */
+            info.si_signo = TARGET_SIGSEGV;
+            info.si_errno = 0;
+            info._sifields._sigfault._addr = env->exception.vaddress;
+            /*
+             * We should only arrive here with EC in {DATAABORT, INSNABORT},
+             * and short-form FSC, which then tells us to look at the FSR.
+             * ??? arm_cpu_reset never sets TTBCR_EAE, so we always get
+             * short-form FSC.
+             */
+            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 */
+                info.si_signo = TARGET_SIGBUS;
+                info.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 */
+                info.si_code = TARGET_SEGV_ACCERR;
+                break;
+            case 0x5: /* Translation fault, level 1 */
+            case 0x7: /* Translation fault, level 2 */
                 info.si_code = TARGET_SEGV_MAPERR;
-                info._sifields._sigfault._addr = addr;
-                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+                break;
+            default:
+                g_assert_not_reached();
             }
+            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
             break;
         case EXCP_DEBUG:
         case EXCP_BKPT:
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2866dd7658..de0d968d76 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1987,11 +1987,11 @@  static const struct TCGCPUOps arm_tcg_ops = {
     .cpu_exec_interrupt = arm_cpu_exec_interrupt,
     .tlb_fill = arm_cpu_tlb_fill,
     .debug_excp_handler = arm_debug_excp_handler,
+    .do_unaligned_access = arm_cpu_do_unaligned_access,
 
 #if !defined(CONFIG_USER_ONLY)
     .do_interrupt = arm_cpu_do_interrupt,
     .do_transaction_failed = arm_cpu_do_transaction_failed,
-    .do_unaligned_access = arm_cpu_do_unaligned_access,
     .adjust_watchpoint_address = arm_adjust_watchpoint_address,
     .debug_check_watchpoint = arm_debug_check_watchpoint,
     .debug_check_breakpoint = arm_debug_check_breakpoint,
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index ed444bf436..1b91fdc890 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -904,11 +904,11 @@  static const struct TCGCPUOps arm_v7m_tcg_ops = {
     .cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt,
     .tlb_fill = arm_cpu_tlb_fill,
     .debug_excp_handler = arm_debug_excp_handler,
+    .do_unaligned_access = arm_cpu_do_unaligned_access,
 
 #if !defined(CONFIG_USER_ONLY)
     .do_interrupt = arm_v7m_cpu_do_interrupt,
     .do_transaction_failed = arm_cpu_do_transaction_failed,
-    .do_unaligned_access = arm_cpu_do_unaligned_access,
     .adjust_watchpoint_address = arm_adjust_watchpoint_address,
     .debug_check_watchpoint = arm_debug_check_watchpoint,
     .debug_check_breakpoint = arm_debug_check_breakpoint,