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 3/4] KVM: arm64: Leave vcpu FPSIMD synchronization in host
Date: Wed, 22 Jul 2020 17:24:08 +0100 [thread overview]
Message-ID: <20200722162407.GM30452@arm.com> (raw)
In-Reply-To: <20200713210505.2959828-4-ascull@google.com>
On Mon, Jul 13, 2020 at 10:05:04PM +0100, Andrew Scull wrote:
vv Nit: Message body doesn't say what changed _or_ why. See comments on
patch 2.
> The task state can be checked by the host and the vcpu flags updated
> before calling into hyp. Hyp simply acts on the state provided to it by
> the host and updates it when switching to the vcpu state.
It would be useful here to explain the renaming of
kvm_arch_vcpu_ctxsync_fp().
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
> arch/arm64/include/asm/kvm_host.h | 3 ++-
> arch/arm64/kvm/arm.c | 4 +++-
> arch/arm64/kvm/fpsimd.c | 19 ++++++++++++++++++-
> arch/arm64/kvm/hyp/include/hyp/switch.h | 19 -------------------
> arch/arm64/kvm/hyp/nvhe/switch.c | 3 +--
> arch/arm64/kvm/hyp/vhe/switch.c | 3 +--
> 6 files changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index b06f24b5f443..1a062d44b395 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -616,7 +616,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> /* Guest/host FPSIMD coordination helpers */
> int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
> -void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu);
> +void kvm_arch_vcpu_sync_fp_before_run(struct kvm_vcpu *vcpu);
> +void kvm_arch_vcpu_sync_fp_after_run(struct kvm_vcpu *vcpu);
> void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
>
> static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr)
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 98f05bdac3c1..c91b0a66bf20 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -682,6 +682,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>
> local_irq_disable();
>
> + kvm_arch_vcpu_sync_fp_before_run(vcpu);
> +
> kvm_vgic_flush_hwstate(vcpu);
>
> /*
> @@ -769,7 +771,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> if (static_branch_unlikely(&userspace_irqchip_in_use))
> kvm_timer_sync_user(vcpu);
>
> - kvm_arch_vcpu_ctxsync_fp(vcpu);
> + kvm_arch_vcpu_sync_fp_after_run(vcpu);
>
> /*
> * We may have taken a host interrupt in HYP mode (ie
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index c6b3197f6754..2779cc11f3dd 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -88,13 +88,30 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> }
> }
>
> +void kvm_arch_vcpu_sync_fp_before_run(struct kvm_vcpu *vcpu)
> +{
> + WARN_ON_ONCE(!irqs_disabled());
> +
> + if (!system_supports_fpsimd())
> + return;
> +
> + /*
> + * If the CPU's FP state is transient, there is no need to save the
See comments on patch 2 regarding "transient".
Beyond not needing to save the state, we must not even attempt to do so.
> + * current state. Without further information, it must also be assumed
> + * that the vcpu's state is not loaded.
> + */
> + if (test_thread_flag(TIF_FOREIGN_FPSTATE))
> + vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
> + KVM_ARM64_FP_HOST);
> +}
> +
> /*
> * If the guest FPSIMD state was loaded, update the host's context
> * tracking data mark the CPU FPSIMD regs as dirty and belonging to vcpu
> * so that they will be written back if the kernel clobbers them due to
> * kernel-mode NEON before re-entry into the guest.
> */
> -void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
> +void kvm_arch_vcpu_sync_fp_after_run(struct kvm_vcpu *vcpu)
> {
> WARN_ON_ONCE(!irqs_disabled());
>
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 0511af14dc81..65cde758abad 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -25,28 +25,9 @@
> #include <asm/fpsimd.h>
> #include <asm/debug-monitors.h>
> #include <asm/processor.h>
> -#include <asm/thread_info.h>
>
> extern const char __hyp_panic_string[];
>
> -/* Check whether the FP regs were dirtied while in the host-side run loop: */
> -static inline bool update_fp_enabled(struct kvm_vcpu *vcpu)
> -{
> - /*
> - * When the system doesn't support FP/SIMD, we cannot rely on
> - * the _TIF_FOREIGN_FPSTATE flag. However, we always inject an
> - * abort on the very first access to FP and thus we should never
> - * see KVM_ARM64_FP_ENABLED. For added safety, make sure we always
> - * trap the accesses.
> - */
> - if (!system_supports_fpsimd() ||
> - vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
> - vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
> - KVM_ARM64_FP_HOST);
> -
> - return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
> -}
> -
> /* Save the 32-bit only FPSIMD system register state */
> static inline void __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
> {
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index 341be2f2f312..3b7306003917 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -25,7 +25,6 @@
> #include <asm/fpsimd.h>
> #include <asm/debug-monitors.h>
> #include <asm/processor.h>
> -#include <asm/thread_info.h>
>
> static void __activate_traps(struct kvm_vcpu *vcpu)
> {
> @@ -36,7 +35,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>
> val = CPTR_EL2_DEFAULT;
> val |= CPTR_EL2_TTA | CPTR_EL2_TZ | CPTR_EL2_TAM;
> - if (!update_fp_enabled(vcpu)) {
> + if (!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED)) {
> val |= CPTR_EL2_TFP;
> __activate_traps_fpsimd32(vcpu);
> }
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index c52d714e0d75..0c08c9123ce5 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -24,7 +24,6 @@
> #include <asm/fpsimd.h>
> #include <asm/debug-monitors.h>
> #include <asm/processor.h>
> -#include <asm/thread_info.h>
>
> const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
>
> @@ -49,7 +48,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>
> val |= CPTR_EL2_TAM;
>
> - if (update_fp_enabled(vcpu)) {
> + if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED) {
> if (vcpu_has_sve(vcpu))
> val |= CPACR_EL1_ZEN;
Looks reasonable otherwise.
[...]
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2020-07-22 16:24 UTC|newest]
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
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 [this message]
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200722162407.GM30452@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 \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).