On 17.06.14 11:14, bharat.bhus...@freescale.com wrote:
-----Original Message-----
From: Alexander Graf [mailto:ag...@suse.de]
Sent: Tuesday, June 17, 2014 1:46 PM
To: Bhushan Bharat-R65777; qemu-...@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 <bharat.bhus...@freescale.com>
---
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
