[Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Peter Maydell]

This patchset converts the m68k target from the deprecated
unassigned_access hook to the new transaction_failed hook.
It's RFC for a couple of reasons:
 * it's untested, since I don't have an m68k test image
 * the second patch just makes "bus error while trying to
   read page tables" be treated as a page fault, when it
   should probably cause a fault reporting it as a bus error
   of some kind
 * I don't understand why the old unassigned_access hook
   set the ATC bit in the MMU SSW, since the docs I have say
   this should be set if the fault happened during a table
   search, but cleared if it's just an ordinary bus-errored
   data or insn access. Probably this is a pre-existing bug?

Anyway, I send it out as a skeleton for comments, because
it would be nice to get rid of the old unassigned_access
hook, which is fundamentally broken (it's still used by m68k,
microblaze, mips and sparc).

thanks
-- PMM

Peter Maydell (3):
  target/m68k: In dump_address_map() check for memory access failures
  target/m68k: In get_physical_address() check for memory access
    failures
  target/m68k: Switch to transaction_failed hook

 target/m68k/cpu.h       |  7 ++--
 target/m68k/cpu.c       |  2 +-
 target/m68k/helper.c    | 84 ++++++++++++++++++++++++++++++++---------
 target/m68k/op_helper.c | 20 ++++------
 4 files changed, 80 insertions(+), 33 deletions(-)

-- 
2.19.2

[Qemu-devel] [RFC 1/3] target/m68k: In dump_address_map() check for memory access failures

[Peter Maydell]

In dump_address_map(), use address_space_ldl() instead of ldl_phys().
This allows us to check whether the memory access failed.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/m68k/helper.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 917d46efcc3..374e4861886 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -411,6 +411,7 @@ static void dump_address_map(FILE *f, fprintf_function cpu_fprintf,
     int last_attr = -1, attr = -1;
     M68kCPU *cpu = m68k_env_get_cpu(env);
     CPUState *cs = CPU(cpu);
+    MemTxResult txres;
 
     if (env->mmu.tcr & M68K_TCR_PAGE_8K) {
         /* 8k page */
@@ -424,22 +425,29 @@ static void dump_address_map(FILE *f, fprintf_function cpu_fprintf,
         tib_mask = M68K_4K_PAGE_MASK;
     }
     for (i = 0; i < M68K_ROOT_POINTER_ENTRIES; i++) {
-        tia = ldl_phys(cs->as, M68K_POINTER_BASE(root_pointer) + i * 4);
-        if (!M68K_UDT_VALID(tia)) {
+        tia = address_space_ldl(cs->as, M68K_POINTER_BASE(root_pointer) + i * 4,
+                                MEMTXATTRS_UNSPECIFIED, &txres);
+        if (txres != MEMTX_OK || !M68K_UDT_VALID(tia)) {
             continue;
         }
         for (j = 0; j < M68K_ROOT_POINTER_ENTRIES; j++) {
-            tib = ldl_phys(cs->as, M68K_POINTER_BASE(tia) + j * 4);
-            if (!M68K_UDT_VALID(tib)) {
+            tib = address_space_ldl(cs->as, M68K_POINTER_BASE(tia) + j * 4,
+                                    MEMTXATTRS_UNSPECIFIED, &txres);
+            if (txres != MEMTX_OK || !M68K_UDT_VALID(tib)) {
                 continue;
             }
             for (k = 0; k < tic_size; k++) {
-                tic = ldl_phys(cs->as, (tib & tib_mask) + k * 4);
-                if (!M68K_PDT_VALID(tic)) {
+                tic = address_space_ldl(cs->as, (tib & tib_mask) + k * 4,
+                                        MEMTXATTRS_UNSPECIFIED, &txres);
+                if (txres != MEMTX_OK || !M68K_PDT_VALID(tic)) {
                     continue;
                 }
                 if (M68K_PDT_INDIRECT(tic)) {
-                    tic = ldl_phys(cs->as, M68K_INDIRECT_POINTER(tic));
+                    tic = address_space_ldl(cs->as, M68K_INDIRECT_POINTER(tic),
+                                            MEMTXATTRS_UNSPECIFIED, &txres);
+                    if (txres != MEMTX_OK) {
+                        continue;
+                    }
                 }
 
                 last_logical = logical;
-- 
2.19.2

Re: [Qemu-devel] [RFC 1/3] target/m68k: In dump_address_map() check for memory access failures

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> In dump_address_map(), use address_space_ldl() instead of ldl_phys().
> This allows us to check whether the memory access failed.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/m68k/helper.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/target/m68k/helper.c b/target/m68k/helper.c
> index 917d46efcc3..374e4861886 100644
> --- a/target/m68k/helper.c
> +++ b/target/m68k/helper.c
> @@ -411,6 +411,7 @@ static void dump_address_map(FILE *f, fprintf_function cpu_fprintf,
>       int last_attr = -1, attr = -1;
>       M68kCPU *cpu = m68k_env_get_cpu(env);
>       CPUState *cs = CPU(cpu);
> +    MemTxResult txres;
>   
>       if (env->mmu.tcr & M68K_TCR_PAGE_8K) {
>           /* 8k page */
> @@ -424,22 +425,29 @@ static void dump_address_map(FILE *f, fprintf_function cpu_fprintf,
>           tib_mask = M68K_4K_PAGE_MASK;
>       }
>       for (i = 0; i < M68K_ROOT_POINTER_ENTRIES; i++) {
> -        tia = ldl_phys(cs->as, M68K_POINTER_BASE(root_pointer) + i * 4);
> -        if (!M68K_UDT_VALID(tia)) {
> +        tia = address_space_ldl(cs->as, M68K_POINTER_BASE(root_pointer) + i * 4,
> +                                MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK || !M68K_UDT_VALID(tia)) {
>               continue;
>           }
>           for (j = 0; j < M68K_ROOT_POINTER_ENTRIES; j++) {
> -            tib = ldl_phys(cs->as, M68K_POINTER_BASE(tia) + j * 4);
> -            if (!M68K_UDT_VALID(tib)) {
> +            tib = address_space_ldl(cs->as, M68K_POINTER_BASE(tia) + j * 4,
> +                                    MEMTXATTRS_UNSPECIFIED, &txres);
> +            if (txres != MEMTX_OK || !M68K_UDT_VALID(tib)) {
>                   continue;
>               }
>               for (k = 0; k < tic_size; k++) {
> -                tic = ldl_phys(cs->as, (tib & tib_mask) + k * 4);
> -                if (!M68K_PDT_VALID(tic)) {
> +                tic = address_space_ldl(cs->as, (tib & tib_mask) + k * 4,
> +                                        MEMTXATTRS_UNSPECIFIED, &txres);
> +                if (txres != MEMTX_OK || !M68K_PDT_VALID(tic)) {
>                       continue;
>                   }
>                   if (M68K_PDT_INDIRECT(tic)) {
> -                    tic = ldl_phys(cs->as, M68K_INDIRECT_POINTER(tic));
> +                    tic = address_space_ldl(cs->as, M68K_INDIRECT_POINTER(tic),
> +                                            MEMTXATTRS_UNSPECIFIED, &txres);
> +                    if (txres != MEMTX_OK) {
> +                        continue;
> +                    }
>                   }
>   
>                   last_logical = logical;
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [RFC 1/3] target/m68k: In dump_address_map() check for memory access failures

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> In dump_address_map(), use address_space_ldl() instead of ldl_phys().
> This allows us to check whether the memory access failed.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/m68k/helper.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/target/m68k/helper.c b/target/m68k/helper.c
> index 917d46efcc3..374e4861886 100644
> --- a/target/m68k/helper.c
> +++ b/target/m68k/helper.c
> @@ -411,6 +411,7 @@ static void dump_address_map(FILE *f, fprintf_function cpu_fprintf,
>       int last_attr = -1, attr = -1;
>       M68kCPU *cpu = m68k_env_get_cpu(env);
>       CPUState *cs = CPU(cpu);
> +    MemTxResult txres;
>   
>       if (env->mmu.tcr & M68K_TCR_PAGE_8K) {
>           /* 8k page */
> @@ -424,22 +425,29 @@ static void dump_address_map(FILE *f, fprintf_function cpu_fprintf,
>           tib_mask = M68K_4K_PAGE_MASK;
>       }
>       for (i = 0; i < M68K_ROOT_POINTER_ENTRIES; i++) {
> -        tia = ldl_phys(cs->as, M68K_POINTER_BASE(root_pointer) + i * 4);
> -        if (!M68K_UDT_VALID(tia)) {
> +        tia = address_space_ldl(cs->as, M68K_POINTER_BASE(root_pointer) + i * 4,
> +                                MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK || !M68K_UDT_VALID(tia)) {
>               continue;
>           }
>           for (j = 0; j < M68K_ROOT_POINTER_ENTRIES; j++) {
> -            tib = ldl_phys(cs->as, M68K_POINTER_BASE(tia) + j * 4);
> -            if (!M68K_UDT_VALID(tib)) {
> +            tib = address_space_ldl(cs->as, M68K_POINTER_BASE(tia) + j * 4,
> +                                    MEMTXATTRS_UNSPECIFIED, &txres);
> +            if (txres != MEMTX_OK || !M68K_UDT_VALID(tib)) {
>                   continue;
>               }
>               for (k = 0; k < tic_size; k++) {
> -                tic = ldl_phys(cs->as, (tib & tib_mask) + k * 4);
> -                if (!M68K_PDT_VALID(tic)) {
> +                tic = address_space_ldl(cs->as, (tib & tib_mask) + k * 4,
> +                                        MEMTXATTRS_UNSPECIFIED, &txres);
> +                if (txres != MEMTX_OK || !M68K_PDT_VALID(tic)) {
>                       continue;
>                   }
>                   if (M68K_PDT_INDIRECT(tic)) {
> -                    tic = ldl_phys(cs->as, M68K_INDIRECT_POINTER(tic));
> +                    tic = address_space_ldl(cs->as, M68K_INDIRECT_POINTER(tic),
> +                                            MEMTXATTRS_UNSPECIFIED, &txres);
> +                    if (txres != MEMTX_OK) {
> +                        continue;
> +                    }
>                   }
>   
>                   last_logical = logical;
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[Qemu-devel] [RFC 2/3] target/m68k: In get_physical_address() check for memory access failures

[Peter Maydell]

In get_physical_address(), use address_space_ldl() and
address_space_stl() instead of ldl_phys() and stl_phys().
This allows us to check whether the memory access failed.
For the moment, we simply return -1 in this case;
add a TODO comment that we should ideally generate the
appropriate kind of fault.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/m68k/helper.c | 62 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index 374e4861886..b5fa2f8056d 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -660,6 +660,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
     bool debug = access_type & ACCESS_DEBUG;
     int page_bits;
     int i;
+    MemTxResult txres;
 
     /* Transparent Translation (physical = logical) */
     for (i = 0; i < M68K_MAX_TTR; i++) {
@@ -689,12 +690,19 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
     /* Root Index */
     entry = M68K_POINTER_BASE(next) | M68K_ROOT_INDEX(address);
 
-    next = ldl_phys(cs->as, entry);
+    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
+    if (txres != MEMTX_OK) {
+        goto txfail;
+    }
     if (!M68K_UDT_VALID(next)) {
         return -1;
     }
     if (!(next & M68K_DESC_USED) && !debug) {
-        stl_phys(cs->as, entry, next | M68K_DESC_USED);
+        address_space_stl(cs->as, entry, next | M68K_DESC_USED,
+                          MEMTXATTRS_UNSPECIFIED, &txres);
+        if (txres != MEMTX_OK) {
+            goto txfail;
+        }
     }
     if (next & M68K_DESC_WRITEPROT) {
         if (access_type & ACCESS_PTEST) {
@@ -709,12 +717,19 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
     /* Pointer Index */
     entry = M68K_POINTER_BASE(next) | M68K_POINTER_INDEX(address);
 
-    next = ldl_phys(cs->as, entry);
+    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
+    if (txres != MEMTX_OK) {
+        goto txfail;
+    }
     if (!M68K_UDT_VALID(next)) {
         return -1;
     }
     if (!(next & M68K_DESC_USED) && !debug) {
-        stl_phys(cs->as, entry, next | M68K_DESC_USED);
+        address_space_stl(cs->as, entry, next | M68K_DESC_USED,
+                          MEMTXATTRS_UNSPECIFIED, &txres);
+        if (txres != MEMTX_OK) {
+            goto txfail;
+        }
     }
     if (next & M68K_DESC_WRITEPROT) {
         if (access_type & ACCESS_PTEST) {
@@ -733,27 +748,46 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
         entry = M68K_4K_PAGE_BASE(next) | M68K_4K_PAGE_INDEX(address);
     }
 
-    next = ldl_phys(cs->as, entry);
+    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
+    if (txres != MEMTX_OK) {
+        goto txfail;
+    }
 
     if (!M68K_PDT_VALID(next)) {
         return -1;
     }
     if (M68K_PDT_INDIRECT(next)) {
-        next = ldl_phys(cs->as, M68K_INDIRECT_POINTER(next));
+        next = address_space_ldl(cs->as, M68K_INDIRECT_POINTER(next),
+                                 MEMTXATTRS_UNSPECIFIED, &txres);
+        if (txres != MEMTX_OK) {
+            goto txfail;
+        }
     }
     if (access_type & ACCESS_STORE) {
         if (next & M68K_DESC_WRITEPROT) {
             if (!(next & M68K_DESC_USED) && !debug) {
-                stl_phys(cs->as, entry, next | M68K_DESC_USED);
+                address_space_stl(cs->as, entry, next | M68K_DESC_USED,
+                                  MEMTXATTRS_UNSPECIFIED, &txres);
+                if (txres != MEMTX_OK) {
+                    goto txfail;
+                }
             }
         } else if ((next & (M68K_DESC_MODIFIED | M68K_DESC_USED)) !=
                            (M68K_DESC_MODIFIED | M68K_DESC_USED) && !debug) {
-                stl_phys(cs->as, entry,
-                         next | (M68K_DESC_MODIFIED | M68K_DESC_USED));
+            address_space_stl(cs->as, entry,
+                              next | (M68K_DESC_MODIFIED | M68K_DESC_USED),
+                              MEMTXATTRS_UNSPECIFIED, &txres);
+            if (txres != MEMTX_OK) {
+                goto txfail;
+            }
         }
     } else {
         if (!(next & M68K_DESC_USED) && !debug) {
-            stl_phys(cs->as, entry, next | M68K_DESC_USED);
+            address_space_stl(cs->as, entry, next | M68K_DESC_USED,
+                              MEMTXATTRS_UNSPECIFIED, &txres);
+            if (txres != MEMTX_OK) {
+                goto txfail;
+            }
         }
     }
 
@@ -785,6 +819,14 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
     }
 
     return 0;
+
+txfail:
+    /*
+     * A page table load/store failed. TODO: we should really raise a
+     * suitable guest fault here if this is not a debug access.
+     * For now just return that the translation failed.
+     */
+    return -1;
 }
 
 hwaddr m68k_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
-- 
2.19.2

Re: [Qemu-devel] [RFC 2/3] target/m68k: In get_physical_address() check for memory access failures

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> In get_physical_address(), use address_space_ldl() and
> address_space_stl() instead of ldl_phys() and stl_phys().
> This allows us to check whether the memory access failed.
> For the moment, we simply return -1 in this case;
> add a TODO comment that we should ideally generate the
> appropriate kind of fault.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/m68k/helper.c | 62 +++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/target/m68k/helper.c b/target/m68k/helper.c
> index 374e4861886..b5fa2f8056d 100644
> --- a/target/m68k/helper.c
> +++ b/target/m68k/helper.c
> @@ -660,6 +660,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       bool debug = access_type & ACCESS_DEBUG;
>       int page_bits;
>       int i;
> +    MemTxResult txres;
>   
>       /* Transparent Translation (physical = logical) */
>       for (i = 0; i < M68K_MAX_TTR; i++) {
> @@ -689,12 +690,19 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       /* Root Index */
>       entry = M68K_POINTER_BASE(next) | M68K_ROOT_INDEX(address);
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>       if (!M68K_UDT_VALID(next)) {
>           return -1;
>       }
>       if (!(next & M68K_DESC_USED) && !debug) {
> -        stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +        address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                          MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (next & M68K_DESC_WRITEPROT) {
>           if (access_type & ACCESS_PTEST) {
> @@ -709,12 +717,19 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       /* Pointer Index */
>       entry = M68K_POINTER_BASE(next) | M68K_POINTER_INDEX(address);
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>       if (!M68K_UDT_VALID(next)) {
>           return -1;
>       }
>       if (!(next & M68K_DESC_USED) && !debug) {
> -        stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +        address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                          MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (next & M68K_DESC_WRITEPROT) {
>           if (access_type & ACCESS_PTEST) {
> @@ -733,27 +748,46 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>           entry = M68K_4K_PAGE_BASE(next) | M68K_4K_PAGE_INDEX(address);
>       }
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>   
>       if (!M68K_PDT_VALID(next)) {
>           return -1;
>       }
>       if (M68K_PDT_INDIRECT(next)) {
> -        next = ldl_phys(cs->as, M68K_INDIRECT_POINTER(next));
> +        next = address_space_ldl(cs->as, M68K_INDIRECT_POINTER(next),
> +                                 MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (access_type & ACCESS_STORE) {
>           if (next & M68K_DESC_WRITEPROT) {
>               if (!(next & M68K_DESC_USED) && !debug) {
> -                stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +                address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                                  MEMTXATTRS_UNSPECIFIED, &txres);
> +                if (txres != MEMTX_OK) {
> +                    goto txfail;
> +                }
>               }
>           } else if ((next & (M68K_DESC_MODIFIED | M68K_DESC_USED)) !=
>                              (M68K_DESC_MODIFIED | M68K_DESC_USED) && !debug) {
> -                stl_phys(cs->as, entry,
> -                         next | (M68K_DESC_MODIFIED | M68K_DESC_USED));
> +            address_space_stl(cs->as, entry,
> +                              next | (M68K_DESC_MODIFIED | M68K_DESC_USED),
> +                              MEMTXATTRS_UNSPECIFIED, &txres);
> +            if (txres != MEMTX_OK) {
> +                goto txfail;
> +            }
>           }
>       } else {
>           if (!(next & M68K_DESC_USED) && !debug) {
> -            stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +            address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                              MEMTXATTRS_UNSPECIFIED, &txres);
> +            if (txres != MEMTX_OK) {
> +                goto txfail;
> +            }
>           }
>       }
>   
> @@ -785,6 +819,14 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       }
>   
>       return 0;
> +
> +txfail:
> +    /*
> +     * A page table load/store failed. TODO: we should really raise a
> +     * suitable guest fault here if this is not a debug access.
> +     * For now just return that the translation failed.
> +     */
> +    return -1;
>   }
>   
>   hwaddr m68k_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [RFC 2/3] target/m68k: In get_physical_address() check for memory access failures

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> In get_physical_address(), use address_space_ldl() and
> address_space_stl() instead of ldl_phys() and stl_phys().
> This allows us to check whether the memory access failed.
> For the moment, we simply return -1 in this case;
> add a TODO comment that we should ideally generate the
> appropriate kind of fault.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/m68k/helper.c | 62 +++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/target/m68k/helper.c b/target/m68k/helper.c
> index 374e4861886..b5fa2f8056d 100644
> --- a/target/m68k/helper.c
> +++ b/target/m68k/helper.c
> @@ -660,6 +660,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       bool debug = access_type & ACCESS_DEBUG;
>       int page_bits;
>       int i;
> +    MemTxResult txres;
>   
>       /* Transparent Translation (physical = logical) */
>       for (i = 0; i < M68K_MAX_TTR; i++) {
> @@ -689,12 +690,19 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       /* Root Index */
>       entry = M68K_POINTER_BASE(next) | M68K_ROOT_INDEX(address);
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>       if (!M68K_UDT_VALID(next)) {
>           return -1;
>       }
>       if (!(next & M68K_DESC_USED) && !debug) {
> -        stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +        address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                          MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (next & M68K_DESC_WRITEPROT) {
>           if (access_type & ACCESS_PTEST) {
> @@ -709,12 +717,19 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       /* Pointer Index */
>       entry = M68K_POINTER_BASE(next) | M68K_POINTER_INDEX(address);
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>       if (!M68K_UDT_VALID(next)) {
>           return -1;
>       }
>       if (!(next & M68K_DESC_USED) && !debug) {
> -        stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +        address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                          MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (next & M68K_DESC_WRITEPROT) {
>           if (access_type & ACCESS_PTEST) {
> @@ -733,27 +748,46 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>           entry = M68K_4K_PAGE_BASE(next) | M68K_4K_PAGE_INDEX(address);
>       }
>   
> -    next = ldl_phys(cs->as, entry);
> +    next = address_space_ldl(cs->as, entry, MEMTXATTRS_UNSPECIFIED, &txres);
> +    if (txres != MEMTX_OK) {
> +        goto txfail;
> +    }
>   
>       if (!M68K_PDT_VALID(next)) {
>           return -1;
>       }
>       if (M68K_PDT_INDIRECT(next)) {
> -        next = ldl_phys(cs->as, M68K_INDIRECT_POINTER(next));
> +        next = address_space_ldl(cs->as, M68K_INDIRECT_POINTER(next),
> +                                 MEMTXATTRS_UNSPECIFIED, &txres);
> +        if (txres != MEMTX_OK) {
> +            goto txfail;
> +        }
>       }
>       if (access_type & ACCESS_STORE) {
>           if (next & M68K_DESC_WRITEPROT) {
>               if (!(next & M68K_DESC_USED) && !debug) {
> -                stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +                address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                                  MEMTXATTRS_UNSPECIFIED, &txres);
> +                if (txres != MEMTX_OK) {
> +                    goto txfail;
> +                }
>               }
>           } else if ((next & (M68K_DESC_MODIFIED | M68K_DESC_USED)) !=
>                              (M68K_DESC_MODIFIED | M68K_DESC_USED) && !debug) {
> -                stl_phys(cs->as, entry,
> -                         next | (M68K_DESC_MODIFIED | M68K_DESC_USED));
> +            address_space_stl(cs->as, entry,
> +                              next | (M68K_DESC_MODIFIED | M68K_DESC_USED),
> +                              MEMTXATTRS_UNSPECIFIED, &txres);
> +            if (txres != MEMTX_OK) {
> +                goto txfail;
> +            }
>           }
>       } else {
>           if (!(next & M68K_DESC_USED) && !debug) {
> -            stl_phys(cs->as, entry, next | M68K_DESC_USED);
> +            address_space_stl(cs->as, entry, next | M68K_DESC_USED,
> +                              MEMTXATTRS_UNSPECIFIED, &txres);
> +            if (txres != MEMTX_OK) {
> +                goto txfail;
> +            }
>           }
>       }
>   
> @@ -785,6 +819,14 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical,
>       }
>   
>       return 0;
> +
> +txfail:
> +    /*
> +     * A page table load/store failed. TODO: we should really raise a
> +     * suitable guest fault here if this is not a debug access.
> +     * For now just return that the translation failed.
> +     */
> +    return -1;
>   }
>   
>   hwaddr m68k_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[Qemu-devel] [RFC 3/3] target/m68k: Switch to transaction_failed hook

[Peter Maydell]

Switch the m68k target from the old unassigned_access hook
to the transaction_failed hook.

The notable difference is that rather than it being called
for all physical memory accesses which fail (including
those made by DMA devices or by the gdbstub), it is only
called for those made by the CPU via its MMU. (In previous
commits we put in explicit checks for the direct physical
loads made by the target/m68k code which will no longer
be handled by calling the unassigned_access hook.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/m68k/cpu.h       |  7 ++++---
 target/m68k/cpu.c       |  2 +-
 target/m68k/op_helper.c | 20 ++++++++------------
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index b288a3864e0..08828b0581b 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -545,9 +545,10 @@ static inline int cpu_mmu_index (CPUM68KState *env, bool ifetch)
 
 int m68k_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw,
                               int mmu_idx);
-void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr,
-                                bool is_write, bool is_exec, int is_asi,
-                                unsigned size);
+void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
+                                 unsigned size, MMUAccessType access_type,
+                                 int mmu_idx, MemTxAttrs attrs,
+                                 MemTxResult response, uintptr_t retaddr);
 
 #include "exec/cpu-all.h"
 
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 582e3a73b37..6d09c630b0e 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -271,7 +271,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_write_register = m68k_cpu_gdb_write_register;
     cc->handle_mmu_fault = m68k_cpu_handle_mmu_fault;
 #if defined(CONFIG_SOFTMMU)
-    cc->do_unassigned_access = m68k_cpu_unassigned_access;
+    cc->do_transaction_failed = m68k_cpu_transaction_failed;
     cc->get_phys_page_debug = m68k_cpu_get_phys_page_debug;
 #endif
     cc->disas_set_info = m68k_cpu_disas_set_info;
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 8d09ed91c49..6739ab8e436 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -454,19 +454,15 @@ static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
     do_interrupt_all(env, 1);
 }
 
-void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
-                                bool is_exec, int is_asi, unsigned size)
+void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
+                                 unsigned size, MMUAccessType access_type,
+                                 int mmu_idx, MemTxAttrs attrs,
+                                 MemTxResult response, uintptr_t retaddr)
 {
     M68kCPU *cpu = M68K_CPU(cs);
     CPUM68KState *env = &cpu->env;
-#ifdef DEBUG_UNASSIGNED
-    qemu_log_mask(CPU_LOG_INT, "Unassigned " TARGET_FMT_plx " wr=%d exe=%d\n",
-             addr, is_write, is_exec);
-#endif
-    if (env == NULL) {
-        /* when called from gdb, env is NULL */
-        return;
-    }
+
+    cpu_restore_state(cs, retaddr, true);
 
     if (m68k_feature(env, M68K_FEATURE_M68040)) {
         env->mmu.mmusr = 0;
@@ -476,7 +472,7 @@ void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
         if (env->sr & SR_S) { /* SUPERVISOR */
             env->mmu.ssw |= M68K_TM_040_SUPER;
         }
-        if (is_exec) { /* instruction or data */
+        if (access_type == MMU_INST_FETCH) { /* instruction or data */
             env->mmu.ssw |= M68K_TM_040_CODE;
         } else {
             env->mmu.ssw |= M68K_TM_040_DATA;
@@ -494,7 +490,7 @@ void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
             break;
         }
 
-        if (!is_write) {
+        if (access_type != MMU_DATA_STORE) {
             env->mmu.ssw |= M68K_RW_040;
         }
 
-- 
2.19.2

Re: [Qemu-devel] [RFC 3/3] target/m68k: Switch to transaction_failed hook

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> Switch the m68k target from the old unassigned_access hook
> to the transaction_failed hook.
> 
> The notable difference is that rather than it being called
> for all physical memory accesses which fail (including
> those made by DMA devices or by the gdbstub), it is only
> called for those made by the CPU via its MMU. (In previous
> commits we put in explicit checks for the direct physical
> loads made by the target/m68k code which will no longer
> be handled by calling the unassigned_access hook.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/m68k/cpu.h       |  7 ++++---
>   target/m68k/cpu.c       |  2 +-
>   target/m68k/op_helper.c | 20 ++++++++------------
>   3 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index b288a3864e0..08828b0581b 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -545,9 +545,10 @@ static inline int cpu_mmu_index (CPUM68KState *env, bool ifetch)
>   
>   int m68k_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw,
>                                 int mmu_idx);
> -void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr,
> -                                bool is_write, bool is_exec, int is_asi,
> -                                unsigned size);
> +void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
> +                                 unsigned size, MMUAccessType access_type,
> +                                 int mmu_idx, MemTxAttrs attrs,
> +                                 MemTxResult response, uintptr_t retaddr);
>   
>   #include "exec/cpu-all.h"
>   
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 582e3a73b37..6d09c630b0e 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -271,7 +271,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>       cc->gdb_write_register = m68k_cpu_gdb_write_register;
>       cc->handle_mmu_fault = m68k_cpu_handle_mmu_fault;
>   #if defined(CONFIG_SOFTMMU)
> -    cc->do_unassigned_access = m68k_cpu_unassigned_access;
> +    cc->do_transaction_failed = m68k_cpu_transaction_failed;
>       cc->get_phys_page_debug = m68k_cpu_get_phys_page_debug;
>   #endif
>       cc->disas_set_info = m68k_cpu_disas_set_info;
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 8d09ed91c49..6739ab8e436 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -454,19 +454,15 @@ static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
>       do_interrupt_all(env, 1);
>   }
>   
> -void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
> -                                bool is_exec, int is_asi, unsigned size)
> +void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
> +                                 unsigned size, MMUAccessType access_type,
> +                                 int mmu_idx, MemTxAttrs attrs,
> +                                 MemTxResult response, uintptr_t retaddr)
>   {
>       M68kCPU *cpu = M68K_CPU(cs);
>       CPUM68KState *env = &cpu->env;
> -#ifdef DEBUG_UNASSIGNED
> -    qemu_log_mask(CPU_LOG_INT, "Unassigned " TARGET_FMT_plx " wr=%d exe=%d\n",
> -             addr, is_write, is_exec);
> -#endif
> -    if (env == NULL) {
> -        /* when called from gdb, env is NULL */
> -        return;
> -    }
> +
> +    cpu_restore_state(cs, retaddr, true);
>   
>       if (m68k_feature(env, M68K_FEATURE_M68040)) {
>           env->mmu.mmusr = 0;
> @@ -476,7 +472,7 @@ void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
>           if (env->sr & SR_S) { /* SUPERVISOR */
>               env->mmu.ssw |= M68K_TM_040_SUPER;
>           }
> -        if (is_exec) { /* instruction or data */
> +        if (access_type == MMU_INST_FETCH) { /* instruction or data */
>               env->mmu.ssw |= M68K_TM_040_CODE;
>           } else {
>               env->mmu.ssw |= M68K_TM_040_DATA;
> @@ -494,7 +490,7 @@ void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
>               break;
>           }
>   
> -        if (!is_write) {
> +        if (access_type != MMU_DATA_STORE) {
>               env->mmu.ssw |= M68K_RW_040;
>           }
>   
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [RFC 3/3] target/m68k: Switch to transaction_failed hook

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> Switch the m68k target from the old unassigned_access hook
> to the transaction_failed hook.
> 
> The notable difference is that rather than it being called
> for all physical memory accesses which fail (including
> those made by DMA devices or by the gdbstub), it is only
> called for those made by the CPU via its MMU. (In previous
> commits we put in explicit checks for the direct physical
> loads made by the target/m68k code which will no longer
> be handled by calling the unassigned_access hook.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/m68k/cpu.h       |  7 ++++---
>   target/m68k/cpu.c       |  2 +-
>   target/m68k/op_helper.c | 20 ++++++++------------
>   3 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index b288a3864e0..08828b0581b 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -545,9 +545,10 @@ static inline int cpu_mmu_index (CPUM68KState *env, bool ifetch)
>   
>   int m68k_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw,
>                                 int mmu_idx);
> -void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr,
> -                                bool is_write, bool is_exec, int is_asi,
> -                                unsigned size);
> +void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
> +                                 unsigned size, MMUAccessType access_type,
> +                                 int mmu_idx, MemTxAttrs attrs,
> +                                 MemTxResult response, uintptr_t retaddr);
>   
>   #include "exec/cpu-all.h"
>   
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 582e3a73b37..6d09c630b0e 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -271,7 +271,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>       cc->gdb_write_register = m68k_cpu_gdb_write_register;
>       cc->handle_mmu_fault = m68k_cpu_handle_mmu_fault;
>   #if defined(CONFIG_SOFTMMU)
> -    cc->do_unassigned_access = m68k_cpu_unassigned_access;
> +    cc->do_transaction_failed = m68k_cpu_transaction_failed;
>       cc->get_phys_page_debug = m68k_cpu_get_phys_page_debug;
>   #endif
>       cc->disas_set_info = m68k_cpu_disas_set_info;
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 8d09ed91c49..6739ab8e436 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -454,19 +454,15 @@ static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
>       do_interrupt_all(env, 1);
>   }
>   
> -void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
> -                                bool is_exec, int is_asi, unsigned size)
> +void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
> +                                 unsigned size, MMUAccessType access_type,
> +                                 int mmu_idx, MemTxAttrs attrs,
> +                                 MemTxResult response, uintptr_t retaddr)
>   {
>       M68kCPU *cpu = M68K_CPU(cs);
>       CPUM68KState *env = &cpu->env;
> -#ifdef DEBUG_UNASSIGNED
> -    qemu_log_mask(CPU_LOG_INT, "Unassigned " TARGET_FMT_plx " wr=%d exe=%d\n",
> -             addr, is_write, is_exec);
> -#endif
> -    if (env == NULL) {
> -        /* when called from gdb, env is NULL */
> -        return;
> -    }
> +
> +    cpu_restore_state(cs, retaddr, true);
>   
>       if (m68k_feature(env, M68K_FEATURE_M68040)) {
>           env->mmu.mmusr = 0;
> @@ -476,7 +472,7 @@ void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
>           if (env->sr & SR_S) { /* SUPERVISOR */
>               env->mmu.ssw |= M68K_TM_040_SUPER;
>           }
> -        if (is_exec) { /* instruction or data */
> +        if (access_type == MMU_INST_FETCH) { /* instruction or data */
>               env->mmu.ssw |= M68K_TM_040_CODE;
>           } else {
>               env->mmu.ssw |= M68K_TM_040_DATA;
> @@ -494,7 +490,7 @@ void m68k_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write,
>               break;
>           }
>   
> -        if (!is_write) {
> +        if (access_type != MMU_DATA_STORE) {
>               env->mmu.ssw |= M68K_RW_040;
>           }
>   
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Thomas Huth]

On 2018-12-10 17:56, Peter Maydell wrote:
> This patchset converts the m68k target from the deprecated
> unassigned_access hook to the new transaction_failed hook.
> It's RFC for a couple of reasons:
>  * it's untested, since I don't have an m68k test image

Have a look at the arnewsh 5206 and 5208evb images here:

https://web.archive.org/web/20180427080645/http://www.uclinux.org/ports/coldfire/binary.html

or

https://www.qemu-advent-calendar.org/2018/#day-7 :-)

 Thomas

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Mark Cave-Ayland]

On 10/12/2018 16:56, Peter Maydell wrote:

> This patchset converts the m68k target from the deprecated
> unassigned_access hook to the new transaction_failed hook.
> It's RFC for a couple of reasons:
>  * it's untested, since I don't have an m68k test image
>  * the second patch just makes "bus error while trying to
>    read page tables" be treated as a page fault, when it
>    should probably cause a fault reporting it as a bus error
>    of some kind
>  * I don't understand why the old unassigned_access hook
>    set the ATC bit in the MMU SSW, since the docs I have say
>    this should be set if the fault happened during a table
>    search, but cleared if it's just an ordinary bus-errored
>    data or insn access. Probably this is a pre-existing bug?
> 
> Anyway, I send it out as a skeleton for comments, because
> it would be nice to get rid of the old unassigned_access
> hook, which is fundamentally broken (it's still used by m68k,
> microblaze, mips and sparc).

Laurent is really the expert here (my work on the q800 was purely on the device
side), however is this also a nudge to see if the unassigned_access hook can be
eliminated from sparc too? ;)


ATB,

Mark.

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Peter Maydell]

On Tue, 11 Dec 2018 at 19:13, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 10/12/2018 16:56, Peter Maydell wrote:
> > Anyway, I send it out as a skeleton for comments, because
> > it would be nice to get rid of the old unassigned_access
> > hook, which is fundamentally broken (it's still used by m68k,
> > microblaze, mips and sparc).
>
> Laurent is really the expert here (my work on the q800 was purely on the device
> side), however is this also a nudge to see if the unassigned_access hook can be
> eliminated from sparc too? ;)

It would certainly be great to convert sparc too;
it and mips are a little more complicated than these
ones, but the principle is the same:
 * helper functions in target/sparc which call
   cpu_unassigned_access() should be changed to call
   some sparc-internal function to raise the right
   exception
 * callsites in target/sparc which do loads or stores
   by physical address should be checked to ensure they
   do the right thing when a bus error is detected;
   this usually means changing them to use address_space_*
   functions and check they return MEMTX_OK. (With the
   old unassigned_access hook these would result in calls
   to the hook, which was often the wrong thing anyway.
   The transaction_failed hook is called only for accesses
   via the TCG MMU.) The docs/devel/loads-stores.rst docs
   have some handy regexes for use with 'git grep'; for sparc
   these catch everything:
     git grep '\<ldu\?[bwlq]\(_[bl]e\)\?_phys\>' target/sparc/
     git grep '\<st[bwlq]\(_[bl]e\)\?_phys\>' target/sparc/
 * convert the hook itself: this requires a little fiddling
   of parameters, and the addition of the cpu_restore_state()
   call

(MIPS has some odd board-specific handling on top of that
which will need to be fixed too.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> This patchset converts the m68k target from the deprecated
> unassigned_access hook to the new transaction_failed hook.
> It's RFC for a couple of reasons:
>  * it's untested, since I don't have an m68k test image
>  * the second patch just makes "bus error while trying to
>    read page tables" be treated as a page fault, when it
>    should probably cause a fault reporting it as a bus error
>    of some kind
>  * I don't understand why the old unassigned_access hook
>    set the ATC bit in the MMU SSW, since the docs I have say
>    this should be set if the fault happened during a table
>    search, but cleared if it's just an ordinary bus-errored
>    data or insn access. Probably this is a pre-existing bug?
> 
> Anyway, I send it out as a skeleton for comments, because
> it would be nice to get rid of the old unassigned_access
> hook, which is fundamentally broken (it's still used by m68k,
> microblaze, mips and sparc).
> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>   target/m68k: In dump_address_map() check for memory access failures
>   target/m68k: In get_physical_address() check for memory access
>     failures
>   target/m68k: Switch to transaction_failed hook
> 
>  target/m68k/cpu.h       |  7 ++--
>  target/m68k/cpu.c       |  2 +-
>  target/m68k/helper.c    | 84 ++++++++++++++++++++++++++++++++---------
>  target/m68k/op_helper.c | 20 ++++------
>  4 files changed, 80 insertions(+), 33 deletions(-)
> 

Tested-by: Laurent Vivier <laurent@vivier.eu>

I'll try to review this later...

Thanks,
Laurent

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Peter Maydell]

On Wed, 12 Dec 2018 at 20:43, Laurent Vivier <laurent@vivier.eu> wrote:
>
> On 10/12/2018 17:56, Peter Maydell wrote:
> > This patchset converts the m68k target from the deprecated
> > unassigned_access hook to the new transaction_failed hook.
> > It's RFC for a couple of reasons:
> >  * it's untested, since I don't have an m68k test image
> >  * the second patch just makes "bus error while trying to
> >    read page tables" be treated as a page fault, when it
> >    should probably cause a fault reporting it as a bus error
> >    of some kind
> >  * I don't understand why the old unassigned_access hook
> >    set the ATC bit in the MMU SSW, since the docs I have say
> >    this should be set if the fault happened during a table
> >    search, but cleared if it's just an ordinary bus-errored
> >    data or insn access. Probably this is a pre-existing bug?
> >
> > Anyway, I send it out as a skeleton for comments, because
> > it would be nice to get rid of the old unassigned_access
> > hook, which is fundamentally broken (it's still used by m68k,
> > microblaze, mips and sparc).
> >
> > thanks
> > -- PMM
> >
> > Peter Maydell (3):
> >   target/m68k: In dump_address_map() check for memory access failures
> >   target/m68k: In get_physical_address() check for memory access
> >     failures
> >   target/m68k: Switch to transaction_failed hook
> >
> >  target/m68k/cpu.h       |  7 ++--
> >  target/m68k/cpu.c       |  2 +-
> >  target/m68k/helper.c    | 84 ++++++++++++++++++++++++++++++++---------
> >  target/m68k/op_helper.c | 20 ++++------
> >  4 files changed, 80 insertions(+), 33 deletions(-)
> >
>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
>
> I'll try to review this later...

Ping! Are we at "later" yet ? :-)

I checked with the mbox of the series from
https://patchew.org/QEMU/20181210165636.28366-1-peter.maydell@linaro.org/
and it still applies cleanly to master.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Peter Maydell]

On Wed, 12 Dec 2018 at 20:43, Laurent Vivier <laurent@vivier.eu> wrote:
>
> On 10/12/2018 17:56, Peter Maydell wrote:
> > This patchset converts the m68k target from the deprecated
> > unassigned_access hook to the new transaction_failed hook.
> > It's RFC for a couple of reasons:
> >  * it's untested, since I don't have an m68k test image
> >  * the second patch just makes "bus error while trying to
> >    read page tables" be treated as a page fault, when it
> >    should probably cause a fault reporting it as a bus error
> >    of some kind
> >  * I don't understand why the old unassigned_access hook
> >    set the ATC bit in the MMU SSW, since the docs I have say
> >    this should be set if the fault happened during a table
> >    search, but cleared if it's just an ordinary bus-errored
> >    data or insn access. Probably this is a pre-existing bug?
> >
> > Anyway, I send it out as a skeleton for comments, because
> > it would be nice to get rid of the old unassigned_access
> > hook, which is fundamentally broken (it's still used by m68k,
> > microblaze, mips and sparc).
> >
> > thanks
> > -- PMM
> >
> > Peter Maydell (3):
> >   target/m68k: In dump_address_map() check for memory access failures
> >   target/m68k: In get_physical_address() check for memory access
> >     failures
> >   target/m68k: Switch to transaction_failed hook
> >
> >  target/m68k/cpu.h       |  7 ++--
> >  target/m68k/cpu.c       |  2 +-
> >  target/m68k/helper.c    | 84 ++++++++++++++++++++++++++++++++---------
> >  target/m68k/op_helper.c | 20 ++++------
> >  4 files changed, 80 insertions(+), 33 deletions(-)
> >
>
> Tested-by: Laurent Vivier <laurent@vivier.eu>
>
> I'll try to review this later...

Ping! Are we at "later" yet ? :-)

I checked with the mbox of the series from
https://patchew.org/QEMU/20181210165636.28366-1-peter.maydell@linaro.org/
and it still applies cleanly to master.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> This patchset converts the m68k target from the deprecated
> unassigned_access hook to the new transaction_failed hook.
> It's RFC for a couple of reasons:
>   * it's untested, since I don't have an m68k test image
>   * the second patch just makes "bus error while trying to
>     read page tables" be treated as a page fault, when it
>     should probably cause a fault reporting it as a bus error
>     of some kind
>   * I don't understand why the old unassigned_access hook
>     set the ATC bit in the MMU SSW, since the docs I have say
>     this should be set if the fault happened during a table
>     search, but cleared if it's just an ordinary bus-errored
>     data or insn access. Probably this is a pre-existing bug?

I think you're right. It must be cleared on bus error.

Thanks,
Laurent

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Laurent Vivier]

On 10/12/2018 17:56, Peter Maydell wrote:
> This patchset converts the m68k target from the deprecated
> unassigned_access hook to the new transaction_failed hook.
> It's RFC for a couple of reasons:
>   * it's untested, since I don't have an m68k test image
>   * the second patch just makes "bus error while trying to
>     read page tables" be treated as a page fault, when it
>     should probably cause a fault reporting it as a bus error
>     of some kind
>   * I don't understand why the old unassigned_access hook
>     set the ATC bit in the MMU SSW, since the docs I have say
>     this should be set if the fault happened during a table
>     search, but cleared if it's just an ordinary bus-errored
>     data or insn access. Probably this is a pre-existing bug?

I think you're right. It must be cleared on bus error.

Thanks,
Laurent

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Peter Maydell]

On Fri, 3 May 2019 at 18:12, Laurent Vivier <laurent@vivier.eu> wrote:
>
> On 10/12/2018 17:56, Peter Maydell wrote:
> > This patchset converts the m68k target from the deprecated
> > unassigned_access hook to the new transaction_failed hook.
> > It's RFC for a couple of reasons:
> >   * it's untested, since I don't have an m68k test image
> >   * the second patch just makes "bus error while trying to
> >     read page tables" be treated as a page fault, when it
> >     should probably cause a fault reporting it as a bus error
> >     of some kind
> >   * I don't understand why the old unassigned_access hook
> >     set the ATC bit in the MMU SSW, since the docs I have say
> >     this should be set if the fault happened during a table
> >     search, but cleared if it's just an ordinary bus-errored
> >     data or insn access. Probably this is a pre-existing bug?
>
> I think you're right. It must be cleared on bus error.

Thanks for the review of this patchset. Is there anything
you want me to do for a v2, or is it ready to be applied ?

-- PMM

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

[Laurent Vivier]

On 16/05/2019 15:26, Peter Maydell wrote:
> On Fri, 3 May 2019 at 18:12, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> On 10/12/2018 17:56, Peter Maydell wrote:
>>> This patchset converts the m68k target from the deprecated
>>> unassigned_access hook to the new transaction_failed hook.
>>> It's RFC for a couple of reasons:
>>>    * it's untested, since I don't have an m68k test image
>>>    * the second patch just makes "bus error while trying to
>>>      read page tables" be treated as a page fault, when it
>>>      should probably cause a fault reporting it as a bus error
>>>      of some kind
>>>    * I don't understand why the old unassigned_access hook
>>>      set the ATC bit in the MMU SSW, since the docs I have say
>>>      this should be set if the fault happened during a table
>>>      search, but cleared if it's just an ordinary bus-errored
>>>      data or insn access. Probably this is a pre-existing bug?
>>
>> I think you're right. It must be cleared on bus error.
> 
> Thanks for the review of this patchset. Is there anything
> you want me to do for a v2, or is it ready to be applied ?

It looks good to me. I'm going to add it to my m68k branch and send a PR.

Thanks,
Laurent