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

[Julien Grall <julien@xxxxxxx>]

Hi Henry and Michal,

On 06/09/2022 07:41, Henry Wang wrote:
> > -----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,
The naming was introduced in this version. So I would not view this as a 
late request.
> 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.
I am fine with the version proposed by Michal. I.e.:

MEMBANK_DEFAULT
MEMBANK_STATIC_DOMAIN
MEMBANK_STATIC_HEAP

Cheers,

--
Julien Grall