Re: [PATCH 13/16] xen/arm32: setup: Move out the code to populate the boot allocator

[Michal Orzel <michal.orzel@xxxxxxx>]

On 23.05.2022 21:51, Julien Grall wrote:
> 
> 
> On 23/05/2022 08:28, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>>
>> On 20.05.2022 14:09, Julien Grall wrote:
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> In a follow-up patch, we will want to populate the boot allocator
>>> separately for arm64. The code will end up to be very similar to the one
>>> on arm32. So move out the code in a new helper populate_boot_allocator().
>>>
>>> For now the code is still protected by CONFIG_ARM_32 to avoid any build
>>> failure on arm64.
>>>
>>> Take the opportunity to replace mfn_add(xen_mfn_start, xenheap_pages) with
>>> xenheap_mfn_end as they are equivalent.
>>>
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> ---
>>>
>>>      Changes in v4:
>>>          - Patch added
>>> ---
>>>   xen/arch/arm/setup.c | 90 +++++++++++++++++++++++++-------------------
>>>   1 file changed, 51 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index d5d0792ed48a..3d5a2283d4ef 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -637,10 +637,58 @@ static void __init init_staticmem_pages(void)
>>>   }
>>>     #ifdef CONFIG_ARM_32
>>> +/*
>>> + * Populate the boot allocator. All the RAM but the following regions
>>> + * will be added:
>>> + *  - Modules (e.g., Xen, Kernel)
>>> + *  - Reserved regions
>>> + *  - Xenheap
>>> + */
>>> +static void __init populate_boot_allocator(void)
>>> +{
>>> +    unsigned int i;
>> Shouldn't this be an int (as it was previously) because ...
>>> +    const struct meminfo *banks = &bootinfo.mem;
>>> +
>>> +    for ( i = 0; i < banks->nr_banks; i++ )
>> ... nr_banks is int ?
> 
> Hmmm... AFAIK banks->nr_banks never hold a negative value, so I am not sure 
> why it was introduced as an "int".
> 
> Looking through the code, we seem to have a mix of "unsigned int" and "int". 
> There seem to be less on the latter, so I have sent a patch to switch 
> nr_banks to "unsigned int" [1].
That's great, thanks.

> 
> This is based on this series thought and I would like to keep the "unsigned 
> int" here.
> 
>>
>> Apart from that:
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> Thanks! Please let me know if this reviewed-by hold.
Definitely yes.
> 
> Cheers,
> 
> [1] https://lore.kernel.org/xen-devel/20220523194631.66262-1-julien@xxxxxxx
> 

Cheers,
Michal