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

[Alexander Graf <agraf@suse.de>]

On 24.06.14 16:37, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 24, 2014 6:50 PM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; maddy@linux.vnet.ibm.com
>> 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 <Bharat.Bhushan@freescale.com>
>>> ---
>>> 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?


>
>>>    }
>>>
>>>    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.


Alex