Re: [PATCH v2 03/17] xen/arm: implement node distance helpers for Arm

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

On 2023/1/12 17:47, Julien Grall wrote:
> Hi,
>
> On 12/01/2023 08:11, Jan Beulich wrote:
> > On 12.01.2023 07:31, Wei Chen wrote:
> > > > -----Original Message-----
> > > > From: Jan Beulich <jbeulich@xxxxxxxx>
> > > > Sent: 2023年1月11日 0:47
> > > >
> > > > On 10.01.2023 09:49, Wei Chen wrote:
> > > > > --- a/xen/arch/arm/include/asm/numa.h
> > > > > +++ b/xen/arch/arm/include/asm/numa.h
> > > > > @@ -28,6 +28,20 @@ enum dt_numa_status {
> > > > >       DT_NUMA_OFF,
> > > > >   };
> > > > >
> > > > > +/*
> > > > > + * In ACPI spec, 0-9 are the reserved values for node distance,
> > > > > + * 10 indicates local node distance, 20 indicates remote node
> > > > > + * distance. Set node distance map in device tree will follow
> > > > > + * the ACPI's definition.
> Looking at the ACPI spec, I agree that the local node distance is 
> defined to 10. But I couldn't find any mention of the value 20.
> How is NUMA_REMOTE_DISTANCE is meant to be used? Is this a default 
> value? If so, maybe we should add "DEFAULT" in the name.
I think you're right, maybe we can rename NUMA_REMOTE_DISTANCE
to NUMA_DEFAULT_DISTANCE.Different pairs of nodes may have different 
remote distance values, I can not define one value for them.
And why use 20 as default value, I have followed the ACPI's logic.
In ACPI NUMA, when ACPI doesn't provide SLIT table, 20 will be used
for all nodes default distance.

> > > > > + */
> > > > > +#define NUMA_DISTANCE_UDF_MIN   0
> > > > > +#define NUMA_DISTANCE_UDF_MAX   9
> > > > > +#define NUMA_LOCAL_DISTANCE     10
> > > > > +#define NUMA_REMOTE_DISTANCE    20
> > > > In the absence of a caller of numa_set_distance() it is entirely 
> > > > unclear
> > > > whether this tying to ACPI used values is actually appropriate.
> > >  From Kernel's NUMA device tree binding, it seems DT NUMA are reusing
> > > ACPI used values for distances [1].
> > I can't find any mention of ACPI in that doc, so the example values used
> > there matching ACPI's may also be coincidental. In no event can a Linux
> > kernel doc serve as DT specification. 
> The directory Documentation/devicetree is the de-facto place where all 
> the bindings are described. This is used by most (to not say all) users.
> I vaguely remember there were plan in the past to move the bindings out 
> of the kernel. But it looks like this has not been done. Yet, they tend 
> to be reviewed independently from the kernel.
> So, as Wei pointed, if we don't follow them then we will not be able to 
> re-use Device-Tree shipped.
> > If values are to match ACPI's, I
> > expect a DT spec to actually say so.
> I don't think it is necessary to say that. Bindings are not meant to 
> change and a developer can rely on the local distance value to not 
> change with the current bindings.
> So IMHO, it is OK to assume that the local distance value is the same 
> between ACPI and DT. That would definitely simplify the parsing and code.
Thanks for clarifying this.

Cheers,
Wei Chen

> Cheers,