Re: [PATCH 3/3] add support for change_pte mmu notifiers

[Izik Eidus <ieidus@redhat.com>]

Marcelo Tosatti wrote:
> On Thu, Sep 10, 2009 at 07:38:58PM +0300, Izik Eidus wrote:
>   
>> this is needed for kvm if it want ksm to directly map pages into its
>> shadow page tables.
>>
>> Signed-off-by: Izik Eidus <ieidus@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h |    1 +
>>  arch/x86/kvm/mmu.c              |   70 ++++++++++++++++++++++++++++++++++----
>>  virt/kvm/kvm_main.c             |   14 ++++++++
>>  3 files changed, 77 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 6046e6f..594d131 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -797,6 +797,7 @@ asmlinkage void kvm_handle_fault_on_reboot(void);
>>  #define KVM_ARCH_WANT_MMU_NOTIFIER
>>  int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
>>  int kvm_age_hva(struct kvm *kvm, unsigned long hva);
>> +void kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte);
>>  int cpuid_maxphyaddr(struct kvm_vcpu *vcpu);
>>  int kvm_cpu_has_interrupt(struct kvm_vcpu *vcpu);
>>  int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu);
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index a7151b8..3fd19f2 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -282,6 +282,11 @@ static pfn_t spte_to_pfn(u64 pte)
>>  	return (pte & PT64_BASE_ADDR_MASK) >> PAGE_SHIFT;
>>  }
>>  
>> +static pte_t ptep_val(pte_t *ptep)
>> +{
>> +	return *ptep;
>> +}
>> +
>>  static gfn_t pse36_gfn_delta(u32 gpte)
>>  {
>>  	int shift = 32 - PT32_DIR_PSE36_SHIFT - PAGE_SHIFT;
>> @@ -748,7 +753,8 @@ static int rmap_write_protect(struct kvm *kvm, u64 gfn)
>>  	return write_protected;
>>  }
>>  
>> -static int kvm_unmap_rmapp(struct kvm *kvm, unsigned long *rmapp)
>> +static int kvm_unmap_rmapp(struct kvm *kvm, unsigned long *rmapp,
>> +			   unsigned long data)
>>  {
>>  	u64 *spte;
>>  	int need_tlb_flush = 0;
>> @@ -763,8 +769,48 @@ static int kvm_unmap_rmapp(struct kvm *kvm, unsigned long *rmapp)
>>  	return need_tlb_flush;
>>  }
>>  
>> +static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
>> +			     unsigned long data)
>> +{
>> +	int need_flush = 0;
>> +	u64 *spte, new_spte;
>> +	pte_t *ptep = (pte_t *)data;
>> +	pfn_t new_pfn;
>> +
>> +	new_pfn = pte_pfn(ptep_val(ptep));
>> +	spte = rmap_next(kvm, rmapp, NULL);
>> +	while (spte) {
>> +		BUG_ON(!is_shadow_present_pte(*spte));
>> +		rmap_printk("kvm_set_pte_rmapp: spte %p %llx\n", spte, *spte);
>> +		need_flush = 1;
>> +		if (pte_write(ptep_val(ptep))) {
>> +			rmap_remove(kvm, spte);
>> +			__set_spte(spte, shadow_trap_nonpresent_pte);
>> +			spte = rmap_next(kvm, rmapp, NULL);
>> +		} else {
>> +			new_spte = *spte &~ (PT64_BASE_ADDR_MASK);
>> +			new_spte |= new_pfn << PAGE_SHIFT;
>> +
>> +			if (!pte_write(ptep_val(ptep))) {
>> +				new_spte &= ~PT_WRITABLE_MASK;
>> +				new_spte &= ~SPTE_HOST_WRITEABLE;
>> +				if (is_writeble_pte(*spte))
>> +					kvm_set_pfn_dirty(spte_to_pfn(*spte));
>> +			}
>> +			__set_spte(spte, new_spte);
>> +			spte = rmap_next(kvm, rmapp, spte);
>> +		}
>> +	}
>> +	if (need_flush)
>> +		kvm_flush_remote_tlbs(kvm);
>> +
>> +	return 0;
>> +}
>> +
>>  static int kvm_handle_hva(struct kvm *kvm, unsigned long hva,
>> -			  int (*handler)(struct kvm *kvm, unsigned long *rmapp))
>> +			  unsigned long data,
>> +			  int (*handler)(struct kvm *kvm, unsigned long *rmapp,
>> +					 unsigned long data))
>>  {
>>  	int i, j;
>>  	int retval = 0;
>> @@ -786,13 +832,15 @@ static int kvm_handle_hva(struct kvm *kvm, unsigned long hva,
>>  		if (hva >= start && hva < end) {
>>  			gfn_t gfn_offset = (hva - start) >> PAGE_SHIFT;
>>  
>> -			retval |= handler(kvm, &memslot->rmap[gfn_offset]);
>> +			retval |= handler(kvm, &memslot->rmap[gfn_offset],
>> +					  data);
>>  
>>  			for (j = 0; j < KVM_NR_PAGE_SIZES - 1; ++j) {
>>  				int idx = gfn_offset;
>>  				idx /= KVM_PAGES_PER_HPAGE(PT_DIRECTORY_LEVEL + j);
>>  				retval |= handler(kvm,
>> -					&memslot->lpage_info[j][idx].rmap_pde);
>> +					&memslot->lpage_info[j][idx].rmap_pde,
>> +					data);
>>     
>
> If change_pte is called to modify a largepage pte, and the shadow has
> that largepage mapped with 4k sptes, you'll set the wrong pfn. That is,
> the patch does not attempt to handle different page sizes properly.
>
> So either disable change_pte updates to non-4k host vmas (making it
> explictly it does not handle different pagesizes), or handle largepages
> properly, although i don't see any use for change_pte or largepage
> mappings?
>   

change_pte doesn't get called on 4k pages...
So adding commet to this function saying it is working just on 4k pages 
would be enough ?