diff mbox series

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

Message ID 20171006113921.24880-3-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 
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(-)

-- 
2.14.1

Comments

Marc Zyngier Oct. 6, 2017, 12:37 p.m. UTC | #1 
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...
Marc Zyngier Oct. 6, 2017, 12:44 p.m. UTC | #2 
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...
Julien Thierry Oct. 6, 2017, 1:13 p.m. UTC | #3 
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
Alex Bennée Oct. 6, 2017, 1:45 p.m. UTC | #4 
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
Alex Bennée Oct. 6, 2017, 1:47 p.m. UTC | #5 
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
Marc Zyngier Oct. 6, 2017, 2 p.m. UTC | #6 
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...
Julien Thierry Oct. 6, 2017, 2:26 p.m. UTC | #7 
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
Julien Thierry Oct. 6, 2017, 2:27 p.m. UTC | #8 
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
Marc Zyngier Oct. 6, 2017, 2:43 p.m. UTC | #9 
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...
Christoffer Dall Oct. 13, 2017, 8:56 a.m. UTC | #10 
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
Christoffer Dall Oct. 13, 2017, 8:59 a.m. UTC | #11 
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
Alex Bennée Oct. 13, 2017, 9:23 a.m. UTC | #12 
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
Alex Bennée Oct. 13, 2017, 9:27 a.m. UTC | #13 
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
Christoffer Dall Oct. 14, 2017, 2:18 p.m. UTC | #14 
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
Christoffer Dall Oct. 14, 2017, 2:20 p.m. UTC | #15 
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
diff mbox series

Patch

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)