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