| Message ID | 20240614023009.221547-1-jhubbard@nvidia.com |
|---|---|
| Headers | show |
| Series | cleanups, fixes, and progress towards avoiding "make headers" | expand |
On 14.06.24 04:30, John Hubbard wrote: > The selftests/mm build isn't exactly "broken", according to the current > documentation, which still claims that one must run "make headers", > before building the kselftests. However, according to the new plan to > get rid of that requirement [1], they are future-broken: attempting to > build selftests/mm *without* first running "make headers" will fail due > to not finding __NR_mseal. > > Therefore, add ./usr/include/asm/unistd_[32|x32|64].h (created via > "make headers") to tools/uapi/, and change the selftests/mm files that > require __NR_mseal to include from the correct location. The way to do > so is to include <linux/unistd.h> instead of just <unistd.h>. > > [1] commit e076eaca5906 ("selftests: break the dependency upon local > header files") > > Fixes: 4926c7a52de7 ("selftest mm/mseal memory sealing") > Cc: Jeff Xu <jeffxu@chromium.org> > Cc: David Hildenbrand <david@redhat.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- If it works, great Acked-by: David Hildenbrand <david@redhat.com>
On 14.06.24 04:30, John Hubbard wrote: > Now that the test macros are factored out into their final location, and > simplified, it's time to rename TEST_END_CHECK to something that > represents its new functionality: REPORT_TEST_PASS. > > Cc: David Hildenbrand <david@redhat.com> > Reviewed-by: Jeff Xu <jeffxu@chromium.org> > Tested-by: Jeff Xu <jeffxu@chromium.org> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- Acked-by: David Hildenbrand <david@redhat.com>
On 14.06.24 20:02, John Hubbard wrote: > On 6/14/24 5:41 AM, David Hildenbrand wrote: >> On 14.06.24 14:28, David Hildenbrand wrote: >>> On 14.06.24 04:30, John Hubbard wrote: >>>> The selftests/mm build isn't exactly "broken", according to the current >>>> documentation, which still claims that one must run "make headers", >>>> before building the kselftests. However, according to the new plan to >>>> get rid of that requirement [1], they are future-broken: attempting to >>>> build selftests/mm *without* first running "make headers" will fail due >>>> to not finding __NR_mseal. >>>> >>>> Therefore, add ./usr/include/asm/unistd_[32|x32|64].h (created via >>>> "make headers") to tools/uapi/, and change the selftests/mm files that >>>> require __NR_mseal to include from the correct location. The way to do >>>> so is to include <linux/unistd.h> instead of just <unistd.h>. >>>> >>>> [1] commit e076eaca5906 ("selftests: break the dependency upon local >>>> header files") >>>> >>>> Fixes: 4926c7a52de7 ("selftest mm/mseal memory sealing") >>>> Cc: Jeff Xu <jeffxu@chromium.org> >>>> Cc: David Hildenbrand <david@redhat.com> >>>> Signed-off-by: John Hubbard <jhubbard@nvidia.com> >>>> --- >>> >>> If it works, great >> >> ... thinking again, are some of these headers arch-specific (IOW, >> generating them per-arch would result in something slightly different)? > > Oh wow, yes they are. I'm guilty of x86-centric thinking (again). > > hmm, this is going to make it really a lot of trouble to do this > approach. But there's no point in turning back now, so I guess I'd > better fire up the cross-compilers and generate for all the arches. > Cross compilers might not be required. At least for me, at simple rm -rf usr/include export ARCH=s390 make headers make headers_install Installed the proper s390 headers, and "usr/include/asm" would contain the s390 specifics. Some scripting would be required to make this much easier to generate+wire up for all archs semi-automatically. >> >> In tools/include/uapi/asm-generic/unistd.h, we already do have >> __NR_mseal ... > > Yes, but it doesn't get used in selftests/mm, with the way headers are > set up right now. Right
On 6/17/24 11:11 AM, David Hildenbrand wrote: > On 14.06.24 20:02, John Hubbard wrote: >> On 6/14/24 5:41 AM, David Hildenbrand wrote: >>> On 14.06.24 14:28, David Hildenbrand wrote: >>>> On 14.06.24 04:30, John Hubbard wrote: ... >>> ... thinking again, are some of these headers arch-specific (IOW, >>> generating them per-arch would result in something slightly different)? >> >> Oh wow, yes they are. I'm guilty of x86-centric thinking (again). >> >> hmm, this is going to make it really a lot of trouble to do this >> approach. But there's no point in turning back now, so I guess I'd >> better fire up the cross-compilers and generate for all the arches. >> > > Cross compilers might not be required. > > At least for me, at simple > > rm -rf usr/include > export ARCH=s390 > make headers > make headers_install > > Installed the proper s390 headers, and "usr/include/asm" would contain the s390 specifics. Yes. > > Some scripting would be required to make this much easier to generate+wire up for all archs semi-automatically. After exploring this, and eventually re-inventing the concept of asm-generic, I now understand What To Do. >>> >>> In tools/include/uapi/asm-generic/unistd.h, we already do have >>> __NR_mseal ... >> >> Yes, but it doesn't get used in selftests/mm, with the way headers are >> set up right now. > > Right > ...it turns out that the right answer is to simply include asm-generic/unistd.h. That is the only way to safely use system call numbers across CPU architectures, and we already have it. So the entire patch simplifies back down to two lines of diff: diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index 41998cf1dcf5..58c888529f42 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -3,7 +3,7 @@ #include <linux/mman.h> #include <sys/mman.h> #include <stdint.h> -#include <unistd.h> +#include <asm-generic/unistd.h> #include <string.h> #include <sys/time.h> #include <sys/resource.h> diff --git a/tools/testing/selftests/mm/seal_elf.c b/tools/testing/selftests/mm/seal_elf.c index f2babec79bb6..27bf2f84231d 100644 --- a/tools/testing/selftests/mm/seal_elf.c +++ b/tools/testing/selftests/mm/seal_elf.c @@ -2,7 +2,7 @@ #define _GNU_SOURCE #include <sys/mman.h> #include <stdint.h> -#include <unistd.h> +#include <asm-generic/unistd.h> #include <string.h> #include <sys/time.h> #include <sys/resource.h> Which is a little embarrassing, after all this churn, but it's not always clear how these headers are meant to be used, at first. Now it's clear. I think. :) I'll post a v3 shortly thanks,
Jeff Xu, I apologize for this churn: I was forced to drop your Reviewed-by and Tested-by tags from 2 of the 3 mseal patches, because the __NR_mseal fix is completely different now. Changes since v1: a) Reworked the mseal fix to use the kernel's in-tree unistd*.h files, instead of hacking in a __NR_mseal definition directly. (Thanks to David Hildenbrand for pointing out that this needed to be done.) b) Fixed the subject line of the kvm and mdwe patch. c) Reordered the patches so as to group the mseal changes together. d) ADDED an additional patch, 6/6, to remove various __NR_xx items and checks from the mm selftests. Cover letter, updated for v2: Eventually, once the build succeeds on a sufficiently old distro, the idea is to delete $(KHDR_INCLUDES) from the selftests/mm build, and then after that, from selftests/lib.mk and all of the other selftest builds. For now, this series merely achieves a clean build of selftests/mm on a not-so-old distro: Ubuntu 23.04. In other words, after this series is applied, it is possible to delete $(KHDR_INCLUDES) from selftests/mm/Makefile and the build will still succeed. 1. Add tools/uapi/asm/unistd_[32|x32|64].h files, which include definitions of __NR_mseal, and include them (indirectly) from the files that use __NR_mseal. The new files are copied from ./usr/include/asm, which is how we have agreed to do this sort of thing, see [1]. 2. Add fs.h, similarly created: it was copied directly from a snapshot of ./usr/include/linux/fs.h after running "make headers". 3. Add a few selected prctl.h values that the ksm and mdwe tests require. 4. Factor out some common code from mseal_test.c and seal_elf.c, into a new mseal_helpers.h file. 5. Remove local __NR_* definitions and checks. [1] commit e076eaca5906 ("selftests: break the dependency upon local header files") John Hubbard (6): selftests/mm: mseal, self_elf: fix missing __NR_mseal selftests/mm: mseal, self_elf: factor out test macros and other duplicated items selftests/mm: mseal, self_elf: rename TEST_END_CHECK to REPORT_TEST_PASS selftests/mm: fix vm_util.c build failures: add snapshot of fs.h selftests/mm: kvm, mdwe fixes to avoid requiring "make headers" selftests/mm: remove local __NR_* definitions tools/include/uapi/asm/unistd_32.h | 458 ++++++++++++++++++ tools/include/uapi/asm/unistd_64.h | 380 +++++++++++++++ tools/include/uapi/asm/unistd_x32.h | 369 ++++++++++++++ tools/include/uapi/linux/fs.h | 392 +++++++++++++++ tools/testing/selftests/mm/hugepage-mremap.c | 2 +- .../selftests/mm/ksm_functional_tests.c | 8 +- tools/testing/selftests/mm/mdwe_test.c | 1 + tools/testing/selftests/mm/memfd_secret.c | 14 +- tools/testing/selftests/mm/mkdirty.c | 8 +- tools/testing/selftests/mm/mlock2.h | 1 + tools/testing/selftests/mm/mrelease_test.c | 2 +- tools/testing/selftests/mm/mseal_helpers.h | 41 ++ tools/testing/selftests/mm/mseal_test.c | 143 ++---- tools/testing/selftests/mm/pagemap_ioctl.c | 2 +- tools/testing/selftests/mm/protection_keys.c | 2 +- tools/testing/selftests/mm/seal_elf.c | 37 +- tools/testing/selftests/mm/uffd-common.c | 4 - tools/testing/selftests/mm/uffd-stress.c | 16 +- tools/testing/selftests/mm/uffd-unit-tests.c | 14 +- tools/testing/selftests/mm/vm_util.h | 15 + 20 files changed, 1717 insertions(+), 192 deletions(-) create mode 100644 tools/include/uapi/asm/unistd_32.h create mode 100644 tools/include/uapi/asm/unistd_64.h create mode 100644 tools/include/uapi/asm/unistd_x32.h create mode 100644 tools/include/uapi/linux/fs.h create mode 100644 tools/testing/selftests/mm/mseal_helpers.h base-commit: 2ccbdf43d5e758f8493a95252073cf9078a5fea5