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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [PATCH v4 09/17] xen/arm: introduce a helper to parse device
> tree NUMA distance map
> 
> 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.

Kind regards,
Henry

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