Re: [PATCH v2 2/7] KVM: arm64: Abstract set/clear of CPTR_EL2 bits behind helper

From: Marc Zyngier <maz@kernel.org>
To: Fuad Tabba <tabba@google.com>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	will@kernel.org, qperret@google.com, seanjc@google.com,
	alexandru.elisei@arm.com, catalin.marinas@arm.com,
	philmd@linaro.org, james.morse@arm.com, suzuki.poulose@arm.com,
	oliver.upton@linux.dev, mark.rutland@arm.com, broonie@kernel.org,
	joey.gouly@arm.com, rananta@google.com, yuzenghui@huawei.com
Subject: Re: [PATCH v2 2/7] KVM: arm64: Abstract set/clear of CPTR_EL2 bits behind helper
Date: Tue, 21 May 2024 22:08:46 +0100	[thread overview]
Message-ID: <86seyana8x.wl-maz@kernel.org> (raw)
In-Reply-To: <20240521163720.3812851-3-tabba@google.com>

On Tue, 21 May 2024 17:37:15 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> The same traps controlled by CPTR_EL2 or CPACR_EL1 need to be
> toggled in different parts of the code, but the exact bits and
> their polarity differ between these two formats and the mode
> (vhe/nvhe/hvhe).
> 
> To reduce the amount of duplicated code and the chance of getting
> the wrong bit/polarity or missing a field, abstract the set/clear
> of CPTR_EL2 bits behind a helper.
> 
> Since (h)VHE is the way of the future, use the CPACR_EL1 format,
> which is a subset of the VHE CPTR_EL2, as a reference.
> 
> No functional change intended.
> 
> Suggested-by: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/include/asm/kvm_emulate.h    | 34 +++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 17 +++----------
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c      |  6 +----
>  3 files changed, 39 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 501e3e019c93..74837d1762e5 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -557,6 +557,40 @@ static __always_inline void kvm_incr_pc(struct kvm_vcpu *vcpu)
>  		vcpu_set_flag((v), e);					\
>  	} while (0)
>  
> +
> +static inline void __cptr_clear_set_nvhe(u64 cpacr_clr, u64 cpacr_set)
> +{
> +	u64 clr = 0, set = 0;
> +
> +	if (cpacr_clr & CPACR_ELx_FPEN)
> +		set |= CPTR_EL2_TFP;
> +	if (cpacr_clr & CPACR_ELx_ZEN)
> +		set |= CPTR_EL2_TZ;
> +	if (cpacr_clr & CPACR_ELx_SMEN)

These 3 fields are actually pairs of bits. Can we have a compile-time
check that both bits are set?

> +		set |= CPTR_EL2_TSM;
> +	if (cpacr_clr & CPACR_ELx_TTA)
> +		clr |= CPTR_EL2_TTA;

How about TCPAC, TAM, and E0POE?

> +
> +	if (cpacr_set & CPACR_ELx_FPEN)
> +		clr |= CPTR_EL2_TFP;
> +	if (cpacr_set & CPACR_ELx_ZEN)
> +		clr |= CPTR_EL2_TZ;
> +	if (cpacr_set & CPACR_ELx_SMEN)
> +		clr |= CPTR_EL2_TSM;
> +	if (cpacr_set & CPACR_ELx_TTA)
> +		set |= CPTR_EL2_TTA;

The duplication is pretty unfortunate. Having a single helper that
translate a register layout into another would be better.

> +
> +	sysreg_clear_set(cptr_el2, clr, set);

And omit this...

> +}
> +
> +static inline void cpacr_clear_set(u64 clr, u64 set)
> +{
> +        if (has_vhe() || has_hvhe())
> +                sysreg_clear_set(cpacr_el1, clr, set);
> +        else
> +                __cptr_clear_set_nvhe(clr, set);

So that this could read as:

	sysreg_clear_set(cptr_el2, cpacr_to_cptr(clr), cpacr_to_cptr(set));

Thanks,

	M.

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