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

[address@hidden]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Friday, June 13, 2014 4:55 PM
> To: Bhushan Bharat-R65777
> Cc: address@hidden; address@hidden
> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
> 
> 
> On 12.06.14 09:05, address@hidden wrote:
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:address@hidden
> >> Sent: Wednesday, June 11, 2014 6:35 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: address@hidden; address@hidden
> >> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
> >>
> >> 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?
> > You mean the code below in this function?
> 
> Yeah :).

Why you think accessing sregs (cssr0/1, dsrr0/1 and ioctl) is e500 specific. 
Are not these valid for 4xx as well?

> 
> >
> >>> +    if (sregs.u.e.features & KVM_SREGS_E_ED) {
> >> Hrm - we never seem to set E_ED in kvm?
> > Uhh, you are right. Going through the whole discussion about interrupt
> injection to guest I found that one patch missed for upstream.
> > Will send that patch
> >
> >>> +        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.
> > I am trying to understand the problem here, We want to byteswap opcode only 
> > if
> it is mixed endian (host and guest are of different endianess) case?
> 
> Yes :).
> 
> >
> >> 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?
> > Ok will do that.
> >
> >> 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?
> > Will add one_reg to get this information.
> 
> We can also fall back to always make this 2 when there's no other way to
> enumerate. But setting the default to 4/2 and then revert to 2/2 hard codedly
> seems odd.
> 
> Btw, where does the 4/2 come from?

Because 44x have 4 IACs.

> 
> >
> >> 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.
> > You mean accessing DBSR, right ?
> > To handle core specific stuff, should we add some generic function like
> kvm_ppc_set/clear_debug_event() in hw/ppc/e500_debug.c, hw/ppc/ppc4xx_debug.c
> etc ?
> 
> I don't think this is hardware, it's part of the core. So it'd belong to 
> target-
> ppc/. Probably part of the excp helper file.

Is not this (dbsr) is booke specific and not just e500 specific? Should this be 
accessed under POWERPC_MMU_BOOKE and POWERPC_MMU_BOOKE206 cpu models?

Thanks
-Bharat

> 
> 
> Alex