Re: [PATCH] KVM: MMU: Add shadow_accessed_shift

[Avi Kivity <avi@qumranet.com>]

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.

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

> @@ -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?)

>  	    && !(*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.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.