Re: unit tests and get_user_pages_ptes_fast()

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Oct 04, 2010 at 03:40:52PM +0200, Andrea Arcangeli wrote:
> Hi Avi,
> 
> On Mon, Oct 04, 2010 at 11:35:28AM +0200, Avi Kivity wrote:
> >   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?
> 
> 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.

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

--- kvm.orig/virt/kvm/kvm_main.c
+++ kvm/virt/kvm/kvm_main.c
@@ -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.

> 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