> -----Original Message-----
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Wednesday, June 11, 2014 6:35 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
>
> On 06/10/2014 05:06 PM, Bharat Bhushan wrote:
> > This patch adds software breakpoint, hardware breakpoint and hardware
> > watchpoint support for ppc. If the debug interrupt is not handled then
> > this is injected to guest.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > ---
> > hw/ppc/e500.c | 1 +
> > target-ppc/kvm.c | 304
> > ++++++++++++++++++++++++++++++++++++++++++++++---
> --
> > target-ppc/kvm_ppc.h | 1 +
> > 3 files changed, 278 insertions(+), 28 deletions(-)
> >
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..514c595
> > 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -853,6 +853,7 @@ void ppce500_init(MachineState *machine, PPCE500Params
> *params)
> > if (kvm_enabled()) {
> > kvmppc_init();
> > }
> > + kvmppc_e500_hw_breakpoint_init();
> > }
> >
> > static int e500_ccsr_initfn(SysBusDevice *dev) diff --git
> > a/target-ppc/kvm.c b/target-ppc/kvm.c index 1d2384d..f5fbec6 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -38,6 +38,7 @@
> > #include "hw/ppc/ppc.h"
> > #include "sysemu/watchdog.h"
> > #include "trace.h"
> > +#include "exec/gdbstub.h"
> >
> > //#define DEBUG_KVM
> >
> > @@ -768,6 +769,38 @@ static int kvm_put_vpa(CPUState *cs)
> >
> > static int kvmppc_inject_debug_exception(CPUState *cs)
> > {
> > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > + CPUPPCState *env = &cpu->env;
> > + struct kvm_sregs sregs;
> > + int ret;
> > +
> > + if (!cap_booke_sregs) {
> > + return -1;
> > + }
> > +
> > + ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
> > + if (ret < 0) {
> > + return -1;
> > + }
> > +
>
> I don't think any of this code should ever run for non-e500, no?
You mean the code below in this function?
>
> > + if (sregs.u.e.features & KVM_SREGS_E_ED) {
>
> Hrm - we never seem to set E_ED in kvm?
Uhh, you are right. Going through the whole discussion about interrupt
injection to guest I found that one patch missed for upstream.
Will send that patch
>
> > + sregs.u.e.dsrr0 = env->nip;
> > + sregs.u.e.dsrr1 = env->msr;
> > + } else {
> > + sregs.u.e.csrr0 = env->nip;
> > + sregs.u.e.csrr1 = env->msr;
> > + }
> > +
> > + sregs.u.e.update_special = KVM_SREGS_E_UPDATE_DBSR;
> > + sregs.u.e.dbsr = env->spr[SPR_BOOKE_DBSR];
> > +
> > + ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> > + if (ret < 0) {
> > + return -1;
> > + }
> > +
> > + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DEBUG);
> > +
> > return 0;
> > }
> >
> > @@ -1275,6 +1308,239 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> uint32_t dcrn, uint32_t dat
> > return 0;
> > }
> >
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > + uint32_t sc = tswap32(debug_inst_opcode);
>
> Heh - this will become a lot of fun for real LE host as well as guest systems.
I am trying to understand the problem here, We want to byteswap opcode only if
it is mixed endian (host and guest are of different endianess) case?
> For now just remove the tswap and add a comment that this needs fixing for LE.
>
> > +
> > + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0)
> > ||
> > + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 1)) {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > + uint32_t sc;
> > +
> > + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 0) ||
> > + sc != tswap32(debug_inst_opcode) ||
>
> Same here.
>
> In fact, neither of the 2 operations are in a fast path. Can't we just fetch
> the
> debug inst opcode on demand in a function here?
Ok will do that.
> That will allow for easier byte
> swapping depending on the guest's MSR.LE setting later as well.
>
> > + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1))
> > {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct HWBreakpoint {
> > + target_ulong addr;
> > + int type;
> > +} hw_breakpoint[6];
> > +
> > +static int nb_hw_breakpoint;
> > +static int nb_hw_watchpoint;
> > +static int max_hw_breakpoint = 4;
> > +static int max_hw_watchpoint = 2;
> > +
> > +void kvmppc_e500_hw_breakpoint_init(void)
> > +{
> > + max_hw_breakpoint = 2;
> > + max_hw_watchpoint = 2;
>
> Can we somehow get this information from kvm and set it in kvm_arch_init?
Will add one_reg to get this information.
> Worst
> case we'll have to look at the cpu class and derive it from there, but it
> really
> should live solely inside the kvm file.
>
> > +}
> > +
> > +static int find_hw_breakpoint(target_ulong addr, int type) {
> > + int n;
> > +
> > + for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) {
> > + if (hw_breakpoint[n].addr == addr && hw_breakpoint[n].type ==
> > type) {
> > + return n;
> > + }
> > + }
> > +
> > + return -1;
> > +}
> > +
> > +static int find_hw_watchpoint(target_ulong addr, int *flag) {
> > + int n;
> > +
> > + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_ACCESS);
> > + if (n >= 0) {
> > + *flag = BP_MEM_ACCESS;
> > + return n;
> > + }
> > +
> > + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_WRITE);
> > + if (n >= 0) {
> > + *flag = BP_MEM_WRITE;
> > + return n;
> > + }
> > +
> > + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_READ);
> > + if (n >= 0) {
> > + *flag = BP_MEM_READ;
> > + return n;
> > + }
> > +
> > + return -1;
> > +}
> > +
> > +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> > + target_ulong len, int type) {
> > + hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr;
> > + hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].type = type;
> > +
> > + switch (type) {
> > + case GDB_BREAKPOINT_HW:
> > + if (nb_hw_breakpoint >= max_hw_breakpoint) {
> > + return -ENOBUFS;
> > + }
> > +
> > + if (find_hw_breakpoint(addr, type) >= 0) {
> > + return -EEXIST;
> > + }
> > +
> > + nb_hw_breakpoint++;
> > + break;
> > +
> > + case GDB_WATCHPOINT_WRITE:
> > + case GDB_WATCHPOINT_READ:
> > + case GDB_WATCHPOINT_ACCESS:
> > + if (nb_hw_watchpoint >= max_hw_watchpoint) {
> > + return -ENOBUFS;
> > + }
> > +
> > + if (find_hw_breakpoint(addr, type) >= 0) {
> > + return -EEXIST;
> > + }
> > +
> > + nb_hw_watchpoint++;
> > + break;
> > +
> > + default:
> > + return -ENOSYS;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int kvm_arch_remove_hw_breakpoint(target_ulong addr,
> > + target_ulong len, int type) {
> > + int n;
> > +
> > + n = find_hw_breakpoint(addr, type);
> > + if (n < 0) {
> > + return -ENOENT;
> > + }
> > +
> > + switch (type) {
> > + case GDB_BREAKPOINT_HW:
> > + nb_hw_breakpoint--;
> > + break;
> > +
> > + case GDB_WATCHPOINT_WRITE:
> > + case GDB_WATCHPOINT_READ:
> > + case GDB_WATCHPOINT_ACCESS:
> > + nb_hw_watchpoint--;
> > + break;
> > +
> > + default:
> > + return -ENOSYS;
> > + }
> > + hw_breakpoint[n] = hw_breakpoint[nb_hw_breakpoint +
> > + nb_hw_watchpoint];
> > +
> > + return 0;
> > +}
> > +
> > +void kvm_arch_remove_all_hw_breakpoints(void)
> > +{
> > + nb_hw_breakpoint = nb_hw_watchpoint = 0; }
> > +
> > +static CPUWatchpoint hw_watchpoint;
> > +
> > +
> > +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) {
> > + CPUState *cs = CPU(cpu);
> > + CPUPPCState *env = &cpu->env;
> > + int handle = 0;
> > + int n;
> > + int flag = 0;
> > + struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> > +
> > + if (cs->singlestep_enabled) {
> > + handle = 1;
> > + } else if (arch_info->status) {
> > + if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {
> > + n = find_hw_breakpoint(arch_info->address, GDB_BREAKPOINT_HW);
> > + if (n >= 0) {
> > + handle = 1;
> > + }
> > + } else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ |
> > + KVMPPC_DEBUG_WATCH_WRITE)) {
> > + n = find_hw_watchpoint(arch_info->address, &flag);
> > + if (n >= 0) {
> > + handle = 1;
> > + cs->watchpoint_hit = &hw_watchpoint;
> > + hw_watchpoint.vaddr = hw_breakpoint[n].addr;
> > + hw_watchpoint.flags = flag;
> > + }
> > + }
> > + } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> > + handle = 1;
> > + }
> > +
> > + cpu_synchronize_state(cs);
> > + if (handle) {
> > + env->spr[SPR_BOOKE_DBSR] = 0;
>
> This is pretty e500 specific.
You mean accessing DBSR, right ?
To handle core specific stuff, should we add some generic function like
kvm_ppc_set/clear_debug_event() in hw/ppc/e500_debug.c, hw/ppc/ppc4xx_debug.c
etc ?
Thanks
-Bharat
>
>
> Alex
>
> > + } else {
> > + printf("unhandled\n");
> > + /* inject guest debug exception */
> > + env->pending_interrupts |= 1 << PPC_INTERRUPT_DEBUG;
> > + }
> > +
> > + return handle;
> > +}
> > +
> > +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug
> > +*dbg) {
> > + if (kvm_sw_breakpoints_active(cs)) {
> > + dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> > + }
> > +
> > + if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> > + int n;
> > +
> > + dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
> > + memset(dbg->arch.bp, 0, sizeof(dbg->arch.bp));
> > + for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) {
> > + switch (hw_breakpoint[n].type) {
> > + case GDB_BREAKPOINT_HW:
> > + dbg->arch.bp[n].type = KVMPPC_DEBUG_BREAKPOINT;
> > + break;
> > + case GDB_WATCHPOINT_WRITE:
> > + dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_WRITE;
> > + break;
> > + case GDB_WATCHPOINT_READ:
> > + dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_READ;
> > + break;
> > + case GDB_WATCHPOINT_ACCESS:
> > + dbg->arch.bp[n].type = KVMPPC_DEBUG_WATCH_WRITE |
> > + KVMPPC_DEBUG_WATCH_READ;
> > + break;
> > + default:
> > + cpu_abort(cs, "Unsupported breakpoint type\n");
> > + }
> > + dbg->arch.bp[n].addr = hw_breakpoint[n].addr;
> > + }
> > + }
> > +}
> > +
> > int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> > {
> > PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1315,6 +1581,16 @@ int
> > kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> > ret = 0;
> > break;
> >
> > + case KVM_EXIT_DEBUG:
> > + DPRINTF("handle debug exception\n");
> > + if (kvm_handle_debug(cpu, run)) {
> > + ret = EXCP_DEBUG;
> > + break;
> > + }
> > + /* re-enter, this exception was guest-internal */
> > + ret = 0;
> > + break;
> > +
> > default:
> > fprintf(stderr, "KVM: unknown exit reason %d\n",
> > run->exit_reason);
> > ret = -1;
> > @@ -2002,34 +2278,6 @@ void kvm_arch_init_irq_routing(KVMState *s)
> > {
> > }
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cpu, struct
> > kvm_sw_breakpoint *bp) -{
> > - return -EINVAL;
> > -}
> > -
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cpu, struct
> > kvm_sw_breakpoint *bp) -{
> > - return -EINVAL;
> > -}
> > -
> > -int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong
> > len, int type) -{
> > - return -EINVAL;
> > -}
> > -
> > -int kvm_arch_remove_hw_breakpoint(target_ulong addr, target_ulong
> > len, int type) -{
> > - return -EINVAL;
> > -}
> > -
> > -void kvm_arch_remove_all_hw_breakpoints(void)
> > -{
> > -}
> > -
> > -void kvm_arch_update_guest_debug(CPUState *cpu, struct
> > kvm_guest_debug *dbg) -{ -}
> > -
> > struct kvm_get_htab_buf {
> > struct kvm_get_htab_header header;
> > /*
> > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h index
> > 412cc7f..89244e5 100644
> > --- a/target-ppc/kvm_ppc.h
> > +++ b/target-ppc/kvm_ppc.h
> > @@ -30,6 +30,7 @@ int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t
> tsr_bits);
> > int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> > int kvmppc_set_tcr(PowerPCCPU *cpu);
> > int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
> > +void kvmppc_e500_hw_breakpoint_init(void);
> > #ifndef CONFIG_USER_ONLY
> > off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem);
> > bool kvmppc_spapr_use_multitce(void);
