Re: [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Julien Grall <julien.grall@linaro.org>]

On 04/09/2014 01:57 PM, Ian Campbell wrote:
> On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 04/09/2014 12:51 PM, Ian Campbell wrote:
>>> This now uses the same decision tree as libxc (which is much easier to test).
>>>
>>> The main change is to explictly handle the placement at 128MB or end of RAM as
>>
>> s/explictly/explicitly/
>> s/mopules/modules/
> 
> Gah, fingers not working right today it seems.
> 
>>> since it is redundant with the case where placing them at the end of RAM ends
>>> up abutting the kernel.
>>>
>>> Also round the kernel size up to a 2MB boundary.
>>
>> A bit surprised that it was not done before :).
> 
> Me too.
> 
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>> I'm sure to regret playing with this yet again...
>>> ---
>>>  xen/arch/arm/kernel.c |   22 ++++++++++++----------
>>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>> index bc625a4..1102392 100644
>>> --- a/xen/arch/arm/kernel.c
>>> +++ b/xen/arch/arm/kernel.c
>>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info,
>>>      const paddr_t rambase = info->mem.bank[0].start;
>>>      const paddr_t ramsize = info->mem.bank[0].size;
>>>      const paddr_t ramend = rambase + ramsize;
>>> -    const paddr_t kernsize = kernend - kernbase;
>>> +    const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
>>
>> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you
>> roundup directly kernend?
> 
> It's passed as a paramter, and it's not possible to round it before
> using it here (code before declarations), so I'd have to make kernsize
> non-const and initialise it after the rounding. I didn't think it was
> worth it.

In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)).

As I understand, we don't need to round up on the second if expression
(see code below).

+    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
+        modbase = ram128mb;
+    else if ( ramend - modsize > ROUNDUP(kernend, MB(2)) )
+        modbase = ramend - modsize;
+    else if ( kernbase - rambase > modsize )

Regards,

-- 
Julien Grall