> -----Original Message-----
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 17, 2014 4:14 PM
> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>
>
> On 17.06.14 12:40, bharat.bhus...@freescale.com wrote:
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:ag...@suse.de]
> >> Sent: Tuesday, June 17, 2014 3:20 PM
> >> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>
> >>
> >> On 17.06.14 11:14, bharat.bhus...@freescale.com wrote:
> >>>> -----Original Message-----
> >>>> From: Alexander Graf [mailto:ag...@suse.de]
> >>>> Sent: Tuesday, June 17, 2014 1:46 PM
> >>>> To: Bhushan Bharat-R65777; qemu-...@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.bhus...@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?
> > You mean setting in kvm_arch_insert_hw_breakpoint() when called first time;
> something like:
> >
> > static bool init = 0;
> >
> > if (!init) {
> > if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > max_hw_breakpoint = 2;
> > max_hw_watchpoint = 2;
> > } else
> > // Add for book3s max_hw_watchpoint = 1;
> > }
> > init = 1;
> > }
>
> I would probably reuse max_hw_breakpoint as a hint whether it's initialized
> and
> put all of this into a small function, but yes :).
Ahh, we cannot do this in kvm_arch_insert_hw_breakpoint() as we can not get
"env" reference in this function. Prototype of this is:
int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int
type);
I will suggest that we initialize this from kvm_arch_init_vcpu(). This way we
are still in KVM zone.
Thanks
-Bharat
>
> >
> >>>>> }
> >>>>>
> >>>>> 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 :).
> > Yes, this is how x86 also works.
> > May be we move this in debug_handler function but ensure to keep it static.
> >
> >>>>> +
> >>>>> +/* 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 ;).
> > Ok :)
> > I will mention that this is untested in book3s
>
> That's ok - just make sure that the code does "the right thing" when all
> numbers
> are 0 ;).
>
> >
> >>>>> +
> >>>>> + 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.
> > Actually this will never overflow here because nb_hw_breakpoint and
> nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
> > Do you thing that to be double safe we can add a check?
>
> We only check against an overflow of hw_breakpoint[], not dbg->arch.bp.
> What if nb_hw_breakpoint becomes 17?
>
> >
> >>>>> + 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.
> > Maybe they use different encoding in type and accordingly other elements of
> struct. But I am fine to assume they will use as is and then change if needed.
>
> Perfect :).
>
>
> Alex
