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

Re: [Qemu-devel] [PATCH 3/3] ppc debug: Add debug stub support

From: Alexander Graf
Subject: Re: [Qemu-devel] [PATCH 3/3] ppc debug: Add debug stub support
Date: Mon, 16 Jun 2014 11:04:06 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 

On 16.06.14 06:27, address@hidden wrote:



-----Original Message-----
From: Alexander Graf [mailto:address@hidden
Sent: Friday, June 13, 2014 4:55 PM
To: Bhushan Bharat-R65777
Cc: address@hidden; address@hidden
Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support


On 12.06.14 09:05, address@hidden wrote:

-----Original Message-----
From: Alexander Graf [mailto:address@hidden
Sent: Wednesday, June 11, 2014 6:35 PM
To: Bhushan Bharat-R65777
Cc: address@hidden; address@hidden
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 <address@hidden>
---
    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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]