Hi Jan,
> -----Original Message-----
> From: Jan Beulich <[email protected]>
> Sent: 2022年7月12日 21:40
> To: Wei Chen <[email protected]>
> Cc: nd <[email protected]>; Andrew Cooper <[email protected]>; Roger Pau
> Monné <[email protected]>; Wei Liu <[email protected]>; George Dunlap
> <[email protected]>; Julien Grall <[email protected]>; Stefano
> Stabellini <[email protected]>; [email protected]
> Subject: Re: [PATCH v2 5/9] xen/x86: use arch_get_memory_map to get
> information from E820 map
>
> On 08.07.2022 16:54, Wei Chen wrote:
> > @@ -82,3 +83,25 @@ 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 -ENODEV for out of scope index, or return
> > + * -EINVAL for non-RAM type memory entry.
> > + */
>
> I think the comment also wants to spell out that the range is
> exclusive at the end (assuming that's suitable for Arm; else now
> would perhaps be the time to change it).
>
Did you mean we have to mention the range is [star, end)?
If yes, I will add related comment. And...
> > +int __init arch_get_memory_map(unsigned int idx, paddr_t *start,
> paddr_t *end)
> > +{
> > + if ( idx >= e820.nr_map )
> > + return -ENODEV;
>
> Perhaps better -ENOENT?
>
Ack.
> > + if ( e820.map[idx].type != E820_RAM )
> > + return -EINVAL;
>
> I'm sorry, this should have occurred to me already when commenting on
> v1: "get_memory_map" doesn't really fit this "RAM only" restriction.
> Maybe arch_get_ram_range()? Or maybe others have some good naming
> suggestion?
arch_get_ram_range makes sense to me. I will update in next version.
>
> > + *start = e820.map[idx].addr;
> > + *end = e820.map[idx].addr + e820.map[idx].size;
>
...In this case, I think we don't need to adjust this line to:
*end = e820.map[idx].addr + e820.map[idx].size - 1;
As we have mentioned the range is [start, end).
> Nit: Would be shorter to read if you (re)used *start.
>
Ack.
> Jan
