From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 3/3] xen/m2p: use GNTTABOP_unmap_and_replace to replace foreign grants with balloon_trade_page Date: Mon, 22 Jul 2013 11:32:36 +0100 Message-ID: References: <1374427951-24126-3-git-send-email-stefano.stabellini@eu.citrix.com> <1374428975.4127.49.camel@hastur.hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1374428975.4127.49.camel@hastur.hellion.org.uk> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xensource.com, Stefano Stabellini , david.vrabel@citrix.com, alex@alex.org.uk, dcrisan@flexiant.com List-Id: xen-devel@lists.xenproject.org On Sun, 21 Jul 2013, Ian Campbell wrote: > On Sun, 2013-07-21 at 18:32 +0100, Stefano Stabellini wrote: > > GNTTABOP_unmap_grant_ref unmaps a grant and replaces it with a 0 > > mapping instead of reinstating the original mapping. > > Doing so separately would be racy. > > To unmap a grant and reinstate the original mapping atomically we use > > GNTTABOP_unmap_and_replace. > > GNTTABOP_unmap_and_replace doesn't work with GNTMAP_contains_pte, so > > don't use it for kmaps. > > > > Note: the old GNTTABOP_unmap_and_replace_legacy zeros the mapping passed > > as "new_addr". This can cause race conditions when another cpu tries to > > use the mapping before it has been restored. > > > > Signed-off-by: Stefano Stabellini > > CC: alex@alex.org.uk > > CC: dcrisan@flexiant.com > > --- > > arch/x86/xen/p2m.c | 25 ++++++++++++++++++------- > > drivers/xen/gntdev.c | 11 ++--------- > > 2 files changed, 20 insertions(+), 16 deletions(-) > > > > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c > > index 95fb2aa..20d7056 100644 > > --- a/arch/x86/xen/p2m.c > > +++ b/arch/x86/xen/p2m.c > > @@ -161,6 +161,7 @@ > > #include > > #include > > #include > > +#include > > #include > > > > #include "multicalls.h" > > @@ -967,7 +968,9 @@ int m2p_remove_override(struct page *page, > > if (kmap_op != NULL) { > > if (!PageHighMem(page)) { > > struct multicall_space mcs; > > - struct gnttab_unmap_grant_ref *unmap_op; > > + struct gnttab_unmap_and_replace *unmap_op; > > + unsigned long trade_page_address = (unsigned long) > > + __va(page_to_pfn(balloon_trade_page) << PAGE_SHIFT); > > Rather than exporting this from the balloon driver could the gnttab code > not remember what the original mapping was and simply replace it? In > pages which came from the balloon driver this would effectively aways be > the trade page but it would work for pages from other sources too. The m2p code already remembers exactly what the original mapping was. In fact the original mfn is stored in page->index. However that is the mfn and gnttab_unmap_and_replace takes a virtual address. It could take a pte pointer but that functionality is not implemented. If I use __va on the page I get the current mapping (the grant). To avoid confusion I exported balloon_trade_page instead. > > /* > > * It might be that we queued all the m2p grant table > > @@ -990,20 +993,28 @@ int m2p_remove_override(struct page *page, > > } > > > > mcs = xen_mc_entry( > > - sizeof(struct gnttab_unmap_grant_ref)); > > + sizeof(struct gnttab_unmap_and_replace)); > > unmap_op = mcs.args; > > unmap_op->host_addr = kmap_op->host_addr; > > + unmap_op->new_addr = trade_page_address; > > If you are using the old replace operation won't this nuke the existing > mapping of the trade page and knacker any future uses? i.e. it breaks > the fallback case? Yes > > unmap_op->handle = kmap_op->handle; > > - unmap_op->dev_bus_addr = 0; > > > > MULTI_grant_table_op(mcs.mc, > > - GNTTABOP_unmap_grant_ref, unmap_op, 1); > > + gnttabop_unmap_and_replace, unmap_op, 1); > > > > xen_mc_issue(PARAVIRT_LAZY_MMU); > > > > - set_pte_at(&init_mm, address, ptep, > > - pfn_pte(pfn, PAGE_KERNEL)); > > - __flush_tlb_single(address); > > + /* this version of the hypercall is racy, let's try to limit > > + * the damage */ > > + if (unlikely(gnttabop_unmap_and_replace == > > + GNTTABOP_unmap_and_replace_legacy)) { > > Does this race go away if you allocate per-cpu virtual addresses which > all alias the same pfn? Yes. I admit that I might have been blinded by my hatred against GNTTABOP_unmap_and_replace: I thought that an interface that broken would need to be fixed and backported. Also I didn't want to allocate nr_cpu pages of virtual addresses and handle cpu-hotplug notifications. If we go through all the trouble of doing that, should we even bother introducing the new GNTTABOP_unmap_and_replace?