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

[Anthony Liguori <anthony@codemonkey.ws>]

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?

>  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.

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