[Qemu-devel] [PATCH 0/5 v3][RESEND] ppc: Add debug stub support

[Qemu-devel] [PATCH 0/5 v3][RESEND] ppc: Add debug stub support

[Bharat Bhushan]

This patchset add support for
 - software breakpoint
 - h/w breakpoint
 - h/w watchpoint

Please find description in individual patch.

v2->v3
 - Sharing of code for book3s support (which may come in future)
 - Initializing number of hw breakpoint/watchpoints from KVM world
 - Other minor cleanup/fixes

v1->v2:
 - use kvm_get_one_reg() for getting trap instruction
 - factored out e500 specific code based on exception model POWERPC_EXCP_BOOKE.
 - Not supporting ppc440

Bharat Bhushan (5):
  ppc: debug stub: Get trap instruction opcode from KVM
  ppc: Add interface to inject interrupt to guest
  ppc: Add debug interrupt injection handler
  ppc: Add software breakpoint support
  ppc: Add hw breakpoint watchpoint support

 target-ppc/kvm.c | 392 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 364 insertions(+), 28 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH 1/5 v3][RESEND] ppc: debug stub: Get trap instruction opcode from KVM

[Bharat Bhushan]

Get trap instruction opcode from KVM and this opcode will
be used for setting software breakpoint in following patch

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 target-ppc/kvm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index dfa5a26..1f78cd1 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -71,6 +71,8 @@ static int cap_papr;
 static int cap_htab_fd;
 static int cap_fixup_hcalls;
 
+static uint32_t debug_inst_opcode;
+
 /* XXX We have a race condition where we actually have a level triggered
  *     interrupt, but the infrastructure can't expose that yet, so the guest
  *     takes but ignores it, goes to sleep and never gets notified that there's
@@ -434,6 +436,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
         break;
     }
 
+    kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode);
+
     return ret;
 }
 
-- 
1.9.0

[Qemu-devel] [PATCH 2/5 v3][RESEND] ppc: Add interface to inject interrupt to guest

[Bharat Bhushan]

This patch adds interface to inject interrupt to guest.
Currently a void debug exception function added.
Follow up patch will use this interface to inject debug
interrupt to guest

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 target-ppc/kvm.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 1f78cd1..70f77d1 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -759,6 +759,24 @@ static int kvm_put_vpa(CPUState *cs)
 }
 #endif /* TARGET_PPC64 */
 
+static int kvmppc_inject_debug_exception(CPUState *cs)
+{
+    return 0;
+}
+
+static void kvmppc_inject_exception(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    if (env->pending_interrupts & (1 << PPC_INTERRUPT_DEBUG)) {
+        if (kvmppc_inject_debug_exception(cs)) {
+            fprintf(stderr, "%s: Debug exception injection failed\n", __func__);
+        }
+        return;
+    }
+}
+
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -772,6 +790,10 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return ret;
     }
 
+    if (env->pending_interrupts) {
+        kvmppc_inject_exception(cs);
+    }
+
     regs.ctr = env->ctr;
     regs.lr  = env->lr;
     regs.xer = cpu_read_xer(env);
-- 
1.9.0

[Qemu-devel] [PATCH 3/5 v3][RESEND] ppc: Add debug interrupt injection handler

[Bharat Bhushan]

With this patch a debug interrupt can be injected to guest.
Follow up patch will use this interface to inject debug
interrupt to guest if qemu will not be able to handle.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 target-ppc/kvm.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 70f77d1..5238de7 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -759,11 +759,59 @@ 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;
+    }
+
     return 0;
 }
 
+static int kvmppc_inject_debug_exception(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    int ret = -1;
+
+    switch (env->excp_model) {
+    case POWERPC_EXCP_BOOKE:
+        ret = kvmppc_e500_inject_debug_exception(cs);
+        break;
+    default:
+        fprintf(stderr, "%s: Invalid exception model %d\n",
+                __func__, env->excp_model);
+        break;
+    }
+    return ret;
+}
+
 static void kvmppc_inject_exception(CPUState *cs)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -772,8 +820,9 @@ static void kvmppc_inject_exception(CPUState *cs)
     if (env->pending_interrupts & (1 << PPC_INTERRUPT_DEBUG)) {
         if (kvmppc_inject_debug_exception(cs)) {
             fprintf(stderr, "%s: Debug exception injection failed\n", __func__);
+            return;
         }
-        return;
+        env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DEBUG);
     }
 }
 
-- 
1.9.0

[Qemu-devel] [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support

[Bharat Bhushan]

This patch allow insert/remove software breakpoint

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 target-ppc/kvm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 14 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 5238de7..8e2dbb3 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1317,6 +1317,53 @@ 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;
+}
+
+void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
+{
+    /* Software Breakpoint updates */
+    if (kvm_sw_breakpoints_active(cs)) {
+        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
+    }
+}
+
+static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
+{
+    CPUState *cs = CPU(cpu);
+    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
+    int handle = 0;
+
+    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
+        handle = 1;
+    }
+
+    return handle;
+}
+
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -1357,6 +1404,16 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
         ret = 0;
         break;
 
+    case KVM_EXIT_DEBUG:
+        DPRINTF("handle debug exception\n");
+        if (kvm_handle_debug(cpu, run)) {
+            ret = EXCP_DEBUG;
+            break;
+        }
+        /* re-enter, this exception was guest-internal */
+        ret = 0;
+        break;
+
     default:
         fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
         ret = -1;
@@ -2044,16 +2101,6 @@ void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
 
-int kvm_arch_insert_sw_breakpoint(CPUState *cpu, struct kvm_sw_breakpoint *bp)
-{
-    return -EINVAL;
-}
-
-int kvm_arch_remove_sw_breakpoint(CPUState *cpu, struct kvm_sw_breakpoint *bp)
-{
-    return -EINVAL;
-}
-
 int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int type)
 {
     return -EINVAL;
@@ -2068,10 +2115,6 @@ void kvm_arch_remove_all_hw_breakpoints(void)
 {
 }
 
-void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
-{
-}
-
 struct kvm_get_htab_buf {
     struct kvm_get_htab_header header;
     /*
-- 
1.9.0

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

[Alexander Graf]

On 24.06.14 14:10, Bharat Bhushan wrote:
> This patch allow insert/remove software breakpoint
>
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
>   target-ppc/kvm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 57 insertions(+), 14 deletions(-)
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 5238de7..8e2dbb3 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1317,6 +1317,53 @@ 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;

What if debug_inst_opcode has never been set (thus is 0)? In that case 
we should fail the insert, no?


Alex

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Tuesday, June 24, 2014 6:35 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; maddy@linux.vnet.ibm.com
> Subject: Re: [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
> 
> 
> On 24.06.14 14:10, Bharat Bhushan wrote:
> > This patch allow insert/remove software breakpoint
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> >   target-ppc/kvm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++---------
> --
> >   1 file changed, 57 insertions(+), 14 deletions(-)
> >
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> > 5238de7..8e2dbb3 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -1317,6 +1317,53 @@ 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;
> 
> What if debug_inst_opcode has never been set (thus is 0)?

Can "0" be a debug_inst_code ?

> In that case we should fail the insert, no?

Yes, will checking for "0" is sufficient or we need a cap_ also ?

Thanks
-Bharat

> 
> 
> Alex

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

[Alexander Graf]

On 24.06.14 15:11, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 24, 2014 6:35 PM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; maddy@linux.vnet.ibm.com
>> Subject: Re: [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
>>
>>
>> On 24.06.14 14:10, Bharat Bhushan wrote:
>>> This patch allow insert/remove software breakpoint
>>>
>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>> ---
>>>    target-ppc/kvm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++---------
>> --
>>>    1 file changed, 57 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
>>> 5238de7..8e2dbb3 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -1317,6 +1317,53 @@ 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;
>> What if debug_inst_opcode has never been set (thus is 0)?
> Can "0" be a debug_inst_code ?
>
>> In that case we should fail the insert, no?
> Yes, will checking for "0" is sufficient or we need a cap_ also ?

"0" can be a valid debug_inst_code, but I don't think it's a very smart 
one. Either way works for me.


Alex

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

[Madhavan Srinivasan]

On Tuesday 24 June 2014 05:40 PM, Bharat Bhushan wrote:
> This patch allow insert/remove software breakpoint
> 
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
>  target-ppc/kvm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 5238de7..8e2dbb3 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1317,6 +1317,53 @@ 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)) {

Instead of hard coding, can we use sizeof ()?

> +        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;
> +    }
> +

Same. Can we use sizeof?

> +    return 0;
> +}
> +
> +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> +{
> +    /* Software Breakpoint updates */
> +    if (kvm_sw_breakpoints_active(cs)) {
> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> +    }
> +}
> +
> +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
> +{
> +    CPUState *cs = CPU(cpu);
> +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> +    int handle = 0;
> +
> +    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> +        handle = 1;
> +    }
> +
> +    return handle;
> +}
> +
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -1357,6 +1404,16 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>          ret = 0;
>          break;
> 
> +    case KVM_EXIT_DEBUG:
> +        DPRINTF("handle debug exception\n");
> +        if (kvm_handle_debug(cpu, run)) {
> +            ret = EXCP_DEBUG;
> +            break;
> +        }
> +        /* re-enter, this exception was guest-internal */

Kindly can you explain when this will happen?

> +        ret = 0;
> +        break;
> +
>      default:
>          fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
>          ret = -1;
> @@ -2044,16 +2101,6 @@ void kvm_arch_init_irq_routing(KVMState *s)
>  {
>  }
> 
> -int kvm_arch_insert_sw_breakpoint(CPUState *cpu, struct kvm_sw_breakpoint *bp)
> -{
> -    return -EINVAL;
> -}
> -
> -int kvm_arch_remove_sw_breakpoint(CPUState *cpu, struct kvm_sw_breakpoint *bp)
> -{
> -    return -EINVAL;
> -}
> -
>  int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int type)
>  {
>      return -EINVAL;
> @@ -2068,10 +2115,6 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>  {
>  }
> 
> -void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
> -{
> -}
> -
>  struct kvm_get_htab_buf {
>      struct kvm_get_htab_header header;
>      /*
> 

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Madhavan Srinivasan [mailto:maddy@linux.vnet.ibm.com]
> Sent: Tuesday, June 24, 2014 8:59 PM
> To: Bhushan Bharat-R65777; agraf@suse.de
> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
> 
> On Tuesday 24 June 2014 05:40 PM, Bharat Bhushan wrote:
> > This patch allow insert/remove software breakpoint
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> >  target-ppc/kvm.c | 71
> > +++++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 57 insertions(+), 14 deletions(-)
> >
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> > 5238de7..8e2dbb3 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -1317,6 +1317,53 @@ 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)) {
> 
> Instead of hard coding, can we use sizeof ()?

Yes

> 
> > +        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;
> > +    }
> > +
> 
> Same. Can we use sizeof?

Yes

> 
> > +    return 0;
> > +}
> > +
> > +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug
> > +*dbg) {
> > +    /* Software Breakpoint updates */
> > +    if (kvm_sw_breakpoints_active(cs)) {
> > +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> > +    }
> > +}
> > +
> > +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) {
> > +    CPUState *cs = CPU(cpu);
> > +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> > +    int handle = 0;
> > +
> > +    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> > +        handle = 1;
> > +    }
> > +
> > +    return handle;
> > +}
> > +
> >  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)  {
> >      PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1357,6 +1404,16 @@ int
> > kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >          ret = 0;
> >          break;
> >
> > +    case KVM_EXIT_DEBUG:
> > +        DPRINTF("handle debug exception\n");
> > +        if (kvm_handle_debug(cpu, run)) {
> > +            ret = EXCP_DEBUG;
> > +            break;
> > +        }
> > +        /* re-enter, this exception was guest-internal */
> 
> Kindly can you explain when this will happen?

If the debug interrupt condition (breakpoint/watchpoint etc) is not set by qemu, i.e that is set by guest. 

Thanks
-Bharat

> 
> > +        ret = 0;
> > +        break;
> > +
> >      default:
> >          fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
> >          ret = -1;
> > @@ -2044,16 +2101,6 @@ void kvm_arch_init_irq_routing(KVMState *s)  {
> > }
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cpu, struct
> > kvm_sw_breakpoint *bp) -{
> > -    return -EINVAL;
> > -}
> > -
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cpu, struct
> > kvm_sw_breakpoint *bp) -{
> > -    return -EINVAL;
> > -}
> > -
> >  int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong
> > len, int type)  {
> >      return -EINVAL;
> > @@ -2068,10 +2115,6 @@ void kvm_arch_remove_all_hw_breakpoints(void)
> >  {
> >  }
> >
> > -void kvm_arch_update_guest_debug(CPUState *cpu, struct
> > kvm_guest_debug *dbg) -{ -}
> > -
> >  struct kvm_get_htab_buf {
> >      struct kvm_get_htab_header header;
> >      /*
> >

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

[Madhavan Srinivasan]

On Tuesday 24 June 2014 10:36 PM, Bharat.Bhushan@freescale.com wrote:
> 
> 
>> -----Original Message-----
>> From: Madhavan Srinivasan [mailto:maddy@linux.vnet.ibm.com]
>> Sent: Tuesday, June 24, 2014 8:59 PM
>> To: Bhushan Bharat-R65777; agraf@suse.de
>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
>>
>> On Tuesday 24 June 2014 05:40 PM, Bharat Bhushan wrote:
>>> This patch allow insert/remove software breakpoint
>>>
>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>> ---
>>>  target-ppc/kvm.c | 71
>>> +++++++++++++++++++++++++++++++++++++++++++++-----------
>>>  1 file changed, 57 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
>>> 5238de7..8e2dbb3 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -1317,6 +1317,53 @@ 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)) {
>>
>> Instead of hard coding, can we use sizeof ()?
> 
> Yes
> 
>>
>>> +        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;
>>> +    }
>>> +
>>
>> Same. Can we use sizeof?
> 
> Yes
> 
>>
>>> +    return 0;
>>> +}
>>> +
>>> +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug
>>> +*dbg) {
>>> +    /* Software Breakpoint updates */
>>> +    if (kvm_sw_breakpoints_active(cs)) {
>>> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>>> +    }
>>> +}
>>> +
>>> +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) {
>>> +    CPUState *cs = CPU(cpu);
>>> +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>>> +    int handle = 0;
>>> +
>>> +    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
>>> +        handle = 1;
>>> +    }
>>> +
>>> +    return handle;
>>> +}
>>> +
>>>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)  {
>>>      PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1357,6 +1404,16 @@ int
>>> kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>>>          ret = 0;
>>>          break;
>>>
>>> +    case KVM_EXIT_DEBUG:
>>> +        DPRINTF("handle debug exception\n");
>>> +        if (kvm_handle_debug(cpu, run)) {
>>> +            ret = EXCP_DEBUG;
>>> +            break;
>>> +        }
>>> +        /* re-enter, this exception was guest-internal */
>>
>> Kindly can you explain when this will happen?
> 
> If the debug interrupt condition (breakpoint/watchpoint etc) is not set by qemu, i.e that is set by guest. 
> 

OK. This is my understanding. Kindly correct if it is wrong.
If we are here without any breakpoint from qemu, are we not suppose to
pass it on to guest with an interrupt inject?

Also, can we make it as if-else with single break for KVM_EXIT_DEBUG case.

Regards
Maddy

> Thanks
> -Bharat
> 
>>
>>> +        ret = 0;
>>> +        break;
>>> +
>>>      default:
>>>          fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
>>>          ret = -1;
>>> @@ -2044,16 +2101,6 @@ void kvm_arch_init_irq_routing(KVMState *s)  {
>>> }
>>>
>>> -int kvm_arch_insert_sw_breakpoint(CPUState *cpu, struct
>>> kvm_sw_breakpoint *bp) -{
>>> -    return -EINVAL;
>>> -}
>>> -
>>> -int kvm_arch_remove_sw_breakpoint(CPUState *cpu, struct
>>> kvm_sw_breakpoint *bp) -{
>>> -    return -EINVAL;
>>> -}
>>> -
>>>  int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong
>>> len, int type)  {
>>>      return -EINVAL;
>>> @@ -2068,10 +2115,6 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>>>  {
>>>  }
>>>
>>> -void kvm_arch_update_guest_debug(CPUState *cpu, struct
>>> kvm_guest_debug *dbg) -{ -}
>>> -
>>>  struct kvm_get_htab_buf {
>>>      struct kvm_get_htab_header header;
>>>      /*
>>>
> 

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

[Alexander Graf]

On 24.06.14 19:59, Madhavan Srinivasan wrote:
> On Tuesday 24 June 2014 10:36 PM, Bharat.Bhushan@freescale.com wrote:
>>
>>> -----Original Message-----
>>> From: Madhavan Srinivasan [mailto:maddy@linux.vnet.ibm.com]
>>> Sent: Tuesday, June 24, 2014 8:59 PM
>>> To: Bhushan Bharat-R65777; agraf@suse.de
>>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>>> Subject: Re: [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
>>>
>>> On Tuesday 24 June 2014 05:40 PM, Bharat Bhushan wrote:
>>>> This patch allow insert/remove software breakpoint
>>>>
>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>> ---
>>>>   target-ppc/kvm.c | 71
>>>> +++++++++++++++++++++++++++++++++++++++++++++-----------
>>>>   1 file changed, 57 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
>>>> 5238de7..8e2dbb3 100644
>>>> --- a/target-ppc/kvm.c
>>>> +++ b/target-ppc/kvm.c
>>>> @@ -1317,6 +1317,53 @@ 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)) {
>>> Instead of hard coding, can we use sizeof ()?
>> Yes
>>
>>>> +        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;
>>>> +    }
>>>> +
>>> Same. Can we use sizeof?
>> Yes
>>
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug
>>>> +*dbg) {
>>>> +    /* Software Breakpoint updates */
>>>> +    if (kvm_sw_breakpoints_active(cs)) {
>>>> +        dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
>>>> +    }
>>>> +}
>>>> +
>>>> +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) {
>>>> +    CPUState *cs = CPU(cpu);
>>>> +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>>>> +    int handle = 0;
>>>> +
>>>> +    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
>>>> +        handle = 1;
>>>> +    }
>>>> +
>>>> +    return handle;
>>>> +}
>>>> +
>>>>   int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)  {
>>>>       PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1357,6 +1404,16 @@ int
>>>> kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>>>>           ret = 0;
>>>>           break;
>>>>
>>>> +    case KVM_EXIT_DEBUG:
>>>> +        DPRINTF("handle debug exception\n");
>>>> +        if (kvm_handle_debug(cpu, run)) {
>>>> +            ret = EXCP_DEBUG;
>>>> +            break;
>>>> +        }
>>>> +        /* re-enter, this exception was guest-internal */
>>> Kindly can you explain when this will happen?
>> If the debug interrupt condition (breakpoint/watchpoint etc) is not set by qemu, i.e that is set by guest.
>>
> OK. This is my understanding. Kindly correct if it is wrong.
> If we are here without any breakpoint from qemu, are we not suppose to
> pass it on to guest with an interrupt inject?

Yes. If the guest issued that instruction itself we need to pass in the 
interrupt that the guest would have received. I think in the book3s case 
this would be a PROGRAM interrupt rather than a DEBUG interrupt.


Alex

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

[Bharat Bhushan]

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;
+
+    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;
 }
 
 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));
+
+        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_debug_points[n].addr;
+                    hw_watchpoint.flags = flag;
+                }
+            }
+        }
+    } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
         handle = 1;
     }
 
+    if (handle) {
+        if (env->excp_model == POWERPC_EXCP_BOOKE) {
+            kvm_e500_handle_debug(cs, handle);
+        }
+    } else {
+       /* inject debug exception into guest */
+       env->pending_interrupts |=  1 << PPC_INTERRUPT_DEBUG;
+    }
     return handle;
 }
 
@@ -2101,20 +2333,6 @@ void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
 
-int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int type)
-{
-    return -EINVAL;
-}
-
-int kvm_arch_remove_hw_breakpoint(target_ulong addr, target_ulong len, int type)
-{
-    return -EINVAL;
-}
-
-void kvm_arch_remove_all_hw_breakpoints(void)
-{
-}
-
 struct kvm_get_htab_buf {
     struct kvm_get_htab_header header;
     /*
-- 
1.9.0

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

[Alexander Graf]

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

> +
> +    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? I don't see where 
we synchonize DBSR.

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


Alex

> +
> +        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_debug_points[n].addr;
> +                    hw_watchpoint.flags = flag;
> +                }
> +            }
> +        }
> +    } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
>           handle = 1;
>       }
>   
> +    if (handle) {
> +        if (env->excp_model == POWERPC_EXCP_BOOKE) {
> +            kvm_e500_handle_debug(cs, handle);
> +        }
> +    } else {
> +       /* inject debug exception into guest */
> +       env->pending_interrupts |=  1 << PPC_INTERRUPT_DEBUG;
> +    }
>       return handle;
>   }
>   
> @@ -2101,20 +2333,6 @@ void kvm_arch_init_irq_routing(KVMState *s)
>   {
>   }
>   
> -int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int type)
> -{
> -    return -EINVAL;
> -}
> -
> -int kvm_arch_remove_hw_breakpoint(target_ulong addr, target_ulong len, int type)
> -{
> -    return -EINVAL;
> -}
> -
> -void kvm_arch_remove_all_hw_breakpoints(void)
> -{
> -}
> -
>   struct kvm_get_htab_buf {
>       struct kvm_get_htab_header header;
>       /*

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

[Bharat.Bhushan]

> -----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 :( ?

> 
> > +
> > +    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

> I don't see where we synchonize DBSR.

I will send a patch which synchromize DBSR.

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

Thanks
-Bharat

> 
> 
> Alex
> 
> > +
> > +        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_debug_points[n].addr;
> > +                    hw_watchpoint.flags = flag;
> > +                }
> > +            }
> > +        }
> > +    } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> >           handle = 1;
> >       }
> >
> > +    if (handle) {
> > +        if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > +            kvm_e500_handle_debug(cs, handle);
> > +        }
> > +    } else {
> > +       /* inject debug exception into guest */
> > +       env->pending_interrupts |=  1 << PPC_INTERRUPT_DEBUG;
> > +    }
> >       return handle;
> >   }
> >
> > @@ -2101,20 +2333,6 @@ void kvm_arch_init_irq_routing(KVMState *s)
> >   {
> >   }
> >
> > -int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int
> type)
> > -{
> > -    return -EINVAL;
> > -}
> > -
> > -int kvm_arch_remove_hw_breakpoint(target_ulong addr, target_ulong len, int
> type)
> > -{
> > -    return -EINVAL;
> > -}
> > -
> > -void kvm_arch_remove_all_hw_breakpoints(void)
> > -{
> > -}
> > -
> >   struct kvm_get_htab_buf {
> >       struct kvm_get_htab_header header;
> >       /*

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

[Alexander Graf]

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

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

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Tuesday, June 24, 2014 8:21 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 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?

I think I did not get; please explain.

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

You mean checking boundary conditions when setting breakpoint is sufficient and no need in debug handler.

Thanks
-Bharat

> 
> 
> Alex

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

[Alexander Graf]

On 24.06.14 18:57, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 24, 2014 8:21 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 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?
> I think I did not get; please explain.

The sequence

   cpu_synchronize_registers(env);
   env->spr[foo] = bar;

is usually used to set an SPR in KVM. The nice thing about writing it 
like this is that it works with KVM and TCG just the same.

However, the function we're talking about here is in KVM only code. So 
instead of sync'ing DBSR we can just set it manually. The only case 
where this could fall apart is when we want to support live migration.

If you care about live migration, then make DBSR a kvm synchronized SPR 
with ONE_REG (we have nice infrastructure for that). But then don't set 
it manually in kvmppc_e500_inject_debug_exception() either, but only 
implicitly via env->spr[foo] values.

>
>>
>>>>>     }
>>>>>
>>>>>     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.
> You mean checking boundary conditions when setting breakpoint is sufficient and no need in debug handler.

I mean that the code below this assert always calls find_hw_breakpoint() 
which has the above assert and which is the only one that actually cares 
about the numbers because it actually uses them.


Alex