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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Julien,

> On 6 Dec 2024, at 19:19, Julien Grall <julien@xxxxxxx> 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 */