[Qemu-devel] [PATCH 00/15] Clean up cpu-ldst ld/st memory accessors

[Qemu-devel] [PATCH 00/15] Clean up cpu-ldst ld/st memory accessors

[Peter Maydell]

I was looking at our confusing mess of memory accessor functions,
and I realised that partly it was confusing because we have a
bunch of unnecessary junk lurking in there :-) This series
attempts to clean things up by removing things we weren't using
at all or were only using by mistake in a few places:

 * ldul_*: not used
 * ld* (ldl, etc): hardly used
 * ld*_kernel: not used
 * ld*_raw: hardly used
 * cpu_{ld,st}{fq,fl}: not used

The dull parts of this series are removing the unused macros
and fixing uses of the hardly-used macros so those can be
deleted too. This series also switches to using inline functions
rather than macros for the user-only cpu_ld/st* accessors, bringing
them into line with the softmmu configs. This has the nice
side effect of letting us get rid of the _raw accessor macros too.
I've also thrown in a commit which cleans up the doc comments.

Peter Maydell (15):
  cpu_ldst.h: Remove unused ldul_ macros
  monitor.c: Use ld*_p() instead of ld*_raw()
  target-sparc: Don't use {ld,st}*_raw functions
  linux-user/elfload.c: Don't use _raw accessor functions
  bsd-user/elfload.c: Don't use ldl() or ldq_raw()
  linux-user/vm86.c: Use cpu_ldl_data &c rather than plain ldl &c
  linux-user/main.c (m68k): Use get_user_u16 rather than lduw in
    cpu_loop
  target-mips: Don't use _raw load/store accessors
  cpu_ldst.h: Drop unused ld/st*_kernel defines
  cpu_ldst.h: Remove unused very short ld*/st* defines
  cpu_ldst.h: Use inline functions for usermode cpu_ld/st accessors
  cpu_ldst_template.h: Use ld*_p directly rather than via ld*_raw macros
  cpu_ldst.h: Drop unused _raw macros, saddr() and laddr()
  cpu_ldst_template.h: Drop unused cpu_ldfq/stfq/ldfl/stfl accessors
  cpu_ldst.h, cpu-all.h, bswap.h: Update documentation on ld/st
    accessors

 bsd-user/elfload.c                        |  11 +-
 include/exec/cpu-all.h                    |  38 +------
 include/exec/cpu_ldst.h                   | 162 +++++++++---------------------
 include/exec/cpu_ldst_template.h          |  60 +----------
 include/exec/cpu_ldst_useronly_template.h |  81 +++++++++++++++
 include/qemu/bswap.h                      |  11 +-
 linux-user/elfload.c                      |   7 +-
 linux-user/main.c                         |   2 +-
 linux-user/vm86.c                         |  57 ++++++-----
 monitor.c                                 |   8 +-
 target-i386/seg_helper.c                  |  16 ++-
 target-mips/op_helper.c                   |   4 +-
 target-sparc/ldst_helper.c                |  24 ++---
 13 files changed, 224 insertions(+), 257 deletions(-)
 create mode 100644 include/exec/cpu_ldst_useronly_template.h

-- 
1.9.1

[Qemu-devel] [PATCH 01/15] cpu_ldst.h: Remove unused ldul_ macros

[Peter Maydell]

The five ldul_ macros are not used anywhere and are marked up with an XXX
comment. "ldul" is a non-standard prefix for our family of load instructions:
we don't mark 32-bit accesses for signedness because they return a 32 bit
quantity. So just delete them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/cpu_ldst.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index e5550e7..4700831 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -151,15 +151,6 @@
 
 #else
 
-/* XXX: find something cleaner.
- * Furthermore, this is false for 64 bits targets
- */
-#define ldul_user       ldl_user
-#define ldul_kernel     ldl_kernel
-#define ldul_hypv       ldl_hypv
-#define ldul_executive  ldl_executive
-#define ldul_supervisor ldl_supervisor
-
 /* The memory helpers for tcg-generated code need tcg_target_long etc.  */
 #include "tcg.h"
 
-- 
1.9.1

[Qemu-devel] [PATCH 02/15] monitor.c: Use ld*_p() instead of ld*_raw()

[Peter Maydell]

The monitor code for doing a memory_dump() was using ld*_raw() to do
target-CPU accesses out of a local buf[] array. The correct functions
for this purpose are ld*_p(), which take a host pointer, rather than
ld*_raw(), which take an integer representing a guest address and
are somewhat meaningless in softmmu configurations. Nobody noticed
because for softmmu the _raw functions are the same as ldl_p but
with some extra casts thrown in. Switch to using the correct functions
instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 monitor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 1808e41..7e4f605 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1292,16 +1292,16 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
             switch(wsize) {
             default:
             case 1:
-                v = ldub_raw(buf + i);
+                v = ldub_p(buf + i);
                 break;
             case 2:
-                v = lduw_raw(buf + i);
+                v = lduw_p(buf + i);
                 break;
             case 4:
-                v = (uint32_t)ldl_raw(buf + i);
+                v = (uint32_t)ldl_p(buf + i);
                 break;
             case 8:
-                v = ldq_raw(buf + i);
+                v = ldq_p(buf + i);
                 break;
             }
             monitor_printf(mon, " ");
-- 
1.9.1

Re: [Qemu-devel] [PATCH 02/15] monitor.c: Use ld*_p() instead of ld*_raw()

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The monitor code for doing a memory_dump() was using ld*_raw() to do
> target-CPU accesses out of a local buf[] array. The correct functions
> for this purpose are ld*_p(), which take a host pointer, rather than
> ld*_raw(), which take an integer representing a guest address and
> are somewhat meaningless in softmmu configurations. Nobody noticed
> because for softmmu the _raw functions are the same as ldl_p but
> with some extra casts thrown in. Switch to using the correct functions
> instead.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  monitor.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 1808e41..7e4f605 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1292,16 +1292,16 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>              switch(wsize) {
>              default:
>              case 1:
> -                v = ldub_raw(buf + i);
> +                v = ldub_p(buf + i);
>                  break;
>              case 2:
> -                v = lduw_raw(buf + i);
> +                v = lduw_p(buf + i);
>                  break;
>              case 4:
> -                v = (uint32_t)ldl_raw(buf + i);
> +                v = (uint32_t)ldl_p(buf + i);
>                  break;
>              case 8:
> -                v = ldq_raw(buf + i);
> +                v = ldq_p(buf + i);
>                  break;
>              }
>              monitor_printf(mon, " ");

-- 
Alex Bennée

[Qemu-devel] [PATCH 03/15] target-sparc: Don't use {ld, st}*_raw functions

[Peter Maydell]

Instead of using the _raw family of ld/st accessor functions, use
cpu_*_data. All this code is CONFIG_USER_ONLY, so the two are the
same semantically, but the _raw functions are really a detail of
the implementation which has leaked into a few callsites like this one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-sparc/ldst_helper.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
index 1a62e19..e62228c 100644
--- a/target-sparc/ldst_helper.c
+++ b/target-sparc/ldst_helper.c
@@ -1122,17 +1122,17 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr, int asi, int size,
         {
             switch (size) {
             case 1:
-                ret = ldub_raw(addr);
+                ret = cpu_ldub_data(env, addr);
                 break;
             case 2:
-                ret = lduw_raw(addr);
+                ret = cpu_lduw_data(env, addr);
                 break;
             case 4:
-                ret = ldl_raw(addr);
+                ret = cpu_ldl_data(env, addr);
                 break;
             default:
             case 8:
-                ret = ldq_raw(addr);
+                ret = cpu_ldq_data(env, addr);
                 break;
             }
         }
@@ -1239,17 +1239,17 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
         {
             switch (size) {
             case 1:
-                stb_raw(addr, val);
+                cpu_stb_data(env, addr, val);
                 break;
             case 2:
-                stw_raw(addr, val);
+                cpu_stw_data(env, addr, val);
                 break;
             case 4:
-                stl_raw(addr, val);
+                cpu_stl_data(env, addr, val);
                 break;
             case 8:
             default:
-                stq_raw(addr, val);
+                cpu_stq_data(env, addr, val);
                 break;
             }
         }
@@ -2289,8 +2289,8 @@ void helper_ldqf(CPUSPARCState *env, target_ulong addr, int mem_idx)
         break;
     }
 #else
-    u.ll.upper = ldq_raw(address_mask(env, addr));
-    u.ll.lower = ldq_raw(address_mask(env, addr + 8));
+    u.ll.upper = cpu_ldq_data(env, address_mask(env, addr));
+    u.ll.lower = cpu_ldq_data(env, address_mask(env, addr + 8));
     QT0 = u.q;
 #endif
 }
@@ -2326,8 +2326,8 @@ void helper_stqf(CPUSPARCState *env, target_ulong addr, int mem_idx)
     }
 #else
     u.q = QT0;
-    stq_raw(address_mask(env, addr), u.ll.upper);
-    stq_raw(address_mask(env, addr + 8), u.ll.lower);
+    cpu_stq_data(env, address_mask(env, addr), u.ll.upper);
+    cpu_stq_data(env, address_mask(env, addr + 8), u.ll.lower);
 #endif
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 03/15] target-sparc: Don't use {ld, st}*_raw functions

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Instead of using the _raw family of ld/st accessor functions, use
> cpu_*_data. All this code is CONFIG_USER_ONLY, so the two are the
> same semantically, but the _raw functions are really a detail of
> the implementation which has leaked into a few callsites like this one.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  target-sparc/ldst_helper.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
> index 1a62e19..e62228c 100644
> --- a/target-sparc/ldst_helper.c
> +++ b/target-sparc/ldst_helper.c
> @@ -1122,17 +1122,17 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong addr, int asi, int size,
>          {
>              switch (size) {
>              case 1:
> -                ret = ldub_raw(addr);
> +                ret = cpu_ldub_data(env, addr);
>                  break;
>              case 2:
> -                ret = lduw_raw(addr);
> +                ret = cpu_lduw_data(env, addr);
>                  break;
>              case 4:
> -                ret = ldl_raw(addr);
> +                ret = cpu_ldl_data(env, addr);
>                  break;
>              default:
>              case 8:
> -                ret = ldq_raw(addr);
> +                ret = cpu_ldq_data(env, addr);
>                  break;
>              }
>          }
> @@ -1239,17 +1239,17 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
>          {
>              switch (size) {
>              case 1:
> -                stb_raw(addr, val);
> +                cpu_stb_data(env, addr, val);
>                  break;
>              case 2:
> -                stw_raw(addr, val);
> +                cpu_stw_data(env, addr, val);
>                  break;
>              case 4:
> -                stl_raw(addr, val);
> +                cpu_stl_data(env, addr, val);
>                  break;
>              case 8:
>              default:
> -                stq_raw(addr, val);
> +                cpu_stq_data(env, addr, val);
>                  break;
>              }
>          }
> @@ -2289,8 +2289,8 @@ void helper_ldqf(CPUSPARCState *env, target_ulong addr, int mem_idx)
>          break;
>      }
>  #else
> -    u.ll.upper = ldq_raw(address_mask(env, addr));
> -    u.ll.lower = ldq_raw(address_mask(env, addr + 8));
> +    u.ll.upper = cpu_ldq_data(env, address_mask(env, addr));
> +    u.ll.lower = cpu_ldq_data(env, address_mask(env, addr + 8));
>      QT0 = u.q;
>  #endif
>  }
> @@ -2326,8 +2326,8 @@ void helper_stqf(CPUSPARCState *env, target_ulong addr, int mem_idx)
>      }
>  #else
>      u.q = QT0;
> -    stq_raw(address_mask(env, addr), u.ll.upper);
> -    stq_raw(address_mask(env, addr + 8), u.ll.lower);
> +    cpu_stq_data(env, address_mask(env, addr), u.ll.upper);
> +    cpu_stq_data(env, address_mask(env, addr + 8), u.ll.lower);
>  #endif
>  }

-- 
Alex Bennée

[Qemu-devel] [PATCH 04/15] linux-user/elfload.c: Don't use _raw accessor functions

[Peter Maydell]

The _raw accessor functions are an implementation detail that has
leaked out to some callsites. Use get_user_u64() instead of ldq_raw().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/elfload.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index e2596a4..399c021 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -829,8 +829,11 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
     _regs->gpr[1] = infop->start_stack;
 #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
     if (get_ppc64_abi(infop) < 2) {
-        _regs->gpr[2] = ldq_raw(infop->entry + 8) + infop->load_bias;
-        infop->entry = ldq_raw(infop->entry) + infop->load_bias;
+        uint64_t val;
+        get_user_u64(val, infop->entry + 8);
+        _regs->gpr[2] = val + infop->load_bias;
+        get_user_u64(val, infop->entry);
+        infop->entry = val + infop->load_bias;
     } else {
         _regs->gpr[12] = infop->entry;  /* r12 set to global entry address */
     }
-- 
1.9.1

Re: [Qemu-devel] [PATCH 04/15] linux-user/elfload.c: Don't use _raw accessor functions

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The _raw accessor functions are an implementation detail that has
> leaked out to some callsites. Use get_user_u64() instead of ldq_raw().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  linux-user/elfload.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index e2596a4..399c021 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -829,8 +829,11 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
>      _regs->gpr[1] = infop->start_stack;
>  #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
>      if (get_ppc64_abi(infop) < 2) {
> -        _regs->gpr[2] = ldq_raw(infop->entry + 8) + infop->load_bias;
> -        infop->entry = ldq_raw(infop->entry) + infop->load_bias;
> +        uint64_t val;
> +        get_user_u64(val, infop->entry + 8);
> +        _regs->gpr[2] = val + infop->load_bias;
> +        get_user_u64(val, infop->entry);
> +        infop->entry = val + infop->load_bias;
>      } else {
>          _regs->gpr[12] = infop->entry;  /* r12 set to global entry address */
>      }

-- 
Alex Bennée

[Qemu-devel] [PATCH 05/15] bsd-user/elfload.c: Don't use ldl() or ldq_raw()

[Peter Maydell]

Use get_user_u64() and get_user_ual() instead of the ldl() and
ldq_raw() functions.

[Note that this change is not compile tested as it is actually
in dead code -- none of the bsd-user configurations are PPC.]

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 bsd-user/elfload.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
index 93fd9e4..2bf57eb 100644
--- a/bsd-user/elfload.c
+++ b/bsd-user/elfload.c
@@ -351,8 +351,10 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
 
     _regs->gpr[1] = infop->start_stack;
 #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
-    entry = ldq_raw(infop->entry) + infop->load_addr;
-    toc = ldq_raw(infop->entry + 8) + infop->load_addr;
+    get_user_u64(entry, infop->entry);
+    entry += infop->load_addr;
+    get_user_u64(toc, infop->entry + 8);
+    toc += infop->load_addr;
     _regs->gpr[2] = toc;
     infop->entry = entry;
 #endif
@@ -365,8 +367,9 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
     get_user_ual(_regs->gpr[3], pos);
     pos += sizeof(abi_ulong);
     _regs->gpr[4] = pos;
-    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong))
-        tmp = ldl(pos);
+    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong)) {
+        get_user_ual(tmp, pos);
+    }
     _regs->gpr[5] = pos;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 05/15] bsd-user/elfload.c: Don't use ldl() or ldq_raw()

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Use get_user_u64() and get_user_ual() instead of the ldl() and
> ldq_raw() functions.
>
> [Note that this change is not compile tested as it is actually
> in dead code -- none of the bsd-user configurations are PPC.]
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  bsd-user/elfload.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 93fd9e4..2bf57eb 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -351,8 +351,10 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
>  
>      _regs->gpr[1] = infop->start_stack;
>  #if defined(TARGET_PPC64) && !defined(TARGET_ABI32)
> -    entry = ldq_raw(infop->entry) + infop->load_addr;
> -    toc = ldq_raw(infop->entry + 8) + infop->load_addr;
> +    get_user_u64(entry, infop->entry);
> +    entry += infop->load_addr;
> +    get_user_u64(toc, infop->entry + 8);
> +    toc += infop->load_addr;
>      _regs->gpr[2] = toc;
>      infop->entry = entry;
>  #endif
> @@ -365,8 +367,9 @@ static inline void init_thread(struct target_pt_regs *_regs, struct image_info *
>      get_user_ual(_regs->gpr[3], pos);
>      pos += sizeof(abi_ulong);
>      _regs->gpr[4] = pos;
> -    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong))
> -        tmp = ldl(pos);
> +    for (tmp = 1; tmp != 0; pos += sizeof(abi_ulong)) {
> +        get_user_ual(tmp, pos);
> +    }
>      _regs->gpr[5] = pos;
>  }

-- 
Alex Bennée

[Qemu-devel] [PATCH 06/15] linux-user/vm86.c: Use cpu_ldl_data &c rather than plain ldl &c

[Peter Maydell]

Use the cpu_ld*_data and cpu_st*_data family of functions to access
guest memory in vm86.c rather than the very short-named ldl/stl functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/vm86.c | 57 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/linux-user/vm86.c b/linux-user/vm86.c
index 45ef559..22a4eb9 100644
--- a/linux-user/vm86.c
+++ b/linux-user/vm86.c
@@ -45,29 +45,34 @@ static inline int is_revectored(int nr, struct target_revectored_struct *bitmap)
     return (((uint8_t *)bitmap)[nr >> 3] >> (nr & 7)) & 1;
 }
 
-static inline void vm_putw(uint32_t segptr, unsigned int reg16, unsigned int val)
+static inline void vm_putw(CPUX86State *env, uint32_t segptr,
+                           unsigned int reg16, unsigned int val)
 {
-    stw(segptr + (reg16 & 0xffff), val);
+    cpu_stw_data(env, segptr + (reg16 & 0xffff), val);
 }
 
-static inline void vm_putl(uint32_t segptr, unsigned int reg16, unsigned int val)
+static inline void vm_putl(CPUX86State *env, uint32_t segptr,
+                           unsigned int reg16, unsigned int val)
 {
-    stl(segptr + (reg16 & 0xffff), val);
+    cpu_stl_data(env, segptr + (reg16 & 0xffff), val);
 }
 
-static inline unsigned int vm_getb(uint32_t segptr, unsigned int reg16)
+static inline unsigned int vm_getb(CPUX86State *env,
+                                   uint32_t segptr, unsigned int reg16)
 {
-    return ldub(segptr + (reg16 & 0xffff));
+    return cpu_ldub_data(env, segptr + (reg16 & 0xffff));
 }
 
-static inline unsigned int vm_getw(uint32_t segptr, unsigned int reg16)
+static inline unsigned int vm_getw(CPUX86State *env,
+                                   uint32_t segptr, unsigned int reg16)
 {
-    return lduw(segptr + (reg16 & 0xffff));
+    return cpu_lduw_data(env, segptr + (reg16 & 0xffff));
 }
 
-static inline unsigned int vm_getl(uint32_t segptr, unsigned int reg16)
+static inline unsigned int vm_getl(CPUX86State *env,
+                                   uint32_t segptr, unsigned int reg16)
 {
-    return ldl(segptr + (reg16 & 0xffff));
+    return cpu_ldl_data(env, segptr + (reg16 & 0xffff));
 }
 
 void save_v86_state(CPUX86State *env)
@@ -221,7 +226,7 @@ static void do_int(CPUX86State *env, int intno)
                                        &ts->vm86plus.int21_revectored))
         goto cannot_handle;
     int_addr = (intno << 2);
-    segoffs = ldl(int_addr);
+    segoffs = cpu_ldl_data(env, int_addr);
     if ((segoffs >> 16) == TARGET_BIOSSEG)
         goto cannot_handle;
     LOG_VM86("VM86: emulating int 0x%x. CS:IP=%04x:%04x\n",
@@ -229,9 +234,9 @@ static void do_int(CPUX86State *env, int intno)
     /* save old state */
     ssp = env->segs[R_SS].selector << 4;
     sp = env->regs[R_ESP] & 0xffff;
-    vm_putw(ssp, sp - 2, get_vflags(env));
-    vm_putw(ssp, sp - 4, env->segs[R_CS].selector);
-    vm_putw(ssp, sp - 6, env->eip);
+    vm_putw(env, ssp, sp - 2, get_vflags(env));
+    vm_putw(env, ssp, sp - 4, env->segs[R_CS].selector);
+    vm_putw(env, ssp, sp - 6, env->eip);
     ADD16(env->regs[R_ESP], -6);
     /* goto interrupt handler */
     env->eip = segoffs & 0xffff;
@@ -285,7 +290,7 @@ void handle_vm86_fault(CPUX86State *env)
     data32 = 0;
     pref_done = 0;
     do {
-        opcode = vm_getb(csp, ip);
+        opcode = vm_getb(env, csp, ip);
         ADD16(ip, 1);
         switch (opcode) {
         case 0x66:      /* 32-bit data */     data32=1; break;
@@ -306,10 +311,10 @@ void handle_vm86_fault(CPUX86State *env)
     switch(opcode) {
     case 0x9c: /* pushf */
         if (data32) {
-            vm_putl(ssp, sp - 4, get_vflags(env));
+            vm_putl(env, ssp, sp - 4, get_vflags(env));
             ADD16(env->regs[R_ESP], -4);
         } else {
-            vm_putw(ssp, sp - 2, get_vflags(env));
+            vm_putw(env, ssp, sp - 2, get_vflags(env));
             ADD16(env->regs[R_ESP], -2);
         }
         env->eip = ip;
@@ -317,10 +322,10 @@ void handle_vm86_fault(CPUX86State *env)
 
     case 0x9d: /* popf */
         if (data32) {
-            newflags = vm_getl(ssp, sp);
+            newflags = vm_getl(env, ssp, sp);
             ADD16(env->regs[R_ESP], 4);
         } else {
-            newflags = vm_getw(ssp, sp);
+            newflags = vm_getw(env, ssp, sp);
             ADD16(env->regs[R_ESP], 2);
         }
         env->eip = ip;
@@ -335,7 +340,7 @@ void handle_vm86_fault(CPUX86State *env)
         VM86_FAULT_RETURN;
 
     case 0xcd: /* int */
-        intno = vm_getb(csp, ip);
+        intno = vm_getb(env, csp, ip);
         ADD16(ip, 1);
         env->eip = ip;
         if (ts->vm86plus.vm86plus.flags & TARGET_vm86dbg_active) {
@@ -350,14 +355,14 @@ void handle_vm86_fault(CPUX86State *env)
 
     case 0xcf: /* iret */
         if (data32) {
-            newip = vm_getl(ssp, sp) & 0xffff;
-            newcs = vm_getl(ssp, sp + 4) & 0xffff;
-            newflags = vm_getl(ssp, sp + 8);
+            newip = vm_getl(env, ssp, sp) & 0xffff;
+            newcs = vm_getl(env, ssp, sp + 4) & 0xffff;
+            newflags = vm_getl(env, ssp, sp + 8);
             ADD16(env->regs[R_ESP], 12);
         } else {
-            newip = vm_getw(ssp, sp);
-            newcs = vm_getw(ssp, sp + 2);
-            newflags = vm_getw(ssp, sp + 4);
+            newip = vm_getw(env, ssp, sp);
+            newcs = vm_getw(env, ssp, sp + 2);
+            newflags = vm_getw(env, ssp, sp + 4);
             ADD16(env->regs[R_ESP], 6);
         }
         env->eip = newip;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 06/15] linux-user/vm86.c: Use cpu_ldl_data &c rather than plain ldl &c

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Use the cpu_ld*_data and cpu_st*_data family of functions to access
> guest memory in vm86.c rather than the very short-named ldl/stl functions.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  linux-user/vm86.c | 57 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/linux-user/vm86.c b/linux-user/vm86.c
> index 45ef559..22a4eb9 100644
> --- a/linux-user/vm86.c
> +++ b/linux-user/vm86.c
> @@ -45,29 +45,34 @@ static inline int is_revectored(int nr, struct target_revectored_struct *bitmap)
>      return (((uint8_t *)bitmap)[nr >> 3] >> (nr & 7)) & 1;
>  }
>  
> -static inline void vm_putw(uint32_t segptr, unsigned int reg16, unsigned int val)
> +static inline void vm_putw(CPUX86State *env, uint32_t segptr,
> +                           unsigned int reg16, unsigned int val)
>  {
> -    stw(segptr + (reg16 & 0xffff), val);
> +    cpu_stw_data(env, segptr + (reg16 & 0xffff), val);
>  }
>  
> -static inline void vm_putl(uint32_t segptr, unsigned int reg16, unsigned int val)
> +static inline void vm_putl(CPUX86State *env, uint32_t segptr,
> +                           unsigned int reg16, unsigned int val)
>  {
> -    stl(segptr + (reg16 & 0xffff), val);
> +    cpu_stl_data(env, segptr + (reg16 & 0xffff), val);
>  }
>  
> -static inline unsigned int vm_getb(uint32_t segptr, unsigned int reg16)
> +static inline unsigned int vm_getb(CPUX86State *env,
> +                                   uint32_t segptr, unsigned int reg16)
>  {
> -    return ldub(segptr + (reg16 & 0xffff));
> +    return cpu_ldub_data(env, segptr + (reg16 & 0xffff));
>  }
>  
> -static inline unsigned int vm_getw(uint32_t segptr, unsigned int reg16)
> +static inline unsigned int vm_getw(CPUX86State *env,
> +                                   uint32_t segptr, unsigned int reg16)
>  {
> -    return lduw(segptr + (reg16 & 0xffff));
> +    return cpu_lduw_data(env, segptr + (reg16 & 0xffff));
>  }
>  
> -static inline unsigned int vm_getl(uint32_t segptr, unsigned int reg16)
> +static inline unsigned int vm_getl(CPUX86State *env,
> +                                   uint32_t segptr, unsigned int reg16)
>  {
> -    return ldl(segptr + (reg16 & 0xffff));
> +    return cpu_ldl_data(env, segptr + (reg16 & 0xffff));
>  }
>  
>  void save_v86_state(CPUX86State *env)
> @@ -221,7 +226,7 @@ static void do_int(CPUX86State *env, int intno)
>                                         &ts->vm86plus.int21_revectored))
>          goto cannot_handle;
>      int_addr = (intno << 2);
> -    segoffs = ldl(int_addr);
> +    segoffs = cpu_ldl_data(env, int_addr);
>      if ((segoffs >> 16) == TARGET_BIOSSEG)
>          goto cannot_handle;
>      LOG_VM86("VM86: emulating int 0x%x. CS:IP=%04x:%04x\n",
> @@ -229,9 +234,9 @@ static void do_int(CPUX86State *env, int intno)
>      /* save old state */
>      ssp = env->segs[R_SS].selector << 4;
>      sp = env->regs[R_ESP] & 0xffff;
> -    vm_putw(ssp, sp - 2, get_vflags(env));
> -    vm_putw(ssp, sp - 4, env->segs[R_CS].selector);
> -    vm_putw(ssp, sp - 6, env->eip);
> +    vm_putw(env, ssp, sp - 2, get_vflags(env));
> +    vm_putw(env, ssp, sp - 4, env->segs[R_CS].selector);
> +    vm_putw(env, ssp, sp - 6, env->eip);
>      ADD16(env->regs[R_ESP], -6);
>      /* goto interrupt handler */
>      env->eip = segoffs & 0xffff;
> @@ -285,7 +290,7 @@ void handle_vm86_fault(CPUX86State *env)
>      data32 = 0;
>      pref_done = 0;
>      do {
> -        opcode = vm_getb(csp, ip);
> +        opcode = vm_getb(env, csp, ip);
>          ADD16(ip, 1);
>          switch (opcode) {
>          case 0x66:      /* 32-bit data */     data32=1; break;
> @@ -306,10 +311,10 @@ void handle_vm86_fault(CPUX86State *env)
>      switch(opcode) {
>      case 0x9c: /* pushf */
>          if (data32) {
> -            vm_putl(ssp, sp - 4, get_vflags(env));
> +            vm_putl(env, ssp, sp - 4, get_vflags(env));
>              ADD16(env->regs[R_ESP], -4);
>          } else {
> -            vm_putw(ssp, sp - 2, get_vflags(env));
> +            vm_putw(env, ssp, sp - 2, get_vflags(env));
>              ADD16(env->regs[R_ESP], -2);
>          }
>          env->eip = ip;
> @@ -317,10 +322,10 @@ void handle_vm86_fault(CPUX86State *env)
>  
>      case 0x9d: /* popf */
>          if (data32) {
> -            newflags = vm_getl(ssp, sp);
> +            newflags = vm_getl(env, ssp, sp);
>              ADD16(env->regs[R_ESP], 4);
>          } else {
> -            newflags = vm_getw(ssp, sp);
> +            newflags = vm_getw(env, ssp, sp);
>              ADD16(env->regs[R_ESP], 2);
>          }
>          env->eip = ip;
> @@ -335,7 +340,7 @@ void handle_vm86_fault(CPUX86State *env)
>          VM86_FAULT_RETURN;
>  
>      case 0xcd: /* int */
> -        intno = vm_getb(csp, ip);
> +        intno = vm_getb(env, csp, ip);
>          ADD16(ip, 1);
>          env->eip = ip;
>          if (ts->vm86plus.vm86plus.flags & TARGET_vm86dbg_active) {
> @@ -350,14 +355,14 @@ void handle_vm86_fault(CPUX86State *env)
>  
>      case 0xcf: /* iret */
>          if (data32) {
> -            newip = vm_getl(ssp, sp) & 0xffff;
> -            newcs = vm_getl(ssp, sp + 4) & 0xffff;
> -            newflags = vm_getl(ssp, sp + 8);
> +            newip = vm_getl(env, ssp, sp) & 0xffff;
> +            newcs = vm_getl(env, ssp, sp + 4) & 0xffff;
> +            newflags = vm_getl(env, ssp, sp + 8);
>              ADD16(env->regs[R_ESP], 12);
>          } else {
> -            newip = vm_getw(ssp, sp);
> -            newcs = vm_getw(ssp, sp + 2);
> -            newflags = vm_getw(ssp, sp + 4);
> +            newip = vm_getw(env, ssp, sp);
> +            newcs = vm_getw(env, ssp, sp + 2);
> +            newflags = vm_getw(env, ssp, sp + 4);
>              ADD16(env->regs[R_ESP], 6);
>          }
>          env->eip = newip;

-- 
Alex Bennée

[Qemu-devel] [PATCH 07/15] linux-user/main.c (m68k): Use get_user_u16 rather than lduw in cpu_loop

[Peter Maydell]

In the m68k cpu_loop() use get_user_u16 to read the immediate for
the simcall rahter than lduw, to bring it into line with how other
archs do it and to remove another user of the ldl family of functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 67b0231..65b5a36 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -2972,7 +2972,7 @@ void cpu_loop(CPUM68KState *env)
             {
                 if (ts->sim_syscalls) {
                     uint16_t nr;
-                    nr = lduw(env->pc + 2);
+                    get_user_u16(nr, env->pc + 2);
                     env->pc += 4;
                     do_m68k_simcall(env, nr);
                 } else {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 07/15] linux-user/main.c (m68k): Use get_user_u16 rather than lduw in cpu_loop

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> In the m68k cpu_loop() use get_user_u16 to read the immediate for
> the simcall rahter than lduw, to bring it into line with how other
> archs do it and to remove another user of the ldl family of functions.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

After getting lost tracing the many macro definitions from lduw ;-)

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  linux-user/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 67b0231..65b5a36 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -2972,7 +2972,7 @@ void cpu_loop(CPUM68KState *env)
>              {
>                  if (ts->sim_syscalls) {
>                      uint16_t nr;
> -                    nr = lduw(env->pc + 2);
> +                    get_user_u16(nr, env->pc + 2);
>                      env->pc += 4;
>                      do_m68k_simcall(env, nr);
>                  } else {

-- 
Alex Bennée

[Qemu-devel] [PATCH 08/15] target-mips: Don't use _raw load/store accessors

[Peter Maydell]

Use cpu_*_data instead of the direct *_raw load/store accessors.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-mips/op_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index d619ba4..ea7d95f 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -74,7 +74,7 @@ void helper_raise_exception(CPUMIPSState *env, uint32_t exception)
 static inline type do_##name(CPUMIPSState *env, target_ulong addr,      \
                              int mem_idx)                               \
 {                                                                       \
-    return (type) insn##_raw(addr);                                     \
+    return (type) cpu_##insn##_data(env, addr);                         \
 }
 #else
 #define HELPER_LD(name, insn, type)                                     \
@@ -101,7 +101,7 @@ HELPER_LD(ld, ldq, int64_t)
 static inline void do_##name(CPUMIPSState *env, target_ulong addr,      \
                              type val, int mem_idx)                     \
 {                                                                       \
-    insn##_raw(addr, val);                                              \
+    cpu_##insn##_data(env, addr, val);                                  \
 }
 #else
 #define HELPER_ST(name, insn, type)                                     \
-- 
1.9.1

Re: [Qemu-devel] [PATCH 08/15] target-mips: Don't use _raw load/store accessors

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Use cpu_*_data instead of the direct *_raw load/store accessors.

I take it this additional level of (macro) redirection is because at
some point there will be a difference between the various cpu accessors?

>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  target-mips/op_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index d619ba4..ea7d95f 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -74,7 +74,7 @@ void helper_raise_exception(CPUMIPSState *env, uint32_t exception)
>  static inline type do_##name(CPUMIPSState *env, target_ulong addr,      \
>                               int mem_idx)                               \
>  {                                                                       \
> -    return (type) insn##_raw(addr);                                     \
> +    return (type) cpu_##insn##_data(env, addr);                         \
>  }
>  #else
>  #define HELPER_LD(name, insn, type)                                     \
> @@ -101,7 +101,7 @@ HELPER_LD(ld, ldq, int64_t)
>  static inline void do_##name(CPUMIPSState *env, target_ulong addr,      \
>                               type val, int mem_idx)                     \
>  {                                                                       \
> -    insn##_raw(addr, val);                                              \
> +    cpu_##insn##_data(env, addr, val);                                  \
>  }
>  #else
>  #define HELPER_ST(name, insn, type)                                     \

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 08/15] target-mips: Don't use _raw load/store accessors

[Peter Maydell]

On 19 January 2015 at 09:05, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Use cpu_*_data instead of the direct *_raw load/store accessors.
>
> I take it this additional level of (macro) redirection is because at
> some point there will be a difference between the various cpu accessors?

Which additional level of redirection? The point of this patch is
to remove a level of redirection (the _raw accessors), and at
the end of the series cpu_*_data aren't macros.

-- PMM

Re: [Qemu-devel] [PATCH 08/15] target-mips: Don't use _raw load/store accessors

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 19 January 2015 at 09:05, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> Use cpu_*_data instead of the direct *_raw load/store accessors.
>>
>> I take it this additional level of (macro) redirection is because at
>> some point there will be a difference between the various cpu accessors?
>
> Which additional level of redirection? The point of this patch is
> to remove a level of redirection (the _raw accessors), and at
> the end of the series cpu_*_data aren't macros.

Ahh I see. I haven't got that far yet as I'm still working through the
glue() magic in the next few patches ;-)


>
> -- PMM

-- 
Alex Bennée

[Qemu-devel] [PATCH 09/15] cpu_ldst.h: Drop unused ld/st*_kernel defines

[Peter Maydell]

The ld*_kernel and st*_kernel defines are not used anywhere;
delete them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu_ldst.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 4700831..64d9087 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -126,21 +126,6 @@
 #define cpu_stl_kernel(env, addr, data) stl_raw(addr, data)
 #define cpu_stq_kernel(env, addr, data) stq_raw(addr, data)
 
-#define ldub_kernel(p) ldub_raw(p)
-#define ldsb_kernel(p) ldsb_raw(p)
-#define lduw_kernel(p) lduw_raw(p)
-#define ldsw_kernel(p) ldsw_raw(p)
-#define ldl_kernel(p) ldl_raw(p)
-#define ldq_kernel(p) ldq_raw(p)
-#define ldfl_kernel(p) ldfl_raw(p)
-#define ldfq_kernel(p) ldfq_raw(p)
-#define stb_kernel(p, v) stb_raw(p, v)
-#define stw_kernel(p, v) stw_raw(p, v)
-#define stl_kernel(p, v) stl_raw(p, v)
-#define stq_kernel(p, v) stq_raw(p, v)
-#define stfl_kernel(p, v) stfl_raw(p, v)
-#define stfq_kernel(p, vt) stfq_raw(p, v)
-
 #define cpu_ldub_data(env, addr) ldub_raw(addr)
 #define cpu_lduw_data(env, addr) lduw_raw(addr)
 #define cpu_ldl_data(env, addr) ldl_raw(addr)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 09/15] cpu_ldst.h: Drop unused ld/st*_kernel defines

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The ld*_kernel and st*_kernel defines are not used anywhere;
> delete them.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  include/exec/cpu_ldst.h | 15 ---------------
>  1 file changed, 15 deletions(-)
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index 4700831..64d9087 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -126,21 +126,6 @@
>  #define cpu_stl_kernel(env, addr, data) stl_raw(addr, data)
>  #define cpu_stq_kernel(env, addr, data) stq_raw(addr, data)
>  
> -#define ldub_kernel(p) ldub_raw(p)
> -#define ldsb_kernel(p) ldsb_raw(p)
> -#define lduw_kernel(p) lduw_raw(p)
> -#define ldsw_kernel(p) ldsw_raw(p)
> -#define ldl_kernel(p) ldl_raw(p)
> -#define ldq_kernel(p) ldq_raw(p)
> -#define ldfl_kernel(p) ldfl_raw(p)
> -#define ldfq_kernel(p) ldfq_raw(p)
> -#define stb_kernel(p, v) stb_raw(p, v)
> -#define stw_kernel(p, v) stw_raw(p, v)
> -#define stl_kernel(p, v) stl_raw(p, v)
> -#define stq_kernel(p, v) stq_raw(p, v)
> -#define stfl_kernel(p, v) stfl_raw(p, v)
> -#define stfq_kernel(p, vt) stfq_raw(p, v)
> -
>  #define cpu_ldub_data(env, addr) ldub_raw(addr)
>  #define cpu_lduw_data(env, addr) lduw_raw(addr)
>  #define cpu_ldl_data(env, addr) ldl_raw(addr)

-- 
Alex Bennée

[Qemu-devel] [PATCH 10/15] cpu_ldst.h: Remove unused very short ld*/st* defines

[Peter Maydell]

The very short ld*/st* defines are now not used anywhere; delete them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu_ldst.h | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 64d9087..2dc4775 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -82,20 +82,6 @@
 #if defined(CONFIG_USER_ONLY)
 
 /* if user mode, no other memory access functions */
-#define ldub(p) ldub_raw(p)
-#define ldsb(p) ldsb_raw(p)
-#define lduw(p) lduw_raw(p)
-#define ldsw(p) ldsw_raw(p)
-#define ldl(p) ldl_raw(p)
-#define ldq(p) ldq_raw(p)
-#define ldfl(p) ldfl_raw(p)
-#define ldfq(p) ldfq_raw(p)
-#define stb(p, v) stb_raw(p, v)
-#define stw(p, v) stw_raw(p, v)
-#define stl(p, v) stl_raw(p, v)
-#define stq(p, v) stq_raw(p, v)
-#define stfl(p, v) stfl_raw(p, v)
-#define stfq(p, v) stfq_raw(p, v)
 
 #define cpu_ldub_code(env1, p) ldub_raw(p)
 #define cpu_ldsb_code(env1, p) ldsb_raw(p)
@@ -287,18 +273,6 @@ uint64_t helper_ldq_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
 #undef CPU_MMU_INDEX
 #undef MEMSUFFIX
 
-#define ldub(p) ldub_data(p)
-#define ldsb(p) ldsb_data(p)
-#define lduw(p) lduw_data(p)
-#define ldsw(p) ldsw_data(p)
-#define ldl(p) ldl_data(p)
-#define ldq(p) ldq_data(p)
-
-#define stb(p, v) stb_data(p, v)
-#define stw(p, v) stw_data(p, v)
-#define stl(p, v) stl_data(p, v)
-#define stq(p, v) stq_data(p, v)
-
 #define CPU_MMU_INDEX (cpu_mmu_index(env))
 #define MEMSUFFIX _code
 #define SOFTMMU_CODE_ACCESS
-- 
1.9.1

Re: [Qemu-devel] [PATCH 10/15] cpu_ldst.h: Remove unused very short ld*/st* defines

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The very short ld*/st* defines are now not used anywhere; delete them.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


> ---
>  include/exec/cpu_ldst.h | 26 --------------------------
>  1 file changed, 26 deletions(-)
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index 64d9087..2dc4775 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -82,20 +82,6 @@
>  #if defined(CONFIG_USER_ONLY)
>  
>  /* if user mode, no other memory access functions */
> -#define ldub(p) ldub_raw(p)
> -#define ldsb(p) ldsb_raw(p)
> -#define lduw(p) lduw_raw(p)
> -#define ldsw(p) ldsw_raw(p)
> -#define ldl(p) ldl_raw(p)
> -#define ldq(p) ldq_raw(p)
> -#define ldfl(p) ldfl_raw(p)
> -#define ldfq(p) ldfq_raw(p)
> -#define stb(p, v) stb_raw(p, v)
> -#define stw(p, v) stw_raw(p, v)
> -#define stl(p, v) stl_raw(p, v)
> -#define stq(p, v) stq_raw(p, v)
> -#define stfl(p, v) stfl_raw(p, v)
> -#define stfq(p, v) stfq_raw(p, v)
>  
>  #define cpu_ldub_code(env1, p) ldub_raw(p)
>  #define cpu_ldsb_code(env1, p) ldsb_raw(p)
> @@ -287,18 +273,6 @@ uint64_t helper_ldq_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
>  #undef CPU_MMU_INDEX
>  #undef MEMSUFFIX
>  
> -#define ldub(p) ldub_data(p)
> -#define ldsb(p) ldsb_data(p)
> -#define lduw(p) lduw_data(p)
> -#define ldsw(p) ldsw_data(p)
> -#define ldl(p) ldl_data(p)
> -#define ldq(p) ldq_data(p)
> -
> -#define stb(p, v) stb_data(p, v)
> -#define stw(p, v) stw_data(p, v)
> -#define stl(p, v) stl_data(p, v)
> -#define stq(p, v) stq_data(p, v)
> -
>  #define CPU_MMU_INDEX (cpu_mmu_index(env))
>  #define MEMSUFFIX _code
>  #define SOFTMMU_CODE_ACCESS

-- 
Alex Bennée

[Qemu-devel] [PATCH 11/15] cpu_ldst.h: Use inline functions for usermode cpu_ld/st accessors

[Peter Maydell]

Use inline functions rather than macros for cpu_ld/st accessors
for the *-user configurations, as we already do for softmmu.
This has a two advantages:
 * we can actually typecheck our arguments
 * we don't need to leak the _raw macros everywhere

Since the _kernel functions were only used by target-i386/seg_helper.c,
put the definitions for them in that file too. (It already has the
similar template include code to define them for the softmmu case,
so it makes sense to have it deal with defining them for user-only.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu_ldst.h                   | 70 ++++++++++++--------------
 include/exec/cpu_ldst_useronly_template.h | 81 +++++++++++++++++++++++++++++++
 target-i386/seg_helper.c                  | 16 +++++-
 3 files changed, 127 insertions(+), 40 deletions(-)
 create mode 100644 include/exec/cpu_ldst_useronly_template.h

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 2dc4775..bef2e5f 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -78,47 +78,39 @@
 #define stfl_raw(p, v) stfl_p(saddr((p)), v)
 #define stfq_raw(p, v) stfq_p(saddr((p)), v)
 
-
 #if defined(CONFIG_USER_ONLY)
 
-/* if user mode, no other memory access functions */
-
-#define cpu_ldub_code(env1, p) ldub_raw(p)
-#define cpu_ldsb_code(env1, p) ldsb_raw(p)
-#define cpu_lduw_code(env1, p) lduw_raw(p)
-#define cpu_ldsw_code(env1, p) ldsw_raw(p)
-#define cpu_ldl_code(env1, p) ldl_raw(p)
-#define cpu_ldq_code(env1, p) ldq_raw(p)
-
-#define cpu_ldub_data(env, addr) ldub_raw(addr)
-#define cpu_lduw_data(env, addr) lduw_raw(addr)
-#define cpu_ldsw_data(env, addr) ldsw_raw(addr)
-#define cpu_ldl_data(env, addr) ldl_raw(addr)
-#define cpu_ldq_data(env, addr) ldq_raw(addr)
-
-#define cpu_stb_data(env, addr, data) stb_raw(addr, data)
-#define cpu_stw_data(env, addr, data) stw_raw(addr, data)
-#define cpu_stl_data(env, addr, data) stl_raw(addr, data)
-#define cpu_stq_data(env, addr, data) stq_raw(addr, data)
-
-#define cpu_ldub_kernel(env, addr) ldub_raw(addr)
-#define cpu_lduw_kernel(env, addr) lduw_raw(addr)
-#define cpu_ldsw_kernel(env, addr) ldsw_raw(addr)
-#define cpu_ldl_kernel(env, addr) ldl_raw(addr)
-#define cpu_ldq_kernel(env, addr) ldq_raw(addr)
-
-#define cpu_stb_kernel(env, addr, data) stb_raw(addr, data)
-#define cpu_stw_kernel(env, addr, data) stw_raw(addr, data)
-#define cpu_stl_kernel(env, addr, data) stl_raw(addr, data)
-#define cpu_stq_kernel(env, addr, data) stq_raw(addr, data)
-
-#define cpu_ldub_data(env, addr) ldub_raw(addr)
-#define cpu_lduw_data(env, addr) lduw_raw(addr)
-#define cpu_ldl_data(env, addr) ldl_raw(addr)
-
-#define cpu_stb_data(env, addr, data) stb_raw(addr, data)
-#define cpu_stw_data(env, addr, data) stw_raw(addr, data)
-#define cpu_stl_data(env, addr, data) stl_raw(addr, data)
+/* In user-only mode we provide only the _code and _data accessors. */
+
+#define MEMSUFFIX _data
+#define DATA_SIZE 1
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 2
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 4
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 8
+#include "exec/cpu_ldst_useronly_template.h"
+#undef MEMSUFFIX
+
+#define MEMSUFFIX _code
+#define CODE_ACCESS
+#define DATA_SIZE 1
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 2
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 4
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 8
+#include "exec/cpu_ldst_useronly_template.h"
+#undef MEMSUFFIX
+#undef CODE_ACCESS
 
 #else
 
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
new file mode 100644
index 0000000..b3b865f
--- /dev/null
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -0,0 +1,81 @@
+/*
+ *  User-only accessor function support
+ *
+ * Generate inline load/store functions for one data size.
+ *
+ * Generate a store function as well as signed and unsigned loads.
+ *
+ * Not used directly but included from cpu_ldst.h.
+ *
+ *  Copyright (c) 2015 Linaro Limited
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+#if DATA_SIZE == 8
+#define SUFFIX q
+#define USUFFIX q
+#define DATA_TYPE uint64_t
+#elif DATA_SIZE == 4
+#define SUFFIX l
+#define USUFFIX l
+#define DATA_TYPE uint32_t
+#elif DATA_SIZE == 2
+#define SUFFIX w
+#define USUFFIX uw
+#define DATA_TYPE uint16_t
+#define DATA_STYPE int16_t
+#elif DATA_SIZE == 1
+#define SUFFIX b
+#define USUFFIX ub
+#define DATA_TYPE uint8_t
+#define DATA_STYPE int8_t
+#else
+#error unsupported data size
+#endif
+
+#if DATA_SIZE == 8
+#define RES_TYPE uint64_t
+#else
+#define RES_TYPE uint32_t
+#endif
+
+static inline RES_TYPE
+glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+{
+    return glue(glue(ld, USUFFIX), _p)(g2h(ptr));
+}
+
+#if DATA_SIZE <= 2
+static inline int
+glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+{
+    return glue(glue(lds, SUFFIX), _p)(g2h(ptr));
+}
+#endif
+
+#ifndef CODE_ACCESS
+static inline void
+glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
+                                      RES_TYPE v)
+{
+    glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
+}
+#endif
+
+#undef RES_TYPE
+#undef DATA_TYPE
+#undef DATA_STYPE
+#undef SUFFIX
+#undef USUFFIX
+#undef DATA_SIZE
diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index c98eeb4..fa374d0 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -34,7 +34,21 @@
 # define LOG_PCALL_STATE(cpu) do { } while (0)
 #endif
 
-#ifndef CONFIG_USER_ONLY
+#ifdef CONFIG_USER_ONLY
+#define MEMSUFFIX _kernel
+#define DATA_SIZE 1
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 2
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 4
+#include "exec/cpu_ldst_useronly_template.h"
+
+#define DATA_SIZE 8
+#include "exec/cpu_ldst_useronly_template.h"
+#undef MEMSUFFIX
+#else
 #define CPU_MMU_INDEX (cpu_mmu_index_kernel(env))
 #define MEMSUFFIX _kernel
 #define DATA_SIZE 1
-- 
1.9.1

[Qemu-devel] [PATCH 12/15] cpu_ldst_template.h: Use ld*_p directly rather than via ld*_raw macros

[Peter Maydell]

The ld*_raw and st*_raw macros are now only used within the code
produced by cpu_ldst_template.h, and only in three places.
Expand these out to just call the ld_p and st_p functions directly.

Note that in all the callsites the address argument is a uintptr_t,
so we can drop that part of the double-cast used in the saddr() and
laddr() macros.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu_ldst_template.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 006093a..3b53605 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -79,7 +79,7 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
         res = glue(glue(helper_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx);
     } else {
         uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
-        res = glue(glue(ld, USUFFIX), _raw)(hostaddr);
+        res = glue(glue(ld, USUFFIX), _p)((uint8_t *)hostaddr);
     }
     return res;
 }
@@ -101,7 +101,7 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
                                MMUSUFFIX)(env, addr, mmu_idx);
     } else {
         uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
-        res = glue(glue(lds, SUFFIX), _raw)(hostaddr);
+        res = glue(glue(lds, SUFFIX), _p)((uint8_t *)hostaddr);
     }
     return res;
 }
@@ -127,7 +127,7 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
         glue(glue(helper_st, SUFFIX), MMUSUFFIX)(env, addr, v, mmu_idx);
     } else {
         uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
-        glue(glue(st, SUFFIX), _raw)(hostaddr, v);
+        glue(glue(st, SUFFIX), _p)((uint8_t *)hostaddr, v);
     }
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 13/15] cpu_ldst.h: Drop unused _raw macros, saddr() and laddr()

[Peter Maydell]

The _raw macros and their helpers saddr() and laddr() are now
totally unused -- delete them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu_ldst.h | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index bef2e5f..16f4e30 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -53,31 +53,8 @@
     h2g_nocheck(x); \
 })
 
-#define saddr(x) g2h(x)
-#define laddr(x) g2h(x)
-
-#else /* !CONFIG_USER_ONLY */
-/* NOTE: we use double casts if pointers and target_ulong have
-   different sizes */
-#define saddr(x) (uint8_t *)(intptr_t)(x)
-#define laddr(x) (uint8_t *)(intptr_t)(x)
 #endif
 
-#define ldub_raw(p) ldub_p(laddr((p)))
-#define ldsb_raw(p) ldsb_p(laddr((p)))
-#define lduw_raw(p) lduw_p(laddr((p)))
-#define ldsw_raw(p) ldsw_p(laddr((p)))
-#define ldl_raw(p) ldl_p(laddr((p)))
-#define ldq_raw(p) ldq_p(laddr((p)))
-#define ldfl_raw(p) ldfl_p(laddr((p)))
-#define ldfq_raw(p) ldfq_p(laddr((p)))
-#define stb_raw(p, v) stb_p(saddr((p)), v)
-#define stw_raw(p, v) stw_p(saddr((p)), v)
-#define stl_raw(p, v) stl_p(saddr((p)), v)
-#define stq_raw(p, v) stq_p(saddr((p)), v)
-#define stfl_raw(p, v) stfl_p(saddr((p)), v)
-#define stfq_raw(p, v) stfq_p(saddr((p)), v)
-
 #if defined(CONFIG_USER_ONLY)
 
 /* In user-only mode we provide only the _code and _data accessors. */
-- 
1.9.1

[Qemu-devel] [PATCH 14/15] cpu_ldst_template.h: Drop unused cpu_ldfq/stfq/ldfl/stfl accessors

[Peter Maydell]

The cpu_ldfq/stfq/ldfl/stfl accessors for loading and storing
float32 and float64 are completely unused, so delete them.
(The union they use for converting from the float32/float64
type to uint32_t or uint64_t is the wrong way to do it anyway:
they should be using make_float* and float*_val.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu_ldst_template.h | 54 +---------------------------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 3b53605..95ab750 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -4,9 +4,7 @@
  * Generate inline load/store functions for one MMU mode and data
  * size.
  *
- * Generate a store function as well as signed and unsigned loads. For
- * 32 and 64 bit cases, also generate floating point functions with
- * the same size.
+ * Generate a store function as well as signed and unsigned loads.
  *
  * Not used directly but included from cpu_ldst.h.
  *
@@ -131,56 +129,6 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
     }
 }
 
-
-
-#if DATA_SIZE == 8
-static inline float64 glue(cpu_ldfq, MEMSUFFIX)(CPUArchState *env,
-                                                target_ulong ptr)
-{
-    union {
-        float64 d;
-        uint64_t i;
-    } u;
-    u.i = glue(cpu_ldq, MEMSUFFIX)(env, ptr);
-    return u.d;
-}
-
-static inline void glue(cpu_stfq, MEMSUFFIX)(CPUArchState *env,
-                                             target_ulong ptr, float64 v)
-{
-    union {
-        float64 d;
-        uint64_t i;
-    } u;
-    u.d = v;
-    glue(cpu_stq, MEMSUFFIX)(env, ptr, u.i);
-}
-#endif /* DATA_SIZE == 8 */
-
-#if DATA_SIZE == 4
-static inline float32 glue(cpu_ldfl, MEMSUFFIX)(CPUArchState *env,
-                                                target_ulong ptr)
-{
-    union {
-        float32 f;
-        uint32_t i;
-    } u;
-    u.i = glue(cpu_ldl, MEMSUFFIX)(env, ptr);
-    return u.f;
-}
-
-static inline void glue(cpu_stfl, MEMSUFFIX)(CPUArchState *env,
-                                             target_ulong ptr, float32 v)
-{
-    union {
-        float32 f;
-        uint32_t i;
-    } u;
-    u.f = v;
-    glue(cpu_stl, MEMSUFFIX)(env, ptr, u.i);
-}
-#endif /* DATA_SIZE == 4 */
-
 #endif /* !SOFTMMU_CODE_ACCESS */
 
 #undef RES_TYPE
-- 
1.9.1

[Qemu-devel] [PATCH 15/15] cpu_ldst.h, cpu-all.h, bswap.h: Update documentation on ld/st accessors

[Peter Maydell]

Add documentation of what the cpu_*_* accessors look like.
Correct some minor errors in the existing documentation of the
direct _p accessor family. Remove the near-duplicate comment
on the _p accessors from cpu-all.h and replace it with a reference
to the comment in bswap.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu-all.h  | 38 ++------------------------------------
 include/exec/cpu_ldst.h | 21 ++++++++++++++++++++-
 include/qemu/bswap.h    | 11 ++++++++++-
 3 files changed, 32 insertions(+), 38 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 5fdd2fe..2c48286 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -115,43 +115,9 @@ static inline void tswap64s(uint64_t *s)
 #define bswaptls(s) bswap64s(s)
 #endif
 
-/* CPU memory access without any memory or io remapping */
-
-/*
- * the generic syntax for the memory accesses is:
- *
- * load: ld{type}{sign}{size}{endian}_{access_type}(ptr)
- *
- * store: st{type}{size}{endian}_{access_type}(ptr, val)
- *
- * type is:
- * (empty): integer access
- *   f    : float access
- *
- * sign is:
- * (empty): for floats or 32 bit size
- *   u    : unsigned
- *   s    : signed
- *
- * size is:
- *   b: 8 bits
- *   w: 16 bits
- *   l: 32 bits
- *   q: 64 bits
- *
- * endian is:
- * (empty): target cpu endianness or 8 bit access
- *   r    : reversed target cpu endianness (not implemented yet)
- *   be   : big endian (not implemented yet)
- *   le   : little endian (not implemented yet)
- *
- * access_type is:
- *   raw    : host memory access
- *   user   : user mode access using soft MMU
- *   kernel : kernel mode access using soft MMU
+/* Target-endianness CPU memory access functions. These fit into the
+ * {ld,st}{type}{sign}{size}{endian}_p naming scheme described in bswap.h.
  */
-
-/* target-endianness CPU memory access functions */
 #if defined(TARGET_WORDS_BIGENDIAN)
 #define lduw_p(p) lduw_be_p(p)
 #define ldsw_p(p) ldsw_be_p(p)
diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 16f4e30..d98ff17 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -23,7 +23,26 @@
  *
  * Used by target op helpers.
  *
- * MMU mode suffixes are defined in target cpu.h.
+ * The syntax for the accessors is:
+ *
+ * load: cpu_ld{sign}{size}_{mmusuffix}(env, ptr)
+ *
+ * store: cpu_st{sign}{size}_{mmusuffix}(env, ptr, val)
+ *
+ * sign is:
+ * (empty): for 32 and 64 bit sizes
+ *   u    : unsigned
+ *   s    : signed
+ *
+ * size is:
+ *   b: 8 bits
+ *   w: 16 bits
+ *   l: 32 bits
+ *   q: 64 bits
+ *
+ * mmusuffix is one of the generic suffixes "data" or "code", or
+ * (for softmmu configs)  a target-specific MMU mode suffix as defined
+ * in target cpu.h.
  */
 #ifndef CPU_LDST_H
 #define CPU_LDST_H
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 78c1ced..07d88de 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -204,7 +204,7 @@ typedef union {
  *   f    : float access
  *
  * sign is:
- * (empty): for floats or 32 bit size
+ * (empty): for 32 or 64 bit sizes (including floats and doubles)
  *   u    : unsigned
  *   s    : signed
  *
@@ -218,7 +218,16 @@ typedef union {
  *   he   : host endian
  *   be   : big endian
  *   le   : little endian
+ *   te   : target endian
  * (except for byte accesses, which have no endian infix).
+ *
+ * The target endian accessors are obviously only available to source
+ * files which are built per-target; they are defined in cpu-all.h.
+ *
+ * In all cases these functions take a host pointer.
+ * For accessors that take a guest address rather than a
+ * host address, see the cpu_{ld,st}_* accessors defined in
+ * cpu_ldst.h.
  */
 
 static inline int ldub_p(const void *ptr)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 00/15] Clean up cpu-ldst ld/st memory accessors

[Lluís Vilanova]

Peter Maydell writes:

> I was looking at our confusing mess of memory accessor functions,
> and I realised that partly it was confusing because we have a
> bunch of unnecessary junk lurking in there :-) This series
> attempts to clean things up by removing things we weren't using
> at all or were only using by mistake in a few places:

>  * ldul_*: not used
>  * ld* (ldl, etc): hardly used
>  * ld*_kernel: not used
>  * ld*_raw: hardly used
>  * cpu_{ld,st}{fq,fl}: not used

> The dull parts of this series are removing the unused macros
> and fixing uses of the hardly-used macros so those can be
> deleted too. This series also switches to using inline functions
> rather than macros for the user-only cpu_ld/st* accessors, bringing
> them into line with the softmmu configs. This has the nice
> side effect of letting us get rid of the _raw accessor macros too.
> I've also thrown in a commit which cleans up the doc comments.

I haven't reviewed the patches, but that's a much appreciated cleanup! I was
also trying to make sense of all the variants while implementing guest memory
access tracing (let's see if I can find some time to polish and post the
series).


Thanks!

Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH 00/15] Clean up cpu-ldst ld/st memory accessors

[Peter Maydell]

On 15 January 2015 at 15:32, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> I haven't reviewed the patches, but that's a much appreciated cleanup! I was
> also trying to make sense of all the variants while implementing guest memory
> access tracing

I drew the following terrible ASCII art diagram of the remaining
memory access functions...
(pastebin link for those without fixed-width mail clients:
http://pastebin.ubuntu.com/9757219/ )

If anybody feels they can rearrange it into something less
confusing do feel free :-)

+--------------------------------------------------------------------------------------------------------------+
|
                                        |
|  +-------------------------------------------------------------------------------------------------+
        |
|  |
                              |         |
|  |
                              |         |
|  |    +--------------------------------------------------------------------------------------+
    |         |
|  |    |
                        |     |         |
|  |    |
                        v     v         |
|  |    |                                       MemoryRegion callbacks
           ld/st_*_p, memcpy, etc  ! <--+
|  |    |                                           ^
           on host memory
|  |    |                                           |
           ^     ^            ^
|  |    |                                           |
           |     |            |
|  |    |                                           |
           |     |            |
|  |    |                                           |
           |     |            |
|  |    |                                           |
           +     |            |
|  |    |                                           +
+ld/st_*_phys !   |            |
|  |    |                +-----------------> io_mem_read,
<-----------+                 |            |
|  |    |                +                   io_mem_write !
                 |            |
|  |    | io_read*, io_write*                             <---+
                 |            |
|  |    |        ^                                            |
                 |            |
|  |    |        |                                            |
                 +            |
|  |    |        |                                            +
address_space_{rw,read,write} !      |
|  |    |        +                                                 ^
   ^                          |
|  |    +helper_ld_* <------+                                      |
   |                          |
|  |            ^           |                                      |
   |                          |
|  |            |           |                                      |
   |                          +
|  |            |           |         cpu_physical_memory_rw &c+---+
   |              cpu_physical_memory_write_rom !
|  |            |           |                     ^         !
   |                      ^
|  |            |           |                     |
   +                      |
|  |            +           |                     |
dma_memory_rw &c !            |
|  +----+ cpu_ld/st_* !     |                     |
                          |
|                           |                     |
                          |
|                           +                     |
                          |
+-------------+ generated TCG code                |
                          +
                                                  +-----------------+
cpu_physical_memory_rw_debug !






             ! marks APIs intended for use by other parts of QEMU


-- PMM

Re: [Qemu-devel] [PATCH 00/15] Clean up cpu-ldst ld/st memory accessors

[Lluís Vilanova]

Peter Maydell writes:

> On 15 January 2015 at 15:32, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> I haven't reviewed the patches, but that's a much appreciated cleanup! I was
>> also trying to make sense of all the variants while implementing guest memory
>> access tracing

> I drew the following terrible ASCII art diagram of the remaining
> memory access functions...
> (pastebin link for those without fixed-width mail clients:
> http://pastebin.ubuntu.com/9757219/ )

> If anybody feels they can rearrange it into something less
> confusing do feel free :-)

Technically, I did not rearrange it :)

  http://pastebin.ubuntu.com/9757456/

PS: some info from the original is missing


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH 00/15] Clean up cpu-ldst ld/st memory accessors

[Paolo Bonzini]

On 15/01/2015 20:10, Lluís Vilanova wrote:
> Peter Maydell writes:
> 
>> On 15 January 2015 at 15:32, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>>> I haven't reviewed the patches, but that's a much appreciated cleanup! I was
>>> also trying to make sense of all the variants while implementing guest memory
>>> access tracing
> 
>> I drew the following terrible ASCII art diagram of the remaining
>> memory access functions...
>> (pastebin link for those without fixed-width mail clients:
>> http://pastebin.ubuntu.com/9757219/ )
> 
>> If anybody feels they can rearrange it into something less
>> confusing do feel free :-)
> 
> Technically, I did not rearrange it :)
> 
>   http://pastebin.ubuntu.com/9757456/
> 
> PS: some info from the original is missing

Removing even more info, ld/st_*_phys is just an optimized version of
address_space_rw/read/write, so we can merge them and get to something
readable...

Paolo


# dot -T pdf -o qemu-mem.pdf qemu-mem.dot
digraph QEMU {
  mr [label="MemoryRegion callbacks"];
  host_mem   [label="ld/st_*_p, memcpy, etc.\n(host memory)"];
  io_mem_mem [label="io_mem_read/write"];
  io_mem     [label="io_read/write*"];
  as_mem     [label="address_space_rw/read/write\nld/st_*_phys"];
  helper_mem [label="helper_ld_*"];
  cpu_phys_mem [label="cpu_physical_memory_rw"];
  cpu_phys_rom [label="cpu_physical_memory_write_rom"];
  dma_mem      [label="dma_memory_rw"];
  cpu_mem      [label="cpu_ld/st_*"];
  tcg          [label="Generated TCG code"];
  cpu_phys_mem_debug [label="cpu_physical_memory_rw_debug"];

  io_mem_mem -> mr;

  io_mem -> io_mem_mem;

  as_mem -> io_mem_mem;
  as_mem -> host_mem;

  helper_mem -> io_mem;
  helper_mem -> host_mem;

  cpu_phys_mem -> as_mem;
  cpu_phys_rom -> host_mem;

  dma_mem -> as_mem;

  cpu_mem -> helper_mem;
  cpu_mem -> host_mem;

  tcg -> helper_mem;
  tcg -> host_mem;

  cpu_phys_mem_debug -> cpu_phys_mem;
  cpu_phys_mem_debug -> cpu_phys_rom;
}

Re: [Qemu-devel] [PATCH 00/15] Clean up cpu-ldst ld/st memory accessors

[Richard Henderson]

On 01/15/2015 07:01 AM, Peter Maydell wrote:
> I was looking at our confusing mess of memory accessor functions,
> and I realised that partly it was confusing because we have a
> bunch of unnecessary junk lurking in there :-) This series
> attempts to clean things up by removing things we weren't using
> at all or were only using by mistake in a few places:
> 
>  * ldul_*: not used
>  * ld* (ldl, etc): hardly used
>  * ld*_kernel: not used
>  * ld*_raw: hardly used
>  * cpu_{ld,st}{fq,fl}: not used
> 
> The dull parts of this series are removing the unused macros
> and fixing uses of the hardly-used macros so those can be
> deleted too. This series also switches to using inline functions
> rather than macros for the user-only cpu_ld/st* accessors, bringing
> them into line with the softmmu configs. This has the nice
> side effect of letting us get rid of the _raw accessor macros too.
> I've also thrown in a commit which cleans up the doc comments.
> 
> Peter Maydell (15):
>   cpu_ldst.h: Remove unused ldul_ macros
>   monitor.c: Use ld*_p() instead of ld*_raw()
>   target-sparc: Don't use {ld,st}*_raw functions
>   linux-user/elfload.c: Don't use _raw accessor functions
>   bsd-user/elfload.c: Don't use ldl() or ldq_raw()
>   linux-user/vm86.c: Use cpu_ldl_data &c rather than plain ldl &c
>   linux-user/main.c (m68k): Use get_user_u16 rather than lduw in
>     cpu_loop
>   target-mips: Don't use _raw load/store accessors
>   cpu_ldst.h: Drop unused ld/st*_kernel defines
>   cpu_ldst.h: Remove unused very short ld*/st* defines
>   cpu_ldst.h: Use inline functions for usermode cpu_ld/st accessors
>   cpu_ldst_template.h: Use ld*_p directly rather than via ld*_raw macros
>   cpu_ldst.h: Drop unused _raw macros, saddr() and laddr()
>   cpu_ldst_template.h: Drop unused cpu_ldfq/stfq/ldfl/stfl accessors
>   cpu_ldst.h, cpu-all.h, bswap.h: Update documentation on ld/st
>     accessors

Reviewed-by: Richard Henderson <rth@twiddle.net>

Nice cleanup.

r~