All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/6] arm/arm64: KVM: Enhance armv7/8 fp/simd lazy switch
@ 2015-12-26 21:54 ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: kvmarm, christoffer.dall, marc.zyngier
  Cc: kvm, linux-arm-kernel, Mario Smarduch

Current lazy fp/simd implementation switches hardware context on guest access 
and again on exit to host, otherwise context switch is skipped. This patch 
set builds on that functionality and executes a hardware context switch on 
first time access and when vCPU is scheduled out or returns to user space (on 
vcpu_put).

For an FP and lmbench load it reduces fp/simd context switch from 30-50% down
to near 0%. Results will vary with load but is no worse then current
approach.

Running floating point application on nearly idle system:
./tst-float 100000uS - (sleep for .1s) fp/simd switch reduced by 99%+
./tst-float 10000uS -  (sleep for 10 ms)              reduced by 98%+
./tst-float 1000uS -   (sleep for 1ms)                reduced by ~98%
...
./tst-float 1uS -                                     reduced by  2%+

Tested on Juno, Foundation Model, and Fast Models. 

Test Details:
-------------
armv7 - with CONFIG_VFP, CONFIG_NEON, CONFIG_KERNEL_MODE_NEON options enabled:

- On host executed 12 fp applications - with ranging sleep intervals
- Two guests - with 12 fp processes  - with ranging sleep intervals

armv8 
- Similar to armv7, with mix of 32 and 64 bit guests - on Juno ran 2-32bit and
  2-64 bit 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 v5->v6:
- Followed up on Christoffers comments
  o armv7 - replaced fp/simd asm with supported function calls 
  o armv7 - save hcptr once on access instead of every exit
  o armv7 - removed hcptr macro
  o armv7 - fixed twisted boolean return logic
  o armv7 - removed isb after setting fpexec32 since its followed with a 
            hyp call
  o armv8 - rebased to 4.4-rc5 - wsinc
  o armv8 - as with hpctpr save cptr_el2 on access instead of every exit
  o armv7/armv8 - restructured patch series to simplify review 

Chances since v4->v5:
- Followed up on Marcs comments
  - Removed dirty flag, and used trap bits to check for dirty fp/simd
  - Seperated host form hyp code
  - As a consequence for arm64 added a commend assember header file
  - Fixed up critical accesses to fpexec, and added isb
  - Converted defines to inline functions

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 (6):
  Introduce armv7 fp/simd vcpu fields and helpers
  Introduce host fp/simd context switch function
  Enable armv7 fp/simd enhanced context switch
  Deleted unused macros
  Introduce armv8 fp/simd vcpu fields and helpers
  Enable armv8 fp/simd enhanced context switch

 arch/arm/include/asm/kvm_emulate.h   | 54 ++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/kvm_host.h      |  8 ++++++
 arch/arm/kernel/asm-offsets.c        |  1 +
 arch/arm/kvm/Makefile                |  2 +-
 arch/arm/kvm/arm.c                   | 19 +++++++++++++
 arch/arm/kvm/fpsimd_switch.S         | 47 +++++++++++++++++++++++++++++++
 arch/arm/kvm/interrupts.S            | 43 ++++++++++------------------
 arch/arm/kvm/interrupts_head.S       | 29 -------------------
 arch/arm64/include/asm/kvm_asm.h     |  5 ++++
 arch/arm64/include/asm/kvm_emulate.h | 30 ++++++++++++++++++++
 arch/arm64/include/asm/kvm_host.h    | 12 ++++++++
 arch/arm64/kernel/asm-offsets.c      |  1 +
 arch/arm64/kvm/hyp/entry.S           |  1 +
 arch/arm64/kvm/hyp/hyp-entry.S       | 26 +++++++++++++++++
 arch/arm64/kvm/hyp/switch.c          | 26 ++---------------
 15 files changed, 222 insertions(+), 82 deletions(-)
 create mode 100644 arch/arm/kvm/fpsimd_switch.S

-- 
1.9.1


^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 0/6] arm/arm64: KVM: Enhance armv7/8 fp/simd lazy switch
@ 2015-12-26 21:54 ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: linux-arm-kernel

Current lazy fp/simd implementation switches hardware context on guest access 
and again on exit to host, otherwise context switch is skipped. This patch 
set builds on that functionality and executes a hardware context switch on 
first time access and when vCPU is scheduled out or returns to user space (on 
vcpu_put).

For an FP and lmbench load it reduces fp/simd context switch from 30-50% down
to near 0%. Results will vary with load but is no worse then current
approach.

Running floating point application on nearly idle system:
./tst-float 100000uS - (sleep for .1s) fp/simd switch reduced by 99%+
./tst-float 10000uS -  (sleep for 10 ms)              reduced by 98%+
./tst-float 1000uS -   (sleep for 1ms)                reduced by ~98%
...
./tst-float 1uS -                                     reduced by  2%+

Tested on Juno, Foundation Model, and Fast Models. 

Test Details:
-------------
armv7 - with CONFIG_VFP, CONFIG_NEON, CONFIG_KERNEL_MODE_NEON options enabled:

- On host executed 12 fp applications - with ranging sleep intervals
- Two guests - with 12 fp processes  - with ranging sleep intervals

armv8 
- Similar to armv7, with mix of 32 and 64 bit guests - on Juno ran 2-32bit and
  2-64 bit 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 v5->v6:
- Followed up on Christoffers comments
  o armv7 - replaced fp/simd asm with supported function calls 
  o armv7 - save hcptr once on access instead of every exit
  o armv7 - removed hcptr macro
  o armv7 - fixed twisted boolean return logic
  o armv7 - removed isb after setting fpexec32 since its followed with a 
            hyp call
  o armv8 - rebased to 4.4-rc5 - wsinc
  o armv8 - as with hpctpr save cptr_el2 on access instead of every exit
  o armv7/armv8 - restructured patch series to simplify review 

Chances since v4->v5:
- Followed up on Marcs comments
  - Removed dirty flag, and used trap bits to check for dirty fp/simd
  - Seperated host form hyp code
  - As a consequence for arm64 added a commend assember header file
  - Fixed up critical accesses to fpexec, and added isb
  - Converted defines to inline functions

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 (6):
  Introduce armv7 fp/simd vcpu fields and helpers
  Introduce host fp/simd context switch function
  Enable armv7 fp/simd enhanced context switch
  Deleted unused macros
  Introduce armv8 fp/simd vcpu fields and helpers
  Enable armv8 fp/simd enhanced context switch

 arch/arm/include/asm/kvm_emulate.h   | 54 ++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/kvm_host.h      |  8 ++++++
 arch/arm/kernel/asm-offsets.c        |  1 +
 arch/arm/kvm/Makefile                |  2 +-
 arch/arm/kvm/arm.c                   | 19 +++++++++++++
 arch/arm/kvm/fpsimd_switch.S         | 47 +++++++++++++++++++++++++++++++
 arch/arm/kvm/interrupts.S            | 43 ++++++++++------------------
 arch/arm/kvm/interrupts_head.S       | 29 -------------------
 arch/arm64/include/asm/kvm_asm.h     |  5 ++++
 arch/arm64/include/asm/kvm_emulate.h | 30 ++++++++++++++++++++
 arch/arm64/include/asm/kvm_host.h    | 12 ++++++++
 arch/arm64/kernel/asm-offsets.c      |  1 +
 arch/arm64/kvm/hyp/entry.S           |  1 +
 arch/arm64/kvm/hyp/hyp-entry.S       | 26 +++++++++++++++++
 arch/arm64/kvm/hyp/switch.c          | 26 ++---------------
 15 files changed, 222 insertions(+), 82 deletions(-)
 create mode 100644 arch/arm/kvm/fpsimd_switch.S

-- 
1.9.1

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2015-12-26 21:54 ` Mario Smarduch
@ 2015-12-26 21:54   ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: kvmarm, christoffer.dall, marc.zyngier; +Cc: linux-arm-kernel, kvm

Add helper functions to enable access to fp/smid on guest entry and save host
fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
fields.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/kvm_host.h      |  6 ++++++
 arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
 3 files changed, 56 insertions(+)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 3095df0..d4d9da1 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -24,6 +24,8 @@
 #include <asm/kvm_mmio.h>
 #include <asm/kvm_arm.h>
 #include <asm/cputype.h>
+#include <asm/vfp.h>
+#include "../vfp/vfpinstr.h"
 
 unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
 unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
@@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
 	}
 }
 
+#ifdef CONFIG_VFPv3
+/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
+static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
+{
+	u32 fpexc;
+
+	/* Save host fpexc, and enable guest access to fp unit */
+	fpexc = fmrx(FPEXC);
+	vcpu->arch.host_fpexc = fpexc;
+	fpexc |= FPEXC_EN;
+	fmxr(FPEXC, fpexc);
+
+	/* Configure HCPTR to trap on tracing and fp/simd access */
+	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
+}
+
+/* Called from vcpu_put - restore host fpexc */
+static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
+{
+	fmxr(FPEXC, vcpu->arch.host_fpexc);
+}
+
+/* If trap bits are reset then fp/simd registers are dirty */
+static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
+{
+	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
+}
+#else
+static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcptr = HCPTR_TTA;
+}
+
+static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
+static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+#endif
+
 #endif /* __ARM_KVM_EMULATE_H__ */
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index f9f2779..d3ef58a 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -105,6 +105,12 @@ struct kvm_vcpu_arch {
 	/* HYP trapping configuration */
 	u32 hcr;
 
+	/* HYP Co-processor fp/simd and trace trapping configuration */
+	u32 hcptr;
+
+	/* Save host FPEXC register to later restore on vcpu put */
+	u32 host_fpexc;
+
 	/* Interrupt related fields */
 	u32 irq_lines;		/* IRQ and FIQ levels */
 
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 3066328..ffe8ccf 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -299,4 +299,12 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
 	return data;		/* Leave LE untouched */
 }
 
+static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu) {}
+static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
+
+static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+
 #endif /* __ARM64_KVM_EMULATE_H__ */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2015-12-26 21:54   ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: linux-arm-kernel

Add helper functions to enable access to fp/smid on guest entry and save host
fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
fields.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/kvm_host.h      |  6 ++++++
 arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
 3 files changed, 56 insertions(+)

diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 3095df0..d4d9da1 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -24,6 +24,8 @@
 #include <asm/kvm_mmio.h>
 #include <asm/kvm_arm.h>
 #include <asm/cputype.h>
+#include <asm/vfp.h>
+#include "../vfp/vfpinstr.h"
 
 unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
 unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
@@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
 	}
 }
 
+#ifdef CONFIG_VFPv3
+/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
+static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
+{
+	u32 fpexc;
+
+	/* Save host fpexc, and enable guest access to fp unit */
+	fpexc = fmrx(FPEXC);
+	vcpu->arch.host_fpexc = fpexc;
+	fpexc |= FPEXC_EN;
+	fmxr(FPEXC, fpexc);
+
+	/* Configure HCPTR to trap on tracing and fp/simd access */
+	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
+}
+
+/* Called from vcpu_put - restore host fpexc */
+static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
+{
+	fmxr(FPEXC, vcpu->arch.host_fpexc);
+}
+
+/* If trap bits are reset then fp/simd registers are dirty */
+static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
+{
+	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
+}
+#else
+static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcptr = HCPTR_TTA;
+}
+
+static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
+static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+#endif
+
 #endif /* __ARM_KVM_EMULATE_H__ */
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index f9f2779..d3ef58a 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -105,6 +105,12 @@ struct kvm_vcpu_arch {
 	/* HYP trapping configuration */
 	u32 hcr;
 
+	/* HYP Co-processor fp/simd and trace trapping configuration */
+	u32 hcptr;
+
+	/* Save host FPEXC register to later restore on vcpu put */
+	u32 host_fpexc;
+
 	/* Interrupt related fields */
 	u32 irq_lines;		/* IRQ and FIQ levels */
 
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 3066328..ffe8ccf 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -299,4 +299,12 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
 	return data;		/* Leave LE untouched */
 }
 
+static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu) {}
+static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
+
+static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+
 #endif /* __ARM64_KVM_EMULATE_H__ */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 46+ messages in thread
* [PATCH v6 2/6] arm: KVM: Introduce host fp/simd context switch function
  2015-12-26 21:54 ` Mario Smarduch
@ 2015-12-26 21:54   ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: kvmarm, christoffer.dall, marc.zyngier; +Cc: linux-arm-kernel, kvm

Add fp/simd context switch function callable from host kernel mode.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/kvm/Makefile        |  2 +-
 arch/arm/kvm/fpsimd_switch.S | 47 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/kvm/fpsimd_switch.S

diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index c5eef02c..411b3e4 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -19,7 +19,7 @@ kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o $(KVM)/vf
 
 obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o
-obj-y += coproc.o coproc_a15.o coproc_a7.o mmio.o psci.o perf.o
+obj-y += coproc.o coproc_a15.o coproc_a7.o mmio.o psci.o perf.o fpsimd_switch.o
 obj-y += $(KVM)/arm/vgic.o
 obj-y += $(KVM)/arm/vgic-v2.o
 obj-y += $(KVM)/arm/vgic-v2-emul.o
diff --git a/arch/arm/kvm/fpsimd_switch.S b/arch/arm/kvm/fpsimd_switch.S
new file mode 100644
index 0000000..7e48c16
--- /dev/null
+++ b/arch/arm/kvm/fpsimd_switch.S
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2015 - Samsung - Open Source Group
+ * Author: Mario Smarduch <m.smarduch@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/linkage.h>
+#include <linux/const.h>
+#include <asm/unified.h>
+#include <asm/page.h>
+#include <asm/ptrace.h>
+#include <asm/asm-offsets.h>
+#include <asm/kvm_asm.h>
+#include <asm/kvm_arm.h>
+#include <asm/vfpmacros.h>
+#include "interrupts_head.S"
+
+	.text
+/**
+  * void vcpu_restore_host_vfp_state(struct vcpu *vcpu) -
+  * 	This function is called from host to save the guest, and restore host
+  *     fp/simd hardware context.
+  */
+ENTRY(vcpu_restore_host_vfp_state)
+#ifdef CONFIG_VFPv3
+	push	{r4-r7}
+
+	add	r7, r0, #VCPU_VFP_GUEST
+	store_vfp_state r7
+
+	add	r7, r0, #VCPU_VFP_HOST
+	ldr	r7, [r7]
+	restore_vfp_state r7
+
+	pop	{r4-r7}
+#endif
+	bx	lr
+ENDPROC(vcpu_restore_host_vfp_state)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 46+ messages in thread
* [PATCH v6 2/6] arm: KVM: Introduce host fp/simd context switch function
@ 2015-12-26 21:54   ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: linux-arm-kernel

Add fp/simd context switch function callable from host kernel mode.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/kvm/Makefile        |  2 +-
 arch/arm/kvm/fpsimd_switch.S | 47 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/kvm/fpsimd_switch.S

diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index c5eef02c..411b3e4 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -19,7 +19,7 @@ kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o $(KVM)/vf
 
 obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o
-obj-y += coproc.o coproc_a15.o coproc_a7.o mmio.o psci.o perf.o
+obj-y += coproc.o coproc_a15.o coproc_a7.o mmio.o psci.o perf.o fpsimd_switch.o
 obj-y += $(KVM)/arm/vgic.o
 obj-y += $(KVM)/arm/vgic-v2.o
 obj-y += $(KVM)/arm/vgic-v2-emul.o
diff --git a/arch/arm/kvm/fpsimd_switch.S b/arch/arm/kvm/fpsimd_switch.S
new file mode 100644
index 0000000..7e48c16
--- /dev/null
+++ b/arch/arm/kvm/fpsimd_switch.S
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2015 - Samsung - Open Source Group
+ * Author: Mario Smarduch <m.smarduch@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/linkage.h>
+#include <linux/const.h>
+#include <asm/unified.h>
+#include <asm/page.h>
+#include <asm/ptrace.h>
+#include <asm/asm-offsets.h>
+#include <asm/kvm_asm.h>
+#include <asm/kvm_arm.h>
+#include <asm/vfpmacros.h>
+#include "interrupts_head.S"
+
+	.text
+/**
+  * void vcpu_restore_host_vfp_state(struct vcpu *vcpu) -
+  * 	This function is called from host to save the guest, and restore host
+  *     fp/simd hardware context.
+  */
+ENTRY(vcpu_restore_host_vfp_state)
+#ifdef CONFIG_VFPv3
+	push	{r4-r7}
+
+	add	r7, r0, #VCPU_VFP_GUEST
+	store_vfp_state r7
+
+	add	r7, r0, #VCPU_VFP_HOST
+	ldr	r7, [r7]
+	restore_vfp_state r7
+
+	pop	{r4-r7}
+#endif
+	bx	lr
+ENDPROC(vcpu_restore_host_vfp_state)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 46+ messages in thread
* [PATCH v6 3/6] arm/arm64: KVM: Enable armv7 fp/simd enhanced context switch
  2015-12-26 21:54 ` Mario Smarduch
@ 2015-12-26 21:54   ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: kvmarm, christoffer.dall, marc.zyngier; +Cc: linux-arm-kernel, kvm

Enable armv7 enhanced fp/simd context switch. Guest and host registers are only
context switched on first access and vcpu put. 

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/include/asm/kvm_host.h   |  2 ++
 arch/arm/kernel/asm-offsets.c     |  1 +
 arch/arm/kvm/arm.c                | 10 +++++++++
 arch/arm/kvm/interrupts.S         | 43 ++++++++++++++-------------------------
 arch/arm64/include/asm/kvm_host.h |  2 ++
 5 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index d3ef58a..90f7f59 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -238,6 +238,8 @@ 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 vcpu_restore_host_vfp_state(struct kvm_vcpu *);
+
 static inline void kvm_arch_hardware_disable(void) {}
 static inline void kvm_arch_hardware_unsetup(void) {}
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 871b826..395ecca 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -185,6 +185,7 @@ int main(void)
   DEFINE(VCPU_PC,		offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_pc));
   DEFINE(VCPU_CPSR,		offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_cpsr));
   DEFINE(VCPU_HCR,		offsetof(struct kvm_vcpu, arch.hcr));
+  DEFINE(VCPU_HCPTR,		offsetof(struct kvm_vcpu, arch.hcptr));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
   DEFINE(VCPU_HSR,		offsetof(struct kvm_vcpu, arch.fault.hsr));
   DEFINE(VCPU_HxFAR,		offsetof(struct kvm_vcpu, arch.fault.hxfar));
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index dda1959..b16ed98 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -308,10 +308,20 @@ 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, and enable default traps */
+	vcpu_trap_vfp_enable(vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 {
+	/* If the fp/simd registers are dirty save guest, restore host. */
+	if (vcpu_vfp_isdirty(vcpu))
+		vcpu_restore_host_vfp_state(vcpu);
+
+	/* Restore host FPEXC trashed in vcpu_load */
+	vcpu_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..245c11f 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -116,22 +116,15 @@ ENTRY(__kvm_vcpu_run)
 	read_cp15_state store_to_vcpu = 0
 	write_cp15_state read_from_vcpu = 1
 
-	@ 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
-#endif
+	@ Configure trapping of access to tracing and fp/simd registers
+	ldr r1, [vcpu, #VCPU_HCPTR]
+	mcr p15, 4, r1, c1, c1, 2
 
 	@ 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 +163,10 @@ __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
+	@ Disable trace and fp/simd traps
+	mov r2, #0
+	mcr p15, 4, r2, c1, c1, 2
 
 	@ Reset Hyp-role
 	configure_hyp_role vmexit
@@ -482,8 +462,15 @@ guest_trap:
 switch_to_guest_vfp:
 	push	{r3-r7}
 
-	@ NEON/VFP used.  Turn on VFP access.
-	set_hcptr vmtrap, (HCPTR_TCP(10) | HCPTR_TCP(11))
+	@ fp/simd was accessed, so disable trapping and save hcptr register
+	@ which is used across exits until next vcpu_load.
+	mrc     p15, 4, r2, c1, c1, 2
+	mov     r3, #(HCPTR_TCP(10) | HCPTR_TCP(11))
+	bic     r3, r2, r3
+	mcr     p15, 4, r3, c1, c1, 2
+	str     r3, [vcpu, #VCPU_HCPTR]
+
+	isb
 
 	@ Switch VFP/NEON hardware state to the guest's
 	add	r7, r0, #VCPU_VFP_HOST
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 689d4c9..bfe4d4e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -338,6 +338,8 @@ 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 vcpu_restore_host_vfp_state(struct kvm_vcpu *vcpu) {}
+
 void kvm_arm_init_debug(void);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 46+ messages in thread
* [PATCH v6 3/6] arm/arm64: KVM: Enable armv7 fp/simd enhanced context switch
@ 2015-12-26 21:54   ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: linux-arm-kernel

Enable armv7 enhanced fp/simd context switch. Guest and host registers are only
context switched on first access and vcpu put. 

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/include/asm/kvm_host.h   |  2 ++
 arch/arm/kernel/asm-offsets.c     |  1 +
 arch/arm/kvm/arm.c                | 10 +++++++++
 arch/arm/kvm/interrupts.S         | 43 ++++++++++++++-------------------------
 arch/arm64/include/asm/kvm_host.h |  2 ++
 5 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index d3ef58a..90f7f59 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -238,6 +238,8 @@ 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 vcpu_restore_host_vfp_state(struct kvm_vcpu *);
+
 static inline void kvm_arch_hardware_disable(void) {}
 static inline void kvm_arch_hardware_unsetup(void) {}
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 871b826..395ecca 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -185,6 +185,7 @@ int main(void)
   DEFINE(VCPU_PC,		offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_pc));
   DEFINE(VCPU_CPSR,		offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_cpsr));
   DEFINE(VCPU_HCR,		offsetof(struct kvm_vcpu, arch.hcr));
+  DEFINE(VCPU_HCPTR,		offsetof(struct kvm_vcpu, arch.hcptr));
   DEFINE(VCPU_IRQ_LINES,	offsetof(struct kvm_vcpu, arch.irq_lines));
   DEFINE(VCPU_HSR,		offsetof(struct kvm_vcpu, arch.fault.hsr));
   DEFINE(VCPU_HxFAR,		offsetof(struct kvm_vcpu, arch.fault.hxfar));
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index dda1959..b16ed98 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -308,10 +308,20 @@ 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, and enable default traps */
+	vcpu_trap_vfp_enable(vcpu);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 {
+	/* If the fp/simd registers are dirty save guest, restore host. */
+	if (vcpu_vfp_isdirty(vcpu))
+		vcpu_restore_host_vfp_state(vcpu);
+
+	/* Restore host FPEXC trashed in vcpu_load */
+	vcpu_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..245c11f 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -116,22 +116,15 @@ ENTRY(__kvm_vcpu_run)
 	read_cp15_state store_to_vcpu = 0
 	write_cp15_state read_from_vcpu = 1
 
-	@ 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
-#endif
+	@ Configure trapping of access to tracing and fp/simd registers
+	ldr r1, [vcpu, #VCPU_HCPTR]
+	mcr p15, 4, r1, c1, c1, 2
 
 	@ 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 +163,10 @@ __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
+	@ Disable trace and fp/simd traps
+	mov r2, #0
+	mcr p15, 4, r2, c1, c1, 2
 
 	@ Reset Hyp-role
 	configure_hyp_role vmexit
@@ -482,8 +462,15 @@ guest_trap:
 switch_to_guest_vfp:
 	push	{r3-r7}
 
-	@ NEON/VFP used.  Turn on VFP access.
-	set_hcptr vmtrap, (HCPTR_TCP(10) | HCPTR_TCP(11))
+	@ fp/simd was accessed, so disable trapping and save hcptr register
+	@ which is used across exits until next vcpu_load.
+	mrc     p15, 4, r2, c1, c1, 2
+	mov     r3, #(HCPTR_TCP(10) | HCPTR_TCP(11))
+	bic     r3, r2, r3
+	mcr     p15, 4, r3, c1, c1, 2
+	str     r3, [vcpu, #VCPU_HCPTR]
+
+	isb
 
 	@ Switch VFP/NEON hardware state to the guest's
 	add	r7, r0, #VCPU_VFP_HOST
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 689d4c9..bfe4d4e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -338,6 +338,8 @@ 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 vcpu_restore_host_vfp_state(struct kvm_vcpu *vcpu) {}
+
 void kvm_arm_init_debug(void);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 46+ messages in thread
* [PATCH v6 4/6] arm: KVM: Delete unused macros
  2015-12-26 21:54 ` Mario Smarduch
@ 2015-12-26 21:54   ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: kvmarm, christoffer.dall, marc.zyngier; +Cc: linux-arm-kernel, kvm

set_hcptr is no longer used so delete it.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/kvm/interrupts_head.S | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 51a5950..f4d8311 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -589,35 +589,6 @@ ARM_BE8(rev	r6, r6  )
 	mcr	p15, 4, r2, c1, c1, 3
 .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
-	mrc	p15, 4, r2, c1, c1, 2
-	ldr	r3, =\mask
-	.if \operation == vmentry
-	orr	r3, r2, r3		@ Trap coproc-accesses defined in mask
-	.else
-	bic	r3, r2, r3		@ Don't trap defined coproc-accesses
-	.endif
-	mcr	p15, 4, r3, c1, c1, 2
-	.if \operation != vmentry
-	.if \operation == vmexit
-	tst	r2, #(HCPTR_TCP(10) | HCPTR_TCP(11))
-	beq	1f
-	.endif
-	isb
-	.if \label != none
-	b	\label
-	.endif
-1:
-	.endif
-.endm
-
 /* Configures the HDCR (Hyp Debug Configuration Register) on entry/return
  * (hardware reset value is 0) */
 .macro set_hdcr operation
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 46+ messages in thread
* [PATCH v6 4/6] arm: KVM: Delete unused macros
@ 2015-12-26 21:54   ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2015-12-26 21:54 UTC (permalink / raw)
  To: linux-arm-kernel

set_hcptr is no longer used so delete it.

Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
---
 arch/arm/kvm/interrupts_head.S | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 51a5950..f4d8311 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -589,35 +589,6 @@ ARM_BE8(rev	r6, r6  )
 	mcr	p15, 4, r2, c1, c1, 3
 .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
-	mrc	p15, 4, r2, c1, c1, 2
-	ldr	r3, =\mask
-	.if \operation == vmentry
-	orr	r3, r2, r3		@ Trap coproc-accesses defined in mask
-	.else
-	bic	r3, r2, r3		@ Don't trap defined coproc-accesses
-	.endif
-	mcr	p15, 4, r3, c1, c1, 2
-	.if \operation != vmentry
-	.if \operation == vmexit
-	tst	r2, #(HCPTR_TCP(10) | HCPTR_TCP(11))
-	beq	1f
-	.endif
-	isb
-	.if \label != none
-	b	\label
-	.endif
-1:
-	.endif
-.endm
-
 /* Configures the HDCR (Hyp Debug Configuration Register) on entry/return
  * (hardware reset value is 0) */
 .macro set_hdcr operation
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2015-12-26 21:54   ` Mario Smarduch
@ 2016-01-05 15:00     ` Christoffer Dall
  -1 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-05 15:00 UTC (permalink / raw)
  To: Mario Smarduch; +Cc: kvmarm, marc.zyngier, kvm, linux-arm-kernel

On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> Add helper functions to enable access to fp/smid on guest entry and save host
> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> fields.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 3095df0..d4d9da1 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -24,6 +24,8 @@
>  #include <asm/kvm_mmio.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/cputype.h>
> +#include <asm/vfp.h>
> +#include "../vfp/vfpinstr.h"

this looks dodgy...

can you move vfpinstr.h instead?

>  
>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +#ifdef CONFIG_VFPv3
> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */

the comment is misleading, you're not enabling guest access to the
fp/simd unit, you're just setting the enabled bit to ensure guest
accesses trap.

> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> +{
> +	u32 fpexc;
> +
> +	/* Save host fpexc, and enable guest access to fp unit */
> +	fpexc = fmrx(FPEXC);
> +	vcpu->arch.host_fpexc = fpexc;
> +	fpexc |= FPEXC_EN;
> +	fmxr(FPEXC, fpexc);
> +
> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> +}
> +
> +/* Called from vcpu_put - restore host fpexc */

I would probably get rid of the "Called from" stuff and just describe
what these functions do locally.  Comments like this are likely to be
out of date soon'ish.

> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> +{
> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> +}
> +
> +/* If trap bits are reset then fp/simd registers are dirty */
> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> +{
> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> +}
> +#else
> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.hcptr = HCPTR_TTA;
> +}
> +
> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +#endif

this kind of feels like it belongs in its own C-file instead of a header
file, perhaps arch/arm/kvm/vfp.C.

Marc, what do you think?

> +
>  #endif /* __ARM_KVM_EMULATE_H__ */
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index f9f2779..d3ef58a 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -105,6 +105,12 @@ struct kvm_vcpu_arch {
>  	/* HYP trapping configuration */
>  	u32 hcr;
>  
> +	/* HYP Co-processor fp/simd and trace trapping configuration */
> +	u32 hcptr;
> +
> +	/* Save host FPEXC register to later restore on vcpu put */
> +	u32 host_fpexc;
> +
>  	/* Interrupt related fields */
>  	u32 irq_lines;		/* IRQ and FIQ levels */
>  
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 3066328..ffe8ccf 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -299,4 +299,12 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>  	return data;		/* Leave LE untouched */
>  }
>  
> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu) {}
> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
> +
> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +
>  #endif /* __ARM64_KVM_EMULATE_H__ */
> -- 
> 1.9.1
> 

Thanks,
-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-05 15:00     ` Christoffer Dall
  0 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-05 15:00 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> Add helper functions to enable access to fp/smid on guest entry and save host
> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> fields.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 3095df0..d4d9da1 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -24,6 +24,8 @@
>  #include <asm/kvm_mmio.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/cputype.h>
> +#include <asm/vfp.h>
> +#include "../vfp/vfpinstr.h"

this looks dodgy...

can you move vfpinstr.h instead?

>  
>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +#ifdef CONFIG_VFPv3
> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */

the comment is misleading, you're not enabling guest access to the
fp/simd unit, you're just setting the enabled bit to ensure guest
accesses trap.

> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> +{
> +	u32 fpexc;
> +
> +	/* Save host fpexc, and enable guest access to fp unit */
> +	fpexc = fmrx(FPEXC);
> +	vcpu->arch.host_fpexc = fpexc;
> +	fpexc |= FPEXC_EN;
> +	fmxr(FPEXC, fpexc);
> +
> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> +}
> +
> +/* Called from vcpu_put - restore host fpexc */

I would probably get rid of the "Called from" stuff and just describe
what these functions do locally.  Comments like this are likely to be
out of date soon'ish.

> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> +{
> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> +}
> +
> +/* If trap bits are reset then fp/simd registers are dirty */
> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> +{
> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> +}
> +#else
> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.hcptr = HCPTR_TTA;
> +}
> +
> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +#endif

this kind of feels like it belongs in its own C-file instead of a header
file, perhaps arch/arm/kvm/vfp.C.

Marc, what do you think?

> +
>  #endif /* __ARM_KVM_EMULATE_H__ */
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index f9f2779..d3ef58a 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -105,6 +105,12 @@ struct kvm_vcpu_arch {
>  	/* HYP trapping configuration */
>  	u32 hcr;
>  
> +	/* HYP Co-processor fp/simd and trace trapping configuration */
> +	u32 hcptr;
> +
> +	/* Save host FPEXC register to later restore on vcpu put */
> +	u32 host_fpexc;
> +
>  	/* Interrupt related fields */
>  	u32 irq_lines;		/* IRQ and FIQ levels */
>  
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 3066328..ffe8ccf 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -299,4 +299,12 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>  	return data;		/* Leave LE untouched */
>  }
>  
> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu) {}
> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
> +
> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +
>  #endif /* __ARM64_KVM_EMULATE_H__ */
> -- 
> 1.9.1
> 

Thanks,
-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-05 15:00     ` Christoffer Dall
@ 2016-01-05 19:28       ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-05 19:28 UTC (permalink / raw)
  To: Christoffer Dall; +Cc: marc.zyngier, kvmarm, kvm, linux-arm-kernel



On 1/5/2016 7:00 AM, Christoffer Dall wrote:
> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>> Add helper functions to enable access to fp/smid on guest entry and save host
>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>> fields.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>  3 files changed, 56 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>> index 3095df0..d4d9da1 100644
>> --- a/arch/arm/include/asm/kvm_emulate.h
>> +++ b/arch/arm/include/asm/kvm_emulate.h
>> @@ -24,6 +24,8 @@
>>  #include <asm/kvm_mmio.h>
>>  #include <asm/kvm_arm.h>
>>  #include <asm/cputype.h>
>> +#include <asm/vfp.h>
>> +#include "../vfp/vfpinstr.h"
> 
> this looks dodgy...
> 
> can you move vfpinstr.h instead?
Sure I'll fix it up, it's in couple other places in kernel and kvm
 - copied it.
> 
>>  
>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_VFPv3
>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> 
> the comment is misleading, you're not enabling guest access to the
> fp/simd unit, you're just setting the enabled bit to ensure guest
> accesses trap.

That's more accurate.
> 
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	u32 fpexc;
>> +
>> +	/* Save host fpexc, and enable guest access to fp unit */
>> +	fpexc = fmrx(FPEXC);
>> +	vcpu->arch.host_fpexc = fpexc;
>> +	fpexc |= FPEXC_EN;
>> +	fmxr(FPEXC, fpexc);
>> +
>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>> +}
>> +
>> +/* Called from vcpu_put - restore host fpexc */
> 
> I would probably get rid of the "Called from" stuff and just describe
> what these functions do locally.  Comments like this are likely to be
> out of date soon'ish.

Yeah true, will do.
> 
>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>> +{
>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>> +}
>> +
>> +/* If trap bits are reset then fp/simd registers are dirty */
>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>> +{
>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>> +}
>> +#else
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu->arch.hcptr = HCPTR_TTA;
>> +}
>> +
>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +#endif
> 
> this kind of feels like it belongs in its own C-file instead of a header
> file, perhaps arch/arm/kvm/vfp.C.
> 
> Marc, what do you think?
> 

That would be starting from vcpu_trap_vfp_enable()? The file is getting
little overloaded.

I'm also thinking that 3rd patch should have one function call for vcpu_put
like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic.
When you have a chance to review that patch please keep that in mind.

>> +
>>  #endif /* __ARM_KVM_EMULATE_H__ */
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index f9f2779..d3ef58a 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -105,6 +105,12 @@ struct kvm_vcpu_arch {
>>  	/* HYP trapping configuration */
>>  	u32 hcr;
>>  
>> +	/* HYP Co-processor fp/simd and trace trapping configuration */
>> +	u32 hcptr;
>> +
>> +	/* Save host FPEXC register to later restore on vcpu put */
>> +	u32 host_fpexc;
>> +
>>  	/* Interrupt related fields */
>>  	u32 irq_lines;		/* IRQ and FIQ levels */
>>  
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index 3066328..ffe8ccf 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -299,4 +299,12 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>  	return data;		/* Leave LE untouched */
>>  }
>>  
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu) {}
>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
>> +
>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>>  #endif /* __ARM64_KVM_EMULATE_H__ */
>> -- 
>> 1.9.1
>>
> 
> Thanks,
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-05 19:28       ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-05 19:28 UTC (permalink / raw)
  To: linux-arm-kernel



On 1/5/2016 7:00 AM, Christoffer Dall wrote:
> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>> Add helper functions to enable access to fp/smid on guest entry and save host
>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>> fields.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>  3 files changed, 56 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>> index 3095df0..d4d9da1 100644
>> --- a/arch/arm/include/asm/kvm_emulate.h
>> +++ b/arch/arm/include/asm/kvm_emulate.h
>> @@ -24,6 +24,8 @@
>>  #include <asm/kvm_mmio.h>
>>  #include <asm/kvm_arm.h>
>>  #include <asm/cputype.h>
>> +#include <asm/vfp.h>
>> +#include "../vfp/vfpinstr.h"
> 
> this looks dodgy...
> 
> can you move vfpinstr.h instead?
Sure I'll fix it up, it's in couple other places in kernel and kvm
 - copied it.
> 
>>  
>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_VFPv3
>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> 
> the comment is misleading, you're not enabling guest access to the
> fp/simd unit, you're just setting the enabled bit to ensure guest
> accesses trap.

That's more accurate.
> 
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	u32 fpexc;
>> +
>> +	/* Save host fpexc, and enable guest access to fp unit */
>> +	fpexc = fmrx(FPEXC);
>> +	vcpu->arch.host_fpexc = fpexc;
>> +	fpexc |= FPEXC_EN;
>> +	fmxr(FPEXC, fpexc);
>> +
>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>> +}
>> +
>> +/* Called from vcpu_put - restore host fpexc */
> 
> I would probably get rid of the "Called from" stuff and just describe
> what these functions do locally.  Comments like this are likely to be
> out of date soon'ish.

Yeah true, will do.
> 
>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>> +{
>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>> +}
>> +
>> +/* If trap bits are reset then fp/simd registers are dirty */
>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>> +{
>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>> +}
>> +#else
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu->arch.hcptr = HCPTR_TTA;
>> +}
>> +
>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +#endif
> 
> this kind of feels like it belongs in its own C-file instead of a header
> file, perhaps arch/arm/kvm/vfp.C.
> 
> Marc, what do you think?
> 

That would be starting from vcpu_trap_vfp_enable()? The file is getting
little overloaded.

I'm also thinking that 3rd patch should have one function call for vcpu_put
like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic.
When you have a chance to review that patch please keep that in mind.

>> +
>>  #endif /* __ARM_KVM_EMULATE_H__ */
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index f9f2779..d3ef58a 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -105,6 +105,12 @@ struct kvm_vcpu_arch {
>>  	/* HYP trapping configuration */
>>  	u32 hcr;
>>  
>> +	/* HYP Co-processor fp/simd and trace trapping configuration */
>> +	u32 hcptr;
>> +
>> +	/* Save host FPEXC register to later restore on vcpu put */
>> +	u32 host_fpexc;
>> +
>>  	/* Interrupt related fields */
>>  	u32 irq_lines;		/* IRQ and FIQ levels */
>>  
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index 3066328..ffe8ccf 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -299,4 +299,12 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>  	return data;		/* Leave LE untouched */
>>  }
>>  
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu) {}
>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
>> +
>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>>  #endif /* __ARM64_KVM_EMULATE_H__ */
>> -- 
>> 1.9.1
>>
> 
> Thanks,
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 2/6] arm: KVM: Introduce host fp/simd context switch function
  2015-12-26 21:54   ` Mario Smarduch
@ 2016-01-10 16:31     ` Christoffer Dall
  -1 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:31 UTC (permalink / raw)
  To: Mario Smarduch; +Cc: kvmarm, marc.zyngier, kvm, linux-arm-kernel

On Sat, Dec 26, 2015 at 01:54:56PM -0800, Mario Smarduch wrote:
> Add fp/simd context switch function callable from host kernel mode.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 2/6] arm: KVM: Introduce host fp/simd context switch function
@ 2016-01-10 16:31     ` Christoffer Dall
  0 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Dec 26, 2015 at 01:54:56PM -0800, Mario Smarduch wrote:
> Add fp/simd context switch function callable from host kernel mode.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-05 19:28       ` Mario Smarduch
@ 2016-01-10 16:32         ` Christoffer Dall
  -1 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:32 UTC (permalink / raw)
  To: Mario Smarduch; +Cc: kvmarm, marc.zyngier, kvm, linux-arm-kernel

On Tue, Jan 05, 2016 at 11:28:18AM -0800, Mario Smarduch wrote:
> 
> 
> On 1/5/2016 7:00 AM, Christoffer Dall wrote:
> > On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> >> Add helper functions to enable access to fp/smid on guest entry and save host
> >> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> >> fields.
> >>
> >> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> >> ---
> >>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
> >>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
> >>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
> >>  3 files changed, 56 insertions(+)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> >> index 3095df0..d4d9da1 100644
> >> --- a/arch/arm/include/asm/kvm_emulate.h
> >> +++ b/arch/arm/include/asm/kvm_emulate.h
> >> @@ -24,6 +24,8 @@
> >>  #include <asm/kvm_mmio.h>
> >>  #include <asm/kvm_arm.h>
> >>  #include <asm/cputype.h>
> >> +#include <asm/vfp.h>
> >> +#include "../vfp/vfpinstr.h"
> > 
> > this looks dodgy...
> > 
> > can you move vfpinstr.h instead?
> Sure I'll fix it up, it's in couple other places in kernel and kvm
>  - copied it.
> > 
> >>  
> >>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
> >>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> >> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
> >>  	}
> >>  }
> >>  
> >> +#ifdef CONFIG_VFPv3
> >> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> > 
> > the comment is misleading, you're not enabling guest access to the
> > fp/simd unit, you're just setting the enabled bit to ensure guest
> > accesses trap.
> 
> That's more accurate.
> > 
> >> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >> +{
> >> +	u32 fpexc;
> >> +
> >> +	/* Save host fpexc, and enable guest access to fp unit */
> >> +	fpexc = fmrx(FPEXC);
> >> +	vcpu->arch.host_fpexc = fpexc;
> >> +	fpexc |= FPEXC_EN;
> >> +	fmxr(FPEXC, fpexc);
> >> +
> >> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> >> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> >> +}
> >> +
> >> +/* Called from vcpu_put - restore host fpexc */
> > 
> > I would probably get rid of the "Called from" stuff and just describe
> > what these functions do locally.  Comments like this are likely to be
> > out of date soon'ish.
> 
> Yeah true, will do.
> > 
> >> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> >> +{
> >> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> >> +}
> >> +
> >> +/* If trap bits are reset then fp/simd registers are dirty */
> >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> >> +}
> >> +#else
> >> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >> +{
> >> +	vcpu->arch.hcptr = HCPTR_TTA;
> >> +}
> >> +
> >> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
> >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return false;
> >> +}
> >> +#endif
> > 
> > this kind of feels like it belongs in its own C-file instead of a header
> > file, perhaps arch/arm/kvm/vfp.C.
> > 
> > Marc, what do you think?
> > 
> 
> That would be starting from vcpu_trap_vfp_enable()? The file is getting
> little overloaded.

yes, starting from vcpu_trap_vfp_enable.

Which file is getting overloaded?

I'm thinking the assembly file you add in the next patch could be
inline assembly in a C-file combined with this code, perhaps.

> 
> I'm also thinking that 3rd patch should have one function call for vcpu_put
> like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic.
> When you have a chance to review that patch please keep that in mind.
> 

ok, will try ;)

-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-10 16:32         ` Christoffer Dall
  0 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jan 05, 2016 at 11:28:18AM -0800, Mario Smarduch wrote:
> 
> 
> On 1/5/2016 7:00 AM, Christoffer Dall wrote:
> > On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> >> Add helper functions to enable access to fp/smid on guest entry and save host
> >> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> >> fields.
> >>
> >> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> >> ---
> >>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
> >>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
> >>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
> >>  3 files changed, 56 insertions(+)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> >> index 3095df0..d4d9da1 100644
> >> --- a/arch/arm/include/asm/kvm_emulate.h
> >> +++ b/arch/arm/include/asm/kvm_emulate.h
> >> @@ -24,6 +24,8 @@
> >>  #include <asm/kvm_mmio.h>
> >>  #include <asm/kvm_arm.h>
> >>  #include <asm/cputype.h>
> >> +#include <asm/vfp.h>
> >> +#include "../vfp/vfpinstr.h"
> > 
> > this looks dodgy...
> > 
> > can you move vfpinstr.h instead?
> Sure I'll fix it up, it's in couple other places in kernel and kvm
>  - copied it.
> > 
> >>  
> >>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
> >>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> >> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
> >>  	}
> >>  }
> >>  
> >> +#ifdef CONFIG_VFPv3
> >> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> > 
> > the comment is misleading, you're not enabling guest access to the
> > fp/simd unit, you're just setting the enabled bit to ensure guest
> > accesses trap.
> 
> That's more accurate.
> > 
> >> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >> +{
> >> +	u32 fpexc;
> >> +
> >> +	/* Save host fpexc, and enable guest access to fp unit */
> >> +	fpexc = fmrx(FPEXC);
> >> +	vcpu->arch.host_fpexc = fpexc;
> >> +	fpexc |= FPEXC_EN;
> >> +	fmxr(FPEXC, fpexc);
> >> +
> >> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> >> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> >> +}
> >> +
> >> +/* Called from vcpu_put - restore host fpexc */
> > 
> > I would probably get rid of the "Called from" stuff and just describe
> > what these functions do locally.  Comments like this are likely to be
> > out of date soon'ish.
> 
> Yeah true, will do.
> > 
> >> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> >> +{
> >> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> >> +}
> >> +
> >> +/* If trap bits are reset then fp/simd registers are dirty */
> >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> >> +}
> >> +#else
> >> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >> +{
> >> +	vcpu->arch.hcptr = HCPTR_TTA;
> >> +}
> >> +
> >> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
> >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return false;
> >> +}
> >> +#endif
> > 
> > this kind of feels like it belongs in its own C-file instead of a header
> > file, perhaps arch/arm/kvm/vfp.C.
> > 
> > Marc, what do you think?
> > 
> 
> That would be starting from vcpu_trap_vfp_enable()? The file is getting
> little overloaded.

yes, starting from vcpu_trap_vfp_enable.

Which file is getting overloaded?

I'm thinking the assembly file you add in the next patch could be
inline assembly in a C-file combined with this code, perhaps.

> 
> I'm also thinking that 3rd patch should have one function call for vcpu_put
> like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic.
> When you have a chance to review that patch please keep that in mind.
> 

ok, will try ;)

-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2015-12-26 21:54   ` Mario Smarduch
@ 2016-01-10 16:32     ` Christoffer Dall
  -1 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:32 UTC (permalink / raw)
  To: Mario Smarduch; +Cc: kvmarm, marc.zyngier, kvm, linux-arm-kernel

Hi Mario,

I spotted one more potential issue...

On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> Add helper functions to enable access to fp/smid on guest entry and save host
> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> fields.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 3095df0..d4d9da1 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -24,6 +24,8 @@
>  #include <asm/kvm_mmio.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/cputype.h>
> +#include <asm/vfp.h>
> +#include "../vfp/vfpinstr.h"
>  
>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +#ifdef CONFIG_VFPv3
> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> +{
> +	u32 fpexc;
> +
> +	/* Save host fpexc, and enable guest access to fp unit */
> +	fpexc = fmrx(FPEXC);
> +	vcpu->arch.host_fpexc = fpexc;
> +	fpexc |= FPEXC_EN;
> +	fmxr(FPEXC, fpexc);
> +
> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> +}
> +
> +/* Called from vcpu_put - restore host fpexc */
> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> +{
> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> +}
> +
> +/* If trap bits are reset then fp/simd registers are dirty */
> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> +{
> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> +}
> +#else
> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.hcptr = HCPTR_TTA;

Is it correct not to trap VFP registers when the host kernel does not
have CONFIG_VFPv3?  I think this is a change in functionality compared
to the current kernels is it not?

Thanks,
-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-10 16:32     ` Christoffer Dall
  0 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:32 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Mario,

I spotted one more potential issue...

On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> Add helper functions to enable access to fp/smid on guest entry and save host
> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> fields.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> ---
>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 3095df0..d4d9da1 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -24,6 +24,8 @@
>  #include <asm/kvm_mmio.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/cputype.h>
> +#include <asm/vfp.h>
> +#include "../vfp/vfpinstr.h"
>  
>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +#ifdef CONFIG_VFPv3
> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> +{
> +	u32 fpexc;
> +
> +	/* Save host fpexc, and enable guest access to fp unit */
> +	fpexc = fmrx(FPEXC);
> +	vcpu->arch.host_fpexc = fpexc;
> +	fpexc |= FPEXC_EN;
> +	fmxr(FPEXC, fpexc);
> +
> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> +}
> +
> +/* Called from vcpu_put - restore host fpexc */
> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> +{
> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> +}
> +
> +/* If trap bits are reset then fp/simd registers are dirty */
> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> +{
> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> +}
> +#else
> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.hcptr = HCPTR_TTA;

Is it correct not to trap VFP registers when the host kernel does not
have CONFIG_VFPv3?  I think this is a change in functionality compared
to the current kernels is it not?

Thanks,
-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 3/6] arm/arm64: KVM: Enable armv7 fp/simd enhanced context switch
  2015-12-26 21:54   ` Mario Smarduch
@ 2016-01-10 16:32     ` Christoffer Dall
  -1 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:32 UTC (permalink / raw)
  To: Mario Smarduch; +Cc: kvmarm, marc.zyngier, kvm, linux-arm-kernel

On Sat, Dec 26, 2015 at 01:54:57PM -0800, Mario Smarduch wrote:
> Enable armv7 enhanced fp/simd context switch. Guest and host registers are only
> context switched on first access and vcpu put. 
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 3/6] arm/arm64: KVM: Enable armv7 fp/simd enhanced context switch
@ 2016-01-10 16:32     ` Christoffer Dall
  0 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Dec 26, 2015 at 01:54:57PM -0800, Mario Smarduch wrote:
> Enable armv7 enhanced fp/simd context switch. Guest and host registers are only
> context switched on first access and vcpu put. 
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 4/6] arm: KVM: Delete unused macros
  2015-12-26 21:54   ` Mario Smarduch
@ 2016-01-10 16:32     ` Christoffer Dall
  -1 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:32 UTC (permalink / raw)
  To: Mario Smarduch; +Cc: marc.zyngier, kvmarm, kvm, linux-arm-kernel

On Sat, Dec 26, 2015 at 01:54:58PM -0800, Mario Smarduch wrote:
> set_hcptr is no longer used so delete it.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 4/6] arm: KVM: Delete unused macros
@ 2016-01-10 16:32     ` Christoffer Dall
  0 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-10 16:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Dec 26, 2015 at 01:54:58PM -0800, Mario Smarduch wrote:
> set_hcptr is no longer used so delete it.
> 
> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-10 16:32         ` Christoffer Dall
@ 2016-01-11 23:17           ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-11 23:17 UTC (permalink / raw)
  To: Christoffer Dall; +Cc: kvmarm, marc.zyngier, kvm, linux-arm-kernel



On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> On Tue, Jan 05, 2016 at 11:28:18AM -0800, Mario Smarduch wrote:
>>
>>
>> On 1/5/2016 7:00 AM, Christoffer Dall wrote:
>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>> fields.
>>>>
>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>> ---
>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>  3 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>> index 3095df0..d4d9da1 100644
>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>> @@ -24,6 +24,8 @@
>>>>  #include <asm/kvm_mmio.h>
>>>>  #include <asm/kvm_arm.h>
>>>>  #include <asm/cputype.h>
>>>> +#include <asm/vfp.h>
>>>> +#include "../vfp/vfpinstr.h"
>>>
>>> this looks dodgy...
>>>
>>> can you move vfpinstr.h instead?
>> Sure I'll fix it up, it's in couple other places in kernel and kvm
>>  - copied it.
>>>
>>>>  
>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>  	}
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_VFPv3
>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>
>>> the comment is misleading, you're not enabling guest access to the
>>> fp/simd unit, you're just setting the enabled bit to ensure guest
>>> accesses trap.
>>
>> That's more accurate.
>>>
>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	u32 fpexc;
>>>> +
>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>> +	fpexc = fmrx(FPEXC);
>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>> +	fpexc |= FPEXC_EN;
>>>> +	fmxr(FPEXC, fpexc);
>>>> +
>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>> +}
>>>> +
>>>> +/* Called from vcpu_put - restore host fpexc */
>>>
>>> I would probably get rid of the "Called from" stuff and just describe
>>> what these functions do locally.  Comments like this are likely to be
>>> out of date soon'ish.
>>
>> Yeah true, will do.
>>>
>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>> +}
>>>> +
>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>> +}
>>>> +#else
>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>> +}
>>>> +
>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	return false;
>>>> +}
>>>> +#endif
>>>
>>> this kind of feels like it belongs in its own C-file instead of a header
>>> file, perhaps arch/arm/kvm/vfp.C.
>>>
>>> Marc, what do you think?
>>>
>>
>> That would be starting from vcpu_trap_vfp_enable()? The file is getting
>> little overloaded.
> 
> yes, starting from vcpu_trap_vfp_enable.
> 
> Which file is getting overloaded?
> 
> I'm thinking the assembly file you add in the next patch could be
> inline assembly in a C-file combined with this code, perhaps.

Yes that makes sense, I'll convert it.
> 
>>
>> I'm also thinking that 3rd patch should have one function call for vcpu_put
>> like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic.
>> When you have a chance to review that patch please keep that in mind.
>>
> 
> ok, will try ;)
> 
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-11 23:17           ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-11 23:17 UTC (permalink / raw)
  To: linux-arm-kernel



On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> On Tue, Jan 05, 2016 at 11:28:18AM -0800, Mario Smarduch wrote:
>>
>>
>> On 1/5/2016 7:00 AM, Christoffer Dall wrote:
>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>> fields.
>>>>
>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>> ---
>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>  3 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>> index 3095df0..d4d9da1 100644
>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>> @@ -24,6 +24,8 @@
>>>>  #include <asm/kvm_mmio.h>
>>>>  #include <asm/kvm_arm.h>
>>>>  #include <asm/cputype.h>
>>>> +#include <asm/vfp.h>
>>>> +#include "../vfp/vfpinstr.h"
>>>
>>> this looks dodgy...
>>>
>>> can you move vfpinstr.h instead?
>> Sure I'll fix it up, it's in couple other places in kernel and kvm
>>  - copied it.
>>>
>>>>  
>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>  	}
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_VFPv3
>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>
>>> the comment is misleading, you're not enabling guest access to the
>>> fp/simd unit, you're just setting the enabled bit to ensure guest
>>> accesses trap.
>>
>> That's more accurate.
>>>
>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	u32 fpexc;
>>>> +
>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>> +	fpexc = fmrx(FPEXC);
>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>> +	fpexc |= FPEXC_EN;
>>>> +	fmxr(FPEXC, fpexc);
>>>> +
>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>> +}
>>>> +
>>>> +/* Called from vcpu_put - restore host fpexc */
>>>
>>> I would probably get rid of the "Called from" stuff and just describe
>>> what these functions do locally.  Comments like this are likely to be
>>> out of date soon'ish.
>>
>> Yeah true, will do.
>>>
>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>> +}
>>>> +
>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>> +}
>>>> +#else
>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>> +}
>>>> +
>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	return false;
>>>> +}
>>>> +#endif
>>>
>>> this kind of feels like it belongs in its own C-file instead of a header
>>> file, perhaps arch/arm/kvm/vfp.C.
>>>
>>> Marc, what do you think?
>>>
>>
>> That would be starting from vcpu_trap_vfp_enable()? The file is getting
>> little overloaded.
> 
> yes, starting from vcpu_trap_vfp_enable.
> 
> Which file is getting overloaded?
> 
> I'm thinking the assembly file you add in the next patch could be
> inline assembly in a C-file combined with this code, perhaps.

Yes that makes sense, I'll convert it.
> 
>>
>> I'm also thinking that 3rd patch should have one function call for vcpu_put
>> like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic.
>> When you have a chance to review that patch please keep that in mind.
>>
> 
> ok, will try ;)
> 
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-10 16:32     ` Christoffer Dall
@ 2016-01-11 23:39       ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-11 23:39 UTC (permalink / raw)
  To: Christoffer Dall; +Cc: marc.zyngier, kvmarm, kvm, linux-arm-kernel



On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> Hi Mario,
> 
> I spotted one more potential issue...
> 
> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>> Add helper functions to enable access to fp/smid on guest entry and save host
>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>> fields.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>  3 files changed, 56 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>> index 3095df0..d4d9da1 100644
>> --- a/arch/arm/include/asm/kvm_emulate.h
>> +++ b/arch/arm/include/asm/kvm_emulate.h
>> @@ -24,6 +24,8 @@
>>  #include <asm/kvm_mmio.h>
>>  #include <asm/kvm_arm.h>
>>  #include <asm/cputype.h>
>> +#include <asm/vfp.h>
>> +#include "../vfp/vfpinstr.h"
>>  
>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_VFPv3
>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	u32 fpexc;
>> +
>> +	/* Save host fpexc, and enable guest access to fp unit */
>> +	fpexc = fmrx(FPEXC);
>> +	vcpu->arch.host_fpexc = fpexc;
>> +	fpexc |= FPEXC_EN;
>> +	fmxr(FPEXC, fpexc);
>> +
>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>> +}
>> +
>> +/* Called from vcpu_put - restore host fpexc */
>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>> +{
>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>> +}
>> +
>> +/* If trap bits are reset then fp/simd registers are dirty */
>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>> +{
>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>> +}
>> +#else
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu->arch.hcptr = HCPTR_TTA;
> 
> Is it correct not to trap VFP registers when the host kernel does not
> have CONFIG_VFPv3?  I think this is a change in functionality compared
> to the current kernels is it not?

With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
with exceptions taken in guest kernel. I don't see a reason why
fp hcptr access should be enabled in that case.

> 
> Thanks,
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-11 23:39       ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-11 23:39 UTC (permalink / raw)
  To: linux-arm-kernel



On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> Hi Mario,
> 
> I spotted one more potential issue...
> 
> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>> Add helper functions to enable access to fp/smid on guest entry and save host
>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>> fields.
>>
>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>> ---
>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>  3 files changed, 56 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>> index 3095df0..d4d9da1 100644
>> --- a/arch/arm/include/asm/kvm_emulate.h
>> +++ b/arch/arm/include/asm/kvm_emulate.h
>> @@ -24,6 +24,8 @@
>>  #include <asm/kvm_mmio.h>
>>  #include <asm/kvm_arm.h>
>>  #include <asm/cputype.h>
>> +#include <asm/vfp.h>
>> +#include "../vfp/vfpinstr.h"
>>  
>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_VFPv3
>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	u32 fpexc;
>> +
>> +	/* Save host fpexc, and enable guest access to fp unit */
>> +	fpexc = fmrx(FPEXC);
>> +	vcpu->arch.host_fpexc = fpexc;
>> +	fpexc |= FPEXC_EN;
>> +	fmxr(FPEXC, fpexc);
>> +
>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>> +}
>> +
>> +/* Called from vcpu_put - restore host fpexc */
>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>> +{
>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>> +}
>> +
>> +/* If trap bits are reset then fp/simd registers are dirty */
>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>> +{
>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>> +}
>> +#else
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu->arch.hcptr = HCPTR_TTA;
> 
> Is it correct not to trap VFP registers when the host kernel does not
> have CONFIG_VFPv3?  I think this is a change in functionality compared
> to the current kernels is it not?

With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
with exceptions taken in guest kernel. I don't see a reason why
fp hcptr access should be enabled in that case.

> 
> Thanks,
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-11 23:39       ` Mario Smarduch
@ 2016-01-12 14:12         ` Christoffer Dall
  -1 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-12 14:12 UTC (permalink / raw)
  To: Mario Smarduch; +Cc: marc.zyngier, kvmarm, kvm, linux-arm-kernel

On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
> 
> 
> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> > Hi Mario,
> > 
> > I spotted one more potential issue...
> > 
> > On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> >> Add helper functions to enable access to fp/smid on guest entry and save host
> >> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> >> fields.
> >>
> >> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> >> ---
> >>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
> >>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
> >>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
> >>  3 files changed, 56 insertions(+)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> >> index 3095df0..d4d9da1 100644
> >> --- a/arch/arm/include/asm/kvm_emulate.h
> >> +++ b/arch/arm/include/asm/kvm_emulate.h
> >> @@ -24,6 +24,8 @@
> >>  #include <asm/kvm_mmio.h>
> >>  #include <asm/kvm_arm.h>
> >>  #include <asm/cputype.h>
> >> +#include <asm/vfp.h>
> >> +#include "../vfp/vfpinstr.h"
> >>  
> >>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
> >>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> >> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
> >>  	}
> >>  }
> >>  
> >> +#ifdef CONFIG_VFPv3
> >> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> >> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >> +{
> >> +	u32 fpexc;
> >> +
> >> +	/* Save host fpexc, and enable guest access to fp unit */
> >> +	fpexc = fmrx(FPEXC);
> >> +	vcpu->arch.host_fpexc = fpexc;
> >> +	fpexc |= FPEXC_EN;
> >> +	fmxr(FPEXC, fpexc);
> >> +
> >> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> >> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> >> +}
> >> +
> >> +/* Called from vcpu_put - restore host fpexc */
> >> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> >> +{
> >> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> >> +}
> >> +
> >> +/* If trap bits are reset then fp/simd registers are dirty */
> >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> >> +}
> >> +#else
> >> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >> +{
> >> +	vcpu->arch.hcptr = HCPTR_TTA;
> > 
> > Is it correct not to trap VFP registers when the host kernel does not
> > have CONFIG_VFPv3?  I think this is a change in functionality compared
> > to the current kernels is it not?
> 
> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
> with exceptions taken in guest kernel. I don't see a reason why
> fp hcptr access should be enabled in that case.
> 

If you have to guests with CONFIG_VFPV3 but your host doesn't have
CONFIG_VFPV3, you will never context-switch the VFP registers between
the two VMs, and mayhem will ensue.

Unless I'm missing something very obvious?

-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-12 14:12         ` Christoffer Dall
  0 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-12 14:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
> 
> 
> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> > Hi Mario,
> > 
> > I spotted one more potential issue...
> > 
> > On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> >> Add helper functions to enable access to fp/smid on guest entry and save host
> >> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> >> fields.
> >>
> >> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> >> ---
> >>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
> >>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
> >>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
> >>  3 files changed, 56 insertions(+)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> >> index 3095df0..d4d9da1 100644
> >> --- a/arch/arm/include/asm/kvm_emulate.h
> >> +++ b/arch/arm/include/asm/kvm_emulate.h
> >> @@ -24,6 +24,8 @@
> >>  #include <asm/kvm_mmio.h>
> >>  #include <asm/kvm_arm.h>
> >>  #include <asm/cputype.h>
> >> +#include <asm/vfp.h>
> >> +#include "../vfp/vfpinstr.h"
> >>  
> >>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
> >>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> >> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
> >>  	}
> >>  }
> >>  
> >> +#ifdef CONFIG_VFPv3
> >> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> >> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >> +{
> >> +	u32 fpexc;
> >> +
> >> +	/* Save host fpexc, and enable guest access to fp unit */
> >> +	fpexc = fmrx(FPEXC);
> >> +	vcpu->arch.host_fpexc = fpexc;
> >> +	fpexc |= FPEXC_EN;
> >> +	fmxr(FPEXC, fpexc);
> >> +
> >> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> >> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> >> +}
> >> +
> >> +/* Called from vcpu_put - restore host fpexc */
> >> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> >> +{
> >> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> >> +}
> >> +
> >> +/* If trap bits are reset then fp/simd registers are dirty */
> >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> >> +{
> >> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> >> +}
> >> +#else
> >> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >> +{
> >> +	vcpu->arch.hcptr = HCPTR_TTA;
> > 
> > Is it correct not to trap VFP registers when the host kernel does not
> > have CONFIG_VFPv3?  I think this is a change in functionality compared
> > to the current kernels is it not?
> 
> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
> with exceptions taken in guest kernel. I don't see a reason why
> fp hcptr access should be enabled in that case.
> 

If you have to guests with CONFIG_VFPV3 but your host doesn't have
CONFIG_VFPV3, you will never context-switch the VFP registers between
the two VMs, and mayhem will ensue.

Unless I'm missing something very obvious?

-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-12 14:12         ` Christoffer Dall
@ 2016-01-13  0:57           ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-13  0:57 UTC (permalink / raw)
  To: Christoffer Dall; +Cc: marc.zyngier, kvmarm, kvm, linux-arm-kernel



On 1/12/2016 6:12 AM, Christoffer Dall wrote:
> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>
>>
>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>> Hi Mario,
>>>
>>> I spotted one more potential issue...
>>>
>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>> fields.
>>>>
>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>> ---
>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>  3 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>> index 3095df0..d4d9da1 100644
>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>> @@ -24,6 +24,8 @@
>>>>  #include <asm/kvm_mmio.h>
>>>>  #include <asm/kvm_arm.h>
>>>>  #include <asm/cputype.h>
>>>> +#include <asm/vfp.h>
>>>> +#include "../vfp/vfpinstr.h"
>>>>  
>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>  	}
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_VFPv3
>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	u32 fpexc;
>>>> +
>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>> +	fpexc = fmrx(FPEXC);
>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>> +	fpexc |= FPEXC_EN;
>>>> +	fmxr(FPEXC, fpexc);
>>>> +
>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>> +}
>>>> +
>>>> +/* Called from vcpu_put - restore host fpexc */
>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>> +}
>>>> +
>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>> +}
>>>> +#else
>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>
>>> Is it correct not to trap VFP registers when the host kernel does not
>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>> to the current kernels is it not?
>>
>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>> with exceptions taken in guest kernel. I don't see a reason why
>> fp hcptr access should be enabled in that case.
>>
> 
> If you have to guests with CONFIG_VFPV3 but your host doesn't have
> CONFIG_VFPV3, you will never context-switch the VFP registers between
> the two VMs, and mayhem will ensue.
> 
> Unless I'm missing something very obvious?

Hi Christoffer,
   - on host I disabled VFP/VFPv3 and got a lot of Illegal instructions
     and many other problems. Perhaps disabling for armv7 this option may need
     re-evaluation.
   - Enabling VFPv3 on host and running guest with no vfpv3 appears
     to work with few glitches.
   - and vpfv3 host/guest works just fine.

Appears disabling vfpv3 on armv7 requires another investigation (atleast
on my end).

BTW this is on fast models.

- Mario

> 
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-13  0:57           ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-13  0:57 UTC (permalink / raw)
  To: linux-arm-kernel



On 1/12/2016 6:12 AM, Christoffer Dall wrote:
> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>
>>
>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>> Hi Mario,
>>>
>>> I spotted one more potential issue...
>>>
>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>> fields.
>>>>
>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>> ---
>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>  3 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>> index 3095df0..d4d9da1 100644
>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>> @@ -24,6 +24,8 @@
>>>>  #include <asm/kvm_mmio.h>
>>>>  #include <asm/kvm_arm.h>
>>>>  #include <asm/cputype.h>
>>>> +#include <asm/vfp.h>
>>>> +#include "../vfp/vfpinstr.h"
>>>>  
>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>  	}
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_VFPv3
>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	u32 fpexc;
>>>> +
>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>> +	fpexc = fmrx(FPEXC);
>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>> +	fpexc |= FPEXC_EN;
>>>> +	fmxr(FPEXC, fpexc);
>>>> +
>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>> +}
>>>> +
>>>> +/* Called from vcpu_put - restore host fpexc */
>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>> +}
>>>> +
>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>> +}
>>>> +#else
>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>
>>> Is it correct not to trap VFP registers when the host kernel does not
>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>> to the current kernels is it not?
>>
>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>> with exceptions taken in guest kernel. I don't see a reason why
>> fp hcptr access should be enabled in that case.
>>
> 
> If you have to guests with CONFIG_VFPV3 but your host doesn't have
> CONFIG_VFPV3, you will never context-switch the VFP registers between
> the two VMs, and mayhem will ensue.
> 
> Unless I'm missing something very obvious?

Hi Christoffer,
   - on host I disabled VFP/VFPv3 and got a lot of Illegal instructions
     and many other problems. Perhaps disabling for armv7 this option may need
     re-evaluation.
   - Enabling VFPv3 on host and running guest with no vfpv3 appears
     to work with few glitches.
   - and vpfv3 host/guest works just fine.

Appears disabling vfpv3 on armv7 requires another investigation (atleast
on my end).

BTW this is on fast models.

- Mario

> 
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-13  0:57           ` Mario Smarduch
@ 2016-01-14  3:03             ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-14  3:03 UTC (permalink / raw)
  To: Christoffer Dall
  Cc: marc.zyngier, kvmarm, kvm, linux-arm-kernel, Russell King - ARM Linux



On 1/12/2016 4:57 PM, Mario Smarduch wrote:
> 
> 
> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>
>>>
>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>> Hi Mario,
>>>>
>>>> I spotted one more potential issue...
>>>>
>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>> fields.
>>>>>
>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>> ---
>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>  3 files changed, 56 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>> index 3095df0..d4d9da1 100644
>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>> @@ -24,6 +24,8 @@
>>>>>  #include <asm/kvm_mmio.h>
>>>>>  #include <asm/kvm_arm.h>
>>>>>  #include <asm/cputype.h>
>>>>> +#include <asm/vfp.h>
>>>>> +#include "../vfp/vfpinstr.h"
>>>>>  
>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>  	}
>>>>>  }
>>>>>  
>>>>> +#ifdef CONFIG_VFPv3
>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	u32 fpexc;
>>>>> +
>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>> +	fpexc = fmrx(FPEXC);
>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>> +	fpexc |= FPEXC_EN;
>>>>> +	fmxr(FPEXC, fpexc);
>>>>> +
>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>> +}
>>>>> +
>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>> +}
>>>>> +
>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>> +}
>>>>> +#else
>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>
>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>> to the current kernels is it not?
>>>
>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>> with exceptions taken in guest kernel. I don't see a reason why
>>> fp hcptr access should be enabled in that case.
>>>
>>
>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>> the two VMs, and mayhem will ensue.
>>
>> Unless I'm missing something very obvious?

Did more testing on this enabling OABI_COMPAT and selecting
NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
selection. I'm wondering if !VFPv3 path should be removed from
the patches?

- Mario

> 
> Hi Christoffer,
>    - on host I disabled VFP/VFPv3 and got a lot of Illegal instructions
>      and many other problems. Perhaps disabling for armv7 this option may need
>      re-evaluation.
>    - Enabling VFPv3 on host and running guest with no vfpv3 appears
>      to work with few glitches.
>    - and vpfv3 host/guest works just fine.
> 
> Appears disabling vfpv3 on armv7 requires another investigation (atleast
> on my end).
> 
> BTW this is on fast models.
> 
> - Mario
> 
>>
>> -Christoffer
>>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-14  3:03             ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-14  3:03 UTC (permalink / raw)
  To: linux-arm-kernel



On 1/12/2016 4:57 PM, Mario Smarduch wrote:
> 
> 
> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>
>>>
>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>> Hi Mario,
>>>>
>>>> I spotted one more potential issue...
>>>>
>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>> fields.
>>>>>
>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>> ---
>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>  3 files changed, 56 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>> index 3095df0..d4d9da1 100644
>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>> @@ -24,6 +24,8 @@
>>>>>  #include <asm/kvm_mmio.h>
>>>>>  #include <asm/kvm_arm.h>
>>>>>  #include <asm/cputype.h>
>>>>> +#include <asm/vfp.h>
>>>>> +#include "../vfp/vfpinstr.h"
>>>>>  
>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>  	}
>>>>>  }
>>>>>  
>>>>> +#ifdef CONFIG_VFPv3
>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	u32 fpexc;
>>>>> +
>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>> +	fpexc = fmrx(FPEXC);
>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>> +	fpexc |= FPEXC_EN;
>>>>> +	fmxr(FPEXC, fpexc);
>>>>> +
>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>> +}
>>>>> +
>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>> +}
>>>>> +
>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>> +}
>>>>> +#else
>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>
>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>> to the current kernels is it not?
>>>
>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>> with exceptions taken in guest kernel. I don't see a reason why
>>> fp hcptr access should be enabled in that case.
>>>
>>
>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>> the two VMs, and mayhem will ensue.
>>
>> Unless I'm missing something very obvious?

Did more testing on this enabling OABI_COMPAT and selecting
NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
selection. I'm wondering if !VFPv3 path should be removed from
the patches?

- Mario

> 
> Hi Christoffer,
>    - on host I disabled VFP/VFPv3 and got a lot of Illegal instructions
>      and many other problems. Perhaps disabling for armv7 this option may need
>      re-evaluation.
>    - Enabling VFPv3 on host and running guest with no vfpv3 appears
>      to work with few glitches.
>    - and vpfv3 host/guest works just fine.
> 
> Appears disabling vfpv3 on armv7 requires another investigation (atleast
> on my end).
> 
> BTW this is on fast models.
> 
> - Mario
> 
>>
>> -Christoffer
>>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-14  3:03             ` Mario Smarduch
@ 2016-01-14 13:27               ` Christoffer Dall
  -1 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-14 13:27 UTC (permalink / raw)
  To: Mario Smarduch
  Cc: marc.zyngier, Russell King - ARM Linux, kvmarm, kvm, linux-arm-kernel

On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
> 
> 
> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
> > 
> > 
> > On 1/12/2016 6:12 AM, Christoffer Dall wrote:
> >> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
> >>>
> >>>
> >>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> >>>> Hi Mario,
> >>>>
> >>>> I spotted one more potential issue...
> >>>>
> >>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> >>>>> Add helper functions to enable access to fp/smid on guest entry and save host
> >>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> >>>>> fields.
> >>>>>
> >>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> >>>>> ---
> >>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
> >>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
> >>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
> >>>>>  3 files changed, 56 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> >>>>> index 3095df0..d4d9da1 100644
> >>>>> --- a/arch/arm/include/asm/kvm_emulate.h
> >>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
> >>>>> @@ -24,6 +24,8 @@
> >>>>>  #include <asm/kvm_mmio.h>
> >>>>>  #include <asm/kvm_arm.h>
> >>>>>  #include <asm/cputype.h>
> >>>>> +#include <asm/vfp.h>
> >>>>> +#include "../vfp/vfpinstr.h"
> >>>>>  
> >>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
> >>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> >>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
> >>>>>  	}
> >>>>>  }
> >>>>>  
> >>>>> +#ifdef CONFIG_VFPv3
> >>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> >>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	u32 fpexc;
> >>>>> +
> >>>>> +	/* Save host fpexc, and enable guest access to fp unit */
> >>>>> +	fpexc = fmrx(FPEXC);
> >>>>> +	vcpu->arch.host_fpexc = fpexc;
> >>>>> +	fpexc |= FPEXC_EN;
> >>>>> +	fmxr(FPEXC, fpexc);
> >>>>> +
> >>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> >>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> >>>>> +}
> >>>>> +
> >>>>> +/* Called from vcpu_put - restore host fpexc */
> >>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> >>>>> +}
> >>>>> +
> >>>>> +/* If trap bits are reset then fp/simd registers are dirty */
> >>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> >>>>> +}
> >>>>> +#else
> >>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
> >>>>
> >>>> Is it correct not to trap VFP registers when the host kernel does not
> >>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
> >>>> to the current kernels is it not?
> >>>
> >>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
> >>> with exceptions taken in guest kernel. I don't see a reason why
> >>> fp hcptr access should be enabled in that case.
> >>>
> >>
> >> If you have to guests with CONFIG_VFPV3 but your host doesn't have
> >> CONFIG_VFPV3, you will never context-switch the VFP registers between
> >> the two VMs, and mayhem will ensue.
> >>
> >> Unless I'm missing something very obvious?
> 
> Did more testing on this enabling OABI_COMPAT and selecting
> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
> selection. I'm wondering if !VFPv3 path should be removed from
> the patches?
> 
I think this is related to your particular choice of userspace.  I think
it's fair to assume VFP is enabled for a KVM host, but I don't have
enough familiarity with this to be sure.

Marc, any thoughts?

-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-14 13:27               ` Christoffer Dall
  0 siblings, 0 replies; 46+ messages in thread
From: Christoffer Dall @ 2016-01-14 13:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
> 
> 
> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
> > 
> > 
> > On 1/12/2016 6:12 AM, Christoffer Dall wrote:
> >> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
> >>>
> >>>
> >>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> >>>> Hi Mario,
> >>>>
> >>>> I spotted one more potential issue...
> >>>>
> >>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
> >>>>> Add helper functions to enable access to fp/smid on guest entry and save host
> >>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
> >>>>> fields.
> >>>>>
> >>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> >>>>> ---
> >>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
> >>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
> >>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
> >>>>>  3 files changed, 56 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> >>>>> index 3095df0..d4d9da1 100644
> >>>>> --- a/arch/arm/include/asm/kvm_emulate.h
> >>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
> >>>>> @@ -24,6 +24,8 @@
> >>>>>  #include <asm/kvm_mmio.h>
> >>>>>  #include <asm/kvm_arm.h>
> >>>>>  #include <asm/cputype.h>
> >>>>> +#include <asm/vfp.h>
> >>>>> +#include "../vfp/vfpinstr.h"
> >>>>>  
> >>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
> >>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
> >>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
> >>>>>  	}
> >>>>>  }
> >>>>>  
> >>>>> +#ifdef CONFIG_VFPv3
> >>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
> >>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	u32 fpexc;
> >>>>> +
> >>>>> +	/* Save host fpexc, and enable guest access to fp unit */
> >>>>> +	fpexc = fmrx(FPEXC);
> >>>>> +	vcpu->arch.host_fpexc = fpexc;
> >>>>> +	fpexc |= FPEXC_EN;
> >>>>> +	fmxr(FPEXC, fpexc);
> >>>>> +
> >>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
> >>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
> >>>>> +}
> >>>>> +
> >>>>> +/* Called from vcpu_put - restore host fpexc */
> >>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
> >>>>> +}
> >>>>> +
> >>>>> +/* If trap bits are reset then fp/simd registers are dirty */
> >>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
> >>>>> +}
> >>>>> +#else
> >>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
> >>>>
> >>>> Is it correct not to trap VFP registers when the host kernel does not
> >>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
> >>>> to the current kernels is it not?
> >>>
> >>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
> >>> with exceptions taken in guest kernel. I don't see a reason why
> >>> fp hcptr access should be enabled in that case.
> >>>
> >>
> >> If you have to guests with CONFIG_VFPV3 but your host doesn't have
> >> CONFIG_VFPV3, you will never context-switch the VFP registers between
> >> the two VMs, and mayhem will ensue.
> >>
> >> Unless I'm missing something very obvious?
> 
> Did more testing on this enabling OABI_COMPAT and selecting
> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
> selection. I'm wondering if !VFPv3 path should be removed from
> the patches?
> 
I think this is related to your particular choice of userspace.  I think
it's fair to assume VFP is enabled for a KVM host, but I don't have
enough familiarity with this to be sure.

Marc, any thoughts?

-Christoffer

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-14 13:27               ` Christoffer Dall
@ 2016-01-14 13:55                 ` Marc Zyngier
  -1 siblings, 0 replies; 46+ messages in thread
From: Marc Zyngier @ 2016-01-14 13:55 UTC (permalink / raw)
  To: Christoffer Dall, Mario Smarduch
  Cc: kvmarm, kvm, linux-arm-kernel, Russell King - ARM Linux

On 14/01/16 13:27, Christoffer Dall wrote:
> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>
>>
>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>
>>>
>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>> Hi Mario,
>>>>>>
>>>>>> I spotted one more potential issue...
>>>>>>
>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>> fields.
>>>>>>>
>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>> ---
>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>  #include <asm/cputype.h>
>>>>>>> +#include <asm/vfp.h>
>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>  
>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>  	}
>>>>>>>  }
>>>>>>>  
>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	u32 fpexc;
>>>>>>> +
>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>> +
>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>
>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>> to the current kernels is it not?
>>>>>
>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>> fp hcptr access should be enabled in that case.
>>>>>
>>>>
>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>> the two VMs, and mayhem will ensue.
>>>>
>>>> Unless I'm missing something very obvious?
>>
>> Did more testing on this enabling OABI_COMPAT and selecting
>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>> selection. I'm wondering if !VFPv3 path should be removed from
>> the patches?
>>
> I think this is related to your particular choice of userspace.  I think
> it's fair to assume VFP is enabled for a KVM host, but I don't have
> enough familiarity with this to be sure.
> 
> Marc, any thoughts?

I'm not so sure about the host kernel configuration - after all, you
could have a very specialized host that doesn't use VFP at all
(softfloat), and yet offers VFP support to guests.

But what I'm worried about is the case where someone has built an
ARMv7+VE without VFP. We may end-up exploding in that case. What we
could do is to probe for the VFP HW, and not enable KVM if absent. This
would give us the freedom to remove the #ifdefery.

What do you think?

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

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-14 13:55                 ` Marc Zyngier
  0 siblings, 0 replies; 46+ messages in thread
From: Marc Zyngier @ 2016-01-14 13:55 UTC (permalink / raw)
  To: linux-arm-kernel

On 14/01/16 13:27, Christoffer Dall wrote:
> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>
>>
>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>
>>>
>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>> Hi Mario,
>>>>>>
>>>>>> I spotted one more potential issue...
>>>>>>
>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>> fields.
>>>>>>>
>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>> ---
>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>  #include <asm/cputype.h>
>>>>>>> +#include <asm/vfp.h>
>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>  
>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>  	}
>>>>>>>  }
>>>>>>>  
>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	u32 fpexc;
>>>>>>> +
>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>> +
>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>
>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>> to the current kernels is it not?
>>>>>
>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>> fp hcptr access should be enabled in that case.
>>>>>
>>>>
>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>> the two VMs, and mayhem will ensue.
>>>>
>>>> Unless I'm missing something very obvious?
>>
>> Did more testing on this enabling OABI_COMPAT and selecting
>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>> selection. I'm wondering if !VFPv3 path should be removed from
>> the patches?
>>
> I think this is related to your particular choice of userspace.  I think
> it's fair to assume VFP is enabled for a KVM host, but I don't have
> enough familiarity with this to be sure.
> 
> Marc, any thoughts?

I'm not so sure about the host kernel configuration - after all, you
could have a very specialized host that doesn't use VFP at all
(softfloat), and yet offers VFP support to guests.

But what I'm worried about is the case where someone has built an
ARMv7+VE without VFP. We may end-up exploding in that case. What we
could do is to probe for the VFP HW, and not enable KVM if absent. This
would give us the freedom to remove the #ifdefery.

What do you think?

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

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-14 13:27               ` Christoffer Dall
@ 2016-01-15  2:02                 ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-15  2:02 UTC (permalink / raw)
  To: Christoffer Dall
  Cc: marc.zyngier, kvmarm, kvm, linux-arm-kernel, Russell King - ARM Linux



On 1/14/2016 5:27 AM, Christoffer Dall wrote:
> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>
>>
>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>
>>>
>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>> Hi Mario,
>>>>>>
>>>>>> I spotted one more potential issue...
>>>>>>
>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>> fields.
>>>>>>>
>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>> ---
>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>  #include <asm/cputype.h>
>>>>>>> +#include <asm/vfp.h>
>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>  
>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>  	}
>>>>>>>  }
>>>>>>>  
>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	u32 fpexc;
>>>>>>> +
>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>> +
>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>
>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>> to the current kernels is it not?
>>>>>
>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>> fp hcptr access should be enabled in that case.
>>>>>
>>>>
>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>> the two VMs, and mayhem will ensue.
>>>>
>>>> Unless I'm missing something very obvious?
>>
>> Did more testing on this enabling OABI_COMPAT and selecting
>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>> selection. I'm wondering if !VFPv3 path should be removed from
>> the patches?
>>
> I think this is related to your particular choice of userspace. 

It appears like there are two soft float implementations.

FastFPE - but that's missing arch/arm/fastfpe directory, the option
can still be selected but nothing is built.

And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
hooked into the kernel. The hook nwfpe_enter is not referenced
anywhere.

I could make this change but have no way to bring the host up to
test it.

- Mario

> I think
> it's fair to assume VFP is enabled for a KVM host, but I don't have
> enough familiarity with this to be sure.
> 
> Marc, any thoughts?
> 
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-15  2:02                 ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-15  2:02 UTC (permalink / raw)
  To: linux-arm-kernel



On 1/14/2016 5:27 AM, Christoffer Dall wrote:
> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>
>>
>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>
>>>
>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>> Hi Mario,
>>>>>>
>>>>>> I spotted one more potential issue...
>>>>>>
>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>> fields.
>>>>>>>
>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>> ---
>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>  #include <asm/cputype.h>
>>>>>>> +#include <asm/vfp.h>
>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>  
>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>  	}
>>>>>>>  }
>>>>>>>  
>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	u32 fpexc;
>>>>>>> +
>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>> +
>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>
>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>> to the current kernels is it not?
>>>>>
>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>> fp hcptr access should be enabled in that case.
>>>>>
>>>>
>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>> the two VMs, and mayhem will ensue.
>>>>
>>>> Unless I'm missing something very obvious?
>>
>> Did more testing on this enabling OABI_COMPAT and selecting
>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>> selection. I'm wondering if !VFPv3 path should be removed from
>> the patches?
>>
> I think this is related to your particular choice of userspace. 

It appears like there are two soft float implementations.

FastFPE - but that's missing arch/arm/fastfpe directory, the option
can still be selected but nothing is built.

And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
hooked into the kernel. The hook nwfpe_enter is not referenced
anywhere.

I could make this change but have no way to bring the host up to
test it.

- Mario

> I think
> it's fair to assume VFP is enabled for a KVM host, but I don't have
> enough familiarity with this to be sure.
> 
> Marc, any thoughts?
> 
> -Christoffer
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-15  2:02                 ` Mario Smarduch
@ 2016-01-15  9:03                   ` Marc Zyngier
  -1 siblings, 0 replies; 46+ messages in thread
From: Marc Zyngier @ 2016-01-15  9:03 UTC (permalink / raw)
  To: Mario Smarduch, Christoffer Dall
  Cc: kvmarm, kvm, linux-arm-kernel, Russell King - ARM Linux

On 15/01/16 02:02, Mario Smarduch wrote:
> 
> 
> On 1/14/2016 5:27 AM, Christoffer Dall wrote:
>> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>>
>>>
>>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>>
>>>>
>>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>>
>>>>>>
>>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>>> Hi Mario,
>>>>>>>
>>>>>>> I spotted one more potential issue...
>>>>>>>
>>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>>> fields.
>>>>>>>>
>>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>>> ---
>>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>>  #include <asm/cputype.h>
>>>>>>>> +#include <asm/vfp.h>
>>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>>  
>>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>>  	}
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> +	u32 fpexc;
>>>>>>>> +
>>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>>> +
>>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>>> +}
>>>>>>>> +#else
>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>>
>>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>>> to the current kernels is it not?
>>>>>>
>>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>>> fp hcptr access should be enabled in that case.
>>>>>>
>>>>>
>>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>>> the two VMs, and mayhem will ensue.
>>>>>
>>>>> Unless I'm missing something very obvious?
>>>
>>> Did more testing on this enabling OABI_COMPAT and selecting
>>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>>> selection. I'm wondering if !VFPv3 path should be removed from
>>> the patches?
>>>
>> I think this is related to your particular choice of userspace. 
> 
> It appears like there are two soft float implementations.
> 
> FastFPE - but that's missing arch/arm/fastfpe directory, the option
> can still be selected but nothing is built.
> 
> And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
> hooked into the kernel. The hook nwfpe_enter is not referenced
> anywhere.

It is:

arch/arm/nwfpe/entry.S: .globl  nwfpe_enter
arch/arm/nwfpe/entry.S:nwfpe_enter:
arch/arm/nwfpe/fpmodule.c:extern void nwfpe_enter(void);
arch/arm/nwfpe/fpmodule.c:      kern_fp_enter = nwfpe_enter;

> I could make this change but have no way to bring the host up to
> test it.

None of these are relevant - they are emulation for the FPA (Floating
Point Accelerator). Most of the time, nobody uses this but instead a
userspace softfloat implementation, which saves the trap to kernel space
for emulation.

You can try Debian armel (as opposed to armhf, which mandates VFP) for
example, which is a softfloat-based distribution.

Thanks,

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

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-15  9:03                   ` Marc Zyngier
  0 siblings, 0 replies; 46+ messages in thread
From: Marc Zyngier @ 2016-01-15  9:03 UTC (permalink / raw)
  To: linux-arm-kernel

On 15/01/16 02:02, Mario Smarduch wrote:
> 
> 
> On 1/14/2016 5:27 AM, Christoffer Dall wrote:
>> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>>
>>>
>>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>>
>>>>
>>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>>
>>>>>>
>>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>>> Hi Mario,
>>>>>>>
>>>>>>> I spotted one more potential issue...
>>>>>>>
>>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>>> fields.
>>>>>>>>
>>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>>> ---
>>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>>  #include <asm/cputype.h>
>>>>>>>> +#include <asm/vfp.h>
>>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>>  
>>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>>  	}
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> +	u32 fpexc;
>>>>>>>> +
>>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>>> +
>>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>>> +}
>>>>>>>> +#else
>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>> +{
>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>>
>>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>>> to the current kernels is it not?
>>>>>>
>>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>>> fp hcptr access should be enabled in that case.
>>>>>>
>>>>>
>>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>>> the two VMs, and mayhem will ensue.
>>>>>
>>>>> Unless I'm missing something very obvious?
>>>
>>> Did more testing on this enabling OABI_COMPAT and selecting
>>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>>> selection. I'm wondering if !VFPv3 path should be removed from
>>> the patches?
>>>
>> I think this is related to your particular choice of userspace. 
> 
> It appears like there are two soft float implementations.
> 
> FastFPE - but that's missing arch/arm/fastfpe directory, the option
> can still be selected but nothing is built.
> 
> And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
> hooked into the kernel. The hook nwfpe_enter is not referenced
> anywhere.

It is:

arch/arm/nwfpe/entry.S: .globl  nwfpe_enter
arch/arm/nwfpe/entry.S:nwfpe_enter:
arch/arm/nwfpe/fpmodule.c:extern void nwfpe_enter(void);
arch/arm/nwfpe/fpmodule.c:      kern_fp_enter = nwfpe_enter;

> I could make this change but have no way to bring the host up to
> test it.

None of these are relevant - they are emulation for the FPA (Floating
Point Accelerator). Most of the time, nobody uses this but instead a
userspace softfloat implementation, which saves the trap to kernel space
for emulation.

You can try Debian armel (as opposed to armhf, which mandates VFP) for
example, which is a softfloat-based distribution.

Thanks,

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

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-15  9:03                   ` Marc Zyngier
@ 2016-01-16  1:21                     ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-16  1:21 UTC (permalink / raw)
  To: Marc Zyngier, Christoffer Dall
  Cc: kvmarm, kvm, linux-arm-kernel, Russell King - ARM Linux



On 1/15/2016 1:03 AM, Marc Zyngier wrote:
> On 15/01/16 02:02, Mario Smarduch wrote:
>>
>>
>> On 1/14/2016 5:27 AM, Christoffer Dall wrote:
>>> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>>>
>>>>
>>>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>>>> Hi Mario,
>>>>>>>>
>>>>>>>> I spotted one more potential issue...
>>>>>>>>
>>>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>>>> fields.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>>>> ---
>>>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>>>  #include <asm/cputype.h>
>>>>>>>>> +#include <asm/vfp.h>
>>>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>>>  
>>>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>>>  	}
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	u32 fpexc;
>>>>>>>>> +
>>>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>>>> +
>>>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>>>> +}
>>>>>>>>> +#else
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>>>
>>>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>>>> to the current kernels is it not?
>>>>>>>
>>>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>>>> fp hcptr access should be enabled in that case.
>>>>>>>
>>>>>>
>>>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>>>> the two VMs, and mayhem will ensue.
>>>>>>
>>>>>> Unless I'm missing something very obvious?
>>>>
>>>> Did more testing on this enabling OABI_COMPAT and selecting
>>>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>>>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>>>> selection. I'm wondering if !VFPv3 path should be removed from
>>>> the patches?
>>>>
>>> I think this is related to your particular choice of userspace. 
>>
>> It appears like there are two soft float implementations.
>>
>> FastFPE - but that's missing arch/arm/fastfpe directory, the option
>> can still be selected but nothing is built.
>>
>> And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
>> hooked into the kernel. The hook nwfpe_enter is not referenced
>> anywhere.
> 
> It is:
> 
> arch/arm/nwfpe/entry.S: .globl  nwfpe_enter
> arch/arm/nwfpe/entry.S:nwfpe_enter:
> arch/arm/nwfpe/fpmodule.c:extern void nwfpe_enter(void);
> arch/arm/nwfpe/fpmodule.c:      kern_fp_enter = nwfpe_enter;
> 
>> I could make this change but have no way to bring the host up to
>> test it.
> 
> None of these are relevant - they are emulation for the FPA (Floating
> Point Accelerator). Most of the time, nobody uses this but instead a
> userspace softfloat implementation, which saves the trap to kernel space
> for emulation.
> 
> You can try Debian armel (as opposed to armhf, which mandates VFP) for
> example, which is a softfloat-based distribution.

I've been using armel debian but it appears some binaries have hard fp
instructions (-mfloat-abi=hard,softfp). I have a simple rootfs now and
it comes up cleanly on the host. I'll work on qemu now.

Thanks for pointing out FPA.

- Mario


> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-16  1:21                     ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-16  1:21 UTC (permalink / raw)
  To: linux-arm-kernel



On 1/15/2016 1:03 AM, Marc Zyngier wrote:
> On 15/01/16 02:02, Mario Smarduch wrote:
>>
>>
>> On 1/14/2016 5:27 AM, Christoffer Dall wrote:
>>> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>>>
>>>>
>>>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>>>> Hi Mario,
>>>>>>>>
>>>>>>>> I spotted one more potential issue...
>>>>>>>>
>>>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>>>> fields.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>>>> ---
>>>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>>>  #include <asm/cputype.h>
>>>>>>>>> +#include <asm/vfp.h>
>>>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>>>  
>>>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>>>  	}
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	u32 fpexc;
>>>>>>>>> +
>>>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>>>> +
>>>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>>>> +}
>>>>>>>>> +#else
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>>>
>>>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>>>> to the current kernels is it not?
>>>>>>>
>>>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>>>> fp hcptr access should be enabled in that case.
>>>>>>>
>>>>>>
>>>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>>>> the two VMs, and mayhem will ensue.
>>>>>>
>>>>>> Unless I'm missing something very obvious?
>>>>
>>>> Did more testing on this enabling OABI_COMPAT and selecting
>>>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>>>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>>>> selection. I'm wondering if !VFPv3 path should be removed from
>>>> the patches?
>>>>
>>> I think this is related to your particular choice of userspace. 
>>
>> It appears like there are two soft float implementations.
>>
>> FastFPE - but that's missing arch/arm/fastfpe directory, the option
>> can still be selected but nothing is built.
>>
>> And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
>> hooked into the kernel. The hook nwfpe_enter is not referenced
>> anywhere.
> 
> It is:
> 
> arch/arm/nwfpe/entry.S: .globl  nwfpe_enter
> arch/arm/nwfpe/entry.S:nwfpe_enter:
> arch/arm/nwfpe/fpmodule.c:extern void nwfpe_enter(void);
> arch/arm/nwfpe/fpmodule.c:      kern_fp_enter = nwfpe_enter;
> 
>> I could make this change but have no way to bring the host up to
>> test it.
> 
> None of these are relevant - they are emulation for the FPA (Floating
> Point Accelerator). Most of the time, nobody uses this but instead a
> userspace softfloat implementation, which saves the trap to kernel space
> for emulation.
> 
> You can try Debian armel (as opposed to armhf, which mandates VFP) for
> example, which is a softfloat-based distribution.

I've been using armel debian but it appears some binaries have hard fp
instructions (-mfloat-abi=hard,softfp). I have a simple rootfs now and
it comes up cleanly on the host. I'll work on qemu now.

Thanks for pointing out FPA.

- Mario


> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
  2016-01-15  9:03                   ` Marc Zyngier
@ 2016-01-21  2:29                     ` Mario Smarduch
  -1 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-21  2:29 UTC (permalink / raw)
  To: Marc Zyngier, Christoffer Dall
  Cc: linux-arm-kernel, Russell King - ARM Linux, kvmarm, kvm



On 1/15/2016 1:03 AM, Marc Zyngier wrote:
> On 15/01/16 02:02, Mario Smarduch wrote:
>>
>>
>> On 1/14/2016 5:27 AM, Christoffer Dall wrote:
>>> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>>>
>>>>
>>>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>>>> Hi Mario,
>>>>>>>>
>>>>>>>> I spotted one more potential issue...
>>>>>>>>
>>>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>>>> fields.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>>>> ---
>>>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>>>  #include <asm/cputype.h>
>>>>>>>>> +#include <asm/vfp.h>
>>>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>>>  
>>>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>>>  	}
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	u32 fpexc;
>>>>>>>>> +
>>>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>>>> +
>>>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>>>> +}
>>>>>>>>> +#else
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>>>
>>>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>>>> to the current kernels is it not?
>>>>>>>
>>>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>>>> fp hcptr access should be enabled in that case.
>>>>>>>
>>>>>>
>>>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>>>> the two VMs, and mayhem will ensue.
>>>>>>
>>>>>> Unless I'm missing something very obvious?
>>>>
>>>> Did more testing on this enabling OABI_COMPAT and selecting
>>>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>>>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>>>> selection. I'm wondering if !VFPv3 path should be removed from
>>>> the patches?
>>>>
>>> I think this is related to your particular choice of userspace. 
>>
>> It appears like there are two soft float implementations.
>>
>> FastFPE - but that's missing arch/arm/fastfpe directory, the option
>> can still be selected but nothing is built.
>>
>> And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
>> hooked into the kernel. The hook nwfpe_enter is not referenced
>> anywhere.
> 
> It is:
> 
> arch/arm/nwfpe/entry.S: .globl  nwfpe_enter
> arch/arm/nwfpe/entry.S:nwfpe_enter:
> arch/arm/nwfpe/fpmodule.c:extern void nwfpe_enter(void);
> arch/arm/nwfpe/fpmodule.c:      kern_fp_enter = nwfpe_enter;
> 
>> I could make this change but have no way to bring the host up to
>> test it.

I investigated VFP and virtualiztion. Came up with few
hw/sw configurations, some of it tested other not so
it may be full of holes.

1. CONFIG_VFP set and VFP not implemented on all CPUs
vfp_init() - discovers VFP is not supported via vfp_vector
and bails out. A guest should be able to boot successfully
going through the same check. And obviously there is no
need to lazy switch vfp registers, vcpu_put/get should know
about this. This would be softfloat/softfloat.

2. CONFIG_VFP set and platform calls vfp_disable() which I believe
the platform is saying not all cpus have a VFP. Some guests may
detect a VFP others not. Doesn't appear like a workable configuration
with virtualization enabled.

3. CONFIG_VFP not set and VFP impl/not impl host using softfloat.
To get this to work code from vfp_init is needed.
- Enable coproc for cp10, 11 on cpus
- Test for vfp support  allow platform to set vfp_disable.

in vput_load()
- if all CPUs have a VFP set FPEXC.EN, set hcptr vfp trapping
- Here guest gets an undefined instruction on fmrx(FPSID), but if
  vfp traps are not set the guest boots just fine, don't know
  what the problem is.

4. CONFIG_VFP set VFP implemented on all CPUs
- This works fine, Guest VFP no exception on FPSIMD access
  like in 3. Appears like full host VFP initialization
  does something to make the guest with hcptr traps work.

Unfortunately my priorities have been changed and need to move to
other work. I could complete 4 per Christoffers last comments.

- Mario


> 
> None of these are relevant - they are emulation for the FPA (Floating
> Point Accelerator). Most of the time, nobody uses this but instead a
> userspace softfloat implementation, which saves the trap to kernel space
> for emulation.
> 
> You can try Debian armel (as opposed to armhf, which mandates VFP) for
> example, which is a softfloat-based distribution.
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
* [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers
@ 2016-01-21  2:29                     ` Mario Smarduch
  0 siblings, 0 replies; 46+ messages in thread
From: Mario Smarduch @ 2016-01-21  2:29 UTC (permalink / raw)
  To: linux-arm-kernel



On 1/15/2016 1:03 AM, Marc Zyngier wrote:
> On 15/01/16 02:02, Mario Smarduch wrote:
>>
>>
>> On 1/14/2016 5:27 AM, Christoffer Dall wrote:
>>> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>>>
>>>>
>>>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>>>> Hi Mario,
>>>>>>>>
>>>>>>>> I spotted one more potential issue...
>>>>>>>>
>>>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>>>> fields.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>>>> ---
>>>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>>>  #include <asm/cputype.h>
>>>>>>>>> +#include <asm/vfp.h>
>>>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>>>  
>>>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>>>  	}
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	u32 fpexc;
>>>>>>>>> +
>>>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>>>> +
>>>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>>>> +}
>>>>>>>>> +#else
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>>>
>>>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>>>> to the current kernels is it not?
>>>>>>>
>>>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>>>> fp hcptr access should be enabled in that case.
>>>>>>>
>>>>>>
>>>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>>>> the two VMs, and mayhem will ensue.
>>>>>>
>>>>>> Unless I'm missing something very obvious?
>>>>
>>>> Did more testing on this enabling OABI_COMPAT and selecting
>>>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>>>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>>>> selection. I'm wondering if !VFPv3 path should be removed from
>>>> the patches?
>>>>
>>> I think this is related to your particular choice of userspace. 
>>
>> It appears like there are two soft float implementations.
>>
>> FastFPE - but that's missing arch/arm/fastfpe directory, the option
>> can still be selected but nothing is built.
>>
>> And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
>> hooked into the kernel. The hook nwfpe_enter is not referenced
>> anywhere.
> 
> It is:
> 
> arch/arm/nwfpe/entry.S: .globl  nwfpe_enter
> arch/arm/nwfpe/entry.S:nwfpe_enter:
> arch/arm/nwfpe/fpmodule.c:extern void nwfpe_enter(void);
> arch/arm/nwfpe/fpmodule.c:      kern_fp_enter = nwfpe_enter;
> 
>> I could make this change but have no way to bring the host up to
>> test it.

I investigated VFP and virtualiztion. Came up with few
hw/sw configurations, some of it tested other not so
it may be full of holes.

1. CONFIG_VFP set and VFP not implemented on all CPUs
vfp_init() - discovers VFP is not supported via vfp_vector
and bails out. A guest should be able to boot successfully
going through the same check. And obviously there is no
need to lazy switch vfp registers, vcpu_put/get should know
about this. This would be softfloat/softfloat.

2. CONFIG_VFP set and platform calls vfp_disable() which I believe
the platform is saying not all cpus have a VFP. Some guests may
detect a VFP others not. Doesn't appear like a workable configuration
with virtualization enabled.

3. CONFIG_VFP not set and VFP impl/not impl host using softfloat.
To get this to work code from vfp_init is needed.
- Enable coproc for cp10, 11 on cpus
- Test for vfp support  allow platform to set vfp_disable.

in vput_load()
- if all CPUs have a VFP set FPEXC.EN, set hcptr vfp trapping
- Here guest gets an undefined instruction on fmrx(FPSID), but if
  vfp traps are not set the guest boots just fine, don't know
  what the problem is.

4. CONFIG_VFP set VFP implemented on all CPUs
- This works fine, Guest VFP no exception on FPSIMD access
  like in 3. Appears like full host VFP initialization
  does something to make the guest with hcptr traps work.

Unfortunately my priorities have been changed and need to move to
other work. I could complete 4 per Christoffers last comments.

- Mario


> 
> None of these are relevant - they are emulation for the FPA (Floating
> Point Accelerator). Most of the time, nobody uses this but instead a
> userspace softfloat implementation, which saves the trap to kernel space
> for emulation.
> 
> You can try Debian armel (as opposed to armhf, which mandates VFP) for
> example, which is a softfloat-based distribution.
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread
end of thread, other threads:[~2016-01-21  2:29 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-26 21:54 [PATCH v6 0/6] arm/arm64: KVM: Enhance armv7/8 fp/simd lazy switch Mario Smarduch
2015-12-26 21:54 ` Mario Smarduch
2015-12-26 21:54 ` [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers Mario Smarduch
2015-12-26 21:54   ` Mario Smarduch
2016-01-05 15:00   ` Christoffer Dall
2016-01-05 15:00     ` Christoffer Dall
2016-01-05 19:28     ` Mario Smarduch
2016-01-05 19:28       ` Mario Smarduch
2016-01-10 16:32       ` Christoffer Dall
2016-01-10 16:32         ` Christoffer Dall
2016-01-11 23:17         ` Mario Smarduch
2016-01-11 23:17           ` Mario Smarduch
2016-01-10 16:32   ` Christoffer Dall
2016-01-10 16:32     ` Christoffer Dall
2016-01-11 23:39     ` Mario Smarduch
2016-01-11 23:39       ` Mario Smarduch
2016-01-12 14:12       ` Christoffer Dall
2016-01-12 14:12         ` Christoffer Dall
2016-01-13  0:57         ` Mario Smarduch
2016-01-13  0:57           ` Mario Smarduch
2016-01-14  3:03           ` Mario Smarduch
2016-01-14  3:03             ` Mario Smarduch
2016-01-14 13:27             ` Christoffer Dall
2016-01-14 13:27               ` Christoffer Dall
2016-01-14 13:55               ` Marc Zyngier
2016-01-14 13:55                 ` Marc Zyngier
2016-01-15  2:02               ` Mario Smarduch
2016-01-15  2:02                 ` Mario Smarduch
2016-01-15  9:03                 ` Marc Zyngier
2016-01-15  9:03                   ` Marc Zyngier
2016-01-16  1:21                   ` Mario Smarduch
2016-01-16  1:21                     ` Mario Smarduch
2016-01-21  2:29                   ` Mario Smarduch
2016-01-21  2:29                     ` Mario Smarduch
2015-12-26 21:54 ` [PATCH v6 2/6] arm: KVM: Introduce host fp/simd context switch function Mario Smarduch
2015-12-26 21:54   ` Mario Smarduch
2016-01-10 16:31   ` Christoffer Dall
2016-01-10 16:31     ` Christoffer Dall
2015-12-26 21:54 ` [PATCH v6 3/6] arm/arm64: KVM: Enable armv7 fp/simd enhanced context switch Mario Smarduch
2015-12-26 21:54   ` Mario Smarduch
2016-01-10 16:32   ` Christoffer Dall
2016-01-10 16:32     ` Christoffer Dall
2015-12-26 21:54 ` [PATCH v6 4/6] arm: KVM: Delete unused macros Mario Smarduch
2015-12-26 21:54   ` Mario Smarduch
2016-01-10 16:32   ` Christoffer Dall
2016-01-10 16:32     ` Christoffer Dall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.