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

[Alexander Graf <agraf@suse.de>]

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