Patchwork [08/28] kvm tools: Fix KVM_RUN exit code check

login
register
mail settings
Submitter Matt Evans
Date Dec. 6, 2011, 3:39 a.m.
Message ID <4EDD8E82.1010909@ozlabs.org>
Download mbox | patch
Permalink /patch/129503/
State New
Headers show

Comments

Matt Evans - Dec. 6, 2011, 3:39 a.m.
kvm_cpu__run() currently die()s if KVM_RUN returns non-zero.  Some architectures
may return positive values in non-error cases, whereas real errors are always
negative return values.  Check for those instead.

Signed-off-by: Matt Evans <matt@ozlabs.org>
---
 tools/kvm/kvm-cpu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin - Dec. 6, 2011, 8:22 a.m.
If KVM_RUN can actually return anything besides 0 or -1 it may be also
worthwhile to update Documentation/virtual/kvm/api.txt .

What are the cases where it happens?

On Tue, 2011-12-06 at 14:39 +1100, Matt Evans wrote:
> kvm_cpu__run() currently die()s if KVM_RUN returns non-zero.  Some architectures
> may return positive values in non-error cases, whereas real errors are always
> negative return values.  Check for those instead.
> 
> Signed-off-by: Matt Evans <matt@ozlabs.org>
> ---
>  tools/kvm/kvm-cpu.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
> index 9bc0796..884a89f 100644
> --- a/tools/kvm/kvm-cpu.c
> +++ b/tools/kvm/kvm-cpu.c
> @@ -30,7 +30,7 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
>  	int err;
>  
>  	err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
> -	if (err && (errno != EINTR && errno != EAGAIN))
> +	if (err < 0 && (errno != EINTR && errno != EAGAIN))
>  		die_perror("KVM_RUN failed");
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Evans - Dec. 7, 2011, 12:32 a.m.
On 06/12/11 19:22, Sasha Levin wrote:
> If KVM_RUN can actually return anything besides 0 or -1 it may be also
> worthwhile to update Documentation/virtual/kvm/api.txt .
> 
> What are the cases where it happens?

Well, on PPC the internal kvmppc_run_vcpu() returns either RESUME_GUEST (which
stays in-kernel and drops back to the guest) or RESUME_HOST, which is propagated
back out to userland as the return value of ioctl(KVM_RUN).  So, anything
kvmtool sees is either <0 for error or RESUME_HOST, i.e. 2.

Alex, do you think the PPC KVM code should be forced to 0 on success, or is
there any value to the expanded the return codes (and updating api.txt) for
varying kinds of positive success?


Cheers,


Matt


> 
> On Tue, 2011-12-06 at 14:39 +1100, Matt Evans wrote:
>> kvm_cpu__run() currently die()s if KVM_RUN returns non-zero.  Some architectures
>> may return positive values in non-error cases, whereas real errors are always
>> negative return values.  Check for those instead.
>>
>> Signed-off-by: Matt Evans <matt@ozlabs.org>
>> ---
>>  tools/kvm/kvm-cpu.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
>> index 9bc0796..884a89f 100644
>> --- a/tools/kvm/kvm-cpu.c
>> +++ b/tools/kvm/kvm-cpu.c
>> @@ -30,7 +30,7 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
>>  	int err;
>>  
>>  	err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
>> -	if (err && (errno != EINTR && errno != EAGAIN))
>> +	if (err < 0 && (errno != EINTR && errno != EAGAIN))
>>  		die_perror("KVM_RUN failed");
>>  }
>>  
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Dec. 7, 2011, 6:44 a.m.
On 07.12.2011, at 01:32, Matt Evans <matt@ozlabs.org> wrote:

> On 06/12/11 19:22, Sasha Levin wrote:
>> If KVM_RUN can actually return anything besides 0 or -1 it may be also
>> worthwhile to update Documentation/virtual/kvm/api.txt .
>> 
>> What are the cases where it happens?
> 
> Well, on PPC the internal kvmppc_run_vcpu() returns either RESUME_GUEST (which
> stays in-kernel and drops back to the guest) or RESUME_HOST, which is propagated
> back out to userland as the return value of ioctl(KVM_RUN).  So, anything
> kvmtool sees is either <0 for error or RESUME_HOST, i.e. 2.
> 
> Alex, do you think the PPC KVM code should be forced to 0 on success, or is
> there any value to the expanded the return codes (and updating api.txt) for
> varying kinds of positive success?

I don't think it's worth the potential ABI breakage to change the current behavior :). Even if we did change it, you would still have to touch kvm tool to work with older kernels.

Alex

> 
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity - Dec. 7, 2011, 2:12 p.m.
On 12/07/2011 08:44 AM, Alexander Graf wrote:
> On 07.12.2011, at 01:32, Matt Evans <matt@ozlabs.org> wrote:
>
> > On 06/12/11 19:22, Sasha Levin wrote:
> >> If KVM_RUN can actually return anything besides 0 or -1 it may be also
> >> worthwhile to update Documentation/virtual/kvm/api.txt .
> >> 
> >> What are the cases where it happens?
> > 
> > Well, on PPC the internal kvmppc_run_vcpu() returns either RESUME_GUEST (which
> > stays in-kernel and drops back to the guest) or RESUME_HOST, which is propagated
> > back out to userland as the return value of ioctl(KVM_RUN).  So, anything
> > kvmtool sees is either <0 for error or RESUME_HOST, i.e. 2.
> > 
> > Alex, do you think the PPC KVM code should be forced to 0 on success, or is
> > there any value to the expanded the return codes (and updating api.txt) for
> > varying kinds of positive success?
>
> I don't think it's worth the potential ABI breakage to change the current behavior :). Even if we did change it, you would still have to touch kvm tool to work with older kernels.

Well it deviates from api.txt, so please fix one or the other.
Alexander Graf - Dec. 7, 2011, 3:01 p.m.
On 07.12.2011, at 15:12, Avi Kivity <avi@redhat.com> wrote:

> On 12/07/2011 08:44 AM, Alexander Graf wrote:
>> On 07.12.2011, at 01:32, Matt Evans <matt@ozlabs.org> wrote:
>> 
>>> On 06/12/11 19:22, Sasha Levin wrote:
>>>> If KVM_RUN can actually return anything besides 0 or -1 it may be also
>>>> worthwhile to update Documentation/virtual/kvm/api.txt .
>>>> 
>>>> What are the cases where it happens?
>>> 
>>> Well, on PPC the internal kvmppc_run_vcpu() returns either RESUME_GUEST (which
>>> stays in-kernel and drops back to the guest) or RESUME_HOST, which is propagated
>>> back out to userland as the return value of ioctl(KVM_RUN).  So, anything
>>> kvmtool sees is either <0 for error or RESUME_HOST, i.e. 2.
>>> 
>>> Alex, do you think the PPC KVM code should be forced to 0 on success, or is
>>> there any value to the expanded the return codes (and updating api.txt) for
>>> varying kinds of positive success?
>> 
>> I don't think it's worth the potential ABI breakage to change the current behavior :). Even if we did change it, you would still have to touch kvm tool to work with older kernels.
> 
> Well it deviates from api.txt, so please fix one or the other.

Hrm - let me check all users. Maybe we can change the return argument. Will do when I'm back on a keyboard ;).

Alex

> 
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity - Dec. 7, 2011, 3:05 p.m.
On 12/07/2011 05:01 PM, Alexander Graf wrote:
> Will do when I'm back on a keyboard ;).

Siri, write a patch for bugzilla #123456 and post it upstream.
Matt Evans - Dec. 8, 2011, 3:03 a.m.
Hi Sasha,

On 06/12/11 19:22, Sasha Levin wrote:
> If KVM_RUN can actually return anything besides 0 or -1 it may be also
> worthwhile to update Documentation/virtual/kvm/api.txt .
> 
> What are the cases where it happens?

It sounds like Alex is considering KVM PPC's return value in this case (and
updating api.txt if appropriate) -- what say you on this patch?  It actually
brings kvmtool's KVM_RUN return val check in line with QEMU's (also "< 0") and
nothing PPC will run without it, currently.  (I'm about to repost a new series,
will include it for these reasons, until I hear more complaint ;) )


Cheers,


Matt

> 
> On Tue, 2011-12-06 at 14:39 +1100, Matt Evans wrote:
>> kvm_cpu__run() currently die()s if KVM_RUN returns non-zero.  Some architectures
>> may return positive values in non-error cases, whereas real errors are always
>> negative return values.  Check for those instead.
>>
>> Signed-off-by: Matt Evans <matt@ozlabs.org>
>> ---
>>  tools/kvm/kvm-cpu.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
>> index 9bc0796..884a89f 100644
>> --- a/tools/kvm/kvm-cpu.c
>> +++ b/tools/kvm/kvm-cpu.c
>> @@ -30,7 +30,7 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
>>  	int err;
>>  
>>  	err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
>> -	if (err && (errno != EINTR && errno != EAGAIN))
>> +	if (err < 0 && (errno != EINTR && errno != EAGAIN))
>>  		die_perror("KVM_RUN failed");
>>  }
>>  
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin - Dec. 8, 2011, 5:31 a.m.
On Thu, 2011-12-08 at 14:03 +1100, Matt Evans wrote:
> Hi Sasha,
> 
> On 06/12/11 19:22, Sasha Levin wrote:
> > If KVM_RUN can actually return anything besides 0 or -1 it may be also
> > worthwhile to update Documentation/virtual/kvm/api.txt .
> > 
> > What are the cases where it happens?
> 
> It sounds like Alex is considering KVM PPC's return value in this case (and
> updating api.txt if appropriate) -- what say you on this patch?  It actually
> brings kvmtool's KVM_RUN return val check in line with QEMU's (also "< 0") and
> nothing PPC will run without it, currently.  (I'm about to repost a new series,
> will include it for these reasons, until I hear more complaint ;) )

'<0' is fine as it's what api.txt says :)
Avi Kivity - Dec. 22, 2011, 10:03 a.m.
On 12/08/2011 07:31 AM, Sasha Levin wrote:
> > 
> > It sounds like Alex is considering KVM PPC's return value in this case (and
> > updating api.txt if appropriate) -- what say you on this patch?  It actually
> > brings kvmtool's KVM_RUN return val check in line with QEMU's (also "< 0") and
> > nothing PPC will run without it, currently.  (I'm about to repost a new series,
> > will include it for these reasons, until I hear more complaint ;) )
>
> '<0' is fine as it's what api.txt says :)

What? ioctls return -1 on error, not <0.

The syscalls do return <0, but libc converts that to -1/errno.
Sasha Levin - Dec. 22, 2011, 10:18 a.m.
On Thu, 2011-12-22 at 12:03 +0200, Avi Kivity wrote:
> On 12/08/2011 07:31 AM, Sasha Levin wrote:
> > > 
> > > It sounds like Alex is considering KVM PPC's return value in this case (and
> > > updating api.txt if appropriate) -- what say you on this patch?  It actually
> > > brings kvmtool's KVM_RUN return val check in line with QEMU's (also "< 0") and
> > > nothing PPC will run without it, currently.  (I'm about to repost a new series,
> > > will include it for these reasons, until I hear more complaint ;) )
> >
> > '<0' is fine as it's what api.txt says :)
> 
> What? ioctls return -1 on error, not <0.

<0 as opposed to the !=0 check we had there before.

Theres no harm in checking for <0 even if the only possible negative
result is -1.

> 
> The syscalls do return <0, but libc converts that to -1/errno.
>
Avi Kivity - Dec. 22, 2011, 10:21 a.m.
On 12/22/2011 12:18 PM, Sasha Levin wrote:
> On Thu, 2011-12-22 at 12:03 +0200, Avi Kivity wrote:
> > On 12/08/2011 07:31 AM, Sasha Levin wrote:
> > > > 
> > > > It sounds like Alex is considering KVM PPC's return value in this case (and
> > > > updating api.txt if appropriate) -- what say you on this patch?  It actually
> > > > brings kvmtool's KVM_RUN return val check in line with QEMU's (also "< 0") and
> > > > nothing PPC will run without it, currently.  (I'm about to repost a new series,
> > > > will include it for these reasons, until I hear more complaint ;) )
> > >
> > > '<0' is fine as it's what api.txt says :)
> > 
> > What? ioctls return -1 on error, not <0.
>
> <0 as opposed to the !=0 check we had there before.
>
> Theres no harm in checking for <0 even if the only possible negative
> result is -1.
>
>

Yes, but the documentation should say -1.  Which ioctl is this?
Sasha Levin - Dec. 22, 2011, 10:34 a.m.
On Thu, 2011-12-22 at 12:21 +0200, Avi Kivity wrote:
> On 12/22/2011 12:18 PM, Sasha Levin wrote:
> > On Thu, 2011-12-22 at 12:03 +0200, Avi Kivity wrote:
> > > On 12/08/2011 07:31 AM, Sasha Levin wrote:
> > > > > 
> > > > > It sounds like Alex is considering KVM PPC's return value in this case (and
> > > > > updating api.txt if appropriate) -- what say you on this patch?  It actually
> > > > > brings kvmtool's KVM_RUN return val check in line with QEMU's (also "< 0") and
> > > > > nothing PPC will run without it, currently.  (I'm about to repost a new series,
> > > > > will include it for these reasons, until I hear more complaint ;) )
> > > >
> > > > '<0' is fine as it's what api.txt says :)
> > > 
> > > What? ioctls return -1 on error, not <0.
> >
> > <0 as opposed to the !=0 check we had there before.
> >
> > Theres no harm in checking for <0 even if the only possible negative
> > result is -1.
> >
> >
> 
> Yes, but the documentation should say -1.  Which ioctl is this?

That was KVM_RUN, but the documentation on that is correct (-1), so I'm
not sure what I was thinking.

On the other hand, there are several ioctls that do need fixing:

 - KVM_IOEVENTFD
 - KVM_PPC_GET_PVINFO
 - KVM_CAP_GET_TSC_KHZ

Patch

diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
index 9bc0796..884a89f 100644
--- a/tools/kvm/kvm-cpu.c
+++ b/tools/kvm/kvm-cpu.c
@@ -30,7 +30,7 @@  void kvm_cpu__run(struct kvm_cpu *vcpu)
 	int err;
 
 	err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
-	if (err && (errno != EINTR && errno != EAGAIN))
+	if (err < 0 && (errno != EINTR && errno != EAGAIN))
 		die_perror("KVM_RUN failed");
 }