[Qemu-devel] [PATCH 0/2] target-arm: ESR IL bit fixes

[Qemu-devel] [PATCH 0/2] target-arm: ESR IL bit fixes

[Peter Maydell]

These patches fix some problems with setting the IL bit in
ESR syndrome register values:
 * we were not setting IL for insn abort, watchpoint or swstep
   (which should all always have IL==1)
 * we were trying to set the IL bit in arm_cpu_do_interrupt_aarch64()
   if doing a 32->64 bit exception entry, which was wrong in
   several ways; instead we should just rely on exception.syndrome
   already having the correct IL bit value

The patches are intended to apply on top of target-arm.next:
 https://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next
(which has Edgar's patch which gets us correct IL bit values for
data abort exceptions).

Peter Maydell (2):
  target-arm: Set IL bit in syndromes for insn abort, watchpoint, swstep
  target-arm: Don't try to set ESR IL bit in
    arm_cpu_do_interrupt_aarch64()

 target-arm/helper.c    | 3 ---
 target-arm/internals.h | 6 +++---
 2 files changed, 3 insertions(+), 6 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] target-arm: Set IL bit in syndromes for insn abort, watchpoint, swstep

[Peter Maydell]

For some exception syndrome types, the IL bit should always be set.
This includes the instruction abort, watchpoint and software step
syndrome types; add the missing ARM_EL_IL bit to the syndrome
values returned by syn_insn_abort(), syn_swstep() and syn_watchpoint().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/internals.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/internals.h b/target-arm/internals.h
index 54a0fb1..7768a24 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -382,7 +382,7 @@ static inline uint32_t syn_fp_access_trap(int cv, int cond, bool is_16bit)
 static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc)
 {
     return (EC_INSNABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
-        | (ea << 9) | (s1ptw << 7) | fsc;
+        | ARM_EL_IL | (ea << 9) | (s1ptw << 7) | fsc;
 }
 
 static inline uint32_t syn_data_abort_no_iss(int same_el,
@@ -411,13 +411,13 @@ static inline uint32_t syn_data_abort_with_iss(int same_el,
 static inline uint32_t syn_swstep(int same_el, int isv, int ex)
 {
     return (EC_SOFTWARESTEP << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
-        | (isv << 24) | (ex << 6) | 0x22;
+        | ARM_EL_IL | (isv << 24) | (ex << 6) | 0x22;
 }
 
 static inline uint32_t syn_watchpoint(int same_el, int cm, int wnr)
 {
     return (EC_WATCHPOINT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
-        | (cm << 8) | (wnr << 6) | 0x22;
+        | ARM_EL_IL | (cm << 8) | (wnr << 6) | 0x22;
 }
 
 static inline uint32_t syn_breakpoint(int same_el)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] target-arm: Set IL bit in syndromes for insn abort, watchpoint, swstep

[Edgar E. Iglesias]

On Tue, May 17, 2016 at 01:14:17PM +0100, Peter Maydell wrote:
> For some exception syndrome types, the IL bit should always be set.
> This includes the instruction abort, watchpoint and software step
> syndrome types; add the missing ARM_EL_IL bit to the syndrome
> values returned by syn_insn_abort(), syn_swstep() and syn_watchpoint().


Hi,

Maybe we should have a reference in a comment to the table in
the pseudo code for AArch64.ExceptionClass?
It makes it a little easier to understand some of these settings...

Either way:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Cheers,
Edgar


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/internals.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 54a0fb1..7768a24 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -382,7 +382,7 @@ static inline uint32_t syn_fp_access_trap(int cv, int cond, bool is_16bit)
>  static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc)
>  {
>      return (EC_INSNABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
> -        | (ea << 9) | (s1ptw << 7) | fsc;
> +        | ARM_EL_IL | (ea << 9) | (s1ptw << 7) | fsc;
>  }
>  
>  static inline uint32_t syn_data_abort_no_iss(int same_el,
> @@ -411,13 +411,13 @@ static inline uint32_t syn_data_abort_with_iss(int same_el,
>  static inline uint32_t syn_swstep(int same_el, int isv, int ex)
>  {
>      return (EC_SOFTWARESTEP << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
> -        | (isv << 24) | (ex << 6) | 0x22;
> +        | ARM_EL_IL | (isv << 24) | (ex << 6) | 0x22;
>  }
>  
>  static inline uint32_t syn_watchpoint(int same_el, int cm, int wnr)
>  {
>      return (EC_WATCHPOINT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
> -        | (cm << 8) | (wnr << 6) | 0x22;
> +        | ARM_EL_IL | (cm << 8) | (wnr << 6) | 0x22;
>  }
>  
>  static inline uint32_t syn_breakpoint(int same_el)
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 1/2] target-arm: Set IL bit in syndromes for insn abort, watchpoint, swstep

[Peter Maydell]

On 17 May 2016 at 13:50, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> On Tue, May 17, 2016 at 01:14:17PM +0100, Peter Maydell wrote:
>> For some exception syndrome types, the IL bit should always be set.
>> This includes the instruction abort, watchpoint and software step
>> syndrome types; add the missing ARM_EL_IL bit to the syndrome
>> values returned by syn_insn_abort(), syn_swstep() and syn_watchpoint().

> Maybe we should have a reference in a comment to the table in
> the pseudo code for AArch64.ExceptionClass?
> It makes it a little easier to understand some of these settings...

I just used the text parts of the ARM ARM as reference for this one,
not the pseudocode (specifically, D7.2.27, the ESR_ELx register description,
has the definition of the IL bit and says which exceptions have IL set).
I think for pretty much any feature in the emulation you need to
look it up in the ARM ARM to understand what it's doing, and we
could end up with cross-references every other line...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] target-arm: Set IL bit in syndromes for insn abort, watchpoint, swstep

[Edgar E. Iglesias]

On Tue, May 17, 2016 at 02:06:28PM +0100, Peter Maydell wrote:
> On 17 May 2016 at 13:50, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> > On Tue, May 17, 2016 at 01:14:17PM +0100, Peter Maydell wrote:
> >> For some exception syndrome types, the IL bit should always be set.
> >> This includes the instruction abort, watchpoint and software step
> >> syndrome types; add the missing ARM_EL_IL bit to the syndrome
> >> values returned by syn_insn_abort(), syn_swstep() and syn_watchpoint().
> 
> > Maybe we should have a reference in a comment to the table in
> > the pseudo code for AArch64.ExceptionClass?
> > It makes it a little easier to understand some of these settings...
> 
> I just used the text parts of the ARM ARM as reference for this one,
> not the pseudocode (specifically, D7.2.27, the ESR_ELx register description,

Aha, I hadn't seen that text. I always end up in D1.10.4 where the IL
description is quite brief.


> has the definition of the IL bit and says which exceptions have IL set).
> I think for pretty much any feature in the emulation you need to
> look it up in the ARM ARM to understand what it's doing, and we
> could end up with cross-references every other line...

Yeah, fair enough.

Cheers,
Edgar

[Qemu-devel] [PATCH 2/2] target-arm: Don't try to set ESR IL bit in arm_cpu_do_interrupt_aarch64()

[Peter Maydell]

Remove some incorrect code from arm_cpu_do_interrupt_aarch64()
which attempts to set the IL bit in the syndrome register based
on the value of env->thumb. This is wrong in several ways:
 * IL doesn't indicate Thumb-vs-ARM, it indicates instruction
   length (which may be 16 or 32 for Thumb and is always 32 for ARM)
 * not every syndrome format uses IL like this -- for some IL is
   always set, and for some it is always clear
 * the code is changing esr_el[new_el] even for interrupt entry,
   which is not supposed to modify ESR_ELx at all

Delete the code, and instead rely on the syndrome value in
env->exception.syndrome having already been set up with the
correct value of IL.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d652c01..df65e68 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6349,9 +6349,6 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
         env->elr_el[new_el] = env->pc;
     } else {
         env->banked_spsr[aarch64_banked_spsr_index(new_el)] = cpsr_read(env);
-        if (!env->thumb) {
-            env->cp15.esr_el[new_el] |= 1 << 25;
-        }
         env->elr_el[new_el] = env->regs[15];
 
         aarch64_sync_32_to_64(env);
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] target-arm: Don't try to set ESR IL bit in arm_cpu_do_interrupt_aarch64()

[Edgar E. Iglesias]

On Tue, May 17, 2016 at 01:14:18PM +0100, Peter Maydell wrote:
> Remove some incorrect code from arm_cpu_do_interrupt_aarch64()
> which attempts to set the IL bit in the syndrome register based
> on the value of env->thumb. This is wrong in several ways:
>  * IL doesn't indicate Thumb-vs-ARM, it indicates instruction
>    length (which may be 16 or 32 for Thumb and is always 32 for ARM)
>  * not every syndrome format uses IL like this -- for some IL is
>    always set, and for some it is always clear
>  * the code is changing esr_el[new_el] even for interrupt entry,
>    which is not supposed to modify ESR_ELx at all
> 
> Delete the code, and instead rely on the syndrome value in
> env->exception.syndrome having already been set up with the
> correct value of IL.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d652c01..df65e68 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6349,9 +6349,6 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>          env->elr_el[new_el] = env->pc;
>      } else {
>          env->banked_spsr[aarch64_banked_spsr_index(new_el)] = cpsr_read(env);
> -        if (!env->thumb) {
> -            env->cp15.esr_el[new_el] |= 1 << 25;
> -        }
>          env->elr_el[new_el] = env->regs[15];
>  
>          aarch64_sync_32_to_64(env);
> -- 
> 1.9.1
>