[PATCH RESEND 0/4] kvmppc/booke: add guest debug support

[PATCH RESEND 0/4] kvmppc/booke: add guest debug support

[Liu Yu]

This patchset add guest debug support for booke.

[PATCH 1/4] kvmppc: guest debug definitions

[Liu Yu]

Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/powerpc/include/asm/kvm.h      |   20 ++++++++++++++++++++
 arch/powerpc/include/asm/kvm_host.h |   16 ++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
index 81f3b0b..b7f7861 100644
--- a/arch/powerpc/include/asm/kvm.h
+++ b/arch/powerpc/include/asm/kvm.h
@@ -22,6 +22,9 @@
 
 #include <linux/types.h>
 
+/* Select powerpc specific features in <linux/kvm.h> */
+#define __KVM_HAVE_GUEST_DEBUG
+
 struct kvm_regs {
 	__u64 pc;
 	__u64 cr;
@@ -71,10 +74,27 @@ struct kvm_fpu {
 };
 
 struct kvm_debug_exit_arch {
+	__u32 exception;
+	__u32 pc;
+	__u32 status;
 };
 
+#define KVM_INST_GUESTGDB               0x44000022
+
+#define KVM_GUESTDBG_USE_SW_BP          0x00010000
+#define KVM_GUESTDBG_USE_HW_BP          0x00020000
+
+#define KVMPPC_DEBUG_NOTYPE             0x0
+#define KVMPPC_DEBUG_BREAKPOINT         (1UL << 1)
+#define KVMPPC_DEBUG_WATCH_WRITE        (1UL << 2)
+#define KVMPPC_DEBUG_WATCH_READ         (1UL << 3)
+
 /* for KVM_SET_GUEST_DEBUG */
 struct kvm_guest_debug_arch {
+	struct {
+		__u32 addr;
+		__u32 type;
+	} bp[6];
 };
 
 #endif /* __LINUX_KVM_POWERPC_H */
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 5e5bae7..a364832 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -157,6 +157,18 @@ struct hpte_cache {
 	struct kvmppc_pte pte;
 };
 
+struct kvmppc_debug_reg {
+	u32 dbcr0;
+	u32 iac[0];
+	u32 iac1;
+	u32 iac2;
+	u32 iac3;
+	u32 iac4;
+	u32 dac[0];
+	u32 dac1;
+	u32 dac2;
+};
+
 struct kvm_vcpu_arch {
 	ulong host_stack;
 	u32 host_pid;
@@ -240,6 +252,9 @@ struct kvm_vcpu_arch {
 	u32 dbcr1;
 	u32 dbsr;
 
+	struct kvmppc_debug_reg shadow_dbg_reg;
+	struct kvmppc_debug_reg host_dbg_reg;
+
 #ifdef CONFIG_KVM_EXIT_TIMING
 	struct kvmppc_exit_timing timing_exit;
 	struct kvmppc_exit_timing timing_last_enter;
@@ -274,6 +289,7 @@ struct kvm_vcpu_arch {
 	struct tasklet_struct tasklet;
 	u64 dec_jiffies;
 	unsigned long pending_exceptions;
+	struct kvm_guest_debug_arch dbg;
 
 #ifdef CONFIG_PPC64
 	struct hpte_cache hpte_cache[HPTEG_CACHE_NUM];
-- 
1.6.4

[PATCH 2/4] kvmppc/booke: switch shadow/host debug registers on guest enter/exit path

[Liu Yu]

This provide a precise way to avoid confounding settings of guest and host.
Also the guest hardware emulation about debug can be implemented based on this.

Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/powerpc/kernel/asm-offsets.c   |    3 ++
 arch/powerpc/kvm/booke_interrupts.S |   58 +++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 957ceb7..67e978d 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -425,6 +425,9 @@ int main(void)
 	DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst));
 	DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear));
 	DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr));
+	DEFINE(VCPU_SHADOW_DBG, offsetof(struct kvm_vcpu, arch.shadow_dbg_reg));
+	DEFINE(VCPU_HOST_DBG, offsetof(struct kvm_vcpu, arch.host_dbg_reg));
+	DEFINE(VCPU_GUEST_DEBUG, offsetof(struct kvm_vcpu, guest_debug));
 
 	/* book3s_64 */
 #ifdef CONFIG_PPC64
diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
index 380a78c..644ff1d 100644
--- a/arch/powerpc/kvm/booke_interrupts.S
+++ b/arch/powerpc/kvm/booke_interrupts.S
@@ -168,6 +168,26 @@ _GLOBAL(kvmppc_resume_host)
 	stw	r9, VCPU_FAULT_ESR(r4)
 ..skip_esr:
 
+	lwz	r6, VCPU_GUEST_DEBUG(r4)
+	or.	r6, r6, r6
+	beq	..skip_load_host_debug
+	addi	r7, r4, VCPU_HOST_DBG - 4
+	lwzu	r9, 4(r7)
+	mtspr	SPRN_DBCR0, r9
+	lwzu	r9, 4(r7)
+	mtspr	SPRN_IAC1, r9
+	lwzu	r9, 4(r7)
+	mtspr	SPRN_IAC2, r9
+	lwzu	r9, 4(r7)
+	mtspr	SPRN_IAC3, r9
+	lwzu	r9, 4(r7)
+	mtspr	SPRN_IAC4, r9
+	lwzu	r9, 4(r7)
+	mtspr	SPRN_DAC1, r9
+	lwzu	r9, 4(r7)
+	mtspr	SPRN_DAC2, r9
+..skip_load_host_debug:
+
 	/* Save remaining volatile guest register state to vcpu. */
 	stw	r0, VCPU_GPR(r0)(r4)
 	stw	r1, VCPU_GPR(r1)(r4)
@@ -392,6 +412,44 @@ lightweight_exit:
 	lwz	r3, VCPU_SPRG7(r4)
 	mtspr	SPRN_SPRG7W, r3
 
+	lwz	r6, VCPU_GUEST_DEBUG(r4)
+	or.	r6, r6, r6
+	beq	..skip_load_guest_debug
+	mfmsr	r7
+	rlwinm	r7, r7, 0, ~MSR_DE
+	mtmsr	r7
+	addi	r7, r4, VCPU_HOST_DBG - 4
+	mfspr	r8, SPRN_DBCR0
+	stwu	r8, 4(r7)
+	mfspr	r8, SPRN_IAC1
+	stwu	r8, 4(r7)
+	mfspr	r8, SPRN_IAC2
+	stwu	r8, 4(r7)
+	mfspr	r8, SPRN_IAC3
+	stwu	r8, 4(r7)
+	mfspr	r8, SPRN_IAC4
+	stwu	r8, 4(r7)
+	mfspr	r8, SPRN_DAC1
+	stwu	r8, 4(r7)
+	mfspr	r8, SPRN_DAC2
+	stwu	r8, 4(r7)
+	addi	r7, r4, VCPU_SHADOW_DBG - 4
+	lwzu	r8, 4(r7)
+	mtspr	SPRN_DBCR0, r8
+	lwzu	r8, 4(r7)
+	mtspr	SPRN_IAC1, r8
+	lwzu	r8, 4(r7)
+	mtspr	SPRN_IAC2, r8
+	lwzu	r8, 4(r7)
+	mtspr	SPRN_IAC3, r8
+	lwzu	r8, 4(r7)
+	mtspr	SPRN_IAC4, r8
+	lwzu	r8, 4(r7)
+	mtspr	SPRN_DAC1, r8
+	lwzu	r8, 4(r7)
+	mtspr	SPRN_DAC2, r8
+..skip_load_guest_debug:
+
 #ifdef CONFIG_KVM_EXIT_TIMING
 	/* save enter time */
 1:
-- 
1.6.4

[PATCH 3/4] kvmppc/booke: guest debug support

[Liu Yu]

According to user's gdb command,
we set the corresponding debug control bits in shadow.

Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/powerpc/include/asm/kvm_ppc.h |    3 +
 arch/powerpc/kvm/booke.c           |   93 ++++++++++++++++++++++++++++++++++--
 arch/powerpc/kvm/e500.c            |    8 ---
 arch/powerpc/kvm/powerpc.c         |    2 +-
 4 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index e264282..8918aac 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -94,6 +94,9 @@ extern int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
 extern int kvmppc_core_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, int rs);
 extern int kvmppc_core_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, int rt);
 
+extern int kvmppc_core_set_guest_debug(struct kvm_vcpu *vcpu,
+                                       struct kvm_guest_debug *dbg);
+
 extern int kvmppc_booke_init(void);
 extern void kvmppc_booke_exit(void);
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 4d686cc..ec2722d 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -267,6 +267,16 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 			break;
 		}
 
+		if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_ENABLE) &&
+		             (vcpu->arch.last_inst == KVM_INST_GUESTGDB)) {
+			run->exit_reason = KVM_EXIT_DEBUG;
+			run->debug.arch.pc = vcpu->arch.pc;
+			run->debug.arch.exception = exit_nr;
+			kvmppc_account_exit(vcpu, DEBUG_EXITS);
+			r = RESUME_HOST;
+			break;
+		}
+
 		er = kvmppc_emulate_instruction(run, vcpu);
 		switch (er) {
 		case EMULATE_DONE:
@@ -293,6 +303,12 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		default:
 			BUG();
 		}
+
+		if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_ENABLE) &&
+		    (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
+			run->exit_reason = KVM_EXIT_DEBUG;
+			r = RESUME_HOST;
+		}
 		break;
 
 	case BOOKE_INTERRUPT_FP_UNAVAIL:
@@ -421,12 +437,27 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 		u32 dbsr;
 
 		vcpu->arch.pc = mfspr(SPRN_CSRR0);
-
-		/* clear IAC events in DBSR register */
 		dbsr = mfspr(SPRN_DBSR);
-		dbsr &= DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4;
-		mtspr(SPRN_DBSR, dbsr);
+		run->debug.arch.pc = vcpu->arch.pc;
+		run->debug.arch.status = 0;
+
+		if (dbsr & (DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4)) {
+			run->debug.arch.status |= KVMPPC_DEBUG_BREAKPOINT;
+		} else {
+			if (dbsr & (DBSR_DAC1W | DBSR_DAC2W))
+				run->debug.arch.status |= KVMPPC_DEBUG_WATCH_WRITE;
+			else if (dbsr & (DBSR_DAC1R | DBSR_DAC2R))
+				run->debug.arch.status |= KVMPPC_DEBUG_WATCH_READ;
+			if (dbsr & (DBSR_DAC1R | DBSR_DAC1W))
+				run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac1;
+			else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
+				run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac2;
+		}
 
+		/* clear events in DBSR register */
+		mtspr(SPRN_DBSR, ~0);
+
+		run->debug.arch.exception = exit_nr;
 		run->exit_reason = KVM_EXIT_DEBUG;
 		kvmppc_account_exit(vcpu, DEBUG_EXITS);
 		r = RESUME_HOST;
@@ -560,6 +591,60 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
 	return -ENOTSUPP;
 }
 
+int kvmppc_core_set_guest_debug(struct kvm_vcpu *vcpu,
+                                struct kvm_guest_debug *dbg)
+{
+	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
+		vcpu->guest_debug = 0;
+		return 0;
+	}
+
+	vcpu->guest_debug = dbg->control;
+	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
+		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
+		struct kvmppc_debug_reg *gdbgr = &(vcpu->arch.shadow_dbg_reg);
+		int n, b = 0, w = 0;
+		const u32 bp_code[] = {
+			DBCR0_IAC1 | DBCR0_IDM,
+			DBCR0_IAC2 | DBCR0_IDM,
+			DBCR0_IAC3 | DBCR0_IDM,
+			DBCR0_IAC4 | DBCR0_IDM
+		};
+		const u32 wp_code[] = {
+			DBCR0_DAC1W | DBCR0_IDM,
+			DBCR0_DAC2W | DBCR0_IDM,
+			DBCR0_DAC1R | DBCR0_IDM,
+			DBCR0_DAC2R | DBCR0_IDM
+		};
+
+		for (n = 0; n < 6 && dbg->arch.bp[n].type; n++) {
+			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_BREAKPOINT)
+				gdbgr->dbcr0 |= bp_code[b];
+			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_WATCH_READ)
+				gdbgr->dbcr0 |= wp_code[w + 2];
+			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_WATCH_WRITE)
+				gdbgr->dbcr0 |= wp_code[w];
+
+			if (b < 4 && (gdbgr->dbcr0 & (DBCR0_IAC1 | DBCR0_IAC2 |
+			              DBCR0_IAC3 | DBCR0_IAC4))) {
+				gdbgr->iac[b] = dbg->arch.bp[n].addr;
+				b++;
+			}
+			if (w < 2 && (gdbgr->dbcr0 & (DBCR0_DAC1W | DBCR0_DAC1R
+			              | DBCR0_DAC2W | DBCR0_DAC2R))) {
+				gdbgr->dac[w] = dbg->arch.bp[n].addr;
+				w++;
+			}
+		}
+	}
+
+	return 0;
+}
+
 int __init kvmppc_booke_init(void)
 {
 	unsigned long ivor[16];
diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
index efa1198..65192eb 100644
--- a/arch/powerpc/kvm/e500.c
+++ b/arch/powerpc/kvm/e500.c
@@ -24,14 +24,6 @@
 #include "booke.h"
 #include "e500_tlb.h"
 
-void kvmppc_core_load_host_debugstate(struct kvm_vcpu *vcpu)
-{
-}
-
-void kvmppc_core_load_guest_debugstate(struct kvm_vcpu *vcpu)
-{
-}
-
 void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 	kvmppc_e500_tlb_load(vcpu, cpu);
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 51aedd7..5bb17e5 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -265,7 +265,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
                                         struct kvm_guest_debug *dbg)
 {
-	return -EINVAL;
+	return kvmppc_core_set_guest_debug(vcpu, dbg);
 }
 
 static void kvmppc_complete_dcr_load(struct kvm_vcpu *vcpu,
-- 
1.6.4

[PATCH 4/4] kvmppc/booke: exit_nr fixup for guest debug single step

[Liu Yu]

As BOOKE doesn't have hardware support for virtualization,
hardware never know who's guest and host.

When enable hardware single step in guest,
we cannot disabled it at the point we switch back to host.
Thus, we'll see that an single step interrupt happens at
the beginning of guest exit path.

Then we need to recognize this kind of single step interrupt
and fix the exit_nr to the original value.
So that everything looks like normal.

Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/powerpc/kvm/booke.c            |   82 +++++++++++++++++++++++++++++++++++
 arch/powerpc/kvm/booke_interrupts.S |    9 ++--
 2 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index ec2722d..9056708 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
+#include <linux/highmem.h>
 
 #include <asm/cputable.h>
 #include <asm/uaccess.h>
@@ -34,6 +35,8 @@
 #include "booke.h"
 
 unsigned long kvmppc_booke_handlers;
+unsigned long kvmppc_booke_handler_addr[16];
+#define handler_vector_num (sizeof(kvmppc_booke_handler_addr)/sizeof(kvmppc_booke_handler_addr[0]))
 
 #define VM_STAT(x) offsetof(struct kvm, stat.x), KVM_STAT_VM
 #define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
@@ -214,6 +217,80 @@ void kvmppc_core_deliver_interrupts(struct kvm_vcpu *vcpu)
 	}
 }
 
+int kvmppc_read_guest(struct kvm_vcpu *vcpu, unsigned long geaddr,
+		      void *data, int len)
+{
+	int gtlb_index;
+	gpa_t gpa;
+	gfn_t gfn;
+	struct page *page;
+	void *headdr, *from;
+
+	/* Check the guest TLB. */
+	gtlb_index = kvmppc_mmu_itlb_index(vcpu, geaddr);
+	if (gtlb_index < 0)
+		return -EFAULT;
+
+	gpa = kvmppc_mmu_xlate(vcpu, gtlb_index, geaddr);
+	gfn = gpa >> PAGE_SHIFT;
+
+	page = gfn_to_page(vcpu->kvm, gfn);
+	if (page == bad_page)
+		return -EFAULT;
+
+	headdr = kmap_atomic(page, KM_USER0);
+	if (!headdr)
+		return -EFAULT;
+	from = headdr + (geaddr & (PAGE_SIZE - 1));
+	memcpy(data, from, len);
+	kunmap_atomic(headdr, KM_USER0);
+
+	return 0;
+}
+
+static unsigned int kvmppc_guest_debug_exitnr_fixup(struct kvm_vcpu *vcpu,
+                                                  unsigned int exit_nr)
+{
+	unsigned int ret = exit_nr;
+
+	u32 csrr0 = mfspr(SPRN_CSRR0);
+	u32 dbsr = mfspr(SPRN_DBSR);
+
+	if ((dbsr | DBSR_IC) &&
+	    csrr0 >= kvmppc_booke_handlers &&
+	    csrr0 < kvmppc_booke_handlers + (PAGE_SIZE << VCPU_SIZE_ORDER)) {
+		int i = 0;
+
+		for (i = 0; i < handler_vector_num; i++) {
+			if (kvmppc_booke_handler_addr[i] &&
+			    csrr0 == kvmppc_booke_handler_addr[i] + 4) {
+				mtspr(SPRN_DBSR, ~0);
+				ret = i;
+				break;
+			}
+		}
+
+	}
+
+	switch (ret) {
+	case BOOKE_INTERRUPT_DEBUG:
+	case BOOKE_INTERRUPT_ITLB_MISS:
+	case BOOKE_INTERRUPT_EXTERNAL:
+	case BOOKE_INTERRUPT_DECREMENTER:
+		break;
+
+	case BOOKE_INTERRUPT_PROGRAM:
+	case BOOKE_INTERRUPT_DTLB_MISS:
+		/* Need to save the last instruction */
+		kvmppc_read_guest(vcpu, vcpu->arch.pc, &vcpu->arch.last_inst, 4);
+		break;
+	default:
+		printk("Unhandled debug after interrupt:%d\n", ret);
+	}
+
+	return ret;
+}
+
 /**
  * kvmppc_handle_exit
  *
@@ -233,6 +310,9 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	run->exit_reason = KVM_EXIT_UNKNOWN;
 	run->ready_for_interrupt_injection = 1;
 
+	if (unlikely(exit_nr == BOOKE_INTERRUPT_DEBUG))
+		exit_nr = kvmppc_guest_debug_exitnr_fixup(vcpu, exit_nr);
+
 	switch (exit_nr) {
 	case BOOKE_INTERRUPT_MACHINE_CHECK:
 		printk("MACHINE CHECK: %lx\n", mfspr(SPRN_MCSR));
@@ -686,6 +766,8 @@ int __init kvmppc_booke_init(void)
 		memcpy((void *)kvmppc_booke_handlers + ivor[i],
 		       kvmppc_handlers_start + i * kvmppc_handler_len,
 		       kvmppc_handler_len);
+		kvmppc_booke_handler_addr[i] =
+			(unsigned long)kvmppc_booke_handlers + ivor[i];
 	}
 	flush_icache_range(kvmppc_booke_handlers,
 	                   kvmppc_booke_handlers + max_ivor + kvmppc_handler_len);
diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
index 644ff1d..fdc48c1 100644
--- a/arch/powerpc/kvm/booke_interrupts.S
+++ b/arch/powerpc/kvm/booke_interrupts.S
@@ -42,16 +42,17 @@
 #define HOST_STACK_LR   (HOST_STACK_SIZE + 4) /* In caller stack frame. */
 
 #define NEED_INST_MASK ((1<<BOOKE_INTERRUPT_PROGRAM) | \
-                        (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
-                        (1<<BOOKE_INTERRUPT_DEBUG))
+                        (1<<BOOKE_INTERRUPT_DTLB_MISS))
 
 #define NEED_DEAR_MASK ((1<<BOOKE_INTERRUPT_DATA_STORAGE) | \
-                        (1<<BOOKE_INTERRUPT_DTLB_MISS))
+                        (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
+                        (1<<BOOKE_INTERRUPT_DEBUG))
 
 #define NEED_ESR_MASK ((1<<BOOKE_INTERRUPT_DATA_STORAGE) | \
                        (1<<BOOKE_INTERRUPT_INST_STORAGE) | \
                        (1<<BOOKE_INTERRUPT_PROGRAM) | \
-                       (1<<BOOKE_INTERRUPT_DTLB_MISS))
+                       (1<<BOOKE_INTERRUPT_DTLB_MISS) | \
+                        (1<<BOOKE_INTERRUPT_DEBUG))
 
 .macro KVM_HANDLER ivor_nr
 _GLOBAL(kvmppc_handler_\ivor_nr)
-- 
1.6.4

Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for guest debug single step

[Alexander Graf]

Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>:

> As BOOKE doesn't have hardware support for virtualization,
> hardware never know who's guest and host.
>
> When enable hardware single step in guest,
> we cannot disabled it at the point we switch back to host.

Why not? We directly arrive in our code. So we can just disable it, no?

Or does that break when you'd try to debug the guest interrupt handlers?

> Thus, we'll see that an single step interrupt happens at
> the beginning of guest exit path.
>
> Then we need to recognize this kind of single step interrupt
> and fix the exit_nr to the original value.
> So that everything looks like normal.
>
> Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
> arch/powerpc/kvm/booke.c            |   82 ++++++++++++++++++++++++++ 
> +++++++++
> arch/powerpc/kvm/booke_interrupts.S |    9 ++--
> 2 files changed, 87 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index ec2722d..9056708 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -24,6 +24,7 @@
> #include <linux/module.h>
> #include <linux/vmalloc.h>
> #include <linux/fs.h>
> +#include <linux/highmem.h>
>
> #include <asm/cputable.h>
> #include <asm/uaccess.h>
> @@ -34,6 +35,8 @@
> #include "booke.h"
>
> unsigned long kvmppc_booke_handlers;
> +unsigned long kvmppc_booke_handler_addr[16];
> +#define handler_vector_num (sizeof(kvmppc_booke_handler_addr)/sizeof 
> (kvmppc_booke_handler_addr[0]))
>
> #define VM_STAT(x) offsetof(struct kvm, stat.x), KVM_STAT_VM
> #define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
> @@ -214,6 +217,80 @@ void kvmppc_core_deliver_interrupts(struct  
> kvm_vcpu *vcpu)
>    }
> }
>
> +int kvmppc_read_guest(struct kvm_vcpu *vcpu, unsigned long geaddr,
> +              void *data, int len)

Ah, nice. I have something similar in book3s.c. IIRC it's called  
kvmppc_ld.

I think we should make the semantics identical and declare it as  
common kvmppc_core function.

Alex

RE: [PATCH 4/4] kvmppc/booke: exit_nr fixup for guest debug single step

[Liu Yu-B13201]

 

> -----Original Message-----
> From: kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
> [mailto:kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Alexander Graf
> Sent: Wednesday, February 03, 2010 5:03 PM
> To: Liu Yu-B13201
> Cc: hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org; kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Liu Yu-B13201
> Subject: Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for 
> guest debug single step
> 
> 
> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>:
> 
> > As BOOKE doesn't have hardware support for virtualization,
> > hardware never know who's guest and host.
> >
> > When enable hardware single step in guest,
> > we cannot disabled it at the point we switch back to host.
> 
> Why not? We directly arrive in our code. So we can just 
> disable it, no?
> 
> Or does that break when you'd try to debug the guest 
> interrupt handlers?

That's the hardware limitition.
Assume received itlb miss interrupt, but it doesn't clear MSR_DE in MSR,
so on the exit path single step still work and then debug interrupt is
triggled.


> 
> > Thus, we'll see that an single step interrupt happens at
> > the beginning of guest exit path.
> >
> > Then we need to recognize this kind of single step interrupt
> > and fix the exit_nr to the original value.
> > So that everything looks like normal.
> >
> > Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> > arch/powerpc/kvm/booke.c            |   82 
> ++++++++++++++++++++++++++ 
> > +++++++++
> > arch/powerpc/kvm/booke_interrupts.S |    9 ++--
> > 2 files changed, 87 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index ec2722d..9056708 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -24,6 +24,7 @@
> > #include <linux/module.h>
> > #include <linux/vmalloc.h>
> > #include <linux/fs.h>
> > +#include <linux/highmem.h>
> >
> > #include <asm/cputable.h>
> > #include <asm/uaccess.h>
> > @@ -34,6 +35,8 @@
> > #include "booke.h"
> >
> > unsigned long kvmppc_booke_handlers;
> > +unsigned long kvmppc_booke_handler_addr[16];
> > +#define handler_vector_num 
> (sizeof(kvmppc_booke_handler_addr)/sizeof 
> > (kvmppc_booke_handler_addr[0]))
> >
> > #define VM_STAT(x) offsetof(struct kvm, stat.x), KVM_STAT_VM
> > #define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), 
> KVM_STAT_VCPU
> > @@ -214,6 +217,80 @@ void kvmppc_core_deliver_interrupts(struct  
> > kvm_vcpu *vcpu)
> >    }
> > }
> >
> > +int kvmppc_read_guest(struct kvm_vcpu *vcpu, unsigned long geaddr,
> > +              void *data, int len)
> 
> Ah, nice. I have something similar in book3s.c. IIRC it's called  
> kvmppc_ld.
> 
> I think we should make the semantics identical and declare it as  
> common kvmppc_core function.
> 
Cool.

Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for guest debug single step

[Alexander Graf]

Liu Yu-B13201 wrote:
>  
>
>   
>> -----Original Message-----
>> From: kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
>> [mailto:kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Alexander Graf
>> Sent: Wednesday, February 03, 2010 5:03 PM
>> To: Liu Yu-B13201
>> Cc: hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org; kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Liu Yu-B13201
>> Subject: Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for 
>> guest debug single step
>>
>>
>> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>:
>>
>>     
>>> As BOOKE doesn't have hardware support for virtualization,
>>> hardware never know who's guest and host.
>>>
>>> When enable hardware single step in guest,
>>> we cannot disabled it at the point we switch back to host.
>>>       
>> Why not? We directly arrive in our code. So we can just 
>> disable it, no?
>>
>> Or does that break when you'd try to debug the guest 
>> interrupt handlers?
>>     
>
> That's the hardware limitition.
> Assume received itlb miss interrupt, but it doesn't clear MSR_DE in MSR,
> so on the exit path single step still work and then debug interrupt is
> triggled.
>   

MSRDE is set to 0 by critical class interrupts
unless Category E.ED is supported, by Debug                   
interrupts, and by Machine Check interrupts,                  
and is left unchanged by all other interrupts.                

Great.

So when single stepping is enabled, you jump into the guest, get an itlb
miss, get out, still have DE set, get in KVM's own DE handler and can
process things from there.

Could you check if the debug instruction was on PR=0? If so, you can
just rfi and be good, right?


Alex

RE: [PATCH 4/4] kvmppc/booke: exit_nr fixup for guest debug single step

[Liu Yu-B13201]

 

> -----Original Message-----
> From: Alexander Graf [mailto:agraf-l3A5Bk7waGM@public.gmane.org] 
> Sent: Wednesday, February 03, 2010 6:14 PM
> To: Liu Yu-B13201
> Cc: hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org; kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for 
> guest debug single step
> 
> Liu Yu-B13201 wrote:
> >  
> >
> >   
> >> -----Original Message-----
> >> From: kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
> >> [mailto:kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Alexander Graf
> >> Sent: Wednesday, February 03, 2010 5:03 PM
> >> To: Liu Yu-B13201
> >> Cc: hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org; kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
> >> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Liu Yu-B13201
> >> Subject: Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for 
> >> guest debug single step
> >>
> >>
> >> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>:
> >>
> >>     
> >>> As BOOKE doesn't have hardware support for virtualization,
> >>> hardware never know who's guest and host.
> >>>
> >>> When enable hardware single step in guest,
> >>> we cannot disabled it at the point we switch back to host.
> >>>       
> >> Why not? We directly arrive in our code. So we can just 
> >> disable it, no?
> >>
> >> Or does that break when you'd try to debug the guest 
> >> interrupt handlers?
> >>     
> >
> > That's the hardware limitition.
> > Assume received itlb miss interrupt, but it doesn't clear 
> MSR_DE in MSR,
> > so on the exit path single step still work and then debug 
> interrupt is
> > triggled.
> >   
> 
> MSRDE is set to 0 by critical class interrupts
> unless Category E.ED is supported, by Debug                   
> interrupts, and by Machine Check interrupts,                  
> and is left unchanged by all other interrupts.                
> 
> Great.
> 
> So when single stepping is enabled, you jump into the guest, 
> get an itlb
> miss, get out, still have DE set, get in KVM's own DE handler and can
> process things from there.
> 
> Could you check if the debug instruction was on PR=0? If so, you can
> just rfi and be good, right?
> 

Hr?
The moment we found this happen we've already saved the guest and loaded host on exit path
Rfi will make exit path again which means save guest again.

Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for guest debug single step

[Alexander Graf]

Liu Yu-B13201 wrote:
>  
>
>   
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf-l3A5Bk7waGM@public.gmane.org] 
>> Sent: Wednesday, February 03, 2010 6:14 PM
>> To: Liu Yu-B13201
>> Cc: hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org; kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for 
>> guest debug single step
>>
>> Liu Yu-B13201 wrote:
>>     
>>>  
>>>
>>>   
>>>       
>>>> -----Original Message-----
>>>> From: kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
>>>> [mailto:kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Alexander Graf
>>>> Sent: Wednesday, February 03, 2010 5:03 PM
>>>> To: Liu Yu-B13201
>>>> Cc: hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org; kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>>>> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Liu Yu-B13201
>>>> Subject: Re: [PATCH 4/4] kvmppc/booke: exit_nr fixup for 
>>>> guest debug single step
>>>>
>>>>
>>>> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>:
>>>>
>>>>     
>>>>         
>>>>> As BOOKE doesn't have hardware support for virtualization,
>>>>> hardware never know who's guest and host.
>>>>>
>>>>> When enable hardware single step in guest,
>>>>> we cannot disabled it at the point we switch back to host.
>>>>>       
>>>>>           
>>>> Why not? We directly arrive in our code. So we can just 
>>>> disable it, no?
>>>>
>>>> Or does that break when you'd try to debug the guest 
>>>> interrupt handlers?
>>>>     
>>>>         
>>> That's the hardware limitition.
>>> Assume received itlb miss interrupt, but it doesn't clear 
>>>       
>> MSR_DE in MSR,
>>     
>>> so on the exit path single step still work and then debug 
>>>       
>> interrupt is
>>     
>>> triggled.
>>>   
>>>       
>> MSRDE is set to 0 by critical class interrupts
>> unless Category E.ED is supported, by Debug                   
>> interrupts, and by Machine Check interrupts,                  
>> and is left unchanged by all other interrupts.                
>>
>> Great.
>>
>> So when single stepping is enabled, you jump into the guest, 
>> get an itlb
>> miss, get out, still have DE set, get in KVM's own DE handler and can
>> process things from there.
>>
>> Could you check if the debug instruction was on PR=0? If so, you can
>> just rfi and be good, right?
>>
>>     
>
> Hr?
> The moment we found this happen we've already saved the guest and loaded host on exit path
> Rfi will make exit path again which means save guest again.
>   

Well the guest saving code is in our hands. So we can just modify the
debug interrupt handler in booke_interrupts.S to check for PR=0 first
thing and then decide whether to save to guest state or return to the
host kernel.

I think that'd make it a lot cleaner.

Alex

Re: [PATCH 3/4] kvmppc/booke: guest debug support

[Jan Kiszka]

Liu Yu wrote:
> According to user's gdb command,
> we set the corresponding debug control bits in shadow.
> 
> Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  arch/powerpc/include/asm/kvm_ppc.h |    3 +
>  arch/powerpc/kvm/booke.c           |   93 ++++++++++++++++++++++++++++++++++--
>  arch/powerpc/kvm/e500.c            |    8 ---
>  arch/powerpc/kvm/powerpc.c         |    2 +-
>  4 files changed, 93 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index e264282..8918aac 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -94,6 +94,9 @@ extern int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  extern int kvmppc_core_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, int rs);
>  extern int kvmppc_core_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, int rt);
>  
> +extern int kvmppc_core_set_guest_debug(struct kvm_vcpu *vcpu,
> +                                       struct kvm_guest_debug *dbg);
> +
>  extern int kvmppc_booke_init(void);
>  extern void kvmppc_booke_exit(void);
>  
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 4d686cc..ec2722d 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -267,6 +267,16 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  			break;
>  		}
>  
> +		if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_ENABLE) &&

This should better check for KVM_GUESTDBG_USE_SW_BP.

> +		             (vcpu->arch.last_inst == KVM_INST_GUESTGDB)) {
> +			run->exit_reason = KVM_EXIT_DEBUG;
> +			run->debug.arch.pc = vcpu->arch.pc;
> +			run->debug.arch.exception = exit_nr;
> +			kvmppc_account_exit(vcpu, DEBUG_EXITS);
> +			r = RESUME_HOST;
> +			break;
> +		}
> +
>  		er = kvmppc_emulate_instruction(run, vcpu);
>  		switch (er) {
>  		case EMULATE_DONE:
> @@ -293,6 +303,12 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  		default:
>  			BUG();
>  		}
> +
> +		if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_ENABLE) &&
> +		    (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {

Checking for KVM_GUESTDBG_ENABLE is redundant as you enforce guest_debug
= 0 in kvmppc_core_set_guest_debug if KVM_GUESTDBG_ENABLE is not set.

> +			run->exit_reason = KVM_EXIT_DEBUG;
> +			r = RESUME_HOST;
> +		}
>  		break;
>  
>  	case BOOKE_INTERRUPT_FP_UNAVAIL:
> @@ -421,12 +437,27 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  		u32 dbsr;
>  
>  		vcpu->arch.pc = mfspr(SPRN_CSRR0);
> -
> -		/* clear IAC events in DBSR register */
>  		dbsr = mfspr(SPRN_DBSR);
> -		dbsr &= DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4;
> -		mtspr(SPRN_DBSR, dbsr);
> +		run->debug.arch.pc = vcpu->arch.pc;
> +		run->debug.arch.status = 0;
> +
> +		if (dbsr & (DBSR_IAC1 | DBSR_IAC2 | DBSR_IAC3 | DBSR_IAC4)) {
> +			run->debug.arch.status |= KVMPPC_DEBUG_BREAKPOINT;
> +		} else {
> +			if (dbsr & (DBSR_DAC1W | DBSR_DAC2W))
> +				run->debug.arch.status |= KVMPPC_DEBUG_WATCH_WRITE;
> +			else if (dbsr & (DBSR_DAC1R | DBSR_DAC2R))
> +				run->debug.arch.status |= KVMPPC_DEBUG_WATCH_READ;
> +			if (dbsr & (DBSR_DAC1R | DBSR_DAC1W))
> +				run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac1;
> +			else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
> +				run->debug.arch.pc = vcpu->arch.shadow_dbg_reg.dac2;
> +		}
>  
> +		/* clear events in DBSR register */
> +		mtspr(SPRN_DBSR, ~0);
> +
> +		run->debug.arch.exception = exit_nr;
>  		run->exit_reason = KVM_EXIT_DEBUG;
>  		kvmppc_account_exit(vcpu, DEBUG_EXITS);
>  		r = RESUME_HOST;
> @@ -560,6 +591,60 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
>  	return -ENOTSUPP;
>  }
>  
> +int kvmppc_core_set_guest_debug(struct kvm_vcpu *vcpu,
> +                                struct kvm_guest_debug *dbg)
> +{
> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> +		vcpu->guest_debug = 0;
> +		return 0;
> +	}
> +
> +	vcpu->guest_debug = dbg->control;
> +	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> +
> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> +		vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
> +
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> +		struct kvmppc_debug_reg *gdbgr = &(vcpu->arch.shadow_dbg_reg);
> +		int n, b = 0, w = 0;
> +		const u32 bp_code[] = {
> +			DBCR0_IAC1 | DBCR0_IDM,
> +			DBCR0_IAC2 | DBCR0_IDM,
> +			DBCR0_IAC3 | DBCR0_IDM,
> +			DBCR0_IAC4 | DBCR0_IDM
> +		};
> +		const u32 wp_code[] = {
> +			DBCR0_DAC1W | DBCR0_IDM,
> +			DBCR0_DAC2W | DBCR0_IDM,
> +			DBCR0_DAC1R | DBCR0_IDM,
> +			DBCR0_DAC2R | DBCR0_IDM
> +		};
> +
> +		for (n = 0; n < 6 && dbg->arch.bp[n].type; n++) {
> +			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_BREAKPOINT)
> +				gdbgr->dbcr0 |= bp_code[b];
> +			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_WATCH_READ)
> +				gdbgr->dbcr0 |= wp_code[w + 2];
> +			if (dbg->arch.bp[n].type & KVMPPC_DEBUG_WATCH_WRITE)
> +				gdbgr->dbcr0 |= wp_code[w];
> +
> +			if (b < 4 && (gdbgr->dbcr0 & (DBCR0_IAC1 | DBCR0_IAC2 |
> +			              DBCR0_IAC3 | DBCR0_IAC4))) {
> +				gdbgr->iac[b] = dbg->arch.bp[n].addr;
> +				b++;
> +			}
> +			if (w < 2 && (gdbgr->dbcr0 & (DBCR0_DAC1W | DBCR0_DAC1R
> +			              | DBCR0_DAC2W | DBCR0_DAC2R))) {
> +				gdbgr->dac[w] = dbg->arch.bp[n].addr;
> +				w++;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  int __init kvmppc_booke_init(void)
>  {
>  	unsigned long ivor[16];
> diff --git a/arch/powerpc/kvm/e500.c b/arch/powerpc/kvm/e500.c
> index efa1198..65192eb 100644
> --- a/arch/powerpc/kvm/e500.c
> +++ b/arch/powerpc/kvm/e500.c
> @@ -24,14 +24,6 @@
>  #include "booke.h"
>  #include "e500_tlb.h"
>  
> -void kvmppc_core_load_host_debugstate(struct kvm_vcpu *vcpu)
> -{
> -}
> -
> -void kvmppc_core_load_guest_debugstate(struct kvm_vcpu *vcpu)
> -{
> -}
> -
>  void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  {
>  	kvmppc_e500_tlb_load(vcpu, cpu);
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 51aedd7..5bb17e5 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -265,7 +265,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>                                          struct kvm_guest_debug *dbg)
>  {
> -	return -EINVAL;
> +	return kvmppc_core_set_guest_debug(vcpu, dbg);
>  }
>  
>  static void kvmppc_complete_dcr_load(struct kvm_vcpu *vcpu,

Series looks good otherwise (from generic guest debugging POV).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/4] kvmppc: guest debug definitions

[Alexander Graf]

Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu@freescale.com>:

> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> ---
> arch/powerpc/include/asm/kvm.h      |   20 ++++++++++++++++++++
> arch/powerpc/include/asm/kvm_host.h |   16 ++++++++++++++++
> 2 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/ 
> asm/kvm.h
> index 81f3b0b..b7f7861 100644
> --- a/arch/powerpc/include/asm/kvm.h
> +++ b/arch/powerpc/include/asm/kvm.h
> @@ -22,6 +22,9 @@
>
> #include <linux/types.h>
>
> +/* Select powerpc specific features in <linux/kvm.h> */
> +#define __KVM_HAVE_GUEST_DEBUG
> +
> struct kvm_regs {
>    __u64 pc;
>    __u64 cr;
> @@ -71,10 +74,27 @@ struct kvm_fpu {
> };
>
> struct kvm_debug_exit_arch {
> +    __u32 exception;
> +    __u32 pc;
> +    __u32 status;
> };
>
> +#define KVM_INST_GUESTGDB               0x44000022

What instruction is this again? :) Is it something reserved for  
purposes like this?

Alex

> +
> +#define KVM_GUESTDBG_USE_SW_BP          0x00010000
> +#define KVM_GUESTDBG_USE_HW_BP          0x00020000
> +
> +#define KVMPPC_DEBUG_NOTYPE             0x0
> +#define KVMPPC_DEBUG_BREAKPOINT         (1UL << 1)
> +#define KVMPPC_DEBUG_WATCH_WRITE        (1UL << 2)
> +#define KVMPPC_DEBUG_WATCH_READ         (1UL << 3)
> +
> /* for KVM_SET_GUEST_DEBUG */
> struct kvm_guest_debug_arch {
> +    struct {
> +        __u32 addr;
> +        __u32 type;
> +    } bp[6];

I can't look up the sources right now. Is this a struct that 1:1 maps  
to an ioctl struct? If so, we should add padding for a possible future  
extension of debug registers.

I'd also prefer to see addr be u64. On 32 bit targets we can just use  
the lower 32 bits only.

Alex

RE: [PATCH 1/4] kvmppc: guest debug definitions

[Liu Yu-B13201]

 

> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org 
> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
> Sent: Wednesday, February 03, 2010 4:57 PM
> To: Liu Yu-B13201
> Cc: hollis@penguinppc.org; kvm-ppc@vger.kernel.org; 
> kvm@vger.kernel.org; Liu Yu-B13201
> Subject: Re: [PATCH 1/4] kvmppc: guest debug definitions
> 
> 
> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu@freescale.com>:
> 
> > Signed-off-by: Liu Yu <yu.liu@freescale.com>
> > ---
> > arch/powerpc/include/asm/kvm.h      |   20 ++++++++++++++++++++
> > arch/powerpc/include/asm/kvm_host.h |   16 ++++++++++++++++
> > 2 files changed, 36 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/ 
> > asm/kvm.h
> > index 81f3b0b..b7f7861 100644
> > --- a/arch/powerpc/include/asm/kvm.h
> > +++ b/arch/powerpc/include/asm/kvm.h
> > @@ -22,6 +22,9 @@
> >
> > #include <linux/types.h>
> >
> > +/* Select powerpc specific features in <linux/kvm.h> */
> > +#define __KVM_HAVE_GUEST_DEBUG
> > +
> > struct kvm_regs {
> >    __u64 pc;
> >    __u64 cr;
> > @@ -71,10 +74,27 @@ struct kvm_fpu {
> > };
> >
> > struct kvm_debug_exit_arch {
> > +    __u32 exception;
> > +    __u32 pc;
> > +    __u32 status;
> > };
> >
> > +#define KVM_INST_GUESTGDB               0x44000022
> 
> What instruction is this again? :) Is it something reserved for  
> purposes like this?
> 

Just an invalid instruction which can generate program interrupt...
I'm open to it's value btw.

> 
> > +
> > +#define KVM_GUESTDBG_USE_SW_BP          0x00010000
> > +#define KVM_GUESTDBG_USE_HW_BP          0x00020000
> > +
> > +#define KVMPPC_DEBUG_NOTYPE             0x0
> > +#define KVMPPC_DEBUG_BREAKPOINT         (1UL << 1)
> > +#define KVMPPC_DEBUG_WATCH_WRITE        (1UL << 2)
> > +#define KVMPPC_DEBUG_WATCH_READ         (1UL << 3)
> > +
> > /* for KVM_SET_GUEST_DEBUG */
> > struct kvm_guest_debug_arch {
> > +    struct {
> > +        __u32 addr;
> > +        __u32 type;
> > +    } bp[6];
> 
> I can't look up the sources right now. Is this a struct that 
> 1:1 maps  
> to an ioctl struct? If so, we should add padding for a 
> possible future  
> extension of debug registers.

Yes it's used by ioctl.
What's the usually pad size?

Re: [PATCH 1/4] kvmppc: guest debug definitions

[Alexander Graf]

Liu Yu-B13201 wrote:
>  
>
>   
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org 
>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
>> Sent: Wednesday, February 03, 2010 4:57 PM
>> To: Liu Yu-B13201
>> Cc: hollis@penguinppc.org; kvm-ppc@vger.kernel.org; 
>> kvm@vger.kernel.org; Liu Yu-B13201
>> Subject: Re: [PATCH 1/4] kvmppc: guest debug definitions
>>
>>
>> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu@freescale.com>:
>>
>>     
>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>> ---
>>> arch/powerpc/include/asm/kvm.h      |   20 ++++++++++++++++++++
>>> arch/powerpc/include/asm/kvm_host.h |   16 ++++++++++++++++
>>> 2 files changed, 36 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/ 
>>> asm/kvm.h
>>> index 81f3b0b..b7f7861 100644
>>> --- a/arch/powerpc/include/asm/kvm.h
>>> +++ b/arch/powerpc/include/asm/kvm.h
>>> @@ -22,6 +22,9 @@
>>>
>>> #include <linux/types.h>
>>>
>>> +/* Select powerpc specific features in <linux/kvm.h> */
>>> +#define __KVM_HAVE_GUEST_DEBUG
>>> +
>>> struct kvm_regs {
>>>    __u64 pc;
>>>    __u64 cr;
>>> @@ -71,10 +74,27 @@ struct kvm_fpu {
>>> };
>>>
>>> struct kvm_debug_exit_arch {
>>> +    __u32 exception;
>>> +    __u32 pc;
>>> +    __u32 status;
>>> };
>>>
>>> +#define KVM_INST_GUESTGDB               0x44000022
>>>       
>> What instruction is this again? :) Is it something reserved for  
>> purposes like this?
>>
>>     
>
> Just an invalid instruction which can generate program interrupt...
> I'm open to it's value btw.
>   

Well this definitely doesn't generate a program interrupt. Or at least
it shouldn't :-).
I just remembered where I've seen an opcode like this before. This is a
part of a dump of arch/powerpc/boot/ps3-hvcall.o

00000000 <lv1_get_logical_ppe_id>:
   0:   7c 08 02 a6     mflr    r0
   4:   90 01 00 04     stw     r0,4(r1)
   8:   94 21 ff f0     stwu    r1,-16(r1)
   c:   90 61 00 08     stw     r3,8(r1)
  10:   39 60 00 45     li      r11,69
  14:   44 00 00 22     sc      1

So as you can see, this is the hypercall instruction for lv1. IIRC beat
uses the same. I don't think we want to reuse that opcode for ourselves.
Maybe one day someone figures it's a good idea to implement a beat-style
ABI in KVM.

But IIRC sc can take a lot of values, so we can just take sc 0x1234 or
so :-).

>>> +
>>> +#define KVM_GUESTDBG_USE_SW_BP          0x00010000
>>> +#define KVM_GUESTDBG_USE_HW_BP          0x00020000
>>> +
>>> +#define KVMPPC_DEBUG_NOTYPE             0x0
>>> +#define KVMPPC_DEBUG_BREAKPOINT         (1UL << 1)
>>> +#define KVMPPC_DEBUG_WATCH_WRITE        (1UL << 2)
>>> +#define KVMPPC_DEBUG_WATCH_READ         (1UL << 3)
>>> +
>>> /* for KVM_SET_GUEST_DEBUG */
>>> struct kvm_guest_debug_arch {
>>> +    struct {
>>> +        __u32 addr;
>>> +        __u32 type;
>>> +    } bp[6];
>>>       
>> I can't look up the sources right now. Is this a struct that 
>> 1:1 maps  
>> to an ioctl struct? If so, we should add padding for a 
>> possible future  
>> extension of debug registers.
>>     
>
> Yes it's used by ioctl.
> What's the usually pad size?
>   

I don't think there's a default. I just tend to pad it to something
reasonable. I guess in this case we can even just extend bp to 128
entries, add a reasonable amount of churn to the debug info and be good:

struct kvm_guest_debug_arch {
    struct {
        __u64 addr;
        __u32 type;
        __u32 pad1;
        __u64 pad2;
    } bp[128];
}

This should be enough to even leverage performance monitoring stuff later on that would be able to check if r1 == 0x1234 and then stop :-).


Alex

RE: [PATCH 1/4] kvmppc: guest debug definitions

[Liu Yu-B13201]

 

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de] 
> Sent: Wednesday, February 03, 2010 5:51 PM
> To: Liu Yu-B13201
> Cc: hollis@penguinppc.org; kvm-ppc@vger.kernel.org; 
> kvm@vger.kernel.org
> Subject: Re: [PATCH 1/4] kvmppc: guest debug definitions
> 
> Liu Yu-B13201 wrote:
> >  
> >
> >   
> >> -----Original Message-----
> >> From: kvm-ppc-owner@vger.kernel.org 
> >> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
> >> Sent: Wednesday, February 03, 2010 4:57 PM
> >> To: Liu Yu-B13201
> >> Cc: hollis@penguinppc.org; kvm-ppc@vger.kernel.org; 
> >> kvm@vger.kernel.org; Liu Yu-B13201
> >> Subject: Re: [PATCH 1/4] kvmppc: guest debug definitions
> >>
> >>
> >> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu@freescale.com>:
> >>
> >>     
> >>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> >>> ---
> >>> arch/powerpc/include/asm/kvm.h      |   20 ++++++++++++++++++++
> >>> arch/powerpc/include/asm/kvm_host.h |   16 ++++++++++++++++
> >>> 2 files changed, 36 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/include/asm/kvm.h 
> b/arch/powerpc/include/ 
> >>> asm/kvm.h
> >>> index 81f3b0b..b7f7861 100644
> >>> --- a/arch/powerpc/include/asm/kvm.h
> >>> +++ b/arch/powerpc/include/asm/kvm.h
> >>> @@ -22,6 +22,9 @@
> >>>
> >>> #include <linux/types.h>
> >>>
> >>> +/* Select powerpc specific features in <linux/kvm.h> */
> >>> +#define __KVM_HAVE_GUEST_DEBUG
> >>> +
> >>> struct kvm_regs {
> >>>    __u64 pc;
> >>>    __u64 cr;
> >>> @@ -71,10 +74,27 @@ struct kvm_fpu {
> >>> };
> >>>
> >>> struct kvm_debug_exit_arch {
> >>> +    __u32 exception;
> >>> +    __u32 pc;
> >>> +    __u32 status;
> >>> };
> >>>
> >>> +#define KVM_INST_GUESTGDB               0x44000022
> >>>       
> >> What instruction is this again? :) Is it something reserved for  
> >> purposes like this?
> >>
> >>     
> >
> > Just an invalid instruction which can generate program interrupt...
> > I'm open to it's value btw.
> >   
> 
> Well this definitely doesn't generate a program interrupt. Or at least
> it shouldn't :-).
> I just remembered where I've seen an opcode like this before. 
> This is a
> part of a dump of arch/powerpc/boot/ps3-hvcall.o
> 
> 00000000 <lv1_get_logical_ppe_id>:
>    0:   7c 08 02 a6     mflr    r0
>    4:   90 01 00 04     stw     r0,4(r1)
>    8:   94 21 ff f0     stwu    r1,-16(r1)
>    c:   90 61 00 08     stw     r3,8(r1)
>   10:   39 60 00 45     li      r11,69
>   14:   44 00 00 22     sc      1
> 
> So as you can see, this is the hypercall instruction for lv1. 
> IIRC beat
> uses the same. I don't think we want to reuse that opcode for 
> ourselves.
> Maybe one day someone figures it's a good idea to implement a 
> beat-style
> ABI in KVM.
> 
> But IIRC sc can take a lot of values, so we can just take sc 0x1234 or
> so :-).
> 
> >>> +
> >>> +#define KVM_GUESTDBG_USE_SW_BP          0x00010000
> >>> +#define KVM_GUESTDBG_USE_HW_BP          0x00020000
> >>> +
> >>> +#define KVMPPC_DEBUG_NOTYPE             0x0
> >>> +#define KVMPPC_DEBUG_BREAKPOINT         (1UL << 1)
> >>> +#define KVMPPC_DEBUG_WATCH_WRITE        (1UL << 2)
> >>> +#define KVMPPC_DEBUG_WATCH_READ         (1UL << 3)
> >>> +
> >>> /* for KVM_SET_GUEST_DEBUG */
> >>> struct kvm_guest_debug_arch {
> >>> +    struct {
> >>> +        __u32 addr;
> >>> +        __u32 type;
> >>> +    } bp[6];
> >>>       
> >> I can't look up the sources right now. Is this a struct that 
> >> 1:1 maps  
> >> to an ioctl struct? If so, we should add padding for a 
> >> possible future  
> >> extension of debug registers.
> >>     
> >
> > Yes it's used by ioctl.
> > What's the usually pad size?
> >   
> 
> I don't think there's a default. I just tend to pad it to something
> reasonable. I guess in this case we can even just extend bp to 128
> entries, add a reasonable amount of churn to the debug info 
> and be good:
> 
> struct kvm_guest_debug_arch {
>     struct {
>         __u64 addr;
>         __u32 type;
>         __u32 pad1;
>         __u64 pad2;
>     } bp[128];
> }
> 

Software breakpoint is maintained by qemu.
Here it's only used by hardware breakpoint/watchpoint
Is 128 much too large?

Re: [PATCH 1/4] kvmppc: guest debug definitions

[Alexander Graf]

Liu Yu-B13201 wrote:
>  
>
>   
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf-l3A5Bk7waGM@public.gmane.org] 
>> Sent: Wednesday, February 03, 2010 5:51 PM
>> To: Liu Yu-B13201
>> Cc: hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org; kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH 1/4] kvmppc: guest debug definitions
>>
>> Liu Yu-B13201 wrote:
>>     
>>>  
>>>
>>>   
>>>       
>>>> -----Original Message-----
>>>> From: kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
>>>> [mailto:kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Alexander Graf
>>>> Sent: Wednesday, February 03, 2010 4:57 PM
>>>> To: Liu Yu-B13201
>>>> Cc: hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org; kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>>>> kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Liu Yu-B13201
>>>> Subject: Re: [PATCH 1/4] kvmppc: guest debug definitions
>>>>
>>>>
>>>> Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>:
>>>>
>>>>     
>>>>         
>>>>> Signed-off-by: Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>> ---
>>>>> arch/powerpc/include/asm/kvm.h      |   20 ++++++++++++++++++++
>>>>> arch/powerpc/include/asm/kvm_host.h |   16 ++++++++++++++++
>>>>> 2 files changed, 36 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/include/asm/kvm.h 
>>>>>           
>> b/arch/powerpc/include/ 
>>     
>>>>> asm/kvm.h
>>>>> index 81f3b0b..b7f7861 100644
>>>>> --- a/arch/powerpc/include/asm/kvm.h
>>>>> +++ b/arch/powerpc/include/asm/kvm.h
>>>>> @@ -22,6 +22,9 @@
>>>>>
>>>>> #include <linux/types.h>
>>>>>
>>>>> +/* Select powerpc specific features in <linux/kvm.h> */
>>>>> +#define __KVM_HAVE_GUEST_DEBUG
>>>>> +
>>>>> struct kvm_regs {
>>>>>    __u64 pc;
>>>>>    __u64 cr;
>>>>> @@ -71,10 +74,27 @@ struct kvm_fpu {
>>>>> };
>>>>>
>>>>> struct kvm_debug_exit_arch {
>>>>> +    __u32 exception;
>>>>> +    __u32 pc;
>>>>> +    __u32 status;
>>>>> };
>>>>>
>>>>> +#define KVM_INST_GUESTGDB               0x44000022
>>>>>       
>>>>>           
>>>> What instruction is this again? :) Is it something reserved for  
>>>> purposes like this?
>>>>
>>>>     
>>>>         
>>> Just an invalid instruction which can generate program interrupt...
>>> I'm open to it's value btw.
>>>   
>>>       
>> Well this definitely doesn't generate a program interrupt. Or at least
>> it shouldn't :-).
>> I just remembered where I've seen an opcode like this before. 
>> This is a
>> part of a dump of arch/powerpc/boot/ps3-hvcall.o
>>
>> 00000000 <lv1_get_logical_ppe_id>:
>>    0:   7c 08 02 a6     mflr    r0
>>    4:   90 01 00 04     stw     r0,4(r1)
>>    8:   94 21 ff f0     stwu    r1,-16(r1)
>>    c:   90 61 00 08     stw     r3,8(r1)
>>   10:   39 60 00 45     li      r11,69
>>   14:   44 00 00 22     sc      1
>>
>> So as you can see, this is the hypercall instruction for lv1. 
>> IIRC beat
>> uses the same. I don't think we want to reuse that opcode for 
>> ourselves.
>> Maybe one day someone figures it's a good idea to implement a 
>> beat-style
>> ABI in KVM.
>>
>> But IIRC sc can take a lot of values, so we can just take sc 0x1234 or
>> so :-).
>>
>>     
>>>>> +
>>>>> +#define KVM_GUESTDBG_USE_SW_BP          0x00010000
>>>>> +#define KVM_GUESTDBG_USE_HW_BP          0x00020000
>>>>> +
>>>>> +#define KVMPPC_DEBUG_NOTYPE             0x0
>>>>> +#define KVMPPC_DEBUG_BREAKPOINT         (1UL << 1)
>>>>> +#define KVMPPC_DEBUG_WATCH_WRITE        (1UL << 2)
>>>>> +#define KVMPPC_DEBUG_WATCH_READ         (1UL << 3)
>>>>> +
>>>>> /* for KVM_SET_GUEST_DEBUG */
>>>>> struct kvm_guest_debug_arch {
>>>>> +    struct {
>>>>> +        __u32 addr;
>>>>> +        __u32 type;
>>>>> +    } bp[6];
>>>>>       
>>>>>           
>>>> I can't look up the sources right now. Is this a struct that 
>>>> 1:1 maps  
>>>> to an ioctl struct? If so, we should add padding for a 
>>>> possible future  
>>>> extension of debug registers.
>>>>     
>>>>         
>>> Yes it's used by ioctl.
>>> What's the usually pad size?
>>>   
>>>       
>> I don't think there's a default. I just tend to pad it to something
>> reasonable. I guess in this case we can even just extend bp to 128
>> entries, add a reasonable amount of churn to the debug info 
>> and be good:
>>
>> struct kvm_guest_debug_arch {
>>     struct {
>>         __u64 addr;
>>         __u32 type;
>>         __u32 pad1;
>>         __u64 pad2;
>>     } bp[128];
>> }
>>
>>     
>
> Software breakpoint is maintained by qemu.
> Here it's only used by hardware breakpoint/watchpoint
> Is 128 much too large?
>   

Well, it's only 3kb. And that way we're _really_ future-proof ;-).

Remember, this is only the interface to userspace. The data we keep
around in the kernel can be much smaller.

Alex

Re: [PATCH RESEND 0/4] kvmppc/booke: add guest debug support

[Alexander Graf]

Am 03.02.2010 um 08:53 schrieb Liu Yu <yu.liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>:

> This patchset add guest debug support for booke.

I'd like to see an ack from Jan here. Some code looks like it uses  
generic interfaces.

Alex