[PATCH v2 0/7] KVM: arm64: Use visibility hook to treat ID regs as RAZ

[PATCH v2 0/7] KVM: arm64: Use visibility hook to treat ID regs as RAZ

[Oliver Upton]

For reasons unknown, the Arm architecture defines the 64-bit views of
the 32-bit ID registers as UNKNOWN [1]. This combines poorly with the
fact that KVM unconditionally exposes these registers to userspace,
which could throw a wrench in migration between 64-bit only systems.

This series reworks KVM's definition of these registers to RAZ/WI with
the goal of providing consistent register values across 64-bit machines.

Patches 1-3 clean up the ID register accessors, taking advantage of the
fact that the generic accessors know how to handle RAZ.

Patches 4-6 start switch the handling of potentially nonzero AArch32 ID
registers to RAZ/WI. RAZ covers up the architecturally UNKNOWN values,
and WI allows for migration off of kernels that may provide garbage.
Note that hidden AArch32 ID registers continue to have RAZ behavior with
the additional expectation of invariance.

Lastly, patch 7 includes a small test for the issue.

Applies to 6.0-rc3. Tested with KVM selftests under the fast model w/
asymmetric 32 bit support and no 32 bit support whatsoever.

v1 -> v2:
 - Collect Reiji's r-b tags (thanks!)
 - Call sysreg_visible_as_raz() from read_id_reg() (Reiji)
 - Hoist sysreg_user_write_ignore() into kvm_sys_reg_set_user() (Reiji)

Oliver Upton (7):
  KVM: arm64: Use visibility hook to treat ID regs as RAZ
  KVM: arm64: Remove internal accessor helpers for id regs
  KVM: arm64: Drop raz parameter from read_id_reg()
  KVM: arm64: Spin off helper for calling visibility hook
  KVM: arm64: Add a visibility bit to ignore user writes
  KVM: arm64: Treat 32bit ID registers as RAZ/WI on 64bit-only system
  KVM: selftests: Add test for RAZ/WI AArch32 ID registers

 arch/arm64/kvm/sys_regs.c                     | 150 +++++++++---------
 arch/arm64/kvm/sys_regs.h                     |  24 ++-
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../kvm/aarch64/aarch64_only_id_regs.c        | 135 ++++++++++++++++
 5 files changed, 225 insertions(+), 86 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c


base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/7] KVM: arm64: Use visibility hook to treat ID regs as RAZ

[Oliver Upton]

The generic id reg accessors already handle RAZ registers by way of the
visibility hook. Add a visibility hook that returns REG_RAZ
unconditionally and throw out the RAZ specific accessors.

Reviewed-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/sys_regs.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 3234f50b8c4b..e18efb9211f0 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1145,6 +1145,12 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static unsigned int raz_visibility(const struct kvm_vcpu *vcpu,
+				   const struct sys_reg_desc *r)
+{
+	return REG_RAZ;
+}
+
 /* cpufeature ID register access trap handlers */
 
 static bool __access_id_reg(struct kvm_vcpu *vcpu,
@@ -1168,13 +1174,6 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
 	return __access_id_reg(vcpu, p, r, raz);
 }
 
-static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
-			      struct sys_reg_params *p,
-			      const struct sys_reg_desc *r)
-{
-	return __access_id_reg(vcpu, p, r, true);
-}
-
 /* Visibility overrides for SVE-specific control registers */
 static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 				   const struct sys_reg_desc *rd)
@@ -1262,12 +1261,6 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return __set_id_reg(vcpu, rd, val, raz);
 }
 
-static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
-			  u64 val)
-{
-	return __set_id_reg(vcpu, rd, val, true);
-}
-
 static int get_raz_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		       u64 *val)
 {
@@ -1374,9 +1367,10 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
  */
 #define ID_UNALLOCATED(crm, op2) {			\
 	Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2),	\
-	.access = access_raz_id_reg,			\
-	.get_user = get_raz_reg,			\
-	.set_user = set_raz_id_reg,			\
+	.access = access_id_reg,			\
+	.get_user = get_id_reg,				\
+	.set_user = set_id_reg,				\
+	.visibility = raz_visibility			\
 }
 
 /*
@@ -1386,9 +1380,10 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
  */
 #define ID_HIDDEN(name) {			\
 	SYS_DESC(SYS_##name),			\
-	.access = access_raz_id_reg,		\
-	.get_user = get_raz_reg,		\
-	.set_user = set_raz_id_reg,		\
+	.access = access_id_reg,		\
+	.get_user = get_id_reg,			\
+	.set_user = set_id_reg,			\
+	.visibility = raz_visibility,		\
 }
 
 /*
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/7] KVM: arm64: Remove internal accessor helpers for id regs

[Oliver Upton]

The internal accessors are only ever called once. Dump out their
contents in the caller.

No functional change intended.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/sys_regs.c | 46 ++++++++++-----------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e18efb9211f0..26210f3a0b27 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1153,25 +1153,17 @@ static unsigned int raz_visibility(const struct kvm_vcpu *vcpu,
 
 /* cpufeature ID register access trap handlers */
 
-static bool __access_id_reg(struct kvm_vcpu *vcpu,
-			    struct sys_reg_params *p,
-			    const struct sys_reg_desc *r,
-			    bool raz)
-{
-	if (p->is_write)
-		return write_to_read_only(vcpu, p, r);
-
-	p->regval = read_id_reg(vcpu, r, raz);
-	return true;
-}
-
 static bool access_id_reg(struct kvm_vcpu *vcpu,
 			  struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
 	bool raz = sysreg_visible_as_raz(vcpu, r);
 
-	return __access_id_reg(vcpu, p, r, raz);
+	if (p->is_write)
+		return write_to_read_only(vcpu, p, r);
+
+	p->regval = read_id_reg(vcpu, r, raz);
+	return true;
 }
 
 /* Visibility overrides for SVE-specific control registers */
@@ -1226,31 +1218,13 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
  * are stored, and for set_id_reg() we don't allow the effective value
  * to be changed.
  */
-static int __get_id_reg(const struct kvm_vcpu *vcpu,
-			const struct sys_reg_desc *rd, u64 *val,
-			bool raz)
-{
-	*val = read_id_reg(vcpu, rd, raz);
-	return 0;
-}
-
-static int __set_id_reg(const struct kvm_vcpu *vcpu,
-			const struct sys_reg_desc *rd, u64 val,
-			bool raz)
-{
-	/* This is what we mean by invariant: you can't change it. */
-	if (val != read_id_reg(vcpu, rd, raz))
-		return -EINVAL;
-
-	return 0;
-}
-
 static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      u64 *val)
 {
 	bool raz = sysreg_visible_as_raz(vcpu, rd);
 
-	return __get_id_reg(vcpu, rd, val, raz);
+	*val = read_id_reg(vcpu, rd, raz);
+	return 0;
 }
 
 static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
@@ -1258,7 +1232,11 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 {
 	bool raz = sysreg_visible_as_raz(vcpu, rd);
 
-	return __set_id_reg(vcpu, rd, val, raz);
+	/* This is what we mean by invariant: you can't change it. */
+	if (val != read_id_reg(vcpu, rd, raz))
+		return -EINVAL;
+
+	return 0;
 }
 
 static int get_raz_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/7] KVM: arm64: Drop raz parameter from read_id_reg()

[Oliver Upton]

There is no longer a need for caller-specified RAZ visibility. Hoist the
call to sysreg_visible_as_raz() into read_id_reg() and drop the
parameter.

No functional change intended.

Suggested-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/sys_regs.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 26210f3a0b27..0e20a311ea20 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1063,13 +1063,12 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
 }
 
 /* Read a sanitised cpufeature ID register by sys_reg_desc */
-static u64 read_id_reg(const struct kvm_vcpu *vcpu,
-		struct sys_reg_desc const *r, bool raz)
+static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
 {
 	u32 id = reg_to_encoding(r);
 	u64 val;
 
-	if (raz)
+	if (sysreg_visible_as_raz(vcpu, r))
 		return 0;
 
 	val = read_sanitised_ftr_reg(id);
@@ -1157,12 +1156,10 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
 			  struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
-	bool raz = sysreg_visible_as_raz(vcpu, r);
-
 	if (p->is_write)
 		return write_to_read_only(vcpu, p, r);
 
-	p->regval = read_id_reg(vcpu, r, raz);
+	p->regval = read_id_reg(vcpu, r);
 	return true;
 }
 
@@ -1199,7 +1196,7 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	/* We can only differ with CSV[23], and anything else is an error */
-	val ^= read_id_reg(vcpu, rd, false);
+	val ^= read_id_reg(vcpu, rd);
 	val &= ~((0xFUL << ID_AA64PFR0_CSV2_SHIFT) |
 		 (0xFUL << ID_AA64PFR0_CSV3_SHIFT));
 	if (val)
@@ -1221,19 +1218,15 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      u64 *val)
 {
-	bool raz = sysreg_visible_as_raz(vcpu, rd);
-
-	*val = read_id_reg(vcpu, rd, raz);
+	*val = read_id_reg(vcpu, rd);
 	return 0;
 }
 
 static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      u64 val)
 {
-	bool raz = sysreg_visible_as_raz(vcpu, rd);
-
 	/* This is what we mean by invariant: you can't change it. */
-	if (val != read_id_reg(vcpu, rd, raz))
+	if (val != read_id_reg(vcpu, rd))
 		return -EINVAL;
 
 	return 0;
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/7] KVM: arm64: Spin off helper for calling visibility hook

[Oliver Upton]

No functional change intended.

Reviewed-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/sys_regs.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index a8c4cc32eb9a..e78b51059622 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -136,22 +136,25 @@ static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r
 	__vcpu_sys_reg(vcpu, r->reg) = r->val;
 }
 
-static inline bool sysreg_hidden(const struct kvm_vcpu *vcpu,
-				 const struct sys_reg_desc *r)
+static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu,
+					     const struct sys_reg_desc *r)
 {
 	if (likely(!r->visibility))
-		return false;
+		return 0;
 
-	return r->visibility(vcpu, r) & REG_HIDDEN;
+	return r->visibility(vcpu, r);
+}
+
+static inline bool sysreg_hidden(const struct kvm_vcpu *vcpu,
+				 const struct sys_reg_desc *r)
+{
+	return sysreg_visibility(vcpu, r) & REG_HIDDEN;
 }
 
 static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu,
 					 const struct sys_reg_desc *r)
 {
-	if (likely(!r->visibility))
-		return false;
-
-	return r->visibility(vcpu, r) & REG_RAZ;
+	return sysreg_visibility(vcpu, r) & REG_RAZ;
 }
 
 static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/7] KVM: arm64: Add a visibility bit to ignore user writes

[Oliver Upton]

We're about to ignore writes to AArch32 ID registers on AArch64-only
systems. Add a bit to indicate a register is handled as write ignore
when accessed from userspace.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/sys_regs.c | 3 +++
 arch/arm64/kvm/sys_regs.h | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0e20a311ea20..6d0511247df4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2775,6 +2775,9 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
 	if (!r)
 		return -ENOENT;
 
+	if (sysreg_user_write_ignore(vcpu, r))
+		return 0;
+
 	if (r->set_user) {
 		ret = (r->set_user)(vcpu, r, val);
 	} else {
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index e78b51059622..e4ebb3a379fd 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -86,6 +86,7 @@ struct sys_reg_desc {
 
 #define REG_HIDDEN		(1 << 0) /* hidden from userspace and guest */
 #define REG_RAZ			(1 << 1) /* RAZ from userspace and guest */
+#define REG_USER_WI		(1 << 2) /* WI from userspace only */
 
 static __printf(2, 3)
 inline void print_sys_reg_msg(const struct sys_reg_params *p,
@@ -157,6 +158,12 @@ static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu,
 	return sysreg_visibility(vcpu, r) & REG_RAZ;
 }
 
+static inline bool sysreg_user_write_ignore(const struct kvm_vcpu *vcpu,
+					    const struct sys_reg_desc *r)
+{
+	return sysreg_visibility(vcpu, r) & REG_USER_WI;
+}
+
 static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
 			      const struct sys_reg_desc *i2)
 {
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/7] KVM: arm64: Treat 32bit ID registers as RAZ/WI on 64bit-only system

[Oliver Upton]

One of the oddities of the architecture is that the AArch64 views of the
AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any EL.
Nonetheless, KVM exposes these registers to userspace for the sake of
save/restore. It is possible that the UNKNOWN value could differ between
systems, leading to a rejected write from userspace.

Avoid the issue altogether by handling the AArch32 ID registers as
RAZ/WI when on an AArch64-only system.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/sys_regs.c | 63 ++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 6d0511247df4..9569772cf09a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1144,6 +1144,20 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
+				       const struct sys_reg_desc *r)
+{
+	/*
+	 * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any
+	 * EL. Promote to RAZ/WI in order to guarantee consistency between
+	 * systems.
+	 */
+	if (!kvm_supports_32bit_el0())
+		return REG_RAZ | REG_USER_WI;
+
+	return id_visibility(vcpu, r);
+}
+
 static unsigned int raz_visibility(const struct kvm_vcpu *vcpu,
 				   const struct sys_reg_desc *r)
 {
@@ -1331,6 +1345,15 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
 	.visibility = id_visibility,		\
 }
 
+/* sys_reg_desc initialiser for known cpufeature ID registers */
+#define AA32_ID_SANITISED(name) {		\
+	SYS_DESC(SYS_##name),			\
+	.access	= access_id_reg,		\
+	.get_user = get_id_reg,			\
+	.set_user = set_id_reg,			\
+	.visibility = aa32_id_visibility,	\
+}
+
 /*
  * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
  * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
@@ -1418,33 +1441,33 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	/* AArch64 mappings of the AArch32 ID registers */
 	/* CRm=1 */
-	ID_SANITISED(ID_PFR0_EL1),
-	ID_SANITISED(ID_PFR1_EL1),
-	ID_SANITISED(ID_DFR0_EL1),
+	AA32_ID_SANITISED(ID_PFR0_EL1),
+	AA32_ID_SANITISED(ID_PFR1_EL1),
+	AA32_ID_SANITISED(ID_DFR0_EL1),
 	ID_HIDDEN(ID_AFR0_EL1),
-	ID_SANITISED(ID_MMFR0_EL1),
-	ID_SANITISED(ID_MMFR1_EL1),
-	ID_SANITISED(ID_MMFR2_EL1),
-	ID_SANITISED(ID_MMFR3_EL1),
+	AA32_ID_SANITISED(ID_MMFR0_EL1),
+	AA32_ID_SANITISED(ID_MMFR1_EL1),
+	AA32_ID_SANITISED(ID_MMFR2_EL1),
+	AA32_ID_SANITISED(ID_MMFR3_EL1),
 
 	/* CRm=2 */
-	ID_SANITISED(ID_ISAR0_EL1),
-	ID_SANITISED(ID_ISAR1_EL1),
-	ID_SANITISED(ID_ISAR2_EL1),
-	ID_SANITISED(ID_ISAR3_EL1),
-	ID_SANITISED(ID_ISAR4_EL1),
-	ID_SANITISED(ID_ISAR5_EL1),
-	ID_SANITISED(ID_MMFR4_EL1),
-	ID_SANITISED(ID_ISAR6_EL1),
+	AA32_ID_SANITISED(ID_ISAR0_EL1),
+	AA32_ID_SANITISED(ID_ISAR1_EL1),
+	AA32_ID_SANITISED(ID_ISAR2_EL1),
+	AA32_ID_SANITISED(ID_ISAR3_EL1),
+	AA32_ID_SANITISED(ID_ISAR4_EL1),
+	AA32_ID_SANITISED(ID_ISAR5_EL1),
+	AA32_ID_SANITISED(ID_MMFR4_EL1),
+	AA32_ID_SANITISED(ID_ISAR6_EL1),
 
 	/* CRm=3 */
-	ID_SANITISED(MVFR0_EL1),
-	ID_SANITISED(MVFR1_EL1),
-	ID_SANITISED(MVFR2_EL1),
+	AA32_ID_SANITISED(MVFR0_EL1),
+	AA32_ID_SANITISED(MVFR1_EL1),
+	AA32_ID_SANITISED(MVFR2_EL1),
 	ID_UNALLOCATED(3,3),
-	ID_SANITISED(ID_PFR2_EL1),
+	AA32_ID_SANITISED(ID_PFR2_EL1),
 	ID_HIDDEN(ID_DFR1_EL1),
-	ID_SANITISED(ID_MMFR5_EL1),
+	AA32_ID_SANITISED(ID_MMFR5_EL1),
 	ID_UNALLOCATED(3,7),
 
 	/* AArch64 ID registers */
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/7] KVM: selftests: Add test for RAZ/WI AArch32 ID registers

[Oliver Upton]

Add a test to assert that KVM handles the AArch64 views of the AArch32
ID registers as RAZ/WI (writable only from userspace).

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../kvm/aarch64/aarch64_only_id_regs.c        | 135 ++++++++++++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index d625a3f83780..4331af62a982 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+/aarch64/aarch64_only_id_regs
 /aarch64/arch_timer
 /aarch64/debug-exceptions
 /aarch64/get-reg-list
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 4c122f1b1737..efe155259095 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -144,6 +144,7 @@ TEST_GEN_PROGS_x86_64 += system_counter_offset_test
 # Compiled outputs used by test targets
 TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
 
+TEST_GEN_PROGS_aarch64 += aarch64/aarch64_only_id_regs
 TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
 TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
diff --git a/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c b/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
new file mode 100644
index 000000000000..704a3e7524a8
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * aarch64_only_id_regs - Test for ID register behavior on AArch64-only systems
+ *
+ * Copyright (c) 2022 Google LLC.
+ *
+ * Test that KVM handles the AArch64 views of the AArch32 ID registers as RAZ
+ * and WI from userspace.
+ */
+
+#include <stdint.h>
+
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+
+#define BAD_ID_REG_VAL	0x1badc0deul
+
+#define GUEST_ASSERT_REG_RAZ(reg)	GUEST_ASSERT_EQ(read_sysreg_s(reg), 0)
+
+static void guest_main(void)
+{
+	GUEST_ASSERT_REG_RAZ(SYS_ID_PFR0_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_PFR1_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_DFR0_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR0_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR1_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR2_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR3_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR0_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR1_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR2_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR3_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR4_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR5_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR4_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR6_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_MVFR0_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_MVFR1_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_MVFR2_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_PFR2_EL1);
+	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR5_EL1);
+
+	GUEST_DONE();
+}
+
+static void test_guest_raz(struct kvm_vcpu *vcpu)
+{
+	struct ucall uc;
+
+	vcpu_run(vcpu);
+
+	switch (get_ucall(vcpu, &uc)) {
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+		break;
+	case UCALL_DONE:
+		break;
+	default:
+		TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
+	}
+}
+
+static uint64_t reg_ids[] = {
+	KVM_ARM64_SYS_REG(SYS_ID_PFR0_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_PFR1_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_DFR0_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_MMFR0_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_MMFR1_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_MMFR2_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_MMFR3_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_ISAR0_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_ISAR1_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_ISAR2_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_ISAR3_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_ISAR4_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_ISAR5_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_MMFR4_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_ISAR6_EL1),
+	KVM_ARM64_SYS_REG(SYS_MVFR0_EL1),
+	KVM_ARM64_SYS_REG(SYS_MVFR1_EL1),
+	KVM_ARM64_SYS_REG(SYS_MVFR2_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_PFR2_EL1),
+	KVM_ARM64_SYS_REG(SYS_ID_MMFR5_EL1),
+};
+
+static void test_user_raz_wi(struct kvm_vcpu *vcpu)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(reg_ids); i++) {
+		uint64_t reg_id = reg_ids[i];
+		uint64_t val;
+
+		vcpu_get_reg(vcpu, reg_id, &val);
+		ASSERT_EQ(val, 0);
+
+		/*
+		 * Expect the ioctl to succeed with no effect on the register
+		 * value.
+		 */
+		vcpu_set_reg(vcpu, reg_id, BAD_ID_REG_VAL);
+
+		vcpu_get_reg(vcpu, reg_id, &val);
+		ASSERT_EQ(val, 0);
+	}
+}
+
+static bool vcpu_aarch64_only(struct kvm_vcpu *vcpu)
+{
+	uint64_t val, el0;
+
+	vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), &val);
+
+	el0 = (val & ARM64_FEATURE_MASK(ID_AA64PFR0_EL0)) >> ID_AA64PFR0_EL0_SHIFT;
+	return el0 == ID_AA64PFR0_ELx_64BIT_ONLY;
+}
+
+int main(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_main);
+
+	TEST_REQUIRE(vcpu_aarch64_only(vcpu));
+
+	ucall_init(vm, NULL);
+
+	test_user_raz_wi(vcpu);
+	test_guest_raz(vcpu);
+
+	ucall_uninit(vm);
+	kvm_vm_free(vm);
+}
-- 
2.37.2.789.g6183377224-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/7] KVM: arm64: Use visibility hook to treat ID regs as RAZ

[Oliver Upton]

Lol, mess up my own copypasta:

On Fri, Sep 02, 2022 at 03:47:56PM +0000, Oliver Upton wrote:
> For reasons unknown, the Arm architecture defines the 64-bit views of
> the 32-bit ID registers as UNKNOWN [1]. This combines poorly with the
					^ on AArch64-only systems.

> fact that KVM unconditionally exposes these registers to userspace,
> which could throw a wrench in migration between 64-bit only systems.
> 
> This series reworks KVM's definition of these registers to RAZ/WI with
> the goal of providing consistent register values across 64-bit machines.
> 
> Patches 1-3 clean up the ID register accessors, taking advantage of the
> fact that the generic accessors know how to handle RAZ.
> 
> Patches 4-6 start switch the handling of potentially nonzero AArch32 ID
> registers to RAZ/WI. RAZ covers up the architecturally UNKNOWN values,
> and WI allows for migration off of kernels that may provide garbage.
> Note that hidden AArch32 ID registers continue to have RAZ behavior with
> the additional expectation of invariance.
> 
> Lastly, patch 7 includes a small test for the issue.
> 
> Applies to 6.0-rc3. Tested with KVM selftests under the fast model w/
> asymmetric 32 bit support and no 32 bit support whatsoever.

[1]: DDI0487H.a Table D12-2 'Instruction encodings for non-Debug System Register accesses'

v1: https://lore.kernel.org/kvmarm/20220817214818.3243383-1-oliver.upton@linux.dev/

--
Thanks,
Oliver

> v1 -> v2:
>  - Collect Reiji's r-b tags (thanks!)
>  - Call sysreg_visible_as_raz() from read_id_reg() (Reiji)
>  - Hoist sysreg_user_write_ignore() into kvm_sys_reg_set_user() (Reiji)
> 
> Oliver Upton (7):
>   KVM: arm64: Use visibility hook to treat ID regs as RAZ
>   KVM: arm64: Remove internal accessor helpers for id regs
>   KVM: arm64: Drop raz parameter from read_id_reg()
>   KVM: arm64: Spin off helper for calling visibility hook
>   KVM: arm64: Add a visibility bit to ignore user writes
>   KVM: arm64: Treat 32bit ID registers as RAZ/WI on 64bit-only system
>   KVM: selftests: Add test for RAZ/WI AArch32 ID registers
> 
>  arch/arm64/kvm/sys_regs.c                     | 150 +++++++++---------
>  arch/arm64/kvm/sys_regs.h                     |  24 ++-
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../kvm/aarch64/aarch64_only_id_regs.c        | 135 ++++++++++++++++
>  5 files changed, 225 insertions(+), 86 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
> 
> 
> base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
> -- 
> 2.37.2.789.g6183377224-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 7/7] KVM: selftests: Add test for RAZ/WI AArch32 ID registers

[Andrew Jones]

On Fri, Sep 02, 2022 at 03:48:03PM +0000, Oliver Upton wrote:
> Add a test to assert that KVM handles the AArch64 views of the AArch32
> ID registers as RAZ/WI (writable only from userspace).
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../kvm/aarch64/aarch64_only_id_regs.c        | 135 ++++++++++++++++++
>  3 files changed, 137 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index d625a3f83780..4331af62a982 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +/aarch64/aarch64_only_id_regs
>  /aarch64/arch_timer
>  /aarch64/debug-exceptions
>  /aarch64/get-reg-list
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 4c122f1b1737..efe155259095 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -144,6 +144,7 @@ TEST_GEN_PROGS_x86_64 += system_counter_offset_test
>  # Compiled outputs used by test targets
>  TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
>  
> +TEST_GEN_PROGS_aarch64 += aarch64/aarch64_only_id_regs
>  TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
>  TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
> diff --git a/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c b/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
> new file mode 100644
> index 000000000000..704a3e7524a8
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * aarch64_only_id_regs - Test for ID register behavior on AArch64-only systems
> + *
> + * Copyright (c) 2022 Google LLC.
> + *
> + * Test that KVM handles the AArch64 views of the AArch32 ID registers as RAZ
> + * and WI from userspace.
> + */
> +
> +#include <stdint.h>
> +
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "test_util.h"
> +
> +#define BAD_ID_REG_VAL	0x1badc0deul
> +
> +#define GUEST_ASSERT_REG_RAZ(reg)	GUEST_ASSERT_EQ(read_sysreg_s(reg), 0)
> +
> +static void guest_main(void)
> +{
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_PFR0_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_PFR1_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_DFR0_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR0_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR1_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR2_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR3_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR0_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR1_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR2_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR3_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR4_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR5_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR4_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_ISAR6_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_MVFR0_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_MVFR1_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_MVFR2_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_PFR2_EL1);
> +	GUEST_ASSERT_REG_RAZ(SYS_ID_MMFR5_EL1);
> +
> +	GUEST_DONE();
> +}
> +
> +static void test_guest_raz(struct kvm_vcpu *vcpu)
> +{
> +	struct ucall uc;
> +
> +	vcpu_run(vcpu);
> +
> +	switch (get_ucall(vcpu, &uc)) {
> +	case UCALL_ABORT:
> +		REPORT_GUEST_ASSERT(uc);
> +		break;
> +	case UCALL_DONE:
> +		break;
> +	default:
> +		TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
> +	}
> +}
> +
> +static uint64_t reg_ids[] = {
> +	KVM_ARM64_SYS_REG(SYS_ID_PFR0_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_PFR1_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_DFR0_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_MMFR0_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_MMFR1_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_MMFR2_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_MMFR3_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_ISAR0_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_ISAR1_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_ISAR2_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_ISAR3_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_ISAR4_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_ISAR5_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_MMFR4_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_ISAR6_EL1),
> +	KVM_ARM64_SYS_REG(SYS_MVFR0_EL1),
> +	KVM_ARM64_SYS_REG(SYS_MVFR1_EL1),
> +	KVM_ARM64_SYS_REG(SYS_MVFR2_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_PFR2_EL1),
> +	KVM_ARM64_SYS_REG(SYS_ID_MMFR5_EL1),

Hi Oliver,

I see all the hidden and unallocated registers have been filtered out of
the test lists. They should also behave as RAZ, right? Maybe we should
keep them in the lists here for consistency and to test them as well.

Thanks,
drew

> +};
> +
> +static void test_user_raz_wi(struct kvm_vcpu *vcpu)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(reg_ids); i++) {
> +		uint64_t reg_id = reg_ids[i];
> +		uint64_t val;
> +
> +		vcpu_get_reg(vcpu, reg_id, &val);
> +		ASSERT_EQ(val, 0);
> +
> +		/*
> +		 * Expect the ioctl to succeed with no effect on the register
> +		 * value.
> +		 */
> +		vcpu_set_reg(vcpu, reg_id, BAD_ID_REG_VAL);
> +
> +		vcpu_get_reg(vcpu, reg_id, &val);
> +		ASSERT_EQ(val, 0);
> +	}
> +}
> +
> +static bool vcpu_aarch64_only(struct kvm_vcpu *vcpu)
> +{
> +	uint64_t val, el0;
> +
> +	vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), &val);
> +
> +	el0 = (val & ARM64_FEATURE_MASK(ID_AA64PFR0_EL0)) >> ID_AA64PFR0_EL0_SHIFT;
> +	return el0 == ID_AA64PFR0_ELx_64BIT_ONLY;
> +}
> +
> +int main(void)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_main);
> +
> +	TEST_REQUIRE(vcpu_aarch64_only(vcpu));
> +
> +	ucall_init(vm, NULL);
> +
> +	test_user_raz_wi(vcpu);
> +	test_guest_raz(vcpu);
> +
> +	ucall_uninit(vm);
> +	kvm_vm_free(vm);
> +}
> -- 
> 2.37.2.789.g6183377224-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 7/7] KVM: selftests: Add test for RAZ/WI AArch32 ID registers

[Oliver Upton]

Hi Drew,

On Mon, Sep 05, 2022 at 09:31:40AM +0200, Andrew Jones wrote:

[...]

> > +static uint64_t reg_ids[] = {
> > +	KVM_ARM64_SYS_REG(SYS_ID_PFR0_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_PFR1_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_DFR0_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_MMFR0_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_MMFR1_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_MMFR2_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_MMFR3_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_ISAR0_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_ISAR1_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_ISAR2_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_ISAR3_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_ISAR4_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_ISAR5_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_MMFR4_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_ISAR6_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_MVFR0_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_MVFR1_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_MVFR2_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_PFR2_EL1),
> > +	KVM_ARM64_SYS_REG(SYS_ID_MMFR5_EL1),
> 
> Hi Oliver,
> 
> I see all the hidden and unallocated registers have been filtered out of
> the test lists. They should also behave as RAZ, right? Maybe we should
> keep them in the lists here for consistency and to test them as well.

Sure, can do. The reason I only tested these registers is because they
have RAZ/WI behavior with this series, whereas the rest are RAZ +
invariant. Should be easy enough to cover the whole range, though.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] KVM: arm64: Remove internal accessor helpers for id regs

[Reiji Watanabe]

On Fri, Sep 2, 2022 at 8:48 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> The internal accessors are only ever called once. Dump out their
> contents in the caller.
>
> No functional change intended.
>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] KVM: arm64: Drop raz parameter from read_id_reg()

[Reiji Watanabe]

On Fri, Sep 2, 2022 at 8:48 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> There is no longer a need for caller-specified RAZ visibility. Hoist the
> call to sysreg_visible_as_raz() into read_id_reg() and drop the
> parameter.
>
> No functional change intended.
>
> Suggested-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/7] KVM: arm64: Add a visibility bit to ignore user writes

[Reiji Watanabe]

On Fri, Sep 2, 2022 at 8:48 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> We're about to ignore writes to AArch32 ID registers on AArch64-only
> systems. Add a bit to indicate a register is handled as write ignore
> when accessed from userspace.
>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/7] KVM: arm64: Treat 32bit ID registers as RAZ/WI on 64bit-only system

[Reiji Watanabe]

Hi Oliver,

On Fri, Sep 2, 2022 at 8:48 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> One of the oddities of the architecture is that the AArch64 views of the
> AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any EL.
> Nonetheless, KVM exposes these registers to userspace for the sake of
> save/restore. It is possible that the UNKNOWN value could differ between
> systems, leading to a rejected write from userspace.
>
> Avoid the issue altogether by handling the AArch32 ID registers as
> RAZ/WI when on an AArch64-only system.
>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/sys_regs.c | 63 ++++++++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 6d0511247df4..9569772cf09a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1144,6 +1144,20 @@ static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
>         return 0;
>  }
>
> +static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
> +                                      const struct sys_reg_desc *r)
> +{
> +       /*
> +        * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any
> +        * EL. Promote to RAZ/WI in order to guarantee consistency between
> +        * systems.
> +        */
> +       if (!kvm_supports_32bit_el0())
> +               return REG_RAZ | REG_USER_WI;
> +
> +       return id_visibility(vcpu, r);
> +}
> +
>  static unsigned int raz_visibility(const struct kvm_vcpu *vcpu,
>                                    const struct sys_reg_desc *r)
>  {
> @@ -1331,6 +1345,15 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>         .visibility = id_visibility,            \
>  }
>
> +/* sys_reg_desc initialiser for known cpufeature ID registers */
> +#define AA32_ID_SANITISED(name) {              \
> +       SYS_DESC(SYS_##name),                   \
> +       .access = access_id_reg,                \
> +       .get_user = get_id_reg,                 \
> +       .set_user = set_id_reg,                 \
> +       .visibility = aa32_id_visibility,       \
> +}
> +
>  /*
>   * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
>   * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
> @@ -1418,33 +1441,33 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>
>         /* AArch64 mappings of the AArch32 ID registers */
>         /* CRm=1 */
> -       ID_SANITISED(ID_PFR0_EL1),
> -       ID_SANITISED(ID_PFR1_EL1),
> -       ID_SANITISED(ID_DFR0_EL1),
> +       AA32_ID_SANITISED(ID_PFR0_EL1),
> +       AA32_ID_SANITISED(ID_PFR1_EL1),
> +       AA32_ID_SANITISED(ID_DFR0_EL1),
>         ID_HIDDEN(ID_AFR0_EL1),
> -       ID_SANITISED(ID_MMFR0_EL1),
> -       ID_SANITISED(ID_MMFR1_EL1),
> -       ID_SANITISED(ID_MMFR2_EL1),
> -       ID_SANITISED(ID_MMFR3_EL1),
> +       AA32_ID_SANITISED(ID_MMFR0_EL1),
> +       AA32_ID_SANITISED(ID_MMFR1_EL1),
> +       AA32_ID_SANITISED(ID_MMFR2_EL1),
> +       AA32_ID_SANITISED(ID_MMFR3_EL1),
>
>         /* CRm=2 */
> -       ID_SANITISED(ID_ISAR0_EL1),
> -       ID_SANITISED(ID_ISAR1_EL1),
> -       ID_SANITISED(ID_ISAR2_EL1),
> -       ID_SANITISED(ID_ISAR3_EL1),
> -       ID_SANITISED(ID_ISAR4_EL1),
> -       ID_SANITISED(ID_ISAR5_EL1),
> -       ID_SANITISED(ID_MMFR4_EL1),
> -       ID_SANITISED(ID_ISAR6_EL1),
> +       AA32_ID_SANITISED(ID_ISAR0_EL1),
> +       AA32_ID_SANITISED(ID_ISAR1_EL1),
> +       AA32_ID_SANITISED(ID_ISAR2_EL1),
> +       AA32_ID_SANITISED(ID_ISAR3_EL1),
> +       AA32_ID_SANITISED(ID_ISAR4_EL1),
> +       AA32_ID_SANITISED(ID_ISAR5_EL1),
> +       AA32_ID_SANITISED(ID_MMFR4_EL1),
> +       AA32_ID_SANITISED(ID_ISAR6_EL1),
>
>         /* CRm=3 */
> -       ID_SANITISED(MVFR0_EL1),
> -       ID_SANITISED(MVFR1_EL1),
> -       ID_SANITISED(MVFR2_EL1),
> +       AA32_ID_SANITISED(MVFR0_EL1),
> +       AA32_ID_SANITISED(MVFR1_EL1),
> +       AA32_ID_SANITISED(MVFR2_EL1),
>         ID_UNALLOCATED(3,3),
> -       ID_SANITISED(ID_PFR2_EL1),
> +       AA32_ID_SANITISED(ID_PFR2_EL1),
>         ID_HIDDEN(ID_DFR1_EL1),

Perhaps it might be better to handle ID_AFR0_EL1 and ID_DFR1_EL1
in the same way as the other AArch32 ID registers for consistency ?
(i.e. treat them RAZ/USER_WI instead of RAZ if kvm_supports_32bit_el0()
 is false instead of RAZ)

Thank you,
Reiji


> -       ID_SANITISED(ID_MMFR5_EL1),
> +       AA32_ID_SANITISED(ID_MMFR5_EL1),
>         ID_UNALLOCATED(3,7),
>
>         /* AArch64 ID registers */
> --
> 2.37.2.789.g6183377224-goog
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/7] KVM: arm64: Treat 32bit ID registers as RAZ/WI on 64bit-only system

[Oliver Upton]

Hi Reiji,

On Tue, Sep 06, 2022 at 09:52:53PM -0700, Reiji Watanabe wrote:

[...]

> >         /* CRm=3 */
> > -       ID_SANITISED(MVFR0_EL1),
> > -       ID_SANITISED(MVFR1_EL1),
> > -       ID_SANITISED(MVFR2_EL1),
> > +       AA32_ID_SANITISED(MVFR0_EL1),
> > +       AA32_ID_SANITISED(MVFR1_EL1),
> > +       AA32_ID_SANITISED(MVFR2_EL1),
> >         ID_UNALLOCATED(3,3),
> > -       ID_SANITISED(ID_PFR2_EL1),
> > +       AA32_ID_SANITISED(ID_PFR2_EL1),
> >         ID_HIDDEN(ID_DFR1_EL1),
> 
> Perhaps it might be better to handle ID_AFR0_EL1 and ID_DFR1_EL1
> in the same way as the other AArch32 ID registers for consistency ?
> (i.e. treat them RAZ/USER_WI instead of RAZ if kvm_supports_32bit_el0()
>  is false instead of RAZ)

Thanks for having a look. I stopped short of treating these registers as
RAZ/USER_WI since an attempted nonzero write to either of these
registers is a userspace bug (KVM always advertised 0).

As the ABI isn't busted for these registers I'd prefer to leave it in
place. Having said that, I'm not too strongly motivated in either
direction.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel