Re: [Xen-devel] Re: [PATCH v4 2/2] xen: modify kernel mappings corresponding to granted pages

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Sep 28, 2011 at 09:12:29AM -0400, Konrad Rzeszutek Wilk wrote:
> > > Anyhow, If you want to modify your patchset to check PagePrivate bit
> > > and set the SetPagePrivate/set_page_private - go ahead.
> > 
> > I'll do that.
> 
> Preempted you a bit :-)
> http://lists.xensource.com/archives/html/xen-devel/2011-09/msg01364.html
> 
> > > > > > +                     gnttab_set_map_op(&map->kmap_ops[i], 
> > > > > > pte_maddr,
> > > > > > +                             map->flags |
> > > > > > +                             GNTMAP_host_map |
> > > > > > +                             GNTMAP_contains_pte,
> > > > > > +                             map->grants[i].ref,
> > > > > > +                             map->grants[i].domid);
> > > > > > +             }
> > > > >
> > > > > So, on startup.. (before this function is called) the
> > > > > find_grant_ptes is called which pretty much does the exact thing for
> > > > > each virtual address.  Except its flags are GNTMAP_application_map
> > > > > instead of GNTMAP_host_map.
> > > > >
> > > > > It even uses the same type structure.. It fills out map_ops[i] one.
> > > > >
> > > > > Can we use that instead of adding a new structure?
> > > >
> > > > Do you mean moving this code inside find_grant_ptes?
> > > > I don't think that can be done because find_grant_ptes is called on a
> > > > range of virtual addresses while this is called on an array of struct
> > > > pages. It is true that the current implementation of
> > > 
> > > But aren't that 'range of virtual address' of struct pages? You
> > > are using 'alloc_xenballooned_pages' to get those pages and that is
> > > what the 'range of virtual adresses' is walking through.
> > 
> > it is not the same range of virtual addresses
> 
> OK, but the pte_maddr is the same, isn't it?

No it is not. It is the machine address of the PTE entry that
points to the 'struct page' (kernel linear address), while the
find_grant_pte's gets the machine address of the PTE entry that
points to the user pages. Completlely different machine addresses
of the PTEs.

Can you add a comment to the patch saying something along what
I just said? Just in case somebody is as thick as I am when looking
at this code/patch.

Otherwise the patch looks fine - just fix up the SetPagePrivate,
the PAGE_GRANT bit, and add extra comments and I am ready to stick it
on my queue.

Thanks!
> 
> > 
> > > > alloc_xenballooned_pages is going to return a consecutive set of pages
> > > > but it might not always be the case.
> > > 
> > > I am sensing some grand plans in work here? I thought we are going to
> > > try to simply our lives and see about making alloc_xenballooned_pages
> > > returned sane pages that are !PageHighMem (or if they are PageHighMem they
> > > are already pinned, and set in the &init_mm)?
> > > 
> > > I am just thinking in terms of lookup_address and 
> > > arbitrary_virt_to_machine
> > > calls being done _twice_. And it seems like we have the find_grant_ptes
> > > which does the bulk of this already - so why not piggyback on it?
> > 
> > It has to be done twice: once for the user ptes and once for the kernel
> > mappings of map->pages.
> > 
> > 
> > > Besides that, the patch set looks fine. .. How do I reproduce the failures
> > > you had encountered with the AIO?
> > > 
> > 
> > Just setup and use upstream qemu and configure your VM to use a disk on
> > a file (file:).
> 
> OK.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel