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

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

[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-2 clean up some of the ID register accessors, taking advantage
of the fact that the generic ones already know how to handle RAZ
registers.

Patches 3-4 wire in a new visibility bit to indicate a register ignores
writes from userspace.

Patch 5 moves all exposed 32-bit ID registers to have RAZ/WI behavior on
64-bit only systems. Note that hidden 32-bit registers continue to have
RAZ behavior and carry the additional requirement of invariance.

Lastly, patch 6 tests that userspace and guest indeed see the registers
as RAZ/WI.

Applies to 6.0-rc1 + the mismatched system fixes [2] picked up earlier
today. Tested on the fast model, both with mismatched AArch32 support
and no AArch32 support whatoever.

[1]: DDI0487H.a Table D12-2 'Instruction encodings for non-Debug System Register accesses'
[2]: https://lore.kernel.org/kvmarm/20220816192554.1455559-1-oliver.upton@linux.dev/

Oliver Upton (6):
  KVM: arm64: Use visibility hook to treat ID regs as RAZ
  KVM: arm64: Remove internal accessor helpers for id regs
  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                     | 137 +++++++++---------
 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, 222 insertions(+), 76 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/aarch64_only_id_regs.c

-- 
2.37.1.595.g718a3a8f04-goog


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

[PATCH 1/6] 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.

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.1.595.g718a3a8f04-goog


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

[PATCH 2/6] 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.1.595.g718a3a8f04-goog


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

[PATCH 3/6] KVM: arm64: Spin off helper for calling visibility hook

[Oliver Upton]

No functional change intended.

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.1.595.g718a3a8f04-goog


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

[PATCH 4/6] 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 26210f3a0b27..9f06c85f26b8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1232,6 +1232,9 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 {
 	bool raz = sysreg_visible_as_raz(vcpu, rd);
 
+	if (sysreg_user_write_ignore(vcpu, rd))
+		return 0;
+
 	/* This is what we mean by invariant: you can't change it. */
 	if (val != read_id_reg(vcpu, rd, raz))
 		return -EINVAL;
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.1.595.g718a3a8f04-goog


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

[PATCH 5/6] 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 9f06c85f26b8..5f6a633182c8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1145,6 +1145,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)
 {
@@ -1341,6 +1355,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
@@ -1428,33 +1451,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.1.595.g718a3a8f04-goog


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

[PATCH 6/6] 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..7589eb7fbb43
--- /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.1.595.g718a3a8f04-goog


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

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

[Marc Zyngier]

On Wed, 17 Aug 2022 22:48:17 +0100,
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 9f06c85f26b8..5f6a633182c8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1145,6 +1145,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;

This is probably only a nit, but why does one visibility has a _USER_
tag while the other doesn't? In other word, what sysregs are WI from
userspace that aren't so from the guest?

Also, do we have any cases where RAZ and WI would be used
independently? My gut feeling is that RAZ implies WI in most (all?)
cases. If this assumption holds, shouldn't we simply rename REG_RAZ to
REG_RAZ_WI and be done with it?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

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

[Oliver Upton]

Hey Marc,

Thanks for the review!

On Tue, Aug 23, 2022 at 06:05:28PM +0100, Marc Zyngier wrote:
> On Wed, 17 Aug 2022 22:48:17 +0100,
> 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 9f06c85f26b8..5f6a633182c8 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1145,6 +1145,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;
> 
> This is probably only a nit, but why does one visibility has a _USER_
> tag while the other doesn't? In other word, what sysregs are WI from
> userspace that aren't so from the guest?
> 
> Also, do we have any cases where RAZ and WI would be used
> independently? My gut feeling is that RAZ implies WI in most (all?)
> cases. If this assumption holds, shouldn't we simply rename REG_RAZ to
> REG_RAZ_WI and be done with it?

Yeah, this reads a bit strange, but there is some reason around it (I
think!)

As it applies to ID registers, REG_RAZ already implies RAZ w/ immutable
writes (-EINVAL if something different is written). As such I didn't want
to change the meaning of the other ID registers to WI and only ignore
writes for the registers that could have an UNKNOWN value. Furthermore,
I added the _USER_ tag to make it clear that we aren't magically allowing
writes from the guest to these registers.

I think we will need an additional visibility bit (or special accessor,
which I tried to avoid) to precisely apply WI to the 32bit registers,
but if the _USER_ tag is distracting I can get rid of it. After all,
hardware should politely UNDEF the guest when writing to such a
register.

Thoughts?

--
Thanks,
Oliver

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

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

[Reiji Watanabe]

On Wed, Aug 17, 2022 at 2:48 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> 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.
>
> 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 2/6] KVM: arm64: Remove internal accessor helpers for id regs

[Reiji Watanabe]

Hi Oliver,

On Wed, Aug 17, 2022 at 2:48 PM 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>
> ---
>  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;
>  }

I see no reason for read_id_reg() to take raz as an argument.
Perhaps having read_id_reg() call sysreg_visible_as_raz() instead
might make those functions even simpler?

Thank you,
Reiji

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

Re: [PATCH 3/6] KVM: arm64: Spin off helper for calling visibility hook

[Reiji Watanabe]

On Wed, Aug 17, 2022 at 2:48 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> No functional change intended.
>
> 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,

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

Thank you,
Reiji

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

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

[Oliver Upton]

Hi Reiji,

On Mon, Aug 29, 2022 at 10:45:09PM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> On Wed, Aug 17, 2022 at 2:48 PM 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>
> > ---
> >  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;
> >  }
> 
> I see no reason for read_id_reg() to take raz as an argument.
> Perhaps having read_id_reg() call sysreg_visible_as_raz() instead
> might make those functions even simpler?

Good point, as this patch has done away with caller-specified RAZ. I'll
incorporate that into v2.

--
Best,
Oliver

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

Re: [PATCH 4/6] KVM: arm64: Add a visibility bit to ignore user writes

[Reiji Watanabe]

Hi Oliver,

On Wed, Aug 17, 2022 at 2:48 PM 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>
> ---
>  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 26210f3a0b27..9f06c85f26b8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1232,6 +1232,9 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  {
>         bool raz = sysreg_visible_as_raz(vcpu, rd);
>
> +       if (sysreg_user_write_ignore(vcpu, rd))
> +               return 0;

Since the visibility flags are not ID register specific,
have you considered checking REG_USER_WI from kvm_sys_reg_set_user()
rather than the ID register specific function ?

This patch made me reconsider my comment for the patch-2.
Perhaps it might be more appropriate to check RAZ visibility from
kvm_sys_reg_get_user() rather than the ID register specific function ?

REG_HIDDEN is already checked from kvm_sys_reg_{s,g}et_user() (indirectly).

Thank you,
Reiji

> +
>         /* This is what we mean by invariant: you can't change it. */
>         if (val != read_id_reg(vcpu, rd, raz))
>                 return -EINVAL;
> 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.1.595.g718a3a8f04-goog
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

Re: [PATCH 4/6] KVM: arm64: Add a visibility bit to ignore user writes

[Oliver Upton]

On Tue, Aug 30, 2022 at 08:29:37PM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> On Wed, Aug 17, 2022 at 2:48 PM 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>
> > ---
> >  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 26210f3a0b27..9f06c85f26b8 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1232,6 +1232,9 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >  {
> >         bool raz = sysreg_visible_as_raz(vcpu, rd);
> >
> > +       if (sysreg_user_write_ignore(vcpu, rd))
> > +               return 0;
> 
> Since the visibility flags are not ID register specific,
> have you considered checking REG_USER_WI from kvm_sys_reg_set_user()
> rather than the ID register specific function ?

Yeah, that's definitely a better place to wire it in.

> This patch made me reconsider my comment for the patch-2.
> Perhaps it might be more appropriate to check RAZ visibility from
> kvm_sys_reg_get_user() rather than the ID register specific function ?

REG_RAZ hides the register value from the guest as well as userspace, so it
might be better to leave it in place. REG_RAZ also has implications for
writing a register from userspace, as we still apply the expectation of
invariance to ID registers that set this flag.

It all 'just works' right now with the check buried in the ID register
accessors. Going the other way around would require sprinkling the check
in several locations.

--
Thanks,
Oliver

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

Re: [PATCH 4/6] KVM: arm64: Add a visibility bit to ignore user writes

[Reiji Watanabe]

On Wed, Aug 31, 2022 at 7:42 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Tue, Aug 30, 2022 at 08:29:37PM -0700, Reiji Watanabe wrote:
> > Hi Oliver,
> >
> > On Wed, Aug 17, 2022 at 2:48 PM 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>
> > > ---
> > >  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 26210f3a0b27..9f06c85f26b8 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1232,6 +1232,9 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > >  {
> > >         bool raz = sysreg_visible_as_raz(vcpu, rd);
> > >
> > > +       if (sysreg_user_write_ignore(vcpu, rd))
> > > +               return 0;
> >
> > Since the visibility flags are not ID register specific,
> > have you considered checking REG_USER_WI from kvm_sys_reg_set_user()
> > rather than the ID register specific function ?
>
> Yeah, that's definitely a better place to wire it in.
>
> > This patch made me reconsider my comment for the patch-2.
> > Perhaps it might be more appropriate to check RAZ visibility from
> > kvm_sys_reg_get_user() rather than the ID register specific function ?
>
> REG_RAZ hides the register value from the guest as well as userspace, so it
> might be better to leave it in place. REG_RAZ also has implications for
> writing a register from userspace, as we still apply the expectation of
> invariance to ID registers that set this flag.
>
> It all 'just works' right now with the check buried in the ID register
> accessors. Going the other way around would require sprinkling the check
> in several locations.

Ah, I see the handling of REG_RAZ is a bit tricky...
I kind of suspect that REG_RAZ won't probably be used for any registers
other than ID registers even in the future...

Anyway, yes, it might be better to leave it in place at least for now.

Thank you,
Reiji

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