[PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions

[PATCH v1 1/2] 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.

We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows
it was a single-step event (and without altering the userspace ABI).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 7debb74843a0..c918d291cb58 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 	return arm_exit_handlers[hsr_ec];
 }
 
+/*
+ * When handling traps we need to ensure exit the guest if we
+ * completely emulated the instruction while single-stepping. Stuff to
+ * be emulated in userspace needs to complete that 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);
+	}
+
+	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
+		handled = 0;
+		run->exit_reason = KVM_EXIT_DEBUG;
+		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
+	}
+
+	return handled;
+}
+
 /*
  * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on
  * proper exit to userspace.
@@ -185,8 +218,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 +245,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.1

Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions

[Marc Zyngier]

On 06/10/17 12:39, 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.
> 
> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows
> it was a single-step event (and without altering the userspace ABI).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 7debb74843a0..c918d291cb58 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>  	return arm_exit_handlers[hsr_ec];
>  }
>  
> +/*
> + * When handling traps we need to ensure exit the guest if we
> + * completely emulated the instruction while single-stepping. Stuff to
> + * be emulated in userspace needs to complete that 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);
> +	}
> +
> +	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
> +		handled = 0;
> +		run->exit_reason = KVM_EXIT_DEBUG;
> +		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
> +	}

Doesn't this break an MMIO read? The registers haven't been updated yet,
and the debugger may not see the right thing...

How about something like:

	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
		if (run->exit_reason == KVM_EXIT_MMIO)
			kvm_handle_mmio_return(vcpu, run);
		[...]
	}

Or am I missing something?

Thanks,

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

Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions

[Julien Thierry]

On 06/10/17 13:27, Marc Zyngier wrote:
> On 06/10/17 12:39, 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.
>>
>> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows
>> it was a single-step event (and without altering the userspace ABI).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++-------------
>>   1 file changed, 34 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 7debb74843a0..c918d291cb58 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>>   	return arm_exit_handlers[hsr_ec];
>>   }
>>   
>> +/*
>> + * When handling traps we need to ensure exit the guest if we
>> + * completely emulated the instruction while single-stepping. Stuff to
>> + * be emulated in userspace needs to complete that 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);
>> +	}
>> +
>> +	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>> +		handled = 0;
>> +		run->exit_reason = KVM_EXIT_DEBUG;
>> +		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>> +	}
> 
> Doesn't this break an MMIO read? The registers haven't been updated yet,
> and the debugger may not see the right thing...
> 
> How about something like:
> 
> 	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
> 		if (run->exit_reason == KVM_EXIT_MMIO)
> 			kvm_handle_mmio_return(vcpu, run);
> 		[...]
> 	}
> 
> Or am I missing something?

If the MMIO was not handled by the kernel, exit_handler will return 0, 
so handled will be false and we won't pretend we have a debug exception 
(but will still return to userland with KVM_EXIT_MMIO).

I think the second patch takes care of properly handling single step for 
userland MMIO.

-- 
Julien Thierry

Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions

[Marc Zyngier]

On 06/10/17 13:34, Julien Thierry wrote:
> 
> 
> On 06/10/17 13:27, Marc Zyngier wrote:
>> On 06/10/17 12:39, 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.
>>>
>>> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows
>>> it was a single-step event (and without altering the userspace ABI).
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>   arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++-------------
>>>   1 file changed, 34 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>>> index 7debb74843a0..c918d291cb58 100644
>>> --- a/arch/arm64/kvm/handle_exit.c
>>> +++ b/arch/arm64/kvm/handle_exit.c
>>> @@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>>>   	return arm_exit_handlers[hsr_ec];
>>>   }
>>>   
>>> +/*
>>> + * When handling traps we need to ensure exit the guest if we
>>> + * completely emulated the instruction while single-stepping. Stuff to
>>> + * be emulated in userspace needs to complete that 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);
>>> +	}
>>> +
>>> +	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>>> +		handled = 0;
>>> +		run->exit_reason = KVM_EXIT_DEBUG;
>>> +		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>>> +	}
>>
>> Doesn't this break an MMIO read? The registers haven't been updated yet,
>> and the debugger may not see the right thing...
>>
>> How about something like:
>>
>> 	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>> 		if (run->exit_reason == KVM_EXIT_MMIO)
>> 			kvm_handle_mmio_return(vcpu, run);
>> 		[...]
>> 	}
>>
>> Or am I missing something?
> 
> If the MMIO was not handled by the kernel, exit_handler will return 0, 
> so handled will be false and we won't pretend we have a debug exception 
> (but will still return to userland with KVM_EXIT_MMIO).
> 
> I think the second patch takes care of properly handling single step for 
> userland MMIO.

Indeed, I was just confused. We do have a kvm_handle_mmio_return in the
vgic emulation, so it is all taken care off at this stage. Blimey.

Thanks,

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

Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions

[Julien Thierry]

On 06/10/17 12:39, 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.
> 
> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows
> it was a single-step event (and without altering the userspace ABI).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

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

Thanks,

-- 
Julien Thierry

Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions

[Christoffer Dall]

On Fri, Oct 06, 2017 at 12:39:20PM +0100, 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.
> 
> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows
> it was a single-step event (and without altering the userspace ABI).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 7debb74843a0..c918d291cb58 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>  	return arm_exit_handlers[hsr_ec];
>  }
>  
> +/*
> + * When handling traps we need to ensure exit the guest if we
> + * completely emulated the instruction while single-stepping. Stuff to
> + * be emulated in userspace needs to complete that first.
> + */

I really don't understand the first sentence here.  We are already out
of the guest, so do you mean a return to userspace?

I think the second sentence could be more clear as well.  Is 'stuff' not
actually 'MMIO emulation' or 'emulation' more broadly?

> +
> +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);
> +	}
> +
> +	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {

Don't you want if (handled == 1) or if (handled > 0) ?

If there was an error I think we want to just return that to userspace
and not override it and present single-stepping.

> +		handled = 0;
> +		run->exit_reason = KVM_EXIT_DEBUG;
> +		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
> +	}
> +
> +	return handled;
> +}
> +
>  /*
>   * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on
>   * proper exit to userspace.
> @@ -185,8 +218,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 +245,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.1
> 

Thanks,
-Christoffer

Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions

[Alex Bennée]

Christoffer Dall <cdall@linaro.org> writes:

> On Fri, Oct 06, 2017 at 12:39:20PM +0100, 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.
>>
>> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows
>> it was a single-step event (and without altering the userspace ABI).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++-------------
>>  1 file changed, 34 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 7debb74843a0..c918d291cb58 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>>  	return arm_exit_handlers[hsr_ec];
>>  }
>>
>> +/*
>> + * When handling traps we need to ensure exit the guest if we
>> + * completely emulated the instruction while single-stepping. Stuff to
>> + * be emulated in userspace needs to complete that first.
>> + */
>
> I really don't understand the first sentence here.  We are already out
> of the guest, so do you mean a return to userspace?
> I think the second sentence could be more clear as well.  Is 'stuff' not
> actually 'MMIO emulation' or 'emulation' more broadly?

Your right - it's sloppily worded how about:

 /*
  * 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 it's
  * emulation first.
  */

For x86 there is also IO emulation but in principle anything that might
be passed off to userspace to be completed should be done 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);
>> +	}
>> +
>> +	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>
> Don't you want if (handled == 1) or if (handled > 0) ?
>
> If there was an error I think we want to just return that to userspace
> and not override it and present single-stepping.

Yes, I'll fix it.

>
>> +		handled = 0;
>> +		run->exit_reason = KVM_EXIT_DEBUG;
>> +		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>> +	}
>> +
>> +	return handled;
>> +}
>> +
>>  /*
>>   * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on
>>   * proper exit to userspace.
>> @@ -185,8 +218,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 +245,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.1
>>
>
> Thanks,
> -Christoffer


--
Alex Bennée

Re: [PATCH v1 1/2] KVM: arm64: handle single-stepping trapped instructions

[Christoffer Dall]

On Fri, Oct 13, 2017 at 10:15:09AM +0100, Alex Bennée wrote:
> 
> Christoffer Dall <cdall@linaro.org> writes:
> 
> > On Fri, Oct 06, 2017 at 12:39:20PM +0100, 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.
> >>
> >> We fake debug.arch.hsr to contain ESR_ELx_EC_SOFTSTP_LOW so QEMU knows
> >> it was a single-step event (and without altering the userspace ABI).
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  arch/arm64/kvm/handle_exit.c | 48 +++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 34 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> >> index 7debb74843a0..c918d291cb58 100644
> >> --- a/arch/arm64/kvm/handle_exit.c
> >> +++ b/arch/arm64/kvm/handle_exit.c
> >> @@ -178,6 +178,39 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
> >>  	return arm_exit_handlers[hsr_ec];
> >>  }
> >>
> >> +/*
> >> + * When handling traps we need to ensure exit the guest if we
> >> + * completely emulated the instruction while single-stepping. Stuff to
> >> + * be emulated in userspace needs to complete that first.
> >> + */
> >
> > I really don't understand the first sentence here.  We are already out
> > of the guest, so do you mean a return to userspace?
> > I think the second sentence could be more clear as well.  Is 'stuff' not
> > actually 'MMIO emulation' or 'emulation' more broadly?
> 
> Your right - it's sloppily worded how about:
> 
>  /*
>   * 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 it's

s/it's/its/

>   * emulation first.
>   */

Otherwise looks much better, thanks.

> 
> For x86 there is also IO emulation but in principle anything that might
> be passed off to userspace to be completed should be done 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);
> >> +	}
> >> +
> >> +	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
> >
> > Don't you want if (handled == 1) or if (handled > 0) ?
> >
> > If there was an error I think we want to just return that to userspace
> > and not override it and present single-stepping.
> 
> Yes, I'll fix it.
> 

Thanks,
-Christoffer

[PATCH v1 2/2] 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.

I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
the differences between arm/arm64 which is currently null for arm.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 arch/arm/include/asm/kvm_host.h   |  2 ++
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
 arch/arm64/kvm/handle_exit.c      |  9 +++------
 virt/kvm/arm/arm.c                |  2 +-
 virt/kvm/arm/mmio.c               |  3 ++-
 6 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
+int  kvm_arm_maybe_return_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..a10a18c55c87 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 		}
 	}
 }
+
+
+/*
+ * When KVM has successfully emulated the instruction we might want to
+ * return we 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.
+ *
+ * Return > 0 to return to guest, 0 (and set exit_reason) on proper
+ * exit to userspace.
+ */
+
+int kvm_arm_maybe_return_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 0;
+	}
+	return 1;
+}
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index c918d291cb58..7b04f59217bf 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		handled = exit_handler(vcpu, run);
 	}
 
-	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
-		handled = 0;
-		run->exit_reason = KVM_EXIT_DEBUG;
-		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
-	}
+	if (handled)
+		return kvm_arm_maybe_return_debug(vcpu, run);
 
-	return handled;
+	return 0;
 }
 
 /*
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index b9f68e4add71..3d28fe2daa26 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 
 	if (run->exit_reason == KVM_EXIT_MMIO) {
 		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
-		if (ret)
+		if (ret < 1)
 			return ret;
 	}
 
diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
index b6e715fd3c90..e43e3bd6222f 100644
--- a/virt/kvm/arm/mmio.c
+++ b/virt/kvm/arm/mmio.c
@@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
 	}
 
-	return 0;
+	/* If debugging in effect we may need to return now */
+	return kvm_arm_maybe_return_debug(vcpu, run);
 }
 
 static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
-- 
2.14.1

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

[Marc Zyngier]

On 06/10/17 12: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.
> 
> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
> the differences between arm/arm64 which is currently null for arm.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  arch/arm/include/asm/kvm_host.h   |  2 ++
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>  arch/arm64/kvm/handle_exit.c      |  9 +++------
>  virt/kvm/arm/arm.c                |  2 +-
>  virt/kvm/arm/mmio.c               |  3 ++-
>  6 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  }
> +
> +
> +/*
> + * When KVM has successfully emulated the instruction we might want to
> + * return we 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.
> + *
> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
> + * exit to userspace.
> + */
> +
> +int kvm_arm_maybe_return_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 0;
> +	}
> +	return 1;
> +}
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index c918d291cb58..7b04f59217bf 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		handled = exit_handler(vcpu, run);
>  	}
>  
> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
> -		handled = 0;
> -		run->exit_reason = KVM_EXIT_DEBUG;
> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
> -	}
> +	if (handled)
> +		return kvm_arm_maybe_return_debug(vcpu, run);
>  
> -	return handled;
> +	return 0;
>  }
>  
>  /*
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index b9f68e4add71..3d28fe2daa26 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  	if (run->exit_reason == KVM_EXIT_MMIO) {
>  		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> -		if (ret)
> +		if (ret < 1)
>  			return ret;
>  	}
>  
> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> index b6e715fd3c90..e43e3bd6222f 100644
> --- a/virt/kvm/arm/mmio.c
> +++ b/virt/kvm/arm/mmio.c
> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>  	}
>  
> -	return 0;
> +	/* If debugging in effect we may need to return now */
> +	return kvm_arm_maybe_return_debug(vcpu, run);

Ah, that's how you do it. OK. Then the patch splitting is wrong, because
everything is broken after patch #1.

I'd prefer something like "kvm_handle_single_step" instead of this
"kvm_arm_maybe_return_debug", but I'm very bad at naming things anyway.

>  }
>  
>  static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
> 

Thanks,

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

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

[Marc Zyngier]

On 06/10/17 13:37, Marc Zyngier wrote:
> On 06/10/17 12: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.
>>
>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>> the differences between arm/arm64 which is currently null for arm.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  2 ++
>>  arch/arm64/include/asm/kvm_host.h |  1 +
>>  arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>>  arch/arm64/kvm/handle_exit.c      |  9 +++------
>>  virt/kvm/arm/arm.c                |  2 +-
>>  virt/kvm/arm/mmio.c               |  3 ++-
>>  6 files changed, 30 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
>> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>  		}
>>  	}
>>  }
>> +
>> +
>> +/*
>> + * When KVM has successfully emulated the instruction we might want to
>> + * return we 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.
>> + *
>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
>> + * exit to userspace.
>> + */
>> +
>> +int kvm_arm_maybe_return_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 0;
>> +	}
>> +	return 1;
>> +}
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index c918d291cb58..7b04f59217bf 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		handled = exit_handler(vcpu, run);
>>  	}
>>  
>> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>> -		handled = 0;
>> -		run->exit_reason = KVM_EXIT_DEBUG;
>> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>> -	}
>> +	if (handled)
>> +		return kvm_arm_maybe_return_debug(vcpu, run);
>>  
>> -	return handled;
>> +	return 0;
>>  }
>>  
>>  /*
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index b9f68e4add71..3d28fe2daa26 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  
>>  	if (run->exit_reason == KVM_EXIT_MMIO) {
>>  		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>> -		if (ret)
>> +		if (ret < 1)
>>  			return ret;
>>  	}
>>  
>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>> index b6e715fd3c90..e43e3bd6222f 100644
>> --- a/virt/kvm/arm/mmio.c
>> +++ b/virt/kvm/arm/mmio.c
>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>>  	}
>>  
>> -	return 0;
>> +	/* If debugging in effect we may need to return now */
>> +	return kvm_arm_maybe_return_debug(vcpu, run);
> 
> Ah, that's how you do it. OK. Then the patch splitting is wrong, because
> everything is broken after patch #1.

Actually, it is not broken at all. I'm just confused by the very
esoteric flow.

Thanks.

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

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

[Alex Bennée]

Marc Zyngier <marc.zyngier@arm.com> writes:

> On 06/10/17 13:37, Marc Zyngier wrote:
>> On 06/10/17 12: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.
>>>
>>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>>> the differences between arm/arm64 which is currently null for arm.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>  arch/arm/include/asm/kvm_host.h   |  2 ++
>>>  arch/arm64/include/asm/kvm_host.h |  1 +
>>>  arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>>>  arch/arm64/kvm/handle_exit.c      |  9 +++------
>>>  virt/kvm/arm/arm.c                |  2 +-
>>>  virt/kvm/arm/mmio.c               |  3 ++-
>>>  6 files changed, 30 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
>>> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
>>> --- a/arch/arm64/kvm/debug.c
>>> +++ b/arch/arm64/kvm/debug.c
>>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>>  		}
>>>  	}
>>>  }
>>> +
>>> +
>>> +/*
>>> + * When KVM has successfully emulated the instruction we might want to
>>> + * return we 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.
>>> + *
>>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
>>> + * exit to userspace.
>>> + */
>>> +
>>> +int kvm_arm_maybe_return_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 0;
>>> +	}
>>> +	return 1;
>>> +}
>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>>> index c918d291cb58..7b04f59217bf 100644
>>> --- a/arch/arm64/kvm/handle_exit.c
>>> +++ b/arch/arm64/kvm/handle_exit.c
>>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>  		handled = exit_handler(vcpu, run);
>>>  	}
>>>
>>> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>>> -		handled = 0;
>>> -		run->exit_reason = KVM_EXIT_DEBUG;
>>> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>>> -	}
>>> +	if (handled)
>>> +		return kvm_arm_maybe_return_debug(vcpu, run);
>>>
>>> -	return handled;
>>> +	return 0;
>>>  }
>>>
>>>  /*
>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>> index b9f68e4add71..3d28fe2daa26 100644
>>> --- a/virt/kvm/arm/arm.c
>>> +++ b/virt/kvm/arm/arm.c
>>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>
>>>  	if (run->exit_reason == KVM_EXIT_MMIO) {
>>>  		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>>> -		if (ret)
>>> +		if (ret < 1)
>>>  			return ret;
>>>  	}
>>>
>>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>>> index b6e715fd3c90..e43e3bd6222f 100644
>>> --- a/virt/kvm/arm/mmio.c
>>> +++ b/virt/kvm/arm/mmio.c
>>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>  		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>>>  	}
>>>
>>> -	return 0;
>>> +	/* If debugging in effect we may need to return now */
>>> +	return kvm_arm_maybe_return_debug(vcpu, run);
>>
>> Ah, that's how you do it. OK. Then the patch splitting is wrong, because
>> everything is broken after patch #1.
>
> Actually, it is not broken at all. I'm just confused by the very
> esoteric flow.

We could just merge the whole patch in one but I wanted to show the
difference between in-kernel and out-of-kernel emulation.

I could also move the step handling to the mmio leg in
kvm_arch_vcpu_ioctl_run but you mentioned you use the mmio completion
elsewhere anyway?

--
Alex Bennée

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

[Marc Zyngier]

On 06/10/17 14:47, Alex Bennée wrote:
> 
> Marc Zyngier <marc.zyngier@arm.com> writes:
> 
>> On 06/10/17 13:37, Marc Zyngier wrote:
>>> On 06/10/17 12: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.
>>>>
>>>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>>>> the differences between arm/arm64 which is currently null for arm.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> ---
>>>>  arch/arm/include/asm/kvm_host.h   |  2 ++
>>>>  arch/arm64/include/asm/kvm_host.h |  1 +
>>>>  arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>>>>  arch/arm64/kvm/handle_exit.c      |  9 +++------
>>>>  virt/kvm/arm/arm.c                |  2 +-
>>>>  virt/kvm/arm/mmio.c               |  3 ++-
>>>>  6 files changed, 30 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
>>>> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
>>>> --- a/arch/arm64/kvm/debug.c
>>>> +++ b/arch/arm64/kvm/debug.c
>>>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>>>  		}
>>>>  	}
>>>>  }
>>>> +
>>>> +
>>>> +/*
>>>> + * When KVM has successfully emulated the instruction we might want to
>>>> + * return we 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.
>>>> + *
>>>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
>>>> + * exit to userspace.
>>>> + */
>>>> +
>>>> +int kvm_arm_maybe_return_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 0;
>>>> +	}
>>>> +	return 1;
>>>> +}
>>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>>>> index c918d291cb58..7b04f59217bf 100644
>>>> --- a/arch/arm64/kvm/handle_exit.c
>>>> +++ b/arch/arm64/kvm/handle_exit.c
>>>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>  		handled = exit_handler(vcpu, run);
>>>>  	}
>>>>
>>>> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>>>> -		handled = 0;
>>>> -		run->exit_reason = KVM_EXIT_DEBUG;
>>>> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>>>> -	}
>>>> +	if (handled)
>>>> +		return kvm_arm_maybe_return_debug(vcpu, run);
>>>>
>>>> -	return handled;
>>>> +	return 0;
>>>>  }
>>>>
>>>>  /*
>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>> index b9f68e4add71..3d28fe2daa26 100644
>>>> --- a/virt/kvm/arm/arm.c
>>>> +++ b/virt/kvm/arm/arm.c
>>>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>
>>>>  	if (run->exit_reason == KVM_EXIT_MMIO) {
>>>>  		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>>>> -		if (ret)
>>>> +		if (ret < 1)
>>>>  			return ret;
>>>>  	}
>>>>
>>>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>>>> index b6e715fd3c90..e43e3bd6222f 100644
>>>> --- a/virt/kvm/arm/mmio.c
>>>> +++ b/virt/kvm/arm/mmio.c
>>>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>  		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>>>>  	}
>>>>
>>>> -	return 0;
>>>> +	/* If debugging in effect we may need to return now */
>>>> +	return kvm_arm_maybe_return_debug(vcpu, run);
>>>
>>> Ah, that's how you do it. OK. Then the patch splitting is wrong, because
>>> everything is broken after patch #1.
>>
>> Actually, it is not broken at all. I'm just confused by the very
>> esoteric flow.
> 
> We could just merge the whole patch in one but I wanted to show the
> difference between in-kernel and out-of-kernel emulation.
> 
> I could also move the step handling to the mmio leg in
> kvm_arch_vcpu_ioctl_run but you mentioned you use the mmio completion
> elsewhere anyway?
Yes, look at the end of io_mem_abort(). This is used by the vgic to
complete a read emulation in the kernel.

And actually, this means that we shouldn't have to mess with
handle_exit. Just check for the return value of kvm_handle_mmio_return
in the call sites (including the one in io_mem_abort), and exit if we
need to single-step...

Thoughts?

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

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

[Julien Thierry]

On 06/10/17 15:00, Marc Zyngier wrote:
> On 06/10/17 14:47, Alex Bennée wrote:
>>
>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>
>>> On 06/10/17 13:37, Marc Zyngier wrote:
>>>> On 06/10/17 12: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.
>>>>>
>>>>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>>>>> the differences between arm/arm64 which is currently null for arm.
>>>>>
>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>> ---
>>>>>   arch/arm/include/asm/kvm_host.h   |  2 ++
>>>>>   arch/arm64/include/asm/kvm_host.h |  1 +
>>>>>   arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>>>>>   arch/arm64/kvm/handle_exit.c      |  9 +++------
>>>>>   virt/kvm/arm/arm.c                |  2 +-
>>>>>   virt/kvm/arm/mmio.c               |  3 ++-
>>>>>   6 files changed, 30 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>>> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
>>>>> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
>>>>> --- a/arch/arm64/kvm/debug.c
>>>>> +++ b/arch/arm64/kvm/debug.c
>>>>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>>>>   		}
>>>>>   	}
>>>>>   }
>>>>> +
>>>>> +
>>>>> +/*
>>>>> + * When KVM has successfully emulated the instruction we might want to
>>>>> + * return we 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.
>>>>> + *
>>>>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
>>>>> + * exit to userspace.
>>>>> + */
>>>>> +
>>>>> +int kvm_arm_maybe_return_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 0;
>>>>> +	}
>>>>> +	return 1;
>>>>> +}
>>>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>>>>> index c918d291cb58..7b04f59217bf 100644
>>>>> --- a/arch/arm64/kvm/handle_exit.c
>>>>> +++ b/arch/arm64/kvm/handle_exit.c
>>>>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>>   		handled = exit_handler(vcpu, run);
>>>>>   	}
>>>>>
>>>>> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>>>>> -		handled = 0;
>>>>> -		run->exit_reason = KVM_EXIT_DEBUG;
>>>>> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>>>>> -	}
>>>>> +	if (handled)
>>>>> +		return kvm_arm_maybe_return_debug(vcpu, run);
>>>>>
>>>>> -	return handled;
>>>>> +	return 0;
>>>>>   }
>>>>>
>>>>>   /*
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index b9f68e4add71..3d28fe2daa26 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>>
>>>>>   	if (run->exit_reason == KVM_EXIT_MMIO) {
>>>>>   		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>>>>> -		if (ret)
>>>>> +		if (ret < 1)
>>>>>   			return ret;
>>>>>   	}
>>>>>
>>>>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>>>>> index b6e715fd3c90..e43e3bd6222f 100644
>>>>> --- a/virt/kvm/arm/mmio.c
>>>>> +++ b/virt/kvm/arm/mmio.c
>>>>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>>   		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>>>>>   	}
>>>>>
>>>>> -	return 0;
>>>>> +	/* If debugging in effect we may need to return now */
>>>>> +	return kvm_arm_maybe_return_debug(vcpu, run);
>>>>
>>>> Ah, that's how you do it. OK. Then the patch splitting is wrong, because
>>>> everything is broken after patch #1.
>>>
>>> Actually, it is not broken at all. I'm just confused by the very
>>> esoteric flow.
>>
>> We could just merge the whole patch in one but I wanted to show the
>> difference between in-kernel and out-of-kernel emulation.
>>
>> I could also move the step handling to the mmio leg in
>> kvm_arch_vcpu_ioctl_run but you mentioned you use the mmio completion
>> elsewhere anyway?
> Yes, look at the end of io_mem_abort(). This is used by the vgic to
> complete a read emulation in the kernel.
> 
> And actually, this means that we shouldn't have to mess with
> handle_exit. Just check for the return value of kvm_handle_mmio_return
> in the call sites (including the one in io_mem_abort), and exit if we
> need to single-step...
> 

I think we need to mess with handle_exit (or at least something else 
than kvm_handle_mmio call sites) because the patches don't only fix MMIO 
single stepping, but also other emulated stuff (system register 
accesses, ...).

But with your suggestion maybe we can at least handle both MMIO cases in 
a similar manner. I think we still need the code in handle_exit, or add 
more code to deal case by case with other emulated instructions.

Thanks,

-- 
Julien Thierry

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

[Marc Zyngier]

On 06/10/17 15:26, Julien Thierry wrote:
> 
> 
> On 06/10/17 15:00, Marc Zyngier wrote:
>> On 06/10/17 14:47, Alex Bennée wrote:
>>>
>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>
>>>> On 06/10/17 13:37, Marc Zyngier wrote:
>>>>> On 06/10/17 12: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.
>>>>>>
>>>>>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>>>>>> the differences between arm/arm64 which is currently null for arm.
>>>>>>
>>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>> ---
>>>>>>   arch/arm/include/asm/kvm_host.h   |  2 ++
>>>>>>   arch/arm64/include/asm/kvm_host.h |  1 +
>>>>>>   arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>>>>>>   arch/arm64/kvm/handle_exit.c      |  9 +++------
>>>>>>   virt/kvm/arm/arm.c                |  2 +-
>>>>>>   virt/kvm/arm/mmio.c               |  3 ++-
>>>>>>   6 files changed, 30 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>>>> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
>>>>>> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
>>>>>> --- a/arch/arm64/kvm/debug.c
>>>>>> +++ b/arch/arm64/kvm/debug.c
>>>>>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>>>>>   		}
>>>>>>   	}
>>>>>>   }
>>>>>> +
>>>>>> +
>>>>>> +/*
>>>>>> + * When KVM has successfully emulated the instruction we might want to
>>>>>> + * return we 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.
>>>>>> + *
>>>>>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
>>>>>> + * exit to userspace.
>>>>>> + */
>>>>>> +
>>>>>> +int kvm_arm_maybe_return_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 0;
>>>>>> +	}
>>>>>> +	return 1;
>>>>>> +}
>>>>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>>>>>> index c918d291cb58..7b04f59217bf 100644
>>>>>> --- a/arch/arm64/kvm/handle_exit.c
>>>>>> +++ b/arch/arm64/kvm/handle_exit.c
>>>>>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>>>   		handled = exit_handler(vcpu, run);
>>>>>>   	}
>>>>>>
>>>>>> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>>>>>> -		handled = 0;
>>>>>> -		run->exit_reason = KVM_EXIT_DEBUG;
>>>>>> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>>>>>> -	}
>>>>>> +	if (handled)
>>>>>> +		return kvm_arm_maybe_return_debug(vcpu, run);
>>>>>>
>>>>>> -	return handled;
>>>>>> +	return 0;
>>>>>>   }
>>>>>>
>>>>>>   /*
>>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>>> index b9f68e4add71..3d28fe2daa26 100644
>>>>>> --- a/virt/kvm/arm/arm.c
>>>>>> +++ b/virt/kvm/arm/arm.c
>>>>>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>>>
>>>>>>   	if (run->exit_reason == KVM_EXIT_MMIO) {
>>>>>>   		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>>>>>> -		if (ret)
>>>>>> +		if (ret < 1)
>>>>>>   			return ret;
>>>>>>   	}
>>>>>>
>>>>>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>>>>>> index b6e715fd3c90..e43e3bd6222f 100644
>>>>>> --- a/virt/kvm/arm/mmio.c
>>>>>> +++ b/virt/kvm/arm/mmio.c
>>>>>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>>>   		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>>>>>>   	}
>>>>>>
>>>>>> -	return 0;
>>>>>> +	/* If debugging in effect we may need to return now */
>>>>>> +	return kvm_arm_maybe_return_debug(vcpu, run);
>>>>>
>>>>> Ah, that's how you do it. OK. Then the patch splitting is wrong, because
>>>>> everything is broken after patch #1.
>>>>
>>>> Actually, it is not broken at all. I'm just confused by the very
>>>> esoteric flow.
>>>
>>> We could just merge the whole patch in one but I wanted to show the
>>> difference between in-kernel and out-of-kernel emulation.
>>>
>>> I could also move the step handling to the mmio leg in
>>> kvm_arch_vcpu_ioctl_run but you mentioned you use the mmio completion
>>> elsewhere anyway?
>> Yes, look at the end of io_mem_abort(). This is used by the vgic to
>> complete a read emulation in the kernel.
>>
>> And actually, this means that we shouldn't have to mess with
>> handle_exit. Just check for the return value of kvm_handle_mmio_return
>> in the call sites (including the one in io_mem_abort), and exit if we
>> need to single-step...
>>
> 
> I think we need to mess with handle_exit (or at least something else 
> than kvm_handle_mmio call sites) because the patches don't only fix MMIO 
> single stepping, but also other emulated stuff (system register 
> accesses, ...).

Ah, true. I was too focussed on the the MMIO problem.

> But with your suggestion maybe we can at least handle both MMIO cases in 
> a similar manner. I think we still need the code in handle_exit, or add 
> more code to deal case by case with other emulated instructions.
Fair enough.

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

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

[Julien Thierry]

On 06/10/17 12: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.
> 
> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
> the differences between arm/arm64 which is currently null for arm.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   arch/arm/include/asm/kvm_host.h   |  2 ++
>   arch/arm64/include/asm/kvm_host.h |  1 +
>   arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>   arch/arm64/kvm/handle_exit.c      |  9 +++------
>   virt/kvm/arm/arm.c                |  2 +-
>   virt/kvm/arm/mmio.c               |  3 ++-
>   6 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu,
> +						struct kvm_run *run) {}
>   

This function should return 1.

>   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..fa67d21662f6 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);
> +int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);

I feel the name could be a little bit more explicit:

kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug, 
kvm_arm_trap_need_return_debug.

At least, I think it would be nice that the name reflect that this check 
is meant for emulated instructions.

Otherwise:

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

Thanks,

-- 
Julien Thierry

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

[Alex Bennée]

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

> On 06/10/17 12: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.
>>
>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>> the differences between arm/arm64 which is currently null for arm.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   arch/arm/include/asm/kvm_host.h   |  2 ++
>>   arch/arm64/include/asm/kvm_host.h |  1 +
>>   arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>>   arch/arm64/kvm/handle_exit.c      |  9 +++------
>>   virt/kvm/arm/arm.c                |  2 +-
>>   virt/kvm/arm/mmio.c               |  3 ++-
>>   6 files changed, 30 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu,
>> +						struct kvm_run *run) {}
>>
>
> This function should return 1.

So I did ponder making this a bool, returning true if we need to exit
and testing in v/k/a/arm.c exit leg rather than in the mmio handler.

At the moment it mirrors the existing exit logic which follows -1 err, 0
return, >0 handled. But as I mentioned in the cover letter this fell
down a bit when dealing with the mmio case.

>
>>   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..fa67d21662f6 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);
>> +int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);
>
> I feel the name could be a little bit more explicit:
>
> kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug,
> kvm_arm_trap_need_return_debug.

I wanted to keep the debug suffix so that's fine although I'm not so
sure trap is correct because on the tail end of mmio emulation are we
still trapping?

Maybe kvm_arm_step_emulated_debug?

> At least, I think it would be nice that the name reflect that this
> check is meant for emulated instructions.
>
> Otherwise:
>
> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
>
> Thanks,


--
Alex Bennée

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

[Julien Thierry]

On 06/10/17 14:45, Alex Bennée wrote:
> 
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> On 06/10/17 12: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.
>>>
>>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>>> the differences between arm/arm64 which is currently null for arm.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>    arch/arm/include/asm/kvm_host.h   |  2 ++
>>>    arch/arm64/include/asm/kvm_host.h |  1 +
>>>    arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>>>    arch/arm64/kvm/handle_exit.c      |  9 +++------
>>>    virt/kvm/arm/arm.c                |  2 +-
>>>    virt/kvm/arm/mmio.c               |  3 ++-
>>>    6 files changed, 30 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu,
>>> +						struct kvm_run *run) {}
>>>
>>
>> This function should return 1.
> 
> So I did ponder making this a bool, returning true if we need to exit
> and testing in v/k/a/arm.c exit leg rather than in the mmio handler.
> 
> At the moment it mirrors the existing exit logic which follows -1 err, 0
> return, >0 handled. But as I mentioned in the cover letter this fell
> down a bit when dealing with the mmio case.
> 

Hmmm, my main issue is that this version doesn't have a return 
statement, which probably triggers a gcc warning with ARCH=arm and also 
might cause arm (32bit) kvm to exit upon handling mmio return when we 
don't want to.

Otherwise, I also wondered about using a bool here. But following the 
pre-existing logic makes sense to me (but I have no strong feeling about 
it).

>>
>>>    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..fa67d21662f6 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);
>>> +int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>
>> I feel the name could be a little bit more explicit:
>>
>> kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug,
>> kvm_arm_trap_need_return_debug.
> 
> I wanted to keep the debug suffix so that's fine although I'm not so
> sure trap is correct because on the tail end of mmio emulation are we
> still trapping?
> 
> Maybe kvm_arm_step_emulated_debug?
> 

Yes, sounds good.

Thanks,

>> At least, I think it would be nice that the name reflect that this
>> check is meant for emulated instructions.
>>
>> Otherwise:
>>
>> Reviewed-by: Julien Thierry <julien.thierry@arm.com>
>>
>> Thanks,
> 
> 
> --
> Alex Bennée
> 

-- 
Julien Thierry

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

[Christoffer Dall]

On Fri, Oct 06, 2017 at 02:45:35PM +0100, Alex Bennée wrote:
> 
> Julien Thierry <julien.thierry@arm.com> writes:
> 
> > On 06/10/17 12: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.
> >>
> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
> >> the differences between arm/arm64 which is currently null for arm.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>   arch/arm/include/asm/kvm_host.h   |  2 ++
> >>   arch/arm64/include/asm/kvm_host.h |  1 +
> >>   arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
> >>   arch/arm64/kvm/handle_exit.c      |  9 +++------
> >>   virt/kvm/arm/arm.c                |  2 +-
> >>   virt/kvm/arm/mmio.c               |  3 ++-
> >>   6 files changed, 30 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu,
> >> +						struct kvm_run *run) {}
> >>
> >
> > This function should return 1.
> 
> So I did ponder making this a bool, returning true if we need to exit
> and testing in v/k/a/arm.c exit leg rather than in the mmio handler.
> 
> At the moment it mirrors the existing exit logic which follows -1 err, 0
> return, >0 handled. But as I mentioned in the cover letter this fell
> down a bit when dealing with the mmio case.
> 
> >
> >>   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..fa67d21662f6 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);
> >> +int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >
> > I feel the name could be a little bit more explicit:
> >
> > kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug,
> > kvm_arm_trap_need_return_debug.
> 
> I wanted to keep the debug suffix so that's fine although I'm not so
> sure trap is correct because on the tail end of mmio emulation are we
> still trapping?
> 
> Maybe kvm_arm_step_emulated_debug?

I think you should name it:

kvm_arm_should_complete_emulated_instr_debug() - or something better -
and call it directly from kvm_arch_vcpu_ioctl_run, so that it becomes:

	ret = kvm_handle_mmio_return(vcpu, vcpu->run);
	if (ret)
		return ret;
	ret = kvm_arm_should_complete_emulated_instr_debug(vcpu);
	if (ret)
		return ret;

> 
> > At least, I think it would be nice that the name reflect that this
> > check is meant for emulated instructions.
> >
> > Otherwise:
> >
> > Reviewed-by: Julien Thierry <julien.thierry@arm.com>
> >
> > Thanks,
> 
> 
Thanks,
-Christoffer

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

[Alex Bennée]

Christoffer Dall <cdall@linaro.org> writes:

> On Fri, Oct 06, 2017 at 02:45:35PM +0100, Alex Bennée wrote:
>>
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>> > On 06/10/17 12: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.
>> >>
>> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>> >> the differences between arm/arm64 which is currently null for arm.
>> >>
>> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> >> ---
>> >>   arch/arm/include/asm/kvm_host.h   |  2 ++
>> >>   arch/arm64/include/asm/kvm_host.h |  1 +
>> >>   arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>> >>   arch/arm64/kvm/handle_exit.c      |  9 +++------
>> >>   virt/kvm/arm/arm.c                |  2 +-
>> >>   virt/kvm/arm/mmio.c               |  3 ++-
>> >>   6 files changed, 30 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> >> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu,
>> >> +						struct kvm_run *run) {}
>> >>
>> >
>> > This function should return 1.
>>
>> So I did ponder making this a bool, returning true if we need to exit
>> and testing in v/k/a/arm.c exit leg rather than in the mmio handler.
>>
>> At the moment it mirrors the existing exit logic which follows -1 err, 0
>> return, >0 handled. But as I mentioned in the cover letter this fell
>> down a bit when dealing with the mmio case.
>>
>> >
>> >>   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..fa67d21662f6 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);
>> >> +int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);
>> >
>> > I feel the name could be a little bit more explicit:
>> >
>> > kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug,
>> > kvm_arm_trap_need_return_debug.
>>
>> I wanted to keep the debug suffix so that's fine although I'm not so
>> sure trap is correct because on the tail end of mmio emulation are we
>> still trapping?
>>
>> Maybe kvm_arm_step_emulated_debug?
>
> I think you should name it:
>
> kvm_arm_should_complete_emulated_instr_debug() - or something better -

Naming is hard :-/

> and call it directly from kvm_arch_vcpu_ioctl_run, so that it becomes:
>
> 	ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> 	if (ret)
> 		return ret;
> 	ret = kvm_arm_should_complete_emulated_instr_debug(vcpu);
> 	if (ret)
> 		return ret;

This runs into the problem of slightly different ret semantics for here
and in handle_exit. Maybe just having a bool response and:

    if (kvm_arm_should_complete_emulated_instr_debug(vcpu))
        return 0;

And then in handle_exit:

    if (handled == 1 && kvm_arm_should_complete_emulated_instr_debug(vcpu))
        return 0;
    else
        return handled;

?
>
>>
>> > At least, I think it would be nice that the name reflect that this
>> > check is meant for emulated instructions.
>> >
>> > Otherwise:
>> >
>> > Reviewed-by: Julien Thierry <julien.thierry@arm.com>
>> >
>> > Thanks,
>>
>>
> Thanks,
> -Christoffer


--
Alex Bennée

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

[Christoffer Dall]

On Fri, Oct 13, 2017 at 10:23:21AM +0100, Alex Bennée wrote:
> 
> Christoffer Dall <cdall@linaro.org> writes:
> 
> > On Fri, Oct 06, 2017 at 02:45:35PM +0100, Alex Bennée wrote:
> >>
> >> Julien Thierry <julien.thierry@arm.com> writes:
> >>
> >> > On 06/10/17 12: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.
> >> >>
> >> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
> >> >> the differences between arm/arm64 which is currently null for arm.
> >> >>
> >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> >> ---
> >> >>   arch/arm/include/asm/kvm_host.h   |  2 ++
> >> >>   arch/arm64/include/asm/kvm_host.h |  1 +
> >> >>   arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
> >> >>   arch/arm64/kvm/handle_exit.c      |  9 +++------
> >> >>   virt/kvm/arm/arm.c                |  2 +-
> >> >>   virt/kvm/arm/mmio.c               |  3 ++-
> >> >>   6 files changed, 30 insertions(+), 8 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> >> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu,
> >> >> +						struct kvm_run *run) {}
> >> >>
> >> >
> >> > This function should return 1.
> >>
> >> So I did ponder making this a bool, returning true if we need to exit
> >> and testing in v/k/a/arm.c exit leg rather than in the mmio handler.
> >>
> >> At the moment it mirrors the existing exit logic which follows -1 err, 0
> >> return, >0 handled. But as I mentioned in the cover letter this fell
> >> down a bit when dealing with the mmio case.
> >>
> >> >
> >> >>   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..fa67d21662f6 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);
> >> >> +int  kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run);
> >> >
> >> > I feel the name could be a little bit more explicit:
> >> >
> >> > kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug,
> >> > kvm_arm_trap_need_return_debug.
> >>
> >> I wanted to keep the debug suffix so that's fine although I'm not so
> >> sure trap is correct because on the tail end of mmio emulation are we
> >> still trapping?
> >>
> >> Maybe kvm_arm_step_emulated_debug?
> >
> > I think you should name it:
> >
> > kvm_arm_should_complete_emulated_instr_debug() - or something better -
> 
> Naming is hard :-/
> 

Yeah, my suggestion was half-way a joke, but as Julien said, it should
reflect what it's trying to tell you as concretely as possible.

> > and call it directly from kvm_arch_vcpu_ioctl_run, so that it becomes:
> >
> > 	ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> > 	if (ret)
> > 		return ret;
> > 	ret = kvm_arm_should_complete_emulated_instr_debug(vcpu);
> > 	if (ret)
> > 		return ret;
> 
> This runs into the problem of slightly different ret semantics for here
> and in handle_exit. Maybe just having a bool response and:
> 
>     if (kvm_arm_should_complete_emulated_instr_debug(vcpu))
>         return 0;

Ah yeah, my example was broken.

> 
> And then in handle_exit:
> 
>     if (handled == 1 && kvm_arm_should_complete_emulated_instr_debug(vcpu))
>         return 0;
>     else
>         return handled;
> 

Yes, looks good, assuming we can find a better name.

Thanks,
-Christoffer

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

[Christoffer Dall]

On Fri, Oct 06, 2017 at 12:39:21PM +0100, 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.
> 
> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
> the differences between arm/arm64 which is currently null for arm.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  arch/arm/include/asm/kvm_host.h   |  2 ++
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>  arch/arm64/kvm/handle_exit.c      |  9 +++------
>  virt/kvm/arm/arm.c                |  2 +-
>  virt/kvm/arm/mmio.c               |  3 ++-
>  6 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  }
> +
> +
> +/*
> + * When KVM has successfully emulated the instruction we might want to
> + * return we 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.
> + *
> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
> + * exit to userspace.
> + */
> +
> +int kvm_arm_maybe_return_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 0;
> +	}
> +	return 1;
> +}
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index c918d291cb58..7b04f59217bf 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		handled = exit_handler(vcpu, run);
>  	}
>  
> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
> -		handled = 0;
> -		run->exit_reason = KVM_EXIT_DEBUG;
> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
> -	}
> +	if (handled)
> +		return kvm_arm_maybe_return_debug(vcpu, run);

Again, this seems to override the return value of exit_handler, which
may be something negative.

Just so I'm clear: There's no intended functionality change of this
particular hunk, it's just to share the logic in
kvm_arm_maybe_return_debug, right?

>  
> -	return handled;
> +	return 0;
>  }
>  
>  /*
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index b9f68e4add71..3d28fe2daa26 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  
>  	if (run->exit_reason == KVM_EXIT_MMIO) {
>  		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> -		if (ret)
> +		if (ret < 1)
>  			return ret;
>  	}
>  
> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> index b6e715fd3c90..e43e3bd6222f 100644
> --- a/virt/kvm/arm/mmio.c
> +++ b/virt/kvm/arm/mmio.c
> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>  	}
>  
> -	return 0;
> +	/* If debugging in effect we may need to return now */

Will this ever be about other types of debugging (watchpoint on a MMIO
access?) or should we limit the text and description to single-stepping?

> +	return kvm_arm_maybe_return_debug(vcpu, run);
>  }
>  
>  static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
> -- 
> 2.14.1
> 

Thanks,
-Christoffer

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

[Alex Bennée]

Christoffer Dall <cdall@linaro.org> writes:

> On Fri, Oct 06, 2017 at 12:39:21PM +0100, 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.
>>
>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
>> the differences between arm/arm64 which is currently null for arm.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  arch/arm/include/asm/kvm_host.h   |  2 ++
>>  arch/arm64/include/asm/kvm_host.h |  1 +
>>  arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
>>  arch/arm64/kvm/handle_exit.c      |  9 +++------
>>  virt/kvm/arm/arm.c                |  2 +-
>>  virt/kvm/arm/mmio.c               |  3 ++-
>>  6 files changed, 30 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
>> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>>  		}
>>  	}
>>  }
>> +
>> +
>> +/*
>> + * When KVM has successfully emulated the instruction we might want to
>> + * return we 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.
>> + *
>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
>> + * exit to userspace.
>> + */
>> +
>> +int kvm_arm_maybe_return_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 0;
>> +	}
>> +	return 1;
>> +}
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index c918d291cb58..7b04f59217bf 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		handled = exit_handler(vcpu, run);
>>  	}
>>
>> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>> -		handled = 0;
>> -		run->exit_reason = KVM_EXIT_DEBUG;
>> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
>> -	}
>> +	if (handled)
>> +		return kvm_arm_maybe_return_debug(vcpu, run);
>
> Again, this seems to override the return value of exit_handler, which
> may be something negative.
>
> Just so I'm clear: There's no intended functionality change of this
> particular hunk, it's just to share the logic in
> kvm_arm_maybe_return_debug, right?

Yes, modulo the annoying semantics in the two places of the vcpu run
ioctl loop.

>
>>
>> -	return handled;
>> +	return 0;
>>  }
>>
>>  /*
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index b9f68e4add71..3d28fe2daa26 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>
>>  	if (run->exit_reason == KVM_EXIT_MMIO) {
>>  		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>> -		if (ret)
>> +		if (ret < 1)
>>  			return ret;
>>  	}
>>
>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>> index b6e715fd3c90..e43e3bd6222f 100644
>> --- a/virt/kvm/arm/mmio.c
>> +++ b/virt/kvm/arm/mmio.c
>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
>>  	}
>>
>> -	return 0;
>> +	/* If debugging in effect we may need to return now */
>
> Will this ever be about other types of debugging (watchpoint on a MMIO
> access?) or should we limit the text and description to
> single-stepping?

Hmm I don't think so. A hbreak should hit (via normal exception path)
before we attempt any emulation. I suspect watchpoints wouldn't hit for
emulation though - that would be trickier to do nicely though as it
would need to be checked for in both kernel and userspace emulation.

>
>> +	return kvm_arm_maybe_return_debug(vcpu, run);
>>  }
>>
>>  static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
>> --
>> 2.14.1
>>
>
> Thanks,
> -Christoffer


--
Alex Bennée

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

[Christoffer Dall]

On Fri, Oct 13, 2017 at 10:27:36AM +0100, Alex Bennée wrote:
> 
> Christoffer Dall <cdall@linaro.org> writes:
> 
> > On Fri, Oct 06, 2017 at 12:39:21PM +0100, 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.
> >>
> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up
> >> the differences between arm/arm64 which is currently null for arm.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  arch/arm/include/asm/kvm_host.h   |  2 ++
> >>  arch/arm64/include/asm/kvm_host.h |  1 +
> >>  arch/arm64/kvm/debug.c            | 21 +++++++++++++++++++++
> >>  arch/arm64/kvm/handle_exit.c      |  9 +++------
> >>  virt/kvm/arm/arm.c                |  2 +-
> >>  virt/kvm/arm/mmio.c               |  3 ++-
> >>  6 files changed, 30 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> index 4a879f6ff13b..aec943f6d123 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 int  kvm_arm_maybe_return_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..fa67d21662f6 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);
> >> +int  kvm_arm_maybe_return_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..a10a18c55c87 100644
> >> --- a/arch/arm64/kvm/debug.c
> >> +++ b/arch/arm64/kvm/debug.c
> >> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
> >>  		}
> >>  	}
> >>  }
> >> +
> >> +
> >> +/*
> >> + * When KVM has successfully emulated the instruction we might want to
> >> + * return we 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.
> >> + *
> >> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper
> >> + * exit to userspace.
> >> + */
> >> +
> >> +int kvm_arm_maybe_return_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 0;
> >> +	}
> >> +	return 1;
> >> +}
> >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> >> index c918d291cb58..7b04f59217bf 100644
> >> --- a/arch/arm64/kvm/handle_exit.c
> >> +++ b/arch/arm64/kvm/handle_exit.c
> >> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >>  		handled = exit_handler(vcpu, run);
> >>  	}
> >>
> >> -	if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
> >> -		handled = 0;
> >> -		run->exit_reason = KVM_EXIT_DEBUG;
> >> -		run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT;
> >> -	}
> >> +	if (handled)
> >> +		return kvm_arm_maybe_return_debug(vcpu, run);
> >
> > Again, this seems to override the return value of exit_handler, which
> > may be something negative.
> >
> > Just so I'm clear: There's no intended functionality change of this
> > particular hunk, it's just to share the logic in
> > kvm_arm_maybe_return_debug, right?
> 
> Yes, modulo the annoying semantics in the two places of the vcpu run
> ioctl loop.
> 
> >
> >>
> >> -	return handled;
> >> +	return 0;
> >>  }
> >>
> >>  /*
> >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> >> index b9f68e4add71..3d28fe2daa26 100644
> >> --- a/virt/kvm/arm/arm.c
> >> +++ b/virt/kvm/arm/arm.c
> >> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >>
> >>  	if (run->exit_reason == KVM_EXIT_MMIO) {
> >>  		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> >> -		if (ret)
> >> +		if (ret < 1)
> >>  			return ret;
> >>  	}
> >>
> >> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> >> index b6e715fd3c90..e43e3bd6222f 100644
> >> --- a/virt/kvm/arm/mmio.c
> >> +++ b/virt/kvm/arm/mmio.c
> >> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >>  		vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
> >>  	}
> >>
> >> -	return 0;
> >> +	/* If debugging in effect we may need to return now */
> >
> > Will this ever be about other types of debugging (watchpoint on a MMIO
> > access?) or should we limit the text and description to
> > single-stepping?
> 
> Hmm I don't think so. A hbreak should hit (via normal exception path)
> before we attempt any emulation. I suspect watchpoints wouldn't hit for
> emulation though - that would be trickier to do nicely though as it
> would need to be checked for in both kernel and userspace emulation.
> 
> >

Then I think we should be specific in function naming and comments and
refer to single-stepping as opposed to something more generic, because
single-stepping seems to be the case we care about.

Thanks,
-Christoffer