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

[Alex BennÃe]

Christoffer Dall <cdall@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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