Re: [PATCH v14 3/4] tools/tests: introduce unit tests for domain ID allocator

[Julien Grall <julien@xxxxxxx>]

Hi Denis,

On 08/08/2025 03:20, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Introduce some basic infrastructure for doing domain ID allocation unit tests,
> and add a few tests that ensure correctness of the domain ID allocator.
I am quite happy to see more unit tests for Xen :).

> Use <xen-tools/bitops.h> and xen/lib/find-next-bit.c in test hardness code.
>
> Adjust find-next-bit.c to be compiled with __XEN_TOOLS__.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
With a couple of remarks below:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
> Changes since v13:
> - reworked bitops integration
> - hooked xen/lib/find-next-bit.c
> - cleaned up harness.h code
> - made test to use more IDs
> ---
>   tools/include/xen-tools/bitops.h       | 10 +++
>   tools/tests/Makefile                   |  2 +-
>   tools/tests/domid/.gitignore           |  2 +
>   tools/tests/domid/Makefile             | 56 +++++++++++++++++
>   tools/tests/domid/harness.h            | 54 ++++++++++++++++
>   tools/tests/domid/include/xen/domain.h |  1 +
>   tools/tests/domid/test-domid.c         | 86 ++++++++++++++++++++++++++
>   xen/lib/find-next-bit.c                |  5 ++
>   8 files changed, 215 insertions(+), 1 deletion(-)
>   create mode 100644 tools/tests/domid/.gitignore
>   create mode 100644 tools/tests/domid/Makefile
>   create mode 100644 tools/tests/domid/harness.h
>   create mode 120000 tools/tests/domid/include/xen/domain.h
>   create mode 100644 tools/tests/domid/test-domid.c
>
> diff --git a/tools/include/xen-tools/bitops.h b/tools/include/xen-tools/bitops.h
> index 681482f6759f..3b98fba6d74c 100644
> --- a/tools/include/xen-tools/bitops.h
> +++ b/tools/include/xen-tools/bitops.h
> @@ -12,6 +12,16 @@
>   #define BITS_PER_LONG 32
>   #endif
>   
> +#define ffsl(x)       __builtin_ffsl(x)
> +
> +#define BIT_WORD(nr)  ((nr) / BITS_PER_LONG)
> +
> +#define BITS_TO_LONGS(bits) \
> +    (((bits) + BITS_PER_LONG - 1) / BITS_PER_LONG)
> +
> +#define DECLARE_BITMAP(name, bits) \
> +    unsigned long name[BITS_TO_LONGS(bits)]
> +
>   #define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr) / 8]
>   #define BITMAP_SHIFT(_nr) ((_nr) % 8)
>   
> diff --git a/tools/tests/Makefile b/tools/tests/Makefile
> index 36928676a666..ff1666425436 100644
> --- a/tools/tests/Makefile
> +++ b/tools/tests/Makefile
> @@ -1,7 +1,7 @@
>   XEN_ROOT = $(CURDIR)/../..
>   include $(XEN_ROOT)/tools/Rules.mk
>   
> -SUBDIRS-y :=
> +SUBDIRS-y := domid
I would prefer if we keep SUBDIRST-y := as it is and add a new line 
SUBDIRS-y +=. This is mostly to reduce the chance that someone will add 
a new directory "abc" and forgot to update the line containing "domid".
>   SUBDIRS-y += resource
>   SUBDIRS-$(CONFIG_X86) += cpu-policy
>   SUBDIRS-$(CONFIG_X86) += tsx
> diff --git a/tools/tests/domid/.gitignore b/tools/tests/domid/.gitignore
> new file mode 100644
> index 000000000000..70e306b3c074
> --- /dev/null
> +++ b/tools/tests/domid/.gitignore
> @@ -0,0 +1,2 @@
> +*.o
> +test-domid
> diff --git a/tools/tests/domid/Makefile b/tools/tests/domid/Makefile
> new file mode 100644
> index 000000000000..d96ceca6d954
> --- /dev/null
> +++ b/tools/tests/domid/Makefile
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Unit tests for domain ID allocator.
> +#
> +# Copyright 2025 Ford Motor Company
> +
> +XEN_ROOT=$(CURDIR)/../../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +TESTS := test-domid
> +
> +vpath domid.c $(XEN_ROOT)/xen/common/
> +vpath find-next-bit.c $(XEN_ROOT)/xen/lib/
> +
> +.PHONY: all
> +all: $(TESTS)
> +
> +.PHONY: run
> +run: $(TESTS)
> +       $(foreach t,$(TESTS),./$(t);)
> +
> +.PHONY: clean
> +clean:
> +       $(RM) -- *.o $(TESTS) $(DEPS_RM)
> +
> +.PHONY: distclean
> +distclean: clean
> +       $(RM) -- *~
> +
> +.PHONY: install
> +install: all
> +       $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
> +       $(INSTALL_PROG) test-domid $(DESTDIR)$(LIBEXEC)/tests
> +
> +.PHONY: uninstall
> +uninstall:
> +       $(RM) -- $(DESTDIR)$(LIBEXEC)/tests/test-domid
> +
> +CFLAGS += -D__XEN_TOOLS__
> +# find-next-bit.c
> +CFLAGS += '-DEXPORT_SYMBOL(x)=' \
> +          -Dfind_first_bit \
> +          -Dfind_first_zero_bit \
> +          -Dfind_next_bit \
> +          -Dfind_next_bit_le \
> +          -Dfind_next_zero_bit_le
> +CFLAGS += $(APPEND_CFLAGS)
> +CFLAGS += $(CFLAGS_xeninclude)
> +CFLAGS += -I./include/
> +
> +LDFLAGS += $(APPEND_LDFLAGS)
> +
> +test-domid: domid.o find-next-bit.o test-domid.o
> +       $(CC) $^ -o $@ $(LDFLAGS)
> +
> +-include $(DEPS_INCLUDE)
> diff --git a/tools/tests/domid/harness.h b/tools/tests/domid/harness.h
> new file mode 100644
> index 000000000000..b043519dcb35
> --- /dev/null
> +++ b/tools/tests/domid/harness.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit test harness for domain ID allocator.
> + *
> + * Copyright 2025 Ford Motor Company
> + */
> +
> +#ifndef _TEST_HARNESS_
> +#define _TEST_HARNESS_
> +
> +#include <assert.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +
> +#include <xen-tools/common-macros.h>
> +#include <xen-tools/bitops.h>
> +
> +typedef bool spinlock_t;
> +typedef uint16_t domid_t;
> +
> +extern domid_t domid_alloc(domid_t domid);
> +extern void domid_free(domid_t domid);
> +
> +extern unsigned long find_next_zero_bit(const unsigned long *addr,
> +                                        unsigned long size,
> +                                        unsigned long offset);
> +
> +#define __test_and_set_bit(nr, addr)    test_and_set_bit(nr, addr)
> +#define __test_and_clear_bit(nr, addr)  test_and_clear_bit(nr, addr)
> +#define __set_bit(nr, addr)             set_bit(nr, addr)
> +
> +#define BUG_ON(x)                       assert(!(x))
> +#define ASSERT(x)                       assert(x)
> +
> +#define DEFINE_SPINLOCK(l)              spinlock_t l
> +#define spin_lock(l)                    (*(l) = true)
> +#define spin_unlock(l)                  (*(l) = false)
NIT: For hardening purpose, I wonder whether we should also assert that 
"l" is "false" for spin_lock() and "true" for spin_unlock(). This would 
help catching any bug in the locking.
> +
> +#define printk                          printf
> +
> +#define DOMID_FIRST_RESERVED            (100)
> +#define DOMID_INVALID                   (101)
> +
> +#endif /* _TEST_HARNESS_ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/tests/domid/include/xen/domain.h 
> b/tools/tests/domid/include/xen/domain.h
> new file mode 120000
> index 000000000000..2eda9aed088e
> --- /dev/null
> +++ b/tools/tests/domid/include/xen/domain.h
> @@ -0,0 +1 @@
> +../../harness.h
> \ No newline at end of file
> diff --git a/tools/tests/domid/test-domid.c b/tools/tests/domid/test-domid.c
> new file mode 100644
> index 000000000000..7b6fb5ee2a7b
> --- /dev/null
> +++ b/tools/tests/domid/test-domid.c
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for domain ID allocator.
> + *
> + * Copyright 2025 Ford Motor Company
> + */
> +
> +#include "harness.h"
> +
> +#define verify(exp, fmt, args...) do { \
> +    if ( !(exp) ) \
> +        printf(fmt, ## args); \
> +    assert(exp); \
> +} while (0);
> +
> +/*
> + * Fail on the first error, since tests are dependent on each other.
> + */
> +int main(int argc, char **argv)
> +{
> +    domid_t expected, allocated;
> +
> +    /* Test ID#0 cannot be allocated twice. */
For future improvement, we could check that for any domid [0; 
DOMID_FIRST_RESERVED[, we can allocate domid_alloc().
This would also confirm that domid_alloc() *only* allocates *one* ID.

Cheers,

--
Julien Grall