> -----Original Message----- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Tuesday, June 24, 2014 6:50 PM > To: Bhushan Bharat-R65777 > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; ma...@linux.vnet.ibm.com > 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 <bharat.bhus...@freescale.com> > > --- > > 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 :( ? > > > + > > + 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 > I don't see where we synchonize DBSR. I will send a patch which synchromize DBSR. > > > } > > > > 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 Thanks -Bharat > > > Alex > > > + > > + if (nb_hw_breakpoint + nb_hw_watchpoint > 0) { > > + if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) { > > + n = find_hw_breakpoint(arch_info->address, > GDB_BREAKPOINT_HW); > > + if (n >= 0) { > > + handle = 1; > > + } > > + } else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ | > > + KVMPPC_DEBUG_WATCH_WRITE)) { > > + n = find_hw_watchpoint(arch_info->address, &flag); > > + if (n >= 0) { > > + handle = 1; > > + cs->watchpoint_hit = &hw_watchpoint; > > + hw_watchpoint.vaddr = hw_debug_points[n].addr; > > + hw_watchpoint.flags = flag; > > + } > > + } > > + } > > + } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) { > > handle = 1; > > } > > > > + if (handle) { > > + if (env->excp_model == POWERPC_EXCP_BOOKE) { > > + kvm_e500_handle_debug(cs, handle); > > + } > > + } else { > > + /* inject debug exception into guest */ > > + env->pending_interrupts |= 1 << PPC_INTERRUPT_DEBUG; > > + } > > return handle; > > } > > > > @@ -2101,20 +2333,6 @@ void kvm_arch_init_irq_routing(KVMState *s) > > { > > } > > > > -int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int > type) > > -{ > > - return -EINVAL; > > -} > > - > > -int kvm_arch_remove_hw_breakpoint(target_ulong addr, target_ulong len, int > type) > > -{ > > - return -EINVAL; > > -} > > - > > -void kvm_arch_remove_all_hw_breakpoints(void) > > -{ > > -} > > - > > struct kvm_get_htab_buf { > > struct kvm_get_htab_header header; > > /*