[Qemu-devel] [PATCH 0/9] linux-user: Wine enablement patch set

[Qemu-devel] [PATCH 0/9] linux-user: Wine enablement patch set

[Alexander Graf]

Howdy,

It's been a while since I've tried to run wine in QEMU's i386
linux-user target, so I figured I'd give it a go again.

Obviously I've hit a bunch of obstacles, some of which were old,
some of which we only introduced recently.

However, with this patch set I am able to successfully run
Civilization II in wine on my ARM Chromebook:

  http://db.tt/AvHJOKSg

Beware that this patch set is based on Andreas' qom-cpu branch.
If you just want a working git tree to try this out on, check out:

  git://github.com/agraf/qemu.git linux-user-x86-tls-v1


Alex

Alexander Graf (9):
  linux-user: fix segmentation fault passing with h2g(x) != x
  linux-user: Add is_write segfault check for ARM hosts
  linux-user: Don't reset a new thread's CPU
  linux-user: Fix sendrecvmsg() with QEMU_GUEST_BASE
  linux-user: Fix epoll on ARM hosts
  linux-user: Add i386 TLS setter
  linux-user: Enable NPTL for i386
  linux-user: Default to 64k guest base
  linux-user: Unlock mmap_lock when resuming guest from page_unprotect

 configure                    |    1 +
 linux-user/i386/target_cpu.h |   12 ++++++++++--
 linux-user/main.c            |    6 +++---
 linux-user/syscall.c         |    4 ++--
 linux-user/syscall_defs.h    |    7 +++++--
 translate-all.c              |   10 +++++++---
 user-exec.c                  |    9 +++++++--
 7 files changed, 35 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH 1/9] linux-user: fix segmentation fault passing with h2g(x) != x

[Alexander Graf]

When forwarding a segmentation fault into the guest process, we were passing
the host's address directly into the guest process's signal descriptor.

That obviously confused the guest process, since it didn't know what to make
of the (usually 32-bit truncated) address. Passing in h2g(address) makes the
guest process a lot happier.

This fixes java running in arm-linux-user for me.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 user-exec.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/user-exec.c b/user-exec.c
index 26cde7c..718c54f 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -94,6 +94,12 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
         return 1;
     }
 
+    if (GUEST_BASE) {
+        /* Convert forcefully to guest address space, invalid addresses
+           are still valid segv ones */
+        address = address - GUEST_BASE;
+    }
+
     env = current_cpu->env_ptr;
     /* see if it is an MMU fault */
     ret = cpu_handle_mmu_fault(env, address, is_write, MMU_USER_IDX);
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/9] linux-user: Add is_write segfault check for ARM hosts

[Alexander Graf]

When we get a segmentation fault we check whether the fault was a write. If
it was a write, it might be a fault because we tried to modify a code region.

This logic does not work on ARM hosts, because they don't evaluate whether a
segementation fault is due to a write. Instead they always declare it a read.

So self modifying code fails with a segmentation fault whenever it tries to
modify itself.

Add the is_write evaluation based on what the kernel tells us as fault reason.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 user-exec.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/user-exec.c b/user-exec.c
index 718c54f..bbeb0dd 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -448,8 +448,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
 #else
     pc = uc->uc_mcontext.arm_pc;
 #endif
-    /* XXX: compute is_write */
-    is_write = 0;
+    is_write = (uc->uc_mcontext.error_code & 0x800) ? 1 : 0;
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
                              is_write,
                              &uc->uc_sigmask, puc);
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/9] linux-user: Don't reset a new thread's CPU

[Alexander Graf]

When we create a new thread, there is no reason to reset it. I'm fairly sure
the code has mostly been left in there because nobody understood why it was
there in the first place.

Remove the reset. A new thread's kernel sided state should be identical to
the old one's.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/syscall.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 433d3ba..d0408a2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4234,7 +4234,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         init_task_state(ts);
         /* we create a new CPU instance. */
         new_env = cpu_copy(env);
-#if defined(TARGET_I386) || defined(TARGET_SPARC) || defined(TARGET_PPC)
+#if defined(TARGET_SPARC) || defined(TARGET_PPC)
         cpu_reset(ENV_GET_CPU(new_env));
 #endif
         /* Init regs that differ from the parent.  */
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/9] linux-user: Fix sendrecvmsg() with QEMU_GUEST_BASE

[Alexander Graf]

While looking for cmsg entries, we want to compare guest pointers to see
whether we're at the end of the passed in array.

However, what we really do is we compare our in-use host pointer with the
to-be-the-end guest pointer. This comparison is obviously bogus.

Change the comparison to compare guest pointer with guest pointer.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/syscall_defs.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 92c01a9..8b06a19 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -214,7 +214,7 @@ __target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cms
 
   __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
                                     + TARGET_CMSG_ALIGN (tswapal(__cmsg->cmsg_len)));
-  if ((unsigned long)((char *)(__ptr+1) - (char *)(size_t)tswapal(__mhdr->msg_control))
+  if ((unsigned long)((char *)(h2g(__ptr+1)) - (char *)(size_t)tswapal(__mhdr->msg_control))
       > tswapal(__mhdr->msg_controllen))
     /* No more entries.  */
     return (struct target_cmsghdr *)0;
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/9] linux-user: Fix epoll on ARM hosts

[Alexander Graf]

The epoll emulation uses data structures without packing them, so the
compiler might choose to add padding inside.

This patch makes the most offending one (target_epoll_event) a packed
structure to make sure we don't pad it by accident. ARM would pad it,
so declare the padding mandatory for ARM targets.

This fixes i386-on-ARM epoll emulation for me.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/syscall_defs.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 8b06a19..fbc3cac 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2434,8 +2434,11 @@ typedef union target_epoll_data {
 
 struct target_epoll_event {
     uint32_t events;
+#ifdef TARGET_ARM
+    uint32_t __pad;
+#endif
     target_epoll_data_t data;
-};
+} QEMU_PACKED;
 #endif
 struct target_rlimit64 {
     uint64_t rlim_cur;
-- 
1.6.0.2

[Qemu-devel] [PATCH 6/9] linux-user: Add i386 TLS setter

[Alexander Graf]

We can easily set the TLS on i386. Add code to do so.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/i386/target_cpu.h |   12 ++++++++++--
 linux-user/syscall.c         |    2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/linux-user/i386/target_cpu.h b/linux-user/i386/target_cpu.h
index abcac79..1170d84 100644
--- a/linux-user/i386/target_cpu.h
+++ b/linux-user/i386/target_cpu.h
@@ -28,6 +28,14 @@ static inline void cpu_clone_regs(CPUX86State *env, target_ulong newsp)
     env->regs[R_EAX] = 0;
 }
 
-/* TODO: need to implement cpu_set_tls() */
+#if defined(TARGET_ABI32)
+abi_long do_set_thread_area(CPUX86State *env, abi_ulong ptr);
 
-#endif
+static inline void cpu_set_tls(CPUX86State *env, target_ulong newtls)
+{
+    do_set_thread_area(env, newtls);
+    cpu_x86_load_seg(env, R_GS, env->segs[R_GS].selector);
+}
+#endif /* defined(TARGET_ABI32) */
+
+#endif /* !defined(TARGET_CPU_H) */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index d0408a2..1996cfb 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3976,7 +3976,7 @@ static abi_long do_modify_ldt(CPUX86State *env, int func, abi_ulong ptr,
 }
 
 #if defined(TARGET_I386) && defined(TARGET_ABI32)
-static abi_long do_set_thread_area(CPUX86State *env, abi_ulong ptr)
+abi_long do_set_thread_area(CPUX86State *env, abi_ulong ptr)
 {
     uint64_t *gdt_table = g2h(env->gdt.base);
     struct target_modify_ldt_ldt_s ldt_info;
-- 
1.6.0.2

[Qemu-devel] [PATCH 7/9] linux-user: Enable NPTL for i386

[Alexander Graf]

The i386 target is now able to properly handle NPTL. Enable it.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 configure |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 0e0adde..61f2770 100755
--- a/configure
+++ b/configure
@@ -4156,6 +4156,7 @@ TARGET_ABI_DIR=""
 
 case "$target_name" in
   i386)
+    target_nptl="yes"
   ;;
   x86_64)
     TARGET_BASE_ARCH=i386
-- 
1.6.0.2

[Qemu-devel] [PATCH 8/9] linux-user: Default to 64k guest base

[Alexander Graf]

Most kernels these days have protection code in place to forbid user space
to access low memory. The barrier varies between architectures though.

For this purpose we have the guest base option that allows us to offset
guest visible memory from host memory, so that the guest process thinks
it can access lower memory than it really can access.

Set the default for the guest base to 64k which should be good enough on
any host system.

This fixes running i386 wine on ARM for me.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/main.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 7f15d3d..a246cff 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -45,8 +45,8 @@ envlist_t *envlist;
 const char *cpu_model;
 unsigned long mmap_min_addr;
 #if defined(CONFIG_USE_GUEST_BASE)
-unsigned long guest_base;
-int have_guest_base;
+unsigned long guest_base = 64 * 1024;
+int have_guest_base = 1;
 #if (TARGET_LONG_BITS == 32) && (HOST_LONG_BITS == 64)
 /*
  * When running 32-on-64 we should make sure we can fit all of the possible
@@ -3294,7 +3294,7 @@ static void handle_arg_cpu(const char *arg)
 static void handle_arg_guest_base(const char *arg)
 {
     guest_base = strtol(arg, NULL, 0);
-    have_guest_base = 1;
+    have_guest_base = guest_base ? 1 : 0;
 }
 
 static void handle_arg_reserved_va(const char *arg)
-- 
1.6.0.2

[Qemu-devel] [PATCH 9/9] linux-user: Unlock mmap_lock when resuming guest from page_unprotect

[Alexander Graf]

The page_unprotect() function is running everything locked. Before every
potential exit path of the function mmap_unlock() gets called to make sure
we don't leak the lock.

However, the function calls tb_invalidate_phys_page() which again can
exit a signal through longjmp, leaving our mmap_unlock() attempts in vain.

Add a hint to tb_invalidate_phys_page() that we need to unlock before we
can leave back into guest context, so that we don't leak the lock.

This fixes 16-bit i386 wine programs running in linux-user for me.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 translate-all.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index e8683d2..3b5fc7c 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1148,7 +1148,8 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
 
 #if !defined(CONFIG_SOFTMMU)
 static void tb_invalidate_phys_page(tb_page_addr_t addr,
-                                    uintptr_t pc, void *puc)
+                                    uintptr_t pc, void *puc,
+                                    bool locked)
 {
     TranslationBlock *tb;
     PageDesc *p;
@@ -1206,6 +1207,9 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
            itself */
         cpu->current_tb = NULL;
         tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
+        if (locked) {
+            mmap_unlock();
+        }
         cpu_resume_from_signal(env, puc);
     }
 #endif
@@ -1723,7 +1727,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
         if (!(p->flags & PAGE_WRITE) &&
             (flags & PAGE_WRITE) &&
             p->first_tb) {
-            tb_invalidate_phys_page(addr, 0, NULL);
+            tb_invalidate_phys_page(addr, 0, NULL, false);
         }
         p->flags = flags;
     }
@@ -1818,7 +1822,7 @@ int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
 
             /* and since the content will be modified, we must invalidate
                the corresponding translated code. */
-            tb_invalidate_phys_page(addr, pc, puc);
+            tb_invalidate_phys_page(addr, pc, puc, true);
 #ifdef DEBUG_TB_CHECK
             tb_invalidate_check(addr);
 #endif
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 2/9] linux-user: Add is_write segfault check for ARM hosts

[Peter Maydell]

On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
> When we get a segmentation fault we check whether the fault was a write. If
> it was a write, it might be a fault because we tried to modify a code region.
>
> This logic does not work on ARM hosts, because they don't evaluate whether a
> segementation fault is due to a write. Instead they always declare it a read.
>
> So self modifying code fails with a segmentation fault whenever it tries to
> modify itself.
>
> Add the is_write evaluation based on what the kernel tells us as fault reason.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

We've already got a patch for this on list :
http://patchwork.ozlabs.org/patch/248590/

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/9] linux-user: fix segmentation fault passing with h2g(x) != x

[Peter Maydell]

On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
> When forwarding a segmentation fault into the guest process, we were passing
> the host's address directly into the guest process's signal descriptor.
>
> That obviously confused the guest process, since it didn't know what to make
> of the (usually 32-bit truncated) address. Passing in h2g(address) makes the
> guest process a lot happier.

Commit message says it uses h2g, code doesn't.  Maybe we should have
an h2g_unchecked() for this sort of use?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/9] linux-user: Add is_write segfault check for ARM hosts

[Alexander Graf]

On 06.07.2013, at 12:24, Peter Maydell wrote:

> On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
>> When we get a segmentation fault we check whether the fault was a write. If
>> it was a write, it might be a fault because we tried to modify a code region.
>> 
>> This logic does not work on ARM hosts, because they don't evaluate whether a
>> segementation fault is due to a write. Instead they always declare it a read.
>> 
>> So self modifying code fails with a segmentation fault whenever it tries to
>> modify itself.
>> 
>> Add the is_write evaluation based on what the kernel tells us as fault reason.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> We've already got a patch for this on list :
> http://patchwork.ozlabs.org/patch/248590/

Ah, seems like we wrote both patches at about the same time. I prefer yours though - it has a nice comment going with it :).


Alex

Re: [Qemu-devel] [PATCH 3/9] linux-user: Don't reset a new thread's CPU

[Peter Maydell]

On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
> When we create a new thread, there is no reason to reset it. I'm fairly sure
> the code has mostly been left in there because nobody understood why it was
> there in the first place.

We had a big discussion on this on IRC. This code is here
because of commit b4558d748, which attempted to fix a breakage
introduced when commit b55a37c9 made these CPUs no longer do
a reset as part of their cpu_init(). However, it's in the
wrong place -- this reset needs to go into cpu_copy(), so
it doesn't undo the copying work done by that function.

> Remove the reset. A new thread's kernel sided state should be identical to
> the old one's.

Just deleting the reset isn't right. We should standardize
whether we think cpu_init() ought to give you a cleanly
reset CPU or not (I think it should); until then, we should
put a cpu_reset() immediately after cpu_init() in cpu_copy().
It doesn't need to be ifdef-guarded either.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/9] linux-user: Fix sendrecvmsg() with QEMU_GUEST_BASE

[Peter Maydell]

On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
> While looking for cmsg entries, we want to compare guest pointers to see
> whether we're at the end of the passed in array.
>
> However, what we really do is we compare our in-use host pointer with the
> to-be-the-end guest pointer. This comparison is obviously bogus.
>
> Change the comparison to compare guest pointer with guest pointer.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  linux-user/syscall_defs.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 92c01a9..8b06a19 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -214,7 +214,7 @@ __target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cms
>
>    __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
>                                      + TARGET_CMSG_ALIGN (tswapal(__cmsg->cmsg_len)));
> -  if ((unsigned long)((char *)(__ptr+1) - (char *)(size_t)tswapal(__mhdr->msg_control))
> +  if ((unsigned long)((char *)(h2g(__ptr+1)) - (char *)(size_t)tswapal(__mhdr->msg_control))
>        > tswapal(__mhdr->msg_controllen))
>      /* No more entries.  */
>      return (struct target_cmsghdr *)0;

I don't think this is right. The passed in __cmsg (and thus the
__ptr we calculate) isn't a guest address -- it's the address
we get back from calling lock_user() on a guest address.
That can't be validly compared with anything except another
address derived by arithmetic from the same lock_user()
return value (because if DEBUG_REMAP is defined then the
value you get back from lock_user() is the result of calling
malloc()). What we ought to be comparing __ptr+1 against
is not tswapal(__mhdr->msg_control) but the initial value
of target_cmsg returned from lock_user().

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/9] linux-user: Fix epoll on ARM hosts

[Peter Maydell]

On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 8b06a19..fbc3cac 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2434,8 +2434,11 @@ typedef union target_epoll_data {
>
>  struct target_epoll_event {
>      uint32_t events;
> +#ifdef TARGET_ARM
> +    uint32_t __pad;
> +#endif
>      target_epoll_data_t data;
> -};
> +} QEMU_PACKED;
>  #endif
>  struct target_rlimit64 {
>      uint64_t rlim_cur;

Is ARM really the only arch that needs the pad field?

-- PMM

Re: [Qemu-devel] [PATCH 4/9] linux-user: Fix sendrecvmsg() with QEMU_GUEST_BASE

[Alexander Graf]

On 06.07.2013, at 12:42, Peter Maydell wrote:

> On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
>> While looking for cmsg entries, we want to compare guest pointers to see
>> whether we're at the end of the passed in array.
>> 
>> However, what we really do is we compare our in-use host pointer with the
>> to-be-the-end guest pointer. This comparison is obviously bogus.
>> 
>> Change the comparison to compare guest pointer with guest pointer.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> linux-user/syscall_defs.h |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
>> index 92c01a9..8b06a19 100644
>> --- a/linux-user/syscall_defs.h
>> +++ b/linux-user/syscall_defs.h
>> @@ -214,7 +214,7 @@ __target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cms
>> 
>>   __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
>>                                     + TARGET_CMSG_ALIGN (tswapal(__cmsg->cmsg_len)));
>> -  if ((unsigned long)((char *)(__ptr+1) - (char *)(size_t)tswapal(__mhdr->msg_control))
>> +  if ((unsigned long)((char *)(h2g(__ptr+1)) - (char *)(size_t)tswapal(__mhdr->msg_control))
>>> tswapal(__mhdr->msg_controllen))
>>     /* No more entries.  */
>>     return (struct target_cmsghdr *)0;
> 
> I don't think this is right. The passed in __cmsg (and thus the
> __ptr we calculate) isn't a guest address -- it's the address
> we get back from calling lock_user() on a guest address.

... which makes it a host address we want to convert into guest address space, so we can do a guest <-> guest comparison.

> That can't be validly compared with anything except another
> address derived by arithmetic from the same lock_user()
> return value (because if DEBUG_REMAP is defined then the

Ah, ok. I didn't know about that debug flag. That might break, yes.

> value you get back from lock_user() is the result of calling
> malloc()). What we ought to be comparing __ptr+1 against
> is not tswapal(__mhdr->msg_control) but the initial value
> of target_cmsg returned from lock_user().

Ok :).


Alex

Re: [Qemu-devel] [PATCH 7/9] linux-user: Enable NPTL for i386

[Peter Maydell]

On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
> The i386 target is now able to properly handle NPTL. Enable it.

This will conflict with the on-list series which reverses
the default for target_nptl in this bit of configure
(though the correction is obviously trivial).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/9] linux-user: Fix epoll on ARM hosts

[Alexander Graf]

On 06.07.2013, at 12:45, Peter Maydell wrote:

> On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
>> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
>> index 8b06a19..fbc3cac 100644
>> --- a/linux-user/syscall_defs.h
>> +++ b/linux-user/syscall_defs.h
>> @@ -2434,8 +2434,11 @@ typedef union target_epoll_data {
>> 
>> struct target_epoll_event {
>>     uint32_t events;
>> +#ifdef TARGET_ARM
>> +    uint32_t __pad;
>> +#endif
>>     target_epoll_data_t data;
>> -};
>> +} QEMU_PACKED;
>> #endif
>> struct target_rlimit64 {
>>     uint64_t rlim_cur;
> 
> Is ARM really the only arch that needs the pad field?

It's the only one I definitely know about. Other targets may add it as they see fit. It shouldn't be more broken than before really, where we just took random host alignment.


Alex

Re: [Qemu-devel] [PATCH 7/9] linux-user: Enable NPTL for i386

[Alexander Graf]

On 06.07.2013, at 12:48, Peter Maydell wrote:

> On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
>> The i386 target is now able to properly handle NPTL. Enable it.
> 
> This will conflict with the on-list series which reverses
> the default for target_nptl in this bit of configure
> (though the correction is obviously trivial).

It's here for completeness only. It's such a trivial change that I'm sure we can easily redo it once your series is in :).


Alex

Re: [Qemu-devel] [PATCH 3/9] linux-user: Don't reset a new thread's CPU

[Andreas Färber]

Am 06.07.2013 12:31, schrieb Peter Maydell:
> On 6 July 2013 01:36, Alexander Graf <agraf@suse.de> wrote:
>> When we create a new thread, there is no reason to reset it. I'm fairly sure
>> the code has mostly been left in there because nobody understood why it was
>> there in the first place.
> 
> We had a big discussion on this on IRC. This code is here
> because of commit b4558d748, which attempted to fix a breakage
> introduced when commit b55a37c9 made these CPUs no longer do
> a reset as part of their cpu_init().

And as I said on IRC, x86 has been fixed to reset as part of cpu_init():
http://repo.or.cz/w/qemu.git/commit/77868120cfe93ad7816dfac6546684e5a6c6e256?f=linux-user/main.c

So since this patch only removes x86 reset and leaves ppc and sparc in
place, I think this patch is more correct than what we have now.

> However, it's in the
> wrong place -- this reset needs to go into cpu_copy(), so
> it doesn't undo the copying work done by that function.

The remaining question Eduardo raised was whether not resetting after
fiddling with fields might cause some kind of inconsistency in the CPU.

But since the current code effectively undoes the effects of cpu_copy(),
I'm willing to pick up this patch as follow-up to my earlier refactoring
(and 6/9 if ack'ed as follow-up to Peter's refactoring) for qom-cpu.

The commit message would need to be reworked though to reference why
this can be done for x86. And we'd still need a follow-up solution for
ppc and sparc.

Peter's suggestion was that we run LTP in a chroot to verify whether our
respective solutions break anything badly:
http://wiki.qemu.org/Testing/LTP
(BTW the May 2013 stable version is available now)

>> Remove the reset. A new thread's kernel sided state should be identical to
>> the old one's.
> 
> Just deleting the reset isn't right. We should standardize
> whether we think cpu_init() ought to give you a cleanly
> reset CPU or not (I think it should);

+1

Specifically as part of QOM realize, which - once cpu_init() is gone -
linux-user/bsd-user would trivially invoke directly after object_new().

softmmu would do it after the future QMP qom-set phase. The mess there
is reset handler registration order: We cannot have most CPUs register a
reset handler themselves yet because some machines (including most ARM
ones) register reset handlers to tweak registers before the CPU would
have reset in that future scenario.

> until then, we should
> put a cpu_reset() immediately after cpu_init() in cpu_copy().
> It doesn't need to be ifdef-guarded either.

Will test and post my patch to that effect.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/9] linux-user: Don't reset a new thread's CPU

[Peter Maydell]

On 6 July 2013 13:40, Andreas Färber <afaerber@suse.de> wrote:
> softmmu would do it after the future QMP qom-set phase. The mess there
> is reset handler registration order: We cannot have most CPUs register a
> reset handler themselves yet because some machines (including most ARM
> ones) register reset handlers to tweak registers before the CPU would
> have reset in that future scenario.

I'm not really a fan of that "use reset handler to simulate
bootloader firmware" code, so if you have a cleaner solution
to suggest I'd be happy to move to that.

(if the cleaner solution is "provide a firmware blob for
all boards" that might be too much work though :-))

-- PMM

Re: [Qemu-devel] [PATCH 3/9] linux-user: Don't reset a new thread's CPU

[Andreas Färber]

Am 06.07.2013 14:44, schrieb Peter Maydell:
> On 6 July 2013 13:40, Andreas Färber <afaerber@suse.de> wrote:
>> softmmu would do it after the future QMP qom-set phase. The mess there
>> is reset handler registration order: We cannot have most CPUs register a
>> reset handler themselves yet because some machines (including most ARM
>> ones) register reset handlers to tweak registers before the CPU would
>> have reset in that future scenario.
> 
> I'm not really a fan of that "use reset handler to simulate
> bootloader firmware" code, so if you have a cleaner solution
> to suggest I'd be happy to move to that.

I once suggested a secondary reset hook in CPUClass, to be invoked from
CPUClass::reset, that boards could set to piggy-back initializations,
but IIRC Anthony didn't like that.

For PReP I am trying to avoid NIP tweaking sneaking in with the ELF
loading requests since I know how hard it'll be to keep working.

Andreas

> (if the cleaner solution is "provide a firmware blob for
> all boards" that might be too much work though :-))
> 
> -- PMM

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg