[PATCH] tests/qtest/migration: Restore include for postcopy

[PATCH] tests/qtest/migration: Restore include for postcopy

[Fabiano Rosas]

Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to
utils") moved the ufd_version_check() function to another file but
failed to bring along the <sys/syscall> include, which is necessary to
pull in <asm/unistd.h> for __NR_userfaultd.

Restore the missing include.

While here, remove the ifdef __linux__ that's redundant and fix a
couple of typos.

Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration/migration-util.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/migration/migration-util.c b/tests/qtest/migration/migration-util.c
index 525bf1eed4..d18841e357 100644
--- a/tests/qtest/migration/migration-util.c
+++ b/tests/qtest/migration/migration-util.c
@@ -22,8 +22,12 @@
 #include "migration/bootfile.h"
 #include "migration/migration-util.h"
 
-/* for uffd_version_check() */
-#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
+#if defined(__linux__)
+#include <sys/syscall.h>
+#endif
+
+/* for ufd_version_check() */
+#if defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
 #include <sys/eventfd.h>
 #include "qemu/userfaultfd.h"
 #endif
@@ -297,7 +301,7 @@ bool probe_o_direct_support(const char *tmpfs)
 }
 #endif
 
-#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
+#if defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD)
 bool ufd_version_check(bool *uffd_feature_thread_id)
 {
     struct uffdio_api api_struct;
@@ -333,7 +337,7 @@ bool ufd_version_check(bool *uffd_feature_thread_id)
 #else
 bool ufd_version_check(bool *uffd_feature_thread_id)
 {
-    g_test_message("Skipping test: Userfault not available (builtdtime)");
+    g_test_message("Skipping test: Userfault not available (buildtime)");
     return false;
 }
 #endif
-- 
2.35.3

Re: [PATCH] tests/qtest/migration: Restore include for postcopy

[Peter Xu]

On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote:
> Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to
> utils") moved the ufd_version_check() function to another file but
> failed to bring along the <sys/syscall> include, which is necessary to
> pull in <asm/unistd.h> for __NR_userfaultd.
> 
> Restore the missing include.

Ohhhhhhh.. so postcopy tests will always be skipped as of now?  Maybe worth
explicit mention that in the commit message if so, only when you merge.

> 
> While here, remove the ifdef __linux__ that's redundant and fix a
> couple of typos.
> 
> Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils")
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

Maybe we don't need to be as careful on old kernels anymore especially in
tests, because userfaultfd syscall existed for ~10 years. So if we want we
can start requiring __NR_userfaultfd present for __linux__, then no way to
miss such spot next time.

-- 
Peter Xu

Re: [PATCH] tests/qtest/migration: Restore include for postcopy

[Daniel P. Berrangé]

On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote:
> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote:
> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to
> > utils") moved the ufd_version_check() function to another file but
> > failed to bring along the <sys/syscall> include, which is necessary to
> > pull in <asm/unistd.h> for __NR_userfaultd.
> > 
> > Restore the missing include.
> 
> Ohhhhhhh.. so postcopy tests will always be skipped as of now?  Maybe worth
> explicit mention that in the commit message if so, only when you merge.
> 
> > 
> > While here, remove the ifdef __linux__ that's redundant and fix a
> > couple of typos.
> > 
> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils")
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> Maybe we don't need to be as careful on old kernels anymore especially in
> tests, because userfaultfd syscall existed for ~10 years. So if we want we
> can start requiring __NR_userfaultfd present for __linux__, then no way to
> miss such spot next time.

Yes, I think that check is obsolete, based on our supported platforms
list. It would suffice to just check __linux__.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] tests/qtest/migration: Restore include for postcopy

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote:
>> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote:
>> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to
>> > utils") moved the ufd_version_check() function to another file but
>> > failed to bring along the <sys/syscall> include, which is necessary to
>> > pull in <asm/unistd.h> for __NR_userfaultd.
>> > 
>> > Restore the missing include.
>> 
>> Ohhhhhhh.. so postcopy tests will always be skipped as of now?  Maybe worth
>> explicit mention that in the commit message if so, only when you merge.
>> 
>> > 
>> > While here, remove the ifdef __linux__ that's redundant and fix a
>> > couple of typos.
>> > 
>> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils")
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> 
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> 
>> Maybe we don't need to be as careful on old kernels anymore especially in
>> tests, because userfaultfd syscall existed for ~10 years. So if we want we
>> can start requiring __NR_userfaultfd present for __linux__, then no way to
>> miss such spot next time.
>
> Yes, I think that check is obsolete, based on our supported platforms
> list. It would suffice to just check __linux__.

This breaks the cross builds. It seems the __NR_userfaultfd was actually
stopping several archs from reaching ufd_version_check(). Since
<sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find
the 'ioctl' symbol:

https://gitlab.com/farosas/qemu/-/pipelines/1594332399

Of course I could just include <sys/ioctl.h> unconditionally, but the
fact that new code is not being built means the assumption that we can
imply __NR_userfaultfd from __linux__ alone is not correct.

Re: [PATCH] tests/qtest/migration: Restore include for postcopy

[Peter Xu]

On Wed, Dec 18, 2024 at 12:12:11PM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote:
> >> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote:
> >> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to
> >> > utils") moved the ufd_version_check() function to another file but
> >> > failed to bring along the <sys/syscall> include, which is necessary to
> >> > pull in <asm/unistd.h> for __NR_userfaultd.
> >> > 
> >> > Restore the missing include.
> >> 
> >> Ohhhhhhh.. so postcopy tests will always be skipped as of now?  Maybe worth
> >> explicit mention that in the commit message if so, only when you merge.
> >> 
> >> > 
> >> > While here, remove the ifdef __linux__ that's redundant and fix a
> >> > couple of typos.
> >> > 
> >> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils")
> >> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> 
> >> Reviewed-by: Peter Xu <peterx@redhat.com>
> >> 
> >> Maybe we don't need to be as careful on old kernels anymore especially in
> >> tests, because userfaultfd syscall existed for ~10 years. So if we want we
> >> can start requiring __NR_userfaultfd present for __linux__, then no way to
> >> miss such spot next time.
> >
> > Yes, I think that check is obsolete, based on our supported platforms
> > list. It would suffice to just check __linux__.
> 
> This breaks the cross builds. It seems the __NR_userfaultfd was actually
> stopping several archs from reaching ufd_version_check(). Since
> <sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find
> the 'ioctl' symbol:
> 
> https://gitlab.com/farosas/qemu/-/pipelines/1594332399
> 
> Of course I could just include <sys/ioctl.h> unconditionally, but the
> fact that new code is not being built means the assumption that we can
> imply __NR_userfaultfd from __linux__ alone is not correct.

What is the new code referenced here?  I still don't yet understand how it
would fail if we move sys/ioctl.h out.

PS: we don't necessarily need to do it right now to remove that ifdef, but
I'm just curious.

-- 
Peter Xu

Re: [PATCH] tests/qtest/migration: Restore include for postcopy

[Daniel P. Berrangé]

On Wed, Dec 18, 2024 at 12:12:11PM -0300, Fabiano Rosas wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote:
> >> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote:
> >> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to
> >> > utils") moved the ufd_version_check() function to another file but
> >> > failed to bring along the <sys/syscall> include, which is necessary to
> >> > pull in <asm/unistd.h> for __NR_userfaultd.
> >> > 
> >> > Restore the missing include.
> >> 
> >> Ohhhhhhh.. so postcopy tests will always be skipped as of now?  Maybe worth
> >> explicit mention that in the commit message if so, only when you merge.
> >> 
> >> > 
> >> > While here, remove the ifdef __linux__ that's redundant and fix a
> >> > couple of typos.
> >> > 
> >> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils")
> >> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> 
> >> Reviewed-by: Peter Xu <peterx@redhat.com>
> >> 
> >> Maybe we don't need to be as careful on old kernels anymore especially in
> >> tests, because userfaultfd syscall existed for ~10 years. So if we want we
> >> can start requiring __NR_userfaultfd present for __linux__, then no way to
> >> miss such spot next time.
> >
> > Yes, I think that check is obsolete, based on our supported platforms
> > list. It would suffice to just check __linux__.
> 
> This breaks the cross builds. It seems the __NR_userfaultfd was actually
> stopping several archs from reaching ufd_version_check(). Since
> <sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find
> the 'ioctl' symbol:
> 
> https://gitlab.com/farosas/qemu/-/pipelines/1594332399
> 
> Of course I could just include <sys/ioctl.h> unconditionally, but the
> fact that new code is not being built means the assumption that we can
> imply __NR_userfaultfd from __linux__ alone is not correct.

I think removing __NR_userfaultfd is still correct. The problem is that
the ufd_version_check code is wrapped in a conditional that is not the
same as the conditional that pulls in ioctl.h. That pre-existing bug
should be fixed regardless, and once that's done, removing __NR_userfaultfd
wouldn't be an issue.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] tests/qtest/migration: Restore include for postcopy

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Dec 18, 2024 at 12:12:11PM -0300, Fabiano Rosas wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote:
>> >> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote:
>> >> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to
>> >> > utils") moved the ufd_version_check() function to another file but
>> >> > failed to bring along the <sys/syscall> include, which is necessary to
>> >> > pull in <asm/unistd.h> for __NR_userfaultd.
>> >> > 
>> >> > Restore the missing include.
>> >> 
>> >> Ohhhhhhh.. so postcopy tests will always be skipped as of now?  Maybe worth
>> >> explicit mention that in the commit message if so, only when you merge.
>> >> 
>> >> > 
>> >> > While here, remove the ifdef __linux__ that's redundant and fix a
>> >> > couple of typos.
>> >> > 
>> >> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils")
>> >> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >> 
>> >> Reviewed-by: Peter Xu <peterx@redhat.com>
>> >> 
>> >> Maybe we don't need to be as careful on old kernels anymore especially in
>> >> tests, because userfaultfd syscall existed for ~10 years. So if we want we
>> >> can start requiring __NR_userfaultfd present for __linux__, then no way to
>> >> miss such spot next time.
>> >
>> > Yes, I think that check is obsolete, based on our supported platforms
>> > list. It would suffice to just check __linux__.
>> 
>> This breaks the cross builds. It seems the __NR_userfaultfd was actually
>> stopping several archs from reaching ufd_version_check(). Since
>> <sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find
>> the 'ioctl' symbol:
>> 
>> https://gitlab.com/farosas/qemu/-/pipelines/1594332399
>> 
>> Of course I could just include <sys/ioctl.h> unconditionally, but the
>> fact that new code is not being built means the assumption that we can
>> imply __NR_userfaultfd from __linux__ alone is not correct.
>
> I think removing __NR_userfaultfd is still correct. The problem is that
> the ufd_version_check code is wrapped in a conditional that is not the
> same as the conditional that pulls in ioctl.h. That pre-existing bug
> should be fixed regardless, and once that's done, removing __NR_userfaultfd
> wouldn't be an issue.

Ah, the <sys/ioctl.h> include used to be under the userfaultfd ifdefs. I
got confused because kvm_dirty_ring_supported() also needs the ioctl and
it was introduced without moving the ioctl to a common ifdef.

>
> With regards,
> Daniel