Swapping

From 8e25e215b8ed95ca4ff51cbfcf5bdc438bb799f4 Mon Sep 17 00:00:00 2001

this patch make the guest non shadowed pages swappedable

------------------------------------------------------------------------

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
------------------------------------------------------------------------

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
From 8e25e215b8ed95ca4ff51cbfcf5bdc438bb799f4 Mon Sep 17 00:00:00 2001
From: Izik Eidus <izik@Home1.(none)>
Date: Sat, 13 Oct 2007 04:03:28 +0200
Subject: [PATCH] make the guest non shadowed memory swappable

Signed-off-by Izik Eidus <izike-atKUWr5tajBWk0Htik3J/[email protected]>

---
 drivers/kvm/kvm.h         |    1 +
 drivers/kvm/kvm_main.c    |   66 +++++++++++++++++++++++++-------------------
 drivers/kvm/mmu.c         |   13 ++++++++-
 drivers/kvm/paging_tmpl.h |   23 +++++++++++++--
 4 files changed, 70 insertions(+), 33 deletions(-)

drivers/kvm/kvm.h +1 -0

index a155c2b..2e83fa7 100644
@@ -409,6 +409,7 @@ struct kvm_memory_slot {
 	unsigned long *rmap;
 	unsigned long *dirty_bitmap;
 	int user_alloc; /* user allocated memory */
+	unsigned long userspace_addr;
 };
 
 struct kvm {

drivers/kvm/kvm_main.c +0 -2

index bfa201c..0dce93c 100644
@@ -321,15 +321,6 @@ static struct kvm *kvm_create_vm(void)
 
 static void kvm_free_userspace_physmem(struct kvm_memory_slot *free)
 {
-	int i;
-

static void kvm_free_kernel_physmem(struct kvm_memory_slot *free)

drivers/kvm/kvm_main.c +0 -5

@@ -771,19 +762,8 @@ static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 		memset(new.phys_mem, 0, npages * sizeof(struct page *));
 		memset(new.rmap, 0, npages * sizeof(*new.rmap));
 		if (user_alloc) {
-			unsigned long pages_num;
-
 			new.user_alloc = 1;
-			down_read(&current->mm->mmap_sem);
-
-			pages_num = get_user_pages(current, current->mm,

drivers/kvm/kvm_main.c +0 -0

 			for (i = 0; i < npages; ++i) {

drivers/kvm/kvm_main.c +9 -1

 				new.phys_mem[i] = alloc_page(GFP_HIGHUSER

@@ -1058,8 +1038,27 @@ struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn)
 
 	gfn = unalias_gfn(kvm, gfn);
 	slot = __gfn_to_memslot(kvm, gfn);
-	if (!slot)
+	if (!slot) {
+		get_page(bad_page);
 		return bad_page;
+	}
+	if (slot->user_alloc) {
+		struct page *page[1];
+		int npages;
+
+		down_read(&current->mm->mmap_sem);
+		npages = get_user_pages(current, current->mm,

drivers/kvm/kvm_main.c +2 -0

+	}
+	get_page(slot->phys_mem[gfn - slot->base_gfn]);
 	return slot->phys_mem[gfn - slot->base_gfn];
 }

EXPORT_SYMBOL_GPL(gfn_to_page);

drivers/kvm/kvm_main.c +15 -7

@@ -1079,13 +1078,16 @@ int kvm_read_guest_page(struct kvm *kvm, gfn_t gfn, void *data, int offset,
 	struct page *page;
 
 	page = gfn_to_page(kvm, gfn);
-	if (is_error_page(page))
+	if (is_error_page(page)) {
+		put_page(page);
 		return -EFAULT;
+	}
 	page_virt = kmap_atomic(page, KM_USER0);
 
 	memcpy(data, page_virt + offset, len);
 
 	kunmap_atomic(page_virt, KM_USER0);
+	put_page(page);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(kvm_read_guest_page);

@@ -1117,14 +1119,17 @@ int kvm_write_guest_page(struct kvm *kvm, gfn_t gfn, const void *data,
 	struct page *page;
 
 	page = gfn_to_page(kvm, gfn);
-	if (is_error_page(page))
+	if (is_error_page(page)) {
+		put_page(page);
 		return -EFAULT;
+	}
 	page_virt = kmap_atomic(page, KM_USER0);
 
 	memcpy(page_virt + offset, data, len);
 
 	kunmap_atomic(page_virt, KM_USER0);
 	mark_page_dirty(kvm, gfn);
+	put_page(page);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(kvm_write_guest_page);

@@ -1155,13 +1160,16 @@ int kvm_clear_guest_page(struct kvm *kvm, gfn_t gfn, int offset, int len)
 	struct page *page;
 
 	page = gfn_to_page(kvm, gfn);
-	if (is_error_page(page))
+	if (is_error_page(page)) {
+		put_page(page);
 		return -EFAULT;
+	}
 	page_virt = kmap_atomic(page, KM_USER0);
 
 	memset(page_virt + offset, 0, len);
 
 	kunmap_atomic(page_virt, KM_USER0);
+	put_page(page);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(kvm_clear_guest_page);

@@ -2090,13 +2098,12 @@ int kvm_emulate_pio_string(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
 	for (i = 0; i < nr_pages; ++i) {
 		mutex_lock(&vcpu->kvm->lock);
 		page = gva_to_page(vcpu, address + i * PAGE_SIZE);
-		if (page)
-			get_page(page);
 		vcpu->pio.guest_pages[i] = page;
 		mutex_unlock(&vcpu->kvm->lock);
 		if (!page) {
 			inject_gp(vcpu);
 			free_pio_guest_pages(vcpu);
 			return 1;
 		}
 	}

@@ -3081,9 +3088,10 @@ static struct page *kvm_vm_nopage(struct vm_area_struct *vma,
 
 	pgoff = ((address - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
 	page = gfn_to_page(kvm, pgoff);
-	if (is_error_page(page))
+	if (is_error_page(page)) {
+		put_page(page);
 		return NOPAGE_SIGBUS;
-	get_page(page);
+	}
 	if (type != NULL)
 		*type = VM_FAULT_MINOR;
 

drivers/kvm/mmu.c +12 -1

index e6a9b4a..19af09a 100644
@@ -425,6 +425,7 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
 	if (!is_rmap_pte(*spte))
 		return;
 	page = page_header(__pa(spte));
+	put_page(pfn_to_page((*spte & PT64_BASE_ADDR_MASK) >> PAGE_SHIFT));
 	rmapp = gfn_to_rmap(kvm, page->gfns[spte - page->spt]);
 	if (!*rmapp) {
 		printk(KERN_ERR "rmap_remove: %p %llx 0->BUG\n", spte, *spte);

@@ -907,6 +908,9 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, hpa_t p)
 			table[index] = p | PT_PRESENT_MASK | PT_WRITABLE_MASK |
 								PT_USER_MASK;
 			rmap_add(vcpu, &table[index], v >> PAGE_SHIFT);
+			if (!is_rmap_pte(table[index]))
+				put_page(pfn_to_page((v & PT64_BASE_ADDR_MASK)
+					 >> PAGE_SHIFT));
 			return 0;
 		}
 

@@ -921,6 +925,8 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, hpa_t p)
 						     1, 3, &table[index]);
 			if (!new_table) {
 				pgprintk("nonpaging_map: ENOMEM\n");
+				put_page(pfn_to_page((v & PT64_BASE_ADDR_MASK)
+						     >> PAGE_SHIFT));
 				return -ENOMEM;
 			}
 

@@ -1035,8 +1041,11 @@ static int nonpaging_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
 
 	paddr = gpa_to_hpa(vcpu->kvm, addr & PT64_BASE_ADDR_MASK);
 
-	if (is_error_hpa(paddr))
+	if (is_error_hpa(paddr)) {
+		put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+				     >> PAGE_SHIFT));
 		return 1;
+	}
 
 	return nonpaging_map(vcpu, addr & PAGE_MASK, paddr);
 }

@@ -1515,6 +1524,8 @@ static void audit_mappings_page(struct kvm_vcpu *vcpu, u64 page_pte,
 				 && !is_error_hpa(hpa))
 				printk(KERN_ERR "audit: (%s) notrap shadow,"
 				       " valid guest gva %lx\n", audit_msg, va);
+			put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+					     >> PAGE_SHIFT));
 
 		}
 	}

drivers/kvm/paging_tmpl.h +19 -3

index 58fd35a..931a308 100644
@@ -175,6 +175,7 @@ static int FNAME(walk_addr)(struct guest_walker *walker,
 		walker->inherited_ar &= walker->table[index];
 		table_gfn = (*ptep & PT_BASE_ADDR_MASK) >> PAGE_SHIFT;
 		kunmap_atomic(walker->table, KM_USER0);
+		put_page(walker->page);
 		walker->page = gfn_to_page(vcpu->kvm, table_gfn);
 		walker->table = kmap_atomic(walker->page, KM_USER0);
 		--walker->level;

@@ -183,8 +184,10 @@ static int FNAME(walk_addr)(struct guest_walker *walker,
 			 walker->level - 1, table_gfn);
 	}
 	walker->pte = *ptep;
-	if (walker->page)
+	if (walker->page) {
 		walker->ptep = NULL;
+		put_page(walker->page);
+	}
 	if (walker->table)
 		kunmap_atomic(walker->table, KM_USER0);
 	pgprintk("%s: pte %llx\n", __FUNCTION__, (u64)*ptep);

@@ -206,6 +209,8 @@ err:
 		walker->error_code |= PFERR_FETCH_MASK;
 	if (walker->table)
 		kunmap_atomic(walker->table, KM_USER0);
+	if (walker->page)
+		put_page(walker->page);
 	return 0;
 }
 

@@ -249,6 +254,8 @@ static void FNAME(set_pte_common)(struct kvm_vcpu *vcpu,
 	if (is_error_hpa(paddr)) {
 		set_shadow_pte(shadow_pte,
 			       shadow_trap_nonpresent_pte | PT_SHADOW_IO_MARK);
+		put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+				     >> PAGE_SHIFT));
 		return;
 	}
 

@@ -286,9 +293,16 @@ unshadowed:
 	pgprintk("%s: setting spte %llx\n", __FUNCTION__, spte);
 	set_shadow_pte(shadow_pte, spte);
 	page_header_update_slot(vcpu->kvm, shadow_pte, gaddr);
-	if (!was_rmapped)
+	if (!was_rmapped) {
 		rmap_add(vcpu, shadow_pte, (gaddr & PT64_BASE_ADDR_MASK)
 			 >> PAGE_SHIFT);
+		if (!is_rmap_pte(*shadow_pte))
+			put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+					     >> PAGE_SHIFT));
+	}
+	else
+		put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+				     >> PAGE_SHIFT));
 	if (!ptwrite || !*ptwrite)
 		vcpu->last_pte_updated = shadow_pte;
 }

@@ -512,19 +526,22 @@ static void FNAME(prefetch_page)(struct kvm_vcpu *vcpu,
 {
 	int i;
 	pt_element_t *gpt;
+	struct page *page;
 
 	if (sp->role.metaphysical || PTTYPE == 32) {
 		nonpaging_prefetch_page(vcpu, sp);
 		return;
 	}
 
-	gpt = kmap_atomic(gfn_to_page(vcpu->kvm, sp->gfn), KM_USER0);
+	page = gfn_to_page(vcpu->kvm, sp->gfn);
+	gpt = kmap_atomic(page, KM_USER0);
 	for (i = 0; i < PT64_ENT_PER_PAGE; ++i)

Quoted 57 lines · Izik Eidus

this patch make the guest non shadowed pages swappedable

------------------------------------------------------------------------

-------------------------------------------------------------------------

This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
------------------------------------------------------------------------

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
From 8e25e215b8ed95ca4ff51cbfcf5bdc438bb799f4 Mon Sep 17 00:00:00 2001
From: Izik Eidus <izik@Home1.(none)>
Date: Sat, 13 Oct 2007 04:03:28 +0200
Subject: [PATCH] make the guest non shadowed memory swappable

Signed-off-by Izik Eidus <izike-atKUWr5tajBWk0Htik3J/[email protected]>

---
 drivers/kvm/kvm.h         |    1 +
 drivers/kvm/kvm_main.c    |   66 
+++++++++++++++++++++++++-------------------
 drivers/kvm/mmu.c         |   13 ++++++++-
 drivers/kvm/paging_tmpl.h |   23 +++++++++++++--
 4 files changed, 70 insertions(+), 33 deletions(-)

drivers/kvm/kvm.h +1 -0

index a155c2b..2e83fa7 100644
@@ -409,6 +409,7 @@ struct kvm_memory_slot {
     unsigned long *rmap;
     unsigned long *dirty_bitmap;
     int user_alloc; /* user allocated memory */
+    unsigned long userspace_addr;
 };
 
 struct kvm {

drivers/kvm/kvm_main.c +0 -7

index bfa201c..0dce93c 100644
@@ -321,15 +321,6 @@ static struct kvm *kvm_create_vm(void)
 
 static void kvm_free_userspace_physmem(struct kvm_memory_slot *free)
 {
-    int i;
-
-    for (i = 0; i < free->npages; ++i) {
-        if (free->phys_mem[i]) {
-            if (!PageReserved(free->phys_mem[i]))
-                SetPageDirty(free->phys_mem[i]);
-            page_cache_release(free->phys_mem[i]);

drivers/kvm/kvm_main.c static struct kvm *kvm_create_vm(void)

Quoted 5 lines

drivers/kvm/kvm_main.c +0 -2

-        }
-    }
 }
 
 

If you're making this a nop, then you might as well just delete the function and it's callers.

Quoted 17 lines

static void kvm_free_kernel_physmem(struct kvm_memory_slot *free)

drivers/kvm/kvm_main.c +0 -9

@@ -771,19 +762,8 @@ static int kvm_vm_ioctl_set_memory_region(struct 
 kvm *kvm,
         memset(new.phys_mem, 0, npages * sizeof(struct page *));
         memset(new.rmap, 0, npages * sizeof(*new.rmap));
         if (user_alloc) {
-            unsigned long pages_num;
-
             new.user_alloc = 1;
-            down_read(&current->mm->mmap_sem);
-
-            pages_num = get_user_pages(current, current->mm,
-                           mem->userspace_addr,
-                           npages, 1, 0, new.phys_mem,
-                           NULL);
-

drivers/kvm/kvm_main.c static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,

Quoted 6 lines

drivers/kvm/kvm_main.c +1 -3

-            up_read(&current->mm->mmap_sem);
-            if (pages_num != npages)
-                goto out_unlock;
+            new.userspace_addr = mem->userspace_addr;
         } else {
 

You can eliminate the braces in this condition.

Quoted 24 lines

for (i = 0; i < npages; ++i) {
new.phys_mem[i] = alloc_page(GFP_HIGHUSER

drivers/kvm/kvm_main.c +14 -1

@@ -1058,8 +1038,27 @@ struct page *gfn_to_page(struct kvm *kvm, 
 gfn_t gfn)
 
     gfn = unalias_gfn(kvm, gfn);
     slot = __gfn_to_memslot(kvm, gfn);
-    if (!slot)
+    if (!slot) {
+        get_page(bad_page);
         return bad_page;
+    }
+    if (slot->user_alloc) {
+        struct page *page[1];
+        int npages;
+
+        down_read(&current->mm->mmap_sem);
+        npages = get_user_pages(current, current->mm,
+                    slot->userspace_addr
+                    + (gfn - slot->base_gfn) * PAGE_SIZE, 1,
+                    1, 0, page, NULL);
+        up_read(&current->mm->mmap_sem);
+        if (npages != 1) {

drivers/kvm/kvm_main.c struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn)

Quoted 5 lines

drivers/kvm/kvm_main.c +4 -0

+            get_page(bad_page);
+            return bad_page;
+        }
+        return page[0];
 

Wouldn't it be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn]?

Regards,

Anthony Liguori

Quoted 236 lines

drivers/kvm/kvm_main.c +2 -0

+    }
+    get_page(slot->phys_mem[gfn - slot->base_gfn]);
     return slot->phys_mem[gfn - slot->base_gfn];

}
EXPORT_SYMBOL_GPL(gfn_to_page);

drivers/kvm/kvm_main.c +4 -1

@@ -1079,13 +1078,16 @@ int kvm_read_guest_page(struct kvm *kvm, 
 gfn_t gfn, void *data, int offset,
     struct page *page;
 
     page = gfn_to_page(kvm, gfn);
-    if (is_error_page(page))
+    if (is_error_page(page)) {
+        put_page(page);
         return -EFAULT;
+    }
     page_virt = kmap_atomic(page, KM_USER0);
 
     memcpy(data, page_virt + offset, len);
 
     kunmap_atomic(page_virt, KM_USER0);
+    put_page(page);
     return 0;
 }

EXPORT_SYMBOL_GPL(kvm_read_guest_page);

drivers/kvm/kvm_main.c +4 -1

@@ -1117,14 +1119,17 @@ int kvm_write_guest_page(struct kvm *kvm, 
 gfn_t gfn, const void *data,
     struct page *page;
 
     page = gfn_to_page(kvm, gfn);
-    if (is_error_page(page))
+    if (is_error_page(page)) {
+        put_page(page);
         return -EFAULT;
+    }
     page_virt = kmap_atomic(page, KM_USER0);
 
     memcpy(page_virt + offset, data, len);
 
     kunmap_atomic(page_virt, KM_USER0);
     mark_page_dirty(kvm, gfn);
+    put_page(page);
     return 0;
 }

EXPORT_SYMBOL_GPL(kvm_write_guest_page);

drivers/kvm/kvm_main.c +4 -1

@@ -1155,13 +1160,16 @@ int kvm_clear_guest_page(struct kvm *kvm, 
 gfn_t gfn, int offset, int len)
     struct page *page;
 
     page = gfn_to_page(kvm, gfn);
-    if (is_error_page(page))
+    if (is_error_page(page)) {
+        put_page(page);
         return -EFAULT;
+    }
     page_virt = kmap_atomic(page, KM_USER0);
 
     memset(page_virt + offset, 0, len);
 
     kunmap_atomic(page_virt, KM_USER0);
+    put_page(page);
     return 0;
 }

EXPORT_SYMBOL_GPL(kvm_clear_guest_page);

drivers/kvm/kvm_main.c +3 -4

@@ -2090,13 +2098,12 @@ int kvm_emulate_pio_string(struct kvm_vcpu 
 *vcpu, struct kvm_run *run, int in,
     for (i = 0; i < nr_pages; ++i) {
         mutex_lock(&vcpu->kvm->lock);
         page = gva_to_page(vcpu, address + i * PAGE_SIZE);
-        if (page)
-            get_page(page);
         vcpu->pio.guest_pages[i] = page;
         mutex_unlock(&vcpu->kvm->lock);
         if (!page) {
             inject_gp(vcpu);
             free_pio_guest_pages(vcpu);
             return 1;
         }
     }

@@ -3081,9 +3088,10 @@ static struct page *kvm_vm_nopage(struct 
 vm_area_struct *vma,
 
     pgoff = ((address - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
     page = gfn_to_page(kvm, pgoff);
-    if (is_error_page(page))
+    if (is_error_page(page)) {
+        put_page(page);
         return NOPAGE_SIGBUS;
-    get_page(page);
+    }
     if (type != NULL)
         *type = VM_FAULT_MINOR;

drivers/kvm/mmu.c +10 -1

index e6a9b4a..19af09a 100644
@@ -425,6 +425,7 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
     if (!is_rmap_pte(*spte))
         return;
     page = page_header(__pa(spte));
+    put_page(pfn_to_page((*spte & PT64_BASE_ADDR_MASK) >> PAGE_SHIFT));
     rmapp = gfn_to_rmap(kvm, page->gfns[spte - page->spt]);
     if (!*rmapp) {
         printk(KERN_ERR "rmap_remove: %p %llx 0->BUG\n", spte, *spte);

@@ -907,6 +908,9 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, 
 gva_t v, hpa_t p)
             table[index] = p | PT_PRESENT_MASK | PT_WRITABLE_MASK |
                                 PT_USER_MASK;
             rmap_add(vcpu, &table[index], v >> PAGE_SHIFT);
+            if (!is_rmap_pte(table[index]))
+                put_page(pfn_to_page((v & PT64_BASE_ADDR_MASK)
+                     >> PAGE_SHIFT));
             return 0;
         }

@@ -921,6 +925,8 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, 
 gva_t v, hpa_t p)
                              1, 3, &table[index]);
             if (!new_table) {
                 pgprintk("nonpaging_map: ENOMEM\n");
+                put_page(pfn_to_page((v & PT64_BASE_ADDR_MASK)
+                             >> PAGE_SHIFT));
                 return -ENOMEM;
             }

@@ -1035,8 +1041,11 @@ static int nonpaging_page_fault(struct 
 kvm_vcpu *vcpu, gva_t gva,
 
     paddr = gpa_to_hpa(vcpu->kvm, addr & PT64_BASE_ADDR_MASK);
 
-    if (is_error_hpa(paddr))
+    if (is_error_hpa(paddr)) {
+        put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+                     >> PAGE_SHIFT));
         return 1;
+    }
 
     return nonpaging_map(vcpu, addr & PAGE_MASK, paddr);

}

drivers/kvm/mmu.c +2 -0

@@ -1515,6 +1524,8 @@ static void audit_mappings_page(struct kvm_vcpu 
 *vcpu, u64 page_pte,
                  && !is_error_hpa(hpa))
                 printk(KERN_ERR "audit: (%s) notrap shadow,"
                        " valid guest gva %lx\n", audit_msg, va);
+            put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+                         >> PAGE_SHIFT));
 
         }

     }

drivers/kvm/paging_tmpl.h +1 -0

index 58fd35a..931a308 100644
@@ -175,6 +175,7 @@ static int FNAME(walk_addr)(struct guest_walker 
 *walker,
         walker->inherited_ar &= walker->table[index];
         table_gfn = (*ptep & PT_BASE_ADDR_MASK) >> PAGE_SHIFT;
         kunmap_atomic(walker->table, KM_USER0);
+        put_page(walker->page);
         walker->page = gfn_to_page(vcpu->kvm, table_gfn);
         walker->table = kmap_atomic(walker->page, KM_USER0);

         --walker->level;

drivers/kvm/paging_tmpl.h +3 -1

@@ -183,8 +184,10 @@ static int FNAME(walk_addr)(struct guest_walker 
 *walker,
              walker->level - 1, table_gfn);
     }
     walker->pte = *ptep;
-    if (walker->page)
+    if (walker->page) {
         walker->ptep = NULL;
+        put_page(walker->page);
+    }
     if (walker->table)
         kunmap_atomic(walker->table, KM_USER0);

     pgprintk("%s: pte %llx\n", __FUNCTION__, (u64)*ptep);

drivers/kvm/paging_tmpl.h +16 -2

@@ -206,6 +209,8 @@ err:
         walker->error_code |= PFERR_FETCH_MASK;
     if (walker->table)
         kunmap_atomic(walker->table, KM_USER0);
+    if (walker->page)
+        put_page(walker->page);
     return 0;
 }
 

@@ -249,6 +254,8 @@ static void FNAME(set_pte_common)(struct kvm_vcpu 
 *vcpu,
     if (is_error_hpa(paddr)) {
         set_shadow_pte(shadow_pte,
                    shadow_trap_nonpresent_pte | PT_SHADOW_IO_MARK);
+        put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+                     >> PAGE_SHIFT));
         return;
     }

@@ -286,9 +293,16 @@ unshadowed:
     pgprintk("%s: setting spte %llx\n", __FUNCTION__, spte);
     set_shadow_pte(shadow_pte, spte);
     page_header_update_slot(vcpu->kvm, shadow_pte, gaddr);
-    if (!was_rmapped)
+    if (!was_rmapped) {
         rmap_add(vcpu, shadow_pte, (gaddr & PT64_BASE_ADDR_MASK)
              >> PAGE_SHIFT);
+        if (!is_rmap_pte(*shadow_pte))
+            put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+                         >> PAGE_SHIFT));
+    }
+    else
+        put_page(pfn_to_page((paddr & PT64_BASE_ADDR_MASK)
+                     >> PAGE_SHIFT));
     if (!ptwrite || !*ptwrite)
         vcpu->last_pte_updated = shadow_pte;
 }

@@ -512,19 +526,22 @@ static void FNAME(prefetch_page)(struct 
 kvm_vcpu *vcpu,
 {
     int i;
     pt_element_t *gpt;
+    struct page *page;
 
     if (sp->role.metaphysical || PTTYPE == 32) {
         nonpaging_prefetch_page(vcpu, sp);
         return;
     }
 
-    gpt = kmap_atomic(gfn_to_page(vcpu->kvm, sp->gfn), KM_USER0);
+    page = gfn_to_page(vcpu->kvm, sp->gfn);
+    gpt = kmap_atomic(page, KM_USER0);
     for (i = 0; i < PT64_ENT_PER_PAGE; ++i)
         if (is_present_pte(gpt[i]))
             sp->spt[i] = shadow_trap_nonpresent_pte;
         else
             sp->spt[i] = shadow_notrap_nonpresent_pte;
     kunmap_atomic(gpt, KM_USER0);
+    put_page(page);
 }
 

Quoted 2 lines

#undef pt_element_t
-- 1.5.2.4

Quoted 23 lines · Anthony Liguori

Quoted 22 lines · Izik Eidus

drivers/kvm/kvm_main.c +14 -1

@@ -1058,8 +1038,27 @@ struct page *gfn_to_page(struct kvm *kvm, 
 gfn_t gfn)
 
     gfn = unalias_gfn(kvm, gfn);
     slot = __gfn_to_memslot(kvm, gfn);
-    if (!slot)
+    if (!slot) {
+        get_page(bad_page);
         return bad_page;
+    }
+    if (slot->user_alloc) {
+        struct page *page[1];
+        int npages;
+
+        down_read(&current->mm->mmap_sem);
+        npages = get_user_pages(current, current->mm,
+                    slot->userspace_addr
+                    + (gfn - slot->base_gfn) * PAGE_SIZE, 1,
+                    1, 0, page, NULL);
+        up_read(&current->mm->mmap_sem);
+        if (npages != 1) {

Quoted 8 lines

drivers/kvm/kvm_main.c struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn)

Quoted 5 lines

drivers/kvm/kvm_main.c +4 -0

+            get_page(bad_page);
+            return bad_page;
+        }
+        return page[0];
 

Wouldn't it be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn]?

Quoted 24 lines · Izik Eidus

Quoted 23 lines · Anthony Liguori

Quoted 22 lines · Izik Eidus

(quoted diff) +14 -1

@@ -1058,8 +1038,27 @@ struct page *gfn_to_page(struct kvm *kvm, 
 gfn_t gfn)
 
     gfn = unalias_gfn(kvm, gfn);
     slot = __gfn_to_memslot(kvm, gfn);
-    if (!slot)
+    if (!slot) {
+        get_page(bad_page);
         return bad_page;
+    }
+    if (slot->user_alloc) {
+        struct page *page[1];
+        int npages;
+
+        down_read(&current->mm->mmap_sem);
+        npages = get_user_pages(current, current->mm,
+                    slot->userspace_addr
+                    + (gfn - slot->base_gfn) * PAGE_SIZE, 1,
+                    1, 0, page, NULL);
+        up_read(&current->mm->mmap_sem);
+        if (npages != 1) {

Quoted 9 lines

Quoted 8 lines

Review struct page *gfn_to_page(struct kvm *kvm, · line 1061

Quoted 5 lines

(quoted diff) +4 -0

+            get_page(bad_page);
+            return bad_page;
+        }
+        return page[0];
 

Wouldn't it be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn]?

sorry, it seems like i missunderstand you in the answer i gave you. it wouldnt be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn], beacuse phys_mem wont have any memory allocate by this time.

with this patch, we are not holding anymore (when using userspace allocation) array of all the memory at phys_mem.
beacuse now that the pages are swappable, the physical address pointed by the virtual address all the time change (for example when swapping happn)
so no one promise us that slot->phys_mem[gfn - slot->base_gfn] will really point to page holding the gfn page.

so what we did, is throw away the phys_mem array (also nice beacuse it waste less ram), and at runtime we are getting the pages by using the virtual address
beacuse the reference of the page get increased, it promised us that untill we release it point to the gfn (release it by doing put_page)

hope i was more clear this time :)

Quoted 53 lines · Izik Eidus

Quoted 24 lines · Izik Eidus

Quoted 23 lines · Anthony Liguori

Quoted 22 lines · Izik Eidus

(quoted diff) +14 -1

@@ -1058,8 +1038,27 @@ struct page *gfn_to_page(struct kvm *kvm, 
 gfn_t gfn)
 
     gfn = unalias_gfn(kvm, gfn);
     slot = __gfn_to_memslot(kvm, gfn);
-    if (!slot)
+    if (!slot) {
+        get_page(bad_page);
         return bad_page;
+    }
+    if (slot->user_alloc) {
+        struct page *page[1];
+        int npages;
+
+        down_read(&current->mm->mmap_sem);
+        npages = get_user_pages(current, current->mm,
+                    slot->userspace_addr
+                    + (gfn - slot->base_gfn) * PAGE_SIZE, 1,
+                    1, 0, page, NULL);
+        up_read(&current->mm->mmap_sem);
+        if (npages != 1) {

Quoted 9 lines

Quoted 8 lines

Review struct page *gfn_to_page(struct kvm *kvm, · line 1061

Quoted 5 lines

(quoted diff) +4 -0

+            get_page(bad_page);
+            return bad_page;
+        }
+        return page[0];
 

Wouldn't it be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn]?

sorry, it seems like i missunderstand you in the answer i gave you. it wouldnt be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn], beacuse phys_mem wont have any memory allocate by this time.

with this patch, we are not holding anymore (when using userspace allocation) array of all the memory at phys_mem.
beacuse now that the pages are swappable, the physical address pointed by the virtual address all the time change (for example when swapping happn)
so no one promise us that slot->phys_mem[gfn - slot->base_gfn] will really point to page holding the gfn page.

so what we did, is throw away the phys_mem array (also nice beacuse it waste less ram), and at runtime we are getting the pages by using the virtual address
beacuse the reference of the page get increased, it promised us that untill we release it point to the gfn (release it by doing put_page)

Quoted 1 line

hope i was more clear this time :)

Yes, that makes sense!

I wonder if there's a more elegant way dealing with older userspaces. For instance, is there any reason why we can allocate a userspace memory region on behalf of userspace. That way swap would even work with older userspaces.

Regards,

Anthony Liguori

Quoted 56 lines · Anthony Liguori

Quoted 54 lines · Izik Eidus

Quoted 24 lines · Izik Eidus

Quoted 23 lines · Anthony Liguori

Quoted 22 lines · Izik Eidus

(quoted diff) +14 -1

@@ -1058,8 +1038,27 @@ struct page *gfn_to_page(struct kvm *kvm, 
 gfn_t gfn)
 
     gfn = unalias_gfn(kvm, gfn);
     slot = __gfn_to_memslot(kvm, gfn);
-    if (!slot)
+    if (!slot) {
+        get_page(bad_page);
         return bad_page;
+    }
+    if (slot->user_alloc) {
+        struct page *page[1];
+        int npages;
+
+        down_read(&current->mm->mmap_sem);
+        npages = get_user_pages(current, current->mm,
+                    slot->userspace_addr
+                    + (gfn - slot->base_gfn) * PAGE_SIZE, 1,
+                    1, 0, page, NULL);
+        up_read(&current->mm->mmap_sem);
+        if (npages != 1) {

Quoted 9 lines

Quoted 8 lines

Review struct page *gfn_to_page(struct kvm *kvm, · line 1061

Quoted 5 lines

(quoted diff) +4 -0

+            get_page(bad_page);
+            return bad_page;
+        }
+        return page[0];
 

Wouldn't it be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn]?

sorry, it seems like i missunderstand you in the answer i gave you. it wouldnt be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn], beacuse phys_mem wont have any memory allocate by this time.

with this patch, we are not holding anymore (when using userspace allocation) array of all the memory at phys_mem.
beacuse now that the pages are swappable, the physical address pointed by the virtual address all the time change (for example when swapping happn)
so no one promise us that slot->phys_mem[gfn - slot->base_gfn] will really point to page holding the gfn page.

so what we did, is throw away the phys_mem array (also nice beacuse it waste less ram), and at runtime we are getting the pages by using the virtual address
beacuse the reference of the page get increased, it promised us that untill we release it point to the gfn (release it by doing put_page)

hope i was more clear this time :)

Quoted 6 lines

Yes, that makes sense!

I wonder if there's a more elegant way dealing with older userspaces. For instance, is there any reason why we can allocate a userspace memory region on behalf of userspace. That way swap would even work with older userspaces.

if we can do that, yes swap will work on older userspace.

Quoted 5 lines

Regards,

Anthony Liguori

Quoted 60 lines · Izik Eidus

Quoted 59 lines · Anthony Liguori

Quoted 47 lines · Izik Eidus

Quoted 24 lines · Izik Eidus

Quoted 23 lines · Anthony Liguori

Quoted 22 lines · Izik Eidus

(quoted diff) +14 -1

@@ -1058,8 +1038,27 @@ struct page *gfn_to_page(struct kvm *kvm, 
 gfn_t gfn)
 
     gfn = unalias_gfn(kvm, gfn);
     slot = __gfn_to_memslot(kvm, gfn);
-    if (!slot)
+    if (!slot) {
+        get_page(bad_page);
         return bad_page;
+    }
+    if (slot->user_alloc) {
+        struct page *page[1];
+        int npages;
+
+        down_read(&current->mm->mmap_sem);
+        npages = get_user_pages(current, current->mm,
+                    slot->userspace_addr
+                    + (gfn - slot->base_gfn) * PAGE_SIZE, 1,
+                    1, 0, page, NULL);
+        up_read(&current->mm->mmap_sem);
+        if (npages != 1) {

Quoted 9 lines

Quoted 8 lines

Review struct page *gfn_to_page(struct kvm *kvm, · line 1061

Quoted 5 lines

(quoted diff) +4 -0

+            get_page(bad_page);
+            return bad_page;
+        }
+        return page[0];
 

Wouldn't it be necessary to assign page[0] to slot->phys_mem[gfn
- slot->base_gfn]?

sorry, it seems like i missunderstand you in the answer i gave you. it wouldnt be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn], beacuse phys_mem wont have any memory allocate by this time.

with this patch, we are not holding anymore (when using userspace allocation) array of all the memory at phys_mem.
beacuse now that the pages are swappable, the physical address pointed by the virtual address all the time change (for example when swapping happn)
so no one promise us that slot->phys_mem[gfn - slot->base_gfn] will really point to page holding the gfn page.

Quoted 8 lines

so what we did, is throw away the phys_mem array (also nice beacuse it waste less ram), and at runtime we are getting the pages by using the virtual address
beacuse the reference of the page get increased, it promised us that untill we release it point to the gfn (release it by doing put_page)

hope i was more clear this time :)

Yes, that makes sense!

Quoted 5 lines

Quoted 4 lines

I wonder if there's a more elegant way dealing with older userspaces. For instance, is there any reason why we can allocate a userspace memory region on behalf of userspace. That way swap would even work with older userspaces.

if we can do that, yes swap will work on older userspace.

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

Quoted 3 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

yea, i think you right, this is excellent idea!, when we will merge the swapping to kvm, we will add swapping support to older userspace.

Quoted 8 lines · Izik Eidus

Quoted 5 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

yea, i think you right, this is excellent idea!, when we will merge the swapping to kvm, we will add swapping support to older userspace.

Here's a patch against your series. The memset in kvmctl ends up making the guest use all physical memory to start off with but I did confirm that once the system is under memory pressure, the guest's memory becomes swappable. Of course, it's quite painful :-)

A nice thing though is that a lot of the code becomes a bit cleaner and we can eliminate the phys_mem array entirely.

Regards,

Anthony Liguori

Quoted 5 lines · Izik Eidus

Quoted 3 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

yea, i think you right, this is excellent idea!, when we will merge the swapping to kvm, we will add swapping support to older userspace.

Here's a patch against your series. The memset in kvmctl ends up making the guest use all physical memory to start off with but I did confirm that once the system is under memory pressure, the guest's memory becomes swappable. Of course, it's quite painful :-)

A nice thing though is that a lot of the code becomes a bit cleaner and we can eliminate the phys_mem array entirely.

     /* Allocate if a slot is being created */

Regards,

Anthony Liguori

Quoted 17 lines · Anthony Liguori

Quoted 8 lines · Izik Eidus

Quoted 5 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

yea, i think you right, this is excellent idea!, when we will merge the swapping to kvm, we will add swapping support to older userspace.

Here's a patch against your series. The memset in kvmctl ends up making the guest use all physical memory to start off with but I did confirm that once the system is under memory pressure, the guest's memory becomes swappable. Of course, it's quite painful :-)

A nice thing though is that a lot of the code becomes a bit cleaner and we can eliminate the phys_mem array entirely.

drivers/kvm/kvm_main.c static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,

Quoted 8 lines

     /* Allocate if a slot is being created */

drivers/kvm/kvm_main.c +1 -6

-    if (npages && !new.phys_mem) {
-        new.phys_mem = vmalloc(npages * sizeof(struct page *));
-
-        if (!new.phys_mem)
-            goto out_unlock;
-
+    if (npages) {

This is wrong apparently. We needed to have new.phys_mem to indicate whether the slot has been created yet or not. The new attached patch uses new.rmap instead of new.phys_mem to accomplish the same goal.

I no longer see the BUG() that I was seeing.

Quoted 5 lines

Regards,

Anthony Liguori

Quoted 19 lines · Anthony Liguori

Quoted 17 lines · Anthony Liguori

Quoted 8 lines · Izik Eidus

Quoted 5 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

yea, i think you right, this is excellent idea!, when we will merge the swapping to kvm, we will add swapping support to older userspace.

Here's a patch against your series. The memset in kvmctl ends up making the guest use all physical memory to start off with but I did confirm that once the system is under memory pressure, the guest's memory becomes swappable. Of course, it's quite painful :-)

A nice thing though is that a lot of the code becomes a bit cleaner and we can eliminate the phys_mem array entirely.

Quoted 13 lines

drivers/kvm/kvm_main.c static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,

Quoted 8 lines

     /* Allocate if a slot is being created */

drivers/kvm/kvm_main.c +1 -6

-    if (npages && !new.phys_mem) {
-        new.phys_mem = vmalloc(npages * sizeof(struct page *));
-
-        if (!new.phys_mem)
-            goto out_unlock;
-
+    if (npages) {

This is wrong apparently. We needed to have new.phys_mem to indicate whether the slot has been created yet or not. The new attached patch uses new.rmap instead of new.phys_mem to accomplish the same goal.

this patch look good, and indeed make all the things simple. but why you didnt use new.phys_mem ?

Quoted 8 lines

I no longer see the BUG() that I was seeing.

Quoted 5 lines

Regards,

Anthony Liguori

Quoted 68 lines · Izik Eidus

Quoted 66 lines · Anthony Liguori

Quoted 49 lines · Izik Eidus

Quoted 26 lines · Izik Eidus

Quoted 24 lines · Anthony Liguori

Quoted 22 lines · Izik Eidus

(quoted diff) +15 -1

@@ -1058,8 +1038,27 @@ struct page *gfn_to_page(struct kvm *kvm, 
 gfn_t gfn)
 
     gfn = unalias_gfn(kvm, gfn);
     slot = __gfn_to_memslot(kvm, gfn);
-    if (!slot)
+    if (!slot) {
+        get_page(bad_page);
         return bad_page;
+    }
+    if (slot->user_alloc) {
+        struct page *page[1];
+        int npages;
+
+        down_read(&current->mm->mmap_sem);
+        npages = get_user_pages(current, current->mm,
+                    slot->userspace_addr
+                    + (gfn - slot->base_gfn) * PAGE_SIZE, 1,
+                    1, 0, page, NULL);
+        up_read(&current->mm->mmap_sem);
+        if (npages != 1) {
+            get_page(bad_page);

Quoted 8 lines

Quoted 8 lines

Review struct page *gfn_to_page(struct kvm *kvm, · line 1062

Quoted 5 lines

(quoted diff) +3 -0

+            return bad_page;
+        }
+        return page[0];
 
 

Wouldn't it be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn]?

sorry, it seems like i missunderstand you in the answer i gave you. it wouldnt be necessary to assign page[0] to slot->phys_mem[gfn - slot->base_gfn], beacuse phys_mem wont have any memory allocate by this time.

with this patch, we are not holding anymore (when using userspace allocation) array of all the memory at phys_mem.
beacuse now that the pages are swappable, the physical address pointed by the virtual address all the time change (for example when swapping happn)
so no one promise us that slot->phys_mem[gfn - slot->base_gfn] will really point to page holding the gfn page.

Quoted 8 lines

so what we did, is throw away the phys_mem array (also nice beacuse it waste less ram), and at runtime we are getting the pages by using the virtual address
beacuse the reference of the page get increased, it promised us that untill we release it point to the gfn (release it by doing put_page)

hope i was more clear this time :)

Yes, that makes sense!

I wonder if there's a more elegant way dealing with older userspaces. For instance, is there any reason why we can allocate a userspace memory region on behalf of userspace. That way swap would even work with older userspaces.

Quoted 2 lines

if we can do that, yes swap will work on older userspace.

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

I wonder if there's a more elegant way dealing with older userspaces. For instance, is there any reason why we can allocate a userspace memory region on behalf of userspace. That way swap would even work with older userspaces.

Quoted 8 lines

Quoted 3 lines

if we can do that, yes swap will work on older userspace.

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

This will halve the maximum size of virtual machines on i386 since userspace will also mmap() the memory, and the virtual address space is restricted to 3GB.

Quoted 5 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

This will halve the maximum size of virtual machines on i386 since userspace will also mmap() the memory, and the virtual address space is restricted to 3GB.

Quoted 8 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

Quoted 4 lines

This will halve the maximum size of virtual machines on i386 since userspace will also mmap() the memory, and the virtual address space is restricted to 3GB.

I wonder if there is a way to force the mmap() to return the userspace address we previously allocated.

unsigned long
get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
unsigned long pgoff, unsigned long flags)
{
unsigned long (*get_area)(struct file *, unsigned long,
unsigned long, unsigned long,
unsigned long);

        get_area = current->mm->get_unmapped_area;
        if (file && file->f_op && file->f_op->get_unmapped_area)
                get_area = file->f_op->get_unmapped_area;
        addr = get_area(file, addr, len, pgoff, flags);

Quoted 14 lines · Anthony Liguori

Quoted 12 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

This will halve the maximum size of virtual machines on i386 since userspace will also mmap() the memory, and the virtual address space is restricted to 3GB.

Quoted 3 lines

I wonder if there is a way to force the mmap() to return the userspace address we previously allocated.

Yes:

Quoted 14 lines

unsigned long
get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
unsigned long pgoff, unsigned long flags)
{
unsigned long (*get_area)(struct file *, unsigned long,
unsigned long, unsigned long,
unsigned long);

        get_area = current->mm->get_unmapped_area;
        if (file && file->f_op && file->f_op->get_unmapped_area)
                get_area = file->f_op->get_unmapped_area;
        addr = get_area(file, addr, len, pgoff, flags);

Quoted 16 lines · Avi Kivity

Quoted 15 lines · Anthony Liguori

Quoted 13 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

I think it's just a matter of calling do_mmap() with the appropriate parameters. It looks likes there's some drivers call do_mmap() directly.

This will halve the maximum size of virtual machines on i386 since userspace will also mmap() the memory, and the virtual address space is
restricted to 3GB.

Quoted 20 lines

Quoted 3 lines

I wonder if there is a way to force the mmap() to return the userspace address we previously allocated.

Yes:

Quoted 14 lines

unsigned long
get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
unsigned long pgoff, unsigned long flags)
{
unsigned long (*get_area)(struct file *, unsigned long,
unsigned long, unsigned long,
unsigned long);

        get_area = current->mm->get_unmapped_area;
        if (file && file->f_op && file->f_op->get_unmapped_area)
                get_area = file->f_op->get_unmapped_area;
        addr = get_area(file, addr, len, pgoff, flags);

Hrm, so you can return the area that should be used but you still install vma_ops? Wouldn't that result in an infinite loop when userspace attempted to map the area since we'll try to satisfy the nopage handler by doing a get_user_pages?