> -----Original Message-----
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 24, 2014 8:21 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 16:37, bharat.bhus...@freescale.com wrote:
> >
> >> -----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 :( ?
>
> 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
