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

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

[Jan Kiszka]

[ 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(-)

Index: b/qemu/cpu-all.h
===================================================================
--- a/qemu/cpu-all.h
+++ 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);
 
Index: b/qemu/exec.c
===================================================================
--- a/qemu/exec.c
+++ 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
 }
 
Index: b/qemu/gdbstub.c
===================================================================
--- a/qemu/gdbstub.c
+++ b/qemu/gdbstub.c
@@ -882,7 +882,7 @@ static void cpu_gdb_write_registers(CPUS
 static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
 {
     const char *p;
-    int ch, reg_size, type;
+    int ch, reg_size, type, res;
     char buf[4096];
     uint8_t mem_buf[4096];
     uint32_t *registers;
@@ -1017,21 +1017,20 @@ static int gdb_handle_packet(GDBState *s
         if (*p == ',')
             p++;
         len = strtoull(p, (char **)&p, 16);
-        if (type == 0 || type == 1) {
-            if (cpu_breakpoint_insert(env, addr) < 0)
-                goto breakpoint_error;
-            put_packet(s, "OK");
+        switch (type) {
+        case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
+            res = cpu_breakpoint_insert(env, addr, len, type);
+            break;
 #ifndef CONFIG_USER_ONLY
-        } else if (type == 2) {
-            if (cpu_watchpoint_insert(env, addr) < 0)
-                goto breakpoint_error;
-            put_packet(s, "OK");
+        case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
+            res = cpu_watchpoint_insert(env, addr, len, type);
+            break;
 #endif
-        } else {
-        breakpoint_error:
-            put_packet(s, "E22");
+        default:
+            res = -ENOSYS;
+            break;
         }
-        break;
+        goto answer_bp_packet;
     case 'z':
         type = strtoul(p, (char **)&p, 16);
         if (*p == ',')
@@ -1040,17 +1039,26 @@ static int gdb_handle_packet(GDBState *s
         if (*p == ',')
             p++;
         len = strtoull(p, (char **)&p, 16);
-        if (type == 0 || type == 1) {
-            cpu_breakpoint_remove(env, addr);
-            put_packet(s, "OK");
+        switch (type) {
+        case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
+            res = cpu_breakpoint_remove(env, addr, len, type);
+            break;
 #ifndef CONFIG_USER_ONLY
-        } else if (type == 2) {
-            cpu_watchpoint_remove(env, addr);
-            put_packet(s, "OK");
+        case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
+            res = cpu_watchpoint_remove(env, addr, len, type);
+            break;
 #endif
-        } else {
-            goto breakpoint_error;
+        default:
+            res = -ENOSYS;
+            break;
         }
+    answer_bp_packet:
+        if (res >= 0)
+            put_packet(s, "OK");
+        else if (res == -ENOSYS)
+            put_packet(s, "");
+        else
+            put_packet(s, "E22");
         break;
 #ifdef CONFIG_LINUX_USER
     case 'q':


-------------------------------------------------------------------------
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/

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

[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).

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

Index: b/arch/x86/kvm/vmx.c
===================================================================
--- a/arch/x86/kvm/vmx.c
+++ 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;
Index: b/include/asm-x86/kvm.h
===================================================================
--- a/include/asm-x86/kvm.h
+++ 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
Index: b/include/linux/kvm.h
===================================================================
--- a/include/linux/kvm.h
+++ 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 {


-------------------------------------------------------------------------
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/

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

[Avi Kivity]

Jan Kiszka wrote:
> 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
> ===================================================================
> --- a/include/asm-x86/kvm.h
> +++ 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];
> +};
> +
>   

Need empty structures for non-x86.

Need a KVM_CAP_ to indicate presence of this feature.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>> 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
>> ===================================================================
>> --- a/include/asm-x86/kvm.h
>> +++ 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];
>> +};
>> +
>>   
> 
> Need empty structures for non-x86.
> 
> 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


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

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

[Avi Kivity]

Jan Kiszka wrote:
> 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).
>  
> +struct kvm_debug_exit_arch {
> +	__u32 exception;
>   

Need padding here.

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

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>> 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).
>>  
>> +struct kvm_debug_exit_arch {
>> +    __u32 exception;
>>   
> 
> Need padding here.
> 
>> +    __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.)

> 
> 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


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

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

[Avi Kivity]

Jan Kiszka wrote:
>
>> 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.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>>
>>> 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.

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


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

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

[Avi Kivity]

Jan Kiszka wrote:
> 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.



-- 
error compiling committee.c: too many arguments to function

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

[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.

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

Index: b/arch/x86/kvm/vmx.c
===================================================================
--- a/arch/x86/kvm/vmx.c
+++ 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;
 }
 

-------------------------------------------------------------------------
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/

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

[Avi Kivity]

Jan Kiszka wrote:
> 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?

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>> 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



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

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

[Avi Kivity]

Jan Kiszka wrote:
> Avi Kivity wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> 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?
>
>   

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

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

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Avi Kivity wrote:
>>  
>>> Jan Kiszka wrote:
>>>    
>>>> 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?
>>
>>   
> 
> No userspace interface for it.  The kernel injects plenty
> (kvm_queue_exception).
> 
>> 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.

/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


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

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

[Avi Kivity]

Jan Kiszka wrote:
> /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.

-- 
error compiling committee.c: too many arguments to function

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

[Hollis Blanchard]

On Thursday 22 May 2008 08:31:46 Jan Kiszka wrote:
> Avi Kivity wrote:
> > Jan Kiszka wrote:
> >> 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)?

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.

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Avi Kivity]

Hollis Blanchard wrote:
> 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.
>
>   

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?)

-- 
error compiling committee.c: too many arguments to function

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

[Hollis Blanchard]

On Thursday 22 May 2008 09:34:08 Avi Kivity wrote:
> Hollis Blanchard wrote:
> > 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".
[snip]
> 
> 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.

> We mostly try to keep cpu emulation outside userspace.

Except for this debug stuff? :)

> (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.

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Avi Kivity]

Hollis Blanchard wrote:
>
>> 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.

>> (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.

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

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(-)

Index: b/libkvm/kvm-common.h
===================================================================
--- a/libkvm/kvm-common.h
+++ 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
Index: b/libkvm/libkvm-x86.c
===================================================================
--- a/libkvm/libkvm-x86.c
+++ 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);
+}
Index: b/libkvm/libkvm.c
===================================================================
--- a/libkvm/libkvm.c
+++ 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);
Index: b/qemu/exec.c
===================================================================
--- a/qemu/exec.c
+++ 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
Index: b/qemu/qemu-kvm.c
===================================================================
--- a/qemu/qemu-kvm.c
+++ 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)
Index: b/libkvm/libkvm.h
===================================================================
--- a/libkvm/libkvm.h
+++ 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.
 	 *

-------------------------------------------------------------------------
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/