|
| From: | Alexander Graf |
| Subject: | Re: [Qemu-devel] [PATCH 3/3] ppc debug: Add debug stub support |
| Date: | Fri, 13 Jun 2014 13:24:52 +0200 |
| User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
On 12.06.14 09:05, address@hidden wrote:
-----Original Message----- From: Alexander Graf [mailto:address@hidden Sent: Wednesday, June 11, 2014 6:35 PM To: Bhushan Bharat-R65777 Cc: address@hidden; address@hidden Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support On 06/10/2014 05:06 PM, Bharat Bhushan wrote:This patch adds software breakpoint, hardware breakpoint and hardware watchpoint support for ppc. If the debug interrupt is not handled then this is injected to guest. Signed-off-by: Bharat Bhushan <address@hidden> --- hw/ppc/e500.c | 1 + target-ppc/kvm.c | 304 ++++++++++++++++++++++++++++++++++++++++++++++-----target-ppc/kvm_ppc.h | 1 + 3 files changed, 278 insertions(+), 28 deletions(-) diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..514c595 100644 --- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -853,6 +853,7 @@ void ppce500_init(MachineState *machine, PPCE500Params*params)if (kvm_enabled()) { kvmppc_init(); } + kvmppc_e500_hw_breakpoint_init(); } static int e500_ccsr_initfn(SysBusDevice *dev) diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 1d2384d..f5fbec6 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -38,6 +38,7 @@ #include "hw/ppc/ppc.h" #include "sysemu/watchdog.h" #include "trace.h" +#include "exec/gdbstub.h" //#define DEBUG_KVM @@ -768,6 +769,38 @@ static int kvm_put_vpa(CPUState *cs) static int kvmppc_inject_debug_exception(CPUState *cs) { + PowerPCCPU *cpu = POWERPC_CPU(cs); + CPUPPCState *env = &cpu->env; + struct kvm_sregs sregs; + int ret; + + if (!cap_booke_sregs) { + return -1; + } + + ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs); + if (ret < 0) { + return -1; + } +I don't think any of this code should ever run for non-e500, no?You mean the code below in this function?
Yeah :).
+ 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 datreturn 0; } +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct +kvm_sw_breakpoint *bp) { + uint32_t sc = tswap32(debug_inst_opcode);Heh - this will become a lot of fun for real LE host as well as guest systems.I am trying to understand the problem here, We want to byteswap opcode only if it is mixed endian (host and guest are of different endianess) case?
Yes :).
For now just remove the tswap and add a comment that this needs fixing for LE.+ + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) || + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 1)) { + return -EINVAL; + } + + return 0; +} + +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct +kvm_sw_breakpoint *bp) { + uint32_t sc; + + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 0) || + sc != tswap32(debug_inst_opcode) ||Same here. In fact, neither of the 2 operations are in a fast path. Can't we just fetch the debug inst opcode on demand in a function here?Ok will do that.That will allow for easier byte swapping depending on the guest's MSR.LE setting later as well.+ cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) { + return -EINVAL; + } + + return 0; +} + +static struct HWBreakpoint { + target_ulong addr; + int type; +} hw_breakpoint[6]; + +static int nb_hw_breakpoint; +static int nb_hw_watchpoint; +static int max_hw_breakpoint = 4; +static int max_hw_watchpoint = 2; + +void kvmppc_e500_hw_breakpoint_init(void) +{ + max_hw_breakpoint = 2; + max_hw_watchpoint = 2;Can we somehow get this information from kvm and set it in kvm_arch_init?Will add one_reg to get this information.
We can also fall back to always make this 2 when there's no other way to enumerate. But setting the default to 4/2 and then revert to 2/2 hard codedly seems odd.
Btw, where does the 4/2 come from?
Worst case we'll have to look at the cpu class and derive it from there, but it really should live solely inside the kvm file.+} + +static int find_hw_breakpoint(target_ulong addr, int type) { + int n; + + for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) { + if (hw_breakpoint[n].addr == addr && hw_breakpoint[n].type == type) { + return n; + } + } + + return -1; +} + +static int find_hw_watchpoint(target_ulong addr, int *flag) { + int n; + + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_ACCESS); + if (n >= 0) { + *flag = BP_MEM_ACCESS; + return n; + } + + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_WRITE); + if (n >= 0) { + *flag = BP_MEM_WRITE; + return n; + } + + n = find_hw_breakpoint(addr, GDB_WATCHPOINT_READ); + if (n >= 0) { + *flag = BP_MEM_READ; + return n; + } + + return -1; +} + +int kvm_arch_insert_hw_breakpoint(target_ulong addr, + target_ulong len, int type) { + hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr; + hw_breakpoint[nb_hw_breakpoint + nb_hw_watchpoint].type = type; + + switch (type) { + case GDB_BREAKPOINT_HW: + if (nb_hw_breakpoint >= max_hw_breakpoint) { + return -ENOBUFS; + } + + if (find_hw_breakpoint(addr, type) >= 0) { + return -EEXIST; + } + + nb_hw_breakpoint++; + break; + + case GDB_WATCHPOINT_WRITE: + case GDB_WATCHPOINT_READ: + case GDB_WATCHPOINT_ACCESS: + if (nb_hw_watchpoint >= max_hw_watchpoint) { + return -ENOBUFS; + } + + if (find_hw_breakpoint(addr, type) >= 0) { + return -EEXIST; + } + + nb_hw_watchpoint++; + break; + + default: + return -ENOSYS; + } + + return 0; +} + +int kvm_arch_remove_hw_breakpoint(target_ulong addr, + target_ulong len, int type) { + int n; + + n = find_hw_breakpoint(addr, type); + if (n < 0) { + return -ENOENT; + } + + switch (type) { + case GDB_BREAKPOINT_HW: + nb_hw_breakpoint--; + break; + + case GDB_WATCHPOINT_WRITE: + case GDB_WATCHPOINT_READ: + case GDB_WATCHPOINT_ACCESS: + nb_hw_watchpoint--; + break; + + default: + return -ENOSYS; + } + hw_breakpoint[n] = hw_breakpoint[nb_hw_breakpoint + + nb_hw_watchpoint]; + + return 0; +} + +void kvm_arch_remove_all_hw_breakpoints(void) +{ + nb_hw_breakpoint = nb_hw_watchpoint = 0; } + +static CPUWatchpoint hw_watchpoint; + + +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) { + CPUState *cs = CPU(cpu); + CPUPPCState *env = &cpu->env; + int handle = 0; + int n; + int flag = 0; + struct kvm_debug_exit_arch *arch_info = &run->debug.arch; + + if (cs->singlestep_enabled) { + handle = 1; + } else if (arch_info->status) { + if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) { + n = find_hw_breakpoint(arch_info->address, GDB_BREAKPOINT_HW); + if (n >= 0) { + handle = 1; + } + } else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ | + KVMPPC_DEBUG_WATCH_WRITE)) { + n = find_hw_watchpoint(arch_info->address, &flag); + if (n >= 0) { + handle = 1; + cs->watchpoint_hit = &hw_watchpoint; + hw_watchpoint.vaddr = hw_breakpoint[n].addr; + hw_watchpoint.flags = flag; + } + } + } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) { + handle = 1; + } + + cpu_synchronize_state(cs); + if (handle) { + env->spr[SPR_BOOKE_DBSR] = 0;This is pretty e500 specific.You mean accessing DBSR, right ? To handle core specific stuff, should we add some generic function like kvm_ppc_set/clear_debug_event() in hw/ppc/e500_debug.c, hw/ppc/ppc4xx_debug.c etc ?
I don't think this is hardware, it's part of the core. So it'd belong to target-ppc/. Probably part of the excp helper file.
Alex
| [Prev in Thread] | Current Thread | [Next in Thread] |