Re: [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/06/2026 8:18 pm, Roger Pau Monne wrote:
> diff --git a/tools/tests/numa/harness.h b/tools/tests/numa/harness.h
> new file mode 100644
> index 000000000000..9eec77f31402
> --- /dev/null
> +++ b/tools/tests/numa/harness.h
> @@ -0,0 +1,184 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for NUMA setup.
> + *
> + * Copyright (C) 2026 Cloud Software Group
> + */
> +
> +#ifndef _TEST_HARNESS_
> +#define _TEST_HARNESS_

This is overly generic, and liable to break if anyone copies it.  Maybe
NUMA_HARNESS, or WRAP_XEN_NUMA because ...

Looking below, how about naming it wrapped-xen-numa.h, so ...

> <snip>
>
> +
> +static inline bool arch_numa_unavailable(void)
> +{
> +    return false;
> +}
> +
> +#include "numa.h"

I presume this is the real xen/numa.h ?

> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
> new file mode 100644
> index 000000000000..bced68d4d7f1
> --- /dev/null
> +++ b/tools/tests/numa/test-numa.c
> @@ -0,0 +1,222 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for NUMA setup.
> + *
> + * Copyright (C) 2026 Cloud Software Group
> + */
> +
> +#include "harness.h"
> +
> +static paddr_t mem_hotplug;
> +unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
> +
> +#include "../../xen/common/numa.c"

... this has 

#include "wrapped-xen-numa.h"
#include "../../xen/common/numa.c"

which I think is clearer to follow.

> <snip>
>
> +int main(int argc, char **argv)
> +{
> +    static const struct {
> +        struct mem_affinity affinity[MAX_RANGES];
> +        struct mem_range ram[MAX_RANGES];
> +    } tests[] = {
> +        /* AMD Turin system. */

I'd suggest /* From an arbitrary AMD Turin system */

Just "AMD Turin system" feels a little as if all systems are like this,
which is absolutely not the case.

> +        {
> +            .affinity = {
> +                { .nid = 0, .start = 0x00000000000ULL, .end = 
> 0x0000009ffffULL },
> +                { .nid = 0, .start = 0x000000c0000ULL, .end = 
> 0x000afffffffULL },
> +                { .nid = 0, .start = 0x00100000000ULL, .end = 
> 0x0c04fffffffULL },
> +                { .nid = 1, .start = 0x0c050000000ULL, .end = 
> 0x0fc4fffffffULL },
> +                { .nid = 1, .start = 0x10000000000ULL, .end = 
> 0x183ffffffffULL },
> +            },
> +            .ram = {
> +                { .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
> +                { .start = 0x00000100000ULL, .end = 0x0007590ffffULL },
> +                { .start = 0x000759d1000ULL, .end = 0x00075a0ffffULL },
> +                { .start = 0x00076000000ULL, .end = 0x00094c73fffULL },
> +                { .start = 0x0009b5ff000ULL, .end = 0x0009fff9fffULL },
> +                { .start = 0x0009ffff000ULL, .end = 0x0009fffffffULL },
> +                { .start = 0x00100010000ULL, .end = 0x0fc4fffffffULL },
> +                { .start = 0x10000000000ULL, .end = 0x183f7ffffffULL },
> +                { .start = 0x183f8800000ULL, .end = 0x183faabffffULL },
> +            },
> +        },
> +    };
> +    int ret_code = EXIT_SUCCESS;
> +
> +    /* Dummy firmware interface provider name, use TST for TEST. */
> +    numa_fw_nid_name = "TST";
> +
> +    for ( unsigned int i = 0 ; i < ARRAY_SIZE(tests); i++ )
> +    {
> +        paddr_t min = ~(paddr_t)0, max = 0;
> +        unsigned int j;
> +
> +        numa_reset_state();
> +
> +        ram = tests[i].ram;
> +
> +        for ( j = 0;
> +              j < ARRAY_SIZE(tests[i].affinity) && tests[i].affinity[j].end;
> +              j++ )
> +        {
> +            const struct mem_affinity *affinity = &tests[i].affinity[j];
> +            paddr_t length = affinity->end - affinity->start + 1;
> +
> +            if ( !numa_update_node_memblks(affinity->nid, affinity->nid,
> +                                           affinity->start, length, false) )
> +            {
> +                printf("Fail to add NID %u [%" PRIpaddr ", %" PRIpaddr "]\n",
> +                        affinity->nid, affinity->start, affinity->end);
> +                ret_code = EXIT_FAILURE;
> +                continue;
> +            }
> +
> +            min = min(min, affinity->start);
> +            max = max(max, affinity->end);
> +        }
> +
> +        if ( !numa_process_nodes(min, max + 1) )
> +        {
> +                printf("Unable to process nodes\n");
> +                print_ranges(tests[i].affinity);
> +                ret_code = EXIT_FAILURE;
> +                continue;

This is mis-indented.  Best double check the whole file.

> +        }
> +
> +        for ( j = 0;
> +              j < ARRAY_SIZE(tests[i].ram) && tests[i].ram[j].end;
> +              j++ )
> +            if ( !test_paddr(tests[i].ram[j].start) ||
> +                 !test_paddr(tests[i].ram[j].end) )
> +                ret_code = EXIT_FAILURE;
> +    }
> +
> +    return ret_code;

This is fine for now, but we're going to have to consolidate the
patterns eventually.

Do you have a Gitlab CI run with this passing?

~Andrew