[PATCH 0/3] arm64: KVM: Allow direct function calls on VHE

[PATCH 0/3] arm64: KVM: Allow direct function calls on VHE

[Marc Zyngier]

It recently appeared that the nasty hack we use to call a HYP function
on a non-VHE system has an interesting side effect on VHE: We wrap any
such call into a hypercall, losing any form of type checking between
the caller and the callee.

This isn't a big deal if you can guarantee to write code that is
always 100% correct, but it appears that I'm not you.

In order to restore some sanity, let's use the following property: On
a VHE system, it is always possible to call any function directly as
they live in the same address space. We can thus always emit a direct
call, and use a static key to flip from one to the other. As a bonus,
this also sanitizes !VHE systems as we always generate code for noth
revisions of the architecture.

Marc Zyngier (3):
  arm/arm64: KVM: Introduce kvm_call_hyp_ret()
  arm64: KVM: Allow for direct call of HYP functions when using VHE
  arm64: KVM: Drop VHE-specific HYP call stub

 arch/arm/include/asm/kvm_host.h   |  3 +++
 arch/arm64/include/asm/kvm_host.h | 31 ++++++++++++++++++++++++++++++-
 arch/arm64/kvm/debug.c            |  2 +-
 arch/arm64/kvm/hyp.S              |  3 ---
 arch/arm64/kvm/hyp/hyp-entry.S    | 12 ------------
 virt/kvm/arm/arm.c                |  2 +-
 virt/kvm/arm/vgic/vgic-v3.c       |  4 ++--
 7 files changed, 37 insertions(+), 20 deletions(-)

-- 
2.20.1

[PATCH 0/3] arm64: KVM: Allow direct function calls on VHE

[Marc Zyngier]

It recently appeared that the nasty hack we use to call a HYP function
on a non-VHE system has an interesting side effect on VHE: We wrap any
such call into a hypercall, losing any form of type checking between
the caller and the callee.

This isn't a big deal if you can guarantee to write code that is
always 100% correct, but it appears that I'm not you.

In order to restore some sanity, let's use the following property: On
a VHE system, it is always possible to call any function directly as
they live in the same address space. We can thus always emit a direct
call, and use a static key to flip from one to the other. As a bonus,
this also sanitizes !VHE systems as we always generate code for noth
revisions of the architecture.

Marc Zyngier (3):
  arm/arm64: KVM: Introduce kvm_call_hyp_ret()
  arm64: KVM: Allow for direct call of HYP functions when using VHE
  arm64: KVM: Drop VHE-specific HYP call stub

 arch/arm/include/asm/kvm_host.h   |  3 +++
 arch/arm64/include/asm/kvm_host.h | 31 ++++++++++++++++++++++++++++++-
 arch/arm64/kvm/debug.c            |  2 +-
 arch/arm64/kvm/hyp.S              |  3 ---
 arch/arm64/kvm/hyp/hyp-entry.S    | 12 ------------
 virt/kvm/arm/arm.c                |  2 +-
 virt/kvm/arm/vgic/vgic-v3.c       |  4 ++--
 7 files changed, 37 insertions(+), 20 deletions(-)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] arm/arm64: KVM: Introduce kvm_call_hyp_ret()

[Marc Zyngier]

Until now, we haven't differentiated between HYP calls that
have a return value and those who don't. As we're about to
change this, introduce kvm_call_hyp_ret(), and change all
call sites that actually make use of a return value.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_host.h   | 3 +++
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/debug.c            | 2 +-
 virt/kvm/arm/arm.c                | 2 +-
 virt/kvm/arm/vgic/vgic-v3.c       | 4 ++--
 5 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index ca56537b61bc..023c9f2b1eea 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -214,7 +214,10 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
 int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
 int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+
 unsigned long kvm_call_hyp(void *hypfn, ...);
+#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
+
 void force_vm_exit(const cpumask_t *mask);
 int __kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
 			      struct kvm_vcpu_events *events);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7732d0ba4e60..e54cb7c88a4e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -371,6 +371,7 @@ void kvm_arm_resume_guest(struct kvm *kvm);
 
 u64 __kvm_call_hyp(void *hypfn, ...);
 #define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
+#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
 
 void force_vm_exit(const cpumask_t *mask);
 void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index f39801e4136c..fd917d6d12af 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -76,7 +76,7 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 
 void kvm_arm_init_debug(void)
 {
-	__this_cpu_write(mdcr_el2, kvm_call_hyp(__kvm_get_mdcr_el2));
+	__this_cpu_write(mdcr_el2, kvm_call_hyp_ret(__kvm_get_mdcr_el2));
 }
 
 /**
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9e350fd34504..4d55f98f97f7 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -765,7 +765,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			ret = kvm_vcpu_run_vhe(vcpu);
 			kvm_arm_vhe_guest_exit();
 		} else {
-			ret = kvm_call_hyp(__kvm_vcpu_run_nvhe, vcpu);
+			ret = kvm_call_hyp_ret(__kvm_vcpu_run_nvhe, vcpu);
 		}
 
 		vcpu->mode = OUTSIDE_GUEST_MODE;
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 9c0dd234ebe8..67f98151c88d 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -589,7 +589,7 @@ early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
  */
 int vgic_v3_probe(const struct gic_kvm_info *info)
 {
-	u32 ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
+	u32 ich_vtr_el2 = kvm_call_hyp_ret(__vgic_v3_get_ich_vtr_el2);
 	int ret;
 
 	/*
@@ -679,7 +679,7 @@ void vgic_v3_put(struct kvm_vcpu *vcpu)
 	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
 
 	if (likely(cpu_if->vgic_sre))
-		cpu_if->vgic_vmcr = kvm_call_hyp(__vgic_v3_read_vmcr);
+		cpu_if->vgic_vmcr = kvm_call_hyp_ret(__vgic_v3_read_vmcr);
 
 	kvm_call_hyp(__vgic_v3_save_aprs, vcpu);
 
-- 
2.20.1

[PATCH 1/3] arm/arm64: KVM: Introduce kvm_call_hyp_ret()

[Marc Zyngier]

Until now, we haven't differentiated between HYP calls that
have a return value and those who don't. As we're about to
change this, introduce kvm_call_hyp_ret(), and change all
call sites that actually make use of a return value.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_host.h   | 3 +++
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/debug.c            | 2 +-
 virt/kvm/arm/arm.c                | 2 +-
 virt/kvm/arm/vgic/vgic-v3.c       | 4 ++--
 5 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index ca56537b61bc..023c9f2b1eea 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -214,7 +214,10 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
 int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
 int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+
 unsigned long kvm_call_hyp(void *hypfn, ...);
+#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
+
 void force_vm_exit(const cpumask_t *mask);
 int __kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
 			      struct kvm_vcpu_events *events);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7732d0ba4e60..e54cb7c88a4e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -371,6 +371,7 @@ void kvm_arm_resume_guest(struct kvm *kvm);
 
 u64 __kvm_call_hyp(void *hypfn, ...);
 #define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
+#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
 
 void force_vm_exit(const cpumask_t *mask);
 void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index f39801e4136c..fd917d6d12af 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -76,7 +76,7 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 
 void kvm_arm_init_debug(void)
 {
-	__this_cpu_write(mdcr_el2, kvm_call_hyp(__kvm_get_mdcr_el2));
+	__this_cpu_write(mdcr_el2, kvm_call_hyp_ret(__kvm_get_mdcr_el2));
 }
 
 /**
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9e350fd34504..4d55f98f97f7 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -765,7 +765,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			ret = kvm_vcpu_run_vhe(vcpu);
 			kvm_arm_vhe_guest_exit();
 		} else {
-			ret = kvm_call_hyp(__kvm_vcpu_run_nvhe, vcpu);
+			ret = kvm_call_hyp_ret(__kvm_vcpu_run_nvhe, vcpu);
 		}
 
 		vcpu->mode = OUTSIDE_GUEST_MODE;
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 9c0dd234ebe8..67f98151c88d 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -589,7 +589,7 @@ early_param("kvm-arm.vgic_v4_enable", early_gicv4_enable);
  */
 int vgic_v3_probe(const struct gic_kvm_info *info)
 {
-	u32 ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
+	u32 ich_vtr_el2 = kvm_call_hyp_ret(__vgic_v3_get_ich_vtr_el2);
 	int ret;
 
 	/*
@@ -679,7 +679,7 @@ void vgic_v3_put(struct kvm_vcpu *vcpu)
 	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
 
 	if (likely(cpu_if->vgic_sre))
-		cpu_if->vgic_vmcr = kvm_call_hyp(__vgic_v3_read_vmcr);
+		cpu_if->vgic_vmcr = kvm_call_hyp_ret(__vgic_v3_read_vmcr);
 
 	kvm_call_hyp(__vgic_v3_save_aprs, vcpu);
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Marc Zyngier]

When running VHE, there is no need to jump via some stub to perform
a "HYP" function call, as there is a single address space.

Let's thus change kvm_call_hyp() and co to perform a direct call
in this case. Although this results in a bit of code expansion,
it allows the compiler to check for type compatibility, something
that we are missing so far.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e54cb7c88a4e..df32edbadd69 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
 void kvm_arm_resume_guest(struct kvm *kvm);
 
 u64 __kvm_call_hyp(void *hypfn, ...);
-#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
-#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
+
+/*
+ * The couple of isb() below are there to guarantee the same behaviour
+ * on VHE as on !VHE, where the eret to EL1 acts as a context
+ * synchronization event.
+ */
+#define kvm_call_hyp(f, ...)						\
+	do {								\
+		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
+			f(__VA_ARGS__);					\
+			isb();						\
+		} else {						\
+			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
+		}							\
+	} while(0)
+
+#define kvm_call_hyp_ret(f, ...)					\
+	({								\
+		u64 ret;						\
+									\
+		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
+			ret = f(__VA_ARGS__);				\
+			isb();						\
+		} else {						\
+			ret = __kvm_call_hyp(kvm_ksym_ref(f),		\
+					     ##__VA_ARGS__);		\
+		}							\
+									\
+		ret;							\
+	})
 
 void force_vm_exit(const cpumask_t *mask);
 void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
-- 
2.20.1

[PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Marc Zyngier]

When running VHE, there is no need to jump via some stub to perform
a "HYP" function call, as there is a single address space.

Let's thus change kvm_call_hyp() and co to perform a direct call
in this case. Although this results in a bit of code expansion,
it allows the compiler to check for type compatibility, something
that we are missing so far.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e54cb7c88a4e..df32edbadd69 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
 void kvm_arm_resume_guest(struct kvm *kvm);
 
 u64 __kvm_call_hyp(void *hypfn, ...);
-#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
-#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
+
+/*
+ * The couple of isb() below are there to guarantee the same behaviour
+ * on VHE as on !VHE, where the eret to EL1 acts as a context
+ * synchronization event.
+ */
+#define kvm_call_hyp(f, ...)						\
+	do {								\
+		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
+			f(__VA_ARGS__);					\
+			isb();						\
+		} else {						\
+			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
+		}							\
+	} while(0)
+
+#define kvm_call_hyp_ret(f, ...)					\
+	({								\
+		u64 ret;						\
+									\
+		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
+			ret = f(__VA_ARGS__);				\
+			isb();						\
+		} else {						\
+			ret = __kvm_call_hyp(kvm_ksym_ref(f),		\
+					     ##__VA_ARGS__);		\
+		}							\
+									\
+		ret;							\
+	})
 
 void force_vm_exit(const cpumask_t *mask);
 void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: KVM: Drop VHE-specific HYP call stub

[Marc Zyngier]

We now call VHE code directly, without going through any central
dispatching function. Let's drop that code.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp.S           |  3 ---
 arch/arm64/kvm/hyp/hyp-entry.S | 12 ------------
 2 files changed, 15 deletions(-)

diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 952f6cb9cf72..2845aa680841 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -40,9 +40,6 @@
  * arch/arm64/kernel/hyp_stub.S.
  */
 ENTRY(__kvm_call_hyp)
-alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
 	hvc	#0
 	ret
-alternative_else_nop_endif
-	b	__vhe_hyp_call
 ENDPROC(__kvm_call_hyp)
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 73c1b483ec39..2b1e686772bf 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -43,18 +43,6 @@
 	ldr	lr, [sp], #16
 .endm
 
-ENTRY(__vhe_hyp_call)
-	do_el2_call
-	/*
-	 * We used to rely on having an exception return to get
-	 * an implicit isb. In the E2H case, we don't have it anymore.
-	 * rather than changing all the leaf functions, just do it here
-	 * before returning to the rest of the kernel.
-	 */
-	isb
-	ret
-ENDPROC(__vhe_hyp_call)
-
 el1_sync:				// Guest trapped into EL2
 
 	mrs	x0, esr_el2
-- 
2.20.1

[PATCH 3/3] arm64: KVM: Drop VHE-specific HYP call stub

[Marc Zyngier]

We now call VHE code directly, without going through any central
dispatching function. Let's drop that code.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp.S           |  3 ---
 arch/arm64/kvm/hyp/hyp-entry.S | 12 ------------
 2 files changed, 15 deletions(-)

diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 952f6cb9cf72..2845aa680841 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -40,9 +40,6 @@
  * arch/arm64/kernel/hyp_stub.S.
  */
 ENTRY(__kvm_call_hyp)
-alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
 	hvc	#0
 	ret
-alternative_else_nop_endif
-	b	__vhe_hyp_call
 ENDPROC(__kvm_call_hyp)
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 73c1b483ec39..2b1e686772bf 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -43,18 +43,6 @@
 	ldr	lr, [sp], #16
 .endm
 
-ENTRY(__vhe_hyp_call)
-	do_el2_call
-	/*
-	 * We used to rely on having an exception return to get
-	 * an implicit isb. In the E2H case, we don't have it anymore.
-	 * rather than changing all the leaf functions, just do it here
-	 * before returning to the rest of the kernel.
-	 */
-	isb
-	ret
-ENDPROC(__vhe_hyp_call)
-
 el1_sync:				// Guest trapped into EL2
 
 	mrs	x0, esr_el2
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] arm64: KVM: Allow direct function calls on VHE

[Andrew Murray]

On Wed, Jan 09, 2019 at 01:54:32PM +0000, Marc Zyngier wrote:
> It recently appeared that the nasty hack we use to call a HYP function
> on a non-VHE system has an interesting side effect on VHE: We wrap any
> such call into a hypercall, losing any form of type checking between
> the caller and the callee.
> 
> This isn't a big deal if you can guarantee to write code that is
> always 100% correct, but it appears that I'm not you.
> 
> In order to restore some sanity, let's use the following property: On
> a VHE system, it is always possible to call any function directly as
> they live in the same address space. We can thus always emit a direct
> call, and use a static key to flip from one to the other. As a bonus,
> this also sanitizes !VHE systems as we always generate code for noth

s/noth/both/g

Andrew Murray

> revisions of the architecture.
> 
> Marc Zyngier (3):
>   arm/arm64: KVM: Introduce kvm_call_hyp_ret()
>   arm64: KVM: Allow for direct call of HYP functions when using VHE
>   arm64: KVM: Drop VHE-specific HYP call stub
> 
>  arch/arm/include/asm/kvm_host.h   |  3 +++
>  arch/arm64/include/asm/kvm_host.h | 31 ++++++++++++++++++++++++++++++-
>  arch/arm64/kvm/debug.c            |  2 +-
>  arch/arm64/kvm/hyp.S              |  3 ---
>  arch/arm64/kvm/hyp/hyp-entry.S    | 12 ------------
>  virt/kvm/arm/arm.c                |  2 +-
>  virt/kvm/arm/vgic/vgic-v3.c       |  4 ++--
>  7 files changed, 37 insertions(+), 20 deletions(-)
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/3] arm64: KVM: Allow direct function calls on VHE

[Andrew Murray]

On Wed, Jan 09, 2019 at 01:54:32PM +0000, Marc Zyngier wrote:
> It recently appeared that the nasty hack we use to call a HYP function
> on a non-VHE system has an interesting side effect on VHE: We wrap any
> such call into a hypercall, losing any form of type checking between
> the caller and the callee.
> 
> This isn't a big deal if you can guarantee to write code that is
> always 100% correct, but it appears that I'm not you.
> 
> In order to restore some sanity, let's use the following property: On
> a VHE system, it is always possible to call any function directly as
> they live in the same address space. We can thus always emit a direct
> call, and use a static key to flip from one to the other. As a bonus,
> this also sanitizes !VHE systems as we always generate code for noth

s/noth/both/g

Andrew Murray

> revisions of the architecture.
> 
> Marc Zyngier (3):
>   arm/arm64: KVM: Introduce kvm_call_hyp_ret()
>   arm64: KVM: Allow for direct call of HYP functions when using VHE
>   arm64: KVM: Drop VHE-specific HYP call stub
> 
>  arch/arm/include/asm/kvm_host.h   |  3 +++
>  arch/arm64/include/asm/kvm_host.h | 31 ++++++++++++++++++++++++++++++-
>  arch/arm64/kvm/debug.c            |  2 +-
>  arch/arm64/kvm/hyp.S              |  3 ---
>  arch/arm64/kvm/hyp/hyp-entry.S    | 12 ------------
>  virt/kvm/arm/arm.c                |  2 +-
>  virt/kvm/arm/vgic/vgic-v3.c       |  4 ++--
>  7 files changed, 37 insertions(+), 20 deletions(-)
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Andrew Murray]

On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
> When running VHE, there is no need to jump via some stub to perform
> a "HYP" function call, as there is a single address space.
> 
> Let's thus change kvm_call_hyp() and co to perform a direct call
> in this case. Although this results in a bit of code expansion,
> it allows the compiler to check for type compatibility, something
> that we are missing so far.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e54cb7c88a4e..df32edbadd69 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>  void kvm_arm_resume_guest(struct kvm *kvm);
>  
>  u64 __kvm_call_hyp(void *hypfn, ...);
> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
> +
> +/*
> + * The couple of isb() below are there to guarantee the same behaviour
> + * on VHE as on !VHE, where the eret to EL1 acts as a context
> + * synchronization event.
> + */
> +#define kvm_call_hyp(f, ...)						\
> +	do {								\
> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
> +			f(__VA_ARGS__);					\
> +			isb();						\
> +		} else {						\
> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
> +		}							\
> +	} while(0)
> +
> +#define kvm_call_hyp_ret(f, ...)					\
> +	({								\
> +		u64 ret;						\
> +									\
> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
> +			ret = f(__VA_ARGS__);				\

__kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
return a smaller type. I guess any issues would be picked up when compiling,
but should the name of the macro be clearer as to the assumptions it makes?
Or perhaps take an argument which is the type of ret?

Andrew Murray

> +			isb();						\
> +		} else {						\
> +			ret = __kvm_call_hyp(kvm_ksym_ref(f),		\
> +					     ##__VA_ARGS__);		\
> +		}							\
> +									\
> +		ret;							\
> +	})
>  
>  void force_vm_exit(const cpumask_t *mask);
>  void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
> -- 
> 2.20.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Andrew Murray]

On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
> When running VHE, there is no need to jump via some stub to perform
> a "HYP" function call, as there is a single address space.
> 
> Let's thus change kvm_call_hyp() and co to perform a direct call
> in this case. Although this results in a bit of code expansion,
> it allows the compiler to check for type compatibility, something
> that we are missing so far.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e54cb7c88a4e..df32edbadd69 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>  void kvm_arm_resume_guest(struct kvm *kvm);
>  
>  u64 __kvm_call_hyp(void *hypfn, ...);
> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
> +
> +/*
> + * The couple of isb() below are there to guarantee the same behaviour
> + * on VHE as on !VHE, where the eret to EL1 acts as a context
> + * synchronization event.
> + */
> +#define kvm_call_hyp(f, ...)						\
> +	do {								\
> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
> +			f(__VA_ARGS__);					\
> +			isb();						\
> +		} else {						\
> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
> +		}							\
> +	} while(0)
> +
> +#define kvm_call_hyp_ret(f, ...)					\
> +	({								\
> +		u64 ret;						\
> +									\
> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
> +			ret = f(__VA_ARGS__);				\

__kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
return a smaller type. I guess any issues would be picked up when compiling,
but should the name of the macro be clearer as to the assumptions it makes?
Or perhaps take an argument which is the type of ret?

Andrew Murray

> +			isb();						\
> +		} else {						\
> +			ret = __kvm_call_hyp(kvm_ksym_ref(f),		\
> +					     ##__VA_ARGS__);		\
> +		}							\
> +									\
> +		ret;							\
> +	})
>  
>  void force_vm_exit(const cpumask_t *mask);
>  void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
> -- 
> 2.20.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Marc Zyngier]

Hi Andrew,

On 09/01/2019 14:24, Andrew Murray wrote:
> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
>> When running VHE, there is no need to jump via some stub to perform
>> a "HYP" function call, as there is a single address space.
>>
>> Let's thus change kvm_call_hyp() and co to perform a direct call
>> in this case. Although this results in a bit of code expansion,
>> it allows the compiler to check for type compatibility, something
>> that we are missing so far.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index e54cb7c88a4e..df32edbadd69 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>>  void kvm_arm_resume_guest(struct kvm *kvm);
>>  
>>  u64 __kvm_call_hyp(void *hypfn, ...);
>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
>> +
>> +/*
>> + * The couple of isb() below are there to guarantee the same behaviour
>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
>> + * synchronization event.
>> + */
>> +#define kvm_call_hyp(f, ...)						\
>> +	do {								\
>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>> +			f(__VA_ARGS__);					\
>> +			isb();						\
>> +		} else {						\
>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
>> +		}							\
>> +	} while(0)
>> +
>> +#define kvm_call_hyp_ret(f, ...)					\
>> +	({								\
>> +		u64 ret;						\
>> +									\
>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>> +			ret = f(__VA_ARGS__);				\
> 
> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
> return a smaller type. I guess any issues would be picked up when compiling,
> but should the name of the macro be clearer as to the assumptions it makes?
> Or perhaps take an argument which is the type of ret?

kvm_call_hyp has always returned a u64, so no semantic has changed here.

Otherwise, your suggestion of specifying a return type is interesting,
but it also gives the programmer another chance to shoot itself in the
foot by not providing the return type corresponding to the function that
is called.

Unless we can extract the return type by pure magic, I'm not sure we
gain much.

Thanks,

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

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Marc Zyngier]

Hi Andrew,

On 09/01/2019 14:24, Andrew Murray wrote:
> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
>> When running VHE, there is no need to jump via some stub to perform
>> a "HYP" function call, as there is a single address space.
>>
>> Let's thus change kvm_call_hyp() and co to perform a direct call
>> in this case. Although this results in a bit of code expansion,
>> it allows the compiler to check for type compatibility, something
>> that we are missing so far.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index e54cb7c88a4e..df32edbadd69 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>>  void kvm_arm_resume_guest(struct kvm *kvm);
>>  
>>  u64 __kvm_call_hyp(void *hypfn, ...);
>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
>> +
>> +/*
>> + * The couple of isb() below are there to guarantee the same behaviour
>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
>> + * synchronization event.
>> + */
>> +#define kvm_call_hyp(f, ...)						\
>> +	do {								\
>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>> +			f(__VA_ARGS__);					\
>> +			isb();						\
>> +		} else {						\
>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
>> +		}							\
>> +	} while(0)
>> +
>> +#define kvm_call_hyp_ret(f, ...)					\
>> +	({								\
>> +		u64 ret;						\
>> +									\
>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>> +			ret = f(__VA_ARGS__);				\
> 
> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
> return a smaller type. I guess any issues would be picked up when compiling,
> but should the name of the macro be clearer as to the assumptions it makes?
> Or perhaps take an argument which is the type of ret?

kvm_call_hyp has always returned a u64, so no semantic has changed here.

Otherwise, your suggestion of specifying a return type is interesting,
but it also gives the programmer another chance to shoot itself in the
foot by not providing the return type corresponding to the function that
is called.

Unless we can extract the return type by pure magic, I'm not sure we
gain much.

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Julien Thierry]

On 09/01/2019 14:45, Marc Zyngier wrote:
> Hi Andrew,
> 
> On 09/01/2019 14:24, Andrew Murray wrote:
>> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
>>> When running VHE, there is no need to jump via some stub to perform
>>> a "HYP" function call, as there is a single address space.
>>>
>>> Let's thus change kvm_call_hyp() and co to perform a direct call
>>> in this case. Although this results in a bit of code expansion,
>>> it allows the compiler to check for type compatibility, something
>>> that we are missing so far.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index e54cb7c88a4e..df32edbadd69 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>>>  void kvm_arm_resume_guest(struct kvm *kvm);
>>>  
>>>  u64 __kvm_call_hyp(void *hypfn, ...);
>>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
>>> +
>>> +/*
>>> + * The couple of isb() below are there to guarantee the same behaviour
>>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
>>> + * synchronization event.
>>> + */
>>> +#define kvm_call_hyp(f, ...)						\
>>> +	do {								\
>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>> +			f(__VA_ARGS__);					\
>>> +			isb();						\
>>> +		} else {						\
>>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
>>> +		}							\
>>> +	} while(0)
>>> +
>>> +#define kvm_call_hyp_ret(f, ...)					\
>>> +	({								\
>>> +		u64 ret;						\
>>> +									\
>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>> +			ret = f(__VA_ARGS__);				\
>>
>> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
>> return a smaller type. I guess any issues would be picked up when compiling,
>> but should the name of the macro be clearer as to the assumptions it makes?
>> Or perhaps take an argument which is the type of ret?
> 
> kvm_call_hyp has always returned a u64, so no semantic has changed here.
> 
> Otherwise, your suggestion of specifying a return type is interesting,
> but it also gives the programmer another chance to shoot itself in the
> foot by not providing the return type corresponding to the function that
> is called.
> 
> Unless we can extract the return type by pure magic, I'm not sure we
> gain much.
> 

Would the following work?

	typeof(f(__VA_ARGS__)) ret;

If typeof works anything like sizeof, I'd expect it would evaluate stuff
passed as argument and we'd have the return type of the function.

Cheers,

-- 
Julien Thierry

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Julien Thierry]

On 09/01/2019 14:45, Marc Zyngier wrote:
> Hi Andrew,
> 
> On 09/01/2019 14:24, Andrew Murray wrote:
>> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
>>> When running VHE, there is no need to jump via some stub to perform
>>> a "HYP" function call, as there is a single address space.
>>>
>>> Let's thus change kvm_call_hyp() and co to perform a direct call
>>> in this case. Although this results in a bit of code expansion,
>>> it allows the compiler to check for type compatibility, something
>>> that we are missing so far.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index e54cb7c88a4e..df32edbadd69 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>>>  void kvm_arm_resume_guest(struct kvm *kvm);
>>>  
>>>  u64 __kvm_call_hyp(void *hypfn, ...);
>>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
>>> +
>>> +/*
>>> + * The couple of isb() below are there to guarantee the same behaviour
>>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
>>> + * synchronization event.
>>> + */
>>> +#define kvm_call_hyp(f, ...)						\
>>> +	do {								\
>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>> +			f(__VA_ARGS__);					\
>>> +			isb();						\
>>> +		} else {						\
>>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
>>> +		}							\
>>> +	} while(0)
>>> +
>>> +#define kvm_call_hyp_ret(f, ...)					\
>>> +	({								\
>>> +		u64 ret;						\
>>> +									\
>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>> +			ret = f(__VA_ARGS__);				\
>>
>> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
>> return a smaller type. I guess any issues would be picked up when compiling,
>> but should the name of the macro be clearer as to the assumptions it makes?
>> Or perhaps take an argument which is the type of ret?
> 
> kvm_call_hyp has always returned a u64, so no semantic has changed here.
> 
> Otherwise, your suggestion of specifying a return type is interesting,
> but it also gives the programmer another chance to shoot itself in the
> foot by not providing the return type corresponding to the function that
> is called.
> 
> Unless we can extract the return type by pure magic, I'm not sure we
> gain much.
> 

Would the following work?

	typeof(f(__VA_ARGS__)) ret;

If typeof works anything like sizeof, I'd expect it would evaluate stuff
passed as argument and we'd have the return type of the function.

Cheers,

-- 
Julien Thierry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Julien Thierry]

On 09/01/2019 14:51, Julien Thierry wrote:
> 
> 
> On 09/01/2019 14:45, Marc Zyngier wrote:
>> Hi Andrew,
>>
>> On 09/01/2019 14:24, Andrew Murray wrote:
>>> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
>>>> When running VHE, there is no need to jump via some stub to perform
>>>> a "HYP" function call, as there is a single address space.
>>>>
>>>> Let's thus change kvm_call_hyp() and co to perform a direct call
>>>> in this case. Although this results in a bit of code expansion,
>>>> it allows the compiler to check for type compatibility, something
>>>> that we are missing so far.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>>>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>>> index e54cb7c88a4e..df32edbadd69 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>>>>  void kvm_arm_resume_guest(struct kvm *kvm);
>>>>  
>>>>  u64 __kvm_call_hyp(void *hypfn, ...);
>>>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>>>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
>>>> +
>>>> +/*
>>>> + * The couple of isb() below are there to guarantee the same behaviour
>>>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
>>>> + * synchronization event.
>>>> + */
>>>> +#define kvm_call_hyp(f, ...)						\
>>>> +	do {								\
>>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>>> +			f(__VA_ARGS__);					\
>>>> +			isb();						\
>>>> +		} else {						\
>>>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
>>>> +		}							\
>>>> +	} while(0)
>>>> +
>>>> +#define kvm_call_hyp_ret(f, ...)					\
>>>> +	({								\
>>>> +		u64 ret;						\
>>>> +									\
>>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>>> +			ret = f(__VA_ARGS__);				\
>>>
>>> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
>>> return a smaller type. I guess any issues would be picked up when compiling,
>>> but should the name of the macro be clearer as to the assumptions it makes?
>>> Or perhaps take an argument which is the type of ret?
>>
>> kvm_call_hyp has always returned a u64, so no semantic has changed here.
>>
>> Otherwise, your suggestion of specifying a return type is interesting,
>> but it also gives the programmer another chance to shoot itself in the
>> foot by not providing the return type corresponding to the function that
>> is called.
>>
>> Unless we can extract the return type by pure magic, I'm not sure we
>> gain much.
>>
> 
> Would the following work?
> 
> 	typeof(f(__VA_ARGS__)) ret;
> 
> If typeof works anything like sizeof, I'd expect it would evaluate stuff

it wouldn't*

> passed as argument and we'd have the return type of the function.
> 
> Cheers,
> 

-- 
Julien Thierry

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Julien Thierry]

On 09/01/2019 14:51, Julien Thierry wrote:
> 
> 
> On 09/01/2019 14:45, Marc Zyngier wrote:
>> Hi Andrew,
>>
>> On 09/01/2019 14:24, Andrew Murray wrote:
>>> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
>>>> When running VHE, there is no need to jump via some stub to perform
>>>> a "HYP" function call, as there is a single address space.
>>>>
>>>> Let's thus change kvm_call_hyp() and co to perform a direct call
>>>> in this case. Although this results in a bit of code expansion,
>>>> it allows the compiler to check for type compatibility, something
>>>> that we are missing so far.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>>>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>>> index e54cb7c88a4e..df32edbadd69 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>>>>  void kvm_arm_resume_guest(struct kvm *kvm);
>>>>  
>>>>  u64 __kvm_call_hyp(void *hypfn, ...);
>>>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>>>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
>>>> +
>>>> +/*
>>>> + * The couple of isb() below are there to guarantee the same behaviour
>>>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
>>>> + * synchronization event.
>>>> + */
>>>> +#define kvm_call_hyp(f, ...)						\
>>>> +	do {								\
>>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>>> +			f(__VA_ARGS__);					\
>>>> +			isb();						\
>>>> +		} else {						\
>>>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
>>>> +		}							\
>>>> +	} while(0)
>>>> +
>>>> +#define kvm_call_hyp_ret(f, ...)					\
>>>> +	({								\
>>>> +		u64 ret;						\
>>>> +									\
>>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>>> +			ret = f(__VA_ARGS__);				\
>>>
>>> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
>>> return a smaller type. I guess any issues would be picked up when compiling,
>>> but should the name of the macro be clearer as to the assumptions it makes?
>>> Or perhaps take an argument which is the type of ret?
>>
>> kvm_call_hyp has always returned a u64, so no semantic has changed here.
>>
>> Otherwise, your suggestion of specifying a return type is interesting,
>> but it also gives the programmer another chance to shoot itself in the
>> foot by not providing the return type corresponding to the function that
>> is called.
>>
>> Unless we can extract the return type by pure magic, I'm not sure we
>> gain much.
>>
> 
> Would the following work?
> 
> 	typeof(f(__VA_ARGS__)) ret;
> 
> If typeof works anything like sizeof, I'd expect it would evaluate stuff

it wouldn't*

> passed as argument and we'd have the return type of the function.
> 
> Cheers,
> 

-- 
Julien Thierry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Marc Zyngier]

On 09/01/2019 14:51, Julien Thierry wrote:
> 
> 
> On 09/01/2019 14:45, Marc Zyngier wrote:
>> Hi Andrew,
>>
>> On 09/01/2019 14:24, Andrew Murray wrote:
>>> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
>>>> When running VHE, there is no need to jump via some stub to perform
>>>> a "HYP" function call, as there is a single address space.
>>>>
>>>> Let's thus change kvm_call_hyp() and co to perform a direct call
>>>> in this case. Although this results in a bit of code expansion,
>>>> it allows the compiler to check for type compatibility, something
>>>> that we are missing so far.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>>>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>>> index e54cb7c88a4e..df32edbadd69 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>>>>  void kvm_arm_resume_guest(struct kvm *kvm);
>>>>  
>>>>  u64 __kvm_call_hyp(void *hypfn, ...);
>>>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>>>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
>>>> +
>>>> +/*
>>>> + * The couple of isb() below are there to guarantee the same behaviour
>>>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
>>>> + * synchronization event.
>>>> + */
>>>> +#define kvm_call_hyp(f, ...)						\
>>>> +	do {								\
>>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>>> +			f(__VA_ARGS__);					\
>>>> +			isb();						\
>>>> +		} else {						\
>>>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
>>>> +		}							\
>>>> +	} while(0)
>>>> +
>>>> +#define kvm_call_hyp_ret(f, ...)					\
>>>> +	({								\
>>>> +		u64 ret;						\
>>>> +									\
>>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>>> +			ret = f(__VA_ARGS__);				\
>>>
>>> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
>>> return a smaller type. I guess any issues would be picked up when compiling,
>>> but should the name of the macro be clearer as to the assumptions it makes?
>>> Or perhaps take an argument which is the type of ret?
>>
>> kvm_call_hyp has always returned a u64, so no semantic has changed here.
>>
>> Otherwise, your suggestion of specifying a return type is interesting,
>> but it also gives the programmer another chance to shoot itself in the
>> foot by not providing the return type corresponding to the function that
>> is called.
>>
>> Unless we can extract the return type by pure magic, I'm not sure we
>> gain much.
>>
> 
> Would the following work?
> 
> 	typeof(f(__VA_ARGS__)) ret;
> 
> If typeof works anything like sizeof, I'd expect it would evaluate stuff
> passed as argument and we'd have the return type of the function.
And it actually works! Thanks for the awful tip! ;-)

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

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Marc Zyngier]

On 09/01/2019 14:51, Julien Thierry wrote:
> 
> 
> On 09/01/2019 14:45, Marc Zyngier wrote:
>> Hi Andrew,
>>
>> On 09/01/2019 14:24, Andrew Murray wrote:
>>> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
>>>> When running VHE, there is no need to jump via some stub to perform
>>>> a "HYP" function call, as there is a single address space.
>>>>
>>>> Let's thus change kvm_call_hyp() and co to perform a direct call
>>>> in this case. Although this results in a bit of code expansion,
>>>> it allows the compiler to check for type compatibility, something
>>>> that we are missing so far.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
>>>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>>> index e54cb7c88a4e..df32edbadd69 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
>>>>  void kvm_arm_resume_guest(struct kvm *kvm);
>>>>  
>>>>  u64 __kvm_call_hyp(void *hypfn, ...);
>>>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>>>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
>>>> +
>>>> +/*
>>>> + * The couple of isb() below are there to guarantee the same behaviour
>>>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
>>>> + * synchronization event.
>>>> + */
>>>> +#define kvm_call_hyp(f, ...)						\
>>>> +	do {								\
>>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>>> +			f(__VA_ARGS__);					\
>>>> +			isb();						\
>>>> +		} else {						\
>>>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
>>>> +		}							\
>>>> +	} while(0)
>>>> +
>>>> +#define kvm_call_hyp_ret(f, ...)					\
>>>> +	({								\
>>>> +		u64 ret;						\
>>>> +									\
>>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
>>>> +			ret = f(__VA_ARGS__);				\
>>>
>>> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
>>> return a smaller type. I guess any issues would be picked up when compiling,
>>> but should the name of the macro be clearer as to the assumptions it makes?
>>> Or perhaps take an argument which is the type of ret?
>>
>> kvm_call_hyp has always returned a u64, so no semantic has changed here.
>>
>> Otherwise, your suggestion of specifying a return type is interesting,
>> but it also gives the programmer another chance to shoot itself in the
>> foot by not providing the return type corresponding to the function that
>> is called.
>>
>> Unless we can extract the return type by pure magic, I'm not sure we
>> gain much.
>>
> 
> Would the following work?
> 
> 	typeof(f(__VA_ARGS__)) ret;
> 
> If typeof works anything like sizeof, I'd expect it would evaluate stuff
> passed as argument and we'd have the return type of the function.
And it actually works! Thanks for the awful tip! ;-)

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Andrew Murray]

On Wed, Jan 09, 2019 at 04:01:56PM +0000, Marc Zyngier wrote:
> On 09/01/2019 14:51, Julien Thierry wrote:
> > 
> > 
> > On 09/01/2019 14:45, Marc Zyngier wrote:
> >> Hi Andrew,
> >>
> >> On 09/01/2019 14:24, Andrew Murray wrote:
> >>> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
> >>>> When running VHE, there is no need to jump via some stub to perform
> >>>> a "HYP" function call, as there is a single address space.
> >>>>
> >>>> Let's thus change kvm_call_hyp() and co to perform a direct call
> >>>> in this case. Although this results in a bit of code expansion,
> >>>> it allows the compiler to check for type compatibility, something
> >>>> that we are missing so far.
> >>>>
> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >>>> ---
> >>>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
> >>>>  1 file changed, 30 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >>>> index e54cb7c88a4e..df32edbadd69 100644
> >>>> --- a/arch/arm64/include/asm/kvm_host.h
> >>>> +++ b/arch/arm64/include/asm/kvm_host.h
> >>>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
> >>>>  void kvm_arm_resume_guest(struct kvm *kvm);
> >>>>  
> >>>>  u64 __kvm_call_hyp(void *hypfn, ...);
> >>>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
> >>>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
> >>>> +
> >>>> +/*
> >>>> + * The couple of isb() below are there to guarantee the same behaviour
> >>>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
> >>>> + * synchronization event.
> >>>> + */
> >>>> +#define kvm_call_hyp(f, ...)						\
> >>>> +	do {								\
> >>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
> >>>> +			f(__VA_ARGS__);					\
> >>>> +			isb();						\
> >>>> +		} else {						\
> >>>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
> >>>> +		}							\
> >>>> +	} while(0)
> >>>> +
> >>>> +#define kvm_call_hyp_ret(f, ...)					\
> >>>> +	({								\
> >>>> +		u64 ret;						\
> >>>> +									\
> >>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
> >>>> +			ret = f(__VA_ARGS__);				\
> >>>
> >>> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
> >>> return a smaller type. I guess any issues would be picked up when compiling,
> >>> but should the name of the macro be clearer as to the assumptions it makes?
> >>> Or perhaps take an argument which is the type of ret?
> >>
> >> kvm_call_hyp has always returned a u64, so no semantic has changed here.

Ah I missed that!

> >>
> >> Otherwise, your suggestion of specifying a return type is interesting,
> >> but it also gives the programmer another chance to shoot itself in the
> >> foot by not providing the return type corresponding to the function that
> >> is called.
> >>
> >> Unless we can extract the return type by pure magic, I'm not sure we
> >> gain much.
> >>
> > 
> > Would the following work?
> > 
> > 	typeof(f(__VA_ARGS__)) ret;
> > 
> > If typeof works anything like sizeof, I'd expect it would evaluate stuff
> > passed as argument and we'd have the return type of the function.
> And it actually works! Thanks for the awful tip! ;-)

Awesome.

Thanks,

Andrew Murray

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

Re: [PATCH 2/3] arm64: KVM: Allow for direct call of HYP functions when using VHE

[Andrew Murray]

On Wed, Jan 09, 2019 at 04:01:56PM +0000, Marc Zyngier wrote:
> On 09/01/2019 14:51, Julien Thierry wrote:
> > 
> > 
> > On 09/01/2019 14:45, Marc Zyngier wrote:
> >> Hi Andrew,
> >>
> >> On 09/01/2019 14:24, Andrew Murray wrote:
> >>> On Wed, Jan 09, 2019 at 01:54:34PM +0000, Marc Zyngier wrote:
> >>>> When running VHE, there is no need to jump via some stub to perform
> >>>> a "HYP" function call, as there is a single address space.
> >>>>
> >>>> Let's thus change kvm_call_hyp() and co to perform a direct call
> >>>> in this case. Although this results in a bit of code expansion,
> >>>> it allows the compiler to check for type compatibility, something
> >>>> that we are missing so far.
> >>>>
> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >>>> ---
> >>>>  arch/arm64/include/asm/kvm_host.h | 32 +++++++++++++++++++++++++++++--
> >>>>  1 file changed, 30 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >>>> index e54cb7c88a4e..df32edbadd69 100644
> >>>> --- a/arch/arm64/include/asm/kvm_host.h
> >>>> +++ b/arch/arm64/include/asm/kvm_host.h
> >>>> @@ -370,8 +370,36 @@ void kvm_arm_halt_guest(struct kvm *kvm);
> >>>>  void kvm_arm_resume_guest(struct kvm *kvm);
> >>>>  
> >>>>  u64 __kvm_call_hyp(void *hypfn, ...);
> >>>> -#define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
> >>>> -#define kvm_call_hyp_ret(f, ...) kvm_call_hyp(f, ##__VA_ARGS__)
> >>>> +
> >>>> +/*
> >>>> + * The couple of isb() below are there to guarantee the same behaviour
> >>>> + * on VHE as on !VHE, where the eret to EL1 acts as a context
> >>>> + * synchronization event.
> >>>> + */
> >>>> +#define kvm_call_hyp(f, ...)						\
> >>>> +	do {								\
> >>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
> >>>> +			f(__VA_ARGS__);					\
> >>>> +			isb();						\
> >>>> +		} else {						\
> >>>> +			__kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \
> >>>> +		}							\
> >>>> +	} while(0)
> >>>> +
> >>>> +#define kvm_call_hyp_ret(f, ...)					\
> >>>> +	({								\
> >>>> +		u64 ret;						\
> >>>> +									\
> >>>> +		if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) {	\
> >>>> +			ret = f(__VA_ARGS__);				\
> >>>
> >>> __kvm_get_mdcr_el2 and __kvm_vcpu_run_nvhe don't return u64 type, they
> >>> return a smaller type. I guess any issues would be picked up when compiling,
> >>> but should the name of the macro be clearer as to the assumptions it makes?
> >>> Or perhaps take an argument which is the type of ret?
> >>
> >> kvm_call_hyp has always returned a u64, so no semantic has changed here.

Ah I missed that!

> >>
> >> Otherwise, your suggestion of specifying a return type is interesting,
> >> but it also gives the programmer another chance to shoot itself in the
> >> foot by not providing the return type corresponding to the function that
> >> is called.
> >>
> >> Unless we can extract the return type by pure magic, I'm not sure we
> >> gain much.
> >>
> > 
> > Would the following work?
> > 
> > 	typeof(f(__VA_ARGS__)) ret;
> > 
> > If typeof works anything like sizeof, I'd expect it would evaluate stuff
> > passed as argument and we'd have the return type of the function.
> And it actually works! Thanks for the awful tip! ;-)

Awesome.

Thanks,

Andrew Murray

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel