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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@xxxxxxx>
> Subject: Re: [PATCH v2 1/4] docs, xen/arm: Introduce reserved heap memory
> 
> Hi Julien,
> 
> On 05/09/2022 19:24, Julien Grall wrote:
> >
> > Hi Michal,
> >
> > On 05/09/2022 13:04, Michal Orzel wrote:
> >> On 05/09/2022 09:26, Henry Wang wrote:
> >>>
> >>> diff --git a/xen/arch/arm/include/asm/setup.h
> b/xen/arch/arm/include/asm/setup.h
> >>> index 5815ccf8c5..d0cc556833 100644
> >>> --- a/xen/arch/arm/include/asm/setup.h
> >>> +++ b/xen/arch/arm/include/asm/setup.h
> >>> @@ -22,11 +22,16 @@ typedef enum {
> >>>       BOOTMOD_UNKNOWN
> >>>   }  bootmodule_kind;
> >>>
> >>> +typedef enum {
> >>> +    MEMBANK_MEMORY,
> >>> +    MEMBANK_XEN_DOMAIN, /* whether the memory bank is bound to
> a Xen domain. */
> >>> +    MEMBANK_RSVD_HEAP, /* whether the memory bank is reserved as
> heap. */
> >>> +} membank_type;
> >> Whereas the patch itself looks ok (it must be modified anyway given the
> comments for patch #2),
> >> MEMBANK_XEN_DOMAIN name is quite ambiguous to me, now when it is
> part of membank_type enum.
> >> Something like MEMBANK_STATIC or MEMBANK_STATICMEM would be
> much cleaner in my opinion
> >> as it would directly indicate what type of memory we are talking about.
> >
> > I am not sure. Technically the reserved heap is static memory that has
> > been allocated for the heap. In fact, I think thn name "staticmem" is
> > now becoming quite confusing because we are referring to a very specific
> > use case (i.e. memory that has been reserved for domain use).
> >
> > So I would prefer if we keep "domain" in the name. Maybe
> > MEMBANK_STATIC_DOMAIN or MEMBANK_RESERVED_DOMAIN.
> >
> Personally I would drop completely using the "reserved heap" naming in
> favor
> of "static heap" because "staticmem" is also something we reserve at boot
> time for a domain use.
> This would also directly correlate to the device tree property "static-heap"
> and "static-mem".
> Then such enum would be created as follows and for me this is the cleanest
> solution:
> MEMBANK_DEFAULT
> MEMBANK_STATIC_DOMAIN
> MEMBANK_STATIC_HEAP
> 
> But I think we are already too late in this series to request such changes,

I am ok with a pure renaming to static heap if Julien is ok with that. I think
Julien has done most of the code review and we still have 2~3 days for it.

Kind regards,
Henry

> So with the current naming we can go for:
> MEMBANK_DEFAULT
> MEMBANK_RSVD_DOMAIN /* memory reserved for a domain use */
> MEMBANK_RSVD_HEAP   /* memory reserved for a heap use */
> 
> > Cheers,
> >
> > --
> > Julien Grall
> 
> ~Michal