Re: [Xen-devel] [PATCH v2 2/2] xen/m2p: use GNTTABOP_unmap_and_replace to reinstate the original mapping

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 22 Jul 2013, David Vrabel wrote:
> On 22/07/13 17:28, 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.  GNTTABOP_unmap_and_replace zeroes the mapping
> > passed in new_addr so we have to reinstate it, however that is a
> > per-cpu mapping only used for balloon trade pages, so we can be sure that
> > it's not going to be accessed while the mapping is not valid.
> 
> This solves the problem of userspace accessing a disk image on an NFS
> mount but what would blkback talking to an iSCSI LUN?  Will that need
> similar fixes to blkback?  This series does not need to fix this now though.

No, it wouldn't. We actually need a better hypercall than
GNTTABOP_unmap_and_replace for that.


> > --- 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(get_balloon_trade_page()) << 
> > PAGE_SHIFT);
> 
>       struct page *trade_page = get_balloon_trade_page();
>       unsigned long trade_page_address = page_address(trade_page);
> 
> (And the corresponding put_balloon_trade_page() once you've added it.)
> 
> Otherwise,
> 
> Reviewed-by: David Vrabel <david.vrabel@xxxxxxxxxx>> 


thanks


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