[PATCH v5 01/12] KVM: add comments for kvm_debug_exit_arch struct

[PATCH v5 01/12] KVM: add comments for kvm_debug_exit_arch struct

[Alex Bennée]

Bring into line with the comments for the other structures and their
KVM_EXIT_* cases. Also update api.txt to reflect use in kvm_run
documentation.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

---

v2
  - add comments for other exit types
v3
  - s/commentary/comments/
  - add rb tags
  - update api.txt kvm_run to include KVM_EXIT_DEBUG desc
v4
  - sp fixes
  - add a-b
---
 Documentation/virtual/kvm/api.txt | 4 +++-
 include/uapi/linux/kvm.h          | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 9fa2bf8..c34c32d 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3070,11 +3070,13 @@ data_offset describes where the data is located (KVM_EXIT_IO_OUT) or
 where kvm expects application code to place the data for the next
 KVM_RUN invocation (KVM_EXIT_IO_IN).  Data format is a packed array.
 
+		/* KVM_EXIT_DEBUG */
 		struct {
 			struct kvm_debug_exit_arch arch;
 		} debug;
 
-Unused.
+If the exit_reason is KVM_EXIT_DEBUG, then a vcpu is processing a debug event
+for which architecture specific information is returned.
 
 		/* KVM_EXIT_MMIO */
 		struct {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4b60056..70ac641 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -237,6 +237,7 @@ struct kvm_run {
 			__u32 count;
 			__u64 data_offset; /* relative to kvm_run start */
 		} io;
+		/* KVM_EXIT_DEBUG */
 		struct {
 			struct kvm_debug_exit_arch arch;
 		} debug;
@@ -285,6 +286,7 @@ struct kvm_run {
 			__u32 data;
 			__u8  is_write;
 		} dcr;
+		/* KVM_EXIT_INTERNAL_ERROR */
 		struct {
 			__u32 suberror;
 			/* Available with KVM_CAP_INTERNAL_ERROR_DATA: */
@@ -295,6 +297,7 @@ struct kvm_run {
 		struct {
 			__u64 gprs[32];
 		} osi;
+		/* KVM_EXIT_PAPR_HCALL */
 		struct {
 			__u64 nr;
 			__u64 ret;
-- 
2.4.1

[PATCH v5 02/12] KVM: arm64: fix misleading comments in save/restore

[Alex Bennée]

The elr_el2 and spsr_el2 registers in fact contain the processor state
before entry into the hypervisor code. In the case of guest state it
could be in either el0 or el1.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 arch/arm64/kvm/hyp.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 5befd01..cb9bdd8 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -50,8 +50,8 @@
 	stp	x29, lr, [x3, #80]
 
 	mrs	x19, sp_el0
-	mrs	x20, elr_el2		// EL1 PC
-	mrs	x21, spsr_el2		// EL1 pstate
+	mrs	x20, elr_el2		// PC before hyp entry
+	mrs	x21, spsr_el2		// pstate before hyp entry
 
 	stp	x19, x20, [x3, #96]
 	str	x21, [x3, #112]
@@ -82,8 +82,8 @@
 	ldr	x21, [x3, #16]
 
 	msr	sp_el0, x19
-	msr	elr_el2, x20 				// EL1 PC
-	msr	spsr_el2, x21 				// EL1 pstate
+	msr	elr_el2, x20 		// PC to restore
+	msr	spsr_el2, x21 		// pstate to restore
 
 	add	x3, x2, #CPU_XREG_OFFSET(19)
 	ldp	x19, x20, [x3]
-- 
2.4.1

[PATCH v5 03/12] KVM: arm64: guest debug, define API headers

[Alex Bennée]

This commit defines the API headers for guest debugging. There are two
architecture specific debug structures:

  - kvm_guest_debug_arch, allows us to pass in HW debug registers
  - kvm_debug_exit_arch, signals exception and possible faulting address

The type of debugging being used is controlled by the architecture
specific control bits of the kvm_guest_debug->control flags in the ioctl
structure.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

---
v2
   - expose hsr and pc directly to user-space
v3
   - s/control/controlled/ in commit message
   - add v8 to ARM ARM comment (ARM Architecture Reference Manual)
   - add rb tag
   - rm pc, add far
   - re-word comments on alignment
   - rename KVM_ARM_NDBG_REGS -> KVM_ARM_MAX_DBG_REGS
v4
   - now uses common HW/SW BP define
   - add a-b-tag
   - use u32 for control regs
v5
   - revert to have arch specific KVM_GUESTDBG_USE_SW/HW_BP
   - rm stale comments dbgctrl was stored as u64
---
 arch/arm64/include/uapi/asm/kvm.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index d268320..43758e7 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -100,12 +100,32 @@ struct kvm_sregs {
 struct kvm_fpu {
 };
 
+/*
+ * See v8 ARM ARM D7.3: Debug Registers
+ *
+ * The architectural limit is 16 debug registers of each type although
+ * in practice there are usually less (see ID_AA64DFR0_EL1).
+ */
+#define KVM_ARM_MAX_DBG_REGS 16
 struct kvm_guest_debug_arch {
+	__u32 dbg_bcr[KVM_ARM_MAX_DBG_REGS];
+	__u64 dbg_bvr[KVM_ARM_MAX_DBG_REGS];
+	__u32 dbg_wcr[KVM_ARM_MAX_DBG_REGS];
+	__u64 dbg_wvr[KVM_ARM_MAX_DBG_REGS];
 };
 
 struct kvm_debug_exit_arch {
+	__u32 hsr;
+	__u64 far;
 };
 
+/*
+ * Architecture specific defines for kvm_guest_debug->control
+ */
+
+#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
+#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)
+
 struct kvm_sync_regs {
 };
 
-- 
2.4.1

[PATCH v5 04/12] KVM: arm: guest debug, add stub KVM_SET_GUEST_DEBUG ioctl

[Alex Bennée]

This commit adds a stub function to support the KVM_SET_GUEST_DEBUG
ioctl. Any unsupported flag will return -EINVAL. For now, only
KVM_GUESTDBG_ENABLE is supported, although it won't have any effects.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>.
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

---
v2
  - simplified form of the ioctl (stuff will go into setup_debug)
v3
 - KVM_GUESTDBG_VALID->KVM_GUESTDBG_VALID_MASK
 - move mask check to the top of function
 - add ioctl doc header
 - split capability into separate patch
 - tweaked commit wording w.r.t return of -EINVAL
v4
 - add r-b-tag
---
 Documentation/virtual/kvm/api.txt |  2 +-
 arch/arm/kvm/arm.c                | 23 ++++++++++++++++++++++-
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index c34c32d..ba635c7 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2645,7 +2645,7 @@ handled.
 4.87 KVM_SET_GUEST_DEBUG
 
 Capability: KVM_CAP_SET_GUEST_DEBUG
-Architectures: x86, s390, ppc
+Architectures: x86, s390, ppc, arm64
 Type: vcpu ioctl
 Parameters: struct kvm_guest_debug (in)
 Returns: 0 on success; -1 on error
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index d9631ec..52a1d4d38 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -302,10 +302,31 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	kvm_arm_set_running_vcpu(NULL);
 }
 
+#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE)
+
+/**
+ * kvm_arch_vcpu_ioctl_set_guest_debug - set up guest debugging
+ * @kvm:	pointer to the KVM struct
+ * @kvm_guest_debug: the ioctl data buffer
+ *
+ * This sets up and enables the VM for guest debugging. Userspace
+ * passes in a control flag to enable different debug types and
+ * potentially other architecture specific information in the rest of
+ * the structure.
+ */
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
 {
-	return -EINVAL;
+	if (dbg->control & ~KVM_GUESTDBG_VALID_MASK)
+		return -EINVAL;
+
+	if (dbg->control & KVM_GUESTDBG_ENABLE) {
+		vcpu->guest_debug = dbg->control;
+	} else {
+		/* If not enabled clear all flags */
+		vcpu->guest_debug = 0;
+	}
+	return 0;
 }
 
 
-- 
2.4.1

[PATCH v5 05/12] KVM: arm: introduce kvm_arm_init/setup/clear_debug

[Alex Bennée]

This is a precursor for later patches which will need to do more to
setup debug state before entering the hyp.S switch code. The existing
functionality for setting mdcr_el2 has been moved out of hyp.S and now
uses the value kept in vcpu->arch.mdcr_el2.

As the assembler used to previously mask and preserve MDCR_EL2.HPMN I've
had to add a mechanism to save the value of mdcr_el2 as a per-cpu
variable during the initialisation code. The kernel never sets this
number so we are assuming the bootcode has set up the correct value
here.

This also moves the conditional setting of the TDA bit from the hyp code
into the C code which is currently used for the lazy debug register
context switch code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v3
  - rename fns from arch->arm
  - preserve MDCR_EL2.HPMN setting
  - re-word some of the comments
  - fix some minor grammar nits
  - merge setting of mdcr_el2
  - introduce trap_debug flag
  - move setup/clear within the irq lock section
v4
  - fix TDOSA desc
  - rm un-needed else leg
  - s/arch/arm/
---
 arch/arm/include/asm/kvm_host.h   |  4 ++
 arch/arm/kvm/arm.c                |  9 ++++-
 arch/arm64/include/asm/kvm_asm.h  |  2 +
 arch/arm64/include/asm/kvm_host.h |  5 +++
 arch/arm64/kernel/asm-offsets.c   |  1 +
 arch/arm64/kvm/Makefile           |  2 +-
 arch/arm64/kvm/debug.c            | 81 +++++++++++++++++++++++++++++++++++++++
 arch/arm64/kvm/hyp.S              | 19 ++++-----
 8 files changed, 110 insertions(+), 13 deletions(-)
 create mode 100644 arch/arm64/kvm/debug.c

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index d71607c..746c0c69 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -236,4 +236,8 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
 
+static inline void kvm_arm_init_debug(void) {}
+static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
+static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {}
+
 #endif /* __ARM_KVM_HOST_H__ */
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 52a1d4d38..4a274e1 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -570,6 +570,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			continue;
 		}
 
+		kvm_arm_setup_debug(vcpu);
+
 		/**************************************************************
 		 * Enter the guest
 		 */
@@ -582,7 +584,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		vcpu->mode = OUTSIDE_GUEST_MODE;
 		kvm_guest_exit();
 		trace_kvm_exit(kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
-		/*
+
+		kvm_arm_clear_debug(vcpu);
+
+                /*
 		 * We may have taken a host interrupt in HYP mode (ie
 		 * while executing the guest). This interrupt is still
 		 * pending, as we haven't serviced it yet!
@@ -930,6 +935,8 @@ static void cpu_init_hyp_mode(void *dummy)
 	vector_ptr = (unsigned long)__kvm_hyp_vector;
 
 	__cpu_init_hyp_mode(boot_pgd_ptr, pgd_ptr, hyp_stack_ptr, vector_ptr);
+
+	kvm_arm_init_debug();
 }
 
 static int hyp_init_cpu_notify(struct notifier_block *self,
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 4f7310f..d6b507e 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -137,6 +137,8 @@ extern char __restore_vgic_v2_state[];
 extern char __save_vgic_v3_state[];
 extern char __restore_vgic_v3_state[];
 
+extern u32 __kvm_get_mdcr_el2(void);
+
 #endif
 
 #endif /* __ARM_KVM_ASM_H__ */
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f0f58c9..7cb99b5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -103,6 +103,7 @@ struct kvm_vcpu_arch {
 
 	/* HYP configuration */
 	u64 hcr_el2;
+	u32 mdcr_el2;
 
 	/* Exception Information */
 	struct kvm_vcpu_fault_info fault;
@@ -250,4 +251,8 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
 
+void kvm_arm_init_debug(void);
+void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
+void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
+
 #endif /* __ARM64_KVM_HOST_H__ */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index da675cc..dfb25a2 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -117,6 +117,7 @@ int main(void)
   DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
   DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
   DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
+  DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
   DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
   DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index d5904f8..90e3f39 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -17,7 +17,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
 
 kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o
 kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
-kvm-$(CONFIG_KVM_ARM_HOST) += guest.o reset.o sys_regs.o sys_regs_generic_v8.o
+kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
 
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic-v2.o
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
new file mode 100644
index 0000000..faf0e1f
--- /dev/null
+++ b/arch/arm64/kvm/debug.c
@@ -0,0 +1,81 @@
+/*
+ * Debug and Guest Debug support
+ *
+ * Copyright (C) 2015 - Linaro Ltd
+ * Author: Alex Bennée <alex.bennee@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kvm_host.h>
+
+#include <asm/kvm_arm.h>
+
+static DEFINE_PER_CPU(u32, mdcr_el2);
+
+/**
+ * kvm_arm_init_debug - grab what we need for debug
+ *
+ * Currently the sole task of this function is to retrieve the initial
+ * value of mdcr_el2 so we can preserve MDCR_EL2.HPMN which has
+ * presumably been set-up by some knowledgeable bootcode.
+ *
+ * It is called once per-cpu during CPU hyp initialisation.
+ */
+
+void kvm_arm_init_debug(void)
+{
+	__this_cpu_write(mdcr_el2, kvm_call_hyp(__kvm_get_mdcr_el2));
+}
+
+
+/**
+ * kvm_arm_setup_debug - set up debug related stuff
+ *
+ * @vcpu:	the vcpu pointer
+ *
+ * This is called before each entry into the hypervisor to setup any
+ * debug related registers. Currently this just ensures we will trap
+ * access to:
+ *  - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR)
+ *  - Debug ROM Address (MDCR_EL2_TDRA)
+ *  - OS related registers (MDCR_EL2_TDOSA)
+ *
+ * Additionally, KVM only traps guest accesses to the debug registers if
+ * the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY
+ * flag on vcpu->arch.debug_flags).  Since the guest must not interfere
+ * with the hardware state when debugging the guest, we must ensure that
+ * trapping is enabled whenever we are debugging the guest using the
+ * debug registers.
+ */
+
+void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
+{
+	bool trap_debug = !(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY);
+
+	vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK;
+	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
+				MDCR_EL2_TPMCR |
+				MDCR_EL2_TDRA |
+				MDCR_EL2_TDOSA);
+
+	/* Trap on access to debug registers? */
+	if (trap_debug)
+		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
+
+}
+
+void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
+{
+	/* Nothing to do yet */
+}
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index cb9bdd8..74e63d8 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -768,17 +768,8 @@
 	mov	x2, #(1 << 15)	// Trap CP15 Cr=15
 	msr	hstr_el2, x2
 
-	mrs	x2, mdcr_el2
-	and	x2, x2, #MDCR_EL2_HPMN_MASK
-	orr	x2, x2, #(MDCR_EL2_TPM | MDCR_EL2_TPMCR)
-	orr	x2, x2, #(MDCR_EL2_TDRA | MDCR_EL2_TDOSA)
-
-	// Check for KVM_ARM64_DEBUG_DIRTY, and set debug to trap
-	// if not dirty.
-	ldr	x3, [x0, #VCPU_DEBUG_FLAGS]
-	tbnz	x3, #KVM_ARM64_DEBUG_DIRTY_SHIFT, 1f
-	orr	x2, x2,  #MDCR_EL2_TDA
-1:
+	// Monitor Debug Config - see kvm_arm_setup_debug()
+	ldr	x2, [x0, #VCPU_MDCR_EL2]
 	msr	mdcr_el2, x2
 .endm
 
@@ -1295,4 +1286,10 @@ ENTRY(__kvm_hyp_vector)
 	ventry	el1_error_invalid		// Error 32-bit EL1
 ENDPROC(__kvm_hyp_vector)
 
+
+ENTRY(__kvm_get_mdcr_el2)
+	mrs	x0, mdcr_el2
+	ret
+ENDPROC(__kvm_get_mdcr_el2)
+
 	.popsection
-- 
2.4.1

[PATCH v5 06/12] KVM: arm64: guest debug, add SW break point support

[Alex Bennée]

This adds support for SW breakpoints inserted by userspace.

We do this by trapping all guest software debug exceptions to the
hypervisor (MDCR_EL2.TDE). The exit handler sets an exit reason of
KVM_EXIT_DEBUG with the kvm_debug_exit_arch structure holding the
exception syndrome information.

It will be up to userspace to extract the PC (via GET_ONE_REG) and
determine if the debug event was for a breakpoint it inserted. If not
userspace will need to re-inject the correct exception restart the
hypervisor to deliver the debug exception to the guest.

Any other guest software debug exception (e.g. single step or HW
assisted breakpoints) will cause an error and the VM to be killed. This
is addressed by later patches which add support for the other debug
types.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

---
v2
  - update to use new exit struct
  - tweak for C setup
  - do our setup in debug_setup/clear code
  - fixed up comments
v3:
  - fix spacing in KVM_GUESTDBG_VALID_MASK
  - fix and clarify wording on kvm_handle_guest_debug
  - handle error case in kvm_handle_guest_debug
  - re-word the commit message
v4
  - rm else leg
  - add r-b-tag
---
 Documentation/virtual/kvm/api.txt |  2 +-
 arch/arm/kvm/arm.c                |  2 +-
 arch/arm64/kvm/debug.c            |  3 +++
 arch/arm64/kvm/handle_exit.c      | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index ba635c7..33c8143 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2667,7 +2667,7 @@ when running. Common control bits are:
 The top 16 bits of the control field are architecture specific control
 flags which can include the following:
 
-  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
+  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
   - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
   - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
   - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 4a274e1..064c105 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -302,7 +302,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	kvm_arm_set_running_vcpu(NULL);
 }
 
-#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE)
+#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
 
 /**
  * kvm_arch_vcpu_ioctl_set_guest_debug - set up guest debugging
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index faf0e1f..8d1bfa4 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -73,6 +73,9 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 	if (trap_debug)
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 
+	/* Trap breakpoints? */
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
+		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
 }
 
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 524fa25..27f38a9 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -82,6 +82,40 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+/**
+ * kvm_handle_guest_debug - handle a debug exception instruction
+ *
+ * @vcpu:	the vcpu pointer
+ * @run:	access to the kvm_run structure for results
+ *
+ * We route all debug exceptions through the same handler. If both the
+ * guest and host are using the same debug facilities it will be up to
+ * userspace to re-inject the correct exception for guest delivery.
+ *
+ * @return: 0 (while setting run->exit_reason), -1 for error
+ */
+static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	u32 hsr = kvm_vcpu_get_hsr(vcpu);
+	int ret = 0;
+
+	run->exit_reason = KVM_EXIT_DEBUG;
+	run->debug.arch.hsr = hsr;
+
+	switch (hsr >> ESR_ELx_EC_SHIFT) {
+	case ESR_ELx_EC_BKPT32:
+	case ESR_ELx_EC_BRK64:
+		break;
+	default:
+		kvm_err("%s: un-handled case hsr: %#08x\n",
+			__func__, (unsigned int) hsr);
+		ret = -1;
+		break;
+	}
+
+	return ret;
+}
+
 static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_WFx]	= kvm_handle_wfx,
 	[ESR_ELx_EC_CP15_32]	= kvm_handle_cp15_32,
@@ -96,6 +130,8 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
 	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
 	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
+	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
+	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
 };
 
 static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
-- 
2.4.1

[PATCH v5 07/12] KVM: arm64: guest debug, add support for single-step

[Alex Bennée]

This adds support for single-stepping the guest. To do this we need to
manipulate the guests PSTATE.SS and MDSCR_EL1.SS bits which we do in the
kvm_arm_setup/clear_debug() so we don't affect the apparent state of the
guest. Additionally while the host is debugging the guest we suppress
the ability of the guest to single-step itself.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - Move pstate/mdscr manipulation into C
  - don't export guest_debug to assembly
  - add accessor for saved_debug regs
  - tweak save/restore of mdscr_el1
v3
  - don't save PC in debug information struct
  - rename debug_saved_regs->guest_debug_state
  - save whole value, only use bits in restore
  - add save/restore_guest-debug_regs helper functions
  - simplify commit message for clarity
  - rm vcpu_debug_saved_reg access fn
v4
  - added more comments based on suggestions
  - guest_debug_state->guest_debug_preserved
  - no point masking restore, we will trap out
v5
  - more comments
  - don't bother preserving pstate.ss
---
 arch/arm/kvm/arm.c                |  4 ++-
 arch/arm64/include/asm/kvm_host.h | 11 ++++++++
 arch/arm64/kvm/debug.c            | 58 ++++++++++++++++++++++++++++++++++++---
 arch/arm64/kvm/handle_exit.c      |  2 ++
 4 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 064c105..9b3ed6d 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -302,7 +302,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	kvm_arm_set_running_vcpu(NULL);
 }
 
-#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
+#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |    \
+			    KVM_GUESTDBG_USE_SW_BP | \
+			    KVM_GUESTDBG_SINGLESTEP)
 
 /**
  * kvm_arch_vcpu_ioctl_set_guest_debug - set up guest debugging
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cb99b5..e2db6a6 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -123,6 +123,17 @@ struct kvm_vcpu_arch {
 	 * here.
 	 */
 
+	/*
+	 * Guest registers we preserve during guest debugging.
+	 *
+	 * These shadow registers are updated by the kvm_handle_sys_reg
+	 * trap handler if the guest accesses or updates them while we
+	 * are using guest debug.
+	 */
+	struct {
+		u32	mdscr_el1;
+	} guest_debug_preserved;
+
 	/* Don't run the guest */
 	bool pause;
 
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 8d1bfa4..10a6baa 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -19,11 +19,41 @@
 
 #include <linux/kvm_host.h>
 
+#include <asm/debug-monitors.h>
+#include <asm/kvm_asm.h>
 #include <asm/kvm_arm.h>
+#include <asm/kvm_emulate.h>
+
+/* These are the bits of MDSCR_EL1 we may manipulate */
+#define MDSCR_EL1_DEBUG_MASK	(DBG_MDSCR_SS | \
+				DBG_MDSCR_KDE | \
+				DBG_MDSCR_MDE)
 
 static DEFINE_PER_CPU(u32, mdcr_el2);
 
 /**
+ * save/restore_guest_debug_regs
+ *
+ * For some debug operations we need to tweak some guest registers. As
+ * a result we need to save the state of those registers before we
+ * make those modifications. This does get confused if the guest
+ * attempts to control single step while being debugged. It will start
+ * working again once it is no longer being debugged by the host.
+ *
+ * Guest access to MDSCR_EL1 is trapped by the hypervisor and handled
+ * after we have restored the preserved value to the main context.
+ */
+static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.guest_debug_preserved.mdscr_el1 = vcpu_sys_reg(vcpu, MDSCR_EL1);
+}
+
+static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
+{
+	vcpu_sys_reg(vcpu, MDSCR_EL1) = vcpu->arch.guest_debug_preserved.mdscr_el1;
+}
+
+/**
  * kvm_arm_init_debug - grab what we need for debug
  *
  * Currently the sole task of this function is to retrieve the initial
@@ -38,7 +68,6 @@ void kvm_arm_init_debug(void)
 	__this_cpu_write(mdcr_el2, kvm_call_hyp(__kvm_get_mdcr_el2));
 }
 
-
 /**
  * kvm_arm_setup_debug - set up debug related stuff
  *
@@ -73,12 +102,33 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 	if (trap_debug)
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 
-	/* Trap breakpoints? */
-	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
+	/* Is Guest debugging in effect? */
+	if (vcpu->guest_debug) {
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
+
+		/* Save guest debug state */
+		save_guest_debug_regs(vcpu);
+
+		/*
+		 * Single Step (ARM ARM D2.12.3 The software step state
+		 * machine)
+		 *
+		 * If we are doing Single Step we need to manipulate
+		 * MDSCR_EL1.SS and PSTATE.SS. If not we need to
+		 * suppress the guests ability to trigger single step
+		 * exceptions as otherwise the host will deal with them.
+		 */
+		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
+			*vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+			vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
+		} else {
+			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
+		}
+	}
 }
 
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
-	/* Nothing to do yet */
+	if (vcpu->guest_debug)
+		restore_guest_debug_regs(vcpu);
 }
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 27f38a9..e9de13e 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -103,6 +103,7 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	run->debug.arch.hsr = hsr;
 
 	switch (hsr >> ESR_ELx_EC_SHIFT) {
+	case ESR_ELx_EC_SOFTSTP_LOW:
 	case ESR_ELx_EC_BKPT32:
 	case ESR_ELx_EC_BRK64:
 		break;
@@ -130,6 +131,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
 	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
 	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
+	[ESR_ELx_EC_SOFTSTP_LOW]= kvm_handle_guest_debug,
 	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
 	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
 };
-- 
2.4.1

[PATCH v5 08/12] KVM: arm64: re-factor hyp.S debug register code

[Alex Bennée]

This is a pre-cursor to sharing the code with the guest debug support.
This replaces the big macro that fishes data out of a fixed location
with a more general helper macro to restore a set of debug registers. It
uses macro substitution so it can be re-used for debug control and value
registers. It does however rely on the debug registers being 64 bit
aligned (as they happen to be in the hyp ABI).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v3:
  - return to the patch series
  - add save and restore targets
  - change register use and document
v4:
  - keep original setup/restore names
  - don't use split u32/u64 structure yet
---
 arch/arm64/kvm/hyp.S | 519 ++++++++++++++-------------------------------------
 1 file changed, 140 insertions(+), 379 deletions(-)

diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 74e63d8..9c4897d 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -228,199 +228,52 @@
 	stp	x24, x25, [x3, #160]
 .endm
 
-.macro save_debug
-	// x2: base address for cpu context
-	// x3: tmp register
-
-	mrs	x26, id_aa64dfr0_el1
-	ubfx	x24, x26, #12, #4	// Extract BRPs
-	ubfx	x25, x26, #20, #4	// Extract WRPs
-	mov	w26, #15
-	sub	w24, w26, w24		// How many BPs to skip
-	sub	w25, w26, w25		// How many WPs to skip
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-1:
-	mrs	x20, dbgbcr15_el1
-	mrs	x19, dbgbcr14_el1
-	mrs	x18, dbgbcr13_el1
-	mrs	x17, dbgbcr12_el1
-	mrs	x16, dbgbcr11_el1
-	mrs	x15, dbgbcr10_el1
-	mrs	x14, dbgbcr9_el1
-	mrs	x13, dbgbcr8_el1
-	mrs	x12, dbgbcr7_el1
-	mrs	x11, dbgbcr6_el1
-	mrs	x10, dbgbcr5_el1
-	mrs	x9, dbgbcr4_el1
-	mrs	x8, dbgbcr3_el1
-	mrs	x7, dbgbcr2_el1
-	mrs	x6, dbgbcr1_el1
-	mrs	x5, dbgbcr0_el1
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-
-1:
-	str	x20, [x3, #(15 * 8)]
-	str	x19, [x3, #(14 * 8)]
-	str	x18, [x3, #(13 * 8)]
-	str	x17, [x3, #(12 * 8)]
-	str	x16, [x3, #(11 * 8)]
-	str	x15, [x3, #(10 * 8)]
-	str	x14, [x3, #(9 * 8)]
-	str	x13, [x3, #(8 * 8)]
-	str	x12, [x3, #(7 * 8)]
-	str	x11, [x3, #(6 * 8)]
-	str	x10, [x3, #(5 * 8)]
-	str	x9, [x3, #(4 * 8)]
-	str	x8, [x3, #(3 * 8)]
-	str	x7, [x3, #(2 * 8)]
-	str	x6, [x3, #(1 * 8)]
-	str	x5, [x3, #(0 * 8)]
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-1:
-	mrs	x20, dbgbvr15_el1
-	mrs	x19, dbgbvr14_el1
-	mrs	x18, dbgbvr13_el1
-	mrs	x17, dbgbvr12_el1
-	mrs	x16, dbgbvr11_el1
-	mrs	x15, dbgbvr10_el1
-	mrs	x14, dbgbvr9_el1
-	mrs	x13, dbgbvr8_el1
-	mrs	x12, dbgbvr7_el1
-	mrs	x11, dbgbvr6_el1
-	mrs	x10, dbgbvr5_el1
-	mrs	x9, dbgbvr4_el1
-	mrs	x8, dbgbvr3_el1
-	mrs	x7, dbgbvr2_el1
-	mrs	x6, dbgbvr1_el1
-	mrs	x5, dbgbvr0_el1
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-
-1:
-	str	x20, [x3, #(15 * 8)]
-	str	x19, [x3, #(14 * 8)]
-	str	x18, [x3, #(13 * 8)]
-	str	x17, [x3, #(12 * 8)]
-	str	x16, [x3, #(11 * 8)]
-	str	x15, [x3, #(10 * 8)]
-	str	x14, [x3, #(9 * 8)]
-	str	x13, [x3, #(8 * 8)]
-	str	x12, [x3, #(7 * 8)]
-	str	x11, [x3, #(6 * 8)]
-	str	x10, [x3, #(5 * 8)]
-	str	x9, [x3, #(4 * 8)]
-	str	x8, [x3, #(3 * 8)]
-	str	x7, [x3, #(2 * 8)]
-	str	x6, [x3, #(1 * 8)]
-	str	x5, [x3, #(0 * 8)]
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
+.macro save_debug type
+	// x4: pointer to register set
+	// x5: number of registers to skip
+	// x6..x22 trashed
+
+	adr	x22, 1f
+	add	x22, x22, x5, lsl #2
+	br	x22
 1:
-	mrs	x20, dbgwcr15_el1
-	mrs	x19, dbgwcr14_el1
-	mrs	x18, dbgwcr13_el1
-	mrs	x17, dbgwcr12_el1
-	mrs	x16, dbgwcr11_el1
-	mrs	x15, dbgwcr10_el1
-	mrs	x14, dbgwcr9_el1
-	mrs	x13, dbgwcr8_el1
-	mrs	x12, dbgwcr7_el1
-	mrs	x11, dbgwcr6_el1
-	mrs	x10, dbgwcr5_el1
-	mrs	x9, dbgwcr4_el1
-	mrs	x8, dbgwcr3_el1
-	mrs	x7, dbgwcr2_el1
-	mrs	x6, dbgwcr1_el1
-	mrs	x5, dbgwcr0_el1
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-
-1:
-	str	x20, [x3, #(15 * 8)]
-	str	x19, [x3, #(14 * 8)]
-	str	x18, [x3, #(13 * 8)]
-	str	x17, [x3, #(12 * 8)]
-	str	x16, [x3, #(11 * 8)]
-	str	x15, [x3, #(10 * 8)]
-	str	x14, [x3, #(9 * 8)]
-	str	x13, [x3, #(8 * 8)]
-	str	x12, [x3, #(7 * 8)]
-	str	x11, [x3, #(6 * 8)]
-	str	x10, [x3, #(5 * 8)]
-	str	x9, [x3, #(4 * 8)]
-	str	x8, [x3, #(3 * 8)]
-	str	x7, [x3, #(2 * 8)]
-	str	x6, [x3, #(1 * 8)]
-	str	x5, [x3, #(0 * 8)]
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
+	mrs	x21, \type\()15_el1
+	mrs	x20, \type\()14_el1
+	mrs	x19, \type\()13_el1
+	mrs	x18, \type\()12_el1
+	mrs	x17, \type\()11_el1
+	mrs	x16, \type\()10_el1
+	mrs	x15, \type\()9_el1
+	mrs	x14, \type\()8_el1
+	mrs	x13, \type\()7_el1
+	mrs	x12, \type\()6_el1
+	mrs	x11, \type\()5_el1
+	mrs	x10, \type\()4_el1
+	mrs	x9, \type\()3_el1
+	mrs	x8, \type\()2_el1
+	mrs	x7, \type\()1_el1
+	mrs	x6, \type\()0_el1
+
+	adr	x22, 1f
+	add	x22, x22, x5, lsl #2
+	br	x22
 1:
-	mrs	x20, dbgwvr15_el1
-	mrs	x19, dbgwvr14_el1
-	mrs	x18, dbgwvr13_el1
-	mrs	x17, dbgwvr12_el1
-	mrs	x16, dbgwvr11_el1
-	mrs	x15, dbgwvr10_el1
-	mrs	x14, dbgwvr9_el1
-	mrs	x13, dbgwvr8_el1
-	mrs	x12, dbgwvr7_el1
-	mrs	x11, dbgwvr6_el1
-	mrs	x10, dbgwvr5_el1
-	mrs	x9, dbgwvr4_el1
-	mrs	x8, dbgwvr3_el1
-	mrs	x7, dbgwvr2_el1
-	mrs	x6, dbgwvr1_el1
-	mrs	x5, dbgwvr0_el1
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-
-1:
-	str	x20, [x3, #(15 * 8)]
-	str	x19, [x3, #(14 * 8)]
-	str	x18, [x3, #(13 * 8)]
-	str	x17, [x3, #(12 * 8)]
-	str	x16, [x3, #(11 * 8)]
-	str	x15, [x3, #(10 * 8)]
-	str	x14, [x3, #(9 * 8)]
-	str	x13, [x3, #(8 * 8)]
-	str	x12, [x3, #(7 * 8)]
-	str	x11, [x3, #(6 * 8)]
-	str	x10, [x3, #(5 * 8)]
-	str	x9, [x3, #(4 * 8)]
-	str	x8, [x3, #(3 * 8)]
-	str	x7, [x3, #(2 * 8)]
-	str	x6, [x3, #(1 * 8)]
-	str	x5, [x3, #(0 * 8)]
-
-	mrs	x21, mdccint_el1
-	str	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
+	str	x21, [x4, #(15 * 8)]
+	str	x20, [x4, #(14 * 8)]
+	str	x19, [x4, #(13 * 8)]
+	str	x18, [x4, #(12 * 8)]
+	str	x17, [x4, #(11 * 8)]
+	str	x16, [x4, #(10 * 8)]
+	str	x15, [x4, #(9 * 8)]
+	str	x14, [x4, #(8 * 8)]
+	str	x13, [x4, #(7 * 8)]
+	str	x12, [x4, #(6 * 8)]
+	str	x11, [x4, #(5 * 8)]
+	str	x10, [x4, #(4 * 8)]
+	str	x9, [x4, #(3 * 8)]
+	str	x8, [x4, #(2 * 8)]
+	str	x7, [x4, #(1 * 8)]
+	str	x6, [x4, #(0 * 8)]
 .endm
 
 .macro restore_sysregs
@@ -465,195 +318,52 @@
 	msr	mdscr_el1,	x25
 .endm
 
-.macro restore_debug
-	// x2: base address for cpu context
-	// x3: tmp register
-
-	mrs	x26, id_aa64dfr0_el1
-	ubfx	x24, x26, #12, #4	// Extract BRPs
-	ubfx	x25, x26, #20, #4	// Extract WRPs
-	mov	w26, #15
-	sub	w24, w26, w24		// How many BPs to skip
-	sub	w25, w26, w25		// How many WPs to skip
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
+.macro restore_debug type
+        // x4: pointer to register set
+        // x5: number of registers to skip
+	// x6..x22 trashed
 
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
+	adr	x22, 1f
+	add	x22, x22, x5, lsl #2
+	br	x22
 1:
-	ldr	x20, [x3, #(15 * 8)]
-	ldr	x19, [x3, #(14 * 8)]
-	ldr	x18, [x3, #(13 * 8)]
-	ldr	x17, [x3, #(12 * 8)]
-	ldr	x16, [x3, #(11 * 8)]
-	ldr	x15, [x3, #(10 * 8)]
-	ldr	x14, [x3, #(9 * 8)]
-	ldr	x13, [x3, #(8 * 8)]
-	ldr	x12, [x3, #(7 * 8)]
-	ldr	x11, [x3, #(6 * 8)]
-	ldr	x10, [x3, #(5 * 8)]
-	ldr	x9, [x3, #(4 * 8)]
-	ldr	x8, [x3, #(3 * 8)]
-	ldr	x7, [x3, #(2 * 8)]
-	ldr	x6, [x3, #(1 * 8)]
-	ldr	x5, [x3, #(0 * 8)]
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
+	ldr	x21, [x4, #(15 * 8)]
+	ldr	x20, [x4, #(14 * 8)]
+	ldr	x19, [x4, #(13 * 8)]
+	ldr	x18, [x4, #(12 * 8)]
+	ldr	x17, [x4, #(11 * 8)]
+	ldr	x16, [x4, #(10 * 8)]
+	ldr	x15, [x4, #(9 * 8)]
+	ldr	x14, [x4, #(8 * 8)]
+	ldr	x13, [x4, #(7 * 8)]
+	ldr	x12, [x4, #(6 * 8)]
+	ldr	x11, [x4, #(5 * 8)]
+	ldr	x10, [x4, #(4 * 8)]
+	ldr	x9, [x4, #(3 * 8)]
+	ldr	x8, [x4, #(2 * 8)]
+	ldr	x7, [x4, #(1 * 8)]
+	ldr	x6, [x4, #(0 * 8)]
+
+	adr	x22, 1f
+	add	x22, x22, x5, lsl #2
+	br	x22
 1:
-	msr	dbgbcr15_el1, x20
-	msr	dbgbcr14_el1, x19
-	msr	dbgbcr13_el1, x18
-	msr	dbgbcr12_el1, x17
-	msr	dbgbcr11_el1, x16
-	msr	dbgbcr10_el1, x15
-	msr	dbgbcr9_el1, x14
-	msr	dbgbcr8_el1, x13
-	msr	dbgbcr7_el1, x12
-	msr	dbgbcr6_el1, x11
-	msr	dbgbcr5_el1, x10
-	msr	dbgbcr4_el1, x9
-	msr	dbgbcr3_el1, x8
-	msr	dbgbcr2_el1, x7
-	msr	dbgbcr1_el1, x6
-	msr	dbgbcr0_el1, x5
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-1:
-	ldr	x20, [x3, #(15 * 8)]
-	ldr	x19, [x3, #(14 * 8)]
-	ldr	x18, [x3, #(13 * 8)]
-	ldr	x17, [x3, #(12 * 8)]
-	ldr	x16, [x3, #(11 * 8)]
-	ldr	x15, [x3, #(10 * 8)]
-	ldr	x14, [x3, #(9 * 8)]
-	ldr	x13, [x3, #(8 * 8)]
-	ldr	x12, [x3, #(7 * 8)]
-	ldr	x11, [x3, #(6 * 8)]
-	ldr	x10, [x3, #(5 * 8)]
-	ldr	x9, [x3, #(4 * 8)]
-	ldr	x8, [x3, #(3 * 8)]
-	ldr	x7, [x3, #(2 * 8)]
-	ldr	x6, [x3, #(1 * 8)]
-	ldr	x5, [x3, #(0 * 8)]
-
-	adr	x26, 1f
-	add	x26, x26, x24, lsl #2
-	br	x26
-1:
-	msr	dbgbvr15_el1, x20
-	msr	dbgbvr14_el1, x19
-	msr	dbgbvr13_el1, x18
-	msr	dbgbvr12_el1, x17
-	msr	dbgbvr11_el1, x16
-	msr	dbgbvr10_el1, x15
-	msr	dbgbvr9_el1, x14
-	msr	dbgbvr8_el1, x13
-	msr	dbgbvr7_el1, x12
-	msr	dbgbvr6_el1, x11
-	msr	dbgbvr5_el1, x10
-	msr	dbgbvr4_el1, x9
-	msr	dbgbvr3_el1, x8
-	msr	dbgbvr2_el1, x7
-	msr	dbgbvr1_el1, x6
-	msr	dbgbvr0_el1, x5
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-1:
-	ldr	x20, [x3, #(15 * 8)]
-	ldr	x19, [x3, #(14 * 8)]
-	ldr	x18, [x3, #(13 * 8)]
-	ldr	x17, [x3, #(12 * 8)]
-	ldr	x16, [x3, #(11 * 8)]
-	ldr	x15, [x3, #(10 * 8)]
-	ldr	x14, [x3, #(9 * 8)]
-	ldr	x13, [x3, #(8 * 8)]
-	ldr	x12, [x3, #(7 * 8)]
-	ldr	x11, [x3, #(6 * 8)]
-	ldr	x10, [x3, #(5 * 8)]
-	ldr	x9, [x3, #(4 * 8)]
-	ldr	x8, [x3, #(3 * 8)]
-	ldr	x7, [x3, #(2 * 8)]
-	ldr	x6, [x3, #(1 * 8)]
-	ldr	x5, [x3, #(0 * 8)]
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-1:
-	msr	dbgwcr15_el1, x20
-	msr	dbgwcr14_el1, x19
-	msr	dbgwcr13_el1, x18
-	msr	dbgwcr12_el1, x17
-	msr	dbgwcr11_el1, x16
-	msr	dbgwcr10_el1, x15
-	msr	dbgwcr9_el1, x14
-	msr	dbgwcr8_el1, x13
-	msr	dbgwcr7_el1, x12
-	msr	dbgwcr6_el1, x11
-	msr	dbgwcr5_el1, x10
-	msr	dbgwcr4_el1, x9
-	msr	dbgwcr3_el1, x8
-	msr	dbgwcr2_el1, x7
-	msr	dbgwcr1_el1, x6
-	msr	dbgwcr0_el1, x5
-
-	add	x3, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-1:
-	ldr	x20, [x3, #(15 * 8)]
-	ldr	x19, [x3, #(14 * 8)]
-	ldr	x18, [x3, #(13 * 8)]
-	ldr	x17, [x3, #(12 * 8)]
-	ldr	x16, [x3, #(11 * 8)]
-	ldr	x15, [x3, #(10 * 8)]
-	ldr	x14, [x3, #(9 * 8)]
-	ldr	x13, [x3, #(8 * 8)]
-	ldr	x12, [x3, #(7 * 8)]
-	ldr	x11, [x3, #(6 * 8)]
-	ldr	x10, [x3, #(5 * 8)]
-	ldr	x9, [x3, #(4 * 8)]
-	ldr	x8, [x3, #(3 * 8)]
-	ldr	x7, [x3, #(2 * 8)]
-	ldr	x6, [x3, #(1 * 8)]
-	ldr	x5, [x3, #(0 * 8)]
-
-	adr	x26, 1f
-	add	x26, x26, x25, lsl #2
-	br	x26
-1:
-	msr	dbgwvr15_el1, x20
-	msr	dbgwvr14_el1, x19
-	msr	dbgwvr13_el1, x18
-	msr	dbgwvr12_el1, x17
-	msr	dbgwvr11_el1, x16
-	msr	dbgwvr10_el1, x15
-	msr	dbgwvr9_el1, x14
-	msr	dbgwvr8_el1, x13
-	msr	dbgwvr7_el1, x12
-	msr	dbgwvr6_el1, x11
-	msr	dbgwvr5_el1, x10
-	msr	dbgwvr4_el1, x9
-	msr	dbgwvr3_el1, x8
-	msr	dbgwvr2_el1, x7
-	msr	dbgwvr1_el1, x6
-	msr	dbgwvr0_el1, x5
-
-	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
-	msr	mdccint_el1, x21
+	msr	\type\()15_el1, x21
+	msr	\type\()14_el1, x20
+	msr	\type\()13_el1, x19
+	msr	\type\()12_el1, x18
+	msr	\type\()11_el1, x17
+	msr	\type\()10_el1, x16
+	msr	\type\()9_el1, x15
+	msr	\type\()8_el1, x14
+	msr	\type\()7_el1, x13
+	msr	\type\()6_el1, x12
+	msr	\type\()5_el1, x11
+	msr	\type\()4_el1, x10
+	msr	\type\()3_el1, x9
+	msr	\type\()2_el1, x8
+	msr	\type\()1_el1, x7
+	msr	\type\()0_el1, x6
 .endm
 
 .macro skip_32bit_state tmp, target
@@ -887,12 +597,63 @@ __restore_sysregs:
 	restore_sysregs
 	ret
 
+/* Save debug state */
 __save_debug:
-	save_debug
+	// x0: base address for vcpu context
+	// x2: ptr to current CPU context
+	// x4/x5: trashed
+
+	mrs	x26, id_aa64dfr0_el1
+	ubfx	x24, x26, #12, #4	// Extract BRPs
+	ubfx	x25, x26, #20, #4	// Extract WRPs
+	mov	w26, #15
+	sub	w24, w26, w24		// How many BPs to skip
+	sub	w25, w26, w25		// How many WPs to skip
+
+	mov     x5, x24
+	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
+	save_debug dbgbcr
+	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
+	save_debug dbgbvr
+
+	mov     x5, x25
+	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
+	save_debug dbgwcr
+	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
+	save_debug dbgwvr
+
+	mrs	x21, mdccint_el1
+	str	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
 	ret
 
+/* Restore debug state */
 __restore_debug:
-	restore_debug
+	// x0: base address for cpu context
+	// x2: ptr to current CPU context
+	// x4/x5: trashed
+
+	mrs	x26, id_aa64dfr0_el1
+	ubfx	x24, x26, #12, #4	// Extract BRPs
+	ubfx	x25, x26, #20, #4	// Extract WRPs
+	mov	w26, #15
+	sub	w24, w26, w24		// How many BPs to skip
+	sub	w25, w26, w25		// How many WPs to skip
+
+	mov     x5, x24
+	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
+	restore_debug dbgbcr
+	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
+	restore_debug dbgbvr
+
+	mov     x5, x25
+	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
+	restore_debug dbgwcr
+	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
+	restore_debug dbgwvr
+
+	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
+	msr	mdccint_el1, x21
+
 	ret
 
 __save_fpsimd:
-- 
2.4.1

[PATCH v5 09/12] KVM: arm64: introduce vcpu->arch.debug_ptr

[Alex Bennée]

This introduces a level of indirection for the debug registers. Instead
of using the sys_regs[] directly we store registers in a structure in
the vcpu. As we are no longer tied to the layout of the sys_regs[] we
can make the copies size appropriate for control and value registers.

This also entails updating the sys_regs code to access this new
structure. Instead of passing a register index we now pass an offset
into the kvm_guest_debug_arch structure.

We also need to ensure the GET/SET_ONE_REG ioctl operations store the
registers in their correct location.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 arch/arm/kvm/arm.c                |   3 +
 arch/arm64/include/asm/kvm_asm.h  |  24 +++-----
 arch/arm64/include/asm/kvm_host.h |  12 +++-
 arch/arm64/kernel/asm-offsets.c   |   6 ++
 arch/arm64/kvm/hyp.S              | 107 +++++++++++++++++---------------
 arch/arm64/kvm/sys_regs.c         | 126 +++++++++++++++++++++++++++++++-------
 6 files changed, 188 insertions(+), 90 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 9b3ed6d..0d17c7b 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -279,6 +279,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 	/* Set up the timer */
 	kvm_timer_vcpu_init(vcpu);
 
+	/* Set the debug registers to be the guests */
+	vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
+
 	return 0;
 }
 
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index d6b507e..e997404 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -46,24 +46,16 @@
 #define	CNTKCTL_EL1	20	/* Timer Control Register (EL1) */
 #define	PAR_EL1		21	/* Physical Address Register */
 #define MDSCR_EL1	22	/* Monitor Debug System Control Register */
-#define DBGBCR0_EL1	23	/* Debug Breakpoint Control Registers (0-15) */
-#define DBGBCR15_EL1	38
-#define DBGBVR0_EL1	39	/* Debug Breakpoint Value Registers (0-15) */
-#define DBGBVR15_EL1	54
-#define DBGWCR0_EL1	55	/* Debug Watchpoint Control Registers (0-15) */
-#define DBGWCR15_EL1	70
-#define DBGWVR0_EL1	71	/* Debug Watchpoint Value Registers (0-15) */
-#define DBGWVR15_EL1	86
-#define MDCCINT_EL1	87	/* Monitor Debug Comms Channel Interrupt Enable Reg */
+#define MDCCINT_EL1	23	/* Monitor Debug Comms Channel Interrupt Enable Reg */
 
 /* 32bit specific registers. Keep them at the end of the range */
-#define	DACR32_EL2	88	/* Domain Access Control Register */
-#define	IFSR32_EL2	89	/* Instruction Fault Status Register */
-#define	FPEXC32_EL2	90	/* Floating-Point Exception Control Register */
-#define	DBGVCR32_EL2	91	/* Debug Vector Catch Register */
-#define	TEECR32_EL1	92	/* ThumbEE Configuration Register */
-#define	TEEHBR32_EL1	93	/* ThumbEE Handler Base Register */
-#define	NR_SYS_REGS	94
+#define	DACR32_EL2	24	/* Domain Access Control Register */
+#define	IFSR32_EL2	25	/* Instruction Fault Status Register */
+#define	FPEXC32_EL2	26	/* Floating-Point Exception Control Register */
+#define	DBGVCR32_EL2	27	/* Debug Vector Catch Register */
+#define	TEECR32_EL1	28	/* ThumbEE Configuration Register */
+#define	TEEHBR32_EL1	29	/* ThumbEE Handler Base Register */
+#define	NR_SYS_REGS	30
 
 /* 32bit mapping */
 #define c0_MPIDR	(MPIDR_EL1 * 2)	/* MultiProcessor ID Register */
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e2db6a6..e5040b6 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -108,11 +108,21 @@ struct kvm_vcpu_arch {
 	/* Exception Information */
 	struct kvm_vcpu_fault_info fault;
 
-	/* Debug state */
+	/* Guest debug state */
 	u64 debug_flags;
 
+	/*
+	 * For debugging the guest we need to keep a set of debug
+	 * registers which can override the guests own debug state
+	 * while being used. These are set via the KVM_SET_GUEST_DEBUG
+	 * ioctl.
+	 */
+	struct kvm_guest_debug_arch *debug_ptr;
+	struct kvm_guest_debug_arch vcpu_debug_state;
+
 	/* Pointer to host CPU context */
 	kvm_cpu_context_t *host_cpu_context;
+	struct kvm_guest_debug_arch host_debug_state;
 
 	/* VGIC state */
 	struct vgic_cpu vgic_cpu;
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index dfb25a2..1a8e97c 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -116,10 +116,16 @@ int main(void)
   DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
   DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
   DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
+  DEFINE(VCPU_DEBUG_PTR,	offsetof(struct kvm_vcpu, arch.debug_ptr));
+  DEFINE(DEBUG_BCR, 		offsetof(struct kvm_guest_debug_arch, dbg_bcr));
+  DEFINE(DEBUG_BVR, 		offsetof(struct kvm_guest_debug_arch, dbg_bvr));
+  DEFINE(DEBUG_WCR, 		offsetof(struct kvm_guest_debug_arch, dbg_wcr));
+  DEFINE(DEBUG_WVR, 		offsetof(struct kvm_guest_debug_arch, dbg_wvr));
   DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
   DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
   DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
+  DEFINE(VCPU_HOST_DEBUG_STATE, offsetof(struct kvm_vcpu, arch.host_debug_state));
   DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
   DEFINE(VCPU_TIMER_CNTV_CVAL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval));
   DEFINE(KVM_TIMER_CNTVOFF,	offsetof(struct kvm, arch.timer.cntvoff));
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 9c4897d..1940a4c 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -228,7 +228,7 @@
 	stp	x24, x25, [x3, #160]
 .endm
 
-.macro save_debug type
+.macro save_debug type regsz offsz
 	// x4: pointer to register set
 	// x5: number of registers to skip
 	// x6..x22 trashed
@@ -258,22 +258,22 @@
 	add	x22, x22, x5, lsl #2
 	br	x22
 1:
-	str	x21, [x4, #(15 * 8)]
-	str	x20, [x4, #(14 * 8)]
-	str	x19, [x4, #(13 * 8)]
-	str	x18, [x4, #(12 * 8)]
-	str	x17, [x4, #(11 * 8)]
-	str	x16, [x4, #(10 * 8)]
-	str	x15, [x4, #(9 * 8)]
-	str	x14, [x4, #(8 * 8)]
-	str	x13, [x4, #(7 * 8)]
-	str	x12, [x4, #(6 * 8)]
-	str	x11, [x4, #(5 * 8)]
-	str	x10, [x4, #(4 * 8)]
-	str	x9, [x4, #(3 * 8)]
-	str	x8, [x4, #(2 * 8)]
-	str	x7, [x4, #(1 * 8)]
-	str	x6, [x4, #(0 * 8)]
+	str	\regsz\()21, [x4, #(15 * \offsz)]
+	str	\regsz\()20, [x4, #(14 * \offsz)]
+	str	\regsz\()19, [x4, #(13 * \offsz)]
+	str	\regsz\()18, [x4, #(12 * \offsz)]
+	str	\regsz\()17, [x4, #(11 * \offsz)]
+	str	\regsz\()16, [x4, #(10 * \offsz)]
+	str	\regsz\()15, [x4, #(9 * \offsz)]
+	str	\regsz\()14, [x4, #(8 * \offsz)]
+	str	\regsz\()13, [x4, #(7 * \offsz)]
+	str	\regsz\()12, [x4, #(6 * \offsz)]
+	str	\regsz\()11, [x4, #(5 * \offsz)]
+	str	\regsz\()10, [x4, #(4 * \offsz)]
+	str	\regsz\()9, [x4, #(3 * \offsz)]
+	str	\regsz\()8, [x4, #(2 * \offsz)]
+	str	\regsz\()7, [x4, #(1 * \offsz)]
+	str	\regsz\()6, [x4, #(0 * \offsz)]
 .endm
 
 .macro restore_sysregs
@@ -318,7 +318,7 @@
 	msr	mdscr_el1,	x25
 .endm
 
-.macro restore_debug type
+.macro restore_debug type regsz offsz
         // x4: pointer to register set
         // x5: number of registers to skip
 	// x6..x22 trashed
@@ -327,22 +327,22 @@
 	add	x22, x22, x5, lsl #2
 	br	x22
 1:
-	ldr	x21, [x4, #(15 * 8)]
-	ldr	x20, [x4, #(14 * 8)]
-	ldr	x19, [x4, #(13 * 8)]
-	ldr	x18, [x4, #(12 * 8)]
-	ldr	x17, [x4, #(11 * 8)]
-	ldr	x16, [x4, #(10 * 8)]
-	ldr	x15, [x4, #(9 * 8)]
-	ldr	x14, [x4, #(8 * 8)]
-	ldr	x13, [x4, #(7 * 8)]
-	ldr	x12, [x4, #(6 * 8)]
-	ldr	x11, [x4, #(5 * 8)]
-	ldr	x10, [x4, #(4 * 8)]
-	ldr	x9, [x4, #(3 * 8)]
-	ldr	x8, [x4, #(2 * 8)]
-	ldr	x7, [x4, #(1 * 8)]
-	ldr	x6, [x4, #(0 * 8)]
+	ldr	\regsz\()21, [x4, #(15 * \offsz)]
+	ldr	\regsz\()20, [x4, #(14 * \offsz)]
+	ldr	\regsz\()19, [x4, #(13 * \offsz)]
+	ldr	\regsz\()18, [x4, #(12 * \offsz)]
+	ldr	\regsz\()17, [x4, #(11 * \offsz)]
+	ldr	\regsz\()16, [x4, #(10 * \offsz)]
+	ldr	\regsz\()15, [x4, #(9 * \offsz)]
+	ldr	\regsz\()14, [x4, #(8 * \offsz)]
+	ldr	\regsz\()13, [x4, #(7 * \offsz)]
+	ldr	\regsz\()12, [x4, #(6 * \offsz)]
+	ldr	\regsz\()11, [x4, #(5 * \offsz)]
+	ldr	\regsz\()10, [x4, #(4 * \offsz)]
+	ldr	\regsz\()9, [x4, #(3 * \offsz)]
+	ldr	\regsz\()8, [x4, #(2 * \offsz)]
+	ldr	\regsz\()7, [x4, #(1 * \offsz)]
+	ldr	\regsz\()6, [x4, #(0 * \offsz)]
 
 	adr	x22, 1f
 	add	x22, x22, x5, lsl #2
@@ -601,6 +601,7 @@ __restore_sysregs:
 __save_debug:
 	// x0: base address for vcpu context
 	// x2: ptr to current CPU context
+	// x3: ptr to debug reg struct
 	// x4/x5: trashed
 
 	mrs	x26, id_aa64dfr0_el1
@@ -611,16 +612,16 @@ __save_debug:
 	sub	w25, w26, w25		// How many WPs to skip
 
 	mov     x5, x24
-	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
-	save_debug dbgbcr
-	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
-	save_debug dbgbvr
+	add	x4, x3, #DEBUG_BCR
+	save_debug dbgbcr w 4
+	add	x4, x3, #DEBUG_BVR
+	save_debug dbgbvr x 8
 
 	mov     x5, x25
-	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
-	save_debug dbgwcr
-	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
-	save_debug dbgwvr
+	add	x4, x3, #DEBUG_WCR
+	save_debug dbgwcr w 4
+	add	x4, x3, #DEBUG_WVR
+	save_debug dbgwvr x 8
 
 	mrs	x21, mdccint_el1
 	str	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
@@ -640,16 +641,16 @@ __restore_debug:
 	sub	w25, w26, w25		// How many WPs to skip
 
 	mov     x5, x24
-	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
-	restore_debug dbgbcr
-	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
-	restore_debug dbgbvr
+	add	x4, x3, #DEBUG_BCR
+	restore_debug dbgbcr w 4
+	add	x4, x3, #DEBUG_BVR
+	restore_debug dbgbvr x 8
 
 	mov     x5, x25
-	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
-	restore_debug dbgwcr
-	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
-	restore_debug dbgwvr
+	add	x4, x3, #DEBUG_WCR
+	restore_debug dbgwcr w 4
+	add	x4, x3, #DEBUG_WVR
+	restore_debug dbgwvr x 8
 
 	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
 	msr	mdccint_el1, x21
@@ -688,6 +689,7 @@ ENTRY(__kvm_vcpu_run)
 	bl __save_sysregs
 
 	compute_debug_state 1f
+	add	x3, x0, #VCPU_HOST_DEBUG_STATE
 	bl	__save_debug
 1:
 	activate_traps
@@ -703,6 +705,8 @@ ENTRY(__kvm_vcpu_run)
 	bl __restore_fpsimd
 
 	skip_debug_state x3, 1f
+	ldr	x3, [x0, #VCPU_DEBUG_PTR]
+	kern_hyp_va x3
 	bl	__restore_debug
 1:
 	restore_guest_32bit_state
@@ -723,6 +727,8 @@ __kvm_vcpu_return:
 	bl __save_sysregs
 
 	skip_debug_state x3, 1f
+	ldr	x3, [x0, #VCPU_DEBUG_PTR]
+	kern_hyp_va x3
 	bl	__save_debug
 1:
 	save_guest_32bit_state
@@ -745,6 +751,7 @@ __kvm_vcpu_return:
 	// already been saved. Note that we nuke the whole 64bit word.
 	// If we ever add more flags, we'll have to be more careful...
 	str	xzr, [x0, #VCPU_DEBUG_FLAGS]
+	add	x3, x0, #VCPU_HOST_DEBUG_STATE
 	bl	__restore_debug
 1:
 	restore_host_regs
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c370b40..405403e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -211,6 +211,48 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static inline bool trap_debug32(struct kvm_vcpu *vcpu,
+				const struct sys_reg_params *p,
+				const struct sys_reg_desc *rd)
+{
+	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
+	if (p->is_write) {
+		*r = *vcpu_reg(vcpu, p->Rt);
+		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
+	} else {
+		*vcpu_reg(vcpu, p->Rt) = *r;
+	}
+
+	return true;
+}
+
+static inline bool trap_debug64(struct kvm_vcpu *vcpu,
+				const struct sys_reg_params *p,
+				const struct sys_reg_desc *rd)
+{
+	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
+	if (p->is_write) {
+		*r = *vcpu_reg(vcpu, p->Rt);
+		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
+	} else {
+		*vcpu_reg(vcpu, p->Rt) = *r;
+	}
+
+	return true;
+}
+
+static inline void reset_debug32(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
+{
+	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
+	*r = rd->val;
+}
+
+static inline void reset_debug64(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
+{
+	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
+	*r = rd->val;
+}
+
 static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 amair;
@@ -240,16 +282,20 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
 	/* DBGBVRn_EL1 */						\
 	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b100),	\
-	  trap_debug_regs, reset_val, (DBGBVR0_EL1 + (n)), 0 },		\
+	  trap_debug64, reset_debug64,					\
+	  offsetof(struct kvm_guest_debug_arch, dbg_bvr[(n)]), 0 },	\
 	/* DBGBCRn_EL1 */						\
 	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b101),	\
-	  trap_debug_regs, reset_val, (DBGBCR0_EL1 + (n)), 0 },		\
+	  trap_debug32, reset_debug32,					\
+	  offsetof(struct kvm_guest_debug_arch, dbg_bcr[(n)]), 0},	\
 	/* DBGWVRn_EL1 */						\
 	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b110),	\
-	  trap_debug_regs, reset_val, (DBGWVR0_EL1 + (n)), 0 },		\
+	  trap_debug64, reset_debug64,					\
+	  offsetof(struct kvm_guest_debug_arch, dbg_wvr[(n)]), 0 },	\
 	/* DBGWCRn_EL1 */						\
 	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b111),	\
-	  trap_debug_regs, reset_val, (DBGWCR0_EL1 + (n)), 0 }
+	  trap_debug32, reset_debug32,					\
+	  offsetof(struct kvm_guest_debug_arch, dbg_wcr[(n)]), 0}
 
 /*
  * Architected system registers.
@@ -502,37 +548,23 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
 	}
 }
 
-static bool trap_debug32(struct kvm_vcpu *vcpu,
-			 const struct sys_reg_params *p,
-			 const struct sys_reg_desc *r)
-{
-	if (p->is_write) {
-		vcpu_cp14(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
-		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
-	} else {
-		*vcpu_reg(vcpu, p->Rt) = vcpu_cp14(vcpu, r->reg);
-	}
-
-	return true;
-}
-
 #define DBG_BCR_BVR_WCR_WVR(n)					\
 	/* DBGBVRn */						\
 	{ Op1( 0), CRn( 0), CRm((n)), Op2( 4), trap_debug32,	\
-	  NULL, (cp14_DBGBVR0 + (n) * 2) },			\
+	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_bvr[(n)]) },	\
 	/* DBGBCRn */						\
 	{ Op1( 0), CRn( 0), CRm((n)), Op2( 5), trap_debug32,	\
-	  NULL, (cp14_DBGBCR0 + (n) * 2) },			\
+	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_bcr[(n)]) },			\
 	/* DBGWVRn */						\
 	{ Op1( 0), CRn( 0), CRm((n)), Op2( 6), trap_debug32,	\
-	  NULL, (cp14_DBGWVR0 + (n) * 2) },			\
+	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_wvr[(n)]) },			\
 	/* DBGWCRn */						\
 	{ Op1( 0), CRn( 0), CRm((n)), Op2( 7), trap_debug32,	\
-	  NULL, (cp14_DBGWCR0 + (n) * 2) }
+	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_wcr[(n)]) }
 
 #define DBGBXVR(n)						\
 	{ Op1( 0), CRn( 1), CRm((n)), Op2( 1), trap_debug32,	\
-	  NULL, cp14_DBGBXVR0 + n * 2 }
+	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_bvr[(n)])+sizeof(__u32) }
 
 /*
  * Trapped cp14 registers. We generally ignore most of the external
@@ -1288,6 +1320,42 @@ static int demux_c15_set(u64 id, void __user *uaddr)
 	}
 }
 
+static int debug_set32(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+		const struct kvm_one_reg *reg, void __user *uaddr)
+{
+	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
+	if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
+		return -EFAULT;
+	return 0;
+}
+
+static int debug_set64(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+		const struct kvm_one_reg *reg, void __user *uaddr)
+{
+	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
+	if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
+		return -EFAULT;
+	return 0;
+}
+
+static int debug_get32(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+		const struct kvm_one_reg *reg, void __user *uaddr)
+{
+	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
+	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
+		return -EFAULT;
+	return 0;
+}
+
+static int debug_get64(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+	const struct kvm_one_reg *reg, void __user *uaddr)
+{
+	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
+	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
+		return -EFAULT;
+	return 0;
+}
+
 int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	const struct sys_reg_desc *r;
@@ -1303,6 +1371,12 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 	if (!r)
 		return get_invariant_sys_reg(reg->id, uaddr);
 
+	if (r->access == trap_debug64)
+		return debug_get64(vcpu, r, reg, uaddr);
+
+	if (r->access == trap_debug32)
+		return debug_get32(vcpu, r, reg, uaddr);
+
 	return reg_to_user(uaddr, &vcpu_sys_reg(vcpu, r->reg), reg->id);
 }
 
@@ -1321,6 +1395,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 	if (!r)
 		return set_invariant_sys_reg(reg->id, uaddr);
 
+	if (r->access == trap_debug64)
+		return debug_set64(vcpu, r, reg, uaddr);
+
+	if (r->access == trap_debug32)
+		return debug_set32(vcpu, r, reg, uaddr);
+
 	return reg_from_user(&vcpu_sys_reg(vcpu, r->reg), uaddr, reg->id);
 }
 
-- 
2.4.1

[PATCH v5 10/12] KVM: arm64: guest debug, HW assisted debug support

[Alex Bennée]

This adds support for userspace to control the HW debug registers for
guest debug. In the debug ioctl we copy the IMPDEF defined number of
registers into a new register set called host_debug_state. There is now
a new vcpu parameter called debug_ptr which selects which register set
is to copied into the real registers when world switch occurs.

I've moved some helper functions into the hw_breakpoint.h header for
re-use.

As with single step we need to tweak the guest registers to enable the
exceptions so we need to save and restore those bits.

Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
userspace to query the number of hardware break and watch points
available on the host hardware.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
   - switched to C setup
   - replace host debug registers directly into context
   - minor tweak to api docs
   - setup right register for debug
   - add FAR_EL2 to debug exit structure
   - add support for trapping debug register access
v3
   - remove stray trace statement
   - fix spacing around operators (various)
   - clean-up usage of trap_debug
   - introduce debug_ptr, replace excessive memcpy stuff
   - don't use memcpy in ioctl, just assign
   - update cap ioctl documentation
   - reword a number comments
   - rename host_debug_state->external_debug_state
v4
   - use the new u32/u64 split debug_ptr approach
   - fix some wording/comments
v5
   - don't set MDSCR_EL1.KDE (not needed)
---
 Documentation/virtual/kvm/api.txt      |  7 ++++++-
 arch/arm/kvm/arm.c                     |  7 +++++++
 arch/arm64/include/asm/hw_breakpoint.h | 12 +++++++++++
 arch/arm64/include/asm/kvm_host.h      |  3 ++-
 arch/arm64/include/uapi/asm/kvm.h      |  2 +-
 arch/arm64/kernel/hw_breakpoint.c      | 12 -----------
 arch/arm64/kvm/debug.c                 | 37 +++++++++++++++++++++++++++++-----
 arch/arm64/kvm/handle_exit.c           |  6 ++++++
 arch/arm64/kvm/reset.c                 | 12 +++++++++++
 include/uapi/linux/kvm.h               |  2 ++
 10 files changed, 80 insertions(+), 20 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 33c8143..ada57df 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2668,7 +2668,7 @@ The top 16 bits of the control field are architecture specific control
 flags which can include the following:
 
   - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
-  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
+  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390, arm64]
   - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
   - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
   - KVM_GUESTDBG_EXIT_PENDING:  trigger an immediate guest exit [s390]
@@ -2683,6 +2683,11 @@ updated to the correct (supplied) values.
 The second part of the structure is architecture specific and
 typically contains a set of debug registers.
 
+For arm64 the number of debug registers is implementation defined and
+can be determined by querying the KVM_CAP_GUEST_DEBUG_HW_BPS and
+KVM_CAP_GUEST_DEBUG_HW_WPS capabilities which return a positive number
+indicating the number of supported registers.
+
 When debug events exit the main run loop with the reason
 KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
 structure containing architecture specific debug information.
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 0d17c7b..6df47c1 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -307,6 +307,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 
 #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |    \
 			    KVM_GUESTDBG_USE_SW_BP | \
+			    KVM_GUESTDBG_USE_HW_BP | \
 			    KVM_GUESTDBG_SINGLESTEP)
 
 /**
@@ -327,6 +328,12 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 
 	if (dbg->control & KVM_GUESTDBG_ENABLE) {
 		vcpu->guest_debug = dbg->control;
+
+		/* Hardware assisted Break and Watch points */
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
+			vcpu->arch.external_debug_state = dbg->arch;
+		}
+
 	} else {
 		/* If not enabled clear all flags */
 		vcpu->guest_debug = 0;
diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index 52b484b..c450552 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
 }
 #endif
 
+/* Determine number of BRP registers available. */
+static inline int get_num_brps(void)
+{
+	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
+}
+
+/* Determine number of WRP registers available. */
+static inline int get_num_wrps(void)
+{
+	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
+}
+
 extern struct pmu perf_ops_bp;
 
 #endif	/* __KERNEL__ */
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e5040b6..498d4f7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -113,12 +113,13 @@ struct kvm_vcpu_arch {
 
 	/*
 	 * For debugging the guest we need to keep a set of debug
-	 * registers which can override the guests own debug state
+	 * registers which can override the guest's own debug state
 	 * while being used. These are set via the KVM_SET_GUEST_DEBUG
 	 * ioctl.
 	 */
 	struct kvm_guest_debug_arch *debug_ptr;
 	struct kvm_guest_debug_arch vcpu_debug_state;
+	struct kvm_guest_debug_arch external_debug_state;
 
 	/* Pointer to host CPU context */
 	kvm_cpu_context_t *host_cpu_context;
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 43758e7..95168c2 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -116,7 +116,7 @@ struct kvm_guest_debug_arch {
 
 struct kvm_debug_exit_arch {
 	__u32 hsr;
-	__u64 far;
+	__u64 far;	/* used for watchpoints */
 };
 
 /*
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index e7d934d..3a41bbf 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -49,18 +49,6 @@ static DEFINE_PER_CPU(int, stepping_kernel_bp);
 static int core_num_brps;
 static int core_num_wrps;
 
-/* Determine number of BRP registers available. */
-static int get_num_brps(void)
-{
-	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
-}
-
-/* Determine number of WRP registers available. */
-static int get_num_wrps(void)
-{
-	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
-}
-
 int hw_breakpoint_slots(int type)
 {
 	/*
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 10a6baa..3c0daae 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -98,10 +98,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 				MDCR_EL2_TDRA |
 				MDCR_EL2_TDOSA);
 
-	/* Trap on access to debug registers? */
-	if (trap_debug)
-		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
-
 	/* Is Guest debugging in effect? */
 	if (vcpu->guest_debug) {
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
@@ -124,11 +120,42 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 		} else {
 			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
 		}
+
+		/*
+		 * HW Break/Watch points
+		 *
+		 * We simply switch the debug_ptr to point to our new
+		 * external_debug_state which has been populated by the
+		 * debug ioctl. The existing KVM_ARM64_DEBUG_DIRTY
+		 * mechanism ensures the registers are updated on the
+		 * world switch.
+		 */
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
+			/* Enable breakpoints/watchpoints */
+			vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_MDE;
+
+			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
+			vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
+			trap_debug = true;
+		}
 	}
+
+	/* Trap debug register access */
+	if (trap_debug)
+		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 }
 
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
-	if (vcpu->guest_debug)
+	if (vcpu->guest_debug) {
 		restore_guest_debug_regs(vcpu);
+
+		/*
+		 * If we were using HW debug we need to restore the
+		 * debug_ptr to the guest debug state.
+		 */
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+			vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
+
+	}
 }
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index e9de13e..68a0759 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -103,7 +103,11 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	run->debug.arch.hsr = hsr;
 
 	switch (hsr >> ESR_ELx_EC_SHIFT) {
+	case ESR_ELx_EC_WATCHPT_LOW:
+		run->debug.arch.far = vcpu->arch.fault.far_el2;
+		/* fall through */
 	case ESR_ELx_EC_SOFTSTP_LOW:
+	case ESR_ELx_EC_BREAKPT_LOW:
 	case ESR_ELx_EC_BKPT32:
 	case ESR_ELx_EC_BRK64:
 		break;
@@ -132,6 +136,8 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
 	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
 	[ESR_ELx_EC_SOFTSTP_LOW]= kvm_handle_guest_debug,
+	[ESR_ELx_EC_WATCHPT_LOW]= kvm_handle_guest_debug,
+	[ESR_ELx_EC_BREAKPT_LOW]= kvm_handle_guest_debug,
 	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
 	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
 };
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 0b43265..21d5a62 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -56,6 +56,12 @@ static bool cpu_has_32bit_el1(void)
 	return !!(pfr0 & 0x20);
 }
 
+/**
+ * kvm_arch_dev_ioctl_check_extension
+ *
+ * We currently assume that the number of HW registers is uniform
+ * across all CPUs (see cpuinfo_sanity_check).
+ */
 int kvm_arch_dev_ioctl_check_extension(long ext)
 {
 	int r;
@@ -64,6 +70,12 @@ int kvm_arch_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_ARM_EL1_32BIT:
 		r = cpu_has_32bit_el1();
 		break;
+	case KVM_CAP_GUEST_DEBUG_HW_BPS:
+		r = get_num_brps();
+		break;
+	case KVM_CAP_GUEST_DEBUG_HW_WPS:
+		r  = get_num_wrps();
+		break;
 	default:
 		r = 0;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 70ac641..f020dd0 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -817,6 +817,8 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_S390_INJECT_IRQ 113
 #define KVM_CAP_S390_IRQ_STATE 114
 #define KVM_CAP_PPC_HWRNG 115
+#define KVM_CAP_GUEST_DEBUG_HW_BPS 116
+#define KVM_CAP_GUEST_DEBUG_HW_WPS 117
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.4.1

[PATCH v5 11/12] KVM: arm64: enable KVM_CAP_SET_GUEST_DEBUG

[Alex Bennée]

Finally advertise the KVM capability for SET_GUEST_DEBUG. Once arm
support is added this check can be moved to the common
kvm_vm_ioctl_check_extension() code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

---

v3:
 - separated capability check from previous patches
 - moved into arm64 specific ioctl handler.
v4:
 - add a-b-tag
---
 arch/arm64/kvm/reset.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 21d5a62..88e5331 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -76,6 +76,9 @@ int kvm_arch_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_GUEST_DEBUG_HW_WPS:
 		r  = get_num_wrps();
 		break;
+	case KVM_CAP_SET_GUEST_DEBUG:
+		r = 1;
+		break;
 	default:
 		r = 0;
 	}
-- 
2.4.1

[PATCH v5 12/12] KVM: arm64: add trace points for guest_debug debug

[Alex Bennée]

This includes trace points for:
  kvm_arch_setup_guest_debug
  kvm_arch_clear_guest_debug

I've also added some generic register setting trace events and also a
trace point to dump the array of hardware registers.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v3
  - add trace event for debug access.
  - remove short trace #define, rename trace events
  - use __print_array with fixed array instead of own func
  - rationalise trace points (only one per register changed)
  - add vcpu ptr to the debug_setup trace
  - remove :: in prints
v4
  - u32/u64 split on debug registers
  - fix for renames
  - add tracing of traps/set_guest_debug
  - remove handle_guest_debug trace
v5
  - minor print fmt fix
  - rm pstate traces
---
 arch/arm/kvm/arm.c        |   2 +
 arch/arm/kvm/trace.h      |  17 ++++++++
 arch/arm64/kvm/debug.c    |  35 +++++++++++++++-
 arch/arm64/kvm/sys_regs.c |  10 +++++
 arch/arm64/kvm/trace.h    | 105 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 168 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 6df47c1..a939a4e 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -323,6 +323,8 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
 {
+	trace_kvm_set_guest_debug(vcpu, dbg->control);
+
 	if (dbg->control & ~KVM_GUESTDBG_VALID_MASK)
 		return -EINVAL;
 
diff --git a/arch/arm/kvm/trace.h b/arch/arm/kvm/trace.h
index 0ec3539..3e346a6 100644
--- a/arch/arm/kvm/trace.h
+++ b/arch/arm/kvm/trace.h
@@ -317,6 +317,23 @@ TRACE_EVENT(kvm_toggle_cache,
 		      __entry->now ? "on" : "off")
 );
 
+TRACE_EVENT(kvm_set_guest_debug,
+	TP_PROTO(struct kvm_vcpu *vcpu, __u32 guest_debug),
+	TP_ARGS(vcpu, guest_debug),
+
+	TP_STRUCT__entry(
+		__field(struct kvm_vcpu *, vcpu)
+		__field(__u32, guest_debug)
+	),
+
+	TP_fast_assign(
+		__entry->vcpu = vcpu;
+		__entry->guest_debug = guest_debug;
+	),
+
+	TP_printk("vcpu: %p, flags: 0x%08x", __entry->vcpu, __entry->guest_debug)
+);
+
 #endif /* _TRACE_KVM_H */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 3c0daae..97204b5 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -24,6 +24,8 @@
 #include <asm/kvm_arm.h>
 #include <asm/kvm_emulate.h>
 
+#include "trace.h"
+
 /* These are the bits of MDSCR_EL1 we may manipulate */
 #define MDSCR_EL1_DEBUG_MASK	(DBG_MDSCR_SS | \
 				DBG_MDSCR_KDE | \
@@ -46,11 +48,17 @@ static DEFINE_PER_CPU(u32, mdcr_el2);
 static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.guest_debug_preserved.mdscr_el1 = vcpu_sys_reg(vcpu, MDSCR_EL1);
+
+	trace_kvm_arm_set_dreg32("Saved MDSCR_EL1",
+				vcpu->arch.guest_debug_preserved.mdscr_el1);
 }
 
 static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 {
 	vcpu_sys_reg(vcpu, MDSCR_EL1) = vcpu->arch.guest_debug_preserved.mdscr_el1;
+
+	trace_kvm_arm_set_dreg32("Restored MDSCR_EL1",
+				vcpu_sys_reg(vcpu, MDSCR_EL1));
 }
 
 /**
@@ -92,6 +100,8 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 {
 	bool trap_debug = !(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY);
 
+	trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug);
+
 	vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK;
 	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
 				MDCR_EL2_TPMCR |
@@ -121,6 +131,8 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
 		}
 
+		trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));
+
 		/*
 		 * HW Break/Watch points
 		 *
@@ -137,16 +149,29 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
 			vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
 			trap_debug = true;
+
+			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
+						&vcpu->arch.debug_ptr->dbg_bcr[0],
+						&vcpu->arch.debug_ptr->dbg_bvr[0]);
+
+			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
+						&vcpu->arch.debug_ptr->dbg_wcr[0],
+						&vcpu->arch.debug_ptr->dbg_wvr[0]);
 		}
 	}
 
 	/* Trap debug register access */
 	if (trap_debug)
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
+
+	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
+	trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_sys_reg(vcpu, MDSCR_EL1));
 }
 
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
+	trace_kvm_arm_clear_debug(vcpu->guest_debug);
+
 	if (vcpu->guest_debug) {
 		restore_guest_debug_regs(vcpu);
 
@@ -154,8 +179,16 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 		 * If we were using HW debug we need to restore the
 		 * debug_ptr to the guest debug state.
 		 */
-		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
+		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
 			vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
 
+			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
+						&vcpu->arch.debug_ptr->dbg_bcr[0],
+						&vcpu->arch.debug_ptr->dbg_bvr[0]);
+
+			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
+						&vcpu->arch.debug_ptr->dbg_wcr[0],
+						&vcpu->arch.debug_ptr->dbg_wvr[0]);
+		}
 	}
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 405403e..15bc851 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -38,6 +38,8 @@
 
 #include "sys_regs.h"
 
+#include "trace.h"
+
 /*
  * All of this file is extremly similar to the ARM coproc.c, but the
  * types are different. My gut feeling is that it should be pretty
@@ -208,6 +210,8 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
 		*vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, r->reg);
 	}
 
+	trace_trap_reg(__func__, r->reg, p->is_write, *vcpu_reg(vcpu, p->Rt));
+
 	return true;
 }
 
@@ -223,6 +227,8 @@ static inline bool trap_debug32(struct kvm_vcpu *vcpu,
 		*vcpu_reg(vcpu, p->Rt) = *r;
 	}
 
+	trace_trap_reg(__func__, rd->reg, p->is_write, *r);
+
 	return true;
 }
 
@@ -238,6 +244,8 @@ static inline bool trap_debug64(struct kvm_vcpu *vcpu,
 		*vcpu_reg(vcpu, p->Rt) = *r;
 	}
 
+	trace_trap_reg(__func__, rd->reg, p->is_write, *r);
+
 	return true;
 }
 
@@ -1031,6 +1039,8 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	struct sys_reg_params params;
 	unsigned long esr = kvm_vcpu_get_hsr(vcpu);
 
+	trace_kvm_handle_sys_reg(esr);
+
 	params.is_aarch32 = false;
 	params.is_32bit = false;
 	params.Op0 = (esr >> 20) & 3;
diff --git a/arch/arm64/kvm/trace.h b/arch/arm64/kvm/trace.h
index 157416e9..5122a55 100644
--- a/arch/arm64/kvm/trace.h
+++ b/arch/arm64/kvm/trace.h
@@ -44,6 +44,111 @@ TRACE_EVENT(kvm_hvc_arm64,
 		  __entry->vcpu_pc, __entry->r0, __entry->imm)
 );
 
+TRACE_EVENT(kvm_arm_setup_debug,
+	TP_PROTO(struct kvm_vcpu *vcpu, __u32 guest_debug),
+	TP_ARGS(vcpu, guest_debug),
+
+	TP_STRUCT__entry(
+		__field(struct kvm_vcpu *, vcpu)
+		__field(__u32, guest_debug)
+	),
+
+	TP_fast_assign(
+		__entry->vcpu = vcpu;
+		__entry->guest_debug = guest_debug;
+	),
+
+	TP_printk("vcpu: %p, flags: 0x%08x", __entry->vcpu, __entry->guest_debug)
+);
+
+TRACE_EVENT(kvm_arm_clear_debug,
+	TP_PROTO(__u32 guest_debug),
+	TP_ARGS(guest_debug),
+
+	TP_STRUCT__entry(
+		__field(__u32, guest_debug)
+	),
+
+	TP_fast_assign(
+		__entry->guest_debug = guest_debug;
+	),
+
+	TP_printk("flags: 0x%08x", __entry->guest_debug)
+);
+
+TRACE_EVENT(kvm_arm_set_dreg32,
+	TP_PROTO(const char *name, __u32 value),
+	TP_ARGS(name, value),
+
+	TP_STRUCT__entry(
+		__field(const char *, name)
+		__field(__u32, value)
+	),
+
+	TP_fast_assign(
+		__entry->name = name;
+		__entry->value = value;
+	),
+
+	TP_printk("%s: 0x%08x", __entry->name, __entry->value)
+);
+
+TRACE_EVENT(kvm_arm_set_regset,
+	TP_PROTO(const char *type, int len, __u32 *control, __u64 *value),
+	TP_ARGS(type, len, control, value),
+	TP_STRUCT__entry(
+		__field(const char *, name)
+		__field(int, len)
+		__array(u32, ctrls, 16)
+		__array(u64, values, 16)
+	),
+	TP_fast_assign(
+		__entry->name = type;
+		__entry->len = len;
+		memcpy(__entry->ctrls, control, len << 2);
+		memcpy(__entry->values, value, len << 3);
+	),
+	TP_printk("%d %s CTRL:%s VALUE:%s", __entry->len, __entry->name,
+		__print_array(__entry->ctrls, __entry->len, sizeof(__u32)),
+		__print_array(__entry->values, __entry->len, sizeof(__u64)))
+);
+
+TRACE_EVENT(trap_reg,
+	TP_PROTO(const char *fn, int reg, bool is_write, u64 write_value),
+	TP_ARGS(fn, reg, is_write, write_value),
+
+	TP_STRUCT__entry(
+		__field(const char *, fn)
+		__field(int, reg)
+		__field(bool, is_write)
+		__field(u64, write_value)
+	),
+
+	TP_fast_assign(
+		__entry->fn = fn;
+		__entry->reg = reg;
+		__entry->is_write = is_write;
+		__entry->write_value = write_value;
+	),
+
+	TP_printk("%s %s reg %d (0x%08llx)", __entry->fn,  __entry->is_write?"write to":"read from", __entry->reg, __entry->write_value)
+);
+
+TRACE_EVENT(kvm_handle_sys_reg,
+	TP_PROTO(unsigned long hsr),
+	TP_ARGS(hsr),
+
+	TP_STRUCT__entry(
+		__field(unsigned long,	hsr)
+	),
+
+	TP_fast_assign(
+		__entry->hsr = hsr;
+	),
+
+	TP_printk("HSR 0x%08lx", __entry->hsr)
+);
+
 #endif /* _TRACE_ARM64_KVM_H */
 
 #undef TRACE_INCLUDE_PATH
-- 
2.4.1

Re: [PATCH v5 10/12] KVM: arm64: guest debug, HW assisted debug support

[Will Deacon]

Hi Alex,

On Fri, May 29, 2015 at 10:30:26AM +0100, Alex Bennée wrote:
> This adds support for userspace to control the HW debug registers for
> guest debug. In the debug ioctl we copy the IMPDEF defined number of
> registers into a new register set called host_debug_state. There is now
> a new vcpu parameter called debug_ptr which selects which register set
> is to copied into the real registers when world switch occurs.
> 
> I've moved some helper functions into the hw_breakpoint.h header for
> re-use.
> 
> As with single step we need to tweak the guest registers to enable the
> exceptions so we need to save and restore those bits.
> 
> Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
> userspace to query the number of hardware break and watch points
> available on the host hardware.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

[...]

> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 33c8143..ada57df 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2668,7 +2668,7 @@ The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
>  
>    - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
> -  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
> +  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390, arm64]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
>    - KVM_GUESTDBG_EXIT_PENDING:  trigger an immediate guest exit [s390]
> @@ -2683,6 +2683,11 @@ updated to the correct (supplied) values.
>  The second part of the structure is architecture specific and
>  typically contains a set of debug registers.
>  
> +For arm64 the number of debug registers is implementation defined and
> +can be determined by querying the KVM_CAP_GUEST_DEBUG_HW_BPS and
> +KVM_CAP_GUEST_DEBUG_HW_WPS capabilities which return a positive number
> +indicating the number of supported registers.
> +
>  When debug events exit the main run loop with the reason
>  KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
>  structure containing architecture specific debug information.
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 0d17c7b..6df47c1 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -307,6 +307,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  
>  #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |    \
>  			    KVM_GUESTDBG_USE_SW_BP | \
> +			    KVM_GUESTDBG_USE_HW_BP | \
>  			    KVM_GUESTDBG_SINGLESTEP)
>  
>  /**
> @@ -327,6 +328,12 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  
>  	if (dbg->control & KVM_GUESTDBG_ENABLE) {
>  		vcpu->guest_debug = dbg->control;
> +
> +		/* Hardware assisted Break and Watch points */
> +		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> +			vcpu->arch.external_debug_state = dbg->arch;
> +		}
> +
>  	} else {
>  		/* If not enabled clear all flags */
>  		vcpu->guest_debug = 0;
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index 52b484b..c450552 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>  }
>  #endif
>  
> +/* Determine number of BRP registers available. */
> +static inline int get_num_brps(void)
> +{
> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> +}
> +
> +/* Determine number of WRP registers available. */
> +static inline int get_num_wrps(void)
> +{
> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> +}

I'm fine with moving these into the header file, but we should probably
revisit this at some point so that we use a shadow value to indicate the
minimum number of registers across the system for e.g. big.LITTLE platforms
that don't have uniform debug resources.

>  extern struct pmu perf_ops_bp;
>  
>  #endif	/* __KERNEL__ */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e5040b6..498d4f7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -113,12 +113,13 @@ struct kvm_vcpu_arch {
>  
>  	/*
>  	 * For debugging the guest we need to keep a set of debug
> -	 * registers which can override the guests own debug state
> +	 * registers which can override the guest's own debug state
>  	 * while being used. These are set via the KVM_SET_GUEST_DEBUG
>  	 * ioctl.
>  	 */
>  	struct kvm_guest_debug_arch *debug_ptr;
>  	struct kvm_guest_debug_arch vcpu_debug_state;
> +	struct kvm_guest_debug_arch external_debug_state;
>  
>  	/* Pointer to host CPU context */
>  	kvm_cpu_context_t *host_cpu_context;
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 43758e7..95168c2 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -116,7 +116,7 @@ struct kvm_guest_debug_arch {
>  
>  struct kvm_debug_exit_arch {
>  	__u32 hsr;
> -	__u64 far;
> +	__u64 far;	/* used for watchpoints */

Why not just add the comment with the field in your earlier patch?

Will

Re: [PATCH v5 10/12] KVM: arm64: guest debug, HW assisted debug support

[Alex Bennée]

Will Deacon <will.deacon@arm.com> writes:

> Hi Alex,
>
> On Fri, May 29, 2015 at 10:30:26AM +0100, Alex Bennée wrote:
>> This adds support for userspace to control the HW debug registers for
>> guest debug. In the debug ioctl we copy the IMPDEF defined number of
>> registers into a new register set called host_debug_state. There is now
>> a new vcpu parameter called debug_ptr which selects which register set
>> is to copied into the real registers when world switch occurs.
>> 
>> I've moved some helper functions into the hw_breakpoint.h header for
>> re-use.
>> 
>> As with single step we need to tweak the guest registers to enable the
>> exceptions so we need to save and restore those bits.
>> 
>> Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
>> userspace to query the number of hardware break and watch points
>> available on the host hardware.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> [...]
>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 33c8143..ada57df 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -2668,7 +2668,7 @@ The top 16 bits of the control field are architecture specific control
>>  flags which can include the following:
>>  
>>    - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
>> -  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
>> +  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390, arm64]
>>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
>>    - KVM_GUESTDBG_EXIT_PENDING:  trigger an immediate guest exit [s390]
>> @@ -2683,6 +2683,11 @@ updated to the correct (supplied) values.
>>  The second part of the structure is architecture specific and
>>  typically contains a set of debug registers.
>>  
>> +For arm64 the number of debug registers is implementation defined and
>> +can be determined by querying the KVM_CAP_GUEST_DEBUG_HW_BPS and
>> +KVM_CAP_GUEST_DEBUG_HW_WPS capabilities which return a positive number
>> +indicating the number of supported registers.
>> +
>>  When debug events exit the main run loop with the reason
>>  KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
>>  structure containing architecture specific debug information.
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 0d17c7b..6df47c1 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -307,6 +307,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>  
>>  #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |    \
>>  			    KVM_GUESTDBG_USE_SW_BP | \
>> +			    KVM_GUESTDBG_USE_HW_BP | \
>>  			    KVM_GUESTDBG_SINGLESTEP)
>>  
>>  /**
>> @@ -327,6 +328,12 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  
>>  	if (dbg->control & KVM_GUESTDBG_ENABLE) {
>>  		vcpu->guest_debug = dbg->control;
>> +
>> +		/* Hardware assisted Break and Watch points */
>> +		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
>> +			vcpu->arch.external_debug_state = dbg->arch;
>> +		}
>> +
>>  	} else {
>>  		/* If not enabled clear all flags */
>>  		vcpu->guest_debug = 0;
>> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
>> index 52b484b..c450552 100644
>> --- a/arch/arm64/include/asm/hw_breakpoint.h
>> +++ b/arch/arm64/include/asm/hw_breakpoint.h
>> @@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>>  }
>>  #endif
>>  
>> +/* Determine number of BRP registers available. */
>> +static inline int get_num_brps(void)
>> +{
>> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
>> +}
>> +
>> +/* Determine number of WRP registers available. */
>> +static inline int get_num_wrps(void)
>> +{
>> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
>> +}
>
> I'm fine with moving these into the header file, but we should probably
> revisit this at some point so that we use a shadow value to indicate the
> minimum number of registers across the system for e.g. big.LITTLE platforms
> that don't have uniform debug resources.

There is work going forward in QEMU to specify this limitation when
creating vcpus. At the moment the kernel sanity checks these are all the
same on boot up but I guess we will have to decide where the smarts will
end up. Do we:

  - report the real number per real-cpu (QEMU figures out vcpu  config)

  or

  - report the lowest common denominator
  

>
>>  extern struct pmu perf_ops_bp;
>>  
>>  #endif	/* __KERNEL__ */
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index e5040b6..498d4f7 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -113,12 +113,13 @@ struct kvm_vcpu_arch {
>>  
>>  	/*
>>  	 * For debugging the guest we need to keep a set of debug
>> -	 * registers which can override the guests own debug state
>> +	 * registers which can override the guest's own debug state
>>  	 * while being used. These are set via the KVM_SET_GUEST_DEBUG
>>  	 * ioctl.
>>  	 */
>>  	struct kvm_guest_debug_arch *debug_ptr;
>>  	struct kvm_guest_debug_arch vcpu_debug_state;
>> +	struct kvm_guest_debug_arch external_debug_state;
>>  
>>  	/* Pointer to host CPU context */
>>  	kvm_cpu_context_t *host_cpu_context;
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index 43758e7..95168c2 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -116,7 +116,7 @@ struct kvm_guest_debug_arch {
>>  
>>  struct kvm_debug_exit_arch {
>>  	__u32 hsr;
>> -	__u64 far;
>> +	__u64 far;	/* used for watchpoints */
>
> Why not just add the comment with the field in your earlier patch?

I can fix that up if there is need to a re-spin.

-- 
Alex Bennée

Re: [PATCH v5 08/12] KVM: arm64: re-factor hyp.S debug register code

[Christoffer Dall]

On Fri, May 29, 2015 at 10:30:24AM +0100, Alex Bennée wrote:
> This is a pre-cursor to sharing the code with the guest debug support.
> This replaces the big macro that fishes data out of a fixed location
> with a more general helper macro to restore a set of debug registers. It
> uses macro substitution so it can be re-used for debug control and value
> registers. It does however rely on the debug registers being 64 bit
> aligned (as they happen to be in the hyp ABI).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v3:
>   - return to the patch series
>   - add save and restore targets
>   - change register use and document
> v4:
>   - keep original setup/restore names
>   - don't use split u32/u64 structure yet
> ---
>  arch/arm64/kvm/hyp.S | 519 ++++++++++++++-------------------------------------
>  1 file changed, 140 insertions(+), 379 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 74e63d8..9c4897d 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S


[...]

> @@ -465,195 +318,52 @@
>  	msr	mdscr_el1,	x25
>  .endm
>  
> -.macro restore_debug
> -	// x2: base address for cpu context
> -	// x3: tmp register
> -
> -	mrs	x26, id_aa64dfr0_el1
> -	ubfx	x24, x26, #12, #4	// Extract BRPs
> -	ubfx	x25, x26, #20, #4	// Extract WRPs
> -	mov	w26, #15
> -	sub	w24, w26, w24		// How many BPs to skip
> -	sub	w25, w26, w25		// How many WPs to skip
> -
> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> +.macro restore_debug type
> +        // x4: pointer to register set
> +        // x5: number of registers to skip

nit: use tabs instead of spaces here...

> +	// x6..x22 trashed
>  

[...]

> @@ -887,12 +597,63 @@ __restore_sysregs:
>  	restore_sysregs
>  	ret
>  
> +/* Save debug state */
>  __save_debug:
> -	save_debug
> +	// x0: base address for vcpu context
> +	// x2: ptr to current CPU context
> +	// x4/x5: trashed

I had a bunch of questions here which I think you missed last time
around:
 1. why do we need the vcpu context?
 2. what does 'current' mean here?
 3. you're also trashing everything that save_debug trashes, so x4/5,
    x6-x22 trashed would be more correct

> +
> +	mrs	x26, id_aa64dfr0_el1
> +	ubfx	x24, x26, #12, #4	// Extract BRPs
> +	ubfx	x25, x26, #20, #4	// Extract WRPs
> +	mov	w26, #15
> +	sub	w24, w26, w24		// How many BPs to skip
> +	sub	w25, w26, w25		// How many WPs to skip
> +
> +	mov     x5, x24
> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> +	save_debug dbgbcr
> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> +	save_debug dbgbvr
> +
> +	mov     x5, x25
> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> +	save_debug dbgwcr
> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> +	save_debug dbgwvr
> +
> +	mrs	x21, mdccint_el1
> +	str	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
>  	ret
>  
> +/* Restore debug state */
>  __restore_debug:
> -	restore_debug
> +	// x0: base address for cpu context
> +	// x2: ptr to current CPU context
> +	// x4/x5: trashed

and you missed these comments too, basically same stuff, but why was it
'cpu' here and not 'vcpu' like above?

note again, that you're explicitly touching x24, xx25, and x26 here as
well, so shouldn't they be listed as trashed?

> +
> +	mrs	x26, id_aa64dfr0_el1
> +	ubfx	x24, x26, #12, #4	// Extract BRPs
> +	ubfx	x25, x26, #20, #4	// Extract WRPs
> +	mov	w26, #15
> +	sub	w24, w26, w24		// How many BPs to skip
> +	sub	w25, w26, w25		// How many WPs to skip
> +
> +	mov     x5, x24
> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> +	restore_debug dbgbcr
> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> +	restore_debug dbgbvr
> +
> +	mov     x5, x25
> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> +	restore_debug dbgwcr
> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> +	restore_debug dbgwvr
> +
> +	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
> +	msr	mdccint_el1, x21
> +
>  	ret
>  
>  __save_fpsimd:

Thanks,
-Christoffer

Re: [PATCH v5 08/12] KVM: arm64: re-factor hyp.S debug register code

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Fri, May 29, 2015 at 10:30:24AM +0100, Alex Bennée wrote:
>> This is a pre-cursor to sharing the code with the guest debug support.
>> This replaces the big macro that fishes data out of a fixed location
>> with a more general helper macro to restore a set of debug registers. It
>> uses macro substitution so it can be re-used for debug control and value
>> registers. It does however rely on the debug registers being 64 bit
>> aligned (as they happen to be in the hyp ABI).

Oops I'd better fix that commit comment.

>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> ---
>> v3:
>>   - return to the patch series
>>   - add save and restore targets
>>   - change register use and document
>> v4:
>>   - keep original setup/restore names
>>   - don't use split u32/u64 structure yet
>> ---
>>  arch/arm64/kvm/hyp.S | 519 ++++++++++++++-------------------------------------
>>  1 file changed, 140 insertions(+), 379 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index 74e63d8..9c4897d 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>
>
> [...]
>
>> @@ -465,195 +318,52 @@
>>  	msr	mdscr_el1,	x25
>>  .endm
>>  
>> -.macro restore_debug
>> -	// x2: base address for cpu context
>> -	// x3: tmp register
>> -
>> -	mrs	x26, id_aa64dfr0_el1
>> -	ubfx	x24, x26, #12, #4	// Extract BRPs
>> -	ubfx	x25, x26, #20, #4	// Extract WRPs
>> -	mov	w26, #15
>> -	sub	w24, w26, w24		// How many BPs to skip
>> -	sub	w25, w26, w25		// How many WPs to skip
>> -
>> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
>> +.macro restore_debug type
>> +        // x4: pointer to register set
>> +        // x5: number of registers to skip
>
> nit: use tabs instead of spaces here...
>
>> +	// x6..x22 trashed
>>  
>
> [...]
>
>> @@ -887,12 +597,63 @@ __restore_sysregs:
>>  	restore_sysregs
>>  	ret
>>  
>> +/* Save debug state */
>>  __save_debug:
>> -	save_debug
>> +	// x0: base address for vcpu context
>> +	// x2: ptr to current CPU context
>> +	// x4/x5: trashed
>
> I had a bunch of questions here which I think you missed last time
> around:
>  1. why do we need the vcpu context?

We don't, I'll drop that

>  2. what does 'current' mean here?

Either the host or vcpu context depending which way we are currently going.

>  3. you're also trashing everything that save_debug trashes, so x4/5,
>     x6-x22 trashed would be more correct

OK

>
>> +
>> +	mrs	x26, id_aa64dfr0_el1
>> +	ubfx	x24, x26, #12, #4	// Extract BRPs
>> +	ubfx	x25, x26, #20, #4	// Extract WRPs
>> +	mov	w26, #15
>> +	sub	w24, w26, w24		// How many BPs to skip
>> +	sub	w25, w26, w25		// How many WPs to skip
>> +
>> +	mov     x5, x24
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
>> +	save_debug dbgbcr
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
>> +	save_debug dbgbvr
>> +
>> +	mov     x5, x25
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
>> +	save_debug dbgwcr
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
>> +	save_debug dbgwvr
>> +
>> +	mrs	x21, mdccint_el1
>> +	str	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
>>  	ret
>>  
>> +/* Restore debug state */
>>  __restore_debug:
>> -	restore_debug
>> +	// x0: base address for cpu context
>> +	// x2: ptr to current CPU context
>> +	// x4/x5: trashed
>
> and you missed these comments too, basically same stuff, but why was it
> 'cpu' here and not 'vcpu' like above?

Again we use the functions both for restoring host and vcpu debug context.

>
> note again, that you're explicitly touching x24, xx25, and x26 here as
> well, so shouldn't they be listed as trashed?
>
>> +
>> +	mrs	x26, id_aa64dfr0_el1
>> +	ubfx	x24, x26, #12, #4	// Extract BRPs
>> +	ubfx	x25, x26, #20, #4	// Extract WRPs
>> +	mov	w26, #15
>> +	sub	w24, w26, w24		// How many BPs to skip
>> +	sub	w25, w26, w25		// How many WPs to skip
>> +
>> +	mov     x5, x24
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
>> +	restore_debug dbgbcr
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
>> +	restore_debug dbgbvr
>> +
>> +	mov     x5, x25
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
>> +	restore_debug dbgwcr
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
>> +	restore_debug dbgwvr
>> +
>> +	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
>> +	msr	mdccint_el1, x21
>> +
>>  	ret
>>  
>>  __save_fpsimd:
>
> Thanks,
> -Christoffer

-- 
Alex Bennée

Re: [PATCH v5 09/12] KVM: arm64: introduce vcpu->arch.debug_ptr

[Christoffer Dall]

On Fri, May 29, 2015 at 10:30:25AM +0100, Alex Bennée wrote:
> This introduces a level of indirection for the debug registers. Instead
> of using the sys_regs[] directly we store registers in a structure in
> the vcpu. As we are no longer tied to the layout of the sys_regs[] we
> can make the copies size appropriate for control and value registers.
> 
> This also entails updating the sys_regs code to access this new
> structure. Instead of passing a register index we now pass an offset
> into the kvm_guest_debug_arch structure.
> 
> We also need to ensure the GET/SET_ONE_REG ioctl operations store the
> registers in their correct location.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  arch/arm/kvm/arm.c                |   3 +
>  arch/arm64/include/asm/kvm_asm.h  |  24 +++-----
>  arch/arm64/include/asm/kvm_host.h |  12 +++-
>  arch/arm64/kernel/asm-offsets.c   |   6 ++
>  arch/arm64/kvm/hyp.S              | 107 +++++++++++++++++---------------
>  arch/arm64/kvm/sys_regs.c         | 126 +++++++++++++++++++++++++++++++-------
>  6 files changed, 188 insertions(+), 90 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9b3ed6d..0d17c7b 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -279,6 +279,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  	/* Set up the timer */
>  	kvm_timer_vcpu_init(vcpu);
>  
> +	/* Set the debug registers to be the guests */
> +	vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
> +
>  	return 0;
>  }
>  
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index d6b507e..e997404 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -46,24 +46,16 @@
>  #define	CNTKCTL_EL1	20	/* Timer Control Register (EL1) */
>  #define	PAR_EL1		21	/* Physical Address Register */
>  #define MDSCR_EL1	22	/* Monitor Debug System Control Register */
> -#define DBGBCR0_EL1	23	/* Debug Breakpoint Control Registers (0-15) */
> -#define DBGBCR15_EL1	38
> -#define DBGBVR0_EL1	39	/* Debug Breakpoint Value Registers (0-15) */
> -#define DBGBVR15_EL1	54
> -#define DBGWCR0_EL1	55	/* Debug Watchpoint Control Registers (0-15) */
> -#define DBGWCR15_EL1	70
> -#define DBGWVR0_EL1	71	/* Debug Watchpoint Value Registers (0-15) */
> -#define DBGWVR15_EL1	86
> -#define MDCCINT_EL1	87	/* Monitor Debug Comms Channel Interrupt Enable Reg */
> +#define MDCCINT_EL1	23	/* Monitor Debug Comms Channel Interrupt Enable Reg */
>  
>  /* 32bit specific registers. Keep them at the end of the range */
> -#define	DACR32_EL2	88	/* Domain Access Control Register */
> -#define	IFSR32_EL2	89	/* Instruction Fault Status Register */
> -#define	FPEXC32_EL2	90	/* Floating-Point Exception Control Register */
> -#define	DBGVCR32_EL2	91	/* Debug Vector Catch Register */
> -#define	TEECR32_EL1	92	/* ThumbEE Configuration Register */
> -#define	TEEHBR32_EL1	93	/* ThumbEE Handler Base Register */
> -#define	NR_SYS_REGS	94
> +#define	DACR32_EL2	24	/* Domain Access Control Register */
> +#define	IFSR32_EL2	25	/* Instruction Fault Status Register */
> +#define	FPEXC32_EL2	26	/* Floating-Point Exception Control Register */
> +#define	DBGVCR32_EL2	27	/* Debug Vector Catch Register */
> +#define	TEECR32_EL1	28	/* ThumbEE Configuration Register */
> +#define	TEEHBR32_EL1	29	/* ThumbEE Handler Base Register */
> +#define	NR_SYS_REGS	30
>  
>  /* 32bit mapping */
>  #define c0_MPIDR	(MPIDR_EL1 * 2)	/* MultiProcessor ID Register */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e2db6a6..e5040b6 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -108,11 +108,21 @@ struct kvm_vcpu_arch {
>  	/* Exception Information */
>  	struct kvm_vcpu_fault_info fault;
>  
> -	/* Debug state */
> +	/* Guest debug state */
>  	u64 debug_flags;
>  
> +	/*
> +	 * For debugging the guest we need to keep a set of debug
> +	 * registers which can override the guests own debug state

the guest's

> +	 * while being used. These are set via the KVM_SET_GUEST_DEBUG
> +	 * ioctl.

Which ones are set via the KVM_SET_GUEST_DEBUG ioctl?

This comment doesn't feel like it's properly explaining the fields
below, because it feels like there's missing a set of registers for this
to add up.  I would suggest rewording this comment to something like:

  We maintain more than a single set of debug registers to support
  debugging the guest from the host and to maintain separate host and
  guest state during world switches.  vcpu_debug_state are the debug
  registers of the vcpu as the guest sees them.  host_debug_state are
  the host registers which are saved and restored during world switches.

  debug_ptr points to the set of debug registers that should be loaded
  onto the hardware when running the guest.

That is, assuming I got this right.

> +	 */
> +	struct kvm_guest_debug_arch *debug_ptr;
> +	struct kvm_guest_debug_arch vcpu_debug_state;
> +
>  	/* Pointer to host CPU context */
>  	kvm_cpu_context_t *host_cpu_context;
> +	struct kvm_guest_debug_arch host_debug_state;
>  
>  	/* VGIC state */
>  	struct vgic_cpu vgic_cpu;
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index dfb25a2..1a8e97c 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -116,10 +116,16 @@ int main(void)
>    DEFINE(VCPU_FAR_EL2,		offsetof(struct kvm_vcpu, arch.fault.far_el2));
>    DEFINE(VCPU_HPFAR_EL2,	offsetof(struct kvm_vcpu, arch.fault.hpfar_el2));
>    DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
> +  DEFINE(VCPU_DEBUG_PTR,	offsetof(struct kvm_vcpu, arch.debug_ptr));
> +  DEFINE(DEBUG_BCR, 		offsetof(struct kvm_guest_debug_arch, dbg_bcr));
> +  DEFINE(DEBUG_BVR, 		offsetof(struct kvm_guest_debug_arch, dbg_bvr));
> +  DEFINE(DEBUG_WCR, 		offsetof(struct kvm_guest_debug_arch, dbg_wcr));
> +  DEFINE(DEBUG_WVR, 		offsetof(struct kvm_guest_debug_arch, dbg_wvr));
>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> +  DEFINE(VCPU_HOST_DEBUG_STATE, offsetof(struct kvm_vcpu, arch.host_debug_state));
>    DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
>    DEFINE(VCPU_TIMER_CNTV_CVAL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval));
>    DEFINE(KVM_TIMER_CNTVOFF,	offsetof(struct kvm, arch.timer.cntvoff));
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 9c4897d..1940a4c 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -228,7 +228,7 @@
>  	stp	x24, x25, [x3, #160]
>  .endm
>  
> -.macro save_debug type
> +.macro save_debug type regsz offsz
>  	// x4: pointer to register set
>  	// x5: number of registers to skip
>  	// x6..x22 trashed
> @@ -258,22 +258,22 @@
>  	add	x22, x22, x5, lsl #2
>  	br	x22
>  1:
> -	str	x21, [x4, #(15 * 8)]
> -	str	x20, [x4, #(14 * 8)]
> -	str	x19, [x4, #(13 * 8)]
> -	str	x18, [x4, #(12 * 8)]
> -	str	x17, [x4, #(11 * 8)]
> -	str	x16, [x4, #(10 * 8)]
> -	str	x15, [x4, #(9 * 8)]
> -	str	x14, [x4, #(8 * 8)]
> -	str	x13, [x4, #(7 * 8)]
> -	str	x12, [x4, #(6 * 8)]
> -	str	x11, [x4, #(5 * 8)]
> -	str	x10, [x4, #(4 * 8)]
> -	str	x9, [x4, #(3 * 8)]
> -	str	x8, [x4, #(2 * 8)]
> -	str	x7, [x4, #(1 * 8)]
> -	str	x6, [x4, #(0 * 8)]
> +	str	\regsz\()21, [x4, #(15 * \offsz)]
> +	str	\regsz\()20, [x4, #(14 * \offsz)]
> +	str	\regsz\()19, [x4, #(13 * \offsz)]
> +	str	\regsz\()18, [x4, #(12 * \offsz)]
> +	str	\regsz\()17, [x4, #(11 * \offsz)]
> +	str	\regsz\()16, [x4, #(10 * \offsz)]
> +	str	\regsz\()15, [x4, #(9 * \offsz)]
> +	str	\regsz\()14, [x4, #(8 * \offsz)]
> +	str	\regsz\()13, [x4, #(7 * \offsz)]
> +	str	\regsz\()12, [x4, #(6 * \offsz)]
> +	str	\regsz\()11, [x4, #(5 * \offsz)]
> +	str	\regsz\()10, [x4, #(4 * \offsz)]
> +	str	\regsz\()9, [x4, #(3 * \offsz)]
> +	str	\regsz\()8, [x4, #(2 * \offsz)]
> +	str	\regsz\()7, [x4, #(1 * \offsz)]
> +	str	\regsz\()6, [x4, #(0 * \offsz)]
>  .endm
>  
>  .macro restore_sysregs
> @@ -318,7 +318,7 @@
>  	msr	mdscr_el1,	x25
>  .endm
>  
> -.macro restore_debug type
> +.macro restore_debug type regsz offsz
>          // x4: pointer to register set
>          // x5: number of registers to skip
>  	// x6..x22 trashed
> @@ -327,22 +327,22 @@
>  	add	x22, x22, x5, lsl #2
>  	br	x22
>  1:
> -	ldr	x21, [x4, #(15 * 8)]
> -	ldr	x20, [x4, #(14 * 8)]
> -	ldr	x19, [x4, #(13 * 8)]
> -	ldr	x18, [x4, #(12 * 8)]
> -	ldr	x17, [x4, #(11 * 8)]
> -	ldr	x16, [x4, #(10 * 8)]
> -	ldr	x15, [x4, #(9 * 8)]
> -	ldr	x14, [x4, #(8 * 8)]
> -	ldr	x13, [x4, #(7 * 8)]
> -	ldr	x12, [x4, #(6 * 8)]
> -	ldr	x11, [x4, #(5 * 8)]
> -	ldr	x10, [x4, #(4 * 8)]
> -	ldr	x9, [x4, #(3 * 8)]
> -	ldr	x8, [x4, #(2 * 8)]
> -	ldr	x7, [x4, #(1 * 8)]
> -	ldr	x6, [x4, #(0 * 8)]
> +	ldr	\regsz\()21, [x4, #(15 * \offsz)]
> +	ldr	\regsz\()20, [x4, #(14 * \offsz)]
> +	ldr	\regsz\()19, [x4, #(13 * \offsz)]
> +	ldr	\regsz\()18, [x4, #(12 * \offsz)]
> +	ldr	\regsz\()17, [x4, #(11 * \offsz)]
> +	ldr	\regsz\()16, [x4, #(10 * \offsz)]
> +	ldr	\regsz\()15, [x4, #(9 * \offsz)]
> +	ldr	\regsz\()14, [x4, #(8 * \offsz)]
> +	ldr	\regsz\()13, [x4, #(7 * \offsz)]
> +	ldr	\regsz\()12, [x4, #(6 * \offsz)]
> +	ldr	\regsz\()11, [x4, #(5 * \offsz)]
> +	ldr	\regsz\()10, [x4, #(4 * \offsz)]
> +	ldr	\regsz\()9, [x4, #(3 * \offsz)]
> +	ldr	\regsz\()8, [x4, #(2 * \offsz)]
> +	ldr	\regsz\()7, [x4, #(1 * \offsz)]
> +	ldr	\regsz\()6, [x4, #(0 * \offsz)]
>  
>  	adr	x22, 1f
>  	add	x22, x22, x5, lsl #2
> @@ -601,6 +601,7 @@ __restore_sysregs:
>  __save_debug:
>  	// x0: base address for vcpu context
>  	// x2: ptr to current CPU context
> +	// x3: ptr to debug reg struct
>  	// x4/x5: trashed
>  
>  	mrs	x26, id_aa64dfr0_el1
> @@ -611,16 +612,16 @@ __save_debug:
>  	sub	w25, w26, w25		// How many WPs to skip
>  
>  	mov     x5, x24
> -	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> -	save_debug dbgbcr
> -	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> -	save_debug dbgbvr
> +	add	x4, x3, #DEBUG_BCR
> +	save_debug dbgbcr w 4
> +	add	x4, x3, #DEBUG_BVR
> +	save_debug dbgbvr x 8
>  
>  	mov     x5, x25
> -	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> -	save_debug dbgwcr
> -	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> -	save_debug dbgwvr
> +	add	x4, x3, #DEBUG_WCR
> +	save_debug dbgwcr w 4
> +	add	x4, x3, #DEBUG_WVR
> +	save_debug dbgwvr x 8
>  
>  	mrs	x21, mdccint_el1
>  	str	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
> @@ -640,16 +641,16 @@ __restore_debug:
>  	sub	w25, w26, w25		// How many WPs to skip
>  
>  	mov     x5, x24
> -	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> -	restore_debug dbgbcr
> -	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> -	restore_debug dbgbvr
> +	add	x4, x3, #DEBUG_BCR
> +	restore_debug dbgbcr w 4
> +	add	x4, x3, #DEBUG_BVR
> +	restore_debug dbgbvr x 8
>  
>  	mov     x5, x25
> -	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> -	restore_debug dbgwcr
> -	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> -	restore_debug dbgwvr
> +	add	x4, x3, #DEBUG_WCR
> +	restore_debug dbgwcr w 4
> +	add	x4, x3, #DEBUG_WVR
> +	restore_debug dbgwvr x 8
>  
>  	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
>  	msr	mdccint_el1, x21
> @@ -688,6 +689,7 @@ ENTRY(__kvm_vcpu_run)
>  	bl __save_sysregs
>  
>  	compute_debug_state 1f
> +	add	x3, x0, #VCPU_HOST_DEBUG_STATE
>  	bl	__save_debug
>  1:
>  	activate_traps
> @@ -703,6 +705,8 @@ ENTRY(__kvm_vcpu_run)
>  	bl __restore_fpsimd
>  
>  	skip_debug_state x3, 1f
> +	ldr	x3, [x0, #VCPU_DEBUG_PTR]
> +	kern_hyp_va x3
>  	bl	__restore_debug
>  1:
>  	restore_guest_32bit_state
> @@ -723,6 +727,8 @@ __kvm_vcpu_return:
>  	bl __save_sysregs
>  
>  	skip_debug_state x3, 1f
> +	ldr	x3, [x0, #VCPU_DEBUG_PTR]
> +	kern_hyp_va x3
>  	bl	__save_debug
>  1:
>  	save_guest_32bit_state
> @@ -745,6 +751,7 @@ __kvm_vcpu_return:
>  	// already been saved. Note that we nuke the whole 64bit word.
>  	// If we ever add more flags, we'll have to be more careful...
>  	str	xzr, [x0, #VCPU_DEBUG_FLAGS]
> +	add	x3, x0, #VCPU_HOST_DEBUG_STATE
>  	bl	__restore_debug
>  1:
>  	restore_host_regs
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index c370b40..405403e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -211,6 +211,48 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static inline bool trap_debug32(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_params *p,
> +				const struct sys_reg_desc *rd)
> +{
> +	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);

Does this work on BE systems?

> +	if (p->is_write) {
> +		*r = *vcpu_reg(vcpu, p->Rt);
> +		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> +	} else {
> +		*vcpu_reg(vcpu, p->Rt) = *r;
> +	}
> +
> +	return true;
> +}
> +
> +static inline bool trap_debug64(struct kvm_vcpu *vcpu,
> +				const struct sys_reg_params *p,
> +				const struct sys_reg_desc *rd)
> +{
> +	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
> +	if (p->is_write) {
> +		*r = *vcpu_reg(vcpu, p->Rt);
> +		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> +	} else {
> +		*vcpu_reg(vcpu, p->Rt) = *r;
> +	}
> +
> +	return true;
> +}
> +
> +static inline void reset_debug32(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> +{
> +	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);

Does this work on BE systems?

> +	*r = rd->val;
> +}
> +
> +static inline void reset_debug64(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> +{
> +	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
> +	*r = rd->val;
> +}
> +
>  static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 amair;
> @@ -240,16 +282,20 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>  	/* DBGBVRn_EL1 */						\
>  	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b100),	\
> -	  trap_debug_regs, reset_val, (DBGBVR0_EL1 + (n)), 0 },		\
> +	  trap_debug64, reset_debug64,					\
> +	  offsetof(struct kvm_guest_debug_arch, dbg_bvr[(n)]), 0 },	\
>  	/* DBGBCRn_EL1 */						\
>  	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b101),	\
> -	  trap_debug_regs, reset_val, (DBGBCR0_EL1 + (n)), 0 },		\
> +	  trap_debug32, reset_debug32,					\
> +	  offsetof(struct kvm_guest_debug_arch, dbg_bcr[(n)]), 0},	\
>  	/* DBGWVRn_EL1 */						\
>  	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b110),	\
> -	  trap_debug_regs, reset_val, (DBGWVR0_EL1 + (n)), 0 },		\
> +	  trap_debug64, reset_debug64,					\
> +	  offsetof(struct kvm_guest_debug_arch, dbg_wvr[(n)]), 0 },	\
>  	/* DBGWCRn_EL1 */						\
>  	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b111),	\
> -	  trap_debug_regs, reset_val, (DBGWCR0_EL1 + (n)), 0 }
> +	  trap_debug32, reset_debug32,					\
> +	  offsetof(struct kvm_guest_debug_arch, dbg_wcr[(n)]), 0}
>  
>  /*
>   * Architected system registers.
> @@ -502,37 +548,23 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> -static bool trap_debug32(struct kvm_vcpu *vcpu,
> -			 const struct sys_reg_params *p,
> -			 const struct sys_reg_desc *r)
> -{
> -	if (p->is_write) {
> -		vcpu_cp14(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
> -		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> -	} else {
> -		*vcpu_reg(vcpu, p->Rt) = vcpu_cp14(vcpu, r->reg);
> -	}
> -
> -	return true;
> -}
> -
>  #define DBG_BCR_BVR_WCR_WVR(n)					\
>  	/* DBGBVRn */						\
>  	{ Op1( 0), CRn( 0), CRm((n)), Op2( 4), trap_debug32,	\
> -	  NULL, (cp14_DBGBVR0 + (n) * 2) },			\
> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_bvr[(n)]) },	\
>  	/* DBGBCRn */						\
>  	{ Op1( 0), CRn( 0), CRm((n)), Op2( 5), trap_debug32,	\
> -	  NULL, (cp14_DBGBCR0 + (n) * 2) },			\
> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_bcr[(n)]) },			\
>  	/* DBGWVRn */						\
>  	{ Op1( 0), CRn( 0), CRm((n)), Op2( 6), trap_debug32,	\
> -	  NULL, (cp14_DBGWVR0 + (n) * 2) },			\
> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_wvr[(n)]) },			\
>  	/* DBGWCRn */						\
>  	{ Op1( 0), CRn( 0), CRm((n)), Op2( 7), trap_debug32,	\
> -	  NULL, (cp14_DBGWCR0 + (n) * 2) }
> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_wcr[(n)]) }
>  
>  #define DBGBXVR(n)						\
>  	{ Op1( 0), CRn( 1), CRm((n)), Op2( 1), trap_debug32,	\
> -	  NULL, cp14_DBGBXVR0 + n * 2 }
> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_bvr[(n)])+sizeof(__u32) }

Does this work on BE systems?

>  
>  /*
>   * Trapped cp14 registers. We generally ignore most of the external
> @@ -1288,6 +1320,42 @@ static int demux_c15_set(u64 id, void __user *uaddr)
>  	}
>  }
>  
> +static int debug_set32(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		const struct kvm_one_reg *reg, void __user *uaddr)
> +{
> +	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);

Does this work on BE systems?

> +	if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)

don't you risk copying beyond the boundaries of the array elements here
because KVM_REG_SIZE() is always 64 bits?

> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int debug_set64(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		const struct kvm_one_reg *reg, void __user *uaddr)
> +{
> +	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
> +	if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int debug_get32(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		const struct kvm_one_reg *reg, void __user *uaddr)
> +{
> +	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);

Does this work on BE systems?

> +	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)

don't you risk reading beyond the boundaries of the array elements here
because KVM_REG_SIZE() is always 64 bits?

> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int debug_get64(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +	const struct kvm_one_reg *reg, void __user *uaddr)
> +{
> +	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
> +	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> +		return -EFAULT;
> +	return 0;
> +}
> +
>  int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	const struct sys_reg_desc *r;
> @@ -1303,6 +1371,12 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>  	if (!r)
>  		return get_invariant_sys_reg(reg->id, uaddr);
>  
> +	if (r->access == trap_debug64)
> +		return debug_get64(vcpu, r, reg, uaddr);
> +
> +	if (r->access == trap_debug32)
> +		return debug_get32(vcpu, r, reg, uaddr);
> +
>  	return reg_to_user(uaddr, &vcpu_sys_reg(vcpu, r->reg), reg->id);
>  }
>  
> @@ -1321,6 +1395,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>  	if (!r)
>  		return set_invariant_sys_reg(reg->id, uaddr);
>  
> +	if (r->access == trap_debug64)
> +		return debug_set64(vcpu, r, reg, uaddr);
> +
> +	if (r->access == trap_debug32)
> +		return debug_set32(vcpu, r, reg, uaddr);
> +
>  	return reg_from_user(&vcpu_sys_reg(vcpu, r->reg), uaddr, reg->id);
>  }
>  
> -- 
> 2.4.1
> 
Thanks,
-Christoffer

Re: [PATCH v5 10/12] KVM: arm64: guest debug, HW assisted debug support

[Christoffer Dall]

On Fri, May 29, 2015 at 10:30:26AM +0100, Alex Bennée wrote:
> This adds support for userspace to control the HW debug registers for
> guest debug. In the debug ioctl we copy the IMPDEF defined number of
> registers into a new register set called host_debug_state. There is now
> a new vcpu parameter called debug_ptr which selects which register set
> is to copied into the real registers when world switch occurs.
> 
> I've moved some helper functions into the hw_breakpoint.h header for
> re-use.
> 
> As with single step we need to tweak the guest registers to enable the
> exceptions so we need to save and restore those bits.
> 
> Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
> userspace to query the number of hardware break and watch points
> available on the host hardware.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - switched to C setup
>    - replace host debug registers directly into context
>    - minor tweak to api docs
>    - setup right register for debug
>    - add FAR_EL2 to debug exit structure
>    - add support for trapping debug register access
> v3
>    - remove stray trace statement
>    - fix spacing around operators (various)
>    - clean-up usage of trap_debug
>    - introduce debug_ptr, replace excessive memcpy stuff
>    - don't use memcpy in ioctl, just assign
>    - update cap ioctl documentation
>    - reword a number comments
>    - rename host_debug_state->external_debug_state
> v4
>    - use the new u32/u64 split debug_ptr approach
>    - fix some wording/comments
> v5
>    - don't set MDSCR_EL1.KDE (not needed)
> ---
>  Documentation/virtual/kvm/api.txt      |  7 ++++++-
>  arch/arm/kvm/arm.c                     |  7 +++++++
>  arch/arm64/include/asm/hw_breakpoint.h | 12 +++++++++++
>  arch/arm64/include/asm/kvm_host.h      |  3 ++-
>  arch/arm64/include/uapi/asm/kvm.h      |  2 +-
>  arch/arm64/kernel/hw_breakpoint.c      | 12 -----------
>  arch/arm64/kvm/debug.c                 | 37 +++++++++++++++++++++++++++++-----
>  arch/arm64/kvm/handle_exit.c           |  6 ++++++
>  arch/arm64/kvm/reset.c                 | 12 +++++++++++
>  include/uapi/linux/kvm.h               |  2 ++
>  10 files changed, 80 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 33c8143..ada57df 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2668,7 +2668,7 @@ The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
>  
>    - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
> -  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
> +  - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390, arm64]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
>    - KVM_GUESTDBG_EXIT_PENDING:  trigger an immediate guest exit [s390]
> @@ -2683,6 +2683,11 @@ updated to the correct (supplied) values.
>  The second part of the structure is architecture specific and
>  typically contains a set of debug registers.
>  
> +For arm64 the number of debug registers is implementation defined and
> +can be determined by querying the KVM_CAP_GUEST_DEBUG_HW_BPS and
> +KVM_CAP_GUEST_DEBUG_HW_WPS capabilities which return a positive number
> +indicating the number of supported registers.
> +
>  When debug events exit the main run loop with the reason
>  KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
>  structure containing architecture specific debug information.
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 0d17c7b..6df47c1 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -307,6 +307,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  
>  #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |    \
>  			    KVM_GUESTDBG_USE_SW_BP | \
> +			    KVM_GUESTDBG_USE_HW_BP | \
>  			    KVM_GUESTDBG_SINGLESTEP)
>  
>  /**
> @@ -327,6 +328,12 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  
>  	if (dbg->control & KVM_GUESTDBG_ENABLE) {
>  		vcpu->guest_debug = dbg->control;
> +
> +		/* Hardware assisted Break and Watch points */
> +		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {

does KVM_GUESTDBG_USE_HW_BP cover watch points as well or why is this
mentionend in the comment?

I asked this once before already...

> +			vcpu->arch.external_debug_state = dbg->arch;
> +		}
> +
>  	} else {
>  		/* If not enabled clear all flags */
>  		vcpu->guest_debug = 0;
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index 52b484b..c450552 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>  }
>  #endif
>  
> +/* Determine number of BRP registers available. */
> +static inline int get_num_brps(void)
> +{
> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> +}
> +
> +/* Determine number of WRP registers available. */
> +static inline int get_num_wrps(void)
> +{
> +	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> +}
> +
>  extern struct pmu perf_ops_bp;
>  
>  #endif	/* __KERNEL__ */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e5040b6..498d4f7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -113,12 +113,13 @@ struct kvm_vcpu_arch {
>  
>  	/*
>  	 * For debugging the guest we need to keep a set of debug
> -	 * registers which can override the guests own debug state
> +	 * registers which can override the guest's own debug state

rebase error?

>  	 * while being used. These are set via the KVM_SET_GUEST_DEBUG
>  	 * ioctl.
>  	 */
>  	struct kvm_guest_debug_arch *debug_ptr;
>  	struct kvm_guest_debug_arch vcpu_debug_state;
> +	struct kvm_guest_debug_arch external_debug_state;
>  
>  	/* Pointer to host CPU context */
>  	kvm_cpu_context_t *host_cpu_context;
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 43758e7..95168c2 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -116,7 +116,7 @@ struct kvm_guest_debug_arch {
>  
>  struct kvm_debug_exit_arch {
>  	__u32 hsr;
> -	__u64 far;
> +	__u64 far;	/* used for watchpoints */

and I noticed this one before as well...

>  };
>  
>  /*
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index e7d934d..3a41bbf 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -49,18 +49,6 @@ static DEFINE_PER_CPU(int, stepping_kernel_bp);
>  static int core_num_brps;
>  static int core_num_wrps;
>  
> -/* Determine number of BRP registers available. */
> -static int get_num_brps(void)
> -{
> -	return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> -}
> -
> -/* Determine number of WRP registers available. */
> -static int get_num_wrps(void)
> -{
> -	return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> -}
> -
>  int hw_breakpoint_slots(int type)
>  {
>  	/*
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 10a6baa..3c0daae 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -98,10 +98,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  				MDCR_EL2_TDRA |
>  				MDCR_EL2_TDOSA);
>  
> -	/* Trap on access to debug registers? */
> -	if (trap_debug)
> -		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
> -
>  	/* Is Guest debugging in effect? */
>  	if (vcpu->guest_debug) {
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> @@ -124,11 +120,42 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  		} else {
>  			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
>  		}
> +
> +		/*
> +		 * HW Break/Watch points
> +		 *
> +		 * We simply switch the debug_ptr to point to our new
> +		 * external_debug_state which has been populated by the
> +		 * debug ioctl. The existing KVM_ARM64_DEBUG_DIRTY
> +		 * mechanism ensures the registers are updated on the
> +		 * world switch.
> +		 */
> +		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> +			/* Enable breakpoints/watchpoints */
> +			vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_MDE;
> +
> +			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
> +			vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> +			trap_debug = true;
> +		}
>  	}
> +
> +	/* Trap debug register access */
> +	if (trap_debug)
> +		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  }
>  
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
> -	if (vcpu->guest_debug)
> +	if (vcpu->guest_debug) {
>  		restore_guest_debug_regs(vcpu);
> +
> +		/*
> +		 * If we were using HW debug we need to restore the
> +		 * debug_ptr to the guest debug state.
> +		 */
> +		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
> +			vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;

you also ignored my comment (or never answered my question) on moving
this to kvm_arm_setup_debug() last time around.... oh well.

Thanks,
-Christoffer

Re: [PATCH v5 03/12] KVM: arm64: guest debug, define API headers

[Christoffer Dall]

On Fri, May 29, 2015 at 10:30:19AM +0100, Alex Bennée wrote:
> This commit defines the API headers for guest debugging. There are two
> architecture specific debug structures:
> 
>   - kvm_guest_debug_arch, allows us to pass in HW debug registers
>   - kvm_debug_exit_arch, signals exception and possible faulting address
> 
> The type of debugging being used is controlled by the architecture
> specific control bits of the kvm_guest_debug->control flags in the ioctl
> structure.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

I can re-confirm my ack despite the changes in v4, but this really is
borderline to keep exiting r-b and a-b tags around from previous patches
I would think, but ok.

-Christoffer

> 
> ---
> v2
>    - expose hsr and pc directly to user-space
> v3
>    - s/control/controlled/ in commit message
>    - add v8 to ARM ARM comment (ARM Architecture Reference Manual)
>    - add rb tag
>    - rm pc, add far
>    - re-word comments on alignment
>    - rename KVM_ARM_NDBG_REGS -> KVM_ARM_MAX_DBG_REGS
> v4
>    - now uses common HW/SW BP define
>    - add a-b-tag
>    - use u32 for control regs
> v5
>    - revert to have arch specific KVM_GUESTDBG_USE_SW/HW_BP
>    - rm stale comments dbgctrl was stored as u64
> ---
>  arch/arm64/include/uapi/asm/kvm.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index d268320..43758e7 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -100,12 +100,32 @@ struct kvm_sregs {
>  struct kvm_fpu {
>  };
>  
> +/*
> + * See v8 ARM ARM D7.3: Debug Registers
> + *
> + * The architectural limit is 16 debug registers of each type although
> + * in practice there are usually less (see ID_AA64DFR0_EL1).
> + */
> +#define KVM_ARM_MAX_DBG_REGS 16
>  struct kvm_guest_debug_arch {
> +	__u32 dbg_bcr[KVM_ARM_MAX_DBG_REGS];
> +	__u64 dbg_bvr[KVM_ARM_MAX_DBG_REGS];
> +	__u32 dbg_wcr[KVM_ARM_MAX_DBG_REGS];
> +	__u64 dbg_wvr[KVM_ARM_MAX_DBG_REGS];
>  };
>  
>  struct kvm_debug_exit_arch {
> +	__u32 hsr;
> +	__u64 far;
>  };
>  
> +/*
> + * Architecture specific defines for kvm_guest_debug->control
> + */
> +
> +#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
> +#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)
> +
>  struct kvm_sync_regs {
>  };
>  
> -- 
> 2.4.1
> 

Re: [PATCH v5 05/12] KVM: arm: introduce kvm_arm_init/setup/clear_debug

[Christoffer Dall]

On Fri, May 29, 2015 at 10:30:21AM +0100, Alex Bennée wrote:
> This is a precursor for later patches which will need to do more to
> setup debug state before entering the hyp.S switch code. The existing
> functionality for setting mdcr_el2 has been moved out of hyp.S and now
> uses the value kept in vcpu->arch.mdcr_el2.
> 
> As the assembler used to previously mask and preserve MDCR_EL2.HPMN I've
> had to add a mechanism to save the value of mdcr_el2 as a per-cpu
> variable during the initialisation code. The kernel never sets this
> number so we are assuming the bootcode has set up the correct value
> here.
> 
> This also moves the conditional setting of the TDA bit from the hyp code
> into the C code which is currently used for the lazy debug register
> context switch code.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Re: [PATCH v5 07/12] KVM: arm64: guest debug, add support for single-step

[Christoffer Dall]

On Fri, May 29, 2015 at 10:30:23AM +0100, Alex Bennée wrote:
> This adds support for single-stepping the guest. To do this we need to
> manipulate the guests PSTATE.SS and MDSCR_EL1.SS bits which we do in the
> kvm_arm_setup/clear_debug() so we don't affect the apparent state of the
> guest. Additionally while the host is debugging the guest we suppress
> the ability of the guest to single-step itself.

I feel like there should be a slightly more elaborate explanation of
exactly what works and what doesn't work when the guest is single
stepping something and which choices we've made for supporting or not
supporting this.

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>   - Move pstate/mdscr manipulation into C
>   - don't export guest_debug to assembly
>   - add accessor for saved_debug regs
>   - tweak save/restore of mdscr_el1
> v3
>   - don't save PC in debug information struct
>   - rename debug_saved_regs->guest_debug_state
>   - save whole value, only use bits in restore
>   - add save/restore_guest-debug_regs helper functions
>   - simplify commit message for clarity
>   - rm vcpu_debug_saved_reg access fn
> v4
>   - added more comments based on suggestions
>   - guest_debug_state->guest_debug_preserved
>   - no point masking restore, we will trap out
> v5
>   - more comments
>   - don't bother preserving pstate.ss

it would have been good if there was some comment explaining the reason
for this change.

> ---
>  arch/arm/kvm/arm.c                |  4 ++-
>  arch/arm64/include/asm/kvm_host.h | 11 ++++++++
>  arch/arm64/kvm/debug.c            | 58 ++++++++++++++++++++++++++++++++++++---
>  arch/arm64/kvm/handle_exit.c      |  2 ++
>  4 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 064c105..9b3ed6d 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -302,7 +302,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	kvm_arm_set_running_vcpu(NULL);
>  }
>  
> -#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
> +#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |    \
> +			    KVM_GUESTDBG_USE_SW_BP | \
> +			    KVM_GUESTDBG_SINGLESTEP)
>  
>  /**
>   * kvm_arch_vcpu_ioctl_set_guest_debug - set up guest debugging
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7cb99b5..e2db6a6 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -123,6 +123,17 @@ struct kvm_vcpu_arch {
>  	 * here.
>  	 */
>  
> +	/*
> +	 * Guest registers we preserve during guest debugging.
> +	 *
> +	 * These shadow registers are updated by the kvm_handle_sys_reg
> +	 * trap handler if the guest accesses or updates them while we
> +	 * are using guest debug.
> +	 */
> +	struct {
> +		u32	mdscr_el1;
> +	} guest_debug_preserved;
> +
>  	/* Don't run the guest */
>  	bool pause;
>  
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 8d1bfa4..10a6baa 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -19,11 +19,41 @@
>  
>  #include <linux/kvm_host.h>
>  
> +#include <asm/debug-monitors.h>
> +#include <asm/kvm_asm.h>
>  #include <asm/kvm_arm.h>
> +#include <asm/kvm_emulate.h>
> +
> +/* These are the bits of MDSCR_EL1 we may manipulate */
> +#define MDSCR_EL1_DEBUG_MASK	(DBG_MDSCR_SS | \
> +				DBG_MDSCR_KDE | \
> +				DBG_MDSCR_MDE)
>  
>  static DEFINE_PER_CPU(u32, mdcr_el2);
>  
>  /**
> + * save/restore_guest_debug_regs
> + *
> + * For some debug operations we need to tweak some guest registers. As
> + * a result we need to save the state of those registers before we
> + * make those modifications. This does get confused if the guest
> + * attempts to control single step while being debugged. It will start
> + * working again once it is no longer being debugged by the host.

What gets confused and what starts working?

> + *
> + * Guest access to MDSCR_EL1 is trapped by the hypervisor and handled
> + * after we have restored the preserved value to the main context.
> + */
> +static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.guest_debug_preserved.mdscr_el1 = vcpu_sys_reg(vcpu, MDSCR_EL1);
> +}
> +
> +static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
> +{
> +	vcpu_sys_reg(vcpu, MDSCR_EL1) = vcpu->arch.guest_debug_preserved.mdscr_el1;
> +}
> +
> +/**
>   * kvm_arm_init_debug - grab what we need for debug
>   *
>   * Currently the sole task of this function is to retrieve the initial
> @@ -38,7 +68,6 @@ void kvm_arm_init_debug(void)
>  	__this_cpu_write(mdcr_el2, kvm_call_hyp(__kvm_get_mdcr_el2));
>  }
>  
> -
>  /**
>   * kvm_arm_setup_debug - set up debug related stuff
>   *
> @@ -73,12 +102,33 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  	if (trap_debug)
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  
> -	/* Trap breakpoints? */
> -	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
> +	/* Is Guest debugging in effect? */
> +	if (vcpu->guest_debug) {
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> +
> +		/* Save guest debug state */
> +		save_guest_debug_regs(vcpu);
> +
> +		/*
> +		 * Single Step (ARM ARM D2.12.3 The software step state
> +		 * machine)
> +		 *
> +		 * If we are doing Single Step we need to manipulate
> +		 * MDSCR_EL1.SS and PSTATE.SS. If not we need to
> +		 * suppress the guests ability to trigger single step
> +		 * exceptions as otherwise the host will deal with them.

is this because if you are doing any kind of guest debugging, we trap
all debug exceptions to EL2 and therefore single-stepping in the guest
won't work anyway and the host doesn't know what to do with such
exceptions?

I would feel slightly better if the comment assured me this doesn't
outright break something, but ok...

> +		 */
> +		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> +			*vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
> +			vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
> +		} else {
> +			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
> +		}
> +	}
>  }
>  
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
> -	/* Nothing to do yet */
> +	if (vcpu->guest_debug)
> +		restore_guest_debug_regs(vcpu);
>  }
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 27f38a9..e9de13e 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -103,6 +103,7 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	run->debug.arch.hsr = hsr;
>  
>  	switch (hsr >> ESR_ELx_EC_SHIFT) {
> +	case ESR_ELx_EC_SOFTSTP_LOW:
>  	case ESR_ELx_EC_BKPT32:
>  	case ESR_ELx_EC_BRK64:
>  		break;
> @@ -130,6 +131,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
>  	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
> +	[ESR_ELx_EC_SOFTSTP_LOW]= kvm_handle_guest_debug,
>  	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
>  	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
>  };
> -- 
> 2.4.1
> 

As for the code of this patch:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Re: [PATCH v5 08/12] KVM: arm64: re-factor hyp.S debug register code

[Christoffer Dall]

On Thu, Jun 04, 2015 at 11:34:46AM +0100, Alex Bennée wrote:
> 
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > On Fri, May 29, 2015 at 10:30:24AM +0100, Alex Bennée wrote:
> >> This is a pre-cursor to sharing the code with the guest debug support.
> >> This replaces the big macro that fishes data out of a fixed location
> >> with a more general helper macro to restore a set of debug registers. It
> >> uses macro substitution so it can be re-used for debug control and value
> >> registers. It does however rely on the debug registers being 64 bit
> >> aligned (as they happen to be in the hyp ABI).
> 
> Oops I'd better fix that commit comment.
> 
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> 
> >> ---
> >> v3:
> >>   - return to the patch series
> >>   - add save and restore targets
> >>   - change register use and document
> >> v4:
> >>   - keep original setup/restore names
> >>   - don't use split u32/u64 structure yet
> >> ---
> >>  arch/arm64/kvm/hyp.S | 519 ++++++++++++++-------------------------------------
> >>  1 file changed, 140 insertions(+), 379 deletions(-)
> >> 
> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> >> index 74e63d8..9c4897d 100644
> >> --- a/arch/arm64/kvm/hyp.S
> >> +++ b/arch/arm64/kvm/hyp.S
> >
> >
> > [...]
> >
> >> @@ -465,195 +318,52 @@
> >>  	msr	mdscr_el1,	x25
> >>  .endm
> >>  
> >> -.macro restore_debug
> >> -	// x2: base address for cpu context
> >> -	// x3: tmp register
> >> -
> >> -	mrs	x26, id_aa64dfr0_el1
> >> -	ubfx	x24, x26, #12, #4	// Extract BRPs
> >> -	ubfx	x25, x26, #20, #4	// Extract WRPs
> >> -	mov	w26, #15
> >> -	sub	w24, w26, w24		// How many BPs to skip
> >> -	sub	w25, w26, w25		// How many WPs to skip
> >> -
> >> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> >> +.macro restore_debug type
> >> +        // x4: pointer to register set
> >> +        // x5: number of registers to skip
> >
> > nit: use tabs instead of spaces here...
> >
> >> +	// x6..x22 trashed
> >>  
> >
> > [...]
> >
> >> @@ -887,12 +597,63 @@ __restore_sysregs:
> >>  	restore_sysregs
> >>  	ret
> >>  
> >> +/* Save debug state */
> >>  __save_debug:
> >> -	save_debug
> >> +	// x0: base address for vcpu context
> >> +	// x2: ptr to current CPU context
> >> +	// x4/x5: trashed
> >
> > I had a bunch of questions here which I think you missed last time
> > around:
> >  1. why do we need the vcpu context?
> 
> We don't, I'll drop that
> 
> >  2. what does 'current' mean here?
> 
> Either the host or vcpu context depending which way we are currently going.
> 

drop 'current' please.

> >  3. you're also trashing everything that save_debug trashes, so x4/5,
> >     x6-x22 trashed would be more correct
> 
> OK
> 
> >
> >> +
> >> +	mrs	x26, id_aa64dfr0_el1
> >> +	ubfx	x24, x26, #12, #4	// Extract BRPs
> >> +	ubfx	x25, x26, #20, #4	// Extract WRPs
> >> +	mov	w26, #15
> >> +	sub	w24, w26, w24		// How many BPs to skip
> >> +	sub	w25, w26, w25		// How many WPs to skip
> >> +
> >> +	mov     x5, x24
> >> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
> >> +	save_debug dbgbcr
> >> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
> >> +	save_debug dbgbvr
> >> +
> >> +	mov     x5, x25
> >> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
> >> +	save_debug dbgwcr
> >> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
> >> +	save_debug dbgwvr
> >> +
> >> +	mrs	x21, mdccint_el1
> >> +	str	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
> >>  	ret
> >>  
> >> +/* Restore debug state */
> >>  __restore_debug:
> >> -	restore_debug
> >> +	// x0: base address for cpu context
> >> +	// x2: ptr to current CPU context
> >> +	// x4/x5: trashed
> >
> > and you missed these comments too, basically same stuff, but why was it
> > 'cpu' here and not 'vcpu' like above?
> 
> Again we use the functions both for restoring host and vcpu debug context.
> 
Well, the two functions operate on the same structures, so I would like
them to be consistent...

-Christoffer

Re: [PATCH v5 07/12] KVM: arm64: guest debug, add support for single-step

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Fri, May 29, 2015 at 10:30:23AM +0100, Alex Bennée wrote:
>> This adds support for single-stepping the guest. To do this we need to
>> manipulate the guests PSTATE.SS and MDSCR_EL1.SS bits which we do in the
>> kvm_arm_setup/clear_debug() so we don't affect the apparent state of the
>> guest. Additionally while the host is debugging the guest we suppress
>> the ability of the guest to single-step itself.
>
> I feel like there should be a slightly more elaborate explanation of
> exactly what works and what doesn't work when the guest is single
> stepping something and which choices we've made for supporting or not
> supporting this.

OK, I shall put bit more explanation. I was trying to avoid too much
exposition in the commit comments vs the code.

>
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> ---
>> v2
>>   - Move pstate/mdscr manipulation into C
>>   - don't export guest_debug to assembly
>>   - add accessor for saved_debug regs
>>   - tweak save/restore of mdscr_el1
>> v3
>>   - don't save PC in debug information struct
>>   - rename debug_saved_regs->guest_debug_state
>>   - save whole value, only use bits in restore
>>   - add save/restore_guest-debug_regs helper functions
>>   - simplify commit message for clarity
>>   - rm vcpu_debug_saved_reg access fn
>> v4
>>   - added more comments based on suggestions
>>   - guest_debug_state->guest_debug_preserved
>>   - no point masking restore, we will trap out
>> v5
>>   - more comments
>>   - don't bother preserving pstate.ss
>
> it would have been good if there was some comment explaining the reason
> for this change.
>
>> ---
>>  arch/arm/kvm/arm.c                |  4 ++-
>>  arch/arm64/include/asm/kvm_host.h | 11 ++++++++
>>  arch/arm64/kvm/debug.c            | 58 ++++++++++++++++++++++++++++++++++++---
>>  arch/arm64/kvm/handle_exit.c      |  2 ++
>>  4 files changed, 70 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 064c105..9b3ed6d 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -302,7 +302,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>  	kvm_arm_set_running_vcpu(NULL);
>>  }
>>  
>> -#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP)
>> +#define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE |    \
>> +			    KVM_GUESTDBG_USE_SW_BP | \
>> +			    KVM_GUESTDBG_SINGLESTEP)
>>  
>>  /**
>>   * kvm_arch_vcpu_ioctl_set_guest_debug - set up guest debugging
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 7cb99b5..e2db6a6 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -123,6 +123,17 @@ struct kvm_vcpu_arch {
>>  	 * here.
>>  	 */
>>  
>> +	/*
>> +	 * Guest registers we preserve during guest debugging.
>> +	 *
>> +	 * These shadow registers are updated by the kvm_handle_sys_reg
>> +	 * trap handler if the guest accesses or updates them while we
>> +	 * are using guest debug.
>> +	 */
>> +	struct {
>> +		u32	mdscr_el1;
>> +	} guest_debug_preserved;
>> +
>>  	/* Don't run the guest */
>>  	bool pause;
>>  
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index 8d1bfa4..10a6baa 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -19,11 +19,41 @@
>>  
>>  #include <linux/kvm_host.h>
>>  
>> +#include <asm/debug-monitors.h>
>> +#include <asm/kvm_asm.h>
>>  #include <asm/kvm_arm.h>
>> +#include <asm/kvm_emulate.h>
>> +
>> +/* These are the bits of MDSCR_EL1 we may manipulate */
>> +#define MDSCR_EL1_DEBUG_MASK	(DBG_MDSCR_SS | \
>> +				DBG_MDSCR_KDE | \
>> +				DBG_MDSCR_MDE)
>>  
>>  static DEFINE_PER_CPU(u32, mdcr_el2);
>>  
>>  /**
>> + * save/restore_guest_debug_regs
>> + *
>> + * For some debug operations we need to tweak some guest registers. As
>> + * a result we need to save the state of those registers before we
>> + * make those modifications. This does get confused if the guest
>> + * attempts to control single step while being debugged. It will start
>> + * working again once it is no longer being debugged by the host.
>
> What gets confused and what starts working?

Maybe I should cut from "This does get..." and put more explanation in
the single step comment later.

>
>> + *
>> + * Guest access to MDSCR_EL1 is trapped by the hypervisor and handled
>> + * after we have restored the preserved value to the main context.
>> + */
>> +static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu->arch.guest_debug_preserved.mdscr_el1 = vcpu_sys_reg(vcpu, MDSCR_EL1);
>> +}
>> +
>> +static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu_sys_reg(vcpu, MDSCR_EL1) = vcpu->arch.guest_debug_preserved.mdscr_el1;
>> +}
>> +
>> +/**
>>   * kvm_arm_init_debug - grab what we need for debug
>>   *
>>   * Currently the sole task of this function is to retrieve the initial
>> @@ -38,7 +68,6 @@ void kvm_arm_init_debug(void)
>>  	__this_cpu_write(mdcr_el2, kvm_call_hyp(__kvm_get_mdcr_el2));
>>  }
>>  
>> -
>>  /**
>>   * kvm_arm_setup_debug - set up debug related stuff
>>   *
>> @@ -73,12 +102,33 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>>  	if (trap_debug)
>>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>>  
>> -	/* Trap breakpoints? */
>> -	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
>> +	/* Is Guest debugging in effect? */
>> +	if (vcpu->guest_debug) {
>>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>> +
>> +		/* Save guest debug state */
>> +		save_guest_debug_regs(vcpu);
>> +
>> +		/*
>> +		 * Single Step (ARM ARM D2.12.3 The software step state
>> +		 * machine)
>> +		 *
>> +		 * If we are doing Single Step we need to manipulate
>> +		 * MDSCR_EL1.SS and PSTATE.SS. If not we need to
>> +		 * suppress the guests ability to trigger single step
>> +		 * exceptions as otherwise the host will deal with them.
>
> is this because if you are doing any kind of guest debugging, we trap
> all debug exceptions to EL2 and therefore single-stepping in the guest
> won't work anyway and the host doesn't know what to do with such
> exceptions?

Correct. As MDCR_EL2.TDE is in effect everything gets routed to EL2. For
breakpoints this is not a major problem as we have all the details
for user space to re-inject the exception if required. However for
single-step it means we get an exception as soon as the guest enables
MDSCR_EL1.SS even if it doesn't have MDSCR_EL1.KDE enabled. The guest
would only be expecting the single step to kick in when it eret's to EL0
setting PSTATE.SS.

In future we could consider getting userspace to singlestep the guest
out of the kernel until it goes over the eret but this is starting to
get hairy.

>
> I would feel slightly better if the comment assured me this doesn't
> outright break something, but ok...

Well nothing breaks per-se but if you have a GDB session in the guest
you'll wonder why "start" and the various breaks don't trigger. They
actually will but the host GDB then tries to SS over them and we
continue on until the host has stopped doing any sort of debug.

>
>> +		 */
>> +		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>> +			*vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
>> +			vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
>> +		} else {
>> +			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
>> +		}
>> +	}
>>  }
>>  
>>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>  {
>> -	/* Nothing to do yet */
>> +	if (vcpu->guest_debug)
>> +		restore_guest_debug_regs(vcpu);
>>  }
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 27f38a9..e9de13e 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -103,6 +103,7 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  	run->debug.arch.hsr = hsr;
>>  
>>  	switch (hsr >> ESR_ELx_EC_SHIFT) {
>> +	case ESR_ELx_EC_SOFTSTP_LOW:
>>  	case ESR_ELx_EC_BKPT32:
>>  	case ESR_ELx_EC_BRK64:
>>  		break;
>> @@ -130,6 +131,7 @@ static exit_handle_fn arm_exit_handlers[] = {
>>  	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
>>  	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
>>  	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
>> +	[ESR_ELx_EC_SOFTSTP_LOW]= kvm_handle_guest_debug,
>>  	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
>>  	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
>>  };
>> -- 
>> 2.4.1
>> 
>
> As for the code of this patch:
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

-- 
Alex Bennée

Re: [PATCH v5 03/12] KVM: arm64: guest debug, define API headers

[Alex Bennée]

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Fri, May 29, 2015 at 10:30:19AM +0100, Alex Bennée wrote:
>> This commit defines the API headers for guest debugging. There are two
>> architecture specific debug structures:
>> 
>>   - kvm_guest_debug_arch, allows us to pass in HW debug registers
>>   - kvm_debug_exit_arch, signals exception and possible faulting address
>> 
>> The type of debugging being used is controlled by the architecture
>> specific control bits of the kvm_guest_debug->control flags in the ioctl
>> structure.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> I can re-confirm my ack despite the changes in v4, but this really is
> borderline to keep exiting r-b and a-b tags around from previous patches
> I would think, but ok.

I was wondering how much a patch has to change for the r-b tags to
become invalid. The meat of the API hasn't changed much though.

Drew/David,

Are you still happy with this patch?

>
> -Christoffer
>
>> 
>> ---
>> v2
>>    - expose hsr and pc directly to user-space
>> v3
>>    - s/control/controlled/ in commit message
>>    - add v8 to ARM ARM comment (ARM Architecture Reference Manual)
>>    - add rb tag
>>    - rm pc, add far
>>    - re-word comments on alignment
>>    - rename KVM_ARM_NDBG_REGS -> KVM_ARM_MAX_DBG_REGS
>> v4
>>    - now uses common HW/SW BP define
>>    - add a-b-tag
>>    - use u32 for control regs
>> v5
>>    - revert to have arch specific KVM_GUESTDBG_USE_SW/HW_BP
>>    - rm stale comments dbgctrl was stored as u64
>> ---
>>  arch/arm64/include/uapi/asm/kvm.h | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>> 
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index d268320..43758e7 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -100,12 +100,32 @@ struct kvm_sregs {
>>  struct kvm_fpu {
>>  };
>>  
>> +/*
>> + * See v8 ARM ARM D7.3: Debug Registers
>> + *
>> + * The architectural limit is 16 debug registers of each type although
>> + * in practice there are usually less (see ID_AA64DFR0_EL1).
>> + */
>> +#define KVM_ARM_MAX_DBG_REGS 16
>>  struct kvm_guest_debug_arch {
>> +	__u32 dbg_bcr[KVM_ARM_MAX_DBG_REGS];
>> +	__u64 dbg_bvr[KVM_ARM_MAX_DBG_REGS];
>> +	__u32 dbg_wcr[KVM_ARM_MAX_DBG_REGS];
>> +	__u64 dbg_wvr[KVM_ARM_MAX_DBG_REGS];
>>  };
>>  
>>  struct kvm_debug_exit_arch {
>> +	__u32 hsr;
>> +	__u64 far;
>>  };
>>  
>> +/*
>> + * Architecture specific defines for kvm_guest_debug->control
>> + */
>> +
>> +#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
>> +#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)
>> +
>>  struct kvm_sync_regs {
>>  };
>>  
>> -- 
>> 2.4.1
>> 

-- 
Alex Bennée

Re: [PATCH v5 03/12] KVM: arm64: guest debug, define API headers

[Andrew Jones]

On Thu, Jun 04, 2015 at 02:49:17PM +0100, Alex Bennée wrote:
> 
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > On Fri, May 29, 2015 at 10:30:19AM +0100, Alex Bennée wrote:
> >> This commit defines the API headers for guest debugging. There are two
> >> architecture specific debug structures:
> >> 
> >>   - kvm_guest_debug_arch, allows us to pass in HW debug registers
> >>   - kvm_debug_exit_arch, signals exception and possible faulting address
> >> 
> >> The type of debugging being used is controlled by the architecture
> >> specific control bits of the kvm_guest_debug->control flags in the ioctl
> >> structure.
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >> Reviewed-by: Andrew Jones <drjones@redhat.com>
> >> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > I can re-confirm my ack despite the changes in v4, but this really is
> > borderline to keep exiting r-b and a-b tags around from previous patches
> > I would think, but ok.
> 
> I was wondering how much a patch has to change for the r-b tags to
> become invalid. The meat of the API hasn't changed much though.
> 
> Drew/David,
> 
> Are you still happy with this patch?

I'm fine with it. Sorry I haven't had time to look over the rest of the
series yet this round.

drew

> 
> >
> > -Christoffer
> >
> >> 
> >> ---
> >> v2
> >>    - expose hsr and pc directly to user-space
> >> v3
> >>    - s/control/controlled/ in commit message
> >>    - add v8 to ARM ARM comment (ARM Architecture Reference Manual)
> >>    - add rb tag
> >>    - rm pc, add far
> >>    - re-word comments on alignment
> >>    - rename KVM_ARM_NDBG_REGS -> KVM_ARM_MAX_DBG_REGS
> >> v4
> >>    - now uses common HW/SW BP define
> >>    - add a-b-tag
> >>    - use u32 for control regs
> >> v5
> >>    - revert to have arch specific KVM_GUESTDBG_USE_SW/HW_BP
> >>    - rm stale comments dbgctrl was stored as u64
> >> ---
> >>  arch/arm64/include/uapi/asm/kvm.h | 20 ++++++++++++++++++++
> >>  1 file changed, 20 insertions(+)
> >> 
> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> >> index d268320..43758e7 100644
> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> @@ -100,12 +100,32 @@ struct kvm_sregs {
> >>  struct kvm_fpu {
> >>  };
> >>  
> >> +/*
> >> + * See v8 ARM ARM D7.3: Debug Registers
> >> + *
> >> + * The architectural limit is 16 debug registers of each type although
> >> + * in practice there are usually less (see ID_AA64DFR0_EL1).
> >> + */
> >> +#define KVM_ARM_MAX_DBG_REGS 16
> >>  struct kvm_guest_debug_arch {
> >> +	__u32 dbg_bcr[KVM_ARM_MAX_DBG_REGS];
> >> +	__u64 dbg_bvr[KVM_ARM_MAX_DBG_REGS];
> >> +	__u32 dbg_wcr[KVM_ARM_MAX_DBG_REGS];
> >> +	__u64 dbg_wvr[KVM_ARM_MAX_DBG_REGS];
> >>  };
> >>  
> >>  struct kvm_debug_exit_arch {
> >> +	__u32 hsr;
> >> +	__u64 far;
> >>  };
> >>  
> >> +/*
> >> + * Architecture specific defines for kvm_guest_debug->control
> >> + */
> >> +
> >> +#define KVM_GUESTDBG_USE_SW_BP		(1 << 16)
> >> +#define KVM_GUESTDBG_USE_HW_BP		(1 << 17)
> >> +
> >>  struct kvm_sync_regs {
> >>  };
> >>  
> >> -- 
> >> 2.4.1
> >> 
> 
> -- 
> Alex Bennée
> --
> 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
> 
>