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

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

[Marc Zyngier]

Running the following code:

root@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@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:
- 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 is applied to the 32bit kernel, except that
we don't ever inject an exception there (no PMU support yet).

* From v1:
  - Got rid of the original sideband information, and declared that an emulation
    can never fail. The return value now indicates whether or not we should advance the PC.
  - Sanitized read_from_write_only/write_to_read_only.
  - Implemented similar semantics on 32bit (although unaffected so far).

Marc Zyngier (9):
  arm64: KVM: PMU: 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: Make unexpected reads from WO registers inject an undef
  arm64: KVM: PMU: Inject UNDEF on read access to PMSWINC_EL0
  arm64: KVM: Treat sysreg accessors returning false as successful
  arm64: KVM: Do not corrupt registers on failed 64bit CP read
  arm: KVM: Make unexpected register accesses inject an undef
  arm: KVM: Treat CP15 accessors returning false as successful

 arch/arm/kvm/coproc.c     |  24 +++++++--
 arch/arm/kvm/coproc.h     |  18 -------
 arch/arm64/kvm/sys_regs.c | 131 ++++++++++++++++++++++++----------------------
 arch/arm64/kvm/sys_regs.h |  18 -------
 4 files changed, 88 insertions(+), 103 deletions(-)

-- 
2.11.0

[PATCH v2 0/9] 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:
- 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 is applied to the 32bit kernel, except that
we don't ever inject an exception there (no PMU support yet).

* From v1:
  - Got rid of the original sideband information, and declared that an emulation
    can never fail. The return value now indicates whether or not we should advance the PC.
  - Sanitized read_from_write_only/write_to_read_only.
  - Implemented similar semantics on 32bit (although unaffected so far).

Marc Zyngier (9):
  arm64: KVM: PMU: 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: Make unexpected reads from WO registers inject an undef
  arm64: KVM: PMU: Inject UNDEF on read access to PMSWINC_EL0
  arm64: KVM: Treat sysreg accessors returning false as successful
  arm64: KVM: Do not corrupt registers on failed 64bit CP read
  arm: KVM: Make unexpected register accesses inject an undef
  arm: KVM: Treat CP15 accessors returning false as successful

 arch/arm/kvm/coproc.c     |  24 +++++++--
 arch/arm/kvm/coproc.h     |  18 -------
 arch/arm64/kvm/sys_regs.c | 131 ++++++++++++++++++++++++----------------------
 arch/arm64/kvm/sys_regs.h |  18 -------
 4 files changed, 88 insertions(+), 103 deletions(-)

-- 
2.11.0

[PATCH v2 1/9] arm64: KVM: PMU: 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 0e26f8c2b56f..7e1d673304d5 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 v2 1/9] arm64: KVM: PMU: 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 0e26f8c2b56f..7e1d673304d5 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 v2 2/9] 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 validity 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.

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7e1d673304d5..d98ce9a52291 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -465,6 +465,9 @@ 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)
+		kvm_inject_undefined(vcpu);
+
 	return !cond;
 }
 
@@ -564,8 +567,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) {
+		kvm_inject_undefined(vcpu);
 		return false;
+	}
 
 	return true;
 }
-- 
2.11.0

[PATCH v2 2/9] 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 validity 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.

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7e1d673304d5..d98ce9a52291 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -465,6 +465,9 @@ 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)
+		kvm_inject_undefined(vcpu);
+
 	return !cond;
 }
 
@@ -564,8 +567,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) {
+		kvm_inject_undefined(vcpu);
 		return false;
+	}
 
 	return true;
 }
-- 
2.11.0

[PATCH v2 3/9] 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d98ce9a52291..5e3ce7890b35 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -709,8 +709,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))
+	if (!vcpu_mode_priv(vcpu)) {
+		kvm_inject_undefined(vcpu);
 		return false;
+	}
 
 	if (p->is_write) {
 		u64 val = p->regval & mask;
@@ -780,8 +782,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))
+		if (!vcpu_mode_priv(vcpu)) {
+			kvm_inject_undefined(vcpu);
 			return false;
+		}
 
 		vcpu_sys_reg(vcpu, PMUSERENR_EL0) = p->regval
 						    & ARMV8_PMU_USERENR_MASK;
-- 
2.11.0

[PATCH v2 3/9] 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d98ce9a52291..5e3ce7890b35 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -709,8 +709,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))
+	if (!vcpu_mode_priv(vcpu)) {
+		kvm_inject_undefined(vcpu);
 		return false;
+	}
 
 	if (p->is_write) {
 		u64 val = p->regval & mask;
@@ -780,8 +782,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))
+		if (!vcpu_mode_priv(vcpu)) {
+			kvm_inject_undefined(vcpu);
 			return false;
+		}
 
 		vcpu_sys_reg(vcpu, PMUSERENR_EL0) = p->regval
 						    & ARMV8_PMU_USERENR_MASK;
-- 
2.11.0

[PATCH v2 4/9] arm64: KVM: Make unexpected reads from WO registers inject an undef

[Marc Zyngier]

Reads from write-only system registers are generally confined to
EL1 and not propagated to EL2 (that's what the architecture
mantates). In order to be sure that we have a sane behaviour
even in the unlikely event that we have a broken system, we still
handle it in KVM.

In that case, let's inject an undef into the guest.

Let's also remove write_to_read_only which isn't used anywhere.

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5e3ce7890b35..19a036b4f6ac 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -55,6 +55,15 @@
  * 64bit interface.
  */
 
+static bool read_from_write_only(struct kvm_vcpu *vcpu,
+				 const struct sys_reg_params *params)
+{
+	WARN_ONCE(1, "Unexpected sys_reg read to write-only register\n");
+	print_sys_reg_instr(params);
+	kvm_inject_undefined(vcpu);
+	return false;
+}
+
 /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
 static u32 cache_levels;
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 9c6ffd0f0196..638f724e45af 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -83,24 +83,6 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
 	return true;
 }
 
-static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
-				      const struct sys_reg_params *params)
-{
-	kvm_debug("sys_reg write to read-only register at: %lx\n",
-		  *vcpu_pc(vcpu));
-	print_sys_reg_instr(params);
-	return false;
-}
-
-static inline bool read_from_write_only(struct kvm_vcpu *vcpu,
-					const struct sys_reg_params *params)
-{
-	kvm_debug("sys_reg read to write-only register at: %lx\n",
-		  *vcpu_pc(vcpu));
-	print_sys_reg_instr(params);
-	return false;
-}
-
 /* Reset functions */
 static inline void reset_unknown(struct kvm_vcpu *vcpu,
 				 const struct sys_reg_desc *r)
-- 
2.11.0

[PATCH v2 4/9] arm64: KVM: Make unexpected reads from WO registers inject an undef

[Marc Zyngier]

Reads from write-only system registers are generally confined to
EL1 and not propagated to EL2 (that's what the architecture
mantates). In order to be sure that we have a sane behaviour
even in the unlikely event that we have a broken system, we still
handle it in KVM.

In that case, let's inject an undef into the guest.

Let's also remove write_to_read_only which isn't used anywhere.

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5e3ce7890b35..19a036b4f6ac 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -55,6 +55,15 @@
  * 64bit interface.
  */
 
+static bool read_from_write_only(struct kvm_vcpu *vcpu,
+				 const struct sys_reg_params *params)
+{
+	WARN_ONCE(1, "Unexpected sys_reg read to write-only register\n");
+	print_sys_reg_instr(params);
+	kvm_inject_undefined(vcpu);
+	return false;
+}
+
 /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
 static u32 cache_levels;
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 9c6ffd0f0196..638f724e45af 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -83,24 +83,6 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
 	return true;
 }
 
-static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
-				      const struct sys_reg_params *params)
-{
-	kvm_debug("sys_reg write to read-only register at: %lx\n",
-		  *vcpu_pc(vcpu));
-	print_sys_reg_instr(params);
-	return false;
-}
-
-static inline bool read_from_write_only(struct kvm_vcpu *vcpu,
-					const struct sys_reg_params *params)
-{
-	kvm_debug("sys_reg read to write-only register at: %lx\n",
-		  *vcpu_pc(vcpu));
-	print_sys_reg_instr(params);
-	return false;
-}
-
 /* Reset functions */
 static inline void reset_unknown(struct kvm_vcpu *vcpu,
 				 const struct sys_reg_desc *r)
-- 
2.11.0

[PATCH v2 5/9] 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
(in the highly hypothetical case where this doesn't UNDEF at EL1).

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 19a036b4f6ac..f80a61af5e88 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -772,16 +772,15 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (!kvm_arm_pmu_v3_ready(vcpu))
 		return trap_raz_wi(vcpu, p, r);
 
+	if (!p->is_write)
+		return read_from_write_only(vcpu, p);
+
 	if (pmu_write_swinc_el0_disabled(vcpu))
 		return false;
 
-	if (p->is_write) {
-		mask = kvm_pmu_valid_counter_mask(vcpu);
-		kvm_pmu_software_increment(vcpu, p->regval & mask);
-		return true;
-	}
-
-	return false;
+	mask = kvm_pmu_valid_counter_mask(vcpu);
+	kvm_pmu_software_increment(vcpu, p->regval & mask);
+	return true;
 }
 
 static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
-- 
2.11.0

[PATCH v2 5/9] 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
(in the highly hypothetical case where this doesn't UNDEF at EL1).

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 19a036b4f6ac..f80a61af5e88 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -772,16 +772,15 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (!kvm_arm_pmu_v3_ready(vcpu))
 		return trap_raz_wi(vcpu, p, r);
 
+	if (!p->is_write)
+		return read_from_write_only(vcpu, p);
+
 	if (pmu_write_swinc_el0_disabled(vcpu))
 		return false;
 
-	if (p->is_write) {
-		mask = kvm_pmu_valid_counter_mask(vcpu);
-		kvm_pmu_software_increment(vcpu, p->regval & mask);
-		return true;
-	}
-
-	return false;
+	mask = kvm_pmu_valid_counter_mask(vcpu);
+	kvm_pmu_software_increment(vcpu, p->regval & mask);
+	return true;
 }
 
 static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
-- 
2.11.0

[PATCH v2 6/9] arm64: KVM: Treat sysreg accessors returning false as successful

[Marc Zyngier]

Instead of considering that a sysreg accessor has failed when
returning false, let's consider that it is *always* successful
(after all, we won't stand for an incomplete emulation).

The return value now simply indicates whether we should skip
the instruction (because it has now been emulated), or if we
should leave the PC alone if the emulation has injected an
exception.

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f80a61af5e88..4e5d4eee8cec 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1571,6 +1571,22 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+static void perform_access(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *params,
+			   const struct sys_reg_desc *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);
+
+	/* Skip instruction if instructed so */
+	if (likely(r->access(vcpu, params, r)))
+		kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+}
+
 /*
  * emulate_cp --  tries to match a sys_reg access in a handling table, and
  *                call the corresponding trap handler.
@@ -1594,20 +1610,8 @@ 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))) {
-			/* Skip instruction, since it was emulated */
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-			/* Handled */
-			return 0;
-		}
+		perform_access(vcpu, params, r);
+		return 0;
 	}
 
 	/* Not handled */
@@ -1777,26 +1781,13 @@ 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))) {
-			/* 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. */
+		perform_access(vcpu, params, r);
 	} else {
 		kvm_err("Unsupported guest sys_reg access at: %lx\n",
 			*vcpu_pc(vcpu));
 		print_sys_reg_instr(params);
+		kvm_inject_undefined(vcpu);
 	}
-	kvm_inject_undefined(vcpu);
 	return 1;
 }
 
-- 
2.11.0

[PATCH v2 6/9] arm64: KVM: Treat sysreg accessors returning false as successful

[Marc Zyngier]

Instead of considering that a sysreg accessor has failed when
returning false, let's consider that it is *always* successful
(after all, we won't stand for an incomplete emulation).

The return value now simply indicates whether we should skip
the instruction (because it has now been emulated), or if we
should leave the PC alone if the emulation has injected an
exception.

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f80a61af5e88..4e5d4eee8cec 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1571,6 +1571,22 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+static void perform_access(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *params,
+			   const struct sys_reg_desc *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);
+
+	/* Skip instruction if instructed so */
+	if (likely(r->access(vcpu, params, r)))
+		kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+}
+
 /*
  * emulate_cp --  tries to match a sys_reg access in a handling table, and
  *                call the corresponding trap handler.
@@ -1594,20 +1610,8 @@ 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))) {
-			/* Skip instruction, since it was emulated */
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-			/* Handled */
-			return 0;
-		}
+		perform_access(vcpu, params, r);
+		return 0;
 	}
 
 	/* Not handled */
@@ -1777,26 +1781,13 @@ 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))) {
-			/* 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. */
+		perform_access(vcpu, params, r);
 	} else {
 		kvm_err("Unsupported guest sys_reg access at: %lx\n",
 			*vcpu_pc(vcpu));
 		print_sys_reg_instr(params);
+		kvm_inject_undefined(vcpu);
 	}
-	kvm_inject_undefined(vcpu);
 	return 1;
 }
 
-- 
2.11.0

[PATCH v2 7/9] 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 4e5d4eee8cec..1080a76e960f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1678,20 +1678,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 v2 7/9] 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 4e5d4eee8cec..1080a76e960f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1678,20 +1678,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 v2 8/9] arm: KVM: Make unexpected register accesses inject an undef

[Marc Zyngier]

Reads from write-only system registers are generally confined to
EL1 and not propagated to EL2 (that's what the architecture
mantates). In order to be sure that we have a sane behaviour
even in the unlikely event that we have a broken system, we still
handle it in KVM. Same goes for write to RO registers.

In that case, let's inject an undef into the guest.

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

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 3e5e4194ef86..519aac12b365 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -40,6 +40,24 @@
  * Co-processor emulation
  *****************************************************************************/
 
+static bool write_to_read_only(struct kvm_vcpu *vcpu,
+			       const struct coproc_params *params)
+{
+	WARN_ONCE(1, "CP15 write to read-only register\n");
+	print_cp_instr(params);
+	kvm_inject_undefined(vcpu);
+	return false;
+}
+
+static bool read_from_write_only(struct kvm_vcpu *vcpu,
+				 const struct coproc_params *params)
+{
+	WARN_ONCE(1, "CP15 read to write-only register\n");
+	print_cp_instr(params);
+	kvm_inject_undefined(vcpu);
+	return false;
+}
+
 /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
 static u32 cache_levels;
 
diff --git a/arch/arm/kvm/coproc.h b/arch/arm/kvm/coproc.h
index eef1759c2b65..3a41b7d1eb86 100644
--- a/arch/arm/kvm/coproc.h
+++ b/arch/arm/kvm/coproc.h
@@ -81,24 +81,6 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
 	return true;
 }
 
-static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
-				      const struct coproc_params *params)
-{
-	kvm_debug("CP15 write to read-only register at: %08lx\n",
-		  *vcpu_pc(vcpu));
-	print_cp_instr(params);
-	return false;
-}
-
-static inline bool read_from_write_only(struct kvm_vcpu *vcpu,
-					const struct coproc_params *params)
-{
-	kvm_debug("CP15 read to write-only register at: %08lx\n",
-		  *vcpu_pc(vcpu));
-	print_cp_instr(params);
-	return false;
-}
-
 /* Reset functions */
 static inline void reset_unknown(struct kvm_vcpu *vcpu,
 				 const struct coproc_reg *r)
-- 
2.11.0

[PATCH v2 8/9] arm: KVM: Make unexpected register accesses inject an undef

[Marc Zyngier]

Reads from write-only system registers are generally confined to
EL1 and not propagated to EL2 (that's what the architecture
mantates). In order to be sure that we have a sane behaviour
even in the unlikely event that we have a broken system, we still
handle it in KVM. Same goes for write to RO registers.

In that case, let's inject an undef into the guest.

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

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 3e5e4194ef86..519aac12b365 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -40,6 +40,24 @@
  * Co-processor emulation
  *****************************************************************************/
 
+static bool write_to_read_only(struct kvm_vcpu *vcpu,
+			       const struct coproc_params *params)
+{
+	WARN_ONCE(1, "CP15 write to read-only register\n");
+	print_cp_instr(params);
+	kvm_inject_undefined(vcpu);
+	return false;
+}
+
+static bool read_from_write_only(struct kvm_vcpu *vcpu,
+				 const struct coproc_params *params)
+{
+	WARN_ONCE(1, "CP15 read to write-only register\n");
+	print_cp_instr(params);
+	kvm_inject_undefined(vcpu);
+	return false;
+}
+
 /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
 static u32 cache_levels;
 
diff --git a/arch/arm/kvm/coproc.h b/arch/arm/kvm/coproc.h
index eef1759c2b65..3a41b7d1eb86 100644
--- a/arch/arm/kvm/coproc.h
+++ b/arch/arm/kvm/coproc.h
@@ -81,24 +81,6 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
 	return true;
 }
 
-static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
-				      const struct coproc_params *params)
-{
-	kvm_debug("CP15 write to read-only register at: %08lx\n",
-		  *vcpu_pc(vcpu));
-	print_cp_instr(params);
-	return false;
-}
-
-static inline bool read_from_write_only(struct kvm_vcpu *vcpu,
-					const struct coproc_params *params)
-{
-	kvm_debug("CP15 read to write-only register at: %08lx\n",
-		  *vcpu_pc(vcpu));
-	print_cp_instr(params);
-	return false;
-}
-
 /* Reset functions */
 static inline void reset_unknown(struct kvm_vcpu *vcpu,
 				 const struct coproc_reg *r)
-- 
2.11.0

[PATCH v2 9/9] arm: KVM: Treat CP15 accessors returning false as successful

[Marc Zyngier]

Instead of considering that a CP15 accessor has failed when
returning false, let's consider that it is *always* successful
(after all, we won't stand for an incomplete emulation).

The return value now simply indicates whether we should skip
the instruction (because it has now been emulated), or if we
should leave the PC alone if the emulation has injected an
exception.

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

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 519aac12b365..2c14b69511e9 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -520,15 +520,15 @@ static int emulate_cp15(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));
-			return 1;
 		}
-		/* If access function fails, it should complain. */
 	} else {
+		/* If access function fails, it should complain. */
 		kvm_err("Unsupported guest CP15 access at: %08lx\n",
 			*vcpu_pc(vcpu));
 		print_cp_instr(params);
+		kvm_inject_undefined(vcpu);
 	}
-	kvm_inject_undefined(vcpu);
+
 	return 1;
 }
 
-- 
2.11.0

[PATCH v2 9/9] arm: KVM: Treat CP15 accessors returning false as successful

[Marc Zyngier]

Instead of considering that a CP15 accessor has failed when
returning false, let's consider that it is *always* successful
(after all, we won't stand for an incomplete emulation).

The return value now simply indicates whether we should skip
the instruction (because it has now been emulated), or if we
should leave the PC alone if the emulation has injected an
exception.

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

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index 519aac12b365..2c14b69511e9 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -520,15 +520,15 @@ static int emulate_cp15(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));
-			return 1;
 		}
-		/* If access function fails, it should complain. */
 	} else {
+		/* If access function fails, it should complain. */
 		kvm_err("Unsupported guest CP15 access at: %08lx\n",
 			*vcpu_pc(vcpu));
 		print_cp_instr(params);
+		kvm_inject_undefined(vcpu);
 	}
-	kvm_inject_undefined(vcpu);
+
 	return 1;
 }
 
-- 
2.11.0

Re: [PATCH v2 1/9] arm64: KVM: PMU: Refactor pmu_*_el0_disabled

[Suzuki K Poulose]

On 27/03/17 17:03, Marc Zyngier wrote:
> 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.

nit: s/functionnal/functional

>
> 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 0e26f8c2b56f..7e1d673304d5 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)

minor nit: check_disabled sounds too generic for a helper which checks for
something specific to pmuserenr_el0 register in a file where we deal with
lot of system registers. check_pmu_access_disabled()  ?

Suzuki

[PATCH v2 1/9] arm64: KVM: PMU: Refactor pmu_*_el0_disabled

[Suzuki K Poulose]

On 27/03/17 17:03, Marc Zyngier wrote:
> 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.

nit: s/functionnal/functional

>
> 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 0e26f8c2b56f..7e1d673304d5 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)

minor nit: check_disabled sounds too generic for a helper which checks for
something specific to pmuserenr_el0 register in a file where we deal with
lot of system registers. check_pmu_access_disabled()  ?

Suzuki

Re: [PATCH v2 1/9] arm64: KVM: PMU: Refactor pmu_*_el0_disabled

[Marc Zyngier]

On 27/03/17 18:03, Suzuki K Poulose wrote:
> On 27/03/17 17:03, Marc Zyngier wrote:
>> 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.
> 
> nit: s/functionnal/functional
> 
>>
>> 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 0e26f8c2b56f..7e1d673304d5 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)
> 
> minor nit: check_disabled sounds too generic for a helper which checks for
> something specific to pmuserenr_el0 register in a file where we deal with
> lot of system registers. check_pmu_access_disabled()  ?

Fair enough. I'll fix that.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v2 1/9] arm64: KVM: PMU: Refactor pmu_*_el0_disabled

[Marc Zyngier]

On 27/03/17 18:03, Suzuki K Poulose wrote:
> On 27/03/17 17:03, Marc Zyngier wrote:
>> 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.
> 
> nit: s/functionnal/functional
> 
>>
>> 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 0e26f8c2b56f..7e1d673304d5 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)
> 
> minor nit: check_disabled sounds too generic for a helper which checks for
> something specific to pmuserenr_el0 register in a file where we deal with
> lot of system registers. check_pmu_access_disabled()  ?

Fair enough. I'll fix that.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 2/9] arm64: KVM: PMU: Inject UNDEF exception on illegal register access

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:38PM +0100, Marc Zyngier wrote:
> Both pmu_*_el0_disabled() and pmu_counter_idx_valid() perform checks
> on the validity 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.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

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

> ---
>  arch/arm64/kvm/sys_regs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7e1d673304d5..d98ce9a52291 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -465,6 +465,9 @@ 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)
> +		kvm_inject_undefined(vcpu);
> +
>  	return !cond;
>  }
>  
> @@ -564,8 +567,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) {
> +		kvm_inject_undefined(vcpu);
>  		return false;
> +	}
>  
>  	return true;
>  }
> -- 
> 2.11.0
> 

[PATCH v2 2/9] arm64: KVM: PMU: Inject UNDEF exception on illegal register access

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:38PM +0100, Marc Zyngier wrote:
> Both pmu_*_el0_disabled() and pmu_counter_idx_valid() perform checks
> on the validity 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.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

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

> ---
>  arch/arm64/kvm/sys_regs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7e1d673304d5..d98ce9a52291 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -465,6 +465,9 @@ 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)
> +		kvm_inject_undefined(vcpu);
> +
>  	return !cond;
>  }
>  
> @@ -564,8 +567,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) {
> +		kvm_inject_undefined(vcpu);
>  		return false;
> +	}
>  
>  	return true;
>  }
> -- 
> 2.11.0
> 

Re: [PATCH v2 3/9] arm64: KVM: PMU: Inject UNDEF on non-privileged accesses

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:39PM +0100, Marc Zyngier wrote:
> 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>

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

> ---
>  arch/arm64/kvm/sys_regs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d98ce9a52291..5e3ce7890b35 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -709,8 +709,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))
> +	if (!vcpu_mode_priv(vcpu)) {
> +		kvm_inject_undefined(vcpu);
>  		return false;
> +	}
>  
>  	if (p->is_write) {
>  		u64 val = p->regval & mask;
> @@ -780,8 +782,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))
> +		if (!vcpu_mode_priv(vcpu)) {
> +			kvm_inject_undefined(vcpu);
>  			return false;
> +		}
>  
>  		vcpu_sys_reg(vcpu, PMUSERENR_EL0) = p->regval
>  						    & ARMV8_PMU_USERENR_MASK;
> -- 
> 2.11.0
> 

[PATCH v2 3/9] arm64: KVM: PMU: Inject UNDEF on non-privileged accesses

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:39PM +0100, Marc Zyngier wrote:
> 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>

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

> ---
>  arch/arm64/kvm/sys_regs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d98ce9a52291..5e3ce7890b35 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -709,8 +709,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))
> +	if (!vcpu_mode_priv(vcpu)) {
> +		kvm_inject_undefined(vcpu);
>  		return false;
> +	}
>  
>  	if (p->is_write) {
>  		u64 val = p->regval & mask;
> @@ -780,8 +782,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))
> +		if (!vcpu_mode_priv(vcpu)) {
> +			kvm_inject_undefined(vcpu);
>  			return false;
> +		}
>  
>  		vcpu_sys_reg(vcpu, PMUSERENR_EL0) = p->regval
>  						    & ARMV8_PMU_USERENR_MASK;
> -- 
> 2.11.0
> 

Re: [PATCH v2 4/9] arm64: KVM: Make unexpected reads from WO registers inject an undef

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:40PM +0100, Marc Zyngier wrote:
> Reads from write-only system registers are generally confined to
> EL1 and not propagated to EL2 (that's what the architecture
> mantates). In order to be sure that we have a sane behaviour
> even in the unlikely event that we have a broken system, we still
> handle it in KVM.
> 
> In that case, let's inject an undef into the guest.
> 
> Let's also remove write_to_read_only which isn't used anywhere.
> 

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

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c |  9 +++++++++
>  arch/arm64/kvm/sys_regs.h | 18 ------------------
>  2 files changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5e3ce7890b35..19a036b4f6ac 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -55,6 +55,15 @@
>   * 64bit interface.
>   */
>  
> +static bool read_from_write_only(struct kvm_vcpu *vcpu,
> +				 const struct sys_reg_params *params)
> +{
> +	WARN_ONCE(1, "Unexpected sys_reg read to write-only register\n");
> +	print_sys_reg_instr(params);
> +	kvm_inject_undefined(vcpu);
> +	return false;
> +}
> +
>  /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
>  static u32 cache_levels;
>  
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 9c6ffd0f0196..638f724e45af 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -83,24 +83,6 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> -static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
> -				      const struct sys_reg_params *params)
> -{
> -	kvm_debug("sys_reg write to read-only register at: %lx\n",
> -		  *vcpu_pc(vcpu));
> -	print_sys_reg_instr(params);
> -	return false;
> -}
> -
> -static inline bool read_from_write_only(struct kvm_vcpu *vcpu,
> -					const struct sys_reg_params *params)
> -{
> -	kvm_debug("sys_reg read to write-only register at: %lx\n",
> -		  *vcpu_pc(vcpu));
> -	print_sys_reg_instr(params);
> -	return false;
> -}
> -
>  /* Reset functions */
>  static inline void reset_unknown(struct kvm_vcpu *vcpu,
>  				 const struct sys_reg_desc *r)
> -- 
> 2.11.0
> 

[PATCH v2 4/9] arm64: KVM: Make unexpected reads from WO registers inject an undef

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:40PM +0100, Marc Zyngier wrote:
> Reads from write-only system registers are generally confined to
> EL1 and not propagated to EL2 (that's what the architecture
> mantates). In order to be sure that we have a sane behaviour
> even in the unlikely event that we have a broken system, we still
> handle it in KVM.
> 
> In that case, let's inject an undef into the guest.
> 
> Let's also remove write_to_read_only which isn't used anywhere.
> 

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

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c |  9 +++++++++
>  arch/arm64/kvm/sys_regs.h | 18 ------------------
>  2 files changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5e3ce7890b35..19a036b4f6ac 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -55,6 +55,15 @@
>   * 64bit interface.
>   */
>  
> +static bool read_from_write_only(struct kvm_vcpu *vcpu,
> +				 const struct sys_reg_params *params)
> +{
> +	WARN_ONCE(1, "Unexpected sys_reg read to write-only register\n");
> +	print_sys_reg_instr(params);
> +	kvm_inject_undefined(vcpu);
> +	return false;
> +}
> +
>  /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
>  static u32 cache_levels;
>  
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 9c6ffd0f0196..638f724e45af 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -83,24 +83,6 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> -static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
> -				      const struct sys_reg_params *params)
> -{
> -	kvm_debug("sys_reg write to read-only register at: %lx\n",
> -		  *vcpu_pc(vcpu));
> -	print_sys_reg_instr(params);
> -	return false;
> -}
> -
> -static inline bool read_from_write_only(struct kvm_vcpu *vcpu,
> -					const struct sys_reg_params *params)
> -{
> -	kvm_debug("sys_reg read to write-only register at: %lx\n",
> -		  *vcpu_pc(vcpu));
> -	print_sys_reg_instr(params);
> -	return false;
> -}
> -
>  /* Reset functions */
>  static inline void reset_unknown(struct kvm_vcpu *vcpu,
>  				 const struct sys_reg_desc *r)
> -- 
> 2.11.0
> 

Re: [PATCH v2 5/9] arm64: KVM: PMU: Inject UNDEF on read access to PMSWINC_EL0

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:41PM +0100, Marc Zyngier wrote:
> PMSWINC_EL0 is a WO register, so let's UNDEF when reading from it
> (in the highly hypothetical case where this doesn't UNDEF at EL1).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

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

> ---
>  arch/arm64/kvm/sys_regs.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 19a036b4f6ac..f80a61af5e88 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -772,16 +772,15 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	if (!kvm_arm_pmu_v3_ready(vcpu))
>  		return trap_raz_wi(vcpu, p, r);
>  
> +	if (!p->is_write)
> +		return read_from_write_only(vcpu, p);
> +
>  	if (pmu_write_swinc_el0_disabled(vcpu))
>  		return false;
>  
> -	if (p->is_write) {
> -		mask = kvm_pmu_valid_counter_mask(vcpu);
> -		kvm_pmu_software_increment(vcpu, p->regval & mask);
> -		return true;
> -	}
> -
> -	return false;
> +	mask = kvm_pmu_valid_counter_mask(vcpu);
> +	kvm_pmu_software_increment(vcpu, p->regval & mask);
> +	return true;
>  }
>  
>  static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> -- 
> 2.11.0
> 

[PATCH v2 5/9] arm64: KVM: PMU: Inject UNDEF on read access to PMSWINC_EL0

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:41PM +0100, Marc Zyngier wrote:
> PMSWINC_EL0 is a WO register, so let's UNDEF when reading from it
> (in the highly hypothetical case where this doesn't UNDEF at EL1).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

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

> ---
>  arch/arm64/kvm/sys_regs.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 19a036b4f6ac..f80a61af5e88 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -772,16 +772,15 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	if (!kvm_arm_pmu_v3_ready(vcpu))
>  		return trap_raz_wi(vcpu, p, r);
>  
> +	if (!p->is_write)
> +		return read_from_write_only(vcpu, p);
> +
>  	if (pmu_write_swinc_el0_disabled(vcpu))
>  		return false;
>  
> -	if (p->is_write) {
> -		mask = kvm_pmu_valid_counter_mask(vcpu);
> -		kvm_pmu_software_increment(vcpu, p->regval & mask);
> -		return true;
> -	}
> -
> -	return false;
> +	mask = kvm_pmu_valid_counter_mask(vcpu);
> +	kvm_pmu_software_increment(vcpu, p->regval & mask);
> +	return true;
>  }
>  
>  static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> -- 
> 2.11.0
> 

Re: [PATCH v2 6/9] arm64: KVM: Treat sysreg accessors returning false as successful

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:42PM +0100, Marc Zyngier wrote:
> Instead of considering that a sysreg accessor has failed when
> returning false, let's consider that it is *always* successful
> (after all, we won't stand for an incomplete emulation).

That's right!

> 
> The return value now simply indicates whether we should skip
> the instruction (because it has now been emulated), or if we
> should leave the PC alone if the emulation has injected an
> exception.

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

(I especially enjoy the much cleaner flow of emulate_sys_reg())

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 49 +++++++++++++++++++----------------------------
>  1 file changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f80a61af5e88..4e5d4eee8cec 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1571,6 +1571,22 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> +static void perform_access(struct kvm_vcpu *vcpu,
> +			   struct sys_reg_params *params,
> +			   const struct sys_reg_desc *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);
> +
> +	/* Skip instruction if instructed so */
> +	if (likely(r->access(vcpu, params, r)))
> +		kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +}
> +
>  /*
>   * emulate_cp --  tries to match a sys_reg access in a handling table, and
>   *                call the corresponding trap handler.
> @@ -1594,20 +1610,8 @@ 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))) {
> -			/* Skip instruction, since it was emulated */
> -			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> -			/* Handled */
> -			return 0;
> -		}
> +		perform_access(vcpu, params, r);
> +		return 0;
>  	}
>  
>  	/* Not handled */
> @@ -1777,26 +1781,13 @@ 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))) {
> -			/* 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. */
> +		perform_access(vcpu, params, r);
>  	} else {
>  		kvm_err("Unsupported guest sys_reg access at: %lx\n",
>  			*vcpu_pc(vcpu));
>  		print_sys_reg_instr(params);
> +		kvm_inject_undefined(vcpu);
>  	}
> -	kvm_inject_undefined(vcpu);
>  	return 1;
>  }
>  
> -- 
> 2.11.0
> 

[PATCH v2 6/9] arm64: KVM: Treat sysreg accessors returning false as successful

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:42PM +0100, Marc Zyngier wrote:
> Instead of considering that a sysreg accessor has failed when
> returning false, let's consider that it is *always* successful
> (after all, we won't stand for an incomplete emulation).

That's right!

> 
> The return value now simply indicates whether we should skip
> the instruction (because it has now been emulated), or if we
> should leave the PC alone if the emulation has injected an
> exception.

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

(I especially enjoy the much cleaner flow of emulate_sys_reg())

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 49 +++++++++++++++++++----------------------------
>  1 file changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f80a61af5e88..4e5d4eee8cec 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1571,6 +1571,22 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> +static void perform_access(struct kvm_vcpu *vcpu,
> +			   struct sys_reg_params *params,
> +			   const struct sys_reg_desc *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);
> +
> +	/* Skip instruction if instructed so */
> +	if (likely(r->access(vcpu, params, r)))
> +		kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +}
> +
>  /*
>   * emulate_cp --  tries to match a sys_reg access in a handling table, and
>   *                call the corresponding trap handler.
> @@ -1594,20 +1610,8 @@ 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))) {
> -			/* Skip instruction, since it was emulated */
> -			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> -			/* Handled */
> -			return 0;
> -		}
> +		perform_access(vcpu, params, r);
> +		return 0;
>  	}
>  
>  	/* Not handled */
> @@ -1777,26 +1781,13 @@ 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))) {
> -			/* 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. */
> +		perform_access(vcpu, params, r);
>  	} else {
>  		kvm_err("Unsupported guest sys_reg access at: %lx\n",
>  			*vcpu_pc(vcpu));
>  		print_sys_reg_instr(params);
> +		kvm_inject_undefined(vcpu);
>  	}
> -	kvm_inject_undefined(vcpu);
>  	return 1;
>  }
>  
> -- 
> 2.11.0
> 

Re: [PATCH v2 7/9] arm64: KVM: Do not corrupt registers on failed 64bit CP read

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:43PM +0100, Marc Zyngier wrote:
> 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 4e5d4eee8cec..1080a76e960f 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1678,20 +1678,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)) {

super nit: I choked a bit on this contruct, any objections to adding a
comment like the following above:

	/*
	 * Try to emulate the coprocessor access using the target
	 * specific table first, and using the global table aftwards.
	 * If either of the tables contains a handler, handle the
	 * potential register operation in the case of a read and return
	 * with success.
	 */

Too much?

(If not, I can also add this when applying).

> +		/* 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
> 

Otherwise:

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

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

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:43PM +0100, Marc Zyngier wrote:
> 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 4e5d4eee8cec..1080a76e960f 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1678,20 +1678,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)) {

super nit: I choked a bit on this contruct, any objections to adding a
comment like the following above:

	/*
	 * Try to emulate the coprocessor access using the target
	 * specific table first, and using the global table aftwards.
	 * If either of the tables contains a handler, handle the
	 * potential register operation in the case of a read and return
	 * with success.
	 */

Too much?

(If not, I can also add this when applying).

> +		/* 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
> 

Otherwise:

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

Re: [PATCH v2 8/9] arm: KVM: Make unexpected register accesses inject an undef

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:44PM +0100, Marc Zyngier wrote:
> Reads from write-only system registers are generally confined to
> EL1 and not propagated to EL2 (that's what the architecture
> mantates). In order to be sure that we have a sane behaviour
> even in the unlikely event that we have a broken system, we still
> handle it in KVM. Same goes for write to RO registers.
> 
> In that case, let's inject an undef into the guest.
> 

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

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c | 18 ++++++++++++++++++
>  arch/arm/kvm/coproc.h | 18 ------------------
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 3e5e4194ef86..519aac12b365 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -40,6 +40,24 @@
>   * Co-processor emulation
>   *****************************************************************************/
>  
> +static bool write_to_read_only(struct kvm_vcpu *vcpu,
> +			       const struct coproc_params *params)
> +{
> +	WARN_ONCE(1, "CP15 write to read-only register\n");
> +	print_cp_instr(params);
> +	kvm_inject_undefined(vcpu);
> +	return false;
> +}
> +
> +static bool read_from_write_only(struct kvm_vcpu *vcpu,
> +				 const struct coproc_params *params)
> +{
> +	WARN_ONCE(1, "CP15 read to write-only register\n");
> +	print_cp_instr(params);
> +	kvm_inject_undefined(vcpu);
> +	return false;
> +}
> +
>  /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
>  static u32 cache_levels;
>  
> diff --git a/arch/arm/kvm/coproc.h b/arch/arm/kvm/coproc.h
> index eef1759c2b65..3a41b7d1eb86 100644
> --- a/arch/arm/kvm/coproc.h
> +++ b/arch/arm/kvm/coproc.h
> @@ -81,24 +81,6 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> -static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
> -				      const struct coproc_params *params)
> -{
> -	kvm_debug("CP15 write to read-only register at: %08lx\n",
> -		  *vcpu_pc(vcpu));
> -	print_cp_instr(params);
> -	return false;
> -}
> -
> -static inline bool read_from_write_only(struct kvm_vcpu *vcpu,
> -					const struct coproc_params *params)
> -{
> -	kvm_debug("CP15 read to write-only register at: %08lx\n",
> -		  *vcpu_pc(vcpu));
> -	print_cp_instr(params);
> -	return false;
> -}
> -
>  /* Reset functions */
>  static inline void reset_unknown(struct kvm_vcpu *vcpu,
>  				 const struct coproc_reg *r)
> -- 
> 2.11.0
> 

[PATCH v2 8/9] arm: KVM: Make unexpected register accesses inject an undef

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:44PM +0100, Marc Zyngier wrote:
> Reads from write-only system registers are generally confined to
> EL1 and not propagated to EL2 (that's what the architecture
> mantates). In order to be sure that we have a sane behaviour
> even in the unlikely event that we have a broken system, we still
> handle it in KVM. Same goes for write to RO registers.
> 
> In that case, let's inject an undef into the guest.
> 

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

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c | 18 ++++++++++++++++++
>  arch/arm/kvm/coproc.h | 18 ------------------
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 3e5e4194ef86..519aac12b365 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -40,6 +40,24 @@
>   * Co-processor emulation
>   *****************************************************************************/
>  
> +static bool write_to_read_only(struct kvm_vcpu *vcpu,
> +			       const struct coproc_params *params)
> +{
> +	WARN_ONCE(1, "CP15 write to read-only register\n");
> +	print_cp_instr(params);
> +	kvm_inject_undefined(vcpu);
> +	return false;
> +}
> +
> +static bool read_from_write_only(struct kvm_vcpu *vcpu,
> +				 const struct coproc_params *params)
> +{
> +	WARN_ONCE(1, "CP15 read to write-only register\n");
> +	print_cp_instr(params);
> +	kvm_inject_undefined(vcpu);
> +	return false;
> +}
> +
>  /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
>  static u32 cache_levels;
>  
> diff --git a/arch/arm/kvm/coproc.h b/arch/arm/kvm/coproc.h
> index eef1759c2b65..3a41b7d1eb86 100644
> --- a/arch/arm/kvm/coproc.h
> +++ b/arch/arm/kvm/coproc.h
> @@ -81,24 +81,6 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> -static inline bool write_to_read_only(struct kvm_vcpu *vcpu,
> -				      const struct coproc_params *params)
> -{
> -	kvm_debug("CP15 write to read-only register at: %08lx\n",
> -		  *vcpu_pc(vcpu));
> -	print_cp_instr(params);
> -	return false;
> -}
> -
> -static inline bool read_from_write_only(struct kvm_vcpu *vcpu,
> -					const struct coproc_params *params)
> -{
> -	kvm_debug("CP15 read to write-only register at: %08lx\n",
> -		  *vcpu_pc(vcpu));
> -	print_cp_instr(params);
> -	return false;
> -}
> -
>  /* Reset functions */
>  static inline void reset_unknown(struct kvm_vcpu *vcpu,
>  				 const struct coproc_reg *r)
> -- 
> 2.11.0
> 

Re: [PATCH v2 9/9] arm: KVM: Treat CP15 accessors returning false as successful

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:45PM +0100, Marc Zyngier wrote:
> Instead of considering that a CP15 accessor has failed when
> returning false, let's consider that it is *always* successful
> (after all, we won't stand for an incomplete emulation).
> 
> The return value now simply indicates whether we should skip
> the instruction (because it has now been emulated), or if we
> should leave the PC alone if the emulation has injected an
> exception.
> 

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

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 519aac12b365..2c14b69511e9 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -520,15 +520,15 @@ static int emulate_cp15(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));
> -			return 1;
>  		}
> -		/* If access function fails, it should complain. */
>  	} else {
> +		/* If access function fails, it should complain. */
>  		kvm_err("Unsupported guest CP15 access at: %08lx\n",
>  			*vcpu_pc(vcpu));
>  		print_cp_instr(params);
> +		kvm_inject_undefined(vcpu);
>  	}
> -	kvm_inject_undefined(vcpu);
> +
>  	return 1;
>  }
>  
> -- 
> 2.11.0
> 

[PATCH v2 9/9] arm: KVM: Treat CP15 accessors returning false as successful

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:45PM +0100, Marc Zyngier wrote:
> Instead of considering that a CP15 accessor has failed when
> returning false, let's consider that it is *always* successful
> (after all, we won't stand for an incomplete emulation).
> 
> The return value now simply indicates whether we should skip
> the instruction (because it has now been emulated), or if we
> should leave the PC alone if the emulation has injected an
> exception.
> 

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

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 519aac12b365..2c14b69511e9 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -520,15 +520,15 @@ static int emulate_cp15(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));
> -			return 1;
>  		}
> -		/* If access function fails, it should complain. */
>  	} else {
> +		/* If access function fails, it should complain. */
>  		kvm_err("Unsupported guest CP15 access at: %08lx\n",
>  			*vcpu_pc(vcpu));
>  		print_cp_instr(params);
> +		kvm_inject_undefined(vcpu);
>  	}
> -	kvm_inject_undefined(vcpu);
> +
>  	return 1;
>  }
>  
> -- 
> 2.11.0
> 

Re: [PATCH v2 1/9] arm64: KVM: PMU: Refactor pmu_*_el0_disabled

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:37PM +0100, Marc Zyngier wrote:
> 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 0e26f8c2b56f..7e1d673304d5 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);

nit: I would call this variable 'enabled' and then return !enabled to
make it clear what's going on.

(If you agree, I can fix this up when applying along with the typo and
rename pointed out by Suzuki).

>  
> -	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
> 

Otherwise:

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

[PATCH v2 1/9] arm64: KVM: PMU: Refactor pmu_*_el0_disabled

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:37PM +0100, Marc Zyngier wrote:
> 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 0e26f8c2b56f..7e1d673304d5 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);

nit: I would call this variable 'enabled' and then return !enabled to
make it clear what's going on.

(If you agree, I can fix this up when applying along with the typo and
rename pointed out by Suzuki).

>  
> -	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
> 

Otherwise:

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

Re: [PATCH v2 1/9] arm64: KVM: PMU: Refactor pmu_*_el0_disabled

[Marc Zyngier]

On 28/03/17 13:46, Christoffer Dall wrote:
> On Mon, Mar 27, 2017 at 05:03:37PM +0100, Marc Zyngier wrote:
>> 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 0e26f8c2b56f..7e1d673304d5 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);
> 
> nit: I would call this variable 'enabled' and then return !enabled to
> make it clear what's going on.
> 
> (If you agree, I can fix this up when applying along with the typo and
> rename pointed out by Suzuki).

Yup, that'd be absolutely fine.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v2 1/9] arm64: KVM: PMU: Refactor pmu_*_el0_disabled

[Marc Zyngier]

On 28/03/17 13:46, Christoffer Dall wrote:
> On Mon, Mar 27, 2017 at 05:03:37PM +0100, Marc Zyngier wrote:
>> 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 0e26f8c2b56f..7e1d673304d5 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);
> 
> nit: I would call this variable 'enabled' and then return !enabled to
> make it clear what's going on.
> 
> (If you agree, I can fix this up when applying along with the typo and
> rename pointed out by Suzuki).

Yup, that'd be absolutely fine.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 7/9] arm64: KVM: Do not corrupt registers on failed 64bit CP read

[Marc Zyngier]

On 28/03/17 13:46, Christoffer Dall wrote:
> On Mon, Mar 27, 2017 at 05:03:43PM +0100, Marc Zyngier wrote:
>> 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 4e5d4eee8cec..1080a76e960f 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1678,20 +1678,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)) {
> 
> super nit: I choked a bit on this contruct, any objections to adding a
> comment like the following above:
> 
> 	/*
> 	 * Try to emulate the coprocessor access using the target
> 	 * specific table first, and using the global table aftwards.
> 	 * If either of the tables contains a handler, handle the
> 	 * potential register operation in the case of a read and return
> 	 * with success.
> 	 */
> 
> Too much?
> 
> (If not, I can also add this when applying).

No, that's great. Thanks!

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 28/03/17 13:46, Christoffer Dall wrote:
> On Mon, Mar 27, 2017 at 05:03:43PM +0100, Marc Zyngier wrote:
>> 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 4e5d4eee8cec..1080a76e960f 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1678,20 +1678,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)) {
> 
> super nit: I choked a bit on this contruct, any objections to adding a
> comment like the following above:
> 
> 	/*
> 	 * Try to emulate the coprocessor access using the target
> 	 * specific table first, and using the global table aftwards.
> 	 * If either of the tables contains a handler, handle the
> 	 * potential register operation in the case of a read and return
> 	 * with success.
> 	 */
> 
> Too much?
> 
> (If not, I can also add this when applying).

No, that's great. Thanks!

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 0/9] arm64: KVM: Fix PMU exception generation

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:36PM +0100, Marc Zyngier wrote:
> Running the following code:
> 
> root@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@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:
> - 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 is applied to the 32bit kernel, except that
> we don't ever inject an exception there (no PMU support yet).
> 

I have applied this series to queue (not next since I haven't tested
thoroughly yet), with the fixups agreed in the series.

Thanks,
-Christoffer

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

[Christoffer Dall]

On Mon, Mar 27, 2017 at 05:03:36PM +0100, 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:
> - 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 is applied to the 32bit kernel, except that
> we don't ever inject an exception there (no PMU support yet).
> 

I have applied this series to queue (not next since I haven't tested
thoroughly yet), with the fixups agreed in the series.

Thanks,
-Christoffer