Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Oliver Upton <oupton@xxxxxxxxxx>]

Hi Marc,

On Wed, Apr 06, 2022 at 04:07:28PM +0100, Marc Zyngier wrote:
> > +	/*
> > +	 * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> > +	 * Avoid conflicting with future expansion of AArch64 feature registers
> > +	 * and simply treat them as RAZ here.
> > +	 */
> > +	if (params->CRm > 3)
> > +		params->regval = 0;
> > +	else
> > +		ret = emulate_sys_reg(vcpu, params);
> > +
> > +	vcpu_set_reg(vcpu, Rt, params->regval);
> 
> It feels odd to update Rt without checking whether the read has
> succeeded. In your case, this is harmless, but would break with the
> approach I'm outlining below.
> 

A total kludge to avoid yet another level of indentation :) I'll go
about this the right way next spin.

> > +	return ret;
> > +}
> > +
> > +/**
> > + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> > + *			  AArch32 ID register.
> > + * @params: the system register access parameters
> > + *
> > + * Note that CP15 ID registers where CRm=0 are excluded from this check. The
> > + * only register trapped in the CRm=0 range is CTR, which is already handled in
> > + * the cp15 register table.
> 
> There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
> which prevents it from fitting in your scheme.
> 
> > + */
> > +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> > +{
> > +	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> > +}
> > +
> >  /**
> >   * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
> >   * @vcpu: The VCPU pointer
> > @@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> >  	params.Op1 = (esr >> 14) & 0x7;
> >  	params.Op2 = (esr >> 17) & 0x7;
> >  
> > +	/*
> > +	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
> > +	 * system register table.
> > +	 */
> > +	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
> > +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> 
> I think this is a bit ugly. We reach this point from a function that
> was cp15-specific, and now we are reconstructing the context. I'd
> rather this is moved to kvm_handle_cp15_32(), and treated there
> (untested):
>

Completely agree, hoisting this would be much more elegant.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7b45c040cc27..a071d89ace92 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>   * @run:  The kvm_run struct
>   */
>  static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> +			    struct sys_reg_params *params,
>  			    const struct sys_reg_desc *global,
>  			    size_t nr_global)
>  {
> -	struct sys_reg_params params;
> -	u32 esr = kvm_vcpu_get_esr(vcpu);
>  	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
>  
> -	params.CRm = (esr >> 1) & 0xf;
> -	params.regval = vcpu_get_reg(vcpu, Rt);
> -	params.is_write = ((esr & 1) == 0);
> -	params.CRn = (esr >> 10) & 0xf;
> -	params.Op0 = 0;
> -	params.Op1 = (esr >> 14) & 0x7;
> -	params.Op2 = (esr >> 17) & 0x7;
> +	params->regval = vcpu_get_reg(vcpu, Rt);
>  
> -	if (!emulate_cp(vcpu, &params, global, nr_global)) {
> -		if (!params.is_write)
> -			vcpu_set_reg(vcpu, Rt, params.regval);
> +	if (!emulate_cp(vcpu, params, global, nr_global)) {
> +		if (!params->is_write)
> +			vcpu_set_reg(vcpu, Rt, params->regval);
>  		return 1;
>  	}
>  
> -	unhandled_cp_access(vcpu, &params);
> +	unhandled_cp_access(vcpu, params);
>  	return 1;
>  }
>  
> @@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
> +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
>  }
>  
>  int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
> @@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
>  }
>  
>  static bool is_imp_def_sys_reg(struct sys_reg_params *params)
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index cc0cc95a0280..fd4b2bb8c782 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -35,6 +35,13 @@ struct sys_reg_params {
>  				  .Op2 = ((esr) >> 17) & 0x7,                  \
>  				  .is_write = !((esr) & 1) })
>  
> +#define esr_cp1x_32_to_params(esr)					       \
> +	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
> +				  .CRn = ((esr) >> 10) & 0xf,                  \
> +				  .CRm = ((esr) >> 1) & 0xf,                   \
> +				  .Op2 = ((esr) >> 17) & 0x7,                  \
> +				  .is_write = !((esr) & 1) })
> +
>  struct sys_reg_desc {
>  	/* Sysreg string for debug */
>  	const char *name;
> 
> 
> What do you think?

Way better. Your suggested patch looks correct, I'll fold all of this
together and test it out. Thanks for the suggestions :)

--
Best,
Oliver