[PATCH v2 0/4] Manage vcpu flags from the host

[PATCH v2 0/4] Manage vcpu flags from the host

[Andrew Scull]

The aim is to keep management of the flags in the host and out of hyp
where possible. I find this makes it easier to understand how the flags
are used as the responsibilities are clearly divided.

The series applies on top of kvmarm/next after VHE and nVHE have been
separated.

From v1 <20200710095754.3641976-1-ascull@google.com>:
 - Split FP change into smaller patches
 - Addressed Dave's other comments

Andrew Scull (4):
  KVM: arm64: Leave KVM_ARM64_DEBUG_DIRTY updates to the host
  KVM: arm64: Predicate FPSIMD vcpu flags on feature support
  KVM: arm64: Leave vcpu FPSIMD synchronization in host
  KVM: arm64: Stop mapping host task thread flags to hyp

 arch/arm64/include/asm/kvm_host.h         |  7 ++-
 arch/arm64/kvm/arm.c                      |  4 +-
 arch/arm64/kvm/debug.c                    |  2 +
 arch/arm64/kvm/fpsimd.c                   | 54 ++++++++++++++++-------
 arch/arm64/kvm/hyp/include/hyp/debug-sr.h |  2 -
 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 +-
 8 files changed, 48 insertions(+), 46 deletions(-)

-- 
2.27.0.383.g050319c2ae-goog

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

[PATCH v2 1/4] KVM: arm64: Leave KVM_ARM64_DEBUG_DIRTY updates to the host

[Andrew Scull]

Move the clearing of KVM_ARM64_DEBUG_DIRTY from being one of the last
things hyp does before exiting to the host to being one of the first
things the host does after hyp exits.

This means the host always manages the state of the bit and hyp simply
respects that in the context switch.

No functional change.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 arch/arm64/include/asm/kvm_host.h         | 2 +-
 arch/arm64/kvm/debug.c                    | 2 ++
 arch/arm64/kvm/hyp/include/hyp/debug-sr.h | 2 --
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e1a32c0707bb..b06f24b5f443 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -404,7 +404,7 @@ struct kvm_vcpu_arch {
 })
 
 /* vcpu_arch flags field values: */
-#define KVM_ARM64_DEBUG_DIRTY		(1 << 0)
+#define KVM_ARM64_DEBUG_DIRTY		(1 << 0) /* vcpu is using debug */
 #define KVM_ARM64_FP_ENABLED		(1 << 1) /* guest FP regs loaded */
 #define KVM_ARM64_FP_HOST		(1 << 2) /* host FP regs loaded */
 #define KVM_ARM64_HOST_SVE_IN_USE	(1 << 3) /* backup for host TIF_SVE */
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 7a7e425616b5..e9932618a362 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -209,6 +209,8 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
 	trace_kvm_arm_clear_debug(vcpu->guest_debug);
 
+	vcpu->arch.flags &= ~KVM_ARM64_DEBUG_DIRTY;
+
 	if (vcpu->guest_debug) {
 		restore_guest_debug_regs(vcpu);
 
diff --git a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
index 0297dc63988c..50ca5d048017 100644
--- a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
@@ -161,8 +161,6 @@ static inline void __debug_switch_to_host_common(struct kvm_vcpu *vcpu)
 
 	__debug_save_state(guest_dbg, guest_ctxt);
 	__debug_restore_state(host_dbg, host_ctxt);
-
-	vcpu->arch.flags &= ~KVM_ARM64_DEBUG_DIRTY;
 }
 
 #endif /* __ARM64_KVM_HYP_DEBUG_SR_H__ */
-- 
2.27.0.383.g050319c2ae-goog

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

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

[Andrew Scull]

If the system doesn't support FPSIMD features then the flags must never
be set. These are the same feature checks performed by hyp when handling
an FPSIMD trap.

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.
+	 */
 	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;
+	}
 }
 
 /*
-- 
2.27.0.383.g050319c2ae-goog

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

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

[Andrew Scull]

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.

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
+	 * 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;
 	} else {
-- 
2.27.0.383.g050319c2ae-goog

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

[PATCH v2 4/4] KVM: arm64: Stop mapping host task thread flags to hyp

[Andrew Scull]

Since hyp now doesn't access the host task's thread flags, there's no
need to map them up to hyp.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  2 --
 arch/arm64/kvm/fpsimd.c           | 11 +----------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1a062d44b395..fb0dfffa8be1 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -24,7 +24,6 @@
 #include <asm/fpsimd.h>
 #include <asm/kvm.h>
 #include <asm/kvm_asm.h>
-#include <asm/thread_info.h>
 
 #define __KVM_HAVE_ARCH_INTC_INITIALIZED
 
@@ -320,7 +319,6 @@ struct kvm_vcpu_arch {
 	struct kvm_guest_debug_arch vcpu_debug_state;
 	struct kvm_guest_debug_arch external_debug_state;
 
-	struct thread_info *host_thread_info;	/* hyp VA */
 	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
 
 	struct {
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 2779cc11f3dd..08ce264c2f41 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -27,22 +27,13 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
 {
 	int ret;
 
-	struct thread_info *ti = &current->thread_info;
 	struct user_fpsimd_state *fpsimd = &current->thread.uw.fpsimd_state;
 
-	/*
-	 * Make sure the host task thread flags and fpsimd state are
-	 * visible to hyp:
-	 */
-	ret = create_hyp_mappings(ti, ti + 1, PAGE_HYP);
-	if (ret)
-		goto error;
-
+	/* Make sure the host task fpsimd state is visible to hyp: */
 	ret = create_hyp_mappings(fpsimd, fpsimd + 1, PAGE_HYP);
 	if (ret)
 		goto error;
 
-	vcpu->arch.host_thread_info = kern_hyp_va(ti);
 	vcpu->arch.host_fpsimd_state = kern_hyp_va(fpsimd);
 error:
 	return ret;
-- 
2.27.0.383.g050319c2ae-goog

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

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

[Dave Martin]

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

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

[Dave Martin]

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

Re: [PATCH v2 4/4] KVM: arm64: Stop mapping host task thread flags to hyp

[Dave Martin]

On Mon, Jul 13, 2020 at 10:05:05PM +0100, Andrew Scull wrote:

Familiar nits about commit message and Subject line.

> Since hyp now doesn't access the host task's thread flags, there's no
> need to map them up to hyp.
> 
> Signed-off-by: Andrew Scull <ascull@google.com>

With a reworked commit message:

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  arch/arm64/include/asm/kvm_host.h |  2 --
>  arch/arm64/kvm/fpsimd.c           | 11 +----------
>  2 files changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1a062d44b395..fb0dfffa8be1 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -24,7 +24,6 @@
>  #include <asm/fpsimd.h>
>  #include <asm/kvm.h>
>  #include <asm/kvm_asm.h>
> -#include <asm/thread_info.h>
>  
>  #define __KVM_HAVE_ARCH_INTC_INITIALIZED
>  
> @@ -320,7 +319,6 @@ struct kvm_vcpu_arch {
>  	struct kvm_guest_debug_arch vcpu_debug_state;
>  	struct kvm_guest_debug_arch external_debug_state;
>  
> -	struct thread_info *host_thread_info;	/* hyp VA */
>  	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
>  
>  	struct {
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index 2779cc11f3dd..08ce264c2f41 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -27,22 +27,13 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
>  {
>  	int ret;
>  
> -	struct thread_info *ti = &current->thread_info;
>  	struct user_fpsimd_state *fpsimd = &current->thread.uw.fpsimd_state;
>  
> -	/*
> -	 * Make sure the host task thread flags and fpsimd state are
> -	 * visible to hyp:
> -	 */
> -	ret = create_hyp_mappings(ti, ti + 1, PAGE_HYP);
> -	if (ret)
> -		goto error;
> -
> +	/* Make sure the host task fpsimd state is visible to hyp: */
>  	ret = create_hyp_mappings(fpsimd, fpsimd + 1, PAGE_HYP);
>  	if (ret)
>  		goto error;
>  
> -	vcpu->arch.host_thread_info = kern_hyp_va(ti);
>  	vcpu->arch.host_fpsimd_state = kern_hyp_va(fpsimd);
>  error:
>  	return ret;
> -- 
> 2.27.0.383.g050319c2ae-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/4] KVM: arm64: Leave KVM_ARM64_DEBUG_DIRTY updates to the host

[Dave Martin]

On Mon, Jul 13, 2020 at 10:05:02PM +0100, Andrew Scull wrote:
> Move the clearing of KVM_ARM64_DEBUG_DIRTY from being one of the last
> things hyp does before exiting to the host to being one of the first
> things the host does after hyp exits.
> 
> This means the host always manages the state of the bit and hyp simply
> respects that in the context switch.
> 
> No functional change.
> 
> Signed-off-by: Andrew Scull <ascull@google.com>

Seems reasonable, though we have to map the vcpu arch flags into hyp
anyway.  For FPSIMD we do maintain these flags from hyp, in order to
void mapping in host-specific stuff (the thread flags).

So maybe this change isn't that useful?

I don't have a strong opinion though.  If this change fits in better
with the broader KVM work you're doing, I don't see a problem with it.

So, FWIW:

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  arch/arm64/include/asm/kvm_host.h         | 2 +-
>  arch/arm64/kvm/debug.c                    | 2 ++
>  arch/arm64/kvm/hyp/include/hyp/debug-sr.h | 2 --
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e1a32c0707bb..b06f24b5f443 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -404,7 +404,7 @@ struct kvm_vcpu_arch {
>  })
>  
>  /* vcpu_arch flags field values: */
> -#define KVM_ARM64_DEBUG_DIRTY		(1 << 0)
> +#define KVM_ARM64_DEBUG_DIRTY		(1 << 0) /* vcpu is using debug */
>  #define KVM_ARM64_FP_ENABLED		(1 << 1) /* guest FP regs loaded */
>  #define KVM_ARM64_FP_HOST		(1 << 2) /* host FP regs loaded */
>  #define KVM_ARM64_HOST_SVE_IN_USE	(1 << 3) /* backup for host TIF_SVE */
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 7a7e425616b5..e9932618a362 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -209,6 +209,8 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>  	trace_kvm_arm_clear_debug(vcpu->guest_debug);
>  
> +	vcpu->arch.flags &= ~KVM_ARM64_DEBUG_DIRTY;
> +
>  	if (vcpu->guest_debug) {
>  		restore_guest_debug_regs(vcpu);
>  
> diff --git a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> index 0297dc63988c..50ca5d048017 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> @@ -161,8 +161,6 @@ static inline void __debug_switch_to_host_common(struct kvm_vcpu *vcpu)
>  
>  	__debug_save_state(guest_dbg, guest_ctxt);
>  	__debug_restore_state(host_dbg, host_ctxt);
> -
> -	vcpu->arch.flags &= ~KVM_ARM64_DEBUG_DIRTY;
>  }
>  
>  #endif /* __ARM64_KVM_HYP_DEBUG_SR_H__ */
> -- 
> 2.27.0.383.g050319c2ae-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 0/4] Manage vcpu flags from the host

[Dave Martin]

On Mon, Jul 13, 2020 at 10:05:01PM +0100, Andrew Scull wrote:
> The aim is to keep management of the flags in the host and out of hyp
> where possible. I find this makes it easier to understand how the flags
> are used as the responsibilities are clearly divided.
> 
> The series applies on top of kvmarm/next after VHE and nVHE have been
> separated.

(A commit ID would be useful for someone trying to the patches.)

> From v1 <20200710095754.3641976-1-ascull@google.com>:

(Nit: Is there some easy way of looking up mails by Message-ID?

Otherwise, it can be helpful to have a mail archive URL here, e.g.,
lore.kernel.org)

>  - Split FP change into smaller patches
>  - Addressed Dave's other comments
> 
> Andrew Scull (4):
>   KVM: arm64: Leave KVM_ARM64_DEBUG_DIRTY updates to the host
>   KVM: arm64: Predicate FPSIMD vcpu flags on feature support
>   KVM: arm64: Leave vcpu FPSIMD synchronization in host
>   KVM: arm64: Stop mapping host task thread flags to hyp
> 
>  arch/arm64/include/asm/kvm_host.h         |  7 ++-
>  arch/arm64/kvm/arm.c                      |  4 +-
>  arch/arm64/kvm/debug.c                    |  2 +
>  arch/arm64/kvm/fpsimd.c                   | 54 ++++++++++++++++-------
>  arch/arm64/kvm/hyp/include/hyp/debug-sr.h |  2 -
>  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 +-
>  8 files changed, 48 insertions(+), 46 deletions(-)
> 
> -- 
> 2.27.0.383.g050319c2ae-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 0/4] Manage vcpu flags from the host

[Marc Zyngier]

On 2020-07-22 17:24, Dave Martin wrote:
> On Mon, Jul 13, 2020 at 10:05:01PM +0100, Andrew Scull wrote:
>> The aim is to keep management of the flags in the host and out of hyp
>> where possible. I find this makes it easier to understand how the 
>> flags
>> are used as the responsibilities are clearly divided.
>> 
>> The series applies on top of kvmarm/next after VHE and nVHE have been
>> separated.
> 
> (A commit ID would be useful for someone trying to the patches.)
> 
>> From v1 <20200710095754.3641976-1-ascull@google.com>:
> 
> (Nit: Is there some easy way of looking up mails by Message-ID?
> 
> Otherwise, it can be helpful to have a mail archive URL here, e.g.,
> lore.kernel.org)

I routinely use https://lore.kernel.org/r/<msgid>, which does
the right thing.  But indeed, including the link directly is
the preferred course of action, and saves having to assemble
the URL by hand.

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 0/4] Manage vcpu flags from the host

[Dave Martin]

On Wed, Jul 22, 2020 at 05:36:34PM +0100, Marc Zyngier wrote:
> On 2020-07-22 17:24, Dave Martin wrote:
> >On Mon, Jul 13, 2020 at 10:05:01PM +0100, Andrew Scull wrote:
> >>The aim is to keep management of the flags in the host and out of hyp
> >>where possible. I find this makes it easier to understand how the flags
> >>are used as the responsibilities are clearly divided.
> >>
> >>The series applies on top of kvmarm/next after VHE and nVHE have been
> >>separated.
> >
> >(A commit ID would be useful for someone trying to the patches.)
> >
> >>From v1 <20200710095754.3641976-1-ascull@google.com>:
> >
> >(Nit: Is there some easy way of looking up mails by Message-ID?
> >
> >Otherwise, it can be helpful to have a mail archive URL here, e.g.,
> >lore.kernel.org)
> 
> I routinely use https://lore.kernel.org/r/<msgid>, which does
> the right thing.  But indeed, including the link directly is
> the preferred course of action, and saves having to assemble
> the URL by hand.

Cool, I'll make a mental note.

Now I just need to start posting non-unique message-IDs ;)

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