[RFC v2 0/5] arm64: kvm: reset hyp context for kexec

[AKASHI Takahiro]

Marc

On 03/31/2015 04:31 PM, Marc Zyngier wrote:
> On Tue, 31 Mar 2015 07:04:44 +0100
> AKASHI Takahiro <takahiro.akashi at linaro.org> wrote:
>
> Hi Takahiro,
>
>> Marc,
>>
>> On 03/30/2015 05:54 PM, AKASHI Takahiro wrote:
>>> On 03/30/2015 04:16 PM, Marc Zyngier wrote:
>>>> On Mon, 30 Mar 2015 02:39:53 +0100
>>>> AKASHI Takahiro <takahiro.akashi at linaro.org> wrote:
>>>>
>>>>> On 03/28/2015 02:40 AM, Kyle McMartin wrote:
>>>>>> On Fri, Mar 27, 2015 at 03:37:04PM +0000, Marc Zyngier wrote:
>>>>>>>> [  236.260863] Kernel panic - not syncing: HYP panic:
>>>>>>>> [  236.260863] PS:600003c9 PC:000003ffffff0830 ESR:0000000096000006
>>>>>>>
>>>>>>> It would be interesting if you could find out what you have at offset
>>>>>>> 0x830 of hyp-init.o (the stack trace is for EL1, and is not going to
>>>>>>> help much).
>>>>>>>
>>>>>>
>>>>>> Given the alignment, i'm going to assume i'm looking at the right thing:
>>>>>>
>>>>>> 0000000000000820 <__kvm_hyp_reset>:
>>>>>>         820:       d51c2000        msr     ttbr0_el2, x0
>>>>>>         824:       d5033fdf        isb
>>>>>>         828:       d50c871f        tlbi    alle2
>>>>>>         82c:       d5033f9f        dsb     sy
>>>>>>         830:       10000060        adr     x0, 83c <__kvm_hyp_reset+0x1c>
>>>>>>         834:       b3403c01        bfxil   x1, x0, #0, #16
>>>>>>         838:       d61f0020        br      x1
>>>>>>         83c:       d53c1000        mrs     x0, sctlr_el2
>>>>>>
>>>>>> but it seems fairly implausible to be trapping on ADR x0, 1f...
>>>>>
>>>>>
>>>>> I've never seen this panic on fast model...
>>>>>
>>>>> ESR shows that
>>>>>      - Exception class: Data abort taken without a change in Exception level
>>>>>      - Data fault status code: Translation fault at EL2
>>>>>
>>>>> and FAR seems not to be a proper address.
>>>>
>>>> ... which is consistent with what we're seeing here (data fault on
>>>> something that doesn't generate a load/store). I'm pretty sure the
>>>> page tables are screwed.
>>>>
>>>> Have you tested it with 64k pages?
>>>
>>> Hmm... It seems that I was able to reproduce the problem if 64k pages enabled.
>>
>> The entry address in trampoline code calc'ed by kvm_virt_to_trampoline(__kvm_hyp_reset)
>> seems to be wrong due to improper page-alignment in hyp-init.S.
>> The following patch fixed this problem, at least, in my environment(fast model).
>> (I don't know why it's PAGE_SHIFT - 1, not PAGE_SHIFT.)
>>
>>   >diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S
>>   >index d212990..45b8d98 100644
>>   >--- a/arch/arm64/kvm/hyp-init.S
>>   >+++ b/arch/arm64/kvm/hyp-init.S
>>   >@@ -24,7 +24,7 @@
>>   >        .text
>>   >        .pushsection    .hyp.idmap.text, "ax"
>>   >
>>   >-       .align  11
>>   >+       .align  (PAGE_SHIFT - 1)
>
> I'm afraid this is wrong. This alignment is for the vectors (which have
> to be aligned on a 2kB boundary, hence the ".align 11"), not for the
> code. Aligning it on a 32kB boundary doesn't make any sense, and just
> hides the bug.
>
> I bet that without this hack, the hyp-init code is spread across two
> 64kB pages, and the kernel generates a bounce page for this code. By
> changing the alignment, you just end up having the code to fit in a
> single page, and no bounce page gets generated.

There seem to be two scenarios that make things go wrong:
1) As you mentioned above, trampoline code is spread across page boundary
    even though the whole size is less than a page.
2) The whole trampoline code fits into a single page, but the physical start address
    of this region (that is, __hyp_idmap_text_start) is not page-aligned.
    In this case, pa of __kvm_hyp_reset should also be offset.

Given any combinations of #1 and #2, __kvm_virt_to_trampoline() would get a bit complicated.

> If I'm right above the above, it means that you're computing something
> against the wrong base. Can you please verify this scenario?
>
> Now, the good news is that Ard is removing the bounce page from the KVM
> code (for unrelated reasons), and this may just be the saving grace.

Ard's patch will fix #1, but not #2. So I modified __kvm_virt_to_trampoline
as followed and it seems to work well both on 4k-page kernel and 64k-page kernel
(in addition to Ard's patch).

But please note that Ard's patch already makes __hyp_idmap_text_start 4kb-aligned.
So why not PAGE_SIZE-aligned as my previous patch does?

 >diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
 >index c191432..facfd6d 100644
 >--- a/arch/arm64/include/asm/kvm_mmu.h
 >+++ b/arch/arm64/include/asm/kvm_mmu.h
 >@@ -308,7 +308,9 @@ void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled);
 >
 > extern char __hyp_idmap_text_start[];
 > #define kvm_virt_to_trampoline(x)      \
 >-                       (TRAMPOLINE_VA + ((x) - __hyp_idmap_text_start))
 >+               (TRAMPOLINE_VA          \
 >+               + ((unsigned long)(x)   \
 >+                 - ((unsigned long)__hyp_idmap_text_start & PAGE_MASK)))
 >
 > #endif /* __ASSEMBLY__ */
 > #endif /* __ARM64_KVM_MMU_H__ */

>>   >
>>   > ENTRY(__kvm_hyp_init)
>>   >        ventry  __invalid               // Synchronous EL2t
>>
>>
>> After applying this patch, I got another problem with kexec-tools on 64k page kernel,
>> but I've already modified kexec-tools.
>
> The idea that userspace behavior is dependent on the kernel page size
> is deeply worrying...

The logic is not directly related to a page size.
Kexec-tools try to allocate several small chunks of memory in a fixed-size region of last part
of main memory. Due to increased page size, the total size of chunks were overflowed.

Thanks,
-Takahiro AKASHI

> Thanks,
>
> 	M.
>