From: Petr Tesarik <[email protected]>
Add unit tests to help avoid regressions in the SWIOTLB code.
These features are covered by the test suite:
* basic functionality (map, sync)
* alignment based on mapping size
* alignment based on min_align_mask
* explicit alignment with alloc_align_mask
* combination of alignment constraints
Select CONFIG_SWIOTLB rather than depend on it, because it allows to run
the test with UML (default KUnit target).
Signed-off-by: Petr Tesarik <[email protected]>
---
kernel/dma/Kconfig | 13 ++
kernel/dma/Makefile | 1 +
kernel/dma/swiotlb_test.c | 413 ++++++++++++++++++++++++++++++++++++++
3 files changed, 427 insertions(+)
create mode 100644 kernel/dma/swiotlb_test.c
diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
index d62f5957f36b..44c62faa8d89 100644
--- a/kernel/dma/Kconfig
+++ b/kernel/dma/Kconfig
@@ -103,6 +103,19 @@ config SWIOTLB_DYNAMIC
If unsure, say N.
+config SWIOTLB_KUNIT_TEST
+ tristate "Unit tests for software IO TLB" if !KUNIT_ALL_TESTS
+ select SWIOTLB
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ Build unit tests for software IO TLB.
+
+ For more information on KUnit and unit tests in general, please refer
+ to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N.
+
config DMA_BOUNCE_UNALIGNED_KMALLOC
bool
depends on SWIOTLB
diff --git a/kernel/dma/Makefile b/kernel/dma/Makefile
index 21926e46ef4f..bfb130020219 100644
--- a/kernel/dma/Makefile
+++ b/kernel/dma/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_DMA_CMA) += contiguous.o
obj-$(CONFIG_DMA_DECLARE_COHERENT) += coherent.o
obj-$(CONFIG_DMA_API_DEBUG) += debug.o
obj-$(CONFIG_SWIOTLB) += swiotlb.o
+obj-$(CONFIG_SWIOTLB_KUNIT_TEST) += swiotlb_test.o
obj-$(CONFIG_DMA_COHERENT_POOL) += pool.o
obj-$(CONFIG_MMU) += remap.o
obj-$(CONFIG_DMA_MAP_BENCHMARK) += map_benchmark.o
diff --git a/kernel/dma/swiotlb_test.c b/kernel/dma/swiotlb_test.c
new file mode 100644
index 000000000000..46e4d8055ef5
--- /dev/null
+++ b/kernel/dma/swiotlb_test.c
@@ -0,0 +1,413 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Huawei Technologies Duesseldorf GmbH
+ */
+
+#include <kunit/test.h>
+#include <kunit/device.h>
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include <linux/kdev_t.h>
+#include <linux/swiotlb.h>
+
+/* Alignment check repeat count. */
+#define NUM_CHECK_ALIGNED 5
+
+/* Offset of mapped data inside the allocated buffer. */
+#define MAP_OFF 128
+
+#define PASS 0x600d600d
+#define FAIL 0xbad00bad
+
+static struct {
+ unsigned char pad1[MAP_OFF];
+ unsigned long value;
+ unsigned char pad2[PAGE_SIZE];
+} test_data __page_aligned_bss;
+
+/**************************************************************
+ * Various helper functions.
+ */
+
+static int swiotlb_suite_init(struct kunit_suite *suite)
+{
+ if (is_swiotlb_allocated())
+ return 0;
+
+ return swiotlb_init_late(swiotlb_size_or_default(), GFP_KERNEL, NULL);
+}
+
+static int swiotlb_drv_probe(struct device *dev)
+{
+ dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
+ GFP_KERNEL);
+ if (!dev->dma_parms)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int swiotlb_test_init(struct kunit *test)
+{
+ struct device_driver *driver;
+
+ driver = kunit_driver_create(test, "swiotlb_driver");
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, driver);
+ driver->probe = swiotlb_drv_probe;
+
+ test->priv = driver;
+ return 0;
+}
+
+/**
+ * test_device() - get a dummy device for testing
+ * @test: KUnit test instance.
+ *
+ * Allocate a device suitable for SWIOTLB.
+ */
+static struct device *test_device(struct kunit *test)
+{
+ struct device_driver *driver = test->priv;
+ struct device *dev;
+ u64 mask;
+
+ dev = kunit_device_register_with_driver(test, "swiotlb", driver);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+
+ mask = DMA_BIT_MASK(64);
+ KUNIT_ASSERT_EQ(test, dma_coerce_mask_and_coherent(dev, mask), 0);
+
+ return dev;
+}
+
+/**
+ * check_aligned() - check that bounce buffers are aligned
+ * @test: KUnit test instance.
+ * @dev: Device.
+ * @buf: Pointer to the original buffer.
+ * @size: Size of the original buffer.
+ * @align: Allocation alignment (in bytes).
+ * @check_bits:
+ * Number of low bits checked in the swiotlb address.
+ * @preserve_bits:
+ * Number of low bits preserved from the original address.
+ *
+ * Mapping is repeated a few times, and a small buffer is allocated after
+ * each attempt. This should cover the case when the first free slot merely
+ * happens to be suitably aligned.
+ */
+static void check_aligned(struct kunit *test, struct device *dev,
+ void *buf, size_t size, unsigned long align,
+ int check_bits, int preserve_bits)
+{
+ dma_addr_t tlb_addr[NUM_CHECK_ALIGNED];
+ dma_addr_t pad_addr[NUM_CHECK_ALIGNED];
+ u64 check_mask, check_val;
+ phys_addr_t phys_addr;
+ char *orig, *tlb;
+ int i;
+
+ orig = (char *)buf;
+ phys_addr = virt_to_phys(buf);
+ check_mask = DMA_BIT_MASK(check_bits);
+ check_val = phys_addr & DMA_BIT_MASK(preserve_bits);
+
+ for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
+ tlb_addr[i] =
+ swiotlb_tbl_map_single(dev, phys_addr, size, size,
+ align - 1, DMA_TO_DEVICE, 0);
+ KUNIT_ASSERT_NE(test, tlb_addr[i], DMA_MAPPING_ERROR);
+ KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr[i]));
+ KUNIT_EXPECT_EQ(test, tlb_addr[i] & check_mask, check_val);
+
+ /* Check sync in both directions. */
+ tlb = phys_to_virt(tlb_addr[i]);
+ KUNIT_EXPECT_EQ(test, *orig, *tlb);
+ *orig ^= 0xff;
+ swiotlb_sync_single_for_device(dev, tlb_addr[i], sizeof(*orig),
+ DMA_TO_DEVICE);
+ KUNIT_EXPECT_EQ(test, *orig, *tlb);
+ *tlb ^= 0xff;
+ swiotlb_sync_single_for_cpu(dev, tlb_addr[i], sizeof(*orig),
+ DMA_FROM_DEVICE);
+ KUNIT_EXPECT_EQ(test, *orig, *tlb);
+
+ pad_addr[i] = swiotlb_map(dev, phys_addr, sizeof(long),
+ DMA_TO_DEVICE, 0);
+ KUNIT_ASSERT_NE(test, pad_addr[i], DMA_MAPPING_ERROR);
+ }
+
+ for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
+ swiotlb_tbl_unmap_single(dev, pad_addr[i], sizeof(long),
+ DMA_FROM_DEVICE, 0);
+ swiotlb_tbl_unmap_single(dev, tlb_addr[i], size,
+ DMA_FROM_DEVICE, 0);
+ }
+}
+
+/**************************************************************
+ * Map a DMA buffer.
+ *
+ * Test that a DMA buffer can be mapped and synced.
+ */
+
+static void swiotlb_test_map(struct kunit *test)
+{
+ struct device *dev = test_device(test);
+ phys_addr_t phys_addr;
+ dma_addr_t tlb_addr;
+ unsigned long *tlb;
+
+ phys_addr = virt_to_phys(&test_data.value);
+ test_data.value = PASS;
+ tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
+ DMA_TO_DEVICE, 0);
+ KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
+ KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
+ tlb = phys_to_virt(tlb_addr);
+
+ /* Bounce buffer is initialized to original buffer. */
+ KUNIT_EXPECT_EQ(test, *tlb, PASS);
+
+ /* Bounce buffer is updated on sync to device. */
+ test_data.value = PASS + 1;
+ swiotlb_sync_single_for_device(dev, tlb_addr, sizeof(unsigned long),
+ DMA_TO_DEVICE);
+ KUNIT_EXPECT_EQ(test, *tlb, PASS + 1);
+
+ /* Original buffer is updated on sync from device. */
+ *tlb = PASS + 2;
+ swiotlb_sync_single_for_cpu(dev, tlb_addr, sizeof(unsigned long),
+ DMA_FROM_DEVICE);
+ KUNIT_EXPECT_EQ(test, test_data.value, PASS + 2);
+
+ /* Original buffer is also updated on unmap. */
+ *tlb = PASS + 3;
+ swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
+ DMA_FROM_DEVICE, 0);
+ KUNIT_EXPECT_EQ(test, test_data.value, PASS + 3);
+}
+
+/**************************************************************
+ * Map DMA buffer as bi-directional.
+ *
+ * Test that buffer is synced with DMA_BIDIRECTIONAL.
+ */
+
+static void swiotlb_test_bidirectional(struct kunit *test)
+{
+ struct device *dev = test_device(test);
+ phys_addr_t phys_addr;
+ dma_addr_t tlb_addr;
+ unsigned long *tlb;
+
+ test_data.value = PASS;
+ phys_addr = virt_to_phys(&test_data.value);
+ tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
+ DMA_BIDIRECTIONAL, 0);
+ KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
+ KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
+ tlb = phys_to_virt(tlb_addr);
+
+ /* Bounce buffer is initialized to original buffer. */
+ KUNIT_EXPECT_EQ(test, *tlb, PASS);
+
+ /* Original buffer is updated on unmap. */
+ *tlb = PASS + 1;
+ swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
+ DMA_BIDIRECTIONAL, 0);
+ KUNIT_EXPECT_EQ(test, test_data.value, PASS + 1);
+}
+
+/**************************************************************
+ * Skip sync on unmap.
+ *
+ * Test that sync does not happen with DMA_ATTR_SKIP_CPU_SYNC.
+ * On swiotlb_map(), this flag skips only sync for non-coherent
+ * DMA; the bounce buffer itself is always synced to the
+ * original buffer.
+ */
+
+static void swiotlb_test_skip_sync(struct kunit *test)
+{
+ struct device *dev = test_device(test);
+ phys_addr_t phys_addr;
+ dma_addr_t tlb_addr;
+ unsigned long *tlb;
+
+ test_data.value = PASS;
+ phys_addr = virt_to_phys(&test_data.value);
+ tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
+ DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+ KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
+ KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
+ tlb = phys_to_virt(tlb_addr);
+
+ /* Bounce buffer is initialized to original buffer anyway. */
+ KUNIT_EXPECT_EQ(test, *tlb, PASS);
+
+ /* Original buffer is not updated on unmap. */
+ *tlb = FAIL;
+ swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
+ DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+ KUNIT_EXPECT_EQ(test, test_data.value, PASS);
+}
+
+/**************************************************************
+ * Historical page alignment.
+ *
+ * Test that mappings of at least PAGE_SIZE get a page-aligned
+ * DMA address.
+ */
+
+static void swiotlb_test_page_align(struct kunit *test)
+{
+ struct device *dev = test_device(test);
+
+ /* Bounce buffer is page-aligned. */
+ check_aligned(test, dev, &test_data, sizeof(test_data), 1,
+ PAGE_SHIFT, 0);
+
+ /* Even if the original buffer is not page-aligned. */
+ check_aligned(test, dev, &test_data.value, PAGE_SIZE, 1,
+ PAGE_SHIFT, 0);
+}
+
+/**************************************************************
+ * Device physical address alignment.
+ *
+ * Test that physical address low bits are preserved.
+ */
+
+static void check_min_align(struct kunit *test, int bits)
+{
+ u64 min_align_mask = DMA_BIT_MASK(bits);
+ struct device *dev = test_device(test);
+ unsigned long vaddr;
+ void *ptr;
+
+ KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev, min_align_mask), 0);
+
+ vaddr = devm_get_free_pages(dev, GFP_KERNEL,
+ bits > PAGE_SHIFT ? bits - PAGE_SHIFT : 0);
+ KUNIT_ASSERT_NE(test, vaddr, 0);
+
+ /* Check low bits */
+ ptr = (void *)vaddr + MAP_OFF;
+ check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
+
+ /* Check high bits */
+ ptr = (void *)vaddr + (1UL << bits) - MAP_OFF - sizeof(long);
+ check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
+
+ kunit_device_unregister(test, dev);
+}
+
+static void swiotlb_test_min_align(struct kunit *test)
+{
+ check_min_align(test, 12);
+ check_min_align(test, PAGE_SHIFT);
+ check_min_align(test, 16);
+}
+
+/**************************************************************
+ * Explicit allocation alignment.
+ *
+ * Test that the bounce buffer is aligned to an explicit value
+ * regardless of allocation size.
+ */
+
+static void check_alloc_align(struct kunit *test, int bits)
+{
+ struct device *dev = test_device(test);
+ void *base, *ptr;
+ size_t size;
+
+ size = 1UL << bits;
+ base = devm_kmalloc(dev, size, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
+
+ /* Check low bits */
+ ptr = base + MAP_OFF;
+ check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
+
+ /* Check high bits */
+ ptr = base + size - MAP_OFF - sizeof(long);
+ check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
+
+ kunit_device_unregister(test, dev);
+}
+
+static void swiotlb_test_alloc_align(struct kunit *test)
+{
+ check_alloc_align(test, 12);
+ check_alloc_align(test, 14);
+}
+
+/**************************************************************
+ * Both allocation and device physical address alignment.
+ *
+ * Test that the bounce buffer is aligned to an explicit value
+ * regardless of allocation size and it also preserves physical
+ * address low bits.
+ */
+
+static void check_both_align(struct kunit *test, int min_align_bits,
+ int alloc_align_bits)
+{
+ u64 min_align_mask = DMA_BIT_MASK(min_align_bits);
+ struct device *dev = test_device(test);
+ void *base, *ptr;
+ size_t size;
+
+ KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev, min_align_mask), 0);
+
+ size = 1UL << max(min_align_bits, alloc_align_bits);
+ base = devm_kmalloc(dev, size, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
+
+ /* Check low bits */
+ ptr = base + MAP_OFF;
+ check_aligned(test, dev, ptr, sizeof(long), size,
+ min_align_bits, min_align_bits);
+
+ /* Check high bits */
+ ptr = base + size - MAP_OFF - sizeof(long);
+ check_aligned(test, dev, ptr, sizeof(long), size,
+ min_align_bits, min_align_bits);
+
+ kunit_device_unregister(test, dev);
+}
+
+static void swiotlb_test_both_align(struct kunit *test)
+{
+ check_both_align(test, 12, 12);
+ check_both_align(test, 12, 16);
+ check_both_align(test, 14, 16);
+}
+
+/**************************************************************
+ * Test suite metadata.
+ */
+
+static struct kunit_case swiotlb_test_cases[] = {
+ KUNIT_CASE(swiotlb_test_map),
+ KUNIT_CASE(swiotlb_test_bidirectional),
+ KUNIT_CASE(swiotlb_test_skip_sync),
+ KUNIT_CASE(swiotlb_test_page_align),
+ KUNIT_CASE(swiotlb_test_min_align),
+ KUNIT_CASE(swiotlb_test_alloc_align),
+ KUNIT_CASE(swiotlb_test_both_align),
+ {}
+};
+
+static struct kunit_suite swiotlb_test_suite = {
+ .name = "swiotlb",
+ .suite_init = swiotlb_suite_init,
+ .init = swiotlb_test_init,
+ .test_cases = swiotlb_test_cases,
+};
+
+kunit_test_suites(&swiotlb_test_suite);
--
2.34.1
On Wed, 13 Mar 2024 10:27:40 +0100
Petr Tesarik <[email protected]> wrote:
> From: Petr Tesarik <[email protected]>
>
> Add unit tests to help avoid regressions in the SWIOTLB code.
>
> These features are covered by the test suite:
>
> * basic functionality (map, sync)
> * alignment based on mapping size
> * alignment based on min_align_mask
> * explicit alignment with alloc_align_mask
> * combination of alignment constraints
>
> Select CONFIG_SWIOTLB rather than depend on it, because it allows to run
> the test with UML (default KUnit target).
Hi all,
I know it's not super-urgent, but I'm just curious: Has anyone had time
to try out this patch? Did it work for you?
Petr T
> Signed-off-by: Petr Tesarik <[email protected]>
> ---
> kernel/dma/Kconfig | 13 ++
> kernel/dma/Makefile | 1 +
> kernel/dma/swiotlb_test.c | 413 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 427 insertions(+)
> create mode 100644 kernel/dma/swiotlb_test.c
>
> diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
> index d62f5957f36b..44c62faa8d89 100644
> --- a/kernel/dma/Kconfig
> +++ b/kernel/dma/Kconfig
> @@ -103,6 +103,19 @@ config SWIOTLB_DYNAMIC
>
> If unsure, say N.
>
> +config SWIOTLB_KUNIT_TEST
> + tristate "Unit tests for software IO TLB" if !KUNIT_ALL_TESTS
> + select SWIOTLB
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + Build unit tests for software IO TLB.
> +
> + For more information on KUnit and unit tests in general, please refer
> + to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N.
> +
> config DMA_BOUNCE_UNALIGNED_KMALLOC
> bool
> depends on SWIOTLB
> diff --git a/kernel/dma/Makefile b/kernel/dma/Makefile
> index 21926e46ef4f..bfb130020219 100644
> --- a/kernel/dma/Makefile
> +++ b/kernel/dma/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_DMA_CMA) += contiguous.o
> obj-$(CONFIG_DMA_DECLARE_COHERENT) += coherent.o
> obj-$(CONFIG_DMA_API_DEBUG) += debug.o
> obj-$(CONFIG_SWIOTLB) += swiotlb.o
> +obj-$(CONFIG_SWIOTLB_KUNIT_TEST) += swiotlb_test.o
> obj-$(CONFIG_DMA_COHERENT_POOL) += pool.o
> obj-$(CONFIG_MMU) += remap.o
> obj-$(CONFIG_DMA_MAP_BENCHMARK) += map_benchmark.o
> diff --git a/kernel/dma/swiotlb_test.c b/kernel/dma/swiotlb_test.c
> new file mode 100644
> index 000000000000..46e4d8055ef5
> --- /dev/null
> +++ b/kernel/dma/swiotlb_test.c
> @@ -0,0 +1,413 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Huawei Technologies Duesseldorf GmbH
> + */
> +
> +#include <kunit/test.h>
> +#include <kunit/device.h>
> +
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/io.h>
> +#include <linux/kdev_t.h>
> +#include <linux/swiotlb.h>
> +
> +/* Alignment check repeat count. */
> +#define NUM_CHECK_ALIGNED 5
> +
> +/* Offset of mapped data inside the allocated buffer. */
> +#define MAP_OFF 128
> +
> +#define PASS 0x600d600d
> +#define FAIL 0xbad00bad
> +
> +static struct {
> + unsigned char pad1[MAP_OFF];
> + unsigned long value;
> + unsigned char pad2[PAGE_SIZE];
> +} test_data __page_aligned_bss;
> +
> +/**************************************************************
> + * Various helper functions.
> + */
> +
> +static int swiotlb_suite_init(struct kunit_suite *suite)
> +{
> + if (is_swiotlb_allocated())
> + return 0;
> +
> + return swiotlb_init_late(swiotlb_size_or_default(), GFP_KERNEL, NULL);
> +}
> +
> +static int swiotlb_drv_probe(struct device *dev)
> +{
> + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> + GFP_KERNEL);
> + if (!dev->dma_parms)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int swiotlb_test_init(struct kunit *test)
> +{
> + struct device_driver *driver;
> +
> + driver = kunit_driver_create(test, "swiotlb_driver");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, driver);
> + driver->probe = swiotlb_drv_probe;
> +
> + test->priv = driver;
> + return 0;
> +}
> +
> +/**
> + * test_device() - get a dummy device for testing
> + * @test: KUnit test instance.
> + *
> + * Allocate a device suitable for SWIOTLB.
> + */
> +static struct device *test_device(struct kunit *test)
> +{
> + struct device_driver *driver = test->priv;
> + struct device *dev;
> + u64 mask;
> +
> + dev = kunit_device_register_with_driver(test, "swiotlb", driver);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
> +
> + mask = DMA_BIT_MASK(64);
> + KUNIT_ASSERT_EQ(test, dma_coerce_mask_and_coherent(dev, mask), 0);
> +
> + return dev;
> +}
> +
> +/**
> + * check_aligned() - check that bounce buffers are aligned
> + * @test: KUnit test instance.
> + * @dev: Device.
> + * @buf: Pointer to the original buffer.
> + * @size: Size of the original buffer.
> + * @align: Allocation alignment (in bytes).
> + * @check_bits:
> + * Number of low bits checked in the swiotlb address.
> + * @preserve_bits:
> + * Number of low bits preserved from the original address.
> + *
> + * Mapping is repeated a few times, and a small buffer is allocated after
> + * each attempt. This should cover the case when the first free slot merely
> + * happens to be suitably aligned.
> + */
> +static void check_aligned(struct kunit *test, struct device *dev,
> + void *buf, size_t size, unsigned long align,
> + int check_bits, int preserve_bits)
> +{
> + dma_addr_t tlb_addr[NUM_CHECK_ALIGNED];
> + dma_addr_t pad_addr[NUM_CHECK_ALIGNED];
> + u64 check_mask, check_val;
> + phys_addr_t phys_addr;
> + char *orig, *tlb;
> + int i;
> +
> + orig = (char *)buf;
> + phys_addr = virt_to_phys(buf);
> + check_mask = DMA_BIT_MASK(check_bits);
> + check_val = phys_addr & DMA_BIT_MASK(preserve_bits);
> +
> + for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
> + tlb_addr[i] =
> + swiotlb_tbl_map_single(dev, phys_addr, size, size,
> + align - 1, DMA_TO_DEVICE, 0);
> + KUNIT_ASSERT_NE(test, tlb_addr[i], DMA_MAPPING_ERROR);
> + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr[i]));
> + KUNIT_EXPECT_EQ(test, tlb_addr[i] & check_mask, check_val);
> +
> + /* Check sync in both directions. */
> + tlb = phys_to_virt(tlb_addr[i]);
> + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> + *orig ^= 0xff;
> + swiotlb_sync_single_for_device(dev, tlb_addr[i], sizeof(*orig),
> + DMA_TO_DEVICE);
> + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> + *tlb ^= 0xff;
> + swiotlb_sync_single_for_cpu(dev, tlb_addr[i], sizeof(*orig),
> + DMA_FROM_DEVICE);
> + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> +
> + pad_addr[i] = swiotlb_map(dev, phys_addr, sizeof(long),
> + DMA_TO_DEVICE, 0);
> + KUNIT_ASSERT_NE(test, pad_addr[i], DMA_MAPPING_ERROR);
> + }
> +
> + for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
> + swiotlb_tbl_unmap_single(dev, pad_addr[i], sizeof(long),
> + DMA_FROM_DEVICE, 0);
> + swiotlb_tbl_unmap_single(dev, tlb_addr[i], size,
> + DMA_FROM_DEVICE, 0);
> + }
> +}
> +
> +/**************************************************************
> + * Map a DMA buffer.
> + *
> + * Test that a DMA buffer can be mapped and synced.
> + */
> +
> +static void swiotlb_test_map(struct kunit *test)
> +{
> + struct device *dev = test_device(test);
> + phys_addr_t phys_addr;
> + dma_addr_t tlb_addr;
> + unsigned long *tlb;
> +
> + phys_addr = virt_to_phys(&test_data.value);
> + test_data.value = PASS;
> + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
> + DMA_TO_DEVICE, 0);
> + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> + tlb = phys_to_virt(tlb_addr);
> +
> + /* Bounce buffer is initialized to original buffer. */
> + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> +
> + /* Bounce buffer is updated on sync to device. */
> + test_data.value = PASS + 1;
> + swiotlb_sync_single_for_device(dev, tlb_addr, sizeof(unsigned long),
> + DMA_TO_DEVICE);
> + KUNIT_EXPECT_EQ(test, *tlb, PASS + 1);
> +
> + /* Original buffer is updated on sync from device. */
> + *tlb = PASS + 2;
> + swiotlb_sync_single_for_cpu(dev, tlb_addr, sizeof(unsigned long),
> + DMA_FROM_DEVICE);
> + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 2);
> +
> + /* Original buffer is also updated on unmap. */
> + *tlb = PASS + 3;
> + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
> + DMA_FROM_DEVICE, 0);
> + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 3);
> +}
> +
> +/**************************************************************
> + * Map DMA buffer as bi-directional.
> + *
> + * Test that buffer is synced with DMA_BIDIRECTIONAL.
> + */
> +
> +static void swiotlb_test_bidirectional(struct kunit *test)
> +{
> + struct device *dev = test_device(test);
> + phys_addr_t phys_addr;
> + dma_addr_t tlb_addr;
> + unsigned long *tlb;
> +
> + test_data.value = PASS;
> + phys_addr = virt_to_phys(&test_data.value);
> + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
> + DMA_BIDIRECTIONAL, 0);
> + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> + tlb = phys_to_virt(tlb_addr);
> +
> + /* Bounce buffer is initialized to original buffer. */
> + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> +
> + /* Original buffer is updated on unmap. */
> + *tlb = PASS + 1;
> + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
> + DMA_BIDIRECTIONAL, 0);
> + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 1);
> +}
> +
> +/**************************************************************
> + * Skip sync on unmap.
> + *
> + * Test that sync does not happen with DMA_ATTR_SKIP_CPU_SYNC.
> + * On swiotlb_map(), this flag skips only sync for non-coherent
> + * DMA; the bounce buffer itself is always synced to the
> + * original buffer.
> + */
> +
> +static void swiotlb_test_skip_sync(struct kunit *test)
> +{
> + struct device *dev = test_device(test);
> + phys_addr_t phys_addr;
> + dma_addr_t tlb_addr;
> + unsigned long *tlb;
> +
> + test_data.value = PASS;
> + phys_addr = virt_to_phys(&test_data.value);
> + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
> + DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> + tlb = phys_to_virt(tlb_addr);
> +
> + /* Bounce buffer is initialized to original buffer anyway. */
> + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> +
> + /* Original buffer is not updated on unmap. */
> + *tlb = FAIL;
> + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
> + DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> + KUNIT_EXPECT_EQ(test, test_data.value, PASS);
> +}
> +
> +/**************************************************************
> + * Historical page alignment.
> + *
> + * Test that mappings of at least PAGE_SIZE get a page-aligned
> + * DMA address.
> + */
> +
> +static void swiotlb_test_page_align(struct kunit *test)
> +{
> + struct device *dev = test_device(test);
> +
> + /* Bounce buffer is page-aligned. */
> + check_aligned(test, dev, &test_data, sizeof(test_data), 1,
> + PAGE_SHIFT, 0);
> +
> + /* Even if the original buffer is not page-aligned. */
> + check_aligned(test, dev, &test_data.value, PAGE_SIZE, 1,
> + PAGE_SHIFT, 0);
> +}
> +
> +/**************************************************************
> + * Device physical address alignment.
> + *
> + * Test that physical address low bits are preserved.
> + */
> +
> +static void check_min_align(struct kunit *test, int bits)
> +{
> + u64 min_align_mask = DMA_BIT_MASK(bits);
> + struct device *dev = test_device(test);
> + unsigned long vaddr;
> + void *ptr;
> +
> + KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev, min_align_mask), 0);
> +
> + vaddr = devm_get_free_pages(dev, GFP_KERNEL,
> + bits > PAGE_SHIFT ? bits - PAGE_SHIFT : 0);
> + KUNIT_ASSERT_NE(test, vaddr, 0);
> +
> + /* Check low bits */
> + ptr = (void *)vaddr + MAP_OFF;
> + check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
> +
> + /* Check high bits */
> + ptr = (void *)vaddr + (1UL << bits) - MAP_OFF - sizeof(long);
> + check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
> +
> + kunit_device_unregister(test, dev);
> +}
> +
> +static void swiotlb_test_min_align(struct kunit *test)
> +{
> + check_min_align(test, 12);
> + check_min_align(test, PAGE_SHIFT);
> + check_min_align(test, 16);
> +}
> +
> +/**************************************************************
> + * Explicit allocation alignment.
> + *
> + * Test that the bounce buffer is aligned to an explicit value
> + * regardless of allocation size.
> + */
> +
> +static void check_alloc_align(struct kunit *test, int bits)
> +{
> + struct device *dev = test_device(test);
> + void *base, *ptr;
> + size_t size;
> +
> + size = 1UL << bits;
> + base = devm_kmalloc(dev, size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
> +
> + /* Check low bits */
> + ptr = base + MAP_OFF;
> + check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
> +
> + /* Check high bits */
> + ptr = base + size - MAP_OFF - sizeof(long);
> + check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
> +
> + kunit_device_unregister(test, dev);
> +}
> +
> +static void swiotlb_test_alloc_align(struct kunit *test)
> +{
> + check_alloc_align(test, 12);
> + check_alloc_align(test, 14);
> +}
> +
> +/**************************************************************
> + * Both allocation and device physical address alignment.
> + *
> + * Test that the bounce buffer is aligned to an explicit value
> + * regardless of allocation size and it also preserves physical
> + * address low bits.
> + */
> +
> +static void check_both_align(struct kunit *test, int min_align_bits,
> + int alloc_align_bits)
> +{
> + u64 min_align_mask = DMA_BIT_MASK(min_align_bits);
> + struct device *dev = test_device(test);
> + void *base, *ptr;
> + size_t size;
> +
> + KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev, min_align_mask), 0);
> +
> + size = 1UL << max(min_align_bits, alloc_align_bits);
> + base = devm_kmalloc(dev, size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
> +
> + /* Check low bits */
> + ptr = base + MAP_OFF;
> + check_aligned(test, dev, ptr, sizeof(long), size,
> + min_align_bits, min_align_bits);
> +
> + /* Check high bits */
> + ptr = base + size - MAP_OFF - sizeof(long);
> + check_aligned(test, dev, ptr, sizeof(long), size,
> + min_align_bits, min_align_bits);
> +
> + kunit_device_unregister(test, dev);
> +}
> +
> +static void swiotlb_test_both_align(struct kunit *test)
> +{
> + check_both_align(test, 12, 12);
> + check_both_align(test, 12, 16);
> + check_both_align(test, 14, 16);
> +}
> +
> +/**************************************************************
> + * Test suite metadata.
> + */
> +
> +static struct kunit_case swiotlb_test_cases[] = {
> + KUNIT_CASE(swiotlb_test_map),
> + KUNIT_CASE(swiotlb_test_bidirectional),
> + KUNIT_CASE(swiotlb_test_skip_sync),
> + KUNIT_CASE(swiotlb_test_page_align),
> + KUNIT_CASE(swiotlb_test_min_align),
> + KUNIT_CASE(swiotlb_test_alloc_align),
> + KUNIT_CASE(swiotlb_test_both_align),
> + {}
> +};
> +
> +static struct kunit_suite swiotlb_test_suite = {
> + .name = "swiotlb",
> + .suite_init = swiotlb_suite_init,
> + .init = swiotlb_test_init,
> + .test_cases = swiotlb_test_cases,
> +};
> +
> +kunit_test_suites(&swiotlb_test_suite);
From: Petr Tesa??k <[email protected]> Sent: Monday, March 25, 2024 7:07 AM
>
> On Wed, 13 Mar 2024 10:27:40 +0100
> Petr Tesarik <[email protected]> wrote:
>
> > From: Petr Tesarik <[email protected]>
> >
> > Add unit tests to help avoid regressions in the SWIOTLB code.
> >
> > These features are covered by the test suite:
> >
> > * basic functionality (map, sync)
> > * alignment based on mapping size
> > * alignment based on min_align_mask
> > * explicit alignment with alloc_align_mask
> > * combination of alignment constraints
> >
> > Select CONFIG_SWIOTLB rather than depend on it, because it allows to run
> > the test with UML (default KUnit target).
>
> Hi all,
>
> I know it's not super-urgent, but I'm just curious: Has anyone had time
> to try out this patch? Did it work for you?
>
I've read through the patch code, but I haven't run it. I'll need to
do some reading on Kunit as it's not something I've used before.
I'm not sure when/if I will get to it.
Michael
On 2024-03-13 at 14:57:40, Petr Tesarik ([email protected]) wrote:
> From: Petr Tesarik <[email protected]>
Hi,
>
> Add unit tests to help avoid regressions in the SWIOTLB code.
>
> These features are covered by the test suite:
>
> * basic functionality (map, sync)
> * alignment based on mapping size
> * alignment based on min_align_mask
> * explicit alignment with alloc_align_mask
> * combination of alignment constraints
>
> Select CONFIG_SWIOTLB rather than depend on it, because it allows to run
> the test with UML (default KUnit target).
>
> Signed-off-by: Petr Tesarik <[email protected]>
> ---
> kernel/dma/Kconfig | 13 ++
> kernel/dma/Makefile | 1 +
> kernel/dma/swiotlb_test.c | 413 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 427 insertions(+)
> create mode 100644 kernel/dma/swiotlb_test.c
>
> diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
> index d62f5957f36b..44c62faa8d89 100644
> --- a/kernel/dma/Kconfig
> +++ b/kernel/dma/Kconfig
> @@ -103,6 +103,19 @@ config SWIOTLB_DYNAMIC
>
> If unsure, say N.
>
> +config SWIOTLB_KUNIT_TEST
> + tristate "Unit tests for software IO TLB" if !KUNIT_ALL_TESTS
> + select SWIOTLB
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + Build unit tests for software IO TLB.
> +
> + For more information on KUnit and unit tests in general, please refer
> + to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N.
> +
> config DMA_BOUNCE_UNALIGNED_KMALLOC
> bool
> depends on SWIOTLB
> diff --git a/kernel/dma/Makefile b/kernel/dma/Makefile
> index 21926e46ef4f..bfb130020219 100644
> --- a/kernel/dma/Makefile
> +++ b/kernel/dma/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_DMA_CMA) += contiguous.o
> obj-$(CONFIG_DMA_DECLARE_COHERENT) += coherent.o
> obj-$(CONFIG_DMA_API_DEBUG) += debug.o
> obj-$(CONFIG_SWIOTLB) += swiotlb.o
> +obj-$(CONFIG_SWIOTLB_KUNIT_TEST) += swiotlb_test.o
> obj-$(CONFIG_DMA_COHERENT_POOL) += pool.o
> obj-$(CONFIG_MMU) += remap.o
> obj-$(CONFIG_DMA_MAP_BENCHMARK) += map_benchmark.o
> diff --git a/kernel/dma/swiotlb_test.c b/kernel/dma/swiotlb_test.c
> new file mode 100644
> index 000000000000..46e4d8055ef5
> --- /dev/null
> +++ b/kernel/dma/swiotlb_test.c
> @@ -0,0 +1,413 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Huawei Technologies Duesseldorf GmbH
> + */
> +
> +#include <kunit/test.h>
> +#include <kunit/device.h>
> +
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/io.h>
> +#include <linux/kdev_t.h>
> +#include <linux/swiotlb.h>
> +
> +/* Alignment check repeat count. */
> +#define NUM_CHECK_ALIGNED 5
> +
> +/* Offset of mapped data inside the allocated buffer. */
> +#define MAP_OFF 128
> +
> +#define PASS 0x600d600d
> +#define FAIL 0xbad00bad
> +
> +static struct {
> + unsigned char pad1[MAP_OFF];
> + unsigned long value;
> + unsigned char pad2[PAGE_SIZE];
> +} test_data __page_aligned_bss;
> +
> +/**************************************************************
> + * Various helper functions.
> + */
> +
> +static int swiotlb_suite_init(struct kunit_suite *suite)
> +{
> + if (is_swiotlb_allocated())
> + return 0;
> +
> + return swiotlb_init_late(swiotlb_size_or_default(), GFP_KERNEL, NULL);
> +}
> +
> +static int swiotlb_drv_probe(struct device *dev)
> +{
> + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> + GFP_KERNEL);
> + if (!dev->dma_parms)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int swiotlb_test_init(struct kunit *test)
> +{
> + struct device_driver *driver;
> +
> + driver = kunit_driver_create(test, "swiotlb_driver");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, driver);
> + driver->probe = swiotlb_drv_probe;
> +
> + test->priv = driver;
> + return 0;
> +}
> +
> +/**
> + * test_device() - get a dummy device for testing
> + * @test: KUnit test instance.
> + *
> + * Allocate a device suitable for SWIOTLB.
> + */
> +static struct device *test_device(struct kunit *test)
> +{
> + struct device_driver *driver = test->priv;
> + struct device *dev;
> + u64 mask;
> +
> + dev = kunit_device_register_with_driver(test, "swiotlb", driver);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
> +
> + mask = DMA_BIT_MASK(64);
> + KUNIT_ASSERT_EQ(test, dma_coerce_mask_and_coherent(dev, mask), 0);
> +
> + return dev;
> +}
> +
> +/**
> + * check_aligned() - check that bounce buffers are aligned
> + * @test: KUnit test instance.
> + * @dev: Device.
> + * @buf: Pointer to the original buffer.
> + * @size: Size of the original buffer.
> + * @align: Allocation alignment (in bytes).
> + * @check_bits:
> + * Number of low bits checked in the swiotlb address.
> + * @preserve_bits:
> + * Number of low bits preserved from the original address.
> + *
> + * Mapping is repeated a few times, and a small buffer is allocated after
> + * each attempt. This should cover the case when the first free slot merely
> + * happens to be suitably aligned.
> + */
> +static void check_aligned(struct kunit *test, struct device *dev,
> + void *buf, size_t size, unsigned long align,
> + int check_bits, int preserve_bits)
> +{
> + dma_addr_t tlb_addr[NUM_CHECK_ALIGNED];
> + dma_addr_t pad_addr[NUM_CHECK_ALIGNED];
> + u64 check_mask, check_val;
> + phys_addr_t phys_addr;
> + char *orig, *tlb;
> + int i;
> +
> + orig = (char *)buf;
> + phys_addr = virt_to_phys(buf);
> + check_mask = DMA_BIT_MASK(check_bits);
> + check_val = phys_addr & DMA_BIT_MASK(preserve_bits);
> +
> + for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
> + tlb_addr[i] =
> + swiotlb_tbl_map_single(dev, phys_addr, size, size,
> + align - 1, DMA_TO_DEVICE, 0);
> + KUNIT_ASSERT_NE(test, tlb_addr[i], DMA_MAPPING_ERROR);
> + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr[i]));
> + KUNIT_EXPECT_EQ(test, tlb_addr[i] & check_mask, check_val);
> +
> + /* Check sync in both directions. */
> + tlb = phys_to_virt(tlb_addr[i]);
> + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> + *orig ^= 0xff;
> + swiotlb_sync_single_for_device(dev, tlb_addr[i], sizeof(*orig),
> + DMA_TO_DEVICE);
> + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> + *tlb ^= 0xff;
> + swiotlb_sync_single_for_cpu(dev, tlb_addr[i], sizeof(*orig),
> + DMA_FROM_DEVICE);
> + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> +
> + pad_addr[i] = swiotlb_map(dev, phys_addr, sizeof(long),
> + DMA_TO_DEVICE, 0);
> + KUNIT_ASSERT_NE(test, pad_addr[i], DMA_MAPPING_ERROR);
> + }
> +
> + for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
> + swiotlb_tbl_unmap_single(dev, pad_addr[i], sizeof(long),
> + DMA_FROM_DEVICE, 0);
> + swiotlb_tbl_unmap_single(dev, tlb_addr[i], size,
> + DMA_FROM_DEVICE, 0);
> + }
> +}
> +
> +/**************************************************************
> + * Map a DMA buffer.
> + *
> + * Test that a DMA buffer can be mapped and synced.
> + */
> +
> +static void swiotlb_test_map(struct kunit *test)
> +{
> + struct device *dev = test_device(test);
> + phys_addr_t phys_addr;
> + dma_addr_t tlb_addr;
> + unsigned long *tlb;
> +
> + phys_addr = virt_to_phys(&test_data.value);
> + test_data.value = PASS;
> + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
> + DMA_TO_DEVICE, 0);
> + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> + tlb = phys_to_virt(tlb_addr);
> +
> + /* Bounce buffer is initialized to original buffer. */
> + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> +
> + /* Bounce buffer is updated on sync to device. */
> + test_data.value = PASS + 1;
> + swiotlb_sync_single_for_device(dev, tlb_addr, sizeof(unsigned long),
> + DMA_TO_DEVICE);
> + KUNIT_EXPECT_EQ(test, *tlb, PASS + 1);
> +
> + /* Original buffer is updated on sync from device. */
> + *tlb = PASS + 2;
> + swiotlb_sync_single_for_cpu(dev, tlb_addr, sizeof(unsigned long),
> + DMA_FROM_DEVICE);
> + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 2);
Should we not try this on a buffer that is mapped with DMA_FROM_DEVICE ?
> +
> + /* Original buffer is also updated on unmap. */
> + *tlb = PASS + 3;
> + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
> + DMA_FROM_DEVICE, 0);
> + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 3);
> +}
> +
> +/**************************************************************
> + * Map DMA buffer as bi-directional.
> + *
> + * Test that buffer is synced with DMA_BIDIRECTIONAL.
> + */
> +
> +static void swiotlb_test_bidirectional(struct kunit *test)
> +{
> + struct device *dev = test_device(test);
> + phys_addr_t phys_addr;
> + dma_addr_t tlb_addr;
> + unsigned long *tlb;
> +
> + test_data.value = PASS;
> + phys_addr = virt_to_phys(&test_data.value);
> + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
> + DMA_BIDIRECTIONAL, 0);
> + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> + tlb = phys_to_virt(tlb_addr);
> +
> + /* Bounce buffer is initialized to original buffer. */
> + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> +
> + /* Original buffer is updated on unmap. */
> + *tlb = PASS + 1;
> + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
> + DMA_BIDIRECTIONAL, 0);
> + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 1);
> +}
> +
> +/**************************************************************
> + * Skip sync on unmap.
> + *
> + * Test that sync does not happen with DMA_ATTR_SKIP_CPU_SYNC.
> + * On swiotlb_map(), this flag skips only sync for non-coherent
> + * DMA; the bounce buffer itself is always synced to the
> + * original buffer.
> + */
> +
> +static void swiotlb_test_skip_sync(struct kunit *test)
> +{
> + struct device *dev = test_device(test);
> + phys_addr_t phys_addr;
> + dma_addr_t tlb_addr;
> + unsigned long *tlb;
> +
> + test_data.value = PASS;
> + phys_addr = virt_to_phys(&test_data.value);
> + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
> + DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> + tlb = phys_to_virt(tlb_addr);
> +
> + /* Bounce buffer is initialized to original buffer anyway. */
> + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> +
> + /* Original buffer is not updated on unmap. */
> + *tlb = FAIL;
> + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned long),
> + DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> + KUNIT_EXPECT_EQ(test, test_data.value, PASS);
> +}
> +
> +/**************************************************************
> + * Historical page alignment.
> + *
> + * Test that mappings of at least PAGE_SIZE get a page-aligned
> + * DMA address.
> + */
> +
> +static void swiotlb_test_page_align(struct kunit *test)
> +{
> + struct device *dev = test_device(test);
> +
> + /* Bounce buffer is page-aligned. */
> + check_aligned(test, dev, &test_data, sizeof(test_data), 1,
> + PAGE_SHIFT, 0);
> +
> + /* Even if the original buffer is not page-aligned. */
> + check_aligned(test, dev, &test_data.value, PAGE_SIZE, 1,
> + PAGE_SHIFT, 0);
> +}
> +
> +/**************************************************************
> + * Device physical address alignment.
> + *
> + * Test that physical address low bits are preserved.
> + */
> +
> +static void check_min_align(struct kunit *test, int bits)
> +{
> + u64 min_align_mask = DMA_BIT_MASK(bits);
> + struct device *dev = test_device(test);
> + unsigned long vaddr;
> + void *ptr;
> +
> + KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev, min_align_mask), 0);
> +
> + vaddr = devm_get_free_pages(dev, GFP_KERNEL,
> + bits > PAGE_SHIFT ? bits - PAGE_SHIFT : 0);
> + KUNIT_ASSERT_NE(test, vaddr, 0);
> +
> + /* Check low bits */
> + ptr = (void *)vaddr + MAP_OFF;
> + check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
> +
> + /* Check high bits */
> + ptr = (void *)vaddr + (1UL << bits) - MAP_OFF - sizeof(long);
> + check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
> +
> + kunit_device_unregister(test, dev);
> +}
> +
> +static void swiotlb_test_min_align(struct kunit *test)
> +{
> + check_min_align(test, 12);
> + check_min_align(test, PAGE_SHIFT);
> + check_min_align(test, 16);
> +}
> +
> +/**************************************************************
> + * Explicit allocation alignment.
> + *
> + * Test that the bounce buffer is aligned to an explicit value
> + * regardless of allocation size.
> + */
> +
> +static void check_alloc_align(struct kunit *test, int bits)
> +{
> + struct device *dev = test_device(test);
> + void *base, *ptr;
> + size_t size;
> +
> + size = 1UL << bits;
> + base = devm_kmalloc(dev, size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
> +
> + /* Check low bits */
> + ptr = base + MAP_OFF;
> + check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
> +
> + /* Check high bits */
> + ptr = base + size - MAP_OFF - sizeof(long);
> + check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
> +
> + kunit_device_unregister(test, dev);
> +}
> +
> +static void swiotlb_test_alloc_align(struct kunit *test)
> +{
> + check_alloc_align(test, 12);
> + check_alloc_align(test, 14);
> +}
> +
> +/**************************************************************
> + * Both allocation and device physical address alignment.
> + *
> + * Test that the bounce buffer is aligned to an explicit value
> + * regardless of allocation size and it also preserves physical
> + * address low bits.
> + */
> +
> +static void check_both_align(struct kunit *test, int min_align_bits,
> + int alloc_align_bits)
> +{
> + u64 min_align_mask = DMA_BIT_MASK(min_align_bits);
> + struct device *dev = test_device(test);
> + void *base, *ptr;
> + size_t size;
> +
> + KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev, min_align_mask), 0);
> +
> + size = 1UL << max(min_align_bits, alloc_align_bits);
> + base = devm_kmalloc(dev, size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
> +
> + /* Check low bits */
> + ptr = base + MAP_OFF;
> + check_aligned(test, dev, ptr, sizeof(long), size,
> + min_align_bits, min_align_bits);
> +
> + /* Check high bits */
> + ptr = base + size - MAP_OFF - sizeof(long);
> + check_aligned(test, dev, ptr, sizeof(long), size,
> + min_align_bits, min_align_bits);
> +
> + kunit_device_unregister(test, dev);
> +}
> +
> +static void swiotlb_test_both_align(struct kunit *test)
> +{
> + check_both_align(test, 12, 12);
> + check_both_align(test, 12, 16);
> + check_both_align(test, 14, 16);
> +}
> +
> +/**************************************************************
> + * Test suite metadata.
> + */
> +
> +static struct kunit_case swiotlb_test_cases[] = {
> + KUNIT_CASE(swiotlb_test_map),
> + KUNIT_CASE(swiotlb_test_bidirectional),
For better coverage, can we keep seperate tests for each direction ?
May be we could have a common function that takes direction as an
argument.
> + KUNIT_CASE(swiotlb_test_skip_sync),
> + KUNIT_CASE(swiotlb_test_page_align),
> + KUNIT_CASE(swiotlb_test_min_align),
> + KUNIT_CASE(swiotlb_test_alloc_align),
> + KUNIT_CASE(swiotlb_test_both_align),
> + {}
> +};
> +
> +static struct kunit_suite swiotlb_test_suite = {
> + .name = "swiotlb",
> + .suite_init = swiotlb_suite_init,
> + .init = swiotlb_test_init,
> + .test_cases = swiotlb_test_cases,
> +};
> +
> +kunit_test_suites(&swiotlb_test_suite);
> --
> 2.34.1
>
On Tue, 26 Mar 2024 08:38:51 +0530
Linu Cherian <[email protected]> wrote:
> On 2024-03-13 at 14:57:40, Petr Tesarik ([email protected]) wrote:
> > From: Petr Tesarik <[email protected]>
>
> Hi,
Hi Linu,
> >
> > Add unit tests to help avoid regressions in the SWIOTLB code.
> >
> > These features are covered by the test suite:
> >
> > * basic functionality (map, sync)
> > * alignment based on mapping size
> > * alignment based on min_align_mask
> > * explicit alignment with alloc_align_mask
> > * combination of alignment constraints
> >
> > Select CONFIG_SWIOTLB rather than depend on it, because it allows to run
> > the test with UML (default KUnit target).
> >
> > Signed-off-by: Petr Tesarik <[email protected]>
> > ---
> > kernel/dma/Kconfig | 13 ++
> > kernel/dma/Makefile | 1 +
> > kernel/dma/swiotlb_test.c | 413 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 427 insertions(+)
> > create mode 100644 kernel/dma/swiotlb_test.c
> >
> > diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
> > index d62f5957f36b..44c62faa8d89 100644
> > --- a/kernel/dma/Kconfig
> > +++ b/kernel/dma/Kconfig
> > @@ -103,6 +103,19 @@ config SWIOTLB_DYNAMIC
> >
> > If unsure, say N.
> >
> > +config SWIOTLB_KUNIT_TEST
> > + tristate "Unit tests for software IO TLB" if !KUNIT_ALL_TESTS
> > + select SWIOTLB
> > + depends on KUNIT
> > + default KUNIT_ALL_TESTS
> > + help
> > + Build unit tests for software IO TLB.
> > +
> > + For more information on KUnit and unit tests in general, please refer
> > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > +
> > + If unsure, say N.
> > +
> > config DMA_BOUNCE_UNALIGNED_KMALLOC
> > bool
> > depends on SWIOTLB
> > diff --git a/kernel/dma/Makefile b/kernel/dma/Makefile
> > index 21926e46ef4f..bfb130020219 100644
> > --- a/kernel/dma/Makefile
> > +++ b/kernel/dma/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_DMA_CMA) += contiguous.o
> > obj-$(CONFIG_DMA_DECLARE_COHERENT) += coherent.o
> > obj-$(CONFIG_DMA_API_DEBUG) += debug.o
> > obj-$(CONFIG_SWIOTLB) += swiotlb.o
> > +obj-$(CONFIG_SWIOTLB_KUNIT_TEST) += swiotlb_test.o
> > obj-$(CONFIG_DMA_COHERENT_POOL) += pool.o
> > obj-$(CONFIG_MMU) += remap.o
> > obj-$(CONFIG_DMA_MAP_BENCHMARK) += map_benchmark.o
> > diff --git a/kernel/dma/swiotlb_test.c b/kernel/dma/swiotlb_test.c
> > new file mode 100644
> > index 000000000000..46e4d8055ef5
> > --- /dev/null
> > +++ b/kernel/dma/swiotlb_test.c
> > @@ -0,0 +1,413 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2024 Huawei Technologies Duesseldorf GmbH
> > + */
> > +
> > +#include <kunit/test.h>
> > +#include <kunit/device.h>
> > +
> > +#include <linux/device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/io.h>
> > +#include <linux/kdev_t.h>
> > +#include <linux/swiotlb.h>
> > +
> > +/* Alignment check repeat count. */
> > +#define NUM_CHECK_ALIGNED 5
> > +
> > +/* Offset of mapped data inside the allocated buffer. */
> > +#define MAP_OFF 128
> > +
> > +#define PASS 0x600d600d
> > +#define FAIL 0xbad00bad
> > +
> > +static struct {
> > + unsigned char pad1[MAP_OFF];
> > + unsigned long value;
> > + unsigned char pad2[PAGE_SIZE];
> > +} test_data __page_aligned_bss;
> > +
> > +/**************************************************************
> > + * Various helper functions.
> > + */
> > +
> > +static int swiotlb_suite_init(struct kunit_suite *suite)
> > +{
> > + if (is_swiotlb_allocated())
> > + return 0;
> > +
> > + return swiotlb_init_late(swiotlb_size_or_default(), GFP_KERNEL, NULL);
> > +}
> > +
> > +static int swiotlb_drv_probe(struct device *dev)
> > +{
> > + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> > + GFP_KERNEL);
> > + if (!dev->dma_parms)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > +static int swiotlb_test_init(struct kunit *test)
> > +{
> > + struct device_driver *driver;
> > +
> > + driver = kunit_driver_create(test, "swiotlb_driver");
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, driver);
> > + driver->probe = swiotlb_drv_probe;
> > +
> > + test->priv = driver;
> > + return 0;
> > +}
> > +
> > +/**
> > + * test_device() - get a dummy device for testing
> > + * @test: KUnit test instance.
> > + *
> > + * Allocate a device suitable for SWIOTLB.
> > + */
> > +static struct device *test_device(struct kunit *test)
> > +{
> > + struct device_driver *driver = test->priv;
> > + struct device *dev;
> > + u64 mask;
> > +
> > + dev = kunit_device_register_with_driver(test, "swiotlb", driver);
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
> > +
> > + mask = DMA_BIT_MASK(64);
> > + KUNIT_ASSERT_EQ(test, dma_coerce_mask_and_coherent(dev, mask), 0);
> > +
> > + return dev;
> > +}
> > +
> > +/**
> > + * check_aligned() - check that bounce buffers are aligned
> > + * @test: KUnit test instance.
> > + * @dev: Device.
> > + * @buf: Pointer to the original buffer.
> > + * @size: Size of the original buffer.
> > + * @align: Allocation alignment (in bytes).
> > + * @check_bits:
> > + * Number of low bits checked in the swiotlb address.
> > + * @preserve_bits:
> > + * Number of low bits preserved from the original address.
> > + *
> > + * Mapping is repeated a few times, and a small buffer is allocated after
> > + * each attempt. This should cover the case when the first free slot merely
> > + * happens to be suitably aligned.
> > + */
> > +static void check_aligned(struct kunit *test, struct device *dev,
> > + void *buf, size_t size, unsigned long align,
> > + int check_bits, int preserve_bits)
> > +{
> > + dma_addr_t tlb_addr[NUM_CHECK_ALIGNED];
> > + dma_addr_t pad_addr[NUM_CHECK_ALIGNED];
> > + u64 check_mask, check_val;
> > + phys_addr_t phys_addr;
> > + char *orig, *tlb;
> > + int i;
> > +
> > + orig = (char *)buf;
> > + phys_addr = virt_to_phys(buf);
> > + check_mask = DMA_BIT_MASK(check_bits);
> > + check_val = phys_addr & DMA_BIT_MASK(preserve_bits);
> > +
> > + for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
> > + tlb_addr[i] =
> > + swiotlb_tbl_map_single(dev, phys_addr, size, size,
> > + align - 1, DMA_TO_DEVICE, 0);
> > + KUNIT_ASSERT_NE(test, tlb_addr[i], DMA_MAPPING_ERROR);
> > + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr[i]));
> > + KUNIT_EXPECT_EQ(test, tlb_addr[i] & check_mask, check_val);
> > +
> > + /* Check sync in both directions. */
> > + tlb = phys_to_virt(tlb_addr[i]);
> > + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> > + *orig ^= 0xff;
> > + swiotlb_sync_single_for_device(dev, tlb_addr[i], sizeof(*orig),
> > + DMA_TO_DEVICE);
> > + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> > + *tlb ^= 0xff;
> > + swiotlb_sync_single_for_cpu(dev, tlb_addr[i], sizeof(*orig),
> > + DMA_FROM_DEVICE);
> > + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> > +
> > + pad_addr[i] = swiotlb_map(dev, phys_addr, sizeof(long),
> > + DMA_TO_DEVICE, 0);
> > + KUNIT_ASSERT_NE(test, pad_addr[i], DMA_MAPPING_ERROR);
> > + }
> > +
> > + for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
> > + swiotlb_tbl_unmap_single(dev, pad_addr[i], sizeof(long),
> > + DMA_FROM_DEVICE, 0);
> > + swiotlb_tbl_unmap_single(dev, tlb_addr[i], size,
> > + DMA_FROM_DEVICE, 0);
> > + }
> > +}
> > +
> > +/**************************************************************
> > + * Map a DMA buffer.
> > + *
> > + * Test that a DMA buffer can be mapped and synced.
> > + */
> > +
> > +static void swiotlb_test_map(struct kunit *test)
> > +{
> > + struct device *dev = test_device(test);
> > + phys_addr_t phys_addr;
> > + dma_addr_t tlb_addr;
> > + unsigned long *tlb;
> > +
> > + phys_addr = virt_to_phys(&test_data.value);
> > + test_data.value = PASS;
> > + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
> > + DMA_TO_DEVICE, 0);
> > + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> > + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> > + tlb = phys_to_virt(tlb_addr);
> > +
> > + /* Bounce buffer is initialized to original buffer. */
> > + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> > +
> > + /* Bounce buffer is updated on sync to device. */
> > + test_data.value = PASS + 1;
> > + swiotlb_sync_single_for_device(dev, tlb_addr, sizeof(unsigned long),
> > + DMA_TO_DEVICE);
> > + KUNIT_EXPECT_EQ(test, *tlb, PASS + 1);
> > +
> > + /* Original buffer is updated on sync from device. */
> > + *tlb = PASS + 2;
> > + swiotlb_sync_single_for_cpu(dev, tlb_addr, sizeof(unsigned long),
> > + DMA_FROM_DEVICE);
> > + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 2);
>
> Should we not try this on a buffer that is mapped with DMA_FROM_DEVICE ?
I'm afraid I don't follow.
AFAICT the direction is a property of the sync operation. In fact,
swiotlb_tbl_map_single() does not even use its direction parameter at
all. Removing that parameter is already on my TODO list of cleanups.
swiotlb_map() uses its direction parameter only to perform the initial
arch sync if DMA is non-coherent.
OTOH I may be missing some high-level logical concepts which do not
correspond to any actual code in the swiotlb implementation, so my use
is still wrong.
> > +
> > + /* Original buffer is also updated on unmap. */
> > + *tlb = PASS + 3;
> > + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned
> > long),
> > + DMA_FROM_DEVICE, 0);
> > + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 3);
> > +}
> > +
> > +/**************************************************************
> > + * Map DMA buffer as bi-directional.
> > + *
> > + * Test that buffer is synced with DMA_BIDIRECTIONAL.
> > + */
> > +
> > +static void swiotlb_test_bidirectional(struct kunit *test)
> > +{
> > + struct device *dev = test_device(test);
> > + phys_addr_t phys_addr;
> > + dma_addr_t tlb_addr;
> > + unsigned long *tlb;
> > +
> > + test_data.value = PASS;
> > + phys_addr = virt_to_phys(&test_data.value);
> > + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned
> > long),
> > + DMA_BIDIRECTIONAL, 0);
> > + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> > + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> > + tlb = phys_to_virt(tlb_addr);
> > +
> > + /* Bounce buffer is initialized to original buffer. */
> > + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> > +
> > + /* Original buffer is updated on unmap. */
> > + *tlb = PASS + 1;
> > + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned
> > long),
> > + DMA_BIDIRECTIONAL, 0);
> > + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 1);
> > +}
> > +
> > +/**************************************************************
> > + * Skip sync on unmap.
> > + *
> > + * Test that sync does not happen with DMA_ATTR_SKIP_CPU_SYNC.
> > + * On swiotlb_map(), this flag skips only sync for non-coherent
> > + * DMA; the bounce buffer itself is always synced to the
> > + * original buffer.
> > + */
> > +
> > +static void swiotlb_test_skip_sync(struct kunit *test)
> > +{
> > + struct device *dev = test_device(test);
> > + phys_addr_t phys_addr;
> > + dma_addr_t tlb_addr;
> > + unsigned long *tlb;
> > +
> > + test_data.value = PASS;
> > + phys_addr = virt_to_phys(&test_data.value);
> > + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned
> > long),
> > + DMA_TO_DEVICE,
> > DMA_ATTR_SKIP_CPU_SYNC);
> > + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> > + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> > + tlb = phys_to_virt(tlb_addr);
> > +
> > + /* Bounce buffer is initialized to original buffer anyway.
> > */
> > + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> > +
> > + /* Original buffer is not updated on unmap. */
> > + *tlb = FAIL;
> > + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned
> > long),
> > + DMA_FROM_DEVICE,
> > DMA_ATTR_SKIP_CPU_SYNC);
> > + KUNIT_EXPECT_EQ(test, test_data.value, PASS);
> > +}
> > +
> > +/**************************************************************
> > + * Historical page alignment.
> > + *
> > + * Test that mappings of at least PAGE_SIZE get a page-aligned
> > + * DMA address.
> > + */
> > +
> > +static void swiotlb_test_page_align(struct kunit *test)
> > +{
> > + struct device *dev = test_device(test);
> > +
> > + /* Bounce buffer is page-aligned. */
> > + check_aligned(test, dev, &test_data, sizeof(test_data), 1,
> > + PAGE_SHIFT, 0);
> > +
> > + /* Even if the original buffer is not page-aligned. */
> > + check_aligned(test, dev, &test_data.value, PAGE_SIZE, 1,
> > + PAGE_SHIFT, 0);
> > +}
> > +
> > +/**************************************************************
> > + * Device physical address alignment.
> > + *
> > + * Test that physical address low bits are preserved.
> > + */
> > +
> > +static void check_min_align(struct kunit *test, int bits)
> > +{
> > + u64 min_align_mask = DMA_BIT_MASK(bits);
> > + struct device *dev = test_device(test);
> > + unsigned long vaddr;
> > + void *ptr;
> > +
> > + KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev,
> > min_align_mask), 0); +
> > + vaddr = devm_get_free_pages(dev, GFP_KERNEL,
> > + bits > PAGE_SHIFT ? bits -
> > PAGE_SHIFT : 0);
> > + KUNIT_ASSERT_NE(test, vaddr, 0);
> > +
> > + /* Check low bits */
> > + ptr = (void *)vaddr + MAP_OFF;
> > + check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
> > +
> > + /* Check high bits */
> > + ptr = (void *)vaddr + (1UL << bits) - MAP_OFF -
> > sizeof(long);
> > + check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
> > +
> > + kunit_device_unregister(test, dev);
> > +}
> > +
> > +static void swiotlb_test_min_align(struct kunit *test)
> > +{
> > + check_min_align(test, 12);
> > + check_min_align(test, PAGE_SHIFT);
> > + check_min_align(test, 16);
> > +}
> > +
> > +/**************************************************************
> > + * Explicit allocation alignment.
> > + *
> > + * Test that the bounce buffer is aligned to an explicit value
> > + * regardless of allocation size.
> > + */
> > +
> > +static void check_alloc_align(struct kunit *test, int bits)
> > +{
> > + struct device *dev = test_device(test);
> > + void *base, *ptr;
> > + size_t size;
> > +
> > + size = 1UL << bits;
> > + base = devm_kmalloc(dev, size, GFP_KERNEL);
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
> > +
> > + /* Check low bits */
> > + ptr = base + MAP_OFF;
> > + check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
> > +
> > + /* Check high bits */
> > + ptr = base + size - MAP_OFF - sizeof(long);
> > + check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
> > +
> > + kunit_device_unregister(test, dev);
> > +}
> > +
> > +static void swiotlb_test_alloc_align(struct kunit *test)
> > +{
> > + check_alloc_align(test, 12);
> > + check_alloc_align(test, 14);
> > +}
> > +
> > +/**************************************************************
> > + * Both allocation and device physical address alignment.
> > + *
> > + * Test that the bounce buffer is aligned to an explicit value
> > + * regardless of allocation size and it also preserves physical
> > + * address low bits.
> > + */
> > +
> > +static void check_both_align(struct kunit *test, int
> > min_align_bits,
> > + int alloc_align_bits)
> > +{
> > + u64 min_align_mask = DMA_BIT_MASK(min_align_bits);
> > + struct device *dev = test_device(test);
> > + void *base, *ptr;
> > + size_t size;
> > +
> > + KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev,
> > min_align_mask), 0); +
> > + size = 1UL << max(min_align_bits, alloc_align_bits);
> > + base = devm_kmalloc(dev, size, GFP_KERNEL);
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
> > +
> > + /* Check low bits */
> > + ptr = base + MAP_OFF;
> > + check_aligned(test, dev, ptr, sizeof(long), size,
> > + min_align_bits, min_align_bits);
> > +
> > + /* Check high bits */
> > + ptr = base + size - MAP_OFF - sizeof(long);
> > + check_aligned(test, dev, ptr, sizeof(long), size,
> > + min_align_bits, min_align_bits);
> > +
> > + kunit_device_unregister(test, dev);
> > +}
> > +
> > +static void swiotlb_test_both_align(struct kunit *test)
> > +{
> > + check_both_align(test, 12, 12);
> > + check_both_align(test, 12, 16);
> > + check_both_align(test, 14, 16);
> > +}
> > +
> > +/**************************************************************
> > + * Test suite metadata.
> > + */
> > +
> > +static struct kunit_case swiotlb_test_cases[] = {
> > + KUNIT_CASE(swiotlb_test_map),
> > + KUNIT_CASE(swiotlb_test_bidirectional),
>
> For better coverage, can we keep seperate tests for each direction ?
> May be we could have a common function that takes direction as an
> argument.
Ack. Finer granularity will help us understand test reports.
Thank you for looking at my code!
Petr T
Hi Petr,
> -----Original Message-----
> From: Petr Tesa??k <[email protected]>
> Sent: Tuesday, March 26, 2024 5:37 PM
> To: Linu Cherian <[email protected]>
> Cc: Petr Tesarik <[email protected]>; Christoph Hellwig
> <[email protected]>; Marek Szyprowski <[email protected]>; Robin
> Murphy <[email protected]>; open list <linux-
> [email protected]>; open list:DMA MAPPING HELPERS
> <[email protected]>; Will Deacon <[email protected]>; Michael Kelley
> <[email protected]>; Roberto Sassu
> <[email protected]>
> Subject: [EXTERNAL] Re: [PATCH 1/1] swiotlb: add a KUnit test suite
>
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
>
> ----------------------------------------------------------------------
> On Tue, 26 Mar 2024 08:38:51 +0530
> Linu Cherian <[email protected]> wrote:
>
> > On 2024-03-13 at 14:57:40, Petr Tesarik ([email protected])
> wrote:
> > > From: Petr Tesarik <[email protected]>
> >
> > Hi,
>
> Hi Linu,
>
> > >
> > > Add unit tests to help avoid regressions in the SWIOTLB code.
> > >
> > > These features are covered by the test suite:
> > >
> > > * basic functionality (map, sync)
> > > * alignment based on mapping size
> > > * alignment based on min_align_mask
> > > * explicit alignment with alloc_align_mask
> > > * combination of alignment constraints
> > >
> > > Select CONFIG_SWIOTLB rather than depend on it, because it allows to
> > > run the test with UML (default KUnit target).
> > >
> > > Signed-off-by: Petr Tesarik <[email protected]>
> > > ---
> > > kernel/dma/Kconfig | 13 ++
> > > kernel/dma/Makefile | 1 +
> > > kernel/dma/swiotlb_test.c | 413
> > > ++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 427 insertions(+)
> > > create mode 100644 kernel/dma/swiotlb_test.c
> > >
> > > diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig index
> > > d62f5957f36b..44c62faa8d89 100644
> > > --- a/kernel/dma/Kconfig
> > > +++ b/kernel/dma/Kconfig
> > > @@ -103,6 +103,19 @@ config SWIOTLB_DYNAMIC
> > >
> > > If unsure, say N.
> > >
> > > +config SWIOTLB_KUNIT_TEST
> > > + tristate "Unit tests for software IO TLB" if !KUNIT_ALL_TESTS
> > > + select SWIOTLB
> > > + depends on KUNIT
> > > + default KUNIT_ALL_TESTS
> > > + help
> > > + Build unit tests for software IO TLB.
> > > +
> > > + For more information on KUnit and unit tests in general, please refer
> > > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > > +
> > > + If unsure, say N.
> > > +
> > > config DMA_BOUNCE_UNALIGNED_KMALLOC
> > > bool
> > > depends on SWIOTLB
> > > diff --git a/kernel/dma/Makefile b/kernel/dma/Makefile index
> > > 21926e46ef4f..bfb130020219 100644
> > > --- a/kernel/dma/Makefile
> > > +++ b/kernel/dma/Makefile
> > > @@ -7,6 +7,7 @@ obj-$(CONFIG_DMA_CMA) +=
> contiguous.o
> > > obj-$(CONFIG_DMA_DECLARE_COHERENT) += coherent.o
> > > obj-$(CONFIG_DMA_API_DEBUG) += debug.o
> > > obj-$(CONFIG_SWIOTLB) += swiotlb.o
> > > +obj-$(CONFIG_SWIOTLB_KUNIT_TEST) += swiotlb_test.o
> > > obj-$(CONFIG_DMA_COHERENT_POOL) += pool.o
> > > obj-$(CONFIG_MMU) += remap.o
> > > obj-$(CONFIG_DMA_MAP_BENCHMARK) += map_benchmark.o
> > > diff --git a/kernel/dma/swiotlb_test.c b/kernel/dma/swiotlb_test.c
> > > new file mode 100644 index 000000000000..46e4d8055ef5
> > > --- /dev/null
> > > +++ b/kernel/dma/swiotlb_test.c
> > > @@ -0,0 +1,413 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2024 Huawei Technologies Duesseldorf GmbH */
> > > +
> > > +#include <kunit/test.h>
> > > +#include <kunit/device.h>
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kdev_t.h>
> > > +#include <linux/swiotlb.h>
> > > +
> > > +/* Alignment check repeat count. */
> > > +#define NUM_CHECK_ALIGNED 5
> > > +
> > > +/* Offset of mapped data inside the allocated buffer. */
> > > +#define MAP_OFF 128
> > > +
> > > +#define PASS 0x600d600d
> > > +#define FAIL 0xbad00bad
> > > +
> > > +static struct {
> > > + unsigned char pad1[MAP_OFF];
> > > + unsigned long value;
> > > + unsigned char pad2[PAGE_SIZE];
> > > +} test_data __page_aligned_bss;
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Various helper functions.
> > > + */
> > > +
> > > +static int swiotlb_suite_init(struct kunit_suite *suite) {
> > > + if (is_swiotlb_allocated())
> > > + return 0;
> > > +
> > > + return swiotlb_init_late(swiotlb_size_or_default(), GFP_KERNEL,
> > > +NULL); }
> > > +
> > > +static int swiotlb_drv_probe(struct device *dev) {
> > > + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> > > + GFP_KERNEL);
> > > + if (!dev->dma_parms)
> > > + return -ENOMEM;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int swiotlb_test_init(struct kunit *test) {
> > > + struct device_driver *driver;
> > > +
> > > + driver = kunit_driver_create(test, "swiotlb_driver");
> > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, driver);
> > > + driver->probe = swiotlb_drv_probe;
> > > +
> > > + test->priv = driver;
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * test_device() - get a dummy device for testing
> > > + * @test: KUnit test instance.
> > > + *
> > > + * Allocate a device suitable for SWIOTLB.
> > > + */
> > > +static struct device *test_device(struct kunit *test) {
> > > + struct device_driver *driver = test->priv;
> > > + struct device *dev;
> > > + u64 mask;
> > > +
> > > + dev = kunit_device_register_with_driver(test, "swiotlb", driver);
> > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
> > > +
> > > + mask = DMA_BIT_MASK(64);
> > > + KUNIT_ASSERT_EQ(test, dma_coerce_mask_and_coherent(dev,
> mask), 0);
> > > +
> > > + return dev;
> > > +}
> > > +
> > > +/**
> > > + * check_aligned() - check that bounce buffers are aligned
> > > + * @test: KUnit test instance.
> > > + * @dev: Device.
> > > + * @buf: Pointer to the original buffer.
> > > + * @size: Size of the original buffer.
> > > + * @align: Allocation alignment (in bytes).
> > > + * @check_bits:
> > > + * Number of low bits checked in the swiotlb address.
> > > + * @preserve_bits:
> > > + * Number of low bits preserved from the original address.
> > > + *
> > > + * Mapping is repeated a few times, and a small buffer is allocated
> > > +after
> > > + * each attempt. This should cover the case when the first free
> > > +slot merely
> > > + * happens to be suitably aligned.
> > > + */
> > > +static void check_aligned(struct kunit *test, struct device *dev,
> > > + void *buf, size_t size, unsigned long align,
> > > + int check_bits, int preserve_bits) {
> > > + dma_addr_t tlb_addr[NUM_CHECK_ALIGNED];
> > > + dma_addr_t pad_addr[NUM_CHECK_ALIGNED];
> > > + u64 check_mask, check_val;
> > > + phys_addr_t phys_addr;
> > > + char *orig, *tlb;
> > > + int i;
> > > +
> > > + orig = (char *)buf;
> > > + phys_addr = virt_to_phys(buf);
> > > + check_mask = DMA_BIT_MASK(check_bits);
> > > + check_val = phys_addr & DMA_BIT_MASK(preserve_bits);
> > > +
> > > + for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
> > > + tlb_addr[i] =
> > > + swiotlb_tbl_map_single(dev, phys_addr, size, size,
> > > + align - 1, DMA_TO_DEVICE, 0);
> > > + KUNIT_ASSERT_NE(test, tlb_addr[i], DMA_MAPPING_ERROR);
> > > + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr[i]));
> > > + KUNIT_EXPECT_EQ(test, tlb_addr[i] & check_mask,
> check_val);
> > > +
> > > + /* Check sync in both directions. */
> > > + tlb = phys_to_virt(tlb_addr[i]);
> > > + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> > > + *orig ^= 0xff;
> > > + swiotlb_sync_single_for_device(dev, tlb_addr[i],
> sizeof(*orig),
> > > + DMA_TO_DEVICE);
> > > + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> > > + *tlb ^= 0xff;
> > > + swiotlb_sync_single_for_cpu(dev, tlb_addr[i], sizeof(*orig),
> > > + DMA_FROM_DEVICE);
> > > + KUNIT_EXPECT_EQ(test, *orig, *tlb);
> > > +
> > > + pad_addr[i] = swiotlb_map(dev, phys_addr, sizeof(long),
> > > + DMA_TO_DEVICE, 0);
> > > + KUNIT_ASSERT_NE(test, pad_addr[i], DMA_MAPPING_ERROR);
> > > + }
> > > +
> > > + for (i = 0; i < NUM_CHECK_ALIGNED; ++i) {
> > > + swiotlb_tbl_unmap_single(dev, pad_addr[i], sizeof(long),
> > > + DMA_FROM_DEVICE, 0);
> > > + swiotlb_tbl_unmap_single(dev, tlb_addr[i], size,
> > > + DMA_FROM_DEVICE, 0);
> > > + }
> > > +}
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Map a DMA buffer.
> > > + *
> > > + * Test that a DMA buffer can be mapped and synced.
> > > + */
> > > +
> > > +static void swiotlb_test_map(struct kunit *test) {
> > > + struct device *dev = test_device(test);
> > > + phys_addr_t phys_addr;
> > > + dma_addr_t tlb_addr;
> > > + unsigned long *tlb;
> > > +
> > > + phys_addr = virt_to_phys(&test_data.value);
> > > + test_data.value = PASS;
> > > + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned long),
> > > + DMA_TO_DEVICE, 0);
> > > + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> > > + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> > > + tlb = phys_to_virt(tlb_addr);
> > > +
> > > + /* Bounce buffer is initialized to original buffer. */
> > > + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> > > +
> > > + /* Bounce buffer is updated on sync to device. */
> > > + test_data.value = PASS + 1;
> > > + swiotlb_sync_single_for_device(dev, tlb_addr, sizeof(unsigned long),
> > > + DMA_TO_DEVICE);
> > > + KUNIT_EXPECT_EQ(test, *tlb, PASS + 1);
> > > +
> > > + /* Original buffer is updated on sync from device. */
> > > + *tlb = PASS + 2;
> > > + swiotlb_sync_single_for_cpu(dev, tlb_addr, sizeof(unsigned long),
> > > + DMA_FROM_DEVICE);
> > > + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 2);
> >
> > Should we not try this on a buffer that is mapped with DMA_FROM_DEVICE ?
>
> I'm afraid I don't follow.
>
> AFAICT the direction is a property of the sync operation. In fact,
> swiotlb_tbl_map_single() does not even use its direction parameter at all.
> Removing that parameter is already on my TODO list of cleanups.
>
Okay. Got it.
> swiotlb_map() uses its direction parameter only to perform the initial arch
> sync if DMA is non-coherent.
>
> OTOH I may be missing some high-level logical concepts which do not
> correspond to any actual code in the swiotlb implementation, so my use is still
> wrong.
>
Just thought that the keeping the DMA direction consistent for the map and sync would be more aligned to typical use case.
For example, a buffer used for transmit in case of networking. OTOH, since the API by itself doesn't have such constraints on the direction parameter, may be it makes sense to test those scenarios.
> > > +
> > > + /* Original buffer is also updated on unmap. */
> > > + *tlb = PASS + 3;
> > > + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned
> > > long),
> > > + DMA_FROM_DEVICE, 0);
> > > + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 3); }
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Map DMA buffer as bi-directional.
> > > + *
> > > + * Test that buffer is synced with DMA_BIDIRECTIONAL.
> > > + */
> > > +
> > > +static void swiotlb_test_bidirectional(struct kunit *test) {
> > > + struct device *dev = test_device(test);
> > > + phys_addr_t phys_addr;
> > > + dma_addr_t tlb_addr;
> > > + unsigned long *tlb;
> > > +
> > > + test_data.value = PASS;
> > > + phys_addr = virt_to_phys(&test_data.value);
> > > + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned
> > > long),
> > > + DMA_BIDIRECTIONAL, 0);
> > > + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> > > + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> > > + tlb = phys_to_virt(tlb_addr);
> > > +
> > > + /* Bounce buffer is initialized to original buffer. */
> > > + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> > > +
> > > + /* Original buffer is updated on unmap. */
> > > + *tlb = PASS + 1;
> > > + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned
> > > long),
> > > + DMA_BIDIRECTIONAL, 0);
> > > + KUNIT_EXPECT_EQ(test, test_data.value, PASS + 1); }
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Skip sync on unmap.
> > > + *
> > > + * Test that sync does not happen with DMA_ATTR_SKIP_CPU_SYNC.
> > > + * On swiotlb_map(), this flag skips only sync for non-coherent
> > > + * DMA; the bounce buffer itself is always synced to the
> > > + * original buffer.
> > > + */
> > > +
> > > +static void swiotlb_test_skip_sync(struct kunit *test) {
> > > + struct device *dev = test_device(test);
> > > + phys_addr_t phys_addr;
> > > + dma_addr_t tlb_addr;
> > > + unsigned long *tlb;
> > > +
> > > + test_data.value = PASS;
> > > + phys_addr = virt_to_phys(&test_data.value);
> > > + tlb_addr = swiotlb_map(dev, phys_addr, sizeof(unsigned
> > > long),
> > > + DMA_TO_DEVICE,
> > > DMA_ATTR_SKIP_CPU_SYNC);
> > > + KUNIT_ASSERT_NE(test, tlb_addr, DMA_MAPPING_ERROR);
> > > + KUNIT_EXPECT_TRUE(test, is_swiotlb_buffer(dev, tlb_addr));
> > > + tlb = phys_to_virt(tlb_addr);
> > > +
> > > + /* Bounce buffer is initialized to original buffer anyway.
> > > */
> > > + KUNIT_EXPECT_EQ(test, *tlb, PASS);
> > > +
> > > + /* Original buffer is not updated on unmap. */
> > > + *tlb = FAIL;
> > > + swiotlb_tbl_unmap_single(dev, tlb_addr, sizeof(unsigned
> > > long),
> > > + DMA_FROM_DEVICE,
> > > DMA_ATTR_SKIP_CPU_SYNC);
> > > + KUNIT_EXPECT_EQ(test, test_data.value, PASS); }
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Historical page alignment.
> > > + *
> > > + * Test that mappings of at least PAGE_SIZE get a page-aligned
> > > + * DMA address.
> > > + */
> > > +
> > > +static void swiotlb_test_page_align(struct kunit *test) {
> > > + struct device *dev = test_device(test);
> > > +
> > > + /* Bounce buffer is page-aligned. */
> > > + check_aligned(test, dev, &test_data, sizeof(test_data), 1,
> > > + PAGE_SHIFT, 0);
> > > +
> > > + /* Even if the original buffer is not page-aligned. */
> > > + check_aligned(test, dev, &test_data.value, PAGE_SIZE, 1,
> > > + PAGE_SHIFT, 0);
> > > +}
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Device physical address alignment.
> > > + *
> > > + * Test that physical address low bits are preserved.
> > > + */
> > > +
> > > +static void check_min_align(struct kunit *test, int bits) {
> > > + u64 min_align_mask = DMA_BIT_MASK(bits);
> > > + struct device *dev = test_device(test);
> > > + unsigned long vaddr;
> > > + void *ptr;
> > > +
> > > + KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev,
> > > min_align_mask), 0); +
> > > + vaddr = devm_get_free_pages(dev, GFP_KERNEL,
> > > + bits > PAGE_SHIFT ? bits -
> > > PAGE_SHIFT : 0);
> > > + KUNIT_ASSERT_NE(test, vaddr, 0);
> > > +
> > > + /* Check low bits */
> > > + ptr = (void *)vaddr + MAP_OFF;
> > > + check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
> > > +
> > > + /* Check high bits */
> > > + ptr = (void *)vaddr + (1UL << bits) - MAP_OFF -
> > > sizeof(long);
> > > + check_aligned(test, dev, ptr, sizeof(long), 1, bits, bits);
> > > +
> > > + kunit_device_unregister(test, dev); }
> > > +
> > > +static void swiotlb_test_min_align(struct kunit *test) {
> > > + check_min_align(test, 12);
> > > + check_min_align(test, PAGE_SHIFT);
> > > + check_min_align(test, 16);
> > > +}
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Explicit allocation alignment.
> > > + *
> > > + * Test that the bounce buffer is aligned to an explicit value
> > > + * regardless of allocation size.
> > > + */
> > > +
> > > +static void check_alloc_align(struct kunit *test, int bits) {
> > > + struct device *dev = test_device(test);
> > > + void *base, *ptr;
> > > + size_t size;
> > > +
> > > + size = 1UL << bits;
> > > + base = devm_kmalloc(dev, size, GFP_KERNEL);
> > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
> > > +
> > > + /* Check low bits */
> > > + ptr = base + MAP_OFF;
> > > + check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
> > > +
> > > + /* Check high bits */
> > > + ptr = base + size - MAP_OFF - sizeof(long);
> > > + check_aligned(test, dev, ptr, sizeof(long), size, bits, 0);
> > > +
> > > + kunit_device_unregister(test, dev); }
> > > +
> > > +static void swiotlb_test_alloc_align(struct kunit *test) {
> > > + check_alloc_align(test, 12);
> > > + check_alloc_align(test, 14);
> > > +}
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Both allocation and device physical address alignment.
> > > + *
> > > + * Test that the bounce buffer is aligned to an explicit value
> > > + * regardless of allocation size and it also preserves physical
> > > + * address low bits.
> > > + */
> > > +
> > > +static void check_both_align(struct kunit *test, int
> > > min_align_bits,
> > > + int alloc_align_bits)
> > > +{
> > > + u64 min_align_mask = DMA_BIT_MASK(min_align_bits);
> > > + struct device *dev = test_device(test);
> > > + void *base, *ptr;
> > > + size_t size;
> > > +
> > > + KUNIT_ASSERT_EQ(test, dma_set_min_align_mask(dev,
> > > min_align_mask), 0); +
> > > + size = 1UL << max(min_align_bits, alloc_align_bits);
> > > + base = devm_kmalloc(dev, size, GFP_KERNEL);
> > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, base);
> > > +
> > > + /* Check low bits */
> > > + ptr = base + MAP_OFF;
> > > + check_aligned(test, dev, ptr, sizeof(long), size,
> > > + min_align_bits, min_align_bits);
> > > +
> > > + /* Check high bits */
> > > + ptr = base + size - MAP_OFF - sizeof(long);
> > > + check_aligned(test, dev, ptr, sizeof(long), size,
> > > + min_align_bits, min_align_bits);
> > > +
> > > + kunit_device_unregister(test, dev); }
> > > +
> > > +static void swiotlb_test_both_align(struct kunit *test) {
> > > + check_both_align(test, 12, 12);
> > > + check_both_align(test, 12, 16);
> > > + check_both_align(test, 14, 16);
> > > +}
> > > +
> > >
> +/**********************************************************
> ****
> > > + * Test suite metadata.
> > > + */
> > > +
> > > +static struct kunit_case swiotlb_test_cases[] = {
> > > + KUNIT_CASE(swiotlb_test_map),
> > > + KUNIT_CASE(swiotlb_test_bidirectional),
> >
> > For better coverage, can we keep seperate tests for each direction ?
> > May be we could have a common function that takes direction as an
> > argument.
>
> Ack. Finer granularity will help us understand test reports.
>
> Thank you for looking at my code!
>
> Petr T
On 2024-04-03 3:19 pm, Linu Cherian wrote:
[...]
>>> Should we not try this on a buffer that is mapped with DMA_FROM_DEVICE ?
>>
>> I'm afraid I don't follow.
>>
>> AFAICT the direction is a property of the sync operation. In fact,
>> swiotlb_tbl_map_single() does not even use its direction parameter at all.
>> Removing that parameter is already on my TODO list of cleanups.
>>
>
> Okay. Got it.
>
>> swiotlb_map() uses its direction parameter only to perform the initial arch
>> sync if DMA is non-coherent.
>>
>> OTOH I may be missing some high-level logical concepts which do not
>> correspond to any actual code in the swiotlb implementation, so my use is still
>> wrong.
>>
>
> Just thought that the keeping the DMA direction consistent for the map and sync would be more aligned to typical use case.
> For example, a buffer used for transmit in case of networking. OTOH, since the API by itself doesn't have such constraints on the direction parameter, may be it makes sense to test those scenarios.
Right, SWIOTLB exists to serve the DMA API, so it makes more sense to me
to test it in the context of valid DMA API usage than to make up
scenarios that aren't representative of real-world usage. The direction
is a property of the whole DMA mapping itself, and thus must be passed
consistently for every operation on a given mapping.
Yes, there is some internal trickery once we get down to the level of
calling swiotlb_bounce() itself, but if we're driving the tests through
the higher-level public interface then I'd prefer to see that used as
expected. Given the whole partial-write-transparency business, the most
significant effect of direction should just be that of DMA_TO_DEVICE
skipping the copy-out for unmap and sync_for_cpu, so for the sake of
coverage you may as well just use DMA_BIDIRECTIONAL everywhere.
Thanks,
Robin.
On Wed, 3 Apr 2024 17:58:47 +0100
Robin Murphy <[email protected]> wrote:
> On 2024-04-03 3:19 pm, Linu Cherian wrote:
> [...]
> >>> Should we not try this on a buffer that is mapped with DMA_FROM_DEVICE ?
> >>
> >> I'm afraid I don't follow.
> >>
> >> AFAICT the direction is a property of the sync operation. In fact,
> >> swiotlb_tbl_map_single() does not even use its direction parameter at all.
> >> Removing that parameter is already on my TODO list of cleanups.
> >>
> >
> > Okay. Got it.
> >
> >> swiotlb_map() uses its direction parameter only to perform the initial arch
> >> sync if DMA is non-coherent.
> >>
> >> OTOH I may be missing some high-level logical concepts which do not
> >> correspond to any actual code in the swiotlb implementation, so my use is still
> >> wrong.
> >>
> >
> > Just thought that the keeping the DMA direction consistent for the map and sync would be more aligned to typical use case.
> > For example, a buffer used for transmit in case of networking. OTOH, since the API by itself doesn't have such constraints on the direction parameter, may be it makes sense to test those scenarios.
>
> Right, SWIOTLB exists to serve the DMA API, so it makes more sense to me
> to test it in the context of valid DMA API usage than to make up
> scenarios that aren't representative of real-world usage. The direction
> is a property of the whole DMA mapping itself, and thus must be passed
> consistently for every operation on a given mapping.
>
> Yes, there is some internal trickery once we get down to the level of
> calling swiotlb_bounce() itself, but if we're driving the tests through
> the higher-level public interface then I'd prefer to see that used as
> expected. Given the whole partial-write-transparency business, the most
> significant effect of direction should just be that of DMA_TO_DEVICE
> skipping the copy-out for unmap and sync_for_cpu, so for the sake of
> coverage you may as well just use DMA_BIDIRECTIONAL everywhere.
Thank you for the explanation. Got it now.
I'm now going to take all comments into account for a v2 series, plus I
want to add a test for partial sync to cover Michael's fix.
Stay tuned,
Petr T