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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.04.2023 08:29, Henry Wang wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>
>>> +        distance = dt_next_cell(1, &matrix);
>>
>> Upon second thought I checked what dt_next_cell() returns: You're silently
>> truncating here and then ...
>>
>>> +            /* 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?
> 
> By hunting down the historical discussions I found that using dt_next_cell() 
> is
> what Julien suggested 2 years ago in the RFC series [1]. Given the truncation
> here is for node id (from/to) and distance which I am pretty sure will not
> exceed 32-bit range, I think the silent truncation is safe.

That discussion is orthogonal; the previously used dt_read_number() is no
different in the regard I'm referring to.

> However I understand your point here, the silent truncation is not ideal, so
> I wonder if you have any suggestions to improve, do you think I should change
> these variables to u64 or maybe I need to do the explicit type cast or any
> better suggestions from you? Thanks!

So one thing I overlooked is that by passing 1 as the first argument, you
only request a 32-bit value. Hence there's no (silent) truncation then on
the dt_next_cell() uses. But the numa_set_distance() calls still truncate
to 8 bits. Adding explicit type casts won't help at all - truncation will
remain as silent as it was before. However, numa_set_distance()'s first
two arguments could easily become "unsigned int", resulting in its node
related bounds checking to take care of all truncation issues. The
"distance" parameter already is unsigned int, and is already being bounds
checked against NUMA_NO_DISTANCE.

Jan

> [1] https://lists.xenproject.org/archives/html/xen-devel/2021-08/msg01175.html
> 
> Kind regards,
> Henry