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

[Michal Orzel <michal.orzel@xxxxxxx>]

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, 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