[PATCH 0/2] target/s390x: s390_probe_access fixes

[PATCH 0/2] target/s390x: s390_probe_access fixes

[Richard Henderson]

First, as pointed out by David; second by inspection.

I really wish there were a better way to structure this,
but alas, I don't see any alternatives that aren't just
different but similar amounts of ugly.


r~


Richard Henderson (2):
  Revert "target/s390x: Use probe_access_flags in s390_probe_access"
  target/s390x: Align __excp_addr in s390_probe_access

 target/s390x/tcg/mem_helper.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/2] Revert "target/s390x: Use probe_access_flags in s390_probe_access"

[Richard Henderson]

This reverts commit db9aab5783a2fb62250e12f0c4cfed5e1778c189.

This patch breaks the contract of s390_probe_access, in that
it no longer returns an exception code, nor set __excp_addr.

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

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index fc52aa128b..4c0f8baa39 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -142,12 +142,20 @@ 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)
 {
-#if defined(CONFIG_USER_ONLY)
-    return probe_access_flags(env, addr, access_type, mmu_idx,
-                              nonfault, phost, ra);
-#else
     int flags;
 
+#if defined(CONFIG_USER_ONLY)
+    flags = page_get_flags(addr);
+    if (!(flags & (access_type == MMU_DATA_LOAD ?  PAGE_READ : PAGE_WRITE_ORG))) {
+        env->__excp_addr = addr;
+        flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING;
+        if (nonfault) {
+            return flags;
+        }
+        tcg_s390_program_interrupt(env, flags, ra);
+    }
+    *phost = g2h(env_cpu(env), addr);
+#else
     /*
      * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or haddr==NULL
      * to detect if there was an exception during tlb_fill().
@@ -166,8 +174,8 @@ 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,
-- 
2.34.1

[PATCH 2/2] target/s390x: Align __excp_addr in s390_probe_access

[Richard Henderson]

Per the comment in s390_cpu_record_sigsegv, the saved address
is always page aligned.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/mem_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 4c0f8baa39..19ea7d2f8d 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -147,7 +147,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
 #if defined(CONFIG_USER_ONLY)
     flags = page_get_flags(addr);
     if (!(flags & (access_type == MMU_DATA_LOAD ?  PAGE_READ : PAGE_WRITE_ORG))) {
-        env->__excp_addr = addr;
+        env->__excp_addr = addr & TARGET_PAGE_MASK;
         flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING;
         if (nonfault) {
             return flags;
-- 
2.34.1

Re: [PATCH 0/2] target/s390x: s390_probe_access fixes

[David Hildenbrand]

On 23.08.22 23:38, Richard Henderson wrote:
> First, as pointed out by David; second by inspection.
> 
> I really wish there were a better way to structure this,
> but alas, I don't see any alternatives that aren't just
> different but similar amounts of ugly.
> 

The only feasible way would be having a arch-specific callback from
inside the probe code that would, similarly to tlb_fill code for !USER
store these values in the cpu environment -- then we could similarly
just look them up after the probe access.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] Revert "target/s390x: Use probe_access_flags in s390_probe_access"

[David Hildenbrand]

On 23.08.22 23:38, Richard Henderson wrote:
> This reverts commit db9aab5783a2fb62250e12f0c4cfed5e1778c189.
> 
> This patch breaks the contract of s390_probe_access, in that
> it no longer returns an exception code, nor set __excp_addr.
> 
> Reported-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/tcg/mem_helper.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index fc52aa128b..4c0f8baa39 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -142,12 +142,20 @@ 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)
>  {
> -#if defined(CONFIG_USER_ONLY)
> -    return probe_access_flags(env, addr, access_type, mmu_idx,
> -                              nonfault, phost, ra);
> -#else
>      int flags;
>  
> +#if defined(CONFIG_USER_ONLY)
> +    flags = page_get_flags(addr);
> +    if (!(flags & (access_type == MMU_DATA_LOAD ?  PAGE_READ : PAGE_WRITE_ORG))) {
> +        env->__excp_addr = addr;
> +        flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING;
> +        if (nonfault) {
> +            return flags;
> +        }
> +        tcg_s390_program_interrupt(env, flags, ra);
> +    }
> +    *phost = g2h(env_cpu(env), addr);
> +#else
>      /*
>       * For !CONFIG_USER_ONLY, we cannot rely on TLB_INVALID_MASK or haddr==NULL
>       * to detect if there was an exception during tlb_fill().
> @@ -166,8 +174,8 @@ 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,

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] target/s390x: Align __excp_addr in s390_probe_access

[David Hildenbrand]

On 23.08.22 23:38, Richard Henderson wrote:
> Per the comment in s390_cpu_record_sigsegv, the saved address
> is always page aligned.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/tcg/mem_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index 4c0f8baa39..19ea7d2f8d 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -147,7 +147,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
>  #if defined(CONFIG_USER_ONLY)
>      flags = page_get_flags(addr);
>      if (!(flags & (access_type == MMU_DATA_LOAD ?  PAGE_READ : PAGE_WRITE_ORG))) {
> -        env->__excp_addr = addr;
> +        env->__excp_addr = addr & TARGET_PAGE_MASK;
>          flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING;
>          if (nonfault) {
>              return flags;

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb