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

[Alexander Graf]

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.

>   }
>   
>   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.

> +
>       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.

> +
> +    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 :).

> +
> +static CPUWatchpoint hw_watchpoint;

What is this?

> +
> +/* 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?

> +        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?

> +    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.

> +
> +    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.

> +       /* 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.

> +            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?

> +}
> +
> +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.


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);