This fixes signal handlers running with the wrong endianness if the
interrupted code used SETEND to dynamically switch endianness.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
---
 linux-user/arm/signal.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/linux-user/arm/signal.c b/linux-user/arm/signal.c
index d96fc27ce1..8aca5f61b7 100644
--- a/linux-user/arm/signal.c
+++ b/linux-user/arm/signal.c
@@ -244,6 +244,12 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
     } else {
         cpsr &= ~CPSR_T;
     }
+    cpsr &= ~CPSR_E;
+#ifdef TARGET_WORDS_BIGENDIAN
+    if (env->cp15.sctlr_el[1] & SCTLR_E0E) {
+        cpsr |= CPSR_E;
+    }
+#endif
 
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         if (is_fdpic) {
@@ -287,7 +293,8 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
     env->regs[13] = frame_addr;
     env->regs[14] = retcode;
     env->regs[15] = handler & (thumb ? ~1 : ~3);
-    cpsr_write(env, cpsr, CPSR_IT | CPSR_T, CPSRWriteByInstr);
+    cpsr_write(env, cpsr, CPSR_IT | CPSR_T | CPSR_E, CPSRWriteByInstr);
+    arm_rebuild_hflags(env);
 
     return 0;
 }
-- 
2.26.2

On Thu, 7 May 2020 at 21:25, Amanieu d'Antras <amanieu@gmail.com> wrote:
>
> This fixes signal handlers running with the wrong endianness if the
> interrupted code used SETEND to dynamically switch endianness.
>
> Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> ---
>  linux-user/arm/signal.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/arm/signal.c b/linux-user/arm/signal.c
> index d96fc27ce1..8aca5f61b7 100644
> --- a/linux-user/arm/signal.c
> +++ b/linux-user/arm/signal.c
> @@ -244,6 +244,12 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
>      } else {
>          cpsr &= ~CPSR_T;
>      }
> +    cpsr &= ~CPSR_E;
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    if (env->cp15.sctlr_el[1] & SCTLR_E0E) {
> +        cpsr |= CPSR_E;
> +    }
> +#endif

The #ifdef isn't incorrect, but I don't think we need it --
if we're emulating a little-endian binary then SCTLR_E0E will
be 0 and we won't set CPSR.E. So we can drop the #ifdef and
the code is a little cleaner to read.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

On Thu, 7 May 2020 at 21:25, Amanieu d'Antras <amanieu@gmail.com> wrote:
>
> This fixes signal handlers running with the wrong endianness if the
> interrupted code used SETEND to dynamically switch endianness.
>
> Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> ---
>  linux-user/arm/signal.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/arm/signal.c b/linux-user/arm/signal.c
> index d96fc27ce1..8aca5f61b7 100644
> --- a/linux-user/arm/signal.c
> +++ b/linux-user/arm/signal.c
> @@ -244,6 +244,12 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
>      } else {
>          cpsr &= ~CPSR_T;
>      }
> +    cpsr &= ~CPSR_E;
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    if (env->cp15.sctlr_el[1] & SCTLR_E0E) {
> +        cpsr |= CPSR_E;
> +    }
> +#endif
>
>      if (ka->sa_flags & TARGET_SA_RESTORER) {
>          if (is_fdpic) {
> @@ -287,7 +293,8 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
>      env->regs[13] = frame_addr;
>      env->regs[14] = retcode;
>      env->regs[15] = handler & (thumb ? ~1 : ~3);
> -    cpsr_write(env, cpsr, CPSR_IT | CPSR_T, CPSRWriteByInstr);
> +    cpsr_write(env, cpsr, CPSR_IT | CPSR_T | CPSR_E, CPSRWriteByInstr);
> +    arm_rebuild_hflags(env);

I was just looking at the signal code's handling of CPSR for a different
reason, and I noticed that at the moment we don't allow CPSR.E to be
updated from the signal frame when the signal handler returns
(because CPSR_USER doesn't contain CPSR_E and that's what we
use in restore_sigcontext() to define what bits from the frame we
allow updating). Don't you find that when the interrupted code
returns from the signal handler that it ends up running with the
wrong endianness (ie the endianness the handler used) ?

I'm going to fix this by putting CPSR_E in CPSR_USER, anyway.

thanks
-- PMM

On Fri, May 15, 2020 at 7:34 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 7 May 2020 at 21:25, Amanieu d'Antras <amanieu@gmail.com> wrote:
> >
> > This fixes signal handlers running with the wrong endianness if the
> > interrupted code used SETEND to dynamically switch endianness.
> >
> > Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
> > ---
> >  linux-user/arm/signal.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/linux-user/arm/signal.c b/linux-user/arm/signal.c
> > index d96fc27ce1..8aca5f61b7 100644
> > --- a/linux-user/arm/signal.c
> > +++ b/linux-user/arm/signal.c
> > @@ -244,6 +244,12 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
> >      } else {
> >          cpsr &= ~CPSR_T;
> >      }
> > +    cpsr &= ~CPSR_E;
> > +#ifdef TARGET_WORDS_BIGENDIAN
> > +    if (env->cp15.sctlr_el[1] & SCTLR_E0E) {
> > +        cpsr |= CPSR_E;
> > +    }
> > +#endif
> >
> >      if (ka->sa_flags & TARGET_SA_RESTORER) {
> >          if (is_fdpic) {
> > @@ -287,7 +293,8 @@ setup_return(CPUARMState *env, struct target_sigaction *ka,
> >      env->regs[13] = frame_addr;
> >      env->regs[14] = retcode;
> >      env->regs[15] = handler & (thumb ? ~1 : ~3);
> > -    cpsr_write(env, cpsr, CPSR_IT | CPSR_T, CPSRWriteByInstr);
> > +    cpsr_write(env, cpsr, CPSR_IT | CPSR_T | CPSR_E, CPSRWriteByInstr);
> > +    arm_rebuild_hflags(env);
>
> I was just looking at the signal code's handling of CPSR for a different
> reason, and I noticed that at the moment we don't allow CPSR.E to be
> updated from the signal frame when the signal handler returns
> (because CPSR_USER doesn't contain CPSR_E and that's what we
> use in restore_sigcontext() to define what bits from the frame we
> allow updating). Don't you find that when the interrupted code
> returns from the signal handler that it ends up running with the
> wrong endianness (ie the endianness the handler used) ?

I actually found this while trying to test the SETEND instruction
under risu. The signal handler was crashing because it loaded a
pointer with the wrong endianness, which was pretty obvious. However I
missed the fact that code was now running with the wrong endianness
after returning from the signal handler since I had both the master
and the apprentice running under qemu-arm.

> I'm going to fix this by putting CPSR_E in CPSR_USER, anyway.

You also need to call arm_rebuild_hflags() after modifying CPSR_E
otherwise the change doesn't take effect.

-- Amanieu

On Fri, 15 May 2020 at 21:41, Amanieu d'Antras <amanieu@gmail.com> wrote:
> On Fri, May 15, 2020 at 7:34 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > I was just looking at the signal code's handling of CPSR for a different
> > reason, and I noticed that at the moment we don't allow CPSR.E to be
> > updated from the signal frame when the signal handler returns
> > (because CPSR_USER doesn't contain CPSR_E and that's what we
> > use in restore_sigcontext() to define what bits from the frame we
> > allow updating). Don't you find that when the interrupted code
> > returns from the signal handler that it ends up running with the
> > wrong endianness (ie the endianness the handler used) ?
>
> I actually found this while trying to test the SETEND instruction
> under risu. The signal handler was crashing because it loaded a
> pointer with the wrong endianness, which was pretty obvious. However I
> missed the fact that code was now running with the wrong endianness
> after returning from the signal handler since I had both the master
> and the apprentice running under qemu-arm.
>
> > I'm going to fix this by putting CPSR_E in CPSR_USER, anyway.
>
> You also need to call arm_rebuild_hflags() after modifying CPSR_E
> otherwise the change doesn't take effect.

Hmm. I was expecting cpsr_write() to take care of that if we
updated a cpsr flag that was in the hflags, but it looks like
the rebuild_hflags() is in the HELPER() wrapper but not in
cpsr_write() itself. Richard, does anything go wrong if
cpsr_write() proper does the hflags rebuild ?

thanks
-- PMM

On 5/15/20 2:25 PM, Peter Maydell wrote:
>> You also need to call arm_rebuild_hflags() after modifying CPSR_E
>> otherwise the change doesn't take effect.
> 
> Hmm. I was expecting cpsr_write() to take care of that if we
> updated a cpsr flag that was in the hflags, but it looks like
> the rebuild_hflags() is in the HELPER() wrapper but not in
> cpsr_write() itself. Richard, does anything go wrong if
> cpsr_write() proper does the hflags rebuild ?

We wind up rebuilding hflags multiple times, is all.

Most of the time we call cpsr_write we also do something else that also
requires a rebuild.  So we do it once after all updates.

We could probably rearrange code such that the cpsr_write is last, and then
sink the rebuild.  But I don't know if that's clearer, hiding the side effect.


r~

On Sat, 16 May 2020 at 05:12, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/15/20 2:25 PM, Peter Maydell wrote:
> >> You also need to call arm_rebuild_hflags() after modifying CPSR_E
> >> otherwise the change doesn't take effect.
> >
> > Hmm. I was expecting cpsr_write() to take care of that if we
> > updated a cpsr flag that was in the hflags, but it looks like
> > the rebuild_hflags() is in the HELPER() wrapper but not in
> > cpsr_write() itself. Richard, does anything go wrong if
> > cpsr_write() proper does the hflags rebuild ?
>
> We wind up rebuilding hflags multiple times, is all.
>
> Most of the time we call cpsr_write we also do something else that also
> requires a rebuild.  So we do it once after all updates.

The downside is that it leaves a trap which makes it really
easy to introduce bugs where hflags aren't rebuilt: as
a caller of cpsr_write() I don't really want to have to
care which cpsr flags happen to be in the hflags or not,
and it's particularly awkward that simply fixing which
flags belong in CPSR_USER suddenly means that a call
that happened to be OK before is now buggy.

thanks
-- PMM

On 5/16/20 5:58 AM, Peter Maydell wrote:
> On Sat, 16 May 2020 at 05:12, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 5/15/20 2:25 PM, Peter Maydell wrote:
>>>> You also need to call arm_rebuild_hflags() after modifying CPSR_E
>>>> otherwise the change doesn't take effect.
>>>
>>> Hmm. I was expecting cpsr_write() to take care of that if we
>>> updated a cpsr flag that was in the hflags, but it looks like
>>> the rebuild_hflags() is in the HELPER() wrapper but not in
>>> cpsr_write() itself. Richard, does anything go wrong if
>>> cpsr_write() proper does the hflags rebuild ?
>>
>> We wind up rebuilding hflags multiple times, is all.
>>
>> Most of the time we call cpsr_write we also do something else that also
>> requires a rebuild.  So we do it once after all updates.
> 
> The downside is that it leaves a trap which makes it really
> easy to introduce bugs where hflags aren't rebuilt: as
> a caller of cpsr_write() I don't really want to have to
> care which cpsr flags happen to be in the hflags or not,
> and it's particularly awkward that simply fixing which
> flags belong in CPSR_USER suddenly means that a call
> that happened to be OK before is now buggy.

I don't see any way around that.

As I said, if we put the rebuild in cpsr_write, then we should also rearrange
the code that calls cpsr_write to assume that's where the rebuild gets done.


r~