Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 2022年8月25日 20:14
> To: Wei Chen <wei.c...@arm.com>
> Cc: nd <n...@arm.com>; Andrew Cooper <andrew.coop...@citrix.com>; Roger Pau
> Monné <roger....@citrix.com>; Wei Liu <w...@xen.org>; George Dunlap
> <george.dun...@citrix.com>; Julien Grall <jul...@xen.org>; Stefano
> Stabellini <sstabell...@kernel.org>; xen-devel@lists.xenproject.org
> 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;
