| Message ID | 4EDD8E82.1010909@ozlabs.org |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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
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.
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
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 :)
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.
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. >
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?
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
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"); }
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