RE: [PATCH v4 4/6] xen/x86: use arch_get_ram_range to get information from E820 map

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年9月8日 20:14
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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.chen@xxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 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