[Qemu-devel] [PATCH 0/3] target-mips: Add support for misaligned accesses

[Qemu-devel] [PATCH 0/3] target-mips: Add support for misaligned accesses

[Yongbok Kim]

This patch set adds support for misaligned memory accesses in MIPS architecture
Release 6 and MIPS SIMD Architecture.

The behaviour, semantics, and architecture specifications of misaligned memory
accesses are described in:
MIPS Architecture For Programmers Volume I-A: Introduction to the MIPS64
Architecture, Appendix B Misaligned Memory Accesses.
Available at http://www.imgtec.com/mips/architectures/mips64.asp

Regards,
Yongbok


Yongbok Kim (3):
  softmmu: Add size argument to do_unaligned_access()
  target-mips: Misaligned Memory Accesses for R6
  target-mips: Misaligned Memory Accesses for MSA

 include/qom/cpu.h            |    7 +++--
 softmmu_template.h           |   24 +++++++++++-----------
 target-alpha/cpu-qom.h       |    3 +-
 target-alpha/mem_helper.c    |    3 +-
 target-mips/cpu-qom.h        |    3 +-
 target-mips/cpu.h            |    2 +
 target-mips/helper.c         |   36 +++++++++++++++++++++++++++++++++
 target-mips/op_helper.c      |   45 +++++++++++++++++++++++++++++++++++++++++-
 target-mips/translate_init.c |    2 +-
 target-sparc/cpu-qom.h       |    3 +-
 target-sparc/ldst_helper.c   |    3 +-
 target-xtensa/cpu-qom.h      |    3 +-
 target-xtensa/op_helper.c    |    2 +-
 13 files changed, 112 insertions(+), 24 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH 1/3] softmmu: Add size argument to do_unaligned_access()

[Yongbok Kim]

Pass a data size argument to do_unaligned_access().
It is unable to find if an access spans two pages without the data size
in the call back function.

Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
---
 include/qom/cpu.h          |    7 ++++---
 softmmu_template.h         |   24 ++++++++++++------------
 target-alpha/cpu-qom.h     |    3 ++-
 target-alpha/mem_helper.c  |    3 ++-
 target-mips/cpu-qom.h      |    3 ++-
 target-mips/op_helper.c    |    2 +-
 target-sparc/cpu-qom.h     |    3 ++-
 target-sparc/ldst_helper.c |    3 ++-
 target-xtensa/cpu-qom.h    |    3 ++-
 target-xtensa/op_helper.c  |    2 +-
 10 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 39f0f19..6ba2dad 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -134,7 +134,8 @@ typedef struct CPUClass {
     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);
+                                int is_write, int is_user, uintptr_t retaddr,
+                                unsigned size);
     bool (*virtio_is_big_endian)(CPUState *cpu);
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
@@ -591,11 +592,11 @@ 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,
-                                        uintptr_t retaddr)
+                                        uintptr_t retaddr, unsigned size)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
-    cc->do_unaligned_access(cpu, addr, is_write, is_user, retaddr);
+    cc->do_unaligned_access(cpu, addr, is_write, is_user, retaddr, size);
 }
 #endif
 
diff --git a/softmmu_template.h b/softmmu_template.h
index 16b0852..d896b9c 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -185,7 +185,7 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
 #ifdef ALIGNED_ONLY
         if ((addr & (DATA_SIZE - 1)) != 0) {
             cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
-                                 mmu_idx, retaddr);
+                                 mmu_idx, retaddr, DATA_SIZE);
         }
 #endif
         if (!VICTIM_TLB_HIT(ADDR_READ)) {
@@ -220,7 +220,7 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
     do_unaligned_access:
 #ifdef ALIGNED_ONLY
         cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
-                             mmu_idx, retaddr);
+                             mmu_idx, retaddr, DATA_SIZE);
 #endif
         addr1 = addr & ~(DATA_SIZE - 1);
         addr2 = addr1 + DATA_SIZE;
@@ -239,7 +239,7 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
 #ifdef ALIGNED_ONLY
     if ((addr & (DATA_SIZE - 1)) != 0) {
         cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
-                             mmu_idx, retaddr);
+                             mmu_idx, retaddr, DATA_SIZE);
     }
 #endif
 
@@ -273,7 +273,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
 #ifdef ALIGNED_ONLY
         if ((addr & (DATA_SIZE - 1)) != 0) {
             cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
-                                 mmu_idx, retaddr);
+                                 mmu_idx, retaddr, DATA_SIZE);
         }
 #endif
         if (!VICTIM_TLB_HIT(ADDR_READ)) {
@@ -308,7 +308,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
     do_unaligned_access:
 #ifdef ALIGNED_ONLY
         cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
-                             mmu_idx, retaddr);
+                             mmu_idx, retaddr, DATA_SIZE);
 #endif
         addr1 = addr & ~(DATA_SIZE - 1);
         addr2 = addr1 + DATA_SIZE;
@@ -327,7 +327,7 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr, int mmu_idx,
 #ifdef ALIGNED_ONLY
     if ((addr & (DATA_SIZE - 1)) != 0) {
         cpu_unaligned_access(ENV_GET_CPU(env), addr, READ_ACCESS_TYPE,
-                             mmu_idx, retaddr);
+                             mmu_idx, retaddr, DATA_SIZE);
     }
 #endif
 
@@ -401,7 +401,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
 #ifdef ALIGNED_ONLY
         if ((addr & (DATA_SIZE - 1)) != 0) {
             cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
-                                 mmu_idx, retaddr);
+                                 mmu_idx, retaddr, DATA_SIZE);
         }
 #endif
         if (!VICTIM_TLB_HIT(addr_write)) {
@@ -433,7 +433,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
     do_unaligned_access:
 #ifdef ALIGNED_ONLY
         cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
-                             mmu_idx, retaddr);
+                             mmu_idx, retaddr, DATA_SIZE);
 #endif
         /* XXX: not efficient, but simple */
         /* Note: relies on the fact that tlb_fill() does not remove the
@@ -453,7 +453,7 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
 #ifdef ALIGNED_ONLY
     if ((addr & (DATA_SIZE - 1)) != 0) {
         cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
-                             mmu_idx, retaddr);
+                             mmu_idx, retaddr, DATA_SIZE);
     }
 #endif
 
@@ -482,7 +482,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
 #ifdef ALIGNED_ONLY
         if ((addr & (DATA_SIZE - 1)) != 0) {
             cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
-                                 mmu_idx, retaddr);
+                                 mmu_idx, retaddr, DATA_SIZE);
         }
 #endif
         if (!VICTIM_TLB_HIT(addr_write)) {
@@ -514,7 +514,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
     do_unaligned_access:
 #ifdef ALIGNED_ONLY
         cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
-                             mmu_idx, retaddr);
+                             mmu_idx, retaddr, DATA_SIZE);
 #endif
         /* XXX: not efficient, but simple */
         /* Note: relies on the fact that tlb_fill() does not remove the
@@ -534,7 +534,7 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
 #ifdef ALIGNED_ONLY
     if ((addr & (DATA_SIZE - 1)) != 0) {
         cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
-                             mmu_idx, retaddr);
+                             mmu_idx, retaddr, DATA_SIZE);
     }
 #endif
 
diff --git a/target-alpha/cpu-qom.h b/target-alpha/cpu-qom.h
index b01c6c8..273a8ed 100644
--- a/target-alpha/cpu-qom.h
+++ b/target-alpha/cpu-qom.h
@@ -86,6 +86,7 @@ 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);
+                                   int is_write, int is_user, uintptr_t retaddr,
+                                   unsigned size);
 
 #endif
diff --git a/target-alpha/mem_helper.c b/target-alpha/mem_helper.c
index fc4f57a..1d4666a 100644
--- a/target-alpha/mem_helper.c
+++ b/target-alpha/mem_helper.c
@@ -97,7 +97,8 @@ uint64_t helper_stq_c_phys(CPUAlphaState *env, uint64_t p, uint64_t v)
 }
 
 void alpha_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
-                                   int is_write, int is_user, uintptr_t retaddr)
+                                   int is_write, int is_user, uintptr_t retaddr,
+                                   unsigned size)
 {
     AlphaCPU *cpu = ALPHA_CPU(cs);
     CPUAlphaState *env = &cpu->env;
diff --git a/target-mips/cpu-qom.h b/target-mips/cpu-qom.h
index 4d6f9de..6d54314 100644
--- a/target-mips/cpu-qom.h
+++ b/target-mips/cpu-qom.h
@@ -86,6 +86,7 @@ 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);
+                                  int is_write, int is_user, uintptr_t retaddr,
+                                  unsigned size);
 
 #endif
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 73a8e45..ca5fe43 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -2208,7 +2208,7 @@ void helper_wait(CPUMIPSState *env)
 
 void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
                                   int access_type, int is_user,
-                                  uintptr_t retaddr)
+                                  uintptr_t retaddr, unsigned size)
 {
     MIPSCPU *cpu = MIPS_CPU(cs);
     CPUMIPSState *env = &cpu->env;
diff --git a/target-sparc/cpu-qom.h b/target-sparc/cpu-qom.h
index 477c4d5..4bfbb84 100644
--- a/target-sparc/cpu-qom.h
+++ b/target-sparc/cpu-qom.h
@@ -83,6 +83,7 @@ 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);
+                                                 int is_user, uintptr_t retaddr,
+                                                 unsigned size);
 
 #endif
diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
index c7ad47d..7837d64 100644
--- a/target-sparc/ldst_helper.c
+++ b/target-sparc/ldst_helper.c
@@ -2420,7 +2420,8 @@ 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)
+                                                 int is_user, uintptr_t retaddr,
+                                                 unsigned size)
 {
     SPARCCPU *cpu = SPARC_CPU(cs);
     CPUSPARCState *env = &cpu->env;
diff --git a/target-xtensa/cpu-qom.h b/target-xtensa/cpu-qom.h
index 2258224..2508228 100644
--- a/target-xtensa/cpu-qom.h
+++ b/target-xtensa/cpu-qom.h
@@ -94,6 +94,7 @@ 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);
+                                    int is_write, int is_user,
+                                    uintptr_t retaddr, unsigned size);
 
 #endif
diff --git a/target-xtensa/op_helper.c b/target-xtensa/op_helper.c
index be657e6..0804791 100644
--- a/target-xtensa/op_helper.c
+++ b/target-xtensa/op_helper.c
@@ -33,7 +33,7 @@
 #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 is_write, int is_user, uintptr_t retaddr, unsigned size)
 {
     XtensaCPU *cpu = XTENSA_CPU(cs);
     CPUXtensaState *env = &cpu->env;
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/3] target-mips: Misaligned Memory Accesses for R6

[Yongbok Kim]

Release 6 requires misaligned memory access support for all ordinary memory
access instructions (for example, LW/SW, LWC1/SWC1).
However misaligned support is not provided for certain special memory accesses
such as atomics (for example, LL/SC).

In the mips_cpu_do_unaligned_access() callback, if it is R6 core it checks further
whether the address is valid.

Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
---
 target-mips/cpu.h            |    2 ++
 target-mips/helper.c         |   36 ++++++++++++++++++++++++++++++++++++
 target-mips/op_helper.c      |   13 +++++++++++++
 target-mips/translate_init.c |    2 +-
 4 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index f9d2b4c..6586d89 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -760,6 +760,8 @@ int mips_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int rw,
 void r4k_invalidate_tlb (CPUMIPSState *env, int idx, int use_extra);
 hwaddr cpu_mips_translate_address (CPUMIPSState *env, target_ulong address,
 		                               int rw);
+bool cpu_mips_validate_access(CPUMIPSState *env, target_ulong address,
+                            target_ulong badvaddr, unsigned data_size, int rw);
 #endif
 target_ulong exception_resume_pc (CPUMIPSState *env);
 
diff --git a/target-mips/helper.c b/target-mips/helper.c
index 8e3204a..6c44124 100644
--- a/target-mips/helper.c
+++ b/target-mips/helper.c
@@ -391,6 +391,42 @@ hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address, int r
     }
 }
 
+bool cpu_mips_validate_access(CPUMIPSState *env, target_ulong address,
+                            target_ulong badvaddr, unsigned data_size, int rw)
+{
+    hwaddr physical;
+    int prot;
+    int access_type = ACCESS_INT;
+    int ret;
+    target_ulong addr;
+
+    addr = address & ~((target_ulong) data_size - 1);
+    ret = get_physical_address(env, &physical, &prot,
+            addr, rw, access_type);
+    if (ret != TLBRET_MATCH) {
+        if (ret != TLBRET_BADADDR && addr > badvaddr) {
+            badvaddr = addr;
+        }
+        raise_mmu_exception(env, badvaddr, rw, ret);
+        return false;
+    }
+    if ((data_size > 1)
+        && unlikely(((address & ~TARGET_PAGE_MASK) + data_size - 1)
+                    >= TARGET_PAGE_SIZE)) {
+        addr += data_size;
+        ret = get_physical_address(env, &physical, &prot, addr, rw,
+              access_type);
+        if (ret != TLBRET_MATCH) {
+            if (ret != TLBRET_BADADDR) {
+                badvaddr = addr;
+            }
+            raise_mmu_exception(env, badvaddr, rw, ret);
+            return false;
+        }
+    }
+    return true;
+}
+
 static const char * const excp_names[EXCP_LAST + 1] = {
     [EXCP_RESET] = "reset",
     [EXCP_SRESET] = "soft reset",
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index ca5fe43..dacc92b 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -2215,6 +2215,19 @@ void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
     int error_code = 0;
     int excp;
 
+    if (env->insn_flags & ISA_MIPS32R6) {
+        /* Release 6 provides support for misaligned memory access for
+         * all ordinary memory reference instructions
+         * */
+        if (!cpu_mips_validate_access(env, addr, addr, size, access_type)) {
+            CPUState *cs = CPU(mips_env_get_cpu(env));
+            do_raise_exception_err(env, cs->exception_index,
+                                   env->error_code, retaddr);
+            return;
+        }
+        return;
+    }
+
     env->CP0_BadVAddr = addr;
 
     if (access_type == MMU_DATA_STORE) {
diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c
index 85a65e7..ec54fef 100644
--- a/target-mips/translate_init.c
+++ b/target-mips/translate_init.c
@@ -607,7 +607,7 @@ static const mips_def_t mips_defs[] =
     },
     {
         /* A generic CPU supporting MIPS64 Release 6 ISA.
-           FIXME: Support IEEE 754-2008 FP and misaligned memory accesses.
+           FIXME: Support IEEE 754-2008 FP.
                   Eventually this should be replaced by a real CPU model. */
         .name = "MIPS64R6-generic",
         .CP0_PRid = 0x00010000,
-- 
1.7.5.4

[Qemu-devel] [PATCH 3/3] target-mips: Misaligned Memory Accesses for MSA

[Yongbok Kim]

MIPS SIMD Architecture vector loads and stores require misalignment support.
MSA Memory access should work as an atomic operation. Therefore, it has to
check validity of all the addresses for the operation.

Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
---
 target-mips/op_helper.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index dacc92b..89a7de6 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -3571,6 +3571,24 @@ FOP_CONDN_S(sne,  (float32_lt(fst1, fst0, &env->active_fpu.fp_status)
 /* Element-by-element access macros */
 #define DF_ELEMENTS(df) (MSA_WRLEN / DF_BITS(df))
 
+#if !defined(CONFIG_USER_ONLY)
+static bool cpu_mips_validate_msa_block_access(CPUMIPSState *env,
+                                        target_ulong address, int df, int rw)
+{
+    int i;
+    for (i = 0; i < DF_ELEMENTS(df); i++) {
+        if (!cpu_mips_validate_access(env, address + (i << df),
+            address, (1 << df), rw)) {
+            CPUState *cs = CPU(mips_env_get_cpu(env));
+            helper_raise_exception_err(env, cs->exception_index,
+                                       env->error_code);
+            return false;
+        }
+    }
+    return true;
+}
+#endif
+
 void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
                      int32_t s10)
 {
@@ -3578,6 +3596,12 @@ void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
     target_ulong addr = env->active_tc.gpr[rs] + (s10 << df);
     int i;
 
+#if !defined(CONFIG_USER_ONLY)
+    if (!cpu_mips_validate_msa_block_access(env, addr, df, MMU_DATA_LOAD)) {
+        return;
+    }
+#endif
+
     switch (df) {
     case DF_BYTE:
         for (i = 0; i < DF_ELEMENTS(DF_BYTE); i++) {
@@ -3613,6 +3637,12 @@ void helper_msa_st_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
     target_ulong addr = env->active_tc.gpr[rs] + (s10 << df);
     int i;
 
+#if !defined(CONFIG_USER_ONLY)
+    if (!cpu_mips_validate_msa_block_access(env, addr, df, MMU_DATA_STORE)) {
+        return;
+    }
+#endif
+
     switch (df) {
     case DF_BYTE:
         for (i = 0; i < DF_ELEMENTS(DF_BYTE); i++) {
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 2/3] target-mips: Misaligned Memory Accesses for R6

[Peter Maydell]

On 1 May 2015 at 16:24, Yongbok Kim <yongbok.kim@imgtec.com> wrote:
> Release 6 requires misaligned memory access support for all ordinary memory
> access instructions (for example, LW/SW, LWC1/SWC1).
> However misaligned support is not provided for certain special memory accesses
> such as atomics (for example, LL/SC).
>
> In the mips_cpu_do_unaligned_access() callback, if it is R6 core it checks further
> whether the address is valid.
>
> Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
> ---
>  target-mips/cpu.h            |    2 ++
>  target-mips/helper.c         |   36 ++++++++++++++++++++++++++++++++++++
>  target-mips/op_helper.c      |   13 +++++++++++++
>  target-mips/translate_init.c |    2 +-
>  4 files changed, 52 insertions(+), 1 deletions(-)
>
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index f9d2b4c..6586d89 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -760,6 +760,8 @@ int mips_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int rw,
>  void r4k_invalidate_tlb (CPUMIPSState *env, int idx, int use_extra);
>  hwaddr cpu_mips_translate_address (CPUMIPSState *env, target_ulong address,
>                                                int rw);
> +bool cpu_mips_validate_access(CPUMIPSState *env, target_ulong address,
> +                            target_ulong badvaddr, unsigned data_size, int rw);
>  #endif
>  target_ulong exception_resume_pc (CPUMIPSState *env);
>
> diff --git a/target-mips/helper.c b/target-mips/helper.c
> index 8e3204a..6c44124 100644
> --- a/target-mips/helper.c
> +++ b/target-mips/helper.c
> @@ -391,6 +391,42 @@ hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address, int r
>      }
>  }
>
> +bool cpu_mips_validate_access(CPUMIPSState *env, target_ulong address,
> +                            target_ulong badvaddr, unsigned data_size, int rw)
> +{
> +    hwaddr physical;
> +    int prot;
> +    int access_type = ACCESS_INT;
> +    int ret;
> +    target_ulong addr;
> +
> +    addr = address & ~((target_ulong) data_size - 1);
> +    ret = get_physical_address(env, &physical, &prot,
> +            addr, rw, access_type);

Isn't a recheck of the TLB on every unaligned access going
to have poor performance? If we know from the type of the
insn whether an unaligned access is OK or not there ought
to be some way to avoid doing the check at all...

-- PMM

Re: [Qemu-devel] [PATCH 3/3] target-mips: Misaligned Memory Accesses for MSA

[Peter Maydell]

On 1 May 2015 at 16:24, Yongbok Kim <yongbok.kim@imgtec.com> wrote:
> MIPS SIMD Architecture vector loads and stores require misalignment support.
> MSA Memory access should work as an atomic operation. Therefore, it has to
> check validity of all the addresses for the operation.
>
> Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
> ---
>  target-mips/op_helper.c |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index dacc92b..89a7de6 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -3571,6 +3571,24 @@ FOP_CONDN_S(sne,  (float32_lt(fst1, fst0, &env->active_fpu.fp_status)
>  /* Element-by-element access macros */
>  #define DF_ELEMENTS(df) (MSA_WRLEN / DF_BITS(df))
>
> +#if !defined(CONFIG_USER_ONLY)
> +static bool cpu_mips_validate_msa_block_access(CPUMIPSState *env,
> +                                        target_ulong address, int df, int rw)
> +{
> +    int i;
> +    for (i = 0; i < DF_ELEMENTS(df); i++) {
> +        if (!cpu_mips_validate_access(env, address + (i << df),
> +            address, (1 << df), rw)) {
> +            CPUState *cs = CPU(mips_env_get_cpu(env));
> +            helper_raise_exception_err(env, cs->exception_index,
> +                                       env->error_code);

I was wondering if this would get the correct PC in the exception
case, but we always call save_cpu_state() before calling the
msa_ld/st_df helpers, so it will.

-- PMM

Re: [Qemu-devel] [PATCH 3/3] target-mips: Misaligned Memory Accesses for MSA

[Yongbok Kim]

On 01/05/2015 16:43, Peter Maydell wrote:
> On 1 May 2015 at 16:24, Yongbok Kim <yongbok.kim@imgtec.com> wrote:
>> MIPS SIMD Architecture vector loads and stores require misalignment support.
>> MSA Memory access should work as an atomic operation. Therefore, it has to
>> check validity of all the addresses for the operation.
>>
>> Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
>> ---
>>  target-mips/op_helper.c |   30 ++++++++++++++++++++++++++++++
>>  1 files changed, 30 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
>> index dacc92b..89a7de6 100644
>> --- a/target-mips/op_helper.c
>> +++ b/target-mips/op_helper.c
>> @@ -3571,6 +3571,24 @@ FOP_CONDN_S(sne,  (float32_lt(fst1, fst0, &env->active_fpu.fp_status)
>>  /* Element-by-element access macros */
>>  #define DF_ELEMENTS(df) (MSA_WRLEN / DF_BITS(df))
>>
>> +#if !defined(CONFIG_USER_ONLY)
>> +static bool cpu_mips_validate_msa_block_access(CPUMIPSState *env,
>> +                                        target_ulong address, int df, int rw)
>> +{
>> +    int i;
>> +    for (i = 0; i < DF_ELEMENTS(df); i++) {
>> +        if (!cpu_mips_validate_access(env, address + (i << df),
>> +            address, (1 << df), rw)) {
>> +            CPUState *cs = CPU(mips_env_get_cpu(env));
>> +            helper_raise_exception_err(env, cs->exception_index,
>> +                                       env->error_code);
> I was wondering if this would get the correct PC in the exception
> case, but we always call save_cpu_state() before calling the
> msa_ld/st_df helpers, so it will.
>
> -- PMM
Yes it does because of the save_cpu_state(). Actually I have considered to use
cpu_restore_state() with GETRA() but it looks like using save_cpu_state() is quite
common in the target-mips. It would be good such clean-up for all the cases in the future work.
But this patch I would follow existing style for the consistency.

Regards,
Yongbok

Re: [Qemu-devel] [PATCH 3/3] target-mips: Misaligned Memory Accesses for MSA

[Leon Alrae]

On 01/05/2015 16:43, Peter Maydell wrote:
>> +#if !defined(CONFIG_USER_ONLY)
>> +static bool cpu_mips_validate_msa_block_access(CPUMIPSState *env,
>> +                                        target_ulong address, int df, int rw)
>> +{
>> +    int i;
>> +    for (i = 0; i < DF_ELEMENTS(df); i++) {
>> +        if (!cpu_mips_validate_access(env, address + (i << df),
>> +            address, (1 << df), rw)) {
>> +            CPUState *cs = CPU(mips_env_get_cpu(env));
>> +            helper_raise_exception_err(env, cs->exception_index,
>> +                                       env->error_code);
> 
> I was wondering if this would get the correct PC in the exception
> case, but we always call save_cpu_state() before calling the
> msa_ld/st_df helpers, so it will.

FYI, quite recently Richard suggested a clean-up for this (and all other
MIPS load/store instructions using helpers), so save_cpu_state()
wouldn’t be required. I haven’t got round to it yet but sounds like a
nice improvement.

Leon

Re: [Qemu-devel] [PATCH 3/3] target-mips: Misaligned Memory Accesses for MSA

[Leon Alrae]

On 01/05/15 16:24, Yongbok Kim wrote:
> MIPS SIMD Architecture vector loads and stores require misalignment support.
> MSA Memory access should work as an atomic operation. Therefore, it has to
> check validity of all the addresses for the operation.

As far as I can tell mips_cpu_do_unaligned_access() will be still
generating AdE exceptions for MSA loads/stores in MIPS R5 which doesn't
seem to be correct.

> 
> Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
> ---
>  target-mips/op_helper.c |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index dacc92b..89a7de6 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -3571,6 +3571,24 @@ FOP_CONDN_S(sne,  (float32_lt(fst1, fst0, &env->active_fpu.fp_status)
>  /* Element-by-element access macros */
>  #define DF_ELEMENTS(df) (MSA_WRLEN / DF_BITS(df))
>  
> +#if !defined(CONFIG_USER_ONLY)
> +static bool cpu_mips_validate_msa_block_access(CPUMIPSState *env,
> +                                        target_ulong address, int df, int rw)
> +{
> +    int i;
> +    for (i = 0; i < DF_ELEMENTS(df); i++) {

Do we really need to check each element, wouldn't byte 0 and byte
(MSA_WRLEN/8 - 1) be enough?

> +        if (!cpu_mips_validate_access(env, address + (i << df),
> +            address, (1 << df), rw)) {

I believe this would look better if this line was aligned with the first
argument of the function (and would be consistent with the helper below).

> +            CPUState *cs = CPU(mips_env_get_cpu(env));
> +            helper_raise_exception_err(env, cs->exception_index,
> +                                       env->error_code);
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +#endif
> +
>  void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
>                       int32_t s10)
>  {
> @@ -3578,6 +3596,12 @@ void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
>      target_ulong addr = env->active_tc.gpr[rs] + (s10 << df);
>      int i;
>  
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!cpu_mips_validate_msa_block_access(env, addr, df, MMU_DATA_LOAD)) {
> +        return;
> +    }

Shouldn't this be called only for cases where page boundary is crossed?
Otherwise I don't think this validation is needed.

> +#endif
> +
>      switch (df) {
>      case DF_BYTE:
>          for (i = 0; i < DF_ELEMENTS(DF_BYTE); i++) {
> @@ -3613,6 +3637,12 @@ void helper_msa_st_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
>      target_ulong addr = env->active_tc.gpr[rs] + (s10 << df);
>      int i;
>  
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!cpu_mips_validate_msa_block_access(env, addr, df, MMU_DATA_STORE)) {
> +        return;
> +    }

Same.

Thanks,
Leon