Re: [PATCH v4 09/17] xen/arm: introduce a helper to parse device tree NUMA distance map

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.04.2023 09:56, Henry Wang wrote:
> --- a/xen/arch/arm/numa_device_tree.c
> +++ b/xen/arch/arm/numa_device_tree.c
> @@ -151,3 +151,111 @@ invalid_data:
>      numa_fw_bad();
>      return -EINVAL;
>  }
> +
> +/* Parse NUMA distance map v1 */
> +static int __init fdt_parse_numa_distance_map_v1(const void *fdt, int node)
> +{
> +    const struct fdt_property *prop;
> +    const __be32 *matrix;
> +    unsigned int i, entry_count;
> +    int len;
> +
> +    printk(XENLOG_INFO "NUMA: parsing numa-distance-map\n");
> +
> +    prop = fdt_get_property(fdt, node, "distance-matrix", &len);
> +    if ( !prop )
> +    {
> +        printk(XENLOG_WARNING
> +               "NUMA: No distance-matrix property in distance-map\n");
> +        goto invalid_data;
> +    }
> +
> +    if ( len % sizeof(__be32) != 0 )
> +    {
> +        printk(XENLOG_WARNING
> +               "distance-matrix in node is not a multiple of u32\n");
> +        goto invalid_data;
> +    }
> +
> +    entry_count = len / sizeof(__be32);
> +    if ( entry_count == 0 )
> +    {
> +        printk(XENLOG_WARNING "NUMA: Invalid distance-matrix\n");
> +        goto invalid_data;
> +    }
> +
> +    matrix = (const __be32 *)prop->data;
> +    for ( i = 0; i + 2 < entry_count; i += 3 )
> +    {
> +        unsigned int from, to, distance, opposite;
> +
> +        from = dt_next_cell(1, &matrix);
> +        to = dt_next_cell(1, &matrix);
> +        distance = dt_next_cell(1, &matrix);

Upon second thought I checked what dt_next_cell() returns: You're silently
truncating here and then ...

> +        if ( (from == to && distance != NUMA_LOCAL_DISTANCE) ||
> +             (from != to && distance <= NUMA_LOCAL_DISTANCE) )
> +        {
> +            printk(XENLOG_WARNING
> +                   "NUMA: Invalid distance: 
> NODE#%"PRIu32"->NODE#%"PRIu32":%"PRIu32"\n",
> +                   from, to, distance);
> +            goto invalid_data;
> +        }
> +
> +        printk(XENLOG_INFO "NUMA: distance: 
> NODE#%"PRIu32"->NODE#%"PRIu32":%"PRIu32"\n",
> +               from, to, distance);
> +
> +        /* Get opposite way distance */
> +        opposite = __node_distance(to, from);
> +        /* The default value in node_distance_map is NUMA_NO_DISTANCE */
> +        if ( opposite == NUMA_NO_DISTANCE )
> +        {
> +            /* Bi-directions are not set, set both */
> +            numa_set_distance(from, to, distance);
> +            numa_set_distance(to, from, distance);

... here again. Is that really the intention?

Jan