On 16.06.14 06:27, bharat.bhus...@freescale.com wrote:
-----Original Message-----
From: Alexander Graf [mailto:ag...@suse.de]
Sent: Friday, June 13, 2014 4:55 PM
To: Bhushan Bharat-R65777
Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org
Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
On 12.06.14 09:05, bharat.bhus...@freescale.com wrote:
-----Original Message-----
From: Alexander Graf [mailto:ag...@suse.de]
Sent: Wednesday, June 11, 2014 6:35 PM
To: Bhushan Bharat-R65777
Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org
Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
On 06/10/2014 05:06 PM, 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>
---
hw/ppc/e500.c | 1 +
target-ppc/kvm.c | 304
++++++++++++++++++++++++++++++++++++++++++++++---
--
target-ppc/kvm_ppc.h | 1 +
3 files changed, 278 insertions(+), 28 deletions(-)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..514c595
100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -853,6 +853,7 @@ void ppce500_init(MachineState *machine,
PPCE500Params
*params)
if (kvm_enabled()) {
kvmppc_init();
}
+ kvmppc_e500_hw_breakpoint_init();
}
static int e500_ccsr_initfn(SysBusDevice *dev) diff --git
a/target-ppc/kvm.c b/target-ppc/kvm.c index 1d2384d..f5fbec6 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
@@ -768,6 +769,38 @@ static int kvm_put_vpa(CPUState *cs)
static int kvmppc_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;
+ }
+
I don't think any of this code should ever run for non-e500, no?
You mean the code below in this function?
Yeah :).
Why you think accessing sregs (cssr0/1, dsrr0/1 and ioctl) is e500 specific.
Are not these valid for 4xx as well?
Ah, misunderstanding. I don't really care about 440 - we don't seem to
have any users there. However, I do care about book3s - and the code
isn't compatible with book3s at all ;).
+ if (sregs.u.e.features & KVM_SREGS_E_ED) {
Hrm - we never seem to set E_ED in kvm?
Uhh, you are right. Going through the whole discussion about interrupt
injection to guest I found that one patch missed for upstream.
Will send that patch
+ 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);
+
return 0;
}
@@ -1275,6 +1308,239 @@ 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) {
+ uint32_t sc = tswap32(debug_inst_opcode);
Heh - this will become a lot of fun for real LE host as well as guest
systems.
I am trying to understand the problem here, We want to byteswap opcode only if
it is mixed endian (host and guest are of different endianess) case?
Yes :).
For now just remove the tswap and add a comment that this needs fixing for
LE.
+
+ 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 != tswap32(debug_inst_opcode) ||
Same here.
In fact, neither of the 2 operations are in a fast path. Can't we
just fetch the debug inst opcode on demand in a function here?
Ok will do that.
That will allow for easier byte
swapping depending on the guest's MSR.LE setting later as well.
+ cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1))
{
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static struct HWBreakpoint {
+ target_ulong addr;
+ int type;
+} hw_breakpoint[6];
+
+static int nb_hw_breakpoint;
+static int nb_hw_watchpoint;
+static int max_hw_breakpoint = 4;
+static int max_hw_watchpoint = 2;
+
+void kvmppc_e500_hw_breakpoint_init(void)
+{
+ max_hw_breakpoint = 2;
+ max_hw_watchpoint = 2;
Can we somehow get this information from kvm and set it in kvm_arch_init?
Will add one_reg to get this information.
We can also fall back to always make this 2 when there's no other way to
enumerate. But setting the default to 4/2 and then revert to 2/2 hard codedly
seems odd.
Btw, where does the 4/2 come from?
Because 44x have 4 IACs.
Ah, ok. Just ignore 440.
Worst
case we'll have to look at the cpu class and derive it from there,
but it really should live solely inside the kvm file.
+}
+
+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) {
+ 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 CPUWatchpoint hw_watchpoint;
+
+
+static int kvm_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 (cs->singlestep_enabled) {
+ handle = 1;
+ } else if (arch_info->status) {
+ 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;
+ }
+ }
+ } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
+ handle = 1;
+ }
+
+ cpu_synchronize_state(cs);
+ if (handle) {
+ env->spr[SPR_BOOKE_DBSR] = 0;
This is pretty e500 specific.
You mean accessing DBSR, right ?
To handle core specific stuff, should we add some generic function like
kvm_ppc_set/clear_debug_event() in hw/ppc/e500_debug.c, hw/ppc/ppc4xx_debug.c
etc ?
I don't think this is hardware, it's part of the core. So it'd belong to target-
ppc/. Probably part of the excp helper file.
Is not this (dbsr) is booke specific and not just e500 specific? Should this be
accessed under POWERPC_MMU_BOOKE and POWERPC_MMU_BOOKE206 cpu models?
Again, I think we can easily ignore 440, so only care about booke206 and
book3s for now. And I'm pretty sure we can't reuse any of the code in
kvm_handle_debug() between the two targets, so you want to branch out
early and have a specific booke206 / e500 handler.
if (env->excp_model == EXCP_BOOKE206)
return kvm_handle_debug_booke206(...);
Alex
