Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Jan,

thanks for your review

> On 25 Nov 2024, at 16:32, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 19.11.2024 09:58, Luca Fancellu wrote:
>> From: Penny Zheng <Penny.Zheng@xxxxxxx>
>> 
>> If Xenheap is statically configured in Device Tree, its size is
>> definite. So, the memory shall not be given back into static heap, like
>> it's normally done in free_init_memory, etc, once the initialization
>> is finished.
> 
> I'm somewhat confused by the use of "back" here? Isn't the change all
> about init-time behavior, i.e. memory that's handed to the allocator for
> the very first time? Else I may be misunderstanding something here, in
> which case I'd like to ask for the description to be refined.

Yes, I’ve tried to rephrase it, do you think this is more clear?

If the Xen heap is statically configured in Device Tree, its size is
definite, so only the defined memory shall be given to the boot
allocator. Have a check where init_domheap_pages() is called
which takes into account if static heap feature is used.

> 
>> --- a/xen/include/xen/bootfdt.h
>> +++ b/xen/include/xen/bootfdt.h
>> @@ -132,7 +132,6 @@ struct bootinfo {
>> #ifdef CONFIG_STATIC_SHM
>>     struct shared_meminfo shmem;
>> #endif
>> -    bool static_heap;
>> };
>> 
>> #ifdef CONFIG_ACPI
>> @@ -157,6 +156,10 @@ struct bootinfo {
>> 
>> extern struct bootinfo bootinfo;
>> 
>> +#ifdef CONFIG_STATIC_MEMORY
>> +extern bool static_heap;
>> +#endif
> 
> Just to double check Misra-wise: Is there a guarantee that this header will
> always be included by page-alloc.c, so that the definition of the symbol
> has an earlier declaration already visible? I ask because this header
> doesn't look like one where symbols defined in page-alloc.c would normally
> be declared. And I sincerely hope that this header isn't one of those that
> end up being included virtually everywhere.

I’ve read again MISRA rule 8.4 and you are right, I should have included 
bootfdt.h in
page-alloc.c in order to have the declaration visible before defining 
static_heap.

I will include the header in page-alloc.c

> 
>> @@ -206,4 +209,13 @@ static inline struct shmem_membank_extra 
>> *bootinfo_get_shmem_extra(void)
>> }
>> #endif
>> 
>> +static inline bool xen_is_using_staticheap(void)
>> +{
>> +#ifdef CONFIG_STATIC_MEMORY
>> +    return static_heap;
>> +#else
>> +    return false;
>> +#endif
>> +}
> 
> As to naming: How about using_static_heap()? The xen_ prefix doesn't look to
> be carrying any useful information, and the then remaining is_ prefix would
> be largely redundant with "using". Plus there surely wants to be a separator
> between "static" and "heap".

yes, sounds a better name, I’ll use it

Cheers,
Luca