diff mbox

[1/2] Fix confusing argument names of do_unaligned_access() functions

Message ID 1465575999-3594234-2-git-send-email-afarallax@yandex.ru
State New
Headers show

Commit Message

Sergey Sorokin June 10, 2016, 4:26 p.m. UTC 
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(-)

Comments

Peter Maydell June 10, 2016, 4:33 p.m. UTC | #1 
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
Sergey Sorokin June 10, 2016, 4:36 p.m. UTC | #2 
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
Sergey Sorokin June 10, 2016, 4:42 p.m. UTC | #3 
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
Peter Maydell June 10, 2016, 4:43 p.m. UTC | #4 
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
Sergey Sorokin June 10, 2016, 5:26 p.m. UTC | #5 
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
Aurelien Jarno June 13, 2016, 7:03 a.m. UTC | #6 
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.
Paolo Bonzini June 13, 2016, 8:23 a.m. UTC | #7 
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
Sergey Sorokin June 14, 2016, 12:01 p.m. UTC | #8 
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
diff mbox

Patch

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;