[PATCH kvmtool 1/2] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly

[PATCH kvmtool 1/2] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly

[Aneesh Kumar K.V (Arm)]

The return value for the KVM_RUN ioctl 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 KVM panic on an unknown exit reason would
help catch these bugs early.

Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
---
 kvm-cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kvm-cpu.c b/kvm-cpu.c
index f66dcd07220c..66e30ba54e26 100644
--- a/kvm-cpu.c
+++ b/kvm-cpu.c
@@ -170,6 +170,7 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
 
 		switch (cpu->kvm_run->exit_reason) {
 		case KVM_EXIT_UNKNOWN:
+			goto panic_kvm;
 			break;
 		case KVM_EXIT_DEBUG:
 			kvm_cpu__show_registers(cpu);

base-commit: 6d754d01fe2cb366f3b636d8a530f46ccf3b10d8
-- 
2.43.0

[PATCH kvmtool 2/2] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT correctly

[Aneesh Kumar K.V (Arm)]

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 the KVM_RUN ioctl error handling to correctly handle
KVM_EXIT_MEMORY_FAULT.

Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
---
 kvm-cpu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kvm-cpu.c b/kvm-cpu.c
index 66e30ba54e26..40e4efc33a1d 100644
--- a/kvm-cpu.c
+++ b/kvm-cpu.c
@@ -41,8 +41,15 @@ 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) {
+		if (errno == EINTR || errno == EAGAIN)
+			return;
+		else if (errno == EFAULT &&
+			 vcpu->kvm_run->exit_reason == KVM_EXIT_MEMORY_FAULT)
+			return;
+		else
+			die_perror("KVM_RUN failed");
+	}
 }
 
 static void kvm_cpu_signal_handler(int signum)
-- 
2.43.0

Re: [PATCH kvmtool 1/2] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly

[Will Deacon]

On Tue, Feb 11, 2025 at 01:08:51PM +0530, Aneesh Kumar K.V (Arm) wrote:
> The return value for the KVM_RUN ioctl 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.

Oops. Did any of those broken handlers reach mainline?

> 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 KVM panic on an unknown exit reason would
> help catch these bugs early.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
>  kvm-cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kvm-cpu.c b/kvm-cpu.c
> index f66dcd07220c..66e30ba54e26 100644
> --- a/kvm-cpu.c
> +++ b/kvm-cpu.c
> @@ -170,6 +170,7 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
>  
>  		switch (cpu->kvm_run->exit_reason) {
>  		case KVM_EXIT_UNKNOWN:
> +			goto panic_kvm;
>  			break;

The break is no longer needed.

Will

Re: [PATCH kvmtool 2/2] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT correctly

[Will Deacon]

On Tue, Feb 11, 2025 at 01:08:52PM +0530, 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 the KVM_RUN ioctl error handling to correctly handle
> KVM_EXIT_MEMORY_FAULT.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
>  kvm-cpu.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kvm-cpu.c b/kvm-cpu.c
> index 66e30ba54e26..40e4efc33a1d 100644
> --- a/kvm-cpu.c
> +++ b/kvm-cpu.c
> @@ -41,8 +41,15 @@ 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) {
> +		if (errno == EINTR || errno == EAGAIN)
> +			return;
> +		else if (errno == EFAULT &&
> +			 vcpu->kvm_run->exit_reason == KVM_EXIT_MEMORY_FAULT)
> +			return;
> +		else
> +			die_perror("KVM_RUN failed");
> +	}

Probably cleaner to switch on errno?

Will

Re: [PATCH kvmtool 1/2] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly

[Alexandru Elisei]

Hi,

On Tue, Feb 11, 2025 at 01:08:51PM +0530, Aneesh Kumar K.V (Arm) wrote:
> The return value for the KVM_RUN ioctl 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.

I find this paragraph confusing. KVM_RUN, as per the documentation, returns 0 or
-1 (on error). As far as I can tell, at least on arm64, KVM_RUN can never return
1.

Are you referring to the loop in kvm_arch_vcpu_ioctl_run() by any chance? That's
the only place I found where a value of 1 from the handlers signifies 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 KVM panic on an unknown exit reason would
> help catch these bugs early.

I would hope that a VMM cannot force KVM to panic at will, which will bring down
the host. Are you referring to kvmtool exiting with an error? That's what the
unfortunately named 'panic_kvm' label seems to be doing.

Thanks,
Alex

> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
>  kvm-cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kvm-cpu.c b/kvm-cpu.c
> index f66dcd07220c..66e30ba54e26 100644
> --- a/kvm-cpu.c
> +++ b/kvm-cpu.c
> @@ -170,6 +170,7 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
>  
>  		switch (cpu->kvm_run->exit_reason) {
>  		case KVM_EXIT_UNKNOWN:
> +			goto panic_kvm;
>  			break;
>  		case KVM_EXIT_DEBUG:
>  			kvm_cpu__show_registers(cpu);
> 
> base-commit: 6d754d01fe2cb366f3b636d8a530f46ccf3b10d8
> -- 
> 2.43.0
> 
> 

Re: [PATCH kvmtool 2/2] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT correctly

[Alexandru Elisei]

Hi,

On Tue, Feb 11, 2025 at 01:08:52PM +0530, 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 the KVM_RUN ioctl error handling to correctly handle
> KVM_EXIT_MEMORY_FAULT.

I've tried to follow how kvmtool handles KVM_EXIT_MEMORY_FAULT before and after
this patch.

Before: calls die_perror().
After: prints more information about the error, in kvm_cpu_thread().

Is that what you want? Because "correctly handle KVM_EXIT_MEMORY_FAULT" can be
interpreted as kvmtool resolving the memory fault, which is something that
kvmtool does not do.

Also, can you update kvm_exit_reasons with KVM_EXIT_MEMORY_FAULT, because
otherwise kvm_cpu_thread() will segfault when it tries to access
kvm_exit_reasons[KVM_EXIT_MEMORY_FAULT].

Thanks,
Alex

> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
>  kvm-cpu.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kvm-cpu.c b/kvm-cpu.c
> index 66e30ba54e26..40e4efc33a1d 100644
> --- a/kvm-cpu.c
> +++ b/kvm-cpu.c
> @@ -41,8 +41,15 @@ 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) {
> +		if (errno == EINTR || errno == EAGAIN)
> +			return;
> +		else if (errno == EFAULT &&
> +			 vcpu->kvm_run->exit_reason == KVM_EXIT_MEMORY_FAULT)
> +			return;
> +		else
> +			die_perror("KVM_RUN failed");
> +	}
>  }
>  
>  static void kvm_cpu_signal_handler(int signum)
> -- 
> 2.43.0
> 
> 

Re: [PATCH kvmtool 2/2] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT correctly

[Aneesh Kumar K.V]

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

> Hi,
>
> On Tue, Feb 11, 2025 at 01:08:52PM +0530, 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 the KVM_RUN ioctl error handling to correctly handle
>> KVM_EXIT_MEMORY_FAULT.
>
> I've tried to follow how kvmtool handles KVM_EXIT_MEMORY_FAULT before and after
> this patch.
>
> Before: calls die_perror().
> After: prints more information about the error, in kvm_cpu_thread().
>
> Is that what you want? Because "correctly handle KVM_EXIT_MEMORY_FAULT" can be
> interpreted as kvmtool resolving the memory fault, which is something that
> kvmtool does not do.
>

That is correct. The changes to enable the handling of
KVM_EXIT_MEMORY_FAULT is not yet part of upstream [1]. But then the
return value for KVM_RUN ioctl() is defined as part of
Documentation/virt/kvm/api.rst in the Linux kernel.

[1] https://gitlab.arm.com/linux-arm/kvmtool-cca/-/blob/cca/v4/arm/kvm-cpu.c?ref_type=heads#L247

>
> Also, can you update kvm_exit_reasons with KVM_EXIT_MEMORY_FAULT, because
> otherwise kvm_cpu_thread() will segfault when it tries to access
> kvm_exit_reasons[KVM_EXIT_MEMORY_FAULT].
>

Will update.

-aneesh

Re: [PATCH kvmtool 2/2] cpu: vmexit: Handle KVM_EXIT_MEMORY_FAULT correctly

[Aneesh Kumar K.V]

Will Deacon <will@kernel.org> writes:

> On Tue, Feb 11, 2025 at 01:08:52PM +0530, 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 the KVM_RUN ioctl error handling to correctly handle
>> KVM_EXIT_MEMORY_FAULT.
>>
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>> ---
>>  kvm-cpu.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/kvm-cpu.c b/kvm-cpu.c
>> index 66e30ba54e26..40e4efc33a1d 100644
>> --- a/kvm-cpu.c
>> +++ b/kvm-cpu.c
>> @@ -41,8 +41,15 @@ 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) {
>> +		if (errno == EINTR || errno == EAGAIN)
>> +			return;
>> +		else if (errno == EFAULT &&
>> +			 vcpu->kvm_run->exit_reason == KVM_EXIT_MEMORY_FAULT)
>> +			return;
>> +		else
>> +			die_perror("KVM_RUN failed");
>> +	}
>
> Probably cleaner to switch on errno?

This? . I will update.

	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");
		}
	}

-aneesh

Re: [PATCH kvmtool 1/2] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly

[Aneesh Kumar K.V]

Will Deacon <will@kernel.org> writes:

> On Tue, Feb 11, 2025 at 01:08:51PM +0530, Aneesh Kumar K.V (Arm) wrote:
>> The return value for the KVM_RUN ioctl 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.
>
> Oops. Did any of those broken handlers reach mainline?
>

Not that I noticed. We do have patches in review.

https://lore.kernel.org/all/20241212155610.76522-18-steven.price@arm.com


>> 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 KVM panic on an unknown exit reason would
>> help catch these bugs early.
>> 
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>> ---
>>  kvm-cpu.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/kvm-cpu.c b/kvm-cpu.c
>> index f66dcd07220c..66e30ba54e26 100644
>> --- a/kvm-cpu.c
>> +++ b/kvm-cpu.c
>> @@ -170,6 +170,7 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
>>  
>>  		switch (cpu->kvm_run->exit_reason) {
>>  		case KVM_EXIT_UNKNOWN:
>> +			goto panic_kvm;
>>  			break;
>
> The break is no longer needed.
>

ok.

-aneesh

Re: [PATCH kvmtool 1/2] cpu: vmexit: Handle KVM_EXIT_UNKNOWN exit reason correctly

[Aneesh Kumar K.V]

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

> Hi,
>
> On Tue, Feb 11, 2025 at 01:08:51PM +0530, Aneesh Kumar K.V (Arm) wrote:
>> The return value for the KVM_RUN ioctl 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.
>
> I find this paragraph confusing. KVM_RUN, as per the documentation, returns 0 or
> -1 (on error). As far as I can tell, at least on arm64, KVM_RUN can never return
> 1.
>
> Are you referring to the loop in kvm_arch_vcpu_ioctl_run() by any chance? That's
> the only place I found where a value of 1 from the handlers signifies return to
> the guest.
>

Yes. I will update the commit message to reflect that. It is the exit
handler return value rather than KVM_RUN ioctl return value.

>
>> 
>> 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 KVM panic on an unknown exit reason would
>> help catch these bugs early.
>
> I would hope that a VMM cannot force KVM to panic at will, which will bring down
> the host. Are you referring to kvmtool exiting with an error? That's what the
> unfortunately named 'panic_kvm' label seems to be doing.
>

yes. I will update the commit message to indicate that for
KVM_EXIT_UNKNOWN exit reason, kvmtool will now exit with an error
instead of returning to the guest."

-aneesh