Re: [PATCH 2/3] KVM: arm64: Check RAZ visibility in ID register accessors

[Marc Zyngier <maz@kernel.org>]

On Thu, 29 Oct 2020 20:11:04 +0000,
Andrew Jones <drjones@redhat.com> wrote:
> 
> The instruction encodings of ID registers are preallocated. Until an
> encoding is assigned a purpose the register is RAZ. KVM's general ID
> register accessor functions already support both paths, RAZ or not.
> If for each ID register we can determine if it's RAZ or not, then all
> ID registers can build on the general functions. The register visibility
> function allows us to check whether a register should be completely
> hidden or not, extending it to also report when the register should
> be RAZ or not allows us to use it for ID registers as well.
> 
> No functional change intended.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 19 ++++++++++++++++---
>  arch/arm64/kvm/sys_regs.h | 20 ++++++++++++++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d24e66ee59b3..9f6151589460 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1171,7 +1171,9 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
>  			  struct sys_reg_params *p,
>  			  const struct sys_reg_desc *r)
>  {
> -	return __access_id_reg(vcpu, p, r, false);
> +	bool raz = sysreg_raz_from_guest(vcpu, r);
> +
> +	return __access_id_reg(vcpu, p, r, raz);
>  }
>  
>  static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> @@ -1283,13 +1285,17 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
>  static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		      const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	return __get_id_reg(vcpu, rd, uaddr, false);
> +	bool raz = sysreg_raz_from_user(vcpu, rd);
> +
> +	return __get_id_reg(vcpu, rd, uaddr, raz);
>  }
>  
>  static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		      const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	return __set_id_reg(vcpu, rd, uaddr, false);
> +	bool raz = sysreg_raz_from_user(vcpu, rd);
> +
> +	return __set_id_reg(vcpu, rd, uaddr, raz);
>  }
>  
>  static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> @@ -1375,12 +1381,19 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	return false;
>  }
>  
> +static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> +				  const struct sys_reg_desc *r)
> +{
> +	return 0;
> +}
> +
>  /* sys_reg_desc initialiser for known cpufeature ID registers */
>  #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,		\
>  }
>  
>  /*
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 5a6fc30f5989..d5add36c130a 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -61,6 +61,8 @@ struct sys_reg_desc {
>  
>  #define REG_HIDDEN_USER		(1 << 0) /* hidden from userspace ioctls */
>  #define REG_HIDDEN_GUEST	(1 << 1) /* hidden from guest */
> +#define REG_RAZ_USER		(1 << 2) /* RAZ from userspace ioctls */
> +#define REG_RAZ_GUEST		(1 << 3) /* RAZ from guest */
>
>  static __printf(2, 3)
>  inline void print_sys_reg_msg(const struct sys_reg_params *p,
> @@ -129,6 +131,24 @@ static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu,
>  	return r->visibility(vcpu, r) & REG_HIDDEN_USER;
>  }
>  
> +static inline bool sysreg_raz_from_guest(const struct kvm_vcpu *vcpu,
> +					 const struct sys_reg_desc *r)
> +{
> +	if (likely(!r->visibility))
> +		return false;
> +
> +	return r->visibility(vcpu, r) & REG_RAZ_GUEST;
> +}
> +
> +static inline bool sysreg_raz_from_user(const struct kvm_vcpu *vcpu,
> +					const struct sys_reg_desc *r)
> +{
> +	if (likely(!r->visibility))
> +		return false;
> +
> +	return r->visibility(vcpu, r) & REG_RAZ_USER;
> +}
> +
>  static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
>  			      const struct sys_reg_desc *i2)
>  {

Is there actually a case for any ID register to have different
RAZ semantics between guest and userspace? I have the feeling that
we'd want them to be consistent at all times. Or did you have any
particular (and future) use case in mind?

Otherwise, looks good.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm