Re: large page support for kvm

[Avi Kivity <avi@qumranet.com>]

Marcelo Tosatti wrote:
> On Wed, Feb 13, 2008 at 08:45:51AM +0200, Avi Kivity wrote:
>   
>>> gfn_to_page() needs to grab the struct page corresponding to the large
>>> page, not the offset struct page for the faulting 4k address within
>>> the large frame. Since gfn_to_page can sleep, there is no way to do
>>> that in the mapping logic which happens under mmu_lock protection.
>>> We don't want to grab the large page frame "struct page" unless the
>>> is_largepage_backed() checks are successful.
>>>
>>> The checks could be done in page_fault() if walker->level == 2, before
>>> gfn_to_page()... But I don't see much difference of that and doing 
>>> it inside walk_addr(). What do you say?
>>>
>>>  
>>>       
>> I'd like to keep walk_addr() independent of the rest of the mmu (i.e. 
>> walk_addr is 100% guest oriented). Also, the issue you point out is 
>> shared by direct_map which doesn't call walk_addr().
>>
>> An unrelated issue (pointed out by Jun Nakajima) is that this kills 
>> dirty log tracking (needed for migration). It could be solved simply by 
>> not using large page backing if dirty log tracking is enabled for that slot.
>>     
>
> Ok, fixed your comments and a bug which a root page was shadowed in the
> large area being mapped. access.flat is happy.
>
> Joerg, can you give this a try on a NPT-enabled system (need the
> attached qemu-largepage-hack.patch).
>
> Thanks
>
> Index: kvm.largepages/arch/x86/kvm/mmu.c
> ===================================================================
> --- kvm.largepages.orig/arch/x86/kvm/mmu.c
> +++ kvm.largepages/arch/x86/kvm/mmu.c
> @@ -27,6 +27,7 @@
>  #include <linux/highmem.h>
>  #include <linux/module.h>
>  #include <linux/swap.h>
> +#include <linux/hugetlb.h>
>  
>  #include <asm/page.h>
>  #include <asm/cmpxchg.h>
> @@ -211,6 +212,11 @@ static int is_shadow_present_pte(u64 pte
>  		&& pte != shadow_notrap_nonpresent_pte;
>  }
>  
> +static int is_large_pte(u64 pte)
> +{
> +	return pte & PT_PAGE_SIZE_MASK;
> +}
> +
>  static int is_writeble_pte(unsigned long pte)
>  {
>  	return pte & PT_WRITABLE_MASK;
> @@ -350,17 +356,120 @@ static void mmu_free_rmap_desc(struct kv
>  	kfree(rd);
>  }
>  
> +static int hpage_align_diff(unsigned long gfn)
> +{
> +	return ((gfn+KVM_PAGES_PER_HPAGE-1) & ~(KVM_PAGES_PER_HPAGE-1)) - gfn;
> +}
> +
> +/*
> + * Return the pointer to the largepage write count for a given
> + * gfn, handling slots that are not large page aligned.
> + */
> +static int *slot_largepage_idx(gfn_t gfn, struct kvm_memory_slot *slot)
> +{
> +	unsigned long idx;
> +
> +	idx = (gfn - slot->base_gfn) + hpage_align_diff(slot->base_gfn);
> +	idx /= KVM_PAGES_PER_HPAGE;
> +	return &slot->lpage_info[idx].write_count;
> +}
>   

Can be further simplified to (gfn / KVM_PAGES_PER_HPAGE) - 
(slot->base_gfn / KVM_PAGES_PER_HPAGE).  Sorry for not noticing earlier.

> +
> +static int is_largepage_backed(struct kvm_vcpu *vcpu, gfn_t large_gfn)
> +{
> +	struct kvm_memory_slot *slot;
> +
> +	if (has_wrprotected_page(vcpu->kvm, large_gfn))
> +		return 0;
> +
> +	if (!host_largepage_backed(vcpu->kvm, large_gfn))
> +		return 0;
> +
> +	slot = gfn_to_memslot(vcpu->kvm, large_gfn);
> +	if (slot && slot->dirty_bitmap)
> +		return 0;
> +
> +	/* guest has 4M pages, host 2M */
> +	if (!is_pae(vcpu) && HPAGE_SHIFT == 21)
> +		return 0;
>   

Is this check necessary?  I think that if we remove it things will just 
work.  A 4MB page will be have either one or two 2MB sptes (which may 
even belong to different slots).

> @@ -894,12 +1030,28 @@ struct page *gva_to_page(struct kvm_vcpu
>  static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *shadow_pte,
>  			 unsigned pt_access, unsigned pte_access,
>  			 int user_fault, int write_fault, int dirty,
> -			 int *ptwrite, gfn_t gfn, struct page *page)
> +			 int *ptwrite, int largepage, gfn_t gfn,
> +			 struct page *page)
>  {
>  	u64 spte;
>  	int was_rmapped = is_rmap_pte(*shadow_pte);
>  	int was_writeble = is_writeble_pte(*shadow_pte);
>  
> +	/*
> + 	 * If we overwrite a PTE page pointer with a 2MB PMD, unlink
> + 	 * the parent of the now unreachable PTE.
> + 	 */
> +	if (largepage) {
> +		if (was_rmapped && !is_large_pte(*shadow_pte)) {
> +			struct kvm_mmu_page *child;
> +			u64 pte = *shadow_pte;
> +
> +			child = page_header(pte & PT64_BASE_ADDR_MASK);
> +			mmu_page_remove_parent_pte(child, shadow_pte);
> +           	}
> +		was_rmapped = is_large_pte(*shadow_pte);
> +	}
> +
>  	pgprintk("%s: spte %llx access %x write_fault %d"
>  		 " user_fault %d gfn %lx\n",
>  		 __FUNCTION__, *shadow_pte, pt_access,
> @@ -919,6 +1071,8 @@ static void mmu_set_spte(struct kvm_vcpu
>  	spte |= PT_PRESENT_MASK;
>  	if (pte_access & ACC_USER_MASK)
>  		spte |= PT_USER_MASK;
> +	if (largepage)
> +		spte |= PT_PAGE_SIZE_MASK;
>  
>  	spte |= page_to_phys(page);
>  
> @@ -933,7 +1087,8 @@ static void mmu_set_spte(struct kvm_vcpu
>  		}
>  
>  		shadow = kvm_mmu_lookup_page(vcpu->kvm, gfn);
> -		if (shadow) {
> +		if (shadow ||
> +		   (largepage && has_wrprotected_page(vcpu->kvm, gfn))) {
>  			pgprintk("%s: found shadow page for %lx, marking ro\n",
>  				 __FUNCTION__, gfn);
>  			pte_access &= ~ACC_WRITE_MASK;
> @@ -941,6 +1096,18 @@ static void mmu_set_spte(struct kvm_vcpu
>  				spte &= ~PT_WRITABLE_MASK;
>  				kvm_x86_ops->tlb_flush(vcpu);
>  			}
> +			/*
> + 			 * Largepage creation is susceptible to a upper-level
> + 			 * table to be shadowed and write-protected in the
> + 			 * area being mapped. If that is the case, invalidate
> + 			 * the entry and let the instruction fault again
> + 			 * and use 4K mappings.
> + 			 */
> +			if (largepage) {
> +				spte = shadow_trap_nonpresent_pte;
> +				kvm_x86_ops->tlb_flush(vcpu);
> +				goto unshadowed;
> +			}
>   

Would it not repeat exactly the same code path?  Or is this just for the 
case of the pte_update path?

> -	page = gfn_to_page(vcpu->kvm, gpa >> PAGE_SHIFT);
> +	if (is_largepage_backed(vcpu, gfn & ~(KVM_PAGES_PER_HPAGE-1))
> +	    && is_physical_memory(vcpu->kvm, gfn)) {
> +		gfn &= ~(KVM_PAGES_PER_HPAGE-1);
> +		largepage = 1;
> +	}
>   

Doesn't is_largepage_backed() imply is_physical_memory?

>  
> Index: kvm.largepages/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm.largepages.orig/arch/x86/kvm/x86.c
> +++ kvm.largepages/arch/x86/kvm/x86.c
> @@ -86,6 +86,7 @@ struct kvm_stats_debugfs_item debugfs_en
>  	{ "mmu_recycled", VM_STAT(mmu_recycled) },
>  	{ "mmu_cache_miss", VM_STAT(mmu_cache_miss) },
>  	{ "remote_tlb_flush", VM_STAT(remote_tlb_flush) },
> +	{ "lpages", VM_STAT(lpages) },
>  	{ NULL }
>  };
>   

s/lpages/largepages/, this is user visible.

> +		new.lpage_info = vmalloc(largepages * sizeof(*new.lpage_info));
> +
> +		if (!new.lpage_info)
> +			goto out_free;
> +
> +		memset(new.lpage_info, 0, largepages * sizeof(*new.lpage_info));
> +		/* large page crosses memslot boundary */
> +		if (npages % KVM_PAGES_PER_HPAGE) {
> +			new.lpage_info[0].write_count = 1;
>   

This seems wrong, say a 3MB slot at 1GB, you kill the first largepage 
which is good.

> +			new.lpage_info[largepages-1].write_count = 1;
>   

OTOH, a 3MB slot at 3MB, the last page is fine.  The check needs to be 
against base_gfn and base_gfn + npages, not the number of pages.

> +		}
>   



> +	}
>  
>  	/* Allocate page dirty bitmap if needed */
>  	if ((new.flags & KVM_MEM_LOG_DIRTY_PAGES) && !new.dirty_bitmap) {
> @@ -444,7 +464,7 @@ int kvm_is_visible_gfn(struct kvm *kvm, 
>  }
>  EXPORT_SYMBOL_GPL(kvm_is_visible_gfn);
>  
> -static unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
> +unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
>  {
>  	struct kvm_memory_slot *slot;
>  
> @@ -454,6 +474,7 @@ static unsigned long gfn_to_hva(struct k
>  		return bad_hva();
>  	return (slot->userspace_addr + (gfn - slot->base_gfn) * PAGE_SIZE);
>  }
> +EXPORT_SYMBOL(gfn_to_hva);
>  
>  /*
>   * Requires current->mm->mmap_sem to be held
>   
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2008.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> ------------------------------------------------------------------------
>
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel


-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/