Re: [PATCH 3/6] KVM: MMU: Add EPT support

["Yang, Sheng" <sheng.yang@intel.com>]

On Friday 18 April 2008 23:54:04 Anthony Liguori wrote:
> Yang, Sheng wrote:
> > On Friday 18 April 2008 21:30:14 Anthony Liguori wrote:
> >> Yang, Sheng wrote:
> >>> @@ -1048,17 +1071,18 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu,
> >>> u64 *shadow_pte,
> >>>  	 * whether the guest actually used the pte (in order to detect
> >>>  	 * demand paging).
> >>>  	 */
> >>> -	spte = PT_PRESENT_MASK | PT_DIRTY_MASK;
> >>> +	spte = shadow_base_present_pte | shadow_dirty_mask;
> >>>  	if (!speculative)
> >>>  		pte_access |= PT_ACCESSED_MASK;
> >>>  	if (!dirty)
> >>>  		pte_access &= ~ACC_WRITE_MASK;
> >>> -	if (!(pte_access & ACC_EXEC_MASK))
> >>> -		spte |= PT64_NX_MASK;
> >>> -
> >>> -	spte |= PT_PRESENT_MASK;
> >>> +	if (pte_access & ACC_EXEC_MASK) {
> >>> +		if (shadow_x_mask)
> >>> +			spte |= shadow_x_mask;
> >>> +	} else if (shadow_nx_mask)
> >>> +		spte |= shadow_nx_mask;
> >>
> >> This looks like it may be a bug.  The old behavior sets NX if
> >> (pte_access & ACC_EXEC_MASK).  The new behavior unconditionally sets NX
> >> and never sets PRESENT.  Also, the if (shadow_x_mas k) checks are
> >> unnecessary.  spte |= 0 is a nop.
> >
> > Thanks for the comment! I realized two judgments of shadow_nx/x_mask is
> > unnecessary... In fact, the correct behavior is either set shadow_x_mask
> > or shadow_nx_mask, may be there is a better approach for this. The logic
> > assured by program itself is always safer. But I will remove the
> > redundant code at first.
> >
> > But I don't think it's a bug. The old behavior set NX if (!(pte_access &
> > ACC_EXEC_MASK)), the same as the new one.
>
> The new behavior sets NX regardless of whether (pte_access &
> ACC_EXEC_MASK).  Is the desired change to unconditionally set NX?

Oh, I may see the point... shadow_x_mask != shadow_nx_mask.

the old behavior was:

if (!(pte_access & ACC_EXEC_MASK))
	spte |= PT64_NX_MASK;

the new behavior is:

if (pte_access & ACC_EXEC_MASK) {
		spte |= shadow_x_mask;
} else spte |= shadow_nx_mask;

For current behavior, kvm_arch_init() got:
       kvm_mmu_set_mask_ptes(PT_USER_MASK, PT_ACCESSED_MASK,
                       PT_DIRTY_MASK, PT64_NX_MASK, 0);
which means shadow_nx_mask = PT64_NX_MASK, and shadow_x_mask = 0 (NX means not 
executable, and X means executable). 

In patch 5/6, EPT got:
       kvm_mmu_set_mask_ptes(0ull, VMX_EPT_FAKE_ACCESSED_MASK,
                       VMX_EPT_FAKE_DIRTY_MASK, 0ull,
                       VMX_EPT_EXECUTABLE_MASK);
which means, shadow_nx_mask = 0, and shadow_x_mask = VMX_EPT_EXECUTABLE_MASK

So, when shadow enabled, and (!(pte_access & ACC_EXEC_MASK)), then spte |= 
shadow_nx_mask = PT64_NX_MASK (no change would happen when the condition is 
not satisfied). 

When EPT enabled, and (pte_access & ACC_EXEC_MASK), then spte |= shadow_x_mask 
= VMX_EPT_EXECUTABLE_MASK (no change would happen when condition is not 
satisfied).

They are two different bit and mutual exclusive ones. Maybe there are some 
better way to get their meaning more clearly...

>
> >  And I also curious about the
> > PRESENT bit. You see, the PRESENT bit was set at the beginning of the
> > code, and I really don't know why the duplicate one exists there...
>
> Looking at the code, you appear to be right.  In the future, I think you
> should separate any cleanups (like removing the redundant setting of
> PRESENT) into a separate patch and stick to just programmatic changes of
> PT_USER_MASK => shadow_user_mask, etc. in this patch.  That makes it a
> lot easier to review correctness.

Thanks for the advice, it's important to separate the cleanups. I will get it 
done more properly next time. 

-- 
Thanks
Yang, Sheng
>
> Regards,
>
> Anthony Liguori
>
> >>>  	if (pte_access & ACC_USER_MASK)
> >>> -		spte |= PT_USER_MASK;
> >>> +		spte |= shadow_user_mask;
> >>>  	if (largepage)
> >>>  		spte |= PT_PAGE_SIZE_MASK;



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone