Hi Julien,
> -----Original Message-----
> From: Julien Grall <[email protected]>
> 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
