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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 26 Nov 2024, at 13:29, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

oh ok, sure of course, thanks for answering.

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

I would be fine to have the declaration in xen/mm.h, I just need to import 
xen/mm.h in bootfdt.h so that it is visible to
“using_static_heap”, @Julien would you be ok with that?

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

Cheers,
Luca