[RFC PATCH 0/8] KVM: arm64: Optimise FPSIMD context switching

[RFC PATCH 0/8] KVM: arm64: Optimise FPSIMD context switching

[Dave Martin]

This is another respin of my attempt to improve FPSIMD context handling
for KVM, building on the previous RFC v3 [1].

It's not completely done yet, but it's sufficiently closer to
completeness that a it's worth people taking another look at.

The main changes since RFC v3 are:

 * Rebased to v4.17-rc1.  Please pay close attention to patch 3 which is
   where most of the pain was felt.  Note that fpsimd_enabled() is now
   gone, since vcpu->arch.fp_enabled provides the same information.  The
   diff is a little confusing to read there.

 * patches 4-6: Make sve_pffr more easliy inlinable into Hyp, along with
   some annoying header refactoring to resolve a #include cycle that is
   otherwise hard to avoid here.

 * patch 7: Add support for saving the host SVE state from hyp.

 * patch 8: Remove SVE host-side SVE flushing logic that is rendered
   unnecessary by patch 7.

Other minor changes as noted.


Some testing done on Juno and the Arm fast model (arm64), including
combinations of (non-)SVE and (non-)VHE configurations.


Blurb:

See the individual patches for detailed explanation.

Some things (still) aren't right yet:

 * The BUG_ON(system_supports_sve()) added in kvm_arch_vcpu_load_fp() by
   patch 3 and subsequently removed by patch 7 is a bisectability break
   (albeit only for people running on SVE-enabled models).  I intend to
   fix this before upstreaming, possibly via a check at VM creation time
   similar to the check added in patch 7.

 * Moving __hyp_switch_fpsimd() to the shallow exit path as requested by
   Marc Zyngier has *not* been done yet.  I will look at that next so
   that people can benchmark with and without that change.

 * task_fpsimd_save() still appears misnamed, but in lieu of having
   decided on a better name I've just exported this function from
   fpsimd.c for now.


Any comments, testing, benchmarks appreciated!

Cheers
---Dave


[1] [RFC PATCH v3 0/4] KVM: arm64: Optimise FPSIMD context switching
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/570740.html


Christoffer Dall (1):
  KVM: arm/arm64: Introduce kvm_arch_vcpu_run_pid_change

Dave Martin (7):
  KVM: arm64: Convert lazy FPSIMD context switch trap to C
  KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing
  arm64/sve: Move read_zcr_features() out of cpufeature.h
  arm64/sve: Switch sve_pffr() argument from task to thread
  arm64/sve: Move sve_pffr() to fpsimd.h and make inline
  KVM: arm64: Save host SVE context as appropriate
  KVM: arm64: Remove eager host SVE state saving

 arch/arm/include/asm/kvm_host.h     |   9 ++-
 arch/arm64/Kconfig                  |   7 +++
 arch/arm64/include/asm/cpufeature.h |  29 ----------
 arch/arm64/include/asm/fpsimd.h     |  20 +++++++
 arch/arm64/include/asm/kvm_host.h   |  22 +++++---
 arch/arm64/include/asm/processor.h  |   2 +
 arch/arm64/kernel/fpsimd.c          |  87 ++++++++++++++++-------------
 arch/arm64/kernel/ptrace.c          |   1 +
 arch/arm64/kvm/Kconfig              |   1 +
 arch/arm64/kvm/Makefile             |   2 +-
 arch/arm64/kvm/fpsimd.c             | 108 ++++++++++++++++++++++++++++++++++++
 arch/arm64/kvm/hyp/entry.S          |  57 ++++++++-----------
 arch/arm64/kvm/hyp/switch.c         |  83 +++++++++++++++++++--------
 include/linux/kvm_host.h            |   9 +++
 virt/kvm/Kconfig                    |   3 +
 virt/kvm/arm/arm.c                  |  25 ++++++++-
 virt/kvm/kvm_main.c                 |   7 ++-
 17 files changed, 332 insertions(+), 140 deletions(-)
 create mode 100644 arch/arm64/kvm/fpsimd.c

-- 
2.1.4

[RFC PATCH 1/8] KVM: arm/arm64: Introduce kvm_arch_vcpu_run_pid_change

[Dave Martin]

From: Christoffer Dall <christoffer.dall@linaro.org>

KVM/ARM differs from other architectures in having to maintain an
additional virtual address space from that of the host and the guest,
because we split the execution of KVM across both EL1 and EL2.

This results in a need to explicitly map data structures into EL2 (hyp)
which are accessed from the hyp code.  As we are about to be more clever
with our FPSIMD handling, which stores data on the task struct and uses
thread_info flags, we have to map the currently executing task struct
into the EL2 virtual address space.

However, we don't want to do this on every KVM_RUN, because it is a
fairly expensive operation to walk the page tables, and the common
execution mode is to map a single thread to a VCPU.  By introducing a
hook that architectures can select with HAVE_KVM_VCPU_RUN_PID_CHANGE, we
do not introduce overhead for other architectures, but have a simple way
to only map the data we need when required for arm64.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Since RFCv1:

Back out setting of hyp_current, which isn't introduced to struct
vcpu_arch by this patch.  This series takes the approach of only
mapping current->thread_info instead in a later patch, which is
sufficient.
---
 arch/arm64/kvm/Kconfig   |  1 +
 include/linux/kvm_host.h |  9 +++++++++
 virt/kvm/Kconfig         |  3 +++
 virt/kvm/arm/arm.c       | 10 ++++++++++
 virt/kvm/kvm_main.c      |  7 ++++++-
 5 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index a2e3a5a..47b23bf 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -39,6 +39,7 @@ config KVM
 	select HAVE_KVM_IRQ_ROUTING
 	select IRQ_BYPASS_MANAGER
 	select HAVE_KVM_IRQ_BYPASS
+	select HAVE_KVM_VCPU_RUN_PID_CHANGE
 	---help---
 	  Support hosting virtualized guest machines.
 	  We don't support KVM with 16K page tables yet, due to the multiple
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6930c63..4268ace 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1276,4 +1276,13 @@ static inline long kvm_arch_vcpu_async_ioctl(struct file *filp,
 void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
 		unsigned long start, unsigned long end);
 
+#ifdef CONFIG_HAVE_KVM_VCPU_RUN_PID_CHANGE
+int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu);
+#else
+static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
+{
+	return 0;
+}
+#endif /* CONFIG_HAVE_KVM_VCPU_RUN_PID_CHANGE */
+
 #endif
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index cca7e06..72143cf 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -54,3 +54,6 @@ config HAVE_KVM_IRQ_BYPASS
 
 config HAVE_KVM_VCPU_ASYNC_IOCTL
        bool
+
+config HAVE_KVM_VCPU_RUN_PID_CHANGE
+       bool
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index dba629c..cd38d7d 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -816,6 +816,16 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return ret;
 }
 
+#ifdef CONFIG_ARM64
+int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
+{
+	struct task_struct *tsk = current;
+
+	/* Make sure the host task fpsimd state is visible to hyp: */
+	return create_hyp_mappings(tsk, tsk + 1, PAGE_HYP);
+}
+#endif
+
 static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
 {
 	int bit_index;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c7b2e92..c32e240 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2550,8 +2550,13 @@ static long kvm_vcpu_ioctl(struct file *filp,
 		oldpid = rcu_access_pointer(vcpu->pid);
 		if (unlikely(oldpid != current->pids[PIDTYPE_PID].pid)) {
 			/* The thread running this VCPU changed. */
-			struct pid *newpid = get_task_pid(current, PIDTYPE_PID);
+			struct pid *newpid;
 
+			r = kvm_arch_vcpu_run_pid_change(vcpu);
+			if (r)
+				break;
+
+			newpid = get_task_pid(current, PIDTYPE_PID);
 			rcu_assign_pointer(vcpu->pid, newpid);
 			if (oldpid)
 				synchronize_rcu();
-- 
2.1.4

[RFC PATCH 2/8] KVM: arm64: Convert lazy FPSIMD context switch trap to C

[Dave Martin]

To make the lazy FPSIMD context switch trap code easier to hack on,
this patch converts it to C.

This is not amazingly efficient, but the trap should typically only
be taken once per host context switch.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Since RFCv1:

 * Fix indentation to be consistent with the rest of the file.
 * Add missing ! to write back to sp with attempting to push regs.
---
 arch/arm64/kvm/hyp/entry.S  | 57 +++++++++++++++++----------------------------
 arch/arm64/kvm/hyp/switch.c | 24 +++++++++++++++++++
 2 files changed, 46 insertions(+), 35 deletions(-)

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index e41a161..40f349b 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -172,40 +172,27 @@ ENTRY(__fpsimd_guest_restore)
 	// x1: vcpu
 	// x2-x29,lr: vcpu regs
 	// vcpu x0-x1 on the stack
-	stp	x2, x3, [sp, #-16]!
-	stp	x4, lr, [sp, #-16]!
-
-alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
-	mrs	x2, cptr_el2
-	bic	x2, x2, #CPTR_EL2_TFP
-	msr	cptr_el2, x2
-alternative_else
-	mrs	x2, cpacr_el1
-	orr	x2, x2, #CPACR_EL1_FPEN
-	msr	cpacr_el1, x2
-alternative_endif
-	isb
-
-	mov	x3, x1
-
-	ldr	x0, [x3, #VCPU_HOST_CONTEXT]
-	kern_hyp_va x0
-	add	x0, x0, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
-	bl	__fpsimd_save_state
-
-	add	x2, x3, #VCPU_CONTEXT
-	add	x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
-	bl	__fpsimd_restore_state
-
-	// Skip restoring fpexc32 for AArch64 guests
-	mrs	x1, hcr_el2
-	tbnz	x1, #HCR_RW_SHIFT, 1f
-	ldr	x4, [x3, #VCPU_FPEXC32_EL2]
-	msr	fpexc32_el2, x4
-1:
-	ldp	x4, lr, [sp], #16
-	ldp	x2, x3, [sp], #16
-	ldp	x0, x1, [sp], #16
-
+	stp	x2, x3, [sp, #-144]!
+	stp	x4, x5, [sp, #16]
+	stp	x6, x7, [sp, #32]
+	stp	x8, x9, [sp, #48]
+	stp	x10, x11, [sp, #64]
+	stp	x12, x13, [sp, #80]
+	stp	x14, x15, [sp, #96]
+	stp	x16, x17, [sp, #112]
+	stp	x18, lr, [sp, #128]
+
+	bl	__hyp_switch_fpsimd
+
+	ldp	x4, x5, [sp, #16]
+	ldp	x6, x7, [sp, #32]
+	ldp	x8, x9, [sp, #48]
+	ldp	x10, x11, [sp, #64]
+	ldp	x12, x13, [sp, #80]
+	ldp	x14, x15, [sp, #96]
+	ldp	x16, x17, [sp, #112]
+	ldp	x18, lr, [sp, #128]
+	ldp	x0, x1, [sp, #144]
+	ldp	x2, x3, [sp], #160
 	eret
 ENDPROC(__fpsimd_guest_restore)
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index d964523..c438ad8 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -499,6 +499,30 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	return exit_code;
 }
 
+void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
+				    struct kvm_vcpu *vcpu)
+{
+	kvm_cpu_context_t *host_ctxt;
+
+	if (has_vhe())
+		write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
+			     cpacr_el1);
+	else
+		write_sysreg(read_sysreg(cptr_el2) & ~(u64)CPTR_EL2_TFP,
+			     cptr_el2);
+
+	isb();
+
+	host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
+	__fpsimd_save_state(&host_ctxt->gp_regs.fp_regs);
+	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
+
+	/* Skip restoring fpexc32 for AArch64 guests */
+	if (!(read_sysreg(hcr_el2) & HCR_RW))
+		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
+			     fpexc32_el2);
+}
+
 static const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
 
 static void __hyp_text __hyp_call_panic_nvhe(u64 spsr, u64 elr, u64 par,
-- 
2.1.4

[RFC PATCH 3/8] KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing

[Dave Martin]

This patch refactors KVM to align the host and guest FPSIMD
save/restore logic with each other for arm64.  This reduces the
number of redundant save/restore operations that must occur, and
reduces the common-case IRQ blackout time during guest exit storms
by saving the host state lazily and optimising away the need to
restore the host state before returning to the run loop.

Four hooks are defined in order to enable this:

 * kvm_arch_vcpu_run_map_fp():
   Called on PID change to map necessary bits of current to Hyp.

 * kvm_arch_vcpu_load_fp():
   Set up FP/SIMD for entering the KVM run loop (parse as
   "vcpu_load fp").

 * kvm_arch_vcpu_park_fp():
   Get FP/SIMD into a safe state for re-enabling interrupts after a
   guest exit back to the run loop.

   For arm64 specifically, this involves updating the host kernel's
   FPSIMD context tracking metadata so that kernel-mode NEON use
   will cause the vcpu's FPSIMD state to be saved back correctly
   into the vcpu struct.  This must be done before re-enabling
   interrupts because kernel-mode NEON may be used my softirqs.

 * kvm_arch_vcpu_put_fp():
   Save guest FP/SIMD state back to memory and dissociate from the
   CPU ("vcpu_put fp").

Also, the arm64 FPSIMD context switch code is updated to enable it
to save back FPSIMD state for a vcpu, not just current.  A few
helpers drive this:

 * fpsimd_bind_state_to_cpu(struct user_fpsimd_state *fp):
   mark this CPU as having context fp (which may belong to a vcpu)
   currently loaded in its registers.  This is the non-task
   equivalent of the static function fpsimd_bind_to_cpu() in
   fpsimd.c.

 * task_fpsimd_save():
   exported to allow KVM to save the guest's FPSIMD state back to
   memory on exit from the run loop.

 * fpsimd_flush_state():
   invalidate any context's FPSIMD state that is currently loaded.
   Used to disassociate the vcpu from the CPU regs on run loop exit.

These changes allow the run loop to enable interrupts (and thus
softirqs that may use kernel-mode NEON) without having to save the
guest's FPSIMD state eagerly.

Some new vcpu_arch fields are added to make all this work.  Because
host FPSIMD state can now be saved back directly into current's
thread_struct as appropriate, host_cpu_context is no longer used
for preserving the FPSIMD state.  However, it is still needed for
preserving other things such as the host's system registers.  To
avoid ABI churn, the redundant storage space in host_cpu_context is
not removed for now.

arch/arm is not addressed by this patch and continues to use its
current save/restore logic.  It could provide implementations of
the helpers later if desired.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since RFC v3:

Requested by Christoffer Dall:

 * Remove fpsimd_flush_state(), which nothing uses.
   Inline it back into fpsimd_flush_task_state().

 * Add __hyp_text anootation to update_fp_enabled().

Other:

 * Remove fp_enabled().

   Now that we have an explicit field vcpu->arch.fp_enabled to
   indicate whether the guest's FPSIMD state is loaded, we can use
   this to determine when to save FPEXC_EL1 for 32-bit guests.
   This brings the logic for FPEXC_EL1 save back into line with the
   (updated) logic for restore of this register, and simplifies the
   code a bit.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/include/asm/kvm_host.h   |   8 +++
 arch/arm64/include/asm/fpsimd.h   |   5 ++
 arch/arm64/include/asm/kvm_host.h |  18 +++++++
 arch/arm64/kernel/fpsimd.c        |  24 ++++++---
 arch/arm64/kvm/Makefile           |   2 +-
 arch/arm64/kvm/fpsimd.c           | 109 ++++++++++++++++++++++++++++++++++++++
 arch/arm64/kvm/hyp/switch.c       |  50 +++++++++--------
 virt/kvm/arm/arm.c                |  14 ++---
 8 files changed, 187 insertions(+), 43 deletions(-)
 create mode 100644 arch/arm64/kvm/fpsimd.c

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index c6a7495..3fe01c7 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -300,6 +300,14 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
 int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 
+/*
+ * VFP/NEON switching is all done by the hyp switch code, so no need to
+ * coordinate with host context handling for this state:
+ */
+static inline void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) {}
+static inline void kvm_arch_vcpu_park_fp(struct kvm_vcpu *vcpu) {}
+static inline void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) {}
+
 /* All host FP/SIMD state is restored on guest exit, so nothing to save: */
 static inline void kvm_fpsimd_flush_cpu_state(void) {}
 
diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index aa7162a..a1c5127 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -41,6 +41,8 @@ struct task_struct;
 extern void fpsimd_save_state(struct user_fpsimd_state *state);
 extern void fpsimd_load_state(struct user_fpsimd_state *state);
 
+extern void task_fpsimd_save(void);
+
 extern void fpsimd_thread_switch(struct task_struct *next);
 extern void fpsimd_flush_thread(void);
 
@@ -49,7 +51,10 @@ extern void fpsimd_preserve_current_state(void);
 extern void fpsimd_restore_current_state(void);
 extern void fpsimd_update_current_state(struct user_fpsimd_state const *state);
 
+extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state);
+
 extern void fpsimd_flush_task_state(struct task_struct *target);
+extern void fpsimd_flush_cpu_state(void);
 extern void sve_flush_cpu_state(void);
 
 /* Maximum VL that SVE VL-agnostic software can transparently support */
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index ab46bc7..2fbfbda 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -30,6 +30,7 @@
 #include <asm/kvm.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmio.h>
+#include <asm/thread_info.h>
 
 #define __KVM_HAVE_ARCH_INTC_INITIALIZED
 
@@ -235,6 +236,12 @@ struct kvm_vcpu_arch {
 
 	/* Pointer to host CPU context */
 	kvm_cpu_context_t *host_cpu_context;
+
+	struct thread_info *host_thread_info;	/* hyp VA */
+	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
+	bool host_sve_in_use;	/* backup for host TIF_SVE while in guest */
+	bool fp_enabled;
+
 	struct {
 		/* {Break,watch}point registers */
 		struct kvm_guest_debug_arch regs;
@@ -417,6 +424,17 @@ static inline void __cpu_init_stage2(void)
 		  "PARange is %d bits, unsupported configuration!", parange);
 }
 
+/* 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_park_fp(struct kvm_vcpu *vcpu);
+void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
+
+static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
+{
+	return kvm_arch_vcpu_run_map_fp(vcpu);
+}
+
 /*
  * All host FP/SIMD state is restored on guest exit, so nothing needs
  * doing here except in the SVE case:
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 87a3536..7f74374 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -270,13 +270,15 @@ static void task_fpsimd_load(void)
 }
 
 /*
- * Ensure current's FPSIMD/SVE storage in thread_struct is up to date
+ * Ensure current's FPSIMD/SVE storage in memory is up to date
  * with respect to the CPU registers.
  *
  * Softirqs (and preemption) must be disabled.
  */
-static void task_fpsimd_save(void)
+void task_fpsimd_save(void)
 {
+	struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st);
+
 	WARN_ON(!in_softirq() && !irqs_disabled());
 
 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
@@ -291,10 +293,9 @@ static void task_fpsimd_save(void)
 				return;
 			}
 
-			sve_save_state(sve_pffr(current),
-				       &current->thread.uw.fpsimd_state.fpsr);
+			sve_save_state(sve_pffr(current), &st->fpsr);
 		} else
-			fpsimd_save_state(&current->thread.uw.fpsimd_state);
+			fpsimd_save_state(st);
 	}
 }
 
@@ -1012,6 +1013,17 @@ static void fpsimd_bind_to_cpu(void)
 	current->thread.fpsimd_cpu = smp_processor_id();
 }
 
+void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
+{
+	struct fpsimd_last_state_struct *last =
+		this_cpu_ptr(&fpsimd_last_state);
+
+	WARN_ON(!in_softirq() && !irqs_disabled());
+
+	last->st = st;
+	last->sve_in_use = false;
+}
+
 /*
  * Load the userland FPSIMD state of 'current' from memory, but only if the
  * FPSIMD state already held in the registers is /not/ the most recent FPSIMD
@@ -1064,7 +1076,7 @@ void fpsimd_flush_task_state(struct task_struct *t)
 	t->thread.fpsimd_cpu = NR_CPUS;
 }
 
-static inline void fpsimd_flush_cpu_state(void)
+void fpsimd_flush_cpu_state(void)
 {
 	__this_cpu_write(fpsimd_last_state.st, NULL);
 }
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 93afff9..0f2a135 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -19,7 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
 kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
 kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
 kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
-kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o
+kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o fpsimd.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/aarch32.o
 
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
new file mode 100644
index 0000000..cc7a068
--- /dev/null
+++ b/arch/arm64/kvm/fpsimd.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * arch/arm64/kvm/fpsimd.c: Guest/host FPSIMD context coordination helpers
+ *
+ * Copyright 2018 Arm Limited
+ * Author: Dave Martin <Dave.Martin@arm.com>
+ */
+#include <linux/bottom_half.h>
+#include <linux/sched.h>
+#include <linux/thread_info.h>
+#include <linux/kvm_host.h>
+#include <asm/kvm_host.h>
+#include <asm/kvm_mmu.h>
+
+/*
+ * Called on entry to KVM_RUN unless this vcpu previously ran at least
+ * once and the most recent prior KVM_RUN for this vcpu was called from
+ * the same task as current (highly likely).
+ *
+ * This is guaranteed to execute before kvm_arch_vcpu_load_fp(vcpu),
+ * such that on entering hyp the relevant parts of current are already
+ * mapped.
+ */
+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;
+
+	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;
+}
+
+/*
+ * 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
+ * state in the cpu regs (if any) belongs to current, and where to write
+ * it back to if/when a FPSIMD access trap is taken.
+ *
+ * TIF_SVE is backed up here, since it may get clobbered with guest state.
+ * This flag is restored by kvm_arch_vcpu_put_fp(vcpu).
+ */
+void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
+{
+	BUG_ON(system_supports_sve());
+	BUG_ON(!current->mm);
+
+	vcpu->arch.fp_enabled = false;
+	vcpu->arch.host_fpsimd_state =
+		kern_hyp_va(&current->thread.uw.fpsimd_state);
+	vcpu->arch.host_sve_in_use = !!test_thread_flag(TIF_SVE);
+}
+
+/*
+ * If the guest FPSIMD state was loaded, mark the CPU FPSIMD regs as
+ * dirty for 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_park_fp(struct kvm_vcpu *vcpu)
+{
+	WARN_ON_ONCE(!irqs_disabled());
+
+	if (vcpu->arch.fp_enabled) {
+		fpsimd_bind_state_to_cpu(&vcpu->arch.ctxt.gp_regs.fp_regs);
+		clear_thread_flag(TIF_FOREIGN_FPSTATE);
+		clear_thread_flag(TIF_SVE);
+	}
+}
+
+/*
+ * Write back the vcpu FPSIMD regs if they are dirty, and invalidate the
+ * cpu FPSIMD regs so that they can't be spuriously reused if this vcpu
+ * disappears and another task or vcpu appears that recycles the same
+ * struct fpsimd_state.
+ */
+void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
+{
+	local_bh_disable();
+
+	if (vcpu->arch.fp_enabled) {
+		task_fpsimd_save();
+		fpsimd_flush_cpu_state();
+		set_thread_flag(TIF_FOREIGN_FPSTATE);
+	}
+
+	if (vcpu->arch.host_sve_in_use)
+		set_thread_flag(TIF_SVE);
+	else
+		clear_thread_flag(TIF_SVE);
+
+	local_bh_enable();
+}
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index c438ad8..0900c2a 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -27,15 +27,16 @@
 #include <asm/kvm_mmu.h>
 #include <asm/fpsimd.h>
 #include <asm/debug-monitors.h>
+#include <asm/thread_info.h>
 
-static bool __hyp_text __fpsimd_enabled_nvhe(void)
+static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
 {
-	return !(read_sysreg(cptr_el2) & CPTR_EL2_TFP);
-}
+	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE) {
+		vcpu->arch.host_fpsimd_state = NULL;
+		vcpu->arch.fp_enabled = false;
+	}
 
-static bool fpsimd_enabled_vhe(void)
-{
-	return !!(read_sysreg(cpacr_el1) & CPACR_EL1_FPEN);
+	return vcpu->arch.fp_enabled;
 }
 
 /* Save the 32-bit only FPSIMD system register state */
@@ -92,7 +93,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
 
 	val = read_sysreg(cpacr_el1);
 	val |= CPACR_EL1_TTA;
-	val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN);
+	val &= ~CPACR_EL1_ZEN;
+	if (!update_fp_enabled(vcpu))
+		val &= ~CPACR_EL1_FPEN;
+
 	write_sysreg(val, cpacr_el1);
 
 	write_sysreg(kvm_get_hyp_vector(), vbar_el1);
@@ -105,7 +109,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 	__activate_traps_common(vcpu);
 
 	val = CPTR_EL2_DEFAULT;
-	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
+	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
+	if (!update_fp_enabled(vcpu))
+		val |= CPTR_EL2_TFP;
+
 	write_sysreg(val, cptr_el2);
 }
 
@@ -394,7 +401,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
-	bool fp_enabled;
 	u64 exit_code;
 
 	host_ctxt = vcpu->arch.host_cpu_context;
@@ -416,19 +422,14 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 		/* And we're baaack! */
 	} while (fixup_guest_exit(vcpu, &exit_code));
 
-	fp_enabled = fpsimd_enabled_vhe();
-
 	sysreg_save_guest_state_vhe(guest_ctxt);
 
 	__deactivate_traps(vcpu);
 
 	sysreg_restore_host_state_vhe(host_ctxt);
 
-	if (fp_enabled) {
-		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
-		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
+	if (vcpu->arch.fp_enabled)
 		__fpsimd_save_fpexc32(vcpu);
-	}
 
 	__debug_switch_to_host(vcpu);
 
@@ -440,7 +441,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_cpu_context *guest_ctxt;
-	bool fp_enabled;
 	u64 exit_code;
 
 	vcpu = kern_hyp_va(vcpu);
@@ -472,8 +472,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 		/* And we're baaack! */
 	} while (fixup_guest_exit(vcpu, &exit_code));
 
-	fp_enabled = __fpsimd_enabled_nvhe();
-
 	__sysreg_save_state_nvhe(guest_ctxt);
 	__sysreg32_save_state(vcpu);
 	__timer_disable_traps(vcpu);
@@ -484,11 +482,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 
 	__sysreg_restore_state_nvhe(host_ctxt);
 
-	if (fp_enabled) {
-		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
-		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
+	if (vcpu->arch.fp_enabled)
 		__fpsimd_save_fpexc32(vcpu);
-	}
 
 	/*
 	 * This must come after restoring the host sysregs, since a non-VHE
@@ -502,8 +497,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
 				    struct kvm_vcpu *vcpu)
 {
-	kvm_cpu_context_t *host_ctxt;
-
 	if (has_vhe())
 		write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
 			     cpacr_el1);
@@ -513,14 +506,19 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
 
 	isb();
 
-	host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
-	__fpsimd_save_state(&host_ctxt->gp_regs.fp_regs);
+	if (vcpu->arch.host_fpsimd_state) {
+		__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
+		vcpu->arch.host_fpsimd_state = NULL;
+	}
+
 	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
 
 	/* Skip restoring fpexc32 for AArch64 guests */
 	if (!(read_sysreg(hcr_el2) & HCR_RW))
 		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
 			     fpexc32_el2);
+
+	vcpu->arch.fp_enabled = true;
 }
 
 static const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index cd38d7d..4fcf6fe 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -363,10 +363,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	kvm_vgic_load(vcpu);
 	kvm_timer_vcpu_load(vcpu);
 	kvm_vcpu_load_sysregs(vcpu);
+	kvm_arch_vcpu_load_fp(vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 {
+	kvm_arch_vcpu_put_fp(vcpu);
 	kvm_vcpu_put_sysregs(vcpu);
 	kvm_timer_vcpu_put(vcpu);
 	kvm_vgic_put(vcpu);
@@ -773,6 +775,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		if (static_branch_unlikely(&userspace_irqchip_in_use))
 			kvm_timer_sync_hwstate(vcpu);
 
+		kvm_arch_vcpu_park_fp(vcpu);
+
 		/*
 		 * We may have taken a host interrupt in HYP mode (ie
 		 * while executing the guest). This interrupt is still
@@ -816,16 +820,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return ret;
 }
 
-#ifdef CONFIG_ARM64
-int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
-{
-	struct task_struct *tsk = current;
-
-	/* Make sure the host task fpsimd state is visible to hyp: */
-	return create_hyp_mappings(tsk, tsk + 1, PAGE_HYP);
-}
-#endif
-
 static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
 {
 	int bit_index;
-- 
2.1.4

[RFC PATCH 3/8] KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing

[Christoffer Dall]

On Fri, Apr 20, 2018 at 05:46:37PM +0100, Dave Martin wrote:
> This patch refactors KVM to align the host and guest FPSIMD
> save/restore logic with each other for arm64.  This reduces the
> number of redundant save/restore operations that must occur, and
> reduces the common-case IRQ blackout time during guest exit storms
> by saving the host state lazily and optimising away the need to
> restore the host state before returning to the run loop.
> 
> Four hooks are defined in order to enable this:
> 
>  * kvm_arch_vcpu_run_map_fp():
>    Called on PID change to map necessary bits of current to Hyp.
> 
>  * kvm_arch_vcpu_load_fp():
>    Set up FP/SIMD for entering the KVM run loop (parse as
>    "vcpu_load fp").
> 
>  * kvm_arch_vcpu_park_fp():
>    Get FP/SIMD into a safe state for re-enabling interrupts after a
>    guest exit back to the run loop.
> 
>    For arm64 specifically, this involves updating the host kernel's
>    FPSIMD context tracking metadata so that kernel-mode NEON use
>    will cause the vcpu's FPSIMD state to be saved back correctly
>    into the vcpu struct.  This must be done before re-enabling
>    interrupts because kernel-mode NEON may be used my softirqs.
> 
>  * kvm_arch_vcpu_put_fp():
>    Save guest FP/SIMD state back to memory and dissociate from the
>    CPU ("vcpu_put fp").
> 
> Also, the arm64 FPSIMD context switch code is updated to enable it
> to save back FPSIMD state for a vcpu, not just current.  A few
> helpers drive this:
> 
>  * fpsimd_bind_state_to_cpu(struct user_fpsimd_state *fp):
>    mark this CPU as having context fp (which may belong to a vcpu)
>    currently loaded in its registers.  This is the non-task
>    equivalent of the static function fpsimd_bind_to_cpu() in
>    fpsimd.c.
> 
>  * task_fpsimd_save():
>    exported to allow KVM to save the guest's FPSIMD state back to
>    memory on exit from the run loop.
> 
>  * fpsimd_flush_state():
>    invalidate any context's FPSIMD state that is currently loaded.
>    Used to disassociate the vcpu from the CPU regs on run loop exit.
> 
> These changes allow the run loop to enable interrupts (and thus
> softirqs that may use kernel-mode NEON) without having to save the
> guest's FPSIMD state eagerly.
> 
> Some new vcpu_arch fields are added to make all this work.  Because
> host FPSIMD state can now be saved back directly into current's
> thread_struct as appropriate, host_cpu_context is no longer used
> for preserving the FPSIMD state.  However, it is still needed for
> preserving other things such as the host's system registers.  To
> avoid ABI churn, the redundant storage space in host_cpu_context is
> not removed for now.
> 
> arch/arm is not addressed by this patch and continues to use its
> current save/restore logic.  It could provide implementations of
> the helpers later if desired.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> 
> ---
> 
> Changes since RFC v3:
> 
> Requested by Christoffer Dall:
> 
>  * Remove fpsimd_flush_state(), which nothing uses.
>    Inline it back into fpsimd_flush_task_state().
> 
>  * Add __hyp_text anootation to update_fp_enabled().
> 
> Other:
> 
>  * Remove fp_enabled().
> 
>    Now that we have an explicit field vcpu->arch.fp_enabled to
>    indicate whether the guest's FPSIMD state is loaded, we can use
>    this to determine when to save FPEXC_EL1 for 32-bit guests.
>    This brings the logic for FPEXC_EL1 save back into line with the
>    (updated) logic for restore of this register, and simplifies the
>    code a bit.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm/include/asm/kvm_host.h   |   8 +++
>  arch/arm64/include/asm/fpsimd.h   |   5 ++
>  arch/arm64/include/asm/kvm_host.h |  18 +++++++
>  arch/arm64/kernel/fpsimd.c        |  24 ++++++---
>  arch/arm64/kvm/Makefile           |   2 +-
>  arch/arm64/kvm/fpsimd.c           | 109 ++++++++++++++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/switch.c       |  50 +++++++++--------
>  virt/kvm/arm/arm.c                |  14 ++---
>  8 files changed, 187 insertions(+), 43 deletions(-)
>  create mode 100644 arch/arm64/kvm/fpsimd.c
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index c6a7495..3fe01c7 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -300,6 +300,14 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
>  int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>  			       struct kvm_device_attr *attr);
>  
> +/*
> + * VFP/NEON switching is all done by the hyp switch code, so no need to
> + * coordinate with host context handling for this state:
> + */
> +static inline void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) {}
> +static inline void kvm_arch_vcpu_park_fp(struct kvm_vcpu *vcpu) {}
> +static inline void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) {}
> +
>  /* All host FP/SIMD state is restored on guest exit, so nothing to save: */
>  static inline void kvm_fpsimd_flush_cpu_state(void) {}
>  
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index aa7162a..a1c5127 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -41,6 +41,8 @@ struct task_struct;
>  extern void fpsimd_save_state(struct user_fpsimd_state *state);
>  extern void fpsimd_load_state(struct user_fpsimd_state *state);
>  
> +extern void task_fpsimd_save(void);
> +
>  extern void fpsimd_thread_switch(struct task_struct *next);
>  extern void fpsimd_flush_thread(void);
>  
> @@ -49,7 +51,10 @@ extern void fpsimd_preserve_current_state(void);
>  extern void fpsimd_restore_current_state(void);
>  extern void fpsimd_update_current_state(struct user_fpsimd_state const *state);
>  
> +extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state);
> +
>  extern void fpsimd_flush_task_state(struct task_struct *target);
> +extern void fpsimd_flush_cpu_state(void);
>  extern void sve_flush_cpu_state(void);
>  
>  /* Maximum VL that SVE VL-agnostic software can transparently support */
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index ab46bc7..2fbfbda 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -30,6 +30,7 @@
>  #include <asm/kvm.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmio.h>
> +#include <asm/thread_info.h>
>  
>  #define __KVM_HAVE_ARCH_INTC_INITIALIZED
>  
> @@ -235,6 +236,12 @@ struct kvm_vcpu_arch {
>  
>  	/* Pointer to host CPU context */
>  	kvm_cpu_context_t *host_cpu_context;
> +
> +	struct thread_info *host_thread_info;	/* hyp VA */
> +	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> +	bool host_sve_in_use;	/* backup for host TIF_SVE while in guest */
> +	bool fp_enabled;
> +
>  	struct {
>  		/* {Break,watch}point registers */
>  		struct kvm_guest_debug_arch regs;
> @@ -417,6 +424,17 @@ static inline void __cpu_init_stage2(void)
>  		  "PARange is %d bits, unsupported configuration!", parange);
>  }
>  
> +/* 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_park_fp(struct kvm_vcpu *vcpu);
> +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
> +
> +static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> +{
> +	return kvm_arch_vcpu_run_map_fp(vcpu);
> +}
> +

nit: It would be nicer to only add the arm64 hook the way they should be
in this patch, and remove the #ifdef CONFIG_ARM64 thing that gets
removed in this patch from patch 1.

>  /*
>   * All host FP/SIMD state is restored on guest exit, so nothing needs
>   * doing here except in the SVE case:
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 87a3536..7f74374 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -270,13 +270,15 @@ static void task_fpsimd_load(void)
>  }
>  
>  /*
> - * Ensure current's FPSIMD/SVE storage in thread_struct is up to date
> + * Ensure current's FPSIMD/SVE storage in memory is up to date
>   * with respect to the CPU registers.
>   *
>   * Softirqs (and preemption) must be disabled.
>   */
> -static void task_fpsimd_save(void)
> +void task_fpsimd_save(void)
>  {
> +	struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st);
> +
>  	WARN_ON(!in_softirq() && !irqs_disabled());
>  
>  	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> @@ -291,10 +293,9 @@ static void task_fpsimd_save(void)
>  				return;
>  			}
>  
> -			sve_save_state(sve_pffr(current),
> -				       &current->thread.uw.fpsimd_state.fpsr);
> +			sve_save_state(sve_pffr(current), &st->fpsr);
>  		} else
> -			fpsimd_save_state(&current->thread.uw.fpsimd_state);
> +			fpsimd_save_state(st);

Is this equivalent to what we had before, because we'll have run
something that sets this cpu's fpsimd_last_state to current's
user_fpsimd_state, when scheduling this thread?

>  	}
>  }
>  
> @@ -1012,6 +1013,17 @@ static void fpsimd_bind_to_cpu(void)
>  	current->thread.fpsimd_cpu = smp_processor_id();
>  }
>  
> +void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
> +{
> +	struct fpsimd_last_state_struct *last =
> +		this_cpu_ptr(&fpsimd_last_state);
> +
> +	WARN_ON(!in_softirq() && !irqs_disabled());
> +
> +	last->st = st;
> +	last->sve_in_use = false;
> +}
> +
>  /*
>   * Load the userland FPSIMD state of 'current' from memory, but only if the
>   * FPSIMD state already held in the registers is /not/ the most recent FPSIMD
> @@ -1064,7 +1076,7 @@ void fpsimd_flush_task_state(struct task_struct *t)
>  	t->thread.fpsimd_cpu = NR_CPUS;
>  }
>  
> -static inline void fpsimd_flush_cpu_state(void)
> +void fpsimd_flush_cpu_state(void)
>  {
>  	__this_cpu_write(fpsimd_last_state.st, NULL);
>  }

You could consider factoring out all changes to kernel/fpsimd.c into a
separate patch explaining why it's correct, which may make bisecting
potential bugs easier, but on the other hand it keeps the changes in the
patch where they're used.  Just a thought.

> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 93afff9..0f2a135 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -19,7 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
> -kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o fpsimd.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/aarch32.o
>  
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> new file mode 100644
> index 0000000..cc7a068
> --- /dev/null
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * arch/arm64/kvm/fpsimd.c: Guest/host FPSIMD context coordination helpers
> + *
> + * Copyright 2018 Arm Limited
> + * Author: Dave Martin <Dave.Martin@arm.com>
> + */
> +#include <linux/bottom_half.h>
> +#include <linux/sched.h>
> +#include <linux/thread_info.h>
> +#include <linux/kvm_host.h>
> +#include <asm/kvm_host.h>
> +#include <asm/kvm_mmu.h>
> +
> +/*
> + * Called on entry to KVM_RUN unless this vcpu previously ran at least
> + * once and the most recent prior KVM_RUN for this vcpu was called from
> + * the same task as current (highly likely).
> + *
> + * This is guaranteed to execute before kvm_arch_vcpu_load_fp(vcpu),
> + * such that on entering hyp the relevant parts of current are already
> + * mapped.
> + */
> +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;
> +
> +	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;
> +}
> +
> +/*
> + * 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
> + * state in the cpu regs (if any) belongs to current, and where to write
> + * it back to if/when a FPSIMD access trap is taken.
> + *
> + * TIF_SVE is backed up here, since it may get clobbered with guest state.
> + * This flag is restored by kvm_arch_vcpu_put_fp(vcpu).

I don't fully understand how TIF_SVE gets clobbered, but perhaps that's
in preparation for a future patch.  (Me keeps reading).

> + */
> +void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> +{
> +	BUG_ON(system_supports_sve());
> +	BUG_ON(!current->mm);
> +
> +	vcpu->arch.fp_enabled = false;
> +	vcpu->arch.host_fpsimd_state =
> +		kern_hyp_va(&current->thread.uw.fpsimd_state);
> +	vcpu->arch.host_sve_in_use = !!test_thread_flag(TIF_SVE);
> +}
> +
> +/*
> + * If the guest FPSIMD state was loaded, mark the CPU FPSIMD regs as
> + * dirty for 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_park_fp(struct kvm_vcpu *vcpu)
> +{
> +	WARN_ON_ONCE(!irqs_disabled());
> +
> +	if (vcpu->arch.fp_enabled) {
> +		fpsimd_bind_state_to_cpu(&vcpu->arch.ctxt.gp_regs.fp_regs);
> +		clear_thread_flag(TIF_FOREIGN_FPSTATE);
> +		clear_thread_flag(TIF_SVE);
> +	}
> +}
> +
> +/*
> + * Write back the vcpu FPSIMD regs if they are dirty, and invalidate the
> + * cpu FPSIMD regs so that they can't be spuriously reused if this vcpu
> + * disappears and another task or vcpu appears that recycles the same
> + * struct fpsimd_state.
> + */
> +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> +{
> +	local_bh_disable();
> +
> +	if (vcpu->arch.fp_enabled) {
> +		task_fpsimd_save();

Just to make sure I'm getting this right: This is safe, because even if
after we exited the VM, before we call this function, if we take a
softirq that has overwritten our fpsimd state, then kernel_neon_begin()
will have set TIF_FOREIGN_FPSTATE, and task_fpsimd_save() won't actually
do anything, and kernel_neon_begin() will have alreay saved the VCPU's
fpsimd state.  Is that right?

> +		fpsimd_flush_cpu_state();
> +		set_thread_flag(TIF_FOREIGN_FPSTATE);
> +	}
> +
> +	if (vcpu->arch.host_sve_in_use)
> +		set_thread_flag(TIF_SVE);
> +	else
> +		clear_thread_flag(TIF_SVE);
> +
> +	local_bh_enable();
> +}
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index c438ad8..0900c2a 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -27,15 +27,16 @@
>  #include <asm/kvm_mmu.h>
>  #include <asm/fpsimd.h>
>  #include <asm/debug-monitors.h>
> +#include <asm/thread_info.h>
>  
> -static bool __hyp_text __fpsimd_enabled_nvhe(void)
> +static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>  {
> -	return !(read_sysreg(cptr_el2) & CPTR_EL2_TFP);
> -}
> +	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE) {
> +		vcpu->arch.host_fpsimd_state = NULL;
> +		vcpu->arch.fp_enabled = false;
> +	}
>  
> -static bool fpsimd_enabled_vhe(void)
> -{
> -	return !!(read_sysreg(cpacr_el1) & CPACR_EL1_FPEN);
> +	return vcpu->arch.fp_enabled;
>  }
>  
>  /* Save the 32-bit only FPSIMD system register state */
> @@ -92,7 +93,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  
>  	val = read_sysreg(cpacr_el1);
>  	val |= CPACR_EL1_TTA;
> -	val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN);
> +	val &= ~CPACR_EL1_ZEN;
> +	if (!update_fp_enabled(vcpu))
> +		val &= ~CPACR_EL1_FPEN;
> +
>  	write_sysreg(val, cpacr_el1);
>  
>  	write_sysreg(kvm_get_hyp_vector(), vbar_el1);
> @@ -105,7 +109,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  	__activate_traps_common(vcpu);
>  
>  	val = CPTR_EL2_DEFAULT;
> -	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
> +	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> +	if (!update_fp_enabled(vcpu))
> +		val |= CPTR_EL2_TFP;
> +
>  	write_sysreg(val, cptr_el2);
>  }
>  
> @@ -394,7 +401,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpu_context *host_ctxt;
>  	struct kvm_cpu_context *guest_ctxt;
> -	bool fp_enabled;
>  	u64 exit_code;
>  
>  	host_ctxt = vcpu->arch.host_cpu_context;
> @@ -416,19 +422,14 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  		/* And we're baaack! */
>  	} while (fixup_guest_exit(vcpu, &exit_code));
>  
> -	fp_enabled = fpsimd_enabled_vhe();
> -
>  	sysreg_save_guest_state_vhe(guest_ctxt);
>  
>  	__deactivate_traps(vcpu);
>  
>  	sysreg_restore_host_state_vhe(host_ctxt);
>  
> -	if (fp_enabled) {
> -		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> -		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> +	if (vcpu->arch.fp_enabled)
>  		__fpsimd_save_fpexc32(vcpu);
> -	}
>  
>  	__debug_switch_to_host(vcpu);
>  
> @@ -440,7 +441,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpu_context *host_ctxt;
>  	struct kvm_cpu_context *guest_ctxt;
> -	bool fp_enabled;
>  	u64 exit_code;
>  
>  	vcpu = kern_hyp_va(vcpu);
> @@ -472,8 +472,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  		/* And we're baaack! */
>  	} while (fixup_guest_exit(vcpu, &exit_code));
>  
> -	fp_enabled = __fpsimd_enabled_nvhe();
> -
>  	__sysreg_save_state_nvhe(guest_ctxt);
>  	__sysreg32_save_state(vcpu);
>  	__timer_disable_traps(vcpu);
> @@ -484,11 +482,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  
>  	__sysreg_restore_state_nvhe(host_ctxt);
>  
> -	if (fp_enabled) {
> -		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> -		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> +	if (vcpu->arch.fp_enabled)
>  		__fpsimd_save_fpexc32(vcpu);
> -	}
>  
>  	/*
>  	 * This must come after restoring the host sysregs, since a non-VHE
> @@ -502,8 +497,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
>  				    struct kvm_vcpu *vcpu)
>  {
> -	kvm_cpu_context_t *host_ctxt;
> -
>  	if (has_vhe())
>  		write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
>  			     cpacr_el1);
> @@ -513,14 +506,19 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
>  
>  	isb();
>  
> -	host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> -	__fpsimd_save_state(&host_ctxt->gp_regs.fp_regs);
> +	if (vcpu->arch.host_fpsimd_state) {
> +		__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
> +		vcpu->arch.host_fpsimd_state = NULL;
> +	}
> +
>  	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
>  
>  	/* Skip restoring fpexc32 for AArch64 guests */
>  	if (!(read_sysreg(hcr_el2) & HCR_RW))
>  		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
>  			     fpexc32_el2);
> +
> +	vcpu->arch.fp_enabled = true;
>  }
>  
>  static const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index cd38d7d..4fcf6fe 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -363,10 +363,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	kvm_vgic_load(vcpu);
>  	kvm_timer_vcpu_load(vcpu);
>  	kvm_vcpu_load_sysregs(vcpu);
> +	kvm_arch_vcpu_load_fp(vcpu);
>  }
>  
>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  {
> +	kvm_arch_vcpu_put_fp(vcpu);
>  	kvm_vcpu_put_sysregs(vcpu);
>  	kvm_timer_vcpu_put(vcpu);
>  	kvm_vgic_put(vcpu);
> @@ -773,6 +775,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		if (static_branch_unlikely(&userspace_irqchip_in_use))
>  			kvm_timer_sync_hwstate(vcpu);
>  
> +		kvm_arch_vcpu_park_fp(vcpu);
> +
>  		/*
>  		 * We may have taken a host interrupt in HYP mode (ie
>  		 * while executing the guest). This interrupt is still
> @@ -816,16 +820,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return ret;
>  }
>  
> -#ifdef CONFIG_ARM64
> -int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> -{
> -	struct task_struct *tsk = current;
> -
> -	/* Make sure the host task fpsimd state is visible to hyp: */
> -	return create_hyp_mappings(tsk, tsk + 1, PAGE_HYP);
> -}
> -#endif
> -
>  static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
>  {
>  	int bit_index;
> -- 
> 2.1.4
> 
Besides the cosmetic comments and questions above, this all looks good
to me!

Thanks,
-Christoffer

[RFC PATCH 3/8] KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing

[Dave Martin]

On Thu, Apr 26, 2018 at 01:21:22PM +0200, Christoffer Dall wrote:
> On Fri, Apr 20, 2018 at 05:46:37PM +0100, Dave Martin wrote:
> > This patch refactors KVM to align the host and guest FPSIMD
> > save/restore logic with each other for arm64.  This reduces the
> > number of redundant save/restore operations that must occur, and
> > reduces the common-case IRQ blackout time during guest exit storms
> > by saving the host state lazily and optimising away the need to
> > restore the host state before returning to the run loop.
> > 
> > Four hooks are defined in order to enable this:
> > 
> >  * kvm_arch_vcpu_run_map_fp():
> >    Called on PID change to map necessary bits of current to Hyp.
> > 
> >  * kvm_arch_vcpu_load_fp():
> >    Set up FP/SIMD for entering the KVM run loop (parse as
> >    "vcpu_load fp").
> > 
> >  * kvm_arch_vcpu_park_fp():
> >    Get FP/SIMD into a safe state for re-enabling interrupts after a
> >    guest exit back to the run loop.
> > 
> >    For arm64 specifically, this involves updating the host kernel's
> >    FPSIMD context tracking metadata so that kernel-mode NEON use
> >    will cause the vcpu's FPSIMD state to be saved back correctly
> >    into the vcpu struct.  This must be done before re-enabling
> >    interrupts because kernel-mode NEON may be used my softirqs.
> > 
> >  * kvm_arch_vcpu_put_fp():
> >    Save guest FP/SIMD state back to memory and dissociate from the
> >    CPU ("vcpu_put fp").
> > 
> > Also, the arm64 FPSIMD context switch code is updated to enable it
> > to save back FPSIMD state for a vcpu, not just current.  A few
> > helpers drive this:
> > 
> >  * fpsimd_bind_state_to_cpu(struct user_fpsimd_state *fp):
> >    mark this CPU as having context fp (which may belong to a vcpu)
> >    currently loaded in its registers.  This is the non-task
> >    equivalent of the static function fpsimd_bind_to_cpu() in
> >    fpsimd.c.
> > 
> >  * task_fpsimd_save():
> >    exported to allow KVM to save the guest's FPSIMD state back to
> >    memory on exit from the run loop.
> > 
> >  * fpsimd_flush_state():
> >    invalidate any context's FPSIMD state that is currently loaded.
> >    Used to disassociate the vcpu from the CPU regs on run loop exit.
> > 
> > These changes allow the run loop to enable interrupts (and thus
> > softirqs that may use kernel-mode NEON) without having to save the
> > guest's FPSIMD state eagerly.
> > 
> > Some new vcpu_arch fields are added to make all this work.  Because
> > host FPSIMD state can now be saved back directly into current's
> > thread_struct as appropriate, host_cpu_context is no longer used
> > for preserving the FPSIMD state.  However, it is still needed for
> > preserving other things such as the host's system registers.  To
> > avoid ABI churn, the redundant storage space in host_cpu_context is
> > not removed for now.
> > 
> > arch/arm is not addressed by this patch and continues to use its
> > current save/restore logic.  It could provide implementations of
> > the helpers later if desired.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > 
> > ---
> > 
> > Changes since RFC v3:
> > 
> > Requested by Christoffer Dall:
> > 
> >  * Remove fpsimd_flush_state(), which nothing uses.
> >    Inline it back into fpsimd_flush_task_state().
> > 
> >  * Add __hyp_text anootation to update_fp_enabled().
> > 
> > Other:
> > 
> >  * Remove fp_enabled().
> > 
> >    Now that we have an explicit field vcpu->arch.fp_enabled to
> >    indicate whether the guest's FPSIMD state is loaded, we can use
> >    this to determine when to save FPEXC_EL1 for 32-bit guests.
> >    This brings the logic for FPEXC_EL1 save back into line with the
> >    (updated) logic for restore of this register, and simplifies the
> >    code a bit.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm/include/asm/kvm_host.h   |   8 +++
> >  arch/arm64/include/asm/fpsimd.h   |   5 ++
> >  arch/arm64/include/asm/kvm_host.h |  18 +++++++
> >  arch/arm64/kernel/fpsimd.c        |  24 ++++++---
> >  arch/arm64/kvm/Makefile           |   2 +-
> >  arch/arm64/kvm/fpsimd.c           | 109 ++++++++++++++++++++++++++++++++++++++
> >  arch/arm64/kvm/hyp/switch.c       |  50 +++++++++--------
> >  virt/kvm/arm/arm.c                |  14 ++---
> >  8 files changed, 187 insertions(+), 43 deletions(-)
> >  create mode 100644 arch/arm64/kvm/fpsimd.c
> > 
> > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > index c6a7495..3fe01c7 100644
> > --- a/arch/arm/include/asm/kvm_host.h
> > +++ b/arch/arm/include/asm/kvm_host.h
> > @@ -300,6 +300,14 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> >  int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> >  			       struct kvm_device_attr *attr);
> >  
> > +/*
> > + * VFP/NEON switching is all done by the hyp switch code, so no need to
> > + * coordinate with host context handling for this state:
> > + */
> > +static inline void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) {}
> > +static inline void kvm_arch_vcpu_park_fp(struct kvm_vcpu *vcpu) {}
> > +static inline void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) {}
> > +
> >  /* All host FP/SIMD state is restored on guest exit, so nothing to save: */
> >  static inline void kvm_fpsimd_flush_cpu_state(void) {}
> >  
> > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> > index aa7162a..a1c5127 100644
> > --- a/arch/arm64/include/asm/fpsimd.h
> > +++ b/arch/arm64/include/asm/fpsimd.h
> > @@ -41,6 +41,8 @@ struct task_struct;
> >  extern void fpsimd_save_state(struct user_fpsimd_state *state);
> >  extern void fpsimd_load_state(struct user_fpsimd_state *state);
> >  
> > +extern void task_fpsimd_save(void);
> > +
> >  extern void fpsimd_thread_switch(struct task_struct *next);
> >  extern void fpsimd_flush_thread(void);
> >  
> > @@ -49,7 +51,10 @@ extern void fpsimd_preserve_current_state(void);
> >  extern void fpsimd_restore_current_state(void);
> >  extern void fpsimd_update_current_state(struct user_fpsimd_state const *state);
> >  
> > +extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state);
> > +
> >  extern void fpsimd_flush_task_state(struct task_struct *target);
> > +extern void fpsimd_flush_cpu_state(void);
> >  extern void sve_flush_cpu_state(void);
> >  
> >  /* Maximum VL that SVE VL-agnostic software can transparently support */
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index ab46bc7..2fbfbda 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -30,6 +30,7 @@
> >  #include <asm/kvm.h>
> >  #include <asm/kvm_asm.h>
> >  #include <asm/kvm_mmio.h>
> > +#include <asm/thread_info.h>
> >  
> >  #define __KVM_HAVE_ARCH_INTC_INITIALIZED
> >  
> > @@ -235,6 +236,12 @@ struct kvm_vcpu_arch {
> >  
> >  	/* Pointer to host CPU context */
> >  	kvm_cpu_context_t *host_cpu_context;
> > +
> > +	struct thread_info *host_thread_info;	/* hyp VA */
> > +	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> > +	bool host_sve_in_use;	/* backup for host TIF_SVE while in guest */
> > +	bool fp_enabled;
> > +
> >  	struct {
> >  		/* {Break,watch}point registers */
> >  		struct kvm_guest_debug_arch regs;
> > @@ -417,6 +424,17 @@ static inline void __cpu_init_stage2(void)
> >  		  "PARange is %d bits, unsupported configuration!", parange);
> >  }
> >  
> > +/* 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_park_fp(struct kvm_vcpu *vcpu);
> > +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
> > +
> > +static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> > +{
> > +	return kvm_arch_vcpu_run_map_fp(vcpu);
> > +}
> > +
> 
> nit: It would be nicer to only add the arm64 hook the way they should be
> in this patch, and remove the #ifdef CONFIG_ARM64 thing that gets
> removed in this patch from patch 1.

Can do.  For now, I was keeping it clear what you write and what I
changed, but if you're happy with the changes I can fold them together
and add by S-o-B on top of yours.

> 
> >  /*
> >   * All host FP/SIMD state is restored on guest exit, so nothing needs
> >   * doing here except in the SVE case:
> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> > index 87a3536..7f74374 100644
> > --- a/arch/arm64/kernel/fpsimd.c
> > +++ b/arch/arm64/kernel/fpsimd.c
> > @@ -270,13 +270,15 @@ static void task_fpsimd_load(void)
> >  }
> >  
> >  /*
> > - * Ensure current's FPSIMD/SVE storage in thread_struct is up to date
> > + * Ensure current's FPSIMD/SVE storage in memory is up to date
> >   * with respect to the CPU registers.
> >   *
> >   * Softirqs (and preemption) must be disabled.
> >   */
> > -static void task_fpsimd_save(void)
> > +void task_fpsimd_save(void)
> >  {
> > +	struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st);
> > +
> >  	WARN_ON(!in_softirq() && !irqs_disabled());
> >  
> >  	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> > @@ -291,10 +293,9 @@ static void task_fpsimd_save(void)
> >  				return;
> >  			}
> >  
> > -			sve_save_state(sve_pffr(current),
> > -				       &current->thread.uw.fpsimd_state.fpsr);
> > +			sve_save_state(sve_pffr(current), &st->fpsr);
> >  		} else
> > -			fpsimd_save_state(&current->thread.uw.fpsimd_state);
> > +			fpsimd_save_state(st);
> 
> Is this equivalent to what we had before, because we'll have run
> something that sets this cpu's fpsimd_last_state to current's
> user_fpsimd_state, when scheduling this thread?

It should be, yes -- at least outside the KVM run loop where
fpsimd_last_state can point into the vcpu instead.

fpsimd_bind_to_cpu() does the setting of fpsimd_last_state.  This is
called when loading current's FPSIMD regs via
fpsimd_restore_current_state(), or just after exiting the guest if
vcpu->arch.fp_enabled became set.

> 
> >  	}
> >  }
> >  
> > @@ -1012,6 +1013,17 @@ static void fpsimd_bind_to_cpu(void)
> >  	current->thread.fpsimd_cpu = smp_processor_id();
> >  }
> >  
> > +void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
> > +{
> > +	struct fpsimd_last_state_struct *last =
> > +		this_cpu_ptr(&fpsimd_last_state);
> > +
> > +	WARN_ON(!in_softirq() && !irqs_disabled());
> > +
> > +	last->st = st;
> > +	last->sve_in_use = false;
> > +}
> > +
> >  /*
> >   * Load the userland FPSIMD state of 'current' from memory, but only if the
> >   * FPSIMD state already held in the registers is /not/ the most recent FPSIMD
> > @@ -1064,7 +1076,7 @@ void fpsimd_flush_task_state(struct task_struct *t)
> >  	t->thread.fpsimd_cpu = NR_CPUS;
> >  }
> >  
> > -static inline void fpsimd_flush_cpu_state(void)
> > +void fpsimd_flush_cpu_state(void)
> >  {
> >  	__this_cpu_write(fpsimd_last_state.st, NULL);
> >  }
> 
> You could consider factoring out all changes to kernel/fpsimd.c into a
> separate patch explaining why it's correct, which may make bisecting
> potential bugs easier, but on the other hand it keeps the changes in the
> patch where they're used.  Just a thought.

I'll have a think about that when I respin.  I think the fpsimd.c-only
part of the patch would be self-evidently correct, so it could make
sense to keep it separate.


> 
> > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> > index 93afff9..0f2a135 100644
> > --- a/arch/arm64/kvm/Makefile
> > +++ b/arch/arm64/kvm/Makefile
> > @@ -19,7 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
> >  kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
> >  kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
> >  kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
> > -kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o
> > +kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o fpsimd.o
> >  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/aarch32.o
> >  
> >  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
> > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> > new file mode 100644
> > index 0000000..cc7a068
> > --- /dev/null
> > +++ b/arch/arm64/kvm/fpsimd.c
> > @@ -0,0 +1,109 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * arch/arm64/kvm/fpsimd.c: Guest/host FPSIMD context coordination helpers
> > + *
> > + * Copyright 2018 Arm Limited
> > + * Author: Dave Martin <Dave.Martin@arm.com>
> > + */
> > +#include <linux/bottom_half.h>
> > +#include <linux/sched.h>
> > +#include <linux/thread_info.h>
> > +#include <linux/kvm_host.h>
> > +#include <asm/kvm_host.h>
> > +#include <asm/kvm_mmu.h>
> > +
> > +/*
> > + * Called on entry to KVM_RUN unless this vcpu previously ran at least
> > + * once and the most recent prior KVM_RUN for this vcpu was called from
> > + * the same task as current (highly likely).
> > + *
> > + * This is guaranteed to execute before kvm_arch_vcpu_load_fp(vcpu),
> > + * such that on entering hyp the relevant parts of current are already
> > + * mapped.
> > + */
> > +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;
> > +
> > +	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;
> > +}
> > +
> > +/*
> > + * 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
> > + * state in the cpu regs (if any) belongs to current, and where to write
> > + * it back to if/when a FPSIMD access trap is taken.
> > + *
> > + * TIF_SVE is backed up here, since it may get clobbered with guest state.
> > + * This flag is restored by kvm_arch_vcpu_put_fp(vcpu).
> 
> I don't fully understand how TIF_SVE gets clobbered, but perhaps that's
> in preparation for a future patch.  (Me keeps reading).

task_fpsimd_save() relies on that flag to tell it whether to save SVE
or FPSIMD state.  For now, we must not attempt to save SVE state for a
vcpu, since we have nowhere to store it, so TIF_SVE is cleared
explicitly in _park_fp() is vcpu->arch.fp_enabled got set while in the
guest (i.e., the guest's FPSIMD state got loaded).

In the future it may get set or not, depending on whether the guest is
using SVE.

Do you think the comment is adequate?  Perhaps I can reword it.

> > + */
> > +void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> > +{
> > +	BUG_ON(system_supports_sve());
> > +	BUG_ON(!current->mm);
> > +
> > +	vcpu->arch.fp_enabled = false;
> > +	vcpu->arch.host_fpsimd_state =
> > +		kern_hyp_va(&current->thread.uw.fpsimd_state);
> > +	vcpu->arch.host_sve_in_use = !!test_thread_flag(TIF_SVE);
> > +}
> > +
> > +/*
> > + * If the guest FPSIMD state was loaded, mark the CPU FPSIMD regs as
> > + * dirty for 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_park_fp(struct kvm_vcpu *vcpu)
> > +{
> > +	WARN_ON_ONCE(!irqs_disabled());
> > +
> > +	if (vcpu->arch.fp_enabled) {
> > +		fpsimd_bind_state_to_cpu(&vcpu->arch.ctxt.gp_regs.fp_regs);
> > +		clear_thread_flag(TIF_FOREIGN_FPSTATE);
> > +		clear_thread_flag(TIF_SVE);
> > +	}
> > +}
> > +
> > +/*
> > + * Write back the vcpu FPSIMD regs if they are dirty, and invalidate the
> > + * cpu FPSIMD regs so that they can't be spuriously reused if this vcpu
> > + * disappears and another task or vcpu appears that recycles the same
> > + * struct fpsimd_state.
> > + */
> > +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > +{
> > +	local_bh_disable();
> > +
> > +	if (vcpu->arch.fp_enabled) {
> > +		task_fpsimd_save();
> 
> Just to make sure I'm getting this right: This is safe, because even if
> after we exited the VM, before we call this function, if we take a
> softirq that has overwritten our fpsimd state, then kernel_neon_begin()
> will have set TIF_FOREIGN_FPSTATE, and task_fpsimd_save() won't actually
> do anything, and kernel_neon_begin() will have alreay saved the VCPU's
> fpsimd state.  Is that right?

Exactly.  The local_bh_disable() here is crucial to make sure that
kernel-mode NEON can't tread on our toes while we do the dirty work.

> > +		fpsimd_flush_cpu_state();
> > +		set_thread_flag(TIF_FOREIGN_FPSTATE);
> > +	}
> > +
> > +	if (vcpu->arch.host_sve_in_use)
> > +		set_thread_flag(TIF_SVE);
> > +	else
> > +		clear_thread_flag(TIF_SVE);
> > +
> > +	local_bh_enable();
> > +}
> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index c438ad8..0900c2a 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -27,15 +27,16 @@
> >  #include <asm/kvm_mmu.h>
> >  #include <asm/fpsimd.h>
> >  #include <asm/debug-monitors.h>
> > +#include <asm/thread_info.h>
> >  
> > -static bool __hyp_text __fpsimd_enabled_nvhe(void)
> > +static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
> >  {
> > -	return !(read_sysreg(cptr_el2) & CPTR_EL2_TFP);
> > -}
> > +	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE) {
> > +		vcpu->arch.host_fpsimd_state = NULL;
> > +		vcpu->arch.fp_enabled = false;
> > +	}
> >  
> > -static bool fpsimd_enabled_vhe(void)
> > -{
> > -	return !!(read_sysreg(cpacr_el1) & CPACR_EL1_FPEN);
> > +	return vcpu->arch.fp_enabled;
> >  }
> >  
> >  /* Save the 32-bit only FPSIMD system register state */
> > @@ -92,7 +93,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
> >  
> >  	val = read_sysreg(cpacr_el1);
> >  	val |= CPACR_EL1_TTA;
> > -	val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN);
> > +	val &= ~CPACR_EL1_ZEN;
> > +	if (!update_fp_enabled(vcpu))
> > +		val &= ~CPACR_EL1_FPEN;
> > +
> >  	write_sysreg(val, cpacr_el1);
> >  
> >  	write_sysreg(kvm_get_hyp_vector(), vbar_el1);
> > @@ -105,7 +109,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
> >  	__activate_traps_common(vcpu);
> >  
> >  	val = CPTR_EL2_DEFAULT;
> > -	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
> > +	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> > +	if (!update_fp_enabled(vcpu))
> > +		val |= CPTR_EL2_TFP;
> > +
> >  	write_sysreg(val, cptr_el2);
> >  }
> >  
> > @@ -394,7 +401,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm_cpu_context *host_ctxt;
> >  	struct kvm_cpu_context *guest_ctxt;
> > -	bool fp_enabled;
> >  	u64 exit_code;
> >  
> >  	host_ctxt = vcpu->arch.host_cpu_context;
> > @@ -416,19 +422,14 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> >  		/* And we're baaack! */
> >  	} while (fixup_guest_exit(vcpu, &exit_code));
> >  
> > -	fp_enabled = fpsimd_enabled_vhe();
> > -
> >  	sysreg_save_guest_state_vhe(guest_ctxt);
> >  
> >  	__deactivate_traps(vcpu);
> >  
> >  	sysreg_restore_host_state_vhe(host_ctxt);
> >  
> > -	if (fp_enabled) {
> > -		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > -		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > +	if (vcpu->arch.fp_enabled)
> >  		__fpsimd_save_fpexc32(vcpu);
> > -	}
> >  
> >  	__debug_switch_to_host(vcpu);
> >  
> > @@ -440,7 +441,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm_cpu_context *host_ctxt;
> >  	struct kvm_cpu_context *guest_ctxt;
> > -	bool fp_enabled;
> >  	u64 exit_code;
> >  
> >  	vcpu = kern_hyp_va(vcpu);
> > @@ -472,8 +472,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> >  		/* And we're baaack! */
> >  	} while (fixup_guest_exit(vcpu, &exit_code));
> >  
> > -	fp_enabled = __fpsimd_enabled_nvhe();
> > -
> >  	__sysreg_save_state_nvhe(guest_ctxt);
> >  	__sysreg32_save_state(vcpu);
> >  	__timer_disable_traps(vcpu);
> > @@ -484,11 +482,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> >  
> >  	__sysreg_restore_state_nvhe(host_ctxt);
> >  
> > -	if (fp_enabled) {
> > -		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > -		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > +	if (vcpu->arch.fp_enabled)
> >  		__fpsimd_save_fpexc32(vcpu);
> > -	}
> >  
> >  	/*
> >  	 * This must come after restoring the host sysregs, since a non-VHE
> > @@ -502,8 +497,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> >  void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> >  				    struct kvm_vcpu *vcpu)
> >  {
> > -	kvm_cpu_context_t *host_ctxt;
> > -
> >  	if (has_vhe())
> >  		write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
> >  			     cpacr_el1);
> > @@ -513,14 +506,19 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> >  
> >  	isb();
> >  
> > -	host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> > -	__fpsimd_save_state(&host_ctxt->gp_regs.fp_regs);
> > +	if (vcpu->arch.host_fpsimd_state) {
> > +		__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
> > +		vcpu->arch.host_fpsimd_state = NULL;
> > +	}
> > +
> >  	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
> >  
> >  	/* Skip restoring fpexc32 for AArch64 guests */
> >  	if (!(read_sysreg(hcr_el2) & HCR_RW))
> >  		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
> >  			     fpexc32_el2);
> > +
> > +	vcpu->arch.fp_enabled = true;
> >  }
> >  
> >  static const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index cd38d7d..4fcf6fe 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
> > @@ -363,10 +363,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >  	kvm_vgic_load(vcpu);
> >  	kvm_timer_vcpu_load(vcpu);
> >  	kvm_vcpu_load_sysregs(vcpu);
> > +	kvm_arch_vcpu_load_fp(vcpu);
> >  }
> >  
> >  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >  {
> > +	kvm_arch_vcpu_put_fp(vcpu);
> >  	kvm_vcpu_put_sysregs(vcpu);
> >  	kvm_timer_vcpu_put(vcpu);
> >  	kvm_vgic_put(vcpu);
> > @@ -773,6 +775,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  		if (static_branch_unlikely(&userspace_irqchip_in_use))
> >  			kvm_timer_sync_hwstate(vcpu);
> >  
> > +		kvm_arch_vcpu_park_fp(vcpu);
> > +
> >  		/*
> >  		 * We may have taken a host interrupt in HYP mode (ie
> >  		 * while executing the guest). This interrupt is still
> > @@ -816,16 +820,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  	return ret;
> >  }
> >  
> > -#ifdef CONFIG_ARM64
> > -int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> > -{
> > -	struct task_struct *tsk = current;
> > -
> > -	/* Make sure the host task fpsimd state is visible to hyp: */
> > -	return create_hyp_mappings(tsk, tsk + 1, PAGE_HYP);
> > -}
> > -#endif
> > -
> >  static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
> >  {
> >  	int bit_index;
> > -- 
> > 2.1.4
> > 
> Besides the cosmetic comments and questions above, this all looks good
> to me!

Thanks for the review.

I wanted to do a respin to clean up the series anyhow, so I'll try to
address your comments there:

Cheers
---Dave

[RFC PATCH 3/8] KVM: arm64: Optimise FPSIMD handling to reduce guest/host thrashing

[Christoffer Dall]

On Thu, Apr 26, 2018 at 03:11:49PM +0100, Dave Martin wrote:
> On Thu, Apr 26, 2018 at 01:21:22PM +0200, Christoffer Dall wrote:
> > On Fri, Apr 20, 2018 at 05:46:37PM +0100, Dave Martin wrote:
> > > This patch refactors KVM to align the host and guest FPSIMD
> > > save/restore logic with each other for arm64.  This reduces the
> > > number of redundant save/restore operations that must occur, and
> > > reduces the common-case IRQ blackout time during guest exit storms
> > > by saving the host state lazily and optimising away the need to
> > > restore the host state before returning to the run loop.
> > > 
> > > Four hooks are defined in order to enable this:
> > > 
> > >  * kvm_arch_vcpu_run_map_fp():
> > >    Called on PID change to map necessary bits of current to Hyp.
> > > 
> > >  * kvm_arch_vcpu_load_fp():
> > >    Set up FP/SIMD for entering the KVM run loop (parse as
> > >    "vcpu_load fp").
> > > 
> > >  * kvm_arch_vcpu_park_fp():
> > >    Get FP/SIMD into a safe state for re-enabling interrupts after a
> > >    guest exit back to the run loop.
> > > 
> > >    For arm64 specifically, this involves updating the host kernel's
> > >    FPSIMD context tracking metadata so that kernel-mode NEON use
> > >    will cause the vcpu's FPSIMD state to be saved back correctly
> > >    into the vcpu struct.  This must be done before re-enabling
> > >    interrupts because kernel-mode NEON may be used my softirqs.
> > > 
> > >  * kvm_arch_vcpu_put_fp():
> > >    Save guest FP/SIMD state back to memory and dissociate from the
> > >    CPU ("vcpu_put fp").
> > > 
> > > Also, the arm64 FPSIMD context switch code is updated to enable it
> > > to save back FPSIMD state for a vcpu, not just current.  A few
> > > helpers drive this:
> > > 
> > >  * fpsimd_bind_state_to_cpu(struct user_fpsimd_state *fp):
> > >    mark this CPU as having context fp (which may belong to a vcpu)
> > >    currently loaded in its registers.  This is the non-task
> > >    equivalent of the static function fpsimd_bind_to_cpu() in
> > >    fpsimd.c.
> > > 
> > >  * task_fpsimd_save():
> > >    exported to allow KVM to save the guest's FPSIMD state back to
> > >    memory on exit from the run loop.
> > > 
> > >  * fpsimd_flush_state():
> > >    invalidate any context's FPSIMD state that is currently loaded.
> > >    Used to disassociate the vcpu from the CPU regs on run loop exit.
> > > 
> > > These changes allow the run loop to enable interrupts (and thus
> > > softirqs that may use kernel-mode NEON) without having to save the
> > > guest's FPSIMD state eagerly.
> > > 
> > > Some new vcpu_arch fields are added to make all this work.  Because
> > > host FPSIMD state can now be saved back directly into current's
> > > thread_struct as appropriate, host_cpu_context is no longer used
> > > for preserving the FPSIMD state.  However, it is still needed for
> > > preserving other things such as the host's system registers.  To
> > > avoid ABI churn, the redundant storage space in host_cpu_context is
> > > not removed for now.
> > > 
> > > arch/arm is not addressed by this patch and continues to use its
> > > current save/restore logic.  It could provide implementations of
> > > the helpers later if desired.
> > > 
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > 
> > > ---
> > > 
> > > Changes since RFC v3:
> > > 
> > > Requested by Christoffer Dall:
> > > 
> > >  * Remove fpsimd_flush_state(), which nothing uses.
> > >    Inline it back into fpsimd_flush_task_state().
> > > 
> > >  * Add __hyp_text anootation to update_fp_enabled().
> > > 
> > > Other:
> > > 
> > >  * Remove fp_enabled().
> > > 
> > >    Now that we have an explicit field vcpu->arch.fp_enabled to
> > >    indicate whether the guest's FPSIMD state is loaded, we can use
> > >    this to determine when to save FPEXC_EL1 for 32-bit guests.
> > >    This brings the logic for FPEXC_EL1 save back into line with the
> > >    (updated) logic for restore of this register, and simplifies the
> > >    code a bit.
> > > 
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > >  arch/arm/include/asm/kvm_host.h   |   8 +++
> > >  arch/arm64/include/asm/fpsimd.h   |   5 ++
> > >  arch/arm64/include/asm/kvm_host.h |  18 +++++++
> > >  arch/arm64/kernel/fpsimd.c        |  24 ++++++---
> > >  arch/arm64/kvm/Makefile           |   2 +-
> > >  arch/arm64/kvm/fpsimd.c           | 109 ++++++++++++++++++++++++++++++++++++++
> > >  arch/arm64/kvm/hyp/switch.c       |  50 +++++++++--------
> > >  virt/kvm/arm/arm.c                |  14 ++---
> > >  8 files changed, 187 insertions(+), 43 deletions(-)
> > >  create mode 100644 arch/arm64/kvm/fpsimd.c
> > > 
> > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > index c6a7495..3fe01c7 100644
> > > --- a/arch/arm/include/asm/kvm_host.h
> > > +++ b/arch/arm/include/asm/kvm_host.h
> > > @@ -300,6 +300,14 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> > >  int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> > >  			       struct kvm_device_attr *attr);
> > >  
> > > +/*
> > > + * VFP/NEON switching is all done by the hyp switch code, so no need to
> > > + * coordinate with host context handling for this state:
> > > + */
> > > +static inline void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) {}
> > > +static inline void kvm_arch_vcpu_park_fp(struct kvm_vcpu *vcpu) {}
> > > +static inline void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) {}
> > > +
> > >  /* All host FP/SIMD state is restored on guest exit, so nothing to save: */
> > >  static inline void kvm_fpsimd_flush_cpu_state(void) {}
> > >  
> > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> > > index aa7162a..a1c5127 100644
> > > --- a/arch/arm64/include/asm/fpsimd.h
> > > +++ b/arch/arm64/include/asm/fpsimd.h
> > > @@ -41,6 +41,8 @@ struct task_struct;
> > >  extern void fpsimd_save_state(struct user_fpsimd_state *state);
> > >  extern void fpsimd_load_state(struct user_fpsimd_state *state);
> > >  
> > > +extern void task_fpsimd_save(void);
> > > +
> > >  extern void fpsimd_thread_switch(struct task_struct *next);
> > >  extern void fpsimd_flush_thread(void);
> > >  
> > > @@ -49,7 +51,10 @@ extern void fpsimd_preserve_current_state(void);
> > >  extern void fpsimd_restore_current_state(void);
> > >  extern void fpsimd_update_current_state(struct user_fpsimd_state const *state);
> > >  
> > > +extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state);
> > > +
> > >  extern void fpsimd_flush_task_state(struct task_struct *target);
> > > +extern void fpsimd_flush_cpu_state(void);
> > >  extern void sve_flush_cpu_state(void);
> > >  
> > >  /* Maximum VL that SVE VL-agnostic software can transparently support */
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index ab46bc7..2fbfbda 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -30,6 +30,7 @@
> > >  #include <asm/kvm.h>
> > >  #include <asm/kvm_asm.h>
> > >  #include <asm/kvm_mmio.h>
> > > +#include <asm/thread_info.h>
> > >  
> > >  #define __KVM_HAVE_ARCH_INTC_INITIALIZED
> > >  
> > > @@ -235,6 +236,12 @@ struct kvm_vcpu_arch {
> > >  
> > >  	/* Pointer to host CPU context */
> > >  	kvm_cpu_context_t *host_cpu_context;
> > > +
> > > +	struct thread_info *host_thread_info;	/* hyp VA */
> > > +	struct user_fpsimd_state *host_fpsimd_state;	/* hyp VA */
> > > +	bool host_sve_in_use;	/* backup for host TIF_SVE while in guest */
> > > +	bool fp_enabled;
> > > +
> > >  	struct {
> > >  		/* {Break,watch}point registers */
> > >  		struct kvm_guest_debug_arch regs;
> > > @@ -417,6 +424,17 @@ static inline void __cpu_init_stage2(void)
> > >  		  "PARange is %d bits, unsupported configuration!", parange);
> > >  }
> > >  
> > > +/* 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_park_fp(struct kvm_vcpu *vcpu);
> > > +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu);
> > > +
> > > +static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> > > +{
> > > +	return kvm_arch_vcpu_run_map_fp(vcpu);
> > > +}
> > > +
> > 
> > nit: It would be nicer to only add the arm64 hook the way they should be
> > in this patch, and remove the #ifdef CONFIG_ARM64 thing that gets
> > removed in this patch from patch 1.
> 
> Can do.  For now, I was keeping it clear what you write and what I
> changed, but if you're happy with the changes I can fold them together
> and add by S-o-B on top of yours.
> 

Yes, I'm happy for you to change the patch in whichever way you find
reasonable for it to fit nicely in this series.  I'll promise to read it
carefully as the series go by.

> > 
> > >  /*
> > >   * All host FP/SIMD state is restored on guest exit, so nothing needs
> > >   * doing here except in the SVE case:
> > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> > > index 87a3536..7f74374 100644
> > > --- a/arch/arm64/kernel/fpsimd.c
> > > +++ b/arch/arm64/kernel/fpsimd.c
> > > @@ -270,13 +270,15 @@ static void task_fpsimd_load(void)
> > >  }
> > >  
> > >  /*
> > > - * Ensure current's FPSIMD/SVE storage in thread_struct is up to date
> > > + * Ensure current's FPSIMD/SVE storage in memory is up to date
> > >   * with respect to the CPU registers.
> > >   *
> > >   * Softirqs (and preemption) must be disabled.
> > >   */
> > > -static void task_fpsimd_save(void)
> > > +void task_fpsimd_save(void)
> > >  {
> > > +	struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st);
> > > +
> > >  	WARN_ON(!in_softirq() && !irqs_disabled());
> > >  
> > >  	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> > > @@ -291,10 +293,9 @@ static void task_fpsimd_save(void)
> > >  				return;
> > >  			}
> > >  
> > > -			sve_save_state(sve_pffr(current),
> > > -				       &current->thread.uw.fpsimd_state.fpsr);
> > > +			sve_save_state(sve_pffr(current), &st->fpsr);
> > >  		} else
> > > -			fpsimd_save_state(&current->thread.uw.fpsimd_state);
> > > +			fpsimd_save_state(st);
> > 
> > Is this equivalent to what we had before, because we'll have run
> > something that sets this cpu's fpsimd_last_state to current's
> > user_fpsimd_state, when scheduling this thread?
> 
> It should be, yes -- at least outside the KVM run loop where
> fpsimd_last_state can point into the vcpu instead.
> 
> fpsimd_bind_to_cpu() does the setting of fpsimd_last_state.  This is
> called when loading current's FPSIMD regs via
> fpsimd_restore_current_state(), or just after exiting the guest if
> vcpu->arch.fp_enabled became set.
> 

I see, that makes sense, thanks.

> > 
> > >  	}
> > >  }
> > >  
> > > @@ -1012,6 +1013,17 @@ static void fpsimd_bind_to_cpu(void)
> > >  	current->thread.fpsimd_cpu = smp_processor_id();
> > >  }
> > >  
> > > +void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
> > > +{
> > > +	struct fpsimd_last_state_struct *last =
> > > +		this_cpu_ptr(&fpsimd_last_state);
> > > +
> > > +	WARN_ON(!in_softirq() && !irqs_disabled());
> > > +
> > > +	last->st = st;
> > > +	last->sve_in_use = false;
> > > +}
> > > +
> > >  /*
> > >   * Load the userland FPSIMD state of 'current' from memory, but only if the
> > >   * FPSIMD state already held in the registers is /not/ the most recent FPSIMD
> > > @@ -1064,7 +1076,7 @@ void fpsimd_flush_task_state(struct task_struct *t)
> > >  	t->thread.fpsimd_cpu = NR_CPUS;
> > >  }
> > >  
> > > -static inline void fpsimd_flush_cpu_state(void)
> > > +void fpsimd_flush_cpu_state(void)
> > >  {
> > >  	__this_cpu_write(fpsimd_last_state.st, NULL);
> > >  }
> > 
> > You could consider factoring out all changes to kernel/fpsimd.c into a
> > separate patch explaining why it's correct, which may make bisecting
> > potential bugs easier, but on the other hand it keeps the changes in the
> > patch where they're used.  Just a thought.
> 
> I'll have a think about that when I respin.  I think the fpsimd.c-only
> part of the patch would be self-evidently correct, so it could make
> sense to keep it separate.
> 
> 
> > 
> > > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> > > index 93afff9..0f2a135 100644
> > > --- a/arch/arm64/kvm/Makefile
> > > +++ b/arch/arm64/kvm/Makefile
> > > @@ -19,7 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
> > >  kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
> > >  kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
> > >  kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
> > > -kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o
> > > +kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o fpsimd.o
> > >  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/aarch32.o
> > >  
> > >  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
> > > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> > > new file mode 100644
> > > index 0000000..cc7a068
> > > --- /dev/null
> > > +++ b/arch/arm64/kvm/fpsimd.c
> > > @@ -0,0 +1,109 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * arch/arm64/kvm/fpsimd.c: Guest/host FPSIMD context coordination helpers
> > > + *
> > > + * Copyright 2018 Arm Limited
> > > + * Author: Dave Martin <Dave.Martin@arm.com>
> > > + */
> > > +#include <linux/bottom_half.h>
> > > +#include <linux/sched.h>
> > > +#include <linux/thread_info.h>
> > > +#include <linux/kvm_host.h>
> > > +#include <asm/kvm_host.h>
> > > +#include <asm/kvm_mmu.h>
> > > +
> > > +/*
> > > + * Called on entry to KVM_RUN unless this vcpu previously ran at least
> > > + * once and the most recent prior KVM_RUN for this vcpu was called from
> > > + * the same task as current (highly likely).
> > > + *
> > > + * This is guaranteed to execute before kvm_arch_vcpu_load_fp(vcpu),
> > > + * such that on entering hyp the relevant parts of current are already
> > > + * mapped.
> > > + */
> > > +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;
> > > +
> > > +	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;
> > > +}
> > > +
> > > +/*
> > > + * 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
> > > + * state in the cpu regs (if any) belongs to current, and where to write
> > > + * it back to if/when a FPSIMD access trap is taken.
> > > + *
> > > + * TIF_SVE is backed up here, since it may get clobbered with guest state.
> > > + * This flag is restored by kvm_arch_vcpu_put_fp(vcpu).
> > 
> > I don't fully understand how TIF_SVE gets clobbered, but perhaps that's
> > in preparation for a future patch.  (Me keeps reading).
> 
> task_fpsimd_save() relies on that flag to tell it whether to save SVE
> or FPSIMD state.  For now, we must not attempt to save SVE state for a
> vcpu, since we have nowhere to store it, so TIF_SVE is cleared
> explicitly in _park_fp() is vcpu->arch.fp_enabled got set while in the
> guest (i.e., the guest's FPSIMD state got loaded).

I missed that during my initial look.  Sorry.

> 
> In the future it may get set or not, depending on whether the guest is
> using SVE.
> 
> Do you think the comment is adequate?  Perhaps I can reword it.
> 

I think it's fine, I should just pay closer attention.

> > > + */
> > > +void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> > > +{
> > > +	BUG_ON(system_supports_sve());
> > > +	BUG_ON(!current->mm);
> > > +
> > > +	vcpu->arch.fp_enabled = false;
> > > +	vcpu->arch.host_fpsimd_state =
> > > +		kern_hyp_va(&current->thread.uw.fpsimd_state);
> > > +	vcpu->arch.host_sve_in_use = !!test_thread_flag(TIF_SVE);
> > > +}
> > > +
> > > +/*
> > > + * If the guest FPSIMD state was loaded, mark the CPU FPSIMD regs as
> > > + * dirty for 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_park_fp(struct kvm_vcpu *vcpu)
> > > +{
> > > +	WARN_ON_ONCE(!irqs_disabled());
> > > +
> > > +	if (vcpu->arch.fp_enabled) {
> > > +		fpsimd_bind_state_to_cpu(&vcpu->arch.ctxt.gp_regs.fp_regs);
> > > +		clear_thread_flag(TIF_FOREIGN_FPSTATE);
> > > +		clear_thread_flag(TIF_SVE);
> > > +	}
> > > +}
> > > +
> > > +/*
> > > + * Write back the vcpu FPSIMD regs if they are dirty, and invalidate the
> > > + * cpu FPSIMD regs so that they can't be spuriously reused if this vcpu
> > > + * disappears and another task or vcpu appears that recycles the same
> > > + * struct fpsimd_state.
> > > + */
> > > +void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> > > +{
> > > +	local_bh_disable();
> > > +
> > > +	if (vcpu->arch.fp_enabled) {
> > > +		task_fpsimd_save();
> > 
> > Just to make sure I'm getting this right: This is safe, because even if
> > after we exited the VM, before we call this function, if we take a
> > softirq that has overwritten our fpsimd state, then kernel_neon_begin()
> > will have set TIF_FOREIGN_FPSTATE, and task_fpsimd_save() won't actually
> > do anything, and kernel_neon_begin() will have alreay saved the VCPU's
> > fpsimd state.  Is that right?
> 
> Exactly.  The local_bh_disable() here is crucial to make sure that
> kernel-mode NEON can't tread on our toes while we do the dirty work.
> 
> > > +		fpsimd_flush_cpu_state();
> > > +		set_thread_flag(TIF_FOREIGN_FPSTATE);
> > > +	}
> > > +
> > > +	if (vcpu->arch.host_sve_in_use)
> > > +		set_thread_flag(TIF_SVE);
> > > +	else
> > > +		clear_thread_flag(TIF_SVE);
> > > +
> > > +	local_bh_enable();
> > > +}
> > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > > index c438ad8..0900c2a 100644
> > > --- a/arch/arm64/kvm/hyp/switch.c
> > > +++ b/arch/arm64/kvm/hyp/switch.c
> > > @@ -27,15 +27,16 @@
> > >  #include <asm/kvm_mmu.h>
> > >  #include <asm/fpsimd.h>
> > >  #include <asm/debug-monitors.h>
> > > +#include <asm/thread_info.h>
> > >  
> > > -static bool __hyp_text __fpsimd_enabled_nvhe(void)
> > > +static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
> > >  {
> > > -	return !(read_sysreg(cptr_el2) & CPTR_EL2_TFP);
> > > -}
> > > +	if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE) {
> > > +		vcpu->arch.host_fpsimd_state = NULL;
> > > +		vcpu->arch.fp_enabled = false;
> > > +	}
> > >  
> > > -static bool fpsimd_enabled_vhe(void)
> > > -{
> > > -	return !!(read_sysreg(cpacr_el1) & CPACR_EL1_FPEN);
> > > +	return vcpu->arch.fp_enabled;
> > >  }
> > >  
> > >  /* Save the 32-bit only FPSIMD system register state */
> > > @@ -92,7 +93,10 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
> > >  
> > >  	val = read_sysreg(cpacr_el1);
> > >  	val |= CPACR_EL1_TTA;
> > > -	val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN);
> > > +	val &= ~CPACR_EL1_ZEN;
> > > +	if (!update_fp_enabled(vcpu))
> > > +		val &= ~CPACR_EL1_FPEN;
> > > +
> > >  	write_sysreg(val, cpacr_el1);
> > >  
> > >  	write_sysreg(kvm_get_hyp_vector(), vbar_el1);
> > > @@ -105,7 +109,10 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
> > >  	__activate_traps_common(vcpu);
> > >  
> > >  	val = CPTR_EL2_DEFAULT;
> > > -	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
> > > +	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> > > +	if (!update_fp_enabled(vcpu))
> > > +		val |= CPTR_EL2_TFP;
> > > +
> > >  	write_sysreg(val, cptr_el2);
> > >  }
> > >  
> > > @@ -394,7 +401,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> > >  {
> > >  	struct kvm_cpu_context *host_ctxt;
> > >  	struct kvm_cpu_context *guest_ctxt;
> > > -	bool fp_enabled;
> > >  	u64 exit_code;
> > >  
> > >  	host_ctxt = vcpu->arch.host_cpu_context;
> > > @@ -416,19 +422,14 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> > >  		/* And we're baaack! */
> > >  	} while (fixup_guest_exit(vcpu, &exit_code));
> > >  
> > > -	fp_enabled = fpsimd_enabled_vhe();
> > > -
> > >  	sysreg_save_guest_state_vhe(guest_ctxt);
> > >  
> > >  	__deactivate_traps(vcpu);
> > >  
> > >  	sysreg_restore_host_state_vhe(host_ctxt);
> > >  
> > > -	if (fp_enabled) {
> > > -		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > > -		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > > +	if (vcpu->arch.fp_enabled)
> > >  		__fpsimd_save_fpexc32(vcpu);
> > > -	}
> > >  
> > >  	__debug_switch_to_host(vcpu);
> > >  
> > > @@ -440,7 +441,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> > >  {
> > >  	struct kvm_cpu_context *host_ctxt;
> > >  	struct kvm_cpu_context *guest_ctxt;
> > > -	bool fp_enabled;
> > >  	u64 exit_code;
> > >  
> > >  	vcpu = kern_hyp_va(vcpu);
> > > @@ -472,8 +472,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> > >  		/* And we're baaack! */
> > >  	} while (fixup_guest_exit(vcpu, &exit_code));
> > >  
> > > -	fp_enabled = __fpsimd_enabled_nvhe();
> > > -
> > >  	__sysreg_save_state_nvhe(guest_ctxt);
> > >  	__sysreg32_save_state(vcpu);
> > >  	__timer_disable_traps(vcpu);
> > > @@ -484,11 +482,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> > >  
> > >  	__sysreg_restore_state_nvhe(host_ctxt);
> > >  
> > > -	if (fp_enabled) {
> > > -		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> > > -		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > > +	if (vcpu->arch.fp_enabled)
> > >  		__fpsimd_save_fpexc32(vcpu);
> > > -	}
> > >  
> > >  	/*
> > >  	 * This must come after restoring the host sysregs, since a non-VHE
> > > @@ -502,8 +497,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> > >  void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> > >  				    struct kvm_vcpu *vcpu)
> > >  {
> > > -	kvm_cpu_context_t *host_ctxt;
> > > -
> > >  	if (has_vhe())
> > >  		write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
> > >  			     cpacr_el1);
> > > @@ -513,14 +506,19 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> > >  
> > >  	isb();
> > >  
> > > -	host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> > > -	__fpsimd_save_state(&host_ctxt->gp_regs.fp_regs);
> > > +	if (vcpu->arch.host_fpsimd_state) {
> > > +		__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
> > > +		vcpu->arch.host_fpsimd_state = NULL;
> > > +	}
> > > +
> > >  	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
> > >  
> > >  	/* Skip restoring fpexc32 for AArch64 guests */
> > >  	if (!(read_sysreg(hcr_el2) & HCR_RW))
> > >  		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
> > >  			     fpexc32_el2);
> > > +
> > > +	vcpu->arch.fp_enabled = true;
> > >  }
> > >  
> > >  static const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
> > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > index cd38d7d..4fcf6fe 100644
> > > --- a/virt/kvm/arm/arm.c
> > > +++ b/virt/kvm/arm/arm.c
> > > @@ -363,10 +363,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > >  	kvm_vgic_load(vcpu);
> > >  	kvm_timer_vcpu_load(vcpu);
> > >  	kvm_vcpu_load_sysregs(vcpu);
> > > +	kvm_arch_vcpu_load_fp(vcpu);
> > >  }
> > >  
> > >  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> > >  {
> > > +	kvm_arch_vcpu_put_fp(vcpu);
> > >  	kvm_vcpu_put_sysregs(vcpu);
> > >  	kvm_timer_vcpu_put(vcpu);
> > >  	kvm_vgic_put(vcpu);
> > > @@ -773,6 +775,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > >  		if (static_branch_unlikely(&userspace_irqchip_in_use))
> > >  			kvm_timer_sync_hwstate(vcpu);
> > >  
> > > +		kvm_arch_vcpu_park_fp(vcpu);
> > > +
> > >  		/*
> > >  		 * We may have taken a host interrupt in HYP mode (ie
> > >  		 * while executing the guest). This interrupt is still
> > > @@ -816,16 +820,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > >  	return ret;
> > >  }
> > >  
> > > -#ifdef CONFIG_ARM64
> > > -int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
> > > -{
> > > -	struct task_struct *tsk = current;
> > > -
> > > -	/* Make sure the host task fpsimd state is visible to hyp: */
> > > -	return create_hyp_mappings(tsk, tsk + 1, PAGE_HYP);
> > > -}
> > > -#endif
> > > -
> > >  static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
> > >  {
> > >  	int bit_index;
> > > -- 
> > > 2.1.4
> > > 
> > Besides the cosmetic comments and questions above, this all looks good
> > to me!
> 
> Thanks for the review.
> 
> I wanted to do a respin to clean up the series anyhow, so I'll try to
> address your comments there:
> 

Thanks,
-Christoffer

[RFC PATCH 4/8] arm64/sve: Move read_zcr_features() out of cpufeature.h

[Dave Martin]

Having read_zcr_features() inline in cpufeature.h results in that
header requiring #includes which make it hard to include
<asm/fpsimd.h> elsewhere without triggering header inclusion
cycles.

This is not a hot-path function and arguably should not be in
cpufeature.h in the first place, so this patch moves it to
fpsimd.c, compiled conditionally if CONFIG_ARM64_SVE=y.

This allows some SVE-related #includes to be dropped from
cpufeature.h, which will ease future maintenance.

A couple of missing #includes of <asm/fpsimd.h> are exposed by this
change under arch/arm64/.  This patch adds the missing #includes as
necessary.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/cpufeature.h | 29 -----------------------------
 arch/arm64/include/asm/fpsimd.h     |  2 ++
 arch/arm64/include/asm/processor.h  |  1 +
 arch/arm64/kernel/fpsimd.c          | 28 ++++++++++++++++++++++++++++
 arch/arm64/kernel/ptrace.c          |  1 +
 5 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 09b0f2a..0a6b713 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -11,9 +11,7 @@
 
 #include <asm/cpucaps.h>
 #include <asm/cputype.h>
-#include <asm/fpsimd.h>
 #include <asm/hwcap.h>
-#include <asm/sigcontext.h>
 #include <asm/sysreg.h>
 
 /*
@@ -510,33 +508,6 @@ static inline bool system_supports_sve(void)
 		cpus_have_const_cap(ARM64_SVE);
 }
 
-/*
- * Read the pseudo-ZCR used by cpufeatures to identify the supported SVE
- * vector length.
- *
- * Use only if SVE is present.
- * This function clobbers the SVE vector length.
- */
-static inline u64 read_zcr_features(void)
-{
-	u64 zcr;
-	unsigned int vq_max;
-
-	/*
-	 * Set the maximum possible VL, and write zeroes to all other
-	 * bits to see if they stick.
-	 */
-	sve_kernel_enable(NULL);
-	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL1);
-
-	zcr = read_sysreg_s(SYS_ZCR_EL1);
-	zcr &= ~(u64)ZCR_ELx_LEN_MASK; /* find sticky 1s outside LEN field */
-	vq_max = sve_vq_from_vl(sve_get_vl());
-	zcr |= vq_max - 1; /* set LEN field to maximum effective value */
-
-	return zcr;
-}
-
 #endif /* __ASSEMBLY__ */
 
 #endif
diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index a1c5127..995d597 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -68,6 +68,8 @@ extern unsigned int sve_get_vl(void);
 struct arm64_cpu_capabilities;
 extern void sve_kernel_enable(const struct arm64_cpu_capabilities *__unused);
 
+extern u64 read_zcr_features(void);
+
 extern int __ro_after_init sve_max_vl;
 
 #ifdef CONFIG_ARM64_SVE
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 7675989..f902b6d 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -40,6 +40,7 @@
 
 #include <asm/alternative.h>
 #include <asm/cpufeature.h>
+#include <asm/fpsimd.h>
 #include <asm/hw_breakpoint.h>
 #include <asm/lse.h>
 #include <asm/pgtable-hwdef.h>
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 7f74374..7fe3c1e 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -37,6 +37,7 @@
 #include <linux/sched/task_stack.h>
 #include <linux/signal.h>
 #include <linux/slab.h>
+#include <linux/stddef.h>
 #include <linux/sysctl.h>
 
 #include <asm/esr.h>
@@ -766,6 +767,33 @@ void sve_kernel_enable(const struct arm64_cpu_capabilities *__always_unused p)
 	isb();
 }
 
+/*
+ * Read the pseudo-ZCR used by cpufeatures to identify the supported SVE
+ * vector length.
+ *
+ * Use only if SVE is present.
+ * This function clobbers the SVE vector length.
+ */
+u64 read_zcr_features(void)
+{
+	u64 zcr;
+	unsigned int vq_max;
+
+	/*
+	 * Set the maximum possible VL, and write zeroes to all other
+	 * bits to see if they stick.
+	 */
+	sve_kernel_enable(NULL);
+	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL1);
+
+	zcr = read_sysreg_s(SYS_ZCR_EL1);
+	zcr &= ~(u64)ZCR_ELx_LEN_MASK; /* find sticky 1s outside LEN field */
+	vq_max = sve_vq_from_vl(sve_get_vl());
+	zcr |= vq_max - 1; /* set LEN field to maximum effective value */
+
+	return zcr;
+}
+
 void __init sve_setup(void)
 {
 	u64 zcr;
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 71d99af..e1bdc86 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -43,6 +43,7 @@
 #include <asm/compat.h>
 #include <asm/cpufeature.h>
 #include <asm/debug-monitors.h>
+#include <asm/fpsimd.h>
 #include <asm/pgtable.h>
 #include <asm/stacktrace.h>
 #include <asm/syscall.h>
-- 
2.1.4

[RFC PATCH 4/8] arm64/sve: Move read_zcr_features() out of cpufeature.h

[Catalin Marinas]

On Fri, Apr 20, 2018 at 05:46:38PM +0100, Dave P Martin wrote:
> Having read_zcr_features() inline in cpufeature.h results in that
> header requiring #includes which make it hard to include
> <asm/fpsimd.h> elsewhere without triggering header inclusion
> cycles.
> 
> This is not a hot-path function and arguably should not be in
> cpufeature.h in the first place, so this patch moves it to
> fpsimd.c, compiled conditionally if CONFIG_ARM64_SVE=y.
> 
> This allows some SVE-related #includes to be dropped from
> cpufeature.h, which will ease future maintenance.
> 
> A couple of missing #includes of <asm/fpsimd.h> are exposed by this
> change under arch/arm64/.  This patch adds the missing #includes as
> necessary.
> 
> No functional change.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

I guess when you eventually drop the RFC, these patches would be merged
through the KVM tree. For the core arm64 parts:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

The KVM changes also look fine to me but I'm not a KVM expert.

-- 
Catalin

[RFC PATCH 5/8] arm64/sve: Switch sve_pffr() argument from task to thread

[Dave Martin]

sve_pffr(), which is used to derive the base address used for
low-level SVE save/restore routines, currently takes the relevant
task_struct as an argument.

The only accessed fields are actually part of thread_struct, so
this patch changes the argument type accordingly.  This is done in
preparation for moving this function to a header, where we do not
want to have to include <linux/sched.h> due to the consequent
circular #include problems.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/fpsimd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 7fe3c1e..168a51e 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -44,6 +44,7 @@
 #include <asm/fpsimd.h>
 #include <asm/cpufeature.h>
 #include <asm/cputype.h>
+#include <asm/processor.h>
 #include <asm/simd.h>
 #include <asm/sigcontext.h>
 #include <asm/sysreg.h>
@@ -167,10 +168,9 @@ static size_t sve_ffr_offset(int vl)
 	return SVE_SIG_FFR_OFFSET(sve_vq_from_vl(vl)) - SVE_SIG_REGS_OFFSET;
 }
 
-static void *sve_pffr(struct task_struct *task)
+static void *sve_pffr(struct thread_struct *thread)
 {
-	return (char *)task->thread.sve_state +
-		sve_ffr_offset(task->thread.sve_vl);
+	return (char *)thread->sve_state + sve_ffr_offset(thread->sve_vl);
 }
 
 static void change_cpacr(u64 val, u64 mask)
@@ -253,7 +253,7 @@ static void task_fpsimd_load(void)
 	WARN_ON(!in_softirq() && !irqs_disabled());
 
 	if (system_supports_sve() && test_thread_flag(TIF_SVE))
-		sve_load_state(sve_pffr(current),
+		sve_load_state(sve_pffr(&current->thread),
 			       &current->thread.uw.fpsimd_state.fpsr,
 			       sve_vq_from_vl(current->thread.sve_vl) - 1);
 	else
@@ -294,7 +294,7 @@ void task_fpsimd_save(void)
 				return;
 			}
 
-			sve_save_state(sve_pffr(current), &st->fpsr);
+			sve_save_state(sve_pffr(&current->thread), &st->fpsr);
 		} else
 			fpsimd_save_state(st);
 	}
-- 
2.1.4

[RFC PATCH 5/8] arm64/sve: Switch sve_pffr() argument from task to thread

[Catalin Marinas]

On Fri, Apr 20, 2018 at 05:46:39PM +0100, Dave P Martin wrote:
> sve_pffr(), which is used to derive the base address used for
> low-level SVE save/restore routines, currently takes the relevant
> task_struct as an argument.
> 
> The only accessed fields are actually part of thread_struct, so
> this patch changes the argument type accordingly.  This is done in
> preparation for moving this function to a header, where we do not
> want to have to include <linux/sched.h> due to the consequent
> circular #include problems.
> 
> No functional change.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[RFC PATCH 6/8] arm64/sve: Move sve_pffr() to fpsimd.h and make inline

[Dave Martin]

In order to make sve_save_state()/sve_load_state() more easily
reusable and to get rid of a potential branch on context switch
critical paths, this patch makes sve_pffr() inline and moves it to
fpsimd.h.

<asm/processor.h> must be included in fpsimd.h in order to make
this work, and this creates an #include cycle that is tricky to
avoid without modifying core code, due to the way the PR_SVE_*()
prctl helpers are included in the core prctl implementation.

Instead of breaking the cycle, this patch defers inclusion of
<asm/fpsimd.h> in <asm/processor.h> until the point where it is
actually needed: i.e., immediately before the prctl definitions.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/fpsimd.h    | 13 +++++++++++++
 arch/arm64/include/asm/processor.h |  3 ++-
 arch/arm64/kernel/fpsimd.c         | 12 ------------
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 995d597..b84c4b5 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -18,6 +18,8 @@
 
 #include <asm/ptrace.h>
 #include <asm/errno.h>
+#include <asm/processor.h>
+#include <asm/sigcontext.h>
 
 #ifndef __ASSEMBLY__
 
@@ -60,6 +62,17 @@ extern void sve_flush_cpu_state(void);
 /* Maximum VL that SVE VL-agnostic software can transparently support */
 #define SVE_VL_ARCH_MAX 0x100
 
+/* Offset of FFR in the SVE register dump */
+static inline size_t sve_ffr_offset(int vl)
+{
+	return SVE_SIG_FFR_OFFSET(sve_vq_from_vl(vl)) - SVE_SIG_REGS_OFFSET;
+}
+
+static inline void *sve_pffr(struct thread_struct *thread)
+{
+	return (char *)thread->sve_state + sve_ffr_offset(thread->sve_vl);
+}
+
 extern void sve_save_state(void *state, u32 *pfpsr);
 extern void sve_load_state(void const *state, u32 const *pfpsr,
 			   unsigned long vq_minus_1);
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index f902b6d..ebaadb1 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -40,7 +40,6 @@
 
 #include <asm/alternative.h>
 #include <asm/cpufeature.h>
-#include <asm/fpsimd.h>
 #include <asm/hw_breakpoint.h>
 #include <asm/lse.h>
 #include <asm/pgtable-hwdef.h>
@@ -245,6 +244,8 @@ void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused);
 void cpu_enable_cache_maint_trap(const struct arm64_cpu_capabilities *__unused);
 void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused);
 
+#include <asm/fpsimd.h>
+
 /* Userspace interface for PR_SVE_{SET,GET}_VL prctl()s: */
 #define SVE_SET_VL(arg)	sve_set_current_vl(arg)
 #define SVE_GET_VL()	sve_get_current_vl()
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 168a51e..fa4f6e2 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -161,18 +161,6 @@ static void sve_free(struct task_struct *task)
 	__sve_free(task);
 }
 
-
-/* Offset of FFR in the SVE register dump */
-static size_t sve_ffr_offset(int vl)
-{
-	return SVE_SIG_FFR_OFFSET(sve_vq_from_vl(vl)) - SVE_SIG_REGS_OFFSET;
-}
-
-static void *sve_pffr(struct thread_struct *thread)
-{
-	return (char *)thread->sve_state + sve_ffr_offset(thread->sve_vl);
-}
-
 static void change_cpacr(u64 val, u64 mask)
 {
 	u64 cpacr = read_sysreg(CPACR_EL1);
-- 
2.1.4

[RFC PATCH 6/8] arm64/sve: Move sve_pffr() to fpsimd.h and make inline

[Catalin Marinas]

On Fri, Apr 20, 2018 at 05:46:40PM +0100, Dave P Martin wrote:
> In order to make sve_save_state()/sve_load_state() more easily
> reusable and to get rid of a potential branch on context switch
> critical paths, this patch makes sve_pffr() inline and moves it to
> fpsimd.h.
> 
> <asm/processor.h> must be included in fpsimd.h in order to make
> this work, and this creates an #include cycle that is tricky to
> avoid without modifying core code, due to the way the PR_SVE_*()
> prctl helpers are included in the core prctl implementation.
> 
> Instead of breaking the cycle, this patch defers inclusion of
> <asm/fpsimd.h> in <asm/processor.h> until the point where it is
> actually needed: i.e., immediately before the prctl definitions.
> 
> No functional change.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[RFC PATCH 7/8] KVM: arm64: Save host SVE context as appropriate

[Dave Martin]

This patch adds SVE context saving to the hyp FPSIMD context switch
path.  This means that it is no longer necessary to save the host
SVE state in advance of entering the guest, when in use.

In order to avoid adding pointless complexity to the code, VHE is
assumed if SVE is in use.  VHE is an architectural prerequisite for
SVE, so there is no good reason to turn CONFIG_ARM64_VHE off in
kernels that support both SVE and KVM.

Historically, software models exist that can expose the
architecturally invalid configuration of SVE without VHE, so if
this situation is detected this patch warns and refuses to create a
VM.  Doing this check at VM creation time avoids race issues
between KVM and SVE initialisation.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/Kconfig          |  7 +++++++
 arch/arm64/kvm/fpsimd.c     |  1 -
 arch/arm64/kvm/hyp/switch.c | 21 +++++++++++++++++++--
 virt/kvm/arm/arm.c          | 18 ++++++++++++++++++
 4 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index eb2cf49..b0d3820 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1130,6 +1130,7 @@ endmenu
 config ARM64_SVE
 	bool "ARM Scalable Vector Extension support"
 	default y
+	depends on !KVM || ARM64_VHE
 	help
 	  The Scalable Vector Extension (SVE) is an extension to the AArch64
 	  execution state which complements and extends the SIMD functionality
@@ -1155,6 +1156,12 @@ config ARM64_SVE
 	  booting the kernel.  If unsure and you are not observing these
 	  symptoms, you should assume that it is safe to say Y.
 
+	  CPUs that support SVE are architecturally required to support the
+	  Virtualization Host Extensions (VHE), so the kernel makes no
+	  provision for supporting SVE alongside KVM without VHE enabled.
+	  Thus, you will need to enable CONFIG_ARM64_VHE if you want to support
+	  KVM in the same kernel image.
+
 config ARM64_MODULE_PLTS
 	bool
 	select HAVE_MOD_ARCH_SPECIFIC
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index cc7a068..c166954 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -59,7 +59,6 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
  */
 void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
 {
-	BUG_ON(system_supports_sve());
 	BUG_ON(!current->mm);
 
 	vcpu->arch.fp_enabled = false;
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 0900c2a..959d634 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -21,12 +21,14 @@
 
 #include <kvm/arm_psci.h>
 
+#include <asm/cpufeature.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_hyp.h>
 #include <asm/kvm_mmu.h>
 #include <asm/fpsimd.h>
 #include <asm/debug-monitors.h>
+#include <asm/processor.h>
 #include <asm/thread_info.h>
 
 static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
@@ -497,6 +499,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
 				    struct kvm_vcpu *vcpu)
 {
+	struct user_fpsimd_state *host_fpsimd = vcpu->arch.host_fpsimd_state;
+
 	if (has_vhe())
 		write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
 			     cpacr_el1);
@@ -506,8 +510,21 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
 
 	isb();
 
-	if (vcpu->arch.host_fpsimd_state) {
-		__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
+	if (host_fpsimd) {
+		/*
+		 * In the SVE case, VHE is assumed: it is enforced by
+		 * Kconfig and kvm_arch_init_vm().
+		 */
+		if (system_supports_sve() && vcpu->arch.host_sve_in_use) {
+			struct thread_struct *thread = container_of(
+				host_fpsimd,
+				struct thread_struct, uw.fpsimd_state);
+
+			sve_save_state(sve_pffr(thread), &host_fpsimd->fpsr);
+		} else {
+			__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
+		}
+
 		vcpu->arch.host_fpsimd_state = NULL;
 	}
 
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 4fcf6fe..147d9d9 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -16,6 +16,7 @@
  * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
+#include <linux/bug.h>
 #include <linux/cpu_pm.h>
 #include <linux/errno.h>
 #include <linux/err.h>
@@ -41,6 +42,7 @@
 #include <asm/mman.h>
 #include <asm/tlbflush.h>
 #include <asm/cacheflush.h>
+#include <asm/cpufeature.h>
 #include <asm/virt.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_asm.h>
@@ -120,6 +122,22 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	if (type)
 		return -EINVAL;
 
+	/*
+	 * VHE is a prerequisite for SVE in the Arm architecture, and
+	 * Kconfig ensures that if system_supports_sve() here then
+	 * CONFIG_ARM64_VHE is enabled, so if VHE support wasn't already
+	 * detected and enabled, the CPU is architecturally
+	 * noncompliant.
+	 *
+	 * Just in case this mismatch is seen, detect it, warn and give
+	 * up.  Supporting this forbidden configuration in Hyp would be
+	 * pointless.
+	 */
+	if (system_supports_sve() && !has_vhe()) {
+		kvm_pr_unimpl("Cannot create VMs on SVE system without VHE.  Broken cpu?");
+		return -ENXIO;
+	}
+
 	kvm->arch.last_vcpu_ran = alloc_percpu(typeof(*kvm->arch.last_vcpu_ran));
 	if (!kvm->arch.last_vcpu_ran)
 		return -ENOMEM;
-- 
2.1.4

[RFC PATCH 7/8] KVM: arm64: Save host SVE context as appropriate

[Christoffer Dall]

On Fri, Apr 20, 2018 at 05:46:41PM +0100, Dave Martin wrote:
> This patch adds SVE context saving to the hyp FPSIMD context switch
> path.  This means that it is no longer necessary to save the host
> SVE state in advance of entering the guest, when in use.
> 
> In order to avoid adding pointless complexity to the code, VHE is
> assumed if SVE is in use.  VHE is an architectural prerequisite for
> SVE, so there is no good reason to turn CONFIG_ARM64_VHE off in
> kernels that support both SVE and KVM.
> 
> Historically, software models exist that can expose the
> architecturally invalid configuration of SVE without VHE, so if
> this situation is detected this patch warns and refuses to create a
> VM.  Doing this check at VM creation time avoids race issues
> between KVM and SVE initialisation.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/Kconfig          |  7 +++++++
>  arch/arm64/kvm/fpsimd.c     |  1 -
>  arch/arm64/kvm/hyp/switch.c | 21 +++++++++++++++++++--
>  virt/kvm/arm/arm.c          | 18 ++++++++++++++++++
>  4 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index eb2cf49..b0d3820 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1130,6 +1130,7 @@ endmenu
>  config ARM64_SVE
>  	bool "ARM Scalable Vector Extension support"
>  	default y
> +	depends on !KVM || ARM64_VHE
>  	help
>  	  The Scalable Vector Extension (SVE) is an extension to the AArch64
>  	  execution state which complements and extends the SIMD functionality
> @@ -1155,6 +1156,12 @@ config ARM64_SVE
>  	  booting the kernel.  If unsure and you are not observing these
>  	  symptoms, you should assume that it is safe to say Y.
>  
> +	  CPUs that support SVE are architecturally required to support the
> +	  Virtualization Host Extensions (VHE), so the kernel makes no
> +	  provision for supporting SVE alongside KVM without VHE enabled.
> +	  Thus, you will need to enable CONFIG_ARM64_VHE if you want to support
> +	  KVM in the same kernel image.
> +
>  config ARM64_MODULE_PLTS
>  	bool
>  	select HAVE_MOD_ARCH_SPECIFIC
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index cc7a068..c166954 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -59,7 +59,6 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
>   */
>  void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
>  {
> -	BUG_ON(system_supports_sve());
>  	BUG_ON(!current->mm);
>  
>  	vcpu->arch.fp_enabled = false;
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 0900c2a..959d634 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -21,12 +21,14 @@
>  
>  #include <kvm/arm_psci.h>
>  
> +#include <asm/cpufeature.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_hyp.h>
>  #include <asm/kvm_mmu.h>
>  #include <asm/fpsimd.h>
>  #include <asm/debug-monitors.h>
> +#include <asm/processor.h>
>  #include <asm/thread_info.h>
>  
>  static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
> @@ -497,6 +499,8 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
>  				    struct kvm_vcpu *vcpu)
>  {
> +	struct user_fpsimd_state *host_fpsimd = vcpu->arch.host_fpsimd_state;
> +
>  	if (has_vhe())
>  		write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
>  			     cpacr_el1);
> @@ -506,8 +510,21 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
>  
>  	isb();
>  
> -	if (vcpu->arch.host_fpsimd_state) {
> -		__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
> +	if (host_fpsimd) {
> +		/*
> +		 * In the SVE case, VHE is assumed: it is enforced by
> +		 * Kconfig and kvm_arch_init_vm().
> +		 */
> +		if (system_supports_sve() && vcpu->arch.host_sve_in_use) {
> +			struct thread_struct *thread = container_of(
> +				host_fpsimd,
> +				struct thread_struct, uw.fpsimd_state);
> +
> +			sve_save_state(sve_pffr(thread), &host_fpsimd->fpsr);
> +		} else {
> +			__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
> +		}
> +
>  		vcpu->arch.host_fpsimd_state = NULL;
>  	}
>  
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 4fcf6fe..147d9d9 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -16,6 +16,7 @@
>   * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
>   */
>  
> +#include <linux/bug.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/errno.h>
>  #include <linux/err.h>
> @@ -41,6 +42,7 @@
>  #include <asm/mman.h>
>  #include <asm/tlbflush.h>
>  #include <asm/cacheflush.h>
> +#include <asm/cpufeature.h>
>  #include <asm/virt.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_asm.h>
> @@ -120,6 +122,22 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	if (type)
>  		return -EINVAL;
>  
> +	/*
> +	 * VHE is a prerequisite for SVE in the Arm architecture, and
> +	 * Kconfig ensures that if system_supports_sve() here then
> +	 * CONFIG_ARM64_VHE is enabled, so if VHE support wasn't already
> +	 * detected and enabled, the CPU is architecturally
> +	 * noncompliant.
> +	 *
> +	 * Just in case this mismatch is seen, detect it, warn and give
> +	 * up.  Supporting this forbidden configuration in Hyp would be
> +	 * pointless.
> +	 */
> +	if (system_supports_sve() && !has_vhe()) {
> +		kvm_pr_unimpl("Cannot create VMs on SVE system without VHE.  Broken cpu?");
> +		return -ENXIO;
> +	}
> +
>  	kvm->arch.last_vcpu_ran = alloc_percpu(typeof(*kvm->arch.last_vcpu_ran));
>  	if (!kvm->arch.last_vcpu_ran)
>  		return -ENOMEM;
> -- 
> 2.1.4
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

[RFC PATCH 8/8] KVM: arm64: Remove eager host SVE state saving

[Dave Martin]

Now that the host SVE context can be saved on demand from Hyp,
there is no longer any need to save this state in advance before
entering the guest.

This patch removes the relevant call to
kvm_fpsimd_flush_cpu_state().

Since the problem that function was intended to solve now no longer
exists, the function and its dependencies are also deleted.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/include/asm/kvm_host.h   |  3 ---
 arch/arm64/include/asm/kvm_host.h | 10 ----------
 arch/arm64/kernel/fpsimd.c        | 21 ---------------------
 virt/kvm/arm/arm.c                |  3 ---
 4 files changed, 37 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 3fe01c7..cf268cb 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -308,9 +308,6 @@ static inline void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_vcpu_park_fp(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) {}
 
-/* All host FP/SIMD state is restored on guest exit, so nothing to save: */
-static inline void kvm_fpsimd_flush_cpu_state(void) {}
-
 static inline void kvm_arm_vhe_guest_enter(void) {}
 static inline void kvm_arm_vhe_guest_exit(void) {}
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2fbfbda..df52254 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -435,16 +435,6 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 	return kvm_arch_vcpu_run_map_fp(vcpu);
 }
 
-/*
- * All host FP/SIMD state is restored on guest exit, so nothing needs
- * doing here except in the SVE case:
-*/
-static inline void kvm_fpsimd_flush_cpu_state(void)
-{
-	if (system_supports_sve())
-		sve_flush_cpu_state();
-}
-
 static inline void kvm_arm_vhe_guest_enter(void)
 {
 	local_daif_mask();
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index fa4f6e2..cdfbcf7 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -120,7 +120,6 @@
  */
 struct fpsimd_last_state_struct {
 	struct user_fpsimd_state *st;
-	bool sve_in_use;
 };
 
 static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
@@ -1025,7 +1024,6 @@ static void fpsimd_bind_to_cpu(void)
 		this_cpu_ptr(&fpsimd_last_state);
 
 	last->st = &current->thread.uw.fpsimd_state;
-	last->sve_in_use = test_thread_flag(TIF_SVE);
 	current->thread.fpsimd_cpu = smp_processor_id();
 }
 
@@ -1037,7 +1035,6 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
 	WARN_ON(!in_softirq() && !irqs_disabled());
 
 	last->st = st;
-	last->sve_in_use = false;
 }
 
 /*
@@ -1097,24 +1094,6 @@ void fpsimd_flush_cpu_state(void)
 	__this_cpu_write(fpsimd_last_state.st, NULL);
 }
 
-/*
- * Invalidate any task SVE state currently held in this CPU's regs.
- *
- * This is used to prevent the kernel from trying to reuse SVE register data
- * that is detroyed by KVM guest enter/exit.  This function should go away when
- * KVM SVE support is implemented.  Don't use it for anything else.
- */
-#ifdef CONFIG_ARM64_SVE
-void sve_flush_cpu_state(void)
-{
-	struct fpsimd_last_state_struct const *last =
-		this_cpu_ptr(&fpsimd_last_state);
-
-	if (last->st && last->sve_in_use)
-		fpsimd_flush_cpu_state();
-}
-#endif /* CONFIG_ARM64_SVE */
-
 #ifdef CONFIG_KERNEL_MODE_NEON
 
 DEFINE_PER_CPU(bool, kernel_neon_busy);
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 147d9d9..501cacb 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -693,9 +693,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		 */
 		preempt_disable();
 
-		/* Flush FP/SIMD state that can't survive guest entry/exit */
-		kvm_fpsimd_flush_cpu_state();
-
 		kvm_pmu_flush_hwstate(vcpu);
 
 		local_irq_disable();
-- 
2.1.4

[RFC PATCH 8/8] KVM: arm64: Remove eager host SVE state saving

[Christoffer Dall]

On Fri, Apr 20, 2018 at 05:46:42PM +0100, Dave Martin wrote:
> Now that the host SVE context can be saved on demand from Hyp,
> there is no longer any need to save this state in advance before
> entering the guest.
> 
> This patch removes the relevant call to
> kvm_fpsimd_flush_cpu_state().
> 
> Since the problem that function was intended to solve now no longer
> exists, the function and its dependencies are also deleted.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm/include/asm/kvm_host.h   |  3 ---
>  arch/arm64/include/asm/kvm_host.h | 10 ----------
>  arch/arm64/kernel/fpsimd.c        | 21 ---------------------
>  virt/kvm/arm/arm.c                |  3 ---
>  4 files changed, 37 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 3fe01c7..cf268cb 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -308,9 +308,6 @@ static inline void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) {}
>  static inline void kvm_arch_vcpu_park_fp(struct kvm_vcpu *vcpu) {}
>  static inline void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) {}
>  
> -/* All host FP/SIMD state is restored on guest exit, so nothing to save: */
> -static inline void kvm_fpsimd_flush_cpu_state(void) {}
> -
>  static inline void kvm_arm_vhe_guest_enter(void) {}
>  static inline void kvm_arm_vhe_guest_exit(void) {}
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2fbfbda..df52254 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -435,16 +435,6 @@ static inline int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  	return kvm_arch_vcpu_run_map_fp(vcpu);
>  }
>  
> -/*
> - * All host FP/SIMD state is restored on guest exit, so nothing needs
> - * doing here except in the SVE case:
> -*/
> -static inline void kvm_fpsimd_flush_cpu_state(void)
> -{
> -	if (system_supports_sve())
> -		sve_flush_cpu_state();
> -}
> -
>  static inline void kvm_arm_vhe_guest_enter(void)
>  {
>  	local_daif_mask();
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index fa4f6e2..cdfbcf7 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -120,7 +120,6 @@
>   */
>  struct fpsimd_last_state_struct {
>  	struct user_fpsimd_state *st;
> -	bool sve_in_use;
>  };
>  
>  static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
> @@ -1025,7 +1024,6 @@ static void fpsimd_bind_to_cpu(void)
>  		this_cpu_ptr(&fpsimd_last_state);
>  
>  	last->st = &current->thread.uw.fpsimd_state;
> -	last->sve_in_use = test_thread_flag(TIF_SVE);
>  	current->thread.fpsimd_cpu = smp_processor_id();
>  }
>  
> @@ -1037,7 +1035,6 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st)
>  	WARN_ON(!in_softirq() && !irqs_disabled());
>  
>  	last->st = st;
> -	last->sve_in_use = false;
>  }
>  
>  /*
> @@ -1097,24 +1094,6 @@ void fpsimd_flush_cpu_state(void)
>  	__this_cpu_write(fpsimd_last_state.st, NULL);
>  }
>  
> -/*
> - * Invalidate any task SVE state currently held in this CPU's regs.
> - *
> - * This is used to prevent the kernel from trying to reuse SVE register data
> - * that is detroyed by KVM guest enter/exit.  This function should go away when
> - * KVM SVE support is implemented.  Don't use it for anything else.
> - */
> -#ifdef CONFIG_ARM64_SVE
> -void sve_flush_cpu_state(void)
> -{
> -	struct fpsimd_last_state_struct const *last =
> -		this_cpu_ptr(&fpsimd_last_state);
> -
> -	if (last->st && last->sve_in_use)
> -		fpsimd_flush_cpu_state();
> -}
> -#endif /* CONFIG_ARM64_SVE */
> -
>  #ifdef CONFIG_KERNEL_MODE_NEON
>  
>  DEFINE_PER_CPU(bool, kernel_neon_busy);
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 147d9d9..501cacb 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -693,9 +693,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		 */
>  		preempt_disable();
>  
> -		/* Flush FP/SIMD state that can't survive guest entry/exit */
> -		kvm_fpsimd_flush_cpu_state();
> -
>  		kvm_pmu_flush_hwstate(vcpu);
>  
>  		local_irq_disable();
> -- 
> 2.1.4
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Acked-by: Christoffer Dall <christoffer.dall@arm.com>