[Qemu-devel] [PATCH for-2.12 0/4] Fix various BRK/BKPT related bugs

[Qemu-devel] [PATCH for-2.12 0/4] Fix various BRK/BKPT related bugs

[Peter Maydell]

The main aim of this patchset is to fix the bug reported in
https://bugs.launchpad.net/qemu/+bug/1756927 where we don't
report the correct FSR when a BKPT instruction causes us to
take an exception to AArch32 with LPAE enabled. The other
patches fix some minor bugs found along the way:

 * we weren't honouring MDCR_EL2.TDE when delivering an
   exception caused by a BRK or BKPT insn, so we'd always
   send it to EL1 even if it should have gone to EL2
 * our logic to decide whether to report a short-format or
   long-format LPAE for hardware breakpoint and watchpoint
   exceptions would get it wrong in some corner cases: we could
   select the short format because we're at EL0 and the EL1
   translation regime is not using LPAE, but then route the
   debug exception to EL2 because of MDCR_EL2.TDE and hand
   EL2 the wrong format FSR.
 * when taking a debug exception in AArch32, we would set the
   guest visible IFAR to whatever old value was lying around
   in env->exception.vaddress. IFAR in this situation is
   architecturally UNKNOWN, but letting it be possibly set
   to an old value that the guest shouldn't be able to see
   at its current security/exception level is not permitted,
   and setting it to a non-deterministic value is bad for
   record-and-replay.

(This patchset further reduces the set of places that are
calling the somewhat dubious extended_addresses_enabled()
function. The last callsite is in the CONTEXTIDR write
function, and that should probably be updated something along
the lines of the ideas I wrote in this email last year:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg491671.html
but that is a cleanup for another day.)

thanks
-- PMM

Peter Maydell (4):
  target/arm: Honour MDCR_EL2.TDE when routing exceptions due to
    BKPT/BRK
  target/arm: Factor out code to calculate FSR for debug exceptions
  target/arm: Set FSR for BKPT, BRK when raising exception
  target/arm: Always set FAR to a known unknown value for debug
    exceptions

 target/arm/helper.h        |  1 +
 target/arm/internals.h     | 25 +++++++++++++++++++++++++
 target/arm/helper.c        |  1 -
 target/arm/op_helper.c     | 33 ++++++++++++++++++++++-----------
 target/arm/translate-a64.c | 15 +++++++++++++--
 target/arm/translate.c     | 19 ++++++++++++++-----
 6 files changed, 75 insertions(+), 19 deletions(-)

-- 
2.16.2

[Qemu-arm] [PATCH for-2.12 1/4] target/arm: Honour MDCR_EL2.TDE when routing exceptions due to BKPT/BRK

[Peter Maydell]

The MDCR_EL2.TDE bit allows the exception level targeted by debug
exceptions to be set to EL2 for code executing at EL0.  We handle
this in the arm_debug_target_el() function, but this is only used for
hardware breakpoint and watchpoint exceptions, not for the exception
generated when the guest executes an AArch32 BKPT or AArch64 BRK
instruction.  We don't have enough information for a translate-time
equivalent of arm_debug_target_el(), so instead make BKPT and BRK
call a special purpose helper which can do the routing, rather than
the generic exception_with_syndrome helper.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.h        |  1 +
 target/arm/op_helper.c     |  8 ++++++++
 target/arm/translate-a64.c | 15 +++++++++++++--
 target/arm/translate.c     | 19 ++++++++++++++-----
 4 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h
index 0d2094f2be..34e8cc8904 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -47,6 +47,7 @@ DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE,
                    i32, i32, i32, i32)
 DEF_HELPER_2(exception_internal, void, env, i32)
 DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
+DEF_HELPER_2(exception_bkpt_insn, void, env, i32)
 DEF_HELPER_1(setend, void, env)
 DEF_HELPER_2(wfi, void, env, i32)
 DEF_HELPER_1(wfe, void, env)
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 7a88fd2c92..4b123d2bd6 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -483,6 +483,14 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
     raise_exception(env, excp, syndrome, target_el);
 }
 
+/* Raise an EXCP_BKPT with the specified syndrome register value,
+ * targeting the correct exception level for debug exceptions.
+ */
+void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
+{
+    raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
+}
+
 uint32_t HELPER(cpsr_read)(CPUARMState *env)
 {
     return cpsr_read(env) & ~(CPSR_EXEC | CPSR_RESERVED);
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 31ff0479e6..510951f7c7 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -321,6 +321,18 @@ static void gen_exception_insn(DisasContext *s, int offset, int excp,
     s->base.is_jmp = DISAS_NORETURN;
 }
 
+static void gen_exception_bkpt_insn(DisasContext *s, int offset,
+                                    uint32_t syndrome)
+{
+    TCGv_i32 tcg_syn;
+
+    gen_a64_set_pc_im(s->pc - offset);
+    tcg_syn = tcg_const_i32(syndrome);
+    gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
+    tcg_temp_free_i32(tcg_syn);
+    s->base.is_jmp = DISAS_NORETURN;
+}
+
 static void gen_ss_advance(DisasContext *s)
 {
     /* If the singlestep state is Active-not-pending, advance to
@@ -1839,8 +1851,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
             break;
         }
         /* BRK */
-        gen_exception_insn(s, 4, EXCP_BKPT, syn_aa64_bkpt(imm16),
-                           default_exception_el(s));
+        gen_exception_bkpt_insn(s, 4, syn_aa64_bkpt(imm16));
         break;
     case 2:
         if (op2_ll != 0) {
diff --git a/target/arm/translate.c b/target/arm/translate.c
index ba6ab7d287..68f0c585f4 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1248,6 +1248,18 @@ static void gen_exception_insn(DisasContext *s, int offset, int excp,
     s->base.is_jmp = DISAS_NORETURN;
 }
 
+static void gen_exception_bkpt_insn(DisasContext *s, int offset, int syn)
+{
+    TCGv_i32 tcg_syn;
+
+    gen_set_condexec(s);
+    gen_set_pc_im(s, s->pc - offset);
+    tcg_syn = tcg_const_i32(syn);
+    gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
+    tcg_temp_free_i32(tcg_syn);
+    s->base.is_jmp = DISAS_NORETURN;
+}
+
 /* Force a TB lookup after an instruction that changes the CPU state.  */
 static inline void gen_lookup_tb(DisasContext *s)
 {
@@ -8774,9 +8786,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
             case 1:
                 /* bkpt */
                 ARCH(5);
-                gen_exception_insn(s, 4, EXCP_BKPT,
-                                   syn_aa32_bkpt(imm16, false),
-                                   default_exception_el(s));
+                gen_exception_bkpt_insn(s, 4, syn_aa32_bkpt(imm16, false));
                 break;
             case 2:
                 /* Hypervisor call (v7) */
@@ -11983,8 +11993,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
         {
             int imm8 = extract32(insn, 0, 8);
             ARCH(5);
-            gen_exception_insn(s, 2, EXCP_BKPT, syn_aa32_bkpt(imm8, true),
-                               default_exception_el(s));
+            gen_exception_bkpt_insn(s, 2, syn_aa32_bkpt(imm8, true));
             break;
         }
 
-- 
2.16.2

Re: [Qemu-arm] [PATCH for-2.12 1/4] target/arm: Honour MDCR_EL2.TDE when routing exceptions due to BKPT/BRK

[Philippe Mathieu-Daudé]

Hi Peter,

On 03/20/2018 02:41 PM, Peter Maydell wrote:
> The MDCR_EL2.TDE bit allows the exception level targeted by debug
> exceptions to be set to EL2 for code executing at EL0.  We handle
> this in the arm_debug_target_el() function, but this is only used for
> hardware breakpoint and watchpoint exceptions, not for the exception
> generated when the guest executes an AArch32 BKPT or AArch64 BRK
> instruction.  We don't have enough information for a translate-time
> equivalent of arm_debug_target_el(), so instead make BKPT and BRK
> call a special purpose helper which can do the routing, rather than
> the generic exception_with_syndrome helper.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.h        |  1 +
>  target/arm/op_helper.c     |  8 ++++++++
>  target/arm/translate-a64.c | 15 +++++++++++++--
>  target/arm/translate.c     | 19 ++++++++++++++-----
>  4 files changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/helper.h b/target/arm/helper.h
> index 0d2094f2be..34e8cc8904 100644
> --- a/target/arm/helper.h
> +++ b/target/arm/helper.h
> @@ -47,6 +47,7 @@ DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE,
>                     i32, i32, i32, i32)
>  DEF_HELPER_2(exception_internal, void, env, i32)
>  DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
> +DEF_HELPER_2(exception_bkpt_insn, void, env, i32)
>  DEF_HELPER_1(setend, void, env)
>  DEF_HELPER_2(wfi, void, env, i32)
>  DEF_HELPER_1(wfe, void, env)
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 7a88fd2c92..4b123d2bd6 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -483,6 +483,14 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
>      raise_exception(env, excp, syndrome, target_el);
>  }
>  
> +/* Raise an EXCP_BKPT with the specified syndrome register value,
> + * targeting the correct exception level for debug exceptions.
> + */
> +void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
> +{
> +    raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
> +}
> +
>  uint32_t HELPER(cpsr_read)(CPUARMState *env)
>  {
>      return cpsr_read(env) & ~(CPSR_EXEC | CPSR_RESERVED);
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 31ff0479e6..510951f7c7 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -321,6 +321,18 @@ static void gen_exception_insn(DisasContext *s, int offset, int excp,
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>  
> +static void gen_exception_bkpt_insn(DisasContext *s, int offset,
> +                                    uint32_t syndrome)
> +{
> +    TCGv_i32 tcg_syn;
> +
> +    gen_a64_set_pc_im(s->pc - offset);
> +    tcg_syn = tcg_const_i32(syndrome);
> +    gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
> +    tcg_temp_free_i32(tcg_syn);
> +    s->base.is_jmp = DISAS_NORETURN;
> +}
> +
>  static void gen_ss_advance(DisasContext *s)
>  {
>      /* If the singlestep state is Active-not-pending, advance to
> @@ -1839,8 +1851,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>              break;
>          }
>          /* BRK */
> -        gen_exception_insn(s, 4, EXCP_BKPT, syn_aa64_bkpt(imm16),
> -                           default_exception_el(s));
> +        gen_exception_bkpt_insn(s, 4, syn_aa64_bkpt(imm16));
>          break;
>      case 2:
>          if (op2_ll != 0) {
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ba6ab7d287..68f0c585f4 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1248,6 +1248,18 @@ static void gen_exception_insn(DisasContext *s, int offset, int excp,
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>  
> +static void gen_exception_bkpt_insn(DisasContext *s, int offset, int syn)

Can you use the same Aa64 prototype when applying please?
(int syn -> uint32_t syndrome)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +{
> +    TCGv_i32 tcg_syn;
> +
> +    gen_set_condexec(s);
> +    gen_set_pc_im(s, s->pc - offset);
> +    tcg_syn = tcg_const_i32(syn);
> +    gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
> +    tcg_temp_free_i32(tcg_syn);
> +    s->base.is_jmp = DISAS_NORETURN;
> +}
> +
>  /* Force a TB lookup after an instruction that changes the CPU state.  */
>  static inline void gen_lookup_tb(DisasContext *s)
>  {
> @@ -8774,9 +8786,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>              case 1:
>                  /* bkpt */
>                  ARCH(5);
> -                gen_exception_insn(s, 4, EXCP_BKPT,
> -                                   syn_aa32_bkpt(imm16, false),
> -                                   default_exception_el(s));
> +                gen_exception_bkpt_insn(s, 4, syn_aa32_bkpt(imm16, false));
>                  break;
>              case 2:
>                  /* Hypervisor call (v7) */
> @@ -11983,8 +11993,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>          {
>              int imm8 = extract32(insn, 0, 8);
>              ARCH(5);
> -            gen_exception_insn(s, 2, EXCP_BKPT, syn_aa32_bkpt(imm8, true),
> -                               default_exception_el(s));
> +            gen_exception_bkpt_insn(s, 2, syn_aa32_bkpt(imm8, true));
>              break;
>          }
>  
> 

[Qemu-arm] [PATCH for-2.12 2/4] target/arm: Factor out code to calculate FSR for debug exceptions

[Peter Maydell]

When a debug exception is taken to AArch32, it appears as a Prefetch
Abort, and the Instruction Fault Status Register (IFSR) must be set.
The IFSR has two possible formats, depending on whether LPAE is in
use. Factor out the code in arm_debug_excp_handler() which picks
an FSR value into its own utility function, update it to use
arm_fi_to_lfsc() and arm_fi_to_sfsc() rather than hard-coded constants,
and use the correct condition to select long or short format.

In particular this fixes a bug where we could select the short
format because we're at EL0 and the EL1 translation regime is
not using LPAE, but then route the debug exception to EL2 because
of MDCR_EL2.TDE and hand EL2 the wrong format FSR.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h | 25 +++++++++++++++++++++++++
 target/arm/op_helper.c | 12 ++----------
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 47cc224a46..8ce944b7a0 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -763,4 +763,29 @@ static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
     }
 }
 
+/* Return the FSR value for a debug exception (watchpoint, hardware
+ * breakpoint or BKPT insn) targeting the specified exception level.
+ */
+static inline uint32_t arm_debug_exception_fsr(CPUARMState *env)
+{
+    ARMMMUFaultInfo fi = { .type = ARMFault_Debug };
+    int target_el = arm_debug_target_el(env);
+    bool using_lpae = false;
+
+    if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
+        using_lpae = true;
+    } else {
+        if (arm_feature(env, ARM_FEATURE_LPAE) &&
+            (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
+            using_lpae = true;
+        }
+    }
+
+    if (using_lpae) {
+        return arm_fi_to_lfsc(&fi);
+    } else {
+        return arm_fi_to_sfsc(&fi);
+    }
+}
+
 #endif
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 4b123d2bd6..75efff9edf 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -1330,11 +1330,7 @@ void arm_debug_excp_handler(CPUState *cs)
 
             cs->watchpoint_hit = NULL;
 
-            if (extended_addresses_enabled(env)) {
-                env->exception.fsr = (1 << 9) | 0x22;
-            } else {
-                env->exception.fsr = 0x2;
-            }
+            env->exception.fsr = arm_debug_exception_fsr(env);
             env->exception.vaddress = wp_hit->hitaddr;
             raise_exception(env, EXCP_DATA_ABORT,
                     syn_watchpoint(same_el, 0, wnr),
@@ -1354,11 +1350,7 @@ void arm_debug_excp_handler(CPUState *cs)
             return;
         }
 
-        if (extended_addresses_enabled(env)) {
-            env->exception.fsr = (1 << 9) | 0x22;
-        } else {
-            env->exception.fsr = 0x2;
-        }
+        env->exception.fsr = arm_debug_exception_fsr(env);
         /* FAR is UNKNOWN, so doesn't need setting */
         raise_exception(env, EXCP_PREFETCH_ABORT,
                         syn_breakpoint(same_el),
-- 
2.16.2

Re: [Qemu-arm] [PATCH for-2.12 2/4] target/arm: Factor out code to calculate FSR for debug exceptions

[Philippe Mathieu-Daudé]

Hi Peter,

This patch might be split in 2.

On 03/20/2018 02:41 PM, Peter Maydell wrote:
> When a debug exception is taken to AArch32, it appears as a Prefetch
> Abort, and the Instruction Fault Status Register (IFSR) must be set.
> The IFSR has two possible formats, depending on whether LPAE is in

^ intro

> use. Factor out the code in arm_debug_excp_handler() which picks
> an FSR value into its own utility function, update it to use
> arm_fi_to_lfsc() and arm_fi_to_sfsc() rather than hard-coded constants,

^ part 1 (refactor):
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> and use the correct condition to select long or short format.

^ part 2 (fix) looks correct:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> In particular this fixes a bug where we could select the short
> format because we're at EL0 and the EL1 translation regime is
> not using LPAE, but then route the debug exception to EL2 because
> of MDCR_EL2.TDE and hand EL2 the wrong format FSR.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/internals.h | 25 +++++++++++++++++++++++++
>  target/arm/op_helper.c | 12 ++----------
>  2 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 47cc224a46..8ce944b7a0 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -763,4 +763,29 @@ static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
>      }
>  }
>  
> +/* Return the FSR value for a debug exception (watchpoint, hardware
> + * breakpoint or BKPT insn) targeting the specified exception level.
> + */
> +static inline uint32_t arm_debug_exception_fsr(CPUARMState *env)
> +{
> +    ARMMMUFaultInfo fi = { .type = ARMFault_Debug };
> +    int target_el = arm_debug_target_el(env);
> +    bool using_lpae = false;
> +
> +    if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
> +        using_lpae = true;
> +    } else {
> +        if (arm_feature(env, ARM_FEATURE_LPAE) &&
> +            (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
> +            using_lpae = true;
> +        }
> +    }

This looks pretty similar to regime_using_lpae_format() but for
ARMFault_Debug.

> +
> +    if (using_lpae) {
> +        return arm_fi_to_lfsc(&fi);
> +    } else {
> +        return arm_fi_to_sfsc(&fi);
> +    }
> +}
> +
>  #endif
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 4b123d2bd6..75efff9edf 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -1330,11 +1330,7 @@ void arm_debug_excp_handler(CPUState *cs)
>  
>              cs->watchpoint_hit = NULL;
>  
> -            if (extended_addresses_enabled(env)) {
> -                env->exception.fsr = (1 << 9) | 0x22;
> -            } else {
> -                env->exception.fsr = 0x2;
> -            }
> +            env->exception.fsr = arm_debug_exception_fsr(env);
>              env->exception.vaddress = wp_hit->hitaddr;
>              raise_exception(env, EXCP_DATA_ABORT,
>                      syn_watchpoint(same_el, 0, wnr),
> @@ -1354,11 +1350,7 @@ void arm_debug_excp_handler(CPUState *cs)
>              return;
>          }
>  
> -        if (extended_addresses_enabled(env)) {
> -            env->exception.fsr = (1 << 9) | 0x22;
> -        } else {
> -            env->exception.fsr = 0x2;
> -        }
> +        env->exception.fsr = arm_debug_exception_fsr(env);
>          /* FAR is UNKNOWN, so doesn't need setting */
>          raise_exception(env, EXCP_PREFETCH_ABORT,
>                          syn_breakpoint(same_el),
> 

Re: [Qemu-arm] [PATCH for-2.12 2/4] target/arm: Factor out code to calculate FSR for debug exceptions

[Peter Maydell]

On 21 March 2018 at 22:26, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> This patch might be split in 2.

For a +27-10 patch, it doesn't really seem necessary.

> On 03/20/2018 02:41 PM, Peter Maydell wrote:
>> When a debug exception is taken to AArch32, it appears as a Prefetch
>> Abort, and the Instruction Fault Status Register (IFSR) must be set.
>> The IFSR has two possible formats, depending on whether LPAE is in
>
> ^ intro
>
>> use. Factor out the code in arm_debug_excp_handler() which picks
>> an FSR value into its own utility function, update it to use
>> arm_fi_to_lfsc() and arm_fi_to_sfsc() rather than hard-coded constants,
>
> ^ part 1 (refactor):
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> and use the correct condition to select long or short format.
>
> ^ part 2 (fix) looks correct:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>>
>> In particular this fixes a bug where we could select the short
>> format because we're at EL0 and the EL1 translation regime is
>> not using LPAE, but then route the debug exception to EL2 because
>> of MDCR_EL2.TDE and hand EL2 the wrong format FSR.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/arm/internals.h | 25 +++++++++++++++++++++++++
>>  target/arm/op_helper.c | 12 ++----------
>>  2 files changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 47cc224a46..8ce944b7a0 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -763,4 +763,29 @@ static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
>>      }
>>  }
>>
>> +/* Return the FSR value for a debug exception (watchpoint, hardware
>> + * breakpoint or BKPT insn) targeting the specified exception level.
>> + */
>> +static inline uint32_t arm_debug_exception_fsr(CPUARMState *env)
>> +{
>> +    ARMMMUFaultInfo fi = { .type = ARMFault_Debug };
>> +    int target_el = arm_debug_target_el(env);
>> +    bool using_lpae = false;
>> +
>> +    if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
>> +        using_lpae = true;
>> +    } else {
>> +        if (arm_feature(env, ARM_FEATURE_LPAE) &&
>> +            (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
>> +            using_lpae = true;
>> +        }
>> +    }
>
> This looks pretty similar to regime_using_lpae_format() but for
> ARMFault_Debug.

Yeah, it's basically similar logic.

I'm a bit confused overall -- are you giving a reviewed-by for
this patch, or do you want changes?

thanks
-- PMM

[Qemu-arm] [PATCH for-2.12 3/4] target/arm: Set FSR for BKPT, BRK when raising exception

[Peter Maydell]

Now that we have a helper function specifically for the BRK and
BKPT instructions, we can set the exception.fsr there rather
than in arm_cpu_do_interrupt_aarch32(). This allows us to
use our new arm_debug_exception_fsr() helper.

In particular this fixes a bug where we were hardcoding the
short-form IFSR value, which is wrong if the target exception
level has LPAE enabled.

Fixes: https://bugs.launchpad.net/qemu/+bug/1756927
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c    | 1 -
 target/arm/op_helper.c | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 09893e3f72..dcb8476d9e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7910,7 +7910,6 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
         offset = 0;
         break;
     case EXCP_BKPT:
-        env->exception.fsr = 2;
         /* Fall through to prefetch abort.  */
     case EXCP_PREFETCH_ABORT:
         A32_BANKED_CURRENT_REG_SET(env, ifsr, env->exception.fsr);
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 75efff9edf..8e1e521193 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -488,6 +488,8 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
  */
 void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
 {
+    /* FSR will only be used if the debug target EL is AArch32. */
+    env->exception.fsr = arm_debug_exception_fsr(env);
     raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
 }
 
-- 
2.16.2

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12 3/4] target/arm: Set FSR for BKPT, BRK when raising exception

[Philippe Mathieu-Daudé]

On 03/20/2018 10:41 AM, Peter Maydell wrote:
> Now that we have a helper function specifically for the BRK and
> BKPT instructions, we can set the exception.fsr there rather
> than in arm_cpu_do_interrupt_aarch32(). This allows us to
> use our new arm_debug_exception_fsr() helper.
> 
> In particular this fixes a bug where we were hardcoding the
> short-form IFSR value, which is wrong if the target exception
> level has LPAE enabled.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1756927
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/helper.c    | 1 -
>  target/arm/op_helper.c | 2 ++
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 09893e3f72..dcb8476d9e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7910,7 +7910,6 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
>          offset = 0;
>          break;
>      case EXCP_BKPT:
> -        env->exception.fsr = 2;
>          /* Fall through to prefetch abort.  */
>      case EXCP_PREFETCH_ABORT:
>          A32_BANKED_CURRENT_REG_SET(env, ifsr, env->exception.fsr);
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 75efff9edf..8e1e521193 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -488,6 +488,8 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
>   */
>  void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
>  {
> +    /* FSR will only be used if the debug target EL is AArch32. */
> +    env->exception.fsr = arm_debug_exception_fsr(env);
>      raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
>  }
>  
> 

[Qemu-devel] [PATCH for-2.12 4/4] target/arm: Always set FAR to a known unknown value for debug exceptions

[Peter Maydell]

For debug exceptions due to breakpoints or the BKPT instruction which
are taken to AArch32, the Fault Address Register is architecturally
UNKNOWN.  We were using that as license to simply not set
env->exception.vaddress, but this isn't correct, because it will
expose to the guest whatever old value was in that field when
arm_cpu_do_interrupt_aarch32() writes it to the guest IFSR.  That old
value might be a FAR for a previous guest EL2 or secure exception, in
which case we shouldn't show it to an EL1 or non-secure exception
handler. It might also be a non-deterministic value, which is bad
for record-and-replay.

Clear env->exception.vaddress before taking breakpoint debug
exceptions, to avoid this minor information leak.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/op_helper.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 8e1e521193..a266cc0116 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -490,6 +490,11 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
 {
     /* FSR will only be used if the debug target EL is AArch32. */
     env->exception.fsr = arm_debug_exception_fsr(env);
+    /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing
+     * values to the guest that it shouldn't be able to see at its
+     * exception/security level.
+     */
+    env->exception.vaddress = 0;
     raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
 }
 
@@ -1353,7 +1358,11 @@ void arm_debug_excp_handler(CPUState *cs)
         }
 
         env->exception.fsr = arm_debug_exception_fsr(env);
-        /* FAR is UNKNOWN, so doesn't need setting */
+        /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing
+         * values to the guest that it shouldn't be able to see at its
+         * exception/security level.
+         */
+        env->exception.vaddress = 0;
         raise_exception(env, EXCP_PREFETCH_ABORT,
                         syn_breakpoint(same_el),
                         arm_debug_target_el(env));
-- 
2.16.2

Re: [Qemu-arm] [Qemu-devel] [PATCH for-2.12 4/4] target/arm: Always set FAR to a known unknown value for debug exceptions

[Philippe Mathieu-Daudé]

On 03/20/2018 02:41 PM, Peter Maydell wrote:
> For debug exceptions due to breakpoints or the BKPT instruction which
> are taken to AArch32, the Fault Address Register is architecturally
> UNKNOWN.  We were using that as license to simply not set
> env->exception.vaddress, but this isn't correct, because it will
> expose to the guest whatever old value was in that field when
> arm_cpu_do_interrupt_aarch32() writes it to the guest IFSR.  That old
> value might be a FAR for a previous guest EL2 or secure exception, in
> which case we shouldn't show it to an EL1 or non-secure exception
> handler. It might also be a non-deterministic value, which is bad
> for record-and-replay.
> 
> Clear env->exception.vaddress before taking breakpoint debug
> exceptions, to avoid this minor information leak.

So this series is worth Cc'ing qemu-stable...?

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/op_helper.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 8e1e521193..a266cc0116 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -490,6 +490,11 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
>  {
>      /* FSR will only be used if the debug target EL is AArch32. */
>      env->exception.fsr = arm_debug_exception_fsr(env);
> +    /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing
> +     * values to the guest that it shouldn't be able to see at its
> +     * exception/security level.
> +     */
> +    env->exception.vaddress = 0;
>      raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
>  }
>  
> @@ -1353,7 +1358,11 @@ void arm_debug_excp_handler(CPUState *cs)
>          }
>  
>          env->exception.fsr = arm_debug_exception_fsr(env);
> -        /* FAR is UNKNOWN, so doesn't need setting */
> +        /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing
> +         * values to the guest that it shouldn't be able to see at its
> +         * exception/security level.
> +         */
> +        env->exception.vaddress = 0;
>          raise_exception(env, EXCP_PREFETCH_ABORT,
>                          syn_breakpoint(same_el),
>                          arm_debug_target_el(env));
> 

Re: [Qemu-devel] [PATCH for-2.12 4/4] target/arm: Always set FAR to a known unknown value for debug exceptions

[Peter Maydell]

On 21 March 2018 at 22:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 03/20/2018 02:41 PM, Peter Maydell wrote:
>> For debug exceptions due to breakpoints or the BKPT instruction which
>> are taken to AArch32, the Fault Address Register is architecturally
>> UNKNOWN.  We were using that as license to simply not set
>> env->exception.vaddress, but this isn't correct, because it will
>> expose to the guest whatever old value was in that field when
>> arm_cpu_do_interrupt_aarch32() writes it to the guest IFSR.  That old
>> value might be a FAR for a previous guest EL2 or secure exception, in
>> which case we shouldn't show it to an EL1 or non-secure exception
>> handler. It might also be a non-deterministic value, which is bad
>> for record-and-replay.
>>
>> Clear env->exception.vaddress before taking breakpoint debug
>> exceptions, to avoid this minor information leak.
>
> So this series is worth Cc'ing qemu-stable...?

I don't think so. This isn't a regression -- we've behaved this
way ever since we introduced LPAE support, five years or so ago.
In particular Linux doesn't care about it.

thanks
-- PMM