| Message ID | 20220926133644.1345269-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm/kvm: Retry KVM_CREATE_VM call if it fails EINTR | expand |
Hi Peter, On 9/26/22 15:36, Peter Maydell wrote: > Occasionally the KVM_CREATE_VM ioctl can return EINTR, even though > there is no pending signal to be taken. In commit 94ccff13382055 > we added a retry-on-EINTR loop to the KVM_CREATE_VM call in the > generic KVM code. Adopt the same approach for the use of the > ioctl in the Arm-specific KVM code (where we use it to create a > scratch VM for probing for various things). > > For more information, see the mailing list thread: > https://lore.kernel.org/qemu-devel/8735e0s1zw.wl-maz@kernel.org/ > > Reported-by: Vitaly Chikunov <vt@altlinux.org> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > The view in the thread seems to be that this is a kernel bug (because > in QEMU's case there shouldn't be a signal to be delivered at this > point because of our signal handling strategy); so I've adopted the > same "just retry-on-EINTR for this specific ioctl" approach that > commit 94ccff13 did, rather than, for instance, something wider like > "make kvm_ioctl() and friends always retry on EINTR". > --- Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > target/arm/kvm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index e5c1bd50d29..2982d216176 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -79,7 +79,9 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > if (max_vm_pa_size < 0) { > max_vm_pa_size = 0; > } > - vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); > + do { > + vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); > + } while (vmfd == -EINTR); > if (vmfd < 0) { > goto err; > }
On Mon, 26 Sep 2022 09:36:44 -0400, Peter Maydell <peter.maydell@linaro.org> wrote: > > Occasionally the KVM_CREATE_VM ioctl can return EINTR, even though > there is no pending signal to be taken. In commit 94ccff13382055 > we added a retry-on-EINTR loop to the KVM_CREATE_VM call in the > generic KVM code. Adopt the same approach for the use of the > ioctl in the Arm-specific KVM code (where we use it to create a > scratch VM for probing for various things). > > For more information, see the mailing list thread: > https://lore.kernel.org/qemu-devel/8735e0s1zw.wl-maz@kernel.org/ > > Reported-by: Vitaly Chikunov <vt@altlinux.org> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Acked-by: Marc Zyngier <maz@kernel.org> M.
Peter, On Mon, Sep 26, 2022 at 02:36:44PM +0100, Peter Maydell wrote: > Occasionally the KVM_CREATE_VM ioctl can return EINTR, even though > there is no pending signal to be taken. In commit 94ccff13382055 > we added a retry-on-EINTR loop to the KVM_CREATE_VM call in the > generic KVM code. Adopt the same approach for the use of the > ioctl in the Arm-specific KVM code (where we use it to create a > scratch VM for probing for various things). > > For more information, see the mailing list thread: > https://lore.kernel.org/qemu-devel/8735e0s1zw.wl-maz@kernel.org/ > > Reported-by: Vitaly Chikunov <vt@altlinux.org> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > The view in the thread seems to be that this is a kernel bug (because > in QEMU's case there shouldn't be a signal to be delivered at this > point because of our signal handling strategy); so I've adopted the > same "just retry-on-EINTR for this specific ioctl" approach that > commit 94ccff13 did, rather than, for instance, something wider like > "make kvm_ioctl() and friends always retry on EINTR". > --- > target/arm/kvm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index e5c1bd50d29..2982d216176 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -79,7 +79,9 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > if (max_vm_pa_size < 0) { > max_vm_pa_size = 0; > } > - vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); > + do { > + vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); > + } while (vmfd == -EINTR); This does not seem correct. ioctl(2) returns -1 on error and will set errno to EINTR (in this case). Thanks, > if (vmfd < 0) { > goto err; > } > -- > 2.25.1
On Mon, 26 Sept 2022 at 19:08, Vitaly Chikunov <vt@altlinux.org> wrote: > > Peter, > > On Mon, Sep 26, 2022 at 02:36:44PM +0100, Peter Maydell wrote: > > Occasionally the KVM_CREATE_VM ioctl can return EINTR, even though > > there is no pending signal to be taken. In commit 94ccff13382055 > > we added a retry-on-EINTR loop to the KVM_CREATE_VM call in the > > generic KVM code. Adopt the same approach for the use of the > > ioctl in the Arm-specific KVM code (where we use it to create a > > scratch VM for probing for various things). > > > > For more information, see the mailing list thread: > > https://lore.kernel.org/qemu-devel/8735e0s1zw.wl-maz@kernel.org/ > > > > Reported-by: Vitaly Chikunov <vt@altlinux.org> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > The view in the thread seems to be that this is a kernel bug (because > > in QEMU's case there shouldn't be a signal to be delivered at this > > point because of our signal handling strategy); so I've adopted the > > same "just retry-on-EINTR for this specific ioctl" approach that > > commit 94ccff13 did, rather than, for instance, something wider like > > "make kvm_ioctl() and friends always retry on EINTR". > > --- > > target/arm/kvm.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > > index e5c1bd50d29..2982d216176 100644 > > --- a/target/arm/kvm.c > > +++ b/target/arm/kvm.c > > @@ -79,7 +79,9 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, > > if (max_vm_pa_size < 0) { > > max_vm_pa_size = 0; > > } > > - vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); > > + do { > > + vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); > > + } while (vmfd == -EINTR); > > This does not seem correct. ioctl(2) returns -1 on error and will set errno > to EINTR (in this case). Oops, thanks for the catch. I was copying the pattern from kvm-all.c, but there we are calling kvm_ioctl(), which does "if ioctl returns -1, return -errno", and here we are calling ioctl() directly. (We can't use kvm_ioctl() here because this stunt VM doesn't have a KVMState.) I'll respin the patch with the obvious fix. thanks -- PMM
On 9/26/22 21:07, Peter Maydell wrote: > On Mon, 26 Sept 2022 at 19:08, Vitaly Chikunov <vt@altlinux.org> wrote: >> >> Peter, >> >> On Mon, Sep 26, 2022 at 02:36:44PM +0100, Peter Maydell wrote: >>> Occasionally the KVM_CREATE_VM ioctl can return EINTR, even though >>> there is no pending signal to be taken. In commit 94ccff13382055 >>> we added a retry-on-EINTR loop to the KVM_CREATE_VM call in the >>> generic KVM code. Adopt the same approach for the use of the >>> ioctl in the Arm-specific KVM code (where we use it to create a >>> scratch VM for probing for various things). >>> >>> For more information, see the mailing list thread: >>> https://lore.kernel.org/qemu-devel/8735e0s1zw.wl-maz@kernel.org/ >>> >>> Reported-by: Vitaly Chikunov <vt@altlinux.org> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> The view in the thread seems to be that this is a kernel bug (because >>> in QEMU's case there shouldn't be a signal to be delivered at this >>> point because of our signal handling strategy); so I've adopted the >>> same "just retry-on-EINTR for this specific ioctl" approach that >>> commit 94ccff13 did, rather than, for instance, something wider like >>> "make kvm_ioctl() and friends always retry on EINTR". >>> --- >>> target/arm/kvm.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c >>> index e5c1bd50d29..2982d216176 100644 >>> --- a/target/arm/kvm.c >>> +++ b/target/arm/kvm.c >>> @@ -79,7 +79,9 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, >>> if (max_vm_pa_size < 0) { >>> max_vm_pa_size = 0; >>> } >>> - vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); >>> + do { >>> + vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); >>> + } while (vmfd == -EINTR); >> >> This does not seem correct. ioctl(2) returns -1 on error and will set errno >> to EINTR (in this case). > > Oops, thanks for the catch. I was copying the pattern from kvm-all.c, > but there we are calling kvm_ioctl(), which does "if ioctl > returns -1, return -errno", and here we are calling ioctl() directly. > (We can't use kvm_ioctl() here because this stunt VM doesn't have > a KVMState.) I'll respin the patch with the obvious fix. Damned, I did not notice either, sorry. Thank you Vitaly for the catch. Eric > > thanks > -- PMM >
diff --git a/target/arm/kvm.c b/target/arm/kvm.c index e5c1bd50d29..2982d216176 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -79,7 +79,9 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try, if (max_vm_pa_size < 0) { max_vm_pa_size = 0; } - vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); + do { + vmfd = ioctl(kvmfd, KVM_CREATE_VM, max_vm_pa_size); + } while (vmfd == -EINTR); if (vmfd < 0) { goto err; }
Occasionally the KVM_CREATE_VM ioctl can return EINTR, even though there is no pending signal to be taken. In commit 94ccff13382055 we added a retry-on-EINTR loop to the KVM_CREATE_VM call in the generic KVM code. Adopt the same approach for the use of the ioctl in the Arm-specific KVM code (where we use it to create a scratch VM for probing for various things). For more information, see the mailing list thread: https://lore.kernel.org/qemu-devel/8735e0s1zw.wl-maz@kernel.org/ Reported-by: Vitaly Chikunov <vt@altlinux.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- The view in the thread seems to be that this is a kernel bug (because in QEMU's case there shouldn't be a signal to be delivered at this point because of our signal handling strategy); so I've adopted the same "just retry-on-EINTR for this specific ioctl" approach that commit 94ccff13 did, rather than, for instance, something wider like "make kvm_ioctl() and friends always retry on EINTR". --- target/arm/kvm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)