Re: [Qemu-ppc] [PATCH 5/5 v5] ppc: Add hw breakpoint watchpoint support

[address@hidden]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Thursday, June 26, 2014 3:52 PM
> To: Bhushan Bharat-R65777
> Cc: address@hidden; address@hidden; address@hidden
> Subject: Re: [PATCH 5/5 v5] ppc: Add hw breakpoint watchpoint support
> 
> 
> On 26.06.14 09:23, 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>
> > ---
> > v4->v5
> >   - cleanup in kvmppc_hw_debug_points_init()
> >   - replaced assert in kvm_arch_insert_hw_breakpoint() with return.
> >     This allows gdb to through warning "You may have requested too many
> >     hardware breakpoints/watchpoints.". Now user can remove extra breakpint
> >     and continue.
> >
> >   target-ppc/kvm.c | 238 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++--
> --
> >   1 file changed, 223 insertions(+), 15 deletions(-)
> >
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> > 3195491..3347c1b 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,38 @@ 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) {
> > +    if (cenv->excp_model == POWERPC_EXCP_BOOKE) {
> > +        max_hw_breakpoint = 2;
> > +        max_hw_watchpoint = 2;
> > +    }
> > +
> > +    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 +470,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;
> >   }
> > @@ -1345,24 +1379,212 @@ 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) {
> > +    if ((nb_hw_breakpoint + nb_hw_watchpoint) >= 
> > ARRAY_SIZE(hw_debug_points))
> > +   return -ENOBUFS;
> > +
> > +    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;
> > +
> > +    env->spr[SPR_BOOKE_DBSR] = 0;
> 
> How would KVM ever know that DBSR is now 0?

Yes, guest will not come to know of this "0" value. I was wrong, thinking about 
that this will be used in inject_debug_exception(), but 
inject_debug_exception() is not called in this flow.

> And why do we need this in the first
> place? The guest's DBSR value doesn't get set on debug interrupts unless we 
> call
> inject_debug_exception().

vcpu->arch.dbsr is set on debug exception in KVM and that is what guest sees, 
so we should clear DBSR.

> So there's no need to clear it either, no?

So I think I need a one_reg interface to set/clear DBSR.

Also I do not need to explicitly set "sregs.u.e.dbsr" in 
inject_debug_exception(). 

Thanks
-Bharat

> 
> 
> Alex
> 
> >   }
> >
> >   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) {
> > +        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;
> >   }
> >
> > @@ -2103,20 +2325,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;
> >       /*