KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Martin <Dave.Martin@arm.com>
To: Andrew Scull <ascull@google.com>
Cc: maz@kernel.org, kernel-team@android.com, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH v2 2/4] KVM: arm64: Predicate FPSIMD vcpu flags on feature support
Date: Wed, 22 Jul 2020 17:23:41 +0100
Message-ID: <20200722162340.GL30452@arm.com> (raw)
In-Reply-To: <20200713210505.2959828-3-ascull@google.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

> 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:


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.


>  	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;
> +	}
>  }


kvmarm mailing list

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-13 21:05 [PATCH v2 0/4] Manage vcpu flags from the host Andrew Scull
2020-07-13 21:05 ` [PATCH v2 1/4] KVM: arm64: Leave KVM_ARM64_DEBUG_DIRTY updates to " Andrew Scull
2020-07-22 16:24   ` Dave Martin
2020-07-13 21:05 ` [PATCH v2 2/4] KVM: arm64: Predicate FPSIMD vcpu flags on feature support Andrew Scull
2020-07-22 16:23   ` Dave Martin [this message]
2020-07-13 21:05 ` [PATCH v2 3/4] KVM: arm64: Leave vcpu FPSIMD synchronization in host Andrew Scull
2020-07-22 16:24   ` Dave Martin
2020-07-13 21:05 ` [PATCH v2 4/4] KVM: arm64: Stop mapping host task thread flags to hyp Andrew Scull
2020-07-22 16:24   ` Dave Martin
2020-07-22 16:24 ` [PATCH v2 0/4] Manage vcpu flags from the host Dave Martin
2020-07-22 16:36   ` Marc Zyngier
2020-07-22 16:40     ` Dave Martin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200722162340.GL30452@arm.com \
    --to=dave.martin@arm.com \
    --cc=ascull@google.com \
    --cc=kernel-team@android.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=maz@kernel.org \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git