diff mbox series

fix WFI/WFE length in syndrome register

Message ID alpine.DEB.2.10.1710181439060.27209@sstabellini-ThinkPad-X260
State New
Headers show
Series fix WFI/WFE length in syndrome register | expand

Commit Message

Stefano Stabellini Oct. 18, 2017, 10:03 p.m. UTC 
WFI/E are 4 bytes long: set ARM_EL_IL_SHIFT in the syndrome.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

Comments

Peter Maydell Oct. 19, 2017, 2:56 p.m. UTC | #1 
On 18 October 2017 at 23:03, Stefano Stabellini <sstabellini@kernel.org> wrote:
> WFI/E are 4 bytes long: set ARM_EL_IL_SHIFT in the syndrome.
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 1f6efef..cf8c966 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -398,6 +398,7 @@ static inline uint32_t syn_breakpoint(int same_el)
>  static inline uint32_t syn_wfx(int cv, int cond, int ti)
>  {
>      return (EC_WFX_TRAP << ARM_EL_EC_SHIFT) |
> +           (1 << ARM_EL_IL_SHIFT) |
>             (cv << 24) | (cond << 20) | ti;
>  }

Hmm. What we do now is definitely wrong, but WFI and WFE can be 2 bytes:
there is a T1 Thumb encoding that is 2 bytes.

HELPER(wfi) doesn't get that right, though:
    if (target_el) {
        env->pc -= 4;
        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
    }

So I think that HELPER(wfi) needs to be passed an extra
parameter is_16bit, which it can then use both in its adjustment
of env->pc and to pass as an extra parameter to syn_wfx(),
which is then syn_wfx(int cv, int cond, int ti, bool is_16bit).

(In theory HELPER(wfe) should also be passed is_16bit, but
since it doesn't currently ever raise an exception it
doesn't matter.)

thanks
-- PMM
Stefano Stabellini Oct. 19, 2017, 8:56 p.m. UTC | #2 
On Thu, 19 Oct 2017, Peter Maydell wrote:
> On 18 October 2017 at 23:03, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > WFI/E are 4 bytes long: set ARM_EL_IL_SHIFT in the syndrome.
> >
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> >
> > diff --git a/target/arm/internals.h b/target/arm/internals.h
> > index 1f6efef..cf8c966 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -398,6 +398,7 @@ static inline uint32_t syn_breakpoint(int same_el)
> >  static inline uint32_t syn_wfx(int cv, int cond, int ti)
> >  {
> >      return (EC_WFX_TRAP << ARM_EL_EC_SHIFT) |
> > +           (1 << ARM_EL_IL_SHIFT) |
> >             (cv << 24) | (cond << 20) | ti;
> >  }
> 
> Hmm. What we do now is definitely wrong, but WFI and WFE can be 2 bytes:
> there is a T1 Thumb encoding that is 2 bytes.
> 
> HELPER(wfi) doesn't get that right, though:
>     if (target_el) {
>         env->pc -= 4;
>         raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
>     }
> 
> So I think that HELPER(wfi) needs to be passed an extra
> parameter is_16bit, which it can then use both in its adjustment
> of env->pc and to pass as an extra parameter to syn_wfx(),
> which is then syn_wfx(int cv, int cond, int ti, bool is_16bit).
> 
> (In theory HELPER(wfe) should also be passed is_16bit, but
> since it doesn't currently ever raise an exception it
> doesn't matter.)

Wouldn't it be better to just check on env->thumb like
HELPER(cpsr_write_eret) for example?

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 670c07a..a451763 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 43106a2..55c70b4 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -428,9 +428,10 @@ static inline uint32_t syn_breakpoint(int same_el)
         | ARM_EL_IL | 0x22;
 }
 
-static inline uint32_t syn_wfx(int cv, int cond, int ti)
+static inline uint32_t syn_wfx(int cv, int cond, int ti, bool is_16bit)
 {
     return (EC_WFX_TRAP << ARM_EL_EC_SHIFT) |
+           (is_16bit ? 0 : (1 << ARM_EL_IL_SHIFT)) |
            (cv << 24) | (cond << 20) | ti;
 }
 
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 3914145..ea16c9a 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -476,8 +476,8 @@ void HELPER(wfi)(CPUARMState *env)
     }
 
     if (target_el) {
-        env->pc -= 4;
-        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
+        env->pc -= env->thumb ? 2 : 4;
+        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0, env->thumb), target_el);
     }
 
     cs->exception_index = EXCP_HLT;
Peter Maydell Oct. 20, 2017, 9:30 a.m. UTC | #3 
On 19 October 2017 at 21:56, Stefano Stabellini <sstabellini@kernel.org> wrote:
> On Thu, 19 Oct 2017, Peter Maydell wrote:
>> On 18 October 2017 at 23:03, Stefano Stabellini <sstabellini@kernel.org> wrote:
>> > WFI/E are 4 bytes long: set ARM_EL_IL_SHIFT in the syndrome.
>> >
>> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>> >
>> > diff --git a/target/arm/internals.h b/target/arm/internals.h
>> > index 1f6efef..cf8c966 100644
>> > --- a/target/arm/internals.h
>> > +++ b/target/arm/internals.h
>> > @@ -398,6 +398,7 @@ static inline uint32_t syn_breakpoint(int same_el)
>> >  static inline uint32_t syn_wfx(int cv, int cond, int ti)
>> >  {
>> >      return (EC_WFX_TRAP << ARM_EL_EC_SHIFT) |
>> > +           (1 << ARM_EL_IL_SHIFT) |
>> >             (cv << 24) | (cond << 20) | ti;
>> >  }
>>
>> Hmm. What we do now is definitely wrong, but WFI and WFE can be 2 bytes:
>> there is a T1 Thumb encoding that is 2 bytes.
>>
>> HELPER(wfi) doesn't get that right, though:
>>     if (target_el) {
>>         env->pc -= 4;
>>         raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
>>     }
>>
>> So I think that HELPER(wfi) needs to be passed an extra
>> parameter is_16bit, which it can then use both in its adjustment
>> of env->pc and to pass as an extra parameter to syn_wfx(),
>> which is then syn_wfx(int cv, int cond, int ti, bool is_16bit).
>>
>> (In theory HELPER(wfe) should also be passed is_16bit, but
>> since it doesn't currently ever raise an exception it
>> doesn't matter.)
>
> Wouldn't it be better to just check on env->thumb like
> HELPER(cpsr_write_eret) for example?

> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 3914145..ea16c9a 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -476,8 +476,8 @@ void HELPER(wfi)(CPUARMState *env)
>      }
>
>      if (target_el) {
> -        env->pc -= 4;
> -        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
> +        env->pc -= env->thumb ? 2 : 4;
> +        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0, env->thumb), target_el);
>      }
>
>      cs->exception_index = EXCP_HLT;

This doesn't work, because there is also a 32 bit Thumb
encoding of WFI. env->thumb says "we are in thumb mode",
which isn't the same as "this insn is 16 bits".

thanks
-- PMM
Stefano Stabellini Oct. 21, 2017, 6:05 p.m. UTC | #4 
On Fri, 20 Oct 2017, Peter Maydell wrote:
> On 19 October 2017 at 21:56, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > On Thu, 19 Oct 2017, Peter Maydell wrote:
> >> On 18 October 2017 at 23:03, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >> > WFI/E are 4 bytes long: set ARM_EL_IL_SHIFT in the syndrome.
> >> >
> >> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> >> >
> >> > diff --git a/target/arm/internals.h b/target/arm/internals.h
> >> > index 1f6efef..cf8c966 100644
> >> > --- a/target/arm/internals.h
> >> > +++ b/target/arm/internals.h
> >> > @@ -398,6 +398,7 @@ static inline uint32_t syn_breakpoint(int same_el)
> >> >  static inline uint32_t syn_wfx(int cv, int cond, int ti)
> >> >  {
> >> >      return (EC_WFX_TRAP << ARM_EL_EC_SHIFT) |
> >> > +           (1 << ARM_EL_IL_SHIFT) |
> >> >             (cv << 24) | (cond << 20) | ti;
> >> >  }
> >>
> >> Hmm. What we do now is definitely wrong, but WFI and WFE can be 2 bytes:
> >> there is a T1 Thumb encoding that is 2 bytes.
> >>
> >> HELPER(wfi) doesn't get that right, though:
> >>     if (target_el) {
> >>         env->pc -= 4;
> >>         raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
> >>     }
> >>
> >> So I think that HELPER(wfi) needs to be passed an extra
> >> parameter is_16bit, which it can then use both in its adjustment
> >> of env->pc and to pass as an extra parameter to syn_wfx(),
> >> which is then syn_wfx(int cv, int cond, int ti, bool is_16bit).
> >>
> >> (In theory HELPER(wfe) should also be passed is_16bit, but
> >> since it doesn't currently ever raise an exception it
> >> doesn't matter.)
> >
> > Wouldn't it be better to just check on env->thumb like
> > HELPER(cpsr_write_eret) for example?
> 
> > diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> > index 3914145..ea16c9a 100644
> > --- a/target/arm/op_helper.c
> > +++ b/target/arm/op_helper.c
> > @@ -476,8 +476,8 @@ void HELPER(wfi)(CPUARMState *env)
> >      }
> >
> >      if (target_el) {
> > -        env->pc -= 4;
> > -        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0), target_el);
> > +        env->pc -= env->thumb ? 2 : 4;
> > +        raise_exception(env, EXCP_UDEF, syn_wfx(1, 0xe, 0, env->thumb), target_el);
> >      }
> >
> >      cs->exception_index = EXCP_HLT;
> 
> This doesn't work, because there is also a 32 bit Thumb
> encoding of WFI. env->thumb says "we are in thumb mode",
> which isn't the same as "this insn is 16 bits".

OK, I understand your suggestion now. I'll send a new patch.
diff mbox series

Patch

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 1f6efef..cf8c966 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -398,6 +398,7 @@  static inline uint32_t syn_breakpoint(int same_el)
 static inline uint32_t syn_wfx(int cv, int cond, int ti)
 {
     return (EC_WFX_TRAP << ARM_EL_EC_SHIFT) |
+           (1 << ARM_EL_IL_SHIFT) |
            (cv << 24) | (cond << 20) | ti;
 }