1. Patchew
  2. Xen
  3. [PATCH v4 0/5] Prerequisite patches for R82 upstreaming
  4. View patch

[PATCH v4 4/5] xen/arm: Check for Static Heap feature when freeing resources

Luca Fancellu posted 5 patches 3 weeks, 2 days ago
[PATCH v4 4/5] xen/arm: Check for Static Heap feature when freeing resources
Posted by Luca Fancellu 3 weeks, 2 days ago 
From: Penny Zheng <Penny.Zheng@arm.com>

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.

Extract static_heap flag from init data bootinfo, as it will be needed
after destroying the init data section, rename it to using_static_heap
and use it to tell whether the Xen static heap feature is enabled.

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 v3:
 - Removed helper using_static_heap(), renamed static_heap variable
   to using_static_heap and simplified #ifdef-ary (Jan suggestion)
Changes from v2:
 - Change xen_is_using_staticheap() to using_static_heap()
 - Move declaration of static_heap to xen/mm.h and import that in
   bootfdt.h
 - Reprased first part of the commit message
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         |  1 -
 xen/include/xen/mm.h              |  6 ++++++
 9 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 063611412be0..0824d61323b5 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 ( using_static_heap )
     {
         const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
 
@@ -246,7 +246,7 @@ void __init setup_mm(void)
 
     do
     {
-        e = bootinfo.static_heap ?
+        e = using_static_heap ?
             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..8270684246ea 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 ( !using_static_heap )
+        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..8c87649bc88e 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 ( !using_static_heap )
+    {
+        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 2e27af4560a5..22ab342dc8f4 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 ( !using_static_heap )
     {
-        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 fc93d86e8232..61ad24c3ddc8 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -410,7 +410,9 @@ static int __init process_chosen_node(const void *fdt, int node,
         if ( rc )
             return rc;
 
-        bootinfo.static_heap = true;
+#ifdef CONFIG_STATIC_MEMORY
+        using_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 0daf5e941a51..76d652c0de0b 100644
--- a/xen/common/device-tree/bootinfo.c
+++ b/xen/common/device-tree/bootinfo.c
@@ -407,7 +407,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 ( using_static_heap )
     {
         for ( i = 0 ; i < reserved_mem->nr_banks; i++ )
         {
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 92abed6514b4..013a1057cc7c 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -165,6 +165,11 @@
 #define PGT_TYPE_INFO_INITIALIZER 0
 #endif
 
+#ifdef CONFIG_STATIC_MEMORY
+/* Flag saved when Xen is using the static heap feature */
+bool __ro_after_init using_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 cea40ee11706..06c4a1023a9b 100644
--- a/xen/include/xen/bootfdt.h
+++ b/xen/include/xen/bootfdt.h
@@ -139,7 +139,6 @@ struct bootinfo {
 #ifdef CONFIG_STATIC_SHM
     struct shared_meminfo shmem;
 #endif
-    bool static_heap;
 };
 
 #ifdef CONFIG_ACPI
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index d7dcf0f06330..88536e8132f5 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -72,6 +72,12 @@
 
 struct page_info;
 
+#ifdef CONFIG_STATIC_MEMORY
+extern bool using_static_heap;
+#else
+#define using_static_heap false
+#endif
+
 void put_page(struct page_info *page);
 bool __must_check get_page(struct page_info *page,
                            const struct domain *domain);
-- 
2.34.1
Re: [PATCH v4 4/5] xen/arm: Check for Static Heap feature when freeing resources
Posted by Julien Grall 2 weeks, 6 days ago 
Hi Luca,

On 03/12/2024 09:48, Luca Fancellu wrote:
> diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> index 9664e85ee6c0..8c87649bc88e 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 ( !using_static_heap )
> +    {
> +        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 2e27af4560a5..22ab342dc8f4 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;

Looking at the change below, it seems that discard_initial_modules()
is a NOP for static_heap. Do you forsee any reason where it would be 
different?

If not, then I would prefer if the code would be:

if (static_heap)
   return;

This would reduce the indention in the most common case and also
make easier to review the diff.

The rest of this patch LGTM

>   
> -    for ( i = 0; i < mi->nr_mods; i++ )
> +    if ( !using_static_heap )
>       {
> -        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;
> +    }
>   }
>   

Cheers,

-- 
Julien Grall
Re: [PATCH v4 4/5] xen/arm: Check for Static Heap feature when freeing resources
Posted by Luca Fancellu 2 weeks, 5 days ago 
Hi Julien,

> On 6 Dec 2024, at 19:19, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 03/12/2024 09:48, Luca Fancellu wrote:
>> diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
>> index 9664e85ee6c0..8c87649bc88e 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 ( !using_static_heap )
>> +    {
>> +        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 2e27af4560a5..22ab342dc8f4 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;
> 
> Looking at the change below, it seems that discard_initial_modules()
> is a NOP for static_heap. Do you forsee any reason where it would be different?

you are right, the code could be a lot simpler.

I’ve prepared a diff on top of this patch if you are ok to do that on commit, but
It won’t be a problem for me to send another patch for that.

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 22ab342dc8f4..85f743a2c6ad 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -206,25 +206,27 @@ void __init discard_initial_modules(void)
     struct bootmodules *mi = &bootinfo.modules;
     int i;
 
-    if ( !using_static_heap )
-    {
-        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 ( using_static_heap )
+        return;
 
-            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);
     }
+
+    mi->nr_mods = 0;
+
+    remove_early_mappings();
 }
 
 /* Relocate the FDT in Xen heap */
Re: [PATCH v4 4/5] xen/arm: Check for Static Heap feature when freeing resources
Posted by Jan Beulich 3 weeks, 2 days ago 
On 03.12.2024 10:48, Luca Fancellu wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> 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.
> 
> Extract static_heap flag from init data bootinfo, as it will be needed
> after destroying the init data section, rename it to using_static_heap
> and use it to tell whether the Xen static heap feature is enabled.
> 
> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com> # common

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.