Re: [PATCH v3 3/7] arm64/fpsimd: Have KVM explicitly say which FP registers to save

[Marc Zyngier <maz@kernel.org>]

On Mon, 15 Aug 2022 23:55:25 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> In order to avoid needlessly saving and restoring the guest registers KVM
> relies on the host FPSMID code to save the guest registers when we context
> switch away from the guest. This is done by binding the KVM guest state to
> the CPU on top of the task state that was originally there, then carefully
> managing the TIF_SVE flag for the task to cause the host to save the full
> SVE state when needed regardless of the needs of the host task. This works
> well enough but isn't terribly direct about what is going on and makes it
> much more complicated to try to optimise what we're doing with the SVE
> register state.
> 
> Let's instead have KVM pass in the register state it wants saving when it
> binds to the CPU. We introduce a new FP_TYPE_TASK for use during normal
> task binding to indicate that we should base our decisions on the current
> task. In order to ease any future debugging that might be required this
> patch does not actually update any of the decision making about what to
> save, it merely starts tracking the new information and warns if the
> requested state is not what we would otherwise have decided to save.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/fpsimd.h    |  3 ++-
>  arch/arm64/include/asm/processor.h |  1 +
>  arch/arm64/kernel/fpsimd.c         | 20 +++++++++++++++++++-
>  arch/arm64/kvm/fpsimd.c            |  9 ++++++++-
>  4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index b74103a79052..21a1dd320ca5 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -61,7 +61,8 @@ extern void fpsimd_kvm_prepare(void);
>  extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state,
>  				     void *sve_state, unsigned int sve_vl,
>  				     void *za_state, unsigned int sme_vl,
> -				     u64 *svcr, enum fp_state *type);
> +				     u64 *svcr, enum fp_state *type,
> +				     enum fp_state to_save);
>  
>  extern void fpsimd_flush_task_state(struct task_struct *target);
>  extern void fpsimd_save_and_flush_cpu_state(void);
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 4818a6b77f39..89c248b8d4ba 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -123,6 +123,7 @@ enum vec_type {
>  };
>  
>  enum fp_state {
> +	FP_STATE_TASK,		/* Save based on current, invalid as fp_type */

How is that related to the FP_TYPE_TASK in the commit message? What
does this 'invalid as fp_type' mean?

>  	FP_STATE_FPSIMD,
>  	FP_STATE_SVE,
>  };
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 6544ae00230f..7be20ced2c45 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -126,6 +126,7 @@ struct fpsimd_last_state_struct {
>  	unsigned int sve_vl;
>  	unsigned int sme_vl;
>  	enum fp_state *fp_type;
> +	enum fp_state to_save;
>  };
>  
>  static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
> @@ -459,6 +460,21 @@ static void fpsimd_save(void)
>  		vl = last->sve_vl;
>  	}
>  
> +	/*
> +	 * For now we're just validating that the requested state is
> +	 * consistent with what we'd otherwise work out.

Nit: work out? or worked out? the "we'd" doesn't help disambiguate it
for a non-native speaker.

> +	 */
> +	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;
> +	}
> +
>  	if (system_supports_sme()) {
>  		u64 *svcr = last->svcr;
>  
> @@ -1693,6 +1709,7 @@ static void fpsimd_bind_task_to_cpu(void)
>  	last->sme_vl = task_get_sme_vl(current);
>  	last->svcr = &current->thread.svcr;
>  	last->fp_type = &current->thread.fp_type;
> +	last->to_save = FP_STATE_TASK;
>  	current->thread.fpsimd_cpu = smp_processor_id();
>  
>  	/*
> @@ -1717,7 +1734,7 @@ static void fpsimd_bind_task_to_cpu(void)
>  void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
>  			      unsigned int sve_vl, void *za_state,
>  			      unsigned int sme_vl, u64 *svcr,
> -			      enum fp_state *type)
> +			      enum fp_state *type, enum fp_state to_save)

OK, how many discrete arguments are we going to pass to this function,
which most of them are part the vcpu structure? It really feels like
what you want is a getter for the per-cpu structure, and let the KVM
code do the actual business. If this function was supposed to provide
some level of abstraction, well, it's a fail.

>  {
>  	struct fpsimd_last_state_struct *last =
>  		this_cpu_ptr(&fpsimd_last_state);
> @@ -1732,6 +1749,7 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
>  	last->sve_vl = sve_vl;
>  	last->sme_vl = sme_vl;
>  	last->fp_type = type;
> +	last->to_save = to_save;
>  }
>  
>  /*
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index a92977759f8d..db0b2bacaeb8 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -130,9 +130,16 @@ void kvm_arch_vcpu_ctxflush_fp(struct kvm_vcpu *vcpu)
>   */
>  void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
>  {
> +	enum fp_state fp_type;
> +
>  	WARN_ON_ONCE(!irqs_disabled());
>  
>  	if (vcpu->arch.fp_state == FP_STATE_GUEST_OWNED) {
> +		if (vcpu_has_sve(vcpu))
> +			fp_type = FP_STATE_SVE;

Eventually, I'd like to relax this, and start tracking the actual use
of the guest rather than assuming that SVE guest use SVE at all times
(odds are they won't).

I hope this series still leaves us with this option.

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