Re: [PATCH 02/10] nEPT: MMU context for nested EPT

[Avi Kivity <avi@redhat.com>]

On 11/13/2011 01:30 PM, Orit Wasserman wrote:
> Maybe this patch can help, this is roughly what Avi wants (I hope) done very quickly.
> I'm sorry I don't have setup to run nested VMX at the moment so i can't test it.
>
> Orit
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 9335e1b..bbe212f 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3180,6 +3180,10 @@ static bool sync_mmio_spte(u64 *sptep, gfn_t gfn, unsigned access,
>  #include "paging_tmpl.h"
>  #undef PTTYPE
>  
> +#define PTTYPE EPT
> +#include "paging_tmpl.h"
> +#undef PTTYPE
> +

Yes, that's the key.


>  int kvm_mmu_get_spte_hierarchy(struct kvm_vcpu *vcpu, u64 addr, u64 sptes[4]);
>  void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask);
>  int handle_mmio_page_fault_common(struct kvm_vcpu *vcpu, u64 addr, bool direct);
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 507e2b8..70d4cfd 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -39,6 +39,21 @@
>  	#define CMPXCHG cmpxchg64
>  	#define PT_MAX_FULL_LEVELS 2
>  	#endif
> +#elif PTTYPE == EPT
> +	#define pt_element_t u64
> +	#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
> +	#define PT_MAX_FULL_LEVELS 2
> +	#endif

The various masks should be defined here, to avoid lots of #ifdefs later.

>  #elif PTTYPE == 32
>  	#define pt_element_t u32
>  	#define guest_walker guest_walker32
> @@ -106,14 +121,19 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, pt_element_t gpte,
>  {
>  	unsigned access;
>  
> +#if PTTYPE == EPT       
>  	access = (gpte & (PT_WRITABLE_MASK | PT_USER_MASK)) | ACC_EXEC_MASK;
> +#else
> +	access = (gpte & EPT_WRITABLE_MASK) | EPT_EXEC_MASK;
>  	if (last && !is_dirty_gpte(gpte))
>  		access &= ~ACC_WRITE_MASK;
> +#endif

Like here, you could make is_dirty_gpte() local to paging_tmpl()
returning true for EPT and the dirty bit otherwise.


>  
>  #if PTTYPE == 64
>  	if (vcpu->arch.mmu.nx)
>  		access &= ~(gpte >> PT64_NX_SHIFT);

The ept X bit is lost.

Could do something like

   access &= (gpte >> PT_X_NX_SHIFT) ^ PT_X_NX_SENSE;


> +#if PTTYPE == EPT
> +	const int write_fault = access & EPT_WRITABLE_MASK;
> +	const int user_fault  = 0;
> +	const int fetch_fault = 0;
> +#else

EPT has fetch permissions (but not user permissions); anyway
translate_nested_gpa() already does this.

>  	const int write_fault = access & PFERR_WRITE_MASK;
>  	const int user_fault  = access & PFERR_USER_MASK;
>  	const int fetch_fault = access & PFERR_FETCH_MASK;
> +#endif
>  	u16 errcode = 0;
>  
>  	trace_kvm_mmu_pagetable_walk(addr, write_fault, user_fault,
> @@ -174,6 +200,9 @@ retry_walk:
>  	       (mmu->get_cr3(vcpu) & CR3_NONPAE_RESERVED_BITS) == 0);
>  
>  	pt_access = ACC_ALL;
> +#if PTTYPE == EPT 
> +	pt_access =  PT_PRESENT_MASK | EPT_WRITABLE_MASK | EPT_EXEC_MASK;
> +#endif

pt_access is not in EPT or ia32 format - it's our own format (xwu).  So
this doesn't need changing.  Updating gpte_access() is sufficient.

>  
>  	for (;;) {
>  		gfn_t real_gfn;
> @@ -186,9 +215,14 @@ retry_walk:
>  		pte_gpa   = gfn_to_gpa(table_gfn) + offset;
>  		walker->table_gfn[walker->level - 1] = table_gfn;
>  		walker->pte_gpa[walker->level - 1] = pte_gpa;
> -
> +#if PTTYPE == EPT 
> +		real_gfn = mmu->translate_gpa(vcpu, gfn_to_gpa(table_gfn),
> +					      EPT_WRITABLE_MASK);
> +#else
>  		real_gfn = mmu->translate_gpa(vcpu, gfn_to_gpa(table_gfn),
>  					      PFERR_USER_MASK|PFERR_WRITE_MASK);
> +#endif
> +

Unneeded, I think.

>  		if (unlikely(real_gfn == UNMAPPED_GVA))
>  			goto error;
>  		real_gfn = gpa_to_gfn(real_gfn);
> @@ -221,6 +255,7 @@ retry_walk:
>  			eperm = true;
>  #endif
>  
> +#if PTTYPE != EPT
>  		if (!eperm && unlikely(!(pte & PT_ACCESSED_MASK))) {
>  			int ret;
>  			trace_kvm_mmu_set_accessed_bit(table_gfn, index,
> @@ -235,7 +270,7 @@ retry_walk:
>  			mark_page_dirty(vcpu->kvm, table_gfn);
>  			pte |= PT_ACCESSED_MASK;
>  		}
> -
> +#endif

If PT_ACCESSED_MASK is 0 for EPT, this goes away without #ifdef.

> +#if PTTYPE != EPT
>  			/* check if the kernel is fetching from user page */
>  			if (unlikely(pte_access & PT_USER_MASK) &&
>  			    kvm_read_cr4_bits(vcpu, X86_CR4_SMEP))
>  				if (fetch_fault && !user_fault)
>  					eperm = true;
> -
> +#endif

Same here.



-- 
error compiling committee.c: too many arguments to function