Re: [PATCH v1 1/2] arm: Add ability to relocate Xen in over 4GB space

[Julien Grall <julien.grall@citrix.com>]

On 10/04/15 14:58, Iurii Konovalenko wrote:
> Hi, Julien!

Hi Iurii,

> On Wed, Apr 8, 2015 at 7:05 PM, Julien Grall <julien.grall@citrix.com> wrote:
> 
>> The virtualization extension requires LPAE. Any reason to make this
>> optional?
> I agree with you - there is no real reason to make it optional. I will
> rework patch
> to include functionality without any flags by default.

Before doing a such change, I'd like the point of view of Stefano or Ian
on this patch.

>>> +#ifdef ARM32_RELOCATE_OVER_4GB
>>> +        ldr   r1, =_start
>>> +        sub r4, r1
>>> +        add r4, #BOOT_RELOC_VIRT_START
>>> +#endif
>>
>> This chunk need some comment in order to explain what it's doing.
>>
>> AFAIU the resulting value in r4, will be exactly the same as  "ldr r4,
>> =init_ttbr".
> Not exactly. Virtual address of _start is 0x00200000, but
> #BOOT_RELOC_VIRT_START is 0x00800000.
> This part of code is needed to get value, that was "stashed by CPU 0" in
> relocated copy of Xen when secondary CPUs run in non-relocated.

And the reloc Xen is mapped by setup_page_tables in mm.c, right?

If so, this definitely need a comment explaining why and how it works.
Because you are relying on multiple things in the code.
	- 1:1 mapping is not overlapped (see my comment on the previous mail)
	- reloc Xen will never overlap the boot Xen (for instance we could
decide that Xen doesn't need to be relocated).
	- After relocation, Xen is still modifying the boot page table (clean
them) but hopefully it's only the relocated version

This is making the code in setup_page_tables quite difficult to
understand now.

>>> +    xen_relocation_offset = __pa(init_secondary) - xen_relocation_offset;
>>
>> The Xen may be relocated below the boot copy. So the final result may be
>> negative.
> This offset is used to get physical address of non-relocated variables
> and write there values.
> If it is negative, then calculating non-relocated address gives bigger
> number (substraction of  negative value).
> In addition, it's hard to imagine when it can be negative if we take
> highest region of highest memory bank.

Simple, the board has all the memory banks below 4G and Xen has been
loaded by the bootloader at a very high address.

Regards,

-- 
Julien Grall