From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v3 03/13] nEPT: Add EPT tables support to paging_tmpl.h Date: Tue, 21 May 2013 15:52:12 +0800 Message-ID: <519B27AC.6090903@linux.vnet.ibm.com> References: <1368939152-11406-1-git-send-email-jun.nakajima@intel.com> <1368939152-11406-3-git-send-email-jun.nakajima@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Gleb Natapov , Paolo Bonzini To: Jun Nakajima Return-path: Received: from e23smtp06.au.ibm.com ([202.81.31.148]:47734 "EHLO e23smtp06.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751677Ab3EUHwY (ORCPT ); Tue, 21 May 2013 03:52:24 -0400 Received: from /spool/local by e23smtp06.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 21 May 2013 17:45:51 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id B35AF2CE8023 for ; Tue, 21 May 2013 17:52:16 +1000 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r4L7c8YW21430358 for ; Tue, 21 May 2013 17:38:09 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r4L7qFfP011291 for ; Tue, 21 May 2013 17:52:15 +1000 In-Reply-To: <1368939152-11406-3-git-send-email-jun.nakajima@intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On 05/19/2013 12:52 PM, Jun Nakajima wrote: > From: Nadav Har'El > > This is the first patch in a series which adds nested EPT support to KVM's > nested VMX. Nested EPT means emulating EPT for an L1 guest so that L1 can use > EPT when running a nested guest L2. When L1 uses EPT, it allows the L2 guest > to set its own cr3 and take its own page faults without either of L0 or L1 > getting involved. This often significanlty improves L2's performance over the > previous two alternatives (shadow page tables over EPT, and shadow page > tables over shadow page tables). > > This patch adds EPT support to paging_tmpl.h. > > paging_tmpl.h contains the code for reading and writing page tables. The code > for 32-bit and 64-bit tables is very similar, but not identical, so > paging_tmpl.h is #include'd twice in mmu.c, once with PTTTYPE=32 and once > with PTTYPE=64, and this generates the two sets of similar functions. > > There are subtle but important differences between the format of EPT tables > and that of ordinary x86 64-bit page tables, so for nested EPT we need a > third set of functions to read the guest EPT table and to write the shadow > EPT table. > > So this patch adds third PTTYPE, PTTYPE_EPT, which creates functions (prefixed > with "EPT") which correctly read and write EPT tables. > > Signed-off-by: Nadav Har'El > Signed-off-by: Jun Nakajima > Signed-off-by: Xinhao Xu > --- > arch/x86/kvm/mmu.c | 5 +++++ > arch/x86/kvm/paging_tmpl.h | 43 +++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 117233f..6c1670f 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -3397,6 +3397,11 @@ static inline bool is_last_gpte(struct kvm_mmu *mmu, unsigned level, unsigned gp > return mmu->last_pte_bitmap & (1 << index); > } > > +#define PTTYPE_EPT 18 /* arbitrary */ > +#define PTTYPE PTTYPE_EPT > +#include "paging_tmpl.h" > +#undef PTTYPE > + > #define PTTYPE 64 > #include "paging_tmpl.h" > #undef PTTYPE > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h > index df34d4a..4c45654 100644 > --- a/arch/x86/kvm/paging_tmpl.h > +++ b/arch/x86/kvm/paging_tmpl.h > @@ -50,6 +50,22 @@ > #define PT_LEVEL_BITS PT32_LEVEL_BITS > #define PT_MAX_FULL_LEVELS 2 > #define CMPXCHG cmpxchg > +#elif PTTYPE == PTTYPE_EPT > + #define pt_element_t u64 > + #define guest_walker guest_walkerEPT > + #define FNAME(name) EPT_##name > + #define PT_BASE_ADDR_MASK PT64_BASE_ADDR_MASK > + #define PT_LVL_ADDR_MASK(lvl) PT64_LVL_ADDR_MASK(lvl) > + #define PT_LVL_OFFSET_MASK(lvl) PT64_LVL_OFFSET_MASK(lvl) > + #define PT_INDEX(addr, level) PT64_INDEX(addr, level) > + #define PT_LEVEL_BITS PT64_LEVEL_BITS > + #ifdef CONFIG_X86_64 > + #define PT_MAX_FULL_LEVELS 4 > + #define CMPXCHG cmpxchg > + #else > + #define CMPXCHG cmpxchg64 CMPXHG is only used in FNAME(cmpxchg_gpte), but you commented it later. Do we really need it? > + #define PT_MAX_FULL_LEVELS 2 And the SDM says: "It uses a page-walk length of 4, meaning that at most 4 EPT paging-structure entriesare accessed to translate a guest-physical address.", Is My SDM obsolete? Which kind of process supports page-walk length = 2? It seems your patch is not able to handle the case that the guest uses walk-lenght = 2 which is running on the host with walk-lenght = 4. (plrease refer to how to handle sp->role.quadrant in FNAME(get_level1_sp_gpa) in the current code.) > + #endif > #else > #error Invalid PTTYPE value > #endif > @@ -80,6 +96,10 @@ static gfn_t gpte_to_gfn_lvl(pt_element_t gpte, int lvl) > return (gpte & PT_LVL_ADDR_MASK(lvl)) >> PAGE_SHIFT; > } > > +#if PTTYPE != PTTYPE_EPT > +/* > + * Comment out this for EPT because update_accessed_dirty_bits() is not used. > + */ > static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > pt_element_t __user *ptep_user, unsigned index, > pt_element_t orig_pte, pt_element_t new_pte) > @@ -102,6 +122,7 @@ static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > > return (ret != orig_pte); > } > +#endif > > static bool FNAME(prefetch_invalid_gpte)(struct kvm_vcpu *vcpu, > struct kvm_mmu_page *sp, u64 *spte, > @@ -126,13 +147,21 @@ no_present: > static inline unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, u64 gpte) > { > unsigned access; > - > +#if PTTYPE == PTTYPE_EPT > + access = (gpte & (VMX_EPT_READABLE_MASK | VMX_EPT_WRITABLE_MASK | > + VMX_EPT_EXECUTABLE_MASK)); It seems wrong. The ACC_XXX definition: #define ACC_EXEC_MASK 1 #define ACC_WRITE_MASK PT_WRITABLE_MASK #define ACC_USER_MASK PT_USER_MASK #define ACC_ALL (ACC_EXEC_MASK | ACC_WRITE_MASK | ACC_USER_MASK) The bits are different with the bits used in EPT page table, for example, your code always see that the execution is not allowed. > +#else > access = (gpte & (PT_WRITABLE_MASK | PT_USER_MASK)) | ACC_EXEC_MASK; > access &= ~(gpte >> PT64_NX_SHIFT); > +#endif > > return access; > } > > +#if PTTYPE != PTTYPE_EPT > +/* > + * EPT A/D bit support is not implemented. > + */ > static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu, > struct kvm_mmu *mmu, > struct guest_walker *walker, > @@ -169,6 +198,7 @@ static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu, > } > return 0; > } > +#endif > > /* > * Fetch a guest pte for a guest virtual address > @@ -177,7 +207,6 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, > struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > gva_t addr, u32 access) > { > - int ret; > pt_element_t pte; > pt_element_t __user *uninitialized_var(ptep_user); > gfn_t table_gfn; > @@ -192,7 +221,9 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, > gfn_t gfn; > > trace_kvm_mmu_pagetable_walk(addr, access); > +#if PTTYPE != PTTYPE_EPT > retry_walk: > +#endif > walker->level = mmu->root_level; > pte = mmu->get_cr3(vcpu); > > @@ -277,6 +308,7 @@ retry_walk: > > walker->gfn = real_gpa >> PAGE_SHIFT; > > +#if PTTYPE != PTTYPE_EPT > if (!write_fault) > protect_clean_gpte(&pte_access, pte); > else > @@ -287,12 +319,15 @@ retry_walk: > accessed_dirty &= pte >> (PT_DIRTY_SHIFT - PT_ACCESSED_SHIFT); > > if (unlikely(!accessed_dirty)) { > + int ret; > + > ret = FNAME(update_accessed_dirty_bits)(vcpu, mmu, walker, write_fault); > if (unlikely(ret < 0)) > goto error; > else if (ret) > goto retry_walk; > } > +#endif There are lots of code in paging_tmpl.h depends on PT_ACCESSED_MASK/PT_DIRTY_MASK. I do not see other parts are adjusted in your patch. How about redefine PT_ACCESSED_MASK / PT_DIRTY_MASK, something like: #if PTTYPE == 32 PT_ACCESS = PT_ACCESSED_MASK; ...... #elif PTTYPE == 64 PT_ACCESS = PT_ACCESSED_MASK; ...... #elif PTTYPE == PTTYPE_EPT PT_ACCESS = 0 #else ....... I guess the compiler can drop the unnecessary branch when PT_ACCESS == 0. Also, it can help use to remove the untidy "#if PTTYPE != PTTYPE_EPT" > > walker->pt_access = pt_access; > walker->pte_access = pte_access; > @@ -323,6 +358,7 @@ static int FNAME(walk_addr)(struct guest_walker *walker, > access); > } > > +#if PTTYPE != PTTYPE_EPT > static int FNAME(walk_addr_nested)(struct guest_walker *walker, > struct kvm_vcpu *vcpu, gva_t addr, > u32 access) > @@ -330,6 +366,7 @@ static int FNAME(walk_addr_nested)(struct guest_walker *walker, > return FNAME(walk_addr_generic)(walker, vcpu, &vcpu->arch.nested_mmu, > addr, access); > } > +#endif > > static bool > FNAME(prefetch_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, > @@ -754,6 +791,7 @@ static gpa_t FNAME(gva_to_gpa)(struct kvm_vcpu *vcpu, gva_t vaddr, u32 access, > return gpa; > } > > +#if PTTYPE != PTTYPE_EPT > static gpa_t FNAME(gva_to_gpa_nested)(struct kvm_vcpu *vcpu, gva_t vaddr, > u32 access, > struct x86_exception *exception) > @@ -772,6 +810,7 @@ static gpa_t FNAME(gva_to_gpa_nested)(struct kvm_vcpu *vcpu, gva_t vaddr, > > return gpa; > } > +#endif Strange! Why does nested ept not need these functions? How to emulate the instruction faulted on L2?