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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2024 11:56, Luca Fancellu wrote:
> 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.

This reads better, thanks. Follow-on question: Is what is statically
configured for the heap guaranteed to never overlap with anything passed
to init_domheap_pages() in those places that you touch?

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

Except that, as said, I don't think that header should be included in this file.
Instead I think the declaration wants to move elsewhere.

Jan