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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jun 05, 2026 at 04:41:58PM +0100, Andrew Cooper wrote:
> 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?

Yes, this for example:

https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/14693648817

Thanks, Roger.