Re: [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Suzuki K Poulose <suzuki.poulose@arm.com> writes:

> Hi Aneesh
>
> On 28/04/2025 12:57, Aneesh Kumar K.V (Arm) wrote:
>> When KVM_RUN fails with EINTR or EAGAIN, we should retry the ioctl
>> without checking kvm_run->exit_reason. These errors don't indicate a
>> valid VM exit, hence exit_reason may contain stale or undefined values.
>> 
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>> ---
>>   include/kvm/kvm-cpu.h |  2 +-
>>   kvm-cpu.c             | 17 ++++++++++++-----
>>   2 files changed, 13 insertions(+), 6 deletions(-)
>> 
>> diff --git a/include/kvm/kvm-cpu.h b/include/kvm/kvm-cpu.h
>> index 8f76f8a1123a..72cbb86e6cef 100644
>> --- a/include/kvm/kvm-cpu.h
>> +++ b/include/kvm/kvm-cpu.h
>> @@ -16,7 +16,7 @@ void kvm_cpu__delete(struct kvm_cpu *vcpu);
>>   void kvm_cpu__reset_vcpu(struct kvm_cpu *vcpu);
>>   void kvm_cpu__setup_cpuid(struct kvm_cpu *vcpu);
>>   void kvm_cpu__enable_singlestep(struct kvm_cpu *vcpu);
>> -void kvm_cpu__run(struct kvm_cpu *vcpu);
>> +int kvm_cpu__run(struct kvm_cpu *vcpu);
>>   int kvm_cpu__start(struct kvm_cpu *cpu);
>>   bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu);
>>   int kvm_cpu__get_endianness(struct kvm_cpu *vcpu);
>> diff --git a/kvm-cpu.c b/kvm-cpu.c
>> index 40041a22b3fe..7abbdcebf075 100644
>> --- a/kvm-cpu.c
>> +++ b/kvm-cpu.c
>> @@ -35,27 +35,32 @@ void kvm_cpu__enable_singlestep(struct kvm_cpu *vcpu)
>>   		pr_warning("KVM_SET_GUEST_DEBUG failed");
>>   }
>>   
>> -void kvm_cpu__run(struct kvm_cpu *vcpu)
>> +/*
>> + * return value -1 if we need to call the kvm_cpu__run again without checking
>> + * exit_reason. return value 0 results in taking action based on exit_reason.
>> + */
>
> minor nit: Should we make the return value meaningful, say -EAGAIN 
> instead of -1 ?
>

I was not sure. Having both EINTR and EAGAIN map to just EAGAIN is
confusing IMHO. Instead having a proper return value (-1) which is
documented to imply a retry is better?

>> +int kvm_cpu__run(struct kvm_cpu *vcpu)
>>   {
>>   	int err;
>>   
>>   	if (!vcpu->is_running)
>> -		return;
>> +		return -1;
>>   
>>   	err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
>>   	if (err < 0) {
>>   		switch (errno) {
>>   		case EINTR:
>>   		case EAGAIN:
>> -			return;
>> +			return -1;
>>   		case EFAULT:
>>   			if (vcpu->kvm_run->exit_reason == KVM_EXIT_MEMORY_FAULT)
>> -				return;
>> +				return 0;
>>   			/* fallthrough */
>>   		default:
>>   			die_perror("KVM_RUN failed");
>>   		}
>>   	}
>> +	return 0;
>>   }
>>   
>>   static void kvm_cpu_signal_handler(int signum)
>> @@ -179,7 +184,9 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
>>   		if (cpu->task)
>>   			kvm_cpu__run_task(cpu);
>>   
>> -		kvm_cpu__run(cpu);
>> +		if (kvm_cpu__run(cpu) == -1)
>
> and this could be :
> 		if (kvm_cpu__run(cpu) == -EAGAIN)
>
>> +			/* retry without an exit_reason check */
>> +			continue;
>>   
>>   		switch (cpu->kvm_run->exit_reason) {
>>   		case KVM_EXIT_UNKNOWN:
>
>
> Suzuki