Re: [PATCH v12 2/3] tools/tests: introduce unit tests for domain ID allocator

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Wed Jul 30, 2025 at 5:34 AM CEST, dmkhn 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.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v11:
> - simplified test, dropped dom0less variant
> ---
>  tools/tests/Makefile                   |   2 +-
>  tools/tests/domid/.gitignore           |   2 +
>  tools/tests/domid/Makefile             |  51 ++++++++++
>  tools/tests/domid/include/xen/domain.h | 135 +++++++++++++++++++++++++
>  tools/tests/domid/test-domid.c         |  78 ++++++++++++++
>  5 files changed, 267 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..91ac43232518
> --- /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..9a817bb70c36
> --- /dev/null
> +++ b/tools/tests/domid/Makefile
> @@ -0,0 +1,51 @@
> +# 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)
> +
> +%.o: %.c
> +     $(CC) $(CFLAGS) -c $^ -o $@ $(LDFLAGS)
> +
> +test-domid: domid.o test-domid.o
> +     $(CC) $^ -o $@ $(LDFLAGS)
> +
> +-include $(DEPS_INCLUDE)

I think the Makefile is off. If I try to recompile after modifying the header it
doesn't compile anymore.

> diff --git a/tools/tests/domid/include/xen/domain.h 
> b/tools/tests/domid/include/xen/domain.h
> new file mode 100644
> index 000000000000..396c3bab9d26
> --- /dev/null
> +++ b/tools/tests/domid/include/xen/domain.h

I wish this could be the real one. Alas, I've been in that boat and I'm aware
you can't. Just came here to whine online.

> @@ -0,0 +1,135 @@
> +/* 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 <errno.h> <======= not needed
> +#include <stdbool.h>
> +#include <stddef.h> <======= not needed
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h> <======= not needed
> +#include <string.h> <======= not needed

Bunch un headers are not required.

> +
> +#include <xen-tools/common-macros.h>
> +
> +#define BUG_ON(x)               assert(!(x))
> +#define ASSERT(x)               assert(x)
> +
> +#define __xen_mk_uint(x)        x ## U
> +#define xen_mk_uint(x)          __xen_mk_uint(x)

Do you need these, if...

> +
> +#define DOMID_FIRST_RESERVED    xen_mk_uint(10)
> +#define DOMID_INVALID           xen_mk_uint(11)

... these were just 10 and 11 without being wrapped?

> +
> +#define DEFINE_SPINLOCK(x)      unsigned long *(x)
> +#define spin_lock(x)            ((*(x))++)
> +#define spin_unlock(x)          ((*(x))--)
> +
> +#define BITS_PER_LONG           sizeof(unsigned long)
> +#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;
> +}
> +
> +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);
> +
> +    *p |= mask;
> +}
> +
> +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;
> +}
> +
> +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;
> +}
> +
> +#define printk printf
> +
> +#define cf_check

No longer needed

> +
> +typedef bool spinlock_t;
> +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..ed041bb56d89
> --- /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>
> +
> +int main(int argc, char **argv)
> +{
> +    domid_t expected, allocated;
> +
> +    printk("%s DOMID_FIRST_RESERVED=%u DOMID_INVALID=%u\n",
> +            argv[0], DOMID_FIRST_RESERVED, DOMID_INVALID);

nit; IMO, argv[0] is inconsequential.

also, printf seeing how this is a unit test. I know it's #define'd, but it's
just easier to eyeball you're in userspace when you don't have printks around.

> +    /* Test ID#0 cannot be allocated twice. */
> +    allocated = domid_alloc(0);
> +    printk("TEST 1: expected %u allocated %u\n", 0, allocated);
> +    ASSERT(allocated == 0);
> +    allocated = domid_alloc(0);
> +    printk("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 = 0; expected < DOMID_FIRST_RESERVED - 1; expected++ )

If this starts on 1...

> +    {
> +        allocated = domid_alloc(DOMID_INVALID);
> +        printk("TEST 2: expected %u allocated %u\n", expected + 1, 
> allocated);
> +        ASSERT(allocated == expected + 1);

... these two doen't have to have it. Which is a bit easier to read and 
unserstand.

"expect" should match the expectation, IMO.

> +    }
> +    for ( expected = 0; expected < DOMID_FIRST_RESERVED; expected++ )
> +    {
> +        allocated = domid_alloc(DOMID_INVALID);
> +        printk("TEST 2: 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);
> +    printk("TEST 3: 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);
> +    printk("TEST 4: expected %u allocated %u\n", expected, allocated);
> +    ASSERT(allocated == expected);
> +
> +    /* Allocate an invalid ID. */
> +    expected = DOMID_INVALID;
> +    allocated = domid_alloc(DOMID_FIRST_RESERVED);
> +    printk("TEST 5: expected %u allocated %u\n", expected, allocated);
> +    ASSERT(allocated == expected);
> +
> +    return 0;
> +}

As far as the test cases go, they seem pretty extensive. I like it.

Cheers,
Alejandro