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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5081 bytes --]

Anthony Liguori wrote:
> 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).

This is news.

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

It's no big deal to do this, but

 a) So far I thought the thing about "if (kvm_enabled()) code" was
    letting the compiler remove any traces of it in case kvm is not
    built-in (or unsupported on some arch). Or is a static-inline
    qemu_load_cpu_state acceptable?

 b) I don't want to start reinventing Glauber's accel abstractions.
    Wouldn't it make sense to do this in one larger step instead of
    permanently changing APIs?

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

OK, will do.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]