| Message ID | 20250523233018.1702151-8-dmatlack@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,01/33] selftests: Create tools/testing/selftests/vfio | expand |
On Mon, May 26, 2025 at 10:15 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Fri, May 23, 2025 at 11:29:52PM +0000, David Matlack wrote: > > From: Josh Hilke <jrhilke@google.com> > > > > Add a command line arg to vfio_dma_mapping_test to choose the size of > > page which is mapped in VFIO. > > This doesn't seem right.. > > Tests should run automously, test all possible sizes using a fixture. This test uses a fixture already. I assume you're referring to FIXTURE_VARIANT()? I'll explore doing this. For a single dimension this looks possible. But for multiple dimensions (e.g. cross product of iommu_mode and backing_src) I don't see a clear way to do it. But that's just after a cursory look. For context, the pattern of passing in test configuration via flags rather than automatically testing all combinations is something inherited from KVM selftests. That's the common pattern there. There's some work happening there to encode configurations at a higher level using testcase files and a runner [1]. There are also some challenges with making VFIO selftests (or any selftest that uses tools/testing/selftests/vfio/lib) truly autonomous: - The library needs to know which device to use. In this RFC that works by the user passing in BDF as a positional argument to each test. - For tests that use HugeTLB (like this one), the test requires the user to have already allocated HugeTLB memory for it to use. One idea would be to have tests test all possible iommu_modes by default (with the ability to override and pick a specific mode) and then let everything else be driven by flags. [1] https://lore.kernel.org/kvm/20250222005943.3348627-1-vipinsh@google.com/
On Fri, May 30, 2025 at 09:50:22AM -0700, David Matlack wrote: > On Mon, May 26, 2025 at 10:15 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > On Fri, May 23, 2025 at 11:29:52PM +0000, David Matlack wrote: > > > From: Josh Hilke <jrhilke@google.com> > > > > > > Add a command line arg to vfio_dma_mapping_test to choose the size of > > > page which is mapped in VFIO. > > > > This doesn't seem right.. > > > > Tests should run automously, test all possible sizes using a fixture. > > This test uses a fixture already. I assume you're referring to > FIXTURE_VARIANT()? Yes > I'll explore doing this. For a single dimension this looks possible. > But for multiple dimensions (e.g. cross product of iommu_mode and > backing_src) I don't see a clear way to do it. But that's just after a > cursory look. Explicitly list all the combinations with macros? Enhance the userspace tests allow code to generate the variants? Kernel tests can do this: /** * KUNIT_CASE_PARAM - A helper for creation a parameterized &struct kunit_case * * @test_name: a reference to a test case function. * @gen_params: a reference to a parameter generator function. * * The generator function:: * * const void* gen_params(const void *prev, char *desc) * * is used to lazily generate a series of arbitrarily typed values that fit into * a void*. The argument @prev is the previously returned value, which should be * used to derive the next value; @prev is set to NULL on the initial generator * call. When no more values are available, the generator must return NULL. * Optionally write a string into @desc (size of KUNIT_PARAM_DESC_SIZE) * describing the parameter. */ #define KUNIT_CASE_PARAM(test_name, gen_params) \ { .run_case = test_name, .name = #test_name, \ .generate_params = gen_params, .module_name = KBUILD_MODNAME} > For context, the pattern of passing in test configuration via flags > rather than automatically testing all combinations is something > inherited from KVM selftests. That's the common pattern there. There's > some work happening there to encode configurations at a higher level > using testcase files and a runner [1]. IMHO it is not good, it should be done with fixtures and variants slicing the test by matching the test fixture/name/etc. It doesn't really make sense to have a C test runner that you have to invoke from bash just because the C stuff is not very good.. > - The library needs to know which device to use. In this RFC that > works by the user passing in BDF as a positional argument to each > test. I'd probably say it should scan the available vfio devices and pick the first that it understands how to use. Command line would be an option. > - For tests that use HugeTLB (like this one), the test requires the > user to have already allocated HugeTLB memory for it to use. Other tests do this already, like the iommufd test assumes it can get hugetlb mmaps. Skip the tests on allocation failure is the best I think you can do. It would be nice to have test metadata so a runner frame work could know to provide this stuff in the environment. Jason
On 2025-05-30 02:25 PM, Jason Gunthorpe wrote: > On Fri, May 30, 2025 at 09:50:22AM -0700, David Matlack wrote: > > I'll explore doing this. For a single dimension this looks possible. > > But for multiple dimensions (e.g. cross product of iommu_mode and > > backing_src) I don't see a clear way to do it. But that's just after a > > cursory look. > > Explicitly list all the combinations with macros? > > Enhance the userspace tests allow code to generate the > variants? Kernel tests can do this: I got a chance to play around with generating fixture variants today and eneded up with this, which I think is pretty clean. tools/testing/selftests/vfio/lib/include/vfio_util.h: #define ALL_IOMMU_MODES_VARIANT_ADD(...) \ __IOMMU_MODE_VARIANT_ADD(vfio_type1_iommu, ##__VA_ARGS__); \ __IOMMU_MODE_VARIANT_ADD(vfio_type1v2_iommu, ##__VA_ARGS__); \ __IOMMU_MODE_VARIANT_ADD(iommufd_compat_type1, ##__VA_ARGS__); \ __IOMMU_MODE_VARIANT_ADD(iommufd_compat_type1v2, ##__VA_ARGS__); \ __IOMMU_MODE_VARIANT_ADD(iommufd, ##__VA_ARGS__) tools/testing/selftests/vfio/vfio_dma_mapping_test.c: #define __IOMMU_MODE_VARIANT_ADD(_iommu_mode, _name, _size, _mmap_flags) \ FIXTURE_VARIANT_ADD(vfio_dma_mapping_test, _iommu_mode ## _name) \ { \ .iommu_mode = #_iommu_mode, \ .size = (_size), \ .mmap_flags = MAP_ANONYMOUS | MAP_PRIVATE | (_mmap_flags), \ } ALL_IOMMU_MODES_VARIANT_ADD(anonymous, 0, 0); ALL_IOMMU_MODES_VARIANT_ADD(anonymous_hugetlb_2mb, SZ_2M, MAP_HUGETLB | MAP_HUGE_2MB); ALL_IOMMU_MODES_VARIANT_ADD(anonymous_hugetlb_1gb, SZ_1G, MAP_HUGETLB | MAP_HUGE_1GB); #undef __IOMMU_MODE_VARIANT_ADD Let me know if you think this looks reasonable.
diff --git a/tools/testing/selftests/vfio/vfio_dma_mapping_test.c b/tools/testing/selftests/vfio/vfio_dma_mapping_test.c index e020f3eb6910..4b838a06a5fe 100644 --- a/tools/testing/selftests/vfio/vfio_dma_mapping_test.c +++ b/tools/testing/selftests/vfio/vfio_dma_mapping_test.c @@ -1,8 +1,11 @@ // SPDX-License-Identifier: GPL-2.0-only -#include <fcntl.h> - +#include <stdio.h> +#include <string.h> #include <sys/mman.h> +#include <unistd.h> +#include <linux/limits.h> +#include <linux/mman.h> #include <linux/sizes.h> #include <linux/vfio.h> @@ -10,7 +13,12 @@ #include "../kselftest_harness.h" -const char *device_bdf; +static struct { + u64 size; + u64 iova; + int mmap_flags; + const char *bdf; +} test_config; FIXTURE(vfio_dma_mapping_test) { @@ -19,7 +27,7 @@ FIXTURE(vfio_dma_mapping_test) FIXTURE_SETUP(vfio_dma_mapping_test) { - self->device = vfio_pci_device_init(device_bdf, VFIO_TYPE1_IOMMU); + self->device = vfio_pci_device_init(test_config.bdf, VFIO_TYPE1_IOMMU); } FIXTURE_TEARDOWN(vfio_dma_mapping_test) @@ -29,29 +37,79 @@ FIXTURE_TEARDOWN(vfio_dma_mapping_test) TEST_F(vfio_dma_mapping_test, dma_map_unmap) { - const u64 size = SZ_2M; - const u64 iova = SZ_4G; void *mem; - mem = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); + mem = mmap(NULL, test_config.size, PROT_READ | PROT_WRITE, + test_config.mmap_flags, -1, 0); ASSERT_NE(mem, MAP_FAILED); - vfio_pci_dma_map(self->device, iova, size, mem); - printf("Mapped HVA %p (size 0x%lx) at IOVA 0x%lx\n", mem, size, iova); - vfio_pci_dma_unmap(self->device, iova, size); + vfio_pci_dma_map(self->device, test_config.iova, test_config.size, mem); + printf("Mapped HVA %p (size 0x%lx) at IOVA 0x%lx\n", mem, + test_config.size, test_config.iova); + + vfio_pci_dma_unmap(self->device, test_config.iova, test_config.size); + + ASSERT_TRUE(!munmap(mem, test_config.size)); +} - ASSERT_TRUE(!munmap(mem, size)); +static void help(const char *name) +{ + printf("Usage: %s [-b backing_src] segment:bus:device.function\n" + " -b: Which backing memory to use (default: anonymous)\n" + "\n" + " anonymous\n" + " anonymous_hugetlb_2mb\n" + " anonymous_hugetlb_1gb\n", + name); + exit(1); +} + +static int set_backing_src(const char *backing_src) +{ + if (!backing_src) + return 1; + + test_config.mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS; + + if (!strcmp(backing_src, "anonymous")) { + test_config.size = SZ_4K; + return 0; + } else if (!strcmp(backing_src, "anonymous_hugetlb_2mb")) { + test_config.size = SZ_2M; + test_config.mmap_flags |= MAP_HUGETLB | MAP_HUGE_2MB; + return 0; + } else if (!strcmp(backing_src, "anonymous_hugetlb_1gb")) { + test_config.size = SZ_1G; + test_config.mmap_flags |= MAP_HUGETLB | MAP_HUGE_1GB; + return 0; + } + + fprintf(stderr, "Unrecognized value for -b: %s\n", backing_src); + return 1; } int main(int argc, char *argv[]) { - if (argc != 2) { - fprintf(stderr, "usage: %s segment:bus:device.function\n", - argv[0]); - return KSFT_FAIL; + const char *backing_src = "anonymous"; + int c; + + while ((c = getopt(argc, argv, "b:")) != -1) { + switch (c) { + case 'b': + backing_src = optarg; + break; + default: + help(argv[0]); + } } - device_bdf = argv[1]; + if (set_backing_src(backing_src)) + help(argv[0]); + + if (optind >= argc) + help(argv[0]); + + test_config.bdf = argv[optind]; - return test_harness_run(1, argv); + return test_harness_run(0, NULL); }