* [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64
@ 2015-03-31 15:40 Alex Bennée
2015-03-31 15:40 ` [PATCH v2 1/4] linux-headers: partial sync from my kernel tree (DEV) Alex Bennée
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Alex Bennée @ 2015-03-31 15:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell, christoffer.dall, zhichao.huang
Cc: kvm, linux-arm-kernel, kvmarm, marc.zyngier, Alex Bennée
Hi,
I thought I'd sent V1 to the list but apparently not. Anyway this
patch series provides the QEMU side of guest debug support for arm64.
I'm assuming the first patch will be dropped when a proper merge of
the linux-headers is done once the kernel side is upstreamed.
There is nothing particularly special about the implementation details
although some of the bit-fiddling is a little fiddly.
GIT Repos:
The patch series is based off a recent master and can be found at:
https://github.com/stsquad/qemu
branch: kvm/guest-debug-v2
The kernel patches for this series are based off a v4.0-rc6 and can be
found at:
https://git.linaro.org/people/alex.bennee/linux.git
branch: guest-debug/4.0-rc6-v2
Alex Bennée (4):
linux-headers: partial sync from my kernel tree (DEV)
target-arm: kvm - implement software breakpoints
target-arm: kvm - support for single step
target-arm: kvm - add support for HW assisted debug
linux-headers/asm-arm64/kvm.h | 22 +++
linux-headers/asm-powerpc/kvm.h | 4 +-
linux-headers/asm-x86/kvm.h | 4 +-
linux-headers/linux/kvm.h | 17 ++-
target-arm/kvm.c | 124 ++++++++++++----
target-arm/kvm64.c | 315 ++++++++++++++++++++++++++++++++++++++++
target-arm/kvm_arm.h | 21 +++
7 files changed, 474 insertions(+), 33 deletions(-)
--
2.3.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/4] linux-headers: partial sync from my kernel tree (DEV)
2015-03-31 15:40 [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
@ 2015-03-31 15:40 ` Alex Bennée
2015-03-31 15:40 ` [PATCH v2 2/4] target-arm: kvm - implement software breakpoints Alex Bennée
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Alex Bennée @ 2015-03-31 15:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell, christoffer.dall, zhichao.huang
Cc: marc.zyngier, linux-arm-kernel, kvm, kvmarm
I assume I'll properly merge the KVM Headers direct from Linux when
done. These headers came from:
https://git.linaro.org/people/alex.bennee/linux.git/shortlog/refs/heads/guest-debug/4.0-rc6-v2
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v2
- update ABI to include ->far
diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index 3ef77a4..73f21e4 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -100,10 +100,25 @@ struct kvm_sregs {
struct kvm_fpu {
};
+/*
+ * See ARM ARM D7.3: Debug Registers
+ *
+ * The control registers are architecturally defined as 32 bits but are
+ * stored as 64 bit values along side the value registers and aligned
+ * with the rest 64 bit registers in the normal CPU context.
+ */
+#define KVM_ARM_NDBG_REGS 16
struct kvm_guest_debug_arch {
+ __u64 dbg_bcr[KVM_ARM_NDBG_REGS];
+ __u64 dbg_bvr[KVM_ARM_NDBG_REGS];
+ __u64 dbg_wcr[KVM_ARM_NDBG_REGS];
+ __u64 dbg_wvr[KVM_ARM_NDBG_REGS];
};
struct kvm_debug_exit_arch {
+ __u64 pc;
+ __u32 hsr;
+ __u64 far; /* used for watchpoints */
};
struct kvm_sync_regs {
@@ -207,4 +222,11 @@ struct kvm_arch_memory_slot {
#endif
+/*
+ * Architecture related debug defines - upper 16 bits of
+ * kvm_guest_debug->control
+ */
+#define KVM_GUESTDBG_USE_SW_BP __KVM_GUESTDBG_USE_SW_BP
+#define KVM_GUESTDBG_USE_HW_BP __KVM_GUESTDBG_USE_HW_BP
+
#endif /* __ARM_KVM_H__ */
diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
index ab4d473..1731569 100644
--- a/linux-headers/asm-powerpc/kvm.h
+++ b/linux-headers/asm-powerpc/kvm.h
@@ -310,8 +310,8 @@ struct kvm_guest_debug_arch {
* and upper 16 bits are architecture specific. Architecture specific defines
* that ioctl is for setting hardware breakpoint or software breakpoint.
*/
-#define KVM_GUESTDBG_USE_SW_BP 0x00010000
-#define KVM_GUESTDBG_USE_HW_BP 0x00020000
+#define KVM_GUESTDBG_USE_SW_BP __KVM_GUESTDBG_USE_SW_BP
+#define KVM_GUESTDBG_USE_HW_BP __KVM_GUESTDBG_USE_HW_BP
/* definition of registers in kvm_run */
struct kvm_sync_regs {
diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
index d7dcef5..1438202 100644
--- a/linux-headers/asm-x86/kvm.h
+++ b/linux-headers/asm-x86/kvm.h
@@ -250,8 +250,8 @@ struct kvm_debug_exit_arch {
__u64 dr7;
};
-#define KVM_GUESTDBG_USE_SW_BP 0x00010000
-#define KVM_GUESTDBG_USE_HW_BP 0x00020000
+#define KVM_GUESTDBG_USE_SW_BP __KVM_GUESTDBG_USE_SW_BP
+#define KVM_GUESTDBG_USE_HW_BP __KVM_GUESTDBG_USE_HW_BP
#define KVM_GUESTDBG_INJECT_DB 0x00040000
#define KVM_GUESTDBG_INJECT_BP 0x00080000
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 60a54c8..fa6c2ac 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -226,6 +226,7 @@ struct kvm_run {
__u32 count;
__u64 data_offset; /* relative to kvm_run start */
} io;
+ /* KVM_EXIT_DEBUG */
struct {
struct kvm_debug_exit_arch arch;
} debug;
@@ -274,6 +275,7 @@ struct kvm_run {
__u32 data;
__u8 is_write;
} dcr;
+ /* KVM_EXIT_INTERNAL_ERROR */
struct {
__u32 suberror;
/* Available with KVM_CAP_INTERNAL_ERROR_DATA: */
@@ -284,6 +286,7 @@ struct kvm_run {
struct {
__u64 gprs[32];
} osi;
+ /* KVM_EXIT_PAPR_HCALL */
struct {
__u64 nr;
__u64 ret;
@@ -522,8 +525,16 @@ struct kvm_s390_irq {
/* for KVM_SET_GUEST_DEBUG */
-#define KVM_GUESTDBG_ENABLE 0x00000001
-#define KVM_GUESTDBG_SINGLESTEP 0x00000002
+#define KVM_GUESTDBG_ENABLE (1 << 0)
+#define KVM_GUESTDBG_SINGLESTEP (1 << 1)
+
+/*
+ * Architecture specific stuff uses the top 16 bits of the field,
+ * however there is some shared commonality for the common cases
+ */
+#define __KVM_GUESTDBG_USE_SW_BP (1 << 16)
+#define __KVM_GUESTDBG_USE_HW_BP (1 << 17)
+
struct kvm_guest_debug {
__u32 control;
@@ -760,6 +771,8 @@ struct kvm_ppc_smmu_info {
#define KVM_CAP_PPC_ENABLE_HCALL 104
#define KVM_CAP_CHECK_EXTENSION_VM 105
#define KVM_CAP_S390_USER_SIGP 106
+#define KVM_CAP_GUEST_DEBUG_HW_BPS 107
+#define KVM_CAP_GUEST_DEBUG_HW_WPS 108
#ifdef KVM_CAP_IRQ_ROUTING
--
2.3.4
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] target-arm: kvm - implement software breakpoints
2015-03-31 15:40 [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
2015-03-31 15:40 ` [PATCH v2 1/4] linux-headers: partial sync from my kernel tree (DEV) Alex Bennée
@ 2015-03-31 15:40 ` Alex Bennée
2015-04-20 19:44 ` Peter Maydell
2015-03-31 15:40 ` [PATCH v2 3/4] target-arm: kvm - support for single step Alex Bennée
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Alex Bennée @ 2015-03-31 15:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell, christoffer.dall, zhichao.huang
Cc: kvm, linux-arm-kernel, kvmarm, marc.zyngier, Alex Bennée,
Paolo Bonzini
These don't involve messing around with debug registers, just setting
the breakpoint instruction in memory. GDB will not use this mechanism if
it can't access the memory to write the breakpoint.
All the kernel has to do is ensure the hypervisor traps the breakpoint
exceptions and returns to userspace.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
--
v2
- handle debug exit with new hsr exception info
- add verbosity to UNIMP message
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 72c1fa1..290c1fe 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -25,6 +25,7 @@
#include "hw/arm/arm.h"
const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
+ KVM_CAP_INFO(SET_GUEST_DEBUG),
KVM_CAP_LAST_INFO
};
@@ -466,9 +467,57 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
{
}
+/* See ARM ARM D7.2.27 ESR_ELx, Exception Syndrome Register
+**
+** To minimise translating between kernel and user-space the kernel
+** ABI just provides user-space with the full exception syndrome
+** register value to be decoded in QEMU.
+*/
+
+#define HSR_EC_SHIFT 26
+#define HSR_EC_SW_BKPT 0x3c
+
+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 >> HSR_EC_SHIFT;
+
+ switch (hsr_ec) {
+ case HSR_EC_SW_BKPT:
+ if (kvm_find_sw_breakpoint(cs, arch_info->pc)) {
+ return true;
+ }
+ break;
+ default:
+ error_report("%s: unhandled debug exit (%x, %llx)\n",
+ __func__, arch_info->hsr, arch_info->pc);
+ }
+
+ /* 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%x, %llx)\n",
+ __func__, hsr_ec, arch_info->pc);
+
+ return false;
+}
+
int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
{
- return 0;
+ int ret = 0;
+
+ switch (run->exit_reason) {
+ case KVM_EXIT_DEBUG:
+ if (kvm_handle_debug(cs, run)) {
+ ret = EXCP_DEBUG;
+ } /* otherwise return to guest */
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
+ __func__, run->exit_reason);
+ break;
+ }
+ return ret;
}
bool kvm_arch_stop_on_emulation_error(CPUState *cs)
@@ -493,14 +542,33 @@ int kvm_arch_on_sigbus(int code, void *addr)
void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
{
- qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
+ if (kvm_sw_breakpoints_active(cs)) {
+ dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
+ }
}
-int kvm_arch_insert_sw_breakpoint(CPUState *cs,
- struct kvm_sw_breakpoint *bp)
+/* C6.6.29 BRK instruction */
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
{
- qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
- return -EINVAL;
+ static const uint32_t brk = 0xd4200000;
+
+ if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
+ cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 1)) {
+ return -EINVAL;
+ }
+ return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+{
+ static uint32_t brk;
+
+ if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
+ brk != 0xd4200000 ||
+ cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
+ return -EINVAL;
+ }
+ return 0;
}
int kvm_arch_insert_hw_breakpoint(target_ulong addr,
@@ -517,12 +585,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
return -EINVAL;
}
-int kvm_arch_remove_sw_breakpoint(CPUState *cs,
- struct kvm_sw_breakpoint *bp)
-{
- qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
- return -EINVAL;
-}
void kvm_arch_remove_all_hw_breakpoints(void)
{
--
2.3.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] target-arm: kvm - support for single step
2015-03-31 15:40 [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
2015-03-31 15:40 ` [PATCH v2 1/4] linux-headers: partial sync from my kernel tree (DEV) Alex Bennée
2015-03-31 15:40 ` [PATCH v2 2/4] target-arm: kvm - implement software breakpoints Alex Bennée
@ 2015-03-31 15:40 ` Alex Bennée
2015-04-20 19:49 ` Peter Maydell
2015-03-31 15:40 ` [PATCH v2 4/4] target-arm: kvm - add support for HW assisted debug Alex Bennée
2015-04-20 15:17 ` [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
4 siblings, 1 reply; 12+ messages in thread
From: Alex Bennée @ 2015-03-31 15:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell, christoffer.dall, zhichao.huang
Cc: kvm, marc.zyngier, Paolo Bonzini, kvmarm, linux-arm-kernel
This adds support for single-step. There isn't much to do on the QEMU
side as after we set-up the request for single step via the debug ioctl
it is all handled within the kernel.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v2
- convert to using HSR_EC
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 290c1fe..ae0f8b2 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -475,6 +475,7 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
*/
#define HSR_EC_SHIFT 26
+#define HSR_EC_SOFT_STEP 0x32
#define HSR_EC_SW_BKPT 0x3c
static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
@@ -483,6 +484,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
int hsr_ec = arch_info->hsr >> HSR_EC_SHIFT;
switch (hsr_ec) {
+ case HSR_EC_SOFT_STEP:
+ if (cs->singlestep_enabled) {
+ return true;
+ } else {
+ error_report("Came out of SINGLE STEP when not enabled");
+ }
+ break;
case HSR_EC_SW_BKPT:
if (kvm_find_sw_breakpoint(cs, arch_info->pc)) {
return true;
@@ -542,6 +550,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
{
+ if (cs->singlestep_enabled) {
+ dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+ }
if (kvm_sw_breakpoints_active(cs)) {
dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
}
--
2.3.4
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] target-arm: kvm - add support for HW assisted debug
2015-03-31 15:40 [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
` (2 preceding siblings ...)
2015-03-31 15:40 ` [PATCH v2 3/4] target-arm: kvm - support for single step Alex Bennée
@ 2015-03-31 15:40 ` Alex Bennée
2015-04-20 20:23 ` Peter Maydell
2015-04-20 15:17 ` [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
4 siblings, 1 reply; 12+ messages in thread
From: Alex Bennée @ 2015-03-31 15:40 UTC (permalink / raw)
To: qemu-devel, peter.maydell, christoffer.dall, zhichao.huang
Cc: kvm, linux-arm-kernel, kvmarm, marc.zyngier, Alex Bennée,
Alex Bennée, Paolo Bonzini
From: Alex Bennée <alex@bennee.com>
This adds basic support for HW assisted debug. The ioctl interface to
KVM allows us to pass an implementation defined number of break and
watch point registers. When KVM_GUESTDBG_USE_HW_BP is specified these
debug registers will be installed in place on the world switch into the
guest.
The hardware is actually capable of more advanced matching but it is
unclear if this expressiveness is available via the gdbstub protocol.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v2
- correct setting of PMC/BAS/MASK
- improved commentary
- added helper function to check watchpoint in range
- fix find/deletion of watchpoints
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index ae0f8b2..d1adf5f 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -17,6 +17,7 @@
#include "qemu-common.h"
#include "qemu/timer.h"
+#include "qemu/error-report.h"
#include "sysemu/sysemu.h"
#include "sysemu/kvm.h"
#include "kvm_arm.h"
@@ -476,6 +477,8 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
#define HSR_EC_SHIFT 26
#define HSR_EC_SOFT_STEP 0x32
+#define HSR_EC_HW_BKPT 0x30
+#define HSR_EC_HW_WATCH 0x34
#define HSR_EC_SW_BKPT 0x3c
static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
@@ -496,6 +499,16 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
return true;
}
break;
+ case HSR_EC_HW_BKPT:
+ if (kvm_arm_find_hw_breakpoint(cs, arch_info->pc)) {
+ return true;
+ }
+ break;
+ case HSR_EC_HW_WATCH:
+ if (kvm_arm_find_hw_watchpoint(cs, arch_info->far)) {
+ return true;
+ }
+ break;
default:
error_report("%s: unhandled debug exit (%x, %llx)\n",
__func__, arch_info->hsr, arch_info->pc);
@@ -556,6 +569,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
if (kvm_sw_breakpoints_active(cs)) {
dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
}
+ if (kvm_hw_breakpoints_active(cs)) {
+ dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
+ kvm_copy_hw_breakpoint_data(&dbg->arch);
+ }
}
/* C6.6.29 BRK instruction */
@@ -582,26 +599,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
return 0;
}
-int kvm_arch_insert_hw_breakpoint(target_ulong addr,
- target_ulong len, int type)
-{
- qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
- return -EINVAL;
-}
-
-int kvm_arch_remove_hw_breakpoint(target_ulong addr,
- target_ulong len, int type)
-{
- qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
- return -EINVAL;
-}
-
-
-void kvm_arch_remove_all_hw_breakpoints(void)
-{
- qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
-}
-
void kvm_arch_init_irq_routing(KVMState *s)
{
}
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 8cf3a62..dbe81a7 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -2,6 +2,7 @@
* ARM implementation of KVM hooks, 64 bit specific code
*
* Copyright Mian-M. Hamayun 2013, Virtual Open Systems
+ * Copyright Alex Bennée 2014, Linaro
*
* This work is licensed under the terms of the GNU GPL, version 2 or later.
* See the COPYING file in the top-level directory.
@@ -12,11 +13,17 @@
#include <sys/types.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
+#include <sys/ptrace.h>
+#include <asm/ptrace.h>
+#include <linux/elf.h>
#include <linux/kvm.h>
#include "qemu-common.h"
#include "qemu/timer.h"
+#include "qemu/host-utils.h"
+#include "qemu/error-report.h"
+#include "exec/gdbstub.h"
#include "sysemu/sysemu.h"
#include "sysemu/kvm.h"
#include "kvm_arm.h"
@@ -24,6 +31,312 @@
#include "internals.h"
#include "hw/arm/arm.h"
+/* Max and current break/watch point counts */
+int max_hw_bp, max_hw_wp;
+int cur_hw_bp, cur_hw_wp;
+struct kvm_guest_debug_arch guest_debug_registers;
+
+/**
+ * kvm_arm_init_debug()
+ * @cs: CPUState
+ *
+ * kvm_check_extension returns 0 if we have no debug registers or the
+ * number we have.
+ *
+ */
+static void kvm_arm_init_debug(CPUState *cs)
+{
+ max_hw_wp = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
+ max_hw_bp = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
+ return;
+}
+
+/**
+ * insert_hw_breakpoint()
+ * @addr: address of breakpoint
+ *
+ * See ARM ARM D2.9.1 for details but here we are only going to create
+ * simple un-linked breakpoints (i.e. we don't chain breakpoints
+ * together to match address and context or vmid). The hardware is
+ * 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
+ *
+ * 31 24 23 20 19 16 15 14 13 12 9 8 5 4 3 2 1 0
+ * +------+------+-------+-----+----+------+-----+------+-----+---+
+ * | RES0 | BT | LBN | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
+ * +------+------+-------+-----+----+------+-----+------+-----+---+
+ *
+ * BT: Breakpoint type (0 = unlinked address match)
+ * LBN: Linked BP number (0 = unused)
+ * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
+ * BAS: Byte Address Select (RES1 for AArch64)
+ * E: Enable bit
+ */
+static int insert_hw_breakpoint(target_ulong addr)
+{
+ uint32_t bcr = 0x1; /* E=1, enable */
+ if (cur_hw_bp >= max_hw_bp) {
+ return -ENOBUFS;
+ }
+ bcr = deposit32(bcr, 1, 2, 0x3); /* PMC = 11 */
+ bcr = deposit32(bcr, 5, 4, 0xf); /* BAS = RES1 */
+ guest_debug_registers.dbg_bcr[cur_hw_bp] = bcr;
+ guest_debug_registers.dbg_bvr[cur_hw_bp] = addr;
+ cur_hw_bp++;
+ return 0;
+}
+
+/**
+ * delete_hw_breakpoint()
+ * @pc: address of breakpoint
+ *
+ * Delete a breakpoint and shuffle any above down
+ */
+
+static int delete_hw_breakpoint(target_ulong pc)
+{
+ int i;
+ for (i = 0; i < cur_hw_bp; i++) {
+ if (guest_debug_registers.dbg_bvr[i] == pc) {
+ while (i < cur_hw_bp) {
+ if (i == max_hw_bp) {
+ guest_debug_registers.dbg_bvr[i] = 0;
+ guest_debug_registers.dbg_bcr[i] = 0;
+ } else {
+ guest_debug_registers.dbg_bvr[i] =
+ guest_debug_registers.dbg_bvr[i + 1];
+ guest_debug_registers.dbg_bcr[i] =
+ guest_debug_registers.dbg_bcr[i + 1];
+ }
+ i++;
+ }
+ cur_hw_bp--;
+ return 0;
+ }
+ }
+ return -ENOENT;
+}
+
+/**
+ * insert_hw_watchpoint()
+ * @addr: address of watch point
+ * @len: size of area
+ * @type: type of watch point
+ *
+ * See ARM ARM D2.10. As with the breakpoints we can do some advanced
+ * stuff if we want to. The watch points can be linked with the break
+ * points above to make them context aware. However for simplicity
+ * currently we only deal with simple read/write watch points.
+ *
+ * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
+ *
+ * 31 29 28 24 23 21 20 19 16 15 14 13 12 5 4 3 2 1 0
+ * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
+ * | RES0 | MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
+ * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
+ *
+ * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
+ * WT: 0 - unlinked, 1 - linked (not currently used)
+ * LBN: Linked BP number (not currently used)
+ * SSC/HMC/PAC: Security, Higher and Priv access control (Table D-12)
+ * BAS: Byte Address Select
+ * LSC: Load/Store control (01: load, 10: store, 11: both)
+ * E: Enable
+ *
+ * The bottom 2 bits of the value register are masked. Therefor to
+ * break on an sizes smaller than unaligned byte you need to set
+ * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
+ * need to ensure you mask the address as required and set BAS=0xff
+ */
+
+static int insert_hw_watchpoint(target_ulong addr,
+ target_ulong len, int type)
+{
+ uint32_t dbgwcr = 1; /* E=1, enable */
+ uint64_t dbgwvr = addr & (~0x7ULL);
+
+ if (cur_hw_wp >= max_hw_wp) {
+ return -ENOBUFS;
+ }
+
+ /* PAC 00 is reserved, assume EL1 exception */
+ dbgwcr = deposit32(dbgwcr, 1, 2, 1);
+
+ switch (type) {
+ case GDB_WATCHPOINT_READ:
+ dbgwcr = deposit32(dbgwcr, 3, 2, 1);
+ break;
+ case GDB_WATCHPOINT_WRITE:
+ dbgwcr = deposit32(dbgwcr, 3, 2, 2);
+ break;
+ case GDB_WATCHPOINT_ACCESS:
+ dbgwcr = deposit32(dbgwcr, 3, 2, 3);
+ break;
+ default:
+ g_assert_not_reached();
+ break;
+ }
+ if (len <= 8) {
+ /* we align the address and set the bits in BAS */
+ int off = addr & 0x7;
+ int bas = (1 << len)-1;
+ dbgwcr = deposit32(dbgwcr, 5+off, 8-off, bas);
+ } else {
+ /* For ranges above 8 bytes we need to be a power of 2 */
+ if (ctpop64(len)==1) {
+ int bits = ctz64(len);
+ dbgwvr &= ~((1 << bits)-1);
+ dbgwcr = deposit32(dbgwcr, 24, 4, bits);
+ dbgwcr = deposit32(dbgwcr, 5, 8, 0xff);
+ } else {
+ return -ENOBUFS;
+ }
+ }
+
+ guest_debug_registers.dbg_wcr[cur_hw_wp] = dbgwcr;
+ guest_debug_registers.dbg_wvr[cur_hw_wp] = dbgwvr;
+ cur_hw_wp++;
+ return 0;
+}
+
+
+static bool check_watchpoint_in_range(int i, target_ulong addr)
+{
+ uint32_t dbgwcr = guest_debug_registers.dbg_wcr[i];
+ uint64_t addr_top, addr_bottom = guest_debug_registers.dbg_wvr[i];
+ int bas = extract32(dbgwcr, 5, 8);
+ int mask = extract32(dbgwcr, 24, 4);
+
+ if (mask) {
+ addr_top = addr_bottom + (1 << mask);
+ } else {
+ /* BAS must be contiguous but can offset against the base
+ * address in DBGWVR */
+ addr_bottom = addr_bottom + ctz32(bas);
+ addr_top = addr_bottom + clo32(bas);
+ }
+
+ if (addr >= addr_bottom && addr <= addr_top ) {
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ * delete_hw_watchpoint()
+ * @addr: address of breakpoint
+ *
+ * Delete a breakpoint and shuffle any above down
+ */
+
+static int delete_hw_watchpoint(target_ulong addr,
+ target_ulong len, int type)
+{
+ int i;
+ for (i = 0; i < cur_hw_wp; i++) {
+ if (check_watchpoint_in_range(i, addr)) {
+ while (i < cur_hw_wp) {
+ if (i == max_hw_wp) {
+ guest_debug_registers.dbg_wvr[i] = 0;
+ guest_debug_registers.dbg_wcr[i] = 0;
+ } else {
+ guest_debug_registers.dbg_wvr[i] =
+ guest_debug_registers.dbg_wvr[i + 1];
+ guest_debug_registers.dbg_wcr[i] =
+ guest_debug_registers.dbg_wcr[i + 1];
+ }
+ i++;
+ }
+ cur_hw_wp--;
+ return 0;
+ }
+ }
+ return -ENOENT;
+}
+
+
+int kvm_arch_insert_hw_breakpoint(target_ulong addr,
+ target_ulong len, int type)
+{
+ switch (type) {
+ case GDB_BREAKPOINT_HW:
+ return insert_hw_breakpoint(addr);
+ break;
+ case GDB_WATCHPOINT_READ:
+ case GDB_WATCHPOINT_WRITE:
+ case GDB_WATCHPOINT_ACCESS:
+ return insert_hw_watchpoint(addr, len, type);
+ default:
+ return -ENOSYS;
+ }
+}
+
+int kvm_arch_remove_hw_breakpoint(target_ulong addr,
+ target_ulong len, int type)
+{
+ switch (type) {
+ case GDB_BREAKPOINT_HW:
+ return delete_hw_breakpoint(addr);
+ break;
+ case GDB_WATCHPOINT_READ:
+ case GDB_WATCHPOINT_WRITE:
+ case GDB_WATCHPOINT_ACCESS:
+ return delete_hw_watchpoint(addr, len, type);
+ default:
+ return -ENOSYS;
+ }
+}
+
+
+void kvm_arch_remove_all_hw_breakpoints(void)
+{
+ memset((void *)&guest_debug_registers, 0, sizeof(guest_debug_registers));
+ cur_hw_bp = 0;
+ cur_hw_wp = 0;
+}
+
+void kvm_copy_hw_breakpoint_data(struct kvm_guest_debug_arch *ptr)
+{
+ /* Compile time assert? */
+ g_assert(sizeof(struct kvm_guest_debug_arch) == sizeof(guest_debug_registers));
+ memcpy(ptr, &guest_debug_registers, sizeof(guest_debug_registers));
+}
+
+bool kvm_hw_breakpoints_active(CPUState *cs)
+{
+ return ( (cur_hw_bp > 0) || (cur_hw_wp >0) ) ? TRUE:FALSE;
+}
+
+bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
+{
+ if (kvm_hw_breakpoints_active(cpu)) {
+ int i;
+ for (i=0; i<cur_hw_bp; i++) {
+ if (guest_debug_registers.dbg_bvr[i] == pc) {
+ return true;
+ }
+ }
+ }
+ return false;
+}
+
+bool kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
+{
+ if (kvm_hw_breakpoints_active(cpu)) {
+ int i;
+ for (i=0; i<cur_hw_wp; i++) {
+ if (check_watchpoint_in_range(i, addr)) {
+ return true;
+ }
+ }
+ }
+ return false;
+}
+
+
static inline void set_feature(uint64_t *features, int feature)
{
*features |= 1ULL << feature;
@@ -106,6 +419,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
return ret;
}
+ kvm_arm_init_debug(cs);
+
return kvm_arm_init_cpreg_list(cpu);
}
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index 455dea3..a4b480b 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -162,6 +162,27 @@ typedef struct ARMHostCPUClass {
*/
bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc);
+bool kvm_hw_breakpoints_active(CPUState *cs);
+void kvm_copy_hw_breakpoint_data(struct kvm_guest_debug_arch *ptr);
+
+/**
+ * kvm_arm_find_hw_breakpoint:
+ * @cpu: CPUState
+ * @pc: pc of breakpoint
+ *
+ * Return TRUE if the pc matches one of our breakpoints.
+ */
+bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
+
+/**
+ * kvm_arm_find_hw_watchpoint:
+ * @cpu: CPUState
+ * @addr: address of watchpoint
+ *
+ * Return TRUE if the addr matches one of our watchpoints.
+ */
+bool kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
+
#endif
#endif
--
2.3.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64
2015-03-31 15:40 [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
` (3 preceding siblings ...)
2015-03-31 15:40 ` [PATCH v2 4/4] target-arm: kvm - add support for HW assisted debug Alex Bennée
@ 2015-04-20 15:17 ` Alex Bennée
4 siblings, 0 replies; 12+ messages in thread
From: Alex Bennée @ 2015-04-20 15:17 UTC (permalink / raw)
To: qemu-devel, peter.maydell, christoffer.dall, zhichao.huang
Cc: kvm, linux-arm-kernel, kvmarm, marc.zyngier
Alex Bennée <alex.bennee@linaro.org> writes:
> Hi,
>
> I thought I'd sent V1 to the list but apparently not. Anyway this
> patch series provides the QEMU side of guest debug support for arm64.
> I'm assuming the first patch will be dropped when a proper merge of
> the linux-headers is done once the kernel side is upstreamed.
>
> There is nothing particularly special about the implementation details
> although some of the bit-fiddling is a little fiddly.
>
> GIT Repos:
>
> The patch series is based off a recent master and can be found at:
>
> https://github.com/stsquad/qemu
> branch: kvm/guest-debug-v2
>
> The kernel patches for this series are based off a v4.0-rc6 and can be
> found at:
>
> https://git.linaro.org/people/alex.bennee/linux.git
> branch: guest-debug/4.0-rc6-v2
>
> Alex Bennée (4):
> linux-headers: partial sync from my kernel tree (DEV)
> target-arm: kvm - implement software breakpoints
> target-arm: kvm - support for single step
> target-arm: kvm - add support for HW assisted debug
>
<snip>
The kernel side has had two rounds of review and is getting into shape.
It would be nice if I could get some review of the QEMU side for balance
;-)
--
Alex Bennée
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] target-arm: kvm - implement software breakpoints
2015-03-31 15:40 ` [PATCH v2 2/4] target-arm: kvm - implement software breakpoints Alex Bennée
@ 2015-04-20 19:44 ` Peter Maydell
0 siblings, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2015-04-20 19:44 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm-devel, Marc Zyngier, QEMU Developers, Zhichao Huang,
Paolo Bonzini, kvmarm, arm-mail-list
On 31 March 2015 at 16:40, Alex Bennée <alex.bennee@linaro.org> wrote:
> These don't involve messing around with debug registers, just setting
> the breakpoint instruction in memory. GDB will not use this mechanism if
> it can't access the memory to write the breakpoint.
>
> All the kernel has to do is ensure the hypervisor traps the breakpoint
> exceptions and returns to userspace.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> --
> v2
> - handle debug exit with new hsr exception info
> - add verbosity to UNIMP message
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 72c1fa1..290c1fe 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -25,6 +25,7 @@
> #include "hw/arm/arm.h"
>
> const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> + KVM_CAP_INFO(SET_GUEST_DEBUG),
> KVM_CAP_LAST_INFO
> };
Doesn't this mean we'll suddenly stop working on older
kernels that didn't implement this capability? It would be
nicer to merely disable the breakpoint/debug support, rather
than refuse to run at all...
> @@ -466,9 +467,57 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
> {
> }
>
> +/* See ARM ARM D7.2.27 ESR_ELx, Exception Syndrome Register
You should probably say 'v8 ARM ARM'.
> +**
> +** To minimise translating between kernel and user-space the kernel
> +** ABI just provides user-space with the full exception syndrome
> +** register value to be decoded in QEMU.
What's with the weird '**' comment format?
> +*/
> +
> +#define HSR_EC_SHIFT 26
> +#define HSR_EC_SW_BKPT 0x3c
> +
> +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 >> HSR_EC_SHIFT;
> +
> + switch (hsr_ec) {
> + case HSR_EC_SW_BKPT:
> + if (kvm_find_sw_breakpoint(cs, arch_info->pc)) {
> + return true;
> + }
> + break;
> + default:
> + error_report("%s: unhandled debug exit (%x, %llx)\n",
> + __func__, arch_info->hsr, arch_info->pc);
Is this intended to be a "can't happen" case, or is it something
a guest can trigger?
> + }
> +
> + /* If we don't handle this it could be it really is for the
> + guest to handle */
(suboptimal multiline comment format)
> + qemu_log_mask(LOG_UNIMP,
> + "%s: re-injecting exception not yet implemented (0x%x, %llx)\n",
> + __func__, hsr_ec, arch_info->pc);
When does this happen? Guest userspace program hits a hardcoded
breakpoint insn while we're trying to debug the VM?
If we just return to the guest in that situation will we
try to re-execute the bkpt insn and loop round taking
exceptions forever?
> +
> + return false;
> +}
> +
> int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> {
> - return 0;
> + int ret = 0;
> +
> + switch (run->exit_reason) {
> + case KVM_EXIT_DEBUG:
> + if (kvm_handle_debug(cs, run)) {
> + ret = EXCP_DEBUG;
> + } /* otherwise return to guest */
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
> + __func__, run->exit_reason);
> + break;
> + }
> + return ret;
> }
>
> bool kvm_arch_stop_on_emulation_error(CPUState *cs)
> @@ -493,14 +542,33 @@ int kvm_arch_on_sigbus(int code, void *addr)
>
> void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> {
> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> + if (kvm_sw_breakpoints_active(cs)) {
> + dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> + }
> }
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs,
> - struct kvm_sw_breakpoint *bp)
> +/* C6.6.29 BRK instruction */
This comment would be better placed next to the magic number it's
explaining.
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> {
> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> - return -EINVAL;
> + static const uint32_t brk = 0xd4200000;
> +
> + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 1)) {
Does this work correctly for big-endian hosts?
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +{
> + static uint32_t brk;
> +
> + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) ||
> + brk != 0xd4200000 ||
If you're going to use this magic number twice please name it...
> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
> + return -EINVAL;
> + }
> + return 0;
> }
>
> int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> @@ -517,12 +585,6 @@ int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> return -EINVAL;
> }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs,
> - struct kvm_sw_breakpoint *bp)
> -{
> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> - return -EINVAL;
> -}
>
> void kvm_arch_remove_all_hw_breakpoints(void)
> {
> --
> 2.3.4
>
thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] target-arm: kvm - support for single step
2015-03-31 15:40 ` [PATCH v2 3/4] target-arm: kvm - support for single step Alex Bennée
@ 2015-04-20 19:49 ` Peter Maydell
2015-04-21 12:56 ` Alex Bennée
0 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2015-04-20 19:49 UTC (permalink / raw)
To: Alex Bennée
Cc: QEMU Developers, Christoffer Dall, Zhichao Huang, kvm-devel,
arm-mail-list, kvmarm, Marc Zyngier, Paolo Bonzini
On 31 March 2015 at 16:40, Alex Bennée <alex.bennee@linaro.org> wrote:
> This adds support for single-step. There isn't much to do on the QEMU
> side as after we set-up the request for single step via the debug ioctl
> it is all handled within the kernel.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
> - convert to using HSR_EC
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 290c1fe..ae0f8b2 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -475,6 +475,7 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
> */
>
> #define HSR_EC_SHIFT 26
> +#define HSR_EC_SOFT_STEP 0x32
> #define HSR_EC_SW_BKPT 0x3c
We already include internals.h in this file, so can you just use
the EC_* constants and ARM_EL_EC_SHIFT rather than defining
new ones? (Applies for patch 1 as well.)
> static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
> @@ -483,6 +484,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
> int hsr_ec = arch_info->hsr >> HSR_EC_SHIFT;
>
> switch (hsr_ec) {
> + case HSR_EC_SOFT_STEP:
> + if (cs->singlestep_enabled) {
> + return true;
> + } else {
> + error_report("Came out of SINGLE STEP when not enabled");
This can only happen if there's a kernel bug, right?
> + }
> + break;
> case HSR_EC_SW_BKPT:
> if (kvm_find_sw_breakpoint(cs, arch_info->pc)) {
> return true;
> @@ -542,6 +550,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
>
> void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> {
> + if (cs->singlestep_enabled) {
> + dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
> + }
> if (kvm_sw_breakpoints_active(cs)) {
> dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> }
> --
> 2.3.4
>
thanks
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] target-arm: kvm - add support for HW assisted debug
2015-03-31 15:40 ` [PATCH v2 4/4] target-arm: kvm - add support for HW assisted debug Alex Bennée
@ 2015-04-20 20:23 ` Peter Maydell
2015-04-21 13:08 ` Alex Bennée
0 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2015-04-20 20:23 UTC (permalink / raw)
To: Alex Bennée
Cc: QEMU Developers, Christoffer Dall, Zhichao Huang, kvm-devel,
arm-mail-list, kvmarm, Marc Zyngier, Alex Bennée,
Paolo Bonzini
On 31 March 2015 at 16:40, Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Alex Bennée <alex@bennee.com>
>
> This adds basic support for HW assisted debug. The ioctl interface to
> KVM allows us to pass an implementation defined number of break and
> watch point registers. When KVM_GUESTDBG_USE_HW_BP is specified these
> debug registers will be installed in place on the world switch into the
> guest.
>
> The hardware is actually capable of more advanced matching but it is
> unclear if this expressiveness is available via the gdbstub protocol.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
> - correct setting of PMC/BAS/MASK
> - improved commentary
> - added helper function to check watchpoint in range
> - fix find/deletion of watchpoints
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index ae0f8b2..d1adf5f 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -17,6 +17,7 @@
>
> #include "qemu-common.h"
> #include "qemu/timer.h"
> +#include "qemu/error-report.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/kvm.h"
> #include "kvm_arm.h"
> @@ -476,6 +477,8 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>
> #define HSR_EC_SHIFT 26
> #define HSR_EC_SOFT_STEP 0x32
> +#define HSR_EC_HW_BKPT 0x30
> +#define HSR_EC_HW_WATCH 0x34
> #define HSR_EC_SW_BKPT 0x3c
>
> static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
> @@ -496,6 +499,16 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
> return true;
> }
> break;
> + case HSR_EC_HW_BKPT:
> + if (kvm_arm_find_hw_breakpoint(cs, arch_info->pc)) {
> + return true;
> + }
> + break;
> + case HSR_EC_HW_WATCH:
> + if (kvm_arm_find_hw_watchpoint(cs, arch_info->far)) {
> + return true;
> + }
> + break;
> default:
> error_report("%s: unhandled debug exit (%x, %llx)\n",
> __func__, arch_info->hsr, arch_info->pc);
> @@ -556,6 +569,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> if (kvm_sw_breakpoints_active(cs)) {
> dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> }
> + if (kvm_hw_breakpoints_active(cs)) {
> + dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
> + kvm_copy_hw_breakpoint_data(&dbg->arch);
> + }
> }
>
> /* C6.6.29 BRK instruction */
> @@ -582,26 +599,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> return 0;
> }
>
> -int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> - target_ulong len, int type)
> -{
> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> - return -EINVAL;
> -}
> -
> -int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> - target_ulong len, int type)
> -{
> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> - return -EINVAL;
> -}
> -
> -
> -void kvm_arch_remove_all_hw_breakpoints(void)
> -{
> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -}
> -
> void kvm_arch_init_irq_routing(KVMState *s)
> {
> }
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 8cf3a62..dbe81a7 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -2,6 +2,7 @@
> * ARM implementation of KVM hooks, 64 bit specific code
> *
> * Copyright Mian-M. Hamayun 2013, Virtual Open Systems
> + * Copyright Alex Bennée 2014, Linaro
> *
> * This work is licensed under the terms of the GNU GPL, version 2 or later.
> * See the COPYING file in the top-level directory.
> @@ -12,11 +13,17 @@
> #include <sys/types.h>
> #include <sys/ioctl.h>
> #include <sys/mman.h>
> +#include <sys/ptrace.h>
> +#include <asm/ptrace.h>
We really need the asm/ include ?
> +#include <linux/elf.h>
> #include <linux/kvm.h>
>
> #include "qemu-common.h"
> #include "qemu/timer.h"
> +#include "qemu/host-utils.h"
> +#include "qemu/error-report.h"
> +#include "exec/gdbstub.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/kvm.h"
> #include "kvm_arm.h"
> @@ -24,6 +31,312 @@
> #include "internals.h"
> #include "hw/arm/arm.h"
>
> +/* Max and current break/watch point counts */
> +int max_hw_bp, max_hw_wp;
> +int cur_hw_bp, cur_hw_wp;
> +struct kvm_guest_debug_arch guest_debug_registers;
How does this work in an SMP guest?
> +
> +/**
> + * kvm_arm_init_debug()
> + * @cs: CPUState
> + *
> + * kvm_check_extension returns 0 if we have no debug registers or the
> + * number we have.
> + *
> + */
> +static void kvm_arm_init_debug(CPUState *cs)
> +{
> + max_hw_wp = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
> + max_hw_bp = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
> + return;
> +}
> +
> +/**
> + * insert_hw_breakpoint()
> + * @addr: address of breakpoint
> + *
> + * See ARM ARM D2.9.1 for details but here we are only going to create
> + * simple un-linked breakpoints (i.e. we don't chain breakpoints
> + * together to match address and context or vmid). The hardware is
> + * 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
> + *
> + * 31 24 23 20 19 16 15 14 13 12 9 8 5 4 3 2 1 0
> + * +------+------+-------+-----+----+------+-----+------+-----+---+
> + * | RES0 | BT | LBN | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
> + * +------+------+-------+-----+----+------+-----+------+-----+---+
> + *
> + * BT: Breakpoint type (0 = unlinked address match)
> + * LBN: Linked BP number (0 = unused)
> + * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
> + * BAS: Byte Address Select (RES1 for AArch64)
> + * E: Enable bit
> + */
> +static int insert_hw_breakpoint(target_ulong addr)
> +{
> + uint32_t bcr = 0x1; /* E=1, enable */
> + if (cur_hw_bp >= max_hw_bp) {
> + return -ENOBUFS;
> + }
> + bcr = deposit32(bcr, 1, 2, 0x3); /* PMC = 11 */
> + bcr = deposit32(bcr, 5, 4, 0xf); /* BAS = RES1 */
> + guest_debug_registers.dbg_bcr[cur_hw_bp] = bcr;
> + guest_debug_registers.dbg_bvr[cur_hw_bp] = addr;
> + cur_hw_bp++;
> + return 0;
> +}
> +
> +/**
> + * delete_hw_breakpoint()
> + * @pc: address of breakpoint
> + *
> + * Delete a breakpoint and shuffle any above down
> + */
> +
> +static int delete_hw_breakpoint(target_ulong pc)
> +{
> + int i;
> + for (i = 0; i < cur_hw_bp; i++) {
> + if (guest_debug_registers.dbg_bvr[i] == pc) {
> + while (i < cur_hw_bp) {
> + if (i == max_hw_bp) {
> + guest_debug_registers.dbg_bvr[i] = 0;
> + guest_debug_registers.dbg_bcr[i] = 0;
> + } else {
> + guest_debug_registers.dbg_bvr[i] =
> + guest_debug_registers.dbg_bvr[i + 1];
> + guest_debug_registers.dbg_bcr[i] =
> + guest_debug_registers.dbg_bcr[i + 1];
Why do we need to shuffle all the registers down
rather than just clearing the enable bit on the one
we stopped using?
> + }
> + i++;
> + }
> + cur_hw_bp--;
> + return 0;
> + }
> + }
> + return -ENOENT;
> +}
> +
> +/**
> + * insert_hw_watchpoint()
> + * @addr: address of watch point
> + * @len: size of area
> + * @type: type of watch point
> + *
> + * See ARM ARM D2.10. As with the breakpoints we can do some advanced
> + * stuff if we want to. The watch points can be linked with the break
> + * points above to make them context aware. However for simplicity
> + * currently we only deal with simple read/write watch points.
> + *
> + * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
> + *
> + * 31 29 28 24 23 21 20 19 16 15 14 13 12 5 4 3 2 1 0
> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
> + * | RES0 | MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
> + *
> + * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
> + * WT: 0 - unlinked, 1 - linked (not currently used)
> + * LBN: Linked BP number (not currently used)
> + * SSC/HMC/PAC: Security, Higher and Priv access control (Table D-12)
> + * BAS: Byte Address Select
> + * LSC: Load/Store control (01: load, 10: store, 11: both)
> + * E: Enable
> + *
> + * The bottom 2 bits of the value register are masked. Therefor to
"Therefore"
> + * break on an sizes smaller than unaligned byte you need to set
"any". Also what's a size smaller than an unaligned byte? :-)
> + * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
> + * need to ensure you mask the address as required and set BAS=0xff
> + */
> +
> +static int insert_hw_watchpoint(target_ulong addr,
> + target_ulong len, int type)
> +{
> + uint32_t dbgwcr = 1; /* E=1, enable */
> + uint64_t dbgwvr = addr & (~0x7ULL);
> +
> + if (cur_hw_wp >= max_hw_wp) {
> + return -ENOBUFS;
> + }
> +
> + /* PAC 00 is reserved, assume EL1 exception */
Not sure what this comment is trying to say. PAC value 00 is
reserved, but you're not trying to set it to 00 so that's OK.
> + dbgwcr = deposit32(dbgwcr, 1, 2, 1);
Why a PAC value of 1 ? That means we will only get watchpoint
hits for accesses from EL1, which doesn't really seem like
what we want. What you want I think is HMC=0 SSC=0 PAC=3,
which will give you hits for EL0 or EL1, any security state,
valid whether EL3 is implemented or not.
> +
> + switch (type) {
> + case GDB_WATCHPOINT_READ:
> + dbgwcr = deposit32(dbgwcr, 3, 2, 1);
> + break;
> + case GDB_WATCHPOINT_WRITE:
> + dbgwcr = deposit32(dbgwcr, 3, 2, 2);
> + break;
> + case GDB_WATCHPOINT_ACCESS:
> + dbgwcr = deposit32(dbgwcr, 3, 2, 3);
> + break;
> + default:
> + g_assert_not_reached();
> + break;
> + }
> + if (len <= 8) {
> + /* we align the address and set the bits in BAS */
> + int off = addr & 0x7;
> + int bas = (1 << len)-1;
> + dbgwcr = deposit32(dbgwcr, 5+off, 8-off, bas);
Missing spaces.
Given the way deposit32() works you could also say
end = MIN(off + len, 8);
dbgwcr = deposit32(dbgwcr, 5 + off, 5 + end, ~0);
which involves a little less manual bit-twiddling.
Note that if the unaligned watchpoint we're trying to set
straddles a doubleword boundary (eg a 4-byte watchpoint at
0x1006) we'll only catch accesses to the first part of it.
That may be OK, I forget what the gdbstub's required
semantics for larger-than-a-byte watchpoints are. If not
you'll need two watchpoint regs to cover both parts.
> + } else {
> + /* For ranges above 8 bytes we need to be a power of 2 */
> + if (ctpop64(len)==1) {
If you want to test whether a number is a power of 2 then
you can use is_power_of_2()...
> + int bits = ctz64(len);
> + dbgwvr &= ~((1 << bits)-1);
More spacing issues.
(Can you actually set a large-range watchpoint with gdb?)
> + dbgwcr = deposit32(dbgwcr, 24, 4, bits);
> + dbgwcr = deposit32(dbgwcr, 5, 8, 0xff);
> + } else {
> + return -ENOBUFS;
> + }
> + }
> +
> + guest_debug_registers.dbg_wcr[cur_hw_wp] = dbgwcr;
> + guest_debug_registers.dbg_wvr[cur_hw_wp] = dbgwvr;
> + cur_hw_wp++;
> + return 0;
> +}
> +
> +
> +static bool check_watchpoint_in_range(int i, target_ulong addr)
> +{
> + uint32_t dbgwcr = guest_debug_registers.dbg_wcr[i];
> + uint64_t addr_top, addr_bottom = guest_debug_registers.dbg_wvr[i];
> + int bas = extract32(dbgwcr, 5, 8);
> + int mask = extract32(dbgwcr, 24, 4);
> +
> + if (mask) {
> + addr_top = addr_bottom + (1 << mask);
> + } else {
> + /* BAS must be contiguous but can offset against the base
> + * address in DBGWVR */
> + addr_bottom = addr_bottom + ctz32(bas);
> + addr_top = addr_bottom + clo32(bas);
> + }
> +
> + if (addr >= addr_bottom && addr <= addr_top ) {
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> + * delete_hw_watchpoint()
> + * @addr: address of breakpoint
> + *
> + * Delete a breakpoint and shuffle any above down
> + */
> +
> +static int delete_hw_watchpoint(target_ulong addr,
> + target_ulong len, int type)
> +{
> + int i;
> + for (i = 0; i < cur_hw_wp; i++) {
> + if (check_watchpoint_in_range(i, addr)) {
> + while (i < cur_hw_wp) {
> + if (i == max_hw_wp) {
> + guest_debug_registers.dbg_wvr[i] = 0;
> + guest_debug_registers.dbg_wcr[i] = 0;
> + } else {
> + guest_debug_registers.dbg_wvr[i] =
> + guest_debug_registers.dbg_wvr[i + 1];
> + guest_debug_registers.dbg_wcr[i] =
> + guest_debug_registers.dbg_wcr[i + 1];
> + }
> + i++;
> + }
> + cur_hw_wp--;
> + return 0;
> + }
> + }
> + return -ENOENT;
> +}
> +
> +
> +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> + target_ulong len, int type)
> +{
> + switch (type) {
> + case GDB_BREAKPOINT_HW:
> + return insert_hw_breakpoint(addr);
> + break;
> + case GDB_WATCHPOINT_READ:
> + case GDB_WATCHPOINT_WRITE:
> + case GDB_WATCHPOINT_ACCESS:
> + return insert_hw_watchpoint(addr, len, type);
> + default:
> + return -ENOSYS;
> + }
> +}
> +
> +int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> + target_ulong len, int type)
> +{
> + switch (type) {
> + case GDB_BREAKPOINT_HW:
> + return delete_hw_breakpoint(addr);
> + break;
> + case GDB_WATCHPOINT_READ:
> + case GDB_WATCHPOINT_WRITE:
> + case GDB_WATCHPOINT_ACCESS:
> + return delete_hw_watchpoint(addr, len, type);
> + default:
> + return -ENOSYS;
> + }
> +}
> +
> +
> +void kvm_arch_remove_all_hw_breakpoints(void)
> +{
> + memset((void *)&guest_debug_registers, 0, sizeof(guest_debug_registers));
Do you really need this void* cast?
> + cur_hw_bp = 0;
> + cur_hw_wp = 0;
> +}
> +
> +void kvm_copy_hw_breakpoint_data(struct kvm_guest_debug_arch *ptr)
> +{
> + /* Compile time assert? */
Better than runtime, but why assert at all, given that we've
simply defined guest_debug_registers to have the right type earlier
and there's nothing likely to make that go subtly wrong?
> + g_assert(sizeof(struct kvm_guest_debug_arch) == sizeof(guest_debug_registers));
> + memcpy(ptr, &guest_debug_registers, sizeof(guest_debug_registers));
> +}
> +
> +bool kvm_hw_breakpoints_active(CPUState *cs)
> +{
> + return ( (cur_hw_bp > 0) || (cur_hw_wp >0) ) ? TRUE:FALSE;
You've been reading too much softfloat code, this spacing
style is atrocious :-) Also, true, not TRUE.
> +}
> +
> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
> +{
> + if (kvm_hw_breakpoints_active(cpu)) {
> + int i;
> + for (i=0; i<cur_hw_bp; i++) {
> + if (guest_debug_registers.dbg_bvr[i] == pc) {
> + return true;
> + }
> + }
> + }
> + return false;
> +}
> +
> +bool kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
> +{
> + if (kvm_hw_breakpoints_active(cpu)) {
> + int i;
> + for (i=0; i<cur_hw_wp; i++) {
Spaces!
> + if (check_watchpoint_in_range(i, addr)) {
> + return true;
> + }
> + }
> + }
> + return false;
> +}
> +
> +
> static inline void set_feature(uint64_t *features, int feature)
> {
> *features |= 1ULL << feature;
> @@ -106,6 +419,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
> return ret;
> }
>
> + kvm_arm_init_debug(cs);
> +
> return kvm_arm_init_cpreg_list(cpu);
> }
>
> diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
> index 455dea3..a4b480b 100644
> --- a/target-arm/kvm_arm.h
> +++ b/target-arm/kvm_arm.h
> @@ -162,6 +162,27 @@ typedef struct ARMHostCPUClass {
> */
> bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc);
>
> +bool kvm_hw_breakpoints_active(CPUState *cs);
> +void kvm_copy_hw_breakpoint_data(struct kvm_guest_debug_arch *ptr);
If these aren't common-to-all-CPUs functions they should
have an _arm_ in their names. (Otherwise you'd expect
to find kvm_hw_breakpoints_active() next to
kvm_sw_breakpoints_active() in kvm-all.c.)
Also, doc comments for the functions would be nice.
> +
> +/**
> + * kvm_arm_find_hw_breakpoint:
> + * @cpu: CPUState
> + * @pc: pc of breakpoint
> + *
> + * Return TRUE if the pc matches one of our breakpoints.
> + */
> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
> +
> +/**
> + * kvm_arm_find_hw_watchpoint:
> + * @cpu: CPUState
> + * @addr: address of watchpoint
> + *
> + * Return TRUE if the addr matches one of our watchpoints.
> + */
> +bool kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
> +
> #endif
>
> #endif
thanks
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] target-arm: kvm - support for single step
2015-04-20 19:49 ` Peter Maydell
@ 2015-04-21 12:56 ` Alex Bennée
2015-04-21 13:01 ` Peter Maydell
0 siblings, 1 reply; 12+ messages in thread
From: Alex Bennée @ 2015-04-21 12:56 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Christoffer Dall, Zhichao Huang, kvm-devel,
arm-mail-list, kvmarm, Marc Zyngier, Paolo Bonzini
Peter Maydell <peter.maydell@linaro.org> writes:
> On 31 March 2015 at 16:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>> This adds support for single-step. There isn't much to do on the QEMU
>> side as after we set-up the request for single step via the debug ioctl
>> it is all handled within the kernel.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>> - convert to using HSR_EC
>>
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index 290c1fe..ae0f8b2 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -475,6 +475,7 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>> */
>>
>> #define HSR_EC_SHIFT 26
>> +#define HSR_EC_SOFT_STEP 0x32
>> #define HSR_EC_SW_BKPT 0x3c
>
> We already include internals.h in this file, so can you just use
> the EC_* constants and ARM_EL_EC_SHIFT rather than defining
> new ones? (Applies for patch 1 as well.)
>
>> static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>> @@ -483,6 +484,13 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>> int hsr_ec = arch_info->hsr >> HSR_EC_SHIFT;
>>
>> switch (hsr_ec) {
>> + case HSR_EC_SOFT_STEP:
>> + if (cs->singlestep_enabled) {
>> + return true;
>> + } else {
>> + error_report("Came out of SINGLE STEP when not enabled");
>
> This can only happen if there's a kernel bug, right?
Sure. Should we report it differently? abort() out?
>
>> + }
>> + break;
>> case HSR_EC_SW_BKPT:
>> if (kvm_find_sw_breakpoint(cs, arch_info->pc)) {
>> return true;
>> @@ -542,6 +550,9 @@ int kvm_arch_on_sigbus(int code, void *addr)
>>
>> void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>> {
>> + if (cs->singlestep_enabled) {
>> + dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
>> + }
>> if (kvm_sw_breakpoints_active(cs)) {
>> dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>> }
>> --
>> 2.3.4
>>
>
>
> thanks
> -- PMM
--
Alex Bennée
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] target-arm: kvm - support for single step
2015-04-21 12:56 ` Alex Bennée
@ 2015-04-21 13:01 ` Peter Maydell
0 siblings, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2015-04-21 13:01 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm-devel, Marc Zyngier, QEMU Developers, Zhichao Huang,
Paolo Bonzini, kvmarm, arm-mail-list
On 21 April 2015 at 13:56, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>>> switch (hsr_ec) {
>>> + case HSR_EC_SOFT_STEP:
>>> + if (cs->singlestep_enabled) {
>>> + return true;
>>> + } else {
>>> + error_report("Came out of SINGLE STEP when not enabled");
>>
>> This can only happen if there's a kernel bug, right?
>
> Sure. Should we report it differently? abort() out?
Saying 'This would be a kernel bug' in a comment would do...
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] target-arm: kvm - add support for HW assisted debug
2015-04-20 20:23 ` Peter Maydell
@ 2015-04-21 13:08 ` Alex Bennée
0 siblings, 0 replies; 12+ messages in thread
From: Alex Bennée @ 2015-04-21 13:08 UTC (permalink / raw)
To: Peter Maydell
Cc: kvm-devel, Marc Zyngier, QEMU Developers, Zhichao Huang,
Paolo Bonzini, kvmarm, arm-mail-list
Peter Maydell <peter.maydell@linaro.org> writes:
> On 31 March 2015 at 16:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>> From: Alex Bennée <alex@bennee.com>
>>
>> This adds basic support for HW assisted debug. The ioctl interface to
>> KVM allows us to pass an implementation defined number of break and
>> watch point registers. When KVM_GUESTDBG_USE_HW_BP is specified these
>> debug registers will be installed in place on the world switch into the
>> guest.
>>
>> The hardware is actually capable of more advanced matching but it is
>> unclear if this expressiveness is available via the gdbstub protocol.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>> - correct setting of PMC/BAS/MASK
>> - improved commentary
>> - added helper function to check watchpoint in range
>> - fix find/deletion of watchpoints
>>
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index ae0f8b2..d1adf5f 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -17,6 +17,7 @@
>>
>> #include "qemu-common.h"
>> #include "qemu/timer.h"
>> +#include "qemu/error-report.h"
>> #include "sysemu/sysemu.h"
>> #include "sysemu/kvm.h"
>> #include "kvm_arm.h"
>> @@ -476,6 +477,8 @@ void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>>
>> #define HSR_EC_SHIFT 26
>> #define HSR_EC_SOFT_STEP 0x32
>> +#define HSR_EC_HW_BKPT 0x30
>> +#define HSR_EC_HW_WATCH 0x34
>> #define HSR_EC_SW_BKPT 0x3c
>>
>> static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>> @@ -496,6 +499,16 @@ static int kvm_handle_debug(CPUState *cs, struct kvm_run *run)
>> return true;
>> }
>> break;
>> + case HSR_EC_HW_BKPT:
>> + if (kvm_arm_find_hw_breakpoint(cs, arch_info->pc)) {
>> + return true;
>> + }
>> + break;
>> + case HSR_EC_HW_WATCH:
>> + if (kvm_arm_find_hw_watchpoint(cs, arch_info->far)) {
>> + return true;
>> + }
>> + break;
>> default:
>> error_report("%s: unhandled debug exit (%x, %llx)\n",
>> __func__, arch_info->hsr, arch_info->pc);
>> @@ -556,6 +569,10 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>> if (kvm_sw_breakpoints_active(cs)) {
>> dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>> }
>> + if (kvm_hw_breakpoints_active(cs)) {
>> + dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
>> + kvm_copy_hw_breakpoint_data(&dbg->arch);
>> + }
>> }
>>
>> /* C6.6.29 BRK instruction */
>> @@ -582,26 +599,6 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
>> return 0;
>> }
>>
>> -int kvm_arch_insert_hw_breakpoint(target_ulong addr,
>> - target_ulong len, int type)
>> -{
>> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
>> - return -EINVAL;
>> -}
>> -
>> -int kvm_arch_remove_hw_breakpoint(target_ulong addr,
>> - target_ulong len, int type)
>> -{
>> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
>> - return -EINVAL;
>> -}
>> -
>> -
>> -void kvm_arch_remove_all_hw_breakpoints(void)
>> -{
>> - qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
>> -}
>> -
>> void kvm_arch_init_irq_routing(KVMState *s)
>> {
>> }
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index 8cf3a62..dbe81a7 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -2,6 +2,7 @@
>> * ARM implementation of KVM hooks, 64 bit specific code
>> *
>> * Copyright Mian-M. Hamayun 2013, Virtual Open Systems
>> + * Copyright Alex Bennée 2014, Linaro
>> *
>> * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> * See the COPYING file in the top-level directory.
>> @@ -12,11 +13,17 @@
>> #include <sys/types.h>
>> #include <sys/ioctl.h>
>> #include <sys/mman.h>
>> +#include <sys/ptrace.h>
>> +#include <asm/ptrace.h>
>
> We really need the asm/ include ?
>
>> +#include <linux/elf.h>
>> #include <linux/kvm.h>
>>
>> #include "qemu-common.h"
>> #include "qemu/timer.h"
>> +#include "qemu/host-utils.h"
>> +#include "qemu/error-report.h"
>> +#include "exec/gdbstub.h"
>> #include "sysemu/sysemu.h"
>> #include "sysemu/kvm.h"
>> #include "kvm_arm.h"
>> @@ -24,6 +31,312 @@
>> #include "internals.h"
>> #include "hw/arm/arm.h"
>>
>> +/* Max and current break/watch point counts */
>> +int max_hw_bp, max_hw_wp;
>> +int cur_hw_bp, cur_hw_wp;
>> +struct kvm_guest_debug_arch guest_debug_registers;
>
> How does this work in an SMP guest?
Badly I guess ;-)
Interesting problem though because GDB only thinks in terms of n
breakpoints rather than a per-cpu quantity so I guess we just ensure we
have the same set of debug values across all the vcpus?
>
>> +
>> +/**
>> + * kvm_arm_init_debug()
>> + * @cs: CPUState
>> + *
>> + * kvm_check_extension returns 0 if we have no debug registers or the
>> + * number we have.
>> + *
>> + */
>> +static void kvm_arm_init_debug(CPUState *cs)
>> +{
>> + max_hw_wp = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS);
>> + max_hw_bp = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS);
>> + return;
>> +}
>> +
>> +/**
>> + * insert_hw_breakpoint()
>> + * @addr: address of breakpoint
>> + *
>> + * See ARM ARM D2.9.1 for details but here we are only going to create
>> + * simple un-linked breakpoints (i.e. we don't chain breakpoints
>> + * together to match address and context or vmid). The hardware is
>> + * 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
>> + *
>> + * 31 24 23 20 19 16 15 14 13 12 9 8 5 4 3 2 1 0
>> + * +------+------+-------+-----+----+------+-----+------+-----+---+
>> + * | RES0 | BT | LBN | SSC | HMC| RES0 | BAS | RES0 | PMC | E |
>> + * +------+------+-------+-----+----+------+-----+------+-----+---+
>> + *
>> + * BT: Breakpoint type (0 = unlinked address match)
>> + * LBN: Linked BP number (0 = unused)
>> + * SSC/HMC/PMC: Security, Higher and Priv access control (Table D-12)
>> + * BAS: Byte Address Select (RES1 for AArch64)
>> + * E: Enable bit
>> + */
>> +static int insert_hw_breakpoint(target_ulong addr)
>> +{
>> + uint32_t bcr = 0x1; /* E=1, enable */
>> + if (cur_hw_bp >= max_hw_bp) {
>> + return -ENOBUFS;
>> + }
>> + bcr = deposit32(bcr, 1, 2, 0x3); /* PMC = 11 */
>> + bcr = deposit32(bcr, 5, 4, 0xf); /* BAS = RES1 */
>> + guest_debug_registers.dbg_bcr[cur_hw_bp] = bcr;
>> + guest_debug_registers.dbg_bvr[cur_hw_bp] = addr;
>> + cur_hw_bp++;
>> + return 0;
>> +}
>> +
>> +/**
>> + * delete_hw_breakpoint()
>> + * @pc: address of breakpoint
>> + *
>> + * Delete a breakpoint and shuffle any above down
>> + */
>> +
>> +static int delete_hw_breakpoint(target_ulong pc)
>> +{
>> + int i;
>> + for (i = 0; i < cur_hw_bp; i++) {
>> + if (guest_debug_registers.dbg_bvr[i] == pc) {
>> + while (i < cur_hw_bp) {
>> + if (i == max_hw_bp) {
>> + guest_debug_registers.dbg_bvr[i] = 0;
>> + guest_debug_registers.dbg_bcr[i] = 0;
>> + } else {
>> + guest_debug_registers.dbg_bvr[i] =
>> + guest_debug_registers.dbg_bvr[i + 1];
>> + guest_debug_registers.dbg_bcr[i] =
>> + guest_debug_registers.dbg_bcr[i + 1];
>
> Why do we need to shuffle all the registers down
> rather than just clearing the enable bit on the one
> we stopped using?
It keeps it simpler when cur_hw_bp et all are both a count and an index.
This is also saves you from having to shuffle them around when packing
the $IMPDEF debug registers to the start of the array.
>
>> + }
>> + i++;
>> + }
>> + cur_hw_bp--;
>> + return 0;
>> + }
>> + }
>> + return -ENOENT;
>> +}
>> +
>> +/**
>> + * insert_hw_watchpoint()
>> + * @addr: address of watch point
>> + * @len: size of area
>> + * @type: type of watch point
>> + *
>> + * See ARM ARM D2.10. As with the breakpoints we can do some advanced
>> + * stuff if we want to. The watch points can be linked with the break
>> + * points above to make them context aware. However for simplicity
>> + * currently we only deal with simple read/write watch points.
>> + *
>> + * D7.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers
>> + *
>> + * 31 29 28 24 23 21 20 19 16 15 14 13 12 5 4 3 2 1 0
>> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
>> + * | RES0 | MASK | RES0 | WT | LBN | SSC | HMC | BAS | LSC | PAC | E |
>> + * +------+-------+------+----+-----+-----+-----+-----+-----+-----+---+
>> + *
>> + * MASK: num bits addr mask (0=none,01/10=res,11=3 bits (8 bytes))
>> + * WT: 0 - unlinked, 1 - linked (not currently used)
>> + * LBN: Linked BP number (not currently used)
>> + * SSC/HMC/PAC: Security, Higher and Priv access control (Table D-12)
>> + * BAS: Byte Address Select
>> + * LSC: Load/Store control (01: load, 10: store, 11: both)
>> + * E: Enable
>> + *
>> + * The bottom 2 bits of the value register are masked. Therefor to
>
> "Therefore"
>
>> + * break on an sizes smaller than unaligned byte you need to set
>
> "any". Also what's a size smaller than an unaligned byte? :-)
oops, no nibble break points I guess ;-)
>
>> + * MASK=0, BAS=bit per byte in question. For larger regions (^2) you
>> + * need to ensure you mask the address as required and set BAS=0xff
>> + */
>> +
>> +static int insert_hw_watchpoint(target_ulong addr,
>> + target_ulong len, int type)
>> +{
>> + uint32_t dbgwcr = 1; /* E=1, enable */
>> + uint64_t dbgwvr = addr & (~0x7ULL);
>> +
>> + if (cur_hw_wp >= max_hw_wp) {
>> + return -ENOBUFS;
>> + }
>> +
>> + /* PAC 00 is reserved, assume EL1 exception */
>
> Not sure what this comment is trying to say. PAC value 00 is
> reserved, but you're not trying to set it to 00 so that's OK.
>
>> + dbgwcr = deposit32(dbgwcr, 1, 2, 1);
>
> Why a PAC value of 1 ? That means we will only get watchpoint
> hits for accesses from EL1, which doesn't really seem like
> what we want. What you want I think is HMC=0 SSC=0 PAC=3,
> which will give you hits for EL0 or EL1, any security state,
> valid whether EL3 is implemented or not.
Thanks - I didn't find the table as intuitive as I might...
>
>
>> +
>> + switch (type) {
>> + case GDB_WATCHPOINT_READ:
>> + dbgwcr = deposit32(dbgwcr, 3, 2, 1);
>> + break;
>> + case GDB_WATCHPOINT_WRITE:
>> + dbgwcr = deposit32(dbgwcr, 3, 2, 2);
>> + break;
>> + case GDB_WATCHPOINT_ACCESS:
>> + dbgwcr = deposit32(dbgwcr, 3, 2, 3);
>> + break;
>> + default:
>> + g_assert_not_reached();
>> + break;
>> + }
>> + if (len <= 8) {
>> + /* we align the address and set the bits in BAS */
>> + int off = addr & 0x7;
>> + int bas = (1 << len)-1;
>
>
>> + dbgwcr = deposit32(dbgwcr, 5+off, 8-off, bas);
>
> Missing spaces.
>
> Given the way deposit32() works you could also say
> end = MIN(off + len, 8);
> dbgwcr = deposit32(dbgwcr, 5 + off, 5 + end, ~0);
> which involves a little less manual bit-twiddling.
>
> Note that if the unaligned watchpoint we're trying to set
> straddles a doubleword boundary (eg a 4-byte watchpoint at
> 0x1006) we'll only catch accesses to the first part of it.
> That may be OK, I forget what the gdbstub's required
> semantics for larger-than-a-byte watchpoints are. If not
> you'll need two watchpoint regs to cover both parts.
>
>> + } else {
>> + /* For ranges above 8 bytes we need to be a power of 2 */
>> + if (ctpop64(len)==1) {
>
> If you want to test whether a number is a power of 2 then
> you can use is_power_of_2()...
>
>> + int bits = ctz64(len);
>> + dbgwvr &= ~((1 << bits)-1);
>
> More spacing issues.
>
> (Can you actually set a large-range watchpoint with gdb?)
I'll double check.
>
>> + dbgwcr = deposit32(dbgwcr, 24, 4, bits);
>> + dbgwcr = deposit32(dbgwcr, 5, 8, 0xff);
>> + } else {
>> + return -ENOBUFS;
>> + }
>> + }
>> +
>> + guest_debug_registers.dbg_wcr[cur_hw_wp] = dbgwcr;
>> + guest_debug_registers.dbg_wvr[cur_hw_wp] = dbgwvr;
>> + cur_hw_wp++;
>> + return 0;
>> +}
>> +
>> +
>> +static bool check_watchpoint_in_range(int i, target_ulong addr)
>> +{
>> + uint32_t dbgwcr = guest_debug_registers.dbg_wcr[i];
>> + uint64_t addr_top, addr_bottom = guest_debug_registers.dbg_wvr[i];
>> + int bas = extract32(dbgwcr, 5, 8);
>> + int mask = extract32(dbgwcr, 24, 4);
>> +
>> + if (mask) {
>> + addr_top = addr_bottom + (1 << mask);
>> + } else {
>> + /* BAS must be contiguous but can offset against the base
>> + * address in DBGWVR */
>> + addr_bottom = addr_bottom + ctz32(bas);
>> + addr_top = addr_bottom + clo32(bas);
>> + }
>> +
>> + if (addr >= addr_bottom && addr <= addr_top ) {
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +/**
>> + * delete_hw_watchpoint()
>> + * @addr: address of breakpoint
>> + *
>> + * Delete a breakpoint and shuffle any above down
>> + */
>> +
>> +static int delete_hw_watchpoint(target_ulong addr,
>> + target_ulong len, int type)
>> +{
>> + int i;
>> + for (i = 0; i < cur_hw_wp; i++) {
>> + if (check_watchpoint_in_range(i, addr)) {
>> + while (i < cur_hw_wp) {
>> + if (i == max_hw_wp) {
>> + guest_debug_registers.dbg_wvr[i] = 0;
>> + guest_debug_registers.dbg_wcr[i] = 0;
>> + } else {
>> + guest_debug_registers.dbg_wvr[i] =
>> + guest_debug_registers.dbg_wvr[i + 1];
>> + guest_debug_registers.dbg_wcr[i] =
>> + guest_debug_registers.dbg_wcr[i + 1];
>> + }
>> + i++;
>> + }
>> + cur_hw_wp--;
>> + return 0;
>> + }
>> + }
>> + return -ENOENT;
>> +}
>> +
>> +
>> +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
>> + target_ulong len, int type)
>> +{
>> + switch (type) {
>> + case GDB_BREAKPOINT_HW:
>> + return insert_hw_breakpoint(addr);
>> + break;
>> + case GDB_WATCHPOINT_READ:
>> + case GDB_WATCHPOINT_WRITE:
>> + case GDB_WATCHPOINT_ACCESS:
>> + return insert_hw_watchpoint(addr, len, type);
>> + default:
>> + return -ENOSYS;
>> + }
>> +}
>> +
>> +int kvm_arch_remove_hw_breakpoint(target_ulong addr,
>> + target_ulong len, int type)
>> +{
>> + switch (type) {
>> + case GDB_BREAKPOINT_HW:
>> + return delete_hw_breakpoint(addr);
>> + break;
>> + case GDB_WATCHPOINT_READ:
>> + case GDB_WATCHPOINT_WRITE:
>> + case GDB_WATCHPOINT_ACCESS:
>> + return delete_hw_watchpoint(addr, len, type);
>> + default:
>> + return -ENOSYS;
>> + }
>> +}
>> +
>> +
>> +void kvm_arch_remove_all_hw_breakpoints(void)
>> +{
>> + memset((void *)&guest_debug_registers, 0, sizeof(guest_debug_registers));
>
> Do you really need this void* cast?
>
>> + cur_hw_bp = 0;
>> + cur_hw_wp = 0;
>> +}
>> +
>> +void kvm_copy_hw_breakpoint_data(struct kvm_guest_debug_arch *ptr)
>> +{
>> + /* Compile time assert? */
>
> Better than runtime, but why assert at all, given that we've
> simply defined guest_debug_registers to have the right type earlier
> and there's nothing likely to make that go subtly wrong?
>
>> + g_assert(sizeof(struct kvm_guest_debug_arch) == sizeof(guest_debug_registers));
>> + memcpy(ptr, &guest_debug_registers, sizeof(guest_debug_registers));
>> +}
>> +
>> +bool kvm_hw_breakpoints_active(CPUState *cs)
>> +{
>> + return ( (cur_hw_bp > 0) || (cur_hw_wp >0) ) ? TRUE:FALSE;
>
> You've been reading too much softfloat code, this spacing
> style is atrocious :-) Also, true, not TRUE.
I'm going to have to investigate Emacs' style settings to see if I can
get the spacing automatically...
>
>> +}
>> +
>> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc)
>> +{
>> + if (kvm_hw_breakpoints_active(cpu)) {
>> + int i;
>> + for (i=0; i<cur_hw_bp; i++) {
>> + if (guest_debug_registers.dbg_bvr[i] == pc) {
>> + return true;
>> + }
>> + }
>> + }
>> + return false;
>> +}
>> +
>> +bool kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr)
>> +{
>> + if (kvm_hw_breakpoints_active(cpu)) {
>> + int i;
>> + for (i=0; i<cur_hw_wp; i++) {
>
> Spaces!
>
>> + if (check_watchpoint_in_range(i, addr)) {
>> + return true;
>> + }
>> + }
>> + }
>> + return false;
>> +}
>> +
>> +
>> static inline void set_feature(uint64_t *features, int feature)
>> {
>> *features |= 1ULL << feature;
>> @@ -106,6 +419,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> return ret;
>> }
>>
>> + kvm_arm_init_debug(cs);
>> +
>> return kvm_arm_init_cpreg_list(cpu);
>> }
>>
>> diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
>> index 455dea3..a4b480b 100644
>> --- a/target-arm/kvm_arm.h
>> +++ b/target-arm/kvm_arm.h
>> @@ -162,6 +162,27 @@ typedef struct ARMHostCPUClass {
>> */
>> bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc);
>>
>> +bool kvm_hw_breakpoints_active(CPUState *cs);
>> +void kvm_copy_hw_breakpoint_data(struct kvm_guest_debug_arch *ptr);
>
> If these aren't common-to-all-CPUs functions they should
> have an _arm_ in their names. (Otherwise you'd expect
> to find kvm_hw_breakpoints_active() next to
> kvm_sw_breakpoints_active() in kvm-all.c.)
OK thanks
>
> Also, doc comments for the functions would be nice.
>
>> +
>> +/**
>> + * kvm_arm_find_hw_breakpoint:
>> + * @cpu: CPUState
>> + * @pc: pc of breakpoint
>> + *
>> + * Return TRUE if the pc matches one of our breakpoints.
>> + */
>> +bool kvm_arm_find_hw_breakpoint(CPUState *cpu, target_ulong pc);
>> +
>> +/**
>> + * kvm_arm_find_hw_watchpoint:
>> + * @cpu: CPUState
>> + * @addr: address of watchpoint
>> + *
>> + * Return TRUE if the addr matches one of our watchpoints.
>> + */
>> +bool kvm_arm_find_hw_watchpoint(CPUState *cpu, target_ulong addr);
>> +
>> #endif
>>
>> #endif
>
> thanks
> -- PMM
--
Alex Bennée
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-04-21 13:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-31 15:40 [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
2015-03-31 15:40 ` [PATCH v2 1/4] linux-headers: partial sync from my kernel tree (DEV) Alex Bennée
2015-03-31 15:40 ` [PATCH v2 2/4] target-arm: kvm - implement software breakpoints Alex Bennée
2015-04-20 19:44 ` Peter Maydell
2015-03-31 15:40 ` [PATCH v2 3/4] target-arm: kvm - support for single step Alex Bennée
2015-04-20 19:49 ` Peter Maydell
2015-04-21 12:56 ` Alex Bennée
2015-04-21 13:01 ` Peter Maydell
2015-03-31 15:40 ` [PATCH v2 4/4] target-arm: kvm - add support for HW assisted debug Alex Bennée
2015-04-20 20:23 ` Peter Maydell
2015-04-21 13:08 ` Alex Bennée
2015-04-20 15:17 ` [PATCH v2 0/4] QEMU support for KVM Guest Debug on arm64 Alex Bennée
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).