Re: [PATCH 1/1] swiotlb: add a KUnit test suite

From: Linu Cherian
Date: Mon Mar 25 2024 - 23:09:31 EST

Next message: Edgecombe, Rick P: "Re: [PATCH v19 035/130] KVM: TDX: Add place holder for TDX VM specific mem_enc_op ioctl"
Previous message: Huang, Ying: "Re: [External] Re: [PATCH v4 2/2] memory tier: create CPUless memory tiers after obtaining HMAT info"
In reply to: Michael Kelley: "RE: [PATCH 1/1] swiotlb: add a KUnit test suite"
Next in thread: Petr Tesařík: "Re: [PATCH 1/1] swiotlb: add a KUnit test suite"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2024-03-13 at 14:57:40, Petr Tesarik (petrtesarik@xxxxxxxxxxxxxxx) wrote:
> From: Petr Tesarik <petr.tesarik1@xxxxxxxxxxxxxxxxxxx>

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 <petr.tesarik1@xxxxxxxxxxxxxxxxxxx>
> ---
> 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
>

Next message: Edgecombe, Rick P: "Re: [PATCH v19 035/130] KVM: TDX: Add place holder for TDX VM specific mem_enc_op ioctl"
Previous message: Huang, Ying: "Re: [External] Re: [PATCH v4 2/2] memory tier: create CPUless memory tiers after obtaining HMAT info"
In reply to: Michael Kelley: "RE: [PATCH 1/1] swiotlb: add a KUnit test suite"
Next in thread: Petr Tesařík: "Re: [PATCH 1/1] swiotlb: add a KUnit test suite"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]