Re: [PATCH v2 6/9] xen/x86: move NUMA scan nodes codes from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.07.2022 16:54, Wei Chen wrote:
> x86 has implemented a set of codes to scan NUMA nodes. These
> codes will parse NUMA memory and processor information from
> ACPI SRAT table. But except some ACPI specific codes, most
> of the scan codes 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 code. 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 move most of NUMA memory affinity init code from
> ACPI. And bad_srat and node_to_pxm functions have been exported
> for common code to do architectural fallback and node to proximity
> converting.

I consider it wrong for generic (ACPI-independent) code to use
terms like "srat" or "pxm". This wants abstracting in some way,
albeit I have to admit I lack a good idea for a suggestion right
now.

> For those NUMA implementations haven't supported
> proximity domain to node map. A simple 1-1 mapping helper can help
> us to avoid the modification of some very valuable print messages.

I don't think a simple 1:1 mapping can do. Surely firmware
represents nodes by some identifiers everywhere. And I don't
consider it very likely that we would want to blindly re-use
such numbering inside Xen.

> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -14,6 +14,21 @@
>  #include <xen/softirq.h>
>  #include <asm/acpi.h>
>  
> +static nodemask_t __initdata processor_nodes_parsed;
> +static nodemask_t __initdata memory_nodes_parsed;
> +static struct node __initdata nodes[MAX_NUMNODES];
> +
> +static int num_node_memblks;
> +static struct node node_memblk_range[NR_NODE_MEMBLKS];
> +static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];

Some (all) of these likely want to become __read_mostly again, at
this occasion.

> @@ -32,6 +47,298 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL 
> } };
>  
>  enum numa_mode numa_status;
>  
> +void __init numa_set_processor_nodes_parsed(nodeid_t node)
> +{
> +    node_set(node, processor_nodes_parsed);
> +}
> +
> +int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node)
> +{
> +    int i;

unsigned int? Then matching e.g. ...

> +static
> +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start,
> +                                          paddr_t end, paddr_t nd_start,
> +                                          paddr_t nd_end, unsigned int 
> *mblkid)
> +{
> +    unsigned int i;

... this. But perhaps also elsewhere.

Jan