Re: [Xen-devel] [PATCH 3/3] xen/m2p: use GNTTABOP_unmap_and_replace to replace foreign grants with balloon_trade_page

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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 <stefano.stabellini@xxxxxxxxxxxxx>
> > CC: alex@xxxxxxxxxxx
> > CC: dcrisan@xxxxxxxxxxxx
> > ---
> >  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 <asm/xen/page.h>
> >  #include <asm/xen/hypercall.h>
> >  #include <asm/xen/hypervisor.h>
> > +#include <xen/balloon.h>
> >  #include <xen/grant_table.h>
> >  
> >  #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?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel