Message ID | alpine.DEB.2.10.1710181439060.27209@sstabellini-ThinkPad-X260 |
---|---|
State | New |
Headers | show |
Series | fix WFI/WFE length in syndrome register | expand |
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
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;
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
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 --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; }
WFI/E are 4 bytes long: set ARM_EL_IL_SHIFT in the syndrome. Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>