Re: [PATCH v4 5/7] KVM: arm64: Add a macro for creating filtered sys_reg_descs entries

[Gavin Shan <gshan@redhat.com>]

On 10/4/24 9:07 PM, Joey Gouly wrote:
> From: James Morse <james.morse@arm.com>
> 
> The sys_reg_descs array holds function pointers and reset value for
> managing the user-space and guest view of system registers. These
> are mostly created by a set of macro's as only some combinations
> of behaviour are needed.
> 
> If a register needs special treatment, its sys_reg_descs entry is
> open-coded. This is true of some id registers where the value provided
> by user-space is validated by some helpers.
> 
> Before adding another one of these, add a helper that covers the
> existing special cases. 'ID_FILTERED' expects helpers to set the
> user-space value, and retrieve the modified reset value.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>   arch/arm64/kvm/sys_regs.c | 54 ++++++++++++++++++++-------------------
>   1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8b1a6cedc49e..1fd08f12f2bb 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1833,6 +1833,12 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>   	return set_id_reg(vcpu, rd, val);
>   }
>   
> +static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> +			       const struct sys_reg_desc *rd, u64 val)
> +{
> +	return set_id_reg(vcpu, rd, val);
> +}
> +
>   /*
>    * cpufeature ID register user accessors
>    *
> @@ -2131,6 +2137,15 @@ static bool bad_redir_trap(struct kvm_vcpu *vcpu,
>   	.val = 0,				\
>   }
>   
> +/* sys_reg_desc initialiser for cpufeature ID registers that need filtering */
> +#define ID_FILTERED(sysreg, name, filtered_fields) {	\
> +	ID_DESC(sysreg),				\
> +	.set_user = set_##name,				\
> +	.visibility = id_visibility,			\
> +	.reset = read_sanitised_##name,			\
> +	.val = (filtered_fields),			\
> +}
> +
>   /* sys_reg_desc initialiser for known cpufeature ID registers */
>   #define AA32_ID_SANITISED(name) {		\
>   	ID_DESC(name),				\
> @@ -2337,14 +2352,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>   	/* CRm=1 */
>   	AA32_ID_SANITISED(ID_PFR0_EL1),
>   	AA32_ID_SANITISED(ID_PFR1_EL1),
> -	{ 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,
> -	  .reset = read_sanitised_id_dfr0_el1,
> -	  .val = ID_DFR0_EL1_PerfMon_MASK |
> -		 ID_DFR0_EL1_CopDbg_MASK, },
> +	ID_FILTERED(ID_DFR0_EL1, id_dfr0_el1,
> +		    ID_DFR0_EL1_PerfMon_MASK |
> +		    ID_DFR0_EL1_CopDbg_MASK),

The replacement causes logical change because the 'visibility' callback has been
changed from aa32_id_visibility() to id_visibility(). I think we need an aa32
variant of ID_FILTERED, to be used here.

  #define AA32_ID_FILTERED(sysreg, name, filtered_fields) {	\
  	ID_DESC(sysreg),				        \
	.set_user = set_##name,				        \
	.visibility = aa32_id_visibility,			\           /* the only differentiated field */
	.reset = read_sanitised_##name,			        \
	.val = (filtered_fields),			        \
}


>   	ID_HIDDEN(ID_AFR0_EL1),
>   	AA32_ID_SANITISED(ID_MMFR0_EL1),
>   	AA32_ID_SANITISED(ID_MMFR1_EL1),
> @@ -2373,17 +2383,13 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>   
>   	/* AArch64 ID registers */
>   	/* CRm=4 */
> -	{ SYS_DESC(SYS_ID_AA64PFR0_EL1),
> -	  .access = access_id_reg,
> -	  .get_user = get_id_reg,
> -	  .set_user = set_id_reg,
> -	  .reset = read_sanitised_id_aa64pfr0_el1,
> -	  .val = ~(ID_AA64PFR0_EL1_AMU |
> -		   ID_AA64PFR0_EL1_MPAM |
> -		   ID_AA64PFR0_EL1_SVE |
> -		   ID_AA64PFR0_EL1_RAS |
> -		   ID_AA64PFR0_EL1_AdvSIMD |
> -		   ID_AA64PFR0_EL1_FP), },
> +	ID_FILTERED(ID_AA64PFR0_EL1, id_aa64pfr0_el1,
> +		    ~(ID_AA64PFR0_EL1_AMU |
> +		      ID_AA64PFR0_EL1_MPAM |
> +		      ID_AA64PFR0_EL1_SVE |
> +		      ID_AA64PFR0_EL1_RAS |
> +		      ID_AA64PFR0_EL1_AdvSIMD |
> +		      ID_AA64PFR0_EL1_FP)),

Note that the 'visibility' callback has been changed from NULL to id_visibility(),
not resulting in logical changes since id_visibility() returns 0 for ID_AA64PFR0_EL1.
However, it would be nice to be documented in the change log?

>   	ID_SANITISED(ID_AA64PFR1_EL1),
>   	ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR),
>   	ID_UNALLOCATED(4,3),
> @@ -2393,13 +2399,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>   	ID_WRITABLE(ID_AA64FPFR0_EL1, ~ID_AA64FPFR0_EL1_RES0),
>   
>   	/* CRm=5 */
> -	{ SYS_DESC(SYS_ID_AA64DFR0_EL1),
> -	  .access = access_id_reg,
> -	  .get_user = get_id_reg,
> -	  .set_user = set_id_aa64dfr0_el1,
> -	  .reset = read_sanitised_id_aa64dfr0_el1,
> -	  .val = ID_AA64DFR0_EL1_PMUVer_MASK |
> -		 ID_AA64DFR0_EL1_DebugVer_MASK, },
> +	ID_FILTERED(ID_AA64DFR0_EL1, id_aa64dfr0_el1,
> +		    ID_AA64DFR0_EL1_PMUVer_MASK |
> +		    ID_AA64DFR0_EL1_DebugVer_MASK),

Same as above. The 'visibility' callback has been changed from NULL to id_visibility(),
even there is no logical changes.

>   	ID_SANITISED(ID_AA64DFR1_EL1),
>   	ID_UNALLOCATED(5,2),
>   	ID_UNALLOCATED(5,3),

Thanks,
Gavin