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

[Julien Grall <julien@xxxxxxx>]

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