Re: [Qemu-devel] [PATCH 1/2] Guest debugging support for KVM - v2

[Anthony Liguori <aliguori@us.ibm.com>]

Jan Kiszka wrote:
> [ Recent kvm headers no longer require CONFIG_<ARCH>, so I dropped the
> related configure change. Moreover, this version is not capable of
> setting soft breakpoint in ROM memory. ]
>
> This is a backport of the guest debugging support for the KVM
> accelerator that is now part of the KVM tree. It implements the reworked
> KVM kernel API for guest debugging (KVM_CAP_SET_GUEST_DEBUG) which is
> not yet part of any mainline kernel but will probably be 2.6.30 stuff.
> So far supported is x86, but PPC is expected to catch up soon.
>
> Core features are:
>  - unlimited soft-breakpoints via code patching
>  - hardware-assisted x86 breakpoints and watchpoints
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
>  exec.c            |   10 ++-
>  gdbstub.c         |   29 ++++++--
>  gdbstub.h         |    7 ++
>  kvm-all.c         |  172 ++++++++++++++++++++++++++++++++++++++++++++++++
>  kvm.h             |   41 +++++++++++
>  target-i386/kvm.c |  191 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 440 insertions(+), 10 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 56e5e48..84c82ec 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1456,9 +1456,13 @@ void cpu_single_step(CPUState *env, int enabled)
>  #if defined(TARGET_HAS_ICE)
>      if (env->singlestep_enabled != enabled) {
>          env->singlestep_enabled = enabled;
> -        /* must flush all the translated code to avoid inconsistancies */
> -        /* XXX: only flush what is necessary */
> -        tb_flush(env);
> +        if (kvm_enabled())
> +            kvm_update_guest_debug(env, 0);
> +        else {
> +            /* must flush all the translated code to avoid inconsistancies */
> +            /* XXX: only flush what is necessary */
> +            tb_flush(env);
> +        }
>      }
>  #endif
>  }
> diff --git a/gdbstub.c b/gdbstub.c
> index b4b8292..0a91c7d 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -38,6 +38,7 @@
>  #define MAX_PACKET_LENGTH 4096
>  
>  #include "qemu_socket.h"
> +#include "kvm.h"
>  
>  
>  enum {
> @@ -1416,13 +1417,6 @@ void gdb_register_coprocessor(CPUState * env,
>      }
>  }
>  
> -/* GDB breakpoint/watchpoint types */
> -#define GDB_BREAKPOINT_SW        0
> -#define GDB_BREAKPOINT_HW        1
> -#define GDB_WATCHPOINT_WRITE     2
> -#define GDB_WATCHPOINT_READ      3
> -#define GDB_WATCHPOINT_ACCESS    4
> -
>  #ifndef CONFIG_USER_ONLY
>  static const int xlat_gdb_type[] = {
>      [GDB_WATCHPOINT_WRITE]  = BP_GDB | BP_MEM_WRITE,
> @@ -1436,6 +1430,9 @@ static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type)
>      CPUState *env;
>      int err = 0;
>  
> +    if (kvm_enabled())
> +        return kvm_insert_breakpoint(gdbserver_state->c_cpu, addr, len, type);
> +
>      switch (type) {
>      case GDB_BREAKPOINT_SW:
>      case GDB_BREAKPOINT_HW:
> @@ -1467,6 +1464,9 @@ static int gdb_breakpoint_remove(target_ulong addr, target_ulong len, int type)
>      CPUState *env;
>      int err = 0;
>  
> +    if (kvm_enabled())
> +        return kvm_remove_breakpoint(gdbserver_state->c_cpu, addr, len, type);
> +
>      switch (type) {
>      case GDB_BREAKPOINT_SW:
>      case GDB_BREAKPOINT_HW:
> @@ -1496,6 +1496,11 @@ static void gdb_breakpoint_remove_all(void)
>  {
>      CPUState *env;
>  
> +    if (kvm_enabled()) {
> +        kvm_remove_all_breakpoints(gdbserver_state->c_cpu);
> +        return;
> +    }
> +
>      for (env = first_cpu; env != NULL; env = env->next_cpu) {
>          cpu_breakpoint_remove_all(env, BP_GDB);
>  #ifndef CONFIG_USER_ONLY
> @@ -1536,6 +1541,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>              addr = strtoull(p, (char **)&p, 16);
>  #if defined(TARGET_I386)
>              s->c_cpu->eip = addr;
> +            if (kvm_enabled())
> +                kvm_put_registers(s->c_cpu);
>   

I really dislike sprinkling kvm_enabled() all over the place (which is 
why this isn't here already).

Can you introduce a qemu function as a generic hook?  Something like 
qemu_load_cpu_state(s->cpu)?

> +
> +#ifdef KVM_CAP_SET_GUEST_DEBUG
> +struct kvm_sw_breakpoint_head kvm_sw_breakpoints =
> +    TAILQ_HEAD_INITIALIZER(kvm_sw_breakpoints);
>   

Please make this part of KVMState.

Regards,

Anthony Liguori