Re: [PATCH v2 2/4] KVM: arm64: Predicate FPSIMD vcpu flags on feature support

[Dave Martin <Dave.Martin@arm.com>]

On Mon, Jul 13, 2020 at 10:05:03PM +0100, Andrew Scull wrote:
> If the system doesn't support FPSIMD features then the flags must never

Mustn't they?  Why not?  I think the flags are currently ignored in this
case, which is just as good.

I'm not disagreeing with the change here; I just want to be clear on the
rationale.

> be set. These are the same feature checks performed by hyp when handling
> an FPSIMD trap.

Nit: Try to ensure that the commit message make sense even without the
subject line: i.e., the subject line is just a one-line summary of the
commit message and should not add any new information.

(This makes life easier for users of mailers that invoke an editor on
the message body only when replying -- i.e., Mutt and probably some
others.  It also helps with understanding the state in .git/rebase-apply/
during a rebase, where the subject line and the rest of the message end
up in different places.)


Also, it's worth nothing the comment additions here, since they look
substantial and it's not clear from just looking at this patch that the
new comments are just clarifying the existing behaviour.

> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  arch/arm64/kvm/fpsimd.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index 3e081d556e81..c6b3197f6754 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -52,7 +52,7 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
>   * Prepare vcpu for saving the host's FPSIMD state and loading the guest's.
>   * The actual loading is done by the FPSIMD access trap taken to hyp.
>   *
> - * Here, we just set the correct metadata to indicate that the FPSIMD
> + * Here, we just set the correct metadata to indicate whether the FPSIMD
>   * state in the cpu regs (if any) belongs to current on the host.
>   *
>   * TIF_SVE is backed up here, since it may get clobbered with guest state.
> @@ -63,15 +63,29 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
>  	BUG_ON(!current->mm);
>  
>  	vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
> +			      KVM_ARM64_FP_HOST |
>  			      KVM_ARM64_HOST_SVE_IN_USE |
>  			      KVM_ARM64_HOST_SVE_ENABLED);
> +
> +	if (!system_supports_fpsimd())
> +		return;
> +
> +	/*
> +	 * Having just come from the user task, if any FP state is loaded it
> +	 * will be that of the task. Make a note of this but, just before
> +	 * entering the vcpu, it will be double checked that the loaded FP
> +	 * state isn't transient because things could change between now and
> +	 * then.
> +	 */

Can we avoid this word "transient"?  Just because the state isn't our
state doesn't mean it will be thrown away.

If the regs contains the state for task foo, and we exit the run loop
before taking an FP trap from the guest, then we might context switch
back to foo before re-entering userspace in the KVM thread.  In that
case the regs aren't reloaded.  Unless someone called
fpsimd_flush_cpu_state() in the meantime, the regs will be assumed still
to be correctly loaded for foo.

To be clear, TIF_FOREIGN_FPSTATE doesn't mean that the regs are garbage,
just that they don't contain the right state for current.


This may not matter that much for this code, but I don't want people to
get confused when maintaining related code...


Here, does it make sense to say something like:

--8<--

Having just come from the user task, if the FP regs contain state for
current then it is definitely host user state, not vcpu state.  Note
this here, ready for the first entry to the guest.

-->8--

>  	vcpu->arch.flags |= KVM_ARM64_FP_HOST;
>  
> -	if (test_thread_flag(TIF_SVE))
> -		vcpu->arch.flags |= KVM_ARM64_HOST_SVE_IN_USE;
> +	if (system_supports_sve()) {
> +		if (test_thread_flag(TIF_SVE))
> +			vcpu->arch.flags |= KVM_ARM64_HOST_SVE_IN_USE;
>  
> -	if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN)
> -		vcpu->arch.flags |= KVM_ARM64_HOST_SVE_ENABLED;
> +		if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN)
> +			vcpu->arch.flags |= KVM_ARM64_HOST_SVE_ENABLED;
> +	}
>  }

[...]

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm