[PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace

[PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace

[Stefano Stabellini]

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

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

[Ian Campbell]

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@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;
> -    }

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 {                   \

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

[Stefano Stabellini]

On Sun, 21 Jul 2013, Ian Campbell wrote:
> 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@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;
> > -    }
> 
> 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.

I don't think there are any existing users, but that's a fair point.

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

I guess I need to bump the version too?

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

[Stefano Stabellini]

On Mon, 22 Jul 2013, Stefano Stabellini wrote:
> On Sun, 21 Jul 2013, Ian Campbell wrote:
> > 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@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;
> > > -    }
> > 
> > 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.
> 
> I don't think there are any existing users, but that's a fair point.
> 
> > > -
> > >      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.
> 
> I guess I need to bump the version too?

Thinking twice about this, even though bumping the interface version
would be easy enough, that would prevent any backports of the "fixed"
hypercalls to older hypervisors.

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

[David Vrabel]

On 21/07/13 18:34, 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.

I don't think you can change the behavior of the existing sub-op in this
way.  XenServer's current netback driver relies on the current
behaviour, for example.

I think this will also mess up the ref counting on the frame that is at
the new_addr virtual address.  It will be mapped twice but with no
additional ref count.

I think the you can fix the problem in Linux without any hypervisor side
changes, but adding a new sub-op for GNTTABOP_unmap_and_duplicate (unmap
and replace with mapping with a duplicate of an existing mapping) will
allow for batching.

David

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

[Konrad Rzeszutek Wilk]

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

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

[Stefano Stabellini]

On Mon, 22 Jul 2013, Konrad Rzeszutek Wilk wrote:
> 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.
> 

Right, but if we have the fallback code in Linux we might as well use it
and avoid messing with the grant table interface here. That's why I
decided to drop this patch.

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

[Jan Beulich]

>>> On 22.07.13 at 12:22, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 22 Jul 2013, Stefano Stabellini wrote:
>> On Sun, 21 Jul 2013, Ian Campbell wrote:
>> > On Sun, 2013-07-21 at 18:34 +0100, Stefano Stabellini wrote:
>> > > --- 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.
>> 
>> I guess I need to bump the version too?
> 
> Thinking twice about this, even though bumping the interface version
> would be easy enough, that would prevent any backports of the "fixed"
> hypercalls to older hypervisors.

Why?

Jan

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

[Jan Beulich]

>>> On 22.07.13 at 12:15, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> On Sun, 21 Jul 2013, Ian Campbell wrote:
>> 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@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;
>> > -    }
>> 
>> 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.
> 
> I don't think there are any existing users, but that's a fair point.

As was hopefully implicit from David's response - there _are_ users
of this interface.

Jan

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

[Stefano Stabellini]

On Mon, 5 Aug 2013, Jan Beulich wrote:
> >>> On 22.07.13 at 12:22, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 22 Jul 2013, Stefano Stabellini wrote:
> >> On Sun, 21 Jul 2013, Ian Campbell wrote:
> >> > On Sun, 2013-07-21 at 18:34 +0100, Stefano Stabellini wrote:
> >> > > --- 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.
> >> 
> >> I guess I need to bump the version too?
> > 
> > Thinking twice about this, even though bumping the interface version
> > would be easy enough, that would prevent any backports of the "fixed"
> > hypercalls to older hypervisors.
> 
> Why?

Jan, I dropped this patch and any Xen changes from my new versions of
the series. I am doing all the changes only on the Linux side.
That means that we don't need a new hypervisor version to have safe
O_DIRECT with files on NFS.

However Roger is now introducing a new hypercall, similar to my new
version of GNTTABOP_unmap_and_replace, to allow batch unmaps without
multicalls. That should be a good performance improvement.