[PATCH 0/8] arm64: KVM: Fix PMU exception generation

[PATCH 0/8] arm64: KVM: Fix PMU exception generation

[Marc Zyngier]

Running the following code:

root at zomby-woof:~# cat test-pmu.c
int main(int argc, char *argv[])
{
	unsigned int val;
	asm ("mrc p15, 0, %0, c9, c13, 0\n" : "=r" (val));
	return val;
}

in a 32bit guest (or a 64bit guest with a 32bit userspace) results in
this surprising result:

[  120.347497] kvm [1150]: Unsupported guest CP15 access at: ab0945ae
[  120.353689] kvm [1142]:  { Op0( 0), Op1( 0), CRn( 9), CRm(13), Op2( 0), func_read },

which is weird, because the guest behaves correctly:
root at zomby-woof:~# ./test-pmu 
[   16.184422] test-pmu[740]: undefined instruction: pc=00000000ab0945ae
[   16.186043] Code: 00340001 b4800000 af00b085 60396078 (3f1dee19) 
Illegal instruction

It gets the expected UNDEF, and all is fine. So what?

It turns out that the PMU emulation code is a bit lazy, and tells the
rest of KVM that the emulation has failed, so that an exception gets
delivered. Subtle differences in the 32bit vs 64bit handling make it
spit an "Unsupported..." error.

This series tries to set things straight:
- Allow an exception to be injected from an emulation handler
- Make all PMU illegal accesses inject an UNDEF
- Make these illegal accesses a successful emulation w.r.t the rest of KVM.

In the process, we also squash an interesting bug in the 64bit CP
access. Similar treatment could be applied to the 32bit kernel, except
that we don't ever inject an exception there (no PMU support yet).

Marc Zyngier (8):
  arm64: KVM: Don't skip an instruction if an exception is pending
  arm64: KVM: Let the vcpu carry a pointer to the sys_reg being emulated
  arm64: KVM: Refactor pmu_*_el0_disabled
  arm64: KVM: pmu: Inject UNDEF exception on illegal register access
  arm64: KVM: PMU: Inject UNDEF on non-privileged accesses
  arm64: KVM: PMU: Inject UNDEF on read access to PMSWINC_EL0
  arm64: KVM: pmu: Make illegal accesses seen as successfully emulated
  arm64: KVM: Do not corrupt registers on failed 64bit CP read

 arch/arm64/include/asm/kvm_host.h |   5 ++
 arch/arm64/kvm/sys_regs.c         | 150 +++++++++++++++++++++-----------------
 arch/arm64/kvm/sys_regs.h         |   1 +
 3 files changed, 91 insertions(+), 65 deletions(-)

-- 
2.11.0

[PATCH 1/8] arm64: KVM: Don't skip an instruction if an exception is pending

[Marc Zyngier]

When a trapping instruction has been successfully emulated, we
skip it in order to start executing the following one. Any change
in the PC made while handling the trap (such as injecting an
exception) is going to completely wreck the execution of the
guest by skiping an instruction that hasn't been executed yet.

In order to avoid this, we introduce an "exception_pending" flag,
kept together with the description of the sys_reg that is being
emulated. If that flag gets set, we avoid the skip, making sure we
execute correctly the first instruction of the handler.

Given that nothing sets the flag to true, this patch doesn't change
the existing behaviour yet.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 9 +++++++--
 arch/arm64/kvm/sys_regs.h | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0e26f8c2b56f..7ac7fb021dde 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1590,7 +1590,8 @@ static int emulate_cp(struct kvm_vcpu *vcpu,
 
 		if (likely(r->access(vcpu, params, r))) {
 			/* Skip instruction, since it was emulated */
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+			if (!params->exception_pending)
+				kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
 			/* Handled */
 			return 0;
 		}
@@ -1645,6 +1646,7 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 	params.is_32bit = false;
 	params.CRm = (hsr >> 1) & 0xf;
 	params.is_write = ((hsr & 1) == 0);
+	params.exception_pending = false;
 
 	params.Op0 = 0;
 	params.Op1 = (hsr >> 16) & 0xf;
@@ -1697,6 +1699,7 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
 	params.CRm = (hsr >> 1) & 0xf;
 	params.regval = vcpu_get_reg(vcpu, Rt);
 	params.is_write = ((hsr & 1) == 0);
+	params.exception_pending = false;
 	params.CRn = (hsr >> 10) & 0xf;
 	params.Op0 = 0;
 	params.Op1 = (hsr >> 14) & 0x7;
@@ -1773,7 +1776,8 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
 
 		if (likely(r->access(vcpu, params, r))) {
 			/* Skip instruction, since it was emulated */
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+			if (!params->exception_pending)
+				kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
 			return 1;
 		}
 		/* If access function fails, it should complain. */
@@ -1819,6 +1823,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	params.Op2 = (esr >> 17) & 0x7;
 	params.regval = vcpu_get_reg(vcpu, Rt);
 	params.is_write = !(esr & 1);
+	params.exception_pending = false;
 
 	ret = emulate_sys_reg(vcpu, &params);
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 9c6ffd0f0196..c7f790277119 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -32,6 +32,7 @@ struct sys_reg_params {
 	bool	is_write;
 	bool	is_aarch32;
 	bool	is_32bit;	/* Only valid if is_aarch32 is true */
+	bool	exception_pending;
 };
 
 struct sys_reg_desc {
-- 
2.11.0

[PATCH 2/8] arm64: KVM: Let the vcpu carry a pointer to the sys_reg being emulated

[Marc Zyngier]

In order to be able to propagate the pointer to the sys_reg descriptor
being currently emulated without having to rewrite a lot of the sys_reg
handling, let's allow the vcpu structure to have a pointer to the
current sys_reg_params.

The pointer is set in a helper function that calls the trap handler,
and reset just after.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/kvm_host.h |  5 +++++
 arch/arm64/kvm/sys_regs.c         | 40 +++++++++++++++++++++------------------
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 4a758cba1262..9dfce56de9c0 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -194,6 +194,8 @@ struct kvm_cpu_context {
 
 typedef struct kvm_cpu_context kvm_cpu_context_t;
 
+struct sys_reg_params;
+
 struct kvm_vcpu_arch {
 	struct kvm_cpu_context ctxt;
 
@@ -270,6 +272,9 @@ struct kvm_vcpu_arch {
 
 	/* Detect first run of a vcpu */
 	bool has_run_once;
+
+	/* system register emulation in progress */
+	struct sys_reg_params *sys_reg;
 };
 
 #define vcpu_gp_regs(v)		(&(v)->arch.ctxt.gp_regs)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7ac7fb021dde..33bad02c0a00 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1557,6 +1557,26 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+static bool perform_access(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *params,
+			   const struct sys_reg_desc *r)
+{
+	bool res;
+
+	/*
+	 * Not having an accessor means that we have configured a trap
+	 * that we don't know how to handle. This certainly qualifies
+	 * as a gross bug that should be fixed right away.
+	 */
+	BUG_ON(!r->access);
+
+	vcpu->arch.sys_reg = params;
+	res = r->access(vcpu, params, r);
+	vcpu->arch.sys_reg = NULL;
+
+	return res;
+}
+
 /*
  * emulate_cp --  tries to match a sys_reg access in a handling table, and
  *                call the corresponding trap handler.
@@ -1580,15 +1600,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu,
 	r = find_reg(params, table, num);
 
 	if (r) {
-		/*
-		 * Not having an accessor means that we have
-		 * configured a trap that we don't know how to
-		 * handle. This certainly qualifies as a gross bug
-		 * that should be fixed right away.
-		 */
-		BUG_ON(!r->access);
-
-		if (likely(r->access(vcpu, params, r))) {
+		if (likely(perform_access(vcpu, params, r))) {
 			/* Skip instruction, since it was emulated */
 			if (!params->exception_pending)
 				kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
@@ -1766,15 +1778,7 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
 		r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
 
 	if (likely(r)) {
-		/*
-		 * Not having an accessor means that we have
-		 * configured a trap that we don't know how to
-		 * handle. This certainly qualifies as a gross bug
-		 * that should be fixed right away.
-		 */
-		BUG_ON(!r->access);
-
-		if (likely(r->access(vcpu, params, r))) {
+		if (likely(perform_access(vcpu, params, r))) {
 			/* Skip instruction, since it was emulated */
 			if (!params->exception_pending)
 				kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-- 
2.11.0

[PATCH 3/8] arm64: KVM: Refactor pmu_*_el0_disabled

[Marc Zyngier]

There is a lot of duplication in the pmu_*_el0_disabled helpers,
and as we're going to modify them shortly, let's move all the
common stuff in a single function.

No functionnal change.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 33bad02c0a00..0ebf27f40f98 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -460,35 +460,32 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	vcpu_sys_reg(vcpu, PMCR_EL0) = val;
 }
 
-static bool pmu_access_el0_disabled(struct kvm_vcpu *vcpu)
+static bool check_disabled(struct kvm_vcpu *vcpu, u64 flags)
 {
 	u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
+	bool cond = (reg & flags) || vcpu_mode_priv(vcpu);
 
-	return !((reg & ARMV8_PMU_USERENR_EN) || vcpu_mode_priv(vcpu));
+	return !cond;
 }
 
-static bool pmu_write_swinc_el0_disabled(struct kvm_vcpu *vcpu)
+static bool pmu_access_el0_disabled(struct kvm_vcpu *vcpu)
 {
-	u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
+	return check_disabled(vcpu, ARMV8_PMU_USERENR_EN);
+}
 
-	return !((reg & (ARMV8_PMU_USERENR_SW | ARMV8_PMU_USERENR_EN))
-		 || vcpu_mode_priv(vcpu));
+static bool pmu_write_swinc_el0_disabled(struct kvm_vcpu *vcpu)
+{
+	return check_disabled(vcpu, ARMV8_PMU_USERENR_SW | ARMV8_PMU_USERENR_EN);
 }
 
 static bool pmu_access_cycle_counter_el0_disabled(struct kvm_vcpu *vcpu)
 {
-	u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
-
-	return !((reg & (ARMV8_PMU_USERENR_CR | ARMV8_PMU_USERENR_EN))
-		 || vcpu_mode_priv(vcpu));
+	return check_disabled(vcpu, ARMV8_PMU_USERENR_CR | ARMV8_PMU_USERENR_EN);
 }
 
 static bool pmu_access_event_counter_el0_disabled(struct kvm_vcpu *vcpu)
 {
-	u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
-
-	return !((reg & (ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_EN))
-		 || vcpu_mode_priv(vcpu));
+	return check_disabled(vcpu, ARMV8_PMU_USERENR_ER | ARMV8_PMU_USERENR_EN);
 }
 
 static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
-- 
2.11.0

[PATCH 4/8] arm64: KVM: pmu: Inject UNDEF exception on illegal register access

[Marc Zyngier]

Both pmu_*_el0_disabled() and pmu_counter_idx_valid() perform checks
on the calidity of an access, but only return a boolean indicating
if the access is valid or not.

Let's allow these functions to also inject an UNDEF exception if
the access was illegal. This is where we start flagging a pending
exception.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0ebf27f40f98..eec543906e21 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -460,11 +460,20 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	vcpu_sys_reg(vcpu, PMCR_EL0) = val;
 }
 
+static void pend_undef(struct kvm_vcpu *vcpu)
+{
+	kvm_inject_undefined(vcpu);
+	vcpu->arch.sys_reg->exception_pending = true;
+}
+
 static bool check_disabled(struct kvm_vcpu *vcpu, u64 flags)
 {
 	u64 reg = vcpu_sys_reg(vcpu, PMUSERENR_EL0);
 	bool cond = (reg & flags) || vcpu_mode_priv(vcpu);
 
+	if (!cond)
+		pend_undef(vcpu);
+
 	return !cond;
 }
 
@@ -564,8 +573,10 @@ static bool pmu_counter_idx_valid(struct kvm_vcpu *vcpu, u64 idx)
 
 	pmcr = vcpu_sys_reg(vcpu, PMCR_EL0);
 	val = (pmcr >> ARMV8_PMU_PMCR_N_SHIFT) & ARMV8_PMU_PMCR_N_MASK;
-	if (idx >= val && idx != ARMV8_PMU_CYCLE_IDX)
+	if (idx >= val && idx != ARMV8_PMU_CYCLE_IDX) {
+		pend_undef(vcpu);
 		return false;
+	}
 
 	return true;
 }
-- 
2.11.0

[PATCH 5/8] arm64: KVM: PMU: Inject UNDEF on non-privileged accesses

[Marc Zyngier]

access_pminten() and access_pmuserenr() can only be accessed when
the CPU is in a priviledged mode. If it is not, let's inject an
UNDEF exception.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index eec543906e21..cbe966658d5e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -715,8 +715,10 @@ static bool access_pminten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (!kvm_arm_pmu_v3_ready(vcpu))
 		return trap_raz_wi(vcpu, p, r);
 
-	if (!vcpu_mode_priv(vcpu))
-		return false;
+	if (!vcpu_mode_priv(vcpu)) {
+		pend_undef(vcpu);
+		return true;
+	}
 
 	if (p->is_write) {
 		u64 val = p->regval & mask;
@@ -786,8 +788,10 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return trap_raz_wi(vcpu, p, r);
 
 	if (p->is_write) {
-		if (!vcpu_mode_priv(vcpu))
-			return false;
+		if (!vcpu_mode_priv(vcpu)) {
+			pend_undef(vcpu);
+			return true;
+		}
 
 		vcpu_sys_reg(vcpu, PMUSERENR_EL0) = p->regval
 						    & ARMV8_PMU_USERENR_MASK;
-- 
2.11.0

[PATCH 6/8] arm64: KVM: PMU: Inject UNDEF on read access to PMSWINC_EL0

[Marc Zyngier]

PMSWINC_EL0 is a WO register, so let's UNDEF when reading from it.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index cbe966658d5e..01c8d841851e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -778,6 +778,7 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return true;
 	}
 
+	pend_undef(vcpu);
 	return false;
 }
 
-- 
2.11.0

[PATCH 7/8] arm64: KVM: pmu: Make illegal accesses seen as successfully emulated

[Marc Zyngier]

Now that we can deliver an exception on an illegal PMU access,
we can stop pretending that they have failed to be emulated.

We can thus return "true" in all these cases.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 01c8d841851e..44b7a7325229 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -506,7 +506,7 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return trap_raz_wi(vcpu, p, r);
 
 	if (pmu_access_el0_disabled(vcpu))
-		return false;
+		return true;
 
 	if (p->is_write) {
 		/* Only update writeable bits of PMCR */
@@ -532,7 +532,7 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return trap_raz_wi(vcpu, p, r);
 
 	if (pmu_access_event_counter_el0_disabled(vcpu))
-		return false;
+		return true;
 
 	if (p->is_write)
 		vcpu_sys_reg(vcpu, PMSELR_EL0) = p->regval;
@@ -555,7 +555,7 @@ static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	BUG_ON(p->is_write);
 
 	if (pmu_access_el0_disabled(vcpu))
-		return false;
+		return true;
 
 	if (!(p->Op2 & 1))
 		pmceid = read_sysreg(pmceid0_el0);
@@ -594,14 +594,14 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 		if (r->Op2 == 2) {
 			/* PMXEVCNTR_EL0 */
 			if (pmu_access_event_counter_el0_disabled(vcpu))
-				return false;
+				return true;
 
 			idx = vcpu_sys_reg(vcpu, PMSELR_EL0)
 			      & ARMV8_PMU_COUNTER_MASK;
 		} else if (r->Op2 == 0) {
 			/* PMCCNTR_EL0 */
 			if (pmu_access_cycle_counter_el0_disabled(vcpu))
-				return false;
+				return true;
 
 			idx = ARMV8_PMU_CYCLE_IDX;
 		} else {
@@ -610,13 +610,13 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 	} else if (r->CRn == 0 && r->CRm == 9) {
 		/* PMCCNTR */
 		if (pmu_access_event_counter_el0_disabled(vcpu))
-			return false;
+			return true;
 
 		idx = ARMV8_PMU_CYCLE_IDX;
 	} else if (r->CRn == 14 && (r->CRm & 12) == 8) {
 		/* PMEVCNTRn_EL0 */
 		if (pmu_access_event_counter_el0_disabled(vcpu))
-			return false;
+			return true;
 
 		idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
 	} else {
@@ -624,11 +624,11 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 	}
 
 	if (!pmu_counter_idx_valid(vcpu, idx))
-		return false;
+		return true;
 
 	if (p->is_write) {
 		if (pmu_access_el0_disabled(vcpu))
-			return false;
+			return true;
 
 		kvm_pmu_set_counter_value(vcpu, idx, p->regval);
 	} else {
@@ -647,7 +647,7 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return trap_raz_wi(vcpu, p, r);
 
 	if (pmu_access_el0_disabled(vcpu))
-		return false;
+		return true;
 
 	if (r->CRn == 9 && r->CRm == 13 && r->Op2 == 1) {
 		/* PMXEVTYPER_EL0 */
@@ -665,7 +665,7 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	}
 
 	if (!pmu_counter_idx_valid(vcpu, idx))
-		return false;
+		return true;
 
 	if (p->is_write) {
 		kvm_pmu_set_counter_event_type(vcpu, p->regval, idx);
@@ -686,7 +686,7 @@ static bool access_pmcnten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return trap_raz_wi(vcpu, p, r);
 
 	if (pmu_access_el0_disabled(vcpu))
-		return false;
+		return true;
 
 	mask = kvm_pmu_valid_counter_mask(vcpu);
 	if (p->is_write) {
@@ -745,7 +745,7 @@ static bool access_pmovs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return trap_raz_wi(vcpu, p, r);
 
 	if (pmu_access_el0_disabled(vcpu))
-		return false;
+		return true;
 
 	if (p->is_write) {
 		if (r->CRm & 0x2)
@@ -770,7 +770,7 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return trap_raz_wi(vcpu, p, r);
 
 	if (pmu_write_swinc_el0_disabled(vcpu))
-		return false;
+		return true;
 
 	if (p->is_write) {
 		mask = kvm_pmu_valid_counter_mask(vcpu);
@@ -779,7 +779,7 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	}
 
 	pend_undef(vcpu);
-	return false;
+	return true;
 }
 
 static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
-- 
2.11.0

[PATCH 8/8] arm64: KVM: Do not corrupt registers on failed 64bit CP read

[Marc Zyngier]

If we fail to emulate a mrrc instruction, we:
1) deliver an exception,
2) spit a nastygram on the console,
3) write back some garbage to Rt/Rt2

While 1) and 2) are perfectly acceptable, 3) is out of the scope of
the architecture... Let's mimick the code in kvm_handle_cp_32 and
be more cautious.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 44b7a7325229..abda10476a04 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1687,20 +1687,18 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 		params.regval |= vcpu_get_reg(vcpu, Rt2) << 32;
 	}
 
-	if (!emulate_cp(vcpu, &params, target_specific, nr_specific))
-		goto out;
-	if (!emulate_cp(vcpu, &params, global, nr_global))
-		goto out;
-
-	unhandled_cp_access(vcpu, &params);
+	if (!emulate_cp(vcpu, &params, target_specific, nr_specific) ||
+	    !emulate_cp(vcpu, &params, global, nr_global)) {
+		/* Split up the value between registers for the read side */
+		if (!params.is_write) {
+			vcpu_set_reg(vcpu, Rt, lower_32_bits(params.regval));
+			vcpu_set_reg(vcpu, Rt2, upper_32_bits(params.regval));
+		}
 
-out:
-	/* Split up the value between registers for the read side */
-	if (!params.is_write) {
-		vcpu_set_reg(vcpu, Rt, lower_32_bits(params.regval));
-		vcpu_set_reg(vcpu, Rt2, upper_32_bits(params.regval));
+		return 1;
 	}
 
+	unhandled_cp_access(vcpu, &params);
 	return 1;
 }
 
-- 
2.11.0

[PATCH 0/8] arm64: KVM: Fix PMU exception generation

[Christoffer Dall]

Hi Marc,

On Wed, Feb 22, 2017 at 11:47:20AM +0000, Marc Zyngier wrote:
> Running the following code:
> 
> root at zomby-woof:~# cat test-pmu.c
> int main(int argc, char *argv[])
> {
> 	unsigned int val;
> 	asm ("mrc p15, 0, %0, c9, c13, 0\n" : "=r" (val));
> 	return val;
> }
> 
> in a 32bit guest (or a 64bit guest with a 32bit userspace) results in
> this surprising result:
> 
> [  120.347497] kvm [1150]: Unsupported guest CP15 access at: ab0945ae
> [  120.353689] kvm [1142]:  { Op0( 0), Op1( 0), CRn( 9), CRm(13), Op2( 0), func_read },
> 
> which is weird, because the guest behaves correctly:
> root at zomby-woof:~# ./test-pmu 
> [   16.184422] test-pmu[740]: undefined instruction: pc=00000000ab0945ae
> [   16.186043] Code: 00340001 b4800000 af00b085 60396078 (3f1dee19) 
> Illegal instruction
> 
> It gets the expected UNDEF, and all is fine. So what?
> 
> It turns out that the PMU emulation code is a bit lazy, and tells the
> rest of KVM that the emulation has failed, so that an exception gets
> delivered. Subtle differences in the 32bit vs 64bit handling make it
> spit an "Unsupported..." error.
> 
> This series tries to set things straight:
> - Allow an exception to be injected from an emulation handler
> - Make all PMU illegal accesses inject an UNDEF
> - Make these illegal accesses a successful emulation w.r.t the rest of KVM.
> 
> In the process, we also squash an interesting bug in the 64bit CP
> access. Similar treatment could be applied to the 32bit kernel, except
> that we don't ever inject an exception there (no PMU support yet).

I'm a bit confused about this series and not too thrilled of the
approach where we add a side-channel of the sys_reg param in the vcpu
structure, which may or may not contain valid data at any given point.

Couldn't we use a slightly bigger hammer (with cleaner semantics) and
let all system register handling (cp on 32-bit and 64-bit sys regs
alike) simply return true if they were emulated, in which case the
caller should advance the PC, or false ifsomething else happened, and
leave it up to the emulation of the individual registers to decide if
any exceptions should be injected.

I don't think we have that many places where we want to inject an
undefined exception in our handlers, and doing it explicitly might
actually be a good idea to make it more clear that we're emulating the
architecture properly.  What do you think?

Thanks,
-Christoffer

[PATCH 0/8] arm64: KVM: Fix PMU exception generation

[Marc Zyngier]

On Sun, Mar 05 2017 at  3:01:09 pm GMT, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Hi Marc,
>
> On Wed, Feb 22, 2017 at 11:47:20AM +0000, Marc Zyngier wrote:
>> Running the following code:
>> 
>> root at zomby-woof:~# cat test-pmu.c
>> int main(int argc, char *argv[])
>> {
>> 	unsigned int val;
>> 	asm ("mrc p15, 0, %0, c9, c13, 0\n" : "=r" (val));
>> 	return val;
>> }
>> 
>> in a 32bit guest (or a 64bit guest with a 32bit userspace) results in
>> this surprising result:
>> 
>> [  120.347497] kvm [1150]: Unsupported guest CP15 access at: ab0945ae
>> [  120.353689] kvm [1142]:  { Op0( 0), Op1( 0), CRn( 9), CRm(13), Op2( 0), func_read },
>> 
>> which is weird, because the guest behaves correctly:
>> root at zomby-woof:~# ./test-pmu 
>> [   16.184422] test-pmu[740]: undefined instruction: pc=00000000ab0945ae
>> [   16.186043] Code: 00340001 b4800000 af00b085 60396078 (3f1dee19) 
>> Illegal instruction
>> 
>> It gets the expected UNDEF, and all is fine. So what?
>> 
>> It turns out that the PMU emulation code is a bit lazy, and tells the
>> rest of KVM that the emulation has failed, so that an exception gets
>> delivered. Subtle differences in the 32bit vs 64bit handling make it
>> spit an "Unsupported..." error.
>> 
>> This series tries to set things straight:
>> - Allow an exception to be injected from an emulation handler
>> - Make all PMU illegal accesses inject an UNDEF
>> - Make these illegal accesses a successful emulation w.r.t the rest of KVM.
>> 
>> In the process, we also squash an interesting bug in the 64bit CP
>> access. Similar treatment could be applied to the 32bit kernel, except
>> that we don't ever inject an exception there (no PMU support yet).
>
> I'm a bit confused about this series and not too thrilled of the
> approach where we add a side-channel of the sys_reg param in the vcpu
> structure, which may or may not contain valid data at any given point.
>
> Couldn't we use a slightly bigger hammer (with cleaner semantics) and
> let all system register handling (cp on 32-bit and 64-bit sys regs
> alike) simply return true if they were emulated, in which case the
> caller should advance the PC, or false ifsomething else happened, and
> leave it up to the emulation of the individual registers to decide if
> any exceptions should be injected.

So that was my other option - changing the semantics of the return
value, and considering that an emulation never fails. At that stage, we
can repurpose the return value form the accessor to simply indicate
whether or not we should skip the current instruction.

> I don't think we have that many places where we want to inject an
> undefined exception in our handlers, and doing it explicitly might
> actually be a good idea to make it more clear that we're emulating the
> architecture properly.  What do you think?

I think that'd work nicely. I'll rework the series along these lines.

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.

[PATCH 0/8] arm64: KVM: Fix PMU exception generation

[Christoffer Dall]

On Tue, Mar 07, 2017 at 09:33:37AM +0000, Marc Zyngier wrote:
> On Sun, Mar 05 2017 at  3:01:09 pm GMT, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Hi Marc,
> >
> > On Wed, Feb 22, 2017 at 11:47:20AM +0000, Marc Zyngier wrote:
> >> Running the following code:
> >> 
> >> root at zomby-woof:~# cat test-pmu.c
> >> int main(int argc, char *argv[])
> >> {
> >> 	unsigned int val;
> >> 	asm ("mrc p15, 0, %0, c9, c13, 0\n" : "=r" (val));
> >> 	return val;
> >> }
> >> 
> >> in a 32bit guest (or a 64bit guest with a 32bit userspace) results in
> >> this surprising result:
> >> 
> >> [  120.347497] kvm [1150]: Unsupported guest CP15 access at: ab0945ae
> >> [  120.353689] kvm [1142]:  { Op0( 0), Op1( 0), CRn( 9), CRm(13), Op2( 0), func_read },
> >> 
> >> which is weird, because the guest behaves correctly:
> >> root at zomby-woof:~# ./test-pmu 
> >> [   16.184422] test-pmu[740]: undefined instruction: pc=00000000ab0945ae
> >> [   16.186043] Code: 00340001 b4800000 af00b085 60396078 (3f1dee19) 
> >> Illegal instruction
> >> 
> >> It gets the expected UNDEF, and all is fine. So what?
> >> 
> >> It turns out that the PMU emulation code is a bit lazy, and tells the
> >> rest of KVM that the emulation has failed, so that an exception gets
> >> delivered. Subtle differences in the 32bit vs 64bit handling make it
> >> spit an "Unsupported..." error.
> >> 
> >> This series tries to set things straight:
> >> - Allow an exception to be injected from an emulation handler
> >> - Make all PMU illegal accesses inject an UNDEF
> >> - Make these illegal accesses a successful emulation w.r.t the rest of KVM.
> >> 
> >> In the process, we also squash an interesting bug in the 64bit CP
> >> access. Similar treatment could be applied to the 32bit kernel, except
> >> that we don't ever inject an exception there (no PMU support yet).
> >
> > I'm a bit confused about this series and not too thrilled of the
> > approach where we add a side-channel of the sys_reg param in the vcpu
> > structure, which may or may not contain valid data at any given point.
> >
> > Couldn't we use a slightly bigger hammer (with cleaner semantics) and
> > let all system register handling (cp on 32-bit and 64-bit sys regs
> > alike) simply return true if they were emulated, in which case the
> > caller should advance the PC, or false ifsomething else happened, and
> > leave it up to the emulation of the individual registers to decide if
> > any exceptions should be injected.
> 
> So that was my other option - changing the semantics of the return
> value, and considering that an emulation never fails. At that stage, we
> can repurpose the return value form the accessor to simply indicate
> whether or not we should skip the current instruction.
> 
> > I don't think we have that many places where we want to inject an
> > undefined exception in our handlers, and doing it explicitly might
> > actually be a good idea to make it more clear that we're emulating the
> > architecture properly.  What do you think?
> 
> I think that'd work nicely. I'll rework the series along these lines.
> 
Awesome, thanks.
-Christoffer