KVM/arm64: VM configuration enforcement

In order to make it easier to check whether a particular feature is exposed to a guest, add a new set of helpers, with kvm_has_feat() being the most useful.

Let's start making use of them in the PMU code (courtesy of Oliver). Follow-up work will intricude additional use patterns.

Co-developed--by Oliver Upton <[email protected]>

Signed-off-by Oliver Upton <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
  arch/arm64/include/asm/kvm_host.h | 53 +++++++++++++++++++++++++++++++
  arch/arm64/kvm/pmu-emul.c         | 11 ++++---
  arch/arm64/kvm/sys_regs.c         |  6 ++--
  include/kvm/arm_pmu.h             | 11 -------
  4 files changed, 61 insertions(+), 20 deletions(-)

arch/arm64/include/asm/kvm_host.h +0 -0

index 21c57b812569..c0cf9c5f5e8d 100644

arch/arm64/include/asm/kvm_host.h +5 -0

+									\
+		__val;							\
+	})
+
+#define expand_field_sign(id, fld, val)					\

arch/arm64/include/asm/kvm_host.h +22 -0

+									\
+		__val;							\
+	})
+
+#define get_idreg_field_enum(kvm, id, fld)				\
+	get_idreg_field_unsigned(kvm, id, fld)
+
+#define get_idreg_field(kvm, id, fld)					\
+	(id##_##fld##_SIGNED ?						\
+	 get_idreg_field_signed(kvm, id, fld) :				\
+	 get_idreg_field_unsigned(kvm, id, fld))
+
+#define kvm_has_feat(kvm, id, fld, limit)				\
+	(get_idreg_field((kvm), id, fld) >= expand_field_sign(id, fld, limit))
+
+#define kvm_has_feat_enum(kvm, id, fld, limit)				\
+	(get_idreg_field_unsigned((kvm), id, fld) == id##_##fld##_##limit)
+
+#define kvm_has_feat_range(kvm, id, fld, min, max)			\
+	(get_idreg_field((kvm), id, fld) >= expand_field_sign(id, fld, min) && \
+	 get_idreg_field((kvm), id, fld) <= expand_field_sign(id, fld, max))
+
  #endif /* __ARM64_KVM_HOST_H__ */

arch/arm64/kvm/pmu-emul.c +6 -5

index 3d9467ff73bc..925522470b2b 100644
@@ -64,12 +64,11 @@ u64 kvm_pmu_evtyper_mask(struct kvm *kvm)
  {
  	u64 mask = ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0 |
  		   kvm_pmu_event_mask(kvm);
-	u64 pfr0 = IDREG(kvm, SYS_ID_AA64PFR0_EL1);
 
-	if (SYS_FIELD_GET(ID_AA64PFR0_EL1, EL2, pfr0))
+	if (kvm_has_feat(kvm, ID_AA64PFR0_EL1, EL2, IMP))
  		mask |= ARMV8_PMU_INCLUDE_EL2;
 
-	if (SYS_FIELD_GET(ID_AA64PFR0_EL1, EL3, pfr0))
+	if (kvm_has_feat(kvm, ID_AA64PFR0_EL1, EL3, IMP))
  		mask |= ARMV8_PMU_EXCLUDE_NS_EL0 |
  			ARMV8_PMU_EXCLUDE_NS_EL1 |
  			ARMV8_PMU_EXCLUDE_EL3;

@@ -83,8 +82,10 @@ u64 kvm_pmu_evtyper_mask(struct kvm *kvm)
   */
  static bool kvm_pmc_is_64bit(struct kvm_pmc *pmc)
  {
+	struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
+
  	return (pmc->idx == ARMV8_PMU_CYCLE_IDX ||
-		kvm_pmu_is_3p5(kvm_pmc_to_vcpu(pmc)));
+		kvm_has_feat(vcpu->kvm, ID_AA64DFR0_EL1, PMUVer, V3P5));
  }
 
  static bool kvm_pmc_has_64bit_overflow(struct kvm_pmc *pmc)

@@ -556,7 +557,7 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
  		return;
 
  	/* Fixup PMCR_EL0 to reconcile the PMU version and the LP bit */
-	if (!kvm_pmu_is_3p5(vcpu))
+	if (!kvm_has_feat(vcpu->kvm, ID_AA64DFR0_EL1, PMUVer, V3P5))
  		val &= ~ARMV8_PMU_PMCR_LP;
 
  	/* The reset bits don't indicate any state, and shouldn't be saved. */

arch/arm64/kvm/sys_regs.c +2 -4

index 041b11825578..3c31f8cb9eef 100644
@@ -505,10 +505,9 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
  			  struct sys_reg_params *p,
  			  const struct sys_reg_desc *r)
  {
-	u64 val = IDREG(vcpu->kvm, SYS_ID_AA64MMFR1_EL1);
  	u32 sr = reg_to_encoding(r);
 
-	if (!(val & (0xfUL << ID_AA64MMFR1_EL1_LO_SHIFT))) {
+	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, LO, IMP)) {
  		kvm_inject_undefined(vcpu);
  		return false;
  	}

@@ -2748,8 +2747,7 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu,
  		return ignore_write(vcpu, p);
  	} else {
  		u64 dfr = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
-		u64 pfr = IDREG(vcpu->kvm, SYS_ID_AA64PFR0_EL1);
-		u32 el3 = !!SYS_FIELD_GET(ID_AA64PFR0_EL1, EL3, pfr);
+		u32 el3 = kvm_has_feat(vcpu->kvm, ID_AA64PFR0_EL1, EL3, IMP);
 
  		p->regval = ((SYS_FIELD_GET(ID_AA64DFR0_EL1, WRPs, dfr) << 28) |
  			     (SYS_FIELD_GET(ID_AA64DFR0_EL1, BRPs, dfr) << 24) |

include/kvm/arm_pmu.h +0 -11

index 4b9d8fb393a8..eb4c369a79eb 100644
@@ -90,16 +90,6 @@ void kvm_vcpu_pmu_resync_el0(void);
  			vcpu->arch.pmu.events = *kvm_get_pmu_events();	\
  	} while (0)
 
-/*
- * Evaluates as true when emulating PMUv3p5, and false otherwise.
- */
-#define kvm_pmu_is_3p5(vcpu) ({						\
-	u64 val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);		\
-	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);	\
-									\
-	pmuver >= ID_AA64DFR0_EL1_PMUVer_V3P5;				\
-})
-
  u8 kvm_arm_pmu_get_pmuver_limit(void);
  u64 kvm_pmu_evtyper_mask(struct kvm *kvm);
  int kvm_arm_set_default_pmu(struct kvm *kvm);

@@ -168,7 +158,6 @@ static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
  }
 
  #define kvm_vcpu_has_pmu(vcpu)		({ false; })
-#define kvm_pmu_is_3p5(vcpu)		({ false; })
  static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {}
  static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {}
  static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {}

Hi Marc,

Quoted 6 lines · Marc Zyngier

In order to make it easier to check whether a particular feature is exposed to a guest, add a new set of helpers, with kvm_has_feat() being the most useful.

Let's start making use of them in the PMU code (courtesy of Oliver). Follow-up work will intricude additional use patterns.

I think there is a bit of inconsistency in the macros for signed and unsigned. The unsigned fields are extracted (i.e., as if they were shifted to bit 0). But the signed fields are not shifted completely to bit '0' (in fact to different positions) and eventually we compare wrong things.

Using ID_AA64PFR0_EL1, fld=EL2, val=IMP for unsigned and ID_AA64PFR0_EL1, AdvSIMD, NI for signed.

Quoted 16 lines

Co-developed--by Oliver Upton <[email protected]>

Signed-off-by Oliver Upton <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
  arch/arm64/include/asm/kvm_host.h | 53 +++++++++++++++++++++++++++++++
  arch/arm64/kvm/pmu-emul.c         | 11 ++++---
  arch/arm64/kvm/sys_regs.c         |  6 ++--
  include/kvm/arm_pmu.h             | 11 -------
  4 files changed, 61 insertions(+), 20 deletions(-)

arch/arm64/include/asm/kvm_host.h +0 -0

index 21c57b812569..c0cf9c5f5e8d 100644

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1237

Quoted 5 lines

arch/arm64/include/asm/kvm_host.h +1 -0

@@ -1233,4 +1233,57 @@ static inline void kvm_hyp_reserve(void) { }
  void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
  bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu);
  +#define __expand_field_sign_unsigned(id, fld, val)
 \
+	((u64)(id##_##fld##_##val))

For unsigned features we get the actual "field" value, not the value in position. e.g,: ID_AA64PFR0_EL1_EL2_IMP = (0x1)

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1243

Quoted 6 lines

arch/arm64/include/asm/kvm_host.h +6 -0

+
+#define __expand_field_sign_signed(id, fld, val)			\
+	({								\
+		s64 __val = id##_##fld##_##val;				\
+		__val <<= 64 - id##_##fld##_WIDTH;			\
+		__val >>= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\

But for signed fields, we shift them back into the "position" as in the ID_REG. e.g.,

ID_AA64PFR0_EL1, AdvSIMD, NI we get:

	__val = ID_AA64PFR0_EL1_AdvSIMD_NI; /* = 0xf */
	__val <<= 64 - 4;		/* 0xf0_00_00_00_00_00_00_00 */
	__val >>= 64 - 20 - 4;		/* 0xff_ff_ff_ff_ff_f0_00_00 */

I think the last line instead should be:

	__val >>= 64 - id##_##fld##_WIDTH;

Quoted 6 lines

arch/arm64/include/asm/kvm_host.h +5 -0

+									\
+		__val;							\
+	})
+
+#define expand_field_sign(id, fld, val)					\

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1258

Quoted 10 lines

arch/arm64/include/asm/kvm_host.h +10 -0

+	(id##_##fld##_SIGNED ?						\
+	 __expand_field_sign_signed(id, fld, val) :			\
+	 __expand_field_sign_unsigned(id, fld, val))
+
+#define get_idreg_field_unsigned(kvm, id, fld)				\
+	({								\
+		u64 __val = IDREG(kvm, SYS_##id);			\
+		__val &= id##_##fld##_MASK;				\
+		__val >>= id##_##fld##_SHIFT;				\
+									\

We extract the field value for unsigned field, i.e., shifted to bit"0" and that matches the expand_field_sign().

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1266

Quoted 8 lines

arch/arm64/include/asm/kvm_host.h +8 -0

+		__val;							\
+	})
+
+#define get_idreg_field_signed(kvm, id, fld)				\
+	({								\
+		s64 __val = IDREG(kvm, SYS_##id);			\
+		__val <<= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\
+		__val >>= id##_##fld##_SHIFT;				\

However, here we get (assuming value ID_AA64PFR0_EL1_ASIMD = 0xf, and all other fields are 0 for clarity)

	__val = IDREG(kvm, SYS_ID_AA64PFR0_EL1); = 0xf0_00_00; 	/* 0xf << 20 */
	__val <<= 64 - 20 - 4;	/* = 0xf0_00_00_00_00_00_00_00 */
	__val >>= 20;		/* = 0xff_ff_ff_00_00_00_00_00 */

Thus they don;t match. Instead the last line should be :

	__val >>= id##_##fld##_WIDTH;

Quoted 55 lines · Suzuki K Poulose

Hi Marc,

Quoted 6 lines · Marc Zyngier

In order to make it easier to check whether a particular feature is exposed to a guest, add a new set of helpers, with kvm_has_feat() being the most useful.

Let's start making use of them in the PMU code (courtesy of Oliver). Follow-up work will intricude additional use patterns.

I think there is a bit of inconsistency in the macros for signed and unsigned. The unsigned fields are extracted (i.e., as if they were shifted to bit 0). But the signed fields are not shifted completely to bit '0' (in fact to different positions) and eventually we compare wrong things.

Using ID_AA64PFR0_EL1, fld=EL2, val=IMP for unsigned and ID_AA64PFR0_EL1, AdvSIMD, NI for signed.

Quoted 16 lines

Co-developed--by Oliver Upton <[email protected]>

Signed-off-by Oliver Upton <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
  arch/arm64/include/asm/kvm_host.h | 53 +++++++++++++++++++++++++++++++
  arch/arm64/kvm/pmu-emul.c         | 11 ++++---
  arch/arm64/kvm/sys_regs.c         |  6 ++--
  include/kvm/arm_pmu.h             | 11 -------
  4 files changed, 61 insertions(+), 20 deletions(-)

arch/arm64/include/asm/kvm_host.h +0 -0

index 21c57b812569..c0cf9c5f5e8d 100644

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1237

Quoted 5 lines

arch/arm64/include/asm/kvm_host.h +1 -0

@@ -1233,4 +1233,57 @@ static inline void kvm_hyp_reserve(void) { }
  void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
  bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu);
  +#define __expand_field_sign_unsigned(id, fld, val)
 \
+	((u64)(id##_##fld##_##val))

For unsigned features we get the actual "field" value, not the value in position. e.g,: ID_AA64PFR0_EL1_EL2_IMP = (0x1)

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1243

Quoted 6 lines

arch/arm64/include/asm/kvm_host.h +6 -0

+
+#define __expand_field_sign_signed(id, fld, val)			\
+	({								\
+		s64 __val = id##_##fld##_##val;				\
+		__val <<= 64 - id##_##fld##_WIDTH;			\
+		__val >>= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\

But for signed fields, we shift them back into the "position" as in the ID_REG. e.g.,

Quoted 8 lines

ID_AA64PFR0_EL1, AdvSIMD, NI we get:

	__val = ID_AA64PFR0_EL1_AdvSIMD_NI; /* = 0xf */
	__val <<= 64 - 4;		/* 0xf0_00_00_00_00_00_00_00 */
	__val >>= 64 - 20 - 4;		/* 0xff_ff_ff_ff_ff_f0_00_00 */

I think the last line instead should be:

	__val >>= 64 - id##_##fld##_WIDTH;

Huh, you're absolutely right.

Quoted 32 lines

Quoted 6 lines

arch/arm64/include/asm/kvm_host.h +5 -0

+									\
+		__val;							\
+	})
+
+#define expand_field_sign(id, fld, val)					\

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1258

Quoted 10 lines

arch/arm64/include/asm/kvm_host.h +10 -0

+	(id##_##fld##_SIGNED ?						\
+	 __expand_field_sign_signed(id, fld, val) :			\
+	 __expand_field_sign_unsigned(id, fld, val))
+
+#define get_idreg_field_unsigned(kvm, id, fld)				\
+	({								\
+		u64 __val = IDREG(kvm, SYS_##id);			\
+		__val &= id##_##fld##_MASK;				\
+		__val >>= id##_##fld##_SHIFT;				\
+									\

We extract the field value for unsigned field, i.e., shifted to bit"0" and that matches the expand_field_sign().

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1266

Quoted 8 lines

arch/arm64/include/asm/kvm_host.h +8 -0

+		__val;							\
+	})
+
+#define get_idreg_field_signed(kvm, id, fld)				\
+	({								\
+		s64 __val = IDREG(kvm, SYS_##id);			\
+		__val <<= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\
+		__val >>= id##_##fld##_SHIFT;				\

However, here we get (assuming value ID_AA64PFR0_EL1_ASIMD = 0xf, and all other fields are 0 for clarity)

Quoted 3 lines

	__val = IDREG(kvm, SYS_ID_AA64PFR0_EL1); = 0xf0_00_00; 	/* 0xf << 20 */
	__val <<= 64 - 20 - 4;	/* = 0xf0_00_00_00_00_00_00_00 */
	__val >>= 20;		/* = 0xff_ff_ff_00_00_00_00_00 */

Gah... again!

Quoted 3 lines

Thus they don;t match. Instead the last line should be :

Quoted 1 line

	__val >>= id##_##fld##_WIDTH;

Shouldn't this be (64 - WIDTH) instead, since we want the value to be shifted to bit 0? Otherwise, you get 0xff_00_00_00_00_00_00_00 (as per your example).

Thanks a lot for spotting those, much appreciated.

Quoted 108 lines · Marc Zyngier

Quoted 55 lines · Suzuki K Poulose

Hi Marc,

Quoted 6 lines · Marc Zyngier

In order to make it easier to check whether a particular feature is exposed to a guest, add a new set of helpers, with kvm_has_feat() being the most useful.

Let's start making use of them in the PMU code (courtesy of Oliver). Follow-up work will intricude additional use patterns.

I think there is a bit of inconsistency in the macros for signed and unsigned. The unsigned fields are extracted (i.e., as if they were shifted to bit 0). But the signed fields are not shifted completely to bit '0' (in fact to different positions) and eventually we compare wrong things.

Using ID_AA64PFR0_EL1, fld=EL2, val=IMP for unsigned and ID_AA64PFR0_EL1, AdvSIMD, NI for signed.

Quoted 16 lines

Co-developed--by Oliver Upton <[email protected]>

Signed-off-by Oliver Upton <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
   arch/arm64/include/asm/kvm_host.h | 53 +++++++++++++++++++++++++++++++
   arch/arm64/kvm/pmu-emul.c         | 11 ++++---
   arch/arm64/kvm/sys_regs.c         |  6 ++--
   include/kvm/arm_pmu.h             | 11 -------
   4 files changed, 61 insertions(+), 20 deletions(-)

arch/arm64/include/asm/kvm_host.h +0 -0

index 21c57b812569..c0cf9c5f5e8d 100644

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1237

Quoted 5 lines

arch/arm64/include/asm/kvm_host.h +1 -0

@@ -1233,4 +1233,57 @@ static inline void kvm_hyp_reserve(void) { }
   void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
   bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu);
   +#define __expand_field_sign_unsigned(id, fld, val)
 \
+	((u64)(id##_##fld##_##val))

For unsigned features we get the actual "field" value, not the value in position. e.g,: ID_AA64PFR0_EL1_EL2_IMP = (0x1)

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1243

Quoted 6 lines

arch/arm64/include/asm/kvm_host.h +6 -0

+
+#define __expand_field_sign_signed(id, fld, val)			\
+	({								\
+		s64 __val = id##_##fld##_##val;				\
+		__val <<= 64 - id##_##fld##_WIDTH;			\
+		__val >>= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\

But for signed fields, we shift them back into the "position" as in the ID_REG. e.g.,

Quoted 8 lines

ID_AA64PFR0_EL1, AdvSIMD, NI we get:

	__val = ID_AA64PFR0_EL1_AdvSIMD_NI; /* = 0xf */
	__val <<= 64 - 4;		/* 0xf0_00_00_00_00_00_00_00 */
	__val >>= 64 - 20 - 4;		/* 0xff_ff_ff_ff_ff_f0_00_00 */

I think the last line instead should be:

	__val >>= 64 - id##_##fld##_WIDTH;

Huh, you're absolutely right.

Quoted 32 lines

Quoted 6 lines

arch/arm64/include/asm/kvm_host.h +5 -0

+									\
+		__val;							\
+	})
+
+#define expand_field_sign(id, fld, val)					\

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1258

Quoted 10 lines

arch/arm64/include/asm/kvm_host.h +10 -0

+	(id##_##fld##_SIGNED ?						\
+	 __expand_field_sign_signed(id, fld, val) :			\
+	 __expand_field_sign_unsigned(id, fld, val))
+
+#define get_idreg_field_unsigned(kvm, id, fld)				\
+	({								\
+		u64 __val = IDREG(kvm, SYS_##id);			\
+		__val &= id##_##fld##_MASK;				\
+		__val >>= id##_##fld##_SHIFT;				\
+									\

We extract the field value for unsigned field, i.e., shifted to bit"0" and that matches the expand_field_sign().

arch/arm64/include/asm/kvm_host.h static inline void kvm_hyp_reserve(void) { } · line 1266

Quoted 8 lines

arch/arm64/include/asm/kvm_host.h +8 -0

+		__val;							\
+	})
+
+#define get_idreg_field_signed(kvm, id, fld)				\
+	({								\
+		s64 __val = IDREG(kvm, SYS_##id);			\
+		__val <<= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\
+		__val >>= id##_##fld##_SHIFT;				\

However, here we get (assuming value ID_AA64PFR0_EL1_ASIMD = 0xf, and all other fields are 0 for clarity)

Quoted 3 lines

	__val = IDREG(kvm, SYS_ID_AA64PFR0_EL1); = 0xf0_00_00; 	/* 0xf << 20 */
	__val <<= 64 - 20 - 4;	/* = 0xf0_00_00_00_00_00_00_00 */
	__val >>= 20;		/* = 0xff_ff_ff_00_00_00_00_00 */

Gah... again!

Quoted 1 line

Quoted 7 lines

Quoted 3 lines

Thus they don;t match. Instead the last line should be :

	__val >>= id##_##fld##_WIDTH;

Shouldn't this be (64 - WIDTH) instead, since we want the value to be shifted to bit 0? Otherwise, you get 0xff_00_00_00_00_00_00_00 (as per your example).

Quoted 2 lines

Thanks a lot for spotting those, much appreciated.

And since it is now obvious to anyone that I can't shift properly, let's just not do that. As it turns out, the kernel already has the required macros, and I can stop being clever with these:

arch/arm64/include/asm/kvm_host.h +2 -5

index ea8acc1914aa..3457fa313bad 100644
@@ -1290,11 +1290,8 @@ bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu);
 
  #define __expand_field_sign_signed(id, fld, val)			\
  	({								\
-		s64 __val = id##_##fld##_##val;				\
-		__val <<= 64 - id##_##fld##_WIDTH;			\
-		__val >>= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\
-									\
-		__val;							\
+		u64 __val = id##_##fld##_##val;				\
+		sign_extend64(__val, id##_##fld##_WIDTH - 1);		\
  	})
 
 

  #define expand_field_sign(id, fld, val)					\

arch/arm64/include/asm/kvm_host.h +3 -5

@@ -1313,11 +1310,9 @@ bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu);
 
  #define get_idreg_field_signed(kvm, id, fld)				\
  	({								\
-		s64 __val = IDREG(kvm, SYS_##id);			\
-		__val <<= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\
-		__val >>= id##_##fld##_SHIFT;				\
-									\
-		__val;							\
+		u64 __val = IDREG(kvm, SYS_##id);			\
+		__val = FIELD_GET(id##_##fld##_MASK, __val);		\
+		sign_extend64(__val, id##_##fld##_WIDTH - 1);		\
  	})
 
  #define get_idreg_field_enum(kvm, id, fld)				\

Thoughts?

	M.

VNCR-backed "registers" are actually only memory. Which means that there is zero control over what the guest can write, and that it is the hypervisor's job to actually sanitise the content of the backing store. Yeah, this is fun.

In order to preserve some form of sanity, add a repainting mechanism that makes use of a per-VM set of RES0/RES1 masks, one pair per VNCR register. These masks get applied on access to the backing store via __vcpu_sys_reg(), ensuring that the state that is consumed by KVM is correct.

So far, nothing populates these masks, but stay tuned.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h | 22 ++++++++++++++++-
 arch/arm64/kvm/arm.c              |  1 +
 arch/arm64/kvm/nested.c           | 41 ++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 2 deletions(-)

arch/arm64/include/asm/kvm_host.h +21 -1

index c0cf9c5f5e8d..816288e2d735 100644
@@ -238,6 +238,8 @@ static inline u16 kvm_mpidr_index(struct kvm_mpidr_data *data, u64 mpidr)
 	return index;
 }
 
+struct kvm_sysreg_masks;
+
 struct kvm_arch {
 	struct kvm_s2_mmu mmu;
 

@@ -312,6 +314,9 @@ struct kvm_arch {
 #define KVM_ARM_ID_REG_NUM	(IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
 	u64 id_regs[KVM_ARM_ID_REG_NUM];
 
+	/* Masks for VNCR-baked sysregs */
+	struct kvm_sysreg_masks	*sysreg_masks;
+
 	/*
 	 * For an untrusted host VM, 'pkvm.handle' is used to lookup
 	 * the associated pKVM instance in the hypervisor.

@@ -474,6 +479,13 @@ enum vcpu_sysreg {
 	NR_SYS_REGS	/* Nothing after this line! */
 };
 
+struct kvm_sysreg_masks {
+	struct {
+		u64	res0;
+		u64	res1;
+	} mask[NR_SYS_REGS - __VNCR_START__];
+};
+
 struct kvm_cpu_context {
 	struct user_pt_regs regs;	/* sp = sp_el0 */
 

@@ -868,7 +880,15 @@ static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
 
 #define ctxt_sys_reg(c,r)	(*__ctxt_sys_reg(c,r))
 
-#define __vcpu_sys_reg(v,r)	(ctxt_sys_reg(&(v)->arch.ctxt, (r)))
+u64 kvm_vcpu_sanitise_vncr_reg(const struct kvm_vcpu *, enum vcpu_sysreg);
+#define __vcpu_sys_reg(v,r)						\
+	(*({								\
+		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
+		u64 *__r = __ctxt_sys_reg(ctxt, (r));			\
+		if (vcpu_has_nv((v)) && (r) >= __VNCR_START__)		\
+			*__r = kvm_vcpu_sanitise_vncr_reg((v), (r));	\
+		__r;							\
+	}))
 
 u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg);
 void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg);

arch/arm64/kvm/arm.c +1 -0

index a25265aca432..c063e84fc72c 100644
@@ -206,6 +206,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 		pkvm_destroy_hyp_vm(kvm);
 
 	kfree(kvm->arch.mpidr_data);
+	kfree(kvm->arch.sysreg_masks);
 	kvm_destroy_vcpus(kvm);
 
 	kvm_unshare_hyp(kvm, kvm + 1);

arch/arm64/kvm/nested.c +38 -0

index d55e809e26cb..c976cd4b8379 100644
@@ -163,15 +163,54 @@ static u64 limit_nv_id_reg(u32 id, u64 val)
 
 	return val;
 }
+
+u64 kvm_vcpu_sanitise_vncr_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg sr)
+{
+	u64 v = ctxt_sys_reg(&vcpu->arch.ctxt, sr);
+	struct kvm_sysreg_masks *masks;
+
+	masks = vcpu->kvm->arch.sysreg_masks;
+
+	if (masks) {
+		sr -= __VNCR_START__;
+
+		v &= ~masks->mask[sr].res0;
+		v |= masks->mask[sr].res1;
+	}
+
+	return v;
+}
+
+static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
+{
+	int i = sr - __VNCR_START__;
+
+	kvm->arch.sysreg_masks->mask[i].res0 = res0;
+	kvm->arch.sysreg_masks->mask[i].res1 = res1;
+}
+
 int kvm_init_nv_sysregs(struct kvm *kvm)
 {
+	int ret = 0;
+
 	mutex_lock(&kvm->arch.config_lock);
 
+	if (kvm->arch.sysreg_masks)
+		goto out;
+
+	kvm->arch.sysreg_masks = kzalloc(sizeof(*(kvm->arch.sysreg_masks)),
+					 GFP_KERNEL);
+	if (!kvm->arch.sysreg_masks) {
+		ret = -ENOMEM;
+		goto out;
+	}
+

We can now start making use of our sanitising masks by setting them to values that depend on the guest's configuration.

First up are VTTBR_EL2, VTCR_EL2, VMPIDR_EL2 and HCR_EL2.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/nested.c | 56 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

arch/arm64/kvm/nested.c +2 -1

index c976cd4b8379..ee461e630527 100644
@@ -181,7 +181,7 @@ u64 kvm_vcpu_sanitise_vncr_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg sr)
 	return v;
 }
 
-static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
+static void set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
 {
 	int i = sr - __VNCR_START__;
 

@@ -191,6 +191,7 @@ static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u
 
 int kvm_init_nv_sysregs(struct kvm *kvm)
 {
+	u64 res0, res1;
 	int ret = 0;
 
 	mutex_lock(&kvm->arch.config_lock);

arch/arm64/kvm/nested.c +13 -0

+
+	/* VTCR_EL2 */
+	res0 = GENMASK(63, 32) | GENMASK(30, 20);
+	res1 = BIT(31);
+	set_sysreg_masks(kvm, VTCR_EL2, res0, res1);
+
+	/* VMPIDR_EL2 */
+	res0 = GENMASK(63, 40) | GENMASK(30, 24);
+	res1 = BIT(31);
+	set_sysreg_masks(kvm, VMPIDR_EL2, res0, res1);
+
+	/* HCR_EL2 */
+	res0 = BIT(48);

arch/arm64/kvm/nested.c +5 -0

+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, RAS, V1P1))
+		res0 |= HCR_FIEN;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, FWB, IMP))
+		res0 |= HCR_FWB;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, NV, NV2))

Quoted 35 lines · Marc Zyngier

We can now start making use of our sanitising masks by setting them to values that depend on the guest's configuration.

First up are VTTBR_EL2, VTCR_EL2, VMPIDR_EL2 and HCR_EL2.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/nested.c | 56 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

arch/arm64/kvm/nested.c +2 -1

index c976cd4b8379..ee461e630527 100644
@@ -181,7 +181,7 @@ u64 kvm_vcpu_sanitise_vncr_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg sr)
 	return v;
 }
 
-static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
+static void set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
 {
 	int i = sr - __VNCR_START__;
 

@@ -191,6 +191,7 @@ static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u
 
 int kvm_init_nv_sysregs(struct kvm *kvm)
 {
+	u64 res0, res1;
 	int ret = 0;
 
 	mutex_lock(&kvm->arch.config_lock);

@@ -209,6 +210,59 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 		kvm->arch.id_regs[i] = limit_nv_id_reg(IDX_IDREG(i),
 						       kvm->arch.id_regs[i]);
 

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 217

Quoted 5 lines

arch/arm64/kvm/nested.c +5 -0

+	/* VTTBR_EL2 */
+	res0 = res1 = 0;
+	if (!kvm_has_feat_enum(kvm, ID_AA64MMFR1_EL1, VMIDBits, 16))
+		res0 |= GENMASK(63, 56);
+	set_sysreg_masks(kvm, VTTBR_EL2, res0, res1);

CnP?

Quoted 14 lines

arch/arm64/kvm/nested.c +13 -0

+
+	/* VTCR_EL2 */
+	res0 = GENMASK(63, 32) | GENMASK(30, 20);
+	res1 = BIT(31);
+	set_sysreg_masks(kvm, VTCR_EL2, res0, res1);
+
+	/* VMPIDR_EL2 */
+	res0 = GENMASK(63, 40) | GENMASK(30, 24);
+	res1 = BIT(31);
+	set_sysreg_masks(kvm, VMPIDR_EL2, res0, res1);
+
+	/* HCR_EL2 */
+	res0 = BIT(48);

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 240

Quoted 10 lines

arch/arm64/kvm/nested.c +10 -0

+	res1 = HCR_RW;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR1_EL1, TWED, IMP))
+		res0 |= GENMASK(63, 59);
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, MTE, MTE2))
+		res0 |= (HCR_TID5 | HCR_DCT | HCR_ATA);
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, EVT, TTLBxS))
+		res0 |= (HCR_TTLBIS | HCR_TTLBOS);
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, CSV2, CSV2_2) &&
+	    !kvm_has_feat(kvm, ID_AA64PFR1_EL1, CSV2_frac, CSV2_1p2))
+		res1 = HCR_ENSCXT;

I'm confused here. If the VM doesn't have either CSV2_2 or CSV2_1p2.. HCR_ENSCXT is res1, that means we wouldn't trap?

The Arm ARM says:

	EnSCXT, bit [53]
		When FEAT_CSV2_2 is implemented or FEAT_CSV2_1p2 is implemented:

			[..]

		Otherwise:
			RES0

And if you actually meant res1, then you need: res1 |= HCR_ENSCXT, otherwise you override the HCR_RW res1 bit!

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 242

Quoted 3 lines

arch/arm64/kvm/nested.c +2 -0

+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, EVT, IMP))
+		res0 |= (HCR_TID4 | HCR_TICAB | HCR_TOCU);

minor nitpick: can you reverse these TOCU | TICAB | TID4 so it matches the relative positions in the register..

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 244

Quoted 3 lines

arch/arm64/kvm/nested.c +2 -0

+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, AMU, V1P1))
+		res0 |= HCR_AMVOFFEN;

We currently mask out ID_AA64PFR0_EL1.AMU (read_sanitised_id_aa64pfr0_el1 and part of the .val in the sys_regs_desc[]), can't hurt to be feature proof.

Quoted 10 lines

arch/arm64/kvm/nested.c +9 -0

+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, RAS, V1P1))
+		res0 |= HCR_FIEN;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, FWB, IMP))
+		res0 |= HCR_FWB;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, NV, NV2))
+		res0 |= HCR_NV2;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, NV, IMP))
+		res0 |= (HCR_AT | HCR_NV1 | HCR_NV);
+	if (!(__vcpu_has_feature(&kvm->arch, KVM_ARM_VCPU_PTRAUTH_ADDRESS) &&

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 259

Quoted 6 lines

arch/arm64/kvm/nested.c +6 -0

+	      __vcpu_has_feature(&kvm->arch, KVM_ARM_VCPU_PTRAUTH_ADDRESS)))
+		res0 |= (HCR_API | HCR_APK);
+	if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TME, IMP))
+		res0 |= BIT(39);
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, RAS, IMP))
+		res0 |= (HCR_TERR | HCR_TEA);

Same annoying nitpick, flip the values order to match the positions.

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 268

Quoted 10 lines

arch/arm64/kvm/nested.c +6 -0

+	if (!kvm_has_feat(kvm, ID_AA64MMFR1_EL1, LO, IMP))
+		res0 |= HCR_TLOR;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR4_EL1, E2H0, IMP))
+		res1 |= HCR_E2H;
+	set_sysreg_masks(kvm, HCR_EL2, res0, res1);
+
 out:
 	mutex_unlock(&kvm->arch.config_lock);
 

I need a lie down now.

We can now start making use of our sanitising masks by setting them to values that depend on the guest's configuration.

First up are VTTBR_EL2, VTCR_EL2, VMPIDR_EL2 and HCR_EL2.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/nested.c | 56 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

arch/arm64/kvm/nested.c +11 -1

index c976cd4b8379..ee461e630527 100644
@@ -181,7 +181,7 @@ u64 kvm_vcpu_sanitise_vncr_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg sr)
 	return v;
 }
 
-static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
+static void set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
 {
 	int i = sr - __VNCR_START__;
 

@@ -191,6 +191,7 @@ static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u
 
 int kvm_init_nv_sysregs(struct kvm *kvm)
 {
+	u64 res0, res1;
 	int ret = 0;
 
 	mutex_lock(&kvm->arch.config_lock);

@@ -209,6 +210,59 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 		kvm->arch.id_regs[i] = limit_nv_id_reg(IDX_IDREG(i),
 						       kvm->arch.id_regs[i]);
 
+	/* VTTBR_EL2 */
+	res0 = res1 = 0;
+	if (!kvm_has_feat_enum(kvm, ID_AA64MMFR1_EL1, VMIDBits, 16))
+		res0 |= GENMASK(63, 56);
+	set_sysreg_masks(kvm, VTTBR_EL2, res0, res1);
+
+	/* VTCR_EL2 */
+	res0 = GENMASK(63, 32) | GENMASK(30, 20);
+	res1 = BIT(31);

arch/arm64/kvm/nested.c +27 -0

+	if (!kvm_has_feat(kvm, ID_AA64MMFR1_EL1, TWED, IMP))
+		res0 |= GENMASK(63, 59);
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, MTE, MTE2))
+		res0 |= (HCR_TID5 | HCR_DCT | HCR_ATA);
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, EVT, TTLBxS))
+		res0 |= (HCR_TTLBIS | HCR_TTLBOS);
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, CSV2, CSV2_2) &&
+	    !kvm_has_feat(kvm, ID_AA64PFR1_EL1, CSV2_frac, CSV2_1p2))
+		res1 = HCR_ENSCXT;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, EVT, IMP))
+		res0 |= (HCR_TID4 | HCR_TICAB | HCR_TOCU);
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, AMU, V1P1))
+		res0 |= HCR_AMVOFFEN;
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, RAS, V1P1))
+		res0 |= HCR_FIEN;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, FWB, IMP))
+		res0 |= HCR_FWB;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, NV, NV2))
+		res0 |= HCR_NV2;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR2_EL1, NV, IMP))
+		res0 |= (HCR_AT | HCR_NV1 | HCR_NV);
+	if (!(__vcpu_has_feature(&kvm->arch, KVM_ARM_VCPU_PTRAUTH_ADDRESS) &&
+	      __vcpu_has_feature(&kvm->arch, KVM_ARM_VCPU_PTRAUTH_ADDRESS)))
+		res0 |= (HCR_API | HCR_APK);
+	if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TME, IMP))
+		res0 |= BIT(39);
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, RAS, IMP))

Quoted 49 lines · Marc Zyngier

We can now start making use of our sanitising masks by setting them to values that depend on the guest's configuration.

First up are VTTBR_EL2, VTCR_EL2, VMPIDR_EL2 and HCR_EL2.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/nested.c | 56 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

arch/arm64/kvm/nested.c +16 -1

index c976cd4b8379..ee461e630527 100644
@@ -181,7 +181,7 @@ u64 kvm_vcpu_sanitise_vncr_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg sr)
 	return v;
 }
 
-static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
+static void set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
 {
 	int i = sr - __VNCR_START__;
 

@@ -191,6 +191,7 @@ static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u
 
 int kvm_init_nv_sysregs(struct kvm *kvm)
 {
+	u64 res0, res1;
 	int ret = 0;
 
 	mutex_lock(&kvm->arch.config_lock);

@@ -209,6 +210,59 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 		kvm->arch.id_regs[i] = limit_nv_id_reg(IDX_IDREG(i),
 						       kvm->arch.id_regs[i]);
 
+	/* VTTBR_EL2 */
+	res0 = res1 = 0;
+	if (!kvm_has_feat_enum(kvm, ID_AA64MMFR1_EL1, VMIDBits, 16))
+		res0 |= GENMASK(63, 56);
+	set_sysreg_masks(kvm, VTTBR_EL2, res0, res1);
+
+	/* VTCR_EL2 */
+	res0 = GENMASK(63, 32) | GENMASK(30, 20);
+	res1 = BIT(31);
+	set_sysreg_masks(kvm, VTCR_EL2, res0, res1);
+
+	/* VMPIDR_EL2 */
+	res0 = GENMASK(63, 40) | GENMASK(30, 24);
+	res1 = BIT(31);

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 231

Quoted 5 lines

arch/arm64/kvm/nested.c +5 -0

+	set_sysreg_masks(kvm, VMPIDR_EL2, res0, res1);
+
+	/* HCR_EL2 */
+	res0 = BIT(48);
+	res1 = HCR_RW;

Just want to clarify this bit actually, this is restricting the (first level only?) nested EL1 to run as AArch64?

Should we be sanitising ID_AA64PFR0_EL1.EL1=0b0001?

Quoted 51 lines · Joey Gouly

Quoted 49 lines · Marc Zyngier

We can now start making use of our sanitising masks by setting them to values that depend on the guest's configuration.

First up are VTTBR_EL2, VTCR_EL2, VMPIDR_EL2 and HCR_EL2.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/nested.c | 56 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

arch/arm64/kvm/nested.c +16 -1

index c976cd4b8379..ee461e630527 100644
@@ -181,7 +181,7 @@ u64 kvm_vcpu_sanitise_vncr_reg(const struct kvm_vcpu *vcpu, enum vcpu_sysreg sr)
 	return v;
 }
 
-static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
+static void set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
 {
 	int i = sr - __VNCR_START__;
 

@@ -191,6 +191,7 @@ static void __maybe_unused set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u
 
 int kvm_init_nv_sysregs(struct kvm *kvm)
 {
+	u64 res0, res1;
 	int ret = 0;
 
 	mutex_lock(&kvm->arch.config_lock);

@@ -209,6 +210,59 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 		kvm->arch.id_regs[i] = limit_nv_id_reg(IDX_IDREG(i),
 						       kvm->arch.id_regs[i]);
 
+	/* VTTBR_EL2 */
+	res0 = res1 = 0;
+	if (!kvm_has_feat_enum(kvm, ID_AA64MMFR1_EL1, VMIDBits, 16))
+		res0 |= GENMASK(63, 56);
+	set_sysreg_masks(kvm, VTTBR_EL2, res0, res1);
+
+	/* VTCR_EL2 */
+	res0 = GENMASK(63, 32) | GENMASK(30, 20);
+	res1 = BIT(31);
+	set_sysreg_masks(kvm, VTCR_EL2, res0, res1);
+
+	/* VMPIDR_EL2 */
+	res0 = GENMASK(63, 40) | GENMASK(30, 24);
+	res1 = BIT(31);

Quoted 8 lines

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 231

Quoted 5 lines

arch/arm64/kvm/nested.c +5 -0

+	set_sysreg_masks(kvm, VMPIDR_EL2, res0, res1);
+
+	/* HCR_EL2 */
+	res0 = BIT(48);
+	res1 = HCR_RW;

Just want to clarify this bit actually, this is restricting the (first level only?) nested EL1 to run as AArch64?

This is restricting it at all levels. The guest hypervisor will eventually write its own view of HCR_EL2.RW in the VNCR page, and if it has promised AArch32 to its own guest (at any level), it is in for a treat, because we will forcefully reset this bit to 1.

Quoted 2 lines

Should we be sanitising ID_AA64PFR0_EL1.EL1=0b0001?

We already do. See limit_nv_id_reg() and how it constraints ID_AA64PFR0_EL1.ELx to 0b0001.

Fine Grained Traps are controlled by a whole bunch of features. Each one of them must be checked and the corresponding masks computed so that we don't let the guest apply traps it shouldn't be using.

This takes care of HFGxTR_EL2, HDFGxTR_EL2, and HAFGRTR_EL2.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/nested.c | 128 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

arch/arm64/kvm/nested.c +113 -0

index ee461e630527..cdeef3259193 100644
@@ -263,6 +263,134 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 		res1 |= HCR_E2H;
 	set_sysreg_masks(kvm, HCR_EL2, res0, res1);
 
+	/* HFG[RW]TR_EL2 */
+	res0 = res1 = 0;
+	if (!(__vcpu_has_feature(&kvm->arch, KVM_ARM_VCPU_PTRAUTH_ADDRESS) &&
+	      __vcpu_has_feature(&kvm->arch, KVM_ARM_VCPU_PTRAUTH_ADDRESS)))
+		res0 |= (HFGxTR_EL2_APDAKey | HFGxTR_EL2_APDBKey |
+			 HFGxTR_EL2_APGAKey | HFGxTR_EL2_APIAKey |
+			 HFGxTR_EL2_APIBKey);
+	if (!kvm_has_feat(kvm, ID_AA64MMFR1_EL1, LO, IMP))
+		res0 |= (HFGxTR_EL2_LORC_EL1 | HFGxTR_EL2_LOREA_EL1 |
+			 HFGxTR_EL2_LORID_EL1 | HFGxTR_EL2_LORN_EL1 |
+			 HFGxTR_EL2_LORSA_EL1);
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, CSV2, CSV2_2) &&
+	    !kvm_has_feat(kvm, ID_AA64PFR1_EL1, CSV2_frac, CSV2_1p2))
+		res0 |= (HFGxTR_EL2_SCXTNUM_EL1 | HFGxTR_EL2_SCXTNUM_EL0);
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, GIC, IMP))
+		res0 |= HFGxTR_EL2_ICC_IGRPENn_EL1;
+	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, RAS, IMP))
+		res0 |= (HFGxTR_EL2_ERRIDR_EL1 | HFGxTR_EL2_ERRSELR_EL1 |
+			 HFGxTR_EL2_ERXFR_EL1 | HFGxTR_EL2_ERXCTLR_EL1 |
+			 HFGxTR_EL2_ERXSTATUS_EL1 | HFGxTR_EL2_ERXMISCn_EL1 |
+			 HFGxTR_EL2_ERXPFGF_EL1 | HFGxTR_EL2_ERXPFGCTL_EL1 |
+			 HFGxTR_EL2_ERXPFGCDN_EL1 | HFGxTR_EL2_ERXADDR_EL1);
+	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, LS64, LS64_ACCDATA))
+		res0 |= HFGxTR_EL2_nACCDATA_EL1;
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP))
+		res0 |= (HFGxTR_EL2_nGCS_EL0 | HFGxTR_EL2_nGCS_EL1);
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, SME, IMP))
+		res0 |= (HFGxTR_EL2_nSMPRI_EL1 | HFGxTR_EL2_nTPIDR2_EL0);
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, THE, IMP))
+		res0 |= HFGxTR_EL2_nRCWMASK_EL1;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1PIE, IMP))
+		res0 |= (HFGxTR_EL2_nPIRE0_EL1 | HFGxTR_EL2_nPIR_EL1);
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1POE, IMP))
+		res0 |= (HFGxTR_EL2_nPOR_EL0 | HFGxTR_EL2_nPOR_EL1);
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S2POE, IMP))
+		res0 |= HFGxTR_EL2_nS2POR_EL1;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, AIE, IMP))
+		res0 |= (HFGxTR_EL2_nMAIR2_EL1 | HFGxTR_EL2_nAMAIR2_EL1);
+	set_sysreg_masks(kvm, HFGRTR_EL2, res0 | __HFGRTR_EL2_RES0, res1);
+	set_sysreg_masks(kvm, HFGWTR_EL2, res0 | __HFGWTR_EL2_RES0, res1);
+
+	/* HDFG[RW]TR_EL2 */
+	res0 = res1 = 0;
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DoubleLock, IMP))
+		res0 |= HDFGRTR_EL2_OSDLR_EL1;
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP))
+		res0 |= (HDFGRTR_EL2_PMEVCNTRn_EL0 | HDFGRTR_EL2_PMEVTYPERn_EL0 |
+			 HDFGRTR_EL2_PMCCFILTR_EL0 | HDFGRTR_EL2_PMCCNTR_EL0 |
+			 HDFGRTR_EL2_PMCNTEN | HDFGRTR_EL2_PMINTEN |
+			 HDFGRTR_EL2_PMOVS | HDFGRTR_EL2_PMSELR_EL0 |
+			 HDFGRTR_EL2_PMMIR_EL1 | HDFGRTR_EL2_PMUSERENR_EL0 |
+			 HDFGRTR_EL2_PMCEIDn_EL0);
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, IMP))
+		res0 |= (HDFGRTR_EL2_PMBLIMITR_EL1 | HDFGRTR_EL2_PMBPTR_EL1 |
+			 HDFGRTR_EL2_PMBSR_EL1 | HDFGRTR_EL2_PMSCR_EL1 |
+			 HDFGRTR_EL2_PMSEVFR_EL1 | HDFGRTR_EL2_PMSFCR_EL1 |
+			 HDFGRTR_EL2_PMSICR_EL1 | HDFGRTR_EL2_PMSIDR_EL1 |
+			 HDFGRTR_EL2_PMSIRR_EL1 | HDFGRTR_EL2_PMSLATFR_EL1 |
+			 HDFGRTR_EL2_PMBIDR_EL1);
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, TraceVer, IMP))
+		res0 |= (HDFGRTR_EL2_TRC | HDFGRTR_EL2_TRCAUTHSTATUS |
+			 HDFGRTR_EL2_TRCAUXCTLR | HDFGRTR_EL2_TRCCLAIM |
+			 HDFGRTR_EL2_TRCCNTVRn | HDFGRTR_EL2_TRCID |
+			 HDFGRTR_EL2_TRCIMSPECn | HDFGRTR_EL2_TRCOSLSR |
+			 HDFGRTR_EL2_TRCPRGCTLR | HDFGRTR_EL2_TRCSEQSTR |
+			 HDFGRTR_EL2_TRCSSCSRn | HDFGRTR_EL2_TRCSTATR |
+			 HDFGRTR_EL2_TRCVICTLR);
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, TraceBuffer, IMP))
+		res0 |= (HDFGRTR_EL2_TRBBASER_EL1 | HDFGRTR_EL2_TRBIDR_EL1 |
+			 HDFGRTR_EL2_TRBLIMITR_EL1 | HDFGRTR_EL2_TRBMAR_EL1 |
+			 HDFGRTR_EL2_TRBPTR_EL1 | HDFGRTR_EL2_TRBSR_EL1 |
+			 HDFGRTR_EL2_TRBTRG_EL1);
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP))
+		res0 |= (HDFGRTR_EL2_nBRBIDR | HDFGRTR_EL2_nBRBCTL |
+			 HDFGRTR_EL2_nBRBDATA);
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P2))
+		res0 |= HDFGRTR_EL2_nPMSNEVFR_EL1;
+	set_sysreg_masks(kvm, HDFGRTR_EL2, res0 | HDFGRTR_EL2_RES0, res1);
+
+	/* Reuse the bits from the read-side and add the write-specific stuff */
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMUVer, IMP))
+		res0 |= (HDFGWTR_EL2_PMCR_EL0 | HDFGWTR_EL2_PMSWINC_EL0);
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, TraceVer, IMP))
+		res0 |= HDFGWTR_EL2_TRCOSLAR;
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, TraceFilt, IMP))
+		res0 |= HDFGWTR_EL2_TRFCR_EL1;
+	set_sysreg_masks(kvm, HFGWTR_EL2, res0 | HDFGWTR_EL2_RES0, res1);
+
+	/* HFGITR_EL2 */
+	res0 = HFGITR_EL2_RES0;
+	res1 = HFGITR_EL2_RES1;
+	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, DPB, DPB2))
+		res0 |= HFGITR_EL2_DCCVADP;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR1_EL1, PAN, PAN2))
+		res0 |= (HFGITR_EL2_ATS1E1RP | HFGITR_EL2_ATS1E1WP);
+	if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, OS))
+		res0 |= (HFGITR_EL2_TLBIRVAALE1OS | HFGITR_EL2_TLBIRVALE1OS |
+			 HFGITR_EL2_TLBIRVAAE1OS | HFGITR_EL2_TLBIRVAE1OS |
+			 HFGITR_EL2_TLBIVAALE1OS | HFGITR_EL2_TLBIVALE1OS |
+			 HFGITR_EL2_TLBIVAAE1OS | HFGITR_EL2_TLBIASIDE1OS |
+			 HFGITR_EL2_TLBIVAE1OS | HFGITR_EL2_TLBIVMALLE1OS);
+	if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, RANGE))
+		res0 |= (HFGITR_EL2_TLBIRVAALE1 | HFGITR_EL2_TLBIRVALE1 |
+			 HFGITR_EL2_TLBIRVAAE1 | HFGITR_EL2_TLBIRVAE1 |
+			 HFGITR_EL2_TLBIRVAALE1IS | HFGITR_EL2_TLBIRVALE1IS |
+			 HFGITR_EL2_TLBIRVAAE1IS | HFGITR_EL2_TLBIRVAE1IS |
+			 HFGITR_EL2_TLBIRVAALE1OS | HFGITR_EL2_TLBIRVALE1OS |
+			 HFGITR_EL2_TLBIRVAAE1OS | HFGITR_EL2_TLBIRVAE1OS);
+	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, SPECRES, IMP))
+		res0 |= (HFGITR_EL2_CFPRCTX | HFGITR_EL2_DVPRCTX |
+			 HFGITR_EL2_CPPRCTX);
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP))
+		res0 |= (HFGITR_EL2_nBRBINJ | HFGITR_EL2_nBRBIALL);

Quoted 6 lines · Marc Zyngier

Fine Grained Traps are controlled by a whole bunch of features. Each one of them must be checked and the corresponding masks computed so that we don't let the guest apply traps it shouldn't be using.

This takes care of HFGxTR_EL2, HDFGxTR_EL2, and HAFGRTR_EL2.

^^ and HFGITR_EL2

Just like its little friends, HCRX_EL2 gets the feature set treatment when backed by VNCR.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/nested.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

arch/arm64/kvm/nested.c +0 -0

index cdeef3259193..72db632b115a 100644

arch/arm64/kvm/nested.c +6 -0

+	if (!kvm_has_feat(kvm, ID_AA64ISAR3_EL1, PACM, TRIVIAL_IMP))
+		res0 |= HCRX_EL2_PACMEn;
+	if (!kvm_has_feat(kvm, ID_AA64PFR2_EL1, FPMR, IMP))
+		res0 |= HCRX_EL2_EnFPM;
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP))
+		res0 |= HCRX_EL2_GCSEn;

arch/arm64/kvm/nested.c +23 -0

+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, SCTLRX, IMP))
+		res0 |= HCRX_EL2_SCTLR2En;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, TCRX, IMP))
+		res0 |= HCRX_EL2_TCR2En;
+	if (!kvm_has_feat(kvm, ID_AA64ISAR2_EL1, MOPS, IMP))
+		res0 |= (HCRX_EL2_MSCEn | HCRX_EL2_MCE2);
+	if (!kvm_has_feat(kvm, ID_AA64MMFR1_EL1, CMOW, IMP))
+		res0 |= HCRX_EL2_CMOW;
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, NMI, IMP))
+		res0 |= (HCRX_EL2_VFNMI | HCRX_EL2_VINMI | HCRX_EL2_TALLINT);
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, SME, IMP) ||
+	    !(read_sysreg_s(SYS_SMIDR_EL1) & SMIDR_EL1_SMPS))
+		res0 |= HCRX_EL2_SMPME;
+	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, XS, IMP))
+		res0 |= (HCRX_EL2_FGTnXS | HCRX_EL2_FnXS);
+	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, LS64, LS64_V))
+		res0 |= HCRX_EL2_EnASR;
+	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, LS64, LS64))
+		res0 |= HCRX_EL2_EnALS;
+	if (!kvm_has_feat(kvm, ID_AA64ISAR1_EL1, LS64, LS64_ACCDATA))
+		res0 |= HCRX_EL2_EnAS0;
+	set_sysreg_masks(kvm, HCRX_EL2, res0, res1);
+
 	/* HFG[RW]TR_EL2 */
 	res0 = res1 = 0;
 	if (!(__vcpu_has_feature(&kvm->arch, KVM_ARM_VCPU_PTRAUTH_ADDRESS) &&

Signature · 1 line

2.39.2

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 284

Quoted 4 lines · Marc Zyngier

arch/arm64/kvm/nested.c +4 -0

+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, D128, IMP))
+		res0 |= HCRX_EL2_D128En;
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, THE, IMP))
+		res0 |= HCRX_EL2_PTTWI;

Ok, not fair. The latest public version of the ARM ARM doesn't have any of this. Where are you getting it from?

Quoted 13 lines · Marc Zyngier

Just like its little friends, HCRX_EL2 gets the feature set treatment when backed by VNCR.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/nested.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

arch/arm64/kvm/nested.c +0 -0

index cdeef3259193..72db632b115a 100644

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 268

Quoted 6 lines

arch/arm64/kvm/nested.c +3 -0

@@ -263,6 +263,48 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 		res1 |= HCR_E2H;
 	set_sysreg_masks(kvm, HCR_EL2, res0, res1);
 
+	/* HCRX_EL2 */
+	res0 = HCRX_EL2_RES0;
+	res1 = HCRX_EL2_RES1;

I'm a bit worried that we're depending on the meaning of these generated RES0/RES1 bitmasks not changing behind our backs.

Not like people read anything, but do you think it'd make sense to add a warning comment to the sysreg file that adding new encodings can have a functional change on KVM?

Quoted 12 lines

arch/arm64/kvm/nested.c +11 -0

+	if (!kvm_has_feat(kvm, ID_AA64ISAR3_EL1, PACM, TRIVIAL_IMP))
+		res0 |= HCRX_EL2_PACMEn;
+	if (!kvm_has_feat(kvm, ID_AA64PFR2_EL1, FPMR, IMP))
+		res0 |= HCRX_EL2_EnFPM;
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP))
+		res0 |= HCRX_EL2_GCSEn;
+	if (!kvm_has_feat(kvm, ID_AA64ISAR2_EL1, SYSREG_128, IMP))
+		res0 |= HCRX_EL2_EnIDCP128;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, ADERR, DEV_ASYNC))
+		res0 |= (HCRX_EL2_EnSDERR | HCRX_EL2_EnSNERR);
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, DF2, IMP))

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 284

Quoted 5 lines

arch/arm64/kvm/nested.c +5 -0

+		res0 |= HCRX_EL2_TMEA;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, D128, IMP))
+		res0 |= HCRX_EL2_D128En;
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, THE, IMP))
+		res0 |= HCRX_EL2_PTTWI;

Ok, not fair. The latest public version of the ARM ARM doesn't have any of this. Where are you getting it from?

No amount of warnings will do, because people don't give a damn.

I'm actually in favour of something far more radical: we snapshot the raw value of all the used RES0/1, and put BUILD_BUG_ON()s that fire if any value has changed. They will have to touch the KVM code to fix that, and we catch them red-handed.

Quoted 12 lines

Quoted 12 lines

arch/arm64/kvm/nested.c +11 -0

+	if (!kvm_has_feat(kvm, ID_AA64ISAR3_EL1, PACM, TRIVIAL_IMP))
+		res0 |= HCRX_EL2_PACMEn;
+	if (!kvm_has_feat(kvm, ID_AA64PFR2_EL1, FPMR, IMP))
+		res0 |= HCRX_EL2_EnFPM;
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP))
+		res0 |= HCRX_EL2_GCSEn;
+	if (!kvm_has_feat(kvm, ID_AA64ISAR2_EL1, SYSREG_128, IMP))
+		res0 |= HCRX_EL2_EnIDCP128;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, ADERR, DEV_ASYNC))
+		res0 |= (HCRX_EL2_EnSDERR | HCRX_EL2_EnSNERR);
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, DF2, IMP))

Quoted 8 lines

arch/arm64/kvm/nested.c int kvm_init_nv_sysregs(struct kvm *kvm) · line 284

Quoted 5 lines

arch/arm64/kvm/nested.c +5 -0

+		res0 |= HCRX_EL2_TMEA;
+	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, D128, IMP))
+		res0 |= HCRX_EL2_D128En;
+	if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, THE, IMP))
+		res0 |= HCRX_EL2_PTTWI;

Ok, not fair. The latest public version of the ARM ARM doesn't have any of this. Where are you getting it from?

The bloody XML, from which all of this should, in theory, be extracted without any human intervention. Sadly, it seems that we are not allowed to do so, from what I have been told.

The ARM ARM is, unfortunately, being quickly rendered obsolete by the lack of updates (FEAT_THE is part of the v9.4 architecture, released in 2022).

Quoted 6 lines · Marc Zyngier

No amount of warnings will do, because people don't give a damn.

I'm actually in favour of something far more radical: we snapshot the raw value of all the used RES0/1, and put BUILD_BUG_ON()s that fire if any value has changed. They will have to touch the KVM code to fix that, and we catch them red-handed.

Oh I like that a lot more.

Quoted 8 lines · Oliver Upton

Quoted 6 lines · Marc Zyngier

No amount of warnings will do, because people don't give a damn.

I'm actually in favour of something far more radical: we snapshot the raw value of all the used RES0/1, and put BUILD_BUG_ON()s that fire if any value has changed. They will have to touch the KVM code to fix that, and we catch them red-handed.

Quoted 1 line

Oh I like that a lot more.

Unsure which registers you planned to give this treatment, but all of the ID registers we know about should probably go in this bucket too.

In order to be able to store different values for member of an encoding range, replace xa_store_range() calls with discrete xa_store() calls and an encoding iterator.

We end-up using a bit more memory, but we gain some flexibility that we will make use of shortly.

Take this opportunity to tidy up the error handling path.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/emulate-nested.c | 49 ++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 13 deletions(-)

arch/arm64/kvm/emulate-nested.c +29 -13

index ef46c2e45307..42f5b4483c80 100644
@@ -1757,6 +1757,28 @@ static __init void print_nv_trap_error(const struct encoding_to_trap_config *tc,
 		err);
 }
 
+static u32 encoding_next(u32 encoding)
+{
+	u8 op0, op1, crn, crm, op2;
+
+	op0 = sys_reg_Op0(encoding);
+	op1 = sys_reg_Op1(encoding);
+	crn = sys_reg_CRn(encoding);
+	crm = sys_reg_CRm(encoding);
+	op2 = sys_reg_Op2(encoding);
+
+	if (op2 < Op2_mask)
+		return sys_reg(op0, op1, crn, crm, op2 + 1);
+	if (crm < CRm_mask)
+		return sys_reg(op0, op1, crn, crm + 1, 0);
+	if (crn < CRn_mask)
+		return sys_reg(op0, op1, crn + 1, 0, 0);
+	if (op1 < Op1_mask)
+		return sys_reg(op0, op1 + 1, 0, 0, 0);
+
+	return sys_reg(op0 + 1, 0, 0, 0, 0);
+}
+
 int __init populate_nv_trap_config(void)
 {
 	int ret = 0;

@@ -1775,23 +1797,18 @@ int __init populate_nv_trap_config(void)
 			ret = -EINVAL;
 		}
 
-		if (cgt->encoding != cgt->end) {
-			prev = xa_store_range(&sr_forward_xa,
-					      cgt->encoding, cgt->end,
-					      xa_mk_value(cgt->tc.val),
-					      GFP_KERNEL);
-		} else {
-			prev = xa_store(&sr_forward_xa, cgt->encoding,
+		for (u32 enc = cgt->encoding; enc <= cgt->end; enc = encoding_next(enc)) {
+			prev = xa_store(&sr_forward_xa, enc,
 					xa_mk_value(cgt->tc.val), GFP_KERNEL);
 			if (prev && !xa_is_err(prev)) {
 				ret = -EINVAL;
 				print_nv_trap_error(cgt, "Duplicate CGT", ret);
 			}
-		}
 
-		if (xa_is_err(prev)) {
-			ret = xa_err(prev);
-			print_nv_trap_error(cgt, "Failed CGT insertion", ret);
+			if (xa_is_err(prev)) {
+				ret = xa_err(prev);
+				print_nv_trap_error(cgt, "Failed CGT insertion", ret);
+			}
 		}
 	}
 

@@ -1804,6 +1821,7 @@ int __init populate_nv_trap_config(void)
 	for (int i = 0; i < ARRAY_SIZE(encoding_to_fgt); i++) {
 		const struct encoding_to_trap_config *fgt = &encoding_to_fgt[i];
 		union trap_config tc;
+		void *prev;
 
 		if (fgt->tc.fgt >= __NR_FGT_GROUP_IDS__) {
 			ret = -EINVAL;

@@ -1818,8 +1836,13 @@ int __init populate_nv_trap_config(void)
 		}
 
 		tc.val |= fgt->tc.val;
-		xa_store(&sr_forward_xa, fgt->encoding,
-			 xa_mk_value(tc.val), GFP_KERNEL);