[PATCH 0/7] target/s390x: mem_helper.c cleanups

[PATCH 0/7] target/s390x: mem_helper.c cleanups

[Richard Henderson]

I discovered this old branch sitting around from August.
I don't think I ever sent it out.


r~


Richard Henderson (7):
  target/s390x: Fix s390_probe_access for user-only
  target/s390x: Pass S390Access pointer into access_prepare
  target/s390x: Use void* for haddr in S390Access
  target/s390x: Tidy access_prepare_nf
  target/s390x: Remove TLB_NOTDIRTY workarounds
  target/s390x: Inline do_access_{get,set}_byte
  target/s390x: Hoist some computation in access_memmove

 target/s390x/tcg/mem_helper.c | 289 +++++++++++++++-------------------
 1 file changed, 126 insertions(+), 163 deletions(-)

-- 
2.34.1

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

[Richard Henderson]

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);
+#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,
-- 
2.34.1

[PATCH 2/7] target/s390x: Pass S390Access pointer into access_prepare

[Richard Henderson]

Passing a pointer from the caller down to access_prepare_nf
eliminates a structure copy.

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

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 5c0a7b1961..6a50189ef0 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -212,15 +212,14 @@ static int access_prepare_nf(S390Access *access, CPUS390XState *env,
     return 0;
 }
 
-static S390Access access_prepare(CPUS390XState *env, vaddr vaddr, int size,
-                                 MMUAccessType access_type, int mmu_idx,
-                                 uintptr_t ra)
+static inline void access_prepare(S390Access *ret, CPUS390XState *env,
+                                  vaddr vaddr, int size,
+                                  MMUAccessType access_type, int mmu_idx,
+                                  uintptr_t ra)
 {
-    S390Access ret;
-    int exc = access_prepare_nf(&ret, env, false, vaddr, size,
+    int exc = access_prepare_nf(ret, env, false, vaddr, size,
                                 access_type, mmu_idx, ra);
     assert(!exc);
-    return ret;
 }
 
 /* Helper to handle memset on a single page. */
@@ -412,9 +411,9 @@ static uint32_t do_helper_nc(CPUS390XState *env, uint32_t l, uint64_t dest,
     /* NC always processes one more byte than specified - maximum is 256 */
     l++;
 
-    srca1 = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
-    srca2 = access_prepare(env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca1, env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&srca2, env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
     for (i = 0; i < l; i++) {
         const uint8_t x = access_get_byte(env, &srca1, i, ra) &
                           access_get_byte(env, &srca2, i, ra);
@@ -446,9 +445,9 @@ static uint32_t do_helper_xc(CPUS390XState *env, uint32_t l, uint64_t dest,
     /* XC always processes one more byte than specified - maximum is 256 */
     l++;
 
-    srca1 = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
-    srca2 = access_prepare(env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca1, env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&srca2, env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
 
     /* xor with itself is the same as memset(0) */
     if (src == dest) {
@@ -487,9 +486,9 @@ static uint32_t do_helper_oc(CPUS390XState *env, uint32_t l, uint64_t dest,
     /* OC always processes one more byte than specified - maximum is 256 */
     l++;
 
-    srca1 = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
-    srca2 = access_prepare(env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca1, env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&srca2, env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
     for (i = 0; i < l; i++) {
         const uint8_t x = access_get_byte(env, &srca1, i, ra) |
                           access_get_byte(env, &srca2, i, ra);
@@ -520,8 +519,8 @@ static uint32_t do_helper_mvc(CPUS390XState *env, uint32_t l, uint64_t dest,
     /* MVC always copies one more byte than specified - maximum is 256 */
     l++;
 
-    srca = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca, env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
 
     /*
      * "When the operands overlap, the result is obtained as if the operands
@@ -559,8 +558,8 @@ void HELPER(mvcrl)(CPUS390XState *env, uint64_t l, uint64_t dest, uint64_t src)
     /* MVCRL always copies one more byte than specified - maximum is 256 */
     l++;
 
-    srca = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca, env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
 
     for (i = l - 1; i >= 0; i--) {
         uint8_t byte = access_get_byte(env, &srca, i, ra);
@@ -580,8 +579,8 @@ void HELPER(mvcin)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src)
     l++;
 
     src = wrap_address(env, src - l + 1);
-    srca = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca, env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
     for (i = 0; i < l; i++) {
         const uint8_t x = access_get_byte(env, &srca, l - i - 1, ra);
 
@@ -600,9 +599,9 @@ void HELPER(mvn)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src)
     /* MVN always copies one more byte than specified - maximum is 256 */
     l++;
 
-    srca1 = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
-    srca2 = access_prepare(env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca1, env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&srca2, env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
     for (i = 0; i < l; i++) {
         const uint8_t x = (access_get_byte(env, &srca1, i, ra) & 0x0f) |
                           (access_get_byte(env, &srca2, i, ra) & 0xf0);
@@ -623,8 +622,8 @@ void HELPER(mvo)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src)
     S390Access srca, desta;
     int i, j;
 
-    srca = access_prepare(env, src, len_src, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, len_dest, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca, env, src, len_src, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, len_dest, MMU_DATA_STORE, mmu_idx, ra);
 
     /* Handle rightmost byte */
     byte_dest = cpu_ldub_data_ra(env, dest + len_dest - 1, ra);
@@ -656,9 +655,9 @@ void HELPER(mvz)(CPUS390XState *env, uint32_t l, uint64_t dest, uint64_t src)
     /* MVZ always copies one more byte than specified - maximum is 256 */
     l++;
 
-    srca1 = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
-    srca2 = access_prepare(env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca1, env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&srca2, env, dest, l, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
     for (i = 0; i < l; i++) {
         const uint8_t x = (access_get_byte(env, &srca1, i, ra) & 0xf0) |
                           (access_get_byte(env, &srca2, i, ra) & 0x0f);
@@ -1005,8 +1004,8 @@ uint32_t HELPER(mvst)(CPUS390XState *env, uint32_t r1, uint32_t r2)
      * this point). We might over-indicate watchpoints within the pages
      * (if we ever care, we have to limit processing to a single byte).
      */
-    srca = access_prepare(env, s, len, MMU_DATA_LOAD, mmu_idx, ra);
-    desta = access_prepare(env, d, len, MMU_DATA_STORE, mmu_idx, ra);
+    access_prepare(&srca, env, s, len, MMU_DATA_LOAD, mmu_idx, ra);
+    access_prepare(&desta, env, d, len, MMU_DATA_STORE, mmu_idx, ra);
     for (i = 0; i < len; i++) {
         const uint8_t v = access_get_byte(env, &srca, i, ra);
 
@@ -1093,19 +1092,19 @@ static inline uint32_t do_mvcl(CPUS390XState *env,
         len = MIN(MIN(*srclen, -(*src | TARGET_PAGE_MASK)), len);
         *destlen -= len;
         *srclen -= len;
-        srca = access_prepare(env, *src, len, MMU_DATA_LOAD, mmu_idx, ra);
-        desta = access_prepare(env, *dest, len, MMU_DATA_STORE, mmu_idx, ra);
+        access_prepare(&srca, env, *src, len, MMU_DATA_LOAD, mmu_idx, ra);
+        access_prepare(&desta, env, *dest, len, MMU_DATA_STORE, mmu_idx, ra);
         access_memmove(env, &desta, &srca, ra);
         *src = wrap_address(env, *src + len);
         *dest = wrap_address(env, *dest + len);
     } else if (wordsize == 1) {
         /* Pad the remaining area */
         *destlen -= len;
-        desta = access_prepare(env, *dest, len, MMU_DATA_STORE, mmu_idx, ra);
+        access_prepare(&desta, env, *dest, len, MMU_DATA_STORE, mmu_idx, ra);
         access_memset(env, &desta, pad, ra);
         *dest = wrap_address(env, *dest + len);
     } else {
-        desta = access_prepare(env, *dest, len, MMU_DATA_STORE, mmu_idx, ra);
+        access_prepare(&desta, env, *dest, len, MMU_DATA_STORE, mmu_idx, ra);
 
         /* The remaining length selects the padding byte. */
         for (i = 0; i < len; (*destlen)--, i++) {
@@ -1161,16 +1160,16 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, uint32_t r2)
     while (destlen) {
         cur_len = MIN(destlen, -(dest | TARGET_PAGE_MASK));
         if (!srclen) {
-            desta = access_prepare(env, dest, cur_len, MMU_DATA_STORE, mmu_idx,
-                                   ra);
+            access_prepare(&desta, env, dest, cur_len,
+                           MMU_DATA_STORE, mmu_idx, ra);
             access_memset(env, &desta, pad, ra);
         } else {
             cur_len = MIN(MIN(srclen, -(src | TARGET_PAGE_MASK)), cur_len);
 
-            srca = access_prepare(env, src, cur_len, MMU_DATA_LOAD, mmu_idx,
-                                  ra);
-            desta = access_prepare(env, dest, cur_len, MMU_DATA_STORE, mmu_idx,
-                                   ra);
+            access_prepare(&srca, env, src, cur_len,
+                           MMU_DATA_LOAD, mmu_idx, ra);
+            access_prepare(&desta, env, dest, cur_len,
+                           MMU_DATA_STORE, mmu_idx, ra);
             access_memmove(env, &desta, &srca, ra);
             src = wrap_address(env, src + cur_len);
             srclen -= cur_len;
@@ -2329,8 +2328,8 @@ uint32_t HELPER(mvcs)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2,
         return cc;
     }
 
-    srca = access_prepare(env, a2, l, MMU_DATA_LOAD, MMU_PRIMARY_IDX, ra);
-    desta = access_prepare(env, a1, l, MMU_DATA_STORE, MMU_SECONDARY_IDX, ra);
+    access_prepare(&srca, env, a2, l, MMU_DATA_LOAD, MMU_PRIMARY_IDX, ra);
+    access_prepare(&desta, env, a1, l, MMU_DATA_STORE, MMU_SECONDARY_IDX, ra);
     access_memmove(env, &desta, &srca, ra);
     return cc;
 }
@@ -2363,9 +2362,8 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2,
     } else if (!l) {
         return cc;
     }
-
-    srca = access_prepare(env, a2, l, MMU_DATA_LOAD, MMU_SECONDARY_IDX, ra);
-    desta = access_prepare(env, a1, l, MMU_DATA_STORE, MMU_PRIMARY_IDX, ra);
+    access_prepare(&srca, env, a2, l, MMU_DATA_LOAD, MMU_SECONDARY_IDX, ra);
+    access_prepare(&desta, env, a1, l, MMU_DATA_STORE, MMU_PRIMARY_IDX, ra);
     access_memmove(env, &desta, &srca, ra);
     return cc;
 }
@@ -2706,10 +2704,12 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src,
 
     /* FIXME: Access using correct keys and AR-mode */
     if (len) {
-        S390Access srca = access_prepare(env, src, len, MMU_DATA_LOAD,
-                                         mmu_idx_from_as(src_as), ra);
-        S390Access desta = access_prepare(env, dest, len, MMU_DATA_STORE,
-                                          mmu_idx_from_as(dest_as), ra);
+        S390Access srca, desta;
+
+        access_prepare(&srca, env, src, len, MMU_DATA_LOAD,
+                       mmu_idx_from_as(src_as), ra);
+        access_prepare(&desta, env, dest, len, MMU_DATA_STORE,
+                       mmu_idx_from_as(dest_as), ra);
 
         access_memmove(env, &desta, &srca, ra);
     }
-- 
2.34.1

[PATCH 3/7] target/s390x: Use void* for haddr in S390Access

[Richard Henderson]

The interface from probe_access_flags is void*, and matching
that will be helpful.  We already rely on the gcc extension
for byte arithmetic on void*.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/mem_helper.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 6a50189ef0..0f2830f87c 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -114,8 +114,8 @@ static inline void cpu_stsize_data_ra(CPUS390XState *env, uint64_t addr,
 typedef struct S390Access {
     target_ulong vaddr1;
     target_ulong vaddr2;
-    char *haddr1;
-    char *haddr2;
+    void *haddr1;
+    void *haddr2;
     uint16_t size1;
     uint16_t size2;
     /*
@@ -268,8 +268,9 @@ static void access_memset(CPUS390XState *env, S390Access *desta,
                      desta->mmu_idx, ra);
 }
 
-static uint8_t do_access_get_byte(CPUS390XState *env, vaddr vaddr, char **haddr,
-                                  int offset, int mmu_idx, uintptr_t ra)
+static uint8_t do_access_get_byte(CPUS390XState *env, vaddr vaddr,
+                                  void **haddr, int offset,
+                                  int mmu_idx, uintptr_t ra)
 {
 #ifdef CONFIG_USER_ONLY
     return ldub_p(*haddr + offset);
@@ -301,7 +302,7 @@ static uint8_t access_get_byte(CPUS390XState *env, S390Access *access,
                               offset - access->size1, access->mmu_idx, ra);
 }
 
-static void do_access_set_byte(CPUS390XState *env, vaddr vaddr, char **haddr,
+static void do_access_set_byte(CPUS390XState *env, vaddr vaddr, void **haddr,
                                int offset, uint8_t byte, int mmu_idx,
                                uintptr_t ra)
 {
-- 
2.34.1

[PATCH 4/7] target/s390x: Tidy access_prepare_nf

[Richard Henderson]

Assign to access struct immediately, rather than waiting
until the end of the function.  This means we can pass
address of haddr struct members instead of allocating
extra space on the local stack.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/mem_helper.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 0f2830f87c..59237fe7de 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -176,39 +176,35 @@ static int access_prepare_nf(S390Access *access, CPUS390XState *env,
                              MMUAccessType access_type,
                              int mmu_idx, uintptr_t ra)
 {
-    void *haddr1, *haddr2 = NULL;
     int size1, size2, exc;
-    vaddr vaddr2 = 0;
 
     assert(size > 0 && size <= 4096);
 
     size1 = MIN(size, -(vaddr1 | TARGET_PAGE_MASK)),
     size2 = size - size1;
 
+    memset(access, 0, sizeof(*access));
+    access->vaddr1 = vaddr1;
+    access->size1 = size1;
+    access->size2 = size2;
+    access->mmu_idx = mmu_idx;
+
     exc = s390_probe_access(env, vaddr1, size1, access_type, mmu_idx, nonfault,
-                            &haddr1, ra);
-    if (exc) {
+                            &access->haddr1, ra);
+    if (unlikely(exc)) {
         return exc;
     }
     if (unlikely(size2)) {
         /* The access crosses page boundaries. */
-        vaddr2 = wrap_address(env, vaddr1 + size1);
+        vaddr vaddr2 = wrap_address(env, vaddr1 + size1);
+
+        access->vaddr2 = vaddr2;
         exc = s390_probe_access(env, vaddr2, size2, access_type, mmu_idx,
-                                nonfault, &haddr2, ra);
-        if (exc) {
+                                nonfault, &access->haddr2, ra);
+        if (unlikely(exc)) {
             return exc;
         }
     }
-
-    *access = (S390Access) {
-        .vaddr1 = vaddr1,
-        .vaddr2 = vaddr2,
-        .haddr1 = haddr1,
-        .haddr2 = haddr2,
-        .size1 = size1,
-        .size2 = size2,
-        .mmu_idx = mmu_idx
-    };
     return 0;
 }
 
-- 
2.34.1

[PATCH 5/7] target/s390x: Remove TLB_NOTDIRTY workarounds

[Richard Henderson]

When this code was written, it was using tlb_vaddr_to_host,
which does not handle TLB_DIRTY.  Since then, it has been
converted to probe_access_flags, which does.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/mem_helper.c | 74 +++++++++++------------------------
 1 file changed, 22 insertions(+), 52 deletions(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 59237fe7de..f7dc710814 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -122,11 +122,7 @@ typedef struct S390Access {
      * If we can't access the host page directly, we'll have to do I/O access
      * via ld/st helpers. These are internal details, so we store the
      * mmu idx to do the access here instead of passing it around in the
-     * helpers. Maybe, one day we can get rid of ld/st access - once we can
-     * handle TLB_NOTDIRTY differently. We don't expect these special accesses
-     * to trigger exceptions - only if we would have TLB_NOTDIRTY on LAP
-     * pages, we might trigger a new MMU translation - very unlikely that
-     * the mapping changes in between and we would trigger a fault.
+     * helpers.
      */
     int mmu_idx;
 } S390Access;
@@ -224,28 +220,14 @@ static void do_access_memset(CPUS390XState *env, vaddr vaddr, char *haddr,
                              uintptr_t ra)
 {
 #ifdef CONFIG_USER_ONLY
-    g_assert(haddr);
     memset(haddr, byte, size);
 #else
-    MemOpIdx oi = make_memop_idx(MO_UB, mmu_idx);
-    int i;
-
     if (likely(haddr)) {
         memset(haddr, byte, size);
     } else {
-        /*
-         * Do a single access and test if we can then get access to the
-         * page. This is especially relevant to speed up TLB_NOTDIRTY.
-         */
-        g_assert(size > 0);
-        cpu_stb_mmu(env, vaddr, byte, oi, ra);
-        haddr = tlb_vaddr_to_host(env, vaddr, MMU_DATA_STORE, mmu_idx);
-        if (likely(haddr)) {
-            memset(haddr + 1, byte, size - 1);
-        } else {
-            for (i = 1; i < size; i++) {
-                cpu_stb_mmu(env, vaddr + i, byte, oi, ra);
-            }
+        MemOpIdx oi = make_memop_idx(MO_UB, mmu_idx);
+        for (int i = 0; i < size; i++) {
+            cpu_stb_mmu(env, vaddr + i, byte, oi, ra);
         }
     }
 #endif
@@ -265,25 +247,18 @@ static void access_memset(CPUS390XState *env, S390Access *desta,
 }
 
 static uint8_t do_access_get_byte(CPUS390XState *env, vaddr vaddr,
-                                  void **haddr, int offset,
+                                  void *haddr, int offset,
                                   int mmu_idx, uintptr_t ra)
 {
 #ifdef CONFIG_USER_ONLY
-    return ldub_p(*haddr + offset);
+    return ldub_p(haddr + offset);
 #else
-    MemOpIdx oi = make_memop_idx(MO_UB, mmu_idx);
-    uint8_t byte;
-
-    if (likely(*haddr)) {
-        return ldub_p(*haddr + offset);
+    if (likely(haddr)) {
+        return ldub_p(haddr + offset);
+    } else {
+        MemOpIdx oi = make_memop_idx(MO_UB, mmu_idx);
+        return cpu_ldb_mmu(env, vaddr + offset, oi, ra);
     }
-    /*
-     * Do a single access and test if we can then get access to the
-     * page. This is especially relevant to speed up TLB_NOTDIRTY.
-     */
-    byte = cpu_ldb_mmu(env, vaddr + offset, oi, ra);
-    *haddr = tlb_vaddr_to_host(env, vaddr, MMU_DATA_LOAD, mmu_idx);
-    return byte;
 #endif
 }
 
@@ -291,32 +266,27 @@ static uint8_t access_get_byte(CPUS390XState *env, S390Access *access,
                                int offset, uintptr_t ra)
 {
     if (offset < access->size1) {
-        return do_access_get_byte(env, access->vaddr1, &access->haddr1,
+        return do_access_get_byte(env, access->vaddr1, access->haddr1,
                                   offset, access->mmu_idx, ra);
     }
-    return do_access_get_byte(env, access->vaddr2, &access->haddr2,
+    return do_access_get_byte(env, access->vaddr2, access->haddr2,
                               offset - access->size1, access->mmu_idx, ra);
 }
 
-static void do_access_set_byte(CPUS390XState *env, vaddr vaddr, void **haddr,
+static void do_access_set_byte(CPUS390XState *env, vaddr vaddr, void *haddr,
                                int offset, uint8_t byte, int mmu_idx,
                                uintptr_t ra)
 {
 #ifdef CONFIG_USER_ONLY
-    stb_p(*haddr + offset, byte);
+    stb_p(haddr + offset, byte);
 #else
-    MemOpIdx oi = make_memop_idx(MO_UB, mmu_idx);
 
-    if (likely(*haddr)) {
-        stb_p(*haddr + offset, byte);
-        return;
+    if (likely(haddr)) {
+        stb_p(haddr + offset, byte);
+    } else {
+        MemOpIdx oi = make_memop_idx(MO_UB, mmu_idx);
+        cpu_stb_mmu(env, vaddr + offset, byte, oi, ra);
     }
-    /*
-     * Do a single access and test if we can then get access to the
-     * page. This is especially relevant to speed up TLB_NOTDIRTY.
-     */
-    cpu_stb_mmu(env, vaddr + offset, byte, oi, ra);
-    *haddr = tlb_vaddr_to_host(env, vaddr, MMU_DATA_STORE, mmu_idx);
 #endif
 }
 
@@ -324,10 +294,10 @@ static void access_set_byte(CPUS390XState *env, S390Access *access,
                             int offset, uint8_t byte, uintptr_t ra)
 {
     if (offset < access->size1) {
-        do_access_set_byte(env, access->vaddr1, &access->haddr1, offset, byte,
+        do_access_set_byte(env, access->vaddr1, access->haddr1, offset, byte,
                            access->mmu_idx, ra);
     } else {
-        do_access_set_byte(env, access->vaddr2, &access->haddr2,
+        do_access_set_byte(env, access->vaddr2, access->haddr2,
                            offset - access->size1, byte, access->mmu_idx, ra);
     }
 }
-- 
2.34.1

[PATCH 6/7] target/s390x: Inline do_access_{get,set}_byte

[Richard Henderson]

Inline into the parent functions with a simple test
to select the page, and a new define to remove ifdefs.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/mem_helper.c | 70 +++++++++++++++--------------------
 1 file changed, 30 insertions(+), 40 deletions(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index f7dc710814..92eb6564c3 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -35,6 +35,12 @@
 #include "hw/boards.h"
 #endif
 
+#ifdef CONFIG_USER_ONLY
+# define user_or_likely(X)    true
+#else
+# define user_or_likely(X)    likely(X)
+#endif
+
 /*****************************************************************************/
 /* Softmmu support */
 
@@ -246,59 +252,43 @@ static void access_memset(CPUS390XState *env, S390Access *desta,
                      desta->mmu_idx, ra);
 }
 
-static uint8_t do_access_get_byte(CPUS390XState *env, vaddr vaddr,
-                                  void *haddr, int offset,
-                                  int mmu_idx, uintptr_t ra)
-{
-#ifdef CONFIG_USER_ONLY
-    return ldub_p(haddr + offset);
-#else
-    if (likely(haddr)) {
-        return ldub_p(haddr + offset);
-    } else {
-        MemOpIdx oi = make_memop_idx(MO_UB, mmu_idx);
-        return cpu_ldb_mmu(env, vaddr + offset, oi, ra);
-    }
-#endif
-}
-
 static uint8_t access_get_byte(CPUS390XState *env, S390Access *access,
                                int offset, uintptr_t ra)
 {
-    if (offset < access->size1) {
-        return do_access_get_byte(env, access->vaddr1, access->haddr1,
-                                  offset, access->mmu_idx, ra);
+    target_ulong vaddr = access->vaddr1;
+    void *haddr = access->haddr1;
+
+    if (unlikely(offset >= access->size1)) {
+        offset -= access->size1;
+        vaddr = access->vaddr2;
+        haddr = access->haddr2;
     }
-    return do_access_get_byte(env, access->vaddr2, access->haddr2,
-                              offset - access->size1, access->mmu_idx, ra);
-}
 
-static void do_access_set_byte(CPUS390XState *env, vaddr vaddr, void *haddr,
-                               int offset, uint8_t byte, int mmu_idx,
-                               uintptr_t ra)
-{
-#ifdef CONFIG_USER_ONLY
-    stb_p(haddr + offset, byte);
-#else
-
-    if (likely(haddr)) {
-        stb_p(haddr + offset, byte);
+    if (user_or_likely(haddr)) {
+        return ldub_p(haddr + offset);
     } else {
-        MemOpIdx oi = make_memop_idx(MO_UB, mmu_idx);
-        cpu_stb_mmu(env, vaddr + offset, byte, oi, ra);
+        MemOpIdx oi = make_memop_idx(MO_UB, access->mmu_idx);
+        return cpu_ldb_mmu(env, vaddr + offset, oi, ra);
     }
-#endif
 }
 
 static void access_set_byte(CPUS390XState *env, S390Access *access,
                             int offset, uint8_t byte, uintptr_t ra)
 {
-    if (offset < access->size1) {
-        do_access_set_byte(env, access->vaddr1, access->haddr1, offset, byte,
-                           access->mmu_idx, ra);
+    target_ulong vaddr = access->vaddr1;
+    void *haddr = access->haddr1;
+
+    if (unlikely(offset >= access->size1)) {
+        offset -= access->size1;
+        vaddr = access->vaddr2;
+        haddr = access->haddr2;
+    }
+
+    if (user_or_likely(haddr)) {
+        stb_p(haddr + offset, byte);
     } else {
-        do_access_set_byte(env, access->vaddr2, access->haddr2,
-                           offset - access->size1, byte, access->mmu_idx, ra);
+        MemOpIdx oi = make_memop_idx(MO_UB, access->mmu_idx);
+        cpu_stb_mmu(env, vaddr + offset, byte, oi, ra);
     }
 }
 
-- 
2.34.1

[PATCH 7/7] target/s390x: Hoist some computation in access_memmove

[Richard Henderson]

Ensure that the total length is in a local variable
across the byte loop.  Compute size1 difference once.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/mem_helper.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 92eb6564c3..5f0dd67985 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -299,16 +299,17 @@ static void access_set_byte(CPUS390XState *env, S390Access *access,
 static void access_memmove(CPUS390XState *env, S390Access *desta,
                            S390Access *srca, uintptr_t ra)
 {
+    int len = desta->size1 + desta->size2;
     int diff;
 
-    g_assert(desta->size1 + desta->size2 == srca->size1 + srca->size2);
+    assert(len == srca->size1 + srca->size2);
 
     /* Fallback to slow access in case we don't have access to all host pages */
     if (unlikely(!desta->haddr1 || (desta->size2 && !desta->haddr2) ||
                  !srca->haddr1 || (srca->size2 && !srca->haddr2))) {
         int i;
 
-        for (i = 0; i < desta->size1 + desta->size2; i++) {
+        for (i = 0; i < len; i++) {
             uint8_t byte = access_get_byte(env, srca, i, ra);
 
             access_set_byte(env, desta, i, byte, ra);
@@ -316,20 +317,20 @@ static void access_memmove(CPUS390XState *env, S390Access *desta,
         return;
     }
 
-    if (srca->size1 == desta->size1) {
+    diff = desta->size1 - srca->size1;
+    if (likely(diff == 0)) {
         memmove(desta->haddr1, srca->haddr1, srca->size1);
         if (unlikely(srca->size2)) {
             memmove(desta->haddr2, srca->haddr2, srca->size2);
         }
-    } else if (srca->size1 < desta->size1) {
-        diff = desta->size1 - srca->size1;
+    } else if (diff > 0) {
         memmove(desta->haddr1, srca->haddr1, srca->size1);
         memmove(desta->haddr1 + srca->size1, srca->haddr2, diff);
         if (likely(desta->size2)) {
             memmove(desta->haddr2, srca->haddr2 + diff, desta->size2);
         }
     } else {
-        diff = srca->size1 - desta->size1;
+        diff = -diff;
         memmove(desta->haddr1, srca->haddr1, desta->size1);
         memmove(desta->haddr2, srca->haddr1 + desta->size1, diff);
         if (likely(srca->size2)) {
-- 
2.34.1

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

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/7] target/s390x: Pass S390Access pointer into access_prepare

[David Hildenbrand]

On 09.01.23 21:18, Richard Henderson wrote:
> Passing a pointer from the caller down to access_prepare_nf
> eliminates a structure copy.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 4/7] target/s390x: Tidy access_prepare_nf

[David Hildenbrand]

On 09.01.23 21:18, Richard Henderson wrote:
> Assign to access struct immediately, rather than waiting
> until the end of the function.  This means we can pass
> address of haddr struct members instead of allocating
> extra space on the local stack.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 3/7] target/s390x: Use void* for haddr in S390Access

[David Hildenbrand]

On 09.01.23 21:18, Richard Henderson wrote:
> The interface from probe_access_flags is void*, and matching
> that will be helpful.  We already rely on the gcc extension
> for byte arithmetic on void*.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 5/7] target/s390x: Remove TLB_NOTDIRTY workarounds

[David Hildenbrand]

On 09.01.23 21:18, Richard Henderson wrote:
> When this code was written, it was using tlb_vaddr_to_host,
> which does not handle TLB_DIRTY.  Since then, it has been
> converted to probe_access_flags, which does.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

It's been a long time ... I have to trust you on some details :D

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 6/7] target/s390x: Inline do_access_{get,set}_byte

[David Hildenbrand]

On 09.01.23 21:18, Richard Henderson wrote:
> Inline into the parent functions with a simple test
> to select the page, and a new define to remove ifdefs.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 7/7] target/s390x: Hoist some computation in access_memmove

[David Hildenbrand]

On 09.01.23 21:18, Richard Henderson wrote:
> Ensure that the total length is in a local variable
> across the byte loop.  Compute size1 difference once.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/tcg/mem_helper.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index 92eb6564c3..5f0dd67985 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -299,16 +299,17 @@ static void access_set_byte(CPUS390XState *env, S390Access *access,
>   static void access_memmove(CPUS390XState *env, S390Access *desta,
>                              S390Access *srca, uintptr_t ra)
>   {
> +    int len = desta->size1 + desta->size2;
>       int diff;

I guess I'd have called "diff" something like "size1_diff" to make the 
comparisons easier to digest.

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/7] target/s390x: mem_helper.c cleanups

[Thomas Huth]

On 09/01/2023 21.18, Richard Henderson wrote:
> I discovered this old branch sitting around from August.
> I don't think I ever sent it out.
> 
> r~
> 
> Richard Henderson (7):
>    target/s390x: Fix s390_probe_access for user-only
>    target/s390x: Pass S390Access pointer into access_prepare
>    target/s390x: Use void* for haddr in S390Access
>    target/s390x: Tidy access_prepare_nf
>    target/s390x: Remove TLB_NOTDIRTY workarounds
>    target/s390x: Inline do_access_{get,set}_byte
>    target/s390x: Hoist some computation in access_memmove

Thanks, applied to my staging branch now:

  https://gitlab.com/thuth/qemu/-/commits/staging

  Thomas

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

[Thomas Huth]

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

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

[Richard Henderson]

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~