[PATCH RFC] Add "uunit" unit testing framework for CXL code

[PATCH RFC] Add "uunit" unit testing framework for CXL code

[Jim Harris]

uunit (userspace unit testing) generates wrappers for kernel source files
which are compiled and linked with necessary function stubs and executed
fully in userspace.

Existing unit testing frameworks (kunit, tools/testing/cxl) have some
limitations that uunit alleviates:

* mock referenced functions without requiring modifications to those functions
* mock register accesses
* does not require execution in a running kernel
* no restrictions currently on CONFIG parameters
  (Note: some CXL code cannot run in UML with kunit due to dependencies on
  CONFIG_SPARSEMEM)

This RFC contains some complex unit tests for core/region.c code, as well as
a simple core/hdm.c test showing how register accesses can be mocked.

This RFC depends on CUnit, which needs to be installed on the system prior to build.

To build the unit tests:

  make -C tools/testing/cxl/uunit

To run the unit tests:

  make -C tools/testing/cxl/uunit runtests

There is a balance currently between creating stubs for referenced functions,
versus just building and linking the referenced source files directly. For
example, CXL uses xarray extensively, so we just build and link the xarray
code directly into unit tests rather than creating stubs for xarray. See
KERNEL_SRC in the Makefile for which kernel source files are directly built
and linked in this manner.

Stubs are in uunit/stub.c, and are grouped by the kernel source file containing
the real definition. Those groups are listed in alphabetical order.

The build process has five primary steps:

Capture kernel build flags - use 'make -qp' to pull LINUXINCLUDE,
KBUILD_CPPFLAGS, KBUILD_CFLAGS from the top-level kernel Makefile.

Create unit test build environment - remove thunking, fentry and
stack-protector options from the kernel build flags. Also provide
KBUILD_XXX strings.

Modified headers - some kernel headers require modification - don't
modify them inline though, create a copy in the uunit/build directory,
modify them, and set the include directories so that the build will
pick up the modified copy first. Explanations for the modified headers
are in the Makefile.

Wrap and build kernel source - kernel defines many functions that are
also in libc. Kernel also defines some data structures with same name
as /usr/include data structures. So we wrap all kernel source files
with uunit/include/pre.h and uunit/include/post.h to do some
pre-processor magic to eliminate these conflicts.

Build unit test applications - found in uunit/app directory, and named
like core_region_ut.c to associate with core/region.c. The test
applications include the stub.c file directly - this will enable future
use cases where stubs can be configured to return specific mocked values.

Note that this framework is not CXL-specific - it could be used to unit
test a vast majority of other kernel code. Proposal is to just start
with this framework in tools/testing/cxl to meet CXL-specific needs.

Todos:

1) Integrate with tools/testing/cxl Makefile
2) Break out CXL stubs separately by source file. This will be required as
   we start testing more and more CXL files and need to exclude stubs that
   are defined in the source file under test.
3) Build common library for building up parts of the CXL topology. For example,
   see everything needed to set up ports, decoders, etc. to test the
   region.c code. Much of this can be simplified in a way that various
   topologies can be built without as much boilerplate code, not only for
   region.c but other files as well.
4) Improve cxl_region tests to cover error conditions and add extensive
   comments explaining how the topology is being built. This overlaps with #3.

Signed-off-by: Jim Harris <jim.harris@samsung.com>
---
 tools/testing/cxl/uunit/Makefile             |  141 +++++++++++
 tools/testing/cxl/uunit/app/core_hdm_ut.c    |   82 ++++++
 tools/testing/cxl/uunit/app/core_region_ut.c |  237 ++++++++++++++++++
 tools/testing/cxl/uunit/include/post.h       |   21 ++
 tools/testing/cxl/uunit/include/pre.h        |   22 ++
 tools/testing/cxl/uunit/stub.c               |  335 ++++++++++++++++++++++++++
 6 files changed, 838 insertions(+)
 create mode 100644 tools/testing/cxl/uunit/Makefile
 create mode 100644 tools/testing/cxl/uunit/app/core_hdm_ut.c
 create mode 100644 tools/testing/cxl/uunit/app/core_region_ut.c
 create mode 100644 tools/testing/cxl/uunit/include/post.h
 create mode 100644 tools/testing/cxl/uunit/include/pre.h
 create mode 100644 tools/testing/cxl/uunit/stub.c

diff --git a/tools/testing/cxl/uunit/Makefile b/tools/testing/cxl/uunit/Makefile
new file mode 100644
index 000000000000..290a4c332b8b
--- /dev/null
+++ b/tools/testing/cxl/uunit/Makefile
@@ -0,0 +1,141 @@
+# SPDX-License-Identifier: GPL-2.0
+ROOT_DIR := $(CURDIR)
+KERNEL_DIR := ../../../..
+UNIT_TEST_DIR := $(ROOT_DIR)/app
+
+V ?= @
+.SECONDARY:
+
+# Pull environment (include paths, cpp/c flags) from top-level Makefile
+LINUXINCLUDE_STRING    = $(shell make -C $(KERNEL_DIR) -qp | grep "^LINUXINCLUDE :=")
+KBUILD_CPPFLAGS_STRING = $(shell make -C $(KERNEL_DIR) -qp | grep "^KBUILD_CPPFLAGS :=")
+KBUILD_CFLAGS_STRING   = $(shell make -C $(KERNEL_DIR) -qp | grep "^KBUILD_CFLAGS :=")
+
+$(eval $(subst ./,$(KERNEL_DIR)/,$(LINUXINCLUDE_STRING)))
+$(eval $(KBUILD_CPPFLAGS_STRING))
+$(eval $(KBUILD_CFLAGS_STRING))
+
+KBUILD_NAME_STRING = -DKBUILD_BASENAME='"uunit"' -DKBUILD_MODNAME='"uunit"' -DKBUILD_MODFILE='"uunit"'
+
+# Add uunit include directories
+INCLUDE_FLAGS := -I$(ROOT_DIR)/include -I$(ROOT_DIR)/build/include
+# Add kernel root to include directories, so unit tests can include kernel source directly
+INCLUDE_FLAGS := $(INCLUDE_FLAGS) -I$(KERNEL_DIR)
+# Add stubs for KBUILD defines
+INCLUDE_FLAGS := $(INCLUDE_FLAGS) $(KBUILD_NAME_STRING)
+# Include kernel build parameters
+INCLUDE_FLAGS := $(INCLUDE_FLAGS) $(LINUXINCLUDE) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS)
+# Adjust kernel parameters
+#  shadow-call-stack not available in userspace
+#  no-frame-larger-than restricts ability to put objects on stack in unit tests
+#  -O0 to enable better debuggability
+#  -D__NO_FORTIFY since fortify does not work with some compilers with -O0
+#  Don't use thunk-extern, fentry, or stack-protector-strong with unit tests
+INCLUDE_FLAGS := $(INCLUDE_FLAGS) -fno-sanitize=shadow-call-stack -Wno-frame-larger-than -O0 -D__NO_FORTIFY
+INCLUDE_FLAGS := $(subst -mindirect-branch=thunk-extern,,$(INCLUDE_FLAGS))
+INCLUDE_FLAGS := $(subst -mfunction-return=thunk-extern,,$(INCLUDE_FLAGS))
+INCLUDE_FLAGS := $(subst -fstack-protector-strong,,$(INCLUDE_FLAGS))
+INCLUDE_FLAGS := $(subst -mfentry,,$(INCLUDE_FLAGS))
+INCLUDE_FLAGS := $(subst -DCC_USING_FENTRY,,$(INCLUDE_FLAGS))
+
+LINK_FLAGS = $(INCLUDE_FLAGS) -no-pie -lcunit
+
+# The following source files are built and linked into each unit test application
+# instead of creating stubs for them.
+KERNEL_SRC = \
+	drivers/base/bus.c \
+	drivers/base/driver.c \
+	lib/kobject.c \
+	lib/klist.c \
+	lib/refcount.c \
+	lib/xarray.c \
+	lib/find_bit.c \
+	lib/hweight.c \
+	lib/radix-tree.c \
+	lib/uuid.c \
+	lib/hexdump.c \
+	lib/kstrtox.c \
+	lib/sort.c
+
+UNIT_TEST_SRC = \
+	$(UNIT_TEST_DIR)/core_hdm_ut.c \
+	$(UNIT_TEST_DIR)/core_region_ut.c
+UNIT_TEST_BIN = $(patsubst $(UNIT_TEST_DIR)/%.c,build/bin/%,$(UNIT_TEST_SRC))
+UNIT_TEST_EXEC = $(patsubst build/bin/%,exec_%,$(UNIT_TEST_BIN))
+
+all: $(UNIT_TEST_BIN)
+
+clean:
+	$(V)rm -rf build
+
+# compiler_attributes.h uses __attribute__(__error__) for static asserts - this depends
+# on the compiler optimizing away the function, so for unit test builds with -O0 we
+# create a new version of that header to ensure the attribute doesn't get used.
+MODIFIED_HEADERS = build/include/linux/compiler_attributes.h
+
+# We will disable some configuration settings for unit tests, to simplify the unit test
+# environment.
+MODIFIED_HEADERS += build/include/generated/autoconf.h
+
+# WARN_FLAGS generates constraint errors on x86. So we will modify bug.h slightly to
+# workaround this problem.
+MODIFIED_HEADERS += build/include/asm-generic/bug.h
+
+build/include/linux/compiler_attributes.h: $(KERNEL_DIR)/include/linux/compiler_attributes.h
+	$(V)echo [HEADER] $(subst build/,,$@)
+	$(V)mkdir -p $(dir $@)
+	$(V)sed 's/__has_attribute(__error__)/& \&\& defined(__OPTIMIZE__)/' $< > $@
+
+build/include/generated/autoconf.h: $(KERNEL_DIR)/include/generated/autoconf.h
+	$(V)echo [HEADER] $(subst build/,,$@)
+	$(V)mkdir -p $(dir $@)
+	$(V)cp $< $@
+	$(V)sed -i 's/#define CONFIG_DYNAMIC_DEBUG 1//' $@
+	$(V)sed -i 's/#define CONFIG_DYNAMIC_DEBUG_CORE 1//' $@
+	$(V)sed -i 's/#define CONFIG_HAVE_FENTRY 1//' $@
+	$(V)sed -i 's/#define CONFIG_LIST_HARDENED 1//' $@
+	$(V)sed -i 's/#define CONFIG_PREEMPTION 1//' $@
+	$(V)sed -i 's/#define CONFIG_PREEMPT_DYNAMIC 1//' $@
+	$(V)sed -i 's/#define CONFIG_DEBUG_PREEMPT 1//' $@
+
+build/include/asm-generic/bug.h: $(KERNEL_DIR)/include/asm-generic/bug.h
+	$(V)echo [HEADER] $(subst build/,,$@)
+	$(V)mkdir -p $(dir $@)
+	$(V)cp $< $@
+	$(V)sed -i 's/#ifndef __WARN_FLAGS/#undef __WARN_FLAGS\n&/' $@
+
+# Kernel source cannot be compiled directly, it defines functions with slightly different
+# signatures than same stdlib functions. So we build them with preprocessor wrappers that
+# re-define offending functions to something non-stdlib.
+WRAPPED_KERNEL_SRC = $(patsubst %.c,build/wrap/%.c,$(KERNEL_SRC))
+WRAPPED_KERNEL_OBJ = $(subst .c,.o,$(WRAPPED_KERNEL_SRC))
+
+build/wrap/%.c: $(MODIFIED_HEADERS) $(KERNEL_DIR)/%.c
+	$(V)echo [SRC] $(subst build/,,$@)
+	$(V)mkdir -p $(dir $@)
+	$(V)echo \#include \"pre.h\" > $@
+	$(V)echo \#include \"$(KERNEL_DIR)/$*.c\" >> $@
+	$(V)echo \#include \"post.h\" >> $@
+
+build/wrap/%.o: build/wrap/%.c Makefile
+	$(V)echo [OBJ] $(subst build/,,$@)
+	$(V)$(CC) -o $@ -c $< $(INCLUDE_FLAGS)
+
+# Unit test applications include cxl source directly, which requires internal cxl headers.
+build/%_ut.o: INCLUDE_FLAGS += -I$(KERNEL_DIR)/drivers/cxl
+
+build/obj/%_ut.o: $(UNIT_TEST_DIR)/%_ut.c $(ROOT_DIR)/stub.c $(MODIFIED_HEADERS)  Makefile
+	$(V)echo [OBJ] $(subst build/,,$@)
+	$(V)mkdir -p $(dir $@)
+	$(V)$(CC) -o $@ -c $< $(INCLUDE_FLAGS) -DMODULE
+
+build/bin/%_ut: $(WRAPPED_KERNEL_OBJ) build/obj/%_ut.o
+	$(V)echo [LINK] $(subst build/,,$@)
+	$(V)mkdir -p $(dir $@)
+	$(V)$(CC) -o $@ $^ $(LINK_FLAGS)
+
+exec_%: build/bin/%
+	$(V)echo [RUN] $(subst build/,,$^)
+	$(V)$^
+
+runtests: $(UNIT_TEST_EXEC)
diff --git a/tools/testing/cxl/uunit/app/core_hdm_ut.c b/tools/testing/cxl/uunit/app/core_hdm_ut.c
new file mode 100644
index 000000000000..cc2ff52d916c
--- /dev/null
+++ b/tools/testing/cxl/uunit/app/core_hdm_ut.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "pre.h"
+#include "drivers/cxl/core/hdm.c"
+#include "post.h"
+#include "../stub.c"
+
+#include <CUnit/Basic.h>
+#include <stdlib.h>
+
+static void
+add_hdm_decoder_test(void)
+{
+	struct cxl_port port;
+	struct cxl_decoder decoder;
+	int targets[8];
+
+	CU_ASSERT(add_hdm_decoder(&port, &decoder, targets) == 0);
+}
+
+static void
+cxl_settle_decoders_test(void)
+{
+	struct cxl_hdm hdm;
+	char _regs[0x200];
+	void *regs = _regs;
+	unsigned int msecs;
+
+	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(0)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
+	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(1)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
+	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(2)] = 0;
+
+	hdm.regs.hdm_decoder = regs;
+
+	/*
+	 * With just one decoder, we should not see a delay, since all decoders report they
+	 * are committed.
+	 */
+	hdm.decoder_count = 1;
+	msecs = g_msecs;
+	cxl_settle_decoders(&hdm);
+	CU_ASSERT(g_msecs == msecs);
+
+	/*
+	 * With two decoders, we should not see a delay, since all decoders report they
+	 * are committed.
+	 */
+	hdm.decoder_count = 2;
+	msecs = g_msecs;
+	cxl_settle_decoders(&hdm);
+	CU_ASSERT(g_msecs == msecs);
+
+	/*
+	 * With three decoders, we should see a delay, since not all decoders report they
+	 * are committed. Check that the delay is at least as big as the spec defined
+	 * 10ms commit timeout (CXL 2.0 8.2.5.12.20)..
+	 */
+	hdm.decoder_count = 3;
+	msecs = g_msecs;
+	cxl_settle_decoders(&hdm);
+	CU_ASSERT(g_msecs >= msecs + 10);
+}
+
+int
+main(int argc, char **argv)
+{
+	CU_pSuite suite = NULL;
+	unsigned int num_failures;
+
+	CU_set_error_action(CUEA_ABORT);
+	CU_initialize_registry();
+
+	suite = CU_add_suite("app_suite", NULL, NULL);
+	CU_ADD_TEST(suite, add_hdm_decoder_test);
+	CU_ADD_TEST(suite, cxl_settle_decoders_test);
+
+	CU_basic_set_mode(CU_BRM_VERBOSE);
+	CU_basic_run_tests();
+	num_failures = CU_get_number_of_failures();
+	CU_cleanup_registry();
+
+	return num_failures;
+}
diff --git a/tools/testing/cxl/uunit/app/core_region_ut.c b/tools/testing/cxl/uunit/app/core_region_ut.c
new file mode 100644
index 000000000000..770ec200c87f
--- /dev/null
+++ b/tools/testing/cxl/uunit/app/core_region_ut.c
@@ -0,0 +1,237 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "pre.h"
+#include "drivers/cxl/core/region.c"
+#include "post.h"
+#include "../stub.c"
+
+#include <CUnit/Basic.h>
+#include <stdlib.h>
+
+DECLARE_RWSEM(cxl_dpa_rwsem);
+
+static void
+uuid_show_test(void)
+{
+	struct cxl_region region = { .dev.type = &cxl_region_type };
+	char buf[128];
+
+	region.mode = CXL_DECODER_RAM;
+	CU_ASSERT(uuid_show(&region.dev, NULL, buf) == 1);
+}
+
+static void
+is_cxl_pmem_region_test(void)
+{
+	struct device dev;
+
+	dev.type = &cxl_pmem_region_type;
+	CU_ASSERT(is_cxl_pmem_region(&dev) == true);
+
+	dev.type = &cxl_dax_region_type;
+	CU_ASSERT(is_cxl_pmem_region(&dev) == false);
+}
+
+const struct device_type dummy_region_type = {
+	.name = "dummy"
+};
+
+static void
+is_dup_test(void)
+{
+	struct device dummy = { .type = &dummy_region_type };
+	struct cxl_region region = { .dev.type = &cxl_region_type };
+	uuid_t uuid = { 0 };
+
+	/* non-region devices should always return 0 */
+	CU_ASSERT(is_dup(&dummy, NULL) == 0);
+
+	/*
+	 * uuid matches, indicates the specified uuid duplicates
+	 * the uuid for an existing region
+	 * return -EBUSY
+	 */
+	CU_ASSERT(is_dup(&region.dev, &uuid) == -EBUSY);
+
+	/*
+	 * uuid does not match
+	 */
+	uuid.b[0] = 1;
+	CU_ASSERT(is_dup(&region.dev, &uuid) == 0);
+}
+
+static void
+interleave_ways_store_test(void)
+{
+	struct cxl_region *region = calloc(1, sizeof(*region));
+	const char *str0 = "0";
+	const char *str1 = "1";
+	const char *str16 = "16";
+	char buf[32];
+	struct cxl_root_decoder *root_decoder = calloc(1, sizeof(*root_decoder));
+
+	region->dev.type = &cxl_region_type;
+	region->dev.parent = &root_decoder->cxlsd.cxld.dev;
+	root_decoder->cxlsd.cxld.interleave_ways = 1;
+	region->params.interleave_ways = 0xFF;
+
+	CU_ASSERT(interleave_ways_store(&region->dev, NULL, str0, strlen(str0)) < 0);
+	CU_ASSERT(region->params.interleave_ways == 0xFF);
+
+	CU_ASSERT(interleave_ways_store(&region->dev, NULL, str1, strlen(str1)) == strlen(str1));
+	CU_ASSERT(region->params.interleave_ways == 1);
+	/* interleave_ways_show appends a newline to the value string */
+	CU_ASSERT(interleave_ways_show(&region->dev, NULL, buf) == strlen(str1) + 1);
+	CU_ASSERT(strncmp(buf, str1, strlen(str1)) == 0);
+
+	region->params.interleave_ways = 0xFF;
+	CU_ASSERT(interleave_ways_store(&region->dev, NULL, str16, strlen(str16)) == strlen(str16));
+	CU_ASSERT(region->params.interleave_ways == 16);
+	/* interleave_ways_show appends a newline to the value string */
+	CU_ASSERT(interleave_ways_show(&region->dev, NULL, buf) == strlen(str16) + 1);
+	CU_ASSERT(strncmp(buf, str16, strlen(str16)) == 0);
+
+	free(root_decoder);
+	free(region);
+}
+
+static void
+cxl_port_setup_targets_test(void)
+{
+	struct cxl_port			port;
+	struct cxl_region		region;
+	struct cxl_endpoint_decoder	ep_decoder, ep_decoder2;
+	struct cxl_port			ep_decoder_port, ep_decoder_port2;
+	struct cxl_region_ref		region_ref;
+	struct cxl_port			parent_port;
+	struct cxl_switch_decoder	switch_decoder;
+	struct cxl_root_decoder		root_decoder;
+	struct resource			resource;
+	struct cxl_memdev		memdev, memdev2;
+	struct cxl_ep			ep, ep2;
+	struct cxl_dport		dport;
+
+	ep_decoder.cxld.dev.parent = &ep_decoder_port.dev;
+	ep_decoder.pos = 0;
+	xa_init(&port.regions);
+	radix_tree_init();
+	CU_ASSERT(xa_insert(&port.regions, (unsigned long)&region, &region_ref, GFP_KERNEL) == 0);
+	region_ref.nr_targets = 2;
+	port.dev.parent = &parent_port.dev;
+	region_ref.region = &region;
+	region_ref.port = &port;
+	region_ref.decoder = &switch_decoder.cxld;
+	region.dev.parent = &root_decoder.cxlsd.cxld.dev;
+	region.params.interleave_granularity = 256;
+	root_decoder.cxlsd.cxld.interleave_ways = 2;
+	switch_decoder.nr_targets = 2;
+	resource.start = 0x1000000000;
+	resource.end =   0x2000000000;
+	region.params.res = &resource;
+	region.params.nr_targets = 2;
+	region.params.targets[0] = &ep_decoder;
+	region.params.targets[1] = &ep_decoder2;
+	ep_decoder_port.uport_dev = &memdev.dev;
+	xa_init(&port.endpoints);
+	ep.dport = &dport;
+	CU_ASSERT(xa_insert(&port.endpoints, (unsigned long)&memdev, &ep, GFP_KERNEL) == 0);
+	CU_ASSERT(cxl_port_setup_targets(&port, &region, &ep_decoder) == 0);
+	CU_ASSERT(region_ref.nr_targets_set == 1);
+	CU_ASSERT(switch_decoder.target[0] == &dport);
+
+	ep_decoder2.pos = 1;
+	ep_decoder2.cxld.dev.parent = &ep_decoder_port2.dev;
+	ep_decoder_port2.uport_dev = &memdev2.dev;
+	ep2.dport = &dport;
+	CU_ASSERT(xa_insert(&port.endpoints, (unsigned long)&memdev2, &ep2, GFP_KERNEL) == 0);
+	CU_ASSERT(cxl_port_setup_targets(&port, &region, &ep_decoder2) == 0);
+	CU_ASSERT(region_ref.nr_targets_set == 1);
+	CU_ASSERT(switch_decoder.target[0] == &dport);
+	CU_ASSERT(switch_decoder.target[1] == NULL);
+}
+
+static void
+cxl_region_setup_targets_test(void)
+{
+	struct cxl_port			port;
+	struct cxl_region		region;
+	struct cxl_endpoint_decoder	ep_decoder, ep_decoder2;
+	struct cxl_port			ep_decoder_port, ep_decoder_port2;
+	struct cxl_region_ref		region_ref;
+	struct cxl_port			parent_port;
+	struct cxl_switch_decoder	switch_decoder;
+	struct cxl_root_decoder		root_decoder;
+	struct resource			resource;
+	struct cxl_memdev		memdev, memdev2;
+	struct cxl_ep			ep, ep2;
+	struct cxl_dport		dport;
+	struct cxl_dev_state		dev_state, dev_state2;
+
+	ep_decoder.cxld.dev.parent = &ep_decoder_port.dev;
+	ep_decoder.pos = 0;
+	xa_init(&port.regions);
+	radix_tree_init();
+	CU_ASSERT(xa_insert(&port.regions, (unsigned long)&region, &region_ref, GFP_KERNEL) == 0);
+	region_ref.nr_targets = 2;
+	port.dev.parent = &parent_port.dev;
+	region_ref.region = &region;
+	region_ref.port = &port;
+	region_ref.decoder = &switch_decoder.cxld;
+	region.dev.parent = &root_decoder.cxlsd.cxld.dev;
+	region.params.interleave_granularity = 256;
+	root_decoder.cxlsd.cxld.interleave_ways = 2;
+	switch_decoder.nr_targets = 2;
+	resource.start = 0x1000000000;
+	resource.end =   0x2000000000;
+	region.params.res = &resource;
+	region.params.nr_targets = 2;
+	region.params.targets[0] = &ep_decoder;
+	region.params.targets[1] = &ep_decoder2;
+	ep_decoder_port.uport_dev = &memdev.dev;
+	ep_decoder_port.dev.parent = &port.dev;
+	ep_decoder_port2.dev.parent = &port.dev;
+	memdev.cxlds = &dev_state;
+	memdev2.cxlds = &dev_state2;
+	dev_state.rcd = 0;
+	dev_state2.rcd = 0;
+	xa_init(&port.endpoints);
+	ep.dport = &dport;
+
+	ep_decoder2.pos = 1;
+	ep_decoder2.cxld.dev.parent = &ep_decoder_port2.dev;
+	ep_decoder_port2.uport_dev = &memdev2.dev;
+	ep2.dport = &dport;
+
+	CU_ASSERT(xa_insert(&port.endpoints, (unsigned long)&memdev, &ep, GFP_KERNEL) == 0);
+	CU_ASSERT(xa_insert(&port.endpoints, (unsigned long)&memdev2, &ep2, GFP_KERNEL) == 0);
+
+	CU_ASSERT(cxl_region_setup_targets(&region) == 0);
+
+	CU_ASSERT(region_ref.nr_targets_set == 1);
+	CU_ASSERT(switch_decoder.target[0] == &dport);
+	CU_ASSERT(switch_decoder.target[1] == NULL);
+}
+
+int
+main(int argc, char **argv)
+{
+	CU_pSuite suite = NULL;
+	unsigned int num_failures;
+
+	CU_set_error_action(CUEA_ABORT);
+	CU_initialize_registry();
+
+	suite = CU_add_suite("app_suite", NULL, NULL);
+	CU_ADD_TEST(suite, uuid_show_test);
+	CU_ADD_TEST(suite, is_dup_test);
+	CU_ADD_TEST(suite, is_cxl_pmem_region_test);
+	CU_ADD_TEST(suite, interleave_ways_store_test);
+	CU_ADD_TEST(suite, cxl_port_setup_targets_test);
+	CU_ADD_TEST(suite, cxl_region_setup_targets_test);
+
+	CU_basic_set_mode(CU_BRM_VERBOSE);
+	CU_basic_run_tests();
+	num_failures = CU_get_number_of_failures();
+	CU_cleanup_registry();
+
+	return num_failures;
+}
diff --git a/tools/testing/cxl/uunit/include/post.h b/tools/testing/cxl/uunit/include/post.h
new file mode 100644
index 000000000000..231f5bc15c3a
--- /dev/null
+++ b/tools/testing/cxl/uunit/include/post.h
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+#undef strncpy
+#undef strcpy
+#undef strcat
+#undef strncat
+#undef strlcat
+#undef strlcpy
+#undef errno
+#undef loff_t
+#undef dev_t
+#undef timer_t
+#undef int64_t
+#undef u_int64_t
+#undef sigset_t
+#undef fd_set
+#undef blkcnt_t
+#undef nlink_t
+#undef ffs
+
+#undef abs
+#undef __alloc_size__
diff --git a/tools/testing/cxl/uunit/include/pre.h b/tools/testing/cxl/uunit/include/pre.h
new file mode 100644
index 000000000000..1eb88e51740f
--- /dev/null
+++ b/tools/testing/cxl/uunit/include/pre.h
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+#define strncpy		__kstrncpy
+#define strcpy		__kstrcpy
+#define strcat		__kstrcat
+#define strncat		__kstrncat
+#define strlcat		__kstrlcat
+#define strlcpy		__kstrlcpy
+#define errno		__kerrno
+#define loff_t		__kloff_t
+#define dev_t		__kdev_t
+#define timer_t		__ktimer_t
+#define int64_t		__kint64_t
+#define u_int64_t	__ku_int64_t
+#define sigset_t	__ksigset_t
+#define fd_set		__kfd_set
+#define blkcnt_t	__kblkcnt_t
+#define nlink_t		__knlink_t
+
+#define TRACE_EVENT(name, ...) static inline void trace_##name(...) {}
+#define WARN(lvl, ...) printk(__VA_ARGS__)
+#define WARN_ON_ONCE(cond) ({if(cond)printk(#cond);cond;})
+#define __DISABLE_TRACE_MMIO__
diff --git a/tools/testing/cxl/uunit/stub.c b/tools/testing/cxl/uunit/stub.c
new file mode 100644
index 000000000000..208dab1e7d6d
--- /dev/null
+++ b/tools/testing/cxl/uunit/stub.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "pre.h"
+#include "linux/cpuhotplug.h"
+#include "linux/rwsem.h"
+#include "linux/kobject.h"
+#include "linux/device/bus.h"
+#include "drivers/cxl/cxl.h"
+#include "post.h"
+
+#include <string.h>
+#include <stdlib.h>
+
+/* All modules require one of these. */
+struct module __this_module;
+
+/*
+ * All stubs must be place in a section for the associated kernel source file that defines it.
+ * All sections must be sorted by pathname.
+ */
+
+/*
+ * arch
+ * These cannot be mapped to a specific file, since archs are free to define them wherever they
+ * want in their arch directory.
+ */
+void __udelay(unsigned long usecs) { }
+unsigned long __stack_chk_guard;
+#ifdef CONFIG_ARCH_HAS_CPU_CACHE_INVALIDATE_MEMREGION
+int cpu_cache_invalidate_memregion(int res_desc) { return 0; }
+bool cpu_cache_has_invalidate_memregion(void) { return true; }
+#endif
+
+#ifdef CONFIG_ARM64
+/* arch/arm64/kernel/alternative.c */
+void alt_cb_patch_nops(struct alt_instr *alt, __le32 *origptr, __le32 *updptr, int nr_inst) {}
+
+/* arch/arm64/lib/copy_from_user.S */
+unsigned long __arch_copy_from_user(void *to, const void *from, unsigned long n) { return n; }
+DECLARE_BITMAP(system_cpucaps, ARM64_NCAPS);
+#endif
+
+#ifdef CONFIG_X86_64
+/* arch/x86/kernel/cpu/common.c */
+struct pcpu_hot pcpu_hot;
+
+/* arch/x86/kernel/setup_percpu.c */
+unsigned long this_cpu_off;
+#endif
+
+/* drivers/base/core.c */
+int device_add(struct device *dev) { return 0; }
+struct device *get_device(struct device *dev) { return NULL; }
+void put_device(struct device *dev) { }
+void device_unregister(struct device *dev) { }
+int device_for_each_child(struct device *dev, void *data,
+		int (*fn)(struct device *dev, void *data)) { return 0; }
+struct device *device_find_child(struct device *dev, void *data,
+		int (*match)(struct device *dev, void *data)) { return NULL; }
+struct device *device_find_child_by_name(struct device *parent, const char *name)
+{ return NULL; }
+const char *dev_driver_string(const struct device *dev) { return NULL; }
+void device_del(struct device *dev) { }
+int device_match_name(struct device *dev, const void *name) { return 0; }
+void device_initialize(struct device *dev) { }
+int dev_set_name(struct device *dev, const char* name, ...) { return 0; }
+void device_remove_groups(struct device *dev, const struct attribute_group **groups) { }
+int device_register(struct device *dev) { return 0; }
+int device_add_groups(struct device *dev, const struct attribute_group **groups) { return 0; }
+struct kobject *virtual_device_parent(struct device *dev) { return NULL; }
+struct kset *devices_kset;
+
+/* drivers/base/dd.c */
+int device_attach(struct device *dev) { return 0; }
+void device_release_driver(struct device *dev) { }
+int device_driver_attach(struct device_driver *drv, struct device *dev) { return 0; }
+void device_driver_detach(struct device *dev) { }
+int driver_attach(struct device_driver *drv) { return 0; }
+void driver_detach(struct device_driver *drv) { }
+void device_initial_probe(struct device *dev) { }
+void deferred_probe_extend_timeout(void) { }
+
+/* drivers/base/devres.c */
+int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name) { return 0; }
+void devm_release_action(struct device *dev, void (*action)(void *), void *data) { }
+void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) { return NULL; }
+void devm_remove_action(struct device *dev, void (*action)(void *), void *data) { }
+
+/* drivers/base/module.c */
+void module_add_driver(struct module *mod, struct device_driver *drv) { }
+void module_remove_driver(struct device_driver *drv) { }
+
+/* drivers/base/power/runtime.c */
+int __pm_runtime_idle(struct device *dev, int rpmflags) { return 0; }
+
+/* drivers/char/random.c */
+void get_random_bytes(void *buf, size_t len) { }
+
+/* drivers/cxl/core/mbox.c */
+int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
+		struct cxl_region *cxlr) { return 0; }
+
+/* drivers/cxl/core/memdev.c */
+bool is_cxl_memdev(const struct device *dev) { return false; }
+
+/* drivers/cxl/core/pmem.c */
+struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd) { return NULL; }
+
+/* drivers/cxl/core/port.c */
+struct attribute_group cxl_base_attribute_group;
+struct bus_type cxl_bus_type;
+DECLARE_RWSEM(cxl_region_rwsem);
+struct cxl_port *to_cxl_port(const struct device *dev)
+{
+	return container_of(dev, struct cxl_port, dev);
+}
+struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev)
+{
+	return container_of(dev, struct cxl_root_decoder, cxlsd.cxld.dev);
+}
+struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev)
+{
+	return container_of(dev, struct cxl_switch_decoder, cxld.dev);
+}
+struct cxl_endpoint_decoder *to_cxl_endpoint_decoder(struct device *dev) { return NULL; }
+struct cxl_decoder *to_cxl_decoder(struct device *dev) { return NULL; }
+bool is_endpoint_decoder(struct device *dev) { return true; }
+bool is_root_decoder(struct device *dev) { return true; }
+bool is_switch_decoder(struct device *dev) { return true; }
+int __cxl_driver_register(struct cxl_driver *cxl_drv, struct module *owner,
+		const char *modname) { return 0; }
+void cxl_driver_unregister(struct cxl_driver *cxl_drv) { }
+int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map) { return 0; }
+int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld) { return 0; }
+struct cxl_switch_decoder *cxl_switch_decoder_alloc(struct cxl_port *port,
+						    unsigned int nr_targets) { return NULL; }
+struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port) { return NULL; }
+int cxl_num_decoders_committed(struct cxl_port *port) { return 0; }
+
+/* drivers/cxl/core/regs.c */
+int cxl_map_component_regs(const struct cxl_register_map *map, struct cxl_component_regs *regs,
+			   unsigned long map_mask) { return 0; }
+void cxl_probe_component_regs(struct device *dev, void *base, struct cxl_component_reg_map *map) { }
+
+/* drivers/irqchip/irq-gic-v3.c */
+struct static_key_false gic_nonsecure_priorities;
+
+/* fs/kernfs/dir.c */
+void kernfs_get(struct kernfs_node *kn) { }
+void kernfs_put(struct kernfs_node *kn) { }
+
+/* fs/seq_file.c */
+void seq_printf(struct seq_file *m, const char *f, ...) { }
+
+/* fs/sysfs/file.c */
+int sysfs_create_file_ns(struct kobject *kobj, const struct attribute *attr, const void *ns) { return 0; }
+void sysfs_remove_file_ns(struct kobject *kobj, const struct attribute *attr, const void *ns) { }
+int sysfs_create_dir_ns(struct kobject *kobj, const void *ns) { return 0; }
+int sysfs_move_dir_ns(struct kobject *kobj, struct kobject *new_parent_kobj, const void *new_ns) { return 0; }
+int sysfs_rename_dir_ns(struct kobject *kobj, const char *new_name, const void *new_ns) { return 0; }
+int sysfs_create_groups(struct kobject *kobj, const struct attribute_group **groups) { return 0; }
+void sysfs_remove_groups(struct kobject *kobj, const struct attribute_group **groups) { }
+int sysfs_update_group(struct kobject *kobj, const struct attribute_group *grp) { return 0; }
+int sysfs_create_link(struct kobject *kobj, struct kobject *target, const char *name) { return 0; }
+void sysfs_remove_link(struct kobject *kobj, const char *name) { }
+void sysfs_remove_dir(struct kobject *kobj) { }
+int sysfs_emit(char *buf, const char *fmt, ...)
+{
+	va_list argptr;
+	int rc;
+
+	va_start(argptr, fmt);
+	rc = vsprintf(buf, fmt, argptr);
+	va_end(argptr);
+
+	return rc;
+}
+bool sysfs_streq(const char *s1, const char *s2) { return true; }
+
+/* include linux/bitfield.h */
+/* Resolves undefined reference error when compiling at -O0 */
+void __bad_mask(void) { }
+
+/* include/linux/dev_printk.h, lib/dynamic_debug.c */
+void _dev_err(const struct device *dev, const char *fmt, ...) { }
+void _dev_warn(const struct device *dev, const char *fmt, ...) { }
+void _dev_info(const struct device *dev, const char *fmt, ...) { }
+void __dynamic_dev_dbg(struct _ddebug *descriptor, const struct device *dev,
+		const char *fmt, ...) { }
+void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...) { }
+void __warn_printk(const char *fmt, ...) { }
+int _printk(const char *fmt, ...) { return 0; }
+const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list ap) { return NULL; }
+
+/* kernel/cpu.c */
+int __cpuhp_setup_state(enum cpuhp_state state, const char *name, bool invoke,
+		int (*startup)(unsigned int cpu),
+		int (*teardown)(unsigned int cpu), bool multi_instance) { return 0; }
+
+/* kernel/locking/mutex.c */
+void mutex_lock(struct mutex *lock) { }
+void mutex_unlock(struct mutex *lock) { }
+void __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key) { }
+
+/* kernel/locking/rwsem.c */
+void __init_rwsem(struct rw_semaphore *sem, const char *name, struct lock_class_key *key) { }
+void up_read(struct rw_semaphore *sem) { }
+void down_read(struct rw_semaphore *sem) { }
+int down_read_interruptible(struct rw_semaphore *sem) { return 0; }
+void up_write(struct rw_semaphore *sem) { }
+void down_write(struct rw_semaphore *sem) { }
+int down_write_killable(struct rw_semaphore *sem) { return 0; }
+
+/* kernel/locking/spinlock.c */
+void _raw_spin_lock(raw_spinlock_t *lock) { }
+void _raw_spin_lock_irq(raw_spinlock_t *lock) { }
+void _raw_spin_lock_bh(raw_spinlock_t *lock) { }
+void _raw_spin_unlock(raw_spinlock_t *lock) { }
+void _raw_spin_unlock_irq(raw_spinlock_t *lock) { }
+void _raw_spin_unlock_bh(raw_spinlock_t *lock) { }
+unsigned long _raw_spin_lock_irqsave(raw_spinlock_t *lock) { return 0; }
+void _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags) { }
+
+/* kernel/notifier.c */
+int blocking_notifier_chain_register(struct blocking_notifier_head *nh, struct notifier_block *n) { return 0; }
+int blocking_notifier_chain_unregister(struct blocking_notifier_head *nh, struct notifier_block *n) { return 0; }
+int blocking_notifier_call_chain(struct blocking_notifier_head *nh, unsigned long val, void *v) { return 0; }
+
+/* kernel/panic.c */
+void __stack_chk_fail(void) { }
+void warn_slowpath_fmt(const char *file, const int line, unsigned taint, const char *fmt, ...) { }
+
+/* kernel/rcu/tree.c */
+void call_rcu(struct rcu_head *head, rcu_callback_t func) { }
+
+/* kernel/rcu/tree_plugin.h */
+void __rcu_read_lock(void) { }
+void __rcu_read_unlock(void) { }
+
+/* kernel/resource.c */
+int insert_resource(struct resource *parent, struct resource *new) { return 0; }
+int remove_resource(struct resource *old) { return 0; }
+struct resource *alloc_free_mem_region(struct resource *base, unsigned long size,
+		unsigned long align, const char *name) { return NULL; }
+struct resource *__request_region(struct resource *parent,
+				  resource_size_t start, resource_size_t n,
+				  const char *name, int flags) { return NULL; }
+void __release_region(struct resource *parent, resource_size_t start, resource_size_t n) { }
+int walk_iomem_res_desc(unsigned long desc, unsigned long flags, u64 start, u64 end,
+		void *arg, int (*func)(struct resource *, void *)) { return 0; }
+
+/* kernel/sched/core.c */
+void dynamic_preempt_schedule(void) { }
+int dynamic_might_resched(void) { return 0; }
+int wake_up_process(struct task_struct *p) { return 0; }
+void schedule(void) { }
+//void preempt_schedule(void) { }
+//DEFINE_STATIC_CALL(preempt_schedule, preempt_schedule);
+
+/* kernel/time/timer.c */
+unsigned int g_msecs;
+void msleep(unsigned int msecs) { g_msecs += msecs; }
+void usleep_range_state(unsigned long min, unsigned long max, unsigned int state) { }
+
+/* lib/bitmap.c */
+void __bitmap_clear(unsigned long *map, unsigned int start, int len) { }
+
+/* lib/ctype.c */
+const unsigned char _ctype[1];
+
+/* lib/dump_stack.c */
+asmlinkage void dump_stack_lvl(const char *log_lvl) { }
+
+/* lib/kobject_uevent.c */
+int kobject_uevent(struct kobject *kobj, enum kobject_action) { return 0; }
+int kobject_uevent_env(struct kobject *kobj, enum kobject_action, char *envp[]) { return 0; }
+int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count) { return 0; }
+
+/* lib/logic_iomem.c */
+#ifndef CONFIG_GENERIC_IOREMAP
+void *ioremap(phys_addr_t offset, size_t size) { return NULL; }
+#endif
+void iounmap(volatile void *addr) { }
+
+/* lib/memregion.c */
+int memregion_alloc(gfp_t gfp) { return 0; }
+void memregion_free(int id) { }
+
+/* lib/smp_processor_id.c */
+unsigned int debug_smp_processor_id(void) { return 0; }
+
+/* lib/string.c */
+char *strnchr(const char *s, size_t count, int c) { return NULL; }
+
+/* lib/string_helpers.c */
+char *strreplace(char *str, char old, char new) { return str; }
+
+/* lib/usercopy.c */
+#ifndef INLINE_COPY_FROM_USER
+unsigned long _copy_from_user(void *to, const void *from, unsigned long n) { return n; }
+#endif
+
+/* mm/maccess.c */
+void __copy_overflow(int size, unsigned long count) { }
+
+/* mm/percpu.c */
+unsigned long __per_cpu_offset[NR_CPUS];
+
+/* mm/slab_common.c */
+void *__kmalloc(size_t size, gfp_t flags) { return malloc(size); }
+void kfree(const void *objp) { free((void *)objp); }
+void kfree_const(const void *x) { free((void *)x); }
+struct kmem_cache {
+	unsigned int obj_size;
+};
+struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, unsigned int align,
+		slab_flags_t flags, void (*ctor)(void *))
+{
+	struct kmem_cache *s = calloc(1, sizeof(*s));
+	s->obj_size = size;
+	return s;
+}
+void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) { return malloc(s->obj_size); }
+void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru, gfp_t gfpflags) { return malloc(s->obj_size); }
+void kmem_cache_free(struct kmem_cache *s, void *objp) { free(objp); }
+struct kmem_cache *kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
+void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size) { return NULL; }
+
+/* mm/usercopy.c */
+void __check_object_size(const void *ptr, unsigned long n, bool to_user) { }
+
+/* mm/util.c */
+char *kstrdup(const char *s, gfp_t gfp) { return strdup(s); }
+const char *kstrdup_const(const char *s, gfp_t gfp) { return strdup(s); }
+char *kstrndup(const char *s, size_t max, gfp_t gfp) { return strndup(s, max); }
+

Re: [PATCH RFC] Add "uunit" unit testing framework for CXL code

[Davidlohr Bueso]

On Mon, 04 Dec 2023, Jim Harris wrote:

>uunit (userspace unit testing) generates wrappers for kernel source files
>which are compiled and linked with necessary function stubs and executed
>fully in userspace.
>
>Existing unit testing frameworks (kunit, tools/testing/cxl) have some
>limitations that uunit alleviates:

Looks very helpful.

>
>* mock referenced functions without requiring modifications to those functions
>* mock register accesses
>* does not require execution in a running kernel
>* no restrictions currently on CONFIG parameters
>  (Note: some CXL code cannot run in UML with kunit due to dependencies on
>  CONFIG_SPARSEMEM)
>
>This RFC contains some complex unit tests for core/region.c code, as well as
>a simple core/hdm.c test showing how register accesses can be mocked.

Documenting these tests at a high level would be useful, particularly as
more tests are added. (actually I think this is your todo #4 :)

>
>This RFC depends on CUnit, which needs to be installed on the system prior to build.
>
>To build the unit tests:
>
>  make -C tools/testing/cxl/uunit
>
>To run the unit tests:
>
>  make -C tools/testing/cxl/uunit runtests
>
>There is a balance currently between creating stubs for referenced functions,
>versus just building and linking the referenced source files directly. For
>example, CXL uses xarray extensively, so we just build and link the xarray
>code directly into unit tests rather than creating stubs for xarray. See
>KERNEL_SRC in the Makefile for which kernel source files are directly built
>and linked in this manner.

I would say that just making the thing bulkier is better than having to
have stub wrappers to a lot of core-kernel machinery. This is particularly
the case for having this under cxl-specific paths in the kernel source,
instead of a general framework as you mention.

Thanks,
Davidlohr

RE: [PATCH RFC] Add "uunit" unit testing framework for CXL code

[Dan Williams]

Jim Harris wrote:
> uunit (userspace unit testing) generates wrappers for kernel source files
> which are compiled and linked with necessary function stubs and executed
> fully in userspace.
> 
> Existing unit testing frameworks (kunit, tools/testing/cxl) have some
> limitations that uunit alleviates:
> 
> * mock referenced functions without requiring modifications to those functions
> * mock register accesses
> * does not require execution in a running kernel
> * no restrictions currently on CONFIG parameters
>   (Note: some CXL code cannot run in UML with kunit due to dependencies on
>   CONFIG_SPARSEMEM)
> 
> This RFC contains some complex unit tests for core/region.c code, as well as
> a simple core/hdm.c test showing how register accesses can be mocked.
> 
> This RFC depends on CUnit, which needs to be installed on the system prior to build.
> 
> To build the unit tests:
> 
>   make -C tools/testing/cxl/uunit
> 
> To run the unit tests:
> 
>   make -C tools/testing/cxl/uunit runtests
> 
> There is a balance currently between creating stubs for referenced functions,
> versus just building and linking the referenced source files directly. For
> example, CXL uses xarray extensively, so we just build and link the xarray
> code directly into unit tests rather than creating stubs for xarray. See
> KERNEL_SRC in the Makefile for which kernel source files are directly built
> and linked in this manner.

Explain that balance a bit more, is it mainly just a "hey I want that
function, oh building that new file is going to take me hours to work
around a bunch of new dependencies, I'll just stub the one function I
need."?

> Stubs are in uunit/stub.c, and are grouped by the kernel source file containing
> the real definition. Those groups are listed in alphabetical order.
> 
> The build process has five primary steps:
> 
> Capture kernel build flags - use 'make -qp' to pull LINUXINCLUDE,
> KBUILD_CPPFLAGS, KBUILD_CFLAGS from the top-level kernel Makefile.

Did you look into stealing from tools/build/? I don't think it includes
those variables, but I suspect it includes some Makefile helpers that
are open coded. Reusing tools/build/ likely helps with selling others on
this uunit approach.

> 
> Create unit test build environment - remove thunking, fentry and
> stack-protector options from the kernel build flags. Also provide
> KBUILD_XXX strings.
> 
> Modified headers - some kernel headers require modification - don't
> modify them inline though, create a copy in the uunit/build directory,
> modify them, and set the include directories so that the build will
> pick up the modified copy first. Explanations for the modified headers
> are in the Makefile.
> 
> Wrap and build kernel source - kernel defines many functions that are
> also in libc. Kernel also defines some data structures with same name
> as /usr/include data structures. So we wrap all kernel source files
> with uunit/include/pre.h and uunit/include/post.h to do some
> pre-processor magic to eliminate these conflicts.

How much of an ongoing maintenance burden do you expect this to be? I
guess it all depends on how fast moving those headers are, but I worry
about bitrot unless we can get this into some automated environment that
notices that new changes heading into linux-next are breaking the uunit
build.

> Build unit test applications - found in uunit/app directory, and named

NIT is "app" idiomatic for CUnit? I would otherwise just call it the
"test" directory for where the test executables end up.

> like core_region_ut.c to associate with core/region.c. The test
> applications include the stub.c file directly - this will enable future
> use cases where stubs can be configured to return specific mocked values.
> 
> Note that this framework is not CXL-specific - it could be used to unit
> test a vast majority of other kernel code. Proposal is to just start
> with this framework in tools/testing/cxl to meet CXL-specific needs.

Certainly all the pre-work you have done to process kernel headers is
generally useful work for others that want to adopt this test approach.

The scaling challenge comes when there is disagreement about
stub-behavior across tests, right? Or would a test just override the
default stub?

> 
> Todos:
> 
> 1) Integrate with tools/testing/cxl Makefile

I am ok if this stands alone. As I mentioned above reusing tools/build/
is likely more important. tools/testing/cxl/ is building a kernel
module, tools/build/ is there to support userpace executables built out
of the tools/ directory.

> 2) Break out CXL stubs separately by source file. This will be required as
>    we start testing more and more CXL files and need to exclude stubs that
>    are defined in the source file under test.

You mean like in cases where one test wants one stub behavior and
another has wants a different one.

> 3) Build common library for building up parts of the CXL topology. For example,
>    see everything needed to set up ports, decoders, etc. to test the
>    region.c code. Much of this can be simplified in a way that various
>    topologies can be built without as much boilerplate code, not only for
>    region.c but other files as well.

Do you mean de-duplicating some of the logic in
cxl_port_setup_targets_test() and cxl_region_setup_targets_test()?

> 4) Improve cxl_region tests to cover error conditions and add extensive
>    comments explaining how the topology is being built. This overlaps with #3.

Some comments on the simple tests would help too just to get folks
ramped about what's going on.

> 
> Signed-off-by: Jim Harris <jim.harris@samsung.com>
> ---
>  tools/testing/cxl/uunit/Makefile             |  141 +++++++++++
>  tools/testing/cxl/uunit/app/core_hdm_ut.c    |   82 ++++++
>  tools/testing/cxl/uunit/app/core_region_ut.c |  237 ++++++++++++++++++
>  tools/testing/cxl/uunit/include/post.h       |   21 ++
>  tools/testing/cxl/uunit/include/pre.h        |   22 ++
>  tools/testing/cxl/uunit/stub.c               |  335 ++++++++++++++++++++++++++
>  6 files changed, 838 insertions(+)
>  create mode 100644 tools/testing/cxl/uunit/Makefile
>  create mode 100644 tools/testing/cxl/uunit/app/core_hdm_ut.c
>  create mode 100644 tools/testing/cxl/uunit/app/core_region_ut.c
>  create mode 100644 tools/testing/cxl/uunit/include/post.h
>  create mode 100644 tools/testing/cxl/uunit/include/pre.h
>  create mode 100644 tools/testing/cxl/uunit/stub.c

This overall looks promising, the tools/build/ question might be the
only consideration for merging in the near term.

Some inline comments below:

[..]
> diff --git a/tools/testing/cxl/uunit/app/core_hdm_ut.c b/tools/testing/cxl/uunit/app/core_hdm_ut.c
> new file mode 100644
> index 000000000000..cc2ff52d916c
> --- /dev/null
> +++ b/tools/testing/cxl/uunit/app/core_hdm_ut.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "pre.h"
> +#include "drivers/cxl/core/hdm.c"
> +#include "post.h"
> +#include "../stub.c"
> +
> +#include <CUnit/Basic.h>
> +#include <stdlib.h>
> +
> +static void
> +add_hdm_decoder_test(void)
> +{
> +	struct cxl_port port;
> +	struct cxl_decoder decoder;
> +	int targets[8];
> +
> +	CU_ASSERT(add_hdm_decoder(&port, &decoder, targets) == 0);

Likely wants some commentary on why it is ok to pass uninitialized stack
variables into add_hdm_decoder().

> +}
> +
> +static void
> +cxl_settle_decoders_test(void)
> +{
> +	struct cxl_hdm hdm;
> +	char _regs[0x200];
> +	void *regs = _regs;
> +	unsigned int msecs;
> +
> +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(0)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
> +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(1)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
> +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(2)] = 0;

Can this be made to look more like idiomatic kernel code? E.g.:

    writel(val, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(x))

...so that code sequences can be copied from the driver into the tests?

> +
> +	hdm.regs.hdm_decoder = regs;
> +
> +	/*
> +	 * With just one decoder, we should not see a delay, since all decoders report they
> +	 * are committed.
> +	 */
> +	hdm.decoder_count = 1;
> +	msecs = g_msecs;
> +	cxl_settle_decoders(&hdm);
> +	CU_ASSERT(g_msecs == msecs);
> +
> +	/*
> +	 * With two decoders, we should not see a delay, since all decoders report they
> +	 * are committed.
> +	 */
> +	hdm.decoder_count = 2;
> +	msecs = g_msecs;
> +	cxl_settle_decoders(&hdm);
> +	CU_ASSERT(g_msecs == msecs);
> +
> +	/*
> +	 * With three decoders, we should see a delay, since not all decoders report they
> +	 * are committed. Check that the delay is at least as big as the spec defined
> +	 * 10ms commit timeout (CXL 2.0 8.2.5.12.20)..
> +	 */
> +	hdm.decoder_count = 3;
> +	msecs = g_msecs;
> +	cxl_settle_decoders(&hdm);
> +	CU_ASSERT(g_msecs >= msecs + 10);
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> +	CU_pSuite suite = NULL;
> +	unsigned int num_failures;
> +
> +	CU_set_error_action(CUEA_ABORT);
> +	CU_initialize_registry();
> +
> +	suite = CU_add_suite("app_suite", NULL, NULL);
> +	CU_ADD_TEST(suite, add_hdm_decoder_test);
> +	CU_ADD_TEST(suite, cxl_settle_decoders_test);
> +
> +	CU_basic_set_mode(CU_BRM_VERBOSE);
> +	CU_basic_run_tests();
> +	num_failures = CU_get_number_of_failures();
> +	CU_cleanup_registry();
> +
> +	return num_failures;
> +}
> diff --git a/tools/testing/cxl/uunit/app/core_region_ut.c b/tools/testing/cxl/uunit/app/core_region_ut.c
> new file mode 100644
> index 000000000000..770ec200c87f
> --- /dev/null
> +++ b/tools/testing/cxl/uunit/app/core_region_ut.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "pre.h"
> +#include "drivers/cxl/core/region.c"
> +#include "post.h"
> +#include "../stub.c"
> +
> +#include <CUnit/Basic.h>
> +#include <stdlib.h>
> +
> +DECLARE_RWSEM(cxl_dpa_rwsem);

This is here because core_region_ut.c is not allowed to use stubs
core_hdm_ut.c?

> +
> +static void
> +uuid_show_test(void)
> +{
> +	struct cxl_region region = { .dev.type = &cxl_region_type };
> +	char buf[128];
> +
> +	region.mode = CXL_DECODER_RAM;
> +	CU_ASSERT(uuid_show(&region.dev, NULL, buf) == 1);
> +}
> +
> +static void
> +is_cxl_pmem_region_test(void)
> +{
> +	struct device dev;
> +
> +	dev.type = &cxl_pmem_region_type;
> +	CU_ASSERT(is_cxl_pmem_region(&dev) == true);
> +
> +	dev.type = &cxl_dax_region_type;
> +	CU_ASSERT(is_cxl_pmem_region(&dev) == false);
> +}
> +
> +const struct device_type dummy_region_type = {
> +	.name = "dummy"
> +};
> +
> +static void
> +is_dup_test(void)
> +{
> +	struct device dummy = { .type = &dummy_region_type };
> +	struct cxl_region region = { .dev.type = &cxl_region_type };
> +	uuid_t uuid = { 0 };
> +
> +	/* non-region devices should always return 0 */
> +	CU_ASSERT(is_dup(&dummy, NULL) == 0);
> +
> +	/*
> +	 * uuid matches, indicates the specified uuid duplicates
> +	 * the uuid for an existing region
> +	 * return -EBUSY
> +	 */
> +	CU_ASSERT(is_dup(&region.dev, &uuid) == -EBUSY);
> +
> +	/*
> +	 * uuid does not match
> +	 */
> +	uuid.b[0] = 1;
> +	CU_ASSERT(is_dup(&region.dev, &uuid) == 0);
> +}
> +
> +static void
> +interleave_ways_store_test(void)
> +{
> +	struct cxl_region *region = calloc(1, sizeof(*region));
> +	const char *str0 = "0";
> +	const char *str1 = "1";
> +	const char *str16 = "16";
> +	char buf[32];
> +	struct cxl_root_decoder *root_decoder = calloc(1, sizeof(*root_decoder));
> +
> +	region->dev.type = &cxl_region_type;
> +	region->dev.parent = &root_decoder->cxlsd.cxld.dev;
> +	root_decoder->cxlsd.cxld.interleave_ways = 1;
> +	region->params.interleave_ways = 0xFF;
> +
> +	CU_ASSERT(interleave_ways_store(&region->dev, NULL, str0, strlen(str0)) < 0);
> +	CU_ASSERT(region->params.interleave_ways == 0xFF);
> +
> +	CU_ASSERT(interleave_ways_store(&region->dev, NULL, str1, strlen(str1)) == strlen(str1));
> +	CU_ASSERT(region->params.interleave_ways == 1);
> +	/* interleave_ways_show appends a newline to the value string */
> +	CU_ASSERT(interleave_ways_show(&region->dev, NULL, buf) == strlen(str1) + 1);
> +	CU_ASSERT(strncmp(buf, str1, strlen(str1)) == 0);
> +
> +	region->params.interleave_ways = 0xFF;
> +	CU_ASSERT(interleave_ways_store(&region->dev, NULL, str16, strlen(str16)) == strlen(str16));
> +	CU_ASSERT(region->params.interleave_ways == 16);
> +	/* interleave_ways_show appends a newline to the value string */
> +	CU_ASSERT(interleave_ways_show(&region->dev, NULL, buf) == strlen(str16) + 1);
> +	CU_ASSERT(strncmp(buf, str16, strlen(str16)) == 0);
> +
> +	free(root_decoder);
> +	free(region);
> +}
> +
> +static void
> +cxl_port_setup_targets_test(void)
> +{
> +	struct cxl_port			port;
> +	struct cxl_region		region;
> +	struct cxl_endpoint_decoder	ep_decoder, ep_decoder2;
> +	struct cxl_port			ep_decoder_port, ep_decoder_port2;
> +	struct cxl_region_ref		region_ref;
> +	struct cxl_port			parent_port;
> +	struct cxl_switch_decoder	switch_decoder;
> +	struct cxl_root_decoder		root_decoder;
> +	struct resource			resource;
> +	struct cxl_memdev		memdev, memdev2;
> +	struct cxl_ep			ep, ep2;
> +	struct cxl_dport		dport;
> +
> +	ep_decoder.cxld.dev.parent = &ep_decoder_port.dev;
> +	ep_decoder.pos = 0;
> +	xa_init(&port.regions);
> +	radix_tree_init();
> +	CU_ASSERT(xa_insert(&port.regions, (unsigned long)&region, &region_ref, GFP_KERNEL) == 0);
> +	region_ref.nr_targets = 2;
> +	port.dev.parent = &parent_port.dev;
> +	region_ref.region = &region;
> +	region_ref.port = &port;
> +	region_ref.decoder = &switch_decoder.cxld;
> +	region.dev.parent = &root_decoder.cxlsd.cxld.dev;
> +	region.params.interleave_granularity = 256;
> +	root_decoder.cxlsd.cxld.interleave_ways = 2;
> +	switch_decoder.nr_targets = 2;
> +	resource.start = 0x1000000000;
> +	resource.end =   0x2000000000;

Spotted this just because of the recent debug adventure here:

http://lore.kernel.org/r/6595c10982cf_8dc6829433@dwillia2-xfh.jf.intel.com.notmuch

...where that resource.end should be the exclusive "end" of 0x1fffffffff

> +	region.params.res = &resource;
> +	region.params.nr_targets = 2;
> +	region.params.targets[0] = &ep_decoder;
> +	region.params.targets[1] = &ep_decoder2;
> +	ep_decoder_port.uport_dev = &memdev.dev;
> +	xa_init(&port.endpoints);
> +	ep.dport = &dport;
> +	CU_ASSERT(xa_insert(&port.endpoints, (unsigned long)&memdev, &ep, GFP_KERNEL) == 0);
> +	CU_ASSERT(cxl_port_setup_targets(&port, &region, &ep_decoder) == 0);
> +	CU_ASSERT(region_ref.nr_targets_set == 1);
> +	CU_ASSERT(switch_decoder.target[0] == &dport);
> +
> +	ep_decoder2.pos = 1;
> +	ep_decoder2.cxld.dev.parent = &ep_decoder_port2.dev;
> +	ep_decoder_port2.uport_dev = &memdev2.dev;
> +	ep2.dport = &dport;
> +	CU_ASSERT(xa_insert(&port.endpoints, (unsigned long)&memdev2, &ep2, GFP_KERNEL) == 0);
> +	CU_ASSERT(cxl_port_setup_targets(&port, &region, &ep_decoder2) == 0);
> +	CU_ASSERT(region_ref.nr_targets_set == 1);
> +	CU_ASSERT(switch_decoder.target[0] == &dport);
> +	CU_ASSERT(switch_decoder.target[1] == NULL);
> +}
> +
[..]
> diff --git a/tools/testing/cxl/uunit/include/post.h b/tools/testing/cxl/uunit/include/post.h
> new file mode 100644
> index 000000000000..231f5bc15c3a
> --- /dev/null
> +++ b/tools/testing/cxl/uunit/include/post.h
[..]
> diff --git a/tools/testing/cxl/uunit/include/pre.h b/tools/testing/cxl/uunit/include/pre.h
> new file mode 100644
> index 000000000000..1eb88e51740f
> --- /dev/null
> +++ b/tools/testing/cxl/uunit/include/pre.h

These last 2 files look like a lot of hard fought meticulous magic,
hence the question above about expected ongoing maintenance burden.

> diff --git a/tools/testing/cxl/uunit/stub.c b/tools/testing/cxl/uunit/stub.c
> new file mode 100644
> index 000000000000..208dab1e7d6d
> --- /dev/null
> +++ b/tools/testing/cxl/uunit/stub.c

It might be worthwhile to let clang-format do a once over on this and all
the other files if only to keep the automated coding-style bots from
tripping on these files.

Re: [PATCH RFC] Add "uunit" unit testing framework for CXL code

[Jim Harris]

On Wed, Jan 03, 2024 at 10:37:09PM -0800, Dan Williams wrote:

Dan,

Thanks for the detailed review!

> Jim Harris wrote:
> > 
> > There is a balance currently between creating stubs for referenced functions,
> > versus just building and linking the referenced source files directly. For
> > example, CXL uses xarray extensively, so we just build and link the xarray
> > code directly into unit tests rather than creating stubs for xarray. See
> > KERNEL_SRC in the Makefile for which kernel source files are directly built
> > and linked in this manner.
> 
> Explain that balance a bit more, is it mainly just a "hey I want that
> function, oh building that new file is going to take me hours to work
> around a bunch of new dependencies, I'll just stub the one function I
> need."?

Yes, that's the general idea. You'll notice in the Makefile that 11 out of the
13 source files that are used directly are in lib/, since these are at the
relative bottom of the dependency chart. drivers/base/ was a bit trickier,
this original RFC builds two of those files directly, but you'll see a lot
of stubs for other drivers/base/ files. IIRC, I tried to use
drivers/base/core.c directly to avoid the 15-20 stubs, but that would have
resulted in needing 30+ stubs for all of its dependencies.

> 
> > Stubs are in uunit/stub.c, and are grouped by the kernel source file containing
> > the real definition. Those groups are listed in alphabetical order.
> > 
> > The build process has five primary steps:
> > 
> > Capture kernel build flags - use 'make -qp' to pull LINUXINCLUDE,
> > KBUILD_CPPFLAGS, KBUILD_CFLAGS from the top-level kernel Makefile.
> 
> Did you look into stealing from tools/build/? I don't think it includes
> those variables, but I suspect it includes some Makefile helpers that
> are open coded. Reusing tools/build/ likely helps with selling others on
> this uunit approach.

No, I'll check that out. Thanks for the tip.

> > 
> > Create unit test build environment - remove thunking, fentry and
> > stack-protector options from the kernel build flags. Also provide
> > KBUILD_XXX strings.
> > 
> > Modified headers - some kernel headers require modification - don't
> > modify them inline though, create a copy in the uunit/build directory,
> > modify them, and set the include directories so that the build will
> > pick up the modified copy first. Explanations for the modified headers
> > are in the Makefile.
> > 
> > Wrap and build kernel source - kernel defines many functions that are
> > also in libc. Kernel also defines some data structures with same name
> > as /usr/include data structures. So we wrap all kernel source files
> > with uunit/include/pre.h and uunit/include/post.h to do some
> > pre-processor magic to eliminate these conflicts.
> 
> How much of an ongoing maintenance burden do you expect this to be? I
> guess it all depends on how fast moving those headers are, but I worry
> about bitrot unless we can get this into some automated environment that
> notices that new changes heading into linux-next are breaking the uunit
> build.

I expect very little if any maintenance burden for the pre.h and post.h
helpers. These handle conflicts for well-established function and type names.
It's certainly possible this list will grow somewhat if this framework scales
outside of just CXL, but generally should be very stable.

The modified headers do carry some maintenance burden however, especially
autoconf.h. I've only tested with a limited number of kernel configurations
and architectures (x86_64, arm64) so the list of config options we need to
disable could grow as this gets exposure to more kernel configs.
 
> > Build unit test applications - found in uunit/app directory, and named
> 
> NIT is "app" idiomatic for CUnit? I would otherwise just call it the
> "test" directory for where the test executables end up.

Ack.
 
> > like core_region_ut.c to associate with core/region.c. The test
> > applications include the stub.c file directly - this will enable future
> > use cases where stubs can be configured to return specific mocked values.
> > 
> > Note that this framework is not CXL-specific - it could be used to unit
> > test a vast majority of other kernel code. Proposal is to just start
> > with this framework in tools/testing/cxl to meet CXL-specific needs.
> 
> Certainly all the pre-work you have done to process kernel headers is
> generally useful work for others that want to adopt this test approach.
> 
> The scaling challenge comes when there is disagreement about
> stub-behavior across tests, right? Or would a test just override the
> default stub?

Ah, you're already getting into stage 2. As we find stubs that require
modified behavior per-test, we will have variables that tests can set to
modify that behavior.

I have an RFC v2 I'm getting ready to send out, I'll add an example of this
just to show the concept. Now that I have your extensive feedback, I'll
probably drop the RFC tag as well.
 
> > Todos:
> > 
> > 1) Integrate with tools/testing/cxl Makefile
> 
> I am ok if this stands alone. As I mentioned above reusing tools/build/
> is likely more important. tools/testing/cxl/ is building a kernel
> module, tools/build/ is there to support userpace executables built out
> of the tools/ directory.

Ack.

> > 2) Break out CXL stubs separately by source file. This will be required as
> >    we start testing more and more CXL files and need to exclude stubs that
> >    are defined in the source file under test.
> 
> You mean like in cases where one test wants one stub behavior and
> another has wants a different one.

No, it's for the case where we have both unit tests and stubs for multiple
source files. For example, if stub.c has port.c stubs in it, we will get
multiple definition failures when compiling port.c unit tests.

In my v2, I just wrap stubs with "#ifndef UUNIT_CORE_PORT_C", and then
the port.c unit tests define this before including stub.c to avoid the
multiple definitions. This seems good enough for now, but may require some
changes if this scales outside of just CXL. Maybe we just use a fully
qualified name like UUNIT_DRIVERS_CXL_CORE_PORT_C.

> 
> > 3) Build common library for building up parts of the CXL topology. For example,
> >    see everything needed to set up ports, decoders, etc. to test the
> >    region.c code. Much of this can be simplified in a way that various
> >    topologies can be built without as much boilerplate code, not only for
> >    region.c but other files as well.
> 
> Do you mean de-duplicating some of the logic in
> cxl_port_setup_targets_test() and cxl_region_setup_targets_test()?

Exactly. I'll have a lot of this in the v2.

> > 4) Improve cxl_region tests to cover error conditions and add extensive
> >    comments explaining how the topology is being built. This overlaps with #3.
> 
> Some comments on the simple tests would help too just to get folks
> ramped about what's going on.

Ack. The comments are absolutely critical and I should have added them in the
v1. They'll be in the v2.

> 
> > 
> > Signed-off-by: Jim Harris <jim.harris@samsung.com>
> > ---
> >  tools/testing/cxl/uunit/Makefile             |  141 +++++++++++
> >  tools/testing/cxl/uunit/app/core_hdm_ut.c    |   82 ++++++
> >  tools/testing/cxl/uunit/app/core_region_ut.c |  237 ++++++++++++++++++
> >  tools/testing/cxl/uunit/include/post.h       |   21 ++
> >  tools/testing/cxl/uunit/include/pre.h        |   22 ++
> >  tools/testing/cxl/uunit/stub.c               |  335 ++++++++++++++++++++++++++
> >  6 files changed, 838 insertions(+)
> >  create mode 100644 tools/testing/cxl/uunit/Makefile
> >  create mode 100644 tools/testing/cxl/uunit/app/core_hdm_ut.c
> >  create mode 100644 tools/testing/cxl/uunit/app/core_region_ut.c
> >  create mode 100644 tools/testing/cxl/uunit/include/post.h
> >  create mode 100644 tools/testing/cxl/uunit/include/pre.h
> >  create mode 100644 tools/testing/cxl/uunit/stub.c
> 
> This overall looks promising, the tools/build/ question might be the
> only consideration for merging in the near term.
> 
> Some inline comments below:
> 
> [..]
> > +
> > +	CU_ASSERT(add_hdm_decoder(&port, &decoder, targets) == 0);
> 
> Likely wants some commentary on why it is ok to pass uninitialized stack
> variables into add_hdm_decoder().

Ack.

> > +}
> > +
> > +static void
> > +cxl_settle_decoders_test(void)
> > +{
> > +	struct cxl_hdm hdm;
> > +	char _regs[0x200];
> > +	void *regs = _regs;
> > +	unsigned int msecs;
> > +
> > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(0)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
> > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(1)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
> > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(2)] = 0;
> 
> Can this be made to look more like idiomatic kernel code? E.g.:
> 
>     writel(val, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(x))
> 
> ...so that code sequences can be copied from the driver into the tests?

Ack.

> > diff --git a/tools/testing/cxl/uunit/app/core_region_ut.c b/tools/testing/cxl/uunit/app/core_region_ut.c
> > new file mode 100644
> > index 000000000000..770ec200c87f
> > --- /dev/null
> > +++ b/tools/testing/cxl/uunit/app/core_region_ut.c
> > @@ -0,0 +1,237 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "pre.h"
> > +#include "drivers/cxl/core/region.c"
> > +#include "post.h"
> > +#include "../stub.c"
> > +
> > +#include <CUnit/Basic.h>
> > +#include <stdlib.h>
> > +
> > +DECLARE_RWSEM(cxl_dpa_rwsem);
> 
> This is here because core_region_ut.c is not allowed to use stubs
> core_hdm_ut.c?

Correct. This will be fixed in v2 with the changes I referenced above.

> > +	resource.start = 0x1000000000;
> > +	resource.end =   0x2000000000;
> 
> Spotted this just because of the recent debug adventure here:
> 
> http://lore.kernel.org/r/6595c10982cf_8dc6829433@dwillia2-xfh.jf.intel.com.notmuch
> 
> ...where that resource.end should be the exclusive "end" of 0x1fffffffff

Ack.

> [..]
> > diff --git a/tools/testing/cxl/uunit/include/post.h b/tools/testing/cxl/uunit/include/post.h
> > new file mode 100644
> > index 000000000000..231f5bc15c3a
> > --- /dev/null
> > +++ b/tools/testing/cxl/uunit/include/post.h
> [..]
> > diff --git a/tools/testing/cxl/uunit/include/pre.h b/tools/testing/cxl/uunit/include/pre.h
> > new file mode 100644
> > index 000000000000..1eb88e51740f
> > --- /dev/null
> > +++ b/tools/testing/cxl/uunit/include/pre.h
> 
> These last 2 files look like a lot of hard fought meticulous magic,
> hence the question above about expected ongoing maintenance burden.

See above. But some extra comments around this wouldn't hurt - I'll add that
in v2.

> > diff --git a/tools/testing/cxl/uunit/stub.c b/tools/testing/cxl/uunit/stub.c
> > new file mode 100644
> > index 000000000000..208dab1e7d6d
> > --- /dev/null
> > +++ b/tools/testing/cxl/uunit/stub.c
> 
> It might be worthwhile to let clang-format do a once over on this and all
> the other files if only to keep the automated coding-style bots from
> tripping on these files.

The formatting is horrible. I need to clean this up and will run clang-format
before sending out the v2.

-Jim

Re: [PATCH RFC] Add "uunit" unit testing framework for CXL code

[Alison Schofield]

On Thu, Jan 04, 2024 at 04:44:57PM +0000, Jim Harris wrote:

Hi Jim,

Looks really useful, cutting to the part about the individual unit tests....


> > > 4) Improve cxl_region tests to cover error conditions and add extensive
> > >    comments explaining how the topology is being built. This overlaps with #3.
> > 
> > Some comments on the simple tests would help too just to get folks
> > ramped about what's going on.
> 
> Ack. The comments are absolutely critical and I should have added them in the
> v1. They'll be in the v2.

As you might imagine, the first thing I'd like to do with this is try
to spin up a simple unit test that matches something I do with cxl_test,
then grow from there.

Coming to this with cxl_test experience, it would be helpful if you
can draw, comment on some parallels - even to the point of showing,
here' what cxl/test: cxl-topology.sh (or similar) looks like in CUunit.

It would be helpful to separate into multiple patches showing what needed
to be done to add a specific unit test. I'm assuming to add core_region_ut.c
support was added elsewhere in cxl/uunit/*.  Separate patches would make
that dependeny clear and help in mimicing the process of adding a new
test.

I'll try it out on next posting!
Thanks,
Alison

> > 
> > > 
> > > Signed-off-by: Jim Harris <jim.harris@samsung.com>
> > > ---
> > >  tools/testing/cxl/uunit/Makefile             |  141 +++++++++++
> > >  tools/testing/cxl/uunit/app/core_hdm_ut.c    |   82 ++++++
> > >  tools/testing/cxl/uunit/app/core_region_ut.c |  237 ++++++++++++++++++
> > >  tools/testing/cxl/uunit/include/post.h       |   21 ++
> > >  tools/testing/cxl/uunit/include/pre.h        |   22 ++
> > >  tools/testing/cxl/uunit/stub.c               |  335 ++++++++++++++++++++++++++
> > >  6 files changed, 838 insertions(+)
> > >  create mode 100644 tools/testing/cxl/uunit/Makefile
> > >  create mode 100644 tools/testing/cxl/uunit/app/core_hdm_ut.c
> > >  create mode 100644 tools/testing/cxl/uunit/app/core_region_ut.c
> > >  create mode 100644 tools/testing/cxl/uunit/include/post.h
> > >  create mode 100644 tools/testing/cxl/uunit/include/pre.h
> > >  create mode 100644 tools/testing/cxl/uunit/stub.c
> > 
> > This overall looks promising, the tools/build/ question might be the
> > only consideration for merging in the near term.
> > 
> > Some inline comments below:
> > 
> > [..]
> > > +
> > > +	CU_ASSERT(add_hdm_decoder(&port, &decoder, targets) == 0);
> > 
> > Likely wants some commentary on why it is ok to pass uninitialized stack
> > variables into add_hdm_decoder().
> 
> Ack.
> 
> > > +}
> > > +
> > > +static void
> > > +cxl_settle_decoders_test(void)
> > > +{
> > > +	struct cxl_hdm hdm;
> > > +	char _regs[0x200];
> > > +	void *regs = _regs;
> > > +	unsigned int msecs;
> > > +
> > > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(0)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
> > > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(1)] = CXL_HDM_DECODER0_CTRL_COMMITTED;
> > > +	*(long *)&_regs[CXL_HDM_DECODER0_CTRL_OFFSET(2)] = 0;
> > 
> > Can this be made to look more like idiomatic kernel code? E.g.:
> > 
> >     writel(val, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(x))
> > 
> > ...so that code sequences can be copied from the driver into the tests?
> 
> Ack.
> 
> > > diff --git a/tools/testing/cxl/uunit/app/core_region_ut.c b/tools/testing/cxl/uunit/app/core_region_ut.c
> > > new file mode 100644
> > > index 000000000000..770ec200c87f
> > > --- /dev/null
> > > +++ b/tools/testing/cxl/uunit/app/core_region_ut.c
> > > @@ -0,0 +1,237 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include "pre.h"
> > > +#include "drivers/cxl/core/region.c"
> > > +#include "post.h"
> > > +#include "../stub.c"
> > > +
> > > +#include <CUnit/Basic.h>
> > > +#include <stdlib.h>
> > > +
> > > +DECLARE_RWSEM(cxl_dpa_rwsem);
> > 
> > This is here because core_region_ut.c is not allowed to use stubs
> > core_hdm_ut.c?
> 
> Correct. This will be fixed in v2 with the changes I referenced above.
> 
> > > +	resource.start = 0x1000000000;
> > > +	resource.end =   0x2000000000;
> > 
> > Spotted this just because of the recent debug adventure here:
> > 
> > http://lore.kernel.org/r/6595c10982cf_8dc6829433@dwillia2-xfh.jf.intel.com.notmuch
> > 
> > ...where that resource.end should be the exclusive "end" of 0x1fffffffff
> 
> Ack.
> 
> > [..]
> > > diff --git a/tools/testing/cxl/uunit/include/post.h b/tools/testing/cxl/uunit/include/post.h
> > > new file mode 100644
> > > index 000000000000..231f5bc15c3a
> > > --- /dev/null
> > > +++ b/tools/testing/cxl/uunit/include/post.h
> > [..]
> > > diff --git a/tools/testing/cxl/uunit/include/pre.h b/tools/testing/cxl/uunit/include/pre.h
> > > new file mode 100644
> > > index 000000000000..1eb88e51740f
> > > --- /dev/null
> > > +++ b/tools/testing/cxl/uunit/include/pre.h
> > 
> > These last 2 files look like a lot of hard fought meticulous magic,
> > hence the question above about expected ongoing maintenance burden.
> 
> See above. But some extra comments around this wouldn't hurt - I'll add that
> in v2.
> 
> > > diff --git a/tools/testing/cxl/uunit/stub.c b/tools/testing/cxl/uunit/stub.c
> > > new file mode 100644
> > > index 000000000000..208dab1e7d6d
> > > --- /dev/null
> > > +++ b/tools/testing/cxl/uunit/stub.c
> > 
> > It might be worthwhile to let clang-format do a once over on this and all
> > the other files if only to keep the automated coding-style bots from
> > tripping on these files.
> 
> The formatting is horrible. I need to clean this up and will run clang-format
> before sending out the v2.
> 
> -Jim

Re: [PATCH RFC] Add "uunit" unit testing framework for CXL code

[Jim Harris]

On Thu, Jan 04, 2024 at 09:36:01AM -0800, Alison Schofield wrote:
> On Thu, Jan 04, 2024 at 04:44:57PM +0000, Jim Harris wrote:
> 
> Hi Jim,
> 
> Looks really useful, cutting to the part about the individual unit tests....
> 
> 
> > > > 4) Improve cxl_region tests to cover error conditions and add extensive
> > > >    comments explaining how the topology is being built. This overlaps with #3.
> > > 
> > > Some comments on the simple tests would help too just to get folks
> > > ramped about what's going on.
> > 
> > Ack. The comments are absolutely critical and I should have added them in the
> > v1. They'll be in the v2.
> 
> As you might imagine, the first thing I'd like to do with this is try
> to spin up a simple unit test that matches something I do with cxl_test,
> then grow from there.
> 
> Coming to this with cxl_test experience, it would be helpful if you
> can draw, comment on some parallels - even to the point of showing,
> here' what cxl/test: cxl-topology.sh (or similar) looks like in CUunit.
> 
> It would be helpful to separate into multiple patches showing what needed
> to be done to add a specific unit test. I'm assuming to add core_region_ut.c
> support was added elsewhere in cxl/uunit/*.  Separate patches would make
> that dependeny clear and help in mimicing the process of adding a new
> test.
> 
> I'll try it out on next posting!
> Thanks,
> Alison
 
Good idea Alison. I'll take a look at this before I send out the next rev.

-Jim