Re: [Qemu-ppc] [Qemu-devel] [PATCH] Change to correct PowerPC on a 64bit host

[Samuel Seay]

The VM I did the work in doesn't have internet access and I was unsure how to do a text only email with gmail. With that said, the line that removed the env->gpr[1] is redudant as a few lines below in the original source it is set with newsp. The removed line would seg fault due to trying to write the value of env->gpr[1] into newsp, which is not valid in host.

I can not speak to the line a bit further with h2g(sc).

Samuel

On Wed, Jan 2, 2013 at 7:00 AM, Peter Maydell <address@hidden> wrote:

On 2 January 2013 04:58, Samuel Seay <address@hidden> wrote:
> Attached is a patch for fixing bug #1052857. My local tests show it working
> properly on 32 and 64bit.

--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -4584,7 +4584,7 @@ static void setup_frame(int sig, struct
target_sigaction *ka,

     signal = current_exec_domain_sig(sig);

-    err |= __put_user(h2g(ka->_sa_handler), &sc->handler);
+    err |= __put_user(ka->_sa_handler, &sc->handler);
     err |= __put_user(set->sig[0], &sc->oldmask);
 #if defined(TARGET_PPC64)
     err |= __put_user(set->sig[0] >> 32, &sc->_unused[3]);

This looks OK...


@@ -4606,8 +4606,6 @@ static void setup_frame(int sig, struct
target_sigaction *ka,

     /* Create a stack frame for the caller of the handler.  */
     newsp = frame_addr - SIGNAL_FRAMESIZE;
-    err |= __put_user(env->gpr[1], (target_ulong *)(uintptr_t) newsp);
-
     if (err)
         goto sigsegv;

...but this bit doesn't. We need to save the old SP to the stack frame,
and your patch just skips this step. You're right that the line in question
is broken though; it has two problems:
 * it's using newsp (a guest address) as an argument to __put_user(),
   which wants a host address
 * it's using __put_user() which works on locked addresses, but newsp
   is below the area we locked with lock_user_struct earlier

Another dodgy line in this function:
    env->gpr[4] = (target_ulong) h2g(sc);
Since sc is an offset into the struct returned by lock_user_struct(),
if DEBUG_REMAP is defined then we're passing the guest a pointer
to memory that is free()d by unlock_user_struct(). This should probably
be setting gpr[4] to frame_addr + offsetof(something) instead.

-- PMM