[Qemu-devel] [PATCH v2 0/6] KVM Guest Debug fixes (plus TCG EL2 debug tweaks)

[Qemu-devel] [PATCH v2 0/6] KVM Guest Debug fixes (plus TCG EL2 debug tweaks)

[Alex Bennée]

Hi,

These are fixes for guest debug when running under KVM. While
re-spinning these I came across an anomaly which pointed to a kernel
bug that caused the 1st single-step to fail. This is being discussed
on the kvm-arm list:

  Subject: [RFC PATCH] KVM: arm64: don't single-step for non-emulated faults
  Date: Wed, 7 Nov 2018 17:10:31 +0000
  Message-Id: <20181107171031.22573-1-alex.bennee@linaro.org>

As debugging HYP mode code is next to impossible on real hardware I
tried re-creating the single-step bug under TCG. As a result I ran into
some debug and EL2 cases that failed. The final two patches are some
fixes but I'm still seeing some weird behaviour although it is currently
obscured by timer interrupts constantly firing as I enter the to be
single-stepped guest EL1 instruction so they can probably be skipped for
3.1.

Alex Bennée (6):
  target/arm64: properly handle DBGVR RESS bits
  target/arm64: hold BQL when calling do_interrupt()
  target/arm64: kvm debug set target_el when passing exception to guest
  tests/guest-debug: fix scoping of failcount
  arm: use symbolic MDCR_TDE in arm_debug_target_el
  arm: fix aa64_generate_debug_exceptions to work with EL2

 target/arm/cpu.h                  | 29 ++++++++++++++++++-----------
 target/arm/kvm64.c                | 20 ++++++++++++++++++--
 tests/guest-debug/test-gdbstub.py |  1 +
 3 files changed, 37 insertions(+), 13 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/6] target/arm64: properly handle DBGVR RESS bits

[Alex Bennée]

This only fails with some (broken) versions of gdb but we should
treat the top bits of DBGBVR as RESS. Properly sign extend QEMU's
reference copy of dbgbvr and also update the register descriptions in
the comment.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - sanitise register on insertion
  - update reference description
---
 target/arm/kvm64.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 5de8ff0ac5..b92ce3437f 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -103,7 +103,7 @@ static void kvm_arm_init_debug(CPUState *cs)
  * capable of fancier matching but that will require exposing that
  * fanciness to GDB's interface
  *
- * D7.3.2 DBGBCR<n>_EL1, Debug Breakpoint Control Registers
+ * DBGBCR<n>_EL1, Debug Breakpoint Control Registers
  *
  *  31  24 23  20 19   16 15 14  13  12   9 8   5 4    3 2   1  0
  * +------+------+-------+-----+----+------+-----+------+-----+---+
@@ -115,12 +115,25 @@ static void kvm_arm_init_debug(CPUState *cs)
  * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
  * BAS: Byte Address Select (RES1 for AArch64)
  * E: Enable bit
+ *
+ * DBGBVR<n>_EL1, Debug Breakpoint Value Registers
+ *
+ *  63  53 52       49 48       2  1 0
+ * +------+-----------+----------+-----+
+ * | RESS | VA[52:49] | VA[48:2] | 0 0 |
+ * +------+-----------+----------+-----+
+ *
+ * Depending on the addressing mode bits the top bits of the register
+ * are a sign extension of the highest applicable VA bit. Some
+ * versions of GDB don't do it correctly so we ensure they are correct
+ * here so future PC comparisons will work properly.
  */
+
 static int insert_hw_breakpoint(target_ulong addr)
 {
     HWBreakpoint brk = {
         .bcr = 0x1,                             /* BCR E=1, enable */
-        .bvr = addr
+        .bvr = sextract64(addr, 52, 53)
     };
 
     if (cur_hw_bps >= max_hw_bps) {
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/6] target/arm64: hold BQL when calling do_interrupt()

[Alex Bennée]

Fix the assertion failure when running interrupts.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/kvm64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index b92ce3437f..03b0f78831 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1000,7 +1000,9 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
     cs->exception_index = EXCP_BKPT;
     env->exception.syndrome = debug_exit->hsr;
     env->exception.vaddress = debug_exit->far;
+    qemu_mutex_lock_iothread();
     cc->do_interrupt(cs);
+    qemu_mutex_unlock_iothread();
 
     return false;
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/6] target/arm64: kvm debug set target_el when passing exception to guest

[Alex Bennée]

When we are debugging the guest all exceptions come our way but might
be for the guest's own debug exceptions. We use the ->do_interrupt()
infrastructure to inject the exception into the guest. However, we are
missing a full setup of the exception structure, causing an assert
later down the line.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

---
v2
  - tweak commit msg for grammar
---
 target/arm/kvm64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 03b0f78831..bf7824d862 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1000,6 +1000,7 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
     cs->exception_index = EXCP_BKPT;
     env->exception.syndrome = debug_exit->hsr;
     env->exception.vaddress = debug_exit->far;
+    env->exception.target_el = 1;
     qemu_mutex_lock_iothread();
     cc->do_interrupt(cs);
     qemu_mutex_unlock_iothread();
-- 
2.17.1

[Qemu-devel] [PATCH v2 4/6] tests/guest-debug: fix scoping of failcount

[Alex Bennée]

You should declare you are using a global version of a variable before
you attempt to modify it in a function.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/guest-debug/test-gdbstub.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py
index 0e4ac01426..c7e3986a24 100644
--- a/tests/guest-debug/test-gdbstub.py
+++ b/tests/guest-debug/test-gdbstub.py
@@ -16,6 +16,7 @@ def report(cond, msg):
         print ("PASS: %s" % (msg))
     else:
         print ("FAIL: %s" % (msg))
+        global failcount
         failcount += 1
 
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 5/6] arm: use symbolic MDCR_TDE in arm_debug_target_el

[Alex Bennée]

We already have this symbol defined so lets use it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b5eff79f73..1efff21a18 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2743,7 +2743,7 @@ static inline int arm_debug_target_el(CPUARMState *env)
 
     if (arm_feature(env, ARM_FEATURE_EL2) && !secure) {
         route_to_el2 = env->cp15.hcr_el2 & HCR_TGE ||
-                       env->cp15.mdcr_el2 & (1 << 8);
+                       env->cp15.mdcr_el2 & MDCR_TDE;
     }
 
     if (route_to_el2) {
-- 
2.17.1

[Qemu-devel] [PATCH v2 6/6] arm: fix aa64_generate_debug_exceptions to work with EL2

[Alex Bennée]

The test was incomplete and incorrectly caused debug exceptions to be
generated when returning to EL2 after a failed attempt to single-step
an EL1 instruction. Fix this while cleaning up the function a little.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/cpu.h | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 1efff21a18..a6d8eb14f6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2764,23 +2764,33 @@ static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu)
     return (cpu->clidr & R_V7M_CLIDR_CTYPE_ALL_MASK) != 0;
 }
 
+/* See AArch64.GenerateDebugExceptionsFrom() in ARM ARM pseudocode */
 static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
 {
+    int cur_el = arm_current_el(env);
+    int debug_el;
+
     if (arm_is_secure(env)) {
         /* MDCR_EL3.SDD disables debug events from Secure state */
         if (extract32(env->cp15.mdcr_el3, 16, 1) != 0
-            || arm_current_el(env) == 3) {
+            || cur_el == 3) {
             return false;
         }
     }
 
-    if (arm_current_el(env) == arm_debug_target_el(env)) {
-        if ((extract32(env->cp15.mdscr_el1, 13, 1) == 0)
-            || (env->daif & PSTATE_D)) {
-            return false;
-        }
+    /*
+     * Same EL to same EL debug exceptions need MDSCR_KDE enabled
+     * while not masking the (D)ebug bit in DAIF.
+     */
+    debug_el = arm_debug_target_el(env);
+
+    if (cur_el == debug_el) {
+        return extract32(env->cp15.mdscr_el1, 13, 1)
+            && !(env->daif & PSTATE_D);
     }
-    return true;
+
+    /* Otherwise the debug target needs to be a higher EL */
+    return debug_el > cur_el;
 }
 
 static inline bool aa32_generate_debug_exceptions(CPUARMState *env)
@@ -2833,9 +2843,6 @@ static inline bool aa32_generate_debug_exceptions(CPUARMState *env)
  * since the pseudocode has it at all callsites except for the one in
  * CheckSoftwareStep(), where it is elided because both branches would
  * always return the same value.
- *
- * Parts of the pseudocode relating to EL2 and EL3 are omitted because we
- * don't yet implement those exception levels or their associated trap bits.
  */
 static inline bool arm_generate_debug_exceptions(CPUARMState *env)
 {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 1/6] target/arm64: properly handle DBGVR RESS bits

[Richard Henderson]

On 11/8/18 5:33 PM, Alex Bennée wrote:
> -        .bvr = addr
> +        .bvr = sextract64(addr, 52, 53)

I think you meant sextract64(addr, 0, 53).
What you wrote *should* have asserted, since 52+53 > 64.


r~

Re: [Qemu-devel] [PATCH v2 2/6] target/arm64: hold BQL when calling do_interrupt()

[Richard Henderson]

On 11/8/18 5:33 PM, Alex Bennée wrote:
> Fix the assertion failure when running interrupts.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/kvm64.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 3/6] target/arm64: kvm debug set target_el when passing exception to guest

[Richard Henderson]

On 11/8/18 5:33 PM, Alex Bennée wrote:
> When we are debugging the guest all exceptions come our way but might
> be for the guest's own debug exceptions. We use the ->do_interrupt()
> infrastructure to inject the exception into the guest. However, we are
> missing a full setup of the exception structure, causing an assert
> later down the line.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 4/6] tests/guest-debug: fix scoping of failcount

[Richard Henderson]

On 11/8/18 5:33 PM, Alex Bennée wrote:
> @@ -16,6 +16,7 @@ def report(cond, msg):
>          print ("PASS: %s" % (msg))
>      else:
>          print ("FAIL: %s" % (msg))
> +        global failcount
>          failcount += 1

Do we usually prefer such declarations at the start of the function?
Anyway,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 5/6] arm: use symbolic MDCR_TDE in arm_debug_target_el

[Richard Henderson]

On 11/8/18 5:33 PM, Alex Bennée wrote:
> We already have this symbol defined so lets use it.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 6/6] arm: fix aa64_generate_debug_exceptions to work with EL2

[Richard Henderson]

On 11/8/18 5:33 PM, Alex Bennée wrote:
> The test was incomplete and incorrectly caused debug exceptions to be
> generated when returning to EL2 after a failed attempt to single-step
> an EL1 instruction. Fix this while cleaning up the function a little.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/cpu.h | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 1efff21a18..a6d8eb14f6 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2764,23 +2764,33 @@ static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu)
>      return (cpu->clidr & R_V7M_CLIDR_CTYPE_ALL_MASK) != 0;
>  }
>  
> +/* See AArch64.GenerateDebugExceptionsFrom() in ARM ARM pseudocode */
>  static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>  {
> +    int cur_el = arm_current_el(env);
> +    int debug_el;
> +
>      if (arm_is_secure(env)) {
>          /* MDCR_EL3.SDD disables debug events from Secure state */
>          if (extract32(env->cp15.mdcr_el3, 16, 1) != 0
> -            || arm_current_el(env) == 3) {
> +            || cur_el == 3) {

Hmm.  Perhaps better as

    if (cur_el == 3) {
        return false;
    }
    /* MDCR_EL3.SDD disables... */
    if (arm_is_secure_below_el3(env)
        && extract32(env->cp15.mdcr_el3, 16, 1)) {
        return false;
    }

and of course more symbols would be nice, but it's not wrong as-is.


r~

Re: [Qemu-devel] [PATCH v2 1/6] target/arm64: properly handle DBGVR RESS bits

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 11/8/18 5:33 PM, Alex Bennée wrote:
>> -        .bvr = addr
>> +        .bvr = sextract64(addr, 52, 53)
>
> I think you meant sextract64(addr, 0, 53).
> What you wrote *should* have asserted, since 52+53 > 64.

Dam, I did fix that. I must have failed to propagate the fix from where
I was hacking :-/

>
>
> r~


--
Alex Bennée