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

[Richard Henderson]

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~