unit tests and get_user_pages_ptes_fast()

During the last kvm forum, I described a unit test framework that can
help test the kvm APIs. Briefly, it starts a process in host userspace, which sets up a memory slot mapping gpa 0:3G to hva 0:3G. It then sets up guest registers for unpaged protected mode (or paged protected mode with 1:1 mapping). The effect is that we have a 1:1 gva->hva translation, and can use KVM_RUN to run host code in guest mode.

There is a snag however. kvm calls get_user_pages_fast(.write = 1), and the host process maps its code pages read-only.

The way I'd like to work around this is to map read-only accesses to read-only pages as read-only. This also prevents ksm cow pages from being broken by read accesses. It can also be used to get the page size for transparent huge pages (and later hugetlbfs too).

So, for a read fault we do:

   pte_t pte;
   get_user_pages_ptes_fast(..., page, &pte, 1, .write = 0)
   ...
   if (pte_transhuge(pte)) // or however it's called
       ...
   ...
   if (pte_write(pte))
         map writeable
   else
         map readonly

Any snags? or alternatives?

Hi Avi,

Quoted 28 lines · Avi Kivity

During the last kvm forum, I described a unit test framework that can
help test the kvm APIs. Briefly, it starts a process in host userspace, which sets up a memory slot mapping gpa 0:3G to hva 0:3G. It then sets up guest registers for unpaged protected mode (or paged protected mode with 1:1 mapping). The effect is that we have a 1:1 gva->hva translation, and can use KVM_RUN to run host code in guest mode.

There is a snag however. kvm calls get_user_pages_fast(.write = 1), and the host process maps its code pages read-only.

The way I'd like to work around this is to map read-only accesses to read-only pages as read-only. This also prevents ksm cow pages from being broken by read accesses. It can also be used to get the page size for transparent huge pages (and later hugetlbfs too).

So, for a read fault we do:

   pte_t pte;
   get_user_pages_ptes_fast(..., page, &pte, 1, .write = 0)
   ...
   if (pte_transhuge(pte)) // or however it's called
       ...
   ...
   if (pte_write(pte))
         map writeable
   else
         map readonly

Quoted 1 line

Any snags? or alternatives?

I think we tried to do this write=0 before, it provides benefits for swapins from swapcache too (after the page is unmapped but the swapcache is present and uptodate and clean). If I recall correctly the only obstacle was the fact the out of sync code wants to map the spte writable if it can, but if we use write=0 in gup, it won't know if it can. If it'd wait a real write access from guest, we'd risk creating a spte wrprotect fault for nothing (it should only write a write access from the guest if the linux VM solves the page fault with a readonly pte, even the minor-read-fault from exclusive swapcache will not set the dirty bit but it will still set the pte_write on the pte so the out of sync code should take advantage of that information in the host pte). In the past you suggested some TRY_WRITE operation instead of only read/write. We just need to pass the write bit of the pte somehow to the gup caller.

Full agreement that once we use write=0 for read faults your problem with the debugging userland running in guest mode will have better chance to work too :).

Andrea

Yep, the drawback is the unnecessary write fault. What i have here is:

virt/kvm/kvm_main.c +9 -1

@@ -827,7 +827,7 @@ unsigned long gfn_to_hva(struct kvm *kvm
  }
  EXPORT_SYMBOL_GPL(gfn_to_hva);
 
-pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
+pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn, int *writable)
  {
         struct page *page[1];
         unsigned long addr;

@@ -842,8 +842,16 @@ pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t
                 return page_to_pfn(bad_page);
         }
 
+       *writable = 1;
         npages = get_user_pages_fast(addr, 1, 1, page);
 
+       /* attempt to map read-only */
+       if (unlikely(npages != 1)) {
+               npages = get_user_pages_fast(addr, 1, 0, page);
+               if (npages == 1)
+                       *writable = 0;
+       }
+
         if (unlikely(npages != 1)) {
                 struct vm_area_struct *vma;
 

Can rebase and resend, if you'd like.

Quoted 30 lines · Marcelo Tosatti

Yep, the drawback is the unnecessary write fault. What i have here is:

virt/kvm/kvm_main.c +9 -1

@@ -827,7 +827,7 @@ unsigned long gfn_to_hva(struct kvm *kvm
 }
 EXPORT_SYMBOL_GPL(gfn_to_hva);
 
-pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
+pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn, int *writable)
 {
        struct page *page[1];
        unsigned long addr;

@@ -842,8 +842,16 @@ pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t
                return page_to_pfn(bad_page);
        }
 
+       *writable = 1;
        npages = get_user_pages_fast(addr, 1, 1, page);
 
+       /* attempt to map read-only */
+       if (unlikely(npages != 1)) {
+               npages = get_user_pages_fast(addr, 1, 0, page);
+               if (npages == 1)
+                       *writable = 0;
+       }
+
        if (unlikely(npages != 1)) {
                struct vm_area_struct *vma;
 

Quoted 2 lines

Can rebase and resend, if you'd like.

That will work for me but not for ksm. I guess it's good to get things going, so please to post it.

It'll not be so advantageous for ksm because there should be read-faults very rarely on that case.

Will post.

Quoted 2 lines · Marcelo Tosatti

It'll not be so advantageous for ksm because there should be read-faults very rarely on that case.

It'll also make all clean swapcache dirty for no good.

Quoted 1 line

Will post.

If we've to walk pagetables twice, why don't you do this:

writable=1
get_user_pages_fast(write=write_fault)
if (!write_fault)
writable = __get_user_pages_fast(write=1)

That will solve the debugging knob and it'll solve ksm and it'll be optimal for read swapins on exclusive clean swapcache too.

Quoted 10 lines · Andrea Arcangeli

Quoted 2 lines · Marcelo Tosatti

It'll not be so advantageous for ksm because there should be read-faults very rarely on that case.

It'll also make all clean swapcache dirty for no good.

Quoted 1 line

Will post.

If we've to walk pagetables twice, why don't you do this:

Quoted 7 lines

writable=1
get_user_pages_fast(write=write_fault)
if (!write_fault)
writable = __get_user_pages_fast(write=1)

That will solve the debugging knob and it'll solve ksm and it'll be optimal for read swapins on exclusive clean swapcache too.

But it means an extra vmexit in the following case:

- read fault
- page is present and writeable in the Linux page table

which is very common. For this you need get_user_pages_ptes_fast().

Quoted 19 lines · Avi Kivity

Quoted 10 lines · Andrea Arcangeli

Quoted 2 lines · Marcelo Tosatti

It'll not be so advantageous for ksm because there should be read-faults very rarely on that case.

It'll also make all clean swapcache dirty for no good.

Quoted 1 line

Will post.

If we've to walk pagetables twice, why don't you do this:

Quoted 7 lines

writable=1
get_user_pages_fast(write=write_fault)
if (!write_fault)
writable = __get_user_pages_fast(write=1)

That will solve the debugging knob and it'll solve ksm and it'll be optimal for read swapins on exclusive clean swapcache too.

Quoted 6 lines

But it means an extra vmexit in the following case:

- read fault
- page is present and writeable in the Linux page table

which is very common. For this you need get_user_pages_ptes_fast().

With a read fault, the VM already sets the pte as writable if the VM permissions allows that and the page isn't shared (i.e. if it's an exclusive swap page). We've just to check if it did that or not. So when it's a read fault we've to run __get_user_pages_fast(write=1) before we can assume the page is mapped writable in the pte.

So I don't see the problem... in terms of page faults is optimal. Only downside is having to walk the pagetables twice, the second time to verify if the first gup_fast has marked the host pte writable or not.