[PATCH 0/7 v3] KVM :PPC: Userspace Debug support

[PATCH 0/7 v3] KVM :PPC: Userspace Debug support

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

This patchset adds the userspace debug support for booke/bookehv.
this is tested on powerpc e500v2/e500mc devices.

We are now assuming that debug resource will not be used by
kernel for its own debugging. It will be used for only kernel
user process debugging. So the kernel debug load interface during
context_to is used to load debug conext for that selected process.

v2->v3
 - We are now assuming that debug resource will not be used by
   kernel for its own debugging.
   It will be used for only kernel user process debugging.
   So the kernel debug load interface during context_to is
   used to load debug conext for that selected process.

v1->v2
 - Debug registers are save/restore in vcpu_put/vcpu_get.
   Earlier the debug registers are saved/restored in guest entry/exit

Bharat Bhushan (7):
  KVM: PPC: debug stub interface parameter defined
  Rename EMULATE_DO_PAPR to EMULATE_EXIT_USER
  KVM: extend EMULATE_EXIT_USER to support different exit reasons
  booke: exit to user space if emulator request
  KVM: PPC: exit to user space on "ehpriv" instruction
  powerpc: export debug register save function for KVM
  KVM: PPC: Add userspace debug stub support

 arch/powerpc/include/asm/kvm_host.h  |    8 +
 arch/powerpc/include/asm/kvm_ppc.h   |    2 +-
 arch/powerpc/include/asm/switch_to.h |    4 +
 arch/powerpc/include/uapi/asm/kvm.h  |   37 +++++
 arch/powerpc/kernel/process.c        |    3 +-
 arch/powerpc/kvm/book3s.c            |    6 +
 arch/powerpc/kvm/book3s_emulate.c    |    4 +-
 arch/powerpc/kvm/book3s_pr.c         |    4 +-
 arch/powerpc/kvm/booke.c             |  239 ++++++++++++++++++++++++++++++++--
 arch/powerpc/kvm/booke.h             |    5 +
 arch/powerpc/kvm/e500_emulate.c      |   10 ++
 arch/powerpc/kvm/powerpc.c           |    6 -
 12 files changed, 304 insertions(+), 24 deletions(-)

[PATCH 1/7 v3] KVM: PPC: debug stub interface parameter defined

[Bharat Bhushan]

This patch defines the interface parameter for KVM_SET_GUEST_DEBUG
ioctl support. Follow up patches will use this for setting up
hardware breakpoints, watchpoints and software breakpoints.

Also kvm_arch_vcpu_ioctl_set_guest_debug() is brought one level below.
This is because I am not sure what is required for book3s. So this ioctl
behaviour will not change for book3s.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/uapi/asm/kvm.h |   23 +++++++++++++++++++++++
 arch/powerpc/kvm/book3s.c           |    6 ++++++
 arch/powerpc/kvm/booke.c            |    6 ++++++
 arch/powerpc/kvm/powerpc.c          |    6 ------
 4 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
index c2ff99c..c0c38ed 100644
--- a/arch/powerpc/include/uapi/asm/kvm.h
+++ b/arch/powerpc/include/uapi/asm/kvm.h
@@ -272,8 +272,31 @@ struct kvm_debug_exit_arch {
 
 /* for KVM_SET_GUEST_DEBUG */
 struct kvm_guest_debug_arch {
+	struct {
+		/* H/W breakpoint/watchpoint address */
+		__u64 addr;
+		/*
+		 * Type denotes h/w breakpoint, read watchpoint, write
+		 * watchpoint or watchpoint (both read and write).
+		 */
+#define KVMPPC_DEBUG_NONE		0x0
+#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
+#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
+#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
+		__u32 type;
+		__u32 reserved;
+	} bp[16];
 };
 
+/* Debug related defines */
+/*
+ * kvm_guest_debug->control is a 32 bit field. The lower 16 bits are generic
+ * and upper 16 bits are architecture specific. Architecture specific defines
+ * that ioctl is for setting hardware breakpoint or software breakpoint.
+ */
+#define KVM_GUESTDBG_USE_SW_BP		0x00010000
+#define KVM_GUESTDBG_USE_HW_BP		0x00020000
+
 /* definition of registers in kvm_run */
 struct kvm_sync_regs {
 };
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 2d32ae4..128ed3a 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -612,6 +612,12 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
+					struct kvm_guest_debug *dbg)
+{
+	return -EINVAL;
+}
+
 void kvmppc_decrementer_func(unsigned long data)
 {
 	struct kvm_vcpu *vcpu = (struct kvm_vcpu *)data;
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index a49a68a..a3e2db0 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1526,6 +1526,12 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	return r;
 }
 
+int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
+					 struct kvm_guest_debug *dbg)
+{
+	return -EINVAL;
+}
+
 int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 {
 	return -ENOTSUPP;
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 16b4595..716c2d4 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -531,12 +531,6 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 #endif
 }
 
-int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
-                                        struct kvm_guest_debug *dbg)
-{
-	return -EINVAL;
-}
-
 static void kvmppc_complete_dcr_load(struct kvm_vcpu *vcpu,
                                      struct kvm_run *run)
 {
-- 
1.7.0.4

[PATCH 2/7 v3] Rename EMULATE_DO_PAPR to EMULATE_EXIT_USER

[Bharat Bhushan]

Instruction emulation return EMULATE_DO_PAPR when it requires
exit to userspace on book3s. Similar return is required
for booke. EMULATE_DO_PAPR reads out to be confusing so it is
renamed to EMULATE_EXIT_USER.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/kvm_ppc.h |    2 +-
 arch/powerpc/kvm/book3s_emulate.c  |    2 +-
 arch/powerpc/kvm/book3s_pr.c       |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index f589307..8c2c8ef 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -44,7 +44,7 @@ enum emulation_result {
 	EMULATE_DO_DCR,       /* kvm_run filled with DCR request */
 	EMULATE_FAIL,         /* can't emulate this instruction */
 	EMULATE_AGAIN,        /* something went wrong. go again */
-	EMULATE_DO_PAPR,      /* kvm_run filled with PAPR request */
+	EMULATE_EXIT_USER,    /* emulation requires exit to user-space */
 };
 
 extern int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu);
diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index 836c569..cdd19d6 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -194,7 +194,7 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
 				run->papr_hcall.args[i] = gpr;
 			}
 
-			emulated = EMULATE_DO_PAPR;
+			emulated = EMULATE_EXIT_USER;
 			break;
 		}
 #endif
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 286e23e..b960faf 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -762,7 +762,7 @@ program_interrupt:
 			run->exit_reason = KVM_EXIT_MMIO;
 			r = RESUME_HOST_NV;
 			break;
-		case EMULATE_DO_PAPR:
+		case EMULATE_EXIT_USER:
 			run->exit_reason = KVM_EXIT_PAPR_HCALL;
 			vcpu->arch.hcall_needed = 1;
 			r = RESUME_HOST_NV;
-- 
1.7.0.4

[PATCH 3/7 v3] KVM: extend EMULATE_EXIT_USER to support different exit reasons

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

Currently the instruction emulator code returns EMULATE_EXIT_USER
and common code initializes the "run->exit_reason = .." and
"vcpu->arch.hcall_needed = .." with one fixed reason.
But there can be different reasons when emulator need to exit
to user space. To support that the "run->exit_reason = .."
and "vcpu->arch.hcall_needed = .." initialization is moved a
level up to emulator.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/kvm/book3s_emulate.c |    2 ++
 arch/powerpc/kvm/book3s_pr.c      |    2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index cdd19d6..1f6344c 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -194,6 +194,8 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
 				run->papr_hcall.args[i] = gpr;
 			}
 
+			run->exit_reason = KVM_EXIT_PAPR_HCALL;
+			vcpu->arch.hcall_needed = 1;
 			emulated = EMULATE_EXIT_USER;
 			break;
 		}
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index b960faf..c1cffa8 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -763,8 +763,6 @@ program_interrupt:
 			r = RESUME_HOST_NV;
 			break;
 		case EMULATE_EXIT_USER:
-			run->exit_reason = KVM_EXIT_PAPR_HCALL;
-			vcpu->arch.hcall_needed = 1;
 			r = RESUME_HOST_NV;
 			break;
 		default:
-- 
1.7.0.4

[PATCH 4/7 v3] booke: exit to user space if emulator request

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

This allows the exit to user space if emulator request by returning
EMULATE_EXIT_USER. This will be used in subsequent patches in list

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/kvm/booke.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index a3e2db0..97ae158 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -745,6 +745,9 @@ static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu)
 		kvmppc_core_queue_program(vcpu, ESR_PIL);
 		return RESUME_HOST;
 
+	case EMULATE_EXIT_USER:
+		return RESUME_HOST;
+
 	default:
 		BUG();
 	}
-- 
1.7.0.4

[PATCH 5/7 v3] KVM: PPC: exit to user space on "ehpriv" instruction

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

"ehpriv" instruction is used for setting software breakpoints
by user space. This patch adds support to exit to user space
with "run->debug" have relevant information.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/kvm/e500_emulate.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
index e78f353..cefdd38 100644
--- a/arch/powerpc/kvm/e500_emulate.c
+++ b/arch/powerpc/kvm/e500_emulate.c
@@ -26,6 +26,7 @@
 #define XOP_TLBRE   946
 #define XOP_TLBWE   978
 #define XOP_TLBILX  18
+#define XOP_EHPRIV  270
 
 #ifdef CONFIG_KVM_E500MC
 static int dbell2prio(ulong param)
@@ -130,6 +131,15 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
 			emulated = kvmppc_e500_emul_tlbivax(vcpu, ea);
 			break;
 
+		case XOP_EHPRIV:
+			run->exit_reason = KVM_EXIT_DEBUG;
+			run->debug.arch.address = vcpu->arch.pc;
+			run->debug.arch.status = 0;
+			kvmppc_account_exit(vcpu, DEBUG_EXITS);
+			emulated = EMULATE_EXIT_USER;
+			*advance = 0;
+			break;
+
 		default:
 			emulated = EMULATE_FAIL;
 		}
-- 
1.7.0.4

[PATCH 6/7 v3] powerpc: export debug register save function for KVM

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

KVM need this function when switching from vcpu to user-space
thread. My subsequent patch will use this function.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/switch_to.h |    4 ++++
 arch/powerpc/kernel/process.c        |    3 ++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h
index 200d763..50b357f 100644
--- a/arch/powerpc/include/asm/switch_to.h
+++ b/arch/powerpc/include/asm/switch_to.h
@@ -30,6 +30,10 @@ extern void enable_kernel_spe(void);
 extern void giveup_spe(struct task_struct *);
 extern void load_up_spe(struct task_struct *);
 
+#ifdef CONFIG_PPC_ADV_DEBUG_REGS
+extern void switch_booke_debug_regs(struct thread_struct *new_thread);
+#endif
+
 #ifndef CONFIG_SMP
 extern void discard_lazy_cpu_state(void);
 #else
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 59dd545..7b2296b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -362,12 +362,13 @@ static void prime_debug_regs(struct thread_struct *thread)
  * debug registers, set the debug registers from the values
  * stored in the new thread.
  */
-static void switch_booke_debug_regs(struct thread_struct *new_thread)
+void switch_booke_debug_regs(struct thread_struct *new_thread)
 {
 	if ((current->thread.dbcr0 & DBCR0_IDM)
 		|| (new_thread->dbcr0 & DBCR0_IDM))
 			prime_debug_regs(new_thread);
 }
+EXPORT_SYMBOL(switch_booke_debug_regs);
 #else	/* !CONFIG_PPC_ADV_DEBUG_REGS */
 #ifndef CONFIG_HAVE_HW_BREAKPOINT
 static void set_debug_reg_defaults(struct thread_struct *thread)
-- 
1.7.0.4

[PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Bharat Bhushan]

From: Bharat Bhushan <bharat.bhushan@freescale.com>

This patch adds the debug stub support on booke/bookehv.
Now QEMU debug stub can use hw breakpoint, watchpoint and software
breakpoint to debug guest.

Debug registers are saved/restored on vcpu_put()/vcpu_get().
Also the debug registers are saved restored only if guest
is using debug resources.

Currently we do not support debug resource emulation to guest,
so always exit to user space irrespective of user space is expecting
the debug exception or not. This is unexpected event and let us
leave the action on user space. This is similar to what it was before,
only thing is that now we have proper exit state available to user space.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 arch/powerpc/include/asm/kvm_host.h |    8 +
 arch/powerpc/include/uapi/asm/kvm.h |   22 +++-
 arch/powerpc/kvm/booke.c            |  242 ++++++++++++++++++++++++++++++++---
 arch/powerpc/kvm/booke.h            |    5 +
 4 files changed, 255 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index e34f8fe..b9ad20f 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -505,7 +505,15 @@ struct kvm_vcpu_arch {
 	u32 mmucfg;
 	u32 epr;
 	u32 crit_save;
+
+	/* Flag indicating that debug registers are used by guest */
+	bool debug_active;
+	/* for save/restore thread->dbcr0 on vcpu run/heavyweight_exit */
+	u32 saved_dbcr0;
+	/* guest debug registers*/
 	struct kvmppc_booke_debug_reg dbg_reg;
+	/* shadow debug registers */
+	struct kvmppc_booke_debug_reg shadow_dbg_reg;
 #endif
 	gpa_t paddr_accessed;
 	gva_t vaddr_accessed;
diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
index c0c38ed..d7ce449 100644
--- a/arch/powerpc/include/uapi/asm/kvm.h
+++ b/arch/powerpc/include/uapi/asm/kvm.h
@@ -25,6 +25,7 @@
 /* Select powerpc specific features in <linux/kvm.h> */
 #define __KVM_HAVE_SPAPR_TCE
 #define __KVM_HAVE_PPC_SMT
+#define __KVM_HAVE_GUEST_DEBUG
 
 struct kvm_regs {
 	__u64 pc;
@@ -267,7 +268,24 @@ struct kvm_fpu {
 	__u64 fpr[32];
 };
 
+/*
+ * Defines for h/w breakpoint, watchpoint (read, write or both) and
+ * software breakpoint.
+ * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
+ * for KVM_DEBUG_EXIT.
+ */
+#define KVMPPC_DEBUG_NONE		0x0
+#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
+#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
+#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
 struct kvm_debug_exit_arch {
+	__u64 address;
+	/*
+	 * exiting to userspace because of h/w breakpoint, watchpoint
+	 * (read, write or both) and software breakpoint.
+	 */
+	__u32 status;
+	__u32 reserved;
 };
 
 /* for KVM_SET_GUEST_DEBUG */
@@ -279,10 +297,6 @@ struct kvm_guest_debug_arch {
 		 * Type denotes h/w breakpoint, read watchpoint, write
 		 * watchpoint or watchpoint (both read and write).
 		 */
-#define KVMPPC_DEBUG_NONE		0x0
-#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
-#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
-#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
 		__u32 type;
 		__u32 reserved;
 	} bp[16];
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 97ae158..0e93416 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
 #endif
 }
 
+static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
+{
+	/* Synchronize guest's desire to get debug interrupts into shadow MSR */
+#ifndef CONFIG_KVM_BOOKE_HV
+	vcpu->arch.shadow_msr &= ~MSR_DE;
+	vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE;
+#endif
+
+	/* Force enable debug interrupts when user space wants to debug */
+	if (vcpu->guest_debug) {
+#ifdef CONFIG_KVM_BOOKE_HV
+		/*
+		 * Since there is no shadow MSR, sync MSR_DE into the guest
+		 * visible MSR.
+		 */
+		vcpu->arch.shared->msr |= MSR_DE;
+#else
+		vcpu->arch.shadow_msr |= MSR_DE;
+		vcpu->arch.shared->msr &= ~MSR_DE;
+#endif
+	}
+}
+
 /*
  * Helper function for "full" MSR writes.  No need to call this if only
  * EE/CE/ME/DE/RI are changing.
@@ -150,6 +173,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
 	kvmppc_mmu_msr_notify(vcpu, old_msr);
 	kvmppc_vcpu_sync_spe(vcpu);
 	kvmppc_vcpu_sync_fpu(vcpu);
+	kvmppc_vcpu_sync_debug(vcpu);
 }
 
 static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,
@@ -646,6 +670,46 @@ int kvmppc_core_check_requests(struct kvm_vcpu *vcpu)
 	return r;
 }
 
+static void kvmppc_load_usespace_gebug(void)
+{
+	switch_booke_debug_regs(&current->thread);
+}
+
+static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu *vcpu)
+{
+	if (!vcpu->arch.debug_active)
+		return;
+
+	/* Disable all debug events and clead pending debug events */
+	mtspr(SPRN_DBCR0, 0x0);
+	kvmppc_clear_dbsr();
+
+	/*
+	 * Check whether guest still need debug resource, if not then there
+	 * is no need to restore guest context.
+	 */
+	if (!vcpu->arch.shadow_dbg_reg.dbcr0)
+		return;
+
+	/* Load Guest Context */
+	mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
+	mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2);
+#ifdef CONFIG_KVM_E500MC
+	mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
+#endif
+	mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
+	mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
+#if CONFIG_PPC_ADV_DEBUG_IACS > 2
+	mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
+	mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
+#endif
+	mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
+	mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
+
+	/* Enable debug events after other debug registers restored */
+	mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0);
+}
+
 int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 {
 	int ret, s;
@@ -693,11 +757,25 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	kvmppc_load_guest_fp(vcpu);
 #endif
 
+	/*
+	 * Clear current->thread.dbcr0 so that kernel does not
+	 * restore h/w registers on context switch in vcpu running state.
+	 */
+	vcpu->arch.debug_active = 1;
+	vcpu->arch.saved_dbcr0 = current->thread.dbcr0;
+	current->thread.dbcr0 = 0;
+	kvmppc_booke_vcpu_load_debug_regs(vcpu);
+
 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
 
 	/* No need for kvm_guest_exit. It's done in handle_exit.
 	   We also get here with interrupts enabled. */
 
+	/* Restore thread->dbcr0 */
+	vcpu->arch.debug_active = 0;
+	current->thread.dbcr0 = vcpu->arch.saved_dbcr0;
+	kvmppc_load_usespace_gebug();
+
 #ifdef CONFIG_PPC_FPU
 	kvmppc_save_guest_fp(vcpu);
 
@@ -753,6 +831,36 @@ static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu)
 	}
 }
 
+/*
+ * Currently we do not support debug resource emulation to guest,
+ * so always exit to user space irrespective of user space is
+ * expecting the debug exception or not. This is unexpected event
+ * and let us leave the action on user space.
+ */
+static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu)
+{
+	u32 dbsr = mfspr(SPRN_DBSR);
+
+	kvmppc_clear_dbsr();
+	run->debug.arch.status = 0;
+	run->debug.arch.address = vcpu->arch.pc;
+
+	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.address = vcpu->arch.shadow_dbg_reg.dac[0];
+		else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
+			run->debug.arch.address = vcpu->arch.shadow_dbg_reg.dac[1];
+	}
+
+	return RESUME_HOST;
+}
+
 static void kvmppc_fill_pt_regs(struct pt_regs *regs)
 {
 	ulong r1, ip, msr, lr;
@@ -1112,18 +1220,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	}
 
 	case BOOKE_INTERRUPT_DEBUG: {
-		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->exit_reason = KVM_EXIT_DEBUG;
+		r = kvmppc_handle_debug(run, vcpu);
+		if (r == RESUME_HOST)
+			run->exit_reason = KVM_EXIT_DEBUG;
 		kvmppc_account_exit(vcpu, DEBUG_EXITS);
-		r = RESUME_HOST;
 		break;
 	}
 
@@ -1174,7 +1274,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 	kvmppc_set_msr(vcpu, 0);
 
 #ifndef CONFIG_KVM_BOOKE_HV
-	vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;
+	vcpu->arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
 	vcpu->arch.shadow_pid = 1;
 	vcpu->arch.shared->msr = 0;
 #endif
@@ -1529,12 +1629,6 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	return r;
 }
 
-int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
-					 struct kvm_guest_debug *dbg)
-{
-	return -EINVAL;
-}
-
 int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 {
 	return -ENOTSUPP;
@@ -1640,16 +1734,128 @@ void kvmppc_decrementer_func(unsigned long data)
 	kvmppc_set_tsr_bits(vcpu, TSR_DIS);
 }
 
+static void kvmppc_booke_vcpu_put_debug_regs(struct kvm_vcpu *vcpu)
+{
+	/* Disable all debug events First */
+	mtspr(SPRN_DBCR0, 0x0);
+	/* Disable pending debug event by clearing DBSR */
+	kvmppc_clear_dbsr();
+}
+
+int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
+					 struct kvm_guest_debug *dbg)
+{
+	struct kvmppc_booke_debug_reg *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
+	};
+
+	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
+		/* Clear All debug events */
+		vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
+		vcpu->guest_debug = 0;
+#ifdef CONFIG_KVM_BOOKE_HV
+		/*
+		 * When user space is not using the debug resources
+		 * then allow guest to change the MSR.DE.
+		 */
+		vcpu->arch.shadow_msrp &= ~MSRP_DEP;
+#endif
+		return 0;
+	}
+
+#ifdef CONFIG_KVM_BOOKE_HV
+		/*
+		 * When user space is using the debug resource then
+		 * do not allow guest to change the MSR.DE.
+		 */
+		vcpu->arch.shadow_msrp &= ~MSRP_DEP;
+#endif
+	vcpu->guest_debug = dbg->control;
+	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
+	/* Set DBCR0_EDM in guest visible DBCR0 register. */
+	vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
+
+	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))
+		/* Code below handles only HW breakpoints */
+		return 0;
+
+	dbg_reg = &(vcpu->arch.shadow_dbg_reg);
+
+	/*
+	 * On BOOKE (e500v2); Set DBCR1 and DBCR2 to allow debug events
+	 * to occur when MSR.PR is set.
+	 * On BOOKE-HV (e500mc+); MSR.PR = 0 when guest is running. So we
+	 * should clear DBCR1 and DBCR2. And EPCR.DUVD is used to control
+	 * that debug events will not come in hypervisor (GS = 0).
+	 */
+#ifdef CONFIG_KVM_BOOKE_HV
+	dbg_reg->dbcr1 = 0;
+	dbg_reg->dbcr2 = 0;
+#else
+	dbg_reg->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US | DBCR1_IAC3US |
+			  DBCR1_IAC4US;
+	dbg_reg->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US;
+#endif
+
+	for (n = 0; n < (KVMPPC_BOOKE_IAC_NUM + KVMPPC_BOOKE_DAC_NUM); n++) {
+		u32 type = dbg->arch.bp[n].type;
+
+		if (!type)
+			continue;
+
+		if (type & (KVMPPC_DEBUG_WATCH_READ |
+			    KVMPPC_DEBUG_WATCH_WRITE)) {
+			if (w >= KVMPPC_BOOKE_DAC_NUM)
+				return -EINVAL;
+
+			if (type & KVMPPC_DEBUG_WATCH_READ)
+				dbg_reg->dbcr0 |= wp_code[w + 2];
+			if (type & KVMPPC_DEBUG_WATCH_WRITE)
+				dbg_reg->dbcr0 |= wp_code[w];
+
+			dbg_reg->dac[w] = dbg->arch.bp[n].addr;
+			w++;
+		} else if (type & KVMPPC_DEBUG_BREAKPOINT) {
+			if (b >= KVMPPC_BOOKE_IAC_NUM)
+				return -EINVAL;
+
+			dbg_reg->dbcr0 |= bp_code[b];
+			dbg_reg->iac[b] = dbg->arch.bp[n].addr;
+			b++;
+		}
+	}
+
+	return 0;
+}
+
 void kvmppc_booke_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 	vcpu->cpu = smp_processor_id();
 	current->thread.kvm_vcpu = vcpu;
+
+	kvmppc_booke_vcpu_load_debug_regs(vcpu);
 }
 
 void kvmppc_booke_vcpu_put(struct kvm_vcpu *vcpu)
 {
 	current->thread.kvm_vcpu = NULL;
 	vcpu->cpu = -1;
+
+	kvmppc_booke_vcpu_put_debug_regs(vcpu);
 }
 
 int __init kvmppc_booke_init(void)
diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h
index 5fd1ba6..a1ff67d 100644
--- a/arch/powerpc/kvm/booke.h
+++ b/arch/powerpc/kvm/booke.h
@@ -129,4 +129,9 @@ static inline void kvmppc_save_guest_fp(struct kvm_vcpu *vcpu)
 		giveup_fpu(current);
 #endif
 }
+
+static inline void kvmppc_clear_dbsr(void)
+{
+	mtspr(SPRN_DBSR, mfspr(SPRN_DBSR));
+}
 #endif /* __KVM_BOOKE_H__ */
-- 
1.7.0.4

Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Alexander Graf]

On 08.04.2013, at 12:32, Bharat Bhushan wrote:

> From: Bharat Bhushan <bharat.bhushan@freescale.com>
> 
> This patch adds the debug stub support on booke/bookehv.
> Now QEMU debug stub can use hw breakpoint, watchpoint and software
> breakpoint to debug guest.
> 
> Debug registers are saved/restored on vcpu_put()/vcpu_get().
> Also the debug registers are saved restored only if guest
> is using debug resources.
> 
> Currently we do not support debug resource emulation to guest,
> so always exit to user space irrespective of user space is expecting
> the debug exception or not. This is unexpected event and let us
> leave the action on user space. This is similar to what it was before,
> only thing is that now we have proper exit state available to user space.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> arch/powerpc/include/asm/kvm_host.h |    8 +
> arch/powerpc/include/uapi/asm/kvm.h |   22 +++-
> arch/powerpc/kvm/booke.c            |  242 ++++++++++++++++++++++++++++++++---
> arch/powerpc/kvm/booke.h            |    5 +
> 4 files changed, 255 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index e34f8fe..b9ad20f 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -505,7 +505,15 @@ struct kvm_vcpu_arch {
> 	u32 mmucfg;
> 	u32 epr;
> 	u32 crit_save;
> +
> +	/* Flag indicating that debug registers are used by guest */
> +	bool debug_active;
> +	/* for save/restore thread->dbcr0 on vcpu run/heavyweight_exit */
> +	u32 saved_dbcr0;
> +	/* guest debug registers*/
> 	struct kvmppc_booke_debug_reg dbg_reg;
> +	/* shadow debug registers */
> +	struct kvmppc_booke_debug_reg shadow_dbg_reg;
> #endif
> 	gpa_t paddr_accessed;
> 	gva_t vaddr_accessed;
> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> index c0c38ed..d7ce449 100644
> --- a/arch/powerpc/include/uapi/asm/kvm.h
> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> @@ -25,6 +25,7 @@
> /* Select powerpc specific features in <linux/kvm.h> */
> #define __KVM_HAVE_SPAPR_TCE
> #define __KVM_HAVE_PPC_SMT
> +#define __KVM_HAVE_GUEST_DEBUG
> 
> struct kvm_regs {
> 	__u64 pc;
> @@ -267,7 +268,24 @@ struct kvm_fpu {
> 	__u64 fpr[32];
> };
> 
> +/*
> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
> + * software breakpoint.
> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
> + * for KVM_DEBUG_EXIT.
> + */
> +#define KVMPPC_DEBUG_NONE		0x0
> +#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
> +#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
> +#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
> struct kvm_debug_exit_arch {
> +	__u64 address;
> +	/*
> +	 * exiting to userspace because of h/w breakpoint, watchpoint
> +	 * (read, write or both) and software breakpoint.
> +	 */
> +	__u32 status;
> +	__u32 reserved;
> };
> 
> /* for KVM_SET_GUEST_DEBUG */
> @@ -279,10 +297,6 @@ struct kvm_guest_debug_arch {
> 		 * Type denotes h/w breakpoint, read watchpoint, write
> 		 * watchpoint or watchpoint (both read and write).
> 		 */
> -#define KVMPPC_DEBUG_NONE		0x0
> -#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
> -#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
> -#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
> 		__u32 type;
> 		__u32 reserved;
> 	} bp[16];
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 97ae158..0e93416 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
> #endif
> }
> 
> +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
> +{
> +	/* Synchronize guest's desire to get debug interrupts into shadow MSR */
> +#ifndef CONFIG_KVM_BOOKE_HV
> +	vcpu->arch.shadow_msr &= ~MSR_DE;
> +	vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE;
> +#endif
> +
> +	/* Force enable debug interrupts when user space wants to debug */
> +	if (vcpu->guest_debug) {
> +#ifdef CONFIG_KVM_BOOKE_HV
> +		/*
> +		 * Since there is no shadow MSR, sync MSR_DE into the guest
> +		 * visible MSR.
> +		 */
> +		vcpu->arch.shared->msr |= MSR_DE;
> +#else
> +		vcpu->arch.shadow_msr |= MSR_DE;
> +		vcpu->arch.shared->msr &= ~MSR_DE;
> +#endif
> +	}
> +}
> +
> /*
>  * Helper function for "full" MSR writes.  No need to call this if only
>  * EE/CE/ME/DE/RI are changing.
> @@ -150,6 +173,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
> 	kvmppc_mmu_msr_notify(vcpu, old_msr);
> 	kvmppc_vcpu_sync_spe(vcpu);
> 	kvmppc_vcpu_sync_fpu(vcpu);
> +	kvmppc_vcpu_sync_debug(vcpu);
> }
> 
> static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,
> @@ -646,6 +670,46 @@ int kvmppc_core_check_requests(struct kvm_vcpu *vcpu)
> 	return r;
> }
> 
> +static void kvmppc_load_usespace_gebug(void)

...

> +{
> +	switch_booke_debug_regs(&current->thread);
> +}
> +
> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu *vcpu)
> +{
> +	if (!vcpu->arch.debug_active)
> +		return;
> +
> +	/* Disable all debug events and clead pending debug events */
> +	mtspr(SPRN_DBCR0, 0x0);
> +	kvmppc_clear_dbsr();
> +
> +	/*
> +	 * Check whether guest still need debug resource, if not then there
> +	 * is no need to restore guest context.
> +	 */
> +	if (!vcpu->arch.shadow_dbg_reg.dbcr0)
> +		return;
> +
> +	/* Load Guest Context */
> +	mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
> +	mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2);
> +#ifdef CONFIG_KVM_E500MC
> +	mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);

You need to make sure DBCR4 is 0 when you leave things back to normal user space. Otherwise guest debug can interfere with host debug.

> +#endif
> +	mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
> +	mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> +	mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
> +	mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
> +#endif
> +	mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
> +	mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
> +
> +	/* Enable debug events after other debug registers restored */
> +	mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0);
> +}

All of the code above looks suspiciously similar to prime_debug_regs();. Can't we somehow reuse that?

> +
> int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> {
> 	int ret, s;
> @@ -693,11 +757,25 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> 	kvmppc_load_guest_fp(vcpu);
> #endif
> 
> +	/*
> +	 * Clear current->thread.dbcr0 so that kernel does not
> +	 * restore h/w registers on context switch in vcpu running state.
> +	 */
> +	vcpu->arch.debug_active = 1;

= true;

> +	vcpu->arch.saved_dbcr0 = current->thread.dbcr0;
> +	current->thread.dbcr0 = 0;
> +	kvmppc_booke_vcpu_load_debug_regs(vcpu);

static void switch_booke_debug_regs(struct thread_struct *new_thread)
{
        if ((current->thread.dbcr0 & DBCR0_IDM)
                || (new_thread->dbcr0 & DBCR0_IDM))
                        prime_debug_regs(new_thread);
}

The kernel will also restore debug state if the process we come from has debugging enabled. Please adjust the comment accordingly.

> +
> 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
> 
> 	/* No need for kvm_guest_exit. It's done in handle_exit.
> 	   We also get here with interrupts enabled. */
> 
> +	/* Restore thread->dbcr0 */
> +	vcpu->arch.debug_active = 0;
> +	current->thread.dbcr0 = vcpu->arch.saved_dbcr0;
> +	kvmppc_load_usespace_gebug();
> +
> #ifdef CONFIG_PPC_FPU
> 	kvmppc_save_guest_fp(vcpu);
> 
> @@ -753,6 +831,36 @@ static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu)
> 	}
> }
> 
> +/*
> + * Currently we do not support debug resource emulation to guest,
> + * so always exit to user space irrespective of user space is
> + * expecting the debug exception or not. This is unexpected event
> + * and let us leave the action on user space.
> + */
> +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu)
> +{
> +	u32 dbsr = mfspr(SPRN_DBSR);
> +
> +	kvmppc_clear_dbsr();
> +	run->debug.arch.status = 0;
> +	run->debug.arch.address = vcpu->arch.pc;
> +
> +	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.address = vcpu->arch.shadow_dbg_reg.dac[0];
> +		else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
> +			run->debug.arch.address = vcpu->arch.shadow_dbg_reg.dac[1];
> +	}
> +
> +	return RESUME_HOST;
> +}
> +
> static void kvmppc_fill_pt_regs(struct pt_regs *regs)
> {
> 	ulong r1, ip, msr, lr;
> @@ -1112,18 +1220,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> 	}
> 
> 	case BOOKE_INTERRUPT_DEBUG: {
> -		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->exit_reason = KVM_EXIT_DEBUG;
> +		r = kvmppc_handle_debug(run, vcpu);
> +		if (r == RESUME_HOST)
> +			run->exit_reason = KVM_EXIT_DEBUG;
> 		kvmppc_account_exit(vcpu, DEBUG_EXITS);
> -		r = RESUME_HOST;
> 		break;
> 	}
> 
> @@ -1174,7 +1274,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
> 	kvmppc_set_msr(vcpu, 0);
> 
> #ifndef CONFIG_KVM_BOOKE_HV
> -	vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;
> +	vcpu->arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
> 	vcpu->arch.shadow_pid = 1;
> 	vcpu->arch.shared->msr = 0;
> #endif
> @@ -1529,12 +1629,6 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
> 	return r;
> }
> 
> -int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> -					 struct kvm_guest_debug *dbg)
> -{
> -	return -EINVAL;
> -}
> -
> int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
> {
> 	return -ENOTSUPP;
> @@ -1640,16 +1734,128 @@ void kvmppc_decrementer_func(unsigned long data)
> 	kvmppc_set_tsr_bits(vcpu, TSR_DIS);
> }
> 
> +static void kvmppc_booke_vcpu_put_debug_regs(struct kvm_vcpu *vcpu)
> +{
> +	/* Disable all debug events First */

first

> +	mtspr(SPRN_DBCR0, 0x0);
> +	/* Disable pending debug event by clearing DBSR */
> +	kvmppc_clear_dbsr();

kvmppc_handle_debug() happens with preemption enabled, no? So we can have a debug event that gets cleared on preempt by this.

> +}
> +
> +int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> +					 struct kvm_guest_debug *dbg)
> +{
> +	struct kvmppc_booke_debug_reg *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
> +	};
> +
> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> +		/* Clear All debug events */
> +		vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> +		vcpu->guest_debug = 0;
> +#ifdef CONFIG_KVM_BOOKE_HV
> +		/*
> +		 * When user space is not using the debug resources
> +		 * then allow guest to change the MSR.DE.
> +		 */
> +		vcpu->arch.shadow_msrp &= ~MSRP_DEP;
> +#endif
> +		return 0;
> +	}
> +
> +#ifdef CONFIG_KVM_BOOKE_HV
> +		/*
> +		 * When user space is using the debug resource then
> +		 * do not allow guest to change the MSR.DE.
> +		 */
> +		vcpu->arch.shadow_msrp &= ~MSRP_DEP;

This is supposed to be |= right?

> +#endif
> +	vcpu->guest_debug = dbg->control;
> +	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> +	/* Set DBCR0_EDM in guest visible DBCR0 register. */
> +	vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
> +
> +	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))
> +		/* Code below handles only HW breakpoints */

Please move the comment out of the one-lined branch. Just put it below the return.

> +		return 0;
> +
> +	dbg_reg = &(vcpu->arch.shadow_dbg_reg);
> +
> +	/*
> +	 * On BOOKE (e500v2); Set DBCR1 and DBCR2 to allow debug events
> +	 * to occur when MSR.PR is set.
> +	 * On BOOKE-HV (e500mc+); MSR.PR = 0 when guest is running. So we
> +	 * should clear DBCR1 and DBCR2. And EPCR.DUVD is used to control
> +	 * that debug events will not come in hypervisor (GS = 0).
> +	 */
> +#ifdef CONFIG_KVM_BOOKE_HV
> +	dbg_reg->dbcr1 = 0;
> +	dbg_reg->dbcr2 = 0;
> +#else
> +	dbg_reg->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US | DBCR1_IAC3US |
> +			  DBCR1_IAC4US;
> +	dbg_reg->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US;
> +#endif
> +
> +	for (n = 0; n < (KVMPPC_BOOKE_IAC_NUM + KVMPPC_BOOKE_DAC_NUM); n++) {
> +		u32 type = dbg->arch.bp[n].type;
> +
> +		if (!type)
> +			continue;

Scott's comment on why a zero-type is different from any other invalid type is still outstanding I think.


Alex

> +
> +		if (type & (KVMPPC_DEBUG_WATCH_READ |
> +			    KVMPPC_DEBUG_WATCH_WRITE)) {
> +			if (w >= KVMPPC_BOOKE_DAC_NUM)
> +				return -EINVAL;
> +
> +			if (type & KVMPPC_DEBUG_WATCH_READ)
> +				dbg_reg->dbcr0 |= wp_code[w + 2];
> +			if (type & KVMPPC_DEBUG_WATCH_WRITE)
> +				dbg_reg->dbcr0 |= wp_code[w];
> +
> +			dbg_reg->dac[w] = dbg->arch.bp[n].addr;
> +			w++;
> +		} else if (type & KVMPPC_DEBUG_BREAKPOINT) {
> +			if (b >= KVMPPC_BOOKE_IAC_NUM)
> +				return -EINVAL;
> +
> +			dbg_reg->dbcr0 |= bp_code[b];
> +			dbg_reg->iac[b] = dbg->arch.bp[n].addr;
> +			b++;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> void kvmppc_booke_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> {
> 	vcpu->cpu = smp_processor_id();
> 	current->thread.kvm_vcpu = vcpu;
> +
> +	kvmppc_booke_vcpu_load_debug_regs(vcpu);
> }
> 
> void kvmppc_booke_vcpu_put(struct kvm_vcpu *vcpu)
> {
> 	current->thread.kvm_vcpu = NULL;
> 	vcpu->cpu = -1;
> +
> +	kvmppc_booke_vcpu_put_debug_regs(vcpu);
> }
> 
> int __init kvmppc_booke_init(void)
> diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h
> index 5fd1ba6..a1ff67d 100644
> --- a/arch/powerpc/kvm/booke.h
> +++ b/arch/powerpc/kvm/booke.h
> @@ -129,4 +129,9 @@ static inline void kvmppc_save_guest_fp(struct kvm_vcpu *vcpu)
> 		giveup_fpu(current);
> #endif
> }
> +
> +static inline void kvmppc_clear_dbsr(void)
> +{
> +	mtspr(SPRN_DBSR, mfspr(SPRN_DBSR));
> +}
> #endif /* __KVM_BOOKE_H__ */
> -- 
> 1.7.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Friday, April 26, 2013 4:46 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support
> 
> 
> On 08.04.2013, at 12:32, Bharat Bhushan wrote:
> 
> > From: Bharat Bhushan <bharat.bhushan@freescale.com>
> >
> > This patch adds the debug stub support on booke/bookehv.
> > Now QEMU debug stub can use hw breakpoint, watchpoint and software
> > breakpoint to debug guest.
> >
> > Debug registers are saved/restored on vcpu_put()/vcpu_get().
> > Also the debug registers are saved restored only if guest
> > is using debug resources.
> >
> > Currently we do not support debug resource emulation to guest,
> > so always exit to user space irrespective of user space is expecting
> > the debug exception or not. This is unexpected event and let us
> > leave the action on user space. This is similar to what it was before,
> > only thing is that now we have proper exit state available to user space.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> > arch/powerpc/include/asm/kvm_host.h |    8 +
> > arch/powerpc/include/uapi/asm/kvm.h |   22 +++-
> > arch/powerpc/kvm/booke.c            |  242 ++++++++++++++++++++++++++++++++---
> > arch/powerpc/kvm/booke.h            |    5 +
> > 4 files changed, 255 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_host.h
> b/arch/powerpc/include/asm/kvm_host.h
> > index e34f8fe..b9ad20f 100644
> > --- a/arch/powerpc/include/asm/kvm_host.h
> > +++ b/arch/powerpc/include/asm/kvm_host.h
> > @@ -505,7 +505,15 @@ struct kvm_vcpu_arch {
> > 	u32 mmucfg;
> > 	u32 epr;
> > 	u32 crit_save;
> > +
> > +	/* Flag indicating that debug registers are used by guest */
> > +	bool debug_active;
> > +	/* for save/restore thread->dbcr0 on vcpu run/heavyweight_exit */
> > +	u32 saved_dbcr0;
> > +	/* guest debug registers*/
> > 	struct kvmppc_booke_debug_reg dbg_reg;
> > +	/* shadow debug registers */
> > +	struct kvmppc_booke_debug_reg shadow_dbg_reg;
> > #endif
> > 	gpa_t paddr_accessed;
> > 	gva_t vaddr_accessed;
> > diff --git a/arch/powerpc/include/uapi/asm/kvm.h
> b/arch/powerpc/include/uapi/asm/kvm.h
> > index c0c38ed..d7ce449 100644
> > --- a/arch/powerpc/include/uapi/asm/kvm.h
> > +++ b/arch/powerpc/include/uapi/asm/kvm.h
> > @@ -25,6 +25,7 @@
> > /* Select powerpc specific features in <linux/kvm.h> */
> > #define __KVM_HAVE_SPAPR_TCE
> > #define __KVM_HAVE_PPC_SMT
> > +#define __KVM_HAVE_GUEST_DEBUG
> >
> > struct kvm_regs {
> > 	__u64 pc;
> > @@ -267,7 +268,24 @@ struct kvm_fpu {
> > 	__u64 fpr[32];
> > };
> >
> > +/*
> > + * Defines for h/w breakpoint, watchpoint (read, write or both) and
> > + * software breakpoint.
> > + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
> > + * for KVM_DEBUG_EXIT.
> > + */
> > +#define KVMPPC_DEBUG_NONE		0x0
> > +#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
> > +#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
> > +#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
> > struct kvm_debug_exit_arch {
> > +	__u64 address;
> > +	/*
> > +	 * exiting to userspace because of h/w breakpoint, watchpoint
> > +	 * (read, write or both) and software breakpoint.
> > +	 */
> > +	__u32 status;
> > +	__u32 reserved;
> > };
> >
> > /* for KVM_SET_GUEST_DEBUG */
> > @@ -279,10 +297,6 @@ struct kvm_guest_debug_arch {
> > 		 * Type denotes h/w breakpoint, read watchpoint, write
> > 		 * watchpoint or watchpoint (both read and write).
> > 		 */
> > -#define KVMPPC_DEBUG_NONE		0x0
> > -#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
> > -#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
> > -#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
> > 		__u32 type;
> > 		__u32 reserved;
> > 	} bp[16];
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index 97ae158..0e93416 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
> > #endif
> > }
> >
> > +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
> > +{
> > +	/* Synchronize guest's desire to get debug interrupts into shadow MSR */
> > +#ifndef CONFIG_KVM_BOOKE_HV
> > +	vcpu->arch.shadow_msr &= ~MSR_DE;
> > +	vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE;
> > +#endif
> > +
> > +	/* Force enable debug interrupts when user space wants to debug */
> > +	if (vcpu->guest_debug) {
> > +#ifdef CONFIG_KVM_BOOKE_HV
> > +		/*
> > +		 * Since there is no shadow MSR, sync MSR_DE into the guest
> > +		 * visible MSR.
> > +		 */
> > +		vcpu->arch.shared->msr |= MSR_DE;
> > +#else
> > +		vcpu->arch.shadow_msr |= MSR_DE;
> > +		vcpu->arch.shared->msr &= ~MSR_DE;
> > +#endif
> > +	}
> > +}
> > +
> > /*
> >  * Helper function for "full" MSR writes.  No need to call this if only
> >  * EE/CE/ME/DE/RI are changing.
> > @@ -150,6 +173,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
> > 	kvmppc_mmu_msr_notify(vcpu, old_msr);
> > 	kvmppc_vcpu_sync_spe(vcpu);
> > 	kvmppc_vcpu_sync_fpu(vcpu);
> > +	kvmppc_vcpu_sync_debug(vcpu);
> > }
> >
> > static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,
> > @@ -646,6 +670,46 @@ int kvmppc_core_check_requests(struct kvm_vcpu *vcpu)
> > 	return r;
> > }
> >
> > +static void kvmppc_load_usespace_gebug(void)
> 
> ...

Please tell What does "..."  mean. Does that mean next comment is about the above function?

> 
> > +{
> > +	switch_booke_debug_regs(&current->thread);
> > +}
> > +
> > +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu *vcpu)
> > +{
> > +	if (!vcpu->arch.debug_active)
> > +		return;
> > +
> > +	/* Disable all debug events and clead pending debug events */
> > +	mtspr(SPRN_DBCR0, 0x0);
> > +	kvmppc_clear_dbsr();
> > +
> > +	/*
> > +	 * Check whether guest still need debug resource, if not then there
> > +	 * is no need to restore guest context.
> > +	 */
> > +	if (!vcpu->arch.shadow_dbg_reg.dbcr0)
> > +		return;
> > +
> > +	/* Load Guest Context */
> > +	mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
> > +	mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2);
> > +#ifdef CONFIG_KVM_E500MC
> > +	mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
> 
> You need to make sure DBCR4 is 0 when you leave things back to normal user
> space. Otherwise guest debug can interfere with host debug.


ok

> 
> > +#endif
> > +	mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
> > +	mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
> > +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> > +	mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
> > +	mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
> > +#endif
> > +	mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
> > +	mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
> > +
> > +	/* Enable debug events after other debug registers restored */
> > +	mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0);
> > +}
> 
> All of the code above looks suspiciously similar to prime_debug_regs();. Can't
> we somehow reuse that?

I think we can if
 - Save thread->debug_regs in local data structure 
 - Load vcpu->arch->debug_regs in thread->debug_regs
 - Call prime_debug_regs();
 - Restore thread->debug_regs from local save values in first step

> 
> > +
> > int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> > {
> > 	int ret, s;
> > @@ -693,11 +757,25 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
> kvm_vcpu *vcpu)
> > 	kvmppc_load_guest_fp(vcpu);
> > #endif
> >
> > +	/*
> > +	 * Clear current->thread.dbcr0 so that kernel does not
> > +	 * restore h/w registers on context switch in vcpu running state.
> > +	 */
> > +	vcpu->arch.debug_active = 1;
> 
> = true;

Ok

> 
> > +	vcpu->arch.saved_dbcr0 = current->thread.dbcr0;
> > +	current->thread.dbcr0 = 0;
> > +	kvmppc_booke_vcpu_load_debug_regs(vcpu);
> 
> static void switch_booke_debug_regs(struct thread_struct *new_thread)
> {
>         if ((current->thread.dbcr0 & DBCR0_IDM)
>                 || (new_thread->dbcr0 & DBCR0_IDM))
>                         prime_debug_regs(new_thread);
> }
> 
> The kernel will also restore debug state if the process we come from has
> debugging enabled. Please adjust the comment accordingly.

kvmppc_booke_vcpu_load_debug_regs(vcpu); cleares DBSR and DBCR0, even if previous process have used debug registers. Is not that sufficient? I do not think we should load all other registers.

> 
> > +
> > 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
> >
> > 	/* No need for kvm_guest_exit. It's done in handle_exit.
> > 	   We also get here with interrupts enabled. */
> >
> > +	/* Restore thread->dbcr0 */
> > +	vcpu->arch.debug_active = 0;
> > +	current->thread.dbcr0 = vcpu->arch.saved_dbcr0;
> > +	kvmppc_load_usespace_gebug();
> > +
> > #ifdef CONFIG_PPC_FPU
> > 	kvmppc_save_guest_fp(vcpu);
> >
> > @@ -753,6 +831,36 @@ static int emulation_exit(struct kvm_run *run, struct
> kvm_vcpu *vcpu)
> > 	}
> > }
> >
> > +/*
> > + * Currently we do not support debug resource emulation to guest,
> > + * so always exit to user space irrespective of user space is
> > + * expecting the debug exception or not. This is unexpected event
> > + * and let us leave the action on user space.
> > + */
> > +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu)
> > +{
> > +	u32 dbsr = mfspr(SPRN_DBSR);
> > +
> > +	kvmppc_clear_dbsr();
> > +	run->debug.arch.status = 0;
> > +	run->debug.arch.address = vcpu->arch.pc;
> > +
> > +	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.address = vcpu->arch.shadow_dbg_reg.dac[0];
> > +		else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
> > +			run->debug.arch.address = vcpu->arch.shadow_dbg_reg.dac[1];
> > +	}
> > +
> > +	return RESUME_HOST;
> > +}
> > +
> > static void kvmppc_fill_pt_regs(struct pt_regs *regs)
> > {
> > 	ulong r1, ip, msr, lr;
> > @@ -1112,18 +1220,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
> kvm_vcpu *vcpu,
> > 	}
> >
> > 	case BOOKE_INTERRUPT_DEBUG: {
> > -		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->exit_reason = KVM_EXIT_DEBUG;
> > +		r = kvmppc_handle_debug(run, vcpu);
> > +		if (r == RESUME_HOST)
> > +			run->exit_reason = KVM_EXIT_DEBUG;
> > 		kvmppc_account_exit(vcpu, DEBUG_EXITS);
> > -		r = RESUME_HOST;
> > 		break;
> > 	}
> >
> > @@ -1174,7 +1274,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
> > 	kvmppc_set_msr(vcpu, 0);
> >
> > #ifndef CONFIG_KVM_BOOKE_HV
> > -	vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;
> > +	vcpu->arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
> > 	vcpu->arch.shadow_pid = 1;
> > 	vcpu->arch.shared->msr = 0;
> > #endif
> > @@ -1529,12 +1629,6 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
> struct kvm_one_reg *reg)
> > 	return r;
> > }
> >
> > -int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > -					 struct kvm_guest_debug *dbg)
> > -{
> > -	return -EINVAL;
> > -}
> > -
> > int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
> > {
> > 	return -ENOTSUPP;
> > @@ -1640,16 +1734,128 @@ void kvmppc_decrementer_func(unsigned long data)
> > 	kvmppc_set_tsr_bits(vcpu, TSR_DIS);
> > }
> >
> > +static void kvmppc_booke_vcpu_put_debug_regs(struct kvm_vcpu *vcpu)
> > +{
> > +	/* Disable all debug events First */
> 
> first
> 
> > +	mtspr(SPRN_DBCR0, 0x0);
> > +	/* Disable pending debug event by clearing DBSR */
> > +	kvmppc_clear_dbsr();
> 
> kvmppc_handle_debug() happens with preemption enabled, no?

Want to clarify,  preemption will be enabled on calling local_irq_enable(); in kvmppc_handle_exit()?

> So we can have a
> debug event that gets cleared on preempt by this.

Should we read the DBSR in before local_irq_enable() in kvmppc_handle_exit()?

> 
> > +}
> > +
> > +int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > +					 struct kvm_guest_debug *dbg)
> > +{
> > +	struct kvmppc_booke_debug_reg *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
> > +	};
> > +
> > +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> > +		/* Clear All debug events */
> > +		vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> > +		vcpu->guest_debug = 0;
> > +#ifdef CONFIG_KVM_BOOKE_HV
> > +		/*
> > +		 * When user space is not using the debug resources
> > +		 * then allow guest to change the MSR.DE.
> > +		 */
> > +		vcpu->arch.shadow_msrp &= ~MSRP_DEP;
> > +#endif
> > +		return 0;
> > +	}
> > +
> > +#ifdef CONFIG_KVM_BOOKE_HV
> > +		/*
> > +		 * When user space is using the debug resource then
> > +		 * do not allow guest to change the MSR.DE.
> > +		 */
> > +		vcpu->arch.shadow_msrp &= ~MSRP_DEP;
> 
> This is supposed to be |= right?

Yes :-)

> 
> > +#endif
> > +	vcpu->guest_debug = dbg->control;
> > +	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> > +	/* Set DBCR0_EDM in guest visible DBCR0 register. */
> > +	vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
> > +
> > +	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))
> > +		/* Code below handles only HW breakpoints */
> 
> Please move the comment out of the one-lined branch. Just put it below the
> return.

Ok

> 
> > +		return 0;
> > +
> > +	dbg_reg = &(vcpu->arch.shadow_dbg_reg);
> > +
> > +	/*
> > +	 * On BOOKE (e500v2); Set DBCR1 and DBCR2 to allow debug events
> > +	 * to occur when MSR.PR is set.
> > +	 * On BOOKE-HV (e500mc+); MSR.PR = 0 when guest is running. So we
> > +	 * should clear DBCR1 and DBCR2. And EPCR.DUVD is used to control
> > +	 * that debug events will not come in hypervisor (GS = 0).
> > +	 */
> > +#ifdef CONFIG_KVM_BOOKE_HV
> > +	dbg_reg->dbcr1 = 0;
> > +	dbg_reg->dbcr2 = 0;
> > +#else
> > +	dbg_reg->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US | DBCR1_IAC3US |
> > +			  DBCR1_IAC4US;
> > +	dbg_reg->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US;
> > +#endif
> > +
> > +	for (n = 0; n < (KVMPPC_BOOKE_IAC_NUM + KVMPPC_BOOKE_DAC_NUM); n++) {
> > +		u32 type = dbg->arch.bp[n].type;
> > +
> > +		if (!type)
> > +			continue;
> 
> Scott's comment on why a zero-type is different from any other invalid type is
> still outstanding I think.

Userspce does following 
 - dbg->arch.bp[] is first zero initialized. So dbg->arch.bp[n].type is 0 (KVMPPC_DEBUG_NONE)
 - Then set following for valid breakpoints/watchpoints:
   -- dbg->arch.bp[n].type (KVMPPC_DEBUG_BREAKPOINT or KVMPPC_DEBUG_WATCH_WRITE or KVMPPC_DEBUG_WATCH_READ)
   -- dbg->arch.bp[n].addr

I tried to avoid loop when type is 0 (probably saying type == KVMPPC_DEBUG_NONE should be more clear).
if type is invalid then should we return -EINVAL.

-Bharat

> 
> 
> Alex
> 
> > +
> > +		if (type & (KVMPPC_DEBUG_WATCH_READ |
> > +			    KVMPPC_DEBUG_WATCH_WRITE)) {
> > +			if (w >= KVMPPC_BOOKE_DAC_NUM)
> > +				return -EINVAL;
> > +
> > +			if (type & KVMPPC_DEBUG_WATCH_READ)
> > +				dbg_reg->dbcr0 |= wp_code[w + 2];
> > +			if (type & KVMPPC_DEBUG_WATCH_WRITE)
> > +				dbg_reg->dbcr0 |= wp_code[w];
> > +
> > +			dbg_reg->dac[w] = dbg->arch.bp[n].addr;
> > +			w++;
> > +		} else if (type & KVMPPC_DEBUG_BREAKPOINT) {
> > +			if (b >= KVMPPC_BOOKE_IAC_NUM)
> > +				return -EINVAL;
> > +
> > +			dbg_reg->dbcr0 |= bp_code[b];
> > +			dbg_reg->iac[b] = dbg->arch.bp[n].addr;
> > +			b++;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > void kvmppc_booke_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > {
> > 	vcpu->cpu = smp_processor_id();
> > 	current->thread.kvm_vcpu = vcpu;
> > +
> > +	kvmppc_booke_vcpu_load_debug_regs(vcpu);
> > }
> >
> > void kvmppc_booke_vcpu_put(struct kvm_vcpu *vcpu)
> > {
> > 	current->thread.kvm_vcpu = NULL;
> > 	vcpu->cpu = -1;
> > +
> > +	kvmppc_booke_vcpu_put_debug_regs(vcpu);
> > }
> >
> > int __init kvmppc_booke_init(void)
> > diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h
> > index 5fd1ba6..a1ff67d 100644
> > --- a/arch/powerpc/kvm/booke.h
> > +++ b/arch/powerpc/kvm/booke.h
> > @@ -129,4 +129,9 @@ static inline void kvmppc_save_guest_fp(struct kvm_vcpu
> *vcpu)
> > 		giveup_fpu(current);
> > #endif
> > }
> > +
> > +static inline void kvmppc_clear_dbsr(void)
> > +{
> > +	mtspr(SPRN_DBSR, mfspr(SPRN_DBSR));
> > +}
> > #endif /* __KVM_BOOKE_H__ */
> > --
> > 1.7.0.4
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Alexander Graf]

On 02.05.2013, at 11:46, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Friday, April 26, 2013 4:46 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
>> Bharat-R65777
>> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support
>> 
>> 
>> On 08.04.2013, at 12:32, Bharat Bhushan wrote:
>> 
>>> From: Bharat Bhushan <bharat.bhushan@freescale.com>
>>> 
>>> This patch adds the debug stub support on booke/bookehv.
>>> Now QEMU debug stub can use hw breakpoint, watchpoint and software
>>> breakpoint to debug guest.
>>> 
>>> Debug registers are saved/restored on vcpu_put()/vcpu_get().
>>> Also the debug registers are saved restored only if guest
>>> is using debug resources.
>>> 
>>> Currently we do not support debug resource emulation to guest,
>>> so always exit to user space irrespective of user space is expecting
>>> the debug exception or not. This is unexpected event and let us
>>> leave the action on user space. This is similar to what it was before,
>>> only thing is that now we have proper exit state available to user space.
>>> 
>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>> ---
>>> arch/powerpc/include/asm/kvm_host.h |    8 +
>>> arch/powerpc/include/uapi/asm/kvm.h |   22 +++-
>>> arch/powerpc/kvm/booke.c            |  242 ++++++++++++++++++++++++++++++++---
>>> arch/powerpc/kvm/booke.h            |    5 +
>>> 4 files changed, 255 insertions(+), 22 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/include/asm/kvm_host.h
>> b/arch/powerpc/include/asm/kvm_host.h
>>> index e34f8fe..b9ad20f 100644
>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>> @@ -505,7 +505,15 @@ struct kvm_vcpu_arch {
>>> 	u32 mmucfg;
>>> 	u32 epr;
>>> 	u32 crit_save;
>>> +
>>> +	/* Flag indicating that debug registers are used by guest */
>>> +	bool debug_active;
>>> +	/* for save/restore thread->dbcr0 on vcpu run/heavyweight_exit */
>>> +	u32 saved_dbcr0;
>>> +	/* guest debug registers*/
>>> 	struct kvmppc_booke_debug_reg dbg_reg;
>>> +	/* shadow debug registers */
>>> +	struct kvmppc_booke_debug_reg shadow_dbg_reg;
>>> #endif
>>> 	gpa_t paddr_accessed;
>>> 	gva_t vaddr_accessed;
>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
>> b/arch/powerpc/include/uapi/asm/kvm.h
>>> index c0c38ed..d7ce449 100644
>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>> @@ -25,6 +25,7 @@
>>> /* Select powerpc specific features in <linux/kvm.h> */
>>> #define __KVM_HAVE_SPAPR_TCE
>>> #define __KVM_HAVE_PPC_SMT
>>> +#define __KVM_HAVE_GUEST_DEBUG
>>> 
>>> struct kvm_regs {
>>> 	__u64 pc;
>>> @@ -267,7 +268,24 @@ struct kvm_fpu {
>>> 	__u64 fpr[32];
>>> };
>>> 
>>> +/*
>>> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
>>> + * software breakpoint.
>>> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
>>> + * for KVM_DEBUG_EXIT.
>>> + */
>>> +#define KVMPPC_DEBUG_NONE		0x0
>>> +#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
>>> +#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
>>> +#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
>>> struct kvm_debug_exit_arch {
>>> +	__u64 address;
>>> +	/*
>>> +	 * exiting to userspace because of h/w breakpoint, watchpoint
>>> +	 * (read, write or both) and software breakpoint.
>>> +	 */
>>> +	__u32 status;
>>> +	__u32 reserved;
>>> };
>>> 
>>> /* for KVM_SET_GUEST_DEBUG */
>>> @@ -279,10 +297,6 @@ struct kvm_guest_debug_arch {
>>> 		 * Type denotes h/w breakpoint, read watchpoint, write
>>> 		 * watchpoint or watchpoint (both read and write).
>>> 		 */
>>> -#define KVMPPC_DEBUG_NONE		0x0
>>> -#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
>>> -#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
>>> -#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
>>> 		__u32 type;
>>> 		__u32 reserved;
>>> 	} bp[16];
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>> index 97ae158..0e93416 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu)
>>> #endif
>>> }
>>> 
>>> +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
>>> +{
>>> +	/* Synchronize guest's desire to get debug interrupts into shadow MSR */
>>> +#ifndef CONFIG_KVM_BOOKE_HV
>>> +	vcpu->arch.shadow_msr &= ~MSR_DE;
>>> +	vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE;
>>> +#endif
>>> +
>>> +	/* Force enable debug interrupts when user space wants to debug */
>>> +	if (vcpu->guest_debug) {
>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>> +		/*
>>> +		 * Since there is no shadow MSR, sync MSR_DE into the guest
>>> +		 * visible MSR.
>>> +		 */
>>> +		vcpu->arch.shared->msr |= MSR_DE;
>>> +#else
>>> +		vcpu->arch.shadow_msr |= MSR_DE;
>>> +		vcpu->arch.shared->msr &= ~MSR_DE;
>>> +#endif
>>> +	}
>>> +}
>>> +
>>> /*
>>> * Helper function for "full" MSR writes.  No need to call this if only
>>> * EE/CE/ME/DE/RI are changing.
>>> @@ -150,6 +173,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
>>> 	kvmppc_mmu_msr_notify(vcpu, old_msr);
>>> 	kvmppc_vcpu_sync_spe(vcpu);
>>> 	kvmppc_vcpu_sync_fpu(vcpu);
>>> +	kvmppc_vcpu_sync_debug(vcpu);
>>> }
>>> 
>>> static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,
>>> @@ -646,6 +670,46 @@ int kvmppc_core_check_requests(struct kvm_vcpu *vcpu)
>>> 	return r;
>>> }
>>> 
>>> +static void kvmppc_load_usespace_gebug(void)
>> 
>> ...
> 
> Please tell What does "..."  mean. Does that mean next comment is about the above function?

It means "this one is so obvious I don't even have to write anything here". Usespace? Gebug? Seriously? :)

> 
>> 
>>> +{
>>> +	switch_booke_debug_regs(&current->thread);
>>> +}
>>> +
>>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu *vcpu)
>>> +{
>>> +	if (!vcpu->arch.debug_active)
>>> +		return;
>>> +
>>> +	/* Disable all debug events and clead pending debug events */
>>> +	mtspr(SPRN_DBCR0, 0x0);
>>> +	kvmppc_clear_dbsr();
>>> +
>>> +	/*
>>> +	 * Check whether guest still need debug resource, if not then there
>>> +	 * is no need to restore guest context.
>>> +	 */
>>> +	if (!vcpu->arch.shadow_dbg_reg.dbcr0)
>>> +		return;
>>> +
>>> +	/* Load Guest Context */
>>> +	mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
>>> +	mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2);
>>> +#ifdef CONFIG_KVM_E500MC
>>> +	mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
>> 
>> You need to make sure DBCR4 is 0 when you leave things back to normal user
>> space. Otherwise guest debug can interfere with host debug.
> 
> 
> ok
> 
>> 
>>> +#endif
>>> +	mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
>>> +	mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
>>> +	mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
>>> +	mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
>>> +#endif
>>> +	mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
>>> +	mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
>>> +
>>> +	/* Enable debug events after other debug registers restored */
>>> +	mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0);
>>> +}
>> 
>> All of the code above looks suspiciously similar to prime_debug_regs();. Can't
>> we somehow reuse that?
> 
> I think we can if
> - Save thread->debug_regs in local data structure 

Yes, it can even be on the stack.

> - Load vcpu->arch->debug_regs in thread->debug_regs
> - Call prime_debug_regs();
> - Restore thread->debug_regs from local save values in first step

On heavyweight exit, based on the values on stack, yes.

> 
>> 
>>> +
>>> int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>>> {
>>> 	int ret, s;
>>> @@ -693,11 +757,25 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
>> kvm_vcpu *vcpu)
>>> 	kvmppc_load_guest_fp(vcpu);
>>> #endif
>>> 
>>> +	/*
>>> +	 * Clear current->thread.dbcr0 so that kernel does not
>>> +	 * restore h/w registers on context switch in vcpu running state.
>>> +	 */
>>> +	vcpu->arch.debug_active = 1;
>> 
>> = true;
> 
> Ok
> 
>> 
>>> +	vcpu->arch.saved_dbcr0 = current->thread.dbcr0;
>>> +	current->thread.dbcr0 = 0;
>>> +	kvmppc_booke_vcpu_load_debug_regs(vcpu);
>> 
>> static void switch_booke_debug_regs(struct thread_struct *new_thread)
>> {
>>        if ((current->thread.dbcr0 & DBCR0_IDM)
>>                || (new_thread->dbcr0 & DBCR0_IDM))
>>                        prime_debug_regs(new_thread);
>> }
>> 
>> The kernel will also restore debug state if the process we come from has
>> debugging enabled. Please adjust the comment accordingly.
> 
> kvmppc_booke_vcpu_load_debug_regs(vcpu); cleares DBSR and DBCR0, even if previous process have used debug registers. Is not that sufficient? I do not think we should load all other registers.

It's sufficient, but the comment is wrong.

> 
>> 
>>> +
>>> 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
>>> 
>>> 	/* No need for kvm_guest_exit. It's done in handle_exit.
>>> 	   We also get here with interrupts enabled. */
>>> 
>>> +	/* Restore thread->dbcr0 */
>>> +	vcpu->arch.debug_active = 0;
>>> +	current->thread.dbcr0 = vcpu->arch.saved_dbcr0;
>>> +	kvmppc_load_usespace_gebug();
>>> +
>>> #ifdef CONFIG_PPC_FPU
>>> 	kvmppc_save_guest_fp(vcpu);
>>> 
>>> @@ -753,6 +831,36 @@ static int emulation_exit(struct kvm_run *run, struct
>> kvm_vcpu *vcpu)
>>> 	}
>>> }
>>> 
>>> +/*
>>> + * Currently we do not support debug resource emulation to guest,
>>> + * so always exit to user space irrespective of user space is
>>> + * expecting the debug exception or not. This is unexpected event
>>> + * and let us leave the action on user space.
>>> + */
>>> +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu)
>>> +{
>>> +	u32 dbsr = mfspr(SPRN_DBSR);
>>> +
>>> +	kvmppc_clear_dbsr();
>>> +	run->debug.arch.status = 0;
>>> +	run->debug.arch.address = vcpu->arch.pc;
>>> +
>>> +	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.address = vcpu->arch.shadow_dbg_reg.dac[0];
>>> +		else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
>>> +			run->debug.arch.address = vcpu->arch.shadow_dbg_reg.dac[1];
>>> +	}
>>> +
>>> +	return RESUME_HOST;
>>> +}
>>> +
>>> static void kvmppc_fill_pt_regs(struct pt_regs *regs)
>>> {
>>> 	ulong r1, ip, msr, lr;
>>> @@ -1112,18 +1220,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
>> kvm_vcpu *vcpu,
>>> 	}
>>> 
>>> 	case BOOKE_INTERRUPT_DEBUG: {
>>> -		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->exit_reason = KVM_EXIT_DEBUG;
>>> +		r = kvmppc_handle_debug(run, vcpu);
>>> +		if (r == RESUME_HOST)
>>> +			run->exit_reason = KVM_EXIT_DEBUG;
>>> 		kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>> -		r = RESUME_HOST;
>>> 		break;
>>> 	}
>>> 
>>> @@ -1174,7 +1274,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>>> 	kvmppc_set_msr(vcpu, 0);
>>> 
>>> #ifndef CONFIG_KVM_BOOKE_HV
>>> -	vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;
>>> +	vcpu->arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
>>> 	vcpu->arch.shadow_pid = 1;
>>> 	vcpu->arch.shared->msr = 0;
>>> #endif
>>> @@ -1529,12 +1629,6 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
>> struct kvm_one_reg *reg)
>>> 	return r;
>>> }
>>> 
>>> -int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>> -					 struct kvm_guest_debug *dbg)
>>> -{
>>> -	return -EINVAL;
>>> -}
>>> -
>>> int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
>>> {
>>> 	return -ENOTSUPP;
>>> @@ -1640,16 +1734,128 @@ void kvmppc_decrementer_func(unsigned long data)
>>> 	kvmppc_set_tsr_bits(vcpu, TSR_DIS);
>>> }
>>> 
>>> +static void kvmppc_booke_vcpu_put_debug_regs(struct kvm_vcpu *vcpu)
>>> +{
>>> +	/* Disable all debug events First */
>> 
>> first
>> 
>>> +	mtspr(SPRN_DBCR0, 0x0);
>>> +	/* Disable pending debug event by clearing DBSR */
>>> +	kvmppc_clear_dbsr();
>> 
>> kvmppc_handle_debug() happens with preemption enabled, no?
> 
> Want to clarify,  preemption will be enabled on calling local_irq_enable(); in kvmppc_handle_exit()?

Yes. Implicitly :).

> 
>> So we can have a
>> debug event that gets cleared on preempt by this.
> 
> Should we read the DBSR in before local_irq_enable() in kvmppc_handle_exit()?

We have to, yes. Otherwise we could get preempted in between and get bogus data I presume.

> 
>> 
>>> +}
>>> +
>>> +int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>> +					 struct kvm_guest_debug *dbg)
>>> +{
>>> +	struct kvmppc_booke_debug_reg *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
>>> +	};
>>> +
>>> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
>>> +		/* Clear All debug events */
>>> +		vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
>>> +		vcpu->guest_debug = 0;
>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>> +		/*
>>> +		 * When user space is not using the debug resources
>>> +		 * then allow guest to change the MSR.DE.
>>> +		 */
>>> +		vcpu->arch.shadow_msrp &= ~MSRP_DEP;
>>> +#endif
>>> +		return 0;
>>> +	}
>>> +
>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>> +		/*
>>> +		 * When user space is using the debug resource then
>>> +		 * do not allow guest to change the MSR.DE.
>>> +		 */
>>> +		vcpu->arch.shadow_msrp &= ~MSRP_DEP;
>> 
>> This is supposed to be |= right?
> 
> Yes :-)
> 
>> 
>>> +#endif
>>> +	vcpu->guest_debug = dbg->control;
>>> +	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
>>> +	/* Set DBCR0_EDM in guest visible DBCR0 register. */
>>> +	vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
>>> +
>>> +	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))
>>> +		/* Code below handles only HW breakpoints */
>> 
>> Please move the comment out of the one-lined branch. Just put it below the
>> return.
> 
> Ok
> 
>> 
>>> +		return 0;
>>> +
>>> +	dbg_reg = &(vcpu->arch.shadow_dbg_reg);
>>> +
>>> +	/*
>>> +	 * On BOOKE (e500v2); Set DBCR1 and DBCR2 to allow debug events
>>> +	 * to occur when MSR.PR is set.
>>> +	 * On BOOKE-HV (e500mc+); MSR.PR = 0 when guest is running. So we
>>> +	 * should clear DBCR1 and DBCR2. And EPCR.DUVD is used to control
>>> +	 * that debug events will not come in hypervisor (GS = 0).
>>> +	 */
>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>> +	dbg_reg->dbcr1 = 0;
>>> +	dbg_reg->dbcr2 = 0;
>>> +#else
>>> +	dbg_reg->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US | DBCR1_IAC3US |
>>> +			  DBCR1_IAC4US;
>>> +	dbg_reg->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US;
>>> +#endif
>>> +
>>> +	for (n = 0; n < (KVMPPC_BOOKE_IAC_NUM + KVMPPC_BOOKE_DAC_NUM); n++) {
>>> +		u32 type = dbg->arch.bp[n].type;
>>> +
>>> +		if (!type)
>>> +			continue;
>> 
>> Scott's comment on why a zero-type is different from any other invalid type is
>> still outstanding I think.
> 
> Userspce does following 
> - dbg->arch.bp[] is first zero initialized. So dbg->arch.bp[n].type is 0 (KVMPPC_DEBUG_NONE)
> - Then set following for valid breakpoints/watchpoints:
>   -- dbg->arch.bp[n].type (KVMPPC_DEBUG_BREAKPOINT or KVMPPC_DEBUG_WATCH_WRITE or KVMPPC_DEBUG_WATCH_READ)
>   -- dbg->arch.bp[n].addr
> 
> I tried to avoid loop when type is 0 (probably saying type == KVMPPC_DEBUG_NONE should be more clear).

That works, yes.

> if type is invalid then should we return -EINVAL.

Yes.


Alex

RE: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, May 02, 2013 4:35 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421
> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support
> 
> 
> On 02.05.2013, at 11:46, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Friday, April 26, 2013 4:46 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421;
> >> Bhushan
> >> Bharat-R65777
> >> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub
> >> support
> >>
> >>
> >> On 08.04.2013, at 12:32, Bharat Bhushan wrote:
> >>
> >>> From: Bharat Bhushan <bharat.bhushan@freescale.com>
> >>>
> >>> This patch adds the debug stub support on booke/bookehv.
> >>> Now QEMU debug stub can use hw breakpoint, watchpoint and software
> >>> breakpoint to debug guest.
> >>>
> >>> Debug registers are saved/restored on vcpu_put()/vcpu_get().
> >>> Also the debug registers are saved restored only if guest is using
> >>> debug resources.
> >>>
> >>> Currently we do not support debug resource emulation to guest, so
> >>> always exit to user space irrespective of user space is expecting
> >>> the debug exception or not. This is unexpected event and let us
> >>> leave the action on user space. This is similar to what it was
> >>> before, only thing is that now we have proper exit state available to user
> space.
> >>>
> >>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> >>> ---
> >>> arch/powerpc/include/asm/kvm_host.h |    8 +
> >>> arch/powerpc/include/uapi/asm/kvm.h |   22 +++-
> >>> arch/powerpc/kvm/booke.c            |  242 ++++++++++++++++++++++++++++++++-
> --
> >>> arch/powerpc/kvm/booke.h            |    5 +
> >>> 4 files changed, 255 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/include/asm/kvm_host.h
> >> b/arch/powerpc/include/asm/kvm_host.h
> >>> index e34f8fe..b9ad20f 100644
> >>> --- a/arch/powerpc/include/asm/kvm_host.h
> >>> +++ b/arch/powerpc/include/asm/kvm_host.h
> >>> @@ -505,7 +505,15 @@ struct kvm_vcpu_arch {
> >>> 	u32 mmucfg;
> >>> 	u32 epr;
> >>> 	u32 crit_save;
> >>> +
> >>> +	/* Flag indicating that debug registers are used by guest */
> >>> +	bool debug_active;
> >>> +	/* for save/restore thread->dbcr0 on vcpu run/heavyweight_exit */
> >>> +	u32 saved_dbcr0;
> >>> +	/* guest debug registers*/
> >>> 	struct kvmppc_booke_debug_reg dbg_reg;
> >>> +	/* shadow debug registers */
> >>> +	struct kvmppc_booke_debug_reg shadow_dbg_reg;
> >>> #endif
> >>> 	gpa_t paddr_accessed;
> >>> 	gva_t vaddr_accessed;
> >>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
> >> b/arch/powerpc/include/uapi/asm/kvm.h
> >>> index c0c38ed..d7ce449 100644
> >>> --- a/arch/powerpc/include/uapi/asm/kvm.h
> >>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> >>> @@ -25,6 +25,7 @@
> >>> /* Select powerpc specific features in <linux/kvm.h> */ #define
> >>> __KVM_HAVE_SPAPR_TCE #define __KVM_HAVE_PPC_SMT
> >>> +#define __KVM_HAVE_GUEST_DEBUG
> >>>
> >>> struct kvm_regs {
> >>> 	__u64 pc;
> >>> @@ -267,7 +268,24 @@ struct kvm_fpu {
> >>> 	__u64 fpr[32];
> >>> };
> >>>
> >>> +/*
> >>> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
> >>> + * software breakpoint.
> >>> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
> >>> + * for KVM_DEBUG_EXIT.
> >>> + */
> >>> +#define KVMPPC_DEBUG_NONE		0x0
> >>> +#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
> >>> +#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
> >>> +#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
> >>> struct kvm_debug_exit_arch {
> >>> +	__u64 address;
> >>> +	/*
> >>> +	 * exiting to userspace because of h/w breakpoint, watchpoint
> >>> +	 * (read, write or both) and software breakpoint.
> >>> +	 */
> >>> +	__u32 status;
> >>> +	__u32 reserved;
> >>> };
> >>>
> >>> /* for KVM_SET_GUEST_DEBUG */
> >>> @@ -279,10 +297,6 @@ struct kvm_guest_debug_arch {
> >>> 		 * Type denotes h/w breakpoint, read watchpoint, write
> >>> 		 * watchpoint or watchpoint (both read and write).
> >>> 		 */
> >>> -#define KVMPPC_DEBUG_NONE		0x0
> >>> -#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
> >>> -#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
> >>> -#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
> >>> 		__u32 type;
> >>> 		__u32 reserved;
> >>> 	} bp[16];
> >>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> >>> index 97ae158..0e93416 100644
> >>> --- a/arch/powerpc/kvm/booke.c
> >>> +++ b/arch/powerpc/kvm/booke.c
> >>> @@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct
> >>> kvm_vcpu *vcpu) #endif }
> >>>
> >>> +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) {
> >>> +	/* Synchronize guest's desire to get debug interrupts into shadow
> >>> +MSR */ #ifndef CONFIG_KVM_BOOKE_HV
> >>> +	vcpu->arch.shadow_msr &= ~MSR_DE;
> >>> +	vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; #endif
> >>> +
> >>> +	/* Force enable debug interrupts when user space wants to debug */
> >>> +	if (vcpu->guest_debug) {
> >>> +#ifdef CONFIG_KVM_BOOKE_HV
> >>> +		/*
> >>> +		 * Since there is no shadow MSR, sync MSR_DE into the guest
> >>> +		 * visible MSR.
> >>> +		 */
> >>> +		vcpu->arch.shared->msr |= MSR_DE; #else
> >>> +		vcpu->arch.shadow_msr |= MSR_DE;
> >>> +		vcpu->arch.shared->msr &= ~MSR_DE; #endif
> >>> +	}
> >>> +}
> >>> +
> >>> /*
> >>> * Helper function for "full" MSR writes.  No need to call this if
> >>> only
> >>> * EE/CE/ME/DE/RI are changing.
> >>> @@ -150,6 +173,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr)
> >>> 	kvmppc_mmu_msr_notify(vcpu, old_msr);
> >>> 	kvmppc_vcpu_sync_spe(vcpu);
> >>> 	kvmppc_vcpu_sync_fpu(vcpu);
> >>> +	kvmppc_vcpu_sync_debug(vcpu);
> >>> }
> >>>
> >>> static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu, @@
> >>> -646,6 +670,46 @@ int kvmppc_core_check_requests(struct kvm_vcpu *vcpu)
> >>> 	return r;
> >>> }
> >>>
> >>> +static void kvmppc_load_usespace_gebug(void)
> >>
> >> ...
> >
> > Please tell What does "..."  mean. Does that mean next comment is about the
> above function?
> 
> It means "this one is so obvious I don't even have to write anything here".
> Usespace? Gebug? Seriously? :)

ehhh, kicking myself 

> 
> >
> >>
> >>> +{
> >>> +	switch_booke_debug_regs(&current->thread);
> >>> +}
> >>> +
> >>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu
> >>> +*vcpu) {
> >>> +	if (!vcpu->arch.debug_active)
> >>> +		return;
> >>> +
> >>> +	/* Disable all debug events and clead pending debug events */
> >>> +	mtspr(SPRN_DBCR0, 0x0);
> >>> +	kvmppc_clear_dbsr();
> >>> +
> >>> +	/*
> >>> +	 * Check whether guest still need debug resource, if not then there
> >>> +	 * is no need to restore guest context.
> >>> +	 */
> >>> +	if (!vcpu->arch.shadow_dbg_reg.dbcr0)
> >>> +		return;
> >>> +
> >>> +	/* Load Guest Context */
> >>> +	mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
> >>> +	mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2); #ifdef
> >>> +CONFIG_KVM_E500MC
> >>> +	mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
> >>
> >> You need to make sure DBCR4 is 0 when you leave things back to normal
> >> user space. Otherwise guest debug can interfere with host debug.
> >
> >
> > ok
> >
> >>
> >>> +#endif
> >>> +	mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
> >>> +	mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
> >>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> >>> +	mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
> >>> +	mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
> >>> +#endif
> >>> +	mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
> >>> +	mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
> >>> +
> >>> +	/* Enable debug events after other debug registers restored */
> >>> +	mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0); }
> >>
> >> All of the code above looks suspiciously similar to
> >> prime_debug_regs();. Can't we somehow reuse that?
> >
> > I think we can if
> > - Save thread->debug_regs in local data structure
> 
> Yes, it can even be on the stack.

Thread->denug_regs is not struct , so memcpy() will not work, it is assigning all registers one by one.

> 
> > - Load vcpu->arch->debug_regs in thread->debug_regs
> > - Call prime_debug_regs();
> > - Restore thread->debug_regs from local save values in first step
> 
> On heavyweight exit, based on the values on stack, yes.
> 
> >
> >>
> >>> +
> >>> int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> >>> {
> >>> 	int ret, s;
> >>> @@ -693,11 +757,25 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,
> >>> struct
> >> kvm_vcpu *vcpu)
> >>> 	kvmppc_load_guest_fp(vcpu);
> >>> #endif
> >>>
> >>> +	/*
> >>> +	 * Clear current->thread.dbcr0 so that kernel does not
> >>> +	 * restore h/w registers on context switch in vcpu running state.
> >>> +	 */
> >>> +	vcpu->arch.debug_active = 1;
> >>
> >> = true;
> >
> > Ok
> >
> >>
> >>> +	vcpu->arch.saved_dbcr0 = current->thread.dbcr0;
> >>> +	current->thread.dbcr0 = 0;
> >>> +	kvmppc_booke_vcpu_load_debug_regs(vcpu);
> >>
> >> static void switch_booke_debug_regs(struct thread_struct *new_thread)
> >> {
> >>        if ((current->thread.dbcr0 & DBCR0_IDM)
> >>                || (new_thread->dbcr0 & DBCR0_IDM))
> >>                        prime_debug_regs(new_thread); }
> >>
> >> The kernel will also restore debug state if the process we come from
> >> has debugging enabled. Please adjust the comment accordingly.
> >
> > kvmppc_booke_vcpu_load_debug_regs(vcpu); cleares DBSR and DBCR0, even if
> previous process have used debug registers. Is not that sufficient? I do not
> think we should load all other registers.
> 
> It's sufficient, but the comment is wrong.

ok

> 
> >
> >>
> >>> +
> >>> 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
> >>>
> >>> 	/* No need for kvm_guest_exit. It's done in handle_exit.
> >>> 	   We also get here with interrupts enabled. */
> >>>
> >>> +	/* Restore thread->dbcr0 */
> >>> +	vcpu->arch.debug_active = 0;
> >>> +	current->thread.dbcr0 = vcpu->arch.saved_dbcr0;
> >>> +	kvmppc_load_usespace_gebug();
> >>> +
> >>> #ifdef CONFIG_PPC_FPU
> >>> 	kvmppc_save_guest_fp(vcpu);
> >>>
> >>> @@ -753,6 +831,36 @@ static int emulation_exit(struct kvm_run *run,
> >>> struct
> >> kvm_vcpu *vcpu)
> >>> 	}
> >>> }
> >>>
> >>> +/*
> >>> + * Currently we do not support debug resource emulation to guest,
> >>> + * so always exit to user space irrespective of user space is
> >>> + * expecting the debug exception or not. This is unexpected event
> >>> + * and let us leave the action on user space.
> >>> + */
> >>> +static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu
> >>> +*vcpu) {
> >>> +	u32 dbsr = mfspr(SPRN_DBSR);
> >>> +
> >>> +	kvmppc_clear_dbsr();
> >>> +	run->debug.arch.status = 0;
> >>> +	run->debug.arch.address = vcpu->arch.pc;
> >>> +
> >>> +	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.address = vcpu->arch.shadow_dbg_reg.dac[0];
> >>> +		else if (dbsr & (DBSR_DAC2R | DBSR_DAC2W))
> >>> +			run->debug.arch.address = vcpu->arch.shadow_dbg_reg.dac[1];
> >>> +	}
> >>> +
> >>> +	return RESUME_HOST;
> >>> +}
> >>> +
> >>> static void kvmppc_fill_pt_regs(struct pt_regs *regs) {
> >>> 	ulong r1, ip, msr, lr;
> >>> @@ -1112,18 +1220,10 @@ int kvmppc_handle_exit(struct kvm_run *run,
> >>> struct
> >> kvm_vcpu *vcpu,
> >>> 	}
> >>>
> >>> 	case BOOKE_INTERRUPT_DEBUG: {
> >>> -		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->exit_reason = KVM_EXIT_DEBUG;
> >>> +		r = kvmppc_handle_debug(run, vcpu);
> >>> +		if (r == RESUME_HOST)
> >>> +			run->exit_reason = KVM_EXIT_DEBUG;
> >>> 		kvmppc_account_exit(vcpu, DEBUG_EXITS);
> >>> -		r = RESUME_HOST;
> >>> 		break;
> >>> 	}
> >>>
> >>> @@ -1174,7 +1274,7 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
> >>> 	kvmppc_set_msr(vcpu, 0);
> >>>
> >>> #ifndef CONFIG_KVM_BOOKE_HV
> >>> -	vcpu->arch.shadow_msr = MSR_USER | MSR_DE | MSR_IS | MSR_DS;
> >>> +	vcpu->arch.shadow_msr = MSR_USER | MSR_IS | MSR_DS;
> >>> 	vcpu->arch.shadow_pid = 1;
> >>> 	vcpu->arch.shared->msr = 0;
> >>> #endif
> >>> @@ -1529,12 +1629,6 @@ int kvm_vcpu_ioctl_set_one_reg(struct
> >>> kvm_vcpu *vcpu,
> >> struct kvm_one_reg *reg)
> >>> 	return r;
> >>> }
> >>>
> >>> -int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >>> -					 struct kvm_guest_debug *dbg)
> >>> -{
> >>> -	return -EINVAL;
> >>> -}
> >>> -
> >>> int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct
> >>> kvm_fpu *fpu) {
> >>> 	return -ENOTSUPP;
> >>> @@ -1640,16 +1734,128 @@ void kvmppc_decrementer_func(unsigned long data)
> >>> 	kvmppc_set_tsr_bits(vcpu, TSR_DIS); }
> >>>
> >>> +static void kvmppc_booke_vcpu_put_debug_regs(struct kvm_vcpu *vcpu)
> >>> +{
> >>> +	/* Disable all debug events First */
> >>
> >> first
> >>
> >>> +	mtspr(SPRN_DBCR0, 0x0);
> >>> +	/* Disable pending debug event by clearing DBSR */
> >>> +	kvmppc_clear_dbsr();
> >>
> >> kvmppc_handle_debug() happens with preemption enabled, no?
> >
> > Want to clarify,  preemption will be enabled on calling local_irq_enable(); in
> kvmppc_handle_exit()?
> 
> Yes. Implicitly :).
> 
> >
> >> So we can have a
> >> debug event that gets cleared on preempt by this.
> >
> > Should we read the DBSR in before local_irq_enable() in kvmppc_handle_exit()?
> 
> We have to, yes. Otherwise we could get preempted in between and get bogus data
> I presume.

Ok

Thanks
-Bharat

> 
> >
> >>
> >>> +}
> >>> +
> >>> +int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >>> +					 struct kvm_guest_debug *dbg)
> >>> +{
> >>> +	struct kvmppc_booke_debug_reg *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
> >>> +	};
> >>> +
> >>> +	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> >>> +		/* Clear All debug events */
> >>> +		vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> >>> +		vcpu->guest_debug = 0;
> >>> +#ifdef CONFIG_KVM_BOOKE_HV
> >>> +		/*
> >>> +		 * When user space is not using the debug resources
> >>> +		 * then allow guest to change the MSR.DE.
> >>> +		 */
> >>> +		vcpu->arch.shadow_msrp &= ~MSRP_DEP; #endif
> >>> +		return 0;
> >>> +	}
> >>> +
> >>> +#ifdef CONFIG_KVM_BOOKE_HV
> >>> +		/*
> >>> +		 * When user space is using the debug resource then
> >>> +		 * do not allow guest to change the MSR.DE.
> >>> +		 */
> >>> +		vcpu->arch.shadow_msrp &= ~MSRP_DEP;
> >>
> >> This is supposed to be |= right?
> >
> > Yes :-)
> >
> >>
> >>> +#endif
> >>> +	vcpu->guest_debug = dbg->control;
> >>> +	vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> >>> +	/* Set DBCR0_EDM in guest visible DBCR0 register. */
> >>> +	vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
> >>> +
> >>> +	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))
> >>> +		/* Code below handles only HW breakpoints */
> >>
> >> Please move the comment out of the one-lined branch. Just put it
> >> below the return.
> >
> > Ok
> >
> >>
> >>> +		return 0;
> >>> +
> >>> +	dbg_reg = &(vcpu->arch.shadow_dbg_reg);
> >>> +
> >>> +	/*
> >>> +	 * On BOOKE (e500v2); Set DBCR1 and DBCR2 to allow debug events
> >>> +	 * to occur when MSR.PR is set.
> >>> +	 * On BOOKE-HV (e500mc+); MSR.PR = 0 when guest is running. So we
> >>> +	 * should clear DBCR1 and DBCR2. And EPCR.DUVD is used to control
> >>> +	 * that debug events will not come in hypervisor (GS = 0).
> >>> +	 */
> >>> +#ifdef CONFIG_KVM_BOOKE_HV
> >>> +	dbg_reg->dbcr1 = 0;
> >>> +	dbg_reg->dbcr2 = 0;
> >>> +#else
> >>> +	dbg_reg->dbcr1 = DBCR1_IAC1US | DBCR1_IAC2US | DBCR1_IAC3US |
> >>> +			  DBCR1_IAC4US;
> >>> +	dbg_reg->dbcr2 = DBCR2_DAC1US | DBCR2_DAC2US; #endif
> >>> +
> >>> +	for (n = 0; n < (KVMPPC_BOOKE_IAC_NUM + KVMPPC_BOOKE_DAC_NUM); n++) {
> >>> +		u32 type = dbg->arch.bp[n].type;
> >>> +
> >>> +		if (!type)
> >>> +			continue;
> >>
> >> Scott's comment on why a zero-type is different from any other
> >> invalid type is still outstanding I think.
> >
> > Userspce does following
> > - dbg->arch.bp[] is first zero initialized. So dbg->arch.bp[n].type is
> > 0 (KVMPPC_DEBUG_NONE)
> > - Then set following for valid breakpoints/watchpoints:
> >   -- dbg->arch.bp[n].type (KVMPPC_DEBUG_BREAKPOINT or KVMPPC_DEBUG_WATCH_WRITE
> or KVMPPC_DEBUG_WATCH_READ)
> >   -- dbg->arch.bp[n].addr
> >
> > I tried to avoid loop when type is 0 (probably saying type ==
> KVMPPC_DEBUG_NONE should be more clear).
> 
> That works, yes.
> 
> > if type is invalid then should we return -EINVAL.
> 
> Yes.
> 
> 
> Alex
> 

Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Alexander Graf]

On 02.05.2013, at 16:00, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, May 02, 2013 4:35 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421
>> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support
>> 
>> 
>> On 02.05.2013, at 11:46, Bhushan Bharat-R65777 wrote:

[...]

>>> 
>>>> 
>>>>> +#endif
>>>>> +	mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
>>>>> +	mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
>>>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
>>>>> +	mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
>>>>> +	mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
>>>>> +#endif
>>>>> +	mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
>>>>> +	mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
>>>>> +
>>>>> +	/* Enable debug events after other debug registers restored */
>>>>> +	mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0); }
>>>> 
>>>> All of the code above looks suspiciously similar to
>>>> prime_debug_regs();. Can't we somehow reuse that?
>>> 
>>> I think we can if
>>> - Save thread->debug_regs in local data structure
>> 
>> Yes, it can even be on the stack.
> 
> Thread->denug_regs is not struct , so memcpy() will not work, it is assigning all registers one by one.

We could make it a struct, no? Then it's a matter of a = b; :)


Alex

RE: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Bhushan Bharat-R65777]

> >>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu
> >>> +*vcpu) {
> >>> +	if (!vcpu->arch.debug_active)
> >>> +		return;
> >>> +
> >>> +	/* Disable all debug events and clead pending debug events */
> >>> +	mtspr(SPRN_DBCR0, 0x0);
> >>> +	kvmppc_clear_dbsr();
> >>> +
> >>> +	/*
> >>> +	 * Check whether guest still need debug resource, if not then there
> >>> +	 * is no need to restore guest context.
> >>> +	 */
> >>> +	if (!vcpu->arch.shadow_dbg_reg.dbcr0)
> >>> +		return;
> >>> +
> >>> +	/* Load Guest Context */
> >>> +	mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
> >>> +	mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2); #ifdef
> >>> +CONFIG_KVM_E500MC
> >>> +	mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
> >>
> >> You need to make sure DBCR4 is 0 when you leave things back to normal
> >> user space. Otherwise guest debug can interfere with host debug.
> >
> >
> > ok
> >
> >>
> >>> +#endif
> >>> +	mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
> >>> +	mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
> >>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> >>> +	mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
> >>> +	mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
> >>> +#endif
> >>> +	mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
> >>> +	mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
> >>> +
> >>> +	/* Enable debug events after other debug registers restored */
> >>> +	mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0); }
> >>
> >> All of the code above looks suspiciously similar to
> >> prime_debug_regs();. Can't we somehow reuse that?
> >
> > I think we can if
> > - Save thread->debug_regs in local data structure
> 
> Yes, it can even be on the stack.
> 
> > - Load vcpu->arch->debug_regs in thread->debug_regs
> > - Call prime_debug_regs();
> > - Restore thread->debug_regs from local save values in first step
> 
> On heavyweight exit, based on the values on stack, yes.

This is how I think we can save/restore debug context. Please correct if I am missing something.

1) When QEMU is running

-> thread->debug_reg == QEMU debug register context.
-> Kernel will handle switching the debug register on context switch.
-> no vcpu_load() called

2) QEMU makes ioctls (except RUN) 
 -> This will call vcpu_load()
 -> should not change context.
 -> Some ioctls can change vcpu debug register, context saved in vcpu->debug_regs

3) QEMU Makes RUN ioctl
 -> Save thread->debug_reg on STACK
 -> Store thread->debug_reg == vcpu->debug_reg
 -> load thread->debug_reg 
 -> RUN VCPU ( So thread points to vcpu context )

4) Context switch happens When VCPU running
 -> makes vcpu_load() should not load any context
 -> kernel loads the vcpu context as thread->debug_regs points to vcpu context.

5) On heavyweight_exit
 -> Load the context saved on stack in thread->debug_reg

Thanks
-Bharat

Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Alexander Graf]

Am 03.05.2013 um 12:48 schrieb Bhushan Bharat-R65777 <R65777@freescale.com>:

>>>>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu
>>>>> +*vcpu) {
>>>>> +    if (!vcpu->arch.debug_active)
>>>>> +        return;
>>>>> +
>>>>> +    /* Disable all debug events and clead pending debug events */
>>>>> +    mtspr(SPRN_DBCR0, 0x0);
>>>>> +    kvmppc_clear_dbsr();
>>>>> +
>>>>> +    /*
>>>>> +     * Check whether guest still need debug resource, if not then there
>>>>> +     * is no need to restore guest context.
>>>>> +     */
>>>>> +    if (!vcpu->arch.shadow_dbg_reg.dbcr0)
>>>>> +        return;
>>>>> +
>>>>> +    /* Load Guest Context */
>>>>> +    mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
>>>>> +    mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2); #ifdef
>>>>> +CONFIG_KVM_E500MC
>>>>> +    mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
>>>> 
>>>> You need to make sure DBCR4 is 0 when you leave things back to normal
>>>> user space. Otherwise guest debug can interfere with host debug.
>>> 
>>> 
>>> ok
>>> 
>>>> 
>>>>> +#endif
>>>>> +    mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
>>>>> +    mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
>>>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
>>>>> +    mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
>>>>> +    mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
>>>>> +#endif
>>>>> +    mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
>>>>> +    mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
>>>>> +
>>>>> +    /* Enable debug events after other debug registers restored */
>>>>> +    mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0); }
>>>> 
>>>> All of the code above looks suspiciously similar to
>>>> prime_debug_regs();. Can't we somehow reuse that?
>>> 
>>> I think we can if
>>> - Save thread->debug_regs in local data structure
>> 
>> Yes, it can even be on the stack.
>> 
>>> - Load vcpu->arch->debug_regs in thread->debug_regs
>>> - Call prime_debug_regs();
>>> - Restore thread->debug_regs from local save values in first step
>> 
>> On heavyweight exit, based on the values on stack, yes.
> 
> This is how I think we can save/restore debug context. Please correct if I am missing something.

Sounds about right :)

Alex

> 
> 1) When QEMU is running
> 
> -> thread->debug_reg == QEMU debug register context.
> -> Kernel will handle switching the debug register on context switch.
> -> no vcpu_load() called
> 
> 2) QEMU makes ioctls (except RUN) 
> -> This will call vcpu_load()
> -> should not change context.
> -> Some ioctls can change vcpu debug register, context saved in vcpu->debug_regs
> 
> 3) QEMU Makes RUN ioctl
> -> Save thread->debug_reg on STACK
> -> Store thread->debug_reg == vcpu->debug_reg
> -> load thread->debug_reg 
> -> RUN VCPU ( So thread points to vcpu context )
> 
> 4) Context switch happens When VCPU running
> -> makes vcpu_load() should not load any context
> -> kernel loads the vcpu context as thread->debug_regs points to vcpu context.
> 
> 5) On heavyweight_exit
> -> Load the context saved on stack in thread->debug_reg
> 
> Thanks
> -Bharat
> 
> 
> 

Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Alexander Graf]

On 03.05.2013, at 13:08, Alexander Graf wrote:

> 
> 
> Am 03.05.2013 um 12:48 schrieb Bhushan Bharat-R65777 <R65777@freescale.com>:
> 
>>>>>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu
>>>>>> +*vcpu) {
>>>>>> +    if (!vcpu->arch.debug_active)
>>>>>> +        return;
>>>>>> +
>>>>>> +    /* Disable all debug events and clead pending debug events */
>>>>>> +    mtspr(SPRN_DBCR0, 0x0);
>>>>>> +    kvmppc_clear_dbsr();
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Check whether guest still need debug resource, if not then there
>>>>>> +     * is no need to restore guest context.
>>>>>> +     */
>>>>>> +    if (!vcpu->arch.shadow_dbg_reg.dbcr0)
>>>>>> +        return;
>>>>>> +
>>>>>> +    /* Load Guest Context */
>>>>>> +    mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
>>>>>> +    mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2); #ifdef
>>>>>> +CONFIG_KVM_E500MC
>>>>>> +    mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
>>>>> 
>>>>> You need to make sure DBCR4 is 0 when you leave things back to normal
>>>>> user space. Otherwise guest debug can interfere with host debug.
>>>> 
>>>> 
>>>> ok
>>>> 
>>>>> 
>>>>>> +#endif
>>>>>> +    mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
>>>>>> +    mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
>>>>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
>>>>>> +    mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
>>>>>> +    mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
>>>>>> +#endif
>>>>>> +    mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
>>>>>> +    mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
>>>>>> +
>>>>>> +    /* Enable debug events after other debug registers restored */
>>>>>> +    mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0); }
>>>>> 
>>>>> All of the code above looks suspiciously similar to
>>>>> prime_debug_regs();. Can't we somehow reuse that?
>>>> 
>>>> I think we can if
>>>> - Save thread->debug_regs in local data structure
>>> 
>>> Yes, it can even be on the stack.
>>> 
>>>> - Load vcpu->arch->debug_regs in thread->debug_regs
>>>> - Call prime_debug_regs();
>>>> - Restore thread->debug_regs from local save values in first step
>>> 
>>> On heavyweight exit, based on the values on stack, yes.
>> 
>> This is how I think we can save/restore debug context. Please correct if I am missing something.
> 
> Sounds about right :)

Actually, what happens if a guest breakpoint is set to a kernel address that happens to be within the scope of kvm code? We do accept debug events between vcpu_run and the assembly code, right?


Alex

RE: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Friday, May 03, 2013 6:00 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421
> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support
> 
> 
> On 03.05.2013, at 13:08, Alexander Graf wrote:
> 
> >
> >
> > Am 03.05.2013 um 12:48 schrieb Bhushan Bharat-R65777 <R65777@freescale.com>:
> >
> >>>>>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu
> >>>>>> +*vcpu) {
> >>>>>> +    if (!vcpu->arch.debug_active)
> >>>>>> +        return;
> >>>>>> +
> >>>>>> +    /* Disable all debug events and clead pending debug events */
> >>>>>> +    mtspr(SPRN_DBCR0, 0x0);
> >>>>>> +    kvmppc_clear_dbsr();
> >>>>>> +
> >>>>>> +    /*
> >>>>>> +     * Check whether guest still need debug resource, if not then there
> >>>>>> +     * is no need to restore guest context.
> >>>>>> +     */
> >>>>>> +    if (!vcpu->arch.shadow_dbg_reg.dbcr0)
> >>>>>> +        return;
> >>>>>> +
> >>>>>> +    /* Load Guest Context */
> >>>>>> +    mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
> >>>>>> +    mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2); #ifdef
> >>>>>> +CONFIG_KVM_E500MC
> >>>>>> +    mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
> >>>>>
> >>>>> You need to make sure DBCR4 is 0 when you leave things back to
> >>>>> normal user space. Otherwise guest debug can interfere with host debug.
> >>>>
> >>>>
> >>>> ok
> >>>>
> >>>>>
> >>>>>> +#endif
> >>>>>> +    mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
> >>>>>> +    mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
> >>>>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> >>>>>> +    mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
> >>>>>> +    mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
> >>>>>> +#endif
> >>>>>> +    mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
> >>>>>> +    mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
> >>>>>> +
> >>>>>> +    /* Enable debug events after other debug registers restored */
> >>>>>> +    mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0); }
> >>>>>
> >>>>> All of the code above looks suspiciously similar to
> >>>>> prime_debug_regs();. Can't we somehow reuse that?
> >>>>
> >>>> I think we can if
> >>>> - Save thread->debug_regs in local data structure
> >>>
> >>> Yes, it can even be on the stack.
> >>>
> >>>> - Load vcpu->arch->debug_regs in thread->debug_regs
> >>>> - Call prime_debug_regs();
> >>>> - Restore thread->debug_regs from local save values in first step
> >>>
> >>> On heavyweight exit, based on the values on stack, yes.
> >>
> >> This is how I think we can save/restore debug context. Please correct if I am
> missing something.
> >
> > Sounds about right :)
> 
> Actually, what happens if a guest breakpoint is set to a kernel address that
> happens to be within the scope of kvm code?

You mean address of kvm code in guest or host?

If host, we already mentioned that we do not support that. Right?

-Bharat

> We do accept debug events between
> vcpu_run and the assembly code, right?
> 
> 
> Alex
> 

Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Alexander Graf]

On 03.05.2013, at 15:11, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Friday, May 03, 2013 6:00 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421
>> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support
>> 
>> 
>> On 03.05.2013, at 13:08, Alexander Graf wrote:
>> 
>>> 
>>> 
>>> Am 03.05.2013 um 12:48 schrieb Bhushan Bharat-R65777 <R65777@freescale.com>:
>>> 
>>>>>>>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu
>>>>>>>> +*vcpu) {
>>>>>>>> +    if (!vcpu->arch.debug_active)
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    /* Disable all debug events and clead pending debug events */
>>>>>>>> +    mtspr(SPRN_DBCR0, 0x0);
>>>>>>>> +    kvmppc_clear_dbsr();
>>>>>>>> +
>>>>>>>> +    /*
>>>>>>>> +     * Check whether guest still need debug resource, if not then there
>>>>>>>> +     * is no need to restore guest context.
>>>>>>>> +     */
>>>>>>>> +    if (!vcpu->arch.shadow_dbg_reg.dbcr0)
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    /* Load Guest Context */
>>>>>>>> +    mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
>>>>>>>> +    mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2); #ifdef
>>>>>>>> +CONFIG_KVM_E500MC
>>>>>>>> +    mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
>>>>>>> 
>>>>>>> You need to make sure DBCR4 is 0 when you leave things back to
>>>>>>> normal user space. Otherwise guest debug can interfere with host debug.
>>>>>> 
>>>>>> 
>>>>>> ok
>>>>>> 
>>>>>>> 
>>>>>>>> +#endif
>>>>>>>> +    mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
>>>>>>>> +    mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
>>>>>>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
>>>>>>>> +    mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
>>>>>>>> +    mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
>>>>>>>> +#endif
>>>>>>>> +    mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
>>>>>>>> +    mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
>>>>>>>> +
>>>>>>>> +    /* Enable debug events after other debug registers restored */
>>>>>>>> +    mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0); }
>>>>>>> 
>>>>>>> All of the code above looks suspiciously similar to
>>>>>>> prime_debug_regs();. Can't we somehow reuse that?
>>>>>> 
>>>>>> I think we can if
>>>>>> - Save thread->debug_regs in local data structure
>>>>> 
>>>>> Yes, it can even be on the stack.
>>>>> 
>>>>>> - Load vcpu->arch->debug_regs in thread->debug_regs
>>>>>> - Call prime_debug_regs();
>>>>>> - Restore thread->debug_regs from local save values in first step
>>>>> 
>>>>> On heavyweight exit, based on the values on stack, yes.
>>>> 
>>>> This is how I think we can save/restore debug context. Please correct if I am
>> missing something.
>>> 
>>> Sounds about right :)
>> 
>> Actually, what happens if a guest breakpoint is set to a kernel address that
>> happens to be within the scope of kvm code?
> 
> You mean address of kvm code in guest or host?
> 
> If host, we already mentioned that we do not support that. Right?

QEMU wants to debug the guest at address 0xc0000123. kvm_run happens to be at that address. We switch the debug registers through prime_debug_regs. Will the host kernel receive a debug interrupt when it runs kvm_run()?


Alex

RE: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of
> Alexander Graf
> Sent: Friday, May 03, 2013 6:48 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421
> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub support
> 
> 
> On 03.05.2013, at 15:11, Bhushan Bharat-R65777 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Friday, May 03, 2013 6:00 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421
> >> Subject: Re: [PATCH 7/7 v3] KVM: PPC: Add userspace debug stub
> >> support
> >>
> >>
> >> On 03.05.2013, at 13:08, Alexander Graf wrote:
> >>
> >>>
> >>>
> >>> Am 03.05.2013 um 12:48 schrieb Bhushan Bharat-R65777 <R65777@freescale.com>:
> >>>
> >>>>>>>> +static void kvmppc_booke_vcpu_load_debug_regs(struct kvm_vcpu
> >>>>>>>> +*vcpu) {
> >>>>>>>> +    if (!vcpu->arch.debug_active)
> >>>>>>>> +        return;
> >>>>>>>> +
> >>>>>>>> +    /* Disable all debug events and clead pending debug events */
> >>>>>>>> +    mtspr(SPRN_DBCR0, 0x0);
> >>>>>>>> +    kvmppc_clear_dbsr();
> >>>>>>>> +
> >>>>>>>> +    /*
> >>>>>>>> +     * Check whether guest still need debug resource, if not then
> there
> >>>>>>>> +     * is no need to restore guest context.
> >>>>>>>> +     */
> >>>>>>>> +    if (!vcpu->arch.shadow_dbg_reg.dbcr0)
> >>>>>>>> +        return;
> >>>>>>>> +
> >>>>>>>> +    /* Load Guest Context */
> >>>>>>>> +    mtspr(SPRN_DBCR1, vcpu->arch.shadow_dbg_reg.dbcr1);
> >>>>>>>> +    mtspr(SPRN_DBCR2, vcpu->arch.shadow_dbg_reg.dbcr2); #ifdef
> >>>>>>>> +CONFIG_KVM_E500MC
> >>>>>>>> +    mtspr(SPRN_DBCR4, vcpu->arch.shadow_dbg_reg.dbcr4);
> >>>>>>>
> >>>>>>> You need to make sure DBCR4 is 0 when you leave things back to
> >>>>>>> normal user space. Otherwise guest debug can interfere with host debug.
> >>>>>>
> >>>>>>
> >>>>>> ok
> >>>>>>
> >>>>>>>
> >>>>>>>> +#endif
> >>>>>>>> +    mtspr(SPRN_IAC1, vcpu->arch.shadow_dbg_reg.iac[0]);
> >>>>>>>> +    mtspr(SPRN_IAC2, vcpu->arch.shadow_dbg_reg.iac[1]);
> >>>>>>>> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> >>>>>>>> +    mtspr(SPRN_IAC3, vcpu->arch.shadow_dbg_reg.iac[2]);
> >>>>>>>> +    mtspr(SPRN_IAC4, vcpu->arch.shadow_dbg_reg.iac[3]);
> >>>>>>>> +#endif
> >>>>>>>> +    mtspr(SPRN_DAC1, vcpu->arch.shadow_dbg_reg.dac[0]);
> >>>>>>>> +    mtspr(SPRN_DAC2, vcpu->arch.shadow_dbg_reg.dac[1]);
> >>>>>>>> +
> >>>>>>>> +    /* Enable debug events after other debug registers restored */
> >>>>>>>> +    mtspr(SPRN_DBCR0, vcpu->arch.shadow_dbg_reg.dbcr0); }
> >>>>>>>
> >>>>>>> All of the code above looks suspiciously similar to
> >>>>>>> prime_debug_regs();. Can't we somehow reuse that?
> >>>>>>
> >>>>>> I think we can if
> >>>>>> - Save thread->debug_regs in local data structure
> >>>>>
> >>>>> Yes, it can even be on the stack.
> >>>>>
> >>>>>> - Load vcpu->arch->debug_regs in thread->debug_regs
> >>>>>> - Call prime_debug_regs();
> >>>>>> - Restore thread->debug_regs from local save values in first step
> >>>>>
> >>>>> On heavyweight exit, based on the values on stack, yes.
> >>>>
> >>>> This is how I think we can save/restore debug context. Please
> >>>> correct if I am
> >> missing something.
> >>>
> >>> Sounds about right :)
> >>
> >> Actually, what happens if a guest breakpoint is set to a kernel
> >> address that happens to be within the scope of kvm code?
> >
> > You mean address of kvm code in guest or host?
> >
> > If host, we already mentioned that we do not support that. Right?
> 
> QEMU wants to debug the guest at address 0xc0000123. kvm_run happens to be at
> that address. We switch the debug registers through prime_debug_regs. Will the
> host kernel receive a debug interrupt when it runs kvm_run()?

No,
On e500v2, we uses DBCR1 and DBCR2 to not allow debug events when MSR.PR = 0
On e500mc+, we uses EPCR.DUVD to not allow debug events when in hypervisor mode.

-Bharat

> 
> 
> Alex
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a
> message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7 v3] KVM: PPC: exit to user space on "ehpriv" instruction

[tiejun.chen]

On 04/08/2013 06:32 PM, Bharat Bhushan wrote:
> From: Bharat Bhushan <bharat.bhushan@freescale.com>
>
> "ehpriv" instruction is used for setting software breakpoints
> by user space. This patch adds support to exit to user space
> with "run->debug" have relevant information.
>
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
>   arch/powerpc/kvm/e500_emulate.c |   10 ++++++++++
>   1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
> index e78f353..cefdd38 100644
> --- a/arch/powerpc/kvm/e500_emulate.c
> +++ b/arch/powerpc/kvm/e500_emulate.c
> @@ -26,6 +26,7 @@
>   #define XOP_TLBRE   946
>   #define XOP_TLBWE   978
>   #define XOP_TLBILX  18
> +#define XOP_EHPRIV  270
>
>   #ifdef CONFIG_KVM_E500MC
>   static int dbell2prio(ulong param)
> @@ -130,6 +131,15 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
>   			emulated = kvmppc_e500_emul_tlbivax(vcpu, ea);
>   			break;
>
> +		case XOP_EHPRIV:
> +			run->exit_reason = KVM_EXIT_DEBUG;

IIRC, the ehpriv instruction should generate a Hypervisor Privilege Exception to 
trap into the Hypervisor proactive. And we can use this ability to design 
something conveniently. And so, that is not only for the debug mechanism like 
you did.

So here if 'run->exit_reason' is fixed to KVM_EXIT_DEBUG, how to distinguish 
other scenarios? So as I understand, we should use 'ehpriv oc' exactly then 
resolve 'oc' further to go different cases, right?

Tiejun

> +			run->debug.arch.address = vcpu->arch.pc;
> +			run->debug.arch.status = 0;
> +			kvmppc_account_exit(vcpu, DEBUG_EXITS);
> +			emulated = EMULATE_EXIT_USER;
> +			*advance = 0;
> +			break;
> +
>   		default:
>   			emulated = EMULATE_FAIL;
>   		}
>

Re: [PATCH 5/7 v3] KVM: PPC: exit to user space on "ehpriv" instruction

[Alexander Graf]

On 08.04.2013, at 12:32, Bharat Bhushan wrote:

> From: Bharat Bhushan <bharat.bhushan@freescale.com>
> 
> "ehpriv" instruction is used for setting software breakpoints
> by user space. This patch adds support to exit to user space
> with "run->debug" have relevant information.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> arch/powerpc/kvm/e500_emulate.c |   10 ++++++++++
> 1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
> index e78f353..cefdd38 100644
> --- a/arch/powerpc/kvm/e500_emulate.c
> +++ b/arch/powerpc/kvm/e500_emulate.c
> @@ -26,6 +26,7 @@
> #define XOP_TLBRE   946
> #define XOP_TLBWE   978
> #define XOP_TLBILX  18
> +#define XOP_EHPRIV  270
> 
> #ifdef CONFIG_KVM_E500MC
> static int dbell2prio(ulong param)
> @@ -130,6 +131,15 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
> 			emulated = kvmppc_e500_emul_tlbivax(vcpu, ea);
> 			break;
> 
> +		case XOP_EHPRIV:

This is supposed to check for oc, no?


Alex

> +			run->exit_reason = KVM_EXIT_DEBUG;
> +			run->debug.arch.address = vcpu->arch.pc;
> +			run->debug.arch.status = 0;
> +			kvmppc_account_exit(vcpu, DEBUG_EXITS);
> +			emulated = EMULATE_EXIT_USER;
> +			*advance = 0;
> +			break;
> +
> 		default:
> 			emulated = EMULATE_FAIL;
> 		}
> -- 
> 1.7.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7 v3] KVM: PPC: exit to user space on "ehpriv" instruction

[tiejun.chen]

On 04/26/2013 06:45 PM, Alexander Graf wrote:
>
> On 08.04.2013, at 12:32, Bharat Bhushan wrote:
>
>> From: Bharat Bhushan <bharat.bhushan@freescale.com>
>>
>> "ehpriv" instruction is used for setting software breakpoints
>> by user space. This patch adds support to exit to user space
>> with "run->debug" have relevant information.
>>
>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>> ---
>> arch/powerpc/kvm/e500_emulate.c |   10 ++++++++++
>> 1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
>> index e78f353..cefdd38 100644
>> --- a/arch/powerpc/kvm/e500_emulate.c
>> +++ b/arch/powerpc/kvm/e500_emulate.c
>> @@ -26,6 +26,7 @@
>> #define XOP_TLBRE   946
>> #define XOP_TLBWE   978
>> #define XOP_TLBILX  18
>> +#define XOP_EHPRIV  270
>>
>> #ifdef CONFIG_KVM_E500MC
>> static int dbell2prio(ulong param)
>> @@ -130,6 +131,15 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
>> 			emulated = kvmppc_e500_emul_tlbivax(vcpu, ea);
>> 			break;
>>
>> +		case XOP_EHPRIV:
>
> This is supposed to check for oc, no?

The other day I already sent one patch only to check OC, "KVM/PPC: emulate ehpriv".

But Bharat said he's waiting for other debug patches to be reviewed.

Tiejun

Re: [PATCH 2/7 v3] Rename EMULATE_DO_PAPR to EMULATE_EXIT_USER

[Paul Mackerras]

On Mon, Apr 08, 2013 at 04:02:13PM +0530, Bharat Bhushan wrote:
> Instruction emulation return EMULATE_DO_PAPR when it requires
> exit to userspace on book3s. Similar return is required
> for booke. EMULATE_DO_PAPR reads out to be confusing so it is
> renamed to EMULATE_EXIT_USER.

This and the following patch look like an unnecessary and confusing
change to me.  If you want an EMULATE_EXIT_USER, why not just add it
and use it in your new code, and leave the EMULATE_DO_PAPR code alone?

Paul.

Re: [PATCH 2/7 v3] Rename EMULATE_DO_PAPR to EMULATE_EXIT_USER

[Alexander Graf]

On 10.04.2013, at 00:37, Paul Mackerras wrote:

> On Mon, Apr 08, 2013 at 04:02:13PM +0530, Bharat Bhushan wrote:
>> Instruction emulation return EMULATE_DO_PAPR when it requires
>> exit to userspace on book3s. Similar return is required
>> for booke. EMULATE_DO_PAPR reads out to be confusing so it is
>> renamed to EMULATE_EXIT_USER.
> 
> This and the following patch look like an unnecessary and confusing
> change to me.  If you want an EMULATE_EXIT_USER, why not just add it
> and use it in your new code, and leave the EMULATE_DO_PAPR code alone?

I actually suggested that change to Bharat on a former patch review.

I think it clutters the EMULATE_ exit ID name space if we extend it with simple "go to user space with message x"  types every time we add a new user space exit.


Alex

Re: [PATCH 0/7 v3] KVM :PPC: Userspace Debug support

[Alexander Graf]

On 08.04.2013, at 12:32, Bharat Bhushan wrote:

> From: Bharat Bhushan <bharat.bhushan@freescale.com>
> 
> This patchset adds the userspace debug support for booke/bookehv.
> this is tested on powerpc e500v2/e500mc devices.
> 
> We are now assuming that debug resource will not be used by
> kernel for its own debugging. It will be used for only kernel
> user process debugging. So the kernel debug load interface during
> context_to is used to load debug conext for that selected process.

Thanks, applied 1-4 to kvm-ppc-queue. 5 and above have comments.


Alex