Re: [PATCH 1/4] KVM: arm64: add emulation for CTR_EL0 register

[Marc Zyngier <maz@kernel.org>]

Please add all the reviewers and the relevant mailing lists.

On Mon, 18 Mar 2024 11:16:33 +0000,
Sebastian Ott <sebott@redhat.com> wrote:
> 
> CTR_EL0 is currently handled as an invariant register, thus
> guests will be presented with the host value of that register.
> Add emulation for CTR_EL0 and maintain a per vcpu value. The
> only thing that is allowed to be changed compared to the host
> value is to switch off the DIC bit which describes Icache
> invalidation requirements.
> 
> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 44 +++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 30253bd19917..b2019faa9d73 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1871,10 +1871,42 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	if (p->is_write)
>  		return write_to_read_only(vcpu, p, r);
>  
> -	p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +	p->regval = __vcpu_sys_reg(vcpu, r->reg);
>  	return true;
>  }
>  
> +static u64 reset_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> +{
> +	u64 val = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> +	__vcpu_sys_reg(vcpu, rd->reg) = val;
> +	return __vcpu_sys_reg(vcpu, rd->reg);

I really don't think we should make this a per-CPU value, and instead
keep it a VM-wide value, just like any other ID register.

Also, rd->reg isn't set to anything useful, leading to memory
corruption (hint, there is no CTR_EL0 in the vcpu sysreg file).

> +}
> +
> +static int get_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		   u64 *val)
> +{
> +	*val = __vcpu_sys_reg(vcpu, rd->reg);
> +	return 0;
> +}
> +
> +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		   u64 val)
> +{
> +	u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> +	if (kvm_vm_has_ran_once(vcpu->kvm) &&
> +	    val != __vcpu_sys_reg(vcpu, rd->reg))
> +		return -EBUSY;
> +
> +	if (((ctr_el0 & ~CTR_EL0_DIC_MASK) != (val & ~CTR_EL0_DIC_MASK)) ||
> +	    ((ctr_el0 & CTR_EL0_DIC_MASK) < (val & CTR_EL0_DIC_MASK)))
> +		return -EINVAL;

Why limit this to DIC only? Anything that advertises something
"weaker" than what the HW has, such as a smaller cache line size, is
equally valid.

> +
> +	__vcpu_sys_reg(vcpu, rd->reg) = val;
> +	return 0;
> +}
> +
>  static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			 const struct sys_reg_desc *r)
>  {
> @@ -2461,7 +2493,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
>  	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
>  	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
> -	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
> +	{ SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
> +	  .get_user = get_ctr, .set_user = set_ctr},

Now, who traps this? Since c876c3f182a5 ("KVM: arm64: Relax trapping
of CTR_EL0 when FEAT_EVT is available"), we don't trap it anymore when
FEAT_EVT is present. Surely you should account for this.

Also, we really shouldn't trap it unless the guest view is different,
as this has a very visible impact on any userspace.

	M.

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