diff mbox series

[v2,20/27] target/sparc: Convert to CPUClass::tlb_fill

Message ID 20190509060246.4031-21-richard.henderson@linaro.org
State New
Headers show
Series tcg: Add CPUClass::tlb_fill | expand

Commit Message

Richard Henderson May 9, 2019, 6:02 a.m. UTC 
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
v2: Keep user-only, sparc32, and sparc64 tlb_fill separate.
---
 target/sparc/cpu.h         |  5 ++-
 target/sparc/cpu.c         |  5 +--
 target/sparc/ldst_helper.c | 11 +-----
 target/sparc/mmu_helper.c  | 78 ++++++++++++++++++++++----------------
 4 files changed, 51 insertions(+), 48 deletions(-)

-- 
2.17.1

Comments

Peter Maydell May 9, 2019, 1:35 p.m. UTC | #1 
On Thu, 9 May 2019 at 07:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Cc: Artyom Tarasenko <atar4qemu@gmail.com>

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

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

> ---

> v2: Keep user-only, sparc32, and sparc64 tlb_fill separate.

> ---

>  target/sparc/cpu.h         |  5 ++-

>  target/sparc/cpu.c         |  5 +--

>  target/sparc/ldst_helper.c | 11 +-----

>  target/sparc/mmu_helper.c  | 78 ++++++++++++++++++++++----------------

>  4 files changed, 51 insertions(+), 48 deletions(-)


>  /* Perform address translation */

> -int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,

> -                               int mmu_idx)

> +bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,

> +                        MMUAccessType access_type, int mmu_idx,

> +                        bool probe, uintptr_t retaddr)

>  {

>      SPARCCPU *cpu = SPARC_CPU(cs);

>      CPUSPARCState *env = &cpu->env;

> @@ -220,22 +222,18 @@ int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,

>

>      address &= TARGET_PAGE_MASK;

>      error_code = get_physical_address(env, &paddr, &prot, &access_index,

> -                                      address, rw, mmu_idx, &page_size);

> +                                      address, access_type,

> +                                      mmu_idx, &page_size);

>      vaddr = address;

> -    if (error_code == 0) {

> +    if (likely(error_code == 0)) {

>          qemu_log_mask(CPU_LOG_MMU,

> -                "Translate at %" VADDR_PRIx " -> " TARGET_FMT_plx ", vaddr "

> -                TARGET_FMT_lx "\n", address, paddr, vaddr);

> +                      "Translate at %" VADDR_PRIx " -> "

> +                      TARGET_FMT_plx ", vaddr " TARGET_FMT_lx "\n",

> +                      address, paddr, vaddr);

>          tlb_set_page(cs, vaddr, paddr, prot, mmu_idx, page_size);

> -        return 0;

> +        return true;

>      }

>

> -    if (env->mmuregs[3]) { /* Fault status register */

> -        env->mmuregs[3] = 1; /* overflow (not read before another fault) */

> -    }

> -    env->mmuregs[3] |= (access_index << 5) | error_code | 2;

> -    env->mmuregs[4] = address; /* Fault address register */

> -


In the old code, we set these MMU registers before checking
for the MMU_NF case...

>      if ((env->mmuregs[0] & MMU_NF) || env->psret == 0)  {

>          /* No fault mode: if a mapping is available, just override

>             permissions. If no mapping is available, redirect accesses to

> @@ -243,15 +241,25 @@ int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,

>             switching to normal mode. */

>          prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;

>          tlb_set_page(cs, vaddr, paddr, prot, mmu_idx, TARGET_PAGE_SIZE);

> -        return 0;

> -    } else {

> -        if (rw & 2) {

> -            cs->exception_index = TT_TFAULT;

> -        } else {

> -            cs->exception_index = TT_DFAULT;

> -        }

> -        return 1;

> +        return true;

>      }

> +

> +    if (probe) {

> +        return false;

> +    }

> +

> +    if (env->mmuregs[3]) { /* Fault status register */

> +        env->mmuregs[3] = 1; /* overflow (not read before another fault) */

> +    }

> +    env->mmuregs[3] |= (access_index << 5) | error_code | 2;

> +    env->mmuregs[4] = address; /* Fault address register */

> +

> +    if (access_type == MMU_INST_FETCH) {

> +        cs->exception_index = TT_TFAULT;

> +    } else {

> +        cs->exception_index = TT_DFAULT;

> +    }

> +    cpu_loop_exit_restore(cs, retaddr);

>  }


...but in the new code we only set them if we're really
going to fault.

The v8 SPARC architecture manual appending H says that
when the NF bit is 1 faults detected by the MMU cause
FSR and FAR to be updated even though no fault is generated
to the processor. So I think the change here is not correct.

(The spec also says that ASI 9 is supposed to be special and
not affected by NF==1; and I think that since we put entries in
the TLB for the NF case we won't correctly set the fault address
register if the CPU makes two successive accesses to the same
page, because the second access won't take the slow path and
won't update the FAR. But those are pre-existing bugs.)

thanks
-- PMM
Richard Henderson May 9, 2019, 3:51 p.m. UTC | #2 
On 5/9/19 6:35 AM, Peter Maydell wrote:
>> +    if (probe) {

>> +        return false;

>> +    }

>> +

>> +    if (env->mmuregs[3]) { /* Fault status register */

>> +        env->mmuregs[3] = 1; /* overflow (not read before another fault) */

>> +    }

>> +    env->mmuregs[3] |= (access_index << 5) | error_code | 2;

>> +    env->mmuregs[4] = address; /* Fault address register */

>> +

>> +    if (access_type == MMU_INST_FETCH) {

>> +        cs->exception_index = TT_TFAULT;

>> +    } else {

>> +        cs->exception_index = TT_DFAULT;

>> +    }

>> +    cpu_loop_exit_restore(cs, retaddr);

>>  }

> 

> ...but in the new code we only set them if we're really

> going to fault.


Yes, I made change assuming that would allow probe to work, which would seem to
require that the FSR not be modified.

> The v8 SPARC architecture manual appending H says that

> when the NF bit is 1 faults detected by the MMU cause

> FSR and FAR to be updated even though no fault is generated

> to the processor. So I think the change here is not correct.

> 

> (The spec also says that ASI 9 is supposed to be special and

> not affected by NF==1; and I think that since we put entries in

> the TLB for the NF case we won't correctly set the fault address

> register if the CPU makes two successive accesses to the same

> page, because the second access won't take the slow path and

> won't update the FAR. But those are pre-existing bugs.)


Thanks for the pointers.  They do look like pre-existing bugs.

I think the only thing to do for sparc is the same as i386 -- assert that probe
is unset.  If someone ever decides to use tlb_vaddr_to_host from the sparc
backend, they'll have to come back and untangle this.


r~
diff mbox series

Patch

diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 85b9665ccc..f31e8535df 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -579,8 +579,9 @@  void cpu_raise_exception_ra(CPUSPARCState *, int, uintptr_t) QEMU_NORETURN;
 void cpu_sparc_set_id(CPUSPARCState *env, unsigned int cpu);
 void sparc_cpu_list(void);
 /* mmu_helper.c */
-int sparc_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw,
-                               int mmu_idx);
+bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
+                        MMUAccessType access_type, int mmu_idx,
+                        bool probe, uintptr_t retaddr);
 target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int mmulev);
 void dump_mmu(CPUSPARCState *env);
 
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 4654c2a6a0..f93ce72eb9 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -875,9 +875,8 @@  static void sparc_cpu_class_init(ObjectClass *oc, void *data)
     cc->synchronize_from_tb = sparc_cpu_synchronize_from_tb;
     cc->gdb_read_register = sparc_cpu_gdb_read_register;
     cc->gdb_write_register = sparc_cpu_gdb_write_register;
-#ifdef CONFIG_USER_ONLY
-    cc->handle_mmu_fault = sparc_cpu_handle_mmu_fault;
-#else
+    cc->tlb_fill = sparc_cpu_tlb_fill;
+#ifndef CONFIG_USER_ONLY
     cc->do_unassigned_access = sparc_cpu_unassigned_access;
     cc->do_unaligned_access = sparc_cpu_do_unaligned_access;
     cc->get_phys_page_debug = sparc_cpu_get_phys_page_debug;
diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index a7fcb84ac0..2558c08a64 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -1925,18 +1925,9 @@  void QEMU_NORETURN sparc_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
     cpu_raise_exception_ra(env, TT_UNALIGNED, retaddr);
 }
 
-/* try to fill the TLB and return an exception if error. If retaddr is
-   NULL, it means that the function was called in C code (i.e. not
-   from generated code or from helper.c) */
-/* XXX: fix it to restore all registers */
 void tlb_fill(CPUState *cs, target_ulong addr, int size,
               MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)
 {
-    int ret;
-
-    ret = sparc_cpu_handle_mmu_fault(cs, addr, size, access_type, mmu_idx);
-    if (ret) {
-        cpu_loop_exit_restore(cs, retaddr);
-    }
+    sparc_cpu_tlb_fill(cs, addr, size, access_type, mmu_idx, false, retaddr);
 }
 #endif
diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index afcc5b617d..7f811ea031 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -27,13 +27,14 @@ 
 
 #if defined(CONFIG_USER_ONLY)
 
-int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
-                               int mmu_idx)
+bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
+                        MMUAccessType access_type, int mmu_idx,
+                        bool probe, uintptr_t retaddr)
 {
     SPARCCPU *cpu = SPARC_CPU(cs);
     CPUSPARCState *env = &cpu->env;
 
-    if (rw & 2) {
+    if (access_type == MMU_INST_FETCH) {
         cs->exception_index = TT_TFAULT;
     } else {
         cs->exception_index = TT_DFAULT;
@@ -43,7 +44,7 @@  int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
         env->mmuregs[4] = address;
 #endif
     }
-    return 1;
+    cpu_loop_exit_restore(cs, retaddr);
 }
 
 #else
@@ -208,8 +209,9 @@  static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
 }
 
 /* Perform address translation */
-int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
-                               int mmu_idx)
+bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
+                        MMUAccessType access_type, int mmu_idx,
+                        bool probe, uintptr_t retaddr)
 {
     SPARCCPU *cpu = SPARC_CPU(cs);
     CPUSPARCState *env = &cpu->env;
@@ -220,22 +222,18 @@  int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
 
     address &= TARGET_PAGE_MASK;
     error_code = get_physical_address(env, &paddr, &prot, &access_index,
-                                      address, rw, mmu_idx, &page_size);
+                                      address, access_type,
+                                      mmu_idx, &page_size);
     vaddr = address;
-    if (error_code == 0) {
+    if (likely(error_code == 0)) {
         qemu_log_mask(CPU_LOG_MMU,
-                "Translate at %" VADDR_PRIx " -> " TARGET_FMT_plx ", vaddr "
-                TARGET_FMT_lx "\n", address, paddr, vaddr);
+                      "Translate at %" VADDR_PRIx " -> "
+                      TARGET_FMT_plx ", vaddr " TARGET_FMT_lx "\n",
+                      address, paddr, vaddr);
         tlb_set_page(cs, vaddr, paddr, prot, mmu_idx, page_size);
-        return 0;
+        return true;
     }
 
-    if (env->mmuregs[3]) { /* Fault status register */
-        env->mmuregs[3] = 1; /* overflow (not read before another fault) */
-    }
-    env->mmuregs[3] |= (access_index << 5) | error_code | 2;
-    env->mmuregs[4] = address; /* Fault address register */
-
     if ((env->mmuregs[0] & MMU_NF) || env->psret == 0)  {
         /* No fault mode: if a mapping is available, just override
            permissions. If no mapping is available, redirect accesses to
@@ -243,15 +241,25 @@  int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
            switching to normal mode. */
         prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         tlb_set_page(cs, vaddr, paddr, prot, mmu_idx, TARGET_PAGE_SIZE);
-        return 0;
-    } else {
-        if (rw & 2) {
-            cs->exception_index = TT_TFAULT;
-        } else {
-            cs->exception_index = TT_DFAULT;
-        }
-        return 1;
+        return true;
     }
+
+    if (probe) {
+        return false;
+    }
+
+    if (env->mmuregs[3]) { /* Fault status register */
+        env->mmuregs[3] = 1; /* overflow (not read before another fault) */
+    }
+    env->mmuregs[3] |= (access_index << 5) | error_code | 2;
+    env->mmuregs[4] = address; /* Fault address register */
+
+    if (access_type == MMU_INST_FETCH) {
+        cs->exception_index = TT_TFAULT;
+    } else {
+        cs->exception_index = TT_DFAULT;
+    }
+    cpu_loop_exit_restore(cs, retaddr);
 }
 
 target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int mmulev)
@@ -713,20 +721,22 @@  static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
 }
 
 /* Perform address translation */
-int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
-                               int mmu_idx)
+bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
+                        MMUAccessType access_type, int mmu_idx,
+                        bool probe, uintptr_t retaddr)
 {
     SPARCCPU *cpu = SPARC_CPU(cs);
     CPUSPARCState *env = &cpu->env;
-    target_ulong vaddr;
     hwaddr paddr;
+    target_ulong vaddr;
     target_ulong page_size;
     int error_code = 0, prot, access_index;
 
     address &= TARGET_PAGE_MASK;
     error_code = get_physical_address(env, &paddr, &prot, &access_index,
-                                      address, rw, mmu_idx, &page_size);
-    if (error_code == 0) {
+                                      address, access_type,
+                                      mmu_idx, &page_size);
+    if (likely(error_code == 0)) {
         vaddr = address;
 
         trace_mmu_helper_mmu_fault(address, paddr, mmu_idx, env->tl,
@@ -734,10 +744,12 @@  int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int size, int rw,
                                    env->dmmu.mmu_secondary_context);
 
         tlb_set_page(cs, vaddr, paddr, prot, mmu_idx, page_size);
-        return 0;
+        return true;
     }
-    /* XXX */
-    return 1;
+    if (probe) {
+        return false;
+    }
+    cpu_loop_exit_restore(cs, retaddr);
 }
 
 void dump_mmu(CPUSPARCState *env)