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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.08.2022 04:58, Wei Chen wrote:
> @@ -96,3 +97,27 @@ unsigned int __init arch_get_dma_bitsize(void)
>                   flsl(node_start_pfn(node) + node_spanned_pages(node) / 4 - 
> 1)
>                   + PAGE_SHIFT, 32);
>  }
> +
> +/*
> + * 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
> + * -EINVAL for non-RAM type memory entry.
> + *
> + * Note: the range is exclusive at the end, e.g. [start, end).
> + */
> +int __init arch_get_ram_range(unsigned int idx, paddr_t *start, paddr_t *end)

Since the comment is intended to apply to all architectures providing,
I think it should go with the declaration (once) rather than the
definition (at least one instance per arch).

> +{
> +    if ( idx >= e820.nr_map )
> +        return -ENOENT;
> +
> +    if ( e820.map[idx].type != E820_RAM )
> +        return -EINVAL;

EINVAL is so heavily (over)loaded that I'm inclined to ask to use e.g.
-ENODATA here.

> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -428,18 +428,22 @@ 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++) {
> +     for (i = 0; ; i++) {
>               int j, found;
>               paddr_t start, end;
>  
> -             if (e820.map[i].type != E820_RAM) {
> +             /* Try to loop memory map from index 0 to end to get RAM 
> ranges. */
> +             found = arch_get_ram_range(i, &start, &end);
> +
> +             /* Index relate entry is not RAM, skip it. */
> +             if (found == -EINVAL)
>                       continue;
> -             }
>  
> -             start = e820.map[i].addr;
> -             end = e820.map[i].addr + e820.map[i].size;
> +             /* Reach the end of arch's memory map */
> +             if (found == -ENOENT)
> +                     break;

What if an arch returns a 3rd error indicator? The way you've written
it code below would assume success and use uninitialized data. I'd
like to suggest to only special-case -ENOENT and treat all other
errors the same. But of course the variable (re)used doesn't really
fit that:

                /* Reach the end of arch's memory map */
                if (found == -ENOENT)
                        break;

                /* Index relate entry is not RAM, skip it. */
                if (found)
                        continue;

because here really you mean "not found". Since in fact "found" would
want to be of "bool" type in the function, and "j" would want to be
"unsigned int" just like "i" is, I recommend introducing a new local
variable, e.g. "err".

Jan

>               do {
>                       found = 0;