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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/06/2026 4:49 pm, Roger Pau Monné wrote:
> 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

Ok.  Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> preferably with
the suggested adjustments.