Re: [PATCH 03/18] KVM: arm64: Drop FP_FOREIGN_STATE from the hypervisor code

From: Reiji Watanabe <reijiw@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	 Linux ARM <linux-arm-kernel@lists.infradead.org>,
	kernel-team@android.com,  Will Deacon <will@kernel.org>,
	Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH 03/18] KVM: arm64: Drop FP_FOREIGN_STATE from the hypervisor code
Date: Thu, 2 Jun 2022 22:23:25 -0700	[thread overview]
Message-ID: <CAAeT=FxmD4Nsrodr-FCjpNghAormCg4P+R7hF3+g_wfQ5T12Rg@mail.gmail.com> (raw)
In-Reply-To: <20220528113829.1043361-4-maz@kernel.org>

Hi Marc,

On Sat, May 28, 2022 at 4:38 AM Marc Zyngier <maz@kernel.org> wrote:
>
> The vcpu KVM_ARM64_FP_FOREIGN_FPSTATE flag tracks the thread's own
> TIF_FOREIGN_FPSTATE so that we can evaluate just before running
> the vcpu whether it the FP regs contain something that is owned
> by the vcpu or not by updating the rest of the FP flags.
>
> We do this in the hypervisor code in order to make sure we're
> in a context where we are not interruptible. But we already
> have a hook in the run loop to generate this flag. We may as
> well update the FP flags directly and save the pointless flag
> tracking.
>
> Whilst we're at it, rename update_fp_enabled() to guest_owns_fp_regs()
> to indicate what the leftover of this helper actually do.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -107,16 +107,19 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
>  }
>
>  /*
> - * Called just before entering the guest once we are no longer
> - * preemptable. Syncs the host's TIF_FOREIGN_FPSTATE with the KVM
> - * mirror of the flag used by the hypervisor.
> + * Called just before entering the guest once we are no longer preemptable
> + * and interrupts are disabled. If we have managed to run anything using
> + * FP while we were preemptible (such as off the back of an interrupt),
> + * then neither the host nor the guest own the FP hardware (and it was the
> + * responsibility of the code that used FP to save the existing state).
> + *
> + * Note that not supporting FP is basically the same thing as far as the
> + * hypervisor is concerned (nothing to save).
>   */
>  void kvm_arch_vcpu_ctxflush_fp(struct kvm_vcpu *vcpu)
>  {
> -       if (test_thread_flag(TIF_FOREIGN_FPSTATE))
> -               vcpu->arch.flags |= KVM_ARM64_FP_FOREIGN_FPSTATE;
> -       else
> -               vcpu->arch.flags &= ~KVM_ARM64_FP_FOREIGN_FPSTATE;
> +       if (!system_supports_fpsimd() || test_thread_flag(TIF_FOREIGN_FPSTATE))
> +               vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED | KVM_ARM64_FP_HOST);
>  }

Although kvm_arch_vcpu_load_fp() unconditionally sets KVM_ARM64_FP_HOST,
perhaps having kvm_arch_vcpu_load_fp() set KVM_ARM64_FP_HOST only when
FP is supported might be more consistent?
Then, checking system_supports_fpsimd() is unnecessary here.
(KVM_ARM64_FP_ENABLED is not set when FP is not supported)

Thanks,
Reiji

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel