kvm-devel

[kvm-devel] [PATCH] Handle vma regions with no backing page

[Anthony L. <ali...@us...>]

This patch allows VMA's that contain no backing page to be used for guest
memory.  This is a drop-in replacement for Ben-Ami's first page in his direct
mmio series.  Here, we continue to allow mmio pages to be represented in the
rmap.

Signed-off-by: Anthony Liguori <ali...@us...>

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f095b73..11b26f5 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -531,6 +531,7 @@ pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
 	struct page *page[1];
 	unsigned long addr;
 	int npages;
+	pfn_t pfn;
 
 	might_sleep();
 
@@ -543,19 +544,36 @@ pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
 	npages = get_user_pages(current, current->mm, addr, 1, 1, 1, page,
 				NULL);
 
-	if (npages != 1) {
-		get_page(bad_page);
-		return page_to_pfn(bad_page);
-	}
+	if (unlikely(npages != 1)) {
+		struct vm_area_struct *vma;
+
+		vma = find_vma(current->mm, addr);
+		if (vma == NULL) {
+			get_page(bad_page);
+			return page_to_pfn(bad_page);
+		}
+
+		BUG_ON(!(vma->vm_flags & VM_IO));
 
-	return page_to_pfn(page[0]);
+		pfn = ((addr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
+		BUG_ON(pfn_valid(pfn));
+	} else
+		pfn = page_to_pfn(page[0]);
+
+	return pfn;
 }
 
 EXPORT_SYMBOL_GPL(gfn_to_pfn);
 
 struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn)
 {
-	return pfn_to_page(gfn_to_pfn(kvm, gfn));
+	pfn_t pfn;
+
+	pfn = gfn_to_pfn(kvm, gfn);
+	if (pfn_valid(pfn))
+		return pfn_to_page(pfn);
+
+	return NULL;
 }
 
 EXPORT_SYMBOL_GPL(gfn_to_page);
@@ -568,7 +586,8 @@ EXPORT_SYMBOL_GPL(kvm_release_page_clean);
 
 void kvm_release_pfn_clean(pfn_t pfn)
 {
-	put_page(pfn_to_page(pfn));
+	if (pfn_valid(pfn))
+		put_page(pfn_to_page(pfn));
 }
 EXPORT_SYMBOL_GPL(kvm_release_pfn_clean);
 
@@ -593,21 +612,25 @@ EXPORT_SYMBOL_GPL(kvm_set_page_dirty);
 
 void kvm_set_pfn_dirty(pfn_t pfn)
 {
-	struct page *page = pfn_to_page(pfn);
-	if (!PageReserved(page))
-		SetPageDirty(page);
+	if (pfn_valid(pfn)) {
+		struct page *page = pfn_to_page(pfn);
+		if (!PageReserved(page))
+			SetPageDirty(page);
+	}
 }
 EXPORT_SYMBOL_GPL(kvm_set_pfn_dirty);
 
 void kvm_set_pfn_accessed(pfn_t pfn)
 {
-	mark_page_accessed(pfn_to_page(pfn));
+	if (pfn_valid(pfn))
+		mark_page_accessed(pfn_to_page(pfn));
 }
 EXPORT_SYMBOL_GPL(kvm_set_pfn_accessed);
 
 void kvm_get_pfn(pfn_t pfn)
 {
-	get_page(pfn_to_page(pfn));
+	if (pfn_valid(pfn))
+		get_page(pfn_to_page(pfn));
 }
 EXPORT_SYMBOL_GPL(kvm_get_pfn);
 

Re: [kvm-devel] [PATCH] Handle vma regions with no backing page

[Andrea A. <an...@qu...>]

On Tue, Apr 29, 2008 at 09:32:09AM -0500, Anthony Liguori wrote:
> +		vma = find_vma(current->mm, addr);
> +		if (vma == NULL) {
> +			get_page(bad_page);
> +			return page_to_pfn(bad_page);
> +		}

Here you must check vm_start address, find_vma only checks addr <
vm_end but there's no guarantee addr >= vm_start yet.

> +
> +		BUG_ON(!(vma->vm_flags & VM_IO));

For consistency we should return bad_page and not bug on, VM_IO and
VM_PFNMAP can theoretically not be set at the same time, otherwise
get_user_pages would be buggy checking against VM_PFNMAP|VM_IO. I
doubt anybody isn't setting VM_IO before calling remap_pfn_range but
anyway...

Secondly the really correct check is against VM_PFNMAP. This is
because PFNMAP is set at the same time of vm_pgoff = pfn. VM_IO is not
even if in theory if a driver uses ->fault instead of remap_pfn_range,
shouldn't set VM_IO and it should only set VM_RESERVED. VM_IO is about
keeping gdb/coredump out as they could mess with the hardware if they
read, PFNMAP is about remap_pfn_range having been called and pgoff
pointing to the first pfn mapped at vm_start address.

Patch is in the right direction, way to go!

Re: [kvm-devel] [PATCH] Handle vma regions with no backing page

[Anthony L. <ali...@us...>]

Andrea Arcangeli wrote:
> On Tue, Apr 29, 2008 at 09:32:09AM -0500, Anthony Liguori wrote:
>   
>> +		vma = find_vma(current->mm, addr);
>> +		if (vma == NULL) {
>> +			get_page(bad_page);
>> +			return page_to_pfn(bad_page);
>> +		}
>>     
>
> Here you must check vm_start address, find_vma only checks addr <
> vm_end but there's no guarantee addr >= vm_start yet.
>   

Indeed.

>> +
>> +		BUG_ON(!(vma->vm_flags & VM_IO));
>>     
>
> For consistency we should return bad_page and not bug on, VM_IO and
> VM_PFNMAP can theoretically not be set at the same time, otherwise
> get_user_pages would be buggy checking against VM_PFNMAP|VM_IO. I
> doubt anybody isn't setting VM_IO before calling remap_pfn_range but
> anyway...
>
> Secondly the really correct check is against VM_PFNMAP. This is
> because PFNMAP is set at the same time of vm_pgoff = pfn. VM_IO is not
> even if in theory if a driver uses ->fault instead of remap_pfn_range,
> shouldn't set VM_IO and it should only set VM_RESERVED. VM_IO is about
> keeping gdb/coredump out as they could mess with the hardware if they
> read, PFNMAP is about remap_pfn_range having been called and pgoff
> pointing to the first pfn mapped at vm_start address.
>   

Good point.  I've updated the patch.  Will send out again once I've 
gotten to test it.

Regards,

Anthony Liguori

> Patch is in the right direction, way to go!
>