Re: [PATCH 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors

[Marc Zyngier <maz@kernel.org>]

On Thu, 29 Oct 2020 20:11:05 +0000,
Andrew Jones <drjones@redhat.com> wrote:
> 
> The AA64ZFR0_EL1 accessors are just the general accessors with
> its visibility function open-coded. It also skips the if-else
> chain in read_id_reg, but there's no reason not to go there.
> Indeed consolidating ID register accessors and removing lines
> of code make it worthwhile.
> 
> No functional change intended.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 61 +++++++--------------------------------
>  1 file changed, 11 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 9f6151589460..1ccaa5f3b081 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1197,55 +1197,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
>  }
>  
> -/* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
> -static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
> -{
> -	if (!vcpu_has_sve(vcpu))
> -		return 0;
> -
> -	return read_sanitised_ftr_reg(SYS_ID_AA64ZFR0_EL1);
> -}
> -
> -static bool access_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> -				   struct sys_reg_params *p,
> -				   const struct sys_reg_desc *rd)
> -{
> -	if (p->is_write)
> -		return write_to_read_only(vcpu, p, rd);
> -
> -	p->regval = guest_id_aa64zfr0_el1(vcpu);
> -	return true;
> -}
> -
> -static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> -		const struct sys_reg_desc *rd,
> -		const struct kvm_one_reg *reg, void __user *uaddr)
> -{
> -	u64 val;
> -
> -	val = guest_id_aa64zfr0_el1(vcpu);
> -	return reg_to_user(uaddr, &val, reg->id);
> -}
> -
> -static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> -		const struct sys_reg_desc *rd,
> -		const struct kvm_one_reg *reg, void __user *uaddr)
> -{
> -	const u64 id = sys_reg_to_index(rd);
> -	int err;
> -	u64 val;
> -
> -	err = reg_from_user(&val, uaddr, id);
> -	if (err)
> -		return err;
> -
> -	/* This is what we mean by invariant: you can't change it. */
> -	if (val != guest_id_aa64zfr0_el1(vcpu))
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  /*
>   * cpufeature ID register user accessors
>   *
> @@ -1384,6 +1335,16 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
>  				  const struct sys_reg_desc *r)
>  {
> +	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
> +			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> +
> +	switch (id) {
> +	case SYS_ID_AA64ZFR0_EL1:
> +		if (!vcpu_has_sve(vcpu))
> +			return REG_RAZ_USER | REG_RAZ_GUEST;
> +		break;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1515,7 +1476,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	ID_SANITISED(ID_AA64PFR1_EL1),
>  	ID_UNALLOCATED(4,2),
>  	ID_UNALLOCATED(4,3),
> -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
> +	ID_SANITISED(ID_AA64ZFR0_EL1),
>  	ID_UNALLOCATED(4,5),
>  	ID_UNALLOCATED(4,6),
>  	ID_UNALLOCATED(4,7),

I really like this, as it establishes a central location to control
the visibility of ID regs, should we need to hide a full register.

Once we establish the actual need to separate RAZ controls between
userspace and guest, I'll be happy to take this.

	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