qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoin

From: Alexander Graf
Subject: Re: [Qemu-devel] [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support
Date: Wed, 25 Jun 2014 00:46:56 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 

On 24.06.14 18:57, address@hidden wrote:



-----Original Message-----
From: Alexander Graf [mailto:address@hidden
Sent: Tuesday, June 24, 2014 8:21 PM
To: Bhushan Bharat-R65777
Cc: address@hidden; address@hidden; address@hidden
Subject: Re: [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support


On 24.06.14 16:37, address@hidden wrote:

-----Original Message-----
From: Alexander Graf [mailto:address@hidden
Sent: Tuesday, June 24, 2014 6:50 PM
To: Bhushan Bharat-R65777
Cc: address@hidden; address@hidden;
address@hidden
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 <address@hidden>
---
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?

I think I did not get; please explain.


The sequence

  cpu_synchronize_registers(env);
  env->spr[foo] = bar;
is usually used to set an SPR in KVM. The nice thing about writing it like this is that it works with KVM and TCG just the same.
However, the function we're talking about here is in KVM only code. So instead of sync'ing DBSR we can just set it manually. The only case where this could fall apart is when we want to support live migration.
If you care about live migration, then make DBSR a kvm synchronized SPR with ONE_REG (we have nice infrastructure for that). But then don't set it manually in kvmppc_e500_inject_debug_exception() either, but only implicitly via env->spr[foo] values. 






    }

    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.

You mean checking boundary conditions when setting breakpoint is sufficient and 
no need in debug handler.
I mean that the code below this assert always calls find_hw_breakpoint() which has the above assert and which is the only one that actually cares about the numbers because it actually uses them. 


Alex
reply via email to
[Prev in Thread] Current Thread [Next in Thread]