On Tue, Aug 05, 2025 at 04:15:28PM +0200, Roger Pau Monné wrote:
> On Wed, Jul 30, 2025 at 05:41:00PM +0000, dm...@proton.me wrote:
> > From: Denis Mukhin <dmuk...@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.
> >
> > Signed-off-by: Denis Mukhin <dmuk...@ford.com>
> > ---
> > Changes since v12:
> > - fixed Makefile
> > - dropped unused symbols/includes from the test harness header
> > - s/printk/printf/g in the test code
> > ---
> > tools/tests/Makefile | 2 +-
> > tools/tests/domid/.gitignore | 2 +
> > tools/tests/domid/Makefile | 48 ++++++++++
> > tools/tests/domid/include/xen/domain.h | 126 +++++++++++++++++++++++++
> > tools/tests/domid/test-domid.c | 78 +++++++++++++++
> > 5 files changed, 255 insertions(+), 1 deletion(-)
> > create mode 100644 tools/tests/domid/.gitignore
> > create mode 100644 tools/tests/domid/Makefile
> > create mode 100644 tools/tests/domid/include/xen/domain.h
> > create mode 100644 tools/tests/domid/test-domid.c
> >
> > 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
> > 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..08fbad096aec
> > --- /dev/null
> > +++ b/tools/tests/domid/Makefile
> > @@ -0,0 +1,48 @@
> > +# 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/
> > +
> > +.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__
> > +CFLAGS += $(APPEND_CFLAGS)
> > +CFLAGS += $(CFLAGS_xeninclude)
> > +CFLAGS += -I./include/
> > +
> > +LDFLAGS += $(APPEND_LDFLAGS)
> > +
> > +test-domid: domid.o test-domid.o
> > + $(CC) $^ -o $@ $(LDFLAGS)
> > +
> > +-include $(DEPS_INCLUDE)
> > diff --git a/tools/tests/domid/include/xen/domain.h
> > b/tools/tests/domid/include/xen/domain.h
> > new file mode 100644
> > index 000000000000..e5db0235445e
> > --- /dev/null
> > +++ b/tools/tests/domid/include/xen/domain.h
> > @@ -0,0 +1,126 @@
> > +/* 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>
> > +
> > +#define BUG_ON(x) assert(!(x))
> > +#define ASSERT(x) assert(x)
> > +
> > +#define DOMID_FIRST_RESERVED (10)
> > +#define DOMID_INVALID (11)
> > +
> > +#define DEFINE_SPINLOCK(x) unsigned long *(x)
>
> I think this shouldn't be a pointer? As you otherwise trigger a NULL
> pointer dereference in the increases and decreases done below?
Sorry, this bitops integration is very raw.
Thanks for all your suggestions, I reworked it.
>
> > +#define spin_lock(x) ((*(x))++)
> > +#define spin_unlock(x) ((*(x))--)
>
> FWIW, I would use a plain bool:
>
> #define DEFINE_SPINLOCK(l) bool l
> #define spin_lock(l) (*(l) = true)
> #define spin_unlock(l) (*(l) = false)
>
> As you don't expect concurrency tests, you could even assert the lock
> is in the expected state before taking/releasing it.
>
> > +
> > +#define printk printf
> > +
> > +#define BITS_PER_LONG sizeof(unsigned long)
>
> That's BYTES_PER_LONG, BITS_PER_LONG would be (sizeof(unsigned long) * 8).
>
> > +#define BITS_PER_WORD (8U * 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)]
> > +
> > +static inline int __test_and_set_bit(unsigned int nr, unsigned long *addr)
> > +{
> > + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> > + unsigned long *p = addr + (nr / BITS_PER_WORD);
> > + int old = (*p & mask) != 0;
> > +
> > + *p |= mask;
> > +
> > + return old;
> > +}
> > +
> > +static inline int __test_and_clear_bit(unsigned int nr, unsigned long
> > *addr)
> > +{
> > + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> > + unsigned long *p = addr + (nr / BITS_PER_WORD);
> > + int old = (*p & mask) != 0;
> > +
> > + *p &= ~mask;
> > +
> > + return old;
> > +}
>
> Could you somehow use the generic__test_and_set_bit() and
> generic__test_and_clear_bit() implementations in bitops.h?
I tried that originally, and it pulls a lot of dependencies from xen/bitops.h;
that will be a mini project to compile xen/bitops.h for the host, which I
think I can skip doing for the purpose of this test.
I followed another approach as discussed offline in matrix: re-purpose
tools/libs/ctrl/xc_bitops.h which seems to be working nice!
>
> > +
> > +static inline void __set_bit(unsigned int nr, volatile unsigned long *addr)
> > +{
> > + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> > + unsigned long *p = (unsigned long *)addr + (nr / BITS_PER_WORD);
>
> Why do you need the cast to drop the volatile here?
>
> > +
> > + *p |= mask;
> > +}
>
> I think you could possibly simplify to a single line:
>
> ((unsigned int *)addr)[nr >> 5] |= (1u << (nr & 31));
>
> That's the implementation of constant_set_bit() in x86.
>
> > +
> > +static inline void __clear_bit(unsigned int nr, volatile unsigned long
> > *addr)
> > +{
> > + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> > + unsigned long *p = (unsigned long *)addr + (nr / BITS_PER_WORD);
> > +
> > + *p &= ~mask;
> > +}
>
> I don't think you need __clear_bit()? It's not used by domid.c AFAICT.
Overlooked, thanks.
>
> > +
> > +static inline unsigned long find_next_zero_bit(const unsigned long *addr,
> > + unsigned long size,
> > + unsigned long offset)
> > +{
> > + unsigned long idx = offset / BITS_PER_WORD;
> > + unsigned long bit = offset % BITS_PER_WORD;
> > +
> > + if (offset >= size)
> > + return size;
> > +
> > + while (offset < size)
> > + {
> > + unsigned long val = addr[idx] | (~0UL >> (BITS_PER_WORD - bit));
> > +
> > + if (~val)
> > + {
> > + unsigned long pos = __builtin_ffsl(~val);
> > +
> > + if (pos > 0)
> > + {
> > + unsigned long rc = idx * BITS_PER_WORD + (pos - 1);
> > +
> > + if (rc < size)
> > + return rc;
> > + }
> > + }
> > +
> > + offset = (idx + 1) * BITS_PER_WORD;
> > + idx++;
> > + bit = 0;
> > + }
> > +
> > + return size;
> > +}
>
> Hm, you need a full find_next_zero_bit() implementation here because
> addr can be arbitrarily long. Could you somehow include
> xen/lib/find-next-bit.c and set the right defines so only the
> implementation of find_next_bit() is included?
That's a good idea!
xen/lib/find-next-bit.c seems to be integrating pretty simple.
Thanks for the hint!
>
> > +
> > +typedef bool spinlock_t;
>
> You want to put this ahead, so that DEFINE_SPINLOCK can be:
>
> #define DEFINE_SPINLOCK(l) spinlock_t l
>
> > +typedef uint16_t domid_t;
> > +
> > +/* See include/xen/domain.h */
> > +extern domid_t domid_alloc(domid_t domid);
> > +extern void domid_free(domid_t domid);
> > +
> > +#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/test-domid.c b/tools/tests/domid/test-domid.c
> > new file mode 100644
> > index 000000000000..d52eaf5f1f55
> > --- /dev/null
> > +++ b/tools/tests/domid/test-domid.c
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Unit tests for domain ID allocator.
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +/* Local test include replicating hypervisor includes. */
> > +#include <xen/domain.h>
>
> I think this is a difficult to maintain position. Right now domid.c
> only includes xen/domain.h, so you can easily replace this in
> user-space. However if/when domid.c starts including more headers,
> replicating this in user-space will be cumbersome IMO.
>
> I would just guard the includes in domid.c with #ifdef __XEN__ for the
> preprocessor to remove them when domid.c is compiled as part of the
> unit-tests harness.
>
> I usually include a harness.h that contains the glue to make the
> imported code build (much like what you have placed in the test
> harness xen/domain.h header.
I like that there's no need to modify the tested code.
I reworked this slightly differently: local include/xen/domain.h is a symlink
to local harness.h, and all future dependendent files will be symlinks to
harness.h as well.
>
> > +
> > +int main(int argc, char **argv)
> > +{
> > + domid_t expected, allocated;
> > +
> > + printf("DOMID_FIRST_RESERVED=%u DOMID_INVALID=%u\n",
> > + DOMID_FIRST_RESERVED, DOMID_INVALID);
> > +
> > + /* Test ID#0 cannot be allocated twice. */
> > + allocated = domid_alloc(0);
> > + printf("TEST 1: expected %u allocated %u\n", 0, allocated);
> > + ASSERT(allocated == 0);
> > + allocated = domid_alloc(0);
> > + printf("TEST 1: expected %u allocated %u\n", DOMID_INVALID, allocated);
> > + ASSERT(allocated == DOMID_INVALID);
> > +
> > + /* Ensure ID is not allocated. */
> > + domid_free(0);
> > +
> > + /*
> > + * Test that that two consecutive calls of domid_alloc(DOMID_INVALID)
> > + * will never return the same ID.
> > + * NB: ID#0 is reserved and shall not be allocated by
> > + * domid_alloc(DOMID_INVALID).
> > + */
> > + for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
> > + {
> > + allocated = domid_alloc(DOMID_INVALID);
> > + printf("TEST 2: expected %u allocated %u\n", expected, allocated);
> > + ASSERT(allocated == expected);
> > + }
> > + for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
> > + {
> > + allocated = domid_alloc(DOMID_INVALID);
> > + printf("TEST 3: expected %u allocated %u\n", DOMID_INVALID,
> > allocated);
> > + ASSERT(allocated == DOMID_INVALID);
> > + }
> > +
> > + /* Re-allocate first ID from [1..DOMID_FIRST_RESERVED - 1]. */
> > + expected = 1;
> > + domid_free(1);
> > + allocated = domid_alloc(DOMID_INVALID);
> > + printf("TEST 4: expected %u allocated %u\n", expected, allocated);
> > + ASSERT(allocated == expected);
> > +
> > + /* Re-allocate last ID from [1..DOMID_FIRST_RESERVED - 1]. */
> > + expected = DOMID_FIRST_RESERVED - 1;
> > + domid_free(DOMID_FIRST_RESERVED - 1);
> > + allocated = domid_alloc(DOMID_INVALID);
> > + printf("TEST 5: expected %u allocated %u\n", expected, allocated);
> > + ASSERT(allocated == expected);
> > +
> > + /* Allocate an invalid ID. */
> > + expected = DOMID_INVALID;
> > + allocated = domid_alloc(DOMID_FIRST_RESERVED);
> > + printf("TEST 6: expected %u allocated %u\n", expected, allocated);
> > + ASSERT(allocated == expected);
>
> I would make this a bit less chatty maybe?
Ack.
>
> I think you only need to print on errors, and you probably don't want
> to ASSERT() on failure, and rather try to finish all the tests in
> order to report multiple failures in a single run.
I thought about it originally, but my "tests" depend on each other, so I'll
keep failing on the first error as is, if there's no strong objection.
>
> Thanks, Roger.
