From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH v3 08/32] arm64: KVM: architecture specific MMU backend Date: Tue, 30 Apr 2013 11:23:45 +0100 Message-ID: <517F9BB1.4090906@arm.com> References: <1365437854-30214-1-git-send-email-marc.zyngier@arm.com> <1365437854-30214-9-git-send-email-marc.zyngier@arm.com> <20130429173521.GB16544@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Cc: "linux-arm-kernel@lists.infradead.org" , "kvmarm@lists.cs.columbia.edu" , "kvm@vger.kernel.org" , Will Deacon , Christopher Covington To: Catalin Marinas Return-path: Received: from service87.mimecast.com ([91.220.42.44]:35857 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756140Ab3D3KXu convert rfc822-to-8bit (ORCPT ); Tue, 30 Apr 2013 06:23:50 -0400 In-Reply-To: <20130429173521.GB16544@arm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 29/04/13 18:35, Catalin Marinas wrote: > On Mon, Apr 08, 2013 at 05:17:10PM +0100, Marc Zyngier wrote: >> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h >> new file mode 100644 >> index 0000000..2eb2230 >> --- /dev/null >> +++ b/arch/arm64/include/asm/kvm_mmu.h > ... >> +/* >> + * Align KVM with the kernel's view of physical memory. Should be >> + * 40bit IPA, with PGD being 8kB aligned. >> + */ >> +#define KVM_PHYS_SHIFT PHYS_MASK_SHIFT > > Just wondering - do we allow the IPA to be bigger than what qemu/kvmtool > can map? > >> +#define KVM_PHYS_SIZE (1UL << KVM_PHYS_SHIFT) >> +#define KVM_PHYS_MASK (KVM_PHYS_SIZE - 1UL) >> + >> +#ifdef CONFIG_ARM64_64K_PAGES >> +#define PAGE_LEVELS 2 >> +#define BITS_PER_LEVEL 13 >> +#else /* 4kB pages */ >> +#define PAGE_LEVELS 3 >> +#define BITS_PER_LEVEL 9 > > You could use (PAGE_SHIFT - 3) for BITS_PER_LEVEL if that's any clearer > but you can get rid of these entirely (see below). > >> +#endif >> + >> +/* Make sure we get the right size, and thus the right alignment */ >> +#define BITS_PER_S2_PGD (KVM_PHYS_SHIFT - (PAGE_LEVELS - 1) * BITS_PER_LEVEL - PAGE_SHIFT) >> +#define PTRS_PER_S2_PGD (1 << max(BITS_PER_LEVEL, BITS_PER_S2_PGD)) >> +#define S2_PGD_ORDER get_order(PTRS_PER_S2_PGD * sizeof(pgd_t)) >> +#define S2_PGD_SIZE (1 << S2_PGD_ORDER) > > It looks like lots of calculations which I can't fully follow ;). So we > need to map KVM_PHYS_SHIFT (40-bit) with a number of stage 2 pgds. We > know that a pgd entry can map PGDIR_SHIFT bits. So the pointers you need > in S2 pgd: > > #define PTRS_PER_S2_PGD (1 << (KVM_PHYS_SHIFT - PGDIR_SHIFT)) > > (no need of PAGE_LEVELS) Now you're ruining all the fun! ;-) You make it look simple and elegant, which sucks in a major way! > With some more juggling you can probably get rid of get_order as well, > though it should be a compile-time constant. Basically KVM_PHYS_SHIFT - > PGDIR_SHIFT - PAGE_SHIFT + 3 (and cope with the negative value for the > 64K pages). get_order seems to do the right thing, and I'm now almost convinced I can loose the max. Almost. Hmmm. >> +static inline void coherent_icache_guest_page(struct kvm *kvm, gfn_t gfn) >> +{ >> + unsigned long hva = gfn_to_hva(kvm, gfn); >> + flush_icache_range(hva, hva + PAGE_SIZE); > > Even ARMv8 can have aliasing VIPT I-cache. Do we need to flush some more > here? Definitely. We should have something similar to what we have for ARMv7. >> +#define kvm_flush_dcache_to_poc(a,l) __flush_dcache_area((a), (l)) > > I had a brief look at the other hyp reworking patches and this function > is used in two situations: (1) flush any data before the Hyp is > initialised and (2) page table creation. The first case looks fine but > the second is not needed on AArch64 (and SMP AArch32) nor entirely > optimal as it should flush to PoU for page table walks. Can we have > different functions, at least we can make one a no-op? Certainly. I think Will has some patches that we could reuse (at least in principle) for AArch32, and have a no-op backend for AArch64. Thanks, M. -- Jazz is not dead. It just smells funny...