Re: [PATCH v5 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3

[Marc Zyngier <maz@kernel.org>]

On Sun, 02 Apr 2023 19:37:35 +0100,
Jing Zhang <jingzhangos@google.com> wrote:
> 
> Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
> ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
> introduced by ID register descriptor.
> 
> No functional change intended.
> 
> Co-developed-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/include/asm/cpufeature.h |   5 +
>  arch/arm64/kernel/cpufeature.c      |   8 +-
>  arch/arm64/kvm/id_regs.c            | 262 +++++++++++++++++++---------
>  arch/arm64/kvm/sys_regs.c           |   3 +-
>  arch/arm64/kvm/sys_regs.h           |   2 +-
>  5 files changed, 191 insertions(+), 89 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 6bf013fb110d..f17e74afe3e9 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -915,6 +915,7 @@ static inline unsigned int get_vmid_bits(u64 mmfr1)
>  	return 8;
>  }
>  
> +s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, s64 cur);
>  struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id);
>  
>  extern struct arm64_ftr_override id_aa64mmfr1_override;
> @@ -925,6 +926,10 @@ extern struct arm64_ftr_override id_aa64smfr0_override;
>  extern struct arm64_ftr_override id_aa64isar1_override;
>  extern struct arm64_ftr_override id_aa64isar2_override;
>  
> +extern const struct arm64_ftr_bits ftr_id_dfr0[];
> +extern const struct arm64_ftr_bits ftr_id_aa64pfr0[];
> +extern const struct arm64_ftr_bits ftr_id_aa64dfr0[];
> +
>  u32 get_kvm_ipa_limit(void);
>  void dump_cpu_features(void);
>  
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index c331c49a7d19..5b0e3379e5f8 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -225,7 +225,7 @@ static const struct arm64_ftr_bits ftr_id_aa64isar2[] = {
>  	ARM64_FTR_END,
>  };
>  
> -static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
> +const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_CSV3_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_CSV2_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_DIT_SHIFT, 4, 0),
> @@ -426,7 +426,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>  	ARM64_FTR_END,
>  };
>  
> -static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
> +const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>  	S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
> @@ -578,7 +578,7 @@ static const struct arm64_ftr_bits ftr_id_pfr2[] = {
>  	ARM64_FTR_END,
>  };
>  
> -static const struct arm64_ftr_bits ftr_id_dfr0[] = {
> +const struct arm64_ftr_bits ftr_id_dfr0[] = {
>  	/* [31:28] TraceFilt */
>  	S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT, ID_DFR0_EL1_PerfMon_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_DFR0_EL1_MProfDbg_SHIFT, 4, 0),

There really isn't a good reason for exposing any of this. You can
readily get to the overarching arm64_ftr_reg.

> @@ -791,7 +791,7 @@ static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
>  	return reg;
>  }
>
> -static s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new,
> +s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new,
>  				s64 cur)
>  {
>  	s64 ret = 0;
> diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> index af86001e2686..395eaf84a0ab 100644
> --- a/arch/arm64/kvm/id_regs.c
> +++ b/arch/arm64/kvm/id_regs.c
> @@ -39,6 +39,64 @@ struct id_reg_desc {
>  	u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr);
>  };
>  
> +static struct id_reg_desc id_reg_descs[];
> +
> +/**
> + * arm64_check_features() - Check if a feature register value constitutes
> + * a subset of features indicated by @limit.
> + *
> + * @ftrp: Pointer to an array of arm64_ftr_bits. It must be terminated by
> + * an item whose width field is zero.
> + * @writable_mask: Indicates writable feature bits.
> + * @val: The feature register value to check
> + * @limit: The limit value of the feature register
> + *
> + * This function will check if each feature field of @val is the "safe" value
> + * against @limit based on @ftrp[], each of which specifies the target field
> + * (shift, width), whether or not the field is for a signed value (sign),
> + * how the field is determined to be "safe" (type), and the safe value
> + * (safe_val) when type == FTR_EXACT (safe_val won't be used by this
> + * function when type != FTR_EXACT). Any other fields in arm64_ftr_bits
> + * won't be used by this function. If a field value in @val is the same
> + * as the one in @limit, it is always considered the safe value regardless
> + * of the type. For register fields that are not in writable, only the value
> + * in @limit is considered the safe value.
> + *
> + * Return: 0 if all the fields are safe. Otherwise, return negative errno.
> + */
> +static int arm64_check_features(const struct arm64_ftr_bits *ftrp,
> +				u64 writable_mask, u64 val, u64 limit)
> +{
> +	u64 mask = 0;
> +
> +	for (; ftrp && ftrp->width; ftrp++) {
> +		s64 f_val, f_lim, safe_val;
> +		u64 ftr_mask;
> +
> +		ftr_mask = arm64_ftr_mask(ftrp);
> +		if ((ftr_mask & writable_mask) != ftr_mask)
> +			continue;
> +
> +		f_val = arm64_ftr_value(ftrp, val);
> +		f_lim = arm64_ftr_value(ftrp, limit);
> +		mask |= ftr_mask;
> +
> +		if (f_val == f_lim)
> +			safe_val = f_val;
> +		else
> +			safe_val = arm64_ftr_safe_value(ftrp, f_val, f_lim);
> +
> +		if (safe_val != f_val)
> +			return -E2BIG;
> +	}
> +
> +	/* For fields that are not writable, values in limit are the safe values. */
> +	if ((val & ~mask) != (limit & ~mask))
> +		return -E2BIG;
> +
> +	return 0;
> +}
> +
>  static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
>  {
>  	if (kvm_vcpu_has_pmu(vcpu))
> @@ -84,7 +142,6 @@ u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
>  	case SYS_ID_AA64PFR0_EL1:
>  		if (!vcpu_has_sve(vcpu))
>  			val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE);
> -		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
>  		if (kvm_vgic_global_state.type == VGIC_V3) {
>  			val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC);
>  			val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC), 1);
> @@ -111,15 +168,10 @@ u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
>  			val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_WFxT);
>  		break;
>  	case SYS_ID_AA64DFR0_EL1:
> -		/* Limit debug to ARMv8.0 */
> -		val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
> -		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
>  		/* Set PMUver to the required version */
>  		val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
>  		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
>  				  vcpu_pmuver(vcpu));
> -		/* Hide SPE from guests */
> -		val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
>  		break;
>  	case SYS_ID_DFR0_EL1:
>  		val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> @@ -178,9 +230,16 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		      u64 val)
>  {
> -	/* This is what we mean by invariant: you can't change it. */
> -	if (val != read_id_reg(vcpu, rd))
> -		return -EINVAL;
> +	int ret;
> +	int id = reg_to_encoding(rd);
> +	const struct id_reg_desc *idr = &id_reg_descs[IDREG_IDX(id)];
> +
> +	ret = arm64_check_features(idr->ftr_bits, idr->writable_mask, val,
> +			idr->read_kvm_sanitised_reg ? idr->read_kvm_sanitised_reg(idr) : 0);
> +	if (ret)
> +		return ret;
> +
> +	IDREG(vcpu->kvm, id) = val;
>  
>  	return 0;
>  }
> @@ -219,12 +278,39 @@ static u64 general_read_kvm_sanitised_reg(const struct id_reg_desc *idr)
>  	return read_sanitised_ftr_reg(reg_to_encoding(&idr->reg_desc));
>  }
>  
> +static u64 read_sanitised_id_aa64pfr0_el1(const struct id_reg_desc *idr)
> +{
> +	u64 val;
> +	u32 id = reg_to_encoding(&idr->reg_desc);
> +
> +	val = read_sanitised_ftr_reg(id);
> +	/*
> +	 * The default is to expose CSV2 == 1 if the HW isn't affected.
> +	 * Although this is a per-CPU feature, we make it global because
> +	 * asymmetric systems are just a nuisance.
> +	 *
> +	 * Userspace can override this as long as it doesn't promise
> +	 * the impossible.
> +	 */
> +	if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
> +		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
> +		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
> +	}
> +	if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
> +		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
> +		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
> +	}
> +
> +	val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
> +
> +	return val;
> +}
> +
>  static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>  			       const struct sys_reg_desc *rd,
>  			       u64 val)
>  {
>  	u8 csv2, csv3;
> -	u64 sval = val;
>  
>  	/*
>  	 * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
> @@ -232,26 +318,37 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>  	 * guest could otherwise be covered in ectoplasmic residue).
>  	 */
>  	csv2 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_EL1_CSV2_SHIFT);
> -	if (csv2 > 1 ||
> -	    (csv2 && arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED))
> +	if (csv2 > 1 || (csv2 && arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED))
>  		return -EINVAL;
>  
>  	/* Same thing for CSV3 */
>  	csv3 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_EL1_CSV3_SHIFT);
> -	if (csv3 > 1 ||
> -	    (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED))
> +	if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED))
>  		return -EINVAL;
>  
> -	/* We can only differ with CSV[23], and anything else is an error */
> -	val ^= read_id_reg(vcpu, rd);
> -	val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
> -		 ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
> -	if (val)
> -		return -EINVAL;
> +	return set_id_reg(vcpu, rd, val);
> +}
>  
> -	IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval;
> +static u64 read_sanitised_id_aa64dfr0_el1(const struct id_reg_desc *idr)
> +{
> +	u64 val;
> +	u32 id = reg_to_encoding(&idr->reg_desc);
>  
> -	return 0;
> +	val = read_sanitised_ftr_reg(id);
> +	/* Limit debug to ARMv8.0 */
> +	val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
> +	val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
> +	/*
> +	 * Initialise the default PMUver before there is a chance to
> +	 * create an actual PMU.
> +	 */
> +	val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> +	val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
> +			  kvm_arm_pmu_get_pmuver_limit());
> +	/* Hide SPE from guests */
> +	val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
> +
> +	return val;
>  }
>  
>  static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> @@ -260,6 +357,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>  {
>  	u8 pmuver, host_pmuver;
>  	bool valid_pmu;
> +	int ret;
>  
>  	host_pmuver = kvm_arm_pmu_get_pmuver_limit();
>  
> @@ -279,23 +377,25 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>  	if (kvm_vcpu_has_pmu(vcpu) != valid_pmu)
>  		return -EINVAL;
>  
> -	/* We can only differ with PMUver, and anything else is an error */
> -	val ^= read_id_reg(vcpu, rd);
> -	val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> -	if (val)
> -		return -EINVAL;
> -
>  	if (valid_pmu) {
>  		mutex_lock(&vcpu->kvm->arch.config_lock);
> -		IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> -		IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) |=
> -			FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), pmuver);
> +		ret = set_id_reg(vcpu, rd, val);
> +		if (ret) {
> +			mutex_unlock(&vcpu->kvm->arch.config_lock);
> +			return ret;
> +		}
>  
>  		IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
>  		IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) |=
>  			FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon), pmuver_to_perfmon(pmuver));

I repeatedly asked for assignments to be *on a single line*.

>  		mutex_unlock(&vcpu->kvm->arch.config_lock);
>  	} else {
> +		/* We can only differ with PMUver, and anything else is an error */
> +		val ^= read_id_reg(vcpu, rd);
> +		val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> +		if (val)
> +			return -EINVAL;
> +

I asked about this before. Why do we need this? Isn't the whole point
of the exercise to have a *unified* way to check for the writable
bits?  If you still need to open-code that, the whole exercise is a
bit pointless, isn't it?

Frankly, the whole thing is going the wrong way, starting with the
wrapping data structure. We already have most of what we need in
sys_reg_desc, and it only takes a bit of imagination to get there.

I've spent a couple of hours hacking away at the series, and got rid
of it entirely, simply by repurposing exist fields (val for the write
mask, reset for the limit value). All I can say is that it compiles.

But at least it doesn't reinvent the wheel.

	M.

diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
index 395eaf84a0ab..30f36e0dd12e 100644
--- a/arch/arm64/kvm/id_regs.c
+++ b/arch/arm64/kvm/id_regs.c
@@ -18,29 +18,6 @@
 
 #include "sys_regs.h"
 
-struct id_reg_desc {
-	const struct sys_reg_desc	reg_desc;
-	/*
-	 * ftr_bits points to the feature bits array defined in cpufeature.c for
-	 * writable CPU ID feature register.
-	 */
-	const struct arm64_ftr_bits *ftr_bits;
-	/*
-	 * Only bits with 1 are writable from userspace.
-	 * This mask might not be necessary in the future whenever all ID
-	 * registers are enabled as writable from userspace.
-	 */
-	const u64 writable_mask;
-	/*
-	 * This function returns the KVM sanitised register value.
-	 * The value would be the same as the host kernel sanitised value if
-	 * there is no KVM sanitisation for this id register.
-	 */
-	u64 (*read_kvm_sanitised_reg)(const struct id_reg_desc *idr);
-};
-
-static struct id_reg_desc id_reg_descs[];
-
 /**
  * arm64_check_features() - Check if a feature register value constitutes
  * a subset of features indicated by @limit.
@@ -64,11 +41,26 @@ static struct id_reg_desc id_reg_descs[];
  *
  * Return: 0 if all the fields are safe. Otherwise, return negative errno.
  */
-static int arm64_check_features(const struct arm64_ftr_bits *ftrp,
-				u64 writable_mask, u64 val, u64 limit)
+static int arm64_check_features(struct kvm_vcpu *vcpu,
+				const struct sys_reg_desc *rd,
+				u64 val)
 {
+	const struct arm64_ftr_reg *ftr_reg;
+	const struct arm64_ftr_bits *ftrp;
+	u32 id = reg_to_encoding(rd);
+	u64 writable_mask = rd->val;
+	u64 limit = 0;
 	u64 mask = 0;
 
+	if (rd->reset)
+		limit = rd->reset(vcpu, rd);
+
+	ftr_reg = get_arm64_ftr_reg(id);
+	if (!ftr_reg)
+		return -EINVAL;
+
+	ftrp = ftr_reg->ftr_bits;
+
 	for (; ftrp && ftrp->width; ftrp++) {
 		s64 f_val, f_lim, safe_val;
 		u64 ftr_mask;
@@ -230,12 +222,10 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      u64 val)
 {
+	u32 id = reg_to_encoding(rd);
 	int ret;
-	int id = reg_to_encoding(rd);
-	const struct id_reg_desc *idr = &id_reg_descs[IDREG_IDX(id)];
 
-	ret = arm64_check_features(idr->ftr_bits, idr->writable_mask, val,
-			idr->read_kvm_sanitised_reg ? idr->read_kvm_sanitised_reg(idr) : 0);
+	ret = arm64_check_features(vcpu, rd, val);
 	if (ret)
 		return ret;
 
@@ -273,15 +263,17 @@ static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
 	return id_visibility(vcpu, r);
 }
 
-static u64 general_read_kvm_sanitised_reg(const struct id_reg_desc *idr)
+static u64 general_read_kvm_sanitised_reg(struct kvm_vcpu *vcpu,
+					  const struct sys_reg_desc *r)
 {
-	return read_sanitised_ftr_reg(reg_to_encoding(&idr->reg_desc));
+	return read_sanitised_ftr_reg(reg_to_encoding(r));
 }
 
-static u64 read_sanitised_id_aa64pfr0_el1(const struct id_reg_desc *idr)
+static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
+					  const struct sys_reg_desc *r)
 {
 	u64 val;
-	u32 id = reg_to_encoding(&idr->reg_desc);
+	u32 id = reg_to_encoding(r);
 
 	val = read_sanitised_ftr_reg(id);
 	/*
@@ -329,10 +321,11 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 	return set_id_reg(vcpu, rd, val);
 }
 
-static u64 read_sanitised_id_aa64dfr0_el1(const struct id_reg_desc *idr)
+static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
+					  const struct sys_reg_desc *r)
 {
 	u64 val;
-	u32 id = reg_to_encoding(&idr->reg_desc);
+	u32 id = reg_to_encoding(r);
 
 	val = read_sanitised_ftr_reg(id);
 	/* Limit debug to ARMv8.0 */
@@ -403,10 +396,11 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-static u64 read_sanitised_id_dfr0_el1(const struct id_reg_desc *idr)
+static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
+				      const struct sys_reg_desc *r)
 {
 	u64 val;
-	u32 id = reg_to_encoding(&idr->reg_desc);
+	u32 id = reg_to_encoding(r);
 
 	val = read_sanitised_ftr_reg(id);
 	/*
@@ -473,33 +467,23 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 }
 
 /* sys_reg_desc initialiser for known cpufeature ID registers */
-#define SYS_DESC_SANITISED(name) {			\
+#define ID_SANITISED(name) {				\
 	SYS_DESC(SYS_##name),				\
 	.access	= access_id_reg,			\
 	.get_user = get_id_reg,				\
 	.set_user = set_id_reg,				\
 	.visibility = id_visibility,			\
-}
-
-#define ID_SANITISED(name) {						\
-	.reg_desc = SYS_DESC_SANITISED(name),				\
-	.ftr_bits = NULL,						\
-	.writable_mask = 0,						\
-	.read_kvm_sanitised_reg = general_read_kvm_sanitised_reg,	\
+	.reset = general_read_kvm_sanitised_reg,	\
 }
 
 /* sys_reg_desc initialiser for known cpufeature ID registers */
-#define AA32_ID_SANITISED(name) {					\
-	.reg_desc = {							\
-		SYS_DESC(SYS_##name),					\
-		.access	= access_id_reg,				\
-		.get_user = get_id_reg,					\
-		.set_user = set_id_reg,					\
-		.visibility = aa32_id_visibility,			\
-	},								\
-	.ftr_bits = NULL,						\
-	.writable_mask = 0,						\
-	.read_kvm_sanitised_reg = general_read_kvm_sanitised_reg,	\
+#define AA32_ID_SANITISED(name) {				\
+	SYS_DESC(SYS_##name),					\
+	.access	= access_id_reg,				\
+	.get_user = get_id_reg,					\
+	.set_user = set_id_reg,					\
+	.visibility = aa32_id_visibility,			\
+	.reset = general_read_kvm_sanitised_reg,		\
 }
 
 /*
@@ -508,16 +492,11 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
  * (1 <= crm < 8, 0 <= Op2 < 8).
  */
 #define ID_UNALLOCATED(crm, op2) {				\
-	.reg_desc = {						\
 		Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2),	\
 		.access = access_id_reg,			\
 		.get_user = get_id_reg,				\
 		.set_user = set_id_reg,				\
 		.visibility = raz_visibility			\
-	},							\
-	.ftr_bits = NULL,					\
-	.writable_mask = 0,					\
-	.read_kvm_sanitised_reg = NULL,				\
 }
 
 /*
@@ -526,19 +505,14 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
  * RAZ for the guest.
  */
 #define ID_HIDDEN(name) {				\
-	.reg_desc = {					\
 		SYS_DESC(SYS_##name),			\
 		.access = access_id_reg,		\
 		.get_user = get_id_reg,			\
 		.set_user = set_id_reg,			\
 		.visibility = raz_visibility,		\
-	},						\
-	.ftr_bits = NULL,				\
-	.writable_mask = 0,				\
-	.read_kvm_sanitised_reg = NULL,			\
 }
 
-static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
+static struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
 	/*
 	 * ID regs: all ID_SANITISED() entries here must have corresponding
 	 * entries in arm64_ftr_regs[].
@@ -548,15 +522,14 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
 	/* CRm=1 */
 	AA32_ID_SANITISED(ID_PFR0_EL1),
 	AA32_ID_SANITISED(ID_PFR1_EL1),
-	{ .reg_desc = {
+	{
 		SYS_DESC(SYS_ID_DFR0_EL1),
 		.access = access_id_reg,
 		.get_user = get_id_reg,
 		.set_user = set_id_dfr0_el1,
-		.visibility = aa32_id_visibility, },
-	  .ftr_bits = ftr_id_dfr0,
-	  .writable_mask = ID_DFR0_EL1_PerfMon_MASK,
-	  .read_kvm_sanitised_reg = read_sanitised_id_dfr0_el1,
+		.visibility = aa32_id_visibility,
+		.val = ID_DFR0_EL1_PerfMon_MASK,
+		.reset = read_sanitised_id_dfr0_el1,
 	},
 	ID_HIDDEN(ID_AFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR0_EL1),
@@ -586,14 +559,13 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
 
 	/* AArch64 ID registers */
 	/* CRm=4 */
-	{ .reg_desc = {
+	{
 		SYS_DESC(SYS_ID_AA64PFR0_EL1),
 		.access = access_id_reg,
 		.get_user = get_id_reg,
-		.set_user = set_id_aa64pfr0_el1, },
-	  .ftr_bits = ftr_id_aa64pfr0,
-	  .writable_mask = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK,
-	  .read_kvm_sanitised_reg = read_sanitised_id_aa64pfr0_el1,
+		.set_user = set_id_aa64pfr0_el1,
+		.val = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK,
+		.reset = read_sanitised_id_aa64pfr0_el1,
 	},
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4, 2),
@@ -604,14 +576,13 @@ static struct id_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
 	ID_UNALLOCATED(4, 7),
 
 	/* CRm=5 */
-	{ .reg_desc = {
+	{
 		SYS_DESC(SYS_ID_AA64DFR0_EL1),
 		.access = access_id_reg,
 		.get_user = get_id_reg,
-		.set_user = set_id_aa64dfr0_el1, },
-	  .ftr_bits = ftr_id_aa64dfr0,
-	  .writable_mask = ID_AA64DFR0_EL1_PMUVer_MASK,
-	  .read_kvm_sanitised_reg = read_sanitised_id_aa64dfr0_el1,
+		.set_user = set_id_aa64dfr0_el1,
+		.val = ID_AA64DFR0_EL1_PMUVer_MASK,
+		.reset = read_sanitised_id_aa64dfr0_el1,
 	},
 	ID_SANITISED(ID_AA64DFR1_EL1),
 	ID_UNALLOCATED(5, 2),
@@ -648,7 +619,7 @@ static const struct sys_reg_desc *id_params_to_desc(struct sys_reg_params *param
 
 	id = reg_to_encoding(params);
 	if (is_id_reg(id))
-		return &id_reg_descs[IDREG_IDX(id)].reg_desc;
+		return &id_reg_descs[IDREG_IDX(id)];
 
 	return NULL;
 }
@@ -742,11 +713,11 @@ bool kvm_arm_idreg_table_init(void)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
-		const struct sys_reg_desc *r = &id_reg_descs[i].reg_desc;
+		const struct sys_reg_desc *r = &id_reg_descs[i];
 
 		if (!is_id_reg(reg_to_encoding(r))) {
 			kvm_err("id_reg table %pS entry %d not set correctly\n",
-				&id_reg_descs[i].reg_desc, i);
+				id_reg_descs, i);
 			return false;
 		}
 	}
@@ -756,7 +727,7 @@ bool kvm_arm_idreg_table_init(void)
 
 int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
 {
-	const struct id_reg_desc *i2, *end2;
+	const struct sys_reg_desc *i2, *end2;
 	unsigned int total = 0;
 	int err;
 
@@ -764,7 +735,7 @@ int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
 	end2 = id_reg_descs + ARRAY_SIZE(id_reg_descs);
 
 	for (; i2 != end2; i2++) {
-		err = walk_one_sys_reg(vcpu, &(i2->reg_desc), &uind, &total);
+		err = walk_one_sys_reg(vcpu, i2, &uind, &total);
 		if (err)
 			return err;
 	}
@@ -779,11 +750,11 @@ void kvm_arm_init_id_regs(struct kvm *kvm)
 	u64 val;
 
 	for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
-		id = reg_to_encoding(&id_reg_descs[i].reg_desc);
+		id = reg_to_encoding(&id_reg_descs[i]);
 
 		val = 0;
-		if (id_reg_descs[i].read_kvm_sanitised_reg)
-			val = id_reg_descs[i].read_kvm_sanitised_reg(&id_reg_descs[i]);
+		if (id_reg_descs[i].reset)
+			val = id_reg_descs[i].reset(NULL, &id_reg_descs[i]);
 
 		IDREG(kvm, id) = val;
 	}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index b90d1d3ad081..c4f498e75315 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -540,10 +540,11 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return 0;
 }
 
-static void reset_bvr(struct kvm_vcpu *vcpu,
+static u64 reset_bvr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
 	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val;
+	return rd->val;
 }
 
 static bool trap_bcr(struct kvm_vcpu *vcpu,
@@ -576,10 +577,11 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return 0;
 }
 
-static void reset_bcr(struct kvm_vcpu *vcpu,
+static u64 reset_bcr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
 	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val;
+	return rd->val;
 }
 
 static bool trap_wvr(struct kvm_vcpu *vcpu,
@@ -613,10 +615,11 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return 0;
 }
 
-static void reset_wvr(struct kvm_vcpu *vcpu,
+static u64 reset_wvr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
 	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val;
+	return rd->val;
 }
 
 static bool trap_wcr(struct kvm_vcpu *vcpu,
@@ -649,25 +652,28 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return 0;
 }
 
-static void reset_wcr(struct kvm_vcpu *vcpu,
+static u64 reset_wcr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
 	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val;
+	return rd->val;
 }
 
-static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 amair = read_sysreg(amair_el1);
 	vcpu_write_sys_reg(vcpu, amair, AMAIR_EL1);
+	return amair;
 }
 
-static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 actlr = read_sysreg(actlr_el1);
 	vcpu_write_sys_reg(vcpu, actlr, ACTLR_EL1);
+	return actlr;
 }
 
-static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 mpidr;
 
@@ -681,7 +687,9 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
 	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
 	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
-	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
+	mpidr |= (1ULL << 31);
+	vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
+	return mpidr;
 }
 
 static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
@@ -693,13 +701,13 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
 	return REG_HIDDEN;
 }
 
-static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
 
 	/* No PMU available, any PMU reg may UNDEF... */
 	if (!kvm_arm_support_pmu_v3())
-		return;
+		return 0;
 
 	n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
 	n &= ARMV8_PMU_PMCR_N_MASK;
@@ -708,33 +716,37 @@ static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 
 	reset_unknown(vcpu, r);
 	__vcpu_sys_reg(vcpu, r->reg) &= mask;
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
-static void reset_pmevcntr(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);
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
-static void reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	reset_unknown(vcpu, r);
 	__vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_EVTYPE_MASK;
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
-static void reset_pmselr(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) &= ARMV8_PMU_COUNTER_MASK;
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
-static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 pmcr;
 
 	/* No PMU available, PMCR_EL0 may UNDEF... */
 	if (!kvm_arm_support_pmu_v3())
-		return;
+		return 0;
 
 	/* Only preserve PMCR_EL0.N, and reset the rest to 0 */
 	pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
@@ -742,6 +754,7 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 		pmcr |= ARMV8_PMU_PMCR_LC;
 
 	__vcpu_sys_reg(vcpu, r->reg) = pmcr;
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
 static bool check_pmu_access_disabled(struct kvm_vcpu *vcpu, u64 flags)
@@ -1205,7 +1218,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
  * Fabricate a CLIDR_EL1 value instead of using the real value, which can vary
  * by the physical CPU which the vcpu currently resides in.
  */
-static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
 	u64 clidr;
@@ -1253,6 +1266,7 @@ static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 		clidr |= 2 << CLIDR_TTYPE_SHIFT(loc);
 
 	__vcpu_sys_reg(vcpu, r->reg) = clidr;
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
 static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
@@ -2603,19 +2617,21 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
  */
 
 #define FUNCTION_INVARIANT(reg)						\
-	static void get_##reg(struct kvm_vcpu *v,			\
+	static u64 get_##reg(struct kvm_vcpu *v,			\
 			      const struct sys_reg_desc *r)		\
 	{								\
 		((struct sys_reg_desc *)r)->val = read_sysreg(reg);	\
+		return ((struct sys_reg_desc *)r)->val;			\
 	}
 
 FUNCTION_INVARIANT(midr_el1)
 FUNCTION_INVARIANT(revidr_el1)
 FUNCTION_INVARIANT(aidr_el1)
 
-static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
+static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
 {
 	((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0);
+	return ((struct sys_reg_desc *)r)->val;
 }
 
 /* ->val is filled in by kvm_sys_reg_table_init() */
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index df8d26df93ec..d14d5b41a222 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -65,12 +65,12 @@ struct sys_reg_desc {
 		       const struct sys_reg_desc *);
 
 	/* Initialization for vcpu. */
-	void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
+	u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
 
 	/* Index into sys_reg[], or 0 if we don't need to save it. */
 	int reg;
 
-	/* Value (usually reset value) */
+	/* Value (usually reset value), or write mask for idregs */
 	u64 val;
 
 	/* Custom get/set_user functions, fallback to generic if NULL */
@@ -123,19 +123,21 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
 }
 
 /* Reset functions */
-static inline void reset_unknown(struct kvm_vcpu *vcpu,
+static inline u64 reset_unknown(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) = 0x1de7ec7edbadc0deULL;
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
-static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+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;
+	return __vcpu_sys_reg(vcpu, r->reg);
 }
 
 static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu,

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