Re: [PATCH] KVM: MMU: Add shadow_accessed_shift

[Sheng Yang <yasker@gmail.com>]

On Fri, Aug 29, 2008 at 02:49:38PM +0300, Avi Kivity wrote:
> Yang, Sheng wrote:
>> From: Sheng Yang <sheng.yang@intel.com>
>> Date: Fri, 29 Aug 2008 14:02:29 +0800
>> Subject: [PATCH] KVM: MMU: Add shadow_accessed_shift
>>   
>
>>  static u64 __read_mostly shadow_dirty_mask;
>>
>>  void kvm_mmu_set_nonpresent_ptes(u64 trap_pte, u64 notrap_pte)
>> @@ -171,6 +172,8 @@ void kvm_mmu_set_mask_ptes(u64 user_mask, u64  
>> accessed_mask,
>>  {
>>  	shadow_user_mask = user_mask;
>>  	shadow_accessed_mask = accessed_mask;
>> +	shadow_accessed_shift = find_first_bit((unsigned long *)&accessed_mask,
>> +					       sizeof(accessed_mask));
>>   
>
> Ah, I thought ept accessed_mask was zero.

Ah... Indeed I gave a FAKE_ACCESSED_MASK(1<<62) to EPT when the patch checked
in. I just want to keep the behaviour consistent with EPT and shadow
rather than do "if xxx_mask == 0" all the time.

I thought the FAKE one can be set when the EPTE is new (and maybe some
rare condition below). But I have neglected the check of
kvm_mmu_access_page() below when I add those fake bits...

>
>>  	shadow_dirty_mask = dirty_mask;
>>  	shadow_nx_mask = nx_mask;
>>  	shadow_x_mask = x_mask;
>> @@ -709,10 +712,10 @@ static int kvm_age_rmapp(struct kvm *kvm, 
>> unsigned long *rmapp)
>>  		int _young;
>>  		u64 _spte = *spte;
>>  		BUG_ON(!(_spte & PT_PRESENT_MASK));
>> -		_young = _spte & PT_ACCESSED_MASK;
>> +		_young = _spte & shadow_accessed_mask;
>>   
>
> _young is an int, so will be set to zero on ept due to truncation.
>
>>  		if (_young) {
>>  			young = 1;
>> -			clear_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
>> +			clear_bit(shadow_accessed_shift, (unsigned long *)spte);
>>  		}
>>  		spte = rmap_next(kvm, rmapp, spte);
>>  	}
>>   
>
> Even if the former is fixed, what does it mean?  guest memory will never  
> be considered young by Linux, and so will be swapped sooner.
>
> Maybe we ought to cheat in the other direction and return young = 1 for ept.

If I understand correctly, now I don't have idea when we can return young=1
except ept entry was just created. For EPT, we may return 0 for the most of
time, after this clear_bit action.

>
>> @@ -1785,7 +1788,7 @@ static void kvm_mmu_access_page(struct kvm_vcpu 
>> *vcpu, gfn_t gfn)
>>  	    && shadow_accessed_mask
>>   
>
> Ah, this is to disable this check for ept (I thought accessed_mask was  
> zero; why?)

My fault. Overlook this from the beginning... The check here no longer
fit for EPT, for shadow_accessed_mask of EPT is not zero now. I will
remove that.

>
>>  	    && !(*spte & shadow_accessed_mask)
>>  	    && is_shadow_present_pte(*spte))
>> -		set_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
>> +		set_bit(shadow_accessed_shift, (unsigned long *)spte);
>>  }
>>   
>
> ... but in any case I don't think this code path is ever hit?  maybe  
> doing a movsb from mmio to RAM.

Thanks... movsb seems reasonable, maybe I just thought it's "in case"... :)

--
regards
Yang, Sheng
>
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html