[Qemu-devel] [PATCH 1/2] linux-user: cleanup msync output in strace

[Qemu-devel] [PATCH 1/2] linux-user: cleanup msync output in strace

[Laurent Vivier]

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/strace.list | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/strace.list b/linux-user/strace.list
index ff8bb19f5f..c4a0e3510a 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -624,7 +624,7 @@
 { TARGET_NR_msgsnd, "msgsnd" , NULL, NULL, NULL },
 #endif
 #ifdef TARGET_NR_msync
-{ TARGET_NR_msync, "msync" , NULL, NULL, NULL },
+{ TARGET_NR_msync, "msync" , "%s(%#x,%d,%x)", NULL, NULL },
 #endif
 #ifdef TARGET_NR_multiplexer
 { TARGET_NR_multiplexer, "multiplexer" , NULL, NULL, NULL },
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] linux-user: fix 32bit g2h()/h2g()

[Laurent Vivier]

sparc32plus has 64bit long type but only 32bit virtual address space.

For instance, "apt-get upgrade" failed because of a mmap()/msync()
sequence.

mmap() returned 0xff252000 but msync() used g2h(0xffffffffff252000)
to find the host address. The "(target_ulong)" in g2h() doesn't fix the
address because it is 64bit long.

This patch introduces a "target_ptr" that is set to uint32_t
if the virtual address space is addressed using 32bit in the linux-user
case. It stays set to target_ulong with softmmu case.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 include/exec/cpu_ldst.h                   | 23 ++++++++++++++++++-----
 include/exec/cpu_ldst_useronly_template.h | 12 ++++++------
 linux-user/syscall.c                      |  2 +-
 3 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 0f2cb717b1..84c058e0f2 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -48,8 +48,19 @@
 #define CPU_LDST_H
 
 #if defined(CONFIG_USER_ONLY)
+/* sparc32plus has 64bit long but 32bit space address
+ * this can make bad result with g2h() and h2g()
+ */
+#if TARGET_VIRT_ADDR_SPACE_BITS <= 32
+typedef uint32_t target_ptr;
+#define TARGET_ABI_FMT_ptr "%x"
+#else
+typedef uint64_t target_ptr;
+#define TARGET_ABI_FMT_ptr PRIx64
+#endif
+
 /* All direct uses of g2h and h2g need to go away for usermode softmmu.  */
-#define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + guest_base))
+#define g2h(x) ((void *)((unsigned long)(target_ptr)(x) + guest_base))
 
 #define guest_addr_valid(x) ((x) <= GUEST_ADDR_MAX)
 #define h2g_valid(x) guest_addr_valid((unsigned long)(x) - guest_base)
@@ -61,7 +72,7 @@ static inline int guest_range_valid(unsigned long start, unsigned long len)
 
 #define h2g_nocheck(x) ({ \
     unsigned long __ret = (unsigned long)(x) - guest_base; \
-    (abi_ulong)__ret; \
+    (target_ptr)__ret; \
 })
 
 #define h2g(x) ({ \
@@ -69,7 +80,9 @@ static inline int guest_range_valid(unsigned long start, unsigned long len)
     assert(h2g_valid(x)); \
     h2g_nocheck(x); \
 })
-
+#else
+typedef target_ulong target_ptr;
+#define TARGET_ABI_FMT_ptr TARGET_ABI_FMT_lx
 #endif
 
 #if defined(CONFIG_USER_ONLY)
@@ -397,7 +410,7 @@ extern __thread uintptr_t helper_retaddr;
  * This is the equivalent of the initial fast-path code used by
  * TCG backends for guest load and store accesses.
  */
-static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
+static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ptr addr,
                                       int access_type, int mmu_idx)
 {
 #if defined(CONFIG_USER_ONLY)
@@ -405,7 +418,7 @@ static inline void *tlb_vaddr_to_host(CPUArchState *env, target_ulong addr,
 #else
     int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     CPUTLBEntry *tlbentry = &env->tlb_table[mmu_idx][index];
-    target_ulong tlb_addr;
+    target_ptr tlb_addr;
     uintptr_t haddr;
 
     switch (access_type) {
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index e30e58ed4a..5bb09d66c8 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -62,7 +62,7 @@
 #endif
 
 static inline RES_TYPE
-glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ptr ptr)
 {
 #if !defined(CODE_ACCESS)
     trace_guest_mem_before_exec(
@@ -74,7 +74,7 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 
 static inline RES_TYPE
 glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
-                                                  target_ulong ptr,
+                                                  target_ptr ptr,
                                                   uintptr_t retaddr)
 {
     RES_TYPE ret;
@@ -86,7 +86,7 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
 
 #if DATA_SIZE <= 2
 static inline int
-glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ptr ptr)
 {
 #if !defined(CODE_ACCESS)
     trace_guest_mem_before_exec(
@@ -98,7 +98,7 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 
 static inline int
 glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
-                                                  target_ulong ptr,
+                                                  target_ptr ptr,
                                                   uintptr_t retaddr)
 {
     int ret;
@@ -111,7 +111,7 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
 
 #ifndef CODE_ACCESS
 static inline void
-glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
+glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ptr ptr,
                                       RES_TYPE v)
 {
 #if !defined(CODE_ACCESS)
@@ -124,7 +124,7 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
 
 static inline void
 glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
-                                                  target_ulong ptr,
+                                                  target_ptr ptr,
                                                   RES_TYPE v,
                                                   uintptr_t retaddr)
 {
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6db0e8b9fb..e5b410c839 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7664,7 +7664,7 @@ static int open_self_maps(void *cpu_env, int fd)
             if (h2g(min) == ts->info->stack_limit) {
                 pstrcpy(path, sizeof(path), "      [stack]");
             }
-            dprintf(fd, TARGET_ABI_FMT_lx "-" TARGET_ABI_FMT_lx
+            dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
                     " %c%c%c%c %08" PRIx64 " %02x:%02x %d %s%s\n",
                     h2g(min), h2g(max - 1) + 1, flag_r, flag_w,
                     flag_x, flag_p, offset, dev_maj, dev_min, inode,
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/2] linux-user: cleanup msync output in strace

[Richard Henderson]

On 08/10/2018 11:53 AM, Laurent Vivier wrote:
> -{ TARGET_NR_msync, "msync" , NULL, NULL, NULL },
> +{ TARGET_NR_msync, "msync" , "%s(%#x,%d,%x)", NULL, NULL },

Would you mind having a look at my linux-user reorg proposal?
It would make this change redundant.

https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg02935.html


r~

Re: [Qemu-devel] [PATCH 2/2] linux-user: fix 32bit g2h()/h2g()

[Richard Henderson]

On 08/10/2018 11:53 AM, Laurent Vivier wrote:
> sparc32plus has 64bit long type but only 32bit virtual address space.
> 
> For instance, "apt-get upgrade" failed because of a mmap()/msync()
> sequence.
> 
> mmap() returned 0xff252000 but msync() used g2h(0xffffffffff252000)
> to find the host address. The "(target_ulong)" in g2h() doesn't fix the
> address because it is 64bit long.
> 
> This patch introduces a "target_ptr" that is set to uint32_t
> if the virtual address space is addressed using 32bit in the linux-user
> case. It stays set to target_ulong with softmmu case.

I would prefer the name "abi_ptr", since the full 64-bits of target_ulong are
still used for any actual dereferences.


r~

Re: [Qemu-devel] [PATCH 2/2] linux-user: fix 32bit g2h()/h2g()

[Laurent Vivier]

Le 10/08/2018 à 21:30, Richard Henderson a écrit :
> On 08/10/2018 11:53 AM, Laurent Vivier wrote:
>> sparc32plus has 64bit long type but only 32bit virtual address space.
>>
>> For instance, "apt-get upgrade" failed because of a mmap()/msync()
>> sequence.
>>
>> mmap() returned 0xff252000 but msync() used g2h(0xffffffffff252000)
>> to find the host address. The "(target_ulong)" in g2h() doesn't fix the
>> address because it is 64bit long.
>>
>> This patch introduces a "target_ptr" that is set to uint32_t
>> if the virtual address space is addressed using 32bit in the linux-user
>> case. It stays set to target_ulong with softmmu case.
> 
> I would prefer the name "abi_ptr", since the full 64-bits of target_ulong are
> still used for any actual dereferences.

I can, I have no preference for that.

The abi_XXX types have an alignment attribute, while the target_XXX
don't. Do you think I should add it to the abi_ptr type?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 1/2] linux-user: cleanup msync output in strace

[Laurent Vivier]

Le 10/08/2018 à 21:28, Richard Henderson a écrit :
> On 08/10/2018 11:53 AM, Laurent Vivier wrote:
>> -{ TARGET_NR_msync, "msync" , NULL, NULL, NULL },
>> +{ TARGET_NR_msync, "msync" , "%s(%#x,%d,%x)", NULL, NULL },
> 
> Would you mind having a look at my linux-user reorg proposal?
> It would make this change redundant.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg02935.html
> 

Yes, I know your series. I posted this patch because it helped me to
debug the patch 2/2. I can remove it for this series if you prefer.

Thanks,
Laurent