[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 1/2] Guest debugging support for KVM - v2
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [PATCH 1/2] Guest debugging support for KVM - v2 |
Date: |
Mon, 26 Jan 2009 20:33:59 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
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 <address@hidden>
>> ---
>>
>> 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
signature.asc
Description: OpenPGP digital signature