Re: [PATCH v3 07/22] xen/device-tree: Read NUMA node distance from Device Tree 'distance-map'

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.06.2026 09:49, Hirokazu Takahashi wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -375,6 +375,10 @@ void asmlinkage __init noreturn start_xen(unsigned long 
> fdt_paddr)
>          device_tree_flattened = NULL;
>      }
>  
> +#ifdef CONFIG_NUMA
> +    numa_distance_table_init();
> +#endif /* CONFIG_NUMA */

Generally we prefer to avoid such #ifdef-ary in main source code by providing 
stub
(inline) functions in headers. Yet then I'm not an Arm maintainer ...

> --- a/xen/common/device-tree/numa.c
> +++ b/xen/common/device-tree/numa.c
> @@ -1,4 +1,80 @@
>  /* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Derived from Linux kernel 7.0's $drivers/of/of_numa.c
> + * Parse 'distance-map'
> + */
> +
> +#include <xen/bootinfo.h>
> +#include <xen/device_tree.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/bootfdt.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/nodemask.h>
> +#include <xen/numa.h>
> +
> +#define LOCAL_DISTANCE      10
> +#define REMOTE_DISTANCE     20
> +
> +/*
> + * Parse the '/distance-map' node from the flattened device tree
> + * and extract the 3-tuple triplets <from, to, distance>.
> + */
> +static void __init dt_numa_parse_distance_map(void)
> +{
> +    const void *fdt = device_tree_flattened;
> +    const struct fdt_property *prop;
> +    const __be32 *matrix;
> +    int entry_count;
> +    int node;
> +    int len;
> +    int i;

Nit: Plain int only when values can actually go negative, or when not-yet-
tidied-up code elsewhere (e.g. libfdt here) makes this necessary.

> +    node = fdt_path_offset(fdt, "/distance-map");
> +    if ( node < 0 )
> +        return;
> +
> +    if ( fdt_node_check_compatible(fdt, node, "numa-distance-map-v1") )
> +        return;
> +
> +    prop = fdt_get_property(fdt, node, "distance-matrix", &len);
> +    if ( !prop )
> +        return;
> +
> +    matrix = (const __be32*)prop->data;

Nit: Blank before * please.

> +    entry_count = len / sizeof(__be32);

Nit: Better sizeof(<expression>).

> +    if ( (entry_count <= 0) || (entry_count % 3) )
> +        return;
> +
> +    for ( i = 0; i + 2 < entry_count; i += 3 )
> +    {
> +        uint32_t nodea, nodeb, distance;

Again, no apparent need for a fixed-width type here.

> +        nodea = dt_next_cell(1, &matrix);
> +        nodeb = dt_next_cell(1, &matrix);
> +        distance = dt_next_cell(1, &matrix);
> +
> +        if ( (nodea == nodeb && distance != LOCAL_DISTANCE) ||
> +             (nodea != nodeb && distance <= LOCAL_DISTANCE) )
> +        {
> +            printk(XENLOG_WARNING "Invalid distance[node%d -> node%d] = 
> %d\n",
> +                   nodea, nodeb, distance);

Nit: %u please with unsigned quantities (applies, like all such comments,
also elsewhere).

> +            continue;
> +        }
> +
> +        numa_set_distance(nodea, nodeb, distance);
> +
> +        /* Set default distance of node B->A same as A->B */
> +        if ( nodeb > nodea )
> +            numa_set_distance(nodeb, nodea, distance);
> +    }
> +}
> +
> +void __init dt_numa_distance_table_init(void)
> +{
> +    dt_numa_parse_distance_map();
> +}

I assume there are going to be further additions to this function?

> --- a/xen/common/numa-distance-map.c
> +++ b/xen/common/numa-distance-map.c
> @@ -1,19 +1,62 @@
>  /* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Derived from Linux kernel 7.0's $mm/numa_memblks.c
> + */
>  
>  #include <xen/errno.h>
>  #include <xen/init.h>
>  #include <xen/nodemask.h>
>  #include <xen/numa.h>
> +#include <xen/acpi.h>
> +
>  
>  #define LOCAL_DISTANCE      10

Nit: No double blank lines please.

>  #define REMOTE_DISTANCE     20
>  
> +uint8_t * __ro_after_init numa_distance;

Nit: Excess blank after *.

>  /*
>   * Get the distance between node 'from' and node 'to'.
>   */
>  uint8_t numa_node_distance(unsigned int from, unsigned int to)
>  {
> -    if ( from != to )
> -        return REMOTE_DISTANCE;
> -    return LOCAL_DISTANCE;

Why did you introduce the function as a fallback when now you remove the
fallback logic entirely? Can't you introduce the function right here,
omitting the earlier patch?

> +    const unsigned int nr_nodes = last_node(node_online_map) + 1U;
> +
> +    if ( from >= nr_nodes || to >= nr_nodes )
> +        return from == to ? LOCAL_DISTANCE : REMOTE_DISTANCE;

What if either node is NUMA_NO_NODE?

> +    return numa_distance[from * nr_nodes + to];
> +}
> +
> +void __init numa_set_distance(unsigned int from, unsigned int to,
> +                                     unsigned int distance)

Nit: Indentation.

> +{
> +    const unsigned int nr_nodes = last_node(node_online_map) + 1U;
> +
> +    if ( (uint8_t)distance != distance || from >= nr_nodes || to >= nr_nodes 
> )
> +        printk(XENLOG_WARNING "Invalid distance[node%d -> node%d] = %d\n",
> +               from, to, distance);
> +    else
> +        numa_distance[from * nr_nodes + to] = distance;
> +}
> +
> +void __init numa_distance_table_init(void)
> +{
> +    const unsigned int nr_nodes = last_node(node_online_map) + 1U;
> +    unsigned int i, j;
> +
> +    numa_distance = xzalloc_array(uint8_t, nr_nodes * nr_nodes);

xvzalloc*() family of functions in new code, please.

Further there's an at least abstract risk of the multiplication overflowing.
See how xvmalloc_array() allows for multiple dimensions to be passed.

> +    if ( !numa_distance )
> +        panic("Failed to allocate memory for numa distance-map array\n");
> +
> +    /* fill with the default distances */

Nit: Comment style.

> +    for ( i = 0U; i < nr_nodes; i++ )
> +        for ( j = 0U; j < nr_nodes; j++ )

Why the U suffixes?

> +            numa_distance[i * nr_nodes + j] = i == j ?
> +                LOCAL_DISTANCE : REMOTE_DISTANCE;

While binary operators really want to go at the end of wrapped lines, for
the conditional operator we would generally prefer e.g.

            numa_distance[i * nr_nodes + j] = i == j
                ? LOCAL_DISTANCE : REMOTE_DISTANCE;

while specifically here it might be yet better as

            numa_distance[i * nr_nodes + j] =
                i == j ? LOCAL_DISTANCE : REMOTE_DISTANCE;

You fill the entire array here. Why do you then use the zero-filling form
of the allocation function?

Jan