On 19/01/2026 14:32, David Hildenbrand (Red Hat) wrote: > On 1/19/26 15:26, Ryan Roberts wrote: >> On 19/01/2026 11:16, David Hildenbrand (Red Hat) wrote: >>> On 1/7/26 17:48, Kevin Brodsky wrote: >>>> pfnmap currently checks the target file in FIXTURE_SETUP(pfnmap), >>>> meaning once for every test, and skips the test if any check fails. >>>> >>>> The target file is the same for every test so this is a little >>>> overkill. More importantly, this approach means that the whole suite >>>> will report PASS even if all the tests are skipped because kernel >>>> configuration (e.g. CONFIG_STRICT_DEVMEM=y) prevented /dev/mem from >>>> being mapped, for instance. >>>> >>>> Let's ensure that KSFT_SKIP is returned as exit code if any check >>>> fails by performing the checks in pfnmap_init(), run once. That >>>> function also takes care of finding the offset of the pages to be >>>> mapped and saves it in a global. The file is still mapped/unmapped >>>> for every test, as some of them modify the mapping. >>>> >>>> Signed-off-by: Kevin Brodsky <[email protected]> >>>> --- >>>> tools/testing/selftests/mm/pfnmap.c | 81 ++++++++++++++++++++--------- >>>> 1 file changed, 55 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/mm/pfnmap.c >>>> b/tools/testing/selftests/mm/ >>>> pfnmap.c >>>> index 35b0e3ed54cd..e41d5464130b 100644 >>>> --- a/tools/testing/selftests/mm/pfnmap.c >>>> +++ b/tools/testing/selftests/mm/pfnmap.c >>>> @@ -25,8 +25,11 @@ >>>> #include "kselftest_harness.h" >>>> #include "vm_util.h" >>>> +#define DEV_MEM_NPAGES 2 >>>> + >>>> static sigjmp_buf sigjmp_buf_env; >>>> static char *file = "/dev/mem"; >>>> +static off_t file_offset; >>>> static void signal_handler(int sig) >>>> { >>>> @@ -88,7 +91,7 @@ static int find_ram_target(off_t *offset, >>>> break; >>>> /* We need two pages. */ >>>> - if (end > start + 2 * pagesize) { >>>> + if (end > start + DEV_MEM_NPAGES * pagesize) { >>>> fclose(file); >>>> *offset = start; >>>> return 0; >>>> @@ -97,9 +100,49 @@ static int find_ram_target(off_t *offset, >>>> return -ENOENT; >>>> } >>>> +static void pfnmap_init(void) >>>> +{ >>>> + size_t pagesize = getpagesize(); >>>> + size_t size = DEV_MEM_NPAGES * pagesize; >>>> + int fd; >>>> + void *addr; >>>> + >>>> + if (strncmp(file, "/dev/mem", strlen("/dev/mem")) == 0) { >>>> + int err = find_ram_target(&file_offset, pagesize); >>>> + >>>> + if (err) >>>> + ksft_exit_skip("Cannot find ram target in '/proc/iomem': >>>> %s\n", >>>> + strerror(-err)); >>>> + } else { >>>> + file_offset = 0; >>>> + } >>>> + >>>> + /* >>>> + * Make sure we can open and map the file, and perform some basic >>>> + * checks; skip the whole suite if anything goes wrong. >>>> + * A fresh mapping is then created for every test case by >>>> + * FIXTURE_SETUP(pfnmap). >>>> + */ >>>> + fd = open(file, O_RDONLY); >>>> + if (fd < 0) >>>> + ksft_exit_skip("Cannot open '%s': %s\n", file, strerror(errno)); >>>> + >>>> + addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, file_offset); >>>> + if (addr == MAP_FAILED) >>>> + ksft_exit_skip("Cannot mmap '%s': %s\n", file, strerror(errno)); >>>> + >>>> + if (!check_vmflag_pfnmap(addr)) >>>> + ksft_exit_skip("Invalid file: '%s'. Not pfnmap'ed\n", file); >>>> + >>>> + if (test_read_access(addr, size)) >>>> + ksft_exit_skip("Cannot read-access mmap'ed '%s'\n", file); >>>> + >>>> + munmap(addr, size); >>> >>> Why not keep the fd open then and supply that to all tests without the need >>> for >>> them to open/close? >>> >>> Then, also the file cannot change etc. >> >> I had a private conversation with Kevin about this before he posted; my very >> minor, theorectical concern about that was that it's possible to pass in a >> custom file to be pfnmapped and I wondered if such a file could map a device >> region that has read side effects? In that case I think you'd want to open it >> fresh for each test to ensure consistent starting state? > > Are we aware of devices where we would actually require a new open, and not > just > a new mmap()?
Nope; as I said all hypothetical. I was just being cautious. > > The reason we added support for other files was "other pfnmap'ed memory like > NVIDIA's EGM". I'd assume that people rather should not pass in something that > has any side-effects. > >> >> But if you think that concern is unfounded, certainly just opening it once >> and >> reusing will simplify. > > I would just keep it simple here, yes. If this ever becomes a real problem, my > intuition would tell me that probably the caller is doing something > unsupported > that we just cannot easily identify+reject. Yeah fair enough. Thanks, Ryan
