[PATCH v4 00/15] KVM: arm: debug infrastructure support

[PATCH v4 00/15] KVM: arm: debug infrastructure support

[Zhichao Huang]

This patch series adds debug support, a key feature missing from the
KVM/armv7 port.

The main idea is to keep track of whether the host and the guest have any
break/watch points enabled or not. We only do the world switch for debug
registers when the host or the guest is actually using it.

We add a function reading the break/watch control variables directly to
indicate whether the host has enabled any break/watch points or not. 
We only call the function upon guest entry, after preempt_disable() and
local_irq_disable(), so there is no race for it.

We also tried implementing this series with trapping host use of debug
registers to hyp mode, and keep track of host/guest use of the debug
hardware in that way. This, however, proved to be very difficult,
because it requires us to: First, we have to add a new mechanism to
support trapping host, jump from EL2 to EL1 to run our host_trap_handlers,
and then jump back to the orginal code which trigger the trap.
Second, we have to take specail care when tearing down KVM to disable
the traps, we also impose an ordering requirement of whether KVM or
the breakpoint functionality gets initialized first. In the end we decided
that this was too difficult and convoluted compared to simply read the
values from variables, so we reverted back to this approach.

The amount of registers is properly frightening, but CPUs actually
only implement a subset of them. Also, there is a number of registers
we don't bother emulating (things having to do with external debug and
OSlock).

External debug is when you actually plug a physical JTAG into the CPU.
OSlock is a way to prevent "other software" to play with the debug
registers. My understanding is that it is only useful in combination
with the external debug. In both case, implementing support for this
is probably not worth the effort, at least for the time being.

This has been tested on a Cortex-A15 platform, running 32bit guests.

The patches for this series are based off v4.2-rc6 and can be found
at:

https://git.linaro.org/people/zhichao.huang/linux.git
branch: guest-debug/4.2-rc6-v4

>From v3 [3]:
- Redefine kvm_cpu_context_t as a new struct including the cp14 states
- Save host cp14 states in the vcpu struct intead of memory
- Add a function to keep track of the host use of the debug registers
- Add new lazy world switch mechanism

>From v2 [2]:
- Delete the debug mode enabling/disabling strategy
- Add missing cp14/cp15 trace events

>From v1 [1]:
- Added missing cp14 reset functions
- Disable debug mode if we don't need it to reduce unnecessary switch

[1]: https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014729.html
[2]: https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014847.html
[3]: https://lists.cs.columbia.edu/pipermail/kvmarm/2015-June/015167.html

Zhichao Huang (15):
  KVM: arm: plug guest debug exploit
  KVM: arm: rename pm_fake handler to trap_raz_wi
  KVM: arm: enable to use the ARM_DSCR_MDBGEN macro from KVM assembly
    code
  KVM: arm: common infrastructure for handling AArch32 CP14/CP15
  KVM: arm: check ordering of all system register tables
  KVM: arm: add trap handlers for 32-bit debug registers
  KVM: arm: add trap handlers for 64-bit debug registers
  KVM: arm: add a trace event for cp14 traps
  KVM: arm: redefine kvm_cpu_context_t to save the host cp14 states
  KVM: arm: implement world switch for debug registers
  KVM: arm: add a function to keep track of host use of the debug
    registers
  KVM: arm: keep track of host use of the debug registers
  KVM: arm: keep track of guest use of the debug registers
  KVM: arm: implement lazy world switch for debug registers
  KVM: arm: enable trapping of all debug registers

 arch/arm/include/asm/hw_breakpoint.h |  59 +++--
 arch/arm/include/asm/kvm_asm.h       |  17 ++
 arch/arm/include/asm/kvm_coproc.h    |   3 +-
 arch/arm/include/asm/kvm_host.h      |  13 +-
 arch/arm/kernel/asm-offsets.c        |   6 +-
 arch/arm/kernel/hw_breakpoint.c      |  21 ++
 arch/arm/kvm/arm.c                   |   2 +
 arch/arm/kvm/coproc.c                | 445 ++++++++++++++++++++++++++++++-----
 arch/arm/kvm/handle_exit.c           |   4 +-
 arch/arm/kvm/interrupts.S            |  18 +-
 arch/arm/kvm/interrupts_head.S       | 188 ++++++++++++++-
 arch/arm/kvm/trace.h                 |  30 +++
 arch/arm64/include/asm/kvm_host.h    |   1 +
 13 files changed, 707 insertions(+), 100 deletions(-)

-- 
1.7.12.4

[PATCH v4 01/15] KVM: arm: plug guest debug exploit

[Zhichao Huang]

Hardware debugging in guests is not intercepted currently, it means
that a malicious guest can bring down the entire machine by writing
to the debug registers.

This patch enable trapping of all debug registers, preventing the guests
to access the debug registers.

This patch also disable the debug mode(DBGDSCR) in the guest world all
the time, preventing the guests to mess with the host state.

However, it is a precursor for later patches which will need to do
more to world switch debug states while necessary.

Cc: <stable@vger.kernel.org>
Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/include/asm/kvm_coproc.h |  3 +-
 arch/arm/kvm/coproc.c             | 82 +++++++++++++++++++++++++++++----------
 arch/arm/kvm/handle_exit.c        |  4 +-
 arch/arm/kvm/interrupts.S         | 12 +++---
 arch/arm/kvm/interrupts_head.S    | 21 ++++++++--
 5 files changed, 89 insertions(+), 33 deletions(-)

diff --git a/arch/arm/include/asm/kvm_coproc.h b/arch/arm/include/asm/kvm_coproc.h
index 4917c2f..e74ab0f 100644
--- a/arch/arm/include/asm/kvm_coproc.h
+++ b/arch/arm/include/asm/kvm_coproc.h
@@ -31,7 +31,8 @@ void kvm_register_target_coproc_table(struct kvm_coproc_target_table *table);
 int kvm_handle_cp10_id(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp_0_13_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run);
-int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
+int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
+int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
 
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index f3d88dc..a885cfe 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -91,12 +91,6 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
-int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run)
-{
-	kvm_inject_undefined(vcpu);
-	return 1;
-}
-
 static void reset_mpidr(struct kvm_vcpu *vcpu, const struct coproc_reg *r)
 {
 	/*
@@ -465,12 +459,8 @@ static int emulate_cp15(struct kvm_vcpu *vcpu,
 	return 1;
 }
 
-/**
- * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15 access
- * @vcpu: The VCPU pointer
- * @run:  The kvm_run struct
- */
-int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
+static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, struct kvm_run *run,
+			    bool cp15)
 {
 	struct coproc_params params;
 
@@ -484,7 +474,35 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
 	params.CRm = 0;
 
-	return emulate_cp15(vcpu, &params);
+	if (cp15)
+		return emulate_cp15(vcpu, &params);
+
+	/* raz_wi cp14 */
+	(void)pm_fake(vcpu, &params, NULL);
+
+	/* handled */
+	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+	return 1;
+}
+
+/**
+ * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15 access
+ * @vcpu: The VCPU pointer
+ * @run:  The kvm_run struct
+ */
+int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	return kvm_handle_cp_64(vcpu, run, 1);
+}
+
+/**
+ * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14 access
+ * @vcpu: The VCPU pointer
+ * @run:  The kvm_run struct
+ */
+int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	return kvm_handle_cp_64(vcpu, run, 0);
 }
 
 static void reset_coproc_regs(struct kvm_vcpu *vcpu,
@@ -497,12 +515,8 @@ static void reset_coproc_regs(struct kvm_vcpu *vcpu,
 			table[i].reset(vcpu, &table[i]);
 }
 
-/**
- * kvm_handle_cp15_32 -- handles a mrc/mcr trap on a guest CP15 access
- * @vcpu: The VCPU pointer
- * @run:  The kvm_run struct
- */
-int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
+static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, struct kvm_run *run,
+			      bool cp15)
 {
 	struct coproc_params params;
 
@@ -516,7 +530,35 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
 	params.Rt2 = 0;
 
-	return emulate_cp15(vcpu, &params);
+	if (cp15)
+		return emulate_cp15(vcpu, &params);
+
+	/* raz_wi cp14 */
+	(void)pm_fake(vcpu, &params, NULL);
+
+	/* handled */
+	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+	return 1;
+}
+
+/**
+ * kvm_handle_cp15_32 -- handles a mrc/mcr trap on a guest CP15 access
+ * @vcpu: The VCPU pointer
+ * @run:  The kvm_run struct
+ */
+int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	return kvm_handle_cp_32(vcpu, run, 1);
+}
+
+/**
+ * kvm_handle_cp14_32 -- handles a mrc/mcr trap on a guest CP14 access
+ * @vcpu: The VCPU pointer
+ * @run:  The kvm_run struct
+ */
+int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	return kvm_handle_cp_32(vcpu, run, 0);
 }
 
 /******************************************************************************
diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
index 95f12b2..357ad1b 100644
--- a/arch/arm/kvm/handle_exit.c
+++ b/arch/arm/kvm/handle_exit.c
@@ -104,9 +104,9 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[HSR_EC_WFI]		= kvm_handle_wfx,
 	[HSR_EC_CP15_32]	= kvm_handle_cp15_32,
 	[HSR_EC_CP15_64]	= kvm_handle_cp15_64,
-	[HSR_EC_CP14_MR]	= kvm_handle_cp14_access,
+	[HSR_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[HSR_EC_CP14_LS]	= kvm_handle_cp14_load_store,
-	[HSR_EC_CP14_64]	= kvm_handle_cp14_access,
+	[HSR_EC_CP14_64]	= kvm_handle_cp14_64,
 	[HSR_EC_CP_0_13]	= kvm_handle_cp_0_13_access,
 	[HSR_EC_CP10_ID]	= kvm_handle_cp10_id,
 	[HSR_EC_SVC_HYP]	= handle_svc_hyp,
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index 568494d..48333ff 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -112,9 +112,9 @@ ENTRY(__kvm_vcpu_run)
 	restore_vgic_state
 	restore_timer_state
 
-	@ Store hardware CP15 state and load guest state
-	read_cp15_state store_to_vcpu = 0
-	write_cp15_state read_from_vcpu = 1
+	@ Store hardware CP14/CP15 state and load guest state
+	read_coproc_state store_to_vcpu = 0
+	write_coproc_state read_from_vcpu = 1
 
 	@ If the host kernel has not been configured with VFPv3 support,
 	@ then it is safer if we deny guests from using it as well.
@@ -199,9 +199,9 @@ after_vfp_restore:
 	mrc	p15, 0, r2, c0, c0, 5
 	mcr	p15, 4, r2, c0, c0, 5
 
-	@ Store guest CP15 state and restore host state
-	read_cp15_state store_to_vcpu = 1
-	write_cp15_state read_from_vcpu = 0
+	@ Store guest CP14/CP15 state and restore host state
+	read_coproc_state store_to_vcpu = 1
+	write_coproc_state read_from_vcpu = 0
 
 	save_timer_state
 	save_vgic_state
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 702740d..7c4075c 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -239,7 +239,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	save_guest_regs_mode irq, #VCPU_IRQ_REGS
 .endm
 
-/* Reads cp15 registers from hardware and stores them in memory
+/* Reads cp14/cp15 registers from hardware and stores them in memory
  * @store_to_vcpu: If 0, registers are written in-order to the stack,
  * 		   otherwise to the VCPU struct pointed to by vcpup
  *
@@ -247,7 +247,12 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  *
  * Clobbers r2 - r12
  */
-.macro read_cp15_state store_to_vcpu
+.macro read_coproc_state store_to_vcpu
+	.if \store_to_vcpu == 0
+	mrc	p14, 0, r2, c0, c1, 0	@ DBGDSCR
+	push	{r2}
+	.endif
+
 	mrc	p15, 0, r2, c1, c0, 0	@ SCTLR
 	mrc	p15, 0, r3, c1, c0, 2	@ CPACR
 	mrc	p15, 0, r4, c2, c0, 2	@ TTBCR
@@ -325,7 +330,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  *
  * Assumes vcpu pointer in vcpu reg
  */
-.macro write_cp15_state read_from_vcpu
+.macro write_coproc_state read_from_vcpu
 	.if \read_from_vcpu == 0
 	pop	{r2,r4-r7}
 	.else
@@ -394,6 +399,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	mcr	p15, 0, r10, c10, c2, 0	@ PRRR
 	mcr	p15, 0, r11, c10, c2, 1	@ NMRR
 	mcr	p15, 2, r12, c0, c0, 0	@ CSSELR
+
+	.if \read_from_vcpu == 0
+	pop	{r2}
+	.else
+	mov	r2, #0
+	.endif
+
+	mcr	p14, 0, r2, c0, c2, 2	@ DBGDSCR
 .endm
 
 /*
@@ -620,7 +633,7 @@ ARM_BE8(rev	r6, r6  )
  * (hardware reset value is 0) */
 .macro set_hdcr operation
 	mrc	p15, 4, r2, c1, c1, 1
-	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
+	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
 	.if \operation == vmentry
 	orr	r2, r2, r3		@ Trap some perfmon accesses
 	.else
-- 
1.7.12.4

[PATCH v4 02/15] KVM: arm: rename pm_fake handler to trap_raz_wi

[Zhichao Huang]

pm_fake doesn't quite describe what the handler does (ignoring writes
and returning 0 for reads).

As we're about to use it (a lot) in a different context, rename it
with a (admitedly cryptic) name that make sense for all users.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
Reviewed-by: Alex Bennee <alex.bennee@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/coproc.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index a885cfe..4db571d 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -229,7 +229,7 @@ bool access_vm_reg(struct kvm_vcpu *vcpu,
  * must always support PMCCNTR (the cycle counter): we just RAZ/WI for
  * all PM registers, which doesn't crash the guest kernel at least.
  */
-static bool pm_fake(struct kvm_vcpu *vcpu,
+static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 		    const struct coproc_params *p,
 		    const struct coproc_reg *r)
 {
@@ -239,19 +239,19 @@ static bool pm_fake(struct kvm_vcpu *vcpu,
 		return read_zero(vcpu, p);
 }
 
-#define access_pmcr pm_fake
-#define access_pmcntenset pm_fake
-#define access_pmcntenclr pm_fake
-#define access_pmovsr pm_fake
-#define access_pmselr pm_fake
-#define access_pmceid0 pm_fake
-#define access_pmceid1 pm_fake
-#define access_pmccntr pm_fake
-#define access_pmxevtyper pm_fake
-#define access_pmxevcntr pm_fake
-#define access_pmuserenr pm_fake
-#define access_pmintenset pm_fake
-#define access_pmintenclr pm_fake
+#define access_pmcr trap_raz_wi
+#define access_pmcntenset trap_raz_wi
+#define access_pmcntenclr trap_raz_wi
+#define access_pmovsr trap_raz_wi
+#define access_pmselr trap_raz_wi
+#define access_pmceid0 trap_raz_wi
+#define access_pmceid1 trap_raz_wi
+#define access_pmccntr trap_raz_wi
+#define access_pmxevtyper trap_raz_wi
+#define access_pmxevcntr trap_raz_wi
+#define access_pmuserenr trap_raz_wi
+#define access_pmintenset trap_raz_wi
+#define access_pmintenclr trap_raz_wi
 
 /* Architected CP15 registers.
  * CRn denotes the primary register number, but is copied to the CRm in the
@@ -478,7 +478,7 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		return emulate_cp15(vcpu, &params);
 
 	/* raz_wi cp14 */
-	(void)pm_fake(vcpu, &params, NULL);
+	(void)trap_raz_wi(vcpu, &params, NULL);
 
 	/* handled */
 	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
@@ -534,7 +534,7 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		return emulate_cp15(vcpu, &params);
 
 	/* raz_wi cp14 */
-	(void)pm_fake(vcpu, &params, NULL);
+	(void)trap_raz_wi(vcpu, &params, NULL);
 
 	/* handled */
 	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-- 
1.7.12.4

[PATCH v4 03/15] KVM: arm: enable to use the ARM_DSCR_MDBGEN macro from KVM assembly code

[Zhichao Huang]

Add #ifndef __ASSEMBLY__ in hw_breakpoint.h, in order to use
the ARM_DSCR_MDBGEN macro from KVM assembly code.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
Reviewed-by: Alex Bennee <alex.bennee@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/include/asm/hw_breakpoint.h | 54 +++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
index 8e427c7..f2f4c61 100644
--- a/arch/arm/include/asm/hw_breakpoint.h
+++ b/arch/arm/include/asm/hw_breakpoint.h
@@ -3,6 +3,8 @@
 
 #ifdef __KERNEL__
 
+#ifndef __ASSEMBLY__
+
 struct task_struct;
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
@@ -44,6 +46,33 @@ static inline void decode_ctrl_reg(u32 reg,
 	ctrl->mismatch	= reg & 0x1;
 }
 
+struct notifier_block;
+struct perf_event;
+struct pmu;
+
+extern struct pmu perf_ops_bp;
+extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
+				  int *gen_len, int *gen_type);
+extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
+extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
+extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
+					   unsigned long val, void *data);
+
+extern u8 arch_get_debug_arch(void);
+extern u8 arch_get_max_wp_len(void);
+extern void clear_ptrace_hw_breakpoint(struct task_struct *tsk);
+
+int arch_install_hw_breakpoint(struct perf_event *bp);
+void arch_uninstall_hw_breakpoint(struct perf_event *bp);
+void hw_breakpoint_pmu_read(struct perf_event *bp);
+int hw_breakpoint_slots(int type);
+
+#else
+static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk) {}
+
+#endif	/* CONFIG_HAVE_HW_BREAKPOINT */
+#endif  /* __ASSEMBLY */
+
 /* Debug architecture numbers. */
 #define ARM_DEBUG_ARCH_RESERVED	0	/* In case of ptrace ABI updates. */
 #define ARM_DEBUG_ARCH_V6	1
@@ -110,30 +139,5 @@ static inline void decode_ctrl_reg(u32 reg,
 	asm volatile("mcr p14, 0, %0, " #N "," #M ", " #OP2 : : "r" (VAL));\
 } while (0)
 
-struct notifier_block;
-struct perf_event;
-struct pmu;
-
-extern struct pmu perf_ops_bp;
-extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
-				  int *gen_len, int *gen_type);
-extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
-extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
-extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
-					   unsigned long val, void *data);
-
-extern u8 arch_get_debug_arch(void);
-extern u8 arch_get_max_wp_len(void);
-extern void clear_ptrace_hw_breakpoint(struct task_struct *tsk);
-
-int arch_install_hw_breakpoint(struct perf_event *bp);
-void arch_uninstall_hw_breakpoint(struct perf_event *bp);
-void hw_breakpoint_pmu_read(struct perf_event *bp);
-int hw_breakpoint_slots(int type);
-
-#else
-static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk) {}
-
-#endif	/* CONFIG_HAVE_HW_BREAKPOINT */
 #endif	/* __KERNEL__ */
 #endif	/* _ARM_HW_BREAKPOINT_H */
-- 
1.7.12.4

[PATCH v4 04/15] KVM: arm: common infrastructure for handling AArch32 CP14/CP15

[Zhichao Huang]

As we're about to trap a bunch of CP14 registers, let's rework
the CP15 handling so it can be generalized and work with multiple
tables.

We stop trapping access here, because we haven't finished our trap
handlers. We will enable trapping agian until everything is OK.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/kvm/coproc.c          | 168 ++++++++++++++++++++++++++---------------
 arch/arm/kvm/interrupts_head.S |   2 +-
 2 files changed, 110 insertions(+), 60 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 4db571d..d23395b 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -375,6 +375,9 @@ static const struct coproc_reg cp15_regs[] = {
 	{ CRn(15), CRm( 0), Op1( 4), Op2( 0), is32, access_cbar},
 };
 
+static const struct coproc_reg cp14_regs[] = {
+};
+
 /* Target specific emulation tables */
 static struct kvm_coproc_target_table *target_tables[KVM_ARM_NUM_TARGETS];
 
@@ -424,43 +427,75 @@ static const struct coproc_reg *find_reg(const struct coproc_params *params,
 	return NULL;
 }
 
-static int emulate_cp15(struct kvm_vcpu *vcpu,
-			const struct coproc_params *params)
+/*
+ * emulate_cp --  tries to match a cp14/cp15 access in a handling table,
+ *                and call the corresponding trap handler.
+ *
+ * @params: pointer to the descriptor of the access
+ * @table: array of trap descriptors
+ * @num: size of the trap descriptor array
+ *
+ * Return 0 if the access has been handled, and -1 if not.
+ */
+static int emulate_cp(struct kvm_vcpu *vcpu,
+			const struct coproc_params *params,
+			const struct coproc_reg *table,
+			size_t num)
 {
-	size_t num;
-	const struct coproc_reg *table, *r;
-
-	trace_kvm_emulate_cp15_imp(params->Op1, params->Rt1, params->CRn,
-				   params->CRm, params->Op2, params->is_write);
+	const struct coproc_reg *r;
 
-	table = get_target_table(vcpu->arch.target, &num);
+	if (!table)
+		return -1;	/* Not handled */
 
-	/* Search target-specific then generic table. */
 	r = find_reg(params, table, num);
-	if (!r)
-		r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs));
 
-	if (likely(r)) {
+	if (r) {
 		/* If we don't have an accessor, we should never get here! */
 		BUG_ON(!r->access);
 
 		if (likely(r->access(vcpu, params, r))) {
 			/* Skip instruction, since it was emulated */
 			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-			return 1;
 		}
-		/* If access function fails, it should complain. */
-	} else {
-		kvm_err("Unsupported guest CP15 access at: %08lx\n",
-			*vcpu_pc(vcpu));
-		print_cp_instr(params);
+
+		/* Handled */
+		return 0;
 	}
+
+	/* Not handled */
+	return -1;
+}
+
+static void unhandled_cp_access(struct kvm_vcpu *vcpu,
+				const struct coproc_params *params)
+{
+	u8 hsr_ec = kvm_vcpu_trap_get_class(vcpu);
+	int cp;
+
+	switch (hsr_ec) {
+	case HSR_EC_CP15_32:
+	case HSR_EC_CP15_64:
+		cp = 15;
+		break;
+	case HSR_EC_CP14_MR:
+	case HSR_EC_CP14_64:
+		cp = 14;
+		break;
+	default:
+		WARN_ON((cp = -1));
+	}
+
+	kvm_err("Unsupported guest CP%d access at: %08lx\n",
+		cp, *vcpu_pc(vcpu));
+	print_cp_instr(params);
 	kvm_inject_undefined(vcpu);
-	return 1;
 }
 
-static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, struct kvm_run *run,
-			    bool cp15)
+int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
+			const struct coproc_reg *global,
+			size_t nr_global,
+			const struct coproc_reg *target_specific,
+			size_t nr_specific)
 {
 	struct coproc_params params;
 
@@ -474,37 +509,15 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
 	params.CRm = 0;
 
-	if (cp15)
-		return emulate_cp15(vcpu, &params);
+	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
+		return 1;
+	if (!emulate_cp(vcpu, &params, global, nr_global))
+		return 1;
 
-	/* raz_wi cp14 */
-	(void)trap_raz_wi(vcpu, &params, NULL);
-
-	/* handled */
-	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+	unhandled_cp_access(vcpu, &params);
 	return 1;
 }
 
-/**
- * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15 access
- * @vcpu: The VCPU pointer
- * @run:  The kvm_run struct
- */
-int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
-{
-	return kvm_handle_cp_64(vcpu, run, 1);
-}
-
-/**
- * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14 access
- * @vcpu: The VCPU pointer
- * @run:  The kvm_run struct
- */
-int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
-{
-	return kvm_handle_cp_64(vcpu, run, 0);
-}
-
 static void reset_coproc_regs(struct kvm_vcpu *vcpu,
 			      const struct coproc_reg *table, size_t num)
 {
@@ -515,8 +528,11 @@ static void reset_coproc_regs(struct kvm_vcpu *vcpu,
 			table[i].reset(vcpu, &table[i]);
 }
 
-static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, struct kvm_run *run,
-			      bool cp15)
+int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
+			const struct coproc_reg *global,
+			size_t nr_global,
+			const struct coproc_reg *target_specific,
+			size_t nr_specific)
 {
 	struct coproc_params params;
 
@@ -530,25 +546,57 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
 	params.Rt2 = 0;
 
-	if (cp15)
-		return emulate_cp15(vcpu, &params);
-
-	/* raz_wi cp14 */
-	(void)trap_raz_wi(vcpu, &params, NULL);
+	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
+		return 1;
+	if (!emulate_cp(vcpu, &params, global, nr_global))
+		return 1;
 
-	/* handled */
-	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+	unhandled_cp_access(vcpu, &params);
 	return 1;
 }
 
 /**
+ * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15 access
+ * @vcpu: The VCPU pointer
+ * @run:  The kvm_run struct
+ */
+int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	const struct coproc_reg *target_specific;
+	size_t num;
+
+	target_specific = get_target_table(vcpu->arch.target, &num);
+	return kvm_handle_cp_64(vcpu,
+				cp15_regs, ARRAY_SIZE(cp15_regs),
+				target_specific, num);
+}
+
+/**
  * kvm_handle_cp15_32 -- handles a mrc/mcr trap on a guest CP15 access
  * @vcpu: The VCPU pointer
  * @run:  The kvm_run struct
  */
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
-	return kvm_handle_cp_32(vcpu, run, 1);
+	const struct coproc_reg *target_specific;
+	size_t num;
+
+	target_specific = get_target_table(vcpu->arch.target, &num);
+	return kvm_handle_cp_32(vcpu,
+				cp15_regs, ARRAY_SIZE(cp15_regs),
+				target_specific, num);
+}
+
+/**
+ * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14 access
+ * @vcpu: The VCPU pointer
+ * @run:  The kvm_run struct
+ */
+int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	return kvm_handle_cp_64(vcpu,
+				cp14_regs, ARRAY_SIZE(cp14_regs),
+				NULL, 0);
 }
 
 /**
@@ -558,7 +606,9 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
  */
 int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
-	return kvm_handle_cp_32(vcpu, run, 0);
+	return kvm_handle_cp_32(vcpu,
+				cp14_regs, ARRAY_SIZE(cp14_regs),
+				NULL, 0);
 }
 
 /******************************************************************************
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 7c4075c..7ac5e51 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -633,7 +633,7 @@ ARM_BE8(rev	r6, r6  )
  * (hardware reset value is 0) */
 .macro set_hdcr operation
 	mrc	p15, 4, r2, c1, c1, 1
-	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
+	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
 	.if \operation == vmentry
 	orr	r2, r2, r3		@ Trap some perfmon accesses
 	.else
-- 
1.7.12.4

[PATCH v4 05/15] KVM: arm: check ordering of all system register tables

[Zhichao Huang]

We now have multiple tables for the various system registers
we trap. Make sure we check the order of all of them, as it is
critical that we get the order right (been there, done that...).

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/coproc.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index d23395b..16d5f69 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -737,6 +737,9 @@ static struct coproc_reg invariant_cp15[] = {
 	{ CRn( 0), CRm( 0), Op1( 0), Op2( 3), is32, NULL, get_TLBTR },
 	{ CRn( 0), CRm( 0), Op1( 0), Op2( 6), is32, NULL, get_REVIDR },
 
+	{ CRn( 0), CRm( 0), Op1( 1), Op2( 1), is32, NULL, get_CLIDR },
+	{ CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
+
 	{ CRn( 0), CRm( 1), Op1( 0), Op2( 0), is32, NULL, get_ID_PFR0 },
 	{ CRn( 0), CRm( 1), Op1( 0), Op2( 1), is32, NULL, get_ID_PFR1 },
 	{ CRn( 0), CRm( 1), Op1( 0), Op2( 2), is32, NULL, get_ID_DFR0 },
@@ -752,9 +755,6 @@ static struct coproc_reg invariant_cp15[] = {
 	{ CRn( 0), CRm( 2), Op1( 0), Op2( 3), is32, NULL, get_ID_ISAR3 },
 	{ CRn( 0), CRm( 2), Op1( 0), Op2( 4), is32, NULL, get_ID_ISAR4 },
 	{ CRn( 0), CRm( 2), Op1( 0), Op2( 5), is32, NULL, get_ID_ISAR5 },
-
-	{ CRn( 0), CRm( 0), Op1( 1), Op2( 1), is32, NULL, get_CLIDR },
-	{ CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
 /*
@@ -1297,13 +1297,29 @@ int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 	return write_demux_regids(uindices);
 }
 
+static int check_sysreg_table(const struct coproc_reg *table, unsigned int n)
+{
+	unsigned int i;
+
+	for (i = 1; i < n; i++) {
+		if (cmp_reg(&table[i-1], &table[i]) >= 0) {
+			kvm_err("sys_reg table %p out of order (%d)\n",
+					table, i - 1);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 void kvm_coproc_table_init(void)
 {
 	unsigned int i;
 
 	/* Make sure tables are unique and in order. */
-	for (i = 1; i < ARRAY_SIZE(cp15_regs); i++)
-		BUG_ON(cmp_reg(&cp15_regs[i-1], &cp15_regs[i]) >= 0);
+	BUG_ON(check_sysreg_table(cp14_regs, ARRAY_SIZE(cp14_regs)));
+	BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
+	BUG_ON(check_sysreg_table(invariant_cp15, ARRAY_SIZE(invariant_cp15)));
 
 	/* We abuse the reset function to overwrite the table itself. */
 	for (i = 0; i < ARRAY_SIZE(invariant_cp15); i++)
-- 
1.7.12.4

[PATCH v4 06/15] KVM: arm: add trap handlers for 32-bit debug registers

[Zhichao Huang]

Add handlers for all the 32-bit debug registers.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/include/asm/kvm_asm.h  |  12 ++++
 arch/arm/include/asm/kvm_host.h |   3 +
 arch/arm/kernel/asm-offsets.c   |   1 +
 arch/arm/kvm/coproc.c           | 124 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 140 insertions(+)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 194c91b..7443a3a 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -52,6 +52,18 @@
 #define c10_AMAIR1	30	/* Auxilary Memory Attribute Indirection Reg1 */
 #define NR_CP15_REGS	31	/* Number of regs (incl. invalid) */
 
+/* 0 is reserved as an invalid value. */
+#define cp14_DBGBVR0	1	/* Debug Breakpoint Control Registers (0-15) */
+#define cp14_DBGBVR15	16
+#define cp14_DBGBCR0	17	/* Debug Breakpoint Value Registers (0-15) */
+#define cp14_DBGBCR15	32
+#define cp14_DBGWVR0	33	/* Debug Watchpoint Control Registers (0-15) */
+#define cp14_DBGWVR15	48
+#define cp14_DBGWCR0	49	/* Debug Watchpoint Value Registers (0-15) */
+#define cp14_DBGWCR15	64
+#define cp14_DBGDSCRext	65	/* Debug Status and Control external */
+#define NR_CP14_REGS	66	/* Number of regs (incl. invalid) */
+
 #define ARM_EXCEPTION_RESET	  0
 #define ARM_EXCEPTION_UNDEFINED   1
 #define ARM_EXCEPTION_SOFTWARE    2
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index e896d2c..63ac005 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -124,6 +124,9 @@ struct kvm_vcpu_arch {
 	struct vgic_cpu vgic_cpu;
 	struct arch_timer_cpu timer_cpu;
 
+	/* System control coprocessor (cp14) */
+	u32 cp14[NR_CP14_REGS];
+
 	/*
 	 * Anything that is not used directly from assembly code goes
 	 * here.
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 871b826..9158de0 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -172,6 +172,7 @@ int main(void)
 #ifdef CONFIG_KVM_ARM_HOST
   DEFINE(VCPU_KVM,		offsetof(struct kvm_vcpu, kvm));
   DEFINE(VCPU_MIDR,		offsetof(struct kvm_vcpu, arch.midr));
+  DEFINE(VCPU_CP14,		offsetof(struct kvm_vcpu, arch.cp14));
   DEFINE(VCPU_CP15,		offsetof(struct kvm_vcpu, arch.cp15));
   DEFINE(VCPU_VFP_GUEST,	offsetof(struct kvm_vcpu, arch.vfp_guest));
   DEFINE(VCPU_VFP_HOST,		offsetof(struct kvm_vcpu, arch.host_cpu_context));
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 16d5f69..b3627f0 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -220,6 +220,47 @@ bool access_vm_reg(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool trap_debug32(struct kvm_vcpu *vcpu,
+			const struct coproc_params *p,
+			const struct coproc_reg *r)
+{
+	if (p->is_write)
+		vcpu->arch.cp14[r->reg] = *vcpu_reg(vcpu, p->Rt1);
+	else
+		*vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp14[r->reg];
+
+	return true;
+}
+
+/* DBGIDR (RO) Debug ID */
+static bool trap_dbgidr(struct kvm_vcpu *vcpu,
+			const struct coproc_params *p,
+			const struct coproc_reg *r)
+{
+	u32 val;
+
+	if (p->is_write)
+		return ignore_write(vcpu, p);
+
+	ARM_DBG_READ(c0, c0, 0, val);
+	*vcpu_reg(vcpu, p->Rt1) = val;
+
+	return true;
+}
+
+/* DBGDSCRint (RO) Debug Status and Control Register */
+static bool trap_dbgdscr(struct kvm_vcpu *vcpu,
+			const struct coproc_params *p,
+			const struct coproc_reg *r)
+{
+	if (p->is_write)
+		return ignore_write(vcpu, p);
+
+	*vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp14[r->reg];
+
+	return true;
+}
+
 /*
  * We could trap ID_DFR0 and tell the guest we don't support performance
  * monitoring.  Unfortunately the patch to make the kernel check ID_DFR0 was
@@ -375,7 +416,90 @@ static const struct coproc_reg cp15_regs[] = {
 	{ CRn(15), CRm( 0), Op1( 4), Op2( 0), is32, access_cbar},
 };
 
+#define DBG_BCR_BVR_WCR_WVR(n)					\
+	/* DBGBVRn */						\
+	{ CRn( 0), CRm((n)), Op1( 0), Op2( 4), is32,		\
+	  trap_debug32,	reset_val, (cp14_DBGBVR0 + (n)), 0 },	\
+	/* DBGBCRn */						\
+	{ CRn( 0), CRm((n)), Op1( 0), Op2( 5), is32,		\
+	  trap_debug32,	reset_val, (cp14_DBGBCR0 + (n)), 0 },	\
+	/* DBGWVRn */						\
+	{ CRn( 0), CRm((n)), Op1( 0), Op2( 6), is32,		\
+	  trap_debug32,	reset_val, (cp14_DBGWVR0 + (n)), 0 },	\
+	/* DBGWCRn */						\
+	{ CRn( 0), CRm((n)), Op1( 0), Op2( 7), is32,		\
+	  trap_debug32,	reset_val, (cp14_DBGWCR0 + (n)), 0 }
+
+/* No OS DBGBXVR machanism implemented. */
+#define DBGBXVR(n)						\
+	{ CRn( 1), CRm((n)), Op1( 0), Op2( 1), is32, trap_raz_wi }
+
+/*
+ * Architected CP14 registers.
+ *
+ * Trapped cp14 registers. We generally ignore most of the external
+ * debug, on the principle that they don't really make sense to a
+ * guest. Revisit this one day, should this principle change.
+ */
 static const struct coproc_reg cp14_regs[] = {
+	/* DBGIDR */
+	{ CRn( 0), CRm( 0), Op1( 0), Op2( 0), is32, trap_dbgidr},
+	/* DBGDTRRXext */
+	{ CRn( 0), CRm( 0), Op1( 0), Op2( 2), is32, trap_raz_wi },
+	DBG_BCR_BVR_WCR_WVR(0),
+	/* DBGDSCRint */
+	{ CRn( 0), CRm( 1), Op1( 0), Op2( 0), is32, trap_dbgdscr,
+				NULL, cp14_DBGDSCRext },
+	DBG_BCR_BVR_WCR_WVR(1),
+	/* DBGDSCRext */
+	{ CRn( 0), CRm( 2), Op1( 0), Op2( 2), is32, trap_debug32,
+				reset_val, cp14_DBGDSCRext, 0 },
+	DBG_BCR_BVR_WCR_WVR(2),
+	/* DBGDTRTXext */
+	{ CRn( 0), CRm( 3), Op1( 0), Op2( 2), is32, trap_raz_wi },
+	DBG_BCR_BVR_WCR_WVR(3),
+	DBG_BCR_BVR_WCR_WVR(4),
+	/* DBGDTR[RT]Xint */
+	{ CRn( 0), CRm( 5), Op1( 0), Op2( 0), is32, trap_raz_wi },
+	DBG_BCR_BVR_WCR_WVR(5),
+	DBG_BCR_BVR_WCR_WVR(6),
+	/* DBGVCR */
+	{ CRn( 0), CRm( 7), Op1( 0), Op2( 0), is32, trap_debug32 },
+	DBG_BCR_BVR_WCR_WVR(7),
+	DBG_BCR_BVR_WCR_WVR(8),
+	DBG_BCR_BVR_WCR_WVR(9),
+	DBG_BCR_BVR_WCR_WVR(10),
+	DBG_BCR_BVR_WCR_WVR(11),
+	DBG_BCR_BVR_WCR_WVR(12),
+	DBG_BCR_BVR_WCR_WVR(13),
+	DBG_BCR_BVR_WCR_WVR(14),
+	DBG_BCR_BVR_WCR_WVR(15),
+
+	DBGBXVR(0),
+	/* DBGOSLAR */
+	{ CRn( 1), CRm( 0), Op1( 0), Op2( 4), is32, trap_raz_wi },
+	DBGBXVR(1),
+	/* DBGOSLSR */
+	{ CRn( 1), CRm( 1), Op1( 0), Op2( 4), is32, trap_raz_wi },
+	DBGBXVR(2),
+	DBGBXVR(3),
+	/* DBGOSDLRd */
+	{ CRn( 1), CRm( 3), Op1( 0), Op2( 4), is32, trap_raz_wi },
+	DBGBXVR(4),
+	DBGBXVR(5),
+	/* DBGPRSRa */
+	{ CRn( 1), CRm( 5), Op1( 0), Op2( 4), is32, trap_raz_wi },
+
+	DBGBXVR(6),
+	DBGBXVR(7),
+	DBGBXVR(8),
+	DBGBXVR(9),
+	DBGBXVR(10),
+	DBGBXVR(11),
+	DBGBXVR(12),
+	DBGBXVR(13),
+	DBGBXVR(14),
+	DBGBXVR(15),
 };
 
 /* Target specific emulation tables */
-- 
1.7.12.4

[PATCH v4 07/15] KVM: arm: add trap handlers for 64-bit debug registers

[Zhichao Huang]

Add handlers for all the 64-bit debug registers.

There is an overlap between 32 and 64bit registers. Make sure that
64-bit registers preceding 32-bit ones.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/coproc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index b3627f0..2164f4e 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -440,6 +440,12 @@ static const struct coproc_reg cp15_regs[] = {
  * Trapped cp14 registers. We generally ignore most of the external
  * debug, on the principle that they don't really make sense to a
  * guest. Revisit this one day, should this principle change.
+ *
+ * CRn denotes the primary register number, but is copied to the CRm in the
+ * user space API for 64-bit register access in line with the terminology used
+ * in the ARM ARM.
+ * Important: Must be sorted ascending by CRn, CRM, Op1, Op2 and with 64-bit
+ *            registers preceding 32-bit ones.
  */
 static const struct coproc_reg cp14_regs[] = {
 	/* DBGIDR */
@@ -447,10 +453,14 @@ static const struct coproc_reg cp14_regs[] = {
 	/* DBGDTRRXext */
 	{ CRn( 0), CRm( 0), Op1( 0), Op2( 2), is32, trap_raz_wi },
 	DBG_BCR_BVR_WCR_WVR(0),
+	/* DBGDRAR (64bit) */
+	{ CRn( 0), CRm( 1), Op1( 0), Op2( 0), is64, trap_raz_wi },
 	/* DBGDSCRint */
 	{ CRn( 0), CRm( 1), Op1( 0), Op2( 0), is32, trap_dbgdscr,
 				NULL, cp14_DBGDSCRext },
 	DBG_BCR_BVR_WCR_WVR(1),
+	/* DBGDSAR (64bit) */
+	{ CRn( 0), CRm( 2), Op1( 0), Op2( 0), is64, trap_raz_wi },
 	/* DBGDSCRext */
 	{ CRn( 0), CRm( 2), Op1( 0), Op2( 2), is32, trap_debug32,
 				reset_val, cp14_DBGDSCRext, 0 },
-- 
1.7.12.4

[PATCH v4 08/15] KVM: arm: add a trace event for cp14 traps

[Zhichao Huang]

There are too many cp15 traps, so we don't reuse the cp15 trace event
but add a new trace event to trace the access of debug registers.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/coproc.c | 14 ++++++++++++++
 arch/arm/kvm/trace.h  | 30 ++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 2164f4e..d15b250 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -643,6 +643,13 @@ int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
 	params.CRm = 0;
 
+	if (global == cp14_regs)
+		trace_kvm_emulate_cp14_imp(params.Op1, params.Rt1, params.CRn,
+			params.CRm, params.Op2, params.is_write);
+	else
+		trace_kvm_emulate_cp15_imp(params.Op1, params.Rt1, params.CRn,
+			params.CRm, params.Op2, params.is_write);
+
 	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
 		return 1;
 	if (!emulate_cp(vcpu, &params, global, nr_global))
@@ -680,6 +687,13 @@ int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
 	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
 	params.Rt2 = 0;
 
+	if (global == cp14_regs)
+		trace_kvm_emulate_cp14_imp(params.Op1, params.Rt1, params.CRn,
+			params.CRm, params.Op2, params.is_write);
+	else
+		trace_kvm_emulate_cp15_imp(params.Op1, params.Rt1, params.CRn,
+			params.CRm, params.Op2, params.is_write);
+
 	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
 		return 1;
 	if (!emulate_cp(vcpu, &params, global, nr_global))
diff --git a/arch/arm/kvm/trace.h b/arch/arm/kvm/trace.h
index 0ec3539..988da03 100644
--- a/arch/arm/kvm/trace.h
+++ b/arch/arm/kvm/trace.h
@@ -159,6 +159,36 @@ TRACE_EVENT(kvm_emulate_cp15_imp,
 			__entry->CRm, __entry->Op2)
 );
 
+/* Architecturally implementation defined CP14 register access */
+TRACE_EVENT(kvm_emulate_cp14_imp,
+	TP_PROTO(unsigned long Op1, unsigned long Rt1, unsigned long CRn,
+		 unsigned long CRm, unsigned long Op2, bool is_write),
+	TP_ARGS(Op1, Rt1, CRn, CRm, Op2, is_write),
+
+	TP_STRUCT__entry(
+		__field(	unsigned int,	Op1		)
+		__field(	unsigned int,	Rt1		)
+		__field(	unsigned int,	CRn		)
+		__field(	unsigned int,	CRm		)
+		__field(	unsigned int,	Op2		)
+		__field(	bool,		is_write	)
+	),
+
+	TP_fast_assign(
+		__entry->is_write		= is_write;
+		__entry->Op1			= Op1;
+		__entry->Rt1			= Rt1;
+		__entry->CRn			= CRn;
+		__entry->CRm			= CRm;
+		__entry->Op2			= Op2;
+	),
+
+	TP_printk("Implementation defined CP14: %s\tp14, %u, r%u, c%u, c%u, %u",
+			(__entry->is_write) ? "mcr" : "mrc",
+			__entry->Op1, __entry->Rt1, __entry->CRn,
+			__entry->CRm, __entry->Op2)
+);
+
 TRACE_EVENT(kvm_wfx,
 	TP_PROTO(unsigned long vcpu_pc, bool is_wfe),
 	TP_ARGS(vcpu_pc, is_wfe),
-- 
1.7.12.4

[PATCH v4 09/15] KVM: arm: redefine kvm_cpu_context_t to save the host cp14 states

[Zhichao Huang]

Redefine kvm_cpu_context_t as a new struct that include the cp14 states,
which we used to save the host cp14 states.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/include/asm/kvm_host.h | 6 +++++-
 arch/arm/kernel/asm-offsets.c   | 4 +++-
 arch/arm/kvm/interrupts.S       | 6 ++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 63ac005..fc461ac 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -91,7 +91,11 @@ struct kvm_vcpu_fault_info {
 	u32 hyp_pc;		/* PC when exception was taken from Hyp mode */
 };
 
-typedef struct vfp_hard_struct kvm_cpu_context_t;
+struct kvm_cpu_context {
+	struct vfp_hard_struct vfp;
+	u32 cp14[NR_CP14_REGS];
+};
+typedef struct kvm_cpu_context kvm_cpu_context_t;
 
 struct kvm_vcpu_arch {
 	struct kvm_regs regs;
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 9158de0..cee4254 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -175,7 +175,9 @@ int main(void)
   DEFINE(VCPU_CP14,		offsetof(struct kvm_vcpu, arch.cp14));
   DEFINE(VCPU_CP15,		offsetof(struct kvm_vcpu, arch.cp15));
   DEFINE(VCPU_VFP_GUEST,	offsetof(struct kvm_vcpu, arch.vfp_guest));
-  DEFINE(VCPU_VFP_HOST,		offsetof(struct kvm_vcpu, arch.host_cpu_context));
+  DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
+  DEFINE(VCPU_VFP_HOST,		offsetof(struct kvm_cpu_context, vfp));
+  DEFINE(VCPU_CP14_HOST,	offsetof(struct kvm_cpu_context, cp14));
   DEFINE(VCPU_REGS,		offsetof(struct kvm_vcpu, arch.regs));
   DEFINE(VCPU_USR_REGS,		offsetof(struct kvm_vcpu, arch.regs.usr_regs));
   DEFINE(VCPU_SVC_REGS,		offsetof(struct kvm_vcpu, arch.regs.svc_regs));
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index 48333ff..d4ff22e 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -176,8 +176,9 @@ __kvm_vcpu_return:
 	@ Switch VFP/NEON hardware state to the host's
 	add	r7, vcpu, #VCPU_VFP_GUEST
 	store_vfp_state r7
-	add	r7, vcpu, #VCPU_VFP_HOST
+	add	r7, vcpu, #VCPU_HOST_CONTEXT
 	ldr	r7, [r7]
+	add	r7, r7, #VCPU_VFP_HOST
 	restore_vfp_state r7
 
 after_vfp_restore:
@@ -484,8 +485,9 @@ switch_to_guest_vfp:
 	set_hcptr vmtrap, (HCPTR_TCP(10) | HCPTR_TCP(11))
 
 	@ Switch VFP/NEON hardware state to the guest's
-	add	r7, r0, #VCPU_VFP_HOST
+	add	r7, r0, #VCPU_HOST_CONTEXT
 	ldr	r7, [r7]
+	add	r7, r7, #VCPU_VFP_HOST
 	store_vfp_state r7
 	add	r7, r0, #VCPU_VFP_GUEST
 	restore_vfp_state r7
-- 
1.7.12.4

[PATCH v4 10/15] KVM: arm: implement world switch for debug registers

[Zhichao Huang]

Implement switching of the debug registers. While the number
of registers is massive, CPUs usually don't implement them all
(A15 has 6 breakpoints and 4 watchpoints, which gives us a total
of 22 registers "only").

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/kvm/interrupts_head.S | 170 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 159 insertions(+), 11 deletions(-)

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 7ac5e51..b9e7410 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -5,6 +5,7 @@
 #define VCPU_USR_SP		(VCPU_USR_REG(13))
 #define VCPU_USR_LR		(VCPU_USR_REG(14))
 #define CP15_OFFSET(_cp15_reg_idx) (VCPU_CP15 + (_cp15_reg_idx * 4))
+#define CP14_OFFSET(_cp14_reg_idx) ((_cp14_reg_idx) * 4)
 
 /*
  * Many of these macros need to access the VCPU structure, which is always
@@ -239,6 +240,136 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	save_guest_regs_mode irq, #VCPU_IRQ_REGS
 .endm
 
+/* Assume r10/r11/r12 are in use, clobbers r2-r3 */
+.macro cp14_read_and_str base Op2 cp14_reg0 skip_num
+	adr	r3, 1f
+	add	r3, r3, \skip_num, lsl #3
+	bx	r3
+1:
+	mrc	p14, 0, r2, c0, c15, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+15)]
+	mrc	p14, 0, r2, c0, c14, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+14)]
+	mrc	p14, 0, r2, c0, c13, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+13)]
+	mrc	p14, 0, r2, c0, c12, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+12)]
+	mrc	p14, 0, r2, c0, c11, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+11)]
+	mrc	p14, 0, r2, c0, c10, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+10)]
+	mrc	p14, 0, r2, c0, c9, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+9)]
+	mrc	p14, 0, r2, c0, c8, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+8)]
+	mrc	p14, 0, r2, c0, c7, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+7)]
+	mrc	p14, 0, r2, c0, c6, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+6)]
+	mrc	p14, 0, r2, c0, c5, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+5)]
+	mrc	p14, 0, r2, c0, c4, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+4)]
+	mrc	p14, 0, r2, c0, c3, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+3)]
+	mrc	p14, 0, r2, c0, c2, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+2)]
+	mrc	p14, 0, r2, c0, c1, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+1)]
+	mrc	p14, 0, r2, c0, c0, \Op2
+	str     r2, [\base, #CP14_OFFSET(\cp14_reg0)]
+.endm
+
+/* Assume r11/r12 are in use, clobbers r2-r3 */
+.macro cp14_ldr_and_write base Op2 cp14_reg0 skip_num
+	adr	r3, 1f
+	add	r3, r3, \skip_num, lsl #3
+	bx	r3
+1:
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+15)]
+	mcr	p14, 0, r2, c0, c15, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+14)]
+	mcr	p14, 0, r2, c0, c14, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+13)]
+	mcr	p14, 0, r2, c0, c13, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+12)]
+	mcr	p14, 0, r2, c0, c12, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+11)]
+	mcr	p14, 0, r2, c0, c11, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+10)]
+	mcr	p14, 0, r2, c0, c10, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+9)]
+	mcr	p14, 0, r2, c0, c9, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+8)]
+	mcr	p14, 0, r2, c0, c8, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+7)]
+	mcr	p14, 0, r2, c0, c7, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+6)]
+	mcr	p14, 0, r2, c0, c6, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+5)]
+	mcr	p14, 0, r2, c0, c5, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+4)]
+	mcr	p14, 0, r2, c0, c4, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+3)]
+	mcr	p14, 0, r2, c0, c3, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+2)]
+	mcr	p14, 0, r2, c0, c2, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+1)]
+	mcr	p14, 0, r2, c0, c1, \Op2
+	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0)]
+	mcr	p14, 0, r2, c0, c0, \Op2
+.endm
+
+/* Get extract number of BRPs and WRPs. Saved in r11/r12 */
+.macro read_hw_dbg_num
+	mrc	p14, 0, r2, c0, c0, 0
+	ubfx	r11, r2, #24, #4
+	add	r11, r11, #1		@ Extract BRPs
+	ubfx	r12, r2, #28, #4
+	add	r12, r12, #1		@ Extract WRPs
+	mov	r2, #16
+	sub	r11, r2, r11		@ How many BPs to skip
+	sub	r12, r2, r12		@ How many WPs to skip
+.endm
+
+/* Reads cp14 registers from hardware.
+ * Writes cp14 registers in-order to the CP14 struct pointed to by r10
+ *
+ * Assumes vcpu pointer in vcpu reg
+ *
+ * Clobbers r2-r12
+ */
+.macro save_debug_state
+	read_hw_dbg_num
+	cp14_read_and_str r10, 4, cp14_DBGBVR0, r11
+	cp14_read_and_str r10, 5, cp14_DBGBCR0, r11
+	cp14_read_and_str r10, 6, cp14_DBGWVR0, r12
+	cp14_read_and_str r10, 7, cp14_DBGWCR0, r12
+
+	/* DBGDSCR reg */
+	mrc	p14, 0, r2, c0, c1, 0
+	str	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
+.endm
+
+/* Reads cp14 registers in-order from the CP14 struct pointed to by r10
+ * Writes cp14 registers to hardware.
+ *
+ * Assumes vcpu pointer in vcpu reg
+ *
+ * Clobbers r2-r12
+ */
+.macro restore_debug_state
+	read_hw_dbg_num
+	cp14_ldr_and_write r10, 4, cp14_DBGBVR0, r11
+	cp14_ldr_and_write r10, 5, cp14_DBGBCR0, r11
+	cp14_ldr_and_write r10, 6, cp14_DBGWVR0, r12
+	cp14_ldr_and_write r10, 7, cp14_DBGWCR0, r12
+
+	/* DBGDSCR reg */
+	ldr	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
+	mcr	p14, 0, r2, c0, c2, 2
+.endm
+
 /* Reads cp14/cp15 registers from hardware and stores them in memory
  * @store_to_vcpu: If 0, registers are written in-order to the stack,
  * 		   otherwise to the VCPU struct pointed to by vcpup
@@ -248,11 +379,17 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  * Clobbers r2 - r12
  */
 .macro read_coproc_state store_to_vcpu
-	.if \store_to_vcpu == 0
-	mrc	p14, 0, r2, c0, c1, 0	@ DBGDSCR
-	push	{r2}
+	.if \store_to_vcpu == 1
+	add	r10, vcpu, #VCPU_CP14
+	.else
+	add	r10, vcpu, #VCPU_HOST_CONTEXT
+	ldr	r10, [r10]
+	add	r10, r10, #VCPU_CP14_HOST
 	.endif
 
+	/* Assumes r10 pointer in cp14 regs  */
+	bl __save_debug_state
+
 	mrc	p15, 0, r2, c1, c0, 0	@ SCTLR
 	mrc	p15, 0, r3, c1, c0, 2	@ CPACR
 	mrc	p15, 0, r4, c2, c0, 2	@ TTBCR
@@ -331,6 +468,17 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  * Assumes vcpu pointer in vcpu reg
  */
 .macro write_coproc_state read_from_vcpu
+	.if \read_from_vcpu == 1
+	add	r10, vcpu, #VCPU_CP14
+	.else
+	add	r10, vcpu, #VCPU_HOST_CONTEXT
+	ldr	r10, [r10]
+	add	r10, r10, #VCPU_CP14_HOST
+	.endif
+
+	/* Assumes r10 pointer in cp14 regs  */
+	bl __restore_debug_state
+
 	.if \read_from_vcpu == 0
 	pop	{r2,r4-r7}
 	.else
@@ -399,14 +547,6 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	mcr	p15, 0, r10, c10, c2, 0	@ PRRR
 	mcr	p15, 0, r11, c10, c2, 1	@ NMRR
 	mcr	p15, 2, r12, c0, c0, 0	@ CSSELR
-
-	.if \read_from_vcpu == 0
-	pop	{r2}
-	.else
-	mov	r2, #0
-	.endif
-
-	mcr	p14, 0, r2, c0, c2, 2	@ DBGDSCR
 .endm
 
 /*
@@ -657,3 +797,11 @@ ARM_BE8(rev	r6, r6  )
 .macro load_vcpu
 	mrc	p15, 4, vcpu, c13, c0, 2	@ HTPIDR
 .endm
+
+__save_debug_state:
+	save_debug_state
+	bx	lr
+
+__restore_debug_state:
+	restore_debug_state
+	bx	lr
-- 
1.7.12.4

[PATCH v4 11/15] KVM: arm: add a function to keep track of host use of the debug registers

[Zhichao Huang]

As we're about to implement a lazy world switch for debug registers,
we add a function reading the break/watch control variables directly to
indicate whether the host has enabled any break/watch points or not.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/include/asm/hw_breakpoint.h |  5 +++++
 arch/arm/kernel/hw_breakpoint.c      | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
index f2f4c61..6f375c5 100644
--- a/arch/arm/include/asm/hw_breakpoint.h
+++ b/arch/arm/include/asm/hw_breakpoint.h
@@ -66,9 +66,14 @@ int arch_install_hw_breakpoint(struct perf_event *bp);
 void arch_uninstall_hw_breakpoint(struct perf_event *bp);
 void hw_breakpoint_pmu_read(struct perf_event *bp);
 int hw_breakpoint_slots(int type);
+bool hw_breakpoint_enabled(void);
 
 #else
 static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk) {}
+static inline bool hw_breakpoint_enabled(void)
+{
+	return false;
+}
 
 #endif	/* CONFIG_HAVE_HW_BREAKPOINT */
 #endif  /* __ASSEMBLY */
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index dc7d0a9..f56788f 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -227,6 +227,27 @@ static int get_num_brps(void)
 	return core_has_mismatch_brps() ? brps - 1 : brps;
 }
 
+/* Indicate whether the host has enabled any break/watch points or not. */
+bool hw_breakpoint_enabled(void)
+{
+	struct perf_event **slots;
+	int i;
+
+	slots = this_cpu_ptr(bp_on_reg);
+	for (i = 0; i < core_num_brps; i++) {
+		if (slots[i])
+			return true;
+	}
+
+	slots = this_cpu_ptr(wp_on_reg);
+	for (i = 0; i < core_num_wrps; i++) {
+		if (slots[i])
+			return true;
+	}
+
+	return false;
+}
+
 /*
  * In order to access the breakpoint/watchpoint control registers,
  * we must be running in debug monitor mode. Unfortunately, we can
-- 
1.7.12.4

[PATCH v4 12/15] KVM: arm: keep track of host use of the debug registers

[Zhichao Huang]

Every guest entry, we need to keep track of host use of the debug
registers.

We only call the function upon guest entry, after preempt_disable()
and local_irq_disable(), so there is no race for it.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/include/asm/kvm_asm.h    | 3 +++
 arch/arm/include/asm/kvm_host.h   | 4 ++++
 arch/arm/kernel/asm-offsets.c     | 1 +
 arch/arm/kvm/arm.c                | 2 ++
 arch/arm/kvm/coproc.c             | 8 ++++++++
 arch/arm64/include/asm/kvm_host.h | 1 +
 6 files changed, 19 insertions(+)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 7443a3a..5b1c3eb 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -64,6 +64,9 @@
 #define cp14_DBGDSCRext	65	/* Debug Status and Control external */
 #define NR_CP14_REGS	66	/* Number of regs (incl. invalid) */
 
+#define KVM_ARM_DEBUG_HOST_INUSE_SHIFT	0
+#define KVM_ARM_DEBUG_HOST_INUSE	(1 << KVM_ARM_DEBUG_HOST_INUSE_SHIFT)
+
 #define ARM_EXCEPTION_RESET	  0
 #define ARM_EXCEPTION_UNDEFINED   1
 #define ARM_EXCEPTION_SOFTWARE    2
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index fc461ac..7338f34 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -131,6 +131,9 @@ struct kvm_vcpu_arch {
 	/* System control coprocessor (cp14) */
 	u32 cp14[NR_CP14_REGS];
 
+	/* Debug State */
+	u32 debug_flags;
+
 	/*
 	 * Anything that is not used directly from assembly code goes
 	 * here.
@@ -237,5 +240,6 @@ static inline void kvm_arch_hardware_unsetup(void) {}
 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_setup_debug(struct kvm_vcpu *vcpu);
 
 #endif /* __ARM_KVM_HOST_H__ */
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index cee4254..7750597 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -178,6 +178,7 @@ int main(void)
   DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
   DEFINE(VCPU_VFP_HOST,		offsetof(struct kvm_cpu_context, vfp));
   DEFINE(VCPU_CP14_HOST,	offsetof(struct kvm_cpu_context, cp14));
+  DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
   DEFINE(VCPU_REGS,		offsetof(struct kvm_vcpu, arch.regs));
   DEFINE(VCPU_USR_REGS,		offsetof(struct kvm_vcpu, arch.regs.usr_regs));
   DEFINE(VCPU_SVC_REGS,		offsetof(struct kvm_vcpu, arch.regs.svc_regs));
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index bc738d2..0129346 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -550,6 +550,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			continue;
 		}
 
+		kvm_arm_setup_debug(vcpu);
+
 		/**************************************************************
 		 * Enter the guest
 		 */
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index d15b250..b37afd6 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -1516,3 +1516,11 @@ void kvm_reset_coprocs(struct kvm_vcpu *vcpu)
 		if (vcpu->arch.cp15[num] == 0x42424242)
 			panic("Didn't reset vcpu->arch.cp15[%zi]", num);
 }
+
+void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
+{
+	if (hw_breakpoint_enabled())
+		vcpu->arch.debug_flags |= KVM_ARM_DEBUG_HOST_INUSE;
+	else
+		vcpu->arch.debug_flags &= ~KVM_ARM_DEBUG_HOST_INUSE;
+}
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2709db2..84fa4f0 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -226,5 +226,6 @@ static inline void kvm_arch_hardware_unsetup(void) {}
 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_setup_debug(struct kvm_vcpu *vcpu) {}
 
 #endif /* __ARM64_KVM_HOST_H__ */
-- 
1.7.12.4

[PATCH v4 13/15] KVM: arm: keep track of guest use of the debug registers

[Zhichao Huang]

We trap debug register accesses from guest all the time, and read the
BCR/WCR to indicate whether the guest has enabled any break/watch points
or not.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/include/asm/kvm_asm.h |  2 ++
 arch/arm/kvm/coproc.c          | 75 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 5b1c3eb..e9e1f0a 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h
@@ -65,7 +65,9 @@
 #define NR_CP14_REGS	66	/* Number of regs (incl. invalid) */
 
 #define KVM_ARM_DEBUG_HOST_INUSE_SHIFT	0
+#define KVM_ARM_DEBUG_GUEST_INUSE_SHIFT	1
 #define KVM_ARM_DEBUG_HOST_INUSE	(1 << KVM_ARM_DEBUG_HOST_INUSE_SHIFT)
+#define KVM_ARM_DEBUG_GUEST_INUSE	(1 << KVM_ARM_DEBUG_GUEST_INUSE_SHIFT)
 
 #define ARM_EXCEPTION_RESET	  0
 #define ARM_EXCEPTION_UNDEFINED   1
diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index b37afd6..d9dcd28b 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -220,7 +220,22 @@ bool access_vm_reg(struct kvm_vcpu *vcpu,
 	return true;
 }
 
-static bool trap_debug32(struct kvm_vcpu *vcpu,
+/* Indicate whether the guest has enabled any break/watch points or not. */
+static bool guest_debug_in_use(struct kvm_vcpu *vcpu)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARM_MAX_BRP; i++)
+		if (vcpu->arch.cp14[cp14_DBGBCR0 + i] & 0x1)
+			return true;
+	for (i = 0; i < ARM_MAX_WRP; i++)
+		if (vcpu->arch.cp14[cp14_DBGWCR0 + i] & 0x1)
+			return true;
+
+	return false;
+}
+
+static bool __trap_debug32(struct kvm_vcpu *vcpu,
 			const struct coproc_params *p,
 			const struct coproc_reg *r)
 {
@@ -232,6 +247,56 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+/*
+ * We want to avoid world-switching all the DBG registers all the
+ * time:
+ *
+ * When we are about to run a guest, we have the following cases:
+ *
+ * 1) Neither the host nor the guest has configured any [WB]points
+ * 2) Only the host has configured any [WB]points
+ * 3) Only the guest has configured any [WB]points
+ * 4) Both the host and the guest have configured any [WB]points
+ *
+ * - In case (1), KVM should enable trapping and swtich the register
+ *   state on guest accesses.
+ * - In cases (2), (3), and (4) we must switch the register state on each
+ *   entry/exit.
+ *
+ * For ARMv7, if the CONFIG_HAVE_HW_BREAKPOINT is set, ARM_DSCR_MDBGEN
+ * is always set(ARM64 use it to indicate that debug registers are actively
+ * in use).
+ *
+ * - We add a function reading the break/watch control variables directly to
+ *   indicate whether the host has enabled any break/watch points or not.
+ *   We only call the function upon guest entry, after preempt_disable() and
+ *   local_irq_disable(), so there is no race for it.
+ *
+ * - We trap debug register accesses from guest all the time, and read the
+ *   BCR/WCR to indicate whether the guest has enabled any break/watch points
+ *   or not.
+ *
+ * For this, we can keep track of the host/guest use of debug registers,
+ * and skip the save/restore dance when neither the host nor the guest has
+ * configured any [WB]points.
+ */
+static bool trap_debug32(struct kvm_vcpu *vcpu,
+			const struct coproc_params *p,
+			const struct coproc_reg *r)
+{
+	__trap_debug32(vcpu, p, r);
+
+	if (p->is_write) {
+		if ((vcpu->arch.cp14[r->reg] & 0x1) ||
+		    guest_debug_in_use(vcpu))
+			vcpu->arch.debug_flags |= KVM_ARM_DEBUG_GUEST_INUSE;
+		else
+			vcpu->arch.debug_flags &= ~KVM_ARM_DEBUG_GUEST_INUSE;
+	}
+
+	return true;
+}
+
 /* DBGIDR (RO) Debug ID */
 static bool trap_dbgidr(struct kvm_vcpu *vcpu,
 			const struct coproc_params *p,
@@ -419,13 +484,13 @@ static const struct coproc_reg cp15_regs[] = {
 #define DBG_BCR_BVR_WCR_WVR(n)					\
 	/* DBGBVRn */						\
 	{ CRn( 0), CRm((n)), Op1( 0), Op2( 4), is32,		\
-	  trap_debug32,	reset_val, (cp14_DBGBVR0 + (n)), 0 },	\
+	  __trap_debug32, reset_val, (cp14_DBGBVR0 + (n)), 0 },	\
 	/* DBGBCRn */						\
 	{ CRn( 0), CRm((n)), Op1( 0), Op2( 5), is32,		\
 	  trap_debug32,	reset_val, (cp14_DBGBCR0 + (n)), 0 },	\
 	/* DBGWVRn */						\
 	{ CRn( 0), CRm((n)), Op1( 0), Op2( 6), is32,		\
-	  trap_debug32,	reset_val, (cp14_DBGWVR0 + (n)), 0 },	\
+	  __trap_debug32, reset_val, (cp14_DBGWVR0 + (n)), 0 },	\
 	/* DBGWCRn */						\
 	{ CRn( 0), CRm((n)), Op1( 0), Op2( 7), is32,		\
 	  trap_debug32,	reset_val, (cp14_DBGWCR0 + (n)), 0 }
@@ -462,7 +527,7 @@ static const struct coproc_reg cp14_regs[] = {
 	/* DBGDSAR (64bit) */
 	{ CRn( 0), CRm( 2), Op1( 0), Op2( 0), is64, trap_raz_wi },
 	/* DBGDSCRext */
-	{ CRn( 0), CRm( 2), Op1( 0), Op2( 2), is32, trap_debug32,
+	{ CRn( 0), CRm( 2), Op1( 0), Op2( 2), is32, __trap_debug32,
 				reset_val, cp14_DBGDSCRext, 0 },
 	DBG_BCR_BVR_WCR_WVR(2),
 	/* DBGDTRTXext */
@@ -474,7 +539,7 @@ static const struct coproc_reg cp14_regs[] = {
 	DBG_BCR_BVR_WCR_WVR(5),
 	DBG_BCR_BVR_WCR_WVR(6),
 	/* DBGVCR */
-	{ CRn( 0), CRm( 7), Op1( 0), Op2( 0), is32, trap_debug32 },
+	{ CRn( 0), CRm( 7), Op1( 0), Op2( 0), is32, __trap_debug32 },
 	DBG_BCR_BVR_WCR_WVR(7),
 	DBG_BCR_BVR_WCR_WVR(8),
 	DBG_BCR_BVR_WCR_WVR(9),
-- 
1.7.12.4

[PATCH v4 14/15] KVM: arm: implement lazy world switch for debug registers

[Zhichao Huang]

Avoid world-switching all the debug registers when neither the host
nor the guest has configured any [WB]points.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
---
 arch/arm/kvm/interrupts_head.S | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index b9e7410..7ad0adf 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -332,6 +332,21 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	sub	r12, r2, r12		@ How many WPs to skip
 .endm
 
+/* If VCPU_DEBUG_FLAGS is not set, it means that neither the host
+ * nor the guest has configured any [WB]points.
+ *
+ * We avoid world-switching all the debug registers in that case.
+ *
+ * Assume vcpu pointer in vcpu reg
+ *
+ * clobbers r2
+ */
+.macro skip_debug_state target
+	ldr	r2, [vcpu, #VCPU_DEBUG_FLAGS]
+	cmp	r2, #0
+	beq	\target
+.endm
+
 /* Reads cp14 registers from hardware.
  * Writes cp14 registers in-order to the CP14 struct pointed to by r10
  *
@@ -340,12 +355,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  * Clobbers r2-r12
  */
 .macro save_debug_state
+	skip_debug_state 9999f
+
 	read_hw_dbg_num
 	cp14_read_and_str r10, 4, cp14_DBGBVR0, r11
 	cp14_read_and_str r10, 5, cp14_DBGBCR0, r11
 	cp14_read_and_str r10, 6, cp14_DBGWVR0, r12
 	cp14_read_and_str r10, 7, cp14_DBGWCR0, r12
-
+9999:
 	/* DBGDSCR reg */
 	mrc	p14, 0, r2, c0, c1, 0
 	str	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
@@ -359,12 +376,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
  * Clobbers r2-r12
  */
 .macro restore_debug_state
+	skip_debug_state 9999f
+
 	read_hw_dbg_num
 	cp14_ldr_and_write r10, 4, cp14_DBGBVR0, r11
 	cp14_ldr_and_write r10, 5, cp14_DBGBCR0, r11
 	cp14_ldr_and_write r10, 6, cp14_DBGWVR0, r12
 	cp14_ldr_and_write r10, 7, cp14_DBGWCR0, r12
-
+9999:
 	/* DBGDSCR reg */
 	ldr	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
 	mcr	p14, 0, r2, c0, c2, 2
-- 
1.7.12.4

[PATCH v4 15/15] KVM: arm: enable trapping of all debug registers

[Zhichao Huang]

Enable trapping of the debug registers unconditionally, allowing guests to
use the debug infrastructure.

Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/interrupts_head.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 7ad0adf..494991d 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -792,7 +792,7 @@ ARM_BE8(rev	r6, r6  )
  * (hardware reset value is 0) */
 .macro set_hdcr operation
 	mrc	p15, 4, r2, c1, c1, 1
-	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
+	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
 	.if \operation == vmentry
 	orr	r2, r2, r3		@ Trap some perfmon accesses
 	.else
-- 
1.7.12.4

[PATCH v4 01/15] KVM: arm: plug guest debug exploit

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:25:53PM +0800, Zhichao Huang wrote:
> Hardware debugging in guests is not intercepted currently, it means
> that a malicious guest can bring down the entire machine by writing
> to the debug registers.
> 
> This patch enable trapping of all debug registers, preventing the guests
> to access the debug registers.
> 
> This patch also disable the debug mode(DBGDSCR) in the guest world all
> the time, preventing the guests to mess with the host state.

Remind me how this works:  How does disabling debug mode prevent the
guest from messing with the host state?  I would think that disabling
debug mode makes sure that exceptions from breakpoints or watchpoints
programmed by the host are ignored?

> 
> However, it is a precursor for later patches which will need to do
> more to world switch debug states while necessary.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/include/asm/kvm_coproc.h |  3 +-
>  arch/arm/kvm/coproc.c             | 82 +++++++++++++++++++++++++++++----------
>  arch/arm/kvm/handle_exit.c        |  4 +-
>  arch/arm/kvm/interrupts.S         | 12 +++---
>  arch/arm/kvm/interrupts_head.S    | 21 ++++++++--
>  5 files changed, 89 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_coproc.h b/arch/arm/include/asm/kvm_coproc.h
> index 4917c2f..e74ab0f 100644
> --- a/arch/arm/include/asm/kvm_coproc.h
> +++ b/arch/arm/include/asm/kvm_coproc.h
> @@ -31,7 +31,8 @@ void kvm_register_target_coproc_table(struct kvm_coproc_target_table *table);
>  int kvm_handle_cp10_id(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp_0_13_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run);
> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
>  
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index f3d88dc..a885cfe 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -91,12 +91,6 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> -int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run)
> -{
> -	kvm_inject_undefined(vcpu);
> -	return 1;
> -}
> -
>  static void reset_mpidr(struct kvm_vcpu *vcpu, const struct coproc_reg *r)
>  {
>  	/*
> @@ -465,12 +459,8 @@ static int emulate_cp15(struct kvm_vcpu *vcpu,
>  	return 1;
>  }
>  
> -/**
> - * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15 access
> - * @vcpu: The VCPU pointer
> - * @run:  The kvm_run struct
> - */
> -int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, struct kvm_run *run,
> +			    bool cp15)

perhaps have cp as an int argument instead and do a switch below?

>  {
>  	struct coproc_params params;
>  
> @@ -484,7 +474,35 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
>  	params.CRm = 0;
>  
> -	return emulate_cp15(vcpu, &params);
> +	if (cp15)
> +		return emulate_cp15(vcpu, &params);
> +
> +	/* raz_wi cp14 */
> +	(void)pm_fake(vcpu, &params, NULL);

I don't think you need the (void) ?

> +
> +	/* handled */
> +	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +	return 1;

perhaps this would be easier to read by adding an emulate_cp14 function
which basically looks like this:

static int emulate_cp14(struct kvm_vcpu *vcpu,
			const struct coproc_params *params)
{
	pm_fake(vcpu, &params, NULL);

	/* handled */
	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
	return 1;
}
	

> +}
> +
> +/**
> + * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15 access
> + * @vcpu: The VCPU pointer
> + * @run:  The kvm_run struct
> + */
> +int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	return kvm_handle_cp_64(vcpu, run, 1);
> +}
> +
> +/**
> + * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14 access
> + * @vcpu: The VCPU pointer
> + * @run:  The kvm_run struct
> + */
> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	return kvm_handle_cp_64(vcpu, run, 0);
>  }
>  
>  static void reset_coproc_regs(struct kvm_vcpu *vcpu,
> @@ -497,12 +515,8 @@ static void reset_coproc_regs(struct kvm_vcpu *vcpu,
>  			table[i].reset(vcpu, &table[i]);
>  }
>  
> -/**
> - * kvm_handle_cp15_32 -- handles a mrc/mcr trap on a guest CP15 access
> - * @vcpu: The VCPU pointer
> - * @run:  The kvm_run struct
> - */
> -int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, struct kvm_run *run,
> +			      bool cp15)
>  {
>  	struct coproc_params params;
>  
> @@ -516,7 +530,35 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
>  	params.Rt2 = 0;
>  
> -	return emulate_cp15(vcpu, &params);
> +	if (cp15)
> +		return emulate_cp15(vcpu, &params);
> +
> +	/* raz_wi cp14 */
> +	(void)pm_fake(vcpu, &params, NULL);
> +
> +	/* handled */
> +	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +	return 1;

same comment as above

> +}
> +
> +/**
> + * kvm_handle_cp15_32 -- handles a mrc/mcr trap on a guest CP15 access
> + * @vcpu: The VCPU pointer
> + * @run:  The kvm_run struct
> + */
> +int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	return kvm_handle_cp_32(vcpu, run, 1);
> +}
> +
> +/**
> + * kvm_handle_cp14_32 -- handles a mrc/mcr trap on a guest CP14 access
> + * @vcpu: The VCPU pointer
> + * @run:  The kvm_run struct
> + */
> +int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	return kvm_handle_cp_32(vcpu, run, 0);
>  }
>  
>  /******************************************************************************
> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
> index 95f12b2..357ad1b 100644
> --- a/arch/arm/kvm/handle_exit.c
> +++ b/arch/arm/kvm/handle_exit.c
> @@ -104,9 +104,9 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[HSR_EC_WFI]		= kvm_handle_wfx,
>  	[HSR_EC_CP15_32]	= kvm_handle_cp15_32,
>  	[HSR_EC_CP15_64]	= kvm_handle_cp15_64,
> -	[HSR_EC_CP14_MR]	= kvm_handle_cp14_access,
> +	[HSR_EC_CP14_MR]	= kvm_handle_cp14_32,
>  	[HSR_EC_CP14_LS]	= kvm_handle_cp14_load_store,
> -	[HSR_EC_CP14_64]	= kvm_handle_cp14_access,
> +	[HSR_EC_CP14_64]	= kvm_handle_cp14_64,
>  	[HSR_EC_CP_0_13]	= kvm_handle_cp_0_13_access,
>  	[HSR_EC_CP10_ID]	= kvm_handle_cp10_id,
>  	[HSR_EC_SVC_HYP]	= handle_svc_hyp,
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index 568494d..48333ff 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -112,9 +112,9 @@ ENTRY(__kvm_vcpu_run)
>  	restore_vgic_state
>  	restore_timer_state
>  
> -	@ Store hardware CP15 state and load guest state
> -	read_cp15_state store_to_vcpu = 0
> -	write_cp15_state read_from_vcpu = 1
> +	@ Store hardware CP14/CP15 state and load guest state
> +	read_coproc_state store_to_vcpu = 0
> +	write_coproc_state read_from_vcpu = 1
>  
>  	@ If the host kernel has not been configured with VFPv3 support,
>  	@ then it is safer if we deny guests from using it as well.
> @@ -199,9 +199,9 @@ after_vfp_restore:
>  	mrc	p15, 0, r2, c0, c0, 5
>  	mcr	p15, 4, r2, c0, c0, 5
>  
> -	@ Store guest CP15 state and restore host state
> -	read_cp15_state store_to_vcpu = 1
> -	write_cp15_state read_from_vcpu = 0
> +	@ Store guest CP14/CP15 state and restore host state
> +	read_coproc_state store_to_vcpu = 1
> +	write_coproc_state read_from_vcpu = 0
>  
>  	save_timer_state
>  	save_vgic_state
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 702740d..7c4075c 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -239,7 +239,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	save_guest_regs_mode irq, #VCPU_IRQ_REGS
>  .endm
>  
> -/* Reads cp15 registers from hardware and stores them in memory
> +/* Reads cp14/cp15 registers from hardware and stores them in memory
>   * @store_to_vcpu: If 0, registers are written in-order to the stack,
>   * 		   otherwise to the VCPU struct pointed to by vcpup
>   *
> @@ -247,7 +247,12 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   *
>   * Clobbers r2 - r12
>   */
> -.macro read_cp15_state store_to_vcpu
> +.macro read_coproc_state store_to_vcpu
> +	.if \store_to_vcpu == 0
> +	mrc	p14, 0, r2, c0, c1, 0	@ DBGDSCR
> +	push	{r2}
> +	.endif
> +
>  	mrc	p15, 0, r2, c1, c0, 0	@ SCTLR
>  	mrc	p15, 0, r3, c1, c0, 2	@ CPACR
>  	mrc	p15, 0, r4, c2, c0, 2	@ TTBCR
> @@ -325,7 +330,7 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   *
>   * Assumes vcpu pointer in vcpu reg
>   */
> -.macro write_cp15_state read_from_vcpu
> +.macro write_coproc_state read_from_vcpu
>  	.if \read_from_vcpu == 0
>  	pop	{r2,r4-r7}
>  	.else
> @@ -394,6 +399,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	mcr	p15, 0, r10, c10, c2, 0	@ PRRR
>  	mcr	p15, 0, r11, c10, c2, 1	@ NMRR
>  	mcr	p15, 2, r12, c0, c0, 0	@ CSSELR
> +
> +	.if \read_from_vcpu == 0
> +	pop	{r2}
> +	.else
> +	mov	r2, #0

I really think that we should read the register, clear the bits you care
about (MDBGen and HDBGen) and then write back the register.

So, if I recall correctly, this is to avoid having to set HDCR_TDE
below?

Given Will's concerns about touching this register, I'm thinking if we
shouldn't start with the HDCR_TDE enabled (and a handler in KVM) and
then see if we want to add this optimization later?

At the very least, you should do as Will pointed out and predicate
writes to this register based on whether the reset code in
hw_breakpoint.c successfully reset the debug regs.  I think checking the
debug_err_mask variable from the C code and pass this on to the Hyp code
would be the right way to go.

But as I said, I think we should just trap debug exceptions to begin
with (to plug the hole) and then add the more intelligent stuff later.

> +	.endif
> +
> +	mcr	p14, 0, r2, c0, c2, 2	@ DBGDSCR
>  .endm
>  
>  /*
> @@ -620,7 +633,7 @@ ARM_BE8(rev	r6, r6  )
>   * (hardware reset value is 0) */
>  .macro set_hdcr operation
>  	mrc	p15, 4, r2, c1, c1, 1
> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)



>  	.if \operation == vmentry
>  	orr	r2, r2, r3		@ Trap some perfmon accesses
>  	.else
> -- 
> 1.7.12.4
> 

Thanks,
-Christoffer

[PATCH v4 04/15] KVM: arm: common infrastructure for handling AArch32 CP14/CP15

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:25:56PM +0800, Zhichao Huang wrote:
> As we're about to trap a bunch of CP14 registers, let's rework
> the CP15 handling so it can be generalized and work with multiple
> tables.
> 
> We stop trapping access here, because we haven't finished our trap
> handlers. We will enable trapping agian until everything is OK.
> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/kvm/coproc.c          | 168 ++++++++++++++++++++++++++---------------
>  arch/arm/kvm/interrupts_head.S |   2 +-
>  2 files changed, 110 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 4db571d..d23395b 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -375,6 +375,9 @@ static const struct coproc_reg cp15_regs[] = {
>  	{ CRn(15), CRm( 0), Op1( 4), Op2( 0), is32, access_cbar},
>  };
>  
> +static const struct coproc_reg cp14_regs[] = {
> +};
> +
>  /* Target specific emulation tables */
>  static struct kvm_coproc_target_table *target_tables[KVM_ARM_NUM_TARGETS];
>  
> @@ -424,43 +427,75 @@ static const struct coproc_reg *find_reg(const struct coproc_params *params,
>  	return NULL;
>  }
>  
> -static int emulate_cp15(struct kvm_vcpu *vcpu,
> -			const struct coproc_params *params)
> +/*
> + * emulate_cp --  tries to match a cp14/cp15 access in a handling table,
> + *                and call the corresponding trap handler.
> + *
> + * @params: pointer to the descriptor of the access
> + * @table: array of trap descriptors
> + * @num: size of the trap descriptor array
> + *
> + * Return 0 if the access has been handled, and -1 if not.
> + */
> +static int emulate_cp(struct kvm_vcpu *vcpu,
> +			const struct coproc_params *params,
> +			const struct coproc_reg *table,
> +			size_t num)
>  {
> -	size_t num;
> -	const struct coproc_reg *table, *r;
> -
> -	trace_kvm_emulate_cp15_imp(params->Op1, params->Rt1, params->CRn,
> -				   params->CRm, params->Op2, params->is_write);
> +	const struct coproc_reg *r;
>  
> -	table = get_target_table(vcpu->arch.target, &num);
> +	if (!table)
> +		return -1;	/* Not handled */
>  
> -	/* Search target-specific then generic table. */
>  	r = find_reg(params, table, num);
> -	if (!r)
> -		r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs));
>  
> -	if (likely(r)) {
> +	if (r) {
>  		/* If we don't have an accessor, we should never get here! */
>  		BUG_ON(!r->access);
>  
>  		if (likely(r->access(vcpu, params, r))) {
>  			/* Skip instruction, since it was emulated */
>  			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> -			return 1;
>  		}
> -		/* If access function fails, it should complain. */
> -	} else {
> -		kvm_err("Unsupported guest CP15 access at: %08lx\n",
> -			*vcpu_pc(vcpu));
> -		print_cp_instr(params);
> +
> +		/* Handled */
> +		return 0;
>  	}
> +
> +	/* Not handled */
> +	return -1;
> +}
> +
> +static void unhandled_cp_access(struct kvm_vcpu *vcpu,
> +				const struct coproc_params *params)
> +{
> +	u8 hsr_ec = kvm_vcpu_trap_get_class(vcpu);
> +	int cp;
> +
> +	switch (hsr_ec) {
> +	case HSR_EC_CP15_32:
> +	case HSR_EC_CP15_64:
> +		cp = 15;
> +		break;
> +	case HSR_EC_CP14_MR:
> +	case HSR_EC_CP14_64:
> +		cp = 14;
> +		break;
> +	default:
> +		WARN_ON((cp = -1));
> +	}
> +
> +	kvm_err("Unsupported guest CP%d access at: %08lx\n",
> +		cp, *vcpu_pc(vcpu));
> +	print_cp_instr(params);
>  	kvm_inject_undefined(vcpu);
> -	return 1;
>  }
>  
> -static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, struct kvm_run *run,
> -			    bool cp15)
> +int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
> +			const struct coproc_reg *global,
> +			size_t nr_global,
> +			const struct coproc_reg *target_specific,
> +			size_t nr_specific)
>  {
>  	struct coproc_params params;
>  
> @@ -474,37 +509,15 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  	params.Rt2 = (kvm_vcpu_get_hsr(vcpu) >> 10) & 0xf;
>  	params.CRm = 0;
>  
> -	if (cp15)
> -		return emulate_cp15(vcpu, &params);
> +	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
> +		return 1;
> +	if (!emulate_cp(vcpu, &params, global, nr_global))
> +		return 1;
>  
> -	/* raz_wi cp14 */
> -	(void)trap_raz_wi(vcpu, &params, NULL);
> -
> -	/* handled */
> -	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +	unhandled_cp_access(vcpu, &params);
>  	return 1;
>  }
>  
> -/**
> - * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15 access
> - * @vcpu: The VCPU pointer
> - * @run:  The kvm_run struct
> - */
> -int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> -{
> -	return kvm_handle_cp_64(vcpu, run, 1);
> -}
> -
> -/**
> - * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14 access
> - * @vcpu: The VCPU pointer
> - * @run:  The kvm_run struct
> - */
> -int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> -{
> -	return kvm_handle_cp_64(vcpu, run, 0);
> -}
> -
>  static void reset_coproc_regs(struct kvm_vcpu *vcpu,
>  			      const struct coproc_reg *table, size_t num)
>  {
> @@ -515,8 +528,11 @@ static void reset_coproc_regs(struct kvm_vcpu *vcpu,
>  			table[i].reset(vcpu, &table[i]);
>  }
>  
> -static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, struct kvm_run *run,
> -			      bool cp15)
> +int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> +			const struct coproc_reg *global,
> +			size_t nr_global,
> +			const struct coproc_reg *target_specific,
> +			size_t nr_specific)
>  {
>  	struct coproc_params params;
>  
> @@ -530,25 +546,57 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  	params.Op2 = (kvm_vcpu_get_hsr(vcpu) >> 17) & 0x7;
>  	params.Rt2 = 0;
>  
> -	if (cp15)
> -		return emulate_cp15(vcpu, &params);
> -
> -	/* raz_wi cp14 */
> -	(void)trap_raz_wi(vcpu, &params, NULL);
> +	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
> +		return 1;
> +	if (!emulate_cp(vcpu, &params, global, nr_global))
> +		return 1;
>  
> -	/* handled */
> -	kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +	unhandled_cp_access(vcpu, &params);
>  	return 1;
>  }
>  
>  /**
> + * kvm_handle_cp15_64 -- handles a mrrc/mcrr trap on a guest CP15 access
> + * @vcpu: The VCPU pointer
> + * @run:  The kvm_run struct
> + */
> +int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	const struct coproc_reg *target_specific;
> +	size_t num;
> +
> +	target_specific = get_target_table(vcpu->arch.target, &num);
> +	return kvm_handle_cp_64(vcpu,
> +				cp15_regs, ARRAY_SIZE(cp15_regs),
> +				target_specific, num);
> +}
> +
> +/**
>   * kvm_handle_cp15_32 -- handles a mrc/mcr trap on a guest CP15 access
>   * @vcpu: The VCPU pointer
>   * @run:  The kvm_run struct
>   */
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
> -	return kvm_handle_cp_32(vcpu, run, 1);
> +	const struct coproc_reg *target_specific;
> +	size_t num;
> +
> +	target_specific = get_target_table(vcpu->arch.target, &num);
> +	return kvm_handle_cp_32(vcpu,
> +				cp15_regs, ARRAY_SIZE(cp15_regs),
> +				target_specific, num);
> +}
> +
> +/**
> + * kvm_handle_cp14_64 -- handles a mrrc/mcrr trap on a guest CP14 access
> + * @vcpu: The VCPU pointer
> + * @run:  The kvm_run struct
> + */
> +int kvm_handle_cp14_64(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	return kvm_handle_cp_64(vcpu,
> +				cp14_regs, ARRAY_SIZE(cp14_regs),
> +				NULL, 0);
>  }
>  
>  /**
> @@ -558,7 +606,9 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>   */
>  int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
> -	return kvm_handle_cp_32(vcpu, run, 0);
> +	return kvm_handle_cp_32(vcpu,
> +				cp14_regs, ARRAY_SIZE(cp14_regs),
> +				NULL, 0);
>  }
>  
>  /******************************************************************************
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 7c4075c..7ac5e51 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -633,7 +633,7 @@ ARM_BE8(rev	r6, r6  )
>   * (hardware reset value is 0) */
>  .macro set_hdcr operation
>  	mrc	p15, 4, r2, c1, c1, 1
> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)

I know I said ok to this, but I don't like the ordering of this with
these patches.

I think we should only stop trapping debug accesses when we have proper
world-switching in place.

You should be able to put all the infrastructure needed in place first,
and then hook things up in the end.

Thanks,
-Christoffer

>  	.if \operation == vmentry
>  	orr	r2, r2, r3		@ Trap some perfmon accesses
>  	.else
> -- 
> 1.7.12.4
> 

[PATCH v4 10/15] KVM: arm: implement world switch for debug registers

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:26:02PM +0800, Zhichao Huang wrote:
> Implement switching of the debug registers. While the number
> of registers is massive, CPUs usually don't implement them all
> (A15 has 6 breakpoints and 4 watchpoints, which gives us a total
> of 22 registers "only").
> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/kvm/interrupts_head.S | 170 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 159 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 7ac5e51..b9e7410 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -5,6 +5,7 @@
>  #define VCPU_USR_SP		(VCPU_USR_REG(13))
>  #define VCPU_USR_LR		(VCPU_USR_REG(14))
>  #define CP15_OFFSET(_cp15_reg_idx) (VCPU_CP15 + (_cp15_reg_idx * 4))
> +#define CP14_OFFSET(_cp14_reg_idx) ((_cp14_reg_idx) * 4)
>  
>  /*
>   * Many of these macros need to access the VCPU structure, which is always
> @@ -239,6 +240,136 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	save_guest_regs_mode irq, #VCPU_IRQ_REGS
>  .endm
>  
> +/* Assume r10/r11/r12 are in use, clobbers r2-r3 */
> +.macro cp14_read_and_str base Op2 cp14_reg0 skip_num
> +	adr	r3, 1f
> +	add	r3, r3, \skip_num, lsl #3

can this code be compiled in Thumb-2 ?  If so, are all the instructions
below 32-bit wide?

> +	bx	r3
> +1:
> +	mrc	p14, 0, r2, c0, c15, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+15)]
> +	mrc	p14, 0, r2, c0, c14, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+14)]
> +	mrc	p14, 0, r2, c0, c13, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+13)]
> +	mrc	p14, 0, r2, c0, c12, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+12)]
> +	mrc	p14, 0, r2, c0, c11, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+11)]
> +	mrc	p14, 0, r2, c0, c10, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+10)]
> +	mrc	p14, 0, r2, c0, c9, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+9)]
> +	mrc	p14, 0, r2, c0, c8, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+8)]
> +	mrc	p14, 0, r2, c0, c7, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+7)]
> +	mrc	p14, 0, r2, c0, c6, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+6)]
> +	mrc	p14, 0, r2, c0, c5, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+5)]
> +	mrc	p14, 0, r2, c0, c4, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+4)]
> +	mrc	p14, 0, r2, c0, c3, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+3)]
> +	mrc	p14, 0, r2, c0, c2, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+2)]
> +	mrc	p14, 0, r2, c0, c1, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0+1)]
> +	mrc	p14, 0, r2, c0, c0, \Op2
> +	str     r2, [\base, #CP14_OFFSET(\cp14_reg0)]
> +.endm
> +
> +/* Assume r11/r12 are in use, clobbers r2-r3 */
> +.macro cp14_ldr_and_write base Op2 cp14_reg0 skip_num
> +	adr	r3, 1f
> +	add	r3, r3, \skip_num, lsl #3

see above

> +	bx	r3
> +1:
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+15)]
> +	mcr	p14, 0, r2, c0, c15, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+14)]
> +	mcr	p14, 0, r2, c0, c14, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+13)]
> +	mcr	p14, 0, r2, c0, c13, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+12)]
> +	mcr	p14, 0, r2, c0, c12, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+11)]
> +	mcr	p14, 0, r2, c0, c11, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+10)]
> +	mcr	p14, 0, r2, c0, c10, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+9)]
> +	mcr	p14, 0, r2, c0, c9, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+8)]
> +	mcr	p14, 0, r2, c0, c8, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+7)]
> +	mcr	p14, 0, r2, c0, c7, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+6)]
> +	mcr	p14, 0, r2, c0, c6, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+5)]
> +	mcr	p14, 0, r2, c0, c5, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+4)]
> +	mcr	p14, 0, r2, c0, c4, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+3)]
> +	mcr	p14, 0, r2, c0, c3, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+2)]
> +	mcr	p14, 0, r2, c0, c2, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0+1)]
> +	mcr	p14, 0, r2, c0, c1, \Op2
> +	ldr     r2, [\base, #CP14_OFFSET(\cp14_reg0)]
> +	mcr	p14, 0, r2, c0, c0, \Op2
> +.endm
> +
> +/* Get extract number of BRPs and WRPs. Saved in r11/r12 */
> +.macro read_hw_dbg_num
> +	mrc	p14, 0, r2, c0, c0, 0

can you add @ DBGDIDR here, so we know which register we are looking at?

> +	ubfx	r11, r2, #24, #4
> +	add	r11, r11, #1		@ Extract BRPs
> +	ubfx	r12, r2, #28, #4
> +	add	r12, r12, #1		@ Extract WRPs
> +	mov	r2, #16
> +	sub	r11, r2, r11		@ How many BPs to skip
> +	sub	r12, r2, r12		@ How many WPs to skip
> +.endm
> +
> +/* Reads cp14 registers from hardware.
> + * Writes cp14 registers in-order to the CP14 struct pointed to by r10
> + *
> + * Assumes vcpu pointer in vcpu reg
> + *
> + * Clobbers r2-r12
> + */
> +.macro save_debug_state
> +	read_hw_dbg_num
> +	cp14_read_and_str r10, 4, cp14_DBGBVR0, r11
> +	cp14_read_and_str r10, 5, cp14_DBGBCR0, r11
> +	cp14_read_and_str r10, 6, cp14_DBGWVR0, r12
> +	cp14_read_and_str r10, 7, cp14_DBGWCR0, r12
> +
> +	/* DBGDSCR reg */
> +	mrc	p14, 0, r2, c0, c1, 0
> +	str	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]

so again we're touching the scary register on every world-switch.  Since
it sounds like we have experience telling us that this can cause
troubles, I'm wondering if we can get around it by:

Only ever allow the guest to use debugging registers if we managed to
enter_monitor_mode on the host, and in that case only allow guest
debugging with the configuration of DBGDSCR that the host has.

If the host never managed to enable debugging, the guest probably won't
succeed either, and we should just trap all guest accesses to the debug
registers.

Does this work?

> +.endm
> +
> +/* Reads cp14 registers in-order from the CP14 struct pointed to by r10
> + * Writes cp14 registers to hardware.
> + *
> + * Assumes vcpu pointer in vcpu reg
> + *
> + * Clobbers r2-r12
> + */
> +.macro restore_debug_state
> +	read_hw_dbg_num
> +	cp14_ldr_and_write r10, 4, cp14_DBGBVR0, r11
> +	cp14_ldr_and_write r10, 5, cp14_DBGBCR0, r11
> +	cp14_ldr_and_write r10, 6, cp14_DBGWVR0, r12
> +	cp14_ldr_and_write r10, 7, cp14_DBGWCR0, r12
> +
> +	/* DBGDSCR reg */
> +	ldr	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
> +	mcr	p14, 0, r2, c0, c2, 2

same as above

> +.endm
> +
>  /* Reads cp14/cp15 registers from hardware and stores them in memory
>   * @store_to_vcpu: If 0, registers are written in-order to the stack,
>   * 		   otherwise to the VCPU struct pointed to by vcpup
> @@ -248,11 +379,17 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   * Clobbers r2 - r12
>   */
>  .macro read_coproc_state store_to_vcpu
> -	.if \store_to_vcpu == 0
> -	mrc	p14, 0, r2, c0, c1, 0	@ DBGDSCR
> -	push	{r2}
> +	.if \store_to_vcpu == 1
> +	add	r10, vcpu, #VCPU_CP14
> +	.else
> +	add	r10, vcpu, #VCPU_HOST_CONTEXT
> +	ldr	r10, [r10]
> +	add	r10, r10, #VCPU_CP14_HOST
>  	.endif
>  
> +	/* Assumes r10 pointer in cp14 regs  */
> +	bl __save_debug_state
> +
>  	mrc	p15, 0, r2, c1, c0, 0	@ SCTLR
>  	mrc	p15, 0, r3, c1, c0, 2	@ CPACR
>  	mrc	p15, 0, r4, c2, c0, 2	@ TTBCR
> @@ -331,6 +468,17 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   * Assumes vcpu pointer in vcpu reg
>   */
>  .macro write_coproc_state read_from_vcpu
> +	.if \read_from_vcpu == 1
> +	add	r10, vcpu, #VCPU_CP14
> +	.else
> +	add	r10, vcpu, #VCPU_HOST_CONTEXT
> +	ldr	r10, [r10]
> +	add	r10, r10, #VCPU_CP14_HOST
> +	.endif
> +
> +	/* Assumes r10 pointer in cp14 regs  */
> +	bl __restore_debug_state
> +
>  	.if \read_from_vcpu == 0
>  	pop	{r2,r4-r7}
>  	.else
> @@ -399,14 +547,6 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	mcr	p15, 0, r10, c10, c2, 0	@ PRRR
>  	mcr	p15, 0, r11, c10, c2, 1	@ NMRR
>  	mcr	p15, 2, r12, c0, c0, 0	@ CSSELR
> -
> -	.if \read_from_vcpu == 0
> -	pop	{r2}
> -	.else
> -	mov	r2, #0
> -	.endif
> -
> -	mcr	p14, 0, r2, c0, c2, 2	@ DBGDSCR
>  .endm
>  
>  /*
> @@ -657,3 +797,11 @@ ARM_BE8(rev	r6, r6  )
>  .macro load_vcpu
>  	mrc	p15, 4, vcpu, c13, c0, 2	@ HTPIDR
>  .endm
> +
> +__save_debug_state:
> +	save_debug_state
> +	bx	lr
> +
> +__restore_debug_state:
> +	restore_debug_state
> +	bx	lr
> -- 
> 1.7.12.4
> 

The rest of the register mangling looks ok this time though.

-Christoffer

[PATCH v4 09/15] KVM: arm: redefine kvm_cpu_context_t to save the host cp14 states

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:26:01PM +0800, Zhichao Huang wrote:
> Redefine kvm_cpu_context_t as a new struct that include the cp14 states,
> which we used to save the host cp14 states.
> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/include/asm/kvm_host.h | 6 +++++-
>  arch/arm/kernel/asm-offsets.c   | 4 +++-
>  arch/arm/kvm/interrupts.S       | 6 ++++--
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 63ac005..fc461ac 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -91,7 +91,11 @@ struct kvm_vcpu_fault_info {
>  	u32 hyp_pc;		/* PC when exception was taken from Hyp mode */
>  };
>  
> -typedef struct vfp_hard_struct kvm_cpu_context_t;
> +struct kvm_cpu_context {
> +	struct vfp_hard_struct vfp;
> +	u32 cp14[NR_CP14_REGS];
> +};
> +typedef struct kvm_cpu_context kvm_cpu_context_t;
>  
>  struct kvm_vcpu_arch {
>  	struct kvm_regs regs;
> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> index 9158de0..cee4254 100644
> --- a/arch/arm/kernel/asm-offsets.c
> +++ b/arch/arm/kernel/asm-offsets.c
> @@ -175,7 +175,9 @@ int main(void)
>    DEFINE(VCPU_CP14,		offsetof(struct kvm_vcpu, arch.cp14));
>    DEFINE(VCPU_CP15,		offsetof(struct kvm_vcpu, arch.cp15));
>    DEFINE(VCPU_VFP_GUEST,	offsetof(struct kvm_vcpu, arch.vfp_guest));
> -  DEFINE(VCPU_VFP_HOST,		offsetof(struct kvm_vcpu, arch.host_cpu_context));
> +  DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
> +  DEFINE(VCPU_VFP_HOST,		offsetof(struct kvm_cpu_context, vfp));
> +  DEFINE(VCPU_CP14_HOST,	offsetof(struct kvm_cpu_context, cp14));

it's a bit weird to call these last two VCPU_ something when they're not
offsets of the vcpu struct...

>    DEFINE(VCPU_REGS,		offsetof(struct kvm_vcpu, arch.regs));
>    DEFINE(VCPU_USR_REGS,		offsetof(struct kvm_vcpu, arch.regs.usr_regs));
>    DEFINE(VCPU_SVC_REGS,		offsetof(struct kvm_vcpu, arch.regs.svc_regs));
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index 48333ff..d4ff22e 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -176,8 +176,9 @@ __kvm_vcpu_return:
>  	@ Switch VFP/NEON hardware state to the host's
>  	add	r7, vcpu, #VCPU_VFP_GUEST
>  	store_vfp_state r7
> -	add	r7, vcpu, #VCPU_VFP_HOST
> +	add	r7, vcpu, #VCPU_HOST_CONTEXT
>  	ldr	r7, [r7]
> +	add	r7, r7, #VCPU_VFP_HOST
>  	restore_vfp_state r7
>  
>  after_vfp_restore:
> @@ -484,8 +485,9 @@ switch_to_guest_vfp:
>  	set_hcptr vmtrap, (HCPTR_TCP(10) | HCPTR_TCP(11))
>  
>  	@ Switch VFP/NEON hardware state to the guest's
> -	add	r7, r0, #VCPU_VFP_HOST
> +	add	r7, r0, #VCPU_HOST_CONTEXT
>  	ldr	r7, [r7]
> +	add	r7, r7, #VCPU_VFP_HOST
>  	store_vfp_state r7
>  	add	r7, r0, #VCPU_VFP_GUEST
>  	restore_vfp_state r7
> -- 
> 1.7.12.4
> 

Otherwise this look ok.

-Christoffer

[PATCH v4 11/15] KVM: arm: add a function to keep track of host use of the debug registers

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:26:03PM +0800, Zhichao Huang wrote:
> As we're about to implement a lazy world switch for debug registers,
> we add a function reading the break/watch control variables directly to
> indicate whether the host has enabled any break/watch points or not.
> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/include/asm/hw_breakpoint.h |  5 +++++
>  arch/arm/kernel/hw_breakpoint.c      | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
> index f2f4c61..6f375c5 100644
> --- a/arch/arm/include/asm/hw_breakpoint.h
> +++ b/arch/arm/include/asm/hw_breakpoint.h
> @@ -66,9 +66,14 @@ int arch_install_hw_breakpoint(struct perf_event *bp);
>  void arch_uninstall_hw_breakpoint(struct perf_event *bp);
>  void hw_breakpoint_pmu_read(struct perf_event *bp);
>  int hw_breakpoint_slots(int type);
> +bool hw_breakpoint_enabled(void);
>  
>  #else
>  static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk) {}
> +static inline bool hw_breakpoint_enabled(void)
> +{
> +	return false;
> +}
>  
>  #endif	/* CONFIG_HAVE_HW_BREAKPOINT */
>  #endif  /* __ASSEMBLY */
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index dc7d0a9..f56788f 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -227,6 +227,27 @@ static int get_num_brps(void)
>  	return core_has_mismatch_brps() ? brps - 1 : brps;
>  }
>  
> +/* Indicate whether the host has enabled any break/watch points or not. */
> +bool hw_breakpoint_enabled(void)
> +{
> +	struct perf_event **slots;
> +	int i;

since this is exported, shouldn't you have BUG_ON(preemptible()) here
isnce you're using this_cpu_ptr()?

At least the comment to the function should describe the context in
which this can be used.

It also looks like these arrays are protected with rcu locks, so don't
you need to do rcu_read_lock() before looking at these?

> +
> +	slots = this_cpu_ptr(bp_on_reg);
> +	for (i = 0; i < core_num_brps; i++) {
> +		if (slots[i])
> +			return true;
> +	}
> +
> +	slots = this_cpu_ptr(wp_on_reg);
> +	for (i = 0; i < core_num_wrps; i++) {
> +		if (slots[i])
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * In order to access the breakpoint/watchpoint control registers,
>   * we must be running in debug monitor mode. Unfortunately, we can
> -- 
> 1.7.12.4
> 

[PATCH v4 12/15] KVM: arm: keep track of host use of the debug registers

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:26:04PM +0800, Zhichao Huang wrote:
> Every guest entry, we need to keep track of host use of the debug
> registers.
> 
> We only call the function upon guest entry, after preempt_disable()
> and local_irq_disable(), so there is no race for it.

what about on SMP?  It may be true because you're only accessing per cpu
data structures which are never accessed by any other CPUs than
yourself, but then this commit message should explain this :)

> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/include/asm/kvm_asm.h    | 3 +++
>  arch/arm/include/asm/kvm_host.h   | 4 ++++
>  arch/arm/kernel/asm-offsets.c     | 1 +
>  arch/arm/kvm/arm.c                | 2 ++
>  arch/arm/kvm/coproc.c             | 8 ++++++++
>  arch/arm64/include/asm/kvm_host.h | 1 +
>  6 files changed, 19 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 7443a3a..5b1c3eb 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -64,6 +64,9 @@
>  #define cp14_DBGDSCRext	65	/* Debug Status and Control external */
>  #define NR_CP14_REGS	66	/* Number of regs (incl. invalid) */
>  
> +#define KVM_ARM_DEBUG_HOST_INUSE_SHIFT	0
> +#define KVM_ARM_DEBUG_HOST_INUSE	(1 << KVM_ARM_DEBUG_HOST_INUSE_SHIFT)
> +
>  #define ARM_EXCEPTION_RESET	  0
>  #define ARM_EXCEPTION_UNDEFINED   1
>  #define ARM_EXCEPTION_SOFTWARE    2
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index fc461ac..7338f34 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -131,6 +131,9 @@ struct kvm_vcpu_arch {
>  	/* System control coprocessor (cp14) */
>  	u32 cp14[NR_CP14_REGS];
>  
> +	/* Debug State */
> +	u32 debug_flags;
> +
>  	/*
>  	 * Anything that is not used directly from assembly code goes
>  	 * here.
> @@ -237,5 +240,6 @@ static inline void kvm_arch_hardware_unsetup(void) {}
>  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_setup_debug(struct kvm_vcpu *vcpu);
>  
>  #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> index cee4254..7750597 100644
> --- a/arch/arm/kernel/asm-offsets.c
> +++ b/arch/arm/kernel/asm-offsets.c
> @@ -178,6 +178,7 @@ int main(void)
>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
>    DEFINE(VCPU_VFP_HOST,		offsetof(struct kvm_cpu_context, vfp));
>    DEFINE(VCPU_CP14_HOST,	offsetof(struct kvm_cpu_context, cp14));
> +  DEFINE(VCPU_DEBUG_FLAGS,	offsetof(struct kvm_vcpu, arch.debug_flags));
>    DEFINE(VCPU_REGS,		offsetof(struct kvm_vcpu, arch.regs));
>    DEFINE(VCPU_USR_REGS,		offsetof(struct kvm_vcpu, arch.regs.usr_regs));
>    DEFINE(VCPU_SVC_REGS,		offsetof(struct kvm_vcpu, arch.regs.svc_regs));
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index bc738d2..0129346 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -550,6 +550,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			continue;
>  		}
>  
> +		kvm_arm_setup_debug(vcpu);
> +

you need to rebase this series on kvmarm/next or mainline once the
KVM/ARM pull request with the v8 debug stuff is merged.

>  		/**************************************************************
>  		 * Enter the guest
>  		 */
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index d15b250..b37afd6 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -1516,3 +1516,11 @@ void kvm_reset_coprocs(struct kvm_vcpu *vcpu)
>  		if (vcpu->arch.cp15[num] == 0x42424242)
>  			panic("Didn't reset vcpu->arch.cp15[%zi]", num);
>  }
> +
> +void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> +{
> +	if (hw_breakpoint_enabled())
> +		vcpu->arch.debug_flags |= KVM_ARM_DEBUG_HOST_INUSE;
> +	else
> +		vcpu->arch.debug_flags &= ~KVM_ARM_DEBUG_HOST_INUSE;
> +}

This otherwise looks reasonable enough.


Thanks,
-Christoffer

[PATCH v4 13/15] KVM: arm: keep track of guest use of the debug registers

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:26:05PM +0800, Zhichao Huang wrote:
> We trap debug register accesses from guest all the time, and read the
> BCR/WCR to indicate whether the guest has enabled any break/watch points
> or not.
> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/include/asm/kvm_asm.h |  2 ++
>  arch/arm/kvm/coproc.c          | 75 +++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 72 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 5b1c3eb..e9e1f0a 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -65,7 +65,9 @@
>  #define NR_CP14_REGS	66	/* Number of regs (incl. invalid) */
>  
>  #define KVM_ARM_DEBUG_HOST_INUSE_SHIFT	0
> +#define KVM_ARM_DEBUG_GUEST_INUSE_SHIFT	1
>  #define KVM_ARM_DEBUG_HOST_INUSE	(1 << KVM_ARM_DEBUG_HOST_INUSE_SHIFT)
> +#define KVM_ARM_DEBUG_GUEST_INUSE	(1 << KVM_ARM_DEBUG_GUEST_INUSE_SHIFT)
>  
>  #define ARM_EXCEPTION_RESET	  0
>  #define ARM_EXCEPTION_UNDEFINED   1
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index b37afd6..d9dcd28b 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -220,7 +220,22 @@ bool access_vm_reg(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> -static bool trap_debug32(struct kvm_vcpu *vcpu,
> +/* Indicate whether the guest has enabled any break/watch points or not. */
> +static bool guest_debug_in_use(struct kvm_vcpu *vcpu)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARM_MAX_BRP; i++)
> +		if (vcpu->arch.cp14[cp14_DBGBCR0 + i] & 0x1)
> +			return true;
> +	for (i = 0; i < ARM_MAX_WRP; i++)
> +		if (vcpu->arch.cp14[cp14_DBGWCR0 + i] & 0x1)
> +			return true;
> +
> +	return false;
> +}
> +
> +static bool __trap_debug32(struct kvm_vcpu *vcpu,
>  			const struct coproc_params *p,
>  			const struct coproc_reg *r)
>  {
> @@ -232,6 +247,56 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +/*
> + * We want to avoid world-switching all the DBG registers all the
> + * time:
> + *
> + * When we are about to run a guest, we have the following cases:
> + *
> + * 1) Neither the host nor the guest has configured any [WB]points
> + * 2) Only the host has configured any [WB]points
> + * 3) Only the guest has configured any [WB]points
> + * 4) Both the host and the guest have configured any [WB]points
> + *
> + * - In case (1), KVM should enable trapping and swtich the register
						    switch
> + *   state on guest accesses.
> + * - In cases (2), (3), and (4) we must switch the register state on each
> + *   entry/exit.
> + *
> + * For ARMv7, if the CONFIG_HAVE_HW_BREAKPOINT is set, ARM_DSCR_MDBGEN
> + * is always set(ARM64 use it to indicate that debug registers are actively
        add space  ^
> + * in use).

While I like having the overall explanation of the flow somewhere, I
feel this is a strange place to put it.  Perhaps there is a more
suitable location?

> + *
> + * - We add a function reading the break/watch control variables directly to
> + *   indicate whether the host has enabled any break/watch points or not.
> + *   We only call the function upon guest entry, after preempt_disable() and
> + *   local_irq_disable(), so there is no race for it.

This paragraph of the commenting doesn't seem to fit with the rest, and
didn't we cover this already in a previous patch?

> + *
> + * - We trap debug register accesses from guest all the time, and read the
> + *   BCR/WCR to indicate whether the guest has enabled any break/watch points
> + *   or not.

do we trap all the time?  not if we're switching the state I suppose?

> + *
> + * For this, we can keep track of the host/guest use of debug registers,
> + * and skip the save/restore dance when neither the host nor the guest has
> + * configured any [WB]points.
> + */
> +static bool trap_debug32(struct kvm_vcpu *vcpu,
> +			const struct coproc_params *p,
> +			const struct coproc_reg *r)
> +{
> +	__trap_debug32(vcpu, p, r);
> +
> +	if (p->is_write) {
> +		if ((vcpu->arch.cp14[r->reg] & 0x1) ||
> +		    guest_debug_in_use(vcpu))
> +			vcpu->arch.debug_flags |= KVM_ARM_DEBUG_GUEST_INUSE;
> +		else
> +			vcpu->arch.debug_flags &= ~KVM_ARM_DEBUG_GUEST_INUSE;

I don't understand this logic, if we are enabling one of the w/b points
or if there was already an enabled w/b point, then we set the flag, but
if you disable a single one then you clear the flag?

It looks to me like you're mixing two approaches here;  either read
through all the registers whenever you need to know to set the flag or
not, or you keep track of this on every read/write of the registers.

> +	}
> +
> +	return true;
> +}
> +
>  /* DBGIDR (RO) Debug ID */
>  static bool trap_dbgidr(struct kvm_vcpu *vcpu,
>  			const struct coproc_params *p,
> @@ -419,13 +484,13 @@ static const struct coproc_reg cp15_regs[] = {
>  #define DBG_BCR_BVR_WCR_WVR(n)					\
>  	/* DBGBVRn */						\
>  	{ CRn( 0), CRm((n)), Op1( 0), Op2( 4), is32,		\
> -	  trap_debug32,	reset_val, (cp14_DBGBVR0 + (n)), 0 },	\
> +	  __trap_debug32, reset_val, (cp14_DBGBVR0 + (n)), 0 },	\
>  	/* DBGBCRn */						\
>  	{ CRn( 0), CRm((n)), Op1( 0), Op2( 5), is32,		\
>  	  trap_debug32,	reset_val, (cp14_DBGBCR0 + (n)), 0 },	\
>  	/* DBGWVRn */						\
>  	{ CRn( 0), CRm((n)), Op1( 0), Op2( 6), is32,		\
> -	  trap_debug32,	reset_val, (cp14_DBGWVR0 + (n)), 0 },	\
> +	  __trap_debug32, reset_val, (cp14_DBGWVR0 + (n)), 0 },	\
>  	/* DBGWCRn */						\
>  	{ CRn( 0), CRm((n)), Op1( 0), Op2( 7), is32,		\
>  	  trap_debug32,	reset_val, (cp14_DBGWCR0 + (n)), 0 }
> @@ -462,7 +527,7 @@ static const struct coproc_reg cp14_regs[] = {
>  	/* DBGDSAR (64bit) */
>  	{ CRn( 0), CRm( 2), Op1( 0), Op2( 0), is64, trap_raz_wi },
>  	/* DBGDSCRext */
> -	{ CRn( 0), CRm( 2), Op1( 0), Op2( 2), is32, trap_debug32,
> +	{ CRn( 0), CRm( 2), Op1( 0), Op2( 2), is32, __trap_debug32,
>  				reset_val, cp14_DBGDSCRext, 0 },
>  	DBG_BCR_BVR_WCR_WVR(2),
>  	/* DBGDTRTXext */
> @@ -474,7 +539,7 @@ static const struct coproc_reg cp14_regs[] = {
>  	DBG_BCR_BVR_WCR_WVR(5),
>  	DBG_BCR_BVR_WCR_WVR(6),
>  	/* DBGVCR */
> -	{ CRn( 0), CRm( 7), Op1( 0), Op2( 0), is32, trap_debug32 },
> +	{ CRn( 0), CRm( 7), Op1( 0), Op2( 0), is32, __trap_debug32 },
>  	DBG_BCR_BVR_WCR_WVR(7),
>  	DBG_BCR_BVR_WCR_WVR(8),
>  	DBG_BCR_BVR_WCR_WVR(9),
> -- 
> 1.7.12.4
> 

So __trap_debug32 is for the non-control registers and trap-debug32 is
for the control registers?

I think specifically naming the control register function
trap_debug_cr would be cleaner in that case.

Thanks,
-Christoffer

[PATCH v4 06/15] KVM: arm: add trap handlers for 32-bit debug registers

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:25:58PM +0800, Zhichao Huang wrote:
> Add handlers for all the 32-bit debug registers.
> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>

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

[PATCH v4 14/15] KVM: arm: implement lazy world switch for debug registers

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:26:06PM +0800, Zhichao Huang wrote:
> Avoid world-switching all the debug registers when neither the host
> nor the guest has configured any [WB]points.
> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> ---
>  arch/arm/kvm/interrupts_head.S | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index b9e7410..7ad0adf 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -332,6 +332,21 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	sub	r12, r2, r12		@ How many WPs to skip
>  .endm
>  

> +/* If VCPU_DEBUG_FLAGS is not set, it means that neither the host

For multi-line comments, can you please have the opening '/*' on a
separate line in keeping with the rest of these files?  Please change
this throughout the patch series (I think I've asked this before).

> + * nor the guest has configured any [WB]points.
> + *
> + * We avoid world-switching all the debug registers in that case.
> + *
> + * Assume vcpu pointer in vcpu reg
> + *
> + * clobbers r2
> + */
> +.macro skip_debug_state target
> +	ldr	r2, [vcpu, #VCPU_DEBUG_FLAGS]
> +	cmp	r2, #0
> +	beq	\target
> +.endm

But if the guest started using the registers and we're not trapping
accesses, then surely you need to store the values of the registers when
you come back from the guest, right?

Something about this feels weird.

> +
>  /* Reads cp14 registers from hardware.
>   * Writes cp14 registers in-order to the CP14 struct pointed to by r10
>   *
> @@ -340,12 +355,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   * Clobbers r2-r12
>   */
>  .macro save_debug_state
> +	skip_debug_state 9999f
> +
>  	read_hw_dbg_num
>  	cp14_read_and_str r10, 4, cp14_DBGBVR0, r11
>  	cp14_read_and_str r10, 5, cp14_DBGBCR0, r11
>  	cp14_read_and_str r10, 6, cp14_DBGWVR0, r12
>  	cp14_read_and_str r10, 7, cp14_DBGWCR0, r12
> -
> +9999:
>  	/* DBGDSCR reg */
>  	mrc	p14, 0, r2, c0, c1, 0
>  	str	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
> @@ -359,12 +376,14 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>   * Clobbers r2-r12
>   */
>  .macro restore_debug_state
> +	skip_debug_state 9999f
> +
>  	read_hw_dbg_num
>  	cp14_ldr_and_write r10, 4, cp14_DBGBVR0, r11
>  	cp14_ldr_and_write r10, 5, cp14_DBGBCR0, r11
>  	cp14_ldr_and_write r10, 6, cp14_DBGWVR0, r12
>  	cp14_ldr_and_write r10, 7, cp14_DBGWCR0, r12
> -
> +9999:
>  	/* DBGDSCR reg */
>  	ldr	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
>  	mcr	p14, 0, r2, c0, c2, 2
> -- 
> 1.7.12.4
> 

[PATCH v4 15/15] KVM: arm: enable trapping of all debug registers

[Christoffer Dall]

On Mon, Aug 10, 2015 at 09:26:07PM +0800, Zhichao Huang wrote:
> Enable trapping of the debug registers unconditionally, allowing guests to
> use the debug infrastructure.
> 
> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/kvm/interrupts_head.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 7ad0adf..494991d 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -792,7 +792,7 @@ ARM_BE8(rev	r6, r6  )
>   * (hardware reset value is 0) */
>  .macro set_hdcr operation
>  	mrc	p15, 4, r2, c1, c1, 1
> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)

eh, but I thought we didn't have to trap accesses to the debug registers
if we switch them, because the guest is likely to be using them?

Maybe I am getting confused, can you repeat for me exactly when we
context-switch the registers and when we trap accesses to them?

Thanks,
-Christoffer

>  	.if \operation == vmentry
>  	orr	r2, r2, r3		@ Trap some perfmon accesses
>  	.else
> -- 
> 1.7.12.4
> 

[PATCH v4 01/15] KVM: arm: plug guest debug exploit

[Zhichao Huang]

On 2015/9/2 19:38, Christoffer Dall wrote:
> 
> I really think that we should read the register, clear the bits you care
> about (MDBGen and HDBGen) and then write back the register.
> 
> So, if I recall correctly, this is to avoid having to set HDCR_TDE
> below?
> 
> Given Will's concerns about touching this register, I'm thinking if we
> shouldn't start with the HDCR_TDE enabled (and a handler in KVM) and
> then see if we want to add this optimization later?
> 
> At the very least, you should do as Will pointed out and predicate
> writes to this register based on whether the reset code in
> hw_breakpoint.c successfully reset the debug regs.  I think checking the
> debug_err_mask variable from the C code and pass this on to the Hyp code
> would be the right way to go.
> 
> But as I said, I think we should just trap debug exceptions to begin
> with (to plug the hole) and then add the more intelligent stuff later.
> 

OK, I will set HDCR_TDE, and ignore all the debug exceptions in
KVM handlers to prevent the guest to mess with the host states.

>> +	.endif
>> +
>> +	mcr	p14, 0, r2, c0, c2, 2	@ DBGDSCR
>>  .endm
>>  
>>  /*
>> @@ -620,7 +633,7 @@ ARM_BE8(rev	r6, r6  )
>>   * (hardware reset value is 0) */
>>  .macro set_hdcr operation
>>  	mrc	p15, 4, r2, c1, c1, 1
>> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
>> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
> 
> 
> 
>>  	.if \operation == vmentry
>>  	orr	r2, r2, r3		@ Trap some perfmon accesses
>>  	.else
>> -- 
>> 1.7.12.4
>>
> 
> Thanks,
> -Christoffer
> 

[PATCH v4 10/15] KVM: arm: implement world switch for debug registers

[Zhichao Huang]

On 2015/9/2 22:53, Christoffer Dall wrote:
>> +/* Reads cp14 registers from hardware.
>> + * Writes cp14 registers in-order to the CP14 struct pointed to by r10
>> + *
>> + * Assumes vcpu pointer in vcpu reg
>> + *
>> + * Clobbers r2-r12
>> + */
>> +.macro save_debug_state
>> +	read_hw_dbg_num
>> +	cp14_read_and_str r10, 4, cp14_DBGBVR0, r11
>> +	cp14_read_and_str r10, 5, cp14_DBGBCR0, r11
>> +	cp14_read_and_str r10, 6, cp14_DBGWVR0, r12
>> +	cp14_read_and_str r10, 7, cp14_DBGWCR0, r12
>> +
>> +	/* DBGDSCR reg */
>> +	mrc	p14, 0, r2, c0, c1, 0
>> +	str	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
> 
> so again we're touching the scary register on every world-switch.  Since
> it sounds like we have experience telling us that this can cause
> troubles, I'm wondering if we can get around it by:
> 
> Only ever allow the guest to use debugging registers if we managed to
> enter_monitor_mode on the host, and in that case only allow guest
> debugging with the configuration of DBGDSCR that the host has.
> 
> If the host never managed to enable debugging, the guest probably won't
> succeed either, and we should just trap all guest accesses to the debug
> registers.
> 
> Does this work?
> 

I think it works. Since the register is dangerous, we will try not to
world switch it. It means that the guest will not be able to write the register,
and will always see what the host set. So the guest will not be able to use
hardware debug feature if the host disable it.

> 
> -Christoffer
> 

[PATCH v4 13/15] KVM: arm: keep track of guest use of the debug registers

[Zhichao Huang]

On 2015/9/3 0:01, Christoffer Dall wrote:
> On Mon, Aug 10, 2015 at 09:26:05PM +0800, Zhichao Huang wrote:
>>  
>> -static bool trap_debug32(struct kvm_vcpu *vcpu,
>> +/* Indicate whether the guest has enabled any break/watch points or not. */
>> +static bool guest_debug_in_use(struct kvm_vcpu *vcpu)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < ARM_MAX_BRP; i++)
>> +		if (vcpu->arch.cp14[cp14_DBGBCR0 + i] & 0x1)
>> +			return true;
>> +	for (i = 0; i < ARM_MAX_WRP; i++)
>> +		if (vcpu->arch.cp14[cp14_DBGWCR0 + i] & 0x1)
>> +			return true;
>> +
>> +	return false;
>> +}
>> +
>> +static bool __trap_debug32(struct kvm_vcpu *vcpu,
>>  			const struct coproc_params *p,
>>  			const struct coproc_reg *r)
>>  {
>> @@ -232,6 +247,56 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
>>  	return true;
>>  }
>>  

>> +static bool trap_debug32(struct kvm_vcpu *vcpu,
>> +			const struct coproc_params *p,
>> +			const struct coproc_reg *r)
>> +{
>> +	__trap_debug32(vcpu, p, r);
>> +
>> +	if (p->is_write) {
>> +		if ((vcpu->arch.cp14[r->reg] & 0x1) ||
>> +		    guest_debug_in_use(vcpu))
>> +			vcpu->arch.debug_flags |= KVM_ARM_DEBUG_GUEST_INUSE;
>> +		else
>> +			vcpu->arch.debug_flags &= ~KVM_ARM_DEBUG_GUEST_INUSE;
> 
> I don't understand this logic, if we are enabling one of the w/b points
> or if there was already an enabled w/b point, then we set the flag, but
> if you disable a single one then you clear the flag?
> 
> It looks to me like you're mixing two approaches here;  either read
> through all the registers whenever you need to know to set the flag or
> not, or you keep track of this on every read/write of the registers.
> 

I did it in the function guest_debug_in_use(), which will read through all the
registers.


> 
> So __trap_debug32 is for the non-control registers and trap-debug32 is
> for the control registers?
> 
> I think specifically naming the control register function
> trap_debug_cr would be cleaner in that case.
> 

OK.

> Thanks,
> -Christoffer
> 

[PATCH v4 15/15] KVM: arm: enable trapping of all debug registers

[Zhichao Huang]

On 2015/9/3 0:08, Christoffer Dall wrote:
> On Mon, Aug 10, 2015 at 09:26:07PM +0800, Zhichao Huang wrote:
>> Enable trapping of the debug registers unconditionally, allowing guests to
>> use the debug infrastructure.
>>
>> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
>> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>>  arch/arm/kvm/interrupts_head.S | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
>> index 7ad0adf..494991d 100644
>> --- a/arch/arm/kvm/interrupts_head.S
>> +++ b/arch/arm/kvm/interrupts_head.S
>> @@ -792,7 +792,7 @@ ARM_BE8(rev	r6, r6  )
>>   * (hardware reset value is 0) */
>>  .macro set_hdcr operation
>>  	mrc	p15, 4, r2, c1, c1, 1
>> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
>> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
> 
> eh, but I thought we didn't have to trap accesses to the debug registers
> if we switch them, because the guest is likely to be using them?
> 
> Maybe I am getting confused, can you repeat for me exactly when we
> context-switch the registers and when we trap accesses to them?
> 

Since we don't want to world switch the dangerous register(DBGDSCR), we have
to trap accesses all the time, to prevent the guest to write to the real register.

> Thanks,
> -Christoffer
> 
>>  	.if \operation == vmentry
>>  	orr	r2, r2, r3		@ Trap some perfmon accesses
>>  	.else
>> -- 
>> 1.7.12.4
>>

[PATCH v4 10/15] KVM: arm: implement world switch for debug registers

[Christoffer Dall]

On Tue, Sep 29, 2015 at 01:32:35PM +0800, Zhichao Huang wrote:
> 
> 
> On 2015/9/2 22:53, Christoffer Dall wrote:
> >> +/* Reads cp14 registers from hardware.
> >> + * Writes cp14 registers in-order to the CP14 struct pointed to by r10
> >> + *
> >> + * Assumes vcpu pointer in vcpu reg
> >> + *
> >> + * Clobbers r2-r12
> >> + */
> >> +.macro save_debug_state
> >> +	read_hw_dbg_num
> >> +	cp14_read_and_str r10, 4, cp14_DBGBVR0, r11
> >> +	cp14_read_and_str r10, 5, cp14_DBGBCR0, r11
> >> +	cp14_read_and_str r10, 6, cp14_DBGWVR0, r12
> >> +	cp14_read_and_str r10, 7, cp14_DBGWCR0, r12
> >> +
> >> +	/* DBGDSCR reg */
> >> +	mrc	p14, 0, r2, c0, c1, 0
> >> +	str	r2, [r10, #CP14_OFFSET(cp14_DBGDSCRext)]
> > 
> > so again we're touching the scary register on every world-switch.  Since
> > it sounds like we have experience telling us that this can cause
> > troubles, I'm wondering if we can get around it by:
> > 
> > Only ever allow the guest to use debugging registers if we managed to
> > enter_monitor_mode on the host, and in that case only allow guest
> > debugging with the configuration of DBGDSCR that the host has.
> > 
> > If the host never managed to enable debugging, the guest probably won't
> > succeed either, and we should just trap all guest accesses to the debug
> > registers.
> > 
> > Does this work?
> > 
> 
> I think it works. Since the register is dangerous, we will try not to
> world switch it. It means that the guest will not be able to write the register,
> and will always see what the host set. So the guest will not be able to use
> hardware debug feature if the host disable it.
> 
I talked to Will about this the last day of Linaro Connect and we
arrived at the conclusion that since you cannot trap on only a subset of
the debug registers (namely the DBGDSCR and the registers related to the
OS lock etc.), there is simply no safe and robus way to do this on
ARMv7.

Therfore, this patch series probably needs to be reworked so that we
*always* set TDA for the guest, and when the guest programs a break- or
watchpoint, then we program the hardware registers directly in KVM
without telling perf or hw_breakpoints about this, and we only do any of
this if monitor_mode_enabled() is true.  If not, then debugging inside
the guest simply won't work.

I also don't think that checking the host's breakpoint and wathcpoint
registers are necessary after all, since they are context switched per
process on the host, so this will only be a problem for guests when the
user uses GDB on the VCPU thread (in which case he's asking for trouble
anyway) or with using the weird perf CPU-wide breakpoints, which is very
far from a the common case, so I wouldn't worry about this for now.

Hopefully this makes sense.

Thanks,
-Christoffer

[PATCH v4 15/15] KVM: arm: enable trapping of all debug registers

[Christoffer Dall]

On Tue, Sep 29, 2015 at 01:41:45PM +0800, Zhichao Huang wrote:
> 
> 
> On 2015/9/3 0:08, Christoffer Dall wrote:
> > On Mon, Aug 10, 2015 at 09:26:07PM +0800, Zhichao Huang wrote:
> >> Enable trapping of the debug registers unconditionally, allowing guests to
> >> use the debug infrastructure.
> >>
> >> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org>
> >> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> >> ---
> >>  arch/arm/kvm/interrupts_head.S | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> >> index 7ad0adf..494991d 100644
> >> --- a/arch/arm/kvm/interrupts_head.S
> >> +++ b/arch/arm/kvm/interrupts_head.S
> >> @@ -792,7 +792,7 @@ ARM_BE8(rev	r6, r6  )
> >>   * (hardware reset value is 0) */
> >>  .macro set_hdcr operation
> >>  	mrc	p15, 4, r2, c1, c1, 1
> >> -	ldr	r3, =(HDCR_TPM|HDCR_TPMCR)
> >> +	ldr	r3, =(HDCR_TPM|HDCR_TPMCR|HDCR_TDRA|HDCR_TDOSA|HDCR_TDA)
> > 
> > eh, but I thought we didn't have to trap accesses to the debug registers
> > if we switch them, because the guest is likely to be using them?
> > 
> > Maybe I am getting confused, can you repeat for me exactly when we
> > context-switch the registers and when we trap accesses to them?
> > 
> 
> Since we don't want to world switch the dangerous register(DBGDSCR), we have
> to trap accesses all the time, to prevent the guest to write to the real register.
> 
ok, so this is in line with my comment to your previous patch, but you
did have world-switching code of DBGDSCR in this series, hence my
confusion.  So you would need to get rid of this for the new version of
the series.

Thanks,
-Christoffer