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

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

[Samuel Seay]

[-- Attachment #1.1: Type: text/plain, Size: 154 bytes --]

Attached is a patch for fixing bug #1052857. My local tests show it working
properly on 32 and 64bit.

Signed-off-by: Samuel Seay <lightningth@gmail.com>

[-- Attachment #1.2: Type: text/html, Size: 213 bytes --]

[-- Attachment #2: 0001-Change-to-correct-PowerPC-on-64bit-host.patch --]
[-- Type: application/octet-stream, Size: 928 bytes --]

---
 linux-user/signal.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 95e2ffa..3835262 100644
--- 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]);
@@ -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;
 
-- 
1.7.9.5

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

[Andreas Färber]

Hello,

Am 02.01.2013 05:58, schrieb Samuel Seay:
> Attached is a patch for fixing bug #1052857. My local tests show it
> working properly on 32 and 64bit.
> 
> Signed-off-by: Samuel Seay <lightningth@gmail.com
> <mailto:lightningth@gmail.com>>

Please submit patches using git-send-email tool so that they arrive
text-only and inline and can be commented on by reviewers.

The commit message should describe what you are changing and why, not
that you're changing something and it happens to fix one bug. ;-)

Also you should cc the linux-user maintainer, ppc maintainer and
qemu-ppc@nongnu.org (see MAINTAINERS file) and make the commit message /
subject start with "linux-user: " to clarify what the patch is about and
who needs to look at it.

http://wiki.qemu.org/Contribute/SubmitAPatch

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] Change to correct PowerPC on a 64bit host

[Peter Maydell]

On 2 January 2013 04:58, Samuel Seay <lightningth@gmail.com> 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

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

[Samuel Seay]

[-- Attachment #1: Type: text/plain, Size: 2307 bytes --]

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 <peter.maydell@linaro.org>wrote:

> On 2 January 2013 04:58, Samuel Seay <lightningth@gmail.com> 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
>

[-- Attachment #2: Type: text/html, Size: 2880 bytes --]

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

[Peter Maydell]

On 2 January 2013 13:01, Samuel Seay <lightningth@gmail.com> wrote:
> 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.

No, it's not redundant -- we must save the old value of gpr[1], exactly
because we are about to change it (set it to newsp). The code is trying
to do the right thing (copy the old env->gpr[1] value into the guest
stack frame it is setting up) but in a broken way, so it must be fixed,
not just removed.

-- PMM

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

[Samuel Seay]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

I did not catch that, somehow I managed to invert the logic when looking at
it. Maybe a g2h() (such a macro exist? what would be the proper method?)
around the newsp value would do it. I'll redo that this evening and attempt
to submit a newer patch. Considering I don't have direct internet access in
the VM, any suggestions to make everyone happy on the patch submission?

I might be able to redo the git setup on my mac and do it from there.

Samuel

On Wed, Jan 2, 2013 at 9:02 AM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 2 January 2013 13:01, Samuel Seay <lightningth@gmail.com> wrote:
> > 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.
>
> No, it's not redundant -- we must save the old value of gpr[1], exactly
> because we are about to change it (set it to newsp). The code is trying
> to do the right thing (copy the old env->gpr[1] value into the guest
> stack frame it is setting up) but in a broken way, so it must be fixed,
> not just removed.
>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 1738 bytes --]

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

[Alexander Graf]

[-- Attachment #1: Type: text/plain, Size: 1687 bytes --]

Please don't top post.

Am 02.01.2013 um 15:34 schrieb Samuel Seay <lightningth@gmail.com>:

> I did not catch that, somehow I managed to invert the logic when looking at it. Maybe a g2h() (such a macro exist? what would be the proper method?) around the newsp value would do it.

Sounds reasonable OTOH ;).

> I'll redo that this evening and attempt to submit a newer patch. Considering I don't have direct internet access in the VM, any suggestions to make everyone happy on the patch submission?

Sure. Just export your patch using git format-patch, copy it to your host and run git send-mail from there ;).

> 
> I might be able to redo the git setup on my mac and do it from there.

There are easy to install git packages for osx readily available, yeah.

Alex

> 
> Samuel
> 
> On Wed, Jan 2, 2013 at 9:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 January 2013 13:01, Samuel Seay <lightningth@gmail.com> wrote:
> > 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.
> 
> No, it's not redundant -- we must save the old value of gpr[1], exactly
> because we are about to change it (set it to newsp). The code is trying
> to do the right thing (copy the old env->gpr[1] value into the guest
> stack frame it is setting up) but in a broken way, so it must be fixed,
> not just removed.
> 
> -- PMM
> 

[-- Attachment #2: Type: text/html, Size: 2456 bytes --]

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

[Peter Maydell]

On 2 January 2013 14:34, Samuel Seay <lightningth@gmail.com> wrote:
> I did not catch that, somehow I managed to invert the logic when looking at
> it. Maybe a g2h() (such a macro exist? what would be the proper method?)
> around the newsp value would do it.

You want to use put_user() (without the __) -- this (a) takes a guest address
and (b) does the error checking for unwritable address, which __put_user
does not. [Don't be fooled by all the err |= __put_user code, this is bogus
because __put_user never fails.]

> I'll redo that this evening and attempt
> to submit a newer patch. Considering I don't have direct internet access in
> the VM, any suggestions to make everyone happy on the patch submission?

Use git format-patch in the VM to write the patch to file, copy the file out of
the VM and use git send-email to actually send it.

-- PMM