RE: [PATCH v3 2/4] docs, xen/arm: Introduce static heap memory

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> > -    /* find first memory range not bound to a Xen domain */
> > -    for ( i = 0; i < mem->nr_banks && mem->bank[i].xen_domain; i++ )
> > +    /* find first memory range not bound to a Xen domain nor heap */
> 
> This comment could become stale if we are adding a new type. So how about:
> 
> /* find the first memory range that is reserved for device (or firmware) */

Ok, will use this one.

> 
> > +enum membank_type {
> > +    /*
> > +     * The MEMBANK_DEFAULT type refers to either reserved memory for
> the
> > +     * device (or firmware) or any memory that will be used by the 
> > allocator.
> 
> I realize the part of the 'or' is what I suggested. However, I wasn't
> correct here (sorry).

No problem, actually, I've learned a lot about how Xen does the memory
management from your comments. So I should say thank you.

> 
> In the context of 'mem' this is referring to any RAM. The setup code
> will then find the list of the regions that doesn't overlap with the
> 'reserved_mem' and then give the pages to the boot allocator (and
> subsequently the buddy allocator). Also...
> 
> > +     * The meaning depends on where the memory bank is actually used.
> 
> ... this doesn't tell the reader which means applies where. So I would
> suggest the following:
> 
> The MEMBANK_DEFAULT type refers to either reserved memory for the
> device/firmware (when the bank is in 'reserved_mem') or any RAM (when
> the bank is in 'mem'

Ok will follow that.

> 
> The rest of the code looks good to me. So once we settled on the two
> comments above:
> 
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall