On 9/26/22 21:07, Peter Maydell wrote:
> On Mon, 26 Sept 2022 at 19:08, Vitaly Chikunov <v...@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-...@kernel.org/
>>>
>>> Reported-by: Vitaly Chikunov <v...@altlinux.org>
>>> Signed-off-by: Peter Maydell <peter.mayd...@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
>
