diff mbox series

target/arm/kvm: Retry KVM_CREATE_VM call if it fails EINTR

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

Commit Message

Peter Maydell Sept. 26, 2022, 1:36 p.m. UTC
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(-)

Comments

Eric Auger Sept. 26, 2022, 1:57 p.m. UTC | #1
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;
>      }
Marc Zyngier Sept. 26, 2022, 2:26 p.m. UTC | #2
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.
Vitaly Chikunov Sept. 26, 2022, 6:08 p.m. UTC | #3
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
Peter Maydell Sept. 26, 2022, 7:07 p.m. UTC | #4
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
Eric Auger Sept. 26, 2022, 7:44 p.m. UTC | #5
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 mbox series

Patch

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