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

[Alexander Graf <agraf@suse.de>]

On 16.06.14 06:27, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Friday, June 13, 2014 4:55 PM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
>>
>>
>> On 12.06.14 09:05, Bharat.Bhushan@freescale.com wrote:
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Wednesday, June 11, 2014 6:35 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: qemu-ppc@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.Bhushan@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