[PATCH v4 0/3] KVM/arm/arm64: enhance armv7/8 fp/simd lazy switch

[PATCH v4 0/3] KVM/arm/arm64: enhance armv7/8 fp/simd lazy switch

[Mario Smarduch]

This patch series combines the previous armv7 and armv8 versions.
For an FP and lmbench load it reduces fp/simd context switch from 30-50% down
to 2%. Results will vary with load but is no worse then current
approach.

In summary current lazy vfp/simd implementation switches hardware context only
on guest access and again on exit to host, otherwise hardware context is
skipped. This patch set builds on that functionality and executes a hardware
context switch only when  vCPU is scheduled out or returns to user space.

Patches were tested on FVP and Foundation Model sw platforms running floating 
point applications comparing outcome against known results. A bad FP/SIMDcontext
switch should result FP errors. Artificially skipping a fp/simd context switch
(1 in 1000) causes the applications to report errors.

The test can be found here, https://github.com/mjsmar/arm-arm64-fpsimd-test

Tests Ran:
armv7:
- On host executed 12 fp applications - evently pinned to cpus
- Two guests - with 12 fp crunching processes - also pinned to vpus.
- half ran with 1ms sleep, remaining with no sleep

armv8:
- same as above except used mix of armv7 and armv8 guests.

These patches are based on earlier arm64 fp/simd optimization work -
https://lists.cs.columbia.edu/pipermail/kvmarm/2015-July/015748.html

And subsequent fixes by Marc and Christoffer at KVM Forum hackathon to handle
32-bit guest on 64 bit host - 
https://lists.cs.columbia.edu/pipermail/kvmarm/2015-August/016128.html

Changes since v3->v4:
- Followup on Christoffers comments 
  - Move fpexc handling to vcpu_load and vcpu_put
  - Enable and restore fpexc in EL2 mode when running a 32 bit guest on 
    64bit EL2
  - rework hcptr handling

Changes since v2->v3:
- combined arm v7 and v8 into one short patch series
- moved access to fpexec_el2 back to EL2
- Move host restore to EL1 from EL2 and call directly from host
- optimize trap enable code 
- renamed some variables to match usage

Changes since v1->v2:
- Fixed vfp/simd trap configuration to enable trace trapping
- Removed set_hcptr branch label
- Fixed handling of FPEXC to restore guest and host versions on vcpu_put
- Tested arm32/arm64
- rebased to 4.3-rc2
- changed a couple register accesses from 64 to 32 bit


Mario Smarduch (3):
  add hooks for armv7 fp/simd lazy switch support
  enable enhanced armv7 fp/simd lazy switch
  enable enhanced armv8 fp/simd lazy switch

 arch/arm/include/asm/kvm_host.h   | 42 ++++++++++++++++++++
 arch/arm/kernel/asm-offsets.c     |  2 +
 arch/arm/kvm/arm.c                | 24 +++++++++++
 arch/arm/kvm/interrupts.S         | 58 ++++++++++++++++-----------
 arch/arm/kvm/interrupts_head.S    | 26 ++++++++----
 arch/arm64/include/asm/kvm_asm.h  |  2 +
 arch/arm64/include/asm/kvm_host.h | 19 +++++++++
 arch/arm64/kernel/asm-offsets.c   |  1 +
 arch/arm64/kvm/hyp.S              | 83 +++++++++++++++++++++++++--------------
 9 files changed, 196 insertions(+), 61 deletions(-)

-- 
1.9.1

[PATCH v4 1/3] KVM/arm/arm64: add hooks for armv7 fp/simd lazy switch support

[Mario Smarduch]

This patch adds vcpu fields to track lazy state, save host FPEXC, and
offsets to fields.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/include/asm/kvm_host.h | 6 ++++++
 arch/arm/kernel/asm-offsets.c   | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 3df1e97..f1bf551 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -107,6 +107,12 @@ struct kvm_vcpu_arch {
 	/* Interrupt related fields */
 	u32 irq_lines;		/* IRQ and FIQ levels */
 
+	/* fp/simd dirty flag true if guest accessed register file */
+	bool    vfp_dirty;
+
+	/* Save host FPEXC register to later restore on vcpu put */
+	u32	host_fpexc;
+
 	/* Exception Information */
 	struct kvm_vcpu_fault_info fault;
 
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 871b826..9f79712 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -186,6 +186,8 @@ int main(void)
   DEFINE(VCPU_CPSR,		offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_cpsr));
   DEFINE(VCPU_HCR,		offsetof(struct kvm_vcpu, arch.hcr));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
+  DEFINE(VCPU_VFP_DIRTY,	offsetof(struct kvm_vcpu, arch.vfp_dirty));
+  DEFINE(VCPU_VFP_HOST_FPEXC,	offsetof(struct kvm_vcpu, arch.host_fpexc));
   DEFINE(VCPU_HSR,		offsetof(struct kvm_vcpu, arch.fault.hsr));
   DEFINE(VCPU_HxFAR,		offsetof(struct kvm_vcpu, arch.fault.hxfar));
   DEFINE(VCPU_HPFAR,		offsetof(struct kvm_vcpu, arch.fault.hpfar));
-- 
1.9.1

[PATCH v4 2/3] KVM/arm/arm64: enable enhanced armv7 fp/simd lazy switch

[Mario Smarduch]

This patch tracks armv7 fp/simd hardware state with a vcpu lazy flag.
On vcpu_load saves host fpexc and enables FP access, and later enables fp/simd
trapping if lazy flag is not set. On first fp/simd access trap to handler 
to save host and restore guest context, disable trapping and set vcpu lazy 
flag. On vcpu_put if flag is set save guest and restore host context and 
always restore host fpexc.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/include/asm/kvm_host.h   | 33 ++++++++++++++++++++++
 arch/arm/kvm/arm.c                | 12 ++++++++
 arch/arm/kvm/interrupts.S         | 58 +++++++++++++++++++++++----------------
 arch/arm/kvm/interrupts_head.S    | 26 +++++++++++++-----
 arch/arm64/include/asm/kvm_host.h |  6 ++++
 5 files changed, 104 insertions(+), 31 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index f1bf551..8fc7a59 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -40,6 +40,38 @@
 
 #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
 
+/*
+ * Reads the host FPEXC register, saves it to vcpu context and enables the
+ * FP/SIMD unit.
+ */
+#ifdef CONFIG_VFPv3
+#define kvm_enable_fpexc(vcpu) {			\
+	u32 fpexc = 0;					\
+	asm volatile(					\
+	"mrc p10, 7, %0, cr8, cr0, 0\n"			\
+	"str %0, [%1]\n"				\
+	"orr %0, %0, #(1 << 30)\n"			\
+	"mcr p10, 7, %0, cr8, cr0, 0\n"			\
+	: "+r" (fpexc)					\
+	: "r" (&vcpu->arch.host_fpexc)			\
+	);						\
+}
+#else
+#define kvm_enable_fpexc(vcpu)
+#endif
+
+/* Restores host FPEXC register */
+#ifdef CONFIG_VFPv3
+#define kvm_restore_host_fpexc(vcpu) {			\
+	asm volatile(					\
+		"mcr p10, 7, %0, cr8, cr0, 0\n"		\
+		: : "r" (vcpu->arch.host_fpexc)		\
+	);						\
+}
+#else
+#define kvm_restore_host_fpexc(vcpu)
+#endif
+
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
@@ -227,6 +259,7 @@ int kvm_perf_teardown(void);
 void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
 
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
+void kvm_restore_host_vfp_state(struct kvm_vcpu *);
 
 static inline void kvm_arch_hardware_disable(void) {}
 static inline void kvm_arch_hardware_unsetup(void) {}
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index dc017ad..cfc348a 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -291,10 +291,22 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	vcpu->arch.host_cpu_context = this_cpu_ptr(kvm_host_cpu_state);
 
 	kvm_arm_set_running_vcpu(vcpu);
+
+	/* Save and enable FPEXC before we load guest context */
+	kvm_enable_fpexc(vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 {
+	/* If the fp/simd registers are dirty save guest, restore host. */
+	if (vcpu->arch.vfp_dirty) {
+		kvm_restore_host_vfp_state(vcpu);
+		vcpu->arch.vfp_dirty = 0;
+	}
+
+	/* Restore host FPEXC trashed in vcpu_load */
+	kvm_restore_host_fpexc(vcpu);
+
 	/*
 	 * The arch-generic KVM code expects the cpu field of a vcpu to be -1
 	 * if the vcpu is no longer assigned to a cpu.  This is used for the
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index 900ef6d..1ddaa89 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -28,6 +28,26 @@
 #include "interrupts_head.S"
 
 	.text
+/**
+ * void kvm_restore_host_vfp_state(struct vcpu *vcpu) -
+ *	This function is called from host to save the guest, and restore host
+ *	fp/simd hardware context. It's placed outside of hyp start/end region.
+ */
+ENTRY(kvm_restore_host_vfp_state)
+#ifdef CONFIG_VFPv3
+	push    {r4-r7}
+
+	add     r7, vcpu, #VCPU_VFP_GUEST
+	store_vfp_state r7
+
+	add     r7, vcpu, #VCPU_VFP_HOST
+	ldr     r7, [r7]
+	restore_vfp_state r7
+
+	pop     {r4-r7}
+#endif
+	bx      lr
+ENDPROC(kvm_restore_host_vfp_state)
 
 __kvm_hyp_code_start:
 	.globl __kvm_hyp_code_start
@@ -116,22 +136,22 @@ ENTRY(__kvm_vcpu_run)
 	read_cp15_state store_to_vcpu = 0
 	write_cp15_state read_from_vcpu = 1
 
+	set_hcptr_bits set, r4, (HCPTR_TTA)
 	@ If the host kernel has not been configured with VFPv3 support,
 	@ then it is safer if we deny guests from using it as well.
 #ifdef CONFIG_VFPv3
-	@ Set FPEXC_EN so the guest doesn't trap floating point instructions
-	VFPFMRX r2, FPEXC		@ VMRS
-	push	{r2}
-	orr	r2, r2, #FPEXC_EN
-	VFPFMXR FPEXC, r2		@ VMSR
+	@ fp/simd register file has already been accessed, so skip trap enable.
+	vfp_skip_if_dirty r7, skip_guest_vfp_trap
+	set_hcptr_bits orr, r4, (HCPTR_TCP(10) | HCPTR_TCP(11))
+skip_guest_vfp_trap:
 #endif
+	set_hcptr vmentry, r4
 
 	@ Configure Hyp-role
 	configure_hyp_role vmentry
 
 	@ Trap coprocessor CRx accesses
 	set_hstr vmentry
-	set_hcptr vmentry, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
 	set_hdcr vmentry
 
 	@ Write configured ID register into MIDR alias
@@ -170,23 +190,8 @@ __kvm_vcpu_return:
 	@ Don't trap coprocessor accesses for host kernel
 	set_hstr vmexit
 	set_hdcr vmexit
-	set_hcptr vmexit, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11)), after_vfp_restore
-
-#ifdef CONFIG_VFPv3
-	@ Switch VFP/NEON hardware state to the host's
-	add	r7, vcpu, #VCPU_VFP_GUEST
-	store_vfp_state r7
-	add	r7, vcpu, #VCPU_VFP_HOST
-	ldr	r7, [r7]
-	restore_vfp_state r7
-
-after_vfp_restore:
-	@ Restore FPEXC_EN which we clobbered on entry
-	pop	{r2}
-	VFPFMXR FPEXC, r2
-#else
-after_vfp_restore:
-#endif
+	set_hcptr_bits clear, r4, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
+	set_hcptr vmexit, r4
 
 	@ Reset Hyp-role
 	configure_hyp_role vmexit
@@ -483,7 +488,12 @@ switch_to_guest_vfp:
 	push	{r3-r7}
 
 	@ NEON/VFP used.  Turn on VFP access.
-	set_hcptr vmtrap, (HCPTR_TCP(10) | HCPTR_TCP(11))
+	set_hcptr_bits clear, r4, (HCPTR_TCP(10) | HCPTR_TCP(11))
+	set_hcptr vmtrap, r4
+
+	@ set lazy mode flag, switch hardware context on vcpu_put
+	mov     r1, #1
+	strb    r1, [vcpu, #VCPU_VFP_DIRTY]
 
 	@ Switch VFP/NEON hardware state to the guest's
 	add	r7, r0, #VCPU_VFP_HOST
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 51a5950..b2b698e 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -589,16 +589,24 @@ ARM_BE8(rev	r6, r6  )
 	mcr	p15, 4, r2, c1, c1, 3
 .endm
 
+/* Prepares HCPTR bit mask to set, clear or 'orr' bits */
+.macro set_hcptr_bits op, reg, mask
+	.if     \op == set || \op == clear
+	ldr     \reg, =\mask
+	.else
+	orr     \reg, \reg, #\mask
+	.endif
+.endm
+
+
 /* Configures the HCPTR (Hyp Coprocessor Trap Register) on entry/return
  * (hardware reset value is 0). Keep previous value in r2.
  * An ISB is emited on vmexit/vmtrap, but executed on vmexit only if
  * VFP wasn't already enabled (always executed on vmtrap).
- * If a label is specified with vmexit, it is branched to if VFP wasn't
- * enabled.
  */
-.macro set_hcptr operation, mask, label = none
+.macro set_hcptr operation, mask
 	mrc	p15, 4, r2, c1, c1, 2
-	ldr	r3, =\mask
+	mov	r3, \mask
 	.if \operation == vmentry
 	orr	r3, r2, r3		@ Trap coproc-accesses defined in mask
 	.else
@@ -611,13 +619,17 @@ ARM_BE8(rev	r6, r6  )
 	beq	1f
 	.endif
 	isb
-	.if \label != none
-	b	\label
-	.endif
 1:
 	.endif
 .endm
 
+/* Checks if VFP/SIMD dirty flag is set, if it is branch to label. */
+.macro vfp_skip_if_dirty, reg, label
+	ldr	\reg, [vcpu, #VCPU_VFP_DIRTY]
+	cmp	\reg, #1
+	beq	\label
+.endm
+
 /* Configures the HDCR (Hyp Debug Configuration Register) on entry/return
  * (hardware reset value is 0) */
 .macro set_hdcr operation
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 4562459..83e65dd 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -157,6 +157,9 @@ struct kvm_vcpu_arch {
 	/* Interrupt related fields */
 	u64 irq_lines;		/* IRQ and FIQ levels */
 
+	/* fp/simd dirty flag true if guest accessed register file */
+	bool    vfp_dirty;
+
 	/* Cache some mmu pages needed inside spinlock regions */
 	struct kvm_mmu_memory_cache mmu_page_cache;
 
@@ -248,6 +251,9 @@ static inline void kvm_arch_hardware_unsetup(void) {}
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
+static inline void kvm_enable_fpexc(struct kvm_vcpu *vcpu) {}
+static inline void kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu) {}
+static inline void kvm_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
 
 void kvm_arm_init_debug(void);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
-- 
1.9.1

[PATCH v4 3/3] KVM/arm/arm64: enable enhanced armv8 fp/simd lazy switch

[Mario Smarduch]

This patch tracks armv7 and armv8 fp/simd hardware state with a vcpu lazy flag.
On vcpu_load for 32 bit guests enable FP access, and later enable fp/simd
trapping for 32 and 64 bit guests if lazy flag is not set. On first fp/simd 
access trap to handler to save host and restore guest context, disable 
trapping and set vcpu lazy flag. On vcpu_put if flag is set save guest and 
restore host context and also save guest fpexc register.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/include/asm/kvm_host.h   |  3 ++
 arch/arm/kvm/arm.c                | 18 +++++++--
 arch/arm64/include/asm/kvm_asm.h  |  2 +
 arch/arm64/include/asm/kvm_host.h | 17 +++++++-
 arch/arm64/kernel/asm-offsets.c   |  1 +
 arch/arm64/kvm/hyp.S              | 83 +++++++++++++++++++++++++--------------
 6 files changed, 89 insertions(+), 35 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 8fc7a59..6960ff2 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -40,6 +40,8 @@
 
 #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
 
+#define kvm_guest_is32bit(vcpu)	true
+
 /*
  * Reads the host FPEXC register, saves to vcpu context and enables the
  * FPEXC.
@@ -260,6 +262,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
 
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
 void kvm_restore_host_vfp_state(struct kvm_vcpu *);
+static inline void kvm_save_guest_fpexc(struct kvm_vcpu *vcpu) {}
 
 static inline void kvm_arch_hardware_disable(void) {}
 static inline void kvm_arch_hardware_unsetup(void) {}
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index cfc348a..7a20530 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -292,8 +292,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 	kvm_arm_set_running_vcpu(vcpu);
 
-	/* Save and enable FPEXC before we load guest context */
-	kvm_enable_fpexc(vcpu);
+	/*
+	 * For 32bit guest executing on arm64, enable fp/simd access in
+	 * EL2. On arm32 save host fpexc and then enable fp/simd access.
+	 */
+	if (kvm_guest_is32bit(vcpu))
+		kvm_enable_fpexc(vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
@@ -301,10 +305,18 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	/* If the fp/simd registers are dirty save guest, restore host. */
 	if (vcpu->arch.vfp_dirty) {
 		kvm_restore_host_vfp_state(vcpu);
+
+		/*
+		 * For 32bit guest on arm64 save the guest fpexc register
+		 * in EL2 mode.
+		 */
+		if (kvm_guest_is32bit(vcpu))
+			kvm_save_guest_fpexc(vcpu);
+
 		vcpu->arch.vfp_dirty = 0;
 	}
 
-	/* Restore host FPEXC trashed in vcpu_load */
+	/* For arm32 restore host FPEXC trashed in vcpu_load. */
 	kvm_restore_host_fpexc(vcpu);
 
 	/*
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 5e37710..c589ca9 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -117,6 +117,8 @@ extern char __kvm_hyp_vector[];
 extern void __kvm_flush_vm_context(void);
 extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
 extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
+extern void __kvm_enable_fpexc32(void);
+extern void __kvm_save_fpexc32(struct kvm_vcpu *vcpu);
 
 extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 83e65dd..6e2d6b5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -41,6 +41,8 @@
 
 #define KVM_VCPU_MAX_FEATURES 3
 
+#define kvm_guest_is32bit(vcpu)	(!(vcpu->arch.hcr_el2 & HCR_RW))
+
 int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
 int kvm_arch_dev_ioctl_check_extension(long ext);
@@ -251,9 +253,20 @@ static inline void kvm_arch_hardware_unsetup(void) {}
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
 static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
-static inline void kvm_enable_fpexc(struct kvm_vcpu *vcpu) {}
-static inline void kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu) {}
+
+static inline void kvm_enable_fpexc(struct kvm_vcpu *vcpu)
+{
+	/* Enable FP/SIMD access from EL2 mode*/
+	kvm_call_hyp(__kvm_enable_fpexc32);
+}
+
+static inline void kvm_save_guest_fpexc(struct kvm_vcpu *vcpu)
+{
+	/* Save FPEXEC32_EL2 in EL2 mode */
+	kvm_call_hyp(__kvm_save_fpexc32, vcpu);
+}
 static inline void kvm_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
+void kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu);
 
 void kvm_arm_init_debug(void);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 8d89cf8..c9c5242 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -124,6 +124,7 @@ int main(void)
   DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
   DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
+  DEFINE(VCPU_VFP_DIRTY,	offsetof(struct kvm_vcpu, arch.vfp_dirty));
   DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
   DEFINE(VCPU_HOST_DEBUG_STATE, offsetof(struct kvm_vcpu, arch.host_debug_state));
   DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index e583613..06efb93 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -36,6 +36,23 @@
 #define CPU_SYSREG_OFFSET(x)	(CPU_SYSREGS + 8*x)
 
 	.text
+/**
+ * void kvm_restore_host_vfp_state(struct vcpu *vcpu) -
+ *	This function saves the guest, restores host, called from host.
+ */
+ENTRY(kvm_restore_host_vfp_state)
+	push	xzr, lr
+
+	add	x2, x0, #VCPU_CONTEXT
+	bl __save_fpsimd
+
+	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
+	bl __restore_fpsimd
+
+	pop	xzr, lr
+	ret
+ENDPROC(kvm_restore_host_vfp_state)
+
 	.pushsection	.hyp.text, "ax"
 	.align	PAGE_SHIFT
 
@@ -385,14 +402,6 @@
 	tbz	\tmp, #KVM_ARM64_DEBUG_DIRTY_SHIFT, \target
 .endm
 
-/*
- * Branch to target if CPTR_EL2.TFP bit is set (VFP/SIMD trapping enabled)
- */
-.macro skip_fpsimd_state tmp, target
-	mrs	\tmp, cptr_el2
-	tbnz	\tmp, #CPTR_EL2_TFP_SHIFT, \target
-.endm
-
 .macro compute_debug_state target
 	// Compute debug state: If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY
 	// is set, we do a full save/restore cycle and disable trapping.
@@ -433,10 +442,6 @@
 	mrs	x5, ifsr32_el2
 	stp	x4, x5, [x3]
 
-	skip_fpsimd_state x8, 2f
-	mrs	x6, fpexc32_el2
-	str	x6, [x3, #16]
-2:
 	skip_debug_state x8, 1f
 	mrs	x7, dbgvcr32_el2
 	str	x7, [x3, #24]
@@ -468,21 +473,13 @@
 .macro activate_traps
 	ldr     x2, [x0, #VCPU_HCR_EL2]
 
-	/*
-	 * We are about to set CPTR_EL2.TFP to trap all floating point
-	 * register accesses to EL2, however, the ARM ARM clearly states that
-	 * traps are only taken to EL2 if the operation would not otherwise
-	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
-	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
-	 */
-	tbnz	x2, #HCR_RW_SHIFT, 99f // open code skip_32bit_state
-	mov	x3, #(1 << 30)
-	msr	fpexc32_el2, x3
-	isb
-99:
 	msr     hcr_el2, x2
 	mov	x2, #CPTR_EL2_TTA
+
+	ldrb    w3, [x0, #VCPU_VFP_DIRTY]
+	tbnz    w3, #0, 98f
 	orr     x2, x2, #CPTR_EL2_TFP
+98:
 	msr	cptr_el2, x2
 
 	mov	x2, #(1 << 15)	// Trap CP15 Cr=15
@@ -669,14 +666,12 @@ __restore_debug:
 	ret
 
 __save_fpsimd:
-	skip_fpsimd_state x3, 1f
 	save_fpsimd
-1:	ret
+	ret
 
 __restore_fpsimd:
-	skip_fpsimd_state x3, 1f
 	restore_fpsimd
-1:	ret
+	ret
 
 switch_to_guest_fpsimd:
 	push	x4, lr
@@ -688,6 +683,9 @@ switch_to_guest_fpsimd:
 
 	mrs	x0, tpidr_el2
 
+	mov	w2, #1
+	strb    w2, [x0, #VCPU_VFP_DIRTY]
+
 	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
 	kern_hyp_va x2
 	bl __save_fpsimd
@@ -763,7 +761,6 @@ __kvm_vcpu_return:
 	add	x2, x0, #VCPU_CONTEXT
 
 	save_guest_regs
-	bl __save_fpsimd
 	bl __save_sysregs
 
 	skip_debug_state x3, 1f
@@ -784,7 +781,6 @@ __kvm_vcpu_return:
 	kern_hyp_va x2
 
 	bl __restore_sysregs
-	bl __restore_fpsimd
 	/* Clear FPSIMD and Trace trapping */
 	msr     cptr_el2, xzr
 
@@ -863,6 +859,33 @@ ENTRY(__kvm_flush_vm_context)
 	ret
 ENDPROC(__kvm_flush_vm_context)
 
+/**
+ * void __kvm_enable_fpexc32(void) -
+ *	We may be entering the guest and set CPTR_EL2.TFP to trap all floating
+ *	point register accesses to EL2, however, the ARM manual clearly states
+ *	that traps are only taken to EL2 if the operation would not otherwise
+ *	trap to EL1.  Therefore, always make sure that for 32-bit guests,
+ *	we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
+ */
+ENTRY(__kvm_enable_fpexc32)
+	mov	x3, #(1 << 30)
+	msr	fpexc32_el2, x3
+	ret
+ENDPROC(__kvm_enable_fpexc32)
+
+/**
+ * void __kvm_save_fpexc32(void) -
+ *	This function restores guest FPEXC to its vcpu context, we call this
+ *	function from vcpu_put.
+ */
+ENTRY(__kvm_save_fpexc32)
+	kern_hyp_va x0
+	add     x2, x0, #VCPU_CONTEXT
+	mrs     x1, fpexc32_el2
+	str     x1, [x2, #CPU_SYSREG_OFFSET(FPEXC32_EL2)]
+	ret
+ENDPROC(__kvm_save_fpexc32)
+
 __kvm_hyp_panic:
 	// Guess the context by looking@VTTBR:
 	// If zero, then we're already a host.
-- 
1.9.1

[PATCH v4 1/3] KVM/arm/arm64: add hooks for armv7 fp/simd lazy switch support

[Marc Zyngier]

On 14/11/15 22:12, Mario Smarduch wrote:
> This patch adds vcpu fields to track lazy state, save host FPEXC, and
> offsets to fields.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  arch/arm/include/asm/kvm_host.h | 6 ++++++
>  arch/arm/kernel/asm-offsets.c   | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 3df1e97..f1bf551 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -107,6 +107,12 @@ struct kvm_vcpu_arch {
>  	/* Interrupt related fields */
>  	u32 irq_lines;		/* IRQ and FIQ levels */
>  
> +	/* fp/simd dirty flag true if guest accessed register file */
> +	bool    vfp_dirty;

I think we do not need this bool, because it is already represented by
the state of the trapping bits.

> +
> +	/* Save host FPEXC register to later restore on vcpu put */
> +	u32	host_fpexc;
> +
>  	/* Exception Information */
>  	struct kvm_vcpu_fault_info fault;
>  
> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> index 871b826..9f79712 100644
> --- a/arch/arm/kernel/asm-offsets.c
> +++ b/arch/arm/kernel/asm-offsets.c
> @@ -186,6 +186,8 @@ int main(void)
>    DEFINE(VCPU_CPSR,		offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_cpsr));
>    DEFINE(VCPU_HCR,		offsetof(struct kvm_vcpu, arch.hcr));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
> +  DEFINE(VCPU_VFP_DIRTY,	offsetof(struct kvm_vcpu, arch.vfp_dirty));
> +  DEFINE(VCPU_VFP_HOST_FPEXC,	offsetof(struct kvm_vcpu, arch.host_fpexc));
>    DEFINE(VCPU_HSR,		offsetof(struct kvm_vcpu, arch.fault.hsr));
>    DEFINE(VCPU_HxFAR,		offsetof(struct kvm_vcpu, arch.fault.hxfar));
>    DEFINE(VCPU_HPFAR,		offsetof(struct kvm_vcpu, arch.fault.hpfar));
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v4 2/3] KVM/arm/arm64: enable enhanced armv7 fp/simd lazy switch

[Marc Zyngier]

On 14/11/15 22:12, Mario Smarduch wrote:
> This patch tracks armv7 fp/simd hardware state with a vcpu lazy flag.
> On vcpu_load saves host fpexc and enables FP access, and later enables fp/simd
> trapping if lazy flag is not set. On first fp/simd access trap to handler 
> to save host and restore guest context, disable trapping and set vcpu lazy 
> flag. On vcpu_put if flag is set save guest and restore host context and 
> always restore host fpexc.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  arch/arm/include/asm/kvm_host.h   | 33 ++++++++++++++++++++++
>  arch/arm/kvm/arm.c                | 12 ++++++++
>  arch/arm/kvm/interrupts.S         | 58 +++++++++++++++++++++++----------------
>  arch/arm/kvm/interrupts_head.S    | 26 +++++++++++++-----
>  arch/arm64/include/asm/kvm_host.h |  6 ++++
>  5 files changed, 104 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index f1bf551..8fc7a59 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -40,6 +40,38 @@
>  
>  #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
>  
> +/*
> + * Reads the host FPEXC register, saves it to vcpu context and enables the
> + * FP/SIMD unit.
> + */
> +#ifdef CONFIG_VFPv3
> +#define kvm_enable_fpexc(vcpu) {			\
> +	u32 fpexc = 0;					\
> +	asm volatile(					\
> +	"mrc p10, 7, %0, cr8, cr0, 0\n"			\
> +	"str %0, [%1]\n"				\
> +	"orr %0, %0, #(1 << 30)\n"			\
> +	"mcr p10, 7, %0, cr8, cr0, 0\n"			\

Don't you need an ISB here? Also, it would be a lot nicer if this was a
real function (possibly inlined). I don't see any real reason to make
this a #define.

Also, you're preserving a lot of the host's FPEXC bits. Is that safe?

> +	: "+r" (fpexc)					\
> +	: "r" (&vcpu->arch.host_fpexc)			\
> +	);						\
> +}
> +#else
> +#define kvm_enable_fpexc(vcpu)
> +#endif
> +
> +/* Restores host FPEXC register */
> +#ifdef CONFIG_VFPv3
> +#define kvm_restore_host_fpexc(vcpu) {			\
> +	asm volatile(					\
> +		"mcr p10, 7, %0, cr8, cr0, 0\n"		\
> +		: : "r" (vcpu->arch.host_fpexc)		\
> +	);						\
> +}
> +#else
> +#define kvm_restore_host_fpexc(vcpu)
> +#endif
> +
>  u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
>  int __attribute_const__ kvm_target_cpu(void);
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
> @@ -227,6 +259,7 @@ int kvm_perf_teardown(void);
>  void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>  
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
> +void kvm_restore_host_vfp_state(struct kvm_vcpu *);
>  
>  static inline void kvm_arch_hardware_disable(void) {}
>  static inline void kvm_arch_hardware_unsetup(void) {}
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index dc017ad..cfc348a 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -291,10 +291,22 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	vcpu->arch.host_cpu_context = this_cpu_ptr(kvm_host_cpu_state);
>  
>  	kvm_arm_set_running_vcpu(vcpu);
> +
> +	/* Save and enable FPEXC before we load guest context */
> +	kvm_enable_fpexc(vcpu);
>  }
>  
>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  {
> +	/* If the fp/simd registers are dirty save guest, restore host. */
> +	if (vcpu->arch.vfp_dirty) {

See my previous comment about the dirty state.

> +		kvm_restore_host_vfp_state(vcpu);
> +		vcpu->arch.vfp_dirty = 0;
> +	}
> +
> +	/* Restore host FPEXC trashed in vcpu_load */
> +	kvm_restore_host_fpexc(vcpu);
> +
>  	/*
>  	 * The arch-generic KVM code expects the cpu field of a vcpu to be -1
>  	 * if the vcpu is no longer assigned to a cpu.  This is used for the
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index 900ef6d..1ddaa89 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -28,6 +28,26 @@
>  #include "interrupts_head.S"
>  
>  	.text
> +/**
> + * void kvm_restore_host_vfp_state(struct vcpu *vcpu) -
> + *	This function is called from host to save the guest, and restore host
> + *	fp/simd hardware context. It's placed outside of hyp start/end region.
> + */
> +ENTRY(kvm_restore_host_vfp_state)
> +#ifdef CONFIG_VFPv3
> +	push    {r4-r7}
> +
> +	add     r7, vcpu, #VCPU_VFP_GUEST
> +	store_vfp_state r7
> +
> +	add     r7, vcpu, #VCPU_VFP_HOST
> +	ldr     r7, [r7]
> +	restore_vfp_state r7
> +
> +	pop     {r4-r7}
> +#endif
> +	bx      lr
> +ENDPROC(kvm_restore_host_vfp_state)
>  

Please don't mix things that are part of the HYP code and things that
are not in the same file. This is just asking for trouble. If that's not
mapped into HYP, put it in a separate file.

>  __kvm_hyp_code_start:
>  	.globl __kvm_hyp_code_start
> @@ -116,22 +136,22 @@ ENTRY(__kvm_vcpu_run)
>  	read_cp15_state store_to_vcpu = 0
>  	write_cp15_state read_from_vcpu = 1
>  
> +	set_hcptr_bits set, r4, (HCPTR_TTA)
>  	@ If the host kernel has not been configured with VFPv3 support,
>  	@ then it is safer if we deny guests from using it as well.
>  #ifdef CONFIG_VFPv3
> -	@ Set FPEXC_EN so the guest doesn't trap floating point instructions
> -	VFPFMRX r2, FPEXC		@ VMRS
> -	push	{r2}
> -	orr	r2, r2, #FPEXC_EN
> -	VFPFMXR FPEXC, r2		@ VMSR
> +	@ fp/simd register file has already been accessed, so skip trap enable.
> +	vfp_skip_if_dirty r7, skip_guest_vfp_trap
> +	set_hcptr_bits orr, r4, (HCPTR_TCP(10) | HCPTR_TCP(11))
> +skip_guest_vfp_trap:
>  #endif
> +	set_hcptr vmentry, r4
>  
>  	@ Configure Hyp-role
>  	configure_hyp_role vmentry
>  
>  	@ Trap coprocessor CRx accesses
>  	set_hstr vmentry
> -	set_hcptr vmentry, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
>  	set_hdcr vmentry
>  
>  	@ Write configured ID register into MIDR alias
> @@ -170,23 +190,8 @@ __kvm_vcpu_return:
>  	@ Don't trap coprocessor accesses for host kernel
>  	set_hstr vmexit
>  	set_hdcr vmexit
> -	set_hcptr vmexit, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11)), after_vfp_restore
> -
> -#ifdef CONFIG_VFPv3
> -	@ Switch VFP/NEON hardware state to the host's
> -	add	r7, vcpu, #VCPU_VFP_GUEST
> -	store_vfp_state r7
> -	add	r7, vcpu, #VCPU_VFP_HOST
> -	ldr	r7, [r7]
> -	restore_vfp_state r7
> -
> -after_vfp_restore:
> -	@ Restore FPEXC_EN which we clobbered on entry
> -	pop	{r2}
> -	VFPFMXR FPEXC, r2
> -#else
> -after_vfp_restore:
> -#endif
> +	set_hcptr_bits clear, r4, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
> +	set_hcptr vmexit, r4
>  
>  	@ Reset Hyp-role
>  	configure_hyp_role vmexit
> @@ -483,7 +488,12 @@ switch_to_guest_vfp:
>  	push	{r3-r7}
>  
>  	@ NEON/VFP used.  Turn on VFP access.
> -	set_hcptr vmtrap, (HCPTR_TCP(10) | HCPTR_TCP(11))
> +	set_hcptr_bits clear, r4, (HCPTR_TCP(10) | HCPTR_TCP(11))
> +	set_hcptr vmtrap, r4
> +
> +	@ set lazy mode flag, switch hardware context on vcpu_put
> +	mov     r1, #1
> +	strb    r1, [vcpu, #VCPU_VFP_DIRTY]

You are assuming that a boolean is a byte. That's wrong, and not endian
safe. If you want to have such a flag in a structure, use a sized type
(u8, u16, u32). But again, I think we should rely on the trap flags to
make decisions outside of the HYP code.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v4 3/3] KVM/arm/arm64: enable enhanced armv8 fp/simd lazy switch

[Marc Zyngier]

On 14/11/15 22:12, Mario Smarduch wrote:
> This patch tracks armv7 and armv8 fp/simd hardware state with a vcpu lazy flag.
> On vcpu_load for 32 bit guests enable FP access, and later enable fp/simd
> trapping for 32 and 64 bit guests if lazy flag is not set. On first fp/simd 
> access trap to handler to save host and restore guest context, disable 
> trapping and set vcpu lazy flag. On vcpu_put if flag is set save guest and 
> restore host context and also save guest fpexc register.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  arch/arm/include/asm/kvm_host.h   |  3 ++
>  arch/arm/kvm/arm.c                | 18 +++++++--
>  arch/arm64/include/asm/kvm_asm.h  |  2 +
>  arch/arm64/include/asm/kvm_host.h | 17 +++++++-
>  arch/arm64/kernel/asm-offsets.c   |  1 +
>  arch/arm64/kvm/hyp.S              | 83 +++++++++++++++++++++++++--------------
>  6 files changed, 89 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 8fc7a59..6960ff2 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -40,6 +40,8 @@
>  
>  #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
>  
> +#define kvm_guest_is32bit(vcpu)	true

This should be defined as an inline function, and placed in
asm/kvm_emulate.h, probably renamed as kvm_guest_vcpu_is_32bit.

> +
>  /*
>   * Reads the host FPEXC register, saves to vcpu context and enables the
>   * FPEXC.
> @@ -260,6 +262,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>  
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>  void kvm_restore_host_vfp_state(struct kvm_vcpu *);
> +static inline void kvm_save_guest_fpexc(struct kvm_vcpu *vcpu) {}
>  
>  static inline void kvm_arch_hardware_disable(void) {}
>  static inline void kvm_arch_hardware_unsetup(void) {}
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index cfc348a..7a20530 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -292,8 +292,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  
>  	kvm_arm_set_running_vcpu(vcpu);
>  
> -	/* Save and enable FPEXC before we load guest context */
> -	kvm_enable_fpexc(vcpu);
> +	/*
> +	 * For 32bit guest executing on arm64, enable fp/simd access in
> +	 * EL2. On arm32 save host fpexc and then enable fp/simd access.
> +	 */
> +	if (kvm_guest_is32bit(vcpu))
> +		kvm_enable_fpexc(vcpu);
>  }
>  
>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> @@ -301,10 +305,18 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	/* If the fp/simd registers are dirty save guest, restore host. */
>  	if (vcpu->arch.vfp_dirty) {
>  		kvm_restore_host_vfp_state(vcpu);
> +
> +		/*
> +		 * For 32bit guest on arm64 save the guest fpexc register
> +		 * in EL2 mode.
> +		 */
> +		if (kvm_guest_is32bit(vcpu))
> +			kvm_save_guest_fpexc(vcpu);
> +
>  		vcpu->arch.vfp_dirty = 0;
>  	}
>  
> -	/* Restore host FPEXC trashed in vcpu_load */
> +	/* For arm32 restore host FPEXC trashed in vcpu_load. */
>  	kvm_restore_host_fpexc(vcpu);
>  
>  	/*
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 5e37710..c589ca9 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -117,6 +117,8 @@ extern char __kvm_hyp_vector[];
>  extern void __kvm_flush_vm_context(void);
>  extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
>  extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
> +extern void __kvm_enable_fpexc32(void);
> +extern void __kvm_save_fpexc32(struct kvm_vcpu *vcpu);
>  
>  extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 83e65dd..6e2d6b5 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -41,6 +41,8 @@
>  
>  #define KVM_VCPU_MAX_FEATURES 3
>  
> +#define kvm_guest_is32bit(vcpu)	(!(vcpu->arch.hcr_el2 & HCR_RW))
> +
>  int __attribute_const__ kvm_target_cpu(void);
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
>  int kvm_arch_dev_ioctl_check_extension(long ext);
> @@ -251,9 +253,20 @@ static inline void kvm_arch_hardware_unsetup(void) {}
>  static inline void kvm_arch_sync_events(struct kvm *kvm) {}
>  static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
>  static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
> -static inline void kvm_enable_fpexc(struct kvm_vcpu *vcpu) {}
> -static inline void kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu) {}
> +
> +static inline void kvm_enable_fpexc(struct kvm_vcpu *vcpu)
> +{
> +	/* Enable FP/SIMD access from EL2 mode*/
> +	kvm_call_hyp(__kvm_enable_fpexc32);
> +}
> +
> +static inline void kvm_save_guest_fpexc(struct kvm_vcpu *vcpu)
> +{
> +	/* Save FPEXEC32_EL2 in EL2 mode */
> +	kvm_call_hyp(__kvm_save_fpexc32, vcpu);
> +}
>  static inline void kvm_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
> +void kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu);
>  
>  void kvm_arm_init_debug(void);
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 8d89cf8..c9c5242 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -124,6 +124,7 @@ int main(void)
>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
> +  DEFINE(VCPU_VFP_DIRTY,	offsetof(struct kvm_vcpu, arch.vfp_dirty));
>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
>    DEFINE(VCPU_HOST_DEBUG_STATE, offsetof(struct kvm_vcpu, arch.host_debug_state));
>    DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index e583613..06efb93 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -36,6 +36,23 @@
>  #define CPU_SYSREG_OFFSET(x)	(CPU_SYSREGS + 8*x)
>  
>  	.text
> +/**
> + * void kvm_restore_host_vfp_state(struct vcpu *vcpu) -
> + *	This function saves the guest, restores host, called from host.
> + */
> +ENTRY(kvm_restore_host_vfp_state)
> +	push	xzr, lr
> +
> +	add	x2, x0, #VCPU_CONTEXT
> +	bl __save_fpsimd
> +
> +	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
> +	bl __restore_fpsimd
> +
> +	pop	xzr, lr
> +	ret
> +ENDPROC(kvm_restore_host_vfp_state)
> +

Same here. Don't mix HYP and non-HYP in the same file.

>  	.pushsection	.hyp.text, "ax"
>  	.align	PAGE_SHIFT
>  
> @@ -385,14 +402,6 @@
>  	tbz	\tmp, #KVM_ARM64_DEBUG_DIRTY_SHIFT, \target
>  .endm
>  
> -/*
> - * Branch to target if CPTR_EL2.TFP bit is set (VFP/SIMD trapping enabled)
> - */
> -.macro skip_fpsimd_state tmp, target
> -	mrs	\tmp, cptr_el2
> -	tbnz	\tmp, #CPTR_EL2_TFP_SHIFT, \target
> -.endm
> -
>  .macro compute_debug_state target
>  	// Compute debug state: If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY
>  	// is set, we do a full save/restore cycle and disable trapping.
> @@ -433,10 +442,6 @@
>  	mrs	x5, ifsr32_el2
>  	stp	x4, x5, [x3]
>  
> -	skip_fpsimd_state x8, 2f
> -	mrs	x6, fpexc32_el2
> -	str	x6, [x3, #16]
> -2:
>  	skip_debug_state x8, 1f
>  	mrs	x7, dbgvcr32_el2
>  	str	x7, [x3, #24]
> @@ -468,21 +473,13 @@
>  .macro activate_traps
>  	ldr     x2, [x0, #VCPU_HCR_EL2]
>  
> -	/*
> -	 * We are about to set CPTR_EL2.TFP to trap all floating point
> -	 * register accesses to EL2, however, the ARM ARM clearly states that
> -	 * traps are only taken to EL2 if the operation would not otherwise
> -	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
> -	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
> -	 */
> -	tbnz	x2, #HCR_RW_SHIFT, 99f // open code skip_32bit_state
> -	mov	x3, #(1 << 30)
> -	msr	fpexc32_el2, x3
> -	isb
> -99:
>  	msr     hcr_el2, x2
>  	mov	x2, #CPTR_EL2_TTA
> +
> +	ldrb    w3, [x0, #VCPU_VFP_DIRTY]
> +	tbnz    w3, #0, 98f
>  	orr     x2, x2, #CPTR_EL2_TFP
> +98:
>  	msr	cptr_el2, x2
>  
>  	mov	x2, #(1 << 15)	// Trap CP15 Cr=15
> @@ -669,14 +666,12 @@ __restore_debug:
>  	ret
>  
>  __save_fpsimd:
> -	skip_fpsimd_state x3, 1f
>  	save_fpsimd
> -1:	ret
> +	ret
>  
>  __restore_fpsimd:
> -	skip_fpsimd_state x3, 1f
>  	restore_fpsimd
> -1:	ret
> +	ret
>  
>  switch_to_guest_fpsimd:
>  	push	x4, lr
> @@ -688,6 +683,9 @@ switch_to_guest_fpsimd:
>  
>  	mrs	x0, tpidr_el2
>  
> +	mov	w2, #1
> +	strb    w2, [x0, #VCPU_VFP_DIRTY]
> +
>  	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
>  	kern_hyp_va x2
>  	bl __save_fpsimd
> @@ -763,7 +761,6 @@ __kvm_vcpu_return:
>  	add	x2, x0, #VCPU_CONTEXT
>  
>  	save_guest_regs
> -	bl __save_fpsimd
>  	bl __save_sysregs
>  
>  	skip_debug_state x3, 1f
> @@ -784,7 +781,6 @@ __kvm_vcpu_return:
>  	kern_hyp_va x2
>  
>  	bl __restore_sysregs
> -	bl __restore_fpsimd
>  	/* Clear FPSIMD and Trace trapping */
>  	msr     cptr_el2, xzr
>  
> @@ -863,6 +859,33 @@ ENTRY(__kvm_flush_vm_context)
>  	ret
>  ENDPROC(__kvm_flush_vm_context)
>  
> +/**
> + * void __kvm_enable_fpexc32(void) -
> + *	We may be entering the guest and set CPTR_EL2.TFP to trap all floating
> + *	point register accesses to EL2, however, the ARM manual clearly states
> + *	that traps are only taken to EL2 if the operation would not otherwise
> + *	trap to EL1.  Therefore, always make sure that for 32-bit guests,
> + *	we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
> + */
> +ENTRY(__kvm_enable_fpexc32)
> +	mov	x3, #(1 << 30)
> +	msr	fpexc32_el2, x3
> +	ret
> +ENDPROC(__kvm_enable_fpexc32)
> +
> +/**
> + * void __kvm_save_fpexc32(void) -
> + *	This function restores guest FPEXC to its vcpu context, we call this
> + *	function from vcpu_put.
> + */
> +ENTRY(__kvm_save_fpexc32)
> +	kern_hyp_va x0
> +	add     x2, x0, #VCPU_CONTEXT
> +	mrs     x1, fpexc32_el2
> +	str     x1, [x2, #CPU_SYSREG_OFFSET(FPEXC32_EL2)]
> +	ret
> +ENDPROC(__kvm_save_fpexc32)
> +
>  __kvm_hyp_panic:
>  	// Guess the context by looking at VTTBR:
>  	// If zero, then we're already a host.
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v4 1/3] KVM/arm/arm64: add hooks for armv7 fp/simd lazy switch support

[Mario Smarduch]

On 12/3/2015 7:46 AM, Marc Zyngier wrote:
> On 14/11/15 22:12, Mario Smarduch wrote:
>> This patch adds vcpu fields to track lazy state, save host FPEXC, and
>> offsets to fields.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  arch/arm/include/asm/kvm_host.h | 6 ++++++
>>  arch/arm/kernel/asm-offsets.c   | 2 ++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 3df1e97..f1bf551 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -107,6 +107,12 @@ struct kvm_vcpu_arch {
>>  	/* Interrupt related fields */
>>  	u32 irq_lines;		/* IRQ and FIQ levels */
>>  
>> +	/* fp/simd dirty flag true if guest accessed register file */
>> +	bool    vfp_dirty;
> 
> I think we do not need this bool, because it is already represented by
> the state of the trapping bits.

The trapping bit state is lost on exit since they're cleared, no?

> 
>> +
>> +	/* Save host FPEXC register to later restore on vcpu put */
>> +	u32	host_fpexc;
>> +
>>  	/* Exception Information */
>>  	struct kvm_vcpu_fault_info fault;
>>  
>> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
>> index 871b826..9f79712 100644
>> --- a/arch/arm/kernel/asm-offsets.c
>> +++ b/arch/arm/kernel/asm-offsets.c
>> @@ -186,6 +186,8 @@ int main(void)
>>    DEFINE(VCPU_CPSR,		offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_cpsr));
>>    DEFINE(VCPU_HCR,		offsetof(struct kvm_vcpu, arch.hcr));
>>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
>> +  DEFINE(VCPU_VFP_DIRTY,	offsetof(struct kvm_vcpu, arch.vfp_dirty));
>> +  DEFINE(VCPU_VFP_HOST_FPEXC,	offsetof(struct kvm_vcpu, arch.host_fpexc));
>>    DEFINE(VCPU_HSR,		offsetof(struct kvm_vcpu, arch.fault.hsr));
>>    DEFINE(VCPU_HxFAR,		offsetof(struct kvm_vcpu, arch.fault.hxfar));
>>    DEFINE(VCPU_HPFAR,		offsetof(struct kvm_vcpu, arch.fault.hpfar));
>>
> 
> Thanks,
> 
> 	M.
> 

[PATCH v4 1/3] KVM/arm/arm64: add hooks for armv7 fp/simd lazy switch support

[Marc Zyngier]

On 03/12/15 19:21, Mario Smarduch wrote:
> 
> 
> On 12/3/2015 7:46 AM, Marc Zyngier wrote:
>> On 14/11/15 22:12, Mario Smarduch wrote:
>>> This patch adds vcpu fields to track lazy state, save host FPEXC, and
>>> offsets to fields.
>>>
>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>> ---
>>>  arch/arm/include/asm/kvm_host.h | 6 ++++++
>>>  arch/arm/kernel/asm-offsets.c   | 2 ++
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>> index 3df1e97..f1bf551 100644
>>> --- a/arch/arm/include/asm/kvm_host.h
>>> +++ b/arch/arm/include/asm/kvm_host.h
>>> @@ -107,6 +107,12 @@ struct kvm_vcpu_arch {
>>>  	/* Interrupt related fields */
>>>  	u32 irq_lines;		/* IRQ and FIQ levels */
>>>  
>>> +	/* fp/simd dirty flag true if guest accessed register file */
>>> +	bool    vfp_dirty;
>>
>> I think we do not need this bool, because it is already represented by
>> the state of the trapping bits.
> 
> The trapping bit state is lost on exit since they're cleared, no?

But that's what should actually be preserved, no? At the moment, you
maintain some side state to reflect what the trapping state is. You
might as well keep it around all the time.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v4 1/3] KVM/arm/arm64: add hooks for armv7 fp/simd lazy switch support

[Mario Smarduch]

On 12/3/2015 11:24 AM, Marc Zyngier wrote:
> On 03/12/15 19:21, Mario Smarduch wrote:
>>
>>
>> On 12/3/2015 7:46 AM, Marc Zyngier wrote:
>>> On 14/11/15 22:12, Mario Smarduch wrote:
>>>> This patch adds vcpu fields to track lazy state, save host FPEXC, and
>>>> offsets to fields.
>>>>
>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>> ---
>>>>  arch/arm/include/asm/kvm_host.h | 6 ++++++
>>>>  arch/arm/kernel/asm-offsets.c   | 2 ++
>>>>  2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>> index 3df1e97..f1bf551 100644
>>>> --- a/arch/arm/include/asm/kvm_host.h
>>>> +++ b/arch/arm/include/asm/kvm_host.h
>>>> @@ -107,6 +107,12 @@ struct kvm_vcpu_arch {
>>>>  	/* Interrupt related fields */
>>>>  	u32 irq_lines;		/* IRQ and FIQ levels */
>>>>  
>>>> +	/* fp/simd dirty flag true if guest accessed register file */
>>>> +	bool    vfp_dirty;
>>>
>>> I think we do not need this bool, because it is already represented by
>>> the state of the trapping bits.
>>
>> The trapping bit state is lost on exit since they're cleared, no?
> 
> But that's what should actually be preserved, no? At the moment, you
> maintain some side state to reflect what the trapping state is. You
> might as well keep it around all the time.

Ok I see, you should be able to preserve and use the trap registers. I'll rework
it.

> 
> Thanks,
> 
> 	M.
> 

[PATCH v4 2/3] KVM/arm/arm64: enable enhanced armv7 fp/simd lazy switch

[Mario Smarduch]

On 12/3/2015 7:58 AM, Marc Zyngier wrote:
> On 14/11/15 22:12, Mario Smarduch wrote:
>> This patch tracks armv7 fp/simd hardware state with a vcpu lazy flag.
>> On vcpu_load saves host fpexc and enables FP access, and later enables fp/simd
>> trapping if lazy flag is not set. On first fp/simd access trap to handler 
>> to save host and restore guest context, disable trapping and set vcpu lazy 
>> flag. On vcpu_put if flag is set save guest and restore host context and 
>> always restore host fpexc.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  arch/arm/include/asm/kvm_host.h   | 33 ++++++++++++++++++++++
>>  arch/arm/kvm/arm.c                | 12 ++++++++
>>  arch/arm/kvm/interrupts.S         | 58 +++++++++++++++++++++++----------------
>>  arch/arm/kvm/interrupts_head.S    | 26 +++++++++++++-----
>>  arch/arm64/include/asm/kvm_host.h |  6 ++++
>>  5 files changed, 104 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index f1bf551..8fc7a59 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -40,6 +40,38 @@
>>  
>>  #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
>>  
>> +/*
>> + * Reads the host FPEXC register, saves it to vcpu context and enables the
>> + * FP/SIMD unit.
>> + */
>> +#ifdef CONFIG_VFPv3
>> +#define kvm_enable_fpexc(vcpu) {			\
>> +	u32 fpexc = 0;					\
>> +	asm volatile(					\
>> +	"mrc p10, 7, %0, cr8, cr0, 0\n"			\
>> +	"str %0, [%1]\n"				\
>> +	"orr %0, %0, #(1 << 30)\n"			\
>> +	"mcr p10, 7, %0, cr8, cr0, 0\n"			\
> 
> Don't you need an ISB here? 
Yes it does (B2.7.3) - was thinking something else but the manual is clear here.

> Also, it would be a lot nicer if this was a
> real function (possibly inlined). I don't see any real reason to make
> this a #define.
Had some trouble reconciling arm and arm64 compile making this
a function in kvm_host.h. I'll work to resolve it.

> 
> Also, you're preserving a lot of the host's FPEXC bits. Is that safe?
No it may not be, should just set the enable bit.
> 
>> +	: "+r" (fpexc)					\
>> +	: "r" (&vcpu->arch.host_fpexc)			\
>> +	);						\
>> +}
>> +#else
>> +#define kvm_enable_fpexc(vcpu)
>> +#endif
>> +
>> +/* Restores host FPEXC register */
>> +#ifdef CONFIG_VFPv3
>> +#define kvm_restore_host_fpexc(vcpu) {			\
>> +	asm volatile(					\
>> +		"mcr p10, 7, %0, cr8, cr0, 0\n"		\
>> +		: : "r" (vcpu->arch.host_fpexc)		\
>> +	);						\
>> +}
>> +#else
>> +#define kvm_restore_host_fpexc(vcpu)
>> +#endif
>> +
>>  u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
>>  int __attribute_const__ kvm_target_cpu(void);
>>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
>> @@ -227,6 +259,7 @@ int kvm_perf_teardown(void);
>>  void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>>  
>>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>> +void kvm_restore_host_vfp_state(struct kvm_vcpu *);
>>  
>>  static inline void kvm_arch_hardware_disable(void) {}
>>  static inline void kvm_arch_hardware_unsetup(void) {}
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index dc017ad..cfc348a 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -291,10 +291,22 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  	vcpu->arch.host_cpu_context = this_cpu_ptr(kvm_host_cpu_state);
>>  
>>  	kvm_arm_set_running_vcpu(vcpu);
>> +
>> +	/* Save and enable FPEXC before we load guest context */
>> +	kvm_enable_fpexc(vcpu);
>>  }
>>  
>>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>  {
>> +	/* If the fp/simd registers are dirty save guest, restore host. */
>> +	if (vcpu->arch.vfp_dirty) {
> 
> See my previous comment about the dirty state.
Yes that change seems to be working out fine.
> 
>> +		kvm_restore_host_vfp_state(vcpu);
>> +		vcpu->arch.vfp_dirty = 0;
>> +	}
>> +
>> +	/* Restore host FPEXC trashed in vcpu_load */
>> +	kvm_restore_host_fpexc(vcpu);
>> +
>>  	/*
>>  	 * The arch-generic KVM code expects the cpu field of a vcpu to be -1
>>  	 * if the vcpu is no longer assigned to a cpu.  This is used for the
>> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
>> index 900ef6d..1ddaa89 100644
>> --- a/arch/arm/kvm/interrupts.S
>> +++ b/arch/arm/kvm/interrupts.S
>> @@ -28,6 +28,26 @@
>>  #include "interrupts_head.S"
>>  
>>  	.text
>> +/**
>> + * void kvm_restore_host_vfp_state(struct vcpu *vcpu) -
>> + *	This function is called from host to save the guest, and restore host
>> + *	fp/simd hardware context. It's placed outside of hyp start/end region.
>> + */
>> +ENTRY(kvm_restore_host_vfp_state)
>> +#ifdef CONFIG_VFPv3
>> +	push    {r4-r7}
>> +
>> +	add     r7, vcpu, #VCPU_VFP_GUEST
>> +	store_vfp_state r7
>> +
>> +	add     r7, vcpu, #VCPU_VFP_HOST
>> +	ldr     r7, [r7]
>> +	restore_vfp_state r7
>> +
>> +	pop     {r4-r7}
>> +#endif
>> +	bx      lr
>> +ENDPROC(kvm_restore_host_vfp_state)
>>  
> 
> Please don't mix things that are part of the HYP code and things that
> are not in the same file. This is just asking for trouble. If that's not
> mapped into HYP, put it in a separate file.
Will do.
> 
>>  __kvm_hyp_code_start:
>>  	.globl __kvm_hyp_code_start
>> @@ -116,22 +136,22 @@ ENTRY(__kvm_vcpu_run)
>>  	read_cp15_state store_to_vcpu = 0
>>  	write_cp15_state read_from_vcpu = 1
>>  
>> +	set_hcptr_bits set, r4, (HCPTR_TTA)
>>  	@ If the host kernel has not been configured with VFPv3 support,
>>  	@ then it is safer if we deny guests from using it as well.
>>  #ifdef CONFIG_VFPv3
>> -	@ Set FPEXC_EN so the guest doesn't trap floating point instructions
>> -	VFPFMRX r2, FPEXC		@ VMRS
>> -	push	{r2}
>> -	orr	r2, r2, #FPEXC_EN
>> -	VFPFMXR FPEXC, r2		@ VMSR
>> +	@ fp/simd register file has already been accessed, so skip trap enable.
>> +	vfp_skip_if_dirty r7, skip_guest_vfp_trap
>> +	set_hcptr_bits orr, r4, (HCPTR_TCP(10) | HCPTR_TCP(11))
>> +skip_guest_vfp_trap:
>>  #endif
>> +	set_hcptr vmentry, r4
>>  
>>  	@ Configure Hyp-role
>>  	configure_hyp_role vmentry
>>  
>>  	@ Trap coprocessor CRx accesses
>>  	set_hstr vmentry
>> -	set_hcptr vmentry, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
>>  	set_hdcr vmentry
>>  
>>  	@ Write configured ID register into MIDR alias
>> @@ -170,23 +190,8 @@ __kvm_vcpu_return:
>>  	@ Don't trap coprocessor accesses for host kernel
>>  	set_hstr vmexit
>>  	set_hdcr vmexit
>> -	set_hcptr vmexit, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11)), after_vfp_restore
>> -
>> -#ifdef CONFIG_VFPv3
>> -	@ Switch VFP/NEON hardware state to the host's
>> -	add	r7, vcpu, #VCPU_VFP_GUEST
>> -	store_vfp_state r7
>> -	add	r7, vcpu, #VCPU_VFP_HOST
>> -	ldr	r7, [r7]
>> -	restore_vfp_state r7
>> -
>> -after_vfp_restore:
>> -	@ Restore FPEXC_EN which we clobbered on entry
>> -	pop	{r2}
>> -	VFPFMXR FPEXC, r2
>> -#else
>> -after_vfp_restore:
>> -#endif
>> +	set_hcptr_bits clear, r4, (HCPTR_TTA | HCPTR_TCP(10) | HCPTR_TCP(11))
>> +	set_hcptr vmexit, r4
>>  
>>  	@ Reset Hyp-role
>>  	configure_hyp_role vmexit
>> @@ -483,7 +488,12 @@ switch_to_guest_vfp:
>>  	push	{r3-r7}
>>  
>>  	@ NEON/VFP used.  Turn on VFP access.
>> -	set_hcptr vmtrap, (HCPTR_TCP(10) | HCPTR_TCP(11))
>> +	set_hcptr_bits clear, r4, (HCPTR_TCP(10) | HCPTR_TCP(11))
>> +	set_hcptr vmtrap, r4
>> +
>> +	@ set lazy mode flag, switch hardware context on vcpu_put
>> +	mov     r1, #1
>> +	strb    r1, [vcpu, #VCPU_VFP_DIRTY]
> 
> You are assuming that a boolean is a byte. That's wrong, and not endian
> safe. If you want to have such a flag in a structure, use a sized type
> (u8, u16, u32). But again, I think we should rely on the trap flags to
> make decisions outside of the HYP code.
Although this is gone now, thanks for the tip very helpful.
> 
> Thanks,
> 
> 	M.
> 

[PATCH v4 3/3] KVM/arm/arm64: enable enhanced armv8 fp/simd lazy switch

[Mario Smarduch]

On 12/3/2015 8:13 AM, Marc Zyngier wrote:
> On 14/11/15 22:12, Mario Smarduch wrote:
>> This patch tracks armv7 and armv8 fp/simd hardware state with a vcpu lazy flag.
>> On vcpu_load for 32 bit guests enable FP access, and later enable fp/simd
>> trapping for 32 and 64 bit guests if lazy flag is not set. On first fp/simd 
>> access trap to handler to save host and restore guest context, disable 
>> trapping and set vcpu lazy flag. On vcpu_put if flag is set save guest and 
>> restore host context and also save guest fpexc register.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  3 ++
>>  arch/arm/kvm/arm.c                | 18 +++++++--
>>  arch/arm64/include/asm/kvm_asm.h  |  2 +
>>  arch/arm64/include/asm/kvm_host.h | 17 +++++++-
>>  arch/arm64/kernel/asm-offsets.c   |  1 +
>>  arch/arm64/kvm/hyp.S              | 83 +++++++++++++++++++++++++--------------
>>  6 files changed, 89 insertions(+), 35 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 8fc7a59..6960ff2 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -40,6 +40,8 @@
>>  
>>  #define KVM_MAX_VCPUS VGIC_V2_MAX_CPUS
>>  
>> +#define kvm_guest_is32bit(vcpu)	true
> 
> This should be defined as an inline function, and placed in
> asm/kvm_emulate.h, probably renamed as kvm_guest_vcpu_is_32bit.

Will do, this header file should also resolve my problems in armv7.
> 
>> +
>>  /*
>>   * Reads the host FPEXC register, saves to vcpu context and enables the
>>   * FPEXC.
>> @@ -260,6 +262,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>>  
>>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>>  void kvm_restore_host_vfp_state(struct kvm_vcpu *);
>> +static inline void kvm_save_guest_fpexc(struct kvm_vcpu *vcpu) {}
>>  
>>  static inline void kvm_arch_hardware_disable(void) {}
>>  static inline void kvm_arch_hardware_unsetup(void) {}
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index cfc348a..7a20530 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -292,8 +292,12 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  
>>  	kvm_arm_set_running_vcpu(vcpu);
>>  
>> -	/* Save and enable FPEXC before we load guest context */
>> -	kvm_enable_fpexc(vcpu);
>> +	/*
>> +	 * For 32bit guest executing on arm64, enable fp/simd access in
>> +	 * EL2. On arm32 save host fpexc and then enable fp/simd access.
>> +	 */
>> +	if (kvm_guest_is32bit(vcpu))
>> +		kvm_enable_fpexc(vcpu);
>>  }
>>  
>>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> @@ -301,10 +305,18 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>  	/* If the fp/simd registers are dirty save guest, restore host. */
>>  	if (vcpu->arch.vfp_dirty) {
>>  		kvm_restore_host_vfp_state(vcpu);
>> +
>> +		/*
>> +		 * For 32bit guest on arm64 save the guest fpexc register
>> +		 * in EL2 mode.
>> +		 */
>> +		if (kvm_guest_is32bit(vcpu))
>> +			kvm_save_guest_fpexc(vcpu);
>> +
>>  		vcpu->arch.vfp_dirty = 0;
>>  	}
>>  
>> -	/* Restore host FPEXC trashed in vcpu_load */
>> +	/* For arm32 restore host FPEXC trashed in vcpu_load. */
>>  	kvm_restore_host_fpexc(vcpu);
>>  
>>  	/*
>> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
>> index 5e37710..c589ca9 100644
>> --- a/arch/arm64/include/asm/kvm_asm.h
>> +++ b/arch/arm64/include/asm/kvm_asm.h
>> @@ -117,6 +117,8 @@ extern char __kvm_hyp_vector[];
>>  extern void __kvm_flush_vm_context(void);
>>  extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
>>  extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
>> +extern void __kvm_enable_fpexc32(void);
>> +extern void __kvm_save_fpexc32(struct kvm_vcpu *vcpu);
>>  
>>  extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
>>  
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 83e65dd..6e2d6b5 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -41,6 +41,8 @@
>>  
>>  #define KVM_VCPU_MAX_FEATURES 3
>>  
>> +#define kvm_guest_is32bit(vcpu)	(!(vcpu->arch.hcr_el2 & HCR_RW))
>> +
>>  int __attribute_const__ kvm_target_cpu(void);
>>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
>>  int kvm_arch_dev_ioctl_check_extension(long ext);
>> @@ -251,9 +253,20 @@ static inline void kvm_arch_hardware_unsetup(void) {}
>>  static inline void kvm_arch_sync_events(struct kvm *kvm) {}
>>  static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
>>  static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
>> -static inline void kvm_enable_fpexc(struct kvm_vcpu *vcpu) {}
>> -static inline void kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu) {}
>> +
>> +static inline void kvm_enable_fpexc(struct kvm_vcpu *vcpu)
>> +{
>> +	/* Enable FP/SIMD access from EL2 mode*/
>> +	kvm_call_hyp(__kvm_enable_fpexc32);
>> +}
>> +
>> +static inline void kvm_save_guest_fpexc(struct kvm_vcpu *vcpu)
>> +{
>> +	/* Save FPEXEC32_EL2 in EL2 mode */
>> +	kvm_call_hyp(__kvm_save_fpexc32, vcpu);
>> +}
>>  static inline void kvm_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
>> +void kvm_restore_host_vfp_state(struct kvm_vcpu *vcpu);
>>  
>>  void kvm_arm_init_debug(void);
>>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index 8d89cf8..c9c5242 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -124,6 +124,7 @@ int main(void)
>>    DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
>>    DEFINE(VCPU_MDCR_EL2,	offsetof(struct kvm_vcpu, arch.mdcr_el2));
>>    DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
>> +  DEFINE(VCPU_VFP_DIRTY,	offsetof(struct kvm_vcpu, arch.vfp_dirty));
>>    DEFINE(VCPU_HOST_CONTEXT,	offsetof(struct kvm_vcpu, arch.host_cpu_context));
>>    DEFINE(VCPU_HOST_DEBUG_STATE, offsetof(struct kvm_vcpu, arch.host_debug_state));
>>    DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index e583613..06efb93 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -36,6 +36,23 @@
>>  #define CPU_SYSREG_OFFSET(x)	(CPU_SYSREGS + 8*x)
>>  
>>  	.text
>> +/**
>> + * void kvm_restore_host_vfp_state(struct vcpu *vcpu) -
>> + *	This function saves the guest, restores host, called from host.
>> + */
>> +ENTRY(kvm_restore_host_vfp_state)
>> +	push	xzr, lr
>> +
>> +	add	x2, x0, #VCPU_CONTEXT
>> +	bl __save_fpsimd
>> +
>> +	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
>> +	bl __restore_fpsimd
>> +
>> +	pop	xzr, lr
>> +	ret
>> +ENDPROC(kvm_restore_host_vfp_state)
>> +
> 
> Same here. Don't mix HYP and non-HYP in the same file.
Yep got it.

Thnaks.
> 
>>  	.pushsection	.hyp.text, "ax"
>>  	.align	PAGE_SHIFT
>>  
>> @@ -385,14 +402,6 @@
>>  	tbz	\tmp, #KVM_ARM64_DEBUG_DIRTY_SHIFT, \target
>>  .endm
>>  
>> -/*
>> - * Branch to target if CPTR_EL2.TFP bit is set (VFP/SIMD trapping enabled)
>> - */
>> -.macro skip_fpsimd_state tmp, target
>> -	mrs	\tmp, cptr_el2
>> -	tbnz	\tmp, #CPTR_EL2_TFP_SHIFT, \target
>> -.endm
>> -
>>  .macro compute_debug_state target
>>  	// Compute debug state: If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY
>>  	// is set, we do a full save/restore cycle and disable trapping.
>> @@ -433,10 +442,6 @@
>>  	mrs	x5, ifsr32_el2
>>  	stp	x4, x5, [x3]
>>  
>> -	skip_fpsimd_state x8, 2f
>> -	mrs	x6, fpexc32_el2
>> -	str	x6, [x3, #16]
>> -2:
>>  	skip_debug_state x8, 1f
>>  	mrs	x7, dbgvcr32_el2
>>  	str	x7, [x3, #24]
>> @@ -468,21 +473,13 @@
>>  .macro activate_traps
>>  	ldr     x2, [x0, #VCPU_HCR_EL2]
>>  
>> -	/*
>> -	 * We are about to set CPTR_EL2.TFP to trap all floating point
>> -	 * register accesses to EL2, however, the ARM ARM clearly states that
>> -	 * traps are only taken to EL2 if the operation would not otherwise
>> -	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
>> -	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
>> -	 */
>> -	tbnz	x2, #HCR_RW_SHIFT, 99f // open code skip_32bit_state
>> -	mov	x3, #(1 << 30)
>> -	msr	fpexc32_el2, x3
>> -	isb
>> -99:
>>  	msr     hcr_el2, x2
>>  	mov	x2, #CPTR_EL2_TTA
>> +
>> +	ldrb    w3, [x0, #VCPU_VFP_DIRTY]
>> +	tbnz    w3, #0, 98f
>>  	orr     x2, x2, #CPTR_EL2_TFP
>> +98:
>>  	msr	cptr_el2, x2
>>  
>>  	mov	x2, #(1 << 15)	// Trap CP15 Cr=15
>> @@ -669,14 +666,12 @@ __restore_debug:
>>  	ret
>>  
>>  __save_fpsimd:
>> -	skip_fpsimd_state x3, 1f
>>  	save_fpsimd
>> -1:	ret
>> +	ret
>>  
>>  __restore_fpsimd:
>> -	skip_fpsimd_state x3, 1f
>>  	restore_fpsimd
>> -1:	ret
>> +	ret
>>  
>>  switch_to_guest_fpsimd:
>>  	push	x4, lr
>> @@ -688,6 +683,9 @@ switch_to_guest_fpsimd:
>>  
>>  	mrs	x0, tpidr_el2
>>  
>> +	mov	w2, #1
>> +	strb    w2, [x0, #VCPU_VFP_DIRTY]
>> +
>>  	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
>>  	kern_hyp_va x2
>>  	bl __save_fpsimd
>> @@ -763,7 +761,6 @@ __kvm_vcpu_return:
>>  	add	x2, x0, #VCPU_CONTEXT
>>  
>>  	save_guest_regs
>> -	bl __save_fpsimd
>>  	bl __save_sysregs
>>  
>>  	skip_debug_state x3, 1f
>> @@ -784,7 +781,6 @@ __kvm_vcpu_return:
>>  	kern_hyp_va x2
>>  
>>  	bl __restore_sysregs
>> -	bl __restore_fpsimd
>>  	/* Clear FPSIMD and Trace trapping */
>>  	msr     cptr_el2, xzr
>>  
>> @@ -863,6 +859,33 @@ ENTRY(__kvm_flush_vm_context)
>>  	ret
>>  ENDPROC(__kvm_flush_vm_context)
>>  
>> +/**
>> + * void __kvm_enable_fpexc32(void) -
>> + *	We may be entering the guest and set CPTR_EL2.TFP to trap all floating
>> + *	point register accesses to EL2, however, the ARM manual clearly states
>> + *	that traps are only taken to EL2 if the operation would not otherwise
>> + *	trap to EL1.  Therefore, always make sure that for 32-bit guests,
>> + *	we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
>> + */
>> +ENTRY(__kvm_enable_fpexc32)
>> +	mov	x3, #(1 << 30)
>> +	msr	fpexc32_el2, x3
>> +	ret
>> +ENDPROC(__kvm_enable_fpexc32)
>> +
>> +/**
>> + * void __kvm_save_fpexc32(void) -
>> + *	This function restores guest FPEXC to its vcpu context, we call this
>> + *	function from vcpu_put.
>> + */
>> +ENTRY(__kvm_save_fpexc32)
>> +	kern_hyp_va x0
>> +	add     x2, x0, #VCPU_CONTEXT
>> +	mrs     x1, fpexc32_el2
>> +	str     x1, [x2, #CPU_SYSREG_OFFSET(FPEXC32_EL2)]
>> +	ret
>> +ENDPROC(__kvm_save_fpexc32)
>> +
>>  __kvm_hyp_panic:
>>  	// Guess the context by looking at VTTBR:
>>  	// If zero, then we're already a host.
>>
> 
> Thanks,
> 
> 	M.
>