From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LRXDn-000220-Hv for qemu-devel@nongnu.org; Mon, 26 Jan 2009 14:34:11 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LRXDl-0001zx-OS for qemu-devel@nongnu.org; Mon, 26 Jan 2009 14:34:10 -0500 Received: from [199.232.76.173] (port=39515 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LRXDl-0001zZ-JB for qemu-devel@nongnu.org; Mon, 26 Jan 2009 14:34:09 -0500 Received: from fmmailgate02.web.de ([217.72.192.227]:50409) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LRXDk-0008WV-PQ for qemu-devel@nongnu.org; Mon, 26 Jan 2009 14:34:09 -0500 Received: from smtp07.web.de (fmsmtp07.dlan.cinetic.de [172.20.5.215]) by fmmailgate02.web.de (Postfix) with ESMTP id C9635F97CA82 for ; Mon, 26 Jan 2009 20:34:03 +0100 (CET) Received: from [88.65.42.77] (helo=[192.168.1.198]) by smtp07.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.110 #277) id 1LRXDf-0004pT-00 for qemu-devel@nongnu.org; Mon, 26 Jan 2009 20:34:03 +0100 Message-ID: <497E1027.3090508@web.de> Date: Mon, 26 Jan 2009 20:33:59 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <497CAB64.4090805@web.de> <497DF9AC.4060405@us.ibm.com> In-Reply-To: <497DF9AC.4060405@us.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigA0011C7C92018278CB2F36DB" Sender: jan.kiszka@web.de Subject: [Qemu-devel] Re: [PATCH 1/2] Guest debugging support for KVM - v2 Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigA0011C7C92018278CB2F36DB Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Anthony Liguori wrote: > Jan Kiszka wrote: >> [ Recent kvm headers no longer require CONFIG_, 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 rework= ed >> 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 >> --- >> >> 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 !=3D enabled) { >> env->singlestep_enabled =3D 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 >> =20 >> #include "qemu_socket.h" >> +#include "kvm.h" >> =20 >> =20 >> enum { >> @@ -1416,13 +1417,6 @@ void gdb_register_coprocessor(CPUState * env, >> } >> } >> =20 >> -/* 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[] =3D { >> [GDB_WATCHPOINT_WRITE] =3D 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 =3D 0; >> =20 >> + 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 =3D 0; >> =20 >> + 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; >> =20 >> + if (kvm_enabled()) { >> + kvm_remove_all_breakpoints(gdbserver_state->c_cpu); >> + return; >> + } >> + >> for (env =3D first_cpu; env !=3D NULL; env =3D 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 =3D strtoull(p, (char **)&p, 16); >> #if defined(TARGET_I386) >> s->c_cpu->eip =3D addr; >> + if (kvm_enabled()) >> + kvm_put_registers(s->c_cpu); >> =20 >=20 > I really dislike sprinkling kvm_enabled() all over the place (which is > why this isn't here already). This is news. >=20 > 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? >=20 >> + >> +#ifdef KVM_CAP_SET_GUEST_DEBUG >> +struct kvm_sw_breakpoint_head kvm_sw_breakpoints =3D >> + TAILQ_HEAD_INITIALIZER(kvm_sw_breakpoints); >> =20 >=20 > Please make this part of KVMState. OK, will do. Jan --------------enigA0011C7C92018278CB2F36DB Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkl+EC4ACgkQniDOoMHTA+kUhQCggubrbSPgRMBUVp+Ed42RSZGz HyoAn1pMEWA1BZ23YWM3klCYvPrAr9I6 =ctKd -----END PGP SIGNATURE----- --------------enigA0011C7C92018278CB2F36DB--