[PATCH v3 0/3] KVM: arm64: single step emulation instructions

[PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Alex Bennée]

Hi,

This is rev 3 of the series, practically the same than rev 2 but fixed
a return 1->0 in the kvm_run loop that Julien caught. I've added his
r-b tags to the other patches.

As usual revision details bellow the --- in each patch.

Alex Bennée (3):
  kvm: arm debug: introduce helper for single-step
  kvm: arm64: handle single-stepping trapped instructions
  kvm: arm64: handle single-step of userspace mmio instructions

 arch/arm/include/asm/kvm_host.h   |  5 +++++
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++
 arch/arm64/kvm/handle_exit.c      | 47 +++++++++++++++++++++++++++------------
 virt/kvm/arm/arm.c                |  3 +++
 5 files changed, 64 insertions(+), 14 deletions(-)

-- 
2.15.0

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

[PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Alex Bennée]

Hi,

This is rev 3 of the series, practically the same than rev 2 but fixed
a return 1->0 in the kvm_run loop that Julien caught. I've added his
r-b tags to the other patches.

As usual revision details bellow the --- in each patch.

Alex Benn?e (3):
  kvm: arm debug: introduce helper for single-step
  kvm: arm64: handle single-stepping trapped instructions
  kvm: arm64: handle single-step of userspace mmio instructions

 arch/arm/include/asm/kvm_host.h   |  5 +++++
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++
 arch/arm64/kvm/handle_exit.c      | 47 +++++++++++++++++++++++++++------------
 virt/kvm/arm/arm.c                |  3 +++
 5 files changed, 64 insertions(+), 14 deletions(-)

-- 
2.15.0

[PATCH v3 1/3] kvm: arm debug: introduce helper for single-step

[Alex Bennée]

After emulating instructions we may want return to user-space to
handle a single-step. If single-step is enabled the helper set the run
structure for return and returns true.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
---
v2
  - kvm_arm_maybe_return_debug -> kvm_arm_handle_step_debug
  - return bool, true if return to userspace is required
---
 arch/arm/include/asm/kvm_host.h   |  5 +++++
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 4a879f6ff13b..26a1ea6c6542 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -285,6 +285,11 @@ static inline void kvm_arm_init_debug(void) {}
 static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {}
+static inline bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu,
+					     struct kvm_run *run)
+{
+	return false;
+}
 
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e923b58606e2..bbfd6a2adb2b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -369,6 +369,7 @@ void kvm_arm_init_debug(void);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
+bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index dbadfaf850a7..95afd22a4634 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -221,3 +221,25 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 		}
 	}
 }
+
+
+/*
+ * When KVM has successfully emulated the instruction we might want to
+ * return to user space with a KVM_EXIT_DEBUG. We can only do this
+ * once the emulation is complete though so for userspace emulations
+ * we have to wait until we have re-entered KVM before calling this
+ * helper.
+ *
+ * Return true (and set exit_reason) to return to userspace or false
+ * if no further action required.
+ */
+
+bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
+		run->exit_reason = KVM_EXIT_DEBUG;
+		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
+		return true;
+	}
+	return false;
+}
-- 
2.15.0

[PATCH v3 1/3] kvm: arm debug: introduce helper for single-step

[Alex Bennée]

After emulating instructions we may want return to user-space to
handle a single-step. If single-step is enabled the helper set the run
structure for return and returns true.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
---
v2
  - kvm_arm_maybe_return_debug -> kvm_arm_handle_step_debug
  - return bool, true if return to userspace is required
---
 arch/arm/include/asm/kvm_host.h   |  5 +++++
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 4a879f6ff13b..26a1ea6c6542 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -285,6 +285,11 @@ static inline void kvm_arm_init_debug(void) {}
 static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {}
+static inline bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu,
+					     struct kvm_run *run)
+{
+	return false;
+}
 
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e923b58606e2..bbfd6a2adb2b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -369,6 +369,7 @@ void kvm_arm_init_debug(void);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
+bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index dbadfaf850a7..95afd22a4634 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -221,3 +221,25 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 		}
 	}
 }
+
+
+/*
+ * When KVM has successfully emulated the instruction we might want to
+ * return to user space with a KVM_EXIT_DEBUG. We can only do this
+ * once the emulation is complete though so for userspace emulations
+ * we have to wait until we have re-entered KVM before calling this
+ * helper.
+ *
+ * Return true (and set exit_reason) to return to userspace or false
+ * if no further action required.
+ */
+
+bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
+		run->exit_reason = KVM_EXIT_DEBUG;
+		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
+		return true;
+	}
+	return false;
+}
-- 
2.15.0

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

[PATCH v3 1/3] kvm: arm debug: introduce helper for single-step

[Alex Bennée]

After emulating instructions we may want return to user-space to
handle a single-step. If single-step is enabled the helper set the run
structure for return and returns true.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
---
v2
  - kvm_arm_maybe_return_debug -> kvm_arm_handle_step_debug
  - return bool, true if return to userspace is required
---
 arch/arm/include/asm/kvm_host.h   |  5 +++++
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 4a879f6ff13b..26a1ea6c6542 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -285,6 +285,11 @@ static inline void kvm_arm_init_debug(void) {}
 static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {}
+static inline bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu,
+					     struct kvm_run *run)
+{
+	return false;
+}
 
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e923b58606e2..bbfd6a2adb2b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -369,6 +369,7 @@ void kvm_arm_init_debug(void);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
+bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index dbadfaf850a7..95afd22a4634 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -221,3 +221,25 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 		}
 	}
 }
+
+
+/*
+ * When KVM has successfully emulated the instruction we might want to
+ * return to user space with a KVM_EXIT_DEBUG. We can only do this
+ * once the emulation is complete though so for userspace emulations
+ * we have to wait until we have re-entered KVM before calling this
+ * helper.
+ *
+ * Return true (and set exit_reason) to return to userspace or false
+ * if no further action required.
+ */
+
+bool kvm_arm_handle_step_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
+		run->exit_reason = KVM_EXIT_DEBUG;
+		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
+		return true;
+	}
+	return false;
+}
-- 
2.15.0

[PATCH v3 2/3] kvm: arm64: handle single-stepping trapped instructions

[Alex Bennée]

If we are using guest debug to single-step the guest we need to ensure
we exit after emulating the instruction. This only affects
instructions completely emulated by the kernel. For userspace emulated
instructions we need to exit and return to complete the emulation.

The kvm_arm_handle_step_debug() helper sets up the necessary exit
state if needed.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
---
v2
  - use helper from patch 1
  - if (handled > 0) instead of if (handled) so errors propagate
---
 arch/arm64/kvm/handle_exit.c | 47 +++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 7debb74843a0..af1c804742f6 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -178,6 +178,38 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 	return arm_exit_handlers[hsr_ec];
 }
 
+/*
+ * We may be single-stepping an emulated instruction. If the emulation
+ * has been completed in-kernel we can return to userspace with a
+ * KVM_EXIT_DEBUG, otherwise the userspace needs to complete its
+ * emulation first.
+ */
+
+static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	int handled;
+
+	/*
+	 * See ARM ARM B1.14.1: "Hyp traps on instructions
+	 * that fail their condition code check"
+	 */
+	if (!kvm_condition_valid(vcpu)) {
+		kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+		handled = 1;
+	} else {
+		exit_handle_fn exit_handler;
+
+		exit_handler = kvm_get_exit_handler(vcpu);
+		handled = exit_handler(vcpu, run);
+	}
+
+	/* helper sets exit_reason if we need to return to userspace */
+	if (handled > 0 && kvm_arm_handle_step_debug(vcpu, run))
+		handled = 0;
+
+	return handled;
+}
+
 /*
  * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on
  * proper exit to userspace.
@@ -185,8 +217,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		       int exception_index)
 {
-	exit_handle_fn exit_handler;
-
 	if (ARM_SERROR_PENDING(exception_index)) {
 		u8 hsr_ec = ESR_ELx_EC(kvm_vcpu_get_hsr(vcpu));
 
@@ -214,18 +244,7 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		kvm_inject_vabt(vcpu);
 		return 1;
 	case ARM_EXCEPTION_TRAP:
-		/*
-		 * See ARM ARM B1.14.1: "Hyp traps on instructions
-		 * that fail their condition code check"
-		 */
-		if (!kvm_condition_valid(vcpu)) {
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-			return 1;
-		}
-
-		exit_handler = kvm_get_exit_handler(vcpu);
-
-		return exit_handler(vcpu, run);
+		return handle_trap_exceptions(vcpu, run);
 	case ARM_EXCEPTION_HYP_GONE:
 		/*
 		 * EL2 has been reset to the hyp-stub. This happens when a guest
-- 
2.15.0

[PATCH v3 2/3] kvm: arm64: handle single-stepping trapped instructions

[Alex Bennée]

If we are using guest debug to single-step the guest we need to ensure
we exit after emulating the instruction. This only affects
instructions completely emulated by the kernel. For userspace emulated
instructions we need to exit and return to complete the emulation.

The kvm_arm_handle_step_debug() helper sets up the necessary exit
state if needed.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
---
v2
  - use helper from patch 1
  - if (handled > 0) instead of if (handled) so errors propagate
---
 arch/arm64/kvm/handle_exit.c | 47 +++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 7debb74843a0..af1c804742f6 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -178,6 +178,38 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 	return arm_exit_handlers[hsr_ec];
 }
 
+/*
+ * We may be single-stepping an emulated instruction. If the emulation
+ * has been completed in-kernel we can return to userspace with a
+ * KVM_EXIT_DEBUG, otherwise the userspace needs to complete its
+ * emulation first.
+ */
+
+static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	int handled;
+
+	/*
+	 * See ARM ARM B1.14.1: "Hyp traps on instructions
+	 * that fail their condition code check"
+	 */
+	if (!kvm_condition_valid(vcpu)) {
+		kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+		handled = 1;
+	} else {
+		exit_handle_fn exit_handler;
+
+		exit_handler = kvm_get_exit_handler(vcpu);
+		handled = exit_handler(vcpu, run);
+	}
+
+	/* helper sets exit_reason if we need to return to userspace */
+	if (handled > 0 && kvm_arm_handle_step_debug(vcpu, run))
+		handled = 0;
+
+	return handled;
+}
+
 /*
  * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on
  * proper exit to userspace.
@@ -185,8 +217,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		       int exception_index)
 {
-	exit_handle_fn exit_handler;
-
 	if (ARM_SERROR_PENDING(exception_index)) {
 		u8 hsr_ec = ESR_ELx_EC(kvm_vcpu_get_hsr(vcpu));
 
@@ -214,18 +244,7 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		kvm_inject_vabt(vcpu);
 		return 1;
 	case ARM_EXCEPTION_TRAP:
-		/*
-		 * See ARM ARM B1.14.1: "Hyp traps on instructions
-		 * that fail their condition code check"
-		 */
-		if (!kvm_condition_valid(vcpu)) {
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-			return 1;
-		}
-
-		exit_handler = kvm_get_exit_handler(vcpu);
-
-		return exit_handler(vcpu, run);
+		return handle_trap_exceptions(vcpu, run);
 	case ARM_EXCEPTION_HYP_GONE:
 		/*
 		 * EL2 has been reset to the hyp-stub. This happens when a guest
-- 
2.15.0

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

[PATCH v3 2/3] kvm: arm64: handle single-stepping trapped instructions

[Alex Bennée]

If we are using guest debug to single-step the guest we need to ensure
we exit after emulating the instruction. This only affects
instructions completely emulated by the kernel. For userspace emulated
instructions we need to exit and return to complete the emulation.

The kvm_arm_handle_step_debug() helper sets up the necessary exit
state if needed.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
Reviewed-by: Julien Thierry <julien.thierry@arm.com>
---
v2
  - use helper from patch 1
  - if (handled > 0) instead of if (handled) so errors propagate
---
 arch/arm64/kvm/handle_exit.c | 47 +++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 7debb74843a0..af1c804742f6 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -178,6 +178,38 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 	return arm_exit_handlers[hsr_ec];
 }
 
+/*
+ * We may be single-stepping an emulated instruction. If the emulation
+ * has been completed in-kernel we can return to userspace with a
+ * KVM_EXIT_DEBUG, otherwise the userspace needs to complete its
+ * emulation first.
+ */
+
+static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	int handled;
+
+	/*
+	 * See ARM ARM B1.14.1: "Hyp traps on instructions
+	 * that fail their condition code check"
+	 */
+	if (!kvm_condition_valid(vcpu)) {
+		kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+		handled = 1;
+	} else {
+		exit_handle_fn exit_handler;
+
+		exit_handler = kvm_get_exit_handler(vcpu);
+		handled = exit_handler(vcpu, run);
+	}
+
+	/* helper sets exit_reason if we need to return to userspace */
+	if (handled > 0 && kvm_arm_handle_step_debug(vcpu, run))
+		handled = 0;
+
+	return handled;
+}
+
 /*
  * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on
  * proper exit to userspace.
@@ -185,8 +217,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		       int exception_index)
 {
-	exit_handle_fn exit_handler;
-
 	if (ARM_SERROR_PENDING(exception_index)) {
 		u8 hsr_ec = ESR_ELx_EC(kvm_vcpu_get_hsr(vcpu));
 
@@ -214,18 +244,7 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		kvm_inject_vabt(vcpu);
 		return 1;
 	case ARM_EXCEPTION_TRAP:
-		/*
-		 * See ARM ARM B1.14.1: "Hyp traps on instructions
-		 * that fail their condition code check"
-		 */
-		if (!kvm_condition_valid(vcpu)) {
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-			return 1;
-		}
-
-		exit_handler = kvm_get_exit_handler(vcpu);
-
-		return exit_handler(vcpu, run);
+		return handle_trap_exceptions(vcpu, run);
 	case ARM_EXCEPTION_HYP_GONE:
 		/*
 		 * EL2 has been reset to the hyp-stub. This happens when a guest
-- 
2.15.0

[PATCH v3 3/3] kvm: arm64: handle single-step of userspace mmio instructions

[Alex Bennée]

The system state of KVM when using userspace emulation is not complete
until we return into KVM_RUN. To handle mmio related updates we wait
until they have been committed and then schedule our KVM_EXIT_DEBUG.

The kvm_arm_handle_step_debug() helper tells us if we need to return
and sets up the exit_reason for us.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - call helper directly from kvm_arch_vcpu_ioctl_run
v3
  - return 0 (ioctl success) instead of 1
---
 virt/kvm/arm/arm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 95cba0799828..b40440defca1 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -625,6 +625,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
 		if (ret)
 			return ret;
+		if (kvm_arm_handle_step_debug(vcpu, vcpu->run))
+			return 0;
+
 	}
 
 	if (run->immediate_exit)
-- 
2.15.0

[PATCH v3 3/3] kvm: arm64: handle single-step of userspace mmio instructions

[Alex Bennée]

The system state of KVM when using userspace emulation is not complete
until we return into KVM_RUN. To handle mmio related updates we wait
until they have been committed and then schedule our KVM_EXIT_DEBUG.

The kvm_arm_handle_step_debug() helper tells us if we need to return
and sets up the exit_reason for us.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - call helper directly from kvm_arch_vcpu_ioctl_run
v3
  - return 0 (ioctl success) instead of 1
---
 virt/kvm/arm/arm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 95cba0799828..b40440defca1 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -625,6 +625,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
 		if (ret)
 			return ret;
+		if (kvm_arm_handle_step_debug(vcpu, vcpu->run))
+			return 0;
+
 	}
 
 	if (run->immediate_exit)
-- 
2.15.0

[PATCH v3 3/3] kvm: arm64: handle single-step of userspace mmio instructions

[Alex Bennée]

The system state of KVM when using userspace emulation is not complete
until we return into KVM_RUN. To handle mmio related updates we wait
until they have been committed and then schedule our KVM_EXIT_DEBUG.

The kvm_arm_handle_step_debug() helper tells us if we need to return
and sets up the exit_reason for us.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>

---
v2
  - call helper directly from kvm_arch_vcpu_ioctl_run
v3
  - return 0 (ioctl success) instead of 1
---
 virt/kvm/arm/arm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 95cba0799828..b40440defca1 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -625,6 +625,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
 		if (ret)
 			return ret;
+		if (kvm_arm_handle_step_debug(vcpu, vcpu->run))
+			return 0;
+
 	}
 
 	if (run->immediate_exit)
-- 
2.15.0

Re: [PATCH v3 3/3] kvm: arm64: handle single-step of userspace mmio instructions

[Julien Thierry]

On 16/11/17 15:39, Alex Bennée wrote:
> The system state of KVM when using userspace emulation is not complete
> until we return into KVM_RUN. To handle mmio related updates we wait
> until they have been committed and then schedule our KVM_EXIT_DEBUG.
> 
> The kvm_arm_handle_step_debug() helper tells us if we need to return
> and sets up the exit_reason for us.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

> ---
> v2
>    - call helper directly from kvm_arch_vcpu_ioctl_run
> v3
>    - return 0 (ioctl success) instead of 1
> ---
>   virt/kvm/arm/arm.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 95cba0799828..b40440defca1 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -625,6 +625,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>   		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>   		if (ret)
>   			return ret;
> +		if (kvm_arm_handle_step_debug(vcpu, vcpu->run))
> +			return 0;
> +
>   	}
>   
>   	if (run->immediate_exit)
> 

-- 
Julien Thierry

Re: [PATCH v3 3/3] kvm: arm64: handle single-step of userspace mmio instructions

[Julien Thierry]

On 16/11/17 15:39, Alex Bennée wrote:
> The system state of KVM when using userspace emulation is not complete
> until we return into KVM_RUN. To handle mmio related updates we wait
> until they have been committed and then schedule our KVM_EXIT_DEBUG.
> 
> The kvm_arm_handle_step_debug() helper tells us if we need to return
> and sets up the exit_reason for us.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

> ---
> v2
>    - call helper directly from kvm_arch_vcpu_ioctl_run
> v3
>    - return 0 (ioctl success) instead of 1
> ---
>   virt/kvm/arm/arm.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 95cba0799828..b40440defca1 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -625,6 +625,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>   		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>   		if (ret)
>   			return ret;
> +		if (kvm_arm_handle_step_debug(vcpu, vcpu->run))
> +			return 0;
> +
>   	}
>   
>   	if (run->immediate_exit)
> 

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

[PATCH v3 3/3] kvm: arm64: handle single-step of userspace mmio instructions

[Julien Thierry]

On 16/11/17 15:39, Alex Benn?e wrote:
> The system state of KVM when using userspace emulation is not complete
> until we return into KVM_RUN. To handle mmio related updates we wait
> until they have been committed and then schedule our KVM_EXIT_DEBUG.
> 
> The kvm_arm_handle_step_debug() helper tells us if we need to return
> and sets up the exit_reason for us.
> 
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> 

Reviewed-by: Julien Thierry <julien.thierry@arm.com>

> ---
> v2
>    - call helper directly from kvm_arch_vcpu_ioctl_run
> v3
>    - return 0 (ioctl success) instead of 1
> ---
>   virt/kvm/arm/arm.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 95cba0799828..b40440defca1 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -625,6 +625,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>   		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>   		if (ret)
>   			return ret;
> +		if (kvm_arm_handle_step_debug(vcpu, vcpu->run))
> +			return 0;
> +
>   	}
>   
>   	if (run->immediate_exit)
> 

-- 
Julien Thierry

Re: [PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Christoffer Dall]

Hi Alex,

On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Bennée wrote:
> Hi,
> 
> This is rev 3 of the series, practically the same than rev 2 but fixed
> a return 1->0 in the kvm_run loop that Julien caught. I've added his
> r-b tags to the other patches.
> 
> As usual revision details bellow the --- in each patch.

Thanks for taking care of this.

I have applied the series and slightly tweaked the commit messages and
commentary.

Did we simply decide to not worry about exiting to userspace if we do
fast-path emulation, such as for the errata workaround and GIC
mashaling in switch.c ?

Thanks,
-Christoffer

> 
> Alex Bennée (3):
>   kvm: arm debug: introduce helper for single-step
>   kvm: arm64: handle single-stepping trapped instructions
>   kvm: arm64: handle single-step of userspace mmio instructions
> 
>  arch/arm/include/asm/kvm_host.h   |  5 +++++
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++
>  arch/arm64/kvm/handle_exit.c      | 47 +++++++++++++++++++++++++++------------
>  virt/kvm/arm/arm.c                |  3 +++
>  5 files changed, 64 insertions(+), 14 deletions(-)
> 
> -- 
> 2.15.0
> 

[PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Christoffer Dall]

Hi Alex,

On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Benn?e wrote:
> Hi,
> 
> This is rev 3 of the series, practically the same than rev 2 but fixed
> a return 1->0 in the kvm_run loop that Julien caught. I've added his
> r-b tags to the other patches.
> 
> As usual revision details bellow the --- in each patch.

Thanks for taking care of this.

I have applied the series and slightly tweaked the commit messages and
commentary.

Did we simply decide to not worry about exiting to userspace if we do
fast-path emulation, such as for the errata workaround and GIC
mashaling in switch.c ?

Thanks,
-Christoffer

> 
> Alex Benn?e (3):
>   kvm: arm debug: introduce helper for single-step
>   kvm: arm64: handle single-stepping trapped instructions
>   kvm: arm64: handle single-step of userspace mmio instructions
> 
>  arch/arm/include/asm/kvm_host.h   |  5 +++++
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++
>  arch/arm64/kvm/handle_exit.c      | 47 +++++++++++++++++++++++++++------------
>  virt/kvm/arm/arm.c                |  3 +++
>  5 files changed, 64 insertions(+), 14 deletions(-)
> 
> -- 
> 2.15.0
> 

Re: [PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Alex Bennée]

Christoffer Dall <cdall@linaro.org> writes:

> Hi Alex,
>
> On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Bennée wrote:
>> Hi,
>>
>> This is rev 3 of the series, practically the same than rev 2 but fixed
>> a return 1->0 in the kvm_run loop that Julien caught. I've added his
>> r-b tags to the other patches.
>>
>> As usual revision details bellow the --- in each patch.
>
> Thanks for taking care of this.
>
> I have applied the series and slightly tweaked the commit messages and
> commentary.
>
> Did we simply decide to not worry about exiting to userspace if we do
> fast-path emulation, such as for the errata workaround and GIC
> mashaling in switch.c ?

Hmm I'd forgotten about that - I figured it was all in handle_exit or
passed to userspace. So I guess in-hyp emulation is a 3rd class of
instruction emulation?

Which particular interface cases are we covering here?

I suspect if these are regions that are only accessed once the system is
up and running we are going to run into problems due to single stepping
while IRQs are enabled. But conceptually we just need to do the same
flag check after __skip_instr has done before deciding to fall out the
loop or goto again;

We would have to fake up HSR so handle_exit did the right thing on the
way out though. That seems a little icky....

>
> Thanks,
> -Christoffer
>
>>
>> Alex Bennée (3):
>>   kvm: arm debug: introduce helper for single-step
>>   kvm: arm64: handle single-stepping trapped instructions
>>   kvm: arm64: handle single-step of userspace mmio instructions
>>
>>  arch/arm/include/asm/kvm_host.h   |  5 +++++
>>  arch/arm64/include/asm/kvm_host.h |  1 +
>>  arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++
>>  arch/arm64/kvm/handle_exit.c      | 47 +++++++++++++++++++++++++++------------
>>  virt/kvm/arm/arm.c                |  3 +++
>>  5 files changed, 64 insertions(+), 14 deletions(-)
>>
>> --
>> 2.15.0
>>


--
Alex Bennée

[PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Alex Bennée]

Christoffer Dall <cdall@linaro.org> writes:

> Hi Alex,
>
> On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Benn?e wrote:
>> Hi,
>>
>> This is rev 3 of the series, practically the same than rev 2 but fixed
>> a return 1->0 in the kvm_run loop that Julien caught. I've added his
>> r-b tags to the other patches.
>>
>> As usual revision details bellow the --- in each patch.
>
> Thanks for taking care of this.
>
> I have applied the series and slightly tweaked the commit messages and
> commentary.
>
> Did we simply decide to not worry about exiting to userspace if we do
> fast-path emulation, such as for the errata workaround and GIC
> mashaling in switch.c ?

Hmm I'd forgotten about that - I figured it was all in handle_exit or
passed to userspace. So I guess in-hyp emulation is a 3rd class of
instruction emulation?

Which particular interface cases are we covering here?

I suspect if these are regions that are only accessed once the system is
up and running we are going to run into problems due to single stepping
while IRQs are enabled. But conceptually we just need to do the same
flag check after __skip_instr has done before deciding to fall out the
loop or goto again;

We would have to fake up HSR so handle_exit did the right thing on the
way out though. That seems a little icky....

>
> Thanks,
> -Christoffer
>
>>
>> Alex Benn?e (3):
>>   kvm: arm debug: introduce helper for single-step
>>   kvm: arm64: handle single-stepping trapped instructions
>>   kvm: arm64: handle single-step of userspace mmio instructions
>>
>>  arch/arm/include/asm/kvm_host.h   |  5 +++++
>>  arch/arm64/include/asm/kvm_host.h |  1 +
>>  arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++
>>  arch/arm64/kvm/handle_exit.c      | 47 +++++++++++++++++++++++++++------------
>>  virt/kvm/arm/arm.c                |  3 +++
>>  5 files changed, 64 insertions(+), 14 deletions(-)
>>
>> --
>> 2.15.0
>>


--
Alex Benn?e

Re: [PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Alex Bennée]

Christoffer Dall <cdall@linaro.org> writes:

> Hi Alex,
>
> On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Bennée wrote:
>> Hi,
>>
>> This is rev 3 of the series, practically the same than rev 2 but fixed
>> a return 1->0 in the kvm_run loop that Julien caught. I've added his
>> r-b tags to the other patches.
>>
>> As usual revision details bellow the --- in each patch.
>
> Thanks for taking care of this.
>
> I have applied the series and slightly tweaked the commit messages and
> commentary.
>
> Did we simply decide to not worry about exiting to userspace if we do
> fast-path emulation, such as for the errata workaround and GIC
> mashaling in switch.c ?

Compile tested only:

--8<---------------cut here---------------start------------->8---
kvm: arm64: handle single-step of hyp emulated mmio

There is a fast-path of MMIO emulation inside hyp mode. The handling
of single-step is broadly the same as kvm_arm_handle_step_debug()
except we just setup ESR/HSR so handle_exit() does the correct thing
as we exit.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 arch/arm64/kvm/hyp/switch.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 945e79c641c4..841dc79d11fd 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -263,7 +263,11 @@ static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
 	return true;
 }

-static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
+/* Skip an instruction which has been emulated. Returns true if
+ * execution can continue or false if we need to exit hyp mode because
+ * single-step was in effect.
+ */
+static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
 {
 	*vcpu_pc(vcpu) = read_sysreg_el2(elr);

@@ -276,6 +280,14 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
 	}

 	write_sysreg_el2(*vcpu_pc(vcpu), elr);
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
+		vcpu->arch.fault.esr_el2 =
+			(ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT) | 0x22;
+		return false;
+	} else {
+		return true;
+	}
 }

 int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
@@ -336,8 +348,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 			int ret = __vgic_v2_perform_cpuif_access(vcpu);

 			if (ret == 1) {
-				__skip_instr(vcpu);
-				goto again;
+				if (__skip_instr(vcpu))
+					goto again;
+				else
+					exit_code = ARM_EXCEPTION_TRAP;
 			}

 			if (ret == -1) {
@@ -357,8 +371,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 		int ret = __vgic_v3_perform_cpuif_access(vcpu);

 		if (ret == 1) {
-			__skip_instr(vcpu);
-			goto again;
+			if (__skip_instr(vcpu))
+				goto again;
+			else
+				exit_code = ARM_EXCEPTION_TRAP;
 		}

 		/* 0 falls through to be handled out of EL2 */
--8<---------------cut here---------------end--------------->8---
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Alex Bennée]

Christoffer Dall <cdall@linaro.org> writes:

> Hi Alex,
>
> On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Benn?e wrote:
>> Hi,
>>
>> This is rev 3 of the series, practically the same than rev 2 but fixed
>> a return 1->0 in the kvm_run loop that Julien caught. I've added his
>> r-b tags to the other patches.
>>
>> As usual revision details bellow the --- in each patch.
>
> Thanks for taking care of this.
>
> I have applied the series and slightly tweaked the commit messages and
> commentary.
>
> Did we simply decide to not worry about exiting to userspace if we do
> fast-path emulation, such as for the errata workaround and GIC
> mashaling in switch.c ?

Compile tested only:

--8<---------------cut here---------------start------------->8---
kvm: arm64: handle single-step of hyp emulated mmio

There is a fast-path of MMIO emulation inside hyp mode. The handling
of single-step is broadly the same as kvm_arm_handle_step_debug()
except we just setup ESR/HSR so handle_exit() does the correct thing
as we exit.

Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
---
 arch/arm64/kvm/hyp/switch.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 945e79c641c4..841dc79d11fd 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -263,7 +263,11 @@ static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
 	return true;
 }

-static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
+/* Skip an instruction which has been emulated. Returns true if
+ * execution can continue or false if we need to exit hyp mode because
+ * single-step was in effect.
+ */
+static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
 {
 	*vcpu_pc(vcpu) = read_sysreg_el2(elr);

@@ -276,6 +280,14 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
 	}

 	write_sysreg_el2(*vcpu_pc(vcpu), elr);
+
+	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
+		vcpu->arch.fault.esr_el2 =
+			(ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT) | 0x22;
+		return false;
+	} else {
+		return true;
+	}
 }

 int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
@@ -336,8 +348,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 			int ret = __vgic_v2_perform_cpuif_access(vcpu);

 			if (ret == 1) {
-				__skip_instr(vcpu);
-				goto again;
+				if (__skip_instr(vcpu))
+					goto again;
+				else
+					exit_code = ARM_EXCEPTION_TRAP;
 			}

 			if (ret == -1) {
@@ -357,8 +371,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 		int ret = __vgic_v3_perform_cpuif_access(vcpu);

 		if (ret == 1) {
-			__skip_instr(vcpu);
-			goto again;
+			if (__skip_instr(vcpu))
+				goto again;
+			else
+				exit_code = ARM_EXCEPTION_TRAP;
 		}

 		/* 0 falls through to be handled out of EL2 */
--8<---------------cut here---------------end--------------->8---

Re: [PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Marc Zyngier]

On 21/11/17 12:43, Alex Bennée wrote:
> 
> Christoffer Dall <cdall@linaro.org> writes:
> 
>> Hi Alex,
>>
>> On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Bennée wrote:
>>> Hi,
>>>
>>> This is rev 3 of the series, practically the same than rev 2 but fixed
>>> a return 1->0 in the kvm_run loop that Julien caught. I've added his
>>> r-b tags to the other patches.
>>>
>>> As usual revision details bellow the --- in each patch.
>>
>> Thanks for taking care of this.
>>
>> I have applied the series and slightly tweaked the commit messages and
>> commentary.
>>
>> Did we simply decide to not worry about exiting to userspace if we do
>> fast-path emulation, such as for the errata workaround and GIC
>> mashaling in switch.c ?
> 
> Compile tested only:
> 
> --8<---------------cut here---------------start------------->8---
> kvm: arm64: handle single-step of hyp emulated mmio
> 
> There is a fast-path of MMIO emulation inside hyp mode. The handling
> of single-step is broadly the same as kvm_arm_handle_step_debug()
> except we just setup ESR/HSR so handle_exit() does the correct thing
> as we exit.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  arch/arm64/kvm/hyp/switch.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 945e79c641c4..841dc79d11fd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -263,7 +263,11 @@ static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
>  	return true;
>  }
> 
> -static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
> +/* Skip an instruction which has been emulated. Returns true if
> + * execution can continue or false if we need to exit hyp mode because
> + * single-step was in effect.
> + */
> +static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>  {
>  	*vcpu_pc(vcpu) = read_sysreg_el2(elr);
> 
> @@ -276,6 +280,14 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>  	}
> 
>  	write_sysreg_el2(*vcpu_pc(vcpu), elr);
> +
> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> +		vcpu->arch.fault.esr_el2 =
> +			(ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT) | 0x22;
> +		return false;
> +	} else {
> +		return true;
> +	}
>  }
> 
>  int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> @@ -336,8 +348,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  			int ret = __vgic_v2_perform_cpuif_access(vcpu);
> 
>  			if (ret == 1) {
> -				__skip_instr(vcpu);
> -				goto again;
> +				if (__skip_instr(vcpu))
> +					goto again;
> +				else
> +					exit_code = ARM_EXCEPTION_TRAP;
>  			}
> 
>  			if (ret == -1) {

Here (in the -1 case), we should still report the step to userspace (it
took place after all), and signal the SError to the guest (because it is
being stupid).

	M. (I really hate that part of the code base...)
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Marc Zyngier]

On 21/11/17 12:43, Alex Benn?e wrote:
> 
> Christoffer Dall <cdall@linaro.org> writes:
> 
>> Hi Alex,
>>
>> On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Benn?e wrote:
>>> Hi,
>>>
>>> This is rev 3 of the series, practically the same than rev 2 but fixed
>>> a return 1->0 in the kvm_run loop that Julien caught. I've added his
>>> r-b tags to the other patches.
>>>
>>> As usual revision details bellow the --- in each patch.
>>
>> Thanks for taking care of this.
>>
>> I have applied the series and slightly tweaked the commit messages and
>> commentary.
>>
>> Did we simply decide to not worry about exiting to userspace if we do
>> fast-path emulation, such as for the errata workaround and GIC
>> mashaling in switch.c ?
> 
> Compile tested only:
> 
> --8<---------------cut here---------------start------------->8---
> kvm: arm64: handle single-step of hyp emulated mmio
> 
> There is a fast-path of MMIO emulation inside hyp mode. The handling
> of single-step is broadly the same as kvm_arm_handle_step_debug()
> except we just setup ESR/HSR so handle_exit() does the correct thing
> as we exit.
> 
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
>  arch/arm64/kvm/hyp/switch.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 945e79c641c4..841dc79d11fd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -263,7 +263,11 @@ static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
>  	return true;
>  }
> 
> -static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
> +/* Skip an instruction which has been emulated. Returns true if
> + * execution can continue or false if we need to exit hyp mode because
> + * single-step was in effect.
> + */
> +static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>  {
>  	*vcpu_pc(vcpu) = read_sysreg_el2(elr);
> 
> @@ -276,6 +280,14 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>  	}
> 
>  	write_sysreg_el2(*vcpu_pc(vcpu), elr);
> +
> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> +		vcpu->arch.fault.esr_el2 =
> +			(ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT) | 0x22;
> +		return false;
> +	} else {
> +		return true;
> +	}
>  }
> 
>  int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> @@ -336,8 +348,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  			int ret = __vgic_v2_perform_cpuif_access(vcpu);
> 
>  			if (ret == 1) {
> -				__skip_instr(vcpu);
> -				goto again;
> +				if (__skip_instr(vcpu))
> +					goto again;
> +				else
> +					exit_code = ARM_EXCEPTION_TRAP;
>  			}
> 
>  			if (ret == -1) {

Here (in the -1 case), we should still report the step to userspace (it
took place after all), and signal the SError to the guest (because it is
being stupid).

	M. (I really hate that part of the code base...)
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Christoffer Dall]

On Tue, Nov 21, 2017 at 12:43:12PM +0000, Alex Bennée wrote:
> 
> Christoffer Dall <cdall@linaro.org> writes:
> 
> > Hi Alex,
> >
> > On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Bennée wrote:
> >> Hi,
> >>
> >> This is rev 3 of the series, practically the same than rev 2 but fixed
> >> a return 1->0 in the kvm_run loop that Julien caught. I've added his
> >> r-b tags to the other patches.
> >>
> >> As usual revision details bellow the --- in each patch.
> >
> > Thanks for taking care of this.
> >
> > I have applied the series and slightly tweaked the commit messages and
> > commentary.
> >
> > Did we simply decide to not worry about exiting to userspace if we do
> > fast-path emulation, such as for the errata workaround and GIC
> > mashaling in switch.c ?
> 
> Compile tested only:
> 
> --8<---------------cut here---------------start------------->8---
> kvm: arm64: handle single-step of hyp emulated mmio
> 
> There is a fast-path of MMIO emulation inside hyp mode. The handling
> of single-step is broadly the same as kvm_arm_handle_step_debug()
> except we just setup ESR/HSR so handle_exit() does the correct thing
> as we exit.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  arch/arm64/kvm/hyp/switch.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 945e79c641c4..841dc79d11fd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -263,7 +263,11 @@ static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
>  	return true;
>  }
> 
> -static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
> +/* Skip an instruction which has been emulated. Returns true if
> + * execution can continue or false if we need to exit hyp mode because
> + * single-step was in effect.
> + */
> +static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>  {
>  	*vcpu_pc(vcpu) = read_sysreg_el2(elr);
> 
> @@ -276,6 +280,14 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>  	}
> 
>  	write_sysreg_el2(*vcpu_pc(vcpu), elr);
> +
> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> +		vcpu->arch.fault.esr_el2 =
> +			(ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT) | 0x22;
> +		return false;
> +	} else {
> +		return true;
> +	}
>  }
> 
>  int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> @@ -336,8 +348,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  			int ret = __vgic_v2_perform_cpuif_access(vcpu);
> 
>  			if (ret == 1) {
> -				__skip_instr(vcpu);
> -				goto again;
> +				if (__skip_instr(vcpu))
> +					goto again;
> +				else
> +					exit_code = ARM_EXCEPTION_TRAP;
>  			}
> 
>  			if (ret == -1) {
> @@ -357,8 +371,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  		int ret = __vgic_v3_perform_cpuif_access(vcpu);
> 
>  		if (ret == 1) {
> -			__skip_instr(vcpu);
> -			goto again;
> +			if (__skip_instr(vcpu))
> +				goto again;
> +			else
> +				exit_code = ARM_EXCEPTION_TRAP;
>  		}
> 
>  		/* 0 falls through to be handled out of EL2 */
> --8<---------------cut here---------------end--------------->8---

Assuming you fix Marc's comment this looks reasonable to me.  Please
send as a separate patch.

Thanks,
-Christoffer

[PATCH v3 0/3] KVM: arm64: single step emulation instructions

[Christoffer Dall]

On Tue, Nov 21, 2017 at 12:43:12PM +0000, Alex Benn?e wrote:
> 
> Christoffer Dall <cdall@linaro.org> writes:
> 
> > Hi Alex,
> >
> > On Thu, Nov 16, 2017 at 03:39:18PM +0000, Alex Benn?e wrote:
> >> Hi,
> >>
> >> This is rev 3 of the series, practically the same than rev 2 but fixed
> >> a return 1->0 in the kvm_run loop that Julien caught. I've added his
> >> r-b tags to the other patches.
> >>
> >> As usual revision details bellow the --- in each patch.
> >
> > Thanks for taking care of this.
> >
> > I have applied the series and slightly tweaked the commit messages and
> > commentary.
> >
> > Did we simply decide to not worry about exiting to userspace if we do
> > fast-path emulation, such as for the errata workaround and GIC
> > mashaling in switch.c ?
> 
> Compile tested only:
> 
> --8<---------------cut here---------------start------------->8---
> kvm: arm64: handle single-step of hyp emulated mmio
> 
> There is a fast-path of MMIO emulation inside hyp mode. The handling
> of single-step is broadly the same as kvm_arm_handle_step_debug()
> except we just setup ESR/HSR so handle_exit() does the correct thing
> as we exit.
> 
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
>  arch/arm64/kvm/hyp/switch.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 945e79c641c4..841dc79d11fd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -263,7 +263,11 @@ static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
>  	return true;
>  }
> 
> -static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
> +/* Skip an instruction which has been emulated. Returns true if
> + * execution can continue or false if we need to exit hyp mode because
> + * single-step was in effect.
> + */
> +static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>  {
>  	*vcpu_pc(vcpu) = read_sysreg_el2(elr);
> 
> @@ -276,6 +280,14 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>  	}
> 
>  	write_sysreg_el2(*vcpu_pc(vcpu), elr);
> +
> +	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> +		vcpu->arch.fault.esr_el2 =
> +			(ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT) | 0x22;
> +		return false;
> +	} else {
> +		return true;
> +	}
>  }
> 
>  int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> @@ -336,8 +348,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  			int ret = __vgic_v2_perform_cpuif_access(vcpu);
> 
>  			if (ret == 1) {
> -				__skip_instr(vcpu);
> -				goto again;
> +				if (__skip_instr(vcpu))
> +					goto again;
> +				else
> +					exit_code = ARM_EXCEPTION_TRAP;
>  			}
> 
>  			if (ret == -1) {
> @@ -357,8 +371,10 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  		int ret = __vgic_v3_perform_cpuif_access(vcpu);
> 
>  		if (ret == 1) {
> -			__skip_instr(vcpu);
> -			goto again;
> +			if (__skip_instr(vcpu))
> +				goto again;
> +			else
> +				exit_code = ARM_EXCEPTION_TRAP;
>  		}
> 
>  		/* 0 falls through to be handled out of EL2 */
> --8<---------------cut here---------------end--------------->8---

Assuming you fix Marc's comment this looks reasonable to me.  Please
send as a separate patch.

Thanks,
-Christoffer