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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2024 14:25, Luca Fancellu wrote:
>> 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?
> 
> I think so, the places of the check are mainly memory regions related to boot 
> modules,
> when we add a boot module we also do a check in order to see if it clashes 
> with any
> reserved regions already defined, which the static heap is part of.
> 
> Could you explain me why the question?

Well, if there was a chance of overlap, then parts of the free region would
need to go one way, and the rest the other way.

>>>>> --- 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.
> 
> Ok sorry, I misunderstood your comment, I thought you were suggesting to have 
> the
> declaration visible in that file since we are defining there the variable.
> 
> So Julien suggested that file, it was hosted before in 
> common/device-tree/device-tree.c,
> see the comment here:
> https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-6-luca.fancellu@xxxxxxx/#26125054
> 
> Since it seems you disagree with Julien, could you suggest a more suitable 
> place?

Anything defined in page-alloc.c should by default have its declaration in
xen/mm.h, imo. Exceptions would need justification.

Obviously a possible alternative is to move the definition, not the declaration.

Jan