diff mbox series

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

Message ID 20171006113921.24880-2-alex.bennee@linaro.org
State New
Headers show
Series [v1,1/2] KVM: arm64: handle single-stepping trapped instructions | expand

Commit Message

Alex Bennée Oct. 6, 2017, 11:39 a.m. UTC 
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(-)

-- 
2.14.1

Comments

Marc Zyngier Oct. 6, 2017, 12:27 p.m. UTC | #1 
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...
Julien Thierry Oct. 6, 2017, 12:34 p.m. UTC | #2 
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
Marc Zyngier Oct. 6, 2017, 12:46 p.m. UTC | #3 
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...
Julien Thierry Oct. 6, 2017, 1:15 p.m. UTC | #4 
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
Christoffer Dall Oct. 13, 2017, 8:26 a.m. UTC | #5 
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
Alex Bennée Oct. 13, 2017, 9:15 a.m. UTC | #6 
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
Christoffer Dall Oct. 14, 2017, 2:16 p.m. UTC | #7 
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
diff mbox series

Patch

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