RE: [XEN RFC PATCH 24/40] xen/arm: introduce a helper to parse device tree NUMA distance map

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月25日 21:56
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>
> Subject: Re: [XEN RFC PATCH 24/40] xen/arm: introduce a helper to parse
> device tree NUMA distance map
> 
> Hi Wei,
> 
> On 11/08/2021 11:24, Wei Chen wrote:
> > A NUMA aware device tree will provide a "distance-map" node to
> > describe distance between any two nodes. This patch introduce a
> 
> s/introduce/introduces/

OK

> 
> > new helper to parse this distance map.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/numa_device_tree.c | 67 +++++++++++++++++++++++++++++++++
> >   1 file changed, 67 insertions(+)
> >
> > diff --git a/xen/arch/arm/numa_device_tree.c
> b/xen/arch/arm/numa_device_tree.c
> > index bbe081dcd1..6e0d1d3d9f 100644
> > --- a/xen/arch/arm/numa_device_tree.c
> > +++ b/xen/arch/arm/numa_device_tree.c
> > @@ -200,3 +200,70 @@ device_tree_parse_numa_memory_node(const void *fdt,
> int node,
> >
> >       return 0;
> >   }
> > +
> > +/* Parse NUMA distance map v1 */
> > +int __init
> > +device_tree_parse_numa_distance_map_v1(const void *fdt, int node)
> > +{
> > +    const struct fdt_property *prop;
> > +    const __be32 *matrix;
> > +    int entry_count, len, i;
> 
> entry_count and i should be unsigned. len unfortunately can't because
> fdt_get_property expects a signed int.
> 

OK

> > +
> > +    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");
> > +
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ( len % sizeof(uint32_t) != 0 )
> > +    {
> > +        printk(XENLOG_WARNING
> > +               "distance-matrix in node is not a multiple of u32\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    entry_count = len / sizeof(uint32_t);
> > +    if ( entry_count <= 0 )
> 
> I understand that entry_count may be 0. But I can't see how it can be
> negative as the property len cannot be (even if it is a signed type). So
> I think this wants to be "== 0".
> 

From the fdt_get_property's comment, when prop is NULL, the len can
be negative. But, yes, prop==NULL check will handled before this code.
negative len will not reach here. I am ok to change it to "== 0"

> > +    {
> > +        printk(XENLOG_WARNING "NUMA: Invalid distance-matrix\n");
> > +
> > +        return -EINVAL;
> > +    }
> > +
> > +    matrix = (const __be32 *)prop->data;
> > +    for ( i = 0; i + 2 < entry_count; i += 3 )
> > +    {
> > +        uint32_t from, to, distance;
> > +
> > +        from = dt_read_number(matrix, 1);
> > +        matrix++;
> 
> You can use dt_next_cell() which will update the pointer for you.
> 

Thanks

> > +        to = dt_read_number(matrix, 1);
> > +        matrix++;
> > +        distance = dt_read_number(matrix, 1);
> > +        matrix++;
> > +
> > +        if ( (from == to && distance != NUMA_LOCAL_DISTANCE) ||
> > +            (from != to && distance <= NUMA_LOCAL_DISTANCE) )
> > +        {
> > +            printk(XENLOG_WARNING
> > +                   "Invalid nodes' distance from node#%d to node#%d
> = %d\n",
> > +                   from, to, distance);
> > +            return -EINVAL;
> > +        }
> > +
> > +        printk(XENLOG_INFO "NUMA: distance from node#%d to node#%d
> = %d\n",
> > +               from, to, distance);
> > +        numa_set_distance(from, to, distance);
> > +
> > +        /* Set default distance of node B->A same as A->B */
> > +        if (to > from)
> > +             numa_set_distance(to, from, distance);
> > +    }
> > +
> > +    return 0;
> > +}
> >
> 
> Cheers,
> 
> --
> Julien Grall