Re: [PATCH v3 4/7] arm64/fpsimd: Stop using TIF_SVE to manage register saving in KVM

[Marc Zyngier <maz@kernel.org>]

On Mon, 15 Aug 2022 23:55:26 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> Now that we are explicitly telling the host FP code which register state
> it needs to save we can remove the manipulation of TIF_SVE from the KVM
> code, simplifying it and allowing us to optimise our handling of normal
> tasks. Remove the manipulation of TIF_SVE from KVM and instead rely on
> to_save to ensure we save the correct data for it.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/fpsimd.c | 22 ++++------------------
>  arch/arm64/kvm/fpsimd.c    |  3 ---
>  2 files changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 7be20ced2c45..aaea2dc02cbd 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -436,8 +436,8 @@ static void task_fpsimd_load(void)
>   * last, if KVM is involved this may be the guest VM context rather
>   * than the host thread for the VM pointed to by current. This means
>   * that we must always reference the state storage via last rather
> - * than via current, other than the TIF_ flags which KVM will
> - * carefully maintain for us.
> + * than via current, if we are saving KVM state then it will have
> + * ensured that the type of registers to save is set in last->to_save.
>   */
>  static void fpsimd_save(void)
>  {
> @@ -454,27 +454,13 @@ static void fpsimd_save(void)
>  	if (test_thread_flag(TIF_FOREIGN_FPSTATE))
>  		return;
>  
> -	if (test_thread_flag(TIF_SVE)) {
> +	if ((last->to_save == FP_STATE_TASK && test_thread_flag(TIF_SVE)) ||
> +	    last->to_save == FP_STATE_SVE) {
>  		save_sve_regs = true;
>  		save_ffr = true;
>  		vl = last->sve_vl;
>  	}
>  
> -	/*
> -	 * For now we're just validating that the requested state is
> -	 * consistent with what we'd otherwise work out.
> -	 */
> -	switch (last->to_save) {
> -	case FP_STATE_TASK:
> -		break;
> -	case FP_STATE_FPSIMD:
> -		WARN_ON_ONCE(save_sve_regs);
> -		break;
> -	case FP_STATE_SVE:
> -		WARN_ON_ONCE(!save_sve_regs);
> -		break;
> -	}
> -

Given how short-lived this code is, consider dropping it altogether.
Actually, the previous patch would make a lot more sense if it was
merged with this one.

>  	if (system_supports_sme()) {
>  		u64 *svcr = last->svcr;
>  
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index db0b2bacaeb8..8a79823fce68 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -151,7 +151,6 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
>  					 &vcpu->arch.fp_type, fp_type);
>  
>  		clear_thread_flag(TIF_FOREIGN_FPSTATE);
> -		update_thread_flag(TIF_SVE, vcpu_has_sve(vcpu));
>  	}
>  }
>  
> @@ -208,7 +207,5 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
>  			sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
>  	}
>  
> -	update_thread_flag(TIF_SVE, 0);
> -
>  	local_irq_restore(flags);
>  }

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm