Re: [PATCH v1 02/44] KVM: arm64: Use enum instead of helper for checking FP-state

[Marc Zyngier <maz@kernel.org>]

On Wed, 27 Mar 2024 17:34:49 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> Before the conversion of the various FP-state booleans into an
> enum representing the state, this helper may have clarified
> things. Since the introduction of the enum, the helper obfuscates
> rather than clarifies. This also makes the code consistent with
> other parts that check the FP-state.
> 
> No functional change intended.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 6 ------
>  arch/arm64/kvm/hyp/nvhe/switch.c        | 2 +-
>  arch/arm64/kvm/hyp/vhe/switch.c         | 2 +-
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index e3fcf8c4d5b4..1a6dfd035531 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -39,12 +39,6 @@ struct kvm_exception_table_entry {
>  extern struct kvm_exception_table_entry __start___kvm_ex_table;
>  extern struct kvm_exception_table_entry __stop___kvm_ex_table;
>  
> -/* Check whether the FP regs are owned by the guest */
> -static inline bool guest_owns_fp_regs(struct kvm_vcpu *vcpu)
> -{
> -	return vcpu->arch.fp_state == FP_STATE_GUEST_OWNED;
> -}
> -
>  /* Save the 32-bit only FPSIMD system register state */
>  static inline void __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
>  {
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index c50f8459e4fc..2a0b0d6da7c7 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -53,7 +53,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>  			val |= CPTR_EL2_TSM;
>  	}
>  
> -	if (!guest_owns_fp_regs(vcpu)) {
> +	if (vcpu->arch.fp_state != FP_STATE_GUEST_OWNED) {
>  		if (has_hvhe())
>  			val &= ~(CPACR_EL1_FPEN_EL0EN | CPACR_EL1_FPEN_EL1EN |
>  				 CPACR_EL1_ZEN_EL0EN | CPACR_EL1_ZEN_EL1EN);
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index 1581df6aec87..e9197f086137 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -75,7 +75,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>  
>  	val |= CPTR_EL2_TAM;
>  
> -	if (guest_owns_fp_regs(vcpu)) {
> +	if (vcpu->arch.fp_state == FP_STATE_GUEST_OWNED) {
>  		if (vcpu_has_sve(vcpu))
>  			val |= CPACR_EL1_ZEN_EL0EN | CPACR_EL1_ZEN_EL1EN;
>  	} else {

I'm not sure this buys us much. If anything, it makes moving the data
around more difficult (see [1]).

Overall, asking whether the vcpu owns the FP state seems a natural
question, and I would have expected this helper to be generalised
instead of being dropped.

Could you please elaborate on this?

Thanks,

	M.

[1] https://lore.kernel.org/all/20240322170945.3292593-6-maz@kernel.org/

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