From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58483) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wwqrh-0000U6-VK for qemu-devel@nongnu.org; Tue, 17 Jun 2014 06:43:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wwqrb-0001cq-Ai for qemu-devel@nongnu.org; Tue, 17 Jun 2014 06:43:45 -0400 Message-ID: <53A01BD9.50207@suse.de> Date: Tue, 17 Jun 2014 12:43:37 +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> <53A00F35.6000909@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 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 >>>>> --- >>>>> 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