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 09:36, Henry Wang wrote:
>> -----Original Message-----
>> From: 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.
> 
> Great points! Thanks for pointing the 8-bit truncation out. You are correct.
> Somehow my impression of numa_set_distance()'s first two arguments are
> already "unsigned int" so I missed this part...Sorry.
> 
> In that case, I think I will add a check between "from, to" and MAX_NUMNODES
> as soon as the values of "from" and "to" are populated by dt_next_cell().
> Hopefully this will address your concern.

While this would address by concern, I don't see why you want to repeat
the checking that numa_set_distance() already does.

Jan