Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Sun, Jul 21, 2013 at 06:34:59PM +0100, Stefano Stabellini wrote:
> GNTTABOP_unmap_and_replace has two issues:
> - it unconditionally replaces the mapping passed in new_addr with 0;

OK, so the caller needs to save this. Perhaps you can also add a patch
to the header file to mention this bug since this API call is quite
"baked"

> - it doesn't support GNTMAP_contains_pte mappings on x86, returning a
> general error instead of some forms of ENOSYS.

Is there a specific version of Xen that has this differently? If the
generel error is replaced with a proper ENOSYS what is the fallout?

> 
> Deprecate GNTTABOP_unmap_and_replace and introduce a new
> GNTTABOP_unmap_and_replace (12) that returns GNTST_enosys for
> GNTMAP_contains_pte requests and doesn't zero the mapping at new_addr.

Or just introduce v2 of said call and leave the old one as is. The
Linux code can try the new one during bootup and if it fails use
the fallback.

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/x86/mm.c                |   12 +-----------
>  xen/include/public/grant_table.h |    7 ++++---
>  2 files changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 77dcafc..610fd09 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4064,7 +4064,7 @@ int replace_grant_host_mapping(
>              return destroy_grant_pte_mapping(addr, frame, curr->domain);
>          
>          MEM_LOG("Unsupported grant table operation");
> -        return GNTST_general_error;
> +        return GNTST_enosys;
>      }
>  
>      if ( !new_addr )
> @@ -4102,16 +4102,6 @@ int replace_grant_host_mapping(
>  
>      ol1e = *pl1e;
>  
> -    if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(),
> -                                gl1mfn, curr, 0)) )
> -    {
> -        page_unlock(l1pg);
> -        put_page(l1pg);
> -        MEM_LOG("Cannot delete PTE entry at %p", (unsigned long *)pl1e);
> -        guest_unmap_l1e(curr, pl1e);
> -        return GNTST_general_error;
> -    }
> -
>      page_unlock(l1pg);
>      put_page(l1pg);
>      guest_unmap_l1e(curr, pl1e);
> diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
> index b8a3d6c..ae841ae 100644
> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -303,12 +303,13 @@ typedef uint16_t grant_status_t;
>  #define GNTTABOP_transfer             4
>  #define GNTTABOP_copy                 5
>  #define GNTTABOP_query_size           6
> -#define GNTTABOP_unmap_and_replace    7
> +#define GNTTABOP_unmap_and_replace_legacy    7
>  #if __XEN_INTERFACE_VERSION__ >= 0x0003020a
>  #define GNTTABOP_set_version          8
>  #define GNTTABOP_get_status_frames    9
>  #define GNTTABOP_get_version          10
>  #define GNTTABOP_swap_grant_ref	      11
> +#define GNTTABOP_unmap_and_replace    12
>  #endif /* __XEN_INTERFACE_VERSION__ */
>  /* ` } */
>  
> @@ -489,8 +490,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_query_size_t);
>  /*
>   * GNTTABOP_unmap_and_replace: Destroy one or more grant-reference mappings
>   * tracked by <handle> but atomically replace the page table entry with one
> - * pointing to the machine address under <new_addr>.  <new_addr> will be
> - * redirected to the null entry.
> + * pointing to the machine address under <new_addr>.
>   * NOTES:
>   *  1. The call may fail in an undefined manner if either mapping is not
>   *     tracked by <handle>.
> @@ -631,6 +631,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
>  #define GNTST_bad_copy_arg    (-10) /* copy arguments cross page boundary.   */
>  #define GNTST_address_too_big (-11) /* transfer page address too large.      */
>  #define GNTST_eagain          (-12) /* Operation not done; try again.        */
> +#define GNTST_enosys          (-13) /* Operation not implemented.            */
>  /* ` } */
>  
>  #define GNTTABOP_error_msgs {                   \
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel