On 01/04/15 06:06, AKASHI Takahiro wrote:
Marc
On 03/31/2015 04:31 PM, Marc Zyngier wrote:
On Tue, 31 Mar 2015 07:04:44 +0100 AKASHI Takahiro takahiro.akashi@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@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:
- As you mentioned above, trampoline code is spread across page boundary even though the whole size is less than a page.
- 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?
Well, there is a difference between wasting up to 4kB, and wasting up to 64kB. We align it on the smallest page size so we can be sure the code always fits in a single page, but I don't really want to bloat the kernel for no reason.
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__ */
This seems like the sensible thing to do.
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.
OK.
M.