Re: [PATCH 02/10] xen/arm: handle static memory in dt_unreserved_regions

From: Julien Grall <julien@xen.org>
To: Penny Zheng <penny.zheng@arm.com>,
	xen-devel@lists.xenproject.org, sstabellini@kernel.org
Cc: Bertrand.Marquis@arm.com, Wei.Chen@arm.com, nd@arm.com
Subject: Re: [PATCH 02/10] xen/arm: handle static memory in dt_unreserved_regions
Date: Tue, 18 May 2021 10:04:05 +0100	[thread overview]
Message-ID: <eb262284-fd02-f5a7-be45-69f11bbca7d6@xen.org> (raw)
In-Reply-To: <20210518052113.725808-3-penny.zheng@arm.com>

Hi Penny,

On 18/05/2021 06:21, Penny Zheng wrote:
> static memory regions overlap with memory nodes. The
> overlapping memory is reserved-memory and should be
> handled accordingly:
> dt_unreserved_regions should skip these regions the
> same way they are already skipping mem-reserved regions.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/setup.c | 39 +++++++++++++++++++++++++++++++++------
>   1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 00aad1c194..444dbbd676 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -201,7 +201,7 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>                                            void (*cb)(paddr_t, paddr_t),
>                                            int first)
>   {
> -    int i, nr = fdt_num_mem_rsv(device_tree_flattened);
> +    int i, nr_reserved, nr_static, nr = fdt_num_mem_rsv(device_tree_flattened);
>   
>       for ( i = first; i < nr ; i++ )
>       {
> @@ -222,18 +222,45 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>       }
>   
>       /*
> -     * i is the current bootmodule we are evaluating across all possible
> -     * kinds.
> +     * i is the current reserved RAM banks we are evaluating across all
> +     * possible kinds.
>        *
>        * When retrieving the corresponding reserved-memory addresses
>        * below, we need to index the bootinfo.reserved_mem bank starting
>        * from 0, and only counting the reserved-memory modules. Hence,
>        * we need to use i - nr.
>        */
> -    for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
> +    i = i - nr;
> +    nr_reserved = bootinfo.reserved_mem.nr_banks;
> +    for ( ; i < nr_reserved; i++ )
>       {
> -        paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
> -        paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
> +        paddr_t r_s = bootinfo.reserved_mem.bank[i].start;
> +        paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i].size;
> +
> +        if ( s < r_e && r_s < e )
> +        {
> +            dt_unreserved_regions(r_e, e, cb, i + 1);
> +            dt_unreserved_regions(s, r_s, cb, i + 1);
> +            return;
> +        }
> +    }
> +
> +    /*
> +     * i is the current reserved RAM banks we are evaluating across all
> +     * possible kinds.
> +     *
> +     * When retrieving the corresponding static-memory bank address
> +     * below, we need to index the bootinfo.static_mem starting
> +     * from 0, and only counting the static-memory bank. Hence,
> +     * we need to use i - nr_reserved.
> +     */
> +
> +    i = i - nr_reserved;
> +    nr_static = bootinfo.static_mem.nr_banks;
> +    for ( ; i < nr_static; i++ )
> +    {
> +        paddr_t r_s = bootinfo.static_mem.bank[i].start; > +        paddr_t r_e = r_s + bootinfo.static_mem.bank[i].size;

This is the 3rd loop we are adding in dt_unreserved_regions(). Each loop 
are doing pretty much the same thing except with a different array. I'd 
like to avoid the new loop if possible.

As mentionned in patch#1, the static memory is another kind of reserved 
memory. So could we describe the static memory using the reserved-memory?

>   
>           if ( s < r_e && r_s < e )
>           {
> 

Cheers,

-- 
Julien Grall