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

[Alexander Graf]

On 06/10/2014 05:06 PM, 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>
> ---
>   hw/ppc/e500.c        |   1 +
>   target-ppc/kvm.c     | 304 ++++++++++++++++++++++++++++++++++++++++++++++-----
>   target-ppc/kvm_ppc.h |   1 +
>   3 files changed, 278 insertions(+), 28 deletions(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index a973c18..514c595 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -853,6 +853,7 @@ void ppce500_init(MachineState *machine, PPCE500Params 
> *params)
>       if (kvm_enabled()) {
>           kvmppc_init();
>       }
> +    kvmppc_e500_hw_breakpoint_init();
>   }
>   
>   static int e500_ccsr_initfn(SysBusDevice *dev)
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 1d2384d..f5fbec6 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
>   
> @@ -768,6 +769,38 @@ static int kvm_put_vpa(CPUState *cs)
>   
>   static int kvmppc_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;
> +    }
> +

I don't think any of this code should ever run for non-e500, no?

> +    if (sregs.u.e.features & KVM_SREGS_E_ED) {

Hrm - we never seem to set E_ED in kvm?

> +        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);
> +
>       return 0;
>   }
>   
> @@ -1275,6 +1308,239 @@ 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)
> +{
> +    uint32_t sc = tswap32(debug_inst_opcode);

Heh - this will become a lot of fun for real LE host as well as guest 
systems. For now just remove the tswap and add a comment that this needs 
fixing for LE.

> +
> +    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 != tswap32(debug_inst_opcode) ||

Same here.

In fact, neither of the 2 operations are in a fast path. Can't we just 
fetch the debug inst opcode on demand in a function here? That will 
allow for easier byte swapping depending on the guest's MSR.LE setting 
later as well.

> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static struct HWBreakpoint {
> +    target_ulong addr;
> +    int type;
> +} hw_breakpoint[6];
> +
> +static int nb_hw_breakpoint;
> +static int nb_hw_watchpoint;
> +static int max_hw_breakpoint = 4;
> +static int max_hw_watchpoint = 2;
> +
> +void kvmppc_e500_hw_breakpoint_init(void)
> +{
> +    max_hw_breakpoint = 2;
> +    max_hw_watchpoint = 2;

Can we somehow get this information from kvm and set it in 
kvm_arch_init? Worst case we'll have to look at the cpu class and derive 
it from there, but it really should live solely inside the kvm file.

> +}
> +
> +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)
> +{
> +    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 CPUWatchpoint hw_watchpoint;
> +
> +
> +static int kvm_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 (cs->singlestep_enabled) {
> +        handle = 1;
> +    } else if (arch_info->status) {
> +        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;
> +            }
> +        }
> +    } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> +        handle = 1;
> +    }
> +
> +    cpu_synchronize_state(cs);
> +    if (handle) {
> +        env->spr[SPR_BOOKE_DBSR] = 0;

This is pretty e500 specific.


Alex

> +    } else {
> +       printf("unhandled\n");
> +       /* inject guest debug exception */
> +       env->pending_interrupts |=  1 << PPC_INTERRUPT_DEBUG;
> +     }
> +
> +    return handle;
> +}
> +
> +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> +{
> +    if (kvm_sw_breakpoints_active(cs)) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> +    }
> +
> +    if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> +        int n;
> +
> +        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_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;
> +        }
> +    }
> +}
> +
>   int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>   {
>       PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -1315,6 +1581,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;
> @@ -2002,34 +2278,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..89244e5 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_e500_hw_breakpoint_init(void);
>   #ifndef CONFIG_USER_ONLY
>   off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem);
>   bool kvmppc_spapr_use_multitce(void);