RE: [PATCH v2 1/4] docs, xen/arm: Introduce reserved heap memory

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Subject: Re: [PATCH v2 1/4] docs, xen/arm: Introduce reserved heap memory
> 
> Hi Henry,
> 
> On 05/09/2022 08:26, Henry Wang wrote:
> > This commit introduces the reserved heap memory, which is parts of RAM
> > reserved in the beginning of the boot time for heap.
> >
> > Firstly, since a new type of memory bank is needed for marking the
> > memory bank solely as the heap, this commit defines `enum
> membank_type`
> 
> The wording is a bit confusing. I read this as the code will use "enum
> membank_type" but this is not possible as your enum is anonymous.
> 
> My suggestion would be to avoid creating a typedef (see below).

Yeah I think you are correct. The typedef is not really necessary.

> 
> > +- For Arm32, since there are seperated heaps, the reserved heap will be
> used
> 
> type: s/seperated/separated/

Oops, sorry again..

> 
> > +typedef enum {
> > +    MEMBANK_MEMORY,
> 
> Technically everything is memory :). I think here you are referring to
> either:
>     - Reserved memory for the device (or firmware)
>     - Any memory that will be used by the allocator.
> 
> I would consider to name the field MEMBANK_UNKNOWN or
> MEMBANK_DEFAULT
> with a comment explaining the meaning depends where it used (we have
> several arrays using struct membank).

MEMBANK_DEFAULT sounds good, and I will add the comment.

> 
> > +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to a
> Xen domain. */
> > +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as
> heap. */
> I would clarify the two values are only valid when the bank in in
> reserved_mem.

Good point, will do.

> 
> > +} membank_type;
> 
> I would prefer if if we don't define any typedef for this enum. But if
> you want to keep it, then please suffix with _t.

No I think you are correct, a enum membank_type instead of a typedef
would be enough here.

Kind regards,
Henry

> 
> >
> >   struct membank {
> >       paddr_t start;
> >       paddr_t size;
> > -    bool xen_domain; /* whether the memory bank is bound to a Xen
> domain. */
> > +    membank_type type;
> >   };
> >
> >   struct meminfo {
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 6e0398f3f6..8d3f859982 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -644,7 +644,7 @@ static void __init init_staticmem_pages(void)
> >
> >       for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> >       {
> > -        if ( bootinfo.reserved_mem.bank[bank].xen_domain )
> > +        if ( bootinfo.reserved_mem.bank[bank].type ==
> MEMBANK_XEN_DOMAIN )
> >           {
> >               mfn_t bank_start =
> _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
> >               unsigned long bank_pages =
> PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);
> 
> Cheers,
> 
> --
> Julien Grall