[Qemu-devel] [PATCH 0/2] Fix confusing argument names in some common functions

[Qemu-devel] [PATCH 0/2] Fix confusing argument names in some common functions

[Sergey Sorokin]

There are functions tlb_fill(), cpu_unaligned_access() and
do_unaligned_access() that are called with access type and mmu index
arguments. But these arguments are named 'is_write' and 'is_user' in their
declarations. The patches fix the names to avoid a confusion.

Sergey Sorokin (2):
  Fix confusing argument names of do_unaligned_access() functions
  Fix a confusing argument name in tlb_fill() function

 include/exec/exec-all.h       |  2 +-
 include/qom/cpu.h             |  8 ++++----
 target-alpha/cpu.h            |  4 ++--
 target-alpha/mem_helper.c     |  8 ++++----
 target-arm/internals.h        |  4 ++--
 target-arm/op_helper.c        | 27 +++++++++++++++------------
 target-cris/op_helper.c       |  4 ++--
 target-i386/mem_helper.c      |  4 ++--
 target-lm32/op_helper.c       |  4 ++--
 target-m68k/op_helper.c       |  4 ++--
 target-microblaze/op_helper.c |  4 ++--
 target-mips/cpu.h             |  4 ++--
 target-mips/op_helper.c       |  6 +++---
 target-moxie/helper.c         |  4 ++--
 target-openrisc/mmu_helper.c  |  4 ++--
 target-ppc/mmu_helper.c       |  6 +++---
 target-s390x/mem_helper.c     |  4 ++--
 target-sh4/op_helper.c        |  4 ++--
 target-sparc/cpu.h            |  5 +++--
 target-sparc/ldst_helper.c    |  9 +++++----
 target-tricore/op_helper.c    |  4 ++--
 target-unicore32/op_helper.c  |  4 ++--
 target-xtensa/cpu.h           |  4 ++--
 target-xtensa/op_helper.c     | 10 ++++++----
 24 files changed, 74 insertions(+), 67 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Sergey Sorokin]

There are functions cpu_unaligned_access() and do_unaligned_access() that
are called with access type and mmu index arguments. But these arguments
are named 'is_write' and 'is_user' in their declarations.
The patch fixes the names to avoid a confusion.

Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
---
 include/qom/cpu.h          |  8 ++++----
 target-alpha/cpu.h         |  4 ++--
 target-alpha/mem_helper.c  |  4 ++--
 target-arm/internals.h     |  4 ++--
 target-arm/op_helper.c     | 17 +++++++++--------
 target-mips/cpu.h          |  4 ++--
 target-mips/op_helper.c    |  2 +-
 target-sparc/cpu.h         |  5 +++--
 target-sparc/ldst_helper.c |  5 +++--
 target-xtensa/cpu.h        |  4 ++--
 target-xtensa/op_helper.c  |  3 ++-
 11 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 32f3af3..60985c2 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -141,8 +141,8 @@ typedef struct CPUClass {
     bool (*has_work)(CPUState *cpu);
     void (*do_interrupt)(CPUState *cpu);
     CPUUnassignedAccess do_unassigned_access;
-    void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
-                                int is_write, int is_user, uintptr_t retaddr);
+    void (*do_unaligned_access)(CPUState *cpu, vaddr addr, int access_type,
+                                int mmu_idx, uintptr_t retaddr);
     bool (*virtio_is_big_endian)(CPUState *cpu);
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
@@ -716,12 +716,12 @@ static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
 }
 
 static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
-                                        int is_write, int is_user,
+                                        int access_type, int mmu_idx,
                                         uintptr_t retaddr)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
-    cc->do_unaligned_access(cpu, addr, is_write, is_user, retaddr);
+    cc->do_unaligned_access(cpu, addr, access_type, mmu_idx, retaddr);
 }
 #endif
 
diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
index e71ea70..4e512a2 100644
--- a/target-alpha/cpu.h
+++ b/target-alpha/cpu.h
@@ -322,8 +322,8 @@ void alpha_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
 hwaddr alpha_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int alpha_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int alpha_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-void alpha_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
-                                   int is_write, int is_user, uintptr_t retaddr);
+void alpha_cpu_do_unaligned_access(CPUState *cpu, vaddr addr, int access_type,
+                                   int mmu_idx, uintptr_t retaddr);
 
 #define cpu_list alpha_cpu_list
 #define cpu_exec cpu_alpha_exec
diff --git a/target-alpha/mem_helper.c b/target-alpha/mem_helper.c
index 7f4d15f..cfb4898 100644
--- a/target-alpha/mem_helper.c
+++ b/target-alpha/mem_helper.c
@@ -98,8 +98,8 @@ uint64_t helper_stq_c_phys(CPUAlphaState *env, uint64_t p, uint64_t v)
     return ret;
 }
 
-void alpha_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
-                                   int is_write, int is_user, uintptr_t retaddr)
+void alpha_cpu_do_unaligned_access(CPUState *cs, vaddr addr, int access_type,
+                                   int mmu_idx, uintptr_t retaddr)
 {
     AlphaCPU *cpu = ALPHA_CPU(cs);
     CPUAlphaState *env = &cpu->env;
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 728ecba..6d469bf 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -476,7 +476,7 @@ bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
 bool arm_s1_regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx);
 
 /* Raise a data fault alignment exception for the specified virtual address */
-void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
-                                 int is_user, uintptr_t retaddr);
+void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int access_type,
+                                 int mmu_idx, uintptr_t retaddr);
 
 #endif
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 35912a1..04316b5 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -79,7 +79,7 @@ uint32_t HELPER(neon_tbl)(CPUARMState *env, uint32_t ireg, uint32_t def,
 static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
                                             unsigned int target_el,
                                             bool same_el,
-                                            bool s1ptw, int is_write,
+                                            bool s1ptw, bool is_write,
                                             int fsc)
 {
     uint32_t syn;
@@ -97,7 +97,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
      */
     if (!(template_syn & ARM_EL_ISV) || target_el != 2 || s1ptw) {
         syn = syn_data_abort_no_iss(same_el,
-                                    0, 0, s1ptw, is_write == 1, fsc);
+                                    0, 0, s1ptw, is_write, fsc);
     } else {
         /* Fields: IL, ISV, SAS, SSE, SRT, SF and AR come from the template
          * syndrome created at translation time.
@@ -105,7 +105,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
          */
         syn = syn_data_abort_with_iss(same_el,
                                       0, 0, 0, 0, 0,
-                                      0, 0, s1ptw, is_write == 1, fsc,
+                                      0, 0, s1ptw, is_write, fsc,
                                       false);
         /* Merge the runtime syndrome with the template syndrome.  */
         syn |= template_syn;
@@ -154,7 +154,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
             exc = EXCP_PREFETCH_ABORT;
         } else {
             syn = merge_syn_data_abort(env->exception.syndrome, target_el,
-                                       same_el, fi.s1ptw, is_write, syn);
+                                       same_el, fi.s1ptw, is_write == 1, syn);
             if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
             }
@@ -168,8 +168,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
 }
 
 /* Raise a data fault alignment exception for the specified virtual address */
-void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
-                                 int is_user, uintptr_t retaddr)
+void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int access_type,
+                                 int mmu_idx, uintptr_t retaddr)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -196,12 +196,13 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
         env->exception.fsr = 0x1;
     }
 
-    if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
+    if (access_type == MMU_DATA_STORE && arm_feature(env, ARM_FEATURE_V6)) {
         env->exception.fsr |= (1 << 11);
     }
 
     syn = merge_syn_data_abort(env->exception.syndrome, target_el,
-                               same_el, 0, is_write, 0x21);
+                               same_el, 0, access_type == MMU_DATA_STORE,
+                               0x21);
     raise_exception(env, EXCP_DATA_ABORT, syn, target_el);
 }
 
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 4ce9d47..ffd5baa 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -650,8 +650,8 @@ void mips_cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
 hwaddr mips_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int mips_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int mips_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-void mips_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
-                                  int is_write, int is_user, uintptr_t retaddr);
+void mips_cpu_do_unaligned_access(CPUState *cpu, vaddr addr, int access_type,
+                                  int mmu_idx, uintptr_t retaddr);
 
 #if !defined(CONFIG_USER_ONLY)
 int no_mmu_map_address (CPUMIPSState *env, hwaddr *physical, int *prot,
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 7cf9807..b5f1641 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -2383,7 +2383,7 @@ void helper_wait(CPUMIPSState *env)
 #if !defined(CONFIG_USER_ONLY)
 
 void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
-                                  int access_type, int is_user,
+                                  int access_type, int mmu_idx,
                                   uintptr_t retaddr)
 {
     MIPSCPU *cpu = MIPS_CPU(cs);
diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index ba37f4b..48298d4 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -541,8 +541,9 @@ hwaddr sparc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int sparc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int sparc_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 void QEMU_NORETURN sparc_cpu_do_unaligned_access(CPUState *cpu,
-                                                 vaddr addr, int is_write,
-                                                 int is_user, uintptr_t retaddr);
+                                                 vaddr addr, int access_type,
+                                                 int mmu_idx,
+                                                 uintptr_t retaddr);
 
 #ifndef NO_CPU_IO_DEFS
 /* cpu_init.c */
diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
index f73cf6d..f1cb821 100644
--- a/target-sparc/ldst_helper.c
+++ b/target-sparc/ldst_helper.c
@@ -2421,8 +2421,9 @@ void sparc_cpu_unassigned_access(CPUState *cs, hwaddr addr,
 
 #if !defined(CONFIG_USER_ONLY)
 void QEMU_NORETURN sparc_cpu_do_unaligned_access(CPUState *cs,
-                                                 vaddr addr, int is_write,
-                                                 int is_user, uintptr_t retaddr)
+                                                 vaddr addr, int access_type,
+                                                 int mmu_idx,
+                                                 uintptr_t retaddr)
 {
     SPARCCPU *cpu = SPARC_CPU(cs);
     CPUSPARCState *env = &cpu->env;
diff --git a/target-xtensa/cpu.h b/target-xtensa/cpu.h
index 442176a..4b05847 100644
--- a/target-xtensa/cpu.h
+++ b/target-xtensa/cpu.h
@@ -413,8 +413,8 @@ void xtensa_cpu_dump_state(CPUState *cpu, FILE *f,
 hwaddr xtensa_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int xtensa_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int xtensa_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-void xtensa_cpu_do_unaligned_access(CPUState *cpu, vaddr addr,
-                                    int is_write, int is_user, uintptr_t retaddr);
+void xtensa_cpu_do_unaligned_access(CPUState *cpu, vaddr addr, int access_type,
+                                    int mmu_idx, uintptr_t retaddr);
 
 #define cpu_exec cpu_xtensa_exec
 #define cpu_signal_handler cpu_xtensa_signal_handler
diff --git a/target-xtensa/op_helper.c b/target-xtensa/op_helper.c
index bc3667f..32dcbdf 100644
--- a/target-xtensa/op_helper.c
+++ b/target-xtensa/op_helper.c
@@ -35,7 +35,8 @@
 #include "qemu/timer.h"
 
 void xtensa_cpu_do_unaligned_access(CPUState *cs,
-        vaddr addr, int is_write, int is_user, uintptr_t retaddr)
+        vaddr addr, int access_type,
+        int mmu_idx, uintptr_t retaddr)
 {
     XtensaCPU *cpu = XTENSA_CPU(cs);
     CPUXtensaState *env = &cpu->env;
-- 
1.9.3

[Qemu-devel] [PATCH 2/2] Fix a confusing argument name in tlb_fill() function

[Sergey Sorokin]

The function tlb_fill() is called with access type argument which is named
'is_write' in its declaration. The patch fixes the argument name
to avoid a confusion.

Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
---
 include/exec/exec-all.h       |  2 +-
 target-alpha/mem_helper.c     |  4 ++--
 target-arm/op_helper.c        | 12 +++++++-----
 target-cris/op_helper.c       |  4 ++--
 target-i386/mem_helper.c      |  4 ++--
 target-lm32/op_helper.c       |  4 ++--
 target-m68k/op_helper.c       |  4 ++--
 target-microblaze/op_helper.c |  4 ++--
 target-mips/op_helper.c       |  4 ++--
 target-moxie/helper.c         |  4 ++--
 target-openrisc/mmu_helper.c  |  4 ++--
 target-ppc/mmu_helper.c       |  6 +++---
 target-s390x/mem_helper.c     |  4 ++--
 target-sh4/op_helper.c        |  4 ++--
 target-sparc/ldst_helper.c    |  4 ++--
 target-tricore/op_helper.c    |  4 ++--
 target-unicore32/op_helper.c  |  4 ++--
 target-xtensa/op_helper.c     |  7 ++++---
 18 files changed, 43 insertions(+), 40 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index e076397..f425576 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -363,7 +363,7 @@ extern uintptr_t tci_tb_ptr;
 struct MemoryRegion *iotlb_to_region(CPUState *cpu,
                                      hwaddr index, MemTxAttrs attrs);
 
-void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cpu, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr);
 
 #endif
diff --git a/target-alpha/mem_helper.c b/target-alpha/mem_helper.c
index cfb4898..53fdae4 100644
--- a/target-alpha/mem_helper.c
+++ b/target-alpha/mem_helper.c
@@ -144,12 +144,12 @@ void alpha_cpu_unassigned_access(CPUState *cs, hwaddr addr,
    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 is_write,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type,
               int mmu_idx, uintptr_t retaddr)
 {
     int ret;
 
-    ret = alpha_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = alpha_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret != 0)) {
         if (retaddr) {
             cpu_restore_state(cs, retaddr);
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 04316b5..dd97760 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -117,14 +117,14 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
  * NULL, it means that the function was called in C code (i.e. not
  * from generated code or from helper.c)
  */
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     bool ret;
     uint32_t fsr = 0;
     ARMMMUFaultInfo fi = {};
 
-    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr, &fi);
+    ret = arm_tlb_fill(cs, addr, access_type, mmu_idx, &fsr, &fi);
     if (unlikely(ret)) {
         ARMCPU *cpu = ARM_CPU(cs);
         CPUARMState *env = &cpu->env;
@@ -149,13 +149,15 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
         /* For insn and data aborts we assume there is no instruction syndrome
          * information; this is always true for exceptions reported to EL1.
          */
-        if (is_write == 2) {
+        if (access_type == MMU_INST_FETCH) {
             syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
             exc = EXCP_PREFETCH_ABORT;
         } else {
             syn = merge_syn_data_abort(env->exception.syndrome, target_el,
-                                       same_el, fi.s1ptw, is_write == 1, syn);
-            if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
+                                       same_el, fi.s1ptw,
+                                       access_type == MMU_DATA_STORE, syn);
+            if (access_type == MMU_DATA_STORE
+                && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
             }
             exc = EXCP_DATA_ABORT;
diff --git a/target-cris/op_helper.c b/target-cris/op_helper.c
index 675ab86..fbb71bc 100644
--- a/target-cris/op_helper.c
+++ b/target-cris/op_helper.c
@@ -41,7 +41,7 @@
 /* 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) */
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     CRISCPU *cpu = CRIS_CPU(cs);
@@ -50,7 +50,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
 
     D_LOG("%s pc=%x tpc=%x ra=%p\n", __func__,
           env->pc, env->pregs[PR_EDA], (void *)retaddr);
-    ret = cris_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = cris_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret)) {
         if (retaddr) {
             /* now we have a real cpu fault */
diff --git a/target-i386/mem_helper.c b/target-i386/mem_helper.c
index c2f4769..b1ea08c 100644
--- a/target-i386/mem_helper.c
+++ b/target-i386/mem_helper.c
@@ -140,12 +140,12 @@ void helper_boundl(CPUX86State *env, target_ulong a0, int v)
  * from generated code or from helper.c)
  */
 /* XXX: fix it to restore all registers */
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = x86_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = x86_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (ret) {
         X86CPU *cpu = X86_CPU(cs);
         CPUX86State *env = &cpu->env;
diff --git a/target-lm32/op_helper.c b/target-lm32/op_helper.c
index 7a550d1..5f29343 100644
--- a/target-lm32/op_helper.c
+++ b/target-lm32/op_helper.c
@@ -144,12 +144,12 @@ uint32_t HELPER(rcsr_jrx)(CPULM32State *env)
  * NULL, it means that the function was called in C code (i.e. not
  * from generated code or from helper.c)
  */
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = lm32_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = lm32_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret)) {
         if (retaddr) {
             /* now we have a real cpu fault */
diff --git a/target-m68k/op_helper.c b/target-m68k/op_helper.c
index ff32e35..12d942e 100644
--- a/target-m68k/op_helper.c
+++ b/target-m68k/op_helper.c
@@ -39,12 +39,12 @@ static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
 /* 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) */
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = m68k_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = m68k_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret)) {
         if (retaddr) {
             /* now we have a real cpu fault */
diff --git a/target-microblaze/op_helper.c b/target-microblaze/op_helper.c
index 0533939..8bdd9f4 100644
--- a/target-microblaze/op_helper.c
+++ b/target-microblaze/op_helper.c
@@ -33,12 +33,12 @@
  * NULL, it means that the function was called in C code (i.e. not
  * from generated code or from helper.c)
  */
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = mb_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = mb_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret)) {
         if (retaddr) {
             /* now we have a real cpu fault */
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index b5f1641..8f440e8 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -2405,12 +2405,12 @@ void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
     do_raise_exception_err(env, excp, error_code, retaddr);
 }
 
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = mips_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = mips_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (ret) {
         MIPSCPU *cpu = MIPS_CPU(cs);
         CPUMIPSState *env = &cpu->env;
diff --git a/target-moxie/helper.c b/target-moxie/helper.c
index d51e9b9..8097004 100644
--- a/target-moxie/helper.c
+++ b/target-moxie/helper.c
@@ -29,12 +29,12 @@
 /* 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) */
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = moxie_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = moxie_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret)) {
         if (retaddr) {
             cpu_restore_state(cs, retaddr);
diff --git a/target-openrisc/mmu_helper.c b/target-openrisc/mmu_helper.c
index c0658c3..37ea725 100644
--- a/target-openrisc/mmu_helper.c
+++ b/target-openrisc/mmu_helper.c
@@ -25,12 +25,12 @@
 
 #ifndef CONFIG_USER_ONLY
 
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type,
               int mmu_idx, uintptr_t retaddr)
 {
     int ret;
 
-    ret = openrisc_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = openrisc_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
 
     if (ret) {
         if (retaddr) {
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 485d5b8..78179ee 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2878,7 +2878,7 @@ void helper_check_tlb_flush(CPUPPCState *env)
    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 is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -2887,9 +2887,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
     int ret;
 
     if (pcc->handle_mmu_fault) {
-        ret = pcc->handle_mmu_fault(cpu, addr, is_write, mmu_idx);
+        ret = pcc->handle_mmu_fault(cpu, addr, access_type, mmu_idx);
     } else {
-        ret = cpu_ppc_handle_mmu_fault(env, addr, is_write, mmu_idx);
+        ret = cpu_ppc_handle_mmu_fault(env, addr, access_type, mmu_idx);
     }
     if (unlikely(ret != 0)) {
         if (likely(retaddr)) {
diff --git a/target-s390x/mem_helper.c b/target-s390x/mem_helper.c
index ec8059a..acc9eb9 100644
--- a/target-s390x/mem_helper.c
+++ b/target-s390x/mem_helper.c
@@ -36,12 +36,12 @@
    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 is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = s390_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = s390_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret != 0)) {
         if (likely(retaddr)) {
             /* now we have a real cpu fault */
diff --git a/target-sh4/op_helper.c b/target-sh4/op_helper.c
index 303e83e..49bd57a 100644
--- a/target-sh4/op_helper.c
+++ b/target-sh4/op_helper.c
@@ -24,12 +24,12 @@
 
 #ifndef CONFIG_USER_ONLY
 
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = superh_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = superh_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (ret) {
         /* now we have a real cpu fault */
         if (retaddr) {
diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
index f1cb821..bc60325 100644
--- a/target-sparc/ldst_helper.c
+++ b/target-sparc/ldst_helper.c
@@ -2442,12 +2442,12 @@ void QEMU_NORETURN sparc_cpu_do_unaligned_access(CPUState *cs,
    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 is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
 
-    ret = sparc_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = sparc_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (ret) {
         if (retaddr) {
             cpu_restore_state(cs, retaddr);
diff --git a/target-tricore/op_helper.c b/target-tricore/op_helper.c
index a73ed53..64337b7 100644
--- a/target-tricore/op_helper.c
+++ b/target-tricore/op_helper.c
@@ -2833,11 +2833,11 @@ static inline void QEMU_NORETURN do_raise_exception_err(CPUTriCoreState *env,
     cpu_loop_exit(cs);
 }
 
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
               uintptr_t retaddr)
 {
     int ret;
-    ret = cpu_tricore_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = cpu_tricore_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (ret) {
         TriCoreCPU *cpu = TRICORE_CPU(cs);
         CPUTriCoreState *env = &cpu->env;
diff --git a/target-unicore32/op_helper.c b/target-unicore32/op_helper.c
index a782d33..2eadb81 100644
--- a/target-unicore32/op_helper.c
+++ b/target-unicore32/op_helper.c
@@ -244,12 +244,12 @@ uint32_t HELPER(ror_cc)(CPUUniCore32State *env, uint32_t x, uint32_t i)
 }
 
 #ifndef CONFIG_USER_ONLY
-void tlb_fill(CPUState *cs, target_ulong addr, int is_write,
+void tlb_fill(CPUState *cs, target_ulong addr, int access_type,
               int mmu_idx, uintptr_t retaddr)
 {
     int ret;
 
-    ret = uc32_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
+    ret = uc32_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret)) {
         if (retaddr) {
             /* now we have a real cpu fault */
diff --git a/target-xtensa/op_helper.c b/target-xtensa/op_helper.c
index 32dcbdf..e9aab6d 100644
--- a/target-xtensa/op_helper.c
+++ b/target-xtensa/op_helper.c
@@ -50,18 +50,19 @@ void xtensa_cpu_do_unaligned_access(CPUState *cs,
 }
 
 void tlb_fill(CPUState *cs,
-              target_ulong vaddr, int is_write, int mmu_idx, uintptr_t retaddr)
+              target_ulong vaddr, int access_type,
+              int mmu_idx, uintptr_t retaddr)
 {
     XtensaCPU *cpu = XTENSA_CPU(cs);
     CPUXtensaState *env = &cpu->env;
     uint32_t paddr;
     uint32_t page_size;
     unsigned access;
-    int ret = xtensa_get_physical_addr(env, true, vaddr, is_write, mmu_idx,
+    int ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx,
             &paddr, &page_size, &access);
 
     qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n",
-                  __func__, vaddr, is_write, mmu_idx, paddr, ret);
+                  __func__, vaddr, access_type, mmu_idx, paddr, ret);
 
     if (ret == 0) {
         tlb_set_page(cs,
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Peter Maydell]

On 10 June 2016 at 17:26, Sergey Sorokin <afarallax@yandex.ru> wrote:
> There are functions cpu_unaligned_access() and do_unaligned_access() that
> are called with access type and mmu index arguments. But these arguments
> are named 'is_write' and 'is_user' in their declarations.
> The patch fixes the names to avoid a confusion.
>
> Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>

If we're going to touch all of these then we have an enum type
we should be using instead of just 'int' for the old
is_write argument: MMUAccessType (defined in cpu-common.h).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Sergey Sorokin]

I agree

10.06.2016, 19:33, "Peter Maydell" <peter.maydell@linaro.org>:
> On 10 June 2016 at 17:26, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>  There are functions cpu_unaligned_access() and do_unaligned_access() that
>>  are called with access type and mmu index arguments. But these arguments
>>  are named 'is_write' and 'is_user' in their declarations.
>>  The patch fixes the names to avoid a confusion.
>>
>>  Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
>
> If we're going to touch all of these then we have an enum type
> we should be using instead of just 'int' for the old
> is_write argument: MMUAccessType (defined in cpu-common.h).
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Sergey Sorokin]

What if I combine both patches into single one?

10.06.2016, 19:33, "Peter Maydell" <peter.maydell@linaro.org>:
> On 10 June 2016 at 17:26, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>  There are functions cpu_unaligned_access() and do_unaligned_access() that
>>  are called with access type and mmu index arguments. But these arguments
>>  are named 'is_write' and 'is_user' in their declarations.
>>  The patch fixes the names to avoid a confusion.
>>
>>  Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
>
> If we're going to touch all of these then we have an enum type
> we should be using instead of just 'int' for the old
> is_write argument: MMUAccessType (defined in cpu-common.h).
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Peter Maydell]

On 10 June 2016 at 17:42, Sergey Sorokin <afarallax@yandex.ru> wrote:
> What if I combine both patches into single one?

No particular objection.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Sergey Sorokin]

cpu-common.h is not included in qom/cpu.h
what do you think? Should it be included? Or may be MMUAccessType should be just moved into another header. For example into exec/memattrs.h

10.06.2016, 19:44, "Peter Maydell" <peter.maydell@linaro.org>:
> On 10 June 2016 at 17:42, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>  What if I combine both patches into single one?
>
> No particular objection.
>
> -- PMM

Re: [Qemu-devel] [PATCH 2/2] Fix a confusing argument name in tlb_fill() function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 17980 bytes --]

On Fri, Jun 10, 2016 at 07:26:39PM +0300, Sergey Sorokin wrote:
> The function tlb_fill() is called with access type argument which is named
> 'is_write' in its declaration. The patch fixes the argument name
> to avoid a confusion.
> 
> Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
> ---
>  include/exec/exec-all.h       |  2 +-
>  target-alpha/mem_helper.c     |  4 ++--
>  target-arm/op_helper.c        | 12 +++++++-----
>  target-cris/op_helper.c       |  4 ++--
>  target-i386/mem_helper.c      |  4 ++--
>  target-lm32/op_helper.c       |  4 ++--
>  target-m68k/op_helper.c       |  4 ++--
>  target-microblaze/op_helper.c |  4 ++--
>  target-mips/op_helper.c       |  4 ++--
>  target-moxie/helper.c         |  4 ++--
>  target-openrisc/mmu_helper.c  |  4 ++--
>  target-ppc/mmu_helper.c       |  6 +++---
>  target-s390x/mem_helper.c     |  4 ++--
>  target-sh4/op_helper.c        |  4 ++--
>  target-sparc/ldst_helper.c    |  4 ++--
>  target-tricore/op_helper.c    |  4 ++--
>  target-unicore32/op_helper.c  |  4 ++--
>  target-xtensa/op_helper.c     |  7 ++++---
>  18 files changed, 43 insertions(+), 40 deletions(-)

ppc portion

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index e076397..f425576 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -363,7 +363,7 @@ extern uintptr_t tci_tb_ptr;
>  struct MemoryRegion *iotlb_to_region(CPUState *cpu,
>                                       hwaddr index, MemTxAttrs attrs);
>  
> -void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cpu, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr);
>  
>  #endif
> diff --git a/target-alpha/mem_helper.c b/target-alpha/mem_helper.c
> index cfb4898..53fdae4 100644
> --- a/target-alpha/mem_helper.c
> +++ b/target-alpha/mem_helper.c
> @@ -144,12 +144,12 @@ void alpha_cpu_unassigned_access(CPUState *cs, hwaddr addr,
>     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 is_write,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type,
>                int mmu_idx, uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = alpha_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = alpha_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret != 0)) {
>          if (retaddr) {
>              cpu_restore_state(cs, retaddr);
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 04316b5..dd97760 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -117,14 +117,14 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
>   * NULL, it means that the function was called in C code (i.e. not
>   * from generated code or from helper.c)
>   */
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      bool ret;
>      uint32_t fsr = 0;
>      ARMMMUFaultInfo fi = {};
>  
> -    ret = arm_tlb_fill(cs, addr, is_write, mmu_idx, &fsr, &fi);
> +    ret = arm_tlb_fill(cs, addr, access_type, mmu_idx, &fsr, &fi);
>      if (unlikely(ret)) {
>          ARMCPU *cpu = ARM_CPU(cs);
>          CPUARMState *env = &cpu->env;
> @@ -149,13 +149,15 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>          /* For insn and data aborts we assume there is no instruction syndrome
>           * information; this is always true for exceptions reported to EL1.
>           */
> -        if (is_write == 2) {
> +        if (access_type == MMU_INST_FETCH) {
>              syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
>              exc = EXCP_PREFETCH_ABORT;
>          } else {
>              syn = merge_syn_data_abort(env->exception.syndrome, target_el,
> -                                       same_el, fi.s1ptw, is_write == 1, syn);
> -            if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
> +                                       same_el, fi.s1ptw,
> +                                       access_type == MMU_DATA_STORE, syn);
> +            if (access_type == MMU_DATA_STORE
> +                && arm_feature(env, ARM_FEATURE_V6)) {
>                  fsr |= (1 << 11);
>              }
>              exc = EXCP_DATA_ABORT;
> diff --git a/target-cris/op_helper.c b/target-cris/op_helper.c
> index 675ab86..fbb71bc 100644
> --- a/target-cris/op_helper.c
> +++ b/target-cris/op_helper.c
> @@ -41,7 +41,7 @@
>  /* 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) */
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      CRISCPU *cpu = CRIS_CPU(cs);
> @@ -50,7 +50,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>  
>      D_LOG("%s pc=%x tpc=%x ra=%p\n", __func__,
>            env->pc, env->pregs[PR_EDA], (void *)retaddr);
> -    ret = cris_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = cris_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret)) {
>          if (retaddr) {
>              /* now we have a real cpu fault */
> diff --git a/target-i386/mem_helper.c b/target-i386/mem_helper.c
> index c2f4769..b1ea08c 100644
> --- a/target-i386/mem_helper.c
> +++ b/target-i386/mem_helper.c
> @@ -140,12 +140,12 @@ void helper_boundl(CPUX86State *env, target_ulong a0, int v)
>   * from generated code or from helper.c)
>   */
>  /* XXX: fix it to restore all registers */
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = x86_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = x86_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (ret) {
>          X86CPU *cpu = X86_CPU(cs);
>          CPUX86State *env = &cpu->env;
> diff --git a/target-lm32/op_helper.c b/target-lm32/op_helper.c
> index 7a550d1..5f29343 100644
> --- a/target-lm32/op_helper.c
> +++ b/target-lm32/op_helper.c
> @@ -144,12 +144,12 @@ uint32_t HELPER(rcsr_jrx)(CPULM32State *env)
>   * NULL, it means that the function was called in C code (i.e. not
>   * from generated code or from helper.c)
>   */
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = lm32_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = lm32_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret)) {
>          if (retaddr) {
>              /* now we have a real cpu fault */
> diff --git a/target-m68k/op_helper.c b/target-m68k/op_helper.c
> index ff32e35..12d942e 100644
> --- a/target-m68k/op_helper.c
> +++ b/target-m68k/op_helper.c
> @@ -39,12 +39,12 @@ static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
>  /* 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) */
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = m68k_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = m68k_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret)) {
>          if (retaddr) {
>              /* now we have a real cpu fault */
> diff --git a/target-microblaze/op_helper.c b/target-microblaze/op_helper.c
> index 0533939..8bdd9f4 100644
> --- a/target-microblaze/op_helper.c
> +++ b/target-microblaze/op_helper.c
> @@ -33,12 +33,12 @@
>   * NULL, it means that the function was called in C code (i.e. not
>   * from generated code or from helper.c)
>   */
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = mb_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = mb_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret)) {
>          if (retaddr) {
>              /* now we have a real cpu fault */
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index b5f1641..8f440e8 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -2405,12 +2405,12 @@ void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>      do_raise_exception_err(env, excp, error_code, retaddr);
>  }
>  
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = mips_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = mips_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (ret) {
>          MIPSCPU *cpu = MIPS_CPU(cs);
>          CPUMIPSState *env = &cpu->env;
> diff --git a/target-moxie/helper.c b/target-moxie/helper.c
> index d51e9b9..8097004 100644
> --- a/target-moxie/helper.c
> +++ b/target-moxie/helper.c
> @@ -29,12 +29,12 @@
>  /* 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) */
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = moxie_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = moxie_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret)) {
>          if (retaddr) {
>              cpu_restore_state(cs, retaddr);
> diff --git a/target-openrisc/mmu_helper.c b/target-openrisc/mmu_helper.c
> index c0658c3..37ea725 100644
> --- a/target-openrisc/mmu_helper.c
> +++ b/target-openrisc/mmu_helper.c
> @@ -25,12 +25,12 @@
>  
>  #ifndef CONFIG_USER_ONLY
>  
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type,
>                int mmu_idx, uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = openrisc_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = openrisc_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>  
>      if (ret) {
>          if (retaddr) {
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 485d5b8..78179ee 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -2878,7 +2878,7 @@ void helper_check_tlb_flush(CPUPPCState *env)
>     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 is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -2887,9 +2887,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>      int ret;
>  
>      if (pcc->handle_mmu_fault) {
> -        ret = pcc->handle_mmu_fault(cpu, addr, is_write, mmu_idx);
> +        ret = pcc->handle_mmu_fault(cpu, addr, access_type, mmu_idx);
>      } else {
> -        ret = cpu_ppc_handle_mmu_fault(env, addr, is_write, mmu_idx);
> +        ret = cpu_ppc_handle_mmu_fault(env, addr, access_type, mmu_idx);
>      }
>      if (unlikely(ret != 0)) {
>          if (likely(retaddr)) {
> diff --git a/target-s390x/mem_helper.c b/target-s390x/mem_helper.c
> index ec8059a..acc9eb9 100644
> --- a/target-s390x/mem_helper.c
> +++ b/target-s390x/mem_helper.c
> @@ -36,12 +36,12 @@
>     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 is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = s390_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = s390_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret != 0)) {
>          if (likely(retaddr)) {
>              /* now we have a real cpu fault */
> diff --git a/target-sh4/op_helper.c b/target-sh4/op_helper.c
> index 303e83e..49bd57a 100644
> --- a/target-sh4/op_helper.c
> +++ b/target-sh4/op_helper.c
> @@ -24,12 +24,12 @@
>  
>  #ifndef CONFIG_USER_ONLY
>  
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = superh_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = superh_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (ret) {
>          /* now we have a real cpu fault */
>          if (retaddr) {
> diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
> index f1cb821..bc60325 100644
> --- a/target-sparc/ldst_helper.c
> +++ b/target-sparc/ldst_helper.c
> @@ -2442,12 +2442,12 @@ void QEMU_NORETURN sparc_cpu_do_unaligned_access(CPUState *cs,
>     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 is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = sparc_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = sparc_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (ret) {
>          if (retaddr) {
>              cpu_restore_state(cs, retaddr);
> diff --git a/target-tricore/op_helper.c b/target-tricore/op_helper.c
> index a73ed53..64337b7 100644
> --- a/target-tricore/op_helper.c
> +++ b/target-tricore/op_helper.c
> @@ -2833,11 +2833,11 @@ static inline void QEMU_NORETURN do_raise_exception_err(CPUTriCoreState *env,
>      cpu_loop_exit(cs);
>  }
>  
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type, int mmu_idx,
>                uintptr_t retaddr)
>  {
>      int ret;
> -    ret = cpu_tricore_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = cpu_tricore_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (ret) {
>          TriCoreCPU *cpu = TRICORE_CPU(cs);
>          CPUTriCoreState *env = &cpu->env;
> diff --git a/target-unicore32/op_helper.c b/target-unicore32/op_helper.c
> index a782d33..2eadb81 100644
> --- a/target-unicore32/op_helper.c
> +++ b/target-unicore32/op_helper.c
> @@ -244,12 +244,12 @@ uint32_t HELPER(ror_cc)(CPUUniCore32State *env, uint32_t x, uint32_t i)
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> -void tlb_fill(CPUState *cs, target_ulong addr, int is_write,
> +void tlb_fill(CPUState *cs, target_ulong addr, int access_type,
>                int mmu_idx, uintptr_t retaddr)
>  {
>      int ret;
>  
> -    ret = uc32_cpu_handle_mmu_fault(cs, addr, is_write, mmu_idx);
> +    ret = uc32_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret)) {
>          if (retaddr) {
>              /* now we have a real cpu fault */
> diff --git a/target-xtensa/op_helper.c b/target-xtensa/op_helper.c
> index 32dcbdf..e9aab6d 100644
> --- a/target-xtensa/op_helper.c
> +++ b/target-xtensa/op_helper.c
> @@ -50,18 +50,19 @@ void xtensa_cpu_do_unaligned_access(CPUState *cs,
>  }
>  
>  void tlb_fill(CPUState *cs,
> -              target_ulong vaddr, int is_write, int mmu_idx, uintptr_t retaddr)
> +              target_ulong vaddr, int access_type,
> +              int mmu_idx, uintptr_t retaddr)
>  {
>      XtensaCPU *cpu = XTENSA_CPU(cs);
>      CPUXtensaState *env = &cpu->env;
>      uint32_t paddr;
>      uint32_t page_size;
>      unsigned access;
> -    int ret = xtensa_get_physical_addr(env, true, vaddr, is_write, mmu_idx,
> +    int ret = xtensa_get_physical_addr(env, true, vaddr, access_type, mmu_idx,
>              &paddr, &page_size, &access);
>  
>      qemu_log_mask(CPU_LOG_MMU, "%s(%08x, %d, %d) -> %08x, ret = %d\n",
> -                  __func__, vaddr, is_write, mmu_idx, paddr, ret);
> +                  __func__, vaddr, access_type, mmu_idx, paddr, ret);
>  
>      if (ret == 0) {
>          tlb_set_page(cs,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Aurelien Jarno]

On 2016-06-10 19:26, Sergey Sorokin wrote:
> There are functions cpu_unaligned_access() and do_unaligned_access() that
> are called with access type and mmu index arguments. But these arguments
> are named 'is_write' and 'is_user' in their declarations.
> The patch fixes the names to avoid a confusion.

Unless I missed something, it seems that the is_user/mmu_idx argument is
never used. Should we maybe just drop it?

Otherwise it looks fine.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Paolo Bonzini]

On 10/06/2016 19:26, Sergey Sorokin wrote:
> cpu-common.h is not included in qom/cpu.h what do you think? Should
> it be included? Or may be MMUAccessType should be just moved into
> another header. For example into exec/memattrs.h

You can move it to qom/cpu.h.

Paolo

Re: [Qemu-devel] [PATCH 1/2] Fix confusing argument names of do_unaligned_access() functions

[Sergey Sorokin]

Seems arm_cpu_do_unaligned_access() function could use it. It uses cpu_mmu_index() for now but I think use of mmu_idx is preferred. Anyway it's the subject for another patch.

13.06.2016, 10:47, "Aurelien Jarno" <aurelien@aurel32.net>:
> On 2016-06-10 19:26, Sergey Sorokin wrote:
>>  There are functions cpu_unaligned_access() and do_unaligned_access() that
>>  are called with access type and mmu index arguments. But these arguments
>>  are named 'is_write' and 'is_user' in their declarations.
>>  The patch fixes the names to avoid a confusion.
>
> Unless I missed something, it seems that the is_user/mmu_idx argument is
> never used. Should we maybe just drop it?
>
> Otherwise it looks fine.
>
> --
> Aurelien Jarno GPG: 4096R/1DDD8C9B
> aurelien@aurel32.net http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/2] Fix a confusing argument name in tlb_fill() function

[Eduardo Habkost]

On Fri, Jun 10, 2016 at 07:26:39PM +0300, Sergey Sorokin wrote:
> The function tlb_fill() is called with access type argument which is named
> 'is_write' in its declaration. The patch fixes the argument name
> to avoid a confusion.
> 
> Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo