Re: [PULL 26/40] linux-user/aarch64: Pass syndrome to EXC_*_ABORT

[Richard Henderson]

On 3/19/21 1:19 PM, Laurent Vivier wrote:
> Richard,
>
> do you have any idea how to fix this problem?

Oops, started building ltp, then forgot about it.  Looking now.


r~

> Thanks,
> Laurent
>
> Le 12/03/2021 à 12:09, Laurent Vivier a écrit :
> > Hi,
> >
> > On 16/02/2021 17:16, Peter Maydell wrote:
> > > From: Richard Henderson <richard.henderson@linaro.org>
> > >
> > > A proper syndrome is required to fill in the proper si_code.
> > > Use page_get_flags to determine permission vs translation for user-only.
> > >
> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > Message-id: 20210212184902.1251044-27-richard.henderson@linaro.org
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >   linux-user/aarch64/cpu_loop.c | 24 +++++++++++++++++++++---
> > >   target/arm/tlb_helper.c       | 15 +++++++++------
> > >   2 files changed, 30 insertions(+), 9 deletions(-)
> >
> > While I was testing my next linux-user pull request I found this patch breaks 
> > something.
> >
> > Following LTP tests are broken:
> >
> > mmap05
> > mprotect02
> > mprotect03
> > mprotect04
> > shmat01
> >
> > with arm64/sid, arm64/trusty, arm64/bionic
> >
> > Bisecting only using mmap05 test I find this patch.
> >
> > Symptoms are:
> >
> > $ sudo unshare --time --ipc --uts --pid --fork --kill-child --mount 
> > --mount-proc --root
> > chroot/arm64/sid /opt/ltp/testcases/bin/mmap05
> > **
> > ERROR:../../../Projects/qemu/linux-user/aarch64/cpu_loop.c:141:cpu_loop: code 
> > should not
> > be reached
> > Bail out! 
> > ERROR:../../../Projects/qemu/linux-user/aarch64/cpu_loop.c:141:cpu_loop: code
> > should not be reached
> > qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x7f45c1cd9706
> >
> > Expected result is:
> >
> > mmap05      1  TPASS  :  Got SIGSEGV as expected
> >
> > Thanks,
> > Laurent
> >
> > > diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
> > > index 42b9c15f536..4e43906e66a 100644
> > > --- a/linux-user/aarch64/cpu_loop.c
> > > +++ b/linux-user/aarch64/cpu_loop.c
> > > @@ -23,6 +23,7 @@
> > >   #include "cpu_loop-common.h"
> > >   #include "qemu/guest-random.h"
> > >   #include "hw/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));       \
> > > @@ -76,7 +77,7 @@
> > >   void cpu_loop(CPUARMState *env)
> > >   {
> > >       CPUState *cs = env_cpu(env);
> > > -    int trapnr;
> > > +    int trapnr, ec, fsc;
> > >       abi_long ret;
> > >       target_siginfo_t info;
> > >   
> > > @@ -117,9 +118,26 @@ void cpu_loop(CPUARMState *env)
> > >           case EXCP_DATA_ABORT:
> > >               info.si_signo = TARGET_SIGSEGV;
> > >               info.si_errno = 0;
> > > -            /* XXX: check env->error_code */
> > > -            info.si_code = TARGET_SEGV_MAPERR;
> > >               info._sifields._sigfault._addr = env->exception.vaddress;
> > > +
> > > +            /* We should only arrive here with EC in {DATAABORT, INSNABORT}. */
> > > +            ec = syn_get_ec(env->exception.syndrome);
> > > +            assert(ec == EC_DATAABORT || ec == EC_INSNABORT);
> > > +
> > > +            /* Both EC have the same format for FSC, or close enough. */
> > > +            fsc = extract32(env->exception.syndrome, 0, 6);
> > > +            switch (fsc) {
> > > +            case 0x04 ... 0x07: /* Translation fault, level {0-3} */
> > > +                info.si_code = TARGET_SEGV_MAPERR;
> > > +                break;
> > > +            case 0x09 ... 0x0b: /* Access flag fault, level {1-3} */
> > > +            case 0x0d ... 0x0f: /* Permission fault, level {1-3} */
> > > +                info.si_code = TARGET_SEGV_ACCERR;
> > > +                break;
> > > +            default:
> > > +                g_assert_not_reached();
> > > +            }
> > > +
> > >               queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
> > >               break;
> > >           case EXCP_DEBUG:
> > > diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> > > index df85079d9f0..9609333cbdf 100644
> > > --- a/target/arm/tlb_helper.c
> > > +++ b/target/arm/tlb_helper.c
> > > @@ -154,21 +154,24 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int 
> > > size,
> > >                         bool probe, uintptr_t retaddr)
> > >   {
> > >       ARMCPU *cpu = ARM_CPU(cs);
> > > +    ARMMMUFaultInfo fi = {};
> > >   
> > >   #ifdef CONFIG_USER_ONLY
> > > -    cpu->env.exception.vaddress = address;
> > > -    if (access_type == MMU_INST_FETCH) {
> > > -        cs->exception_index = EXCP_PREFETCH_ABORT;
> > > +    int flags = page_get_flags(useronly_clean_ptr(address));
> > > +    if (flags & PAGE_VALID) {
> > > +        fi.type = ARMFault_Permission;
> > >       } else {
> > > -        cs->exception_index = EXCP_DATA_ABORT;
> > > +        fi.type = ARMFault_Translation;
> > >       }
> > > -    cpu_loop_exit_restore(cs, retaddr);
> > > +
> > > +    /* now we have a real cpu fault */
> > > +    cpu_restore_state(cs, retaddr, true);
> > > +    arm_deliver_fault(cpu, address, access_type, mmu_idx, &fi);
> > >   #else
> > >       hwaddr phys_addr;
> > >       target_ulong page_size;
> > >       int prot, ret;
> > >       MemTxAttrs attrs = {};
> > > -    ARMMMUFaultInfo fi = {};
> > >       ARMCacheAttrs cacheattrs = {};
> > >   
> > >       /*