|
| From: | Alexander Graf |
| Subject: | Re: [Qemu-devel] [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support |
| Date: | Tue, 24 Jun 2014 16:50:52 +0200 |
| User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 |
On 24.06.14 16:37, address@hidden wrote:
-----Original Message----- From: Alexander Graf [mailto:address@hidden Sent: Tuesday, June 24, 2014 6:50 PM To: Bhushan Bharat-R65777 Cc: address@hidden; address@hidden; address@hidden Subject: Re: [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support On 24.06.14 14:10, Bharat Bhushan wrote:This patch adds hardware breakpoint and hardware watchpoint support for ppc. If the debug interrupt is not handled then this is injected to guest. Signed-off-by: Bharat Bhushan <address@hidden> --- v2->v3 - Shared as much code as much possible for futuristic book3s support - Initializing number of hw breakpoint/watchpoints from KVM world - Other minor cleanup/fixes target-ppc/kvm.c | 248 +++++++++++++++++++++++++++++++++++++++++++++++++++----1 file changed, 233 insertions(+), 15 deletions(-) diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 8e2dbb3..4fb0efd 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -38,6 +38,7 @@ #include "hw/ppc/ppc.h" #include "sysemu/watchdog.h" #include "trace.h" +#include "exec/gdbstub.h" //#define DEBUG_KVM @@ -410,6 +411,44 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu) return ppc_get_vcpu_dt_id(POWERPC_CPU(cpu)); } +/* e500 supports 2 h/w breakpoint and 2 watchpoint. + * book3s supports only 1 watchpoint, so array size + * of 4 is sufficient for now. + */ +#define MAX_HW_BKPTS 4 + +static struct HWBreakpoint { + target_ulong addr; + int type; +} hw_debug_points[MAX_HW_BKPTS]; + +static CPUWatchpoint hw_watchpoint; + +/* Default there is no breakpoint and watchpoint supported */ static +int max_hw_breakpoint; static int max_hw_watchpoint; static int +nb_hw_breakpoint; static int nb_hw_watchpoint; + +static void kvmppc_hw_debug_points_init(CPUPPCState *cenv) { + static bool initialize = true; + + if (initialize) { + if (cenv->excp_model == POWERPC_EXCP_BOOKE) { + max_hw_breakpoint = 2; + max_hw_watchpoint = 2; + } + + initialize = false; + } + + if ((max_hw_breakpoint + max_hw_watchpoint) > MAX_HW_BKPTS) { + fprintf(stderr, "Error initializing h/w breakpoints\n"); + return; + } +} + int kvm_arch_init_vcpu(CPUState *cs) { PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -437,6 +476,7 @@ int kvm_arch_init_vcpu(CPUState *cs) } kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode); + kvmppc_hw_debug_points_init(cenv); return ret; } @@ -1343,24 +1383,216 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs,struct kvm_sw_breakpoint *bp)return 0; } +static int find_hw_breakpoint(target_ulong addr, int type) { + int n; + + assert((nb_hw_breakpoint + nb_hw_watchpoint) + <= ARRAY_SIZE(hw_debug_points)); + + for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) { + if (hw_debug_points[n].addr == addr && hw_debug_points[n].type ==type) {+ return n; + } + } + + return -1; +} + +static int find_hw_watchpoint(target_ulong addr, int *flag) { + int n; + + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_ACCESS); + if (n >= 0) { + *flag = BP_MEM_ACCESS; + return n; + } + + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_WRITE); + if (n >= 0) { + *flag = BP_MEM_WRITE; + return n; + } + + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_READ); + if (n >= 0) { + *flag = BP_MEM_READ; + return n; + } + + return -1; +} + +int kvm_arch_insert_hw_breakpoint(target_ulong addr, + target_ulong len, int type) { + assert((nb_hw_breakpoint + nb_hw_watchpoint) + <= ARRAY_SIZE(hw_debug_points)); + + hw_debug_points[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr; + hw_debug_points[nb_hw_breakpoint + nb_hw_watchpoint].type = type;Imagine the following: nb_hw_breakpoint = 2 nb_hw_watchpoint = 2 The assert above succeeds, because 4 <= 4. However, the array shuffling below accesses memory that is out of bounds: hw_debug_points[4].Right, this is just " < "; but why not this crashed for me :( ?
Because running over arrays usually doesn't crash on you ;).
+ + switch (type) { + case GDB_BREAKPOINT_HW: + if (nb_hw_breakpoint >= max_hw_breakpoint) { + return -ENOBUFS; + } + + if (find_hw_breakpoint(addr, type) >= 0) { + return -EEXIST; + } + + nb_hw_breakpoint++; + break; + + case GDB_WATCHPOINT_WRITE: + case GDB_WATCHPOINT_READ: + case GDB_WATCHPOINT_ACCESS: + if (nb_hw_watchpoint >= max_hw_watchpoint) { + return -ENOBUFS; + } + + if (find_hw_breakpoint(addr, type) >= 0) { + return -EEXIST; + } + + nb_hw_watchpoint++; + break; + + default: + return -ENOSYS; + } + + return 0; +} + +int kvm_arch_remove_hw_breakpoint(target_ulong addr, + target_ulong len, int type) { + int n; + + n = find_hw_breakpoint(addr, type); + if (n < 0) { + return -ENOENT; + } + + switch (type) { + case GDB_BREAKPOINT_HW: + nb_hw_breakpoint--; + break; + + case GDB_WATCHPOINT_WRITE: + case GDB_WATCHPOINT_READ: + case GDB_WATCHPOINT_ACCESS: + nb_hw_watchpoint--; + break; + + default: + return -ENOSYS; + } + hw_debug_points[n] = hw_debug_points[nb_hw_breakpoint + + nb_hw_watchpoint]; + + return 0; +} + +void kvm_arch_remove_all_hw_breakpoints(void) +{ + nb_hw_breakpoint = nb_hw_watchpoint = 0; } + void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg) { + int n; + /* Software Breakpoint updates */ if (kvm_sw_breakpoints_active(cs)) { dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP; } + + assert((nb_hw_breakpoint + nb_hw_watchpoint) + <= ARRAY_SIZE(hw_debug_points)); + assert((nb_hw_breakpoint + nb_hw_watchpoint) <= + ARRAY_SIZE(dbg->arch.bp)); + + if (nb_hw_breakpoint + nb_hw_watchpoint > 0) { + dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP; + memset(dbg->arch.bp, 0, sizeof(dbg->arch.bp)); + for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) { + switch (hw_debug_points[n].type) { + case GDB_BREAKPOINT_HW: + dbg->arch.bp[n].type = KVMPPC_DEBUG_BREAKPOINT; + break; + case GDB_WATCHPOINT_WRITE: + dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_WRITE; + break; + case GDB_WATCHPOINT_READ: + dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_READ; + break; + case GDB_WATCHPOINT_ACCESS: + dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_WRITE | + KVMPPC_DEBUG_WATCH_READ; + break; + default: + cpu_abort(cs, "Unsupported breakpoint type\n"); + } + dbg->arch.bp[n].addr = hw_debug_points[n].addr; + } + } +} + +static void kvm_e500_handle_debug(CPUState *cs, int handle) { + PowerPCCPU *cpu = POWERPC_CPU(cs); + CPUPPCState *env = &cpu->env; + + cpu_synchronize_state(cs); + env->spr[SPR_BOOKE_DBSR] = 0;I don't see how this would take any effect with KVM?You mean we should move this to non-kvm; like excp_helper.c
No, I mean I don't see where we synchronize the register to actually take an effect.
I don't see where we synchonize DBSR.I will send a patch which synchromize DBSR.
We're already in KVM code anyway. Why not set it explicitly? You already do set it explicitly in
kvmppc_e500_inject_debug_exception(), no?
} static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) { CPUState *cs = CPU(cpu); + CPUPPCState *env = &cpu->env; struct kvm_debug_exit_arch *arch_info = &run->debug.arch; int handle = 0; + int n; + int flag = 0; - if (kvm_find_sw_breakpoint(cs, arch_info->address)) { + if (cs->singlestep_enabled) { + handle = 1; + } else if (arch_info->status) { + assert((nb_hw_breakpoint + nb_hw_watchpoint) + <= ARRAY_SIZE(hw_debug_points));I don't think this assert needs to be here :). You already assert() properly in the actual find function.The find function whet down in if-else
Yes, but we never access an array based on the offsets, so we're safe to only do it inside the find functions.
Alex
| [Prev in Thread] | Current Thread | [Next in Thread] |