RE: [PATCH v3 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年8月25日 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 v3 4/6] xen/x86: use arch_get_ram_range to get
> information from E820 map
> 
> 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).
> 

Ok, I will move the comment to header file.

> > +{
> > +    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.
> 

Ok.

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

Yes, I hadn't taken the 3rd error case into account. I will follow your
Comment in next version.

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

Ok.

Cheers,
Wei Chen

> Jan
> 
> >             do {
> >                     found = 0;