Stefano Stabellini
2013-Jul-21 17:34 UTC
[PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
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
Ian Campbell
2013-Jul-21 17:52 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
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 { \
Stefano Stabellini
2013-Jul-22 10:15 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
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?
Stefano Stabellini
2013-Jul-22 10:22 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
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.
David Vrabel
2013-Jul-22 11:40 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
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
Konrad Rzeszutek Wilk
2013-Jul-22 19:45 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
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
Stefano Stabellini
2013-Jul-23 11:52 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
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.
Jan Beulich
2013-Aug-05 12:06 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
>>> 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
Jan Beulich
2013-Aug-05 12:07 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
>>> 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
Stefano Stabellini
2013-Aug-05 13:00 UTC
Re: [PATCH RFC] xen/grant_table: deprecate GNTTABOP_unmap_and_replace
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.