|
From: | Alexander Graf |
Subject: | Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support |
Date: | Tue, 17 Jun 2014 12:43:37 +0200 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 |
On 17.06.14 12:40, address@hidden wrote:
-----Original Message----- From: Alexander Graf [mailto:address@hidden Sent: Tuesday, June 17, 2014 3:20 PM To: Bhushan Bharat-R65777; address@hidden; address@hidden Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support On 17.06.14 11:14, address@hidden wrote:-----Original Message----- From: Alexander Graf [mailto:address@hidden Sent: Tuesday, June 17, 2014 1:46 PM To: Bhushan Bharat-R65777; address@hidden; address@hidden 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 <address@hidden> --- v1->v2: - factored out e500 specific code based on exception modelPOWERPC_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_tdatreturn 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. Pleasesee 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 " structkvm_debug_exit_arch *arch_info" in different way ?So I left this booke specific. When someone implements h/w break/watch_pointon 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 "structkvm_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
[Prev in Thread] | Current Thread | [Next in Thread] |