From: Penny Zheng <Penny.Zheng@arm.com>

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.

Extract static_heap flag from init data bootinfo, as it will be needed
after destroying the init data section.
Introduce a new helper xen_is_using_staticheap() to tell whether Xenheap
is statically configured in the Device Tree.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v1:
 - moved static_heap to common/page_alloc.c
 - protect static_heap access with CONFIG_STATIC_MEMORY
 - update comment in arm/kernel.c kernel_decompress()
---
---
 xen/arch/arm/arm32/mmu/mm.c       |  4 ++--
 xen/arch/arm/kernel.c             |  7 ++++---
 xen/arch/arm/mmu/setup.c          |  8 ++++++--
 xen/arch/arm/setup.c              | 27 ++++++++++++++-------------
 xen/common/device-tree/bootfdt.c  |  4 +++-
 xen/common/device-tree/bootinfo.c |  2 +-
 xen/common/page_alloc.c           |  5 +++++
 xen/include/xen/bootfdt.h         | 14 +++++++++++++-
 8 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 063611412be0..b7ca7c94c9ca 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -199,7 +199,7 @@ void __init setup_mm(void)
 
     total_pages = ram_size >> PAGE_SHIFT;
 
-    if ( bootinfo.static_heap )
+    if ( xen_is_using_staticheap() )
     {
         const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
 
@@ -246,7 +246,7 @@ void __init setup_mm(void)
 
     do
     {
-        e = bootinfo.static_heap ?
+        e = xen_is_using_staticheap() ?
             fit_xenheap_in_static_heap(pfn_to_paddr(xenheap_pages), MB(32)) :
             consider_modules(ram_start, ram_end,
                              pfn_to_paddr(xenheap_pages),
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 293d7efaed9c..d2245ec9d2ef 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -244,10 +244,11 @@ static __init int kernel_decompress(struct bootmodule *mod, uint32_t offset)
     size += offset;
 
     /*
-     * Free the original kernel, update the pointers to the
-     * decompressed kernel
+     * In case Xen is not using the static heap feature, free the original
+     * kernel, update the pointers to the decompressed kernel
      */
-    fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+    if ( !xen_is_using_staticheap() )
+        fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
 
     return 0;
 }
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index 9664e85ee6c0..83c0a1480447 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -341,8 +341,12 @@ void free_init_memory(void)
     if ( rc )
         panic("Unable to remove the init section (rc = %d)\n", rc);
 
-    init_domheap_pages(pa, pa + len);
-    printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
+    if ( !xen_is_using_staticheap() )
+    {
+        init_domheap_pages(pa, pa + len);
+        printk("Freed %ldkB init memory.\n",
+               (long)(__init_end-__init_begin) >> 10);
+    }
 }
 
 /**
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 71ebaa77ca94..91340d5dc201 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -206,24 +206,25 @@ void __init discard_initial_modules(void)
     struct bootmodules *mi = &bootinfo.modules;
     int i;
 
-    for ( i = 0; i < mi->nr_mods; i++ )
+    if ( !xen_is_using_staticheap() )
     {
-        paddr_t s = mi->module[i].start;
-        paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
-
-        if ( mi->module[i].kind == BOOTMOD_XEN )
-            continue;
+        for ( i = 0; i < mi->nr_mods; i++ )
+        {
+            paddr_t s = mi->module[i].start;
+            paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
 
-        if ( !mfn_valid(maddr_to_mfn(s)) ||
-             !mfn_valid(maddr_to_mfn(e)) )
-            continue;
+            if ( mi->module[i].kind == BOOTMOD_XEN )
+                continue;
 
-        fw_unreserved_regions(s, e, init_domheap_pages, 0);
-    }
+            if ( !mfn_valid(maddr_to_mfn(s)) ||
+                 !mfn_valid(maddr_to_mfn(e)) )
+                continue;
 
-    mi->nr_mods = 0;
+            fw_unreserved_regions(s, e, init_domheap_pages, 0);
+        }
 
-    remove_early_mappings();
+        mi->nr_mods = 0;
+    }
 }
 
 /* Relocate the FDT in Xen heap */
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..6cc9ae146a97 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -403,7 +403,9 @@ static int __init process_chosen_node(const void *fdt, int node,
         if ( rc )
             return rc;
 
-        bootinfo.static_heap = true;
+#ifdef CONFIG_STATIC_MEMORY
+        static_heap = true;
+#endif
     }
 
     printk("Checking for initrd in /chosen\n");
diff --git a/xen/common/device-tree/bootinfo.c b/xen/common/device-tree/bootinfo.c
index f2e6a1145b7c..1e83d5172938 100644
--- a/xen/common/device-tree/bootinfo.c
+++ b/xen/common/device-tree/bootinfo.c
@@ -386,7 +386,7 @@ void __init populate_boot_allocator(void)
     const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
     paddr_t s, e;
 
-    if ( bootinfo.static_heap )
+    if ( xen_is_using_staticheap() )
     {
         for ( i = 0 ; i < reserved_mem->nr_banks; i++ )
         {
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 33c8c917d984..b1fdb4efcff0 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -164,6 +164,11 @@
 #define PGT_TYPE_INFO_INITIALIZER 0
 #endif
 
+#ifdef CONFIG_STATIC_MEMORY
+/* Flag saved when Xen is using the static heap feature (xen,static-heap) */
+bool __ro_after_init static_heap;
+#endif
+
 unsigned long __read_mostly max_page;
 unsigned long __read_mostly total_pages;
 paddr_t __ro_after_init mem_hotplug;
diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
index 16fa05f38f38..c861590e38c8 100644
--- 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
+
 bool check_reserved_regions_overlap(paddr_t region_start,
                                     paddr_t region_size);
 
@@ -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
+}
+
 #endif /* XEN_BOOTFDT_H */
-- 
2.34.1

On 19.11.2024 09:58, Luca Fancellu wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> 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.

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

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

Jan

Hi Jan,

thanks for your review

> On 25 Nov 2024, at 16:32, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.11.2024 09:58, Luca Fancellu wrote:
>> From: Penny Zheng <Penny.Zheng@arm.com>
>> 
>> 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

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@suse.com> wrote:
>>
>> On 19.11.2024 09:58, Luca Fancellu wrote:
>>> From: Penny Zheng <Penny.Zheng@arm.com>
>>>
>>> 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

Hi Jan,

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

> 
>>>> --- 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@arm.com/#26125054

Since it seems you disagree with Julien, could you suggest a more suitable place?

Cheers,
Luca

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@arm.com/#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

> On 26 Nov 2024, at 13:29, Jan Beulich <jbeulich@suse.com> 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@arm.com/#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

Hi Luca,

Sorry for the late answer.

On 26/11/2024 13:52, Luca Fancellu wrote:
> 
> 
>> On 26 Nov 2024, at 13:29, Jan Beulich <jbeulich@suse.com> 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@arm.com/#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?

I think using_static_heap() should be defined in xen/mm.h as well 
because we expect everyone to use using_static_heap() rather than 
static_heap.

This is to avoid adding #ifdef for every user.

Cheers,

-- 
Julien Grall