Re: [Xen-devel] [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Sun, 2013-07-21 at 18:34 +0100, Stefano Stabellini wrote:
> GNTTABOP_unmap_and_replace has two issues:
> - it unconditionally replaces the mapping passed in new_addr with 0;
> - it doesn't support GNTMAP_contains_pte mappings on x86, returning a
> general error instead of some forms of ENOSYS.
> 
> 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  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;
> -    }

Doesn't this break all existing users of the subop? I think you need to
stick a if (op == unmap_and_replace_legacy) in here and keep the code
for that case.

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

You need an ifdef here so that users specifying an old
__XEN_INTERFACE_VERSION__ get the old hypercall under the non-legacy
name.

>  /* ` } */
>  
> @@ -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 {                   \



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