On 28.05.25 12:34, Ryan Roberts wrote:
Hi David,
On 09/05/2025 16:30, David Hildenbrand wrote:
Let's test some basic functionality using /dev/mem. These tests will implicitly cover some PAT (Page Attribute Handling) handling on x86.
These tests will only run when /dev/mem access to the first two pages in physical address space is possible and allowed; otherwise, the tests are skipped.
We are seeing really horrible RAS errors with this test when run on arm64 tx2 machine. Based solely on reviewing the code, I think the problem is that tx2 doesn't have anything at phys address 0, so test_read_access() is trying to put trasactions out to a bad address on the bus.
tx2 /proc/iomem:
$ sudo cat /proc/iomem 30000000-37ffffff : PCI ECAM 38000000-3fffffff : PCI ECAM 40000000-5fffffff : PCI Bus 0000:00 ...
Whereas my x86 box has some reserved memory:
$ sudo cat /proc/iomem 00000000-00000fff : Reserved 00001000-0003dfff : System RAM ...
A quick fix would be to make this test specific to x86 (the only one I tested on). We should always have the lower two pages IIRC (BIOS stuff etc).
I think perhaps the only safe way to handle this is to parse /proc/iomem for a region of "System RAM" that is at least 2 pages then use that for your read tests. This would also solve the hypothetical issue of reading something that has read size effects.
That sounds also plausible yes. I somehow remembered that mmap() would fail if "there is nothing".
I also spotted a few nits while reading the code...
On current x86-64 with PAT inside a VM, all tests pass:
TAP version 13 1..6 # Starting 6 tests from 1 test cases. # RUN pfnmap.madvise_disallowed ... # OK pfnmap.madvise_disallowed ok 1 pfnmap.madvise_disallowed # RUN pfnmap.munmap_split ... # OK pfnmap.munmap_split ok 2 pfnmap.munmap_split # RUN pfnmap.mremap_fixed ... # OK pfnmap.mremap_fixed ok 3 pfnmap.mremap_fixed # RUN pfnmap.mremap_shrink ... # OK pfnmap.mremap_shrink ok 4 pfnmap.mremap_shrink # RUN pfnmap.mremap_expand ... # OK pfnmap.mremap_expand ok 5 pfnmap.mremap_expand # RUN pfnmap.fork ... # OK pfnmap.fork ok 6 pfnmap.fork # PASSED: 6 / 6 tests passed. # Totals: pass:6 fail:0 xfail:0 xpass:0 skip:0 error:0
However, we are able to trigger:
[ 27.888251] x86/PAT: pfnmap:1790 freeing invalid memtype [mem 0x00000000-0x00000fff]
There are probably more things worth testing in the future, such
as>> MAP_PRIVATE handling. But this set of tests is sufficient to cover most of
the things we will rework regarding PAT handling.
Cc: Andrew Morton akpm@linux-foundation.org Cc: Shuah Khan shuah@kernel.org Cc: Lorenzo Stoakes lorenzo.stoakes@oracle.com Cc: Ingo Molnar mingo@redhat.com Cc: Peter Xu peterx@redhat.com Cc: Dev Jain dev.jain@arm.com Signed-off-by: David Hildenbrand david@redhat.com
Hopefully I didn't miss any review feedback.
v1 -> v2:
- Rewrite using kselftest_harness, which simplifies a lot of things
- Add to .gitignore and run_vmtests.sh
- Register signal handler on demand
- Use volatile trick to force a read (not factoring out FORCE_READ just yet)
- Drop mprotect() test case
- Add some more comments why we test certain things
- Use NULL for mmap() first parameter instead of 0
- Smaller fixes
tools/testing/selftests/mm/.gitignore | 1 + tools/testing/selftests/mm/Makefile | 1 + tools/testing/selftests/mm/pfnmap.c | 196 ++++++++++++++++++++++ tools/testing/selftests/mm/run_vmtests.sh | 4 + 4 files changed, 202 insertions(+) create mode 100644 tools/testing/selftests/mm/pfnmap.c
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore index 91db34941a143..824266982aa36 100644 --- a/tools/testing/selftests/mm/.gitignore +++ b/tools/testing/selftests/mm/.gitignore @@ -20,6 +20,7 @@ mremap_test on-fault-limit transhuge-stress pagemap_ioctl +pfnmap *.tmp* protection_keys protection_keys_32 diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index ad4d6043a60f0..ae6f994d3add7 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -84,6 +84,7 @@ TEST_GEN_FILES += mremap_test TEST_GEN_FILES += mseal_test TEST_GEN_FILES += on-fault-limit TEST_GEN_FILES += pagemap_ioctl +TEST_GEN_FILES += pfnmap TEST_GEN_FILES += thuge-gen TEST_GEN_FILES += transhuge-stress TEST_GEN_FILES += uffd-stress diff --git a/tools/testing/selftests/mm/pfnmap.c b/tools/testing/selftests/mm/pfnmap.c new file mode 100644 index 0000000000000..8a9d19b6020c7 --- /dev/null +++ b/tools/testing/selftests/mm/pfnmap.c @@ -0,0 +1,196 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Basic VM_PFNMAP tests relying on mmap() of '/dev/mem'
- Copyright 2025, Red Hat, Inc.
- Author(s): David Hildenbrand david@redhat.com
- */
+#define _GNU_SOURCE +#include <stdlib.h> +#include <string.h> +#include <stdint.h> +#include <unistd.h> +#include <errno.h> +#include <fcntl.h> +#include <signal.h> +#include <setjmp.h> +#include <linux/mman.h> +#include <sys/mman.h> +#include <sys/wait.h>
+#include "../kselftest_harness.h" +#include "vm_util.h"
+static sigjmp_buf sigjmp_buf_env;
+static void signal_handler(int sig) +{
- siglongjmp(sigjmp_buf_env, -EFAULT);
+}
+static int test_read_access(char *addr, size_t size, size_t pagesize) +{
- size_t offs;
- int ret;
- if (signal(SIGSEGV, signal_handler) == SIG_ERR)
return -EINVAL;
- ret = sigsetjmp(sigjmp_buf_env, 1);
- if (!ret) {
for (offs = 0; offs < size; offs += pagesize)
/* Force a read that the compiler cannot optimize out. */
*((volatile char *)(addr + offs));
- }
- if (signal(SIGSEGV, signal_handler) == SIG_ERR)
I think you mean: if (signal(SIGSEGV, SIG_DFL) == SIG_ERR)
Otherwise your signal_handler will remain installed, right?
Yeah, copy and past error.
return -EINVAL;
- return ret;
+}
+FIXTURE(pfnmap) +{
- size_t pagesize;
- int dev_mem_fd;
- char *addr1;
- size_t size1;
- char *addr2;
- size_t size2;
+};
+FIXTURE_SETUP(pfnmap) +{
- self->pagesize = getpagesize();
- self->dev_mem_fd = open("/dev/mem", O_RDONLY);
- if (self->dev_mem_fd < 0)
SKIP(return, "Cannot open '/dev/mem'\n");
- /* We'll require the first two pages throughout our tests ... */
- self->size1 = self->pagesize * 2;
- self->addr1 = mmap(NULL, self->size1, PROT_READ, MAP_SHARED,
self->dev_mem_fd, 0);
- if (self->addr1 == MAP_FAILED)
SKIP(return, "Cannot mmap '/dev/mem'\n");
- /* ... and want to be able to read from them. */
- if (test_read_access(self->addr1, self->size1, self->pagesize))
SKIP(return, "Cannot read-access mmap'ed '/dev/mem'\n");
- self->size2 = 0;
- self->addr2 = MAP_FAILED;
Do you need to init all the params in FIXTURE(pfnmap) to their "safe" values before any possible early returns? We don't want FIXTURE_TEARDOWN(pfnmap) getting confused.
If FIXTURE_SETUP fails, we'll exit immediately. See __TEST_F_IMPL().
Note that all tests are executed in a fork'ed child process, so quitting early (or not even having FIXTURE_TEARDOWN in most cases ...) does not really matter.