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

[dmkhn@proton.me]

On Fri, Aug 08, 2025 at 06:56:16PM +0100, Julien Grall wrote:
> Hi Denis,
> 
> On 08/08/2025 03:20, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > 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@ford.com>
> 
> With a couple of remarks below:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> > ---
> > 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".

Ack.

> 
> >   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.

Good idea! Will do.

> 
> > +
> > +#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.

Will update.

> 
> Cheers,
> 
> --
> Julien Grall
> 
>