From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v1 1/2] arm: Add ability to relocate Xen in over 4GB space Date: Fri, 10 Apr 2015 15:25:40 +0100 Message-ID: <5527DD64.5040206@citrix.com> References: <1428496597-18981-1-git-send-email-oiurii.konovalenko@globallogic.com> <1428496597-18981-2-git-send-email-oiurii.konovalenko@globallogic.com> <552551B9.2040602@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Iurii Konovalenko , Julien Grall Cc: "xen-devel@lists.xen.org" , Julien Grall , tim@xen.org, Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 10/04/15 14:58, Iurii Konovalenko wrote: > Hi, Julien! Hi Iurii, > On Wed, Apr 8, 2015 at 7:05 PM, Julien Grall 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