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

[Ian Campbell <ian.campbell@citrix.com>]

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@eu.citrix.com>
> 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 <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.

>  			/*
>  			 * 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?


>  			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?

> +				mcs = __xen_mc_entry(0);
> +
> +				MULTI_update_va_mapping(mcs.mc, trade_page_address,
> +						pfn_pte(page_to_pfn(balloon_trade_page), PAGE_KERNEL_RO),
> +						0);
> +				xen_mc_issue(PARAVIRT_LAZY_MMU);
> +			}
>  			kmap_op->host_addr = 0;
>  		}
>  	}