diff mbox series

[1/7] target/s390x: Fix s390_probe_access for user-only

Message ID 20230109201856.3916639-2-richard.henderson@linaro.org
State Superseded
Headers show
Series target/s390x: mem_helper.c cleanups | expand

Commit Message

Richard Henderson Jan. 9, 2023, 8:18 p.m. UTC
In db9aab5783a2 we broke the contract of s390_probe_access, in that it
no longer returned an exception code, nor set __excp_addr.  Fix both.

Reported-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/mem_helper.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Comments

David Hildenbrand Jan. 11, 2023, 10:12 a.m. UTC | #1
On 09.01.23 21:18, Richard Henderson wrote:
> In db9aab5783a2 we broke the contract of s390_probe_access, in that it
> no longer returned an exception code, nor set __excp_addr.  Fix both.
> 
> Reported-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Should we add a Fixes tag?

Reviewed-by: David Hildenbrand <david@redhat.com>
Thomas Huth March 15, 2023, 3:30 p.m. UTC | #2
On 09/01/2023 21.18, Richard Henderson wrote:
> In db9aab5783a2 we broke the contract of s390_probe_access, in that it
> no longer returned an exception code, nor set __excp_addr.  Fix both.
> 
> Reported-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/tcg/mem_helper.c | 31 ++++++++++++++++++-------------
>   1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index cb82cd1c1d..5c0a7b1961 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -138,23 +138,27 @@ typedef struct S390Access {
>    * For !CONFIG_USER_ONLY, the TEC is stored stored to env->tlb_fill_tec.
>    * For CONFIG_USER_ONLY, the faulting address is stored to env->__excp_addr.
>    */
> -static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
> -                             MMUAccessType access_type, int mmu_idx,
> -                             bool nonfault, void **phost, uintptr_t ra)
> +static inline int s390_probe_access(CPUArchState *env, target_ulong addr,
> +                                    int size, MMUAccessType access_type,
> +                                    int mmu_idx, bool nonfault,
> +                                    void **phost, uintptr_t ra)
>   {
> -#if defined(CONFIG_USER_ONLY)
> -    return probe_access_flags(env, addr, access_type, mmu_idx,
> -                              nonfault, phost, ra);
> -#else
> -    int flags;
> +    int flags = probe_access_flags(env, addr, access_type, mmu_idx,
> +                                   nonfault, phost, ra);
>   
> -    env->tlb_fill_exc = 0;
> -    flags = probe_access_flags(env, addr, access_type, mmu_idx, nonfault, phost,
> -                               ra);
> -    if (env->tlb_fill_exc) {
> +    if (unlikely(flags & TLB_INVALID_MASK)) {
> +        assert(!nonfault);

  Hi Richard,

qemu-system-s390x now triggers on this assert() if running the
kvm-unit-tests in TCG mode:

$ qemu-system-s390x -nographic -kernel s390x/mvpg.elf
...
PASS: mvpg: exceptions: specification: Key Function Control value 27
PASS: mvpg: exceptions: specification: Key Function Control value 28
PASS: mvpg: exceptions: specification: Key Function Control value 29
PASS: mvpg: exceptions: specification: Key Function Control value 30
PASS: mvpg: exceptions: specification: Key Function Control value 31
qemu-system-s390x: ../../devel/qemu/target/s390x/tcg/mem_helper.c:152: s390_probe_access: Assertion `!nonfault' failed.
Aborted (core dumped)

If I've got the test right, it tries to do a "mvpg" with an illegal
address and expects to see an addressing exception.

It seems to work when I remove the assert() statement. Could we maybe
replace it with a qemu_log_mask(LOG_GUEST_ERROR, ...) instead?

> +#ifdef CONFIG_USER_ONLY
> +        /* Address is in TEC in system mode; see s390_cpu_record_sigsegv. */
> +        env->__excp_addr = addr & TARGET_PAGE_MASK;
> +        return (page_get_flags(addr) & PAGE_VALID
> +                ? PGM_PROTECTION : PGM_ADDRESSING);
> +#else
>           return env->tlb_fill_exc;
> +#endif
>       }

  Thomas
Richard Henderson March 16, 2023, 2:29 p.m. UTC | #3
On 3/15/23 08:30, Thomas Huth wrote:
> On 09/01/2023 21.18, Richard Henderson wrote:
>> In db9aab5783a2 we broke the contract of s390_probe_access, in that it
>> no longer returned an exception code, nor set __excp_addr.  Fix both.
>>
>> Reported-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/s390x/tcg/mem_helper.c | 31 ++++++++++++++++++-------------
>>   1 file changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
>> index cb82cd1c1d..5c0a7b1961 100644
>> --- a/target/s390x/tcg/mem_helper.c
>> +++ b/target/s390x/tcg/mem_helper.c
>> @@ -138,23 +138,27 @@ typedef struct S390Access {
>>    * For !CONFIG_USER_ONLY, the TEC is stored stored to env->tlb_fill_tec.
>>    * For CONFIG_USER_ONLY, the faulting address is stored to env->__excp_addr.
>>    */
>> -static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
>> -                             MMUAccessType access_type, int mmu_idx,
>> -                             bool nonfault, void **phost, uintptr_t ra)
>> +static inline int s390_probe_access(CPUArchState *env, target_ulong addr,
>> +                                    int size, MMUAccessType access_type,
>> +                                    int mmu_idx, bool nonfault,
>> +                                    void **phost, uintptr_t ra)
>>   {
>> -#if defined(CONFIG_USER_ONLY)
>> -    return probe_access_flags(env, addr, access_type, mmu_idx,
>> -                              nonfault, phost, ra);
>> -#else
>> -    int flags;
>> +    int flags = probe_access_flags(env, addr, access_type, mmu_idx,
>> +                                   nonfault, phost, ra);
>> -    env->tlb_fill_exc = 0;
>> -    flags = probe_access_flags(env, addr, access_type, mmu_idx, nonfault, phost,
>> -                               ra);
>> -    if (env->tlb_fill_exc) {
>> +    if (unlikely(flags & TLB_INVALID_MASK)) {
>> +        assert(!nonfault);
> 
>   Hi Richard,
> 
> qemu-system-s390x now triggers on this assert() if running the
> kvm-unit-tests in TCG mode:
> 
> $ qemu-system-s390x -nographic -kernel s390x/mvpg.elf
> ...
> PASS: mvpg: exceptions: specification: Key Function Control value 27
> PASS: mvpg: exceptions: specification: Key Function Control value 28
> PASS: mvpg: exceptions: specification: Key Function Control value 29
> PASS: mvpg: exceptions: specification: Key Function Control value 30
> PASS: mvpg: exceptions: specification: Key Function Control value 31
> qemu-system-s390x: ../../devel/qemu/target/s390x/tcg/mem_helper.c:152: s390_probe_access: 
> Assertion `!nonfault' failed.
> Aborted (core dumped)
> 
> If I've got the test right, it tries to do a "mvpg" with an illegal
> address and expects to see an addressing exception.
> 
> It seems to work when I remove the assert() statement. Could we maybe
> replace it with a qemu_log_mask(LOG_GUEST_ERROR, ...) instead?

This is a pre-coffee guess, but the assert looks backward.

We should only arrive there if nonfault was true for the probe (otherwise the probe would 
have raised the exception directly).  I would think we could just remove the assert.


r~
diff mbox series

Patch

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index cb82cd1c1d..5c0a7b1961 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -138,23 +138,27 @@  typedef struct S390Access {
  * For !CONFIG_USER_ONLY, the TEC is stored stored to env->tlb_fill_tec.
  * For CONFIG_USER_ONLY, the faulting address is stored to env->__excp_addr.
  */
-static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
-                             MMUAccessType access_type, int mmu_idx,
-                             bool nonfault, void **phost, uintptr_t ra)
+static inline int s390_probe_access(CPUArchState *env, target_ulong addr,
+                                    int size, MMUAccessType access_type,
+                                    int mmu_idx, bool nonfault,
+                                    void **phost, uintptr_t ra)
 {
-#if defined(CONFIG_USER_ONLY)
-    return probe_access_flags(env, addr, access_type, mmu_idx,
-                              nonfault, phost, ra);
-#else
-    int flags;
+    int flags = probe_access_flags(env, addr, access_type, mmu_idx,
+                                   nonfault, phost, ra);
 
-    env->tlb_fill_exc = 0;
-    flags = probe_access_flags(env, addr, access_type, mmu_idx, nonfault, phost,
-                               ra);
-    if (env->tlb_fill_exc) {
+    if (unlikely(flags & TLB_INVALID_MASK)) {
+        assert(!nonfault);
+#ifdef CONFIG_USER_ONLY
+        /* Address is in TEC in system mode; see s390_cpu_record_sigsegv. */
+        env->__excp_addr = addr & TARGET_PAGE_MASK;
+        return (page_get_flags(addr) & PAGE_VALID
+                ? PGM_PROTECTION : PGM_ADDRESSING);
+#else
         return env->tlb_fill_exc;
+#endif
     }
 
+#ifndef CONFIG_USER_ONLY
     if (unlikely(flags & TLB_WATCHPOINT)) {
         /* S390 does not presently use transaction attributes. */
         cpu_check_watchpoint(env_cpu(env), addr, size,
@@ -162,8 +166,9 @@  static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
                              (access_type == MMU_DATA_STORE
                               ? BP_MEM_WRITE : BP_MEM_READ), ra);
     }
-    return 0;
 #endif
+
+    return 0;
 }
 
 static int access_prepare_nf(S390Access *access, CPUS390XState *env,