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

Maintainers: Havard Skinnemoen <hskinnemoen@google.com>, Thomas Huth <thuth@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Riku Voipio <riku.voipio@iki.fi>, Cornelia Huck <cohuck@redhat.com>, Laurent Vivier <lvivier@redhat.com>, David Hildenbrand <david@redhat.com>, Jason Wang <jasowang@redhat.com>, Tyrone Ting <kfting@nuvoton.com>, Richard Henderson <richard.henderson@linaro.org>, Peter Maydell <peter.maydell@linaro.org>, Laurent Vivier <laurent@vivier.eu>
There is a newer version of this series
[PULL 26/40] linux-user/aarch64: Pass syndrome to EXC_*_ABORT
Posted by Peter Maydell 4 years, 2 months ago
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(-)

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 = {};
 
     /*
-- 
2.20.1


Re: [PULL 26/40] linux-user/aarch64: Pass syndrome to EXC_*_ABORT
Posted by Laurent Vivier 4 years, 2 months ago
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 = {};
>  
>      /*
> 


Re: [PULL 26/40] linux-user/aarch64: Pass syndrome to EXC_*_ABORT
Posted by Laurent Vivier 4 years, 1 month ago
Richard,

do you have any idea how to fix this problem?

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 = {};
>>  
>>      /*
>>
> 
> 


Re: [PULL 26/40] linux-user/aarch64: Pass syndrome to EXC_*_ABORT
Posted by Richard Henderson 4 years, 1 month ago
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 = {};
>>>   
>>>       /*
>>>
>>
>>
>