From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 5/6] xen-gntdev: Support mapping in HVM domains Date: Tue, 18 Jan 2011 11:09:54 -0500 Message-ID: <4D35BB52.5080907@tycho.nsa.gov> References: <1295019976-605-1-git-send-email-dgdegra@tycho.nsa.gov> <1295019976-605-6-git-send-email-dgdegra@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Stefano Stabellini Cc: Ian Campbell , "jeremy@goop.org" , "xen-devel@lists.xensource.com" , "konrad.wilk@oracle.com" List-Id: xen-devel@lists.xenproject.org On 01/17/2011 09:28 AM, Stefano Stabellini wrote: > On Fri, 14 Jan 2011, Daniel De Graaf wrote: >> HVM does not allow direct PTE modification, so instead we request >> that Xen change its internal p2m mappings on the allocated pages and >> map the memory into userspace normally. >> >> Signed-off-by: Daniel De Graaf >> --- >> drivers/xen/gntdev.c | 117 +++++++++++++++++++++++++++++++-------------- >> drivers/xen/grant-table.c | 6 ++ >> 2 files changed, 87 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c >> index 8a12857..1931657 100644 >> --- a/drivers/xen/gntdev.c >> +++ b/drivers/xen/gntdev.c >> @@ -32,6 +32,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -52,6 +53,8 @@ MODULE_PARM_DESC(limit, "Maximum number of grants that may be mapped by " >> >> static atomic_t pages_mapped = ATOMIC_INIT(0); >> >> +static int use_ptemod = 0; >> + >> struct gntdev_priv { >> struct list_head maps; >> /* lock protects maps from concurrent changes */ >> @@ -184,6 +187,9 @@ static void gntdev_put_map(struct grant_map *map) >> >> atomic_sub(map->count, &pages_mapped); >> >> + if (!use_ptemod) >> + unmap_grant_pages(map, 0, map->count); >> + >> for (i = 0; i < map->count; i++) { >> if (map->pages[i]) >> __free_page(map->pages[i]); >> @@ -212,9 +218,12 @@ static int find_grant_ptes(pte_t *pte, pgtable_t token, >> static int map_grant_pages(struct grant_map *map) >> { >> int i, flags, err = 0; >> + phys_addr_t addr; >> struct gnttab_map_grant_ref* map_ops = NULL; >> >> - flags = GNTMAP_host_map | GNTMAP_application_map | GNTMAP_contains_pte; >> + flags = GNTMAP_host_map; >> + if (use_ptemod) >> + flags |= GNTMAP_application_map | GNTMAP_contains_pte; >> if (map->is_ro) >> flags |= GNTMAP_readonly; >> >> @@ -224,7 +233,11 @@ static int map_grant_pages(struct grant_map *map) >> goto out; >> >> for(i=0; i < map->count; i++) { >> - gnttab_set_map_op(&map_ops[i], map->pginfo[i].pte_maddr, flags, >> + if (use_ptemod) >> + addr = map->pginfo[i].pte_maddr; >> + else >> + addr = (phys_addr_t)pfn_to_kaddr(page_to_pfn(map->pages[i])); >> + gnttab_set_map_op(&map_ops[i], addr, flags, >> map->pginfo[i].target.ref, >> map->pginfo[i].target.domid); >> } > > If I am not mistaken you are asking Xen to modify the virtual kernel > mappings of map->pages, but it is not enough to have correctly working > userspace mappings of map->pages: you need gnttab_map_refs to correctly > fix the p2m and add an entry to m2p_override too. > However in the last gntdev patch series I sent, requests with > !GNTMAP_contains_pte are not added to m2p_override and the p2m entries > of map->pages are not updated either. > > Therefore you probably need this (untested) patch to make it work: > It looks like this patch only applies if using gnttab_map_refs without GNTMAP_contains_pte on PV guests; my patch only uses !GNTMAP_contains_pte on HVM, so it doesn't need this change. I think this patch would be needed if we wanted to remove GNTMAP_contains_pte from the PV case, which might make the code cleaner (it would remove the dependency on the MM notifier, and allow multiple mmap() of the device in PV). I didn't want to change the working PV case when adding HVM support, so I left that code mostly alone. Just from quickly looking at the patch, since it uses current->active_mm, it seems that it might still have issues with multiple processes mapping the area. I would assume the update would be purely to the p2m table, not involving any page tables (since the pages should not yet be mapped). > --- > > Add support for !GNTMAP_contains_pte mappings to gnttab_map_refs > > Signed-off-by: Stefano Stabellini > > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c > index 9ef54eb..7d6d2ba 100644 > --- a/drivers/xen/grant-table.c > +++ b/drivers/xen/grant-table.c > @@ -459,12 +459,38 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops, > return ret; > > for (i = 0; i < count; i++) { > - /* m2p override only supported for GNTMAP_contains_pte mappings */ > - if (!(map_ops[i].flags & GNTMAP_contains_pte)) > - continue; > - pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) + > - (map_ops[i].host_addr & ~PAGE_MASK)); > - mfn = pte_mfn(*pte); > + if ((map_ops[i].flags & GNTMAP_contains_pte)) { > + pte = (pte_t *) (mfn_to_virt(PFN_DOWN(map_ops[i].host_addr)) + > + (map_ops[i].host_addr & ~PAGE_MASK)); > + mfn = pte_mfn(*pte); > + } else { > + pgd_t *pgd; > + pud_t *pud; > + pmd_t *pmd; > + unsigned long addr = map_ops[i].host_addr; > + pgd = pgd_offset(current->active_mm, addr); > + if (pgd_none(*pgd)) { > + printk(KERN_WARNING "invalid pgd found, skip address=%lx\n", addr); > + continue; > + } > + pud = pud_offset(pgd, addr); > + if (pud_none(*pud)) { > + printk(KERN_WARNING "invalid pud found, skip address=%lx\n", addr); > + continue; > + } > + pmd = pmd_offset(pud, addr); > + if (pmd_none(*pmd)) { > + printk(KERN_WARNING "invalid pmd found, skip address=%lx\n", addr); > + continue; > + } > + pte = pte_offset_map(pmd, addr); > + if (pte_none(*pte)) { > + printk(KERN_WARNING "invalid pte found, skip address=%lx\n", addr); > + continue; > + } > + mfn = pte_mfn(*pte); > + pte_unmap(pte); > + } > ret = m2p_add_override(mfn, pages[i]); > if (ret) > return ret;