Re: [PATCH 1/1] target/ppc: fix ELFv2 signal handler endianness

[Vincent Fazio]

Laurent,

On Sun, Mar 15, 2020 at 1:10 PM Laurent Vivier <address@hidden> wrote:
>
> Le 15/03/2020 à 16:52, Vincent Fazio a écrit :
> > From: Vincent Fazio <address@hidden>
> >
> > In ELFv2, function pointers are entry points and are in host endianness.
>
> "host endianness" is misleading here. "target endianness" is better.

I do want to clarify here. In a mixed endian scenario (my test case
was an x86 host and e5500 PPC BE target), the function pointers are in
host endianness (little endian) so that the virtual address can be
dereferenced by the host for the target instructions to be translated.

>
> >
> > Previously, the signal handler would be swapped if the target CPU was a
> > different endianness than the host. This would cause a SIGSEGV when
> > attempting to translate the opcode pointed to by the swapped address.
>
> This is correct.
>
> >  Thread 1 "qemu-ppc64" received signal SIGSEGV, Segmentation fault.
> >  0x00000000600a9257 in ldl_he_p (ptr=0x4c2c061000000000) at 
> > qemu/include/qemu/bswap.h:351
> >  351        __builtin_memcpy(&r, ptr, sizeof(r));
> >
> >  #0  0x00000000600a9257 in ldl_he_p (ptr=0x4c2c061000000000) at 
> > qemu/include/qemu/bswap.h:351
> >  #1  0x00000000600a92fe in ldl_be_p (ptr=0x4c2c061000000000) at 
> > qemu/include/qemu/bswap.h:449
> >  #2  0x00000000600c0790 in translator_ldl_swap at 
> > qemu/include/exec/translator.h:201
> >  #3  0x000000006011c1ab in ppc_tr_translate_insn at 
> > qemu/target/ppc/translate.c:7856
> >  #4  0x000000006005ae70 in translator_loop at 
> > qemu/accel/tcg/translator.c:102
> >
> > Now, no swap is performed and execution continues properly.
> >
> > Signed-off-by: Vincent Fazio <address@hidden>
> > ---
> >  linux-user/ppc/signal.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> > index 5b82af6cb6..c7f6455170 100644
> > --- a/linux-user/ppc/signal.c
> > +++ b/linux-user/ppc/signal.c
> > @@ -567,9 +567,13 @@ void setup_rt_frame(int sig, struct target_sigaction 
> > *ka,
> >          env->nip = tswapl(handler->entry);
> >          env->gpr[2] = tswapl(handler->toc);
> >      } else {
> > -        /* ELFv2 PPC64 function pointers are entry points, but R12
> > -         * must also be set */
> > -        env->nip = tswapl((target_ulong) ka->_sa_handler);
> > +        /*
> > +         * ELFv2 PPC64 function pointers are entry points and are in host
> > +         * endianness so should not to be swapped.
>
> "target endianness"
>
> > +         *
> > +         * Note: R12 must also be set.
> > +         */
> > +        env->nip = (target_ulong) ka->_sa_handler;
>
> The cast is not needed: nip and _sa_handler are abi_ulong.

I'll drop this in v2

>
> >          env->gpr[12] = env->nip;
> >      }
> >  #else
> >
>
> If you repost with the fix I've reported above you can add my:
>
> Reviewed-by: Laurent Vivier <address@hidden>
>

I'll hold off on reposting until the endianness wording is figured out.

> Thanks,
> Laurent

Thanks,
-Vincent