[PATCH v9 5/6] target-arm: kvm - re-inject guest debug exceptions | linux-arm-kernel

[PATCH v9 5/6] target-arm: kvm - re-inject guest debug exceptions

From: Peter Maydell <hidden>
Date: 2015-11-20 16:14:41
Also in: kvm, kvmarm, qemu-arm, qemu-devel

On 12 November 2015 at 16:20, Alex Benn?e [off-list ref] wrote:

From: Alex Benn?e <redacted>

If we can't find details for the debug exception in our debug state
then we can assume the exception is due to debugging inside the guest.
To inject the exception into the guest state we re-use the TCG exception
code (do_interupt).

"do_interrupt".

However while guest debugging is in effect we currently can't handle the
guest using single step which is heavily used by GDB.

Can you expand this to be clearer about what the problem is here?
Is this a thing fixed by this commit or a remaining issue after it?

quoted hunk ↗ jump to hunk

Signed-off-by: Alex Benn?e <redacted>

---
v5:
  - new for v5
---
 target-arm/helper-a64.c | 12 ++++++++++--
 target-arm/kvm.c        | 27 +++++++++++++++++++--------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index deb8dbe..fc3ccdf 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c

@@ -25,6 +25,7 @@
 #include "qemu/bitops.h"
 #include "internals.h"
 #include "qemu/crc32c.h"
+#include "sysemu/kvm.h"
 #include <zlib.h> /* For crc32 */

 /* C2.4.7 Multiply and divide */

@@ -469,7 +470,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
                   new_el);
     if (qemu_loglevel_mask(CPU_LOG_INT)
         && !excp_is_internal(cs->exception_index)) {
-        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%" PRIx32 "\n",
+        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
+                      env->exception.syndrome >> ARM_EL_EC_SHIFT,
                       env->exception.syndrome);
     }

@@ -535,6 +537,12 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     aarch64_restore_sp(env, new_el);

     env->pc = addr;
-    cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    qemu_log_mask(CPU_LOG_INT, "...to EL%d PC 0x%" PRIx64 " PSTATE 0x%x\n",
+                  new_el, env->pc, pstate_read(env));
+
+    if (!kvm_enabled()) {
+        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+    }
 }
 #endif

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 1f57e92..4ac177a 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c

@@ -529,9 +529,10 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
     int hsr_ec = arch_info->hsr >> ARM_EL_EC_SHIFT;
     ARMCPU *cpu = ARM_CPU(cs);
+    CPUClass *cc = CPU_GET_CLASS(cs);
     CPUARMState *env = &cpu->env;

-    /* Ensure PC is synchronised */
+    /* Ensure all state is synchronised */

You might as well have just written the comment like that to start with :-)

quoted hunk ↗ jump to hunk

     kvm_cpu_synchronize_state(cs);

     switch (hsr_ec) {

@@ -539,7 +540,14 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
         if (cs->singlestep_enabled) {
             return true;
         } else {
-            error_report("Came out of SINGLE STEP when not enabled");
+            /*
+             * The kernel should have supressed the guests ability to

"suppressed". "guest's".

+             * single step at this point so something has gone wrong.
+             */
+            error_report("%s: guest single-step while debugging unsupported"
+                         " (%"PRIx64", %"PRIx32")\n",
+                         __func__, env->pc, arch_info->hsr);
+            return false;

Why didn't we just write the error_report this way to start with?

quoted hunk ↗ jump to hunk

         }
         break;
     case EC_AA64_BKPT:

@@ -564,14 +572,17 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
     default:
         error_report("%s: unhandled debug exit (%"PRIx32", %"PRIx64")\n",
                      __func__, arch_info->hsr, env->pc);
+        return false;

Might as well put this "return false;" in the original code that
adds the default case, rather than changing the control flow in this
patch.

     }

-    /* If we don't handle this it could be it really is for the
-       guest to handle */
-    qemu_log_mask(LOG_UNIMP,
-                  "%s: re-injecting exception not yet implemented"
-                  " (0x%"PRIx32", %"PRIx64")\n",
-                  __func__, hsr_ec, env->pc);
+    /* If we are not handling the debug exception it must belong to
+     * the guest. Let's re-use the existing TCG interrupt code to set
+     * everything up properly

Missing trailing ".".

+     */
+    cs->exception_index = EXCP_BKPT;
+    env->exception.syndrome = arch_info->hsr;
+    env->exception.vaddress = arch_info->far;

You need to set env->exception.target_el to 1 as well.

+    cc->do_interrupt(cs);

     return false;
 }
--
2.6.3

thanks
-- PMM

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help