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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2022 07:34, Wei Chen wrote:
> x86 has implemented a set of codes to process NUMA nodes. These
> codes will parse NUMA memory and processor information from
> ACPI SRAT table. But except some ACPI specific codes, most
> of the process code like memory blocks validation, node memory
> range updates and some sanity check can be reused by other
> NUMA implementation.
> 
> So in this patch, we move some variables and related functions
> for NUMA memory and processor to common as library. At the
> same time, numa_set_processor_nodes_parsed has been introduced
> for ACPI specific code to update processor parsing results.
> With this helper, we can reuse most of NUMA memory affinity init
> code from ACPI. As bad_srat and node_to_pxm functions have been
> used in common code to do architectural fallback and node to
> architectural node info translation. But it doesn't make sense
> to reuse the functions names in common code, we have rename them
> to neutral names as well.
> 
> PXM is an ACPI specific item, we can't use it in common code
> directly. As an alternative, we extend the parameters of
> numa_update_node_memblks. The caller can pass the PXM as print
> messages' prefix or as architectural node id.

The use of "prefix" here must have been stale for a while, perhaps
resulting from an incomplete re-write of what was here earlier on?

> -static int __init nodes_cover_memory(void)
> -{
> -     unsigned int i;
> -
> -     for (i = 0; ; i++) {
> -             int err;
> -             unsigned int j;
> -             bool found;
> -             paddr_t start, end;
> -
> -             /* Try to loop memory map from index 0 to end to get RAM 
> ranges. */
> -             err = arch_get_ram_range(i, &start, &end);
> -
> -             /* Reached the end of the memory map? */
> -             if (err == -ENOENT)
> -                     break;
> -
> -             /* Skip non-RAM entries. */
> -             if (err)
> -                     continue;
> -
> -             do {
> -                     found = false;
> -                     for_each_node_mask(j, memory_nodes_parsed)
> -                             if (start < nodes[j].end
> -                                 && end > nodes[j].start) {
> -                                     if (start >= nodes[j].start) {
> -                                             start = nodes[j].end;
> -                                             found = true;
> -                                     }
> -                                     if (end <= nodes[j].end) {
> -                                             end = nodes[j].start;
> -                                             found = true;
> -                                     }
> -                             }
> -             } 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 ...

> +     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.

Jan