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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年1月18日 23:23
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [PATCH 07/37] xen/x86: use paddr_t for addresses in NUMA node
> structure
> 
> 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.
> 

Ok, I will do it.

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

But if this function returns in numa_fake && !numa_emulation,
will the two pfn_to_paddr operations be waste?

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

Oh, yes, I forgot to drop the duplicated 016. I would do it.

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

current addr type has been changed to paddr_t, but pdx_init_mask
accept parameter type is u64. I know paddr_t is a typedef of
u64 on Arm64/32, or unsinged long on x86. In current stage,
their machine byte-lengths are the same. But in case of future
changes I think an explicit case here maybe better? 

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

Ok

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

Ok

> Jan