qemu: refactor cpu_watch/breakpoint API

RFC PATCH In review Jan Kiszka 3 participants 20 messages May 16, 2008 – May 25, 2008 1 message not in the archive

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jan Kiszka

author

2008-05-16 16:01 UTC #1 lore raw

RFC PATCH 1/4 qemu: refactor cpu_watch/breakpoint API

[ Should apply against vanilla QEMU, but not ATM due to ongoing constructions in gdbstub. ]

This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow us hooking in with KVM and doing guest debugging differently (maybe QEMUAccel should provide appropriate callbacks for this, too). But it also allows to extend QEMU's debugging features one day, specifically /wrt different watchpoint types.

---
 qemu/cpu-all.h |   14 ++++++++++----
 qemu/exec.c    |   30 ++++++++++++++++++++----------
 qemu/gdbstub.c |   50 +++++++++++++++++++++++++++++---------------------
 3 files changed, 59 insertions(+), 35 deletions(-)

qemu/cpu-all.h +10 -4

Index: b/qemu/cpu-all.h
===================================================================
@@ -758,10 +758,16 @@ extern int code_copy_enabled;
 void cpu_interrupt(CPUState *s, int mask);
 void cpu_reset_interrupt(CPUState *env, int mask);
 
-int cpu_watchpoint_insert(CPUState *env, target_ulong addr);
-int cpu_watchpoint_remove(CPUState *env, target_ulong addr);
-int cpu_breakpoint_insert(CPUState *env, target_ulong pc);
-int cpu_breakpoint_remove(CPUState *env, target_ulong pc);
+#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
+
+int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len, int type);
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len, int type);
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, target_ulong len, int type);
+int cpu_breakpoint_remove(CPUState *env, target_ulong pc, target_ulong len, int type);
 void cpu_single_step(CPUState *env, int enabled);
 void cpu_reset(CPUState *s);

qemu/exec.c +20 ~~-10~~

Index: b/qemu/exec.c
===================================================================
@@ -1104,16 +1104,20 @@ static void breakpoint_invalidate(CPUSta
 #endif
 
 /* Add a watchpoint.  */
-int  cpu_watchpoint_insert(CPUState *env, target_ulong addr)
+int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
+                          int type)
 {
     int i;
 
+    if (type != GDB_WATCHPOINT_WRITE)
+        return -ENOSYS;
+
     for (i = 0; i < env->nb_watchpoints; i++) {
         if (addr == env->watchpoint[i].vaddr)
             return 0;
     }
     if (env->nb_watchpoints >= MAX_WATCHPOINTS)
-        return -1;
+        return -ENOBUFS;
 
     i = env->nb_watchpoints++;
     env->watchpoint[i].vaddr = addr;

@@ -1126,10 +1130,14 @@ int  cpu_watchpoint_insert(CPUState *env
 }
 
 /* Remove a watchpoint.  */
-int cpu_watchpoint_remove(CPUState *env, target_ulong addr)
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
+                          int type)
 {
     int i;
 
+    if (type != GDB_WATCHPOINT_WRITE)
+        return -ENOSYS;
+
     for (i = 0; i < env->nb_watchpoints; i++) {
         if (addr == env->watchpoint[i].vaddr) {
             env->nb_watchpoints--;

@@ -1138,12 +1146,13 @@ int cpu_watchpoint_remove(CPUState *env,
             return 0;
         }
     }
-    return -1;
+    return -ENOENT;
 }
 
 /* add a breakpoint. EXCP_DEBUG is returned by the CPU loop if a
    breakpoint is reached */
-int cpu_breakpoint_insert(CPUState *env, target_ulong pc)
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, target_ulong len,
+                          int type)
 {
 #if defined(TARGET_HAS_ICE)
     int i;

@@ -1154,7 +1163,7 @@ int cpu_breakpoint_insert(CPUState *env,
     }
 
     if (env->nb_breakpoints >= MAX_BREAKPOINTS)
-        return -1;
+        return -ENOBUFS;
     env->breakpoints[env->nb_breakpoints++] = pc;
 
     if (kvm_enabled())

@@ -1163,12 +1172,13 @@ int cpu_breakpoint_insert(CPUState *env,
     breakpoint_invalidate(env, pc);
     return 0;
 #else
-    return -1;
+    return -ENOSYS;
 #endif
 }
 
 /* remove a breakpoint */
-int cpu_breakpoint_remove(CPUState *env, target_ulong pc)
+int cpu_breakpoint_remove(CPUState *env, target_ulong pc, target_ulong len,
+                          int type)
 {
 #if defined(TARGET_HAS_ICE)
     int i;

@@ -1176,7 +1186,7 @@ int cpu_breakpoint_remove(CPUState *env,
         if (env->breakpoints[i] == pc)
             goto found;
     }
-    return -1;
+    return -ENOENT;
  found:
     env->nb_breakpoints--;
     if (i < env->nb_breakpoints)

@@ -1188,7 +1198,7 @@ int cpu_breakpoint_remove(CPUState *env,
     breakpoint_invalidate(env, pc);
     return 0;
 #else
-    return -1;
+    return -ENOSYS;
 #endif
 }

qemu/gdbstub.c +29 ~~-21~~

74 lines not on this page · raw message

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Jan Kiszka

author

2008-05-16 16:01 UTC #2 lore raw

RFC PATCH 2/4 kvm: Arch-specifc KVM_EXIT_DEBUG payload

In reply to a message not in the archive

This adds an arch field to kvm_run.debug, the payload that is returned to user space on KVM_EXIT_DEBUG guest exits. For x86, this field is now supposed to report the precise debug exception (#DB or #BP) and the current state of the debug registers (the latter is not yet implemented).

---
 arch/x86/kvm/vmx.c    |    1 +
 include/asm-x86/kvm.h |    5 +++++
 include/linux/kvm.h   |    1 +
 3 files changed, 7 insertions(+)

arch/x86/kvm/vmx.c +1 -0

Index: b/arch/x86/kvm/vmx.c
===================================================================
@@ -2282,6 +2282,7 @@ static int handle_exception(struct kvm_v
 	if ((intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VECTOR_MASK)) ==
 	    (INTR_TYPE_EXCEPTION | 1)) {
 		kvm_run->exit_reason = KVM_EXIT_DEBUG;
+		kvm_run->debug.arch.exception = 1;
 		return 0;
 	}
 	kvm_run->exit_reason = KVM_EXIT_EXCEPTION;

include/asm-x86/kvm.h +5 -0

Index: b/include/asm-x86/kvm.h
===================================================================
@@ -230,4 +230,9 @@ struct kvm_pit_state {
 #define KVM_TRC_APIC_ACCESS      (KVM_TRC_HANDLER + 0x14)
 #define KVM_TRC_TDP_FAULT        (KVM_TRC_HANDLER + 0x15)
 
+struct kvm_debug_exit_arch {
+	__u32 exception;
+	__u64 dr[8];
+};
+
 #endif

include/linux/kvm.h +1 -0

Index: b/include/linux/kvm.h
===================================================================
@@ -125,6 +125,7 @@ struct kvm_run {
 			__u64 data_offset; /* relative to kvm_run start */
 		} io;
 		struct {
+			struct kvm_debug_exit_arch arch;
 		} debug;
 		/* KVM_EXIT_MMIO */
 		struct {

Avi Kivity

2008-05-21 15:59 UTC #3 lore raw

Re: [kvm-devel] [RFC][PATCH 2/4] kvm: Arch-specifc KVM_EXIT_DEBUG payload

Quoted 11 lines · Jan Kiszka

This adds an arch field to kvm_run.debug, the payload that is returned to user space on KVM_EXIT_DEBUG guest exits. For x86, this field is now supposed to report the precise debug exception (#DB or #BP) and the current state of the debug registers (the latter is not yet implemented).
Index: b/include/asm-x86/kvm.h
===================================================================

include/asm-x86/kvm.h struct kvm_pit_state { · line 238

Quoted 9 lines

include/asm-x86/kvm.h +5 -0

@@ -230,4 +230,9 @@ struct kvm_pit_state {
 #define KVM_TRC_APIC_ACCESS      (KVM_TRC_HANDLER + 0x14)
 #define KVM_TRC_TDP_FAULT        (KVM_TRC_HANDLER + 0x15)
 
+struct kvm_debug_exit_arch {
+	__u32 exception;
+	__u64 dr[8];
+};
+

Need empty structures for non-x86.

Need a KVM_CAP_ to indicate presence of this feature.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2008-05-22 13:27 UTC #4 lore raw

Re: [kvm-devel] [RFC][PATCH 2/4] kvm: Arch-specifc KVM_EXIT_DEBUG payload

Quoted 24 lines · Avi Kivity

Quoted 11 lines · Jan Kiszka
This adds an arch field to kvm_run.debug, the payload that is returned to user space on KVM_EXIT_DEBUG guest exits. For x86, this field is now supposed to report the precise debug exception (#DB or #BP) and the current state of the debug registers (the latter is not yet implemented).
Index: b/include/asm-x86/kvm.h
===================================================================
include/asm-x86/kvm.h struct kvm_pit_state { · line 238
Quoted 9 lines
include/asm-x86/kvm.h +5 -0
@@ -230,4 +230,9 @@ struct kvm_pit_state {
 #define KVM_TRC_APIC_ACCESS      (KVM_TRC_HANDLER + 0x14)
 #define KVM_TRC_TDP_FAULT        (KVM_TRC_HANDLER + 0x15)
 
+struct kvm_debug_exit_arch {
+    __u32 exception;
+    __u64 dr[8];
+};
+
 
Need empty structures for non-x86.

Quoted 1 line

Need a KVM_CAP_ to indicate presence of this feature.

Have all this in patch already, but need time to finish it, test it, and roll it out. There are two usability issues I would like to resolve first (to see if that has impact on the kernel-user interface), see following posts.

Jan

Avi Kivity

2008-05-21 16:04 UTC #5 lore raw

Re: [kvm-devel] [RFC][PATCH 2/4] kvm: Arch-specifc KVM_EXIT_DEBUG payload

In reply to Jan Kiszka

include/asm-x86/kvm.h struct kvm_pit_state { · line 234

Quoted 9 lines · Jan Kiszka

This adds an arch field to kvm_run.debug, the payload that is returned to user space on KVM_EXIT_DEBUG guest exits. For x86, this field is now supposed to report the precise debug exception (#DB or #BP) and the current state of the debug registers (the latter is not yet implemented).
include/asm-x86/kvm.h +2 -0
+struct kvm_debug_exit_arch {
+	__u32 exception;

Need padding here.

include/asm-x86/kvm.h struct kvm_pit_state { · line 237

Quoted 4 lines

include/asm-x86/kvm.h +3 -0
+	__u64 dr[8];
+};
+

In general I prefer decoding bitfields into something more usable (see for example the segment registers), but in this case, I guess the raw registers are better.

We need to handle branch tracing and last branch recording as well. It doesn't have to be in this patchset, though.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2008-05-22 13:42 UTC #6 lore raw

Re: [kvm-devel] [RFC][PATCH 2/4] kvm: Arch-specifc KVM_EXIT_DEBUG payload

Quoted 13 lines · Avi Kivity

include/asm-x86/kvm.h struct kvm_pit_state { · line 234
Quoted 9 lines · Jan Kiszka
This adds an arch field to kvm_run.debug, the payload that is returned to user space on KVM_EXIT_DEBUG guest exits. For x86, this field is now supposed to report the precise debug exception (#DB or #BP) and the current state of the debug registers (the latter is not yet implemented).
include/asm-x86/kvm.h +2 -0
+struct kvm_debug_exit_arch {
+    __u32 exception;
Need padding here.

Quoted 8 lines

include/asm-x86/kvm.h struct kvm_pit_state { · line 237
Quoted 4 lines
include/asm-x86/kvm.h +3 -0
+    __u64 dr[8];
+};
+
In general I prefer decoding bitfields into something more usable (see for example the segment registers), but in this case, I guess the raw registers are better.

IMHO, there is no point in parsing those arch-specific bit field in kernel space as long as the kernel has nothing to do with them (except saving and restoring them).

BTW, the upcoming version will look like this:

struct kvm_debug_exit_arch {
        __u32 exception;
        __u32 pad;
        __u64 dr6;
        __u64 dr7;
};

(No need for reporting unused or unmodified registers here.)

Quoted 3 lines

We need to handle branch tracing and last branch recording as well. It doesn't have to be in this patchset, though.

IIRC, both features are fairly vendor specific. Do we have the same level of support on both AMD and Intel CPUs? Moreover, I doubt that the debug interface would be the right place for them.

Jan

Avi Kivity

2008-05-22 13:59 UTC #7 lore raw

Re: [kvm-devel] [RFC][PATCH 2/4] kvm: Arch-specifc KVM_EXIT_DEBUG payload

Quoted 9 lines · Jan Kiszka

Quoted 4 lines
We need to handle branch tracing and last branch recording as well. It doesn't have to be in this patchset, though.
IIRC, both features are fairly vendor specific. Do we have the same level of support on both AMD and Intel CPUs? Moreover, I doubt that the debug interface would be the right place for them.

Branch tracing is common, and is very much related to debugging (it modifies singlestep to trap only on branches, not on non-branching insns). LBR is either common or very similar, and is also very important for debugging.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2008-05-22 14:32 UTC #8 lore raw

Re: [kvm-devel] [RFC][PATCH 2/4] kvm: Arch-specifc KVM_EXIT_DEBUG payload

Quoted 11 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka
Quoted 4 lines
We need to handle branch tracing and last branch recording as well. It doesn't have to be in this patchset, though.
IIRC, both features are fairly vendor specific. Do we have the same level of support on both AMD and Intel CPUs? Moreover, I doubt that the debug interface would be the right place for them.

Quoted 4 lines

Branch tracing is common, and is very much related to debugging (it modifies singlestep to trap only on branches, not on non-branching insns). LBR is either common or very similar, and is also very important for debugging.

Well, might be useful. But then we would also have to teach this support to the gdb remote protocol and the gdb frontend, because neither of both seems to make use of it yet. And do other archs have it as well? If at all, I guess the way this is realized there varies a lot, and thus abstracting it is the real problem.

However, we can keep it in mind for the time the basic feature set works perfectly. :)

Jan

Avi Kivity

2008-05-22 14:35 UTC #9 lore raw

Re: [kvm-devel] [RFC][PATCH 2/4] kvm: Arch-specifc KVM_EXIT_DEBUG payload

Quoted 9 lines · Jan Kiszka

Well, might be useful. But then we would also have to teach this support to the gdb remote protocol and the gdb frontend, because neither of both seems to make use of it yet. And do other archs have it as well? If at all, I guess the way this is realized there varies a lot, and thus abstracting it is the real problem.
However, we can keep it in mind for the time the basic feature set works perfectly. :)

Right, no need to implement it now. But you can reserve some bytes in the ioctl argument.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2008-05-16 16:02 UTC #10 lore raw

RFC PATCH 3/4 kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

In reply to a message not in the archive

In order to allow the gdbstub of QEMU to push (soft) breakpoint handling completely into the gdb frontend, this patch enables guest exits also for #BP exceptions - in case guest debugging was turned on.

Along this enhancement, this patch also fixes the flag manipulation for the singlestep mode.

---
 arch/x86/kvm/vmx.c |   38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

arch/x86/kvm/vmx.c +15 ~~-23~~

Index: b/arch/x86/kvm/vmx.c
===================================================================
@@ -461,7 +461,7 @@ static void update_exception_bitmap(stru
 	if (!vcpu->fpu_active)
 		eb |= 1u << NM_VECTOR;
 	if (vcpu->guest_debug.enabled)
-		eb |= 1u << 1;
+		eb |= (1u << 1) | (1u << 3);
 	if (vcpu->arch.rmode.active)
 		eb = ~0;
 	if (vm_need_ept())

@@ -949,6 +949,7 @@ static int set_guest_debug(struct kvm_vc
 {
 	unsigned long dr7 = 0x400;
 	int old_singlestep;
+	unsigned long flags;
 
 	old_singlestep = vcpu->guest_debug.singlestep;

@@ -969,13 +970,12 @@ static int set_guest_debug(struct kvm_vc
 	} else
 		vcpu->guest_debug.singlestep = 0;
 
-	if (old_singlestep && !vcpu->guest_debug.singlestep) {
-		unsigned long flags;
-
-		flags = vmcs_readl(GUEST_RFLAGS);
+	flags = vmcs_readl(GUEST_RFLAGS);
+	if (vcpu->guest_debug.singlestep)
+		flags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
+	else if (old_singlestep && !vcpu->guest_debug.singlestep)
 		flags &= ~(X86_EFLAGS_TF | X86_EFLAGS_RF);
-		vmcs_writel(GUEST_RFLAGS, flags);
-	}
+	vmcs_writel(GUEST_RFLAGS, flags);
 
 	update_exception_bitmap(vcpu);
 	vmcs_writel(GUEST_DR7, dr7);

@@ -2192,14 +2192,6 @@ static void kvm_guest_debug_pre(struct k
 	set_debugreg(dbg->bp[1], 1);
 	set_debugreg(dbg->bp[2], 2);
 	set_debugreg(dbg->bp[3], 3);
-
-	if (dbg->singlestep) {
-		unsigned long flags;
-
-		flags = vmcs_readl(GUEST_RFLAGS);
-		flags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
-		vmcs_writel(GUEST_RFLAGS, flags);
-	}
 }
 
 static int handle_rmode_exception(struct kvm_vcpu *vcpu,

@@ -2221,7 +2213,7 @@ static int handle_rmode_exception(struct
 static int handle_exception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	u32 intr_info, error_code;
+	u32 intr_info, ex_no, error_code;
 	unsigned long cr2, rip;
 	u32 vect_info;
 	enum emulation_result er;

@@ -2279,15 +2271,15 @@ static int handle_exception(struct kvm_v
 		return 1;
 	}
 
-	if ((intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VECTOR_MASK)) ==
-	    (INTR_TYPE_EXCEPTION | 1)) {
+	ex_no = intr_info & INTR_INFO_VECTOR_MASK;
+	if (ex_no == 1 || ex_no == 3) {
 		kvm_run->exit_reason = KVM_EXIT_DEBUG;
-		kvm_run->debug.arch.exception = 1;
-		return 0;
+		kvm_run->debug.arch.exception = ex_no;
+	} else {
+		kvm_run->exit_reason = KVM_EXIT_EXCEPTION;
+		kvm_run->ex.exception = ex_no;
+		kvm_run->ex.error_code = error_code;
 	}
-	kvm_run->exit_reason = KVM_EXIT_EXCEPTION;
-	kvm_run->ex.exception = intr_info & INTR_INFO_VECTOR_MASK;
-	kvm_run->ex.error_code = error_code;
 	return 0;
 }

Avi Kivity

2008-05-21 16:01 UTC #11 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

Quoted 7 lines · Jan Kiszka

In order to allow the gdbstub of QEMU to push (soft) breakpoint handling completely into the gdb frontend, this patch enables guest exits also for #BP exceptions - in case guest debugging was turned on.
Along this enhancement, this patch also fixes the flag manipulation for the singlestep mode.

Suppose userspace determines the exception is due to a guest breakpoint. How does it inject the debug exception?

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2008-05-22 13:34 UTC #12 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

Quoted 11 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka
In order to allow the gdbstub of QEMU to push (soft) breakpoint handling completely into the gdb frontend, this patch enables guest exits also for #BP exceptions - in case guest debugging was turned on.
Along this enhancement, this patch also fixes the flag manipulation for the singlestep mode.
Suppose userspace determines the exception is due to a guest breakpoint. How does it inject the debug exception?

Good question. Is there no "inject exception #XX" mechanism in kvm yet?

Will need this, as my current impression is that we better keep track of breakpoints at qemu level to tell guest soft-BPs apart from host injected ones. Would you suggest to add a separate IOCTL for exception injection then? Or should the new guest debug IOCTL contain a flag that signals "inject breakpoint trap" (both for guest soft-BP hits as well as guests already in single step mode)?

Jan

Avi Kivity

2008-05-22 13:58 UTC #13 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

Quoted 12 lines · Jan Kiszka

Quoted 10 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
In order to allow the gdbstub of QEMU to push (soft) breakpoint handling completely into the gdb frontend, this patch enables guest exits also for #BP exceptions - in case guest debugging was turned on.
Along this enhancement, this patch also fixes the flag manipulation for the singlestep mode.

Quoted 7 lines

Quoted 3 lines
Suppose userspace determines the exception is due to a guest breakpoint. How does it inject the debug exception?
Good question. Is there no "inject exception #XX" mechanism in kvm yet?

No userspace interface for it. The kernel injects plenty (kvm_queue_exception).

Quoted 6 lines

Will need this, as my current impression is that we better keep track of breakpoints at qemu level to tell guest soft-BPs apart from host injected ones. Would you suggest to add a separate IOCTL for exception injection then? Or should the new guest debug IOCTL contain a flag that signals "inject breakpoint trap" (both for guest soft-BP hits as well as guests already in single step mode)?

A debug specific thing may allow us to limit the generality of the implementation.

Or maybe, disable int 3 trapping, single step, reenable int 3 trapping -> no need to inject vectors.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2008-05-22 14:24 UTC #14 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

Quoted 31 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 5 lines · Jan Kiszka
In order to allow the gdbstub of QEMU to push (soft) breakpoint handling
completely into the gdb frontend, this patch enables guest exits also for #BP exceptions - in case guest debugging was turned on.
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Along this enhancement, this patch also fixes the flag manipulation for the singlestep mode.
Suppose userspace determines the exception is due to a guest breakpoint. How does it inject the debug exception?
Good question. Is there no "inject exception #XX" mechanism in kvm yet?
No userspace interface for it. The kernel injects plenty (kvm_queue_exception).
Quoted 6 lines
Will need this, as my current impression is that we better keep track of breakpoints at qemu level to tell guest soft-BPs apart from host injected ones. Would you suggest to add a separate IOCTL for exception injection then? Or should the new guest debug IOCTL contain a flag that signals "inject breakpoint trap" (both for guest soft-BP hits as well as guests already in single step mode)?

Quoted 5 lines

A debug specific thing may allow us to limit the generality of the implementation.
Or maybe, disable int 3 trapping, single step, reenable int 3 trapping -> no need to inject vectors.

/me is still trying to find explicit statements in the Intel docs about what happens to the TF flag when the CPU enters an interrupt or an exception handler. This influences how single stepping guests can be realized, specifically when trying to step into guest's int3 handling...

Jan

Avi Kivity

2008-05-22 14:31 UTC #15 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

Quoted 6 lines · Jan Kiszka

/me is still trying to find explicit statements in the Intel docs about what happens to the TF flag when the CPU enters an interrupt or an exception handler. This influences how single stepping guests can be realized, specifically when trying to step into guest's int3 handling...

It's cleared.

Signature · 1 line

error compiling committee.c: too many arguments to function

Hollis Blanchard

2008-05-22 14:28 UTC #16 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

In reply to Jan Kiszka

Quoted 13 lines · On Thursday 22 May 2008 08:31:46 Jan Kiszka wrote:

Quoted 11 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka
In order to allow the gdbstub of QEMU to push (soft) breakpoint handling completely into the gdb frontend, this patch enables guest exits also for #BP exceptions - in case guest debugging was turned on.
Along this enhancement, this patch also fixes the flag manipulation for the singlestep mode.
Suppose userspace determines the exception is due to a guest breakpoint. How does it inject the debug exception?

Quoted 8 lines

Good question. Is there no "inject exception #XX" mechanism in kvm yet?
Will need this, as my current impression is that we better keep track of breakpoints at qemu level to tell guest soft-BPs apart from host injected ones. Would you suggest to add a separate IOCTL for exception injection then? Or should the new guest debug IOCTL contain a flag that signals "inject breakpoint trap" (both for guest soft-BP hits as well as guests already in single step mode)?

In addition to injecting debug interrupts, qemu should also be able to inject machine checks, for the case of undecoded MMIO accesses.

As long as qemu can access the complete register state (e.g. including "reason for machine check" registers), I don't think the injection interface *needs* to be more complicated than "exception number".

OTOH, I can see the argument for atomic injection operations, so to support that you'd end up with a sub-structure like kvm_exit, something like:

struct kvm_inject_arch {
	u32 exception_type;
	union {
		struct machine_check {
			u32 mcsr;
		} mc;
		struct debug {
		} debug;
	};
}

This stuff is completely arch-specific, so making a common "inject" ioctl number that simply calls arch function (like GET_SREGS) would be appropriate.

Signature · 2 lines

Hollis Blanchard
IBM Linux Technology Center

Avi Kivity

2008-05-22 14:34 UTC #17 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

Quoted 17 lines · Hollis Blanchard

In addition to injecting debug interrupts, qemu should also be able to inject machine checks, for the case of undecoded MMIO accesses.
As long as qemu can access the complete register state (e.g. including "reason for machine check" registers), I don't think the injection interface *needs* to be more complicated than "exception number".
OTOH, I can see the argument for atomic injection operations, so to support that you'd end up with a sub-structure like kvm_exit, something like:
struct kvm_inject_arch {
	u32 exception_type;
	union {
		struct machine_check {
			u32 mcsr;
		} mc;
		struct debug {
		} debug;
	};
}

Quoted 3 lines

This stuff is completely arch-specific, so making a common "inject" ioctl number that simply calls arch function (like GET_SREGS) would be appropriate.

In the mmio case, I think it makes more sense to have a 'mmio failed' flag, and kvm can generate and inject the exception.

We mostly try to keep cpu emulation outside userspace.

(of course, that depends on what happens on real hardware. Is there a machine check pin? or does the cpu generate the exception internally?)

Signature · 1 line

error compiling committee.c: too many arguments to function

Hollis Blanchard

2008-05-22 18:27 UTC #18 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

Quoted 2 lines · On Thursday 22 May 2008 09:34:08 Avi Kivity wrote:

Quoted 1 line · Hollis Blanchard
In addition to injecting debug interrupts, qemu should also be able to

inject

Quoted 3 lines

Quoted 3 lines
machine checks, for the case of undecoded MMIO accesses.
As long as qemu can access the complete register state (e.g.

including "reason

Quoted 1 line

Quoted 1 line
for machine check" registers), I don't think the injection interface

*needs*

Quoted 1 line

Quoted 1 line
to be more complicated than "exception number".

[snip]

Quoted 3 lines

In the mmio case, I think it makes more sense to have a 'mmio failed' flag, and kvm can generate and inject the exception.

OK, that sounds reasonable.

Quoted 1 line

We mostly try to keep cpu emulation outside userspace.

Except for this debug stuff? :)

Quoted 2 lines

(of course, that depends on what happens on real hardware. Is there a machine check pin? or does the cpu generate the exception internally?)

There is a machine check pin, FWIW. It's the chipset (potentially several IO bridges away) that discovers no device decoded the MMIO address, and asynchronously notifies the core.

I'm happy with machine check as an "MMIO failed" flag, so I guess that leaves us with a separate "inject debug event" ioctl.

Signature · 2 lines

Hollis Blanchard
IBM Linux Technology Center

Avi Kivity

2008-05-25 10:24 UTC #19 lore raw

Re: [kvm-devel] [RFC][PATCH 3/4] kvm-vmx: KVM_EXIT_DEBUG on #BP exceptions

Quoted 7 lines · Hollis Blanchard

Quoted 3 lines
We mostly try to keep cpu emulation outside userspace.
Except for this debug stuff? :)

Except when it's on the wrong side of the complexity/benefit or performance/cleanliness tradeoffs.

Quoted 7 lines

Quoted 2 lines
(of course, that depends on what happens on real hardware. Is there a machine check pin? or does the cpu generate the exception internally?)
There is a machine check pin, FWIW. It's the chipset (potentially several IO bridges away) that discovers no device decoded the MMIO address, and asynchronously notifies the core.
I'm happy with machine check as an "MMIO failed" flag, so I guess that leaves us with a separate "inject debug event" ioctl.

In that case it's more reasonable to have a machine check ioctl. Are the other chipset-discovered causes of this? Perhaps on other ppc boards?

The x86 analog is triple faults. On real hardware, a triple fault asserts a processor pin and shuts down the processor. The motherboard detects this and causes a system reset. So we have a triple fault exit reason, and reset generation happens in userspace.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2008-05-16 16:02 UTC #20 lore raw

RFC PATCH 4/4 kvm-userspace: use soft-BPs for guest debugging

In reply to a message not in the archive

With this patch applied, kvm is able to ignore breakpoint requests of an attached gdb frontend so that the latter is motivated to insert soft breakpoints into the guest code. All we need to do for this is to catch and forward #BP exceptions which are now provided by the kernel module.

Along this, the patch makes vm_stop-on-breakpoint start to work.

Limitations:
- only takes care of x86 so far
- gdbstub state tracking is broken (artificially incrementing nb_breakpoints won't fly, as we'll have no chance to decrement it), we need an enhanced interface to the stub

---
 libkvm/kvm-common.h |    2 ++
 libkvm/libkvm-x86.c |   16 ++++++++++++++++
 libkvm/libkvm.c     |    5 -----
 libkvm/libkvm.h     |    8 +++++++-
 qemu/exec.c         |   19 +++++++++++++------
 qemu/qemu-kvm.c     |   22 +++++++++++++---------
 6 files changed, 51 insertions(+), 21 deletions(-)

libkvm/kvm-common.h +2 -0

Index: b/libkvm/kvm-common.h
===================================================================
@@ -85,4 +85,6 @@ int handle_io_window(kvm_context_t kvm);
 int handle_debug(kvm_context_t kvm, int vcpu);
 int try_push_interrupts(kvm_context_t kvm);
 
+int handle_debug(kvm_context_t kvm, int vcpu);
+
 #endif

libkvm/libkvm-x86.c +16 -0

Index: b/libkvm/libkvm-x86.c
===================================================================
@@ -661,3 +661,19 @@ int kvm_disable_tpr_access_reporting(kvm
 }
 
 #endif
+
+int handle_debug(kvm_context_t kvm, int vcpu)
+{
+	struct kvm_run *run = kvm->run[vcpu];
+	unsigned long watchpoint = 0;
+	int break_type;
+
+	if (run->debug.arch.exception == 1) {
+		/* TODO: fully process run->debug.arch */
+		break_type = KVM_GDB_BREAKPOINT_HW;
+	} else
+		break_type = KVM_GDB_BREAKPOINT_SW;
+
+	return kvm->callbacks->debug(kvm->opaque, vcpu, break_type,
+				     watchpoint);
+}

libkvm/libkvm.c +0 -5

Index: b/libkvm/libkvm.c
===================================================================
@@ -738,11 +738,6 @@ static int handle_io(kvm_context_t kvm, 
 	return 0;
 }
 
-int handle_debug(kvm_context_t kvm, int vcpu)
-{
-	return kvm->callbacks->debug(kvm->opaque, vcpu);
-}
-
 int kvm_get_regs(kvm_context_t kvm, int vcpu, struct kvm_regs *regs)
 {
     return ioctl(kvm->vcpu_fd[vcpu], KVM_GET_REGS, regs);

qemu/exec.c +13 -6

Index: b/qemu/exec.c
===================================================================
@@ -1157,6 +1157,12 @@ int cpu_breakpoint_insert(CPUState *env,
 #if defined(TARGET_HAS_ICE)
     int i;
 
+    if (kvm_enabled()) {
+        env->nb_breakpoints++;
+        kvm_update_debugger(env);
+        return -ENOSYS;
+    }
+
     for(i = 0; i < env->nb_breakpoints; i++) {
         if (env->breakpoints[i] == pc)
             return 0;

@@ -1166,9 +1172,6 @@ int cpu_breakpoint_insert(CPUState *env,
         return -ENOBUFS;
     env->breakpoints[env->nb_breakpoints++] = pc;
 
-    if (kvm_enabled())
-	kvm_update_debugger(env);
-
     breakpoint_invalidate(env, pc);
     return 0;
 #else

@@ -1182,6 +1185,13 @@ int cpu_breakpoint_remove(CPUState *env,
 {
 #if defined(TARGET_HAS_ICE)
     int i;
+
+    if (kvm_enabled()) {
+        env->nb_breakpoints--;
+        kvm_update_debugger(env);
+        return -ENOSYS;
+    }
+
     for(i = 0; i < env->nb_breakpoints; i++) {
         if (env->breakpoints[i] == pc)
             goto found;

@@ -1192,9 +1202,6 @@ int cpu_breakpoint_remove(CPUState *env,
     if (i < env->nb_breakpoints)
       env->breakpoints[i] = env->breakpoints[env->nb_breakpoints];
 
-    if (kvm_enabled())
-	kvm_update_debugger(env);
-    
     breakpoint_invalidate(env, pc);
     return 0;
 #else

qemu/qemu-kvm.c +13 -9

Index: b/qemu/qemu-kvm.c
===================================================================
@@ -58,6 +58,8 @@ pthread_t io_thread;
 static int io_thread_fd = -1;
 static int io_thread_sigfd = -1;
 
+static int kvm_debug_stop_requested;
+
 static inline unsigned long kvm_get_thread_id(void)
 {
     return syscall(SYS_gettid);

@@ -517,6 +519,10 @@ int kvm_main_loop(void)
             qemu_system_powerdown();
         else if (qemu_reset_requested())
 	    qemu_kvm_system_reset();
+	else if (kvm_debug_stop_requested) {
+	    kvm_debug_stop_requested = 0;
+	    vm_stop(EXCP_DEBUG);
+	}
     }
 
     pause_all_threads();

@@ -525,11 +531,12 @@ int kvm_main_loop(void)
     return 0;
 }
 
-static int kvm_debug(void *opaque, int vcpu)
+static int kvm_debug(void *opaque, int vcpu, int break_type,
+                     uint64_t watchpoint_addr)
 {
-    CPUState *env = cpu_single_env;
-
-    env->exception_index = EXCP_DEBUG;
+    /* TODO: process break_type */
+    kvm_debug_stop_requested = 1;
+    vcpu_info[vcpu].stopped = 1;
     return 1;
 }

@@ -748,15 +755,12 @@ int kvm_qemu_init_env(CPUState *cenv)
 int kvm_update_debugger(CPUState *env)
 {
     struct kvm_debug_guest dbg;
-    int i, r;
+    int r;
 
     dbg.enabled = 0;
     if (env->nb_breakpoints || env->singlestep_enabled) {
 	dbg.enabled = 1;
-	for (i = 0; i < 4 && i < env->nb_breakpoints; ++i) {
-	    dbg.breakpoints[i].enabled = 1;
-	    dbg.breakpoints[i].address = env->breakpoints[i];
-	}
+	memset(dbg.breakpoints, 0, sizeof(dbg.breakpoints));
 	dbg.singlestep = env->singlestep_enabled;
     }
     if (vm_running)

libkvm/libkvm.h +7 -1

Index: b/libkvm/libkvm.h
===================================================================
@@ -25,6 +25,12 @@ int kvm_get_msrs(kvm_context_t, int vcpu
 int kvm_set_msrs(kvm_context_t, int vcpu, struct kvm_msr_entry *msrs, int n);
 #endif
 
+#define KVM_GDB_BREAKPOINT_SW		0
+#define KVM_GDB_BREAKPOINT_HW		1
+#define KVM_GDB_WATCHPOINT_WRITE	2
+#define KVM_GDB_WATCHPOINT_READ		3
+#define KVM_GDB_WATCHPOINT_ACCESS	4
+
 /*!
  * \brief KVM callbacks structure
  *

@@ -51,7 +57,7 @@ struct kvm_callbacks {
 	/// generic memory writes to unmapped memory (For MMIO devices)
     int (*mmio_write)(void *opaque, uint64_t addr, uint8_t *data,
 					int len);
-    int (*debug)(void *opaque, int vcpu);
+    int (*debug)(void *opaque, int vcpu, int break_type, uint64_t watchpoint);
 	/*!
 	 * \brief Called when the VCPU issues an 'hlt' instruction.
 	 *