Re: [PATCH 14/25] KVM: arm64: Register AArch64 system register entries with the sysreg xarray

[Marc Zyngier <maz@kernel.org>]

On Wed, 24 Jan 2024 16:34:25 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> On Mon, Jan 22, 2024 at 08:18:41PM +0000, Marc Zyngier wrote:
> > In order to reduce the number of lookups that we have to perform
> > when handling a sysreg, register each AArch64 sysreg descriptor
> > with the global xarray. The index of the descriptor is stored
> > as a 10 bit field in the data word.
> > 
> > Subsequent patches will retrieve and use the stored index.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  3 +++
> >  arch/arm64/kvm/emulate-nested.c   | 39 +++++++++++++++++++++++++++++--
> >  arch/arm64/kvm/sys_regs.c         | 11 ++++++++-
> >  3 files changed, 50 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index fe35c59214ad..e7a6219f2929 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -1083,6 +1083,9 @@ int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
> >  void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
> >  
> >  int __init kvm_sys_reg_table_init(void);
> > +struct sys_reg_desc;
> > +int __init populate_sysreg_config(const struct sys_reg_desc *sr,
> > +				  unsigned int idx);
> >  int __init populate_nv_trap_config(void);
> >  
> >  bool lock_all_vcpus(struct kvm *kvm);
> > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> > index 59622636b723..342d43b66fda 100644
> > --- a/arch/arm64/kvm/emulate-nested.c
> > +++ b/arch/arm64/kvm/emulate-nested.c
> > @@ -427,12 +427,14 @@ static const complex_condition_check ccc[] = {
> >   * [19:14]	bit number in the FGT register (6 bits)
> >   * [20]		trap polarity (1 bit)
> >   * [25:21]	FG filter (5 bits)
> > - * [62:26]	Unused (37 bits)
> > + * [35:26]	Main SysReg table index (10 bits)
> > + * [62:36]	Unused (27 bits)
> >   * [63]		RES0 - Must be zero, as lost on insertion in the xarray
> >   */
> >  #define TC_CGT_BITS	10
> >  #define TC_FGT_BITS	4
> >  #define TC_FGF_BITS	5
> > +#define TC_MSR_BITS	10
> >  
> >  union trap_config {
> >  	u64	val;
> > @@ -442,7 +444,8 @@ union trap_config {
> >  		unsigned long	bit:6;		 /* Bit number */
> >  		unsigned long	pol:1;		 /* Polarity */
> >  		unsigned long	fgf:TC_FGF_BITS; /* Fine Grained Filter */
> > -		unsigned long	unused:37;	 /* Unused, should be zero */
> > +		unsigned long	msr:TC_MSR_BITS; /* Main SysReg index */
> > +		unsigned long	unused:27;	 /* Unused, should be zero */
> >  		unsigned long	mbz:1;		 /* Must Be Zero */
> >  	};
> >  };
> > @@ -1862,6 +1865,38 @@ int __init populate_nv_trap_config(void)
> >  	return ret;
> >  }
> >  
> > +int __init populate_sysreg_config(const struct sys_reg_desc *sr,
> > +				  unsigned int idx)
> > +{
> > +	union trap_config tc;
> > +	u32 encoding;
> > +	void *ret;
> > +
> > +	/*
> > +	 * 0 is a valid value for the index, but not for the storage.
> > +	 * We'll store (idx+1), so check against an offset'd limit.
> > +	 */
> > +	if (idx >= (BIT(TC_MSR_BITS) - 1)) {
> > +		kvm_err("sysreg %s (%d) out of range\n", sr->name, idx);
> > +		return -EINVAL;
> > +	}
> > +
> > +	encoding = sys_reg(sr->Op0, sr->Op1, sr->CRn, sr->CRm, sr->Op2);
> > +	tc = get_trap_config(encoding);
> > +
> > +	if (tc.msr) {
> > +		kvm_err("sysreg %s (%d) duplicate entry (%d)\n",
> > +			sr->name, idx - 1, tc.msr);
> > +		return -EINVAL;
> > +	}
> > +
> > +	tc.msr = idx + 1;
> > +	ret = xa_store(&sr_forward_xa, encoding,
> > +		       xa_mk_value(tc.val), GFP_KERNEL);
> > +
> > +	return xa_err(ret);
> > +}
> > +
> >  static enum trap_behaviour get_behaviour(struct kvm_vcpu *vcpu,
> >  					 const struct trap_bits *tb)
> >  {
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 77cd818c23b0..65319193e443 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -3974,6 +3974,7 @@ int __init kvm_sys_reg_table_init(void)
> >  	struct sys_reg_params params;
> >  	bool valid = true;
> >  	unsigned int i;
> > +	int ret = 0;
> >  
> >  	/* Make sure tables are unique and in order. */
> >  	valid &= check_sysreg_table(sys_reg_descs, ARRAY_SIZE(sys_reg_descs), false);
> > @@ -3997,5 +3998,13 @@ int __init kvm_sys_reg_table_init(void)
> >  	if (!first_idreg)
> >  		return -EINVAL;
> >  
> > -	return populate_nv_trap_config();
> > +	ret = populate_nv_trap_config();
> > +
> > +	for (i = 0; !ret && i < ARRAY_SIZE(sys_reg_descs); i++)
> > +		ret = populate_sysreg_config(sys_reg_descs + i, i);
> > +
> > +	for (i = 0; !ret && i < ARRAY_SIZE(sys_insn_descs); i++)
> > +		ret = populate_sysreg_config(sys_insn_descs + i, i);
> > +
> > +	return ret;
> >  }
> 
> The choice of `msr` was a tiny bit confusing due to the conflict with the asm
> instruction `msr`, but not enough to warrant renaming.

No, that's actually a very good point. How about SRI (Sys Reg Index)?

> Reviewed-by: Joey Gouly <joey.gouly@arm.com>

Thanks!

	M.

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