Hi, Nathan, Nathan Chancellor <nathan@xxxxxxxxxx> writes: > On Tue, Sep 17, 2024 at 01:08:49AM -0700, Andrew Morton wrote: >> >> The quilt patch titled >> Subject: resource, kunit: add test case for region_intersects() >> has been removed from the -mm tree. Its filename was >> resource-kunit-add-test-case-for-region_intersects.patch >> >> This patch was dropped because it was merged into the mm-stable branch >> of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm >> >> ------------------------------------------------------ >> From: Huang Ying <ying.huang@xxxxxxxxx> >> Subject: resource, kunit: add test case for region_intersects() >> Date: Fri, 6 Sep 2024 11:07:13 +0800 >> >> Patch series "resource: Fix region_intersects() vs >> add_memory_driver_managed()", v3. >> >> The patchset fixes a bug of region_intersects() for systems with CXL >> memory. The details of the bug can be found in [1/3]. To avoid similar >> bugs in the future. A kunit test case for region_intersects() is added in >> [3/3]. [2/3] is a preparation patch for [3/3]. >> >> >> This patch (of 3): >> >> region_intersects() is important because it's used for /dev/mem permission >> checking. To avoid possible bug of region_intersects() in the future, a >> kunit test case for region_intersects() is added. >> >> Link: https://lkml.kernel.org/r/20240906030713.204292-1-ying.huang@xxxxxxxxx >> Link: https://lkml.kernel.org/r/20240906030713.204292-4-ying.huang@xxxxxxxxx >> Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx> >> Cc: Dan Williams <dan.j.williams@xxxxxxxxx> >> Cc: David Hildenbrand <david@xxxxxxxxxx> >> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx> >> Cc: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx> >> Cc: Dave Jiang <dave.jiang@xxxxxxxxx> >> Cc: Alison Schofield <alison.schofield@xxxxxxxxx> >> Cc: Vishal Verma <vishal.l.verma@xxxxxxxxx> >> Cc: Ira Weiny <ira.weiny@xxxxxxxxx> >> Cc: Alistair Popple <apopple@xxxxxxxxxx> >> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> >> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> >> Cc: Baoquan He <bhe@xxxxxxxxxx> >> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> >> --- >> >> kernel/resource.c | 20 +++-- >> kernel/resource_kunit.c | 143 ++++++++++++++++++++++++++++++++++++++ >> lib/Kconfig.debug | 1 >> 3 files changed, 158 insertions(+), 6 deletions(-) >> >> --- a/kernel/resource.c~resource-kunit-add-test-case-for-region_intersects >> +++ a/kernel/resource.c >> @@ -1817,7 +1817,17 @@ EXPORT_SYMBOL(resource_list_free); >> #ifdef CONFIG_GET_FREE_REGION >> #define GFR_DESCENDING (1UL << 0) >> #define GFR_REQUEST_REGION (1UL << 1) >> -#define GFR_DEFAULT_ALIGN (1UL << PA_SECTION_SHIFT) >> +#ifdef PA_SECTION_SHIFT >> +#define GFR_DEFAULT_ALIGN (1UL << PA_SECTION_SHIFT) >> +#else >> +#define GFR_DEFAULT_ALIGN PAGE_SIZE >> +#endif >> + >> +#ifdef MAX_PHYSMEM_BITS >> +#define MAX_PHYS_ADDR ((1ULL << MAX_PHYSMEM_BITS) - 1) >> +#else >> +#define MAX_PHYS_ADDR (-1ULL) >> +#endif >> >> static resource_size_t gfr_start(struct resource *base, resource_size_t size, >> resource_size_t align, unsigned long flags) >> @@ -1825,8 +1835,7 @@ static resource_size_t gfr_start(struct >> if (flags & GFR_DESCENDING) { >> resource_size_t end; >> >> - end = min_t(resource_size_t, base->end, >> - (1ULL << MAX_PHYSMEM_BITS) - 1); >> + end = min_t(resource_size_t, base->end, MAX_PHYS_ADDR); >> return end - size + 1; >> } >> >> @@ -1843,8 +1852,7 @@ static bool gfr_continue(struct resource >> * @size did not wrap 0. >> */ >> return addr > addr - size && >> - addr <= min_t(resource_size_t, base->end, >> - (1ULL << MAX_PHYSMEM_BITS) - 1); >> + addr <= min_t(resource_size_t, base->end, MAX_PHYS_ADDR); >> } >> >> static resource_size_t gfr_next(resource_size_t addr, resource_size_t size, >> @@ -2005,7 +2013,7 @@ struct resource *alloc_free_mem_region(s >> return get_free_mem_region(NULL, base, size, align, name, >> IORES_DESC_NONE, flags); >> } >> -EXPORT_SYMBOL_NS_GPL(alloc_free_mem_region, CXL); >> +EXPORT_SYMBOL_GPL(alloc_free_mem_region); >> #endif /* CONFIG_GET_FREE_REGION */ >> >> static int __init strict_iomem(char *str) >> --- a/kernel/resource_kunit.c~resource-kunit-add-test-case-for-region_intersects >> +++ a/kernel/resource_kunit.c >> @@ -7,6 +7,8 @@ >> #include <linux/ioport.h> >> #include <linux/kernel.h> >> #include <linux/string.h> >> +#include <linux/sizes.h> >> +#include <linux/mm.h> >> >> #define R0_START 0x0000 >> #define R0_END 0xffff >> @@ -137,9 +139,150 @@ static void resource_test_intersection(s >> } while (++i < ARRAY_SIZE(results_for_intersection)); >> } >> >> +/* >> + * The test resource tree for region_intersects() test: >> + * >> + * BASE-BASE+1M-1 : Test System RAM 0 >> + * # hole 0 (BASE+1M-BASE+2M) >> + * BASE+2M-BASE+3M-1 : Test CXL Window 0 >> + * BASE+3M-BASE+4M-1 : Test System RAM 1 >> + * BASE+4M-BASE+7M-1 : Test CXL Window 1 >> + * BASE+4M-BASE+5M-1 : Test System RAM 2 >> + * BASE+4M+128K-BASE+4M+256K-1: Test Code >> + * BASE+5M-BASE+6M-1 : Test System RAM 3 >> + */ >> +#define RES_TEST_RAM0_OFFSET 0 >> +#define RES_TEST_RAM0_SIZE SZ_1M >> +#define RES_TEST_HOLE0_OFFSET (RES_TEST_RAM0_OFFSET + RES_TEST_RAM0_SIZE) >> +#define RES_TEST_HOLE0_SIZE SZ_1M >> +#define RES_TEST_WIN0_OFFSET (RES_TEST_HOLE0_OFFSET + RES_TEST_HOLE0_SIZE) >> +#define RES_TEST_WIN0_SIZE SZ_1M >> +#define RES_TEST_RAM1_OFFSET (RES_TEST_WIN0_OFFSET + RES_TEST_WIN0_SIZE) >> +#define RES_TEST_RAM1_SIZE SZ_1M >> +#define RES_TEST_WIN1_OFFSET (RES_TEST_RAM1_OFFSET + RES_TEST_RAM1_SIZE) >> +#define RES_TEST_WIN1_SIZE (SZ_1M * 3) >> +#define RES_TEST_RAM2_OFFSET RES_TEST_WIN1_OFFSET >> +#define RES_TEST_RAM2_SIZE SZ_1M >> +#define RES_TEST_CODE_OFFSET (RES_TEST_RAM2_OFFSET + SZ_128K) >> +#define RES_TEST_CODE_SIZE SZ_128K >> +#define RES_TEST_RAM3_OFFSET (RES_TEST_RAM2_OFFSET + RES_TEST_RAM2_SIZE) >> +#define RES_TEST_RAM3_SIZE SZ_1M >> +#define RES_TEST_TOTAL_SIZE ((RES_TEST_WIN1_OFFSET + RES_TEST_WIN1_SIZE)) >> + >> +static void remove_free_resource(void *ctx) >> +{ >> + struct resource *res = (struct resource *)ctx; >> + >> + remove_resource(res); >> + kfree(res); >> +} >> + >> +static void resource_test_request_region(struct kunit *test, struct resource *parent, >> + resource_size_t start, resource_size_t size, >> + const char *name, unsigned long flags) >> +{ >> + struct resource *res; >> + >> + res = __request_region(parent, start, size, name, flags); >> + KUNIT_ASSERT_NOT_NULL(test, res); >> + kunit_add_action_or_reset(test, remove_free_resource, res); >> +} >> + >> +static void resource_test_insert_resource(struct kunit *test, struct resource *parent, >> + resource_size_t start, resource_size_t size, >> + const char *name, unsigned long flags) >> +{ >> + struct resource *res; >> + >> + res = kzalloc(sizeof(*res), GFP_KERNEL); >> + KUNIT_ASSERT_NOT_NULL(test, res); >> + >> + res->name = name; >> + res->start = start; >> + res->end = start + size - 1; >> + res->flags = flags; >> + if (insert_resource(parent, res)) { >> + kfree(res); >> + KUNIT_FAIL_AND_ABORT(test, "Fail to insert resource %pR\n", res); >> + } >> + >> + kunit_add_action_or_reset(test, remove_free_resource, res); >> +} >> + >> +static void resource_test_region_intersects(struct kunit *test) >> +{ >> + unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY; >> + struct resource *parent; >> + resource_size_t start; >> + >> + /* Find an iomem_resource hole to hold test resources */ >> + parent = alloc_free_mem_region(&iomem_resource, RES_TEST_TOTAL_SIZE, SZ_1M, >> + "test resources"); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent); >> + start = parent->start; >> + kunit_add_action_or_reset(test, remove_free_resource, parent); >> + >> + resource_test_request_region(test, parent, start + RES_TEST_RAM0_OFFSET, >> + RES_TEST_RAM0_SIZE, "Test System RAM 0", flags); >> + resource_test_insert_resource(test, parent, start + RES_TEST_WIN0_OFFSET, >> + RES_TEST_WIN0_SIZE, "Test CXL Window 0", >> + IORESOURCE_MEM); >> + resource_test_request_region(test, parent, start + RES_TEST_RAM1_OFFSET, >> + RES_TEST_RAM1_SIZE, "Test System RAM 1", flags); >> + resource_test_insert_resource(test, parent, start + RES_TEST_WIN1_OFFSET, >> + RES_TEST_WIN1_SIZE, "Test CXL Window 1", >> + IORESOURCE_MEM); >> + resource_test_request_region(test, parent, start + RES_TEST_RAM2_OFFSET, >> + RES_TEST_RAM2_SIZE, "Test System RAM 2", flags); >> + resource_test_insert_resource(test, parent, start + RES_TEST_CODE_OFFSET, >> + RES_TEST_CODE_SIZE, "Test Code", flags); >> + resource_test_request_region(test, parent, start + RES_TEST_RAM3_OFFSET, >> + RES_TEST_RAM3_SIZE, "Test System RAM 3", flags); >> + kunit_release_action(test, remove_free_resource, parent); >> + >> + KUNIT_EXPECT_EQ(test, REGION_INTERSECTS, >> + region_intersects(start + RES_TEST_RAM0_OFFSET, PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_INTERSECTS, >> + region_intersects(start + RES_TEST_RAM0_OFFSET + >> + RES_TEST_RAM0_SIZE - PAGE_SIZE, 2 * PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_DISJOINT, >> + region_intersects(start + RES_TEST_HOLE0_OFFSET, PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_DISJOINT, >> + region_intersects(start + RES_TEST_HOLE0_OFFSET + >> + RES_TEST_HOLE0_SIZE - PAGE_SIZE, 2 * PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_MIXED, >> + region_intersects(start + RES_TEST_WIN0_OFFSET + >> + RES_TEST_WIN0_SIZE - PAGE_SIZE, 2 * PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_INTERSECTS, >> + region_intersects(start + RES_TEST_RAM1_OFFSET + >> + RES_TEST_RAM1_SIZE - PAGE_SIZE, 2 * PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_INTERSECTS, >> + region_intersects(start + RES_TEST_RAM2_OFFSET + >> + RES_TEST_RAM2_SIZE - PAGE_SIZE, 2 * PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_INTERSECTS, >> + region_intersects(start + RES_TEST_CODE_OFFSET, PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_INTERSECTS, >> + region_intersects(start + RES_TEST_RAM2_OFFSET, >> + RES_TEST_RAM2_SIZE + PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> + KUNIT_EXPECT_EQ(test, REGION_MIXED, >> + region_intersects(start + RES_TEST_RAM3_OFFSET, >> + RES_TEST_RAM3_SIZE + PAGE_SIZE, >> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)); >> +} >> + >> static struct kunit_case resource_test_cases[] = { >> KUNIT_CASE(resource_test_union), >> KUNIT_CASE(resource_test_intersection), >> + KUNIT_CASE(resource_test_region_intersects), >> {} >> }; >> >> --- a/lib/Kconfig.debug~resource-kunit-add-test-case-for-region_intersects >> +++ a/lib/Kconfig.debug >> @@ -2616,6 +2616,7 @@ config RESOURCE_KUNIT_TEST >> tristate "KUnit test for resource API" if !KUNIT_ALL_TESTS >> depends on KUNIT >> default KUNIT_ALL_TESTS >> + select GET_FREE_REGION >> help >> This builds the resource API unit test. >> Tests the logic of API provided by resource.c and ioport.h. >> _ >> >> Patches currently in -mm which might be from ying.huang@xxxxxxxxx are >> >> > > This change results in the following Kconfig warning and compiler > warnings (or errors with CONFIG_WERROR): > > $ make -skj"$(nproc)" ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- allmodconfig kernel/resource.o > WARNING: unmet direct dependencies detected for GET_FREE_REGION > Depends on [n]: SPARSEMEM [=n] > Selected by [m]: > - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m] Thanks for reporting! After more investigation, it appears that GET_FREE_REGION depends on SPARSEMEM just for several constant. That can be fixed relatively easily. I will do that. > In file included from include/linux/ioport.h:15, > from kernel/resource.c:15: > kernel/resource.c: In function 'gfr_start': > include/linux/minmax.h:93:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Werror=overflow] > 93 | ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); }) > | ^ > include/linux/minmax.h:96:9: note: in expansion of macro '__cmp_once_unique' > 96 | __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) > | ^~~~~~~~~~~~~~~~~ > include/linux/minmax.h:213:27: note: in expansion of macro '__cmp_once' > 213 | #define min_t(type, x, y) __cmp_once(min, type, x, y) > | ^~~~~~~~~~ > kernel/resource.c:1874:23: note: in expansion of macro 'min_t' > 1874 | end = min_t(resource_size_t, base->end, PHYSMEM_END); > | ^~~~~ > kernel/resource.c: In function 'gfr_continue': > include/linux/minmax.h:93:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Werror=overflow] > 93 | ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); }) > | ^ > include/linux/minmax.h:96:9: note: in expansion of macro '__cmp_once_unique' > 96 | __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) > | ^~~~~~~~~~~~~~~~~ > include/linux/minmax.h:213:27: note: in expansion of macro '__cmp_once' > 213 | #define min_t(type, x, y) __cmp_once(min, type, x, y) > | ^~~~~~~~~~ > kernel/resource.c:1891:24: note: in expansion of macro 'min_t' > 1891 | addr <= min_t(resource_size_t, base->end, PHYSMEM_END); > | ^~~~~ > cc1: all warnings being treated as errors > > Arnd had a patch in another bug report to resolve the Kconfig warning, > which happens to resolve the compiler warning for allmodconfig, but I > suspect that a configuration with SPARSEMEM and !ARM_LPAE could still > trigger the compiler warnings. > > https://lore.kernel.org/aaee4ddb-68a8-42ae-bb68-11ef991ada1c@xxxxxxxxxxxxxxxx/ Yes. That is a quick fix. I will prepare a complete fix ASAP. But the merge path of the patch is a little complex. I think that it may be better to fix it after current patch and fixes being landed. -- Best Regards, Huang, Ying
