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

[Peter Maydell <peter.maydell@linaro.org>]

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