From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59391) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzS3p-0003Nm-5S for qemu-devel@nongnu.org; Tue, 24 Jun 2014 10:51:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WzS3j-0000Y1-3z for qemu-devel@nongnu.org; Tue, 24 Jun 2014 10:51:01 -0400 Message-ID: <53A9904C.6030905@suse.de> Date: Tue, 24 Jun 2014 16:50:52 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1403611807-7082-1-git-send-email-Bharat.Bhushan@freescale.com> <1403611807-7082-6-git-send-email-Bharat.Bhushan@freescale.com> <53A97AF8.3010408@suse.de> <292533a7000b4089933c9e44bbc38212@BLUPR03MB566.namprd03.prod.outlook.com> In-Reply-To: <292533a7000b4089933c9e44bbc38212@BLUPR03MB566.namprd03.prod.outlook.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Bharat.Bhushan@freescale.com" Cc: "maddy@linux.vnet.ibm.com" , "qemu-ppc@nongnu.org" , "qemu-devel@nongnu.org" On 24.06.14 16:37, Bharat.Bhushan@freescale.com wrote: > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Tuesday, June 24, 2014 6:50 PM >> To: Bhushan Bharat-R65777 >> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; maddy@linux.vnet.ibm.com >> Subject: Re: [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support >> >> >> On 24.06.14 14:10, Bharat Bhushan wrote: >>> This patch adds 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 >>> --- >>> v2->v3 >>> - Shared as much code as much possible for futuristic book3s support >>> - Initializing number of hw breakpoint/watchpoints from KVM world >>> - Other minor cleanup/fixes >>> >>> target-ppc/kvm.c | 248 +++++++++++++++++++++++++++++++++++++++++++++++++++-- >> -- >>> 1 file changed, 233 insertions(+), 15 deletions(-) >>> >>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index >>> 8e2dbb3..4fb0efd 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 >>> >>> @@ -410,6 +411,44 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu) >>> return ppc_get_vcpu_dt_id(POWERPC_CPU(cpu)); >>> } >>> >>> +/* e500 supports 2 h/w breakpoint and 2 watchpoint. >>> + * book3s supports only 1 watchpoint, so array size >>> + * of 4 is sufficient for now. >>> + */ >>> +#define MAX_HW_BKPTS 4 >>> + >>> +static struct HWBreakpoint { >>> + target_ulong addr; >>> + int type; >>> +} hw_debug_points[MAX_HW_BKPTS]; >>> + >>> +static CPUWatchpoint hw_watchpoint; >>> + >>> +/* 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; >>> + >>> +static void kvmppc_hw_debug_points_init(CPUPPCState *cenv) { >>> + static bool initialize = true; >>> + >>> + if (initialize) { >>> + if (cenv->excp_model == POWERPC_EXCP_BOOKE) { >>> + max_hw_breakpoint = 2; >>> + max_hw_watchpoint = 2; >>> + } >>> + >>> + initialize = false; >>> + } >>> + >>> + if ((max_hw_breakpoint + max_hw_watchpoint) > MAX_HW_BKPTS) { >>> + fprintf(stderr, "Error initializing h/w breakpoints\n"); >>> + return; >>> + } >>> +} >>> + >>> int kvm_arch_init_vcpu(CPUState *cs) >>> { >>> PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -437,6 +476,7 @@ int >>> kvm_arch_init_vcpu(CPUState *cs) >>> } >>> >>> kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode); >>> + kvmppc_hw_debug_points_init(cenv); >>> >>> return ret; >>> } >>> @@ -1343,24 +1383,216 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, >> struct kvm_sw_breakpoint *bp) >>> return 0; >>> } >>> >>> +static int find_hw_breakpoint(target_ulong addr, int type) { >>> + int n; >>> + >>> + assert((nb_hw_breakpoint + nb_hw_watchpoint) >>> + <= ARRAY_SIZE(hw_debug_points)); >>> + >>> + for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) { >>> + if (hw_debug_points[n].addr == addr && hw_debug_points[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) { >>> + assert((nb_hw_breakpoint + nb_hw_watchpoint) >>> + <= ARRAY_SIZE(hw_debug_points)); >>> + >>> + hw_debug_points[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr; >>> + hw_debug_points[nb_hw_breakpoint + nb_hw_watchpoint].type = type; >> Imagine the following: >> >> nb_hw_breakpoint = 2 >> nb_hw_watchpoint = 2 >> >> The assert above succeeds, because 4 <= 4. However, the array shuffling below >> accesses memory that is out of bounds: hw_debug_points[4]. > Right, this is just " < "; > but why not this crashed for me :( ? Because running over arrays usually doesn't crash on you ;). > >>> + >>> + 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_debug_points[n] = hw_debug_points[nb_hw_breakpoint + >>> + nb_hw_watchpoint]; >>> + >>> + return 0; >>> +} >>> + >>> +void kvm_arch_remove_all_hw_breakpoints(void) >>> +{ >>> + nb_hw_breakpoint = nb_hw_watchpoint = 0; } >>> + >>> void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg) >>> { >>> + int n; >>> + >>> /* Software Breakpoint updates */ >>> if (kvm_sw_breakpoints_active(cs)) { >>> dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP; >>> } >>> + >>> + assert((nb_hw_breakpoint + nb_hw_watchpoint) >>> + <= ARRAY_SIZE(hw_debug_points)); >>> + assert((nb_hw_breakpoint + nb_hw_watchpoint) <= >>> + ARRAY_SIZE(dbg->arch.bp)); >>> + >>> + 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++) { >>> + switch (hw_debug_points[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_debug_points[n].addr; >>> + } >>> + } >>> +} >>> + >>> +static void kvm_e500_handle_debug(CPUState *cs, int handle) { >>> + PowerPCCPU *cpu = POWERPC_CPU(cs); >>> + CPUPPCState *env = &cpu->env; >>> + >>> + cpu_synchronize_state(cs); >>> + env->spr[SPR_BOOKE_DBSR] = 0; >> I don't see how this would take any effect with KVM? > You mean we should move this to non-kvm; like excp_helper.c No, I mean I don't see where we synchronize the register to actually take an effect. > >> I don't see where we synchonize DBSR. > I will send a patch which synchromize DBSR. We're already in KVM code anyway. Why not set it explicitly? You already do set it explicitly in kvmppc_e500_inject_debug_exception(), no? > >>> } >>> >>> static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) >>> { >>> CPUState *cs = CPU(cpu); >>> + CPUPPCState *env = &cpu->env; >>> struct kvm_debug_exit_arch *arch_info = &run->debug.arch; >>> int handle = 0; >>> + int n; >>> + int flag = 0; >>> >>> - if (kvm_find_sw_breakpoint(cs, arch_info->address)) { >>> + if (cs->singlestep_enabled) { >>> + handle = 1; >>> + } else if (arch_info->status) { >>> + assert((nb_hw_breakpoint + nb_hw_watchpoint) >>> + <= ARRAY_SIZE(hw_debug_points)); >> I don't think this assert needs to be here :). You already assert() properly in >> the actual find function. > The find function whet down in if-else Yes, but we never access an array based on the offsets, so we're safe to only do it inside the find functions. Alex