| Message ID | 20210923120236.3692135-13-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device tree based NUMA support to Arm | expand |
+x86 maintainers On Thu, 23 Sep 2021, Wei Chen wrote: > We will reuse nodes_cover_memory for Arm to check its bootmem > info. So we introduce two arch helpers to get memory map's > entry number and specified entry's range: > arch_get_memory_bank_number > arch_get_memory_bank_range > > Depends above two helpers, we make nodes_cover_memory become > architecture independent. And the only change from an x86 > perspective is the additional checks: > !start || !end > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/x86/numa.c | 18 ++++++++++++++++++ > xen/arch/x86/srat.c | 11 ++++------- > xen/include/asm-x86/numa.h | 3 +++ > 3 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c > index 6337bbdf31..6bc4ade411 100644 > --- a/xen/arch/x86/numa.c > +++ b/xen/arch/x86/numa.c > @@ -378,6 +378,24 @@ unsigned int arch_have_default_dmazone(void) > return ( num_online_nodes() > 1 ) ? 1 : 0; > } > > +uint32_t __init arch_meminfo_get_nr_bank(void) > +{ > + return e820.nr_map; > +} > + > +int __init arch_meminfo_get_ram_bank_range(uint32_t bank, > + paddr_t *start, paddr_t *end) > +{ > + if (e820.map[bank].type != E820_RAM || !start || !end) { > + return -1; > + } > + > + *start = e820.map[bank].addr; > + *end = e820.map[bank].addr + e820.map[bank].size; > + > + return 0; > +} > + > static void dump_numa(unsigned char key) > { > s_time_t now = NOW(); > diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c > index 18bc6b19bb..aa07a7e975 100644 > --- a/xen/arch/x86/srat.c > +++ b/xen/arch/x86/srat.c > @@ -419,17 +419,14 @@ acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) > static int __init nodes_cover_memory(void) > { > int i; > + uint32_t nr_banks = arch_meminfo_get_nr_bank(); > > - for (i = 0; i < e820.nr_map; i++) { > + for (i = 0; i < nr_banks; i++) { > int j, found; > paddr_t start, end; > > - if (e820.map[i].type != E820_RAM) { > + if (arch_meminfo_get_ram_bank_range(i, &start, &end)) > continue; > - } > - > - start = e820.map[i].addr; > - end = e820.map[i].addr + e820.map[i].size; > > do { > found = 0; > @@ -448,7 +445,7 @@ static int __init nodes_cover_memory(void) > } while (found && start < end); > > if (start < end) { > - printk(KERN_ERR "SRAT: No PXM for e820 range: " > + printk(KERN_ERR "SRAT: No NODE for memory map range: " > "%"PRIpaddr" - %"PRIpaddr"\n", start, end); > return 0; > } > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h > index 5772a70665..78e044a390 100644 > --- a/xen/include/asm-x86/numa.h > +++ b/xen/include/asm-x86/numa.h > @@ -82,5 +82,8 @@ 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); > +extern uint32_t arch_meminfo_get_nr_bank(void); > +extern int arch_meminfo_get_ram_bank_range(uint32_t bank, > + paddr_t *start, paddr_t *end); > > #endif > -- > 2.25.1 >
On 23.09.2021 14:02, Wei Chen wrote: > We will reuse nodes_cover_memory for Arm to check its bootmem > info. So we introduce two arch helpers to get memory map's > entry number and specified entry's range: > arch_get_memory_bank_number > arch_get_memory_bank_range I'm sorry, but personally I see no way for you to introduce the term "memory bank" into x86 code. > --- a/xen/arch/x86/numa.c > +++ b/xen/arch/x86/numa.c > @@ -378,6 +378,24 @@ unsigned int arch_have_default_dmazone(void) > return ( num_online_nodes() > 1 ) ? 1 : 0; > } > > +uint32_t __init arch_meminfo_get_nr_bank(void) unsigned int (also elsewhere) > +{ > + return e820.nr_map; > +} > + > +int __init arch_meminfo_get_ram_bank_range(uint32_t bank, > + paddr_t *start, paddr_t *end) > +{ > + if (e820.map[bank].type != E820_RAM || !start || !end) { I see no reason for the checking of start and end. > + return -1; > + } No need for braces here. > + *start = e820.map[bank].addr; > + *end = e820.map[bank].addr + e820.map[bank].size; > + > + return 0; > +} > + > static void dump_numa(unsigned char key) > { > s_time_t now = NOW(); Judging by just the two pieces of patch context you're inserting a Linux-style code fragment in the middle of two Xen-style ones. Various other comments given for earlier patches apply here as well. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年1月25日 0:59 > 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 12/37] xen/x86: decouple nodes_cover_memory from E820 > map > > On 23.09.2021 14:02, Wei Chen wrote: > > We will reuse nodes_cover_memory for Arm to check its bootmem > > info. So we introduce two arch helpers to get memory map's > > entry number and specified entry's range: > > arch_get_memory_bank_number > > arch_get_memory_bank_range > > I'm sorry, but personally I see no way for you to introduce the term > "memory bank" into x86 code. > In my latest changes, I have updated these two helpers to: uint32_t __init arch_meminfo_get_nr_bank(void) __init arch_meminfo_get_ram_bank_range(...) I am sorry, I forgot to change the commit log accordingly. I will update it in next version. > > --- a/xen/arch/x86/numa.c > > +++ b/xen/arch/x86/numa.c > > @@ -378,6 +378,24 @@ unsigned int arch_have_default_dmazone(void) > > return ( num_online_nodes() > 1 ) ? 1 : 0; > > } > > > > +uint32_t __init arch_meminfo_get_nr_bank(void) > > unsigned int (also elsewhere) > OK. > > +{ > > + return e820.nr_map; > > +} > > + > > +int __init arch_meminfo_get_ram_bank_range(uint32_t bank, > > + paddr_t *start, paddr_t *end) > > +{ > > + if (e820.map[bank].type != E820_RAM || !start || !end) { > > I see no reason for the checking of start and end. > Yes, I have removed this checking in the latest version. > > + return -1; > > + } > > No need for braces here. > Ok. > > + *start = e820.map[bank].addr; > > + *end = e820.map[bank].addr + e820.map[bank].size; > > + > > + return 0; > > +} > > + > > static void dump_numa(unsigned char key) > > { > > s_time_t now = NOW(); > > Judging by just the two pieces of patch context you're inserting > a Linux-style code fragment in the middle of two Xen-style ones. > > Various other comments given for earlier patches apply here as well. > Yes, the original file is xen-style. I will change the inserted code to xen-style too. Thanks! > Jan
On 27.01.2022 09:03, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年1月25日 0:59 >> >> On 23.09.2021 14:02, Wei Chen wrote: >>> We will reuse nodes_cover_memory for Arm to check its bootmem >>> info. So we introduce two arch helpers to get memory map's >>> entry number and specified entry's range: >>> arch_get_memory_bank_number >>> arch_get_memory_bank_range >> >> I'm sorry, but personally I see no way for you to introduce the term >> "memory bank" into x86 code. > > In my latest changes, I have updated these two helpers to: > uint32_t __init arch_meminfo_get_nr_bank(void) > __init arch_meminfo_get_ram_bank_range(...) > I am sorry, I forgot to change the commit log accordingly. > I will update it in next version. I'm sorry for the ambiguity of my earlier reply, but my objection was against "bank", not "memory". As an aside, you also don't want the function return "uint32_t" - see ./CODING_STYLE. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年1月27日 16:09 > 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 12/37] xen/x86: decouple nodes_cover_memory from E820 > map > > On 27.01.2022 09:03, Wei Chen wrote: > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: 2022年1月25日 0:59 > >> > >> On 23.09.2021 14:02, Wei Chen wrote: > >>> We will reuse nodes_cover_memory for Arm to check its bootmem > >>> info. So we introduce two arch helpers to get memory map's > >>> entry number and specified entry's range: > >>> arch_get_memory_bank_number > >>> arch_get_memory_bank_range > >> > >> I'm sorry, but personally I see no way for you to introduce the term > >> "memory bank" into x86 code. > > > > In my latest changes, I have updated these two helpers to: > > uint32_t __init arch_meminfo_get_nr_bank(void) > > __init arch_meminfo_get_ram_bank_range(...) > > I am sorry, I forgot to change the commit log accordingly. > > I will update it in next version. > > I'm sorry for the ambiguity of my earlier reply, but my objection was > against "bank", not "memory". As an aside, you also don't want the How about arch_meminfo_get_nr_map/ arch_meminfo_get_map_range? I am sorry, I am not very familiar with e820 map, could you give me some suggestions? > function return "uint32_t" - see ./CODING_STYLE. OK. > > Jan
On 27.01.2022 10:03, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年1月27日 16:09 >> >> On 27.01.2022 09:03, Wei Chen wrote: >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: 2022年1月25日 0:59 >>>> >>>> On 23.09.2021 14:02, Wei Chen wrote: >>>>> We will reuse nodes_cover_memory for Arm to check its bootmem >>>>> info. So we introduce two arch helpers to get memory map's >>>>> entry number and specified entry's range: >>>>> arch_get_memory_bank_number >>>>> arch_get_memory_bank_range >>>> >>>> I'm sorry, but personally I see no way for you to introduce the term >>>> "memory bank" into x86 code. >>> >>> In my latest changes, I have updated these two helpers to: >>> uint32_t __init arch_meminfo_get_nr_bank(void) >>> __init arch_meminfo_get_ram_bank_range(...) >>> I am sorry, I forgot to change the commit log accordingly. >>> I will update it in next version. >> >> I'm sorry for the ambiguity of my earlier reply, but my objection was >> against "bank", not "memory". As an aside, you also don't want the > > How about arch_meminfo_get_nr_map/ arch_meminfo_get_map_range? > I am sorry, I am not very familiar with e820 map, could you > give me some suggestions? First of all I don't think you need a "get_nr" function at all, which eliminates the need to find a good name for it. The "get_range" function can easily provide back a unique indicator when the passed in index is beyond the number of regions. For this function's name, how about arch_get_memory_map() or arch_get_memory_map_block() or arch_get_memory_map_range() (in order of my personal preference)? Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年1月27日 17:22 > 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 12/37] xen/x86: decouple nodes_cover_memory from E820 > map > > On 27.01.2022 10:03, Wei Chen wrote: > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: 2022年1月27日 16:09 > >> > >> On 27.01.2022 09:03, Wei Chen wrote: > >>>> From: Jan Beulich <jbeulich@suse.com> > >>>> Sent: 2022年1月25日 0:59 > >>>> > >>>> On 23.09.2021 14:02, Wei Chen wrote: > >>>>> We will reuse nodes_cover_memory for Arm to check its bootmem > >>>>> info. So we introduce two arch helpers to get memory map's > >>>>> entry number and specified entry's range: > >>>>> arch_get_memory_bank_number > >>>>> arch_get_memory_bank_range > >>>> > >>>> I'm sorry, but personally I see no way for you to introduce the term > >>>> "memory bank" into x86 code. > >>> > >>> In my latest changes, I have updated these two helpers to: > >>> uint32_t __init arch_meminfo_get_nr_bank(void) > >>> __init arch_meminfo_get_ram_bank_range(...) > >>> I am sorry, I forgot to change the commit log accordingly. > >>> I will update it in next version. > >> > >> I'm sorry for the ambiguity of my earlier reply, but my objection was > >> against "bank", not "memory". As an aside, you also don't want the > > > > How about arch_meminfo_get_nr_map/ arch_meminfo_get_map_range? > > I am sorry, I am not very familiar with e820 map, could you > > give me some suggestions? > > First of all I don't think you need a "get_nr" function at all, which > eliminates the need to find a good name for it. The "get_range" function > can easily provide back a unique indicator when the passed in index is > beyond the number of regions. For this function's name, how about > arch_get_memory_map() or arch_get_memory_map_block() or > arch_get_memory_map_range() (in order of my personal preference)? > Thanks, I will try to use arch_get_memory_map for x86 and arm. > Jan
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c index 6337bbdf31..6bc4ade411 100644 --- a/xen/arch/x86/numa.c +++ b/xen/arch/x86/numa.c @@ -378,6 +378,24 @@ unsigned int arch_have_default_dmazone(void) return ( num_online_nodes() > 1 ) ? 1 : 0; } +uint32_t __init arch_meminfo_get_nr_bank(void) +{ + return e820.nr_map; +} + +int __init arch_meminfo_get_ram_bank_range(uint32_t bank, + paddr_t *start, paddr_t *end) +{ + if (e820.map[bank].type != E820_RAM || !start || !end) { + return -1; + } + + *start = e820.map[bank].addr; + *end = e820.map[bank].addr + e820.map[bank].size; + + return 0; +} + static void dump_numa(unsigned char key) { s_time_t now = NOW(); diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c index 18bc6b19bb..aa07a7e975 100644 --- a/xen/arch/x86/srat.c +++ b/xen/arch/x86/srat.c @@ -419,17 +419,14 @@ acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma) static int __init nodes_cover_memory(void) { int i; + uint32_t nr_banks = arch_meminfo_get_nr_bank(); - for (i = 0; i < e820.nr_map; i++) { + for (i = 0; i < nr_banks; i++) { int j, found; paddr_t start, end; - if (e820.map[i].type != E820_RAM) { + if (arch_meminfo_get_ram_bank_range(i, &start, &end)) continue; - } - - start = e820.map[i].addr; - end = e820.map[i].addr + e820.map[i].size; do { found = 0; @@ -448,7 +445,7 @@ static int __init nodes_cover_memory(void) } while (found && start < end); if (start < end) { - printk(KERN_ERR "SRAT: No PXM for e820 range: " + printk(KERN_ERR "SRAT: No NODE for memory map range: " "%"PRIpaddr" - %"PRIpaddr"\n", start, end); return 0; } diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h index 5772a70665..78e044a390 100644 --- a/xen/include/asm-x86/numa.h +++ b/xen/include/asm-x86/numa.h @@ -82,5 +82,8 @@ 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); +extern uint32_t arch_meminfo_get_nr_bank(void); +extern int arch_meminfo_get_ram_bank_range(uint32_t bank, + paddr_t *start, paddr_t *end); #endif
We will reuse nodes_cover_memory for Arm to check its bootmem info. So we introduce two arch helpers to get memory map's entry number and specified entry's range: arch_get_memory_bank_number arch_get_memory_bank_range Depends above two helpers, we make nodes_cover_memory become architecture independent. And the only change from an x86 perspective is the additional checks: !start || !end Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/arch/x86/numa.c | 18 ++++++++++++++++++ xen/arch/x86/srat.c | 11 ++++------- xen/include/asm-x86/numa.h | 3 +++ 3 files changed, 25 insertions(+), 7 deletions(-)