[PATCH 4/7] ARM: KVM: enforce page alignment for identity mapped code

[Marc Zyngier]

On 04/04/13 00:15, Christoffer Dall wrote:
> On Wed, Apr 03, 2013 at 11:00:39AM +0100, Marc Zyngier wrote:
>> On 03/04/13 10:50, Will Deacon wrote:
>>> On Tue, Apr 02, 2013 at 02:25:12PM +0100, Marc Zyngier wrote:
>>>> We're about to move to a init procedure where we rely on the
>>>> fact that the init code fits in a single page. Make sure we
>>>> align the idmap text on a page boundary, and that the code is
>>>> not bigger than a single page.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
>>>> ---
>>>>  arch/arm/kernel/vmlinux.lds.S | 2 +-
>>>>  arch/arm/kvm/init.S           | 7 +++++++
>>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
>>>> index b571484..d9dd265 100644
>>>> --- a/arch/arm/kernel/vmlinux.lds.S
>>>> +++ b/arch/arm/kernel/vmlinux.lds.S
>>>> @@ -20,7 +20,7 @@
>>>>  	VMLINUX_SYMBOL(__idmap_text_start) = .;				\
>>>>  	*(.idmap.text)							\
>>>>  	VMLINUX_SYMBOL(__idmap_text_end) = .;				\
>>>> -	ALIGN_FUNCTION();						\
>>>> +	. = ALIGN(PAGE_SIZE);						\
>>>>  	VMLINUX_SYMBOL(__hyp_idmap_text_start) = .;			\
>>>>  	*(.hyp.idmap.text)						\
>>>>  	VMLINUX_SYMBOL(__hyp_idmap_text_end) = .;
>>>> diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
>>>> index 9f37a79..35a463f 100644
>>>> --- a/arch/arm/kvm/init.S
>>>> +++ b/arch/arm/kvm/init.S
>>>> @@ -111,4 +111,11 @@ __do_hyp_init:
>>>>  	.globl __kvm_hyp_init_end
>>>>  __kvm_hyp_init_end:
>>>>  
>>>> +	/*
>>>> +	 * The above code *must* fit in a single page for the trampoline
>>>> +	 * madness to work. Whoever decides to change it must make sure
>>>> +	 * we map the right amount of memory for the trampoline to work.
>>>> +	 * The line below ensures any breakage will get noticed.
> 
> This comment is a little funny. What's this about changing it? And why
> madness? OK, it's a little mad.
> 
> I think the nice thing to explain here is why .org would complain - I
> had to look in the AS reference to figure out that it will complain if
> the location pointer is forwarded outside the section - if that is the
> right reason?
> 
>>>> +	 */
>>>> +	.org	__kvm_hyp_init + PAGE_SIZE
>>>>  	.popsection
>>>
>>> What effect does this have on the size of the kernel image? I'd expect the
>>> idmap code to be pretty small, so aligning to a page might be overkill a lot
>>> of the time.
>>
>> So we're indeed wasting memory between the kernel and the HYP idmaps.
>> The reason for this is to ensure that the init code is not spread across
>> two pages, which would make the trampoline harder to allocate.
>>
>> If there's a way to ensure that the init.S code will fit a single page,
>> then we can remove this ALIGN(PAGE_SIZE).
> 
> Just allocate a temporary page and copy the bit of code into there?  It
> should be easy enough to make sure it's location independent.  We can
> even give that page back later if we want and re-do the trick when a CPU
> gets hot-plugged if we want. No?

I'd really like to avoid freeing stuff, because it gives us even more
chances to screw our TLBs (we'd have to invalidate on unmap, and deal
with potential races between CPUs coming up).

We could allocate this page *if* the HYP init code happens to cross a
page boundary, and only then. I'll have a look at implementing this as
it should solve Will's issue (which is even worse on arm64 with 64k pages).

	M.
-- 
Jazz is not dead. It just smells funny...