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

[Wei Chen <Wei.Chen@xxxxxxx>]

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Sent: 2021年9月24日 8:14
> To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> julien@xxxxxxx; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> jbeulich@xxxxxxxx; andrew.cooper3@xxxxxxxxxx; roger.pau@xxxxxxxxxx;
> wl@xxxxxxx
> Subject: Re: [PATCH 07/37] xen/x86: use paddr_t for addresses in NUMA node
> structure
> 
> You forgot to add the x86 maintainers in CC to all the patches touching
> x86 code in this series. Adding them now but you should probably resend.
>

I am very sorry about it. I realized the problem when I pressed Enter.
I had wanted to repost it at that time, but I didn't know whether these
patches will be turned into spamming... 

I will resend this series ASAP with some changes to address your comments.

> 
> On Thu, 23 Sep 2021, Stefano Stabellini wrote:
> > On Thu, 23 Sep 2021, Wei Chen wrote:
> > > NUMA node structure "struct node" is using u64 as node memory
> > > range. In order to make other architectures can reuse this
> > > NUMA node relative code, we replace the u64 to paddr_t. And
> > > use pfn_to_paddr and paddr_to_pfn to replace explicit shift
> > > operations. The relate PRIx64 in print messages have been
> > > replaced by PRIpaddr at the same time.
> > >
> > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > ---
> > >  xen/arch/x86/numa.c        | 32 +++++++++++++++++---------------
> > >  xen/arch/x86/srat.c        | 26 +++++++++++++-------------
> > >  xen/include/asm-x86/numa.h |  8 ++++----
> > >  3 files changed, 34 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
> > > index 1fabbe8281..6337bbdf31 100644
> > > --- a/xen/arch/x86/numa.c
> > > +++ b/xen/arch/x86/numa.c
> > > @@ -165,12 +165,12 @@ int __init compute_hash_shift(struct node *nodes,
> int numnodes,
> > >      return shift;
> > >  }
> > >  /* initialize NODE_DATA given nodeid and start/end */
> > > -void __init setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end)
> > > -{
> > > +void __init setup_node_bootmem(nodeid_t nodeid, paddr_t start,
> paddr_t end)
> > > +{
> > >      unsigned long start_pfn, end_pfn;
> > >
> > > -    start_pfn = start >> PAGE_SHIFT;
> > > -    end_pfn = end >> PAGE_SHIFT;
> > > +    start_pfn = paddr_to_pfn(start);
> > > +    end_pfn = paddr_to_pfn(end);
> > >
> > >      NODE_DATA(nodeid)->node_start_pfn = start_pfn;
> > >      NODE_DATA(nodeid)->node_spanned_pages = end_pfn - start_pfn;
> > > @@ -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)
> >
> > Why not changing numa_emulation to take paddr_t too?
> >

numa_emulation parameter is pfn, it's not address. I have discussed
with Julien in RFC about pfn. He suggested to use mfn_t or unsigned
long for pfn. Comparing to mfn_t, use unsigned long brings less
changes.

> >
> > >  {
> > >      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;
> > >
> > >      /* Kludge needed for the hash function */
> > >      if ( hweight64(sz) > 1 )
> > > @@ -221,9 +222,9 @@ static int __init numa_emulation(u64 start_pfn,
> u64 end_pfn)
> > >      memset(&nodes,0,sizeof(nodes));
> > >      for ( i = 0; i < numa_fake; i++ )
> > >      {
> > > -        nodes[i].start = (start_pfn<<PAGE_SHIFT) + i*sz;
> > > +        nodes[i].start = pfn_to_paddr(start_pfn) + i*sz;
> > >          if ( i == numa_fake - 1 )
> > > -            sz = (end_pfn<<PAGE_SHIFT) - nodes[i].start;
> > > +            sz = pfn_to_paddr(end_pfn) - nodes[i].start;
> > >          nodes[i].end = nodes[i].start + sz;
> > >          printk(KERN_INFO "Faking node %d at %"PRIx64"-%"PRIx64"
> (%"PRIu64"MB)\n",
> > >                 i,
> > > @@ -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)
> >
> > same here
> >
> >
> > >  {
> > >      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);
> > > +
> > >  #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);
> > >      /* setup dummy node covering all memory */
> > >      memnode_shift = BITS_PER_LONG - 1;
> > >      memnodemap = _memnodemap;
> > > @@ -279,8 +282,7 @@ void __init numa_initmem_init(unsigned long
> start_pfn, unsigned long end_pfn)
> > >      for ( i = 0; i < nr_cpu_ids; i++ )
> > >          numa_set_node(i, 0);
> > >      cpumask_copy(&node_to_cpumask[0], cpumask_of(0));
> > > -    setup_node_bootmem(0, (u64)start_pfn << PAGE_SHIFT,
> > > -                    (u64)end_pfn << PAGE_SHIFT);
> > > +    setup_node_bootmem(0, start, end);
> > >  }
> > >
> > >  void numa_add_cpu(int cpu)
> > > diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> > > index 6b77b98201..7d20d7f222 100644
> > > --- a/xen/arch/x86/srat.c
> > > +++ b/xen/arch/x86/srat.c
> > > @@ -104,7 +104,7 @@ nodeid_t setup_node(unsigned pxm)
> > >   return node;
> > >  }
> > >
> > > -int valid_numa_range(u64 start, u64 end, nodeid_t node)
> > > +int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node)
> > >  {
> > >   int i;
> > >
> > > @@ -119,7 +119,7 @@ int valid_numa_range(u64 start, u64 end, nodeid_t
> node)
> > >   return 0;
> > >  }
> > >
> > > -static __init int conflicting_memblks(u64 start, u64 end)
> > > +static __init int conflicting_memblks(paddr_t start, paddr_t end)
> > >  {
> > >   int i;
> > >
> > > @@ -135,7 +135,7 @@ static __init int conflicting_memblks(u64 start,
> u64 end)
> > >   return -1;
> > >  }
> > >
> > > -static __init void cutoff_node(int i, u64 start, u64 end)
> > > +static __init void cutoff_node(int i, paddr_t start, paddr_t end)
> > >  {
> > >   struct node *nd = &nodes[i];
> > >   if (nd->start < start) {
> > > @@ -275,7 +275,7 @@ acpi_numa_processor_affinity_init(const struct
> acpi_srat_cpu_affinity *pa)
> > >  void __init
> > >  acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity
> *ma)
> > >  {
> > > - u64 start, end;
> > > + paddr_t start, end;
> > >   unsigned pxm;
> > >   nodeid_t node;
> > >   int i;
> > > @@ -318,7 +318,7 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> > >           bool mismatch = !(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) !=
> > >                           !test_bit(i, memblk_hotplug);
> > >
> > > -         printk("%sSRAT: PXM %u (%"PRIx64"-%"PRIx64") overlaps with
> itself (%"PRIx64"-%"PRIx64")\n",
> > > +         printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with
> itself (%"PRIpaddr"-%"PRIpaddr")\n",
> > >                  mismatch ? KERN_ERR : KERN_WARNING, pxm, start, end,
> > >                  node_memblk_range[i].start, node_memblk_range[i].end);
> > >           if (mismatch) {
> > > @@ -327,7 +327,7 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> > >           }
> > >   } else {
> > >           printk(KERN_ERR
> > > -                "SRAT: PXM %u (%"PRIx64"-%"PRIx64") overlaps with
> PXM %u (%"PRIx64"-%"PRIx64")\n",
> > > +                "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with
> PXM %u (%"PRIpaddr"-%"PRIpaddr")\n",
> > >                  pxm, start, end, node_to_pxm(memblk_nodeid[i]),
> > >                  node_memblk_range[i].start, node_memblk_range[i].end);
> > >           bad_srat();
> > > @@ -346,7 +346,7 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> > >                           nd->end = end;
> > >           }
> > >   }
> > > - printk(KERN_INFO "SRAT: Node %u PXM %u %"PRIx64"-%"PRIx64"%s\n",
> > > + printk(KERN_INFO "SRAT: Node %u PXM %u %"PRIpaddr"-%"PRIpaddr"%s\n",
> > >          node, pxm, start, end,
> > >          ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
> > >
> > > @@ -369,7 +369,7 @@ static int __init nodes_cover_memory(void)
> > >
> > >   for (i = 0; i < e820.nr_map; i++) {
> > >           int j, found;
> > > -         unsigned long long start, end;
> > > +         paddr_t start, end;
> > >
> > >           if (e820.map[i].type != E820_RAM) {
> > >                   continue;
> > > @@ -396,7 +396,7 @@ static int __init nodes_cover_memory(void)
> > >
> > >           if (start < end) {
> > >                   printk(KERN_ERR "SRAT: No PXM for e820 range: "
> > > -                         "%016Lx - %016Lx\n", start, end);
> > > +                         "%"PRIpaddr" - %"PRIpaddr"\n", start, end);
> > >                   return 0;
> > >           }
> > >   }
> > > @@ -432,7 +432,7 @@ static int __init srat_parse_region(struct
> acpi_subtable_header *header,
> > >   return 0;
> > >  }
> > >
> > > -void __init srat_parse_regions(u64 addr)
> > > +void __init srat_parse_regions(paddr_t addr)
> > >  {
> > >   u64 mask;
> > >   unsigned int i;
> > > @@ -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);
> > >   acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
> > >                         srat_parse_region, 0);
> > >
> > > @@ -457,7 +457,7 @@ void __init srat_parse_regions(u64 addr)
> > >  }
> > >
> > >  /* Use the information discovered above to actually set up the nodes.
> */
> > > -int __init acpi_scan_nodes(u64 start, u64 end)
> > > +int __init acpi_scan_nodes(paddr_t start, paddr_t end)
> > >  {
> > >   int i;
> > >   nodemask_t all_nodes_parsed;
> > > @@ -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 )
> > >                   printk(KERN_WARNING "SRAT: Node %u has no memory. "
> > >                          "BIOS Bug or mis-configured hardware?\n", i);
> > > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> > > index 8060cbf3f4..50cfd8e7ef 100644
> > > --- 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;
> > >  };
> > >
> > >  extern int compute_hash_shift(struct node *nodes, int numnodes,
> > > @@ -36,7 +36,7 @@ extern void numa_set_node(int cpu, nodeid_t node);
> > >  extern nodeid_t setup_node(unsigned int pxm);
> > >  extern void srat_detect_node(int cpu);
> > >
> > > -extern void setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end);
> > > +extern void setup_node_bootmem(nodeid_t nodeid, paddr_t start,
> paddr_t end);
> > >  extern nodeid_t apicid_to_node[];
> > >  extern void init_cpu_to_node(void);
> > >
> > > @@ -73,9 +73,9 @@ static inline __attribute__((pure)) nodeid_t
> phys_to_nid(paddr_t addr)
> > >  #define node_end_pfn(nid)       (NODE_DATA(nid)->node_start_pfn + \
> > >                            NODE_DATA(nid)->node_spanned_pages)
> > >
> > > -extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
> > > +extern int valid_numa_range(paddr_t start, paddr_t end, nodeid_t
> node);
> > >
> > > -void srat_parse_regions(u64 addr);
> > > +void srat_parse_regions(paddr_t addr);
> > >  extern u8 __node_distance(nodeid_t a, nodeid_t b);
> > >  unsigned int arch_get_dma_bitsize(void);
> > >  unsigned int arch_have_default_dmazone(void);
> > > --
> > > 2.25.1
> > >
> >