Espen Skoglund
2008-May-23 12:30 UTC
[Xen-devel] [PATCH] Remove physmap page upon granttab xfer
Mapping should be removed from current domain''s p2m table when doing a
grant table transfer.
Signed-off-by: Espen Skoglund <espen.skoglund@netronome.com>
===============================================================diff -r
2e6a51378451 xen/common/grant_table.c
--- a/xen/common/grant_table.c Thu May 22 15:11:06 2008 +0100
+++ b/xen/common/grant_table.c Fri May 23 13:23:56 2008 +0100
@@ -1073,6 +1073,8 @@
gop.status = GNTST_bad_page;
goto copyback;
}
+
+ guest_physmap_remove_page(d, gop.mfn, mfn, 0);
/* Find the target domain. */
if ( unlikely((e = rcu_lock_domain_by_id(gop.domid)) == NULL) )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Isaku Yamahata
2008-May-28 10:28 UTC
Re: [Xen-devel] [PATCH] Remove physmap page upon granttab xfer
Hi Espen. I think the issues exist in all functions in linux kernel which uses MMU_MACHPHYS_UPDATE. gnttab_copy_grant_page(), netbk_gop_frag(), xennet_get_responses() and netif_release_rx_bufs_flip() The p2m/m2p table manipulation is done by MMU_MACHPHYS_UPDATE on x86. (More exactly the p2m operation is done in linux kernel on x86.) So at the line after steal_page(), the p2m/m2p table is supposed to be already updated. It implys that iommu is also supposed to be updated already. Although I know that grant table page transfer is obsoleted, what is the expected exact semantics of GNTTABOP_transfer with the p2m/m2p table? I suppose the right fix would be - revert this patch and - put the iommu opration of pv domain into MMU_MACHPHYS_UPDATE. Some comments on xen/ia64 side: This patch breaks xen/ia64 grant table page transfer. When I made netback/netfront work on ia64, I thought the tranfer operation shouldn''t change the m2p/p2m table. On the other hand, I didn''t want to add new hypecall to manipulate the m2p/p2m table for auto translated mode that corresponds to MMU_MACHPHYS_UPDATE and set_phys_to_machine(). So I worked around it such that xen/ia64 steal_page() does the p2m/m2p manipulation and fill new page after page owner change. (There is a risk to fail to allocate a new page during page tranfer, though.) thanks On Fri, May 23, 2008 at 01:30:16PM +0100, Espen Skoglund wrote:> Mapping should be removed from current domain''s p2m table when doing a > grant table transfer. > > Signed-off-by: Espen Skoglund <espen.skoglund@netronome.com> > > > ===============================================================> diff -r 2e6a51378451 xen/common/grant_table.c > --- a/xen/common/grant_table.c Thu May 22 15:11:06 2008 +0100 > +++ b/xen/common/grant_table.c Fri May 23 13:23:56 2008 +0100 > @@ -1073,6 +1073,8 @@ > gop.status = GNTST_bad_page; > goto copyback; > } > + > + guest_physmap_remove_page(d, gop.mfn, mfn, 0); > > /* Find the target domain. */ > if ( unlikely((e = rcu_lock_domain_by_id(gop.domid)) == NULL) ) > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >-- yamahata _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-May-28 10:39 UTC
Re: [Xen-devel] [PATCH] Remove physmap page upon granttab xfer
On 28/5/08 11:28, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote:> Although I know that grant table page transfer is obsoleted, > what is the expected exact semantics of GNTTABOP_transfer with > the p2m/m2p table? > > I suppose the right fix would be > - revert this patch and > - put the iommu opration of pv domain into MMU_MACHPHYS_UPDATE.After the grant transfer operation completes the page no longer belongs to the backend domain. Thus, for absolute safety and isolation, the page must be removed from the backend domain''s IOMMU mappings before completion of the grant transfer operation. Actually we should be flushing host CPU TLBs too. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Isaku Yamahata
2008-May-29 10:02 UTC
Re: [Xen-devel] [PATCH] Remove physmap page upon granttab xfer
On Wed, May 28, 2008 at 11:39:16AM +0100, Keir Fraser wrote:> On 28/5/08 11:28, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote: > > > Although I know that grant table page transfer is obsoleted, > > what is the expected exact semantics of GNTTABOP_transfer with > > the p2m/m2p table? > > > > I suppose the right fix would be > > - revert this patch and > > - put the iommu opration of pv domain into MMU_MACHPHYS_UPDATE. > > After the grant transfer operation completes the page no longer belongs to > the backend domain. Thus, for absolute safety and isolation, the page must > be removed from the backend domain''s IOMMU mappings before completion of the > grant transfer operation. Actually we should be flushing host CPU TLBs too.Thank you for explanation. The patch breaks ia64 granttab xfer breakage so that it have to be fixed. However the operation is now obsoleted one, so I don''t want to make big effort to fix. What do you think about the following patch? Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- ./grant_table.c 2008-05-28 18:47:40.000000000 +0900 +++ ./grant_table.c-if 2008-05-29 18:27:05.000000000 +0900 @@ -1112,7 +1112,8 @@ gnttab_transfer( goto copyback; } - guest_physmap_remove_page(d, gop.mfn, mfn, 0); + if (!paging_mode_translate(d)) + guest_physmap_remove_page(d, gop.mfn, mfn, 0); /* Find the target domain. */ if ( unlikely((e = rcu_lock_domain_by_id(gop.domid)) == NULL) ) or #ifndef __ia64__ -- yamahata _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-May-29 10:10 UTC
Re: [Xen-devel] [PATCH] Remove physmap page upon granttab xfer
On 29/5/08 11:02, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote:>> After the grant transfer operation completes the page no longer belongs to >> the backend domain. Thus, for absolute safety and isolation, the page must >> be removed from the backend domain''s IOMMU mappings before completion of the >> grant transfer operation. Actually we should be flushing host CPU TLBs too. > > Thank you for explanation. > The patch breaks ia64 granttab xfer breakage so that it have to be fixed. > However the operation is now obsoleted one, so I don''t want to make > big effort to fix. > What do you think about the following patch?How does grant transfer work on ia64? Looking at netback I don''t even see how the ''hole'' left by transfer of the old page gets filled by mapping in a new page. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-May-29 10:15 UTC
Re: [Xen-devel] [PATCH] Remove physmap page upon granttab xfer
On 29/5/08 11:10, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:>> Thank you for explanation. >> The patch breaks ia64 granttab xfer breakage so that it have to be fixed. >> However the operation is now obsoleted one, so I don''t want to make >> big effort to fix. >> What do you think about the following patch? > > How does grant transfer work on ia64? Looking at netback I don''t even see > how the ''hole'' left by transfer of the old page gets filled by mapping in a > new page.Oh, I see you do it steal_page(). Okay, I think best to make the remove_page() call ifndef ia64 then. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Isaku Yamahata
2008-May-29 10:30 UTC
Re: [Xen-devel] [PATCH] Remove physmap page upon granttab xfer
On Thu, May 29, 2008 at 11:15:31AM +0100, Keir Fraser wrote:> > > > On 29/5/08 11:10, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote: > > >> Thank you for explanation. > >> The patch breaks ia64 granttab xfer breakage so that it have to be fixed. > >> However the operation is now obsoleted one, so I don''t want to make > >> big effort to fix. > >> What do you think about the following patch? > > > > How does grant transfer work on ia64? Looking at netback I don''t even see > > how the ''hole'' left by transfer of the old page gets filled by mapping in a > > new page. > > Oh, I see you do it steal_page(). Okay, I think best to make the > remove_page() call ifndef ia64 then.Thanks, here is the patch. [IA64] fix ia64 granttab xfer. On ia64 steal_page() also update the p2m table so that guest_physmap_remove_page() call isn''t necessary. #ifndef out the call. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> diff -r 74c35b543f27 xen/common/grant_table.c --- a/xen/common/grant_table.c Thu May 29 18:14:33 2008 +0900 +++ b/xen/common/grant_table.c Thu May 29 19:23:07 2008 +0900 @@ -1112,7 +1112,9 @@ goto copyback; } +#ifndef __ia64__ /* on ia64 steal_page() also does remove_page() */ guest_physmap_remove_page(d, gop.mfn, mfn, 0); +#endif /* Find the target domain. */ if ( unlikely((e = rcu_lock_domain_by_id(gop.domid)) == NULL) ) -- yamahata _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel