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 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? 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. -- Jazz is not dead. It just smells funny...
