| Message ID | 20210923120236.3692135-8-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device tree based NUMA support to Arm | expand |
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@arm.com> > --- > 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? > { > 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 >
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. 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@arm.com> > > --- > > 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? > > > > { > > 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 > > >
> -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: 2021年9月24日 8:14 > To: Stefano Stabellini <sstabellini@kernel.org> > Cc: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > julien@xen.org; Bertrand Marquis <Bertrand.Marquis@arm.com>; > jbeulich@suse.com; andrew.cooper3@citrix.com; roger.pau@citrix.com; > wl@xen.org > 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@arm.com> > > > --- > > > 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 > > > > >
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
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年1月18日 23:23 > To: Wei Chen <Wei.Chen@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; xen- > devel@lists.xenproject.org; sstabellini@kernel.org; julien@xen.org > 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
On 19.01.2022 07:33, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年1月18日 23:23 >> >> On 23.09.2021 14:02, Wei Chen wrote: >>> @@ -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? And what harm would that do? >>> @@ -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? It may only ever be an up-cast, yet the compiler would do a widening conversion (according to the usual type conversion rules) for us anyway no matter whether there's a cast. Down-casts (in the general compiler case, i.e. considering a wider set than just gcc and clang) sometimes need making explicit to silence compiler warnings about truncation, but I've not observed any compiler warning when widening values. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年1月19日 15:55 > To: Wei Chen <Wei.Chen@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; xen- > devel@lists.xenproject.org; sstabellini@kernel.org; julien@xen.org > Subject: Re: [PATCH 07/37] xen/x86: use paddr_t for addresses in NUMA node > structure > > On 19.01.2022 07:33, Wei Chen wrote: > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: 2022年1月18日 23:23 > >> > >> On 23.09.2021 14:02, Wei Chen wrote: > >>> @@ -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? > > And what harm would that do? Ok, two or three instructions waste in init time will not take too much harm. I will move them above with initializers. > > >>> @@ -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? > > It may only ever be an up-cast, yet the compiler would do a widening > conversion (according to the usual type conversion rules) for us > anyway no matter whether there's a cast. Down-casts (in the general > compiler case, i.e. considering a wider set than just gcc and clang) > sometimes need making explicit to silence compiler warnings about > truncation, but I've not observed any compiler warning when widening > values. Ok, I will drop that cast. > > Jan
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) { 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) { 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);
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@arm.com> --- 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(-)