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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:

> --- 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