From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48654) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wwq1V-0006NM-22 for qemu-devel@nongnu.org; Tue, 17 Jun 2014 05:49:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wwq1P-0007nO-Ov for qemu-devel@nongnu.org; Tue, 17 Jun 2014 05:49:49 -0400 Message-ID: <53A00F35.6000909@suse.de> Date: Tue, 17 Jun 2014 11:49:41 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1402988887-30418-1-git-send-email-Bharat.Bhushan@freescale.com> <1402988887-30418-4-git-send-email-Bharat.Bhushan@freescale.com> <539FF926.4020300@suse.de> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Bharat.Bhushan@freescale.com" , "qemu-ppc@nongnu.org" , "qemu-devel@nongnu.org" 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 >>> --- >>> 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