Re: [PATCH v8 5/6] xen/x86: move NUMA process nodes nodes code from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2022 08:23, Wei Chen wrote:
>>> -static int __init nodes_cover_memory(void)
>>> -{
>>> -   unsigned int i;
>>> -
>>> -   for (i = 0; ; i++) {
>>> -                           }
>>> -           } while (found && start < end);
>>> -
>>> -           if (start < end) {
>>> -                   printk(KERN_ERR "NUMA: No NODE for RAM range: "
>>> -                           "[%"PRIpaddr", %"PRIpaddr"]\n", start, end - 1);
>>> -                   return 0;
>>> -           }
>>> -   }
>>> -   return 1;
>>> +   numa_fw_nid_name = "PXM";
>>
>> I guess this can't go without a comment, now that we have ...
>>
> 
> Ok, how about following comment:
>       /*
>        * In an extremely unlikely case, srat_parse_regions might not
>        * be called. So set "PXM" before the first caller to use it to
>        * make it more safe.
>        */

Largely okay, but "first caller" looks wrong here (without further
attribution I would read it as meaning "caller of the function we're
in", which makes no sense). Hence I'd make the 2nd sentence just "So
set the variable here just in case."

> And ...
> 
>>> +   if (!numa_update_node_memblks(node, pxm, ma->base_address, ma-
>>> length,
>>> +                                 ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE))
>>> +           numa_fw_bad();
>>>  }
>>>
>>>  void __init acpi_numa_arch_fixup(void) {}
>>> @@ -534,6 +295,7 @@ void __init srat_parse_regions(paddr_t addr)
>>>         acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat))
>>>             return;
>>>
>>> +   numa_fw_nid_name = "PXM";
>>
>> ... this as well. Otherwise someone may spot the redundancy and either
>> propose to drop one, or it'll take them quite a bit of time to figure
>> why both are there. I thought this would go without saying, so I'm
>> sorry for not making this explicit earlier on.
> 
> ...
>       /* Set "PXM" as earlier as we can for those functions will use it. */
>>> +   numa_fw_nid_name = "PXM";

Just 'Set "PXM" as early as feasible'? ("can" would be wrong, because in
principle we could of course set it earlier. Yet we want to keep the
setting within the present flow of setup, and without introducing a
layering violation.)

Jan