[PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Marc Zyngier]

This series tries to bring some sanity to the way the RESx masks
are applied when accessing the in-memory view of the guest's
system registers.

Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
be used as a rvalue or lvalue while that applies the RESx masks behind
the scenes. This works fine when used as a rvalue.

However, when used as a lvalue, it does the wrong thing, as it only
sanitises the value we're about to overwrite. This is pointless work
and potentially hides bugs.

I propose that we move to a set of store-specific accessors (for
assignments and RMW) instead of the lvalue hack, ensuring that the
assigned value is the one that gets sanitised. This then allows the 
legacy accessor to be converted to rvalue-only.

Given the level of churn this introduces, I'd like this to land very
early in the cycle. Either before 6.16-rc2, or early in 6.17.

* From v1 [1]

  - rebased to kvmarm-fixes-6.16-1

[1] https://lore.kernel.org/all/20250113183524.1378778-1-maz@kernel.org/

Marc Zyngier (4):
  KVM: arm64: Add assignment-specific sysreg accessor
  KVM: arm64: Add RMW specific sysreg accessor
  KVM: arm64: Don't use __vcpu_sys_reg() to get the address of a sysreg
  KVM: arm64: Make __vcpu_sys_reg() a pure rvalue operand

 arch/arm64/include/asm/kvm_host.h          | 31 +++++++++--
 arch/arm64/kvm/arch_timer.c                | 18 +++----
 arch/arm64/kvm/debug.c                     |  4 +-
 arch/arm64/kvm/fpsimd.c                    |  4 +-
 arch/arm64/kvm/hyp/exception.c             |  4 +-
 arch/arm64/kvm/hyp/include/hyp/switch.h    |  4 +-
 arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  6 +--
 arch/arm64/kvm/hyp/nvhe/hyp-main.c         |  4 +-
 arch/arm64/kvm/hyp/vhe/switch.c            |  4 +-
 arch/arm64/kvm/hyp/vhe/sysreg-sr.c         | 48 ++++++++---------
 arch/arm64/kvm/nested.c                    |  2 +-
 arch/arm64/kvm/pmu-emul.c                  | 24 ++++-----
 arch/arm64/kvm/sys_regs.c                  | 60 +++++++++++-----------
 arch/arm64/kvm/sys_regs.h                  |  4 +-
 arch/arm64/kvm/vgic/vgic-v3-nested.c       | 10 ++--
 15 files changed, 125 insertions(+), 102 deletions(-)

-- 
2.39.2

[PATCH v2 1/4] KVM: arm64: Add assignment-specific sysreg accessor

[Marc Zyngier]

Assigning a value to a system register doesn't do what it is
supposed to be doing if that register is one that has RESx bits.

The main problem is that we use __vcpu_sys_reg(), which can be used
both as a lvalue and rvalue. When used as a lvalue, the bit masking
occurs *before* the new value is assigned, meaning that we (1) do
pointless work on the old cvalue, and (2) potentially assign an
invalid value as we fail to apply the masks to it.

Fix this by providing a new __vcpu_assign_sys_reg() that does
what it says on the tin, and sanitises the *new* value instead of
the old one. This comes with a significant amount of churn.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h          | 11 ++++++
 arch/arm64/kvm/arch_timer.c                | 16 ++++----
 arch/arm64/kvm/hyp/exception.c             |  4 +-
 arch/arm64/kvm/hyp/include/hyp/switch.h    |  4 +-
 arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  6 +--
 arch/arm64/kvm/hyp/nvhe/hyp-main.c         |  4 +-
 arch/arm64/kvm/hyp/vhe/switch.c            |  4 +-
 arch/arm64/kvm/hyp/vhe/sysreg-sr.c         | 44 +++++++++++-----------
 arch/arm64/kvm/pmu-emul.c                  | 14 +++----
 arch/arm64/kvm/sys_regs.c                  | 38 ++++++++++---------
 arch/arm64/kvm/sys_regs.h                  |  4 +-
 arch/arm64/kvm/vgic/vgic-v3-nested.c       | 10 ++---
 12 files changed, 86 insertions(+), 73 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d941abc6b5eef..3b84ad91116b4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1107,6 +1107,17 @@ 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))
 
 u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
+
+#define __vcpu_assign_sys_reg(v, r, val)				\
+	do {								\
+		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
+		u64 __v = (val);					\
+		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)	\
+			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);	\
+									\
+		ctxt_sys_reg(ctxt, (r)) = __v;				\
+	} while (0)
+
 #define __vcpu_sys_reg(v,r)						\
 	(*({								\
 		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 5133dcbfe9f76..5a67a6d4d95b7 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -108,16 +108,16 @@ static void timer_set_ctl(struct arch_timer_context *ctxt, u32 ctl)
 
 	switch(arch_timer_ctx_index(ctxt)) {
 	case TIMER_VTIMER:
-		__vcpu_sys_reg(vcpu, CNTV_CTL_EL0) = ctl;
+		__vcpu_assign_sys_reg(vcpu, CNTV_CTL_EL0, ctl);
 		break;
 	case TIMER_PTIMER:
-		__vcpu_sys_reg(vcpu, CNTP_CTL_EL0) = ctl;
+		__vcpu_assign_sys_reg(vcpu, CNTP_CTL_EL0, ctl);
 		break;
 	case TIMER_HVTIMER:
-		__vcpu_sys_reg(vcpu, CNTHV_CTL_EL2) = ctl;
+		__vcpu_assign_sys_reg(vcpu, CNTHV_CTL_EL2, ctl);
 		break;
 	case TIMER_HPTIMER:
-		__vcpu_sys_reg(vcpu, CNTHP_CTL_EL2) = ctl;
+		__vcpu_assign_sys_reg(vcpu, CNTHP_CTL_EL2, ctl);
 		break;
 	default:
 		WARN_ON(1);
@@ -130,16 +130,16 @@ static void timer_set_cval(struct arch_timer_context *ctxt, u64 cval)
 
 	switch(arch_timer_ctx_index(ctxt)) {
 	case TIMER_VTIMER:
-		__vcpu_sys_reg(vcpu, CNTV_CVAL_EL0) = cval;
+		__vcpu_assign_sys_reg(vcpu, CNTV_CVAL_EL0, cval);
 		break;
 	case TIMER_PTIMER:
-		__vcpu_sys_reg(vcpu, CNTP_CVAL_EL0) = cval;
+		__vcpu_assign_sys_reg(vcpu, CNTP_CVAL_EL0, cval);
 		break;
 	case TIMER_HVTIMER:
-		__vcpu_sys_reg(vcpu, CNTHV_CVAL_EL2) = cval;
+		__vcpu_assign_sys_reg(vcpu, CNTHV_CVAL_EL2, cval);
 		break;
 	case TIMER_HPTIMER:
-		__vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2) = cval;
+		__vcpu_assign_sys_reg(vcpu, CNTHP_CVAL_EL2, cval);
 		break;
 	default:
 		WARN_ON(1);
diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
index 424a5107cddb5..6a2a899a344e6 100644
--- a/arch/arm64/kvm/hyp/exception.c
+++ b/arch/arm64/kvm/hyp/exception.c
@@ -37,7 +37,7 @@ static inline void __vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
 	if (unlikely(vcpu_has_nv(vcpu)))
 		vcpu_write_sys_reg(vcpu, val, reg);
 	else if (!__vcpu_write_sys_reg_to_cpu(val, reg))
-		__vcpu_sys_reg(vcpu, reg) = val;
+		__vcpu_assign_sys_reg(vcpu, reg, val);
 }
 
 static void __vcpu_write_spsr(struct kvm_vcpu *vcpu, unsigned long target_mode,
@@ -51,7 +51,7 @@ static void __vcpu_write_spsr(struct kvm_vcpu *vcpu, unsigned long target_mode,
 	} else if (has_vhe()) {
 		write_sysreg_el1(val, SYS_SPSR);
 	} else {
-		__vcpu_sys_reg(vcpu, SPSR_EL1) = val;
+		__vcpu_assign_sys_reg(vcpu, SPSR_EL1, val);
 	}
 }
 
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index eef310cdbdbd5..aa5b561b92182 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -45,7 +45,7 @@ static inline void __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
 	if (!vcpu_el1_is_32bit(vcpu))
 		return;
 
-	__vcpu_sys_reg(vcpu, FPEXC32_EL2) = read_sysreg(fpexc32_el2);
+	__vcpu_assign_sys_reg(vcpu, FPEXC32_EL2, read_sysreg(fpexc32_el2));
 }
 
 static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
@@ -457,7 +457,7 @@ static inline void fpsimd_lazy_switch_to_host(struct kvm_vcpu *vcpu)
 	 */
 	if (vcpu_has_sve(vcpu)) {
 		zcr_el1 = read_sysreg_el1(SYS_ZCR);
-		__vcpu_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu)) = zcr_el1;
+		__vcpu_assign_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu), zcr_el1);
 
 		/*
 		 * The guest's state is always saved using the guest's max VL.
diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
index b9cff893bbe0b..4d0dbea4c56f7 100644
--- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
@@ -307,11 +307,11 @@ static inline void __sysreg32_save_state(struct kvm_vcpu *vcpu)
 	vcpu->arch.ctxt.spsr_irq = read_sysreg(spsr_irq);
 	vcpu->arch.ctxt.spsr_fiq = read_sysreg(spsr_fiq);
 
-	__vcpu_sys_reg(vcpu, DACR32_EL2) = read_sysreg(dacr32_el2);
-	__vcpu_sys_reg(vcpu, IFSR32_EL2) = read_sysreg(ifsr32_el2);
+	__vcpu_assign_sys_reg(vcpu, DACR32_EL2, read_sysreg(dacr32_el2));
+	__vcpu_assign_sys_reg(vcpu, IFSR32_EL2, read_sysreg(ifsr32_el2));
 
 	if (has_vhe() || kvm_debug_regs_in_use(vcpu))
-		__vcpu_sys_reg(vcpu, DBGVCR32_EL2) = read_sysreg(dbgvcr32_el2);
+		__vcpu_assign_sys_reg(vcpu, DBGVCR32_EL2, read_sysreg(dbgvcr32_el2));
 }
 
 static inline void __sysreg32_restore_state(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 8e8848de4d470..e9198e56e784b 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -26,7 +26,7 @@ void __kvm_hyp_host_forward_smc(struct kvm_cpu_context *host_ctxt);
 
 static void __hyp_sve_save_guest(struct kvm_vcpu *vcpu)
 {
-	__vcpu_sys_reg(vcpu, ZCR_EL1) = read_sysreg_el1(SYS_ZCR);
+	__vcpu_assign_sys_reg(vcpu, ZCR_EL1, read_sysreg_el1(SYS_ZCR));
 	/*
 	 * On saving/restoring guest sve state, always use the maximum VL for
 	 * the guest. The layout of the data when saving the sve state depends
@@ -79,7 +79,7 @@ static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
 
 	has_fpmr = kvm_has_fpmr(kern_hyp_va(vcpu->kvm));
 	if (has_fpmr)
-		__vcpu_sys_reg(vcpu, FPMR) = read_sysreg_s(SYS_FPMR);
+		__vcpu_assign_sys_reg(vcpu, FPMR, read_sysreg_s(SYS_FPMR));
 
 	if (system_supports_sve())
 		__hyp_sve_restore_host();
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index c9b330dc20669..09df2b42bc1b7 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -223,9 +223,9 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
 		 */
 		val = read_sysreg_el0(SYS_CNTP_CVAL);
 		if (map.direct_ptimer == vcpu_ptimer(vcpu))
-			__vcpu_sys_reg(vcpu, CNTP_CVAL_EL0) = val;
+			__vcpu_assign_sys_reg(vcpu, CNTP_CVAL_EL0, val);
 		if (map.direct_ptimer == vcpu_hptimer(vcpu))
-			__vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2) = val;
+			__vcpu_assign_sys_reg(vcpu, CNTHP_CVAL_EL2, val);
 
 		offset = read_sysreg_s(SYS_CNTPOFF_EL2);
 
diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
index 3814b0b2c937f..34c7bf7fe9def 100644
--- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
@@ -18,17 +18,17 @@
 static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
 {
 	/* These registers are common with EL1 */
-	__vcpu_sys_reg(vcpu, PAR_EL1)	= read_sysreg(par_el1);
-	__vcpu_sys_reg(vcpu, TPIDR_EL1)	= read_sysreg(tpidr_el1);
-
-	__vcpu_sys_reg(vcpu, ESR_EL2)	= read_sysreg_el1(SYS_ESR);
-	__vcpu_sys_reg(vcpu, AFSR0_EL2)	= read_sysreg_el1(SYS_AFSR0);
-	__vcpu_sys_reg(vcpu, AFSR1_EL2)	= read_sysreg_el1(SYS_AFSR1);
-	__vcpu_sys_reg(vcpu, FAR_EL2)	= read_sysreg_el1(SYS_FAR);
-	__vcpu_sys_reg(vcpu, MAIR_EL2)	= read_sysreg_el1(SYS_MAIR);
-	__vcpu_sys_reg(vcpu, VBAR_EL2)	= read_sysreg_el1(SYS_VBAR);
-	__vcpu_sys_reg(vcpu, CONTEXTIDR_EL2) = read_sysreg_el1(SYS_CONTEXTIDR);
-	__vcpu_sys_reg(vcpu, AMAIR_EL2)	= read_sysreg_el1(SYS_AMAIR);
+	__vcpu_assign_sys_reg(vcpu, PAR_EL1,	 read_sysreg(par_el1));
+	__vcpu_assign_sys_reg(vcpu, TPIDR_EL1,	 read_sysreg(tpidr_el1));
+
+	__vcpu_assign_sys_reg(vcpu, ESR_EL2,	 read_sysreg_el1(SYS_ESR));
+	__vcpu_assign_sys_reg(vcpu, AFSR0_EL2,	 read_sysreg_el1(SYS_AFSR0));
+	__vcpu_assign_sys_reg(vcpu, AFSR1_EL2,	 read_sysreg_el1(SYS_AFSR1));
+	__vcpu_assign_sys_reg(vcpu, FAR_EL2,	 read_sysreg_el1(SYS_FAR));
+	__vcpu_assign_sys_reg(vcpu, MAIR_EL2,	 read_sysreg_el1(SYS_MAIR));
+	__vcpu_assign_sys_reg(vcpu, VBAR_EL2,	 read_sysreg_el1(SYS_VBAR));
+	__vcpu_assign_sys_reg(vcpu, CONTEXTIDR_EL2, read_sysreg_el1(SYS_CONTEXTIDR));
+	__vcpu_assign_sys_reg(vcpu, AMAIR_EL2,	 read_sysreg_el1(SYS_AMAIR));
 
 	/*
 	 * In VHE mode those registers are compatible between EL1 and EL2,
@@ -46,21 +46,21 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
 		 * are always trapped, ensuring that the in-memory
 		 * copy is always up-to-date. A small blessing...
 		 */
-		__vcpu_sys_reg(vcpu, SCTLR_EL2)	= read_sysreg_el1(SYS_SCTLR);
-		__vcpu_sys_reg(vcpu, TTBR0_EL2)	= read_sysreg_el1(SYS_TTBR0);
-		__vcpu_sys_reg(vcpu, TTBR1_EL2)	= read_sysreg_el1(SYS_TTBR1);
-		__vcpu_sys_reg(vcpu, TCR_EL2)	= read_sysreg_el1(SYS_TCR);
+		__vcpu_assign_sys_reg(vcpu, SCTLR_EL2,	 read_sysreg_el1(SYS_SCTLR));
+		__vcpu_assign_sys_reg(vcpu, TTBR0_EL2,	 read_sysreg_el1(SYS_TTBR0));
+		__vcpu_assign_sys_reg(vcpu, TTBR1_EL2,	 read_sysreg_el1(SYS_TTBR1));
+		__vcpu_assign_sys_reg(vcpu, TCR_EL2,	 read_sysreg_el1(SYS_TCR));
 
 		if (ctxt_has_tcrx(&vcpu->arch.ctxt)) {
-			__vcpu_sys_reg(vcpu, TCR2_EL2) = read_sysreg_el1(SYS_TCR2);
+			__vcpu_assign_sys_reg(vcpu, TCR2_EL2, read_sysreg_el1(SYS_TCR2));
 
 			if (ctxt_has_s1pie(&vcpu->arch.ctxt)) {
-				__vcpu_sys_reg(vcpu, PIRE0_EL2) = read_sysreg_el1(SYS_PIRE0);
-				__vcpu_sys_reg(vcpu, PIR_EL2) = read_sysreg_el1(SYS_PIR);
+				__vcpu_assign_sys_reg(vcpu, PIRE0_EL2, read_sysreg_el1(SYS_PIRE0));
+				__vcpu_assign_sys_reg(vcpu, PIR_EL2, read_sysreg_el1(SYS_PIR));
 			}
 
 			if (ctxt_has_s1poe(&vcpu->arch.ctxt))
-				__vcpu_sys_reg(vcpu, POR_EL2) = read_sysreg_el1(SYS_POR);
+				__vcpu_assign_sys_reg(vcpu, POR_EL2, read_sysreg_el1(SYS_POR));
 		}
 
 		/*
@@ -74,9 +74,9 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
 		__vcpu_sys_reg(vcpu, CNTHCTL_EL2) |= val;
 	}
 
-	__vcpu_sys_reg(vcpu, SP_EL2)	= read_sysreg(sp_el1);
-	__vcpu_sys_reg(vcpu, ELR_EL2)	= read_sysreg_el1(SYS_ELR);
-	__vcpu_sys_reg(vcpu, SPSR_EL2)	= read_sysreg_el1(SYS_SPSR);
+	__vcpu_assign_sys_reg(vcpu, SP_EL2,	 read_sysreg(sp_el1));
+	__vcpu_assign_sys_reg(vcpu, ELR_EL2,	 read_sysreg_el1(SYS_ELR));
+	__vcpu_assign_sys_reg(vcpu, SPSR_EL2,	 read_sysreg_el1(SYS_SPSR));
 }
 
 static void __sysreg_restore_vel2_state(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 25c29107f13fd..4f0ae8073f788 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -178,7 +178,7 @@ static void kvm_pmu_set_pmc_value(struct kvm_pmc *pmc, u64 val, bool force)
 		val |= lower_32_bits(val);
 	}
 
-	__vcpu_sys_reg(vcpu, reg) = val;
+	__vcpu_assign_sys_reg(vcpu, reg, val);
 
 	/* Recreate the perf event to reflect the updated sample_period */
 	kvm_pmu_create_perf_event(pmc);
@@ -204,7 +204,7 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
 void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
 {
 	kvm_pmu_release_perf_event(kvm_vcpu_idx_to_pmc(vcpu, select_idx));
-	__vcpu_sys_reg(vcpu, counter_index_to_reg(select_idx)) = val;
+	__vcpu_assign_sys_reg(vcpu, counter_index_to_reg(select_idx), val);
 	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
 }
 
@@ -239,7 +239,7 @@ static void kvm_pmu_stop_counter(struct kvm_pmc *pmc)
 
 	reg = counter_index_to_reg(pmc->idx);
 
-	__vcpu_sys_reg(vcpu, reg) = val;
+	__vcpu_assign_sys_reg(vcpu, reg, val);
 
 	kvm_pmu_release_perf_event(pmc);
 }
@@ -503,7 +503,7 @@ static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
 		reg = __vcpu_sys_reg(vcpu, counter_index_to_reg(i)) + 1;
 		if (!kvm_pmc_is_64bit(pmc))
 			reg = lower_32_bits(reg);
-		__vcpu_sys_reg(vcpu, counter_index_to_reg(i)) = reg;
+		__vcpu_assign_sys_reg(vcpu, counter_index_to_reg(i), reg);
 
 		/* No overflow? move on */
 		if (kvm_pmc_has_64bit_overflow(pmc) ? reg : lower_32_bits(reg))
@@ -602,7 +602,7 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
 		kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
 
 	/* The reset bits don't indicate any state, and shouldn't be saved. */
-	__vcpu_sys_reg(vcpu, PMCR_EL0) = val & ~(ARMV8_PMU_PMCR_C | ARMV8_PMU_PMCR_P);
+	__vcpu_assign_sys_reg(vcpu, PMCR_EL0, (val & ~(ARMV8_PMU_PMCR_C | ARMV8_PMU_PMCR_P)));
 
 	if (val & ARMV8_PMU_PMCR_C)
 		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
@@ -779,7 +779,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
 	u64 reg;
 
 	reg = counter_index_to_evtreg(pmc->idx);
-	__vcpu_sys_reg(vcpu, reg) = data & kvm_pmu_evtyper_mask(vcpu->kvm);
+	__vcpu_assign_sys_reg(vcpu, reg, (data & kvm_pmu_evtyper_mask(vcpu->kvm)));
 
 	kvm_pmu_create_perf_event(pmc);
 }
@@ -1038,7 +1038,7 @@ static void kvm_arm_set_nr_counters(struct kvm *kvm, unsigned int nr)
 			u64 val = __vcpu_sys_reg(vcpu, MDCR_EL2);
 			val &= ~MDCR_EL2_HPMN;
 			val |= FIELD_PREP(MDCR_EL2_HPMN, kvm->arch.nr_pmu_counters);
-			__vcpu_sys_reg(vcpu, MDCR_EL2) = val;
+			__vcpu_assign_sys_reg(vcpu, MDCR_EL2, val);
 		}
 	}
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 707c651aff031..93d0ca7ed9365 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -228,7 +228,7 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
 		 * to reverse-translate virtual EL2 system registers for a
 		 * non-VHE guest hypervisor.
 		 */
-		__vcpu_sys_reg(vcpu, reg) = val;
+		__vcpu_assign_sys_reg(vcpu, reg, val);
 
 		switch (reg) {
 		case CNTHCTL_EL2:
@@ -263,7 +263,7 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
 		return;
 
 memory_write:
-	 __vcpu_sys_reg(vcpu, reg) = val;
+	__vcpu_assign_sys_reg(vcpu, reg, val);
 }
 
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
@@ -605,7 +605,7 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	if ((val ^ rd->val) & ~OSLSR_EL1_OSLK)
 		return -EINVAL;
 
-	__vcpu_sys_reg(vcpu, rd->reg) = val;
+	__vcpu_assign_sys_reg(vcpu, rd->reg, val);
 	return 0;
 }
 
@@ -835,7 +835,7 @@ static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	 * The value of PMCR.N field is included when the
 	 * vCPU register is read via kvm_vcpu_read_pmcr().
 	 */
-	__vcpu_sys_reg(vcpu, r->reg) = pmcr;
+	__vcpu_assign_sys_reg(vcpu, r->reg, pmcr);
 
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
@@ -907,7 +907,7 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		return false;
 
 	if (p->is_write)
-		__vcpu_sys_reg(vcpu, PMSELR_EL0) = p->regval;
+		__vcpu_assign_sys_reg(vcpu, PMSELR_EL0, p->regval);
 	else
 		/* return PMSELR.SEL field */
 		p->regval = __vcpu_sys_reg(vcpu, PMSELR_EL0)
@@ -1076,7 +1076,7 @@ static int set_pmreg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r, u64 va
 {
 	u64 mask = kvm_pmu_accessible_counter_mask(vcpu);
 
-	__vcpu_sys_reg(vcpu, r->reg) = val & mask;
+	__vcpu_assign_sys_reg(vcpu, r->reg, val & mask);
 	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
 
 	return 0;
@@ -1185,8 +1185,8 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		if (!vcpu_mode_priv(vcpu))
 			return undef_access(vcpu, p, r);
 
-		__vcpu_sys_reg(vcpu, PMUSERENR_EL0) =
-			       p->regval & ARMV8_PMU_USERENR_MASK;
+		__vcpu_assign_sys_reg(vcpu, PMUSERENR_EL0,
+				      (p->regval & ARMV8_PMU_USERENR_MASK));
 	} else {
 		p->regval = __vcpu_sys_reg(vcpu, PMUSERENR_EL0)
 			    & ARMV8_PMU_USERENR_MASK;
@@ -1237,7 +1237,7 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
 	if (!kvm_supports_32bit_el0())
 		val |= ARMV8_PMU_PMCR_LC;
 
-	__vcpu_sys_reg(vcpu, r->reg) = val;
+	__vcpu_assign_sys_reg(vcpu, r->reg, val);
 	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
 
 	return 0;
@@ -2207,7 +2207,7 @@ static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	if (kvm_has_mte(vcpu->kvm))
 		clidr |= 2ULL << CLIDR_TTYPE_SHIFT(loc);
 
-	__vcpu_sys_reg(vcpu, r->reg) = clidr;
+	__vcpu_assign_sys_reg(vcpu, r->reg, clidr);
 
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
@@ -2221,7 +2221,7 @@ static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	if ((val & CLIDR_EL1_RES0) || (!(ctr_el0 & CTR_EL0_IDC) && idc))
 		return -EINVAL;
 
-	__vcpu_sys_reg(vcpu, rd->reg) = val;
+	__vcpu_assign_sys_reg(vcpu, rd->reg, val);
 
 	return 0;
 }
@@ -2398,7 +2398,7 @@ static bool access_sp_el1(struct kvm_vcpu *vcpu,
 			  const struct sys_reg_desc *r)
 {
 	if (p->is_write)
-		__vcpu_sys_reg(vcpu, SP_EL1) = p->regval;
+		__vcpu_assign_sys_reg(vcpu, SP_EL1, p->regval);
 	else
 		p->regval = __vcpu_sys_reg(vcpu, SP_EL1);
 
@@ -2422,7 +2422,7 @@ static bool access_spsr(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *r)
 {
 	if (p->is_write)
-		__vcpu_sys_reg(vcpu, SPSR_EL1) = p->regval;
+		__vcpu_assign_sys_reg(vcpu, SPSR_EL1, p->regval);
 	else
 		p->regval = __vcpu_sys_reg(vcpu, SPSR_EL1);
 
@@ -2434,7 +2434,7 @@ static bool access_cntkctl_el12(struct kvm_vcpu *vcpu,
 				const struct sys_reg_desc *r)
 {
 	if (p->is_write)
-		__vcpu_sys_reg(vcpu, CNTKCTL_EL1) = p->regval;
+		__vcpu_assign_sys_reg(vcpu, CNTKCTL_EL1, p->regval);
 	else
 		p->regval = __vcpu_sys_reg(vcpu, CNTKCTL_EL1);
 
@@ -2448,7 +2448,9 @@ static u64 reset_hcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	if (!cpus_have_final_cap(ARM64_HAS_HCR_NV1))
 		val |= HCR_E2H;
 
-	return __vcpu_sys_reg(vcpu, r->reg) = val;
+	__vcpu_assign_sys_reg(vcpu, r->reg, val);
+
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
 static unsigned int __el2_visibility(const struct kvm_vcpu *vcpu,
@@ -2619,7 +2621,7 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
 		u64_replace_bits(val, hpmn, MDCR_EL2_HPMN);
 	}
 
-	__vcpu_sys_reg(vcpu, MDCR_EL2) = val;
+	__vcpu_assign_sys_reg(vcpu, MDCR_EL2, val);
 
 	/*
 	 * Request a reload of the PMU to enable/disable the counters
@@ -2748,7 +2750,7 @@ static int set_imp_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
 
 static u64 reset_mdcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
-	__vcpu_sys_reg(vcpu, r->reg) = vcpu->kvm->arch.nr_pmu_counters;
+	__vcpu_assign_sys_reg(vcpu, r->reg, vcpu->kvm->arch.nr_pmu_counters);
 	return vcpu->kvm->arch.nr_pmu_counters;
 }
 
@@ -5006,7 +5008,7 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
 	if (r->set_user) {
 		ret = (r->set_user)(vcpu, r, val);
 	} else {
-		__vcpu_sys_reg(vcpu, r->reg) = val;
+		__vcpu_assign_sys_reg(vcpu, r->reg, val);
 		ret = 0;
 	}
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index cc6338d387663..ef97d9fc67cc5 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -137,7 +137,7 @@ static inline u64 reset_unknown(struct kvm_vcpu *vcpu,
 {
 	BUG_ON(!r->reg);
 	BUG_ON(r->reg >= NR_SYS_REGS);
-	__vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL;
+	__vcpu_assign_sys_reg(vcpu, r->reg, 0x1de7ec7edbadc0deULL);
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
@@ -145,7 +145,7 @@ static inline u64 reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	BUG_ON(!r->reg);
 	BUG_ON(r->reg >= NR_SYS_REGS);
-	__vcpu_sys_reg(vcpu, r->reg) = r->val;
+	__vcpu_assign_sys_reg(vcpu, r->reg, r->val);
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
index 4f6954c306747..d22a8ad7bcc51 100644
--- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
+++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
@@ -356,12 +356,12 @@ void vgic_v3_put_nested(struct kvm_vcpu *vcpu)
 	val = __vcpu_sys_reg(vcpu, ICH_HCR_EL2);
 	val &= ~ICH_HCR_EL2_EOIcount_MASK;
 	val |= (s_cpu_if->vgic_hcr & ICH_HCR_EL2_EOIcount_MASK);
-	__vcpu_sys_reg(vcpu, ICH_HCR_EL2) = val;
-	__vcpu_sys_reg(vcpu, ICH_VMCR_EL2) = s_cpu_if->vgic_vmcr;
+	__vcpu_assign_sys_reg(vcpu, ICH_HCR_EL2, val);
+	__vcpu_assign_sys_reg(vcpu, ICH_VMCR_EL2, s_cpu_if->vgic_vmcr);
 
 	for (i = 0; i < 4; i++) {
-		__vcpu_sys_reg(vcpu, ICH_AP0RN(i)) = s_cpu_if->vgic_ap0r[i];
-		__vcpu_sys_reg(vcpu, ICH_AP1RN(i)) = s_cpu_if->vgic_ap1r[i];
+		__vcpu_assign_sys_reg(vcpu, ICH_AP0RN(i), s_cpu_if->vgic_ap0r[i]);
+		__vcpu_assign_sys_reg(vcpu, ICH_AP1RN(i), s_cpu_if->vgic_ap1r[i]);
 	}
 
 	for_each_set_bit(i, &shadow_if->lr_map, kvm_vgic_global_state.nr_lr) {
@@ -370,7 +370,7 @@ void vgic_v3_put_nested(struct kvm_vcpu *vcpu)
 		val &= ~ICH_LR_STATE;
 		val |= s_cpu_if->vgic_lr[i] & ICH_LR_STATE;
 
-		__vcpu_sys_reg(vcpu, ICH_LRN(i)) = val;
+		__vcpu_assign_sys_reg(vcpu, ICH_LRN(i), val);
 		s_cpu_if->vgic_lr[i] = 0;
 	}
 
-- 
2.39.2

[PATCH v2 2/4] KVM: arm64: Add RMW specific sysreg accessor

[Marc Zyngier]

In a number of cases, we perform a Read-Modify-Write operation on
a system register, meaning that we would apply the RESx masks twice.

Instead, provide a new accessor that performs this RMW operation,
allowing the masks to be applied exactly once per operation.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h  | 10 ++++++++++
 arch/arm64/kvm/debug.c             |  4 ++--
 arch/arm64/kvm/hyp/vhe/sysreg-sr.c |  4 ++--
 arch/arm64/kvm/nested.c            |  2 +-
 arch/arm64/kvm/pmu-emul.c          | 10 +++++-----
 arch/arm64/kvm/sys_regs.c          | 22 +++++++++++-----------
 6 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3b84ad91116b4..c361f8aa16c90 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1118,6 +1118,16 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 		ctxt_sys_reg(ctxt, (r)) = __v;				\
 	} while (0)
 
+#define __vcpu_rmw_sys_reg(v, r, op, val)				\
+	do {								\
+		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
+		u64 __v = ctxt_sys_reg(ctxt, (r)) op (val);		\
+		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)	\
+			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);	\
+									\
+		ctxt_sys_reg(ctxt, (r)) = __v;				\
+	} while (0)
+
 #define __vcpu_sys_reg(v,r)						\
 	(*({								\
 		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 0e4c805e7e891..1a7dab333f557 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -216,9 +216,9 @@ void kvm_debug_set_guest_ownership(struct kvm_vcpu *vcpu)
 void kvm_debug_handle_oslar(struct kvm_vcpu *vcpu, u64 val)
 {
 	if (val & OSLAR_EL1_OSLK)
-		__vcpu_sys_reg(vcpu, OSLSR_EL1) |= OSLSR_EL1_OSLK;
+		__vcpu_rmw_sys_reg(vcpu, OSLSR_EL1, |=, OSLSR_EL1_OSLK);
 	else
-		__vcpu_sys_reg(vcpu, OSLSR_EL1) &= ~OSLSR_EL1_OSLK;
+		__vcpu_rmw_sys_reg(vcpu, OSLSR_EL1, &=, ~OSLSR_EL1_OSLK);
 
 	preempt_disable();
 	kvm_arch_vcpu_put(vcpu);
diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
index 34c7bf7fe9def..73e4bc7fde9e4 100644
--- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
@@ -70,8 +70,8 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
 		 */
 		val = read_sysreg_el1(SYS_CNTKCTL);
 		val &= CNTKCTL_VALID_BITS;
-		__vcpu_sys_reg(vcpu, CNTHCTL_EL2) &= ~CNTKCTL_VALID_BITS;
-		__vcpu_sys_reg(vcpu, CNTHCTL_EL2) |= val;
+		__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, &=, ~CNTKCTL_VALID_BITS);
+		__vcpu_rmw_sys_reg(vcpu, CNTHCTL_EL2, |=, val);
 	}
 
 	__vcpu_assign_sys_reg(vcpu, SP_EL2,	 read_sysreg(sp_el1));
diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index 4a53e4147fb01..5b191f4dc5668 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -1757,7 +1757,7 @@ int kvm_init_nv_sysregs(struct kvm_vcpu *vcpu)
 
 out:
 	for (enum vcpu_sysreg sr = __SANITISED_REG_START__; sr < NR_SYS_REGS; sr++)
-		(void)__vcpu_sys_reg(vcpu, sr);
+		__vcpu_rmw_sys_reg(vcpu, sr, |=, 0);
 
 	return 0;
 }
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 4f0ae8073f788..b03dbda7f1ab9 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -510,7 +510,7 @@ static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
 			continue;
 
 		/* Mark overflow */
-		__vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
+		__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, |=, BIT(i));
 
 		if (kvm_pmu_counter_can_chain(pmc))
 			kvm_pmu_counter_increment(vcpu, BIT(i + 1),
@@ -556,7 +556,7 @@ static void kvm_pmu_perf_overflow(struct perf_event *perf_event,
 	perf_event->attr.sample_period = period;
 	perf_event->hw.sample_period = period;
 
-	__vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(idx);
+	__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, |=, BIT(idx));
 
 	if (kvm_pmu_counter_can_chain(pmc))
 		kvm_pmu_counter_increment(vcpu, BIT(idx + 1),
@@ -914,9 +914,9 @@ void kvm_vcpu_reload_pmu(struct kvm_vcpu *vcpu)
 {
 	u64 mask = kvm_pmu_implemented_counter_mask(vcpu);
 
-	__vcpu_sys_reg(vcpu, PMOVSSET_EL0) &= mask;
-	__vcpu_sys_reg(vcpu, PMINTENSET_EL1) &= mask;
-	__vcpu_sys_reg(vcpu, PMCNTENSET_EL0) &= mask;
+	__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, &=, mask);
+	__vcpu_rmw_sys_reg(vcpu, PMINTENSET_EL1, &=, mask);
+	__vcpu_rmw_sys_reg(vcpu, PMCNTENSET_EL0, &=, mask);
 
 	kvm_pmu_reprogram_counter_mask(vcpu, mask);
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 93d0ca7ed9365..e89d345e42d09 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -791,7 +791,7 @@ static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 		mask |= GENMASK(n - 1, 0);
 
 	reset_unknown(vcpu, r);
-	__vcpu_sys_reg(vcpu, r->reg) &= mask;
+	__vcpu_rmw_sys_reg(vcpu, r->reg, &=, mask);
 
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
@@ -799,7 +799,7 @@ static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	reset_unknown(vcpu, r);
-	__vcpu_sys_reg(vcpu, r->reg) &= GENMASK(31, 0);
+	__vcpu_rmw_sys_reg(vcpu, r->reg, &=, GENMASK(31, 0));
 
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
@@ -811,7 +811,7 @@ static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 		return 0;
 
 	reset_unknown(vcpu, r);
-	__vcpu_sys_reg(vcpu, r->reg) &= kvm_pmu_evtyper_mask(vcpu->kvm);
+	__vcpu_rmw_sys_reg(vcpu, r->reg, &=, kvm_pmu_evtyper_mask(vcpu->kvm));
 
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
@@ -819,7 +819,7 @@ static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 static u64 reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	reset_unknown(vcpu, r);
-	__vcpu_sys_reg(vcpu, r->reg) &= PMSELR_EL0_SEL_MASK;
+	__vcpu_rmw_sys_reg(vcpu, r->reg, &=, PMSELR_EL0_SEL_MASK);
 
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
@@ -1103,10 +1103,10 @@ static bool access_pmcnten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		val = p->regval & mask;
 		if (r->Op2 & 0x1)
 			/* accessing PMCNTENSET_EL0 */
-			__vcpu_sys_reg(vcpu, PMCNTENSET_EL0) |= val;
+			__vcpu_rmw_sys_reg(vcpu, PMCNTENSET_EL0, |=, val);
 		else
 			/* accessing PMCNTENCLR_EL0 */
-			__vcpu_sys_reg(vcpu, PMCNTENSET_EL0) &= ~val;
+			__vcpu_rmw_sys_reg(vcpu, PMCNTENSET_EL0, &=, ~val);
 
 		kvm_pmu_reprogram_counter_mask(vcpu, val);
 	} else {
@@ -1129,10 +1129,10 @@ static bool access_pminten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 
 		if (r->Op2 & 0x1)
 			/* accessing PMINTENSET_EL1 */
-			__vcpu_sys_reg(vcpu, PMINTENSET_EL1) |= val;
+			__vcpu_rmw_sys_reg(vcpu, PMINTENSET_EL1, |=, val);
 		else
 			/* accessing PMINTENCLR_EL1 */
-			__vcpu_sys_reg(vcpu, PMINTENSET_EL1) &= ~val;
+			__vcpu_rmw_sys_reg(vcpu, PMINTENSET_EL1, &=, ~val);
 	} else {
 		p->regval = __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
 	}
@@ -1151,10 +1151,10 @@ static bool access_pmovs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (p->is_write) {
 		if (r->CRm & 0x2)
 			/* accessing PMOVSSET_EL0 */
-			__vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= (p->regval & mask);
+			__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, |=, (p->regval & mask));
 		else
 			/* accessing PMOVSCLR_EL0 */
-			__vcpu_sys_reg(vcpu, PMOVSSET_EL0) &= ~(p->regval & mask);
+			__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, &=, ~(p->regval & mask));
 	} else {
 		p->regval = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
 	}
@@ -4786,7 +4786,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
 			r->reset(vcpu, r);
 
 		if (r->reg >= __SANITISED_REG_START__ && r->reg < NR_SYS_REGS)
-			(void)__vcpu_sys_reg(vcpu, r->reg);
+			__vcpu_rmw_sys_reg(vcpu, r->reg, |=, 0);
 	}
 
 	set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags);
-- 
2.39.2

[PATCH v2 3/4] KVM: arm64: Don't use __vcpu_sys_reg() to get the address of a sysreg

[Marc Zyngier]

We are about to prevent the use of __vcpu_sys_reg() as a lvalue,
and getting the address of a rvalue is not a thing.

Update the couple of places where we do this to use the __ctxt_sys_reg()
accessor, which return the address of a register.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arch_timer.c | 2 +-
 arch/arm64/kvm/fpsimd.c     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 5a67a6d4d95b7..c6b4fb4c8e08f 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -1036,7 +1036,7 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
 	if (vcpu_has_nv(vcpu)) {
 		struct arch_timer_offset *offs = &vcpu_vtimer(vcpu)->offset;
 
-		offs->vcpu_offset = &__vcpu_sys_reg(vcpu, CNTVOFF_EL2);
+		offs->vcpu_offset = __ctxt_sys_reg(&vcpu->arch.ctxt, CNTVOFF_EL2);
 		offs->vm_offset = &vcpu->kvm->arch.timer_data.poffset;
 	}
 
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 7f6e43d256915..8f6c8f57c6b9c 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -103,8 +103,8 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
 		fp_state.sve_state = vcpu->arch.sve_state;
 		fp_state.sve_vl = vcpu->arch.sve_max_vl;
 		fp_state.sme_state = NULL;
-		fp_state.svcr = &__vcpu_sys_reg(vcpu, SVCR);
-		fp_state.fpmr = &__vcpu_sys_reg(vcpu, FPMR);
+		fp_state.svcr = __ctxt_sys_reg(&vcpu->arch.ctxt, SVCR);
+		fp_state.fpmr = __ctxt_sys_reg(&vcpu->arch.ctxt, FPMR);
 		fp_state.fp_type = &vcpu->arch.fp_type;
 
 		if (vcpu_has_sve(vcpu))
-- 
2.39.2

[PATCH v2 4/4] KVM: arm64: Make __vcpu_sys_reg() a pure rvalue operand

[Marc Zyngier]

Now that we don't have any use of __vcpu_sys_reg() as a lvalue,
remove the in-place update, and directly return the sanitised
value.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c361f8aa16c90..b4ac2f515f94f 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1129,13 +1129,13 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 	} while (0)
 
 #define __vcpu_sys_reg(v,r)						\
-	(*({								\
+	({								\
 		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
-		u64 *__r = __ctxt_sys_reg(ctxt, (r));			\
+		u64 __v = ctxt_sys_reg(ctxt, (r));			\
 		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)	\
-			*__r = kvm_vcpu_apply_reg_masks((v), (r), *__r);\
-		__r;							\
-	}))
+			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);	\
+		__v;							\
+	})
 
 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);
-- 
2.39.2

Re: [PATCH v2 1/4] KVM: arm64: Add assignment-specific sysreg accessor

[Miguel Luis]

Hi Marc,

> On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
> 
> Assigning a value to a system register doesn't do what it is
> supposed to be doing if that register is one that has RESx bits.
> 
> The main problem is that we use __vcpu_sys_reg(), which can be used
> both as a lvalue and rvalue. When used as a lvalue, the bit masking
> occurs *before* the new value is assigned, meaning that we (1) do
> pointless work on the old cvalue, and (2) potentially assign an
> invalid value as we fail to apply the masks to it.
> 
> Fix this by providing a new __vcpu_assign_sys_reg() that does
> what it says on the tin, and sanitises the *new* value instead of
> the old one. This comes with a significant amount of churn.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/include/asm/kvm_host.h          | 11 ++++++
> arch/arm64/kvm/arch_timer.c                | 16 ++++----
> arch/arm64/kvm/hyp/exception.c             |  4 +-
> arch/arm64/kvm/hyp/include/hyp/switch.h    |  4 +-
> arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  6 +--
> arch/arm64/kvm/hyp/nvhe/hyp-main.c         |  4 +-
> arch/arm64/kvm/hyp/vhe/switch.c            |  4 +-
> arch/arm64/kvm/hyp/vhe/sysreg-sr.c         | 44 +++++++++++-----------
> arch/arm64/kvm/pmu-emul.c                  | 14 +++----
> arch/arm64/kvm/sys_regs.c                  | 38 ++++++++++---------
> arch/arm64/kvm/sys_regs.h                  |  4 +-
> arch/arm64/kvm/vgic/vgic-v3-nested.c       | 10 ++---
> 12 files changed, 86 insertions(+), 73 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d941abc6b5eef..3b84ad91116b4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1107,6 +1107,17 @@ 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))
> 
> u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
> +
> +#define __vcpu_assign_sys_reg(v, r, val) \
> + do { \
> + const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt; \
> + u64 __v = (val); \
> + if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__) \

Would it make sense to make it more strict by testing < NR_SYS_REGS too?

Thanks
Miguel

> + __v = kvm_vcpu_apply_reg_masks((v), (r), __v); \
> + \
> + ctxt_sys_reg(ctxt, (r)) = __v; \
> + } while (0)
> +
> #define __vcpu_sys_reg(v,r) \
> (*({ \
> const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt; \
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 5133dcbfe9f76..5a67a6d4d95b7 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -108,16 +108,16 @@ static void timer_set_ctl(struct arch_timer_context *ctxt, u32 ctl)
> 
> switch(arch_timer_ctx_index(ctxt)) {
> case TIMER_VTIMER:
> - __vcpu_sys_reg(vcpu, CNTV_CTL_EL0) = ctl;
> + __vcpu_assign_sys_reg(vcpu, CNTV_CTL_EL0, ctl);
> break;
> case TIMER_PTIMER:
> - __vcpu_sys_reg(vcpu, CNTP_CTL_EL0) = ctl;
> + __vcpu_assign_sys_reg(vcpu, CNTP_CTL_EL0, ctl);
> break;
> case TIMER_HVTIMER:
> - __vcpu_sys_reg(vcpu, CNTHV_CTL_EL2) = ctl;
> + __vcpu_assign_sys_reg(vcpu, CNTHV_CTL_EL2, ctl);
> break;
> case TIMER_HPTIMER:
> - __vcpu_sys_reg(vcpu, CNTHP_CTL_EL2) = ctl;
> + __vcpu_assign_sys_reg(vcpu, CNTHP_CTL_EL2, ctl);
> break;
> default:
> WARN_ON(1);
> @@ -130,16 +130,16 @@ static void timer_set_cval(struct arch_timer_context *ctxt, u64 cval)
> 
> switch(arch_timer_ctx_index(ctxt)) {
> case TIMER_VTIMER:
> - __vcpu_sys_reg(vcpu, CNTV_CVAL_EL0) = cval;
> + __vcpu_assign_sys_reg(vcpu, CNTV_CVAL_EL0, cval);
> break;
> case TIMER_PTIMER:
> - __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0) = cval;
> + __vcpu_assign_sys_reg(vcpu, CNTP_CVAL_EL0, cval);
> break;
> case TIMER_HVTIMER:
> - __vcpu_sys_reg(vcpu, CNTHV_CVAL_EL2) = cval;
> + __vcpu_assign_sys_reg(vcpu, CNTHV_CVAL_EL2, cval);
> break;
> case TIMER_HPTIMER:
> - __vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2) = cval;
> + __vcpu_assign_sys_reg(vcpu, CNTHP_CVAL_EL2, cval);
> break;
> default:
> WARN_ON(1);
> diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
> index 424a5107cddb5..6a2a899a344e6 100644
> --- a/arch/arm64/kvm/hyp/exception.c
> +++ b/arch/arm64/kvm/hyp/exception.c
> @@ -37,7 +37,7 @@ static inline void __vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
> if (unlikely(vcpu_has_nv(vcpu)))
> vcpu_write_sys_reg(vcpu, val, reg);
> else if (!__vcpu_write_sys_reg_to_cpu(val, reg))
> - __vcpu_sys_reg(vcpu, reg) = val;
> + __vcpu_assign_sys_reg(vcpu, reg, val);
> }
> 
> static void __vcpu_write_spsr(struct kvm_vcpu *vcpu, unsigned long target_mode,
> @@ -51,7 +51,7 @@ static void __vcpu_write_spsr(struct kvm_vcpu *vcpu, unsigned long target_mode,
> } else if (has_vhe()) {
> write_sysreg_el1(val, SYS_SPSR);
> } else {
> - __vcpu_sys_reg(vcpu, SPSR_EL1) = val;
> + __vcpu_assign_sys_reg(vcpu, SPSR_EL1, val);
> }
> }
> 
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index eef310cdbdbd5..aa5b561b92182 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -45,7 +45,7 @@ static inline void __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
> if (!vcpu_el1_is_32bit(vcpu))
> return;
> 
> - __vcpu_sys_reg(vcpu, FPEXC32_EL2) = read_sysreg(fpexc32_el2);
> + __vcpu_assign_sys_reg(vcpu, FPEXC32_EL2, read_sysreg(fpexc32_el2));
> }
> 
> static inline void __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
> @@ -457,7 +457,7 @@ static inline void fpsimd_lazy_switch_to_host(struct kvm_vcpu *vcpu)
> */
> if (vcpu_has_sve(vcpu)) {
> zcr_el1 = read_sysreg_el1(SYS_ZCR);
> - __vcpu_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu)) = zcr_el1;
> + __vcpu_assign_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu), zcr_el1);
> 
> /*
> * The guest's state is always saved using the guest's max VL.
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index b9cff893bbe0b..4d0dbea4c56f7 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -307,11 +307,11 @@ static inline void __sysreg32_save_state(struct kvm_vcpu *vcpu)
> vcpu->arch.ctxt.spsr_irq = read_sysreg(spsr_irq);
> vcpu->arch.ctxt.spsr_fiq = read_sysreg(spsr_fiq);
> 
> - __vcpu_sys_reg(vcpu, DACR32_EL2) = read_sysreg(dacr32_el2);
> - __vcpu_sys_reg(vcpu, IFSR32_EL2) = read_sysreg(ifsr32_el2);
> + __vcpu_assign_sys_reg(vcpu, DACR32_EL2, read_sysreg(dacr32_el2));
> + __vcpu_assign_sys_reg(vcpu, IFSR32_EL2, read_sysreg(ifsr32_el2));
> 
> if (has_vhe() || kvm_debug_regs_in_use(vcpu))
> - __vcpu_sys_reg(vcpu, DBGVCR32_EL2) = read_sysreg(dbgvcr32_el2);
> + __vcpu_assign_sys_reg(vcpu, DBGVCR32_EL2, read_sysreg(dbgvcr32_el2));
> }
> 
> static inline void __sysreg32_restore_state(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 8e8848de4d470..e9198e56e784b 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -26,7 +26,7 @@ void __kvm_hyp_host_forward_smc(struct kvm_cpu_context *host_ctxt);
> 
> static void __hyp_sve_save_guest(struct kvm_vcpu *vcpu)
> {
> - __vcpu_sys_reg(vcpu, ZCR_EL1) = read_sysreg_el1(SYS_ZCR);
> + __vcpu_assign_sys_reg(vcpu, ZCR_EL1, read_sysreg_el1(SYS_ZCR));
> /*
> * On saving/restoring guest sve state, always use the maximum VL for
> * the guest. The layout of the data when saving the sve state depends
> @@ -79,7 +79,7 @@ static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
> 
> has_fpmr = kvm_has_fpmr(kern_hyp_va(vcpu->kvm));
> if (has_fpmr)
> - __vcpu_sys_reg(vcpu, FPMR) = read_sysreg_s(SYS_FPMR);
> + __vcpu_assign_sys_reg(vcpu, FPMR, read_sysreg_s(SYS_FPMR));
> 
> if (system_supports_sve())
> __hyp_sve_restore_host();
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index c9b330dc20669..09df2b42bc1b7 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -223,9 +223,9 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
> */
> val = read_sysreg_el0(SYS_CNTP_CVAL);
> if (map.direct_ptimer == vcpu_ptimer(vcpu))
> - __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0) = val;
> + __vcpu_assign_sys_reg(vcpu, CNTP_CVAL_EL0, val);
> if (map.direct_ptimer == vcpu_hptimer(vcpu))
> - __vcpu_sys_reg(vcpu, CNTHP_CVAL_EL2) = val;
> + __vcpu_assign_sys_reg(vcpu, CNTHP_CVAL_EL2, val);
> 
> offset = read_sysreg_s(SYS_CNTPOFF_EL2);
> 
> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> index 3814b0b2c937f..34c7bf7fe9def 100644
> --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> @@ -18,17 +18,17 @@
> static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
> {
> /* These registers are common with EL1 */
> - __vcpu_sys_reg(vcpu, PAR_EL1) = read_sysreg(par_el1);
> - __vcpu_sys_reg(vcpu, TPIDR_EL1) = read_sysreg(tpidr_el1);
> -
> - __vcpu_sys_reg(vcpu, ESR_EL2) = read_sysreg_el1(SYS_ESR);
> - __vcpu_sys_reg(vcpu, AFSR0_EL2) = read_sysreg_el1(SYS_AFSR0);
> - __vcpu_sys_reg(vcpu, AFSR1_EL2) = read_sysreg_el1(SYS_AFSR1);
> - __vcpu_sys_reg(vcpu, FAR_EL2) = read_sysreg_el1(SYS_FAR);
> - __vcpu_sys_reg(vcpu, MAIR_EL2) = read_sysreg_el1(SYS_MAIR);
> - __vcpu_sys_reg(vcpu, VBAR_EL2) = read_sysreg_el1(SYS_VBAR);
> - __vcpu_sys_reg(vcpu, CONTEXTIDR_EL2) = read_sysreg_el1(SYS_CONTEXTIDR);
> - __vcpu_sys_reg(vcpu, AMAIR_EL2) = read_sysreg_el1(SYS_AMAIR);
> + __vcpu_assign_sys_reg(vcpu, PAR_EL1, read_sysreg(par_el1));
> + __vcpu_assign_sys_reg(vcpu, TPIDR_EL1, read_sysreg(tpidr_el1));
> +
> + __vcpu_assign_sys_reg(vcpu, ESR_EL2, read_sysreg_el1(SYS_ESR));
> + __vcpu_assign_sys_reg(vcpu, AFSR0_EL2, read_sysreg_el1(SYS_AFSR0));
> + __vcpu_assign_sys_reg(vcpu, AFSR1_EL2, read_sysreg_el1(SYS_AFSR1));
> + __vcpu_assign_sys_reg(vcpu, FAR_EL2, read_sysreg_el1(SYS_FAR));
> + __vcpu_assign_sys_reg(vcpu, MAIR_EL2, read_sysreg_el1(SYS_MAIR));
> + __vcpu_assign_sys_reg(vcpu, VBAR_EL2, read_sysreg_el1(SYS_VBAR));
> + __vcpu_assign_sys_reg(vcpu, CONTEXTIDR_EL2, read_sysreg_el1(SYS_CONTEXTIDR));
> + __vcpu_assign_sys_reg(vcpu, AMAIR_EL2, read_sysreg_el1(SYS_AMAIR));
> 
> /*
> * In VHE mode those registers are compatible between EL1 and EL2,
> @@ -46,21 +46,21 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
> * are always trapped, ensuring that the in-memory
> * copy is always up-to-date. A small blessing...
> */
> - __vcpu_sys_reg(vcpu, SCTLR_EL2) = read_sysreg_el1(SYS_SCTLR);
> - __vcpu_sys_reg(vcpu, TTBR0_EL2) = read_sysreg_el1(SYS_TTBR0);
> - __vcpu_sys_reg(vcpu, TTBR1_EL2) = read_sysreg_el1(SYS_TTBR1);
> - __vcpu_sys_reg(vcpu, TCR_EL2) = read_sysreg_el1(SYS_TCR);
> + __vcpu_assign_sys_reg(vcpu, SCTLR_EL2, read_sysreg_el1(SYS_SCTLR));
> + __vcpu_assign_sys_reg(vcpu, TTBR0_EL2, read_sysreg_el1(SYS_TTBR0));
> + __vcpu_assign_sys_reg(vcpu, TTBR1_EL2, read_sysreg_el1(SYS_TTBR1));
> + __vcpu_assign_sys_reg(vcpu, TCR_EL2, read_sysreg_el1(SYS_TCR));
> 
> if (ctxt_has_tcrx(&vcpu->arch.ctxt)) {
> - __vcpu_sys_reg(vcpu, TCR2_EL2) = read_sysreg_el1(SYS_TCR2);
> + __vcpu_assign_sys_reg(vcpu, TCR2_EL2, read_sysreg_el1(SYS_TCR2));
> 
> if (ctxt_has_s1pie(&vcpu->arch.ctxt)) {
> - __vcpu_sys_reg(vcpu, PIRE0_EL2) = read_sysreg_el1(SYS_PIRE0);
> - __vcpu_sys_reg(vcpu, PIR_EL2) = read_sysreg_el1(SYS_PIR);
> + __vcpu_assign_sys_reg(vcpu, PIRE0_EL2, read_sysreg_el1(SYS_PIRE0));
> + __vcpu_assign_sys_reg(vcpu, PIR_EL2, read_sysreg_el1(SYS_PIR));
> }
> 
> if (ctxt_has_s1poe(&vcpu->arch.ctxt))
> - __vcpu_sys_reg(vcpu, POR_EL2) = read_sysreg_el1(SYS_POR);
> + __vcpu_assign_sys_reg(vcpu, POR_EL2, read_sysreg_el1(SYS_POR));
> }
> 
> /*
> @@ -74,9 +74,9 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
> __vcpu_sys_reg(vcpu, CNTHCTL_EL2) |= val;
> }
> 
> - __vcpu_sys_reg(vcpu, SP_EL2) = read_sysreg(sp_el1);
> - __vcpu_sys_reg(vcpu, ELR_EL2) = read_sysreg_el1(SYS_ELR);
> - __vcpu_sys_reg(vcpu, SPSR_EL2) = read_sysreg_el1(SYS_SPSR);
> + __vcpu_assign_sys_reg(vcpu, SP_EL2, read_sysreg(sp_el1));
> + __vcpu_assign_sys_reg(vcpu, ELR_EL2, read_sysreg_el1(SYS_ELR));
> + __vcpu_assign_sys_reg(vcpu, SPSR_EL2, read_sysreg_el1(SYS_SPSR));
> }
> 
> static void __sysreg_restore_vel2_state(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 25c29107f13fd..4f0ae8073f788 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -178,7 +178,7 @@ static void kvm_pmu_set_pmc_value(struct kvm_pmc *pmc, u64 val, bool force)
> val |= lower_32_bits(val);
> }
> 
> - __vcpu_sys_reg(vcpu, reg) = val;
> + __vcpu_assign_sys_reg(vcpu, reg, val);
> 
> /* Recreate the perf event to reflect the updated sample_period */
> kvm_pmu_create_perf_event(pmc);
> @@ -204,7 +204,7 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> {
> kvm_pmu_release_perf_event(kvm_vcpu_idx_to_pmc(vcpu, select_idx));
> - __vcpu_sys_reg(vcpu, counter_index_to_reg(select_idx)) = val;
> + __vcpu_assign_sys_reg(vcpu, counter_index_to_reg(select_idx), val);
> kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> }
> 
> @@ -239,7 +239,7 @@ static void kvm_pmu_stop_counter(struct kvm_pmc *pmc)
> 
> reg = counter_index_to_reg(pmc->idx);
> 
> - __vcpu_sys_reg(vcpu, reg) = val;
> + __vcpu_assign_sys_reg(vcpu, reg, val);
> 
> kvm_pmu_release_perf_event(pmc);
> }
> @@ -503,7 +503,7 @@ static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
> reg = __vcpu_sys_reg(vcpu, counter_index_to_reg(i)) + 1;
> if (!kvm_pmc_is_64bit(pmc))
> reg = lower_32_bits(reg);
> - __vcpu_sys_reg(vcpu, counter_index_to_reg(i)) = reg;
> + __vcpu_assign_sys_reg(vcpu, counter_index_to_reg(i), reg);
> 
> /* No overflow? move on */
> if (kvm_pmc_has_64bit_overflow(pmc) ? reg : lower_32_bits(reg))
> @@ -602,7 +602,7 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
> kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> 
> /* The reset bits don't indicate any state, and shouldn't be saved. */
> - __vcpu_sys_reg(vcpu, PMCR_EL0) = val & ~(ARMV8_PMU_PMCR_C | ARMV8_PMU_PMCR_P);
> + __vcpu_assign_sys_reg(vcpu, PMCR_EL0, (val & ~(ARMV8_PMU_PMCR_C | ARMV8_PMU_PMCR_P)));
> 
> if (val & ARMV8_PMU_PMCR_C)
> kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
> @@ -779,7 +779,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
> u64 reg;
> 
> reg = counter_index_to_evtreg(pmc->idx);
> - __vcpu_sys_reg(vcpu, reg) = data & kvm_pmu_evtyper_mask(vcpu->kvm);
> + __vcpu_assign_sys_reg(vcpu, reg, (data & kvm_pmu_evtyper_mask(vcpu->kvm)));
> 
> kvm_pmu_create_perf_event(pmc);
> }
> @@ -1038,7 +1038,7 @@ static void kvm_arm_set_nr_counters(struct kvm *kvm, unsigned int nr)
> u64 val = __vcpu_sys_reg(vcpu, MDCR_EL2);
> val &= ~MDCR_EL2_HPMN;
> val |= FIELD_PREP(MDCR_EL2_HPMN, kvm->arch.nr_pmu_counters);
> - __vcpu_sys_reg(vcpu, MDCR_EL2) = val;
> + __vcpu_assign_sys_reg(vcpu, MDCR_EL2, val);
> }
> }
> }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 707c651aff031..93d0ca7ed9365 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -228,7 +228,7 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
> * to reverse-translate virtual EL2 system registers for a
> * non-VHE guest hypervisor.
> */
> - __vcpu_sys_reg(vcpu, reg) = val;
> + __vcpu_assign_sys_reg(vcpu, reg, val);
> 
> switch (reg) {
> case CNTHCTL_EL2:
> @@ -263,7 +263,7 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
> return;
> 
> memory_write:
> - __vcpu_sys_reg(vcpu, reg) = val;
> + __vcpu_assign_sys_reg(vcpu, reg, val);
> }
> 
> /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
> @@ -605,7 +605,7 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> if ((val ^ rd->val) & ~OSLSR_EL1_OSLK)
> return -EINVAL;
> 
> - __vcpu_sys_reg(vcpu, rd->reg) = val;
> + __vcpu_assign_sys_reg(vcpu, rd->reg, val);
> return 0;
> }
> 
> @@ -835,7 +835,7 @@ static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> * The value of PMCR.N field is included when the
> * vCPU register is read via kvm_vcpu_read_pmcr().
> */
> - __vcpu_sys_reg(vcpu, r->reg) = pmcr;
> + __vcpu_assign_sys_reg(vcpu, r->reg, pmcr);
> 
> return __vcpu_sys_reg(vcpu, r->reg);
> }
> @@ -907,7 +907,7 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> return false;
> 
> if (p->is_write)
> - __vcpu_sys_reg(vcpu, PMSELR_EL0) = p->regval;
> + __vcpu_assign_sys_reg(vcpu, PMSELR_EL0, p->regval);
> else
> /* return PMSELR.SEL field */
> p->regval = __vcpu_sys_reg(vcpu, PMSELR_EL0)
> @@ -1076,7 +1076,7 @@ static int set_pmreg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r, u64 va
> {
> u64 mask = kvm_pmu_accessible_counter_mask(vcpu);
> 
> - __vcpu_sys_reg(vcpu, r->reg) = val & mask;
> + __vcpu_assign_sys_reg(vcpu, r->reg, val & mask);
> kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> 
> return 0;
> @@ -1185,8 +1185,8 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (!vcpu_mode_priv(vcpu))
> return undef_access(vcpu, p, r);
> 
> - __vcpu_sys_reg(vcpu, PMUSERENR_EL0) =
> -       p->regval & ARMV8_PMU_USERENR_MASK;
> + __vcpu_assign_sys_reg(vcpu, PMUSERENR_EL0,
> +      (p->regval & ARMV8_PMU_USERENR_MASK));
> } else {
> p->regval = __vcpu_sys_reg(vcpu, PMUSERENR_EL0)
>    & ARMV8_PMU_USERENR_MASK;
> @@ -1237,7 +1237,7 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> if (!kvm_supports_32bit_el0())
> val |= ARMV8_PMU_PMCR_LC;
> 
> - __vcpu_sys_reg(vcpu, r->reg) = val;
> + __vcpu_assign_sys_reg(vcpu, r->reg, val);
> kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> 
> return 0;
> @@ -2207,7 +2207,7 @@ static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> if (kvm_has_mte(vcpu->kvm))
> clidr |= 2ULL << CLIDR_TTYPE_SHIFT(loc);
> 
> - __vcpu_sys_reg(vcpu, r->reg) = clidr;
> + __vcpu_assign_sys_reg(vcpu, r->reg, clidr);
> 
> return __vcpu_sys_reg(vcpu, r->reg);
> }
> @@ -2221,7 +2221,7 @@ static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> if ((val & CLIDR_EL1_RES0) || (!(ctr_el0 & CTR_EL0_IDC) && idc))
> return -EINVAL;
> 
> - __vcpu_sys_reg(vcpu, rd->reg) = val;
> + __vcpu_assign_sys_reg(vcpu, rd->reg, val);
> 
> return 0;
> }
> @@ -2398,7 +2398,7 @@ static bool access_sp_el1(struct kvm_vcpu *vcpu,
>  const struct sys_reg_desc *r)
> {
> if (p->is_write)
> - __vcpu_sys_reg(vcpu, SP_EL1) = p->regval;
> + __vcpu_assign_sys_reg(vcpu, SP_EL1, p->regval);
> else
> p->regval = __vcpu_sys_reg(vcpu, SP_EL1);
> 
> @@ -2422,7 +2422,7 @@ static bool access_spsr(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *r)
> {
> if (p->is_write)
> - __vcpu_sys_reg(vcpu, SPSR_EL1) = p->regval;
> + __vcpu_assign_sys_reg(vcpu, SPSR_EL1, p->regval);
> else
> p->regval = __vcpu_sys_reg(vcpu, SPSR_EL1);
> 
> @@ -2434,7 +2434,7 @@ static bool access_cntkctl_el12(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *r)
> {
> if (p->is_write)
> - __vcpu_sys_reg(vcpu, CNTKCTL_EL1) = p->regval;
> + __vcpu_assign_sys_reg(vcpu, CNTKCTL_EL1, p->regval);
> else
> p->regval = __vcpu_sys_reg(vcpu, CNTKCTL_EL1);
> 
> @@ -2448,7 +2448,9 @@ static u64 reset_hcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> if (!cpus_have_final_cap(ARM64_HAS_HCR_NV1))
> val |= HCR_E2H;
> 
> - return __vcpu_sys_reg(vcpu, r->reg) = val;
> + __vcpu_assign_sys_reg(vcpu, r->reg, val);
> +
> + return __vcpu_sys_reg(vcpu, r->reg);
> }
> 
> static unsigned int __el2_visibility(const struct kvm_vcpu *vcpu,
> @@ -2619,7 +2621,7 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
> u64_replace_bits(val, hpmn, MDCR_EL2_HPMN);
> }
> 
> - __vcpu_sys_reg(vcpu, MDCR_EL2) = val;
> + __vcpu_assign_sys_reg(vcpu, MDCR_EL2, val);
> 
> /*
> * Request a reload of the PMU to enable/disable the counters
> @@ -2748,7 +2750,7 @@ static int set_imp_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> 
> static u64 reset_mdcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> {
> - __vcpu_sys_reg(vcpu, r->reg) = vcpu->kvm->arch.nr_pmu_counters;
> + __vcpu_assign_sys_reg(vcpu, r->reg, vcpu->kvm->arch.nr_pmu_counters);
> return vcpu->kvm->arch.nr_pmu_counters;
> }
> 
> @@ -5006,7 +5008,7 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
> if (r->set_user) {
> ret = (r->set_user)(vcpu, r, val);
> } else {
> - __vcpu_sys_reg(vcpu, r->reg) = val;
> + __vcpu_assign_sys_reg(vcpu, r->reg, val);
> ret = 0;
> }
> 
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index cc6338d387663..ef97d9fc67cc5 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -137,7 +137,7 @@ static inline u64 reset_unknown(struct kvm_vcpu *vcpu,
> {
> BUG_ON(!r->reg);
> BUG_ON(r->reg >= NR_SYS_REGS);
> - __vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL;
> + __vcpu_assign_sys_reg(vcpu, r->reg, 0x1de7ec7edbadc0deULL);
> return __vcpu_sys_reg(vcpu, r->reg);
> }
> 
> @@ -145,7 +145,7 @@ static inline u64 reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> {
> BUG_ON(!r->reg);
> BUG_ON(r->reg >= NR_SYS_REGS);
> - __vcpu_sys_reg(vcpu, r->reg) = r->val;
> + __vcpu_assign_sys_reg(vcpu, r->reg, r->val);
> return __vcpu_sys_reg(vcpu, r->reg);
> }
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v3-nested.c b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> index 4f6954c306747..d22a8ad7bcc51 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3-nested.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3-nested.c
> @@ -356,12 +356,12 @@ void vgic_v3_put_nested(struct kvm_vcpu *vcpu)
> val = __vcpu_sys_reg(vcpu, ICH_HCR_EL2);
> val &= ~ICH_HCR_EL2_EOIcount_MASK;
> val |= (s_cpu_if->vgic_hcr & ICH_HCR_EL2_EOIcount_MASK);
> - __vcpu_sys_reg(vcpu, ICH_HCR_EL2) = val;
> - __vcpu_sys_reg(vcpu, ICH_VMCR_EL2) = s_cpu_if->vgic_vmcr;
> + __vcpu_assign_sys_reg(vcpu, ICH_HCR_EL2, val);
> + __vcpu_assign_sys_reg(vcpu, ICH_VMCR_EL2, s_cpu_if->vgic_vmcr);
> 
> for (i = 0; i < 4; i++) {
> - __vcpu_sys_reg(vcpu, ICH_AP0RN(i)) = s_cpu_if->vgic_ap0r[i];
> - __vcpu_sys_reg(vcpu, ICH_AP1RN(i)) = s_cpu_if->vgic_ap1r[i];
> + __vcpu_assign_sys_reg(vcpu, ICH_AP0RN(i), s_cpu_if->vgic_ap0r[i]);
> + __vcpu_assign_sys_reg(vcpu, ICH_AP1RN(i), s_cpu_if->vgic_ap1r[i]);
> }
> 
> for_each_set_bit(i, &shadow_if->lr_map, kvm_vgic_global_state.nr_lr) {
> @@ -370,7 +370,7 @@ void vgic_v3_put_nested(struct kvm_vcpu *vcpu)
> val &= ~ICH_LR_STATE;
> val |= s_cpu_if->vgic_lr[i] & ICH_LR_STATE;
> 
> - __vcpu_sys_reg(vcpu, ICH_LRN(i)) = val;
> + __vcpu_assign_sys_reg(vcpu, ICH_LRN(i), val);
> s_cpu_if->vgic_lr[i] = 0;
> }
> 
> -- 
> 2.39.2
> 
> 

Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Oliver Upton]

On Tue, Jun 03, 2025 at 08:08:20AM +0100, Marc Zyngier wrote:
> This series tries to bring some sanity to the way the RESx masks
> are applied when accessing the in-memory view of the guest's
> system registers.
> 
> Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
> be used as a rvalue or lvalue while that applies the RESx masks behind
> the scenes. This works fine when used as a rvalue.
> 
> However, when used as a lvalue, it does the wrong thing, as it only
> sanitises the value we're about to overwrite. This is pointless work
> and potentially hides bugs.
> 
> I propose that we move to a set of store-specific accessors (for
> assignments and RMW) instead of the lvalue hack, ensuring that the
> assigned value is the one that gets sanitised. This then allows the 
> legacy accessor to be converted to rvalue-only.

Very happy with how this is shaping up.

> Given the level of churn this introduces, I'd like this to land very
> early in the cycle. Either before 6.16-rc2, or early in 6.17.

What's your mood on sneaking this in as a fix for 6.16? It'd fix the
unmasked write bug you mention and hopefully give a semi-stable / early
base for building the 6.17 content on top off.

Otherwise this will be the very first thing I take for 6.17.

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks,
Oliver

Re: [PATCH v2 1/4] KVM: arm64: Add assignment-specific sysreg accessor

[Marc Zyngier]

On Tue, 03 Jun 2025 19:01:34 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> Hi Marc,
> 
> > On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
> > 
> > Assigning a value to a system register doesn't do what it is
> > supposed to be doing if that register is one that has RESx bits.
> > 
> > The main problem is that we use __vcpu_sys_reg(), which can be used
> > both as a lvalue and rvalue. When used as a lvalue, the bit masking
> > occurs *before* the new value is assigned, meaning that we (1) do
> > pointless work on the old cvalue, and (2) potentially assign an
> > invalid value as we fail to apply the masks to it.
> > 
> > Fix this by providing a new __vcpu_assign_sys_reg() that does
> > what it says on the tin, and sanitises the *new* value instead of
> > the old one. This comes with a significant amount of churn.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/include/asm/kvm_host.h          | 11 ++++++
> > arch/arm64/kvm/arch_timer.c                | 16 ++++----
> > arch/arm64/kvm/hyp/exception.c             |  4 +-
> > arch/arm64/kvm/hyp/include/hyp/switch.h    |  4 +-
> > arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  6 +--
> > arch/arm64/kvm/hyp/nvhe/hyp-main.c         |  4 +-
> > arch/arm64/kvm/hyp/vhe/switch.c            |  4 +-
> > arch/arm64/kvm/hyp/vhe/sysreg-sr.c         | 44 +++++++++++-----------
> > arch/arm64/kvm/pmu-emul.c                  | 14 +++----
> > arch/arm64/kvm/sys_regs.c                  | 38 ++++++++++---------
> > arch/arm64/kvm/sys_regs.h                  |  4 +-
> > arch/arm64/kvm/vgic/vgic-v3-nested.c       | 10 ++---
> > 12 files changed, 86 insertions(+), 73 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index d941abc6b5eef..3b84ad91116b4 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -1107,6 +1107,17 @@ 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))
> > 
> > u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
> > +
> > +#define __vcpu_assign_sys_reg(v, r, val) \
> > + do { \
> > + const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt; \
> > + u64 __v = (val); \
> > + if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__) \
> 
> Would it make sense to make it more strict by testing < NR_SYS_REGS too?

It is important to notice that you don't pass random integers here.
You pass something that is of an enum type, for which the maximum
value is NR_SYS_REGS. So as long as you pass a literal value, you're
100% safe. If you pass a constant value that goes over the limit, you
will hit the *existing* BUILD_BUG_ON().

You could fall into a trap by iterating over a base value and going
over the limit. But your check will only catch you going over the
array, and not the state corruption you will have introduced.

Finally, this is performance critical code, and I'd rather not add
extra checks that will only be a burden at runtime. If you want to
catch these overflows, using KASAN+UBSAN makes a lot more sense.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Marc Zyngier]

On Tue, 03 Jun 2025 22:06:27 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Jun 03, 2025 at 08:08:20AM +0100, Marc Zyngier wrote:
> > This series tries to bring some sanity to the way the RESx masks
> > are applied when accessing the in-memory view of the guest's
> > system registers.
> > 
> > Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
> > be used as a rvalue or lvalue while that applies the RESx masks behind
> > the scenes. This works fine when used as a rvalue.
> > 
> > However, when used as a lvalue, it does the wrong thing, as it only
> > sanitises the value we're about to overwrite. This is pointless work
> > and potentially hides bugs.
> > 
> > I propose that we move to a set of store-specific accessors (for
> > assignments and RMW) instead of the lvalue hack, ensuring that the
> > assigned value is the one that gets sanitised. This then allows the 
> > legacy accessor to be converted to rvalue-only.
> 
> Very happy with how this is shaping up.
> 
> > Given the level of churn this introduces, I'd like this to land very
> > early in the cycle. Either before 6.16-rc2, or early in 6.17.
> 
> What's your mood on sneaking this in as a fix for 6.16? It'd fix the
> unmasked write bug you mention and hopefully give a semi-stable / early
> base for building the 6.17 content on top off.

I'm absolutely in favour of taking this in ASAP. The impact should be
minimal for new code (and trivial to fix). I'll send this to Paolo by
the end of the week, or early next week at the latest.

> Otherwise this will be the very first thing I take for 6.17.
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks!

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v2 1/4] KVM: arm64: Add assignment-specific sysreg accessor

[Miguel Luis]

Hi Marc,

> On 4 Jun 2025, at 06:52, Marc Zyngier <maz@kernel.org> wrote:
> 
> On Tue, 03 Jun 2025 19:01:34 +0100,
> Miguel Luis <miguel.luis@oracle.com> wrote:
>> 
>> Hi Marc,
>> 
>>> On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
>>> 
>>> Assigning a value to a system register doesn't do what it is
>>> supposed to be doing if that register is one that has RESx bits.
>>> 
>>> The main problem is that we use __vcpu_sys_reg(), which can be used
>>> both as a lvalue and rvalue. When used as a lvalue, the bit masking
>>> occurs *before* the new value is assigned, meaning that we (1) do
>>> pointless work on the old cvalue, and (2) potentially assign an
>>> invalid value as we fail to apply the masks to it.
>>> 
>>> Fix this by providing a new __vcpu_assign_sys_reg() that does
>>> what it says on the tin, and sanitises the *new* value instead of
>>> the old one. This comes with a significant amount of churn.
>>> 
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>> arch/arm64/include/asm/kvm_host.h          | 11 ++++++
>>> arch/arm64/kvm/arch_timer.c                | 16 ++++----
>>> arch/arm64/kvm/hyp/exception.c             |  4 +-
>>> arch/arm64/kvm/hyp/include/hyp/switch.h    |  4 +-
>>> arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  6 +--
>>> arch/arm64/kvm/hyp/nvhe/hyp-main.c         |  4 +-
>>> arch/arm64/kvm/hyp/vhe/switch.c            |  4 +-
>>> arch/arm64/kvm/hyp/vhe/sysreg-sr.c         | 44 +++++++++++-----------
>>> arch/arm64/kvm/pmu-emul.c                  | 14 +++----
>>> arch/arm64/kvm/sys_regs.c                  | 38 ++++++++++---------
>>> arch/arm64/kvm/sys_regs.h                  |  4 +-
>>> arch/arm64/kvm/vgic/vgic-v3-nested.c       | 10 ++---
>>> 12 files changed, 86 insertions(+), 73 deletions(-)
>>> 
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index d941abc6b5eef..3b84ad91116b4 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -1107,6 +1107,17 @@ 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))
>>> 
>>> u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
>>> +
>>> +#define __vcpu_assign_sys_reg(v, r, val) \
>>> + do { \
>>> + const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt; \
>>> + u64 __v = (val); \
>>> + if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__) \
>> 
>> Would it make sense to make it more strict by testing < NR_SYS_REGS too?
> 
> It is important to notice that you don't pass random integers here.
> You pass something that is of an enum type, for which the maximum
> value is NR_SYS_REGS. So as long as you pass a literal value, you're
> 100% safe. If you pass a constant value that goes over the limit, you
> will hit the *existing* BUILD_BUG_ON().
> 

That’s correct.

> You could fall into a trap by iterating over a base value and going
> over the limit. But your check will only catch you going over the
> array, and not the state corruption you will have introduced.
> 

That’s also correct.

> Finally, this is performance critical code, and I'd rather not add
> extra checks that will only be a burden at runtime.

That's understandable and so far acceptable as we're trusting reg, rd->reg and r->reg
providers' code paths.

> If you want to
> catch these overflows, using KASAN+UBSAN makes a lot more sense.
> 

Thanks for your suggestion.

Thanks
Miguel

> M.
> 
> -- 
> Jazz isn't dead. It just smells funny.

Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Miguel Luis]

Hi Marc,

> On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
> 
> This series tries to bring some sanity to the way the RESx masks
> are applied when accessing the in-memory view of the guest's
> system registers.
> 
> Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
> be used as a rvalue or lvalue while that applies the RESx masks behind
> the scenes. This works fine when used as a rvalue.
> 
> However, when used as a lvalue, it does the wrong thing, as it only
> sanitises the value we're about to overwrite. This is pointless work
> and potentially hides bugs.
> 
> I propose that we move to a set of store-specific accessors (for
> assignments and RMW) instead of the lvalue hack, ensuring that the
> assigned value is the one that gets sanitised. This then allows the 
> legacy accessor to be converted to rvalue-only.
> 
> Given the level of churn this introduces, I'd like this to land very
> early in the cycle. Either before 6.16-rc2, or early in 6.17.
> 

For the series:
Reviewed-by: Miguel Luis <miguel.luis@oracle.com>

nit: the rmw accessor implies an implicit assignment which could be specified
within its macro instead but it's fine by me.

Thanks
Miguel

> * From v1 [1]
> 
>  - rebased to kvmarm-fixes-6.16-1
> 
> [1] https://lore.kernel.org/all/20250113183524.1378778-1-maz@kernel.org/
> 
> Marc Zyngier (4):
>  KVM: arm64: Add assignment-specific sysreg accessor
>  KVM: arm64: Add RMW specific sysreg accessor
>  KVM: arm64: Don't use __vcpu_sys_reg() to get the address of a sysreg
>  KVM: arm64: Make __vcpu_sys_reg() a pure rvalue operand
> 
> arch/arm64/include/asm/kvm_host.h          | 31 +++++++++--
> arch/arm64/kvm/arch_timer.c                | 18 +++----
> arch/arm64/kvm/debug.c                     |  4 +-
> arch/arm64/kvm/fpsimd.c                    |  4 +-
> arch/arm64/kvm/hyp/exception.c             |  4 +-
> arch/arm64/kvm/hyp/include/hyp/switch.h    |  4 +-
> arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  6 +--
> arch/arm64/kvm/hyp/nvhe/hyp-main.c         |  4 +-
> arch/arm64/kvm/hyp/vhe/switch.c            |  4 +-
> arch/arm64/kvm/hyp/vhe/sysreg-sr.c         | 48 ++++++++---------
> arch/arm64/kvm/nested.c                    |  2 +-
> arch/arm64/kvm/pmu-emul.c                  | 24 ++++-----
> arch/arm64/kvm/sys_regs.c                  | 60 +++++++++++-----------
> arch/arm64/kvm/sys_regs.h                  |  4 +-
> arch/arm64/kvm/vgic/vgic-v3-nested.c       | 10 ++--
> 15 files changed, 125 insertions(+), 102 deletions(-)
> 
> -- 
> 2.39.2
> 
> 

Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Marc Zyngier]

On Wed, 04 Jun 2025 11:47:57 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> Hi Marc,
> 
> > On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
> > 
> > This series tries to bring some sanity to the way the RESx masks
> > are applied when accessing the in-memory view of the guest's
> > system registers.
> > 
> > Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
> > be used as a rvalue or lvalue while that applies the RESx masks behind
> > the scenes. This works fine when used as a rvalue.
> > 
> > However, when used as a lvalue, it does the wrong thing, as it only
> > sanitises the value we're about to overwrite. This is pointless work
> > and potentially hides bugs.
> > 
> > I propose that we move to a set of store-specific accessors (for
> > assignments and RMW) instead of the lvalue hack, ensuring that the
> > assigned value is the one that gets sanitised. This then allows the 
> > legacy accessor to be converted to rvalue-only.
> > 
> > Given the level of churn this introduces, I'd like this to land very
> > early in the cycle. Either before 6.16-rc2, or early in 6.17.
> > 
> 
> For the series:
> Reviewed-by: Miguel Luis <miguel.luis@oracle.com>

Thanks.

> 
> nit: the rmw accessor implies an implicit assignment which could be specified
> within its macro instead but it's fine by me.

I'm not sure what you mean. Looking at this macro again, the early
writeback to the sysreg array is pretty unfortunate, and could be
avoided.

Does the following change address your concern? If not, please clearly
point out what you're after.

	M.

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index b4ac2f515f94..382b382d14da 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1121,7 +1121,8 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 #define __vcpu_rmw_sys_reg(v, r, op, val)				\
 	do {								\
 		const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;	\
-		u64 __v = ctxt_sys_reg(ctxt, (r)) op (val);		\
+		u64 __v = ctxt_sys_reg(ctxt, (r));			\
+		__v op (val);						\
 		if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__)	\
 			__v = kvm_vcpu_apply_reg_masks((v), (r), __v);	\
 									\

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Miguel Luis]

> On 4 Jun 2025, at 15:17, Marc Zyngier <maz@kernel.org> wrote:
> 
> On Wed, 04 Jun 2025 11:47:57 +0100,
> Miguel Luis <miguel.luis@oracle.com> wrote:
>> 
>> Hi Marc,
>> 
>>> On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
>>> 
>>> This series tries to bring some sanity to the way the RESx masks
>>> are applied when accessing the in-memory view of the guest's
>>> system registers.
>>> 
>>> Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
>>> be used as a rvalue or lvalue while that applies the RESx masks behind
>>> the scenes. This works fine when used as a rvalue.
>>> 
>>> However, when used as a lvalue, it does the wrong thing, as it only
>>> sanitises the value we're about to overwrite. This is pointless work
>>> and potentially hides bugs.
>>> 
>>> I propose that we move to a set of store-specific accessors (for
>>> assignments and RMW) instead of the lvalue hack, ensuring that the
>>> assigned value is the one that gets sanitised. This then allows the 
>>> legacy accessor to be converted to rvalue-only.
>>> 
>>> Given the level of churn this introduces, I'd like this to land very
>>> early in the cycle. Either before 6.16-rc2, or early in 6.17.
>>> 
>> 
>> For the series:
>> Reviewed-by: Miguel Luis <miguel.luis@oracle.com>
> 
> Thanks.
> 
>> 
>> nit: the rmw accessor implies an implicit assignment which could be specified
>> within its macro instead but it's fine by me.
> 
> I'm not sure what you mean. Looking at this macro again, the early
> writeback to the sysreg array is pretty unfortunate, and could be
> avoided.
> 
> Does the following change address your concern? If not, please clearly
> point out what you're after.

I believe this should give a clearer idea while avoids repeating the assignment everywhere
the accessor is used. Just replace ‘&=‘ and ‘|=‘ by ‘&’ and ‘|’.

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index b4ac2f515f94..bde1e31b4dee 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1121,7 +1121,7 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
 #define __vcpu_rmw_sys_reg(v, r, op, val)                              \
        do {                                                            \
                const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt;   \
-               u64 __v = ctxt_sys_reg(ctxt, (r)) op (val);             \
+               u64 __v = ctxt_sys_reg(ctxt, (r)) op##= (val);          \
                if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__) \
                        __v = kvm_vcpu_apply_reg_masks((v), (r), __v);  \
                                                                        \
Miguel

> 
> M.
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index b4ac2f515f94..382b382d14da 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1121,7 +1121,8 @@ u64 kvm_vcpu_apply_reg_masks(const struct kvm_vcpu *, enum vcpu_sysreg, u64);
> #define __vcpu_rmw_sys_reg(v, r, op, val) \
> do { \
> const struct kvm_cpu_context *ctxt = &(v)->arch.ctxt; \
> - u64 __v = ctxt_sys_reg(ctxt, (r)) op (val); \
> + u64 __v = ctxt_sys_reg(ctxt, (r)); \
> + __v op (val); \
> if (vcpu_has_nv((v)) && (r) >= __SANITISED_REG_START__) \
> __v = kvm_vcpu_apply_reg_masks((v), (r), __v); \
> \
> 
> -- 
> Jazz isn't dead. It just smells funny.

Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Marc Zyngier]

On Wed, 04 Jun 2025 16:53:01 +0100,
Miguel Luis <miguel.luis@oracle.com> wrote:
> 
> 
> 
> > On 4 Jun 2025, at 15:17, Marc Zyngier <maz@kernel.org> wrote:
> > 
> > On Wed, 04 Jun 2025 11:47:57 +0100,
> > Miguel Luis <miguel.luis@oracle.com> wrote:
> >> 
> >> Hi Marc,
> >> 
> >>> On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
> >>> 
> >>> This series tries to bring some sanity to the way the RESx masks
> >>> are applied when accessing the in-memory view of the guest's
> >>> system registers.
> >>> 
> >>> Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
> >>> be used as a rvalue or lvalue while that applies the RESx masks behind
> >>> the scenes. This works fine when used as a rvalue.
> >>> 
> >>> However, when used as a lvalue, it does the wrong thing, as it only
> >>> sanitises the value we're about to overwrite. This is pointless work
> >>> and potentially hides bugs.
> >>> 
> >>> I propose that we move to a set of store-specific accessors (for
> >>> assignments and RMW) instead of the lvalue hack, ensuring that the
> >>> assigned value is the one that gets sanitised. This then allows the 
> >>> legacy accessor to be converted to rvalue-only.
> >>> 
> >>> Given the level of churn this introduces, I'd like this to land very
> >>> early in the cycle. Either before 6.16-rc2, or early in 6.17.
> >>> 
> >> 
> >> For the series:
> >> Reviewed-by: Miguel Luis <miguel.luis@oracle.com>
> > 
> > Thanks.
> > 
> >> 
> >> nit: the rmw accessor implies an implicit assignment which could be specified
> >> within its macro instead but it's fine by me.
> > 
> > I'm not sure what you mean. Looking at this macro again, the early
> > writeback to the sysreg array is pretty unfortunate, and could be
> > avoided.
> > 
> > Does the following change address your concern? If not, please clearly
> > point out what you're after.
> 
> I believe this should give a clearer idea while avoids repeating the assignment everywhere
> the accessor is used. Just replace ‘&=‘ and ‘|=‘ by ‘&’ and ‘|’.

I have the opposite view. I think the '=' sign conveys an important
meaning, and hiding it makes things less obvious. If I had lost common
sense and was writing C++ , I would have simply overloaded the various
operators, retaining the existing syntax. This macro does the hard job
of applying the RESx masks, but the operator is what is semantically
significant.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Miguel Luis]

> On 4 Jun 2025, at 18:58, Marc Zyngier <maz@kernel.org> wrote:
> 
> On Wed, 04 Jun 2025 16:53:01 +0100,
> Miguel Luis <miguel.luis@oracle.com> wrote:
>> 
>> 
>> 
>>> On 4 Jun 2025, at 15:17, Marc Zyngier <maz@kernel.org> wrote:
>>> 
>>> On Wed, 04 Jun 2025 11:47:57 +0100,
>>> Miguel Luis <miguel.luis@oracle.com> wrote:
>>>> 
>>>> Hi Marc,
>>>> 
>>>>> On 3 Jun 2025, at 07:08, Marc Zyngier <maz@kernel.org> wrote:
>>>>> 
>>>>> This series tries to bring some sanity to the way the RESx masks
>>>>> are applied when accessing the in-memory view of the guest's
>>>>> system registers.
>>>>> 
>>>>> Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
>>>>> be used as a rvalue or lvalue while that applies the RESx masks behind
>>>>> the scenes. This works fine when used as a rvalue.
>>>>> 
>>>>> However, when used as a lvalue, it does the wrong thing, as it only
>>>>> sanitises the value we're about to overwrite. This is pointless work
>>>>> and potentially hides bugs.
>>>>> 
>>>>> I propose that we move to a set of store-specific accessors (for
>>>>> assignments and RMW) instead of the lvalue hack, ensuring that the
>>>>> assigned value is the one that gets sanitised. This then allows the 
>>>>> legacy accessor to be converted to rvalue-only.
>>>>> 
>>>>> Given the level of churn this introduces, I'd like this to land very
>>>>> early in the cycle. Either before 6.16-rc2, or early in 6.17.
>>>>> 
>>>> 
>>>> For the series:
>>>> Reviewed-by: Miguel Luis <miguel.luis@oracle.com>
>>> 
>>> Thanks.
>>> 
>>>> 
>>>> nit: the rmw accessor implies an implicit assignment which could be specified
>>>> within its macro instead but it's fine by me.
>>> 
>>> I'm not sure what you mean. Looking at this macro again, the early
>>> writeback to the sysreg array is pretty unfortunate, and could be
>>> avoided.
>>> 
>>> Does the following change address your concern? If not, please clearly
>>> point out what you're after.
>> 
>> I believe this should give a clearer idea while avoids repeating the assignment everywhere
>> the accessor is used. Just replace ‘&=‘ and ‘|=‘ by ‘&’ and ‘|’.
> 
> I have the opposite view. I think the '=' sign conveys an important
> meaning, and hiding it makes things less obvious. If I had lost common
> sense and was writing C++ , I would have simply overloaded the various
> operators, retaining the existing syntax. This macro does the hard job
> of applying the RESx masks, but the operator is what is semantically
> significant.
> 

That’s understandable. The Read-Modify-Write or ‘rmw’ on the accessor
reinforces the same important meaning as the ‘=‘ sign is conveying when
expressed on the accessor usage. Per se not a problem.

Thanks,
Miguel

> Thanks,
> 
> M.
> 
> -- 
> Jazz isn't dead. It just smells funny.

Re: [PATCH v2 0/4] KVM: arm64: vcpu sysreg accessor rework

[Marc Zyngier]

On Tue, 03 Jun 2025 08:08:20 +0100, Marc Zyngier wrote:
> This series tries to bring some sanity to the way the RESx masks
> are applied when accessing the in-memory view of the guest's
> system registers.
> 
> Currently, we have *one* accessor (__vcpu_sys_reg()) that can either
> be used as a rvalue or lvalue while that applies the RESx masks behind
> the scenes. This works fine when used as a rvalue.
> 
> [...]

Applied to fixes, thanks!

[1/4] KVM: arm64: Add assignment-specific sysreg accessor
      commit: 6678791ee3da0b78c28fe7d77814097f53cbb8df
[2/4] KVM: arm64: Add RMW specific sysreg accessor
      commit: 8800b7c4bbede3cd40831726d3f98e8080baf4df
[3/4] KVM: arm64: Don't use __vcpu_sys_reg() to get the address of a sysreg
      commit: b61fae4ee120f337b8700dff43b2fd639f3bf6a5
[4/4] KVM: arm64: Make __vcpu_sys_reg() a pure rvalue operand
      commit: b5fa1f91e11fdf74ad4e2ac6dae246a57cbd2d95

Cheers,

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