kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes
@ 2025-04-28 11:57 Aneesh Kumar K.V (Arm)
  2025-04-28 11:57 ` [PATCH kvmtool v3 1/3] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return Aneesh Kumar K.V (Arm)
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Aneesh Kumar K.V (Arm) @ 2025-04-28 11:57 UTC (permalink / raw)
  To: kvm
  Cc: Suzuki K Poulose, Steven Price, Will Deacon, Julien Thierry,
	Aneesh Kumar K.V (Arm)

Changes from v2:
* Fix smp boot failure on x86.

Aneesh Kumar K.V (Arm) (3):
  cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return
  cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN
  cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly

 include/kvm/kvm-cpu.h |  2 +-
 kvm-cpu.c             | 30 ++++++++++++++++++++++++------
 kvm.c                 |  1 +
 3 files changed, 26 insertions(+), 7 deletions(-)


base-commit: d410d9a16f91458ae2b912cc088015396f22dfad
-- 
2.43.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH kvmtool v3 1/3] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return
  2025-04-28 11:57 [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes Aneesh Kumar K.V (Arm)
@ 2025-04-28 11:57 ` Aneesh Kumar K.V (Arm)
  2025-04-29 11:07   ` Suzuki K Poulose
  2025-04-28 11:57 ` [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN Aneesh Kumar K.V (Arm)
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Aneesh Kumar K.V (Arm) @ 2025-04-28 11:57 UTC (permalink / raw)
  To: kvm
  Cc: Suzuki K Poulose, Steven Price, Will Deacon, Julien Thierry,
	Aneesh Kumar K.V (Arm), Alexandru Elisei

Linux kernel documentation states:

"Note! KVM_EXIT_MEMORY_FAULT is unique among all KVM exit reasons in
that it accompanies a return code of '-1', not '0'! errno will always be
set to EFAULT or EHWPOISON when KVM exits with KVM_EXIT_MEMORY_FAULT,
userspace should assume kvm_run.exit_reason is stale/undefined for all
other error numbers." "

Update KVM_RUN ioctl error handling to correctly handle
KVM_EXIT_MEMORY_FAULT. This enables the memory fault exit handlers in
the kernel to return -EFAULT as the return value. VMM support is
still required to handle these memory fault exits, but that is not
included in this change

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
---
 kvm-cpu.c | 15 +++++++++++++--
 kvm.c     |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/kvm-cpu.c b/kvm-cpu.c
index 7362f2e99261..40041a22b3fe 100644
--- a/kvm-cpu.c
+++ b/kvm-cpu.c
@@ -43,8 +43,19 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
 		return;
 
 	err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
-	if (err < 0 && (errno != EINTR && errno != EAGAIN))
-		die_perror("KVM_RUN failed");
+	if (err < 0) {
+		switch (errno) {
+		case EINTR:
+		case EAGAIN:
+			return;
+		case EFAULT:
+			if (vcpu->kvm_run->exit_reason == KVM_EXIT_MEMORY_FAULT)
+				return;
+			/* fallthrough */
+		default:
+			die_perror("KVM_RUN failed");
+		}
+	}
 }
 
 static void kvm_cpu_signal_handler(int signum)
diff --git a/kvm.c b/kvm.c
index 07089cf1b332..b6375a114d11 100644
--- a/kvm.c
+++ b/kvm.c
@@ -55,6 +55,7 @@ const char *kvm_exit_reasons[] = {
 #ifdef CONFIG_PPC64
 	DEFINE_KVM_EXIT_REASON(KVM_EXIT_PAPR_HCALL),
 #endif
+	DEFINE_KVM_EXIT_REASON(KVM_EXIT_MEMORY_FAULT),
 };
 
 static int pause_event;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN
  2025-04-28 11:57 [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes Aneesh Kumar K.V (Arm)
  2025-04-28 11:57 ` [PATCH kvmtool v3 1/3] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return Aneesh Kumar K.V (Arm)
@ 2025-04-28 11:57 ` Aneesh Kumar K.V (Arm)
  2025-04-28 13:37   ` Suzuki K Poulose
  2025-04-28 15:22   ` Alexandru Elisei
  2025-04-28 11:57 ` [PATCH kvmtool v3 3/3] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly Aneesh Kumar K.V (Arm)
  2025-06-10  8:01 ` [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes Aneesh Kumar K.V
  3 siblings, 2 replies; 11+ messages in thread
From: Aneesh Kumar K.V (Arm) @ 2025-04-28 11:57 UTC (permalink / raw)
  To: kvm
  Cc: Suzuki K Poulose, Steven Price, Will Deacon, Julien Thierry,
	Aneesh Kumar K.V (Arm)

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.
+ */
+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)
+			/* retry without an exit_reason check */
+			continue;
 
 		switch (cpu->kvm_run->exit_reason) {
 		case KVM_EXIT_UNKNOWN:
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH kvmtool v3 3/3] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly
  2025-04-28 11:57 [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes Aneesh Kumar K.V (Arm)
  2025-04-28 11:57 ` [PATCH kvmtool v3 1/3] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return Aneesh Kumar K.V (Arm)
  2025-04-28 11:57 ` [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN Aneesh Kumar K.V (Arm)
@ 2025-04-28 11:57 ` Aneesh Kumar K.V (Arm)
  2025-06-10  8:01 ` [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes Aneesh Kumar K.V
  3 siblings, 0 replies; 11+ messages in thread
From: Aneesh Kumar K.V (Arm) @ 2025-04-28 11:57 UTC (permalink / raw)
  To: kvm
  Cc: Suzuki K Poulose, Steven Price, Will Deacon, Julien Thierry,
	Aneesh Kumar K.V (Arm), Alexandru Elisei

The return value for kernel VM exit handlers is confusing and has led to
errors in different kernel exit handlers. A return value of 0 indicates
a return to the VMM, whereas a return value of 1 indicates resuming
execution in the guest. Some handlers mistakenly return 0 to force a
return to the guest.

This worked in kvmtool because the exit_reason defaulted to
0 (KVM_EXIT_UNKNOWN), and kvmtool did not error out on an unknown exit
reason. However, forcing a VMM exit with error on KVM_EXIT_UNKNOWN
exit_reson would help catch these bugs early.

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
---
 kvm-cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kvm-cpu.c b/kvm-cpu.c
index 7abbdcebf075..a76dfee561ec 100644
--- a/kvm-cpu.c
+++ b/kvm-cpu.c
@@ -190,7 +190,7 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
 
 		switch (cpu->kvm_run->exit_reason) {
 		case KVM_EXIT_UNKNOWN:
-			break;
+			goto panic_kvm;
 		case KVM_EXIT_DEBUG:
 			kvm_cpu__show_registers(cpu);
 			kvm_cpu__show_code(cpu);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN
  2025-04-28 11:57 ` [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN Aneesh Kumar K.V (Arm)
@ 2025-04-28 13:37   ` Suzuki K Poulose
  2025-04-29  3:31     ` Aneesh Kumar K.V
  2025-04-28 15:22   ` Alexandru Elisei
  1 sibling, 1 reply; 11+ messages in thread
From: Suzuki K Poulose @ 2025-04-28 13:37 UTC (permalink / raw)
  To: Aneesh Kumar K.V (Arm), kvm; +Cc: Steven Price, Will Deacon, Julien Thierry

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 ?

> +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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN
  2025-04-28 11:57 ` [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN Aneesh Kumar K.V (Arm)
  2025-04-28 13:37   ` Suzuki K Poulose
@ 2025-04-28 15:22   ` Alexandru Elisei
  2025-04-29  3:37     ` Aneesh Kumar K.V
  1 sibling, 1 reply; 11+ messages in thread
From: Alexandru Elisei @ 2025-04-28 15:22 UTC (permalink / raw)
  To: Aneesh Kumar K.V (Arm)
  Cc: kvm, Suzuki K Poulose, Steven Price, Will Deacon, Julien Thierry

Hi Aneesh,

Is this to fix Will's report that the series breaks boot on x86?

On Mon, Apr 28, 2025 at 05:27:44PM +0530, 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.

EAGAIN is not documented in Documentation/virt/kvm/api.rst. So I'm going to
assume it's this code path that is causing the -EAGAIN return value [1].

If that's the case, how does retrying KVM_RUN solve the issue? Just trying to
get to the bottom of it, because there's not much detail in the docs.

[1] https://elixir.bootlin.com/linux/v6.15-rc3/source/arch/x86/kvm/x86.c#L11532


Thanks,
Alex

> 
> 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.
> + */
> +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)
> +			/* retry without an exit_reason check */
> +			continue;
>  
>  		switch (cpu->kvm_run->exit_reason) {
>  		case KVM_EXIT_UNKNOWN:
> -- 
> 2.43.0
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN
  2025-04-28 13:37   ` Suzuki K Poulose
@ 2025-04-29  3:31     ` Aneesh Kumar K.V
  0 siblings, 0 replies; 11+ messages in thread
From: Aneesh Kumar K.V @ 2025-04-29  3:31 UTC (permalink / raw)
  To: Suzuki K Poulose, kvm; +Cc: Steven Price, Will Deacon, Julien Thierry

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN
  2025-04-28 15:22   ` Alexandru Elisei
@ 2025-04-29  3:37     ` Aneesh Kumar K.V
  2025-04-29  8:38       ` Alexandru Elisei
  0 siblings, 1 reply; 11+ messages in thread
From: Aneesh Kumar K.V @ 2025-04-29  3:37 UTC (permalink / raw)
  To: Alexandru Elisei
  Cc: kvm, Suzuki K Poulose, Steven Price, Will Deacon, Julien Thierry

Alexandru Elisei <alexandru.elisei@arm.com> writes:

> Hi Aneesh,
>
> Is this to fix Will's report that the series breaks boot on x86?
>

Yes.

> On Mon, Apr 28, 2025 at 05:27:44PM +0530, 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.
>
> EAGAIN is not documented in Documentation/virt/kvm/api.rst. So I'm going to
> assume it's this code path that is causing the -EAGAIN return value [1].
>

IIUC, EAGAIN and EINTR are syscall return errno that indicates a system
call need to be retried. Documentation/virt/kvm/api.rst do mention that
exit_reason is value only with return value 0.

	__u32 exit_reason;

When KVM_RUN has returned successfully (return value 0), this informs
application code why KVM_RUN has returned.

>
> If that's the case, how does retrying KVM_RUN solve the issue? Just trying to
> get to the bottom of it, because there's not much detail in the docs.
>
> [1] https://elixir.bootlin.com/linux/v6.15-rc3/source/arch/x86/kvm/x86.c#L11532
>
>

So in that code path vcpu will be in kvm_vcpu_block(vcpu) waiting for
the IPI. On IPI kvm_apic_accept_events() will return 0 after setting the
vcpu->arch.mp_state. Hence a KVM_RUN ioctl again will find the mp_state
correctly updated. 

-aneesh

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN
  2025-04-29  3:37     ` Aneesh Kumar K.V
@ 2025-04-29  8:38       ` Alexandru Elisei
  0 siblings, 0 replies; 11+ messages in thread
From: Alexandru Elisei @ 2025-04-29  8:38 UTC (permalink / raw)
  To: Aneesh Kumar K.V
  Cc: kvm, Suzuki K Poulose, Steven Price, Will Deacon, Julien Thierry

Hi Anessh,

On Tue, Apr 29, 2025 at 09:07:02AM +0530, Aneesh Kumar K.V wrote:
> Alexandru Elisei <alexandru.elisei@arm.com> writes:
> 
> > Hi Aneesh,
> >
> > Is this to fix Will's report that the series breaks boot on x86?
> >
> 
> Yes.
> 
> > On Mon, Apr 28, 2025 at 05:27:44PM +0530, 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.
> >
> > EAGAIN is not documented in Documentation/virt/kvm/api.rst. So I'm going to
> > assume it's this code path that is causing the -EAGAIN return value [1].
> >
> 
> IIUC, EAGAIN and EINTR are syscall return errno that indicates a system
> call need to be retried. Documentation/virt/kvm/api.rst do mention that
> exit_reason is value only with return value 0.
> 
> 	__u32 exit_reason;
> 
> When KVM_RUN has returned successfully (return value 0), this informs
> application code why KVM_RUN has returned.

Yeah, that makes sense. api.rst is a bit weird, because it doesn't document
all of the error codes, and they are documented in different places, like
-EFAULT which is under KVM_EXIT_MEMORY_FAULT, not under KVM_RUN.

Thanks for the explanation!

> 
> >
> > If that's the case, how does retrying KVM_RUN solve the issue? Just trying to
> > get to the bottom of it, because there's not much detail in the docs.
> >
> > [1] https://elixir.bootlin.com/linux/v6.15-rc3/source/arch/x86/kvm/x86.c#L11532
> >
> >
> 
> So in that code path vcpu will be in kvm_vcpu_block(vcpu) waiting for
> the IPI. On IPI kvm_apic_accept_events() will return 0 after setting the
> vcpu->arch.mp_state. Hence a KVM_RUN ioctl again will find the mp_state
> correctly updated. 

I think that would be useful to have in the commit message.

Thanks,
Alex

> 
> -aneesh

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH kvmtool v3 1/3] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return
  2025-04-28 11:57 ` [PATCH kvmtool v3 1/3] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return Aneesh Kumar K.V (Arm)
@ 2025-04-29 11:07   ` Suzuki K Poulose
  0 siblings, 0 replies; 11+ messages in thread
From: Suzuki K Poulose @ 2025-04-29 11:07 UTC (permalink / raw)
  To: Aneesh Kumar K.V (Arm), kvm
  Cc: Steven Price, Will Deacon, Julien Thierry, Alexandru Elisei

On 28/04/2025 12:57, Aneesh Kumar K.V (Arm) wrote:
> Linux kernel documentation states:
> 
> "Note! KVM_EXIT_MEMORY_FAULT is unique among all KVM exit reasons in
> that it accompanies a return code of '-1', not '0'! errno will always be
> set to EFAULT or EHWPOISON when KVM exits with KVM_EXIT_MEMORY_FAULT,
> userspace should assume kvm_run.exit_reason is stale/undefined for all
> other error numbers." "
> 
> Update KVM_RUN ioctl error handling to correctly handle
> KVM_EXIT_MEMORY_FAULT. This enables the memory fault exit handlers in
> the kernel to return -EFAULT as the return value. VMM support is
> still required to handle these memory fault exits, but that is not
> included in this change
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
>   kvm-cpu.c | 15 +++++++++++++--
>   kvm.c     |  1 +
>   2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/kvm-cpu.c b/kvm-cpu.c
> index 7362f2e99261..40041a22b3fe 100644
> --- a/kvm-cpu.c
> +++ b/kvm-cpu.c
> @@ -43,8 +43,19 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
>   		return;
>   
>   	err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
> -	if (err < 0 && (errno != EINTR && errno != EAGAIN))
> -		die_perror("KVM_RUN failed");
> +	if (err < 0) {
> +		switch (errno) {
> +		case EINTR:
> +		case EAGAIN:
> +			return;
> +		case EFAULT:

Do we need to handle EHWPOISON too, as per the API ?


Suzuki


> +			if (vcpu->kvm_run->exit_reason == KVM_EXIT_MEMORY_FAULT)
> +				return;
> +			/* fallthrough */
> +		default:
> +			die_perror("KVM_RUN failed");


> +		}
> +	}
>   }
>   
>   static void kvm_cpu_signal_handler(int signum)
> diff --git a/kvm.c b/kvm.c
> index 07089cf1b332..b6375a114d11 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -55,6 +55,7 @@ const char *kvm_exit_reasons[] = {
>   #ifdef CONFIG_PPC64
>   	DEFINE_KVM_EXIT_REASON(KVM_EXIT_PAPR_HCALL),
>   #endif
> +	DEFINE_KVM_EXIT_REASON(KVM_EXIT_MEMORY_FAULT),
>   };
>   
>   static int pause_event;


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes
  2025-04-28 11:57 [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes Aneesh Kumar K.V (Arm)
                   ` (2 preceding siblings ...)
  2025-04-28 11:57 ` [PATCH kvmtool v3 3/3] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly Aneesh Kumar K.V (Arm)
@ 2025-06-10  8:01 ` Aneesh Kumar K.V
  3 siblings, 0 replies; 11+ messages in thread
From: Aneesh Kumar K.V @ 2025-06-10  8:01 UTC (permalink / raw)
  To: kvm

"Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> writes:

> Changes from v2:
> * Fix smp boot failure on x86.
>
> Aneesh Kumar K.V (Arm) (3):
>   cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return
>   cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN
>   cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly
>
>  include/kvm/kvm-cpu.h |  2 +-
>  kvm-cpu.c             | 30 ++++++++++++++++++++++++------
>  kvm.c                 |  1 +
>  3 files changed, 26 insertions(+), 7 deletions(-)
>
>
> base-commit: d410d9a16f91458ae2b912cc088015396f22dfad

Any feedback on this?

-aneesh

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-06-10 13:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-28 11:57 [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes Aneesh Kumar K.V (Arm)
2025-04-28 11:57 ` [PATCH kvmtool v3 1/3] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT in KVM_RUN ioctl return Aneesh Kumar K.V (Arm)
2025-04-29 11:07   ` Suzuki K Poulose
2025-04-28 11:57 ` [PATCH kvmtool v3 2/3] cpu: vmexit: Retry KVM_RUN ioctl on EINTR and EAGAIN Aneesh Kumar K.V (Arm)
2025-04-28 13:37   ` Suzuki K Poulose
2025-04-29  3:31     ` Aneesh Kumar K.V
2025-04-28 15:22   ` Alexandru Elisei
2025-04-29  3:37     ` Aneesh Kumar K.V
2025-04-29  8:38       ` Alexandru Elisei
2025-04-28 11:57 ` [PATCH kvmtool v3 3/3] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly Aneesh Kumar K.V (Arm)
2025-06-10  8:01 ` [PATCH kvmtool v3 0/3] cpu: vmexit: KVM_RUN ioctl error handling fixes Aneesh Kumar K.V

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).