Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 2022年9月8日 20:14
> To: Wei Chen <wei.c...@arm.com>
> Cc: nd <n...@arm.com>; Andrew Cooper <andrew.coop...@citrix.com>; Roger Pau
> Monné <roger....@citrix.com>; Wei Liu <w...@xen.org>; George Dunlap
> <george.dun...@citrix.com>; Julien Grall <jul...@xen.org>; Stefano
> Stabellini <sstabell...@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 4/6] xen/x86: use arch_get_ram_range to get
> information from E820 map
>
> On 02.09.2022 05:31, Wei Chen wrote:
> > The sanity check of nodes_cover_memory is also a requirement of
> > other architectures that support NUMA. But now, the code of
> > nodes_cover_memory is tied to the x86 E820. In this case, we
> > introduce arch_get_ram_range to decouple architecture specific
> > memory map from this function. This means, other architectures
> > like Arm can also use it to check its node and memory coverage
> > from bootmem info.
> >
> > Depends arch_get_ram_range, we make nodes_cover_memory become
> > architecture independent. We also use neutral words to replace
> > SRAT and E820 in the print message of this function. This will
> > to make the massage seems more common.
> >
> > As arch_get_ram_range use unsigned int for index, we also adjust
> > the index in nodes_cover_memory from int to unsigned int.
> >
> > Signed-off-by: Wei Chen <wei.c...@arm.com>
>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> albeit still with a couple of suggestions:
>
Thanks, I will adjust the code comments to address your suggestions.
> > --- a/xen/arch/x86/srat.c
> > +++ b/xen/arch/x86/srat.c
> > @@ -428,37 +428,43 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> > Make sure the PXMs cover all memory. */
> > static int __init nodes_cover_memory(void)
> > {
> > - int i;
> > + unsigned int i;
> >
> > - for (i = 0; i < e820.nr_map; i++) {
> > - int j, found;
> > + for (i = 0; ; i++) {
> > + int err;
> > + unsigned int j;
> > + bool found;
> > paddr_t start, end;
> >
> > - if (e820.map[i].type != E820_RAM) {
> > - continue;
> > - }
> > + /* Try to loop memory map from index 0 to end to get RAM
> ranges. */
> > + err = arch_get_ram_range(i, &start, &end);
> >
> > - start = e820.map[i].addr;
> > - end = e820.map[i].addr + e820.map[i].size;
> > + /* Reach the end of arch's memory map */
> > + if (err == -ENOENT)
> > + break;
>
> Such a comment ahead of an if() is often better put as a question, e.g.
> "Reached the end of the memory map?" here or, if you dislike using a
> question, "Exit the loop at the end of the memory map".
>
> > + /* Index relate entry is not RAM, skip it. */
> > + if (err)
> > + continue;
>
> And then perhaps "Skip non-RAM entries" here.
>
> > --- a/xen/include/xen/numa.h
> > +++ b/xen/include/xen/numa.h
> > @@ -81,6 +81,19 @@ static inline nodeid_t __attribute_pure__
> phys_to_nid(paddr_t addr)
> > #define node_end_pfn(nid) (NODE_DATA(nid)->node_start_pfn + \
> > NODE_DATA(nid)->node_spanned_pages)
> >
> > +/*
> > + * This function provides the ability for caller to get one RAM entry
> > + * from architectural memory map by index.
> > + *
> > + * This function will return zero if it can return a proper RAM entry.
> > + * otherwise it will return -ENOENT for out of scope index, or return
> > + * -ENODATA for non-RAM type memory entry.
>
> The way you've implemented things, -ENODATA isn't special anymore, so
> better wouldn't be called out as special here. May I suggest to at
> least insert "e.g." in front of it? (An alternative would be to check
> for -ENODATA in nodes_cover_memory() again, followed by ASSERT(!err).)
>
> > + * Note: the range is exclusive at the end, e.g. [start, end).
>
> Perhaps better [*start, *end) to match ...
>
> > + */
> > +extern int arch_get_ram_range(unsigned int idx,
> > + paddr_t *start, paddr_t *end);
>
> ... this?
>
> Jan
