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

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 <Bharat.Bhushan@freescale.com>
> ---
> 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);

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Tuesday, June 17, 2014 1:46 PM
> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> 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 <Bharat.Bhushan@freescale.com>
> > ---
> > 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);

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

[Alexander Graf]

On 17.06.14 11:14, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 17, 2014 1:46 PM
>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> 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 <Bharat.Bhushan@freescale.com>
>>> ---
>>> 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 ?

I think it makes sense to leave it in KVM land. Why not do it lazily on 
insert_hw_breakpoint?

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

Man, this is ugly :).

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

You have a typo.

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

Let's assume it's generic for now. That way we maybe have a slight 
change to push the IBM guys into the right direction ;).

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

dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint + 
nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite memory we 
don't want to overwrite.

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

They can't have their own struct kvm_guest_debug, so I really think this 
should be shared.


Alex

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Tuesday, June 17, 2014 3:20 PM
> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> 
> 
> On 17.06.14 11:14, Bharat.Bhushan@freescale.com wrote:
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Tuesday, June 17, 2014 1:46 PM
> >> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> >> 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 <Bharat.Bhushan@freescale.com>
> >>> ---
> >>> 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 ?
> 
> I think it makes sense to leave it in KVM land. Why not do it lazily on
> insert_hw_breakpoint?

You mean setting in kvm_arch_insert_hw_breakpoint() when called first time; something like:

    static bool init = 0;

    if (!init) {
        if (env->excp_model == POWERPC_EXCP_BOOKE) {
            max_hw_breakpoint = 2;
            max_hw_watchpoint = 2;
        } else
	   // Add for book3s max_hw_watchpoint = 1;
	 }
	 init = 1;
    }

> 
> >
> >>>    }
> >>>
> >>>    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.
> 
> Man, this is ugly :).

Yes, this is how x86 also works.
May be we move this in debug_handler function but ensure to keep it static.

> 
> >
> >>> +
> >>> +/* 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"
> 
> You have a typo.
> 
> >
> >>> +        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.
> 
> Let's assume it's generic for now. That way we maybe have a slight change to
> push the IBM guys into the right direction ;).

Ok :)
I will mention that this is untested in book3s

> 
> >
> >>> +
> >>> +    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" ?
> 
> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite memory we don't
> want to overwrite.

Actually this will never overflow here because nb_hw_breakpoint and nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
Do you thing that to be double safe we can add a check?

> 
> >
> >>> +            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.
> 
> They can't have their own struct kvm_guest_debug, so I really think this should
> be shared.

Maybe they use different encoding in type and accordingly other elements of struct. But I am fine to assume they will use as is and then change if needed.

Thanks
-Bharat

> 
> 
> Alex

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

[Alexander Graf]

On 17.06.14 12:40, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 17, 2014 3:20 PM
>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>
>>
>> On 17.06.14 11:14, Bharat.Bhushan@freescale.com wrote:
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Tuesday, June 17, 2014 1:46 PM
>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>>>> 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 <Bharat.Bhushan@freescale.com>
>>>>> ---
>>>>> 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 ?
>> I think it makes sense to leave it in KVM land. Why not do it lazily on
>> insert_hw_breakpoint?
> You mean setting in kvm_arch_insert_hw_breakpoint() when called first time; something like:
>
>      static bool init = 0;
>
>      if (!init) {
>          if (env->excp_model == POWERPC_EXCP_BOOKE) {
>              max_hw_breakpoint = 2;
>              max_hw_watchpoint = 2;
>          } else
> 	   // Add for book3s max_hw_watchpoint = 1;
> 	 }
> 	 init = 1;
>      }

I would probably reuse max_hw_breakpoint as a hint whether it's 
initialized and put all of this into a small function, but yes :).

>
>>>>>     }
>>>>>
>>>>>     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.
>>
>> Man, this is ugly :).
> Yes, this is how x86 also works.
> May be we move this in debug_handler function but ensure to keep it static.
>
>>>>> +
>>>>> +/* 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"
>> You have a typo.
>>
>>>>> +        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.
>>
>> Let's assume it's generic for now. That way we maybe have a slight change to
>> push the IBM guys into the right direction ;).
> Ok :)
> I will mention that this is untested in book3s

That's ok - just make sure that the code does "the right thing" when all 
numbers are 0 ;).

>
>>>>> +
>>>>> +    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" ?
>> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
>> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite memory we don't
>> want to overwrite.
> Actually this will never overflow here because nb_hw_breakpoint and nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
> Do you thing that to be double safe we can add a check?

We only check against an overflow of hw_breakpoint[], not dbg->arch.bp. 
What if nb_hw_breakpoint becomes 17?

>
>>>>> +            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.
>>
>> They can't have their own struct kvm_guest_debug, so I really think this should
>> be shared.
> Maybe they use different encoding in type and accordingly other elements of struct. But I am fine to assume they will use as is and then change if needed.

Perfect :).


Alex

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

[Bharat.Bhushan]

> >>>>> +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" ?
> >> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
> >> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite memory
> >> we don't want to overwrite.
> > Actually this will never overflow here because nb_hw_breakpoint and
> nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
> > Do you thing that to be double safe we can add a check?
> 
> We only check against an overflow of hw_breakpoint[], not dbg->arch.bp.
> What if nb_hw_breakpoint becomes 17?

nb_hw_breakpoint can never be more than max_hw_breakpoint, how nb_hw_breakpoint can be 17 ?


Thanks
-Bharat

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

[Alexander Graf]

On 17.06.14 13:01, Bharat.Bhushan@freescale.com wrote:
>>>>>>> +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" ?
>>>> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
>>>> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite memory
>>>> we don't want to overwrite.
>>> Actually this will never overflow here because nb_hw_breakpoint and
>> nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
>>> Do you thing that to be double safe we can add a check?
>> We only check against an overflow of hw_breakpoint[], not dbg->arch.bp.
>> What if nb_hw_breakpoint becomes 17?
> nb_hw_breakpoint can never be more than max_hw_breakpoint, how nb_hw_breakpoint can be 17 ?

Someone comes along and bumps up max_hw_breakpoint to 17? Just add an 
assert() somewhere that makes sure we can't run over bp :).


Alex

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Tuesday, June 17, 2014 4:33 PM
> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> 
> 
> On 17.06.14 13:01, Bharat.Bhushan@freescale.com wrote:
> >>>>>>> +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" ?
> >>>> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
> >>>> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite
> >>>> memory we don't want to overwrite.
> >>> Actually this will never overflow here because nb_hw_breakpoint and
> >> nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
> >>> Do you thing that to be double safe we can add a check?
> >> We only check against an overflow of hw_breakpoint[], not dbg->arch.bp.
> >> What if nb_hw_breakpoint becomes 17?
> > nb_hw_breakpoint can never be more than max_hw_breakpoint, how
> nb_hw_breakpoint can be 17 ?
> 
> Someone comes along and bumps up max_hw_breakpoint to 17?

You mean some buggy code in qemu does this?

Thanks
-Bharat

> Just add an
> assert() somewhere that makes sure we can't run over bp :).
> 
> 
> Alex

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

[Alexander Graf]

On 17.06.14 13:05, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 17, 2014 4:33 PM
>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>
>>
>> On 17.06.14 13:01, Bharat.Bhushan@freescale.com wrote:
>>>>>>>>> +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" ?
>>>>>> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
>>>>>> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite
>>>>>> memory we don't want to overwrite.
>>>>> Actually this will never overflow here because nb_hw_breakpoint and
>>>> nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
>>>>> Do you thing that to be double safe we can add a check?
>>>> We only check against an overflow of hw_breakpoint[], not dbg->arch.bp.
>>>> What if nb_hw_breakpoint becomes 17?
>>> nb_hw_breakpoint can never be more than max_hw_breakpoint, how
>> nb_hw_breakpoint can be 17 ?
>>
>> Someone comes along and bumps up max_hw_breakpoint to 17?
> You mean some buggy code in qemu does this?

I mean the next person that comes along and touches this code might not 
realize that dbg->arch.bp[] is an array of 16 and by the time I review 
that code I might have forgotten as well :)


Alex

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Tuesday, June 17, 2014 4:14 PM
> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> 
> 
> On 17.06.14 12:40, Bharat.Bhushan@freescale.com wrote:
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Tuesday, June 17, 2014 3:20 PM
> >> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>
> >>
> >> On 17.06.14 11:14, Bharat.Bhushan@freescale.com wrote:
> >>>> -----Original Message-----
> >>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>> Sent: Tuesday, June 17, 2014 1:46 PM
> >>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org;
> >>>> qemu-devel@nongnu.org
> >>>> 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 <Bharat.Bhushan@freescale.com>
> >>>>> ---
> >>>>> 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
> ?
> >> I think it makes sense to leave it in KVM land. Why not do it lazily
> >> on insert_hw_breakpoint?
> > You mean setting in kvm_arch_insert_hw_breakpoint() when called first time;
> something like:
> >
> >      static bool init = 0;
> >
> >      if (!init) {
> >          if (env->excp_model == POWERPC_EXCP_BOOKE) {
> >              max_hw_breakpoint = 2;
> >              max_hw_watchpoint = 2;
> >          } else
> > 	   // Add for book3s max_hw_watchpoint = 1;
> > 	 }
> > 	 init = 1;
> >      }
> 
> I would probably reuse max_hw_breakpoint as a hint whether it's initialized and
> put all of this into a small function, but yes :).

Ahh, we cannot do this in kvm_arch_insert_hw_breakpoint() as we can not get "env" reference in this function. Prototype of this is:
int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int type);

I will suggest that we initialize this from kvm_arch_init_vcpu(). This way we are still in KVM zone.

Thanks
-Bharat

> 
> >
> >>>>>     }
> >>>>>
> >>>>>     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.
> >>
> >> Man, this is ugly :).
> > Yes, this is how x86 also works.
> > May be we move this in debug_handler function but ensure to keep it static.
> >
> >>>>> +
> >>>>> +/* 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"
> >> You have a typo.
> >>
> >>>>> +        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.
> >>
> >> Let's assume it's generic for now. That way we maybe have a slight
> >> change to push the IBM guys into the right direction ;).
> > Ok :)
> > I will mention that this is untested in book3s
> 
> That's ok - just make sure that the code does "the right thing" when all numbers
> are 0 ;).
> 
> >
> >>>>> +
> >>>>> +    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" ?
> >> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
> >> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite memory
> >> we don't want to overwrite.
> > Actually this will never overflow here because nb_hw_breakpoint and
> nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
> > Do you thing that to be double safe we can add a check?
> 
> We only check against an overflow of hw_breakpoint[], not dbg->arch.bp.
> What if nb_hw_breakpoint becomes 17?
> 
> >
> >>>>> +            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.
> >>
> >> They can't have their own struct kvm_guest_debug, so I really think
> >> this should be shared.
> > Maybe they use different encoding in type and accordingly other elements of
> struct. But I am fine to assume they will use as is and then change if needed.
> 
> Perfect :).
> 
> 
> Alex

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

[Alexander Graf]

On 18.06.14 06:39, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 17, 2014 4:14 PM
>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>
>>
>> On 17.06.14 12:40, Bharat.Bhushan@freescale.com wrote:
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Tuesday, June 17, 2014 3:20 PM
>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>>>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>>>
>>>>
>>>> On 17.06.14 11:14, Bharat.Bhushan@freescale.com wrote:
>>>>>> -----Original Message-----
>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>> Sent: Tuesday, June 17, 2014 1:46 PM
>>>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org;
>>>>>> qemu-devel@nongnu.org
>>>>>> 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 <Bharat.Bhushan@freescale.com>
>>>>>>> ---
>>>>>>> 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
>> ?
>>>> I think it makes sense to leave it in KVM land. Why not do it lazily
>>>> on insert_hw_breakpoint?
>>> You mean setting in kvm_arch_insert_hw_breakpoint() when called first time;
>> something like:
>>>       static bool init = 0;
>>>
>>>       if (!init) {
>>>           if (env->excp_model == POWERPC_EXCP_BOOKE) {
>>>               max_hw_breakpoint = 2;
>>>               max_hw_watchpoint = 2;
>>>           } else
>>> 	   // Add for book3s max_hw_watchpoint = 1;
>>> 	 }
>>> 	 init = 1;
>>>       }
>> I would probably reuse max_hw_breakpoint as a hint whether it's initialized and
>> put all of this into a small function, but yes :).
> Ahh, we cannot do this in kvm_arch_insert_hw_breakpoint() as we can not get "env" reference in this function. Prototype of this is:
> int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int type);

Just use first_cpu? :)

>
> I will suggest that we initialize this from kvm_arch_init_vcpu(). This way we are still in KVM zone.

That works too, yes.



Alex

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Tuesday, June 24, 2014 5:02 PM
> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> 
> 
> On 18.06.14 06:39, Bharat.Bhushan@freescale.com wrote:
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Tuesday, June 17, 2014 4:14 PM
> >> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>
> >>
> >> On 17.06.14 12:40, Bharat.Bhushan@freescale.com wrote:
> >>>> -----Original Message-----
> >>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>> Sent: Tuesday, June 17, 2014 3:20 PM
> >>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org;
> >>>> qemu-devel@nongnu.org
> >>>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>>>
> >>>>
> >>>> On 17.06.14 11:14, Bharat.Bhushan@freescale.com wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>>>> Sent: Tuesday, June 17, 2014 1:46 PM
> >>>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org;
> >>>>>> qemu-devel@nongnu.org
> >>>>>> 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 <Bharat.Bhushan@freescale.com>
> >>>>>>> ---
> >>>>>>> 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
> >> ?
> >>>> I think it makes sense to leave it in KVM land. Why not do it
> >>>> lazily on insert_hw_breakpoint?
> >>> You mean setting in kvm_arch_insert_hw_breakpoint() when called
> >>> first time;
> >> something like:
> >>>       static bool init = 0;
> >>>
> >>>       if (!init) {
> >>>           if (env->excp_model == POWERPC_EXCP_BOOKE) {
> >>>               max_hw_breakpoint = 2;
> >>>               max_hw_watchpoint = 2;
> >>>           } else
> >>> 	   // Add for book3s max_hw_watchpoint = 1;
> >>> 	 }
> >>> 	 init = 1;
> >>>       }
> >> I would probably reuse max_hw_breakpoint as a hint whether it's
> >> initialized and put all of this into a small function, but yes :).
> > Ahh, we cannot do this in kvm_arch_insert_hw_breakpoint() as we can not get
> "env" reference in this function. Prototype of this is:
> > int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len,
> > int type);
> 
> Just use first_cpu? :)
> 
> >
> > I will suggest that we initialize this from kvm_arch_init_vcpu(). This way we
> are still in KVM zone.
> 
> That works too, yes.

V3 version of path is based on this :)

Thanks
-Bharat

> 
> 
> 
> Alex

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

[Alexander Graf]

On 24.06.14 13:32, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 24, 2014 5:02 PM
>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>
>>
>> On 18.06.14 06:39, Bharat.Bhushan@freescale.com wrote:
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Tuesday, June 17, 2014 4:14 PM
>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>>>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>>>
>>>>
>>>> On 17.06.14 12:40, Bharat.Bhushan@freescale.com wrote:
>>>>>> -----Original Message-----
>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>> Sent: Tuesday, June 17, 2014 3:20 PM
>>>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org;
>>>>>> qemu-devel@nongnu.org
>>>>>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>>>>>
>>>>>>
>>>>>> On 17.06.14 11:14, Bharat.Bhushan@freescale.com wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>> Sent: Tuesday, June 17, 2014 1:46 PM
>>>>>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org;
>>>>>>>> qemu-devel@nongnu.org
>>>>>>>> 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 <Bharat.Bhushan@freescale.com>
>>>>>>>>> ---
>>>>>>>>> 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
>>>> ?
>>>>>> I think it makes sense to leave it in KVM land. Why not do it
>>>>>> lazily on insert_hw_breakpoint?
>>>>> You mean setting in kvm_arch_insert_hw_breakpoint() when called
>>>>> first time;
>>>> something like:
>>>>>        static bool init = 0;
>>>>>
>>>>>        if (!init) {
>>>>>            if (env->excp_model == POWERPC_EXCP_BOOKE) {
>>>>>                max_hw_breakpoint = 2;
>>>>>                max_hw_watchpoint = 2;
>>>>>            } else
>>>>> 	   // Add for book3s max_hw_watchpoint = 1;
>>>>> 	 }
>>>>> 	 init = 1;
>>>>>        }
>>>> I would probably reuse max_hw_breakpoint as a hint whether it's
>>>> initialized and put all of this into a small function, but yes :).
>>> Ahh, we cannot do this in kvm_arch_insert_hw_breakpoint() as we can not get
>> "env" reference in this function. Prototype of this is:
>>> int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len,
>>> int type);
>> Just use first_cpu? :)
>>
>>> I will suggest that we initialize this from kvm_arch_init_vcpu(). This way we
>> are still in KVM zone.
>>
>> That works too, yes.
> V3 version of path is based on this :)

Uh, I don't see a v3 yet? :)


Alex