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

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

[Alex Bennée]

Hi,

I missed a fix I'd applied locally from v2 so this is a resend with
some additional tags, some changes suggested by rth and one more fix
for the test case.

So 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 at 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>

It looks like there will be another patch series on its way to address
this.

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.

The following patches still need review:
 0001/target arm64 properly handle DBGVR RESS bits.patch
 0005/tests guest debug don t use symbol resolution for.patch
 0007/arm fix aa64_generate_debug_exceptions to work wi.patch

Alex Bennée (7):
  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
  tests/guest-debug: don't use symbol resolution for PC checks
  arm: use symbolic MDCR_TDE in arm_debug_target_el
  arm: fix aa64_generate_debug_exceptions to work with EL2

 target/arm/cpu.h                  | 41 +++++++++++++++++++------------
 target/arm/kvm64.c                | 20 +++++++++++++--
 tests/guest-debug/test-gdbstub.py | 24 +++++++++++-------
 3 files changed, 58 insertions(+), 27 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v3 1/7] 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
v3
  - fix bogus sextract64
---
 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..6351a54b28 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, 0, 53)
     };
 
     if (cur_hw_bps >= max_hw_bps) {
-- 
2.17.1

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

[Richard Henderson]

On 11/9/18 4:21 PM, Alex Bennée wrote:
> 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>

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


r~

[Qemu-devel] [PATCH v3 2/7] 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>
Reviewed-by: Richard Henderson <richard.henderson@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 6351a54b28..c39150e5e1 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 v3 3/7] 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>
Reviewed-by: Richard Henderson <richard.henderson@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 c39150e5e1..46fbe6d8ff 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 v3 4/7] 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>
Reviewed-by: Richard Henderson <richard.henderson@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 v3 5/7] tests/guest-debug: don't use symbol resolution for PC checks

[Alex Bennée]

It turns out symbol resolution isn't enough as modern kernels are
often padded with check code at the start of functions. GDB seems to
put the breakpoint at the first non-check instruction which causes
comparisons with the symbol resolution to fail.

For normal breakpoints we can detect the hit just by checking
hit_count instead. For hardware breakpoints we fish the breakpoint
address out of what gdb.execute() reported it was set at.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/guest-debug/test-gdbstub.py | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py
index c7e3986a24..3de174b74b 100644
--- a/tests/guest-debug/test-gdbstub.py
+++ b/tests/guest-debug/test-gdbstub.py
@@ -6,9 +6,10 @@ from __future__ import print_function
 # gdb ${KERNEL}.vmlinux -x ${QEMU_SRC}/tests/guest-debug/test-gdbstub.py
 
 import gdb
+import re
 
 failcount = 0
-
+addr_match = re.compile("(0x[0-9a-f]{4,16})")
 
 def report(cond, msg):
     "Report success/fail of test"
@@ -37,26 +38,30 @@ def check_break(sym_name):
     gdb.execute("c")
 
     # hopefully we came back
-    end_pc = gdb.parse_and_eval('$pc')
-    print ("%s == %s %d" % (end_pc, sym.value(), bp.hit_count))
+    hit = bp.hit_count
     bp.delete()
 
-    # can we test we hit bp?
-    return end_pc == sym.value()
+    # did we hit bp?
+    return hit > 0
 
 
 # We need to do hbreak manually as the python interface doesn't export it
+# As the resolution of sym_name might not exactly match where the
+# breakpoint actually ends up we need to fish it out from result of
+# gdb.execute.
 def check_hbreak(sym_name):
     "Setup hardware breakpoint, continue and check we stopped."
-    sym, ok = gdb.lookup_symbol(sym_name)
-    gdb.execute("hbreak %s" % (sym_name))
+    result = gdb.execute("hbreak %s" % (sym_name), to_string=True)
+    addr_txt = addr_match.search(result).group()
+    addr = int(addr_txt, 16)
+
     gdb.execute("c")
 
     # hopefully we came back
     end_pc = gdb.parse_and_eval('$pc')
-    print ("%s == %s" % (end_pc, sym.value()))
+    print ("%s == %s" % (end_pc, addr))
 
-    if end_pc == sym.value():
+    if end_pc == addr:
         gdb.execute("d 1")
         return True
     else:
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 5/7] tests/guest-debug: don't use symbol resolution for PC checks

[Richard Henderson]

On 11/9/18 4:21 PM, Alex Bennée wrote:
> It turns out symbol resolution isn't enough as modern kernels are
> often padded with check code at the start of functions. GDB seems to
> put the breakpoint at the first non-check instruction which causes
> comparisons with the symbol resolution to fail.

If you want breakpoints at a fixed location, use "*symbol", which will disable
gdb's prologue checking.


r~

[Qemu-devel] [PATCH v3 6/7] 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>
Reviewed-by: Richard Henderson <richard.henderson@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 v3 7/7] 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>

---
v3
  - further re-arrangement as suggested by rth
---
 target/arm/cpu.h | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 1efff21a18..814ff69bc2 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2764,23 +2764,35 @@ 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)
 {
-    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) {
-            return false;
-        }
+    int cur_el = arm_current_el(env);
+    int debug_el;
+
+    if (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;
-        }
+    /* MDCR_EL3.SDD disables debug events from Secure state */
+    if (arm_is_secure_below_el3(env)
+        && extract32(env->cp15.mdcr_el3, 16, 1)) {
+        return false;
     }
-    return true;
+
+    /*
+     * 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);
+    }
+
+    /* 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 +2845,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 v3 7/7] arm: fix aa64_generate_debug_exceptions to work with EL2

[Richard Henderson]

On 11/9/18 4:21 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>
> 
> ---
> v3
>   - further re-arrangement as suggested by rth
> ---
>  target/arm/cpu.h | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)

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


r~

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

[Peter Maydell]

On 9 November 2018 at 15:21, Alex Bennée <alex.bennee@linaro.org> 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.

> @@ -2833,9 +2845,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.
>   */

In hindsight I regret not standardizing on a greppable tag for
marking these kinds of "we don't do X because we don't implement
feature Y" comments...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/7] KVM Guest Debug fixes (plus TCG EL2 debug tweaks)

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Hi,
>
> I missed a fix I'd applied locally from v2 so this is a resend with
> some additional tags, some changes suggested by rth and one more fix
> for the test case.
>
> So 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 at 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>
>
> It looks like there will be another patch series on its way to address
> this.

For reference Mark has posted this now:

   Subject: [PATCH 0/2] kvm/arm: make singlestep behaviour consistent
   Date: Fri,  9 Nov 2018 15:07:09 +0000
   Message-Id: <20181109150711.45864-1-mark.rutland@arm.com>
--
Alex Bennée

Re: [Qemu-devel] [PATCH v3 0/7] KVM Guest Debug fixes (plus TCG EL2 debug tweaks)

[Peter Maydell]

On 9 November 2018 at 15:21, Alex Bennée <alex.bennee@linaro.org> wrote:
> Hi,
>
> I missed a fix I'd applied locally from v2 so this is a resend with
> some additional tags, some changes suggested by rth and one more fix
> for the test case.
>
> So 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 at 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>
>
> It looks like there will be another patch series on its way to address
> this.
>
> 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.
>
> The following patches still need review:
>  0001/target arm64 properly handle DBGVR RESS bits.patch
>  0005/tests guest debug don t use symbol resolution for.patch
>  0007/arm fix aa64_generate_debug_exceptions to work wi.patch

Richard had a review comment on 5, so I'm applying 1-4 and 6-7 to
target-arm.next and will let you respin 5.

thanks
-- PMM