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

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

On 04/09/2014 02:15 PM, Ian Campbell wrote:
> On Wed, 2014-04-09 at 14:09 +0100, Julien Grall wrote:
>> 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).
> 
> It ensures that the modules don't start until at least the 2M boundary
> after the kernel's end. The userspace side does the same.

The userspace is consistent with the usage of kernend. It's not the case
here. There is no reason to mix the usage of kernend with and without
ROUNDUP.

In another point, I don't think 2MB-aligned is enough to ensure the
kernel won't decompress on the device tree.

-- 
Julien Grall