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

[PATCH v2 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>

---
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   |  2 ++
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 4a879f6ff13b..a2e881d6108e 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -285,6 +285,8 @@ 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) {}
 
 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.14.2

[PATCH] fixup! kvm: arm debug: introduce helper for single-step

[Alex Bennée]

---
 arch/arm/include/asm/kvm_host.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index a2e881d6108e..26a1ea6c6542 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -286,7 +286,10 @@ 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) {}
+					     struct kvm_run *run)
+{
+	return false;
+}
 
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
-- 
2.14.2

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

[Julien Thierry]

On 09/11/17 17:00, Alex Bennée wrote:
> 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>

With the fixup:

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   |  2 ++
>   arch/arm64/include/asm/kvm_host.h |  1 +
>   arch/arm64/kvm/debug.c            | 22 ++++++++++++++++++++++
>   3 files changed, 25 insertions(+)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 4a879f6ff13b..a2e881d6108e 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -285,6 +285,8 @@ 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) {}
>   
>   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;
> +}
> 

-- 
Julien Thierry

[PATCH v2 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>

---
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.14.2

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

[Julien Thierry]

On 09/11/17 17:00, Alex Bennée wrote:
> 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
> 

-- 
Julien Thierry

[PATCH v2 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
---
 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..2991adfaca9d 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 1;
+
 	}
 
 	if (run->immediate_exit)
-- 
2.14.2

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

[Julien Thierry]

Hi Alex,

On 09/11/17 17:00, 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>
> 
> ---
> v2
>    - call helper directly from kvm_arch_vcpu_ioctl_run
> ---
>   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..2991adfaca9d 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 1;
> +

In the previous patch, kvm_arch_vcpu_ioctl_run return 0 when telling 
userspace about a debug exception. Shouldn't this branch return 0 
instead of 1?

Returning on non-zero for kvm_handle_mmio_return is done because it 
means there was an error. This is not the case for 
kvm_arm_handle_step_debug.

The description in the comment of kvm_arch_vcpu_ioctl_run is not very 
clear whether non-zero result should be used for errors or if only the 
negative values are treated as such, and positive values seems to be 
generally used to keep the vcpu going. So, I thought it might make sense 
to always return the same value upon debug exceptions.

Cheers,

-- 
Julien Thierry

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

[Alex Bennée]

Julien Thierry <julien.thierry@arm.com> writes:

> Hi Alex,
>
> On 09/11/17 17:00, 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>
>>
>> ---
>> v2
>>    - call helper directly from kvm_arch_vcpu_ioctl_run
>> ---
>>   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..2991adfaca9d 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 1;
>> +
>
> In the previous patch, kvm_arch_vcpu_ioctl_run return 0 when telling
> userspace about a debug exception. Shouldn't this branch return 0
> instead of 1?

Probably - although in practice it makes no difference. In QEMU for
example the test is if (run_ret < 0) to handle errors. Otherwise success
is assumed.

> Returning on non-zero for kvm_handle_mmio_return is done because it
> means there was an error. This is not the case for
> kvm_arm_handle_step_debug.
>
> The description in the comment of kvm_arch_vcpu_ioctl_run is not very
> clear whether non-zero result should be used for errors or if only the
> negative values are treated as such, and positive values seems to be
> generally used to keep the vcpu going. So, I thought it might make
> sense to always return the same value upon debug exceptions.

There is a subtle mis-match between what gets passed back to the ioctl
and what terminates the while() loop later on. As far as the ioctl is
concerned it's 0 success and - error. Once you get to the while loop
you'll only ever exit once ret is no longer > 0.

Anyway for consistency we should certainly return 0, I'll fix it on the
next iteration.

>
> Cheers,


--
Alex Bennée