[Qemu-devel] [PATCH 0/7] target/sparc: Convert to do_transaction_failed hook

[Qemu-devel] [PATCH 0/7] target/sparc: Convert to do_transaction_failed hook

[Peter Maydell]

This patchset converts the SPARC target away from the old
broken do_unassigned_access hook to the new (added in 2017...)
do_transaction_failed hook. In the process it fixes a number
of bugs in corner cases.

The SPARC ld/st-with-ASI helper functions are odd in that they
make use of the cpu_unassigned_access() function as a way of
raising an MMU fault. We start by making them just directly
call a new sparc_raise_mmu_fault() function so they don't go
through the hook function. This in-passing fixes a bug where
the hook was using GETPC(), which won't work inside a function
several levels deep in the callstack from a helper function.

The next four patches convert places that were using ld*_phys()
and st*_phys() and thus getting "implicitly causes an exception
via do_unassigned_access if it gets a bus error" behaviour.
We make them all use address_space_ld*/st* functions instead,
and explicitly handle the transaction-failure case. Variously:
 * for MMU passthrough, this doesn't change behaviour
 * for the MXCC stream source/destination ASI accesses,
   this doesn't change behaviour, but the current behaviour
   looks a bit weird, so a TODO comment is left in case anybody
   wants to chase up the actual right behaviour in future
 * for page table walks this fixes a bug where we would take
   an exception instead of reporting the page table walk failure
   with the correct fault status register information
 * for the page table walk in mmu_probe() this fixes a bug where
   we would take an exception when we are supposed to just report
   failure. Note that the spec says that MMU probe operations
   are supposed to set the fault status registers, but we do not;
   again I leave this pre-existing bug as a TODO note.
Next, we remove one entirely pointless and unused ldl_phys()
call from dump_mmu().

Finally, the last patch can do the very small amount of work to
change over to the new hook function. This will cause all the
"handle error" code paths in the preceding patches to become
active (when a do_unassigned_access hook is present the load
or store functions will never return an error, because the hook
will get called and throw an exception first).

I have tested that I can boot a sparc32 debian image, and
that sparc64 boots up to the firmware prompt, but this could
certainly use more extensive testing than I have given it.

(After SPARC, the only remaining unassigned-access-hook users
are RISCV, which crept in while I wasn't looking, and MIPS,
which is doing something complicated with the Jazz board that
I haven't yet investigated.)

thanks
--PMM

Peter Maydell (7):
  target/sparc: Factor out the body of sparc_cpu_unassigned_access()
  target/sparc: Check for transaction failures in MMU passthrough ASIs
  target/sparc: Check for transaction failures in MXCC stream ASI
    accesses
  target/sparc: Correctly handle bus errors in page table walks
  target/sparc: Handle bus errors in mmu_probe()
  target/sparc: Remove unused ldl_phys from dump_mmu()
  target/sparc: Switch to do_transaction_failed() hook

 target/sparc/cpu.h         |   8 +-
 target/sparc/cpu.c         |   2 +-
 target/sparc/ldst_helper.c | 319 +++++++++++++++++++++----------------
 target/sparc/mmu_helper.c  |  57 +++++--
 4 files changed, 238 insertions(+), 148 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/7] target/sparc: Factor out the body of sparc_cpu_unassigned_access()

[Peter Maydell]

Currently the SPARC target uses the old-style do_unassigned_access
hook.  We want to switch it over to do_transaction_failed, but to do
this we must first remove all the direct calls in ldst_helper.c to
cpu_unassigned_access().  Factor out the body of the hook function's
code into a new sparc_raise_mmu_fault() and call it from the hook and
from the various places that used to call cpu_unassigned_access().

In passing, this fixes a bug where the code that raised the
MMU exception was directly calling GETPC() from a function that
was several levels deep in the callstack from the original
helper function: the new sparc_raise_mmu_fault() instead takes
the return address as an argument.

Other than the use of retaddr rather than GETPC() and a comment
format fixup, the body of the new function has no changes from
that of the old hook function.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/ldst_helper.c | 201 +++++++++++++++++++------------------
 1 file changed, 106 insertions(+), 95 deletions(-)

diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index 7f56c100c69..26876e5a575 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -422,6 +422,99 @@ static void dump_asi(const char *txt, target_ulong addr, int asi, int size,
 }
 #endif
 
+#ifndef CONFIG_USER_ONLY
+#ifndef TARGET_SPARC64
+static void sparc_raise_mmu_fault(CPUState *cs, hwaddr addr,
+                                  bool is_write, bool is_exec, int is_asi,
+                                  unsigned size, uintptr_t retaddr)
+{
+    SPARCCPU *cpu = SPARC_CPU(cs);
+    CPUSPARCState *env = &cpu->env;
+    int fault_type;
+
+#ifdef DEBUG_UNASSIGNED
+    if (is_asi) {
+        printf("Unassigned mem %s access of %d byte%s to " TARGET_FMT_plx
+               " asi 0x%02x from " TARGET_FMT_lx "\n",
+               is_exec ? "exec" : is_write ? "write" : "read", size,
+               size == 1 ? "" : "s", addr, is_asi, env->pc);
+    } else {
+        printf("Unassigned mem %s access of %d byte%s to " TARGET_FMT_plx
+               " from " TARGET_FMT_lx "\n",
+               is_exec ? "exec" : is_write ? "write" : "read", size,
+               size == 1 ? "" : "s", addr, env->pc);
+    }
+#endif
+    /* Don't overwrite translation and access faults */
+    fault_type = (env->mmuregs[3] & 0x1c) >> 2;
+    if ((fault_type > 4) || (fault_type == 0)) {
+        env->mmuregs[3] = 0; /* Fault status register */
+        if (is_asi) {
+            env->mmuregs[3] |= 1 << 16;
+        }
+        if (env->psrs) {
+            env->mmuregs[3] |= 1 << 5;
+        }
+        if (is_exec) {
+            env->mmuregs[3] |= 1 << 6;
+        }
+        if (is_write) {
+            env->mmuregs[3] |= 1 << 7;
+        }
+        env->mmuregs[3] |= (5 << 2) | 2;
+        /* SuperSPARC will never place instruction fault addresses in the FAR */
+        if (!is_exec) {
+            env->mmuregs[4] = addr; /* Fault address register */
+        }
+    }
+    /* overflow (same type fault was not read before another fault) */
+    if (fault_type == ((env->mmuregs[3] & 0x1c)) >> 2) {
+        env->mmuregs[3] |= 1;
+    }
+
+    if ((env->mmuregs[0] & MMU_E) && !(env->mmuregs[0] & MMU_NF)) {
+        int tt = is_exec ? TT_CODE_ACCESS : TT_DATA_ACCESS;
+        cpu_raise_exception_ra(env, tt, retaddr);
+    }
+
+    /*
+     * flush neverland mappings created during no-fault mode,
+     * so the sequential MMU faults report proper fault types
+     */
+    if (env->mmuregs[0] & MMU_NF) {
+        tlb_flush(cs);
+    }
+}
+#else
+static void sparc_raise_mmu_fault(CPUState *cs, hwaddr addr,
+                                  bool is_write, bool is_exec, int is_asi,
+                                  unsigned size, uintptr_t retaddr)
+{
+    SPARCCPU *cpu = SPARC_CPU(cs);
+    CPUSPARCState *env = &cpu->env;
+
+#ifdef DEBUG_UNASSIGNED
+    printf("Unassigned mem access to " TARGET_FMT_plx " from " TARGET_FMT_lx
+           "\n", addr, env->pc);
+#endif
+
+    if (is_exec) { /* XXX has_hypervisor */
+        if (env->lsu & (IMMU_E)) {
+            cpu_raise_exception_ra(env, TT_CODE_ACCESS, retaddr);
+        } else if (cpu_has_hypervisor(env) && !(env->hpstate & HS_PRIV)) {
+            cpu_raise_exception_ra(env, TT_INSN_REAL_TRANSLATION_MISS, retaddr);
+        }
+    } else {
+        if (env->lsu & (DMMU_E)) {
+            cpu_raise_exception_ra(env, TT_DATA_ACCESS, retaddr);
+        } else if (cpu_has_hypervisor(env) && !(env->hpstate & HS_PRIV)) {
+            cpu_raise_exception_ra(env, TT_DATA_REAL_TRANSLATION_MISS, retaddr);
+        }
+    }
+}
+#endif
+#endif
+
 #ifndef TARGET_SPARC64
 #ifndef CONFIG_USER_ONLY
 
@@ -688,7 +781,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr,
         break;
     case ASI_USERTXT: /* User code access, XXX */
     default:
-        cpu_unassigned_access(cs, addr, false, false, asi, size);
+        sparc_raise_mmu_fault(cs, addr, false, false, asi, size, GETPC());
         ret = 0;
         break;
 
@@ -1026,7 +1119,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, uint64_t val,
     case ASI_USERTXT: /* User code access, XXX */
     case ASI_KERNELTXT: /* Supervisor code access, XXX */
     default:
-        cpu_unassigned_access(cs, addr, true, false, asi, size);
+        sparc_raise_mmu_fault(cs, addr, true, false, asi, size, GETPC());
         break;
 
     case ASI_USERDATA: /* User data access */
@@ -1292,7 +1385,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr,
                 ret = env->immu.tag_access;
                 break;
             default:
-                cpu_unassigned_access(cs, addr, false, false, 1, size);
+                sparc_raise_mmu_fault(cs, addr, false, false, 1, size, GETPC());
                 ret = 0;
             }
             break;
@@ -1358,7 +1451,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr,
                 ret = env->dmmu.physical_watchpoint;
                 break;
             default:
-                cpu_unassigned_access(cs, addr, false, false, 1, size);
+                sparc_raise_mmu_fault(cs, addr, false, false, 1, size, GETPC());
                 ret = 0;
             }
             break;
@@ -1407,7 +1500,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr,
     case ASI_SCRATCHPAD: /* UA2005 privileged scratchpad */
         if (unlikely((addr >= 0x20) && (addr < 0x30))) {
             /* Hyperprivileged access only */
-            cpu_unassigned_access(cs, addr, false, false, 1, size);
+            sparc_raise_mmu_fault(cs, addr, false, false, 1, size, GETPC());
         }
         /* fall through */
     case ASI_HYP_SCRATCHPAD: /* UA2005 hyperprivileged scratchpad */
@@ -1425,7 +1518,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr,
             ret = env->dmmu.mmu_secondary_context;
             break;
         default:
-          cpu_unassigned_access(cs, addr, true, false, 1, size);
+          sparc_raise_mmu_fault(cs, addr, true, false, 1, size, GETPC());
         }
         break;
     case ASI_DCACHE_DATA:     /* D-cache data */
@@ -1448,7 +1541,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr,
     case ASI_DMMU_DEMAP:          /* D-MMU demap, WO */
     case ASI_INTR_W:              /* Interrupt vector, WO */
     default:
-        cpu_unassigned_access(cs, addr, false, false, 1, size);
+        sparc_raise_mmu_fault(cs, addr, false, false, 1, size, GETPC());
         ret = 0;
         break;
     }
@@ -1622,7 +1715,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
             case 8:
                 return;
             default:
-                cpu_unassigned_access(cs, addr, true, false, 1, size);
+                sparc_raise_mmu_fault(cs, addr, true, false, 1, size, GETPC());
                 break;
             }
 
@@ -1706,7 +1799,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
                 env->dmmu.physical_watchpoint = val;
                 break;
             default:
-                cpu_unassigned_access(cs, addr, true, false, 1, size);
+                sparc_raise_mmu_fault(cs, addr, true, false, 1, size, GETPC());
                 break;
             }
 
@@ -1750,7 +1843,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
     case ASI_SCRATCHPAD: /* UA2005 privileged scratchpad */
         if (unlikely((addr >= 0x20) && (addr < 0x30))) {
             /* Hyperprivileged access only */
-            cpu_unassigned_access(cs, addr, true, false, 1, size);
+            sparc_raise_mmu_fault(cs, addr, true, false, 1, size, GETPC());
         }
         /* fall through */
     case ASI_HYP_SCRATCHPAD: /* UA2005 hyperprivileged scratchpad */
@@ -1776,7 +1869,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
                                   (1 << MMU_KERNEL_SECONDARY_IDX));
               break;
           default:
-              cpu_unassigned_access(cs, addr, true, false, 1, size);
+              sparc_raise_mmu_fault(cs, addr, true, false, 1, size, GETPC());
           }
         }
         return;
@@ -1808,7 +1901,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
     case ASI_PNFL: /* Primary no-fault LE, RO */
     case ASI_SNFL: /* Secondary no-fault LE, RO */
     default:
-        cpu_unassigned_access(cs, addr, true, false, 1, size);
+        sparc_raise_mmu_fault(cs, addr, true, false, 1, size, GETPC());
         return;
     }
 }
@@ -1816,94 +1909,12 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
 #endif /* TARGET_SPARC64 */
 
 #if !defined(CONFIG_USER_ONLY)
-#ifndef TARGET_SPARC64
 void sparc_cpu_unassigned_access(CPUState *cs, hwaddr addr,
                                  bool is_write, bool is_exec, int is_asi,
                                  unsigned size)
 {
-    SPARCCPU *cpu = SPARC_CPU(cs);
-    CPUSPARCState *env = &cpu->env;
-    int fault_type;
-
-#ifdef DEBUG_UNASSIGNED
-    if (is_asi) {
-        printf("Unassigned mem %s access of %d byte%s to " TARGET_FMT_plx
-               " asi 0x%02x from " TARGET_FMT_lx "\n",
-               is_exec ? "exec" : is_write ? "write" : "read", size,
-               size == 1 ? "" : "s", addr, is_asi, env->pc);
-    } else {
-        printf("Unassigned mem %s access of %d byte%s to " TARGET_FMT_plx
-               " from " TARGET_FMT_lx "\n",
-               is_exec ? "exec" : is_write ? "write" : "read", size,
-               size == 1 ? "" : "s", addr, env->pc);
-    }
-#endif
-    /* Don't overwrite translation and access faults */
-    fault_type = (env->mmuregs[3] & 0x1c) >> 2;
-    if ((fault_type > 4) || (fault_type == 0)) {
-        env->mmuregs[3] = 0; /* Fault status register */
-        if (is_asi) {
-            env->mmuregs[3] |= 1 << 16;
-        }
-        if (env->psrs) {
-            env->mmuregs[3] |= 1 << 5;
-        }
-        if (is_exec) {
-            env->mmuregs[3] |= 1 << 6;
-        }
-        if (is_write) {
-            env->mmuregs[3] |= 1 << 7;
-        }
-        env->mmuregs[3] |= (5 << 2) | 2;
-        /* SuperSPARC will never place instruction fault addresses in the FAR */
-        if (!is_exec) {
-            env->mmuregs[4] = addr; /* Fault address register */
-        }
-    }
-    /* overflow (same type fault was not read before another fault) */
-    if (fault_type == ((env->mmuregs[3] & 0x1c)) >> 2) {
-        env->mmuregs[3] |= 1;
-    }
-
-    if ((env->mmuregs[0] & MMU_E) && !(env->mmuregs[0] & MMU_NF)) {
-        int tt = is_exec ? TT_CODE_ACCESS : TT_DATA_ACCESS;
-        cpu_raise_exception_ra(env, tt, GETPC());
-    }
-
-    /* flush neverland mappings created during no-fault mode,
-       so the sequential MMU faults report proper fault types */
-    if (env->mmuregs[0] & MMU_NF) {
-        tlb_flush(cs);
-    }
+    sparc_raise_mmu_fault(cs, addr, is_write, is_exec, is_asi, size, GETPC());
 }
-#else
-void sparc_cpu_unassigned_access(CPUState *cs, hwaddr addr,
-                                 bool is_write, bool is_exec, int is_asi,
-                                 unsigned size)
-{
-    SPARCCPU *cpu = SPARC_CPU(cs);
-    CPUSPARCState *env = &cpu->env;
-
-#ifdef DEBUG_UNASSIGNED
-    printf("Unassigned mem access to " TARGET_FMT_plx " from " TARGET_FMT_lx
-           "\n", addr, env->pc);
-#endif
-
-    if (is_exec) { /* XXX has_hypervisor */
-        if (env->lsu & (IMMU_E)) {
-            cpu_raise_exception_ra(env, TT_CODE_ACCESS, GETPC());
-        } else if (cpu_has_hypervisor(env) && !(env->hpstate & HS_PRIV)) {
-            cpu_raise_exception_ra(env, TT_INSN_REAL_TRANSLATION_MISS, GETPC());
-        }
-    } else {
-        if (env->lsu & (DMMU_E)) {
-            cpu_raise_exception_ra(env, TT_DATA_ACCESS, GETPC());
-        } else if (cpu_has_hypervisor(env) && !(env->hpstate & HS_PRIV)) {
-            cpu_raise_exception_ra(env, TT_DATA_REAL_TRANSLATION_MISS, GETPC());
-        }
-    }
-}
-#endif
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
-- 
2.20.1

[Qemu-devel] [PATCH 2/7] target/sparc: Check for transaction failures in MMU passthrough ASIs

[Peter Maydell]

Currently the ld/st_asi helper functions make calls to the
ld*_phys() and st*_phys() functions for those ASIs which
imply direct accesses to physical addresses. These implicitly
rely on the unassigned_access hook to cause them to generate
an MMU fault if the access fails.

Switch to using the address_space_* functions instead, which
return a MemTxResult that we can check. This means that when
we switch SPARC over to using the do_transaction_failed hook
we'll still get the same MMU faults we did before.

This commit converts the ASIs which do "MMU passthrough".

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/ldst_helper.c | 49 +++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index 26876e5a575..39698c58859 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -718,26 +718,36 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr,
     case ASI_M_DATAC_DATA: /* SparcStation 5 D-cache data */
         break;
     case 0x21 ... 0x2f: /* MMU passthrough, 0x100000000 to 0xfffffffff */
+    {
+        MemTxResult result;
+        hwaddr access_addr = (hwaddr)addr | ((hwaddr)(asi & 0xf) << 32);
+
         switch (size) {
         case 1:
-            ret = ldub_phys(cs->as, (hwaddr)addr
-                            | ((hwaddr)(asi & 0xf) << 32));
+            ret = address_space_ldub(cs->as, access_addr,
+                                     MEMTXATTRS_UNSPECIFIED, &result);
             break;
         case 2:
-            ret = lduw_phys(cs->as, (hwaddr)addr
-                            | ((hwaddr)(asi & 0xf) << 32));
+            ret = address_space_lduw(cs->as, access_addr,
+                                     MEMTXATTRS_UNSPECIFIED, &result);
             break;
         default:
         case 4:
-            ret = ldl_phys(cs->as, (hwaddr)addr
-                           | ((hwaddr)(asi & 0xf) << 32));
+            ret = address_space_ldl(cs->as, access_addr,
+                                    MEMTXATTRS_UNSPECIFIED, &result);
             break;
         case 8:
-            ret = ldq_phys(cs->as, (hwaddr)addr
-                           | ((hwaddr)(asi & 0xf) << 32));
+            ret = address_space_ldq(cs->as, access_addr,
+                                    MEMTXATTRS_UNSPECIFIED, &result);
             break;
         }
+
+        if (result != MEMTX_OK) {
+            sparc_raise_mmu_fault(cs, access_addr, false, false, false,
+                                  size, GETPC());
+        }
         break;
+    }
     case 0x30: /* Turbosparc secondary cache diagnostic */
     case 0x31: /* Turbosparc RAM snoop */
     case 0x32: /* Turbosparc page table descriptor diagnostic */
@@ -1053,25 +1063,32 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, uint64_t val,
         break;
     case 0x21 ... 0x2f: /* MMU passthrough, 0x100000000 to 0xfffffffff */
         {
+            MemTxResult result;
+            hwaddr access_addr = (hwaddr)addr | ((hwaddr)(asi & 0xf) << 32);
+
             switch (size) {
             case 1:
-                stb_phys(cs->as, (hwaddr)addr
-                         | ((hwaddr)(asi & 0xf) << 32), val);
+                address_space_stb(cs->as, access_addr, val,
+                                  MEMTXATTRS_UNSPECIFIED, &result);
                 break;
             case 2:
-                stw_phys(cs->as, (hwaddr)addr
-                         | ((hwaddr)(asi & 0xf) << 32), val);
+                address_space_stw(cs->as, access_addr, val,
+                                  MEMTXATTRS_UNSPECIFIED, &result);
                 break;
             case 4:
             default:
-                stl_phys(cs->as, (hwaddr)addr
-                         | ((hwaddr)(asi & 0xf) << 32), val);
+                address_space_stl(cs->as, access_addr, val,
+                                  MEMTXATTRS_UNSPECIFIED, &result);
                 break;
             case 8:
-                stq_phys(cs->as, (hwaddr)addr
-                         | ((hwaddr)(asi & 0xf) << 32), val);
+                address_space_stq(cs->as, access_addr, val,
+                                  MEMTXATTRS_UNSPECIFIED, &result);
                 break;
             }
+            if (result != MEMTX_OK) {
+                sparc_raise_mmu_fault(cs, access_addr, true, false, false,
+                                      size, GETPC());
+            }
         }
         break;
     case 0x30: /* store buffer tags or Turbosparc secondary cache diagnostic */
-- 
2.20.1

[Qemu-devel] [PATCH 3/7] target/sparc: Check for transaction failures in MXCC stream ASI accesses

[Peter Maydell]

Currently the ld/st_asi helper functions make calls to the
ld*_phys() and st*_phys() functions for those ASIs which
imply direct accesses to physical addresses. These implicitly
rely on the unassigned_access hook to cause them to generate
an MMU fault if the access fails.

Switch to using the address_space_* functions instead, which
return a MemTxResult that we can check. This means that when
we switch SPARC over to using the do_transaction_failed hook
we'll still get the same MMU faults we did before.

This commit converts the ASIs which do MXCC stream source
and destination accesses.

It's not clear to me whether raising an MMU fault like this
is the correct behaviour if we encounter a bus error, but
we retain the same behaviour that the old unassigned_access
hook would implement.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/ldst_helper.c | 57 +++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index 39698c58859..91cd0b593ef 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -880,6 +880,9 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, uint64_t val,
             }
             break;
         case 0x01c00100: /* MXCC stream source */
+        {
+            int i;
+
             if (size == 8) {
                 env->mxccregs[0] = val;
             } else {
@@ -887,20 +890,27 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, uint64_t val,
                               "%08x: unimplemented access size: %d\n", addr,
                               size);
             }
-            env->mxccdata[0] = ldq_phys(cs->as,
-                                        (env->mxccregs[0] & 0xffffffffULL) +
-                                        0);
-            env->mxccdata[1] = ldq_phys(cs->as,
-                                        (env->mxccregs[0] & 0xffffffffULL) +
-                                        8);
-            env->mxccdata[2] = ldq_phys(cs->as,
-                                        (env->mxccregs[0] & 0xffffffffULL) +
-                                        16);
-            env->mxccdata[3] = ldq_phys(cs->as,
-                                        (env->mxccregs[0] & 0xffffffffULL) +
-                                        24);
+
+            for (i = 0; i < 4; i++) {
+                MemTxResult result;
+                hwaddr access_addr = (env->mxccregs[0] & 0xffffffffULL) + 8 * i;
+
+                env->mxccdata[i] = address_space_ldq(cs->as,
+                                                     access_addr,
+                                                     MEMTXATTRS_UNSPECIFIED,
+                                                     &result);
+                if (result != MEMTX_OK) {
+                    /* TODO: investigate whether this is the right behaviour */
+                    sparc_raise_mmu_fault(cs, access_addr, false, false,
+                                          false, size, GETPC());
+                }
+            }
             break;
+        }
         case 0x01c00200: /* MXCC stream destination */
+        {
+            int i;
+
             if (size == 8) {
                 env->mxccregs[1] = val;
             } else {
@@ -908,15 +918,22 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, uint64_t val,
                               "%08x: unimplemented access size: %d\n", addr,
                               size);
             }
-            stq_phys(cs->as, (env->mxccregs[1] & 0xffffffffULL) +  0,
-                     env->mxccdata[0]);
-            stq_phys(cs->as, (env->mxccregs[1] & 0xffffffffULL) +  8,
-                     env->mxccdata[1]);
-            stq_phys(cs->as, (env->mxccregs[1] & 0xffffffffULL) + 16,
-                     env->mxccdata[2]);
-            stq_phys(cs->as, (env->mxccregs[1] & 0xffffffffULL) + 24,
-                     env->mxccdata[3]);
+
+            for (i = 0; i < 4; i++) {
+                MemTxResult result;
+                hwaddr access_addr = (env->mxccregs[1] & 0xffffffffULL) + 8 * i;
+
+                address_space_stq(cs->as, access_addr, env->mxccdata[i],
+                                  MEMTXATTRS_UNSPECIFIED, &result);
+
+                if (result != MEMTX_OK) {
+                    /* TODO: investigate whether this is the right behaviour */
+                    sparc_raise_mmu_fault(cs, access_addr, true, false,
+                                          false, size, GETPC());
+                }
+            }
             break;
+        }
         case 0x01c00a00: /* MXCC control register */
             if (size == 8) {
                 env->mxccregs[3] = val;
-- 
2.20.1

[Qemu-devel] [PATCH 4/7] target/sparc: Correctly handle bus errors in page table walks

[Peter Maydell]

Currently we use the ldl_phys() function to read page table entries.
With the unassigned_access hook in place, if these hit an unassigned
area of memory then the hook will cause us to wrongly generate
an exception with a fault address matching the address of the
page table entry.

Change to using address_space_ldl() so we can detect and correctly
handle bus errors and give them their correct behaviour of
causing a translation error with a suitable fault status register.

Note that this won't actually take effect until we switch the
over to using the do_translation_failed hook.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/mmu_helper.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index cbd1e911796..351055a09b1 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -98,6 +98,7 @@ static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
     int error_code = 0, is_dirty, is_user;
     unsigned long page_offset;
     CPUState *cs = env_cpu(env);
+    MemTxResult result;
 
     is_user = mmu_idx == MMU_USER_IDX;
 
@@ -120,7 +121,10 @@ static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
     /* SPARC reference MMU table walk: Context table->L1->L2->PTE */
     /* Context base + context number */
     pde_ptr = (env->mmuregs[1] << 4) + (env->mmuregs[2] << 2);
-    pde = ldl_phys(cs->as, pde_ptr);
+    pde = address_space_ldl(cs->as, pde_ptr, MEMTXATTRS_UNSPECIFIED, &result);
+    if (result != MEMTX_OK) {
+        return 4 << 2; /* Translation fault, L = 0 */
+    }
 
     /* Ctx pde */
     switch (pde & PTE_ENTRYTYPE_MASK) {
@@ -132,7 +136,11 @@ static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
         return 4 << 2;
     case 1: /* L0 PDE */
         pde_ptr = ((address >> 22) & ~3) + ((pde & ~3) << 4);
-        pde = ldl_phys(cs->as, pde_ptr);
+        pde = address_space_ldl(cs->as, pde_ptr,
+                                MEMTXATTRS_UNSPECIFIED, &result);
+        if (result != MEMTX_OK) {
+            return (1 << 8) | (4 << 2); /* Translation fault, L = 1 */
+        }
 
         switch (pde & PTE_ENTRYTYPE_MASK) {
         default:
@@ -142,7 +150,11 @@ static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
             return (1 << 8) | (4 << 2);
         case 1: /* L1 PDE */
             pde_ptr = ((address & 0xfc0000) >> 16) + ((pde & ~3) << 4);
-            pde = ldl_phys(cs->as, pde_ptr);
+            pde = address_space_ldl(cs->as, pde_ptr,
+                                    MEMTXATTRS_UNSPECIFIED, &result);
+            if (result != MEMTX_OK) {
+                return (2 << 8) | (4 << 2); /* Translation fault, L = 2 */
+            }
 
             switch (pde & PTE_ENTRYTYPE_MASK) {
             default:
@@ -152,7 +164,11 @@ static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
                 return (2 << 8) | (4 << 2);
             case 1: /* L2 PDE */
                 pde_ptr = ((address & 0x3f000) >> 10) + ((pde & ~3) << 4);
-                pde = ldl_phys(cs->as, pde_ptr);
+                pde = address_space_ldl(cs->as, pde_ptr,
+                                        MEMTXATTRS_UNSPECIFIED, &result);
+                if (result != MEMTX_OK) {
+                    return (3 << 8) | (4 << 2); /* Translation fault, L = 3 */
+                }
 
                 switch (pde & PTE_ENTRYTYPE_MASK) {
                 default:
-- 
2.20.1

[Qemu-devel] [PATCH 5/7] target/sparc: Handle bus errors in mmu_probe()

[Peter Maydell]

Convert the mmu_probe() function to using address_space_ldl()
rather than ldl_phys(), so we can explicitly detect memory
transaction failures.

This makes no practical difference at the moment, because
ldl_phys() will return 0 on a transaction failure, and we
treat transaction failures and 0 PDEs identically. However
the spec says that MMU probe operations are supposed to
update the fault status registers, and if we ever implement
that we'll want to distinguish the difference. For the
moment, just add a TODO comment about the bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/mmu_helper.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index 351055a09b1..d90aabfa4d2 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -287,11 +287,20 @@ target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int mmulev)
     CPUState *cs = env_cpu(env);
     hwaddr pde_ptr;
     uint32_t pde;
+    MemTxResult result;
+
+    /*
+     * TODO: MMU probe operations are supposed to set the fault
+     * status registers, but we don't do this.
+     */
 
     /* Context base + context number */
     pde_ptr = (hwaddr)(env->mmuregs[1] << 4) +
         (env->mmuregs[2] << 2);
-    pde = ldl_phys(cs->as, pde_ptr);
+    pde = address_space_ldl(cs->as, pde_ptr, MEMTXATTRS_UNSPECIFIED, &result);
+    if (result != MEMTX_OK) {
+        return 0;
+    }
 
     switch (pde & PTE_ENTRYTYPE_MASK) {
     default:
@@ -304,7 +313,11 @@ target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int mmulev)
             return pde;
         }
         pde_ptr = ((address >> 22) & ~3) + ((pde & ~3) << 4);
-        pde = ldl_phys(cs->as, pde_ptr);
+        pde = address_space_ldl(cs->as, pde_ptr,
+                                MEMTXATTRS_UNSPECIFIED, &result);
+        if (result != MEMTX_OK) {
+            return 0;
+        }
 
         switch (pde & PTE_ENTRYTYPE_MASK) {
         default:
@@ -318,7 +331,11 @@ target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int mmulev)
                 return pde;
             }
             pde_ptr = ((address & 0xfc0000) >> 16) + ((pde & ~3) << 4);
-            pde = ldl_phys(cs->as, pde_ptr);
+            pde = address_space_ldl(cs->as, pde_ptr,
+                                    MEMTXATTRS_UNSPECIFIED, &result);
+            if (result != MEMTX_OK) {
+                return 0;
+            }
 
             switch (pde & PTE_ENTRYTYPE_MASK) {
             default:
@@ -332,7 +349,11 @@ target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int mmulev)
                     return pde;
                 }
                 pde_ptr = ((address & 0x3f000) >> 10) + ((pde & ~3) << 4);
-                pde = ldl_phys(cs->as, pde_ptr);
+                pde = address_space_ldl(cs->as, pde_ptr,
+                                        MEMTXATTRS_UNSPECIFIED, &result);
+                if (result != MEMTX_OK) {
+                    return 0;
+                }
 
                 switch (pde & PTE_ENTRYTYPE_MASK) {
                 default:
-- 
2.20.1

[Qemu-devel] [PATCH 6/7] target/sparc: Remove unused ldl_phys from dump_mmu()

[Peter Maydell]

The dump_mmu() function does a ldl_phys() at the start, but
then never uses the value it loads at all. Remove the
unused code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/mmu_helper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index d90aabfa4d2..9a9f2cf9535 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -375,11 +375,9 @@ void dump_mmu(CPUSPARCState *env)
     CPUState *cs = env_cpu(env);
     target_ulong va, va1, va2;
     unsigned int n, m, o;
-    hwaddr pde_ptr, pa;
+    hwaddr pa;
     uint32_t pde;
 
-    pde_ptr = (env->mmuregs[1] << 4) + (env->mmuregs[2] << 2);
-    pde = ldl_phys(cs->as, pde_ptr);
     qemu_printf("Root ptr: " TARGET_FMT_plx ", ctx: %d\n",
                 (hwaddr)env->mmuregs[1] << 4, env->mmuregs[2]);
     for (n = 0, va = 0; n < 256; n++, va += 16 * 1024 * 1024) {
-- 
2.20.1

[Qemu-devel] [PATCH 7/7] target/sparc: Switch to do_transaction_failed() hook

[Peter Maydell]

Switch the SPARC target from the old unassigned_access hook to the
new do_transaction_failed hook.

This will cause the "if transaction failed" code paths added in
the previous commits to become active if the access is to an
unassigned address. In particular we'll now handle bus errors
during page table walks correctly (generating a translation
error with the right kind of fault status).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/cpu.h         |  8 +++++---
 target/sparc/cpu.c         |  2 +-
 target/sparc/ldst_helper.c | 16 ++++++++++++----
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 8ed2250cd03..0bf365bed22 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -614,9 +614,11 @@ static inline int tlb_compare_context(const SparcTLBEntry *tlb,
 
 /* cpu-exec.c */
 #if !defined(CONFIG_USER_ONLY)
-void sparc_cpu_unassigned_access(CPUState *cpu, hwaddr addr,
-                                 bool is_write, bool is_exec, int is_asi,
-                                 unsigned size);
+void sparc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                     vaddr addr, unsigned size,
+                                     MMUAccessType access_type,
+                                     int mmu_idx, MemTxAttrs attrs,
+                                     MemTxResult response, uintptr_t retaddr);
 #if defined(TARGET_SPARC64)
 hwaddr cpu_get_phys_page_nofault(CPUSPARCState *env, target_ulong addr,
                                            int mmu_idx);
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index ee60a5536a0..bc659295520 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -877,7 +877,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_write_register = sparc_cpu_gdb_write_register;
     cc->tlb_fill = sparc_cpu_tlb_fill;
 #ifndef CONFIG_USER_ONLY
-    cc->do_unassigned_access = sparc_cpu_unassigned_access;
+    cc->do_transaction_failed = sparc_cpu_do_transaction_failed;
     cc->do_unaligned_access = sparc_cpu_do_unaligned_access;
     cc->get_phys_page_debug = sparc_cpu_get_phys_page_debug;
     cc->vmsd = &vmstate_sparc_cpu;
diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index 91cd0b593ef..7345827a966 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -1943,11 +1943,19 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
 #endif /* TARGET_SPARC64 */
 
 #if !defined(CONFIG_USER_ONLY)
-void sparc_cpu_unassigned_access(CPUState *cs, hwaddr addr,
-                                 bool is_write, bool is_exec, int is_asi,
-                                 unsigned size)
+
+void sparc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                     vaddr addr, unsigned size,
+                                     MMUAccessType access_type,
+                                     int mmu_idx, MemTxAttrs attrs,
+                                     MemTxResult response, uintptr_t retaddr)
 {
-    sparc_raise_mmu_fault(cs, addr, is_write, is_exec, is_asi, size, GETPC());
+    bool is_write = access_type == MMU_DATA_STORE;
+    bool is_exec = access_type == MMU_INST_FETCH;
+    bool is_asi = false;
+
+    sparc_raise_mmu_fault(cs, physaddr, is_write, is_exec,
+                          is_asi, size, retaddr);
 }
 #endif
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/7] target/sparc: Factor out the body of sparc_cpu_unassigned_access()

[Richard Henderson]

On 8/1/19 11:30 AM, Peter Maydell wrote:
> Currently the SPARC target uses the old-style do_unassigned_access
> hook.  We want to switch it over to do_transaction_failed, but to do
> this we must first remove all the direct calls in ldst_helper.c to
> cpu_unassigned_access().  Factor out the body of the hook function's
> code into a new sparc_raise_mmu_fault() and call it from the hook and
> from the various places that used to call cpu_unassigned_access().
> 
> In passing, this fixes a bug where the code that raised the
> MMU exception was directly calling GETPC() from a function that
> was several levels deep in the callstack from the original
> helper function: the new sparc_raise_mmu_fault() instead takes
> the return address as an argument.
> 
> Other than the use of retaddr rather than GETPC() and a comment
> format fixup, the body of the new function has no changes from
> that of the old hook function.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/ldst_helper.c | 201 +++++++++++++++++++------------------
>  1 file changed, 106 insertions(+), 95 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/7] target/sparc: Check for transaction failures in MMU passthrough ASIs

[Richard Henderson]

On 8/1/19 11:30 AM, Peter Maydell wrote:
> Currently the ld/st_asi helper functions make calls to the
> ld*_phys() and st*_phys() functions for those ASIs which
> imply direct accesses to physical addresses. These implicitly
> rely on the unassigned_access hook to cause them to generate
> an MMU fault if the access fails.
> 
> Switch to using the address_space_* functions instead, which
> return a MemTxResult that we can check. This means that when
> we switch SPARC over to using the do_transaction_failed hook
> we'll still get the same MMU faults we did before.
> 
> This commit converts the ASIs which do "MMU passthrough".
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/ldst_helper.c | 49 +++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 3/7] target/sparc: Check for transaction failures in MXCC stream ASI accesses

[Richard Henderson]

On 8/1/19 11:30 AM, Peter Maydell wrote:
> Currently the ld/st_asi helper functions make calls to the
> ld*_phys() and st*_phys() functions for those ASIs which
> imply direct accesses to physical addresses. These implicitly
> rely on the unassigned_access hook to cause them to generate
> an MMU fault if the access fails.
> 
> Switch to using the address_space_* functions instead, which
> return a MemTxResult that we can check. This means that when
> we switch SPARC over to using the do_transaction_failed hook
> we'll still get the same MMU faults we did before.
> 
> This commit converts the ASIs which do MXCC stream source
> and destination accesses.
> 
> It's not clear to me whether raising an MMU fault like this
> is the correct behaviour if we encounter a bus error, but
> we retain the same behaviour that the old unassigned_access
> hook would implement.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/ldst_helper.c | 57 +++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 20 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 4/7] target/sparc: Correctly handle bus errors in page table walks

[Richard Henderson]

On 8/1/19 11:30 AM, Peter Maydell wrote:
> Currently we use the ldl_phys() function to read page table entries.
> With the unassigned_access hook in place, if these hit an unassigned
> area of memory then the hook will cause us to wrongly generate
> an exception with a fault address matching the address of the
> page table entry.
> 
> Change to using address_space_ldl() so we can detect and correctly
> handle bus errors and give them their correct behaviour of
> causing a translation error with a suitable fault status register.
> 
> Note that this won't actually take effect until we switch the
> over to using the do_translation_failed hook.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/mmu_helper.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 5/7] target/sparc: Handle bus errors in mmu_probe()

[Richard Henderson]

On 8/1/19 11:30 AM, Peter Maydell wrote:
> +    /*
> +     * TODO: MMU probe operations are supposed to set the fault
> +     * status registers, but we don't do this.
> +     */

Well, the todo should be using sparc_cpu_tlb_fill with probe=true, since this
function appears to be otherwise redundant.  Of course, there's currently a big
fixme for that as well.

In the meantime, this does one step better than before.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 6/7] target/sparc: Remove unused ldl_phys from dump_mmu()

[Richard Henderson]

On 8/1/19 11:30 AM, Peter Maydell wrote:
> The dump_mmu() function does a ldl_phys() at the start, but
> then never uses the value it loads at all. Remove the
> unused code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/mmu_helper.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 7/7] target/sparc: Switch to do_transaction_failed() hook

[Richard Henderson]

On 8/1/19 11:30 AM, Peter Maydell wrote:
> Switch the SPARC target from the old unassigned_access hook to the
> new do_transaction_failed hook.
> 
> This will cause the "if transaction failed" code paths added in
> the previous commits to become active if the access is to an
> unassigned address. In particular we'll now handle bus errors
> during page table walks correctly (generating a translation
> error with the right kind of fault status).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/cpu.h         |  8 +++++---
>  target/sparc/cpu.c         |  2 +-
>  target/sparc/ldst_helper.c | 16 ++++++++++++----
>  3 files changed, 18 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 0/7] target/sparc: Convert to do_transaction_failed hook

[Peter Maydell]

Mark -- ping? Richard has reviewed this series; do you want
more time to test it, or should I just apply it to master
if you don't have any other pending sparc patches?

thanks
-- PMM

On Thu, 1 Aug 2019 at 19:30, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset converts the SPARC target away from the old
> broken do_unassigned_access hook to the new (added in 2017...)
> do_transaction_failed hook. In the process it fixes a number
> of bugs in corner cases.
>
> The SPARC ld/st-with-ASI helper functions are odd in that they
> make use of the cpu_unassigned_access() function as a way of
> raising an MMU fault. We start by making them just directly
> call a new sparc_raise_mmu_fault() function so they don't go
> through the hook function. This in-passing fixes a bug where
> the hook was using GETPC(), which won't work inside a function
> several levels deep in the callstack from a helper function.
>
> The next four patches convert places that were using ld*_phys()
> and st*_phys() and thus getting "implicitly causes an exception
> via do_unassigned_access if it gets a bus error" behaviour.
> We make them all use address_space_ld*/st* functions instead,
> and explicitly handle the transaction-failure case. Variously:
>  * for MMU passthrough, this doesn't change behaviour
>  * for the MXCC stream source/destination ASI accesses,
>    this doesn't change behaviour, but the current behaviour
>    looks a bit weird, so a TODO comment is left in case anybody
>    wants to chase up the actual right behaviour in future
>  * for page table walks this fixes a bug where we would take
>    an exception instead of reporting the page table walk failure
>    with the correct fault status register information
>  * for the page table walk in mmu_probe() this fixes a bug where
>    we would take an exception when we are supposed to just report
>    failure. Note that the spec says that MMU probe operations
>    are supposed to set the fault status registers, but we do not;
>    again I leave this pre-existing bug as a TODO note.
> Next, we remove one entirely pointless and unused ldl_phys()
> call from dump_mmu().
>
> Finally, the last patch can do the very small amount of work to
> change over to the new hook function. This will cause all the
> "handle error" code paths in the preceding patches to become
> active (when a do_unassigned_access hook is present the load
> or store functions will never return an error, because the hook
> will get called and throw an exception first).
>
> I have tested that I can boot a sparc32 debian image, and
> that sparc64 boots up to the firmware prompt, but this could
> certainly use more extensive testing than I have given it.
>
> (After SPARC, the only remaining unassigned-access-hook users
> are RISCV, which crept in while I wasn't looking, and MIPS,
> which is doing something complicated with the Jazz board that
> I haven't yet investigated.)
>
> thanks
> --PMM
>
> Peter Maydell (7):
>   target/sparc: Factor out the body of sparc_cpu_unassigned_access()
>   target/sparc: Check for transaction failures in MMU passthrough ASIs
>   target/sparc: Check for transaction failures in MXCC stream ASI
>     accesses
>   target/sparc: Correctly handle bus errors in page table walks
>   target/sparc: Handle bus errors in mmu_probe()
>   target/sparc: Remove unused ldl_phys from dump_mmu()
>   target/sparc: Switch to do_transaction_failed() hook
>
>  target/sparc/cpu.h         |   8 +-
>  target/sparc/cpu.c         |   2 +-
>  target/sparc/ldst_helper.c | 319 +++++++++++++++++++++----------------
>  target/sparc/mmu_helper.c  |  57 +++++--
>  4 files changed, 238 insertions(+), 148 deletions(-)
>

Re: [Qemu-devel] [PATCH 0/7] target/sparc: Convert to do_transaction_failed hook

[Mark Cave-Ayland]

On 03/09/2019 14:15, Peter Maydell wrote:

> Mark -- ping? Richard has reviewed this series; do you want
> more time to test it, or should I just apply it to master
> if you don't have any other pending sparc patches?

Yes, sorry - it has been on my TODO list for a while to look at this, but I've been
quite short of time these past few weeks. I should be able to run it through my SPARC
test images before the end of the week if that helps?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/7] target/sparc: Convert to do_transaction_failed hook

[Peter Maydell]

On Tue, 3 Sep 2019 at 18:00, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 03/09/2019 14:15, Peter Maydell wrote:
>
> > Mark -- ping? Richard has reviewed this series; do you want
> > more time to test it, or should I just apply it to master
> > if you don't have any other pending sparc patches?
>
> Yes, sorry - it has been on my TODO list for a while to look at this, but I've been
> quite short of time these past few weeks. I should be able to run it through my SPARC
> test images before the end of the week if that helps?

Yeah, that would be great. It's not hugely urgent, but I would
like to get this refactoring complete some time this
release cycle so we can drop the old hooks.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/7] target/sparc: Convert to do_transaction_failed hook

[Mark Cave-Ayland]

On 01/08/2019 19:30, Peter Maydell wrote:

> This patchset converts the SPARC target away from the old
> broken do_unassigned_access hook to the new (added in 2017...)
> do_transaction_failed hook. In the process it fixes a number
> of bugs in corner cases.
> 
> The SPARC ld/st-with-ASI helper functions are odd in that they
> make use of the cpu_unassigned_access() function as a way of
> raising an MMU fault. We start by making them just directly
> call a new sparc_raise_mmu_fault() function so they don't go
> through the hook function. This in-passing fixes a bug where
> the hook was using GETPC(), which won't work inside a function
> several levels deep in the callstack from a helper function.
> 
> The next four patches convert places that were using ld*_phys()
> and st*_phys() and thus getting "implicitly causes an exception
> via do_unassigned_access if it gets a bus error" behaviour.
> We make them all use address_space_ld*/st* functions instead,
> and explicitly handle the transaction-failure case. Variously:
>  * for MMU passthrough, this doesn't change behaviour
>  * for the MXCC stream source/destination ASI accesses,
>    this doesn't change behaviour, but the current behaviour
>    looks a bit weird, so a TODO comment is left in case anybody
>    wants to chase up the actual right behaviour in future
>  * for page table walks this fixes a bug where we would take
>    an exception instead of reporting the page table walk failure
>    with the correct fault status register information
>  * for the page table walk in mmu_probe() this fixes a bug where
>    we would take an exception when we are supposed to just report
>    failure. Note that the spec says that MMU probe operations
>    are supposed to set the fault status registers, but we do not;
>    again I leave this pre-existing bug as a TODO note.
> Next, we remove one entirely pointless and unused ldl_phys()
> call from dump_mmu().
> 
> Finally, the last patch can do the very small amount of work to
> change over to the new hook function. This will cause all the
> "handle error" code paths in the preceding patches to become
> active (when a do_unassigned_access hook is present the load
> or store functions will never return an error, because the hook
> will get called and throw an exception first).
> 
> I have tested that I can boot a sparc32 debian image, and
> that sparc64 boots up to the firmware prompt, but this could
> certainly use more extensive testing than I have given it.
> 
> (After SPARC, the only remaining unassigned-access-hook users
> are RISCV, which crept in while I wasn't looking, and MIPS,
> which is doing something complicated with the Jazz board that
> I haven't yet investigated.)
> 
> thanks
> --PMM
> 
> Peter Maydell (7):
>   target/sparc: Factor out the body of sparc_cpu_unassigned_access()
>   target/sparc: Check for transaction failures in MMU passthrough ASIs
>   target/sparc: Check for transaction failures in MXCC stream ASI
>     accesses
>   target/sparc: Correctly handle bus errors in page table walks
>   target/sparc: Handle bus errors in mmu_probe()
>   target/sparc: Remove unused ldl_phys from dump_mmu()
>   target/sparc: Switch to do_transaction_failed() hook
> 
>  target/sparc/cpu.h         |   8 +-
>  target/sparc/cpu.c         |   2 +-
>  target/sparc/ldst_helper.c | 319 +++++++++++++++++++++----------------
>  target/sparc/mmu_helper.c  |  57 +++++--
>  4 files changed, 238 insertions(+), 148 deletions(-)

I've just run this through my SPARC test images with a fairly recent git master and I
don't see any obvious regressions so:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks for taking the time to do the conversion for the SPARC target.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/7] target/sparc: Convert to do_transaction_failed hook

[Peter Maydell]

On Fri, 6 Sep 2019 at 15:34, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 01/08/2019 19:30, Peter Maydell wrote:
>
> > This patchset converts the SPARC target away from the old
> > broken do_unassigned_access hook to the new (added in 2017...)
> > do_transaction_failed hook. In the process it fixes a number
> > of bugs in corner cases.

> > Peter Maydell (7):
> >   target/sparc: Factor out the body of sparc_cpu_unassigned_access()
> >   target/sparc: Check for transaction failures in MMU passthrough ASIs
> >   target/sparc: Check for transaction failures in MXCC stream ASI
> >     accesses
> >   target/sparc: Correctly handle bus errors in page table walks
> >   target/sparc: Handle bus errors in mmu_probe()
> >   target/sparc: Remove unused ldl_phys from dump_mmu()
> >   target/sparc: Switch to do_transaction_failed() hook
> >
> >  target/sparc/cpu.h         |   8 +-
> >  target/sparc/cpu.c         |   2 +-
> >  target/sparc/ldst_helper.c | 319 +++++++++++++++++++++----------------
> >  target/sparc/mmu_helper.c  |  57 +++++--
> >  4 files changed, 238 insertions(+), 148 deletions(-)
>
> I've just run this through my SPARC test images with a fairly recent git master and I
> don't see any obvious regressions so:
>
> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> Thanks for taking the time to do the conversion for the SPARC target.

Thanks; I've applied the patchset to master.

-- PMM