Re: [PATCH v2 04/12] KVM: arm64: nv: Add support for describing traps that affect Host EL0

[Marc Zyngier <maz@kernel.org>]

On Tue, 27 Aug 2024 01:22:27 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The EL2 PMU trap controls are rather annoying, because they take effect
> for *host* EL0 in addition to the guest. Set up some basic support for
> describing EL2 traps that affect host EL0.
> 
> Do so by describing trap bits that can take effect in host EL0. Preserve
> the early return for most hyp context traps by taking a bit out of the
> trap_config value to indicate if the trap might take effect InHost.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/emulate-nested.c | 37 ++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> index 26d32f308dd3..b9f3e19a1b96 100644
> --- a/arch/arm64/kvm/emulate-nested.c
> +++ b/arch/arm64/kvm/emulate-nested.c
> @@ -20,6 +20,9 @@ enum trap_behaviour {
>  	BEHAVE_FORWARD_READ	= BIT(0),
>  	BEHAVE_FORWARD_WRITE	= BIT(1),
>  	BEHAVE_FORWARD_RW	= BEHAVE_FORWARD_READ | BEHAVE_FORWARD_WRITE,
> +
> +	/* Traps that take effect in Host EL0, this is rare! */
> +	BEHAVE_IN_HOST_EL0	= BIT(2),

Do we need to distinguish reads from writes here? There are precedents
with things like TVM/TRVM, though not affecting EL0.

>  };
>  
>  struct trap_bits {
> @@ -478,7 +481,8 @@ static const complex_condition_check ccc[] = {
>   * [20]		trap polarity (1 bit)
>   * [25:21]	FG filter (5 bits)
>   * [35:26]	Main SysReg table index (10 bits)
> - * [62:36]	Unused (27 bits)
> + * [36]		Trap applies to Host EL0 (1 bit)
> + * [62:37]	Unused (26 bits)
>   * [63]		RES0 - Must be zero, as lost on insertion in the xarray
>   */
>  #define TC_CGT_BITS	10
> @@ -495,7 +499,8 @@ union trap_config {
>  		unsigned long	pol:1;		 /* Polarity */
>  		unsigned long	fgf:TC_FGF_BITS; /* Fine Grained Filter */
>  		unsigned long	sri:TC_SRI_BITS; /* SysReg Index */
> -		unsigned long	unused:27;	 /* Unused, should be zero */
> +		unsigned long	in_host_el0:1;	 /* Applies to Host EL0 */

I'm a bit confused about the meaning of this new bit. If something
applies to a Host EL0 access, we already have the information as part
of the coarse grained traps. Why do we need something extra?

I understand that it is used as some form of summary information and
avoids looking at the CGT chain, but this also muddies the purpose of
the trap_config word, because this doesn't describe *why* we have
trapped.

It's not really a big deal, as we have plenty of spare bits so far,
but I find it a bit jarring.

> +		unsigned long	unused:26;	 /* Unused, should be zero */
>  		unsigned long	mbz:1;		 /* Must Be Zero */
>  	};
>  };
> @@ -1875,6 +1880,28 @@ static u32 encoding_next(u32 encoding)
>  	return sys_reg(op0 + 1, 0, 0, 0, 0);
>  }
>  
> +static bool trap_effective_in_host_el0(const enum cgt_group_id id)
> +{
> +	switch (id) {
> +	case __RESERVED__ ... __MULTIPLE_CONTROL_BITS__ - 1:
> +		return coarse_trap_bits[id].behaviour & BEHAVE_IN_HOST_EL0;
> +	case __MULTIPLE_CONTROL_BITS__ ... __COMPLEX_CONDITIONS__ - 1: {
> +		const enum cgt_group_id *cgids;
> +		int i;
> +
> +		cgids = coarse_control_combo[id - __MULTIPLE_CONTROL_BITS__];
> +		for (i = 0; cgids[i] != __RESERVED__; i++)
> +			if (trap_effective_in_host_el0(cgids[i]))
> +				return true;
> +
> +		return false;
> +	}
> +	/* Just treat complex traps as InHost for now. */

s/InHost/InHost-capable/

> +	default:
> +		return true;
> +	}
> +}
> +
>  int __init populate_nv_trap_config(void)
>  {
>  	int ret = 0;
> @@ -1886,13 +1913,17 @@ int __init populate_nv_trap_config(void)
>  
>  	for (int i = 0; i < ARRAY_SIZE(encoding_to_cgt); i++) {
>  		const struct encoding_to_trap_config *cgt = &encoding_to_cgt[i];
> +		union trap_config tc = cgt->tc;
>  		void *prev;
>  
> -		if (cgt->tc.val & BIT(63)) {
> +		if (tc.val & BIT(63)) {
>  			kvm_err("CGT[%d] has MBZ bit set\n", i);
>  			ret = -EINVAL;
>  		}
>  
> +		if (trap_effective_in_host_el0(tc.cgt))
> +			tc.in_host_el0 = true;

nit: do a direct assignment without the test.

> +
>  		for (u32 enc = cgt->encoding; enc <= cgt->end; enc = encoding_next(enc)) {
>  			prev = xa_store(&sr_forward_xa, enc,
>  					xa_mk_value(cgt->tc.val), GFP_KERNEL);

Thanks,

	M.

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