Re: [PATCH 07/37] xen/x86: use paddr_t for addresses in NUMA node structure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.09.2021 14:02, Wei Chen wrote:
> @@ -201,11 +201,12 @@ void __init numa_init_array(void)
>  static int numa_fake __initdata = 0;
>  
>  /* Numa emulation */
> -static int __init numa_emulation(u64 start_pfn, u64 end_pfn)
> +static int __init numa_emulation(unsigned long start_pfn,
> +                                 unsigned long end_pfn)
>  {
>      int i;
>      struct node nodes[MAX_NUMNODES];
> -    u64 sz = ((end_pfn - start_pfn)<<PAGE_SHIFT) / numa_fake;
> +    u64 sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;

Nit: Please convert to uint64_t (and alike) whenever you touch a line
anyway that uses being-phased-out types.

> @@ -249,24 +250,26 @@ static int __init numa_emulation(u64 start_pfn, u64 
> end_pfn)
>  void __init numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn)
>  { 
>      int i;
> +    paddr_t start, end;
>  
>  #ifdef CONFIG_NUMA_EMU
>      if ( numa_fake && !numa_emulation(start_pfn, end_pfn) )
>          return;
>  #endif
>  
> +    start = pfn_to_paddr(start_pfn);
> +    end = pfn_to_paddr(end_pfn);

Nit: Would be slightly neater if these were the initializers of the
variables.

>  #ifdef CONFIG_ACPI_NUMA
> -    if ( !numa_off && !acpi_scan_nodes((u64)start_pfn << PAGE_SHIFT,
> -         (u64)end_pfn << PAGE_SHIFT) )
> +    if ( !numa_off && !acpi_scan_nodes(start, end) )
>          return;
>  #endif
>  
>      printk(KERN_INFO "%s\n",
>             numa_off ? "NUMA turned off" : "No NUMA configuration found");
>  
> -    printk(KERN_INFO "Faking a node at %016"PRIx64"-%016"PRIx64"\n",
> -           (u64)start_pfn << PAGE_SHIFT,
> -           (u64)end_pfn << PAGE_SHIFT);
> +    printk(KERN_INFO "Faking a node at %016"PRIpaddr"-%016"PRIpaddr"\n",
> +           start, end);

When switching to PRIpaddr I suppose you did look up what that one
expands to? IOW - please drop the 016 from here.

> @@ -441,7 +441,7 @@ void __init srat_parse_regions(u64 addr)
>           acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat))
>               return;
>  
> -     srat_region_mask = pdx_init_mask(addr);
> +     srat_region_mask = pdx_init_mask((u64)addr);

I don't see the need for a cast here.

> @@ -489,7 +489,7 @@ int __init acpi_scan_nodes(u64 start, u64 end)
>       /* Finally register nodes */
>       for_each_node_mask(i, all_nodes_parsed)
>       {
> -             u64 size = nodes[i].end - nodes[i].start;
> +             paddr_t size = nodes[i].end - nodes[i].start;
>               if ( size == 0 )

Please take the opportunity and add the missing blank line between
declarations and statements.

> --- a/xen/include/asm-x86/numa.h
> +++ b/xen/include/asm-x86/numa.h
> @@ -16,7 +16,7 @@ extern cpumask_t     node_to_cpumask[];
>  #define node_to_cpumask(node)    (node_to_cpumask[node])
>  
>  struct node { 
> -     u64 start,end; 
> +     paddr_t start,end;

Please take the opportunity and add the missing blank after the comma.

Jan