Re: [PATCH v5 2/6] xen/x86: move generically usable NUMA code from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2022 10:19, Jan Beulich wrote:
> On 20.09.2022 11:12, Wei Chen wrote:
>> +static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
>> +                                                  nodeid_t numnodes)
>> +{
>> +    unsigned int i, nodes_used = 0;
>> +    unsigned long spdx, epdx;
>> +    unsigned long bitfield = 0, memtop = 0;
>> +
>> +    for ( i = 0; i < numnodes; i++ )
>> +    {
>> +        spdx = paddr_to_pdx(nodes[i].start);
>> +        epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
>> +
>> +        if ( spdx >= epdx )
>> +            continue;
>> +
>> +        bitfield |= spdx;
> 
> Perhaps to be taken care of in a separate patch: We accumulate only
> the bits from the start addresses here, contrary to what the comment
> ahead of the function says (and I think it is the comment which is
> putting things correctly).

It's the comment which is wrong - it wasn't updated in Linux commit
54413927f022 ("x86-64: x86_64-make-the-numa-hash-function-nodemap-allocation
fix fix"). Our code was cloned from Linux'es. In fact when memory is not
contiguous, too small a shift value might be determined. This in particular
also may matter for the lowest range covered by any node: On x86 this will
always start at zero (and hence won't influence the final calculation), but
iirc on Arm memory may (and typically will) start at non-zero addresses. In
such a case the first node's start address should be ignored, as it's fine
to (kind of) associate all lower addresses (where no memory is) with this
same node. But that's now indeed something which will want taking care of
while making the code usable for other architectures.

Jan