| Message ID | 1403611807-7082-6-git-send-email-Bharat.Bhushan@freescale.com |
|---|---|
| State | New |
| Headers | show |
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.Bhushan@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]. > + > + 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? I don't see where we synchonize 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. 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; > /*
> -----Original Message----- > From: Alexander Graf [mailto:agraf@suse.de] > Sent: Tuesday, June 24, 2014 6:50 PM > To: Bhushan Bharat-R65777 > Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; maddy@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.Bhushan@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; > > /*
On 24.06.14 16:37, Bharat.Bhushan@freescale.com wrote: > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Tuesday, June 24, 2014 6:50 PM >> To: Bhushan Bharat-R65777 >> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; maddy@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.Bhushan@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 :( ? 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
> -----Original Message----- > From: Alexander Graf [mailto:agraf@suse.de] > Sent: Tuesday, June 24, 2014 8:21 PM > To: Bhushan Bharat-R65777 > Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; maddy@linux.vnet.ibm.com > Subject: Re: [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support > > > On 24.06.14 16:37, Bharat.Bhushan@freescale.com wrote: > > > >> -----Original Message----- > >> From: Alexander Graf [mailto:agraf@suse.de] > >> Sent: Tuesday, June 24, 2014 6:50 PM > >> To: Bhushan Bharat-R65777 > >> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; > >> maddy@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.Bhushan@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 :( ? > > 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? I think I did not get; please explain. > > > > > >>> } > >>> > >>> 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. You mean checking boundary conditions when setting breakpoint is sufficient and no need in debug handler. Thanks -Bharat > > > Alex
On 24.06.14 18:57, Bharat.Bhushan@freescale.com wrote: > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Tuesday, June 24, 2014 8:21 PM >> To: Bhushan Bharat-R65777 >> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; maddy@linux.vnet.ibm.com >> Subject: Re: [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support >> >> >> On 24.06.14 16:37, Bharat.Bhushan@freescale.com wrote: >>>> -----Original Message----- >>>> From: Alexander Graf [mailto:agraf@suse.de] >>>> Sent: Tuesday, June 24, 2014 6:50 PM >>>> To: Bhushan Bharat-R65777 >>>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; >>>> maddy@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.Bhushan@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 :( ? >> 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? > I think I did not get; please explain. The sequence cpu_synchronize_registers(env); env->spr[foo] = bar; is usually used to set an SPR in KVM. The nice thing about writing it like this is that it works with KVM and TCG just the same. However, the function we're talking about here is in KVM only code. So instead of sync'ing DBSR we can just set it manually. The only case where this could fall apart is when we want to support live migration. If you care about live migration, then make DBSR a kvm synchronized SPR with ONE_REG (we have nice infrastructure for that). But then don't set it manually in kvmppc_e500_inject_debug_exception() either, but only implicitly via env->spr[foo] values. > >> >>>>> } >>>>> >>>>> 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. > You mean checking boundary conditions when setting breakpoint is sufficient and no need in debug handler. I mean that the code below this assert always calls find_hw_breakpoint() which has the above assert and which is the only one that actually cares about the numbers because it actually uses them. Alex
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; + + 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; } 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)); + + 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; /*
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.Bhushan@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(-)