Re: [PATCH v2 3/4] KVM: arm64: Leave vcpu FPSIMD synchronization in host

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm



[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux