Re: [Qemu-devel] [PATCH 3/4 v8] ppc: Add software breakpoint support

[Madhavan Srinivasan]

On Monday 14 July 2014 02:45 PM, Bharat Bhushan wrote:
> This patch allow insert/remove software breakpoint.
> 
> When QEMU is not able to handle debug exception then we inject
> program exception to guest because for software breakpoint QEMU
> uses a ehpriv-1 instruction;
> So there cannot be any reason that we are in qemu with exit reason
> KVM_EXIT_DEBUG  for guest set debug exception, only possibility is
> guest executed ehpriv-1 privilege instruction and that's why we are
> injecting program exception.
> 
> Signed-off-by: Bharat Bhushan <address@hidden>
> ---
> v7->v8
>  - Decrement nip by 4 before calling ppc_cpu_do_interrupt()
>    as pointed by "Madhavan Srinivasan"

Just a minor change. We increment the nip.

>  - Also added missing cpu_sync()
> 
>  target-ppc/kvm.c | 93 
> +++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 79 insertions(+), 14 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index e00a20f..0f2e8e4 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1275,6 +1275,75 @@ 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,
> +                            sizeof(sc), 0) ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, sizeof(sc), 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, sizeof(sc), 0) ||
> +        sc != debug_inst_opcode ||
> +        cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> +                            sizeof(sc), 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);
> +    CPUPPCState *env = &cpu->env;
> +    struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> +    int handle = 0;
> +
> +    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> +        handle = 1;
> +    } else {
> +        /* QEMU is not able to handle debug exception, so inject
> +         * program exception to guest;
> +         * Yes program exception NOT debug exception !!
> +         * For software breakpoint QEMU uses a ehpriv-1 instruction;
Instead, can you say "QEMU uses a SW breakpoint instruction". In server
side, we use illegal instruction to trap to hypervisor ("0x00dddd00")
> +         * So there cannot be any reason that we are here for guest
> +         * set debug exception, only possibility is guest executed a
> +         * privilege instruction and that's why we are injecting
                ^^^^privilege or illegal instruction

My Bad. I should have read just last time. But just minor and I am ok as is.

Reviewed By: Madhavan Srinivasan <address@hidden>

> +         * program exception.
> +         */
> +
> +        cpu_synchronize_state(cs);
> +        /* env->nip is PC, so increment this by 4 to use
> +         * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
> +         */
> +        env->nip += 4;
> +        cs->exception_index = POWERPC_EXCP_PROGRAM;
> +        env->error_code = POWERPC_EXCP_INVAL;
> +        ppc_cpu_do_interrupt(cs);
> +    }
> +
> +    return handle;
> +}
> +
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -1315,6 +1384,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;
> @@ -2003,16 +2082,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;
> @@ -2027,10 +2096,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;
>      /*
>