Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support

[address@hidden]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Tuesday, June 17, 2014 1:46 PM
> To: Bhushan Bharat-R65777; address@hidden; address@hidden
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> 
> 
> On 17.06.14 09:08, Bharat Bhushan wrote:
> > This patch adds software breakpoint, 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>
> > ---
> > v1->v2:
> >   - factored out e500 specific code based on exception model
> POWERPC_EXCP_BOOKE.
> >   - Not supporting ppc440
> >
> >   hw/ppc/e500.c        |   3 +
> >   target-ppc/kvm.c     | 355 
> > ++++++++++++++++++++++++++++++++++++++++++++++---
> --
> >   target-ppc/kvm_ppc.h |   1 +
> >   3 files changed, 330 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..47caa84
> > 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -853,6 +853,9 @@ void ppce500_init(MachineState *machine, PPCE500Params
> *params)
> >       if (kvm_enabled()) {
> >           kvmppc_init();
> >       }
> > +
> > +    /* E500 supports 2 h/w breakpoints and 2 watchpoints */
> > +    kvmppc_hw_breakpoint_init(2, 2);
> 
> This does not belong into the machine file.

What about calling this from init_proc_e500() in target-ppc/translate_init.c ?

> 
> >   }
> >
> >   static int e500_ccsr_initfn(SysBusDevice *dev) diff --git
> > a/target-ppc/kvm.c b/target-ppc/kvm.c index 70f77d1..994a618 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
> >
> > @@ -759,11 +760,55 @@ static int kvm_put_vpa(CPUState *cs)
> >   }
> >   #endif /* TARGET_PPC64 */
> >
> > -static int kvmppc_inject_debug_exception(CPUState *cs)
> > +static int kvmppc_e500_inject_debug_exception(CPUState *cs)
> >   {
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    struct kvm_sregs sregs;
> > +    int ret;
> > +
> > +    if (!cap_booke_sregs) {
> > +        return -1;
> > +    }
> > +
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
> > +    if (ret < 0) {
> > +        return -1;
> > +    }
> > +
> > +    if (sregs.u.e.features & KVM_SREGS_E_ED) {
> > +        sregs.u.e.dsrr0 = env->nip;
> > +        sregs.u.e.dsrr1 = env->msr;
> > +    } else {
> > +        sregs.u.e.csrr0 = env->nip;
> > +        sregs.u.e.csrr1 = env->msr;
> > +    }
> > +
> > +    sregs.u.e.update_special = KVM_SREGS_E_UPDATE_DBSR;
> > +    sregs.u.e.dbsr = env->spr[SPR_BOOKE_DBSR];
> > +
> > +    ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> > +    if (ret < 0) {
> > +        return -1;
> > +    }
> > +
> > +    env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DEBUG);
> 
> I think it makes sense to move this into kvmppc_inject_exception(). Then we 
> have
> everything dealing with pending_interrupts in one spot.

Will do

> 
> > +
> >       return 0;
> >   }
> >
> > +static int kvmppc_inject_debug_exception(CPUState *cs) {
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +
> > +    if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > +        return kvmppc_e500_inject_debug_exception(cs);
> > +    }
> 
> Yes, exactly the way I wanted to see it :). Please make this a switch though -
> that'll make it easier for others to plug in later.

Will do

> 
> > +
> > +    return -1;
> > +}
> > +
> >   static void kvmppc_inject_exception(CPUState *cs)
> >   {
> >       PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1268,6 +1313,276 @@
> > static int kvmppc_handle_dcr_write(CPUPPCState *env, uint32_t dcrn, uint32_t
> dat
> >       return 0;
> >   }
> >
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +    /* Mixed endian case is not handled */
> > +    uint32_t sc = debug_inst_opcode;
> > +
> > +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) 
> > ||
> > +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 1)) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +    uint32_t sc;
> > +
> > +    if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 0) ||
> > +        sc != debug_inst_opcode ||
> > +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) 
> > {
> > +        return -EINVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +#define MAX_HW_BKPTS 4
> > +
> > +static struct HWBreakpoint {
> > +    target_ulong addr;
> > +    int type;
> > +} hw_breakpoint[MAX_HW_BKPTS];
> 
> This struct contains both watchpoints and breakpoints, no? It really should be
> named accordingly. Maybe only call them points? Not sure :).

May be hw_debug_points/ hw_wb_points :)

> 
> > +
> > +static CPUWatchpoint hw_watchpoint;
> 
> What is this?

This struct needed to be passed to debugstub when watchpoint triggered. Please 
see debug_handler.

> 
> > +
> > +/* 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;
> > +
> > +void kvmppc_hw_breakpoint_init(int num_breakpoints, int
> > +num_watchpoints) {
> > +    if ((num_breakpoints + num_watchpoints) > MAX_HW_BKPTS) {
> > +        fprintf(stderr, "Error initializing h/w breakpints\n");
> 
> breakpoints?

"debug break/watch_points"

> 
> > +        return;
> > +    }
> > +
> > +    max_hw_breakpoint = num_breakpoints;
> > +    max_hw_watchpoint = num_watchpoints; }
> > +
> > +static int find_hw_breakpoint(target_ulong addr, int type) {
> > +    int n;
> > +
> > +    for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) {
> > +        if (hw_breakpoint[n].addr == addr && hw_breakpoint[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) {
> 
> Boundary check?

Yes, Good catch 

> 
> > +    hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr;
> > +    hw_breakpoint[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_breakpoint[n] = hw_breakpoint[nb_hw_breakpoint +
> > + nb_hw_watchpoint];
> > +
> > +    return 0;
> > +}
> > +
> > +void kvm_arch_remove_all_hw_breakpoints(void)
> > +{
> > +    nb_hw_breakpoint = nb_hw_watchpoint = 0; }
> > +
> > +static int kvm_e500_handle_debug(PowerPCCPU *cpu, struct kvm_run
> > +*run) {
> > +    CPUState *cs = CPU(cpu);
> > +    CPUPPCState *env = &cpu->env;
> > +    int handle = 0;
> > +    int n;
> > +    int flag = 0;
> > +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> > +
> > +    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_breakpoint[n].addr;
> > +                hw_watchpoint.flags = flag;
> > +            }
> > +        }
> > +    }
> 
> I think the above could easily be shared with book3s. Please put it into a
> helper function.

This is something I am not sure about, may be book3s was to interpret " struct 
kvm_debug_exit_arch *arch_info" in different way ?
So I left this booke specific. When someone implements h/w break/watch_point on 
book3s then he can decide to re-use this if it fits.

> 
> > +
> > +    cpu_synchronize_state(cs);
> > +    if (handle) {
> > +        env->spr[SPR_BOOKE_DBSR] = 0;
> > +    } else {
> > +       printf("unhandled\n");
> 
> This debug output would spawn every time the guest does in-guest debugging, 
> no?
> Please remove it.

Yes, Will remove

> 
> > +       /* inject debug exception into guest */
> > +       env->pending_interrupts |=  1 << PPC_INTERRUPT_DEBUG;
> > +    }
> > +
> > +    return handle;
> > +}
> > +
> > +static void kvm_arch_e500_update_guest_debug(CPUState *cs,
> > +                                             struct kvm_guest_debug
> > +*dbg) {
> > +    int n;
> > +
> > +    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++) {
> 
> Boundary check against dbg->arch.bp missing.

Did not get, what you mean by " dbg->arch.bp missing" ?

> 
> > +            switch (hw_breakpoint[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_breakpoint[n].addr;
> > +        }
> > +    }
> 
> I think this function is pretty universal, no?

Again I was not sure that about this, may be book3s wants to use "struct 
kvm_guest_debug {" differently. This has extension like DABRX etc, So may be 
they want to may then in this register. So I left to the developer to decide.

> 
> > +}
> > +
> > +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;
> > +
> > +    if (cs->singlestep_enabled) {
> > +        return 1;
> > +    } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> > +        return 1;
> > +    }
> > +
> > +    /* Hardware Breakpoints are architecture dependent */
> > +    if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > +        return kvm_e500_handle_debug(cpu, run);
> 
> Ah, you can just move most of the code you have in the e500 specific path down
> here. Just pass "handled" into kvm_e500_handle_debug().
> 
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug
> > +*dbg) {
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +
> > +    /* Software Breakpoint updates */
> > +    if (kvm_sw_breakpoints_active(cs)) {
> > +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> > +    }
> > +
> > +    /* Hardware Breakpoints are architecture dependent */
> 
> You're abstracting hardware breakpoints already so well that they're not 
> subarch
> dependent anymore :). As soon as they are in in the breakpoint array they are
> pretty much agnostic.
> 
> Please split this patch into smaller patches next time. I'm sure you can at
> least isolate software and hardware breakpoints. The patch as is is quite hard
> to review.

Sure.

Thanks
-Bharat

> 
> 
> Alex
> 
> > +    if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > +        kvm_arch_e500_update_guest_debug(cs, dbg);
> > +    }
> > +}
> > +
> >   int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >   {
> >       PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1308,6 +1623,16 @@ int
> > kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >           ret = 0;
> >           break;
> >
> > +    case KVM_EXIT_DEBUG:
> > +        DPRINTF("handle debug exception\n");
> > +        if (kvm_handle_debug(cpu, run)) {
> > +            ret = EXCP_DEBUG;
> > +            break;
> > +        }
> > +        /* re-enter, this exception was guest-internal */
> > +        ret = 0;
> > +        break;
> > +
> >       default:
> >           fprintf(stderr, "KVM: unknown exit reason %d\n", 
> > run->exit_reason);
> >           ret = -1;
> > @@ -1995,34 +2320,6 @@ void kvm_arch_init_irq_routing(KVMState *s)
> >   {
> >   }
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cpu, struct
> > kvm_sw_breakpoint *bp) -{
> > -    return -EINVAL;
> > -}
> > -
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cpu, struct
> > kvm_sw_breakpoint *bp) -{
> > -    return -EINVAL;
> > -}
> > -
> > -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)
> > -{
> > -}
> > -
> > -void kvm_arch_update_guest_debug(CPUState *cpu, struct
> > kvm_guest_debug *dbg) -{ -}
> > -
> >   struct kvm_get_htab_buf {
> >       struct kvm_get_htab_header header;
> >       /*
> > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h index
> > 412cc7f..bfc4a36 100644
> > --- a/target-ppc/kvm_ppc.h
> > +++ b/target-ppc/kvm_ppc.h
> > @@ -30,6 +30,7 @@ int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t
> tsr_bits);
> >   int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> >   int kvmppc_set_tcr(PowerPCCPU *cpu);
> >   int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
> > +void kvmppc_hw_breakpoint_init(int num_breakpoints, int
> > +num_watchpoints);
> >   #ifndef CONFIG_USER_ONLY
> >   off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem);
> >   bool kvmppc_spapr_use_multitce(void);