Roger Pau Monne
2013-Nov-04 16:30 UTC
[PATCH] xen/grant_table: introduce GNTTABOP_unmap_and_duplicate
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 form of ENOSYS.
Introduce a new operation called GNTTABOP_unmap_and_duplicate that
behaves like GNTTABOP_unmap_and_replace without zeroing the mapping
passed in new_addr. Also return GNTST_enosys instead of
GNTST_general_error if GNTMAP_contains_pte is specified.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
---
xen/arch/arm/mm.c | 2 +-
xen/arch/x86/mm.c | 36 ++++++++++++----
xen/common/compat/grant_table.c | 8 ++++
xen/common/grant_table.c | 83 ++++++++++++++++++++++++++++++++++++-
xen/include/asm-arm/grant_table.h | 2 +-
xen/include/asm-x86/grant_table.h | 3 +-
xen/include/public/grant_table.h | 23 ++++++++++
xen/include/xlat.lst | 1 +
8 files changed, 145 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 123280e..39e6071 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1289,7 +1289,7 @@ int create_grant_host_mapping(unsigned long addr, unsigned
long frame,
}
int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
- unsigned long new_addr, unsigned int flags)
+ unsigned long new_addr, unsigned int flags, bool_t zero_new_addr)
{
unsigned long gfn = (unsigned long)(addr >> PAGE_SHIFT);
struct domain *d = current->domain;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 43aaceb..89e1b43 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4047,7 +4047,8 @@ static int replace_grant_p2m_mapping(
}
int replace_grant_host_mapping(
- uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags)
+ uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags,
+ bool_t zero_new_addr)
{
struct vcpu *curr = current;
l1_pgentry_t *pl1e, ol1e;
@@ -4064,7 +4065,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,14 +4103,31 @@ int replace_grant_host_mapping(
ol1e = *pl1e;
- if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(),
- gl1mfn, curr, 0)) )
+ if ( zero_new_addr )
{
- 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;
+ 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;
+ }
+ }
+ else
+ {
+ /* Increase refcount */
+ if ( unlikely(!get_page_and_type(l1e_get_page(ol1e), curr->domain,
+ PGT_writable_page)) )
+ {
+ page_unlock(l1pg);
+ put_page(l1pg);
+ guest_unmap_l1e(curr, pl1e);
+ MEM_LOG("Cannot increase refcount of frame at %"PRI_mfn,
+ page_to_mfn(l1e_get_page(ol1e)));
+ return GNTST_general_error;
+ }
}
page_unlock(l1pg);
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 7ebbbc1..8ab5e78 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -51,6 +51,10 @@ CHECK_gnttab_get_version;
CHECK_gnttab_swap_grant_ref;
#undef xen_gnttab_swap_grant_ref
+#define xen_gnttab_unmap_and_duplicate gnttab_unmap_and_duplicate
+CHECK_gnttab_unmap_and_duplicate;
+#undef xen_gnttab_unmap_and_duplicate
+
int compat_grant_table_op(unsigned int cmd,
XEN_GUEST_HANDLE_PARAM(void) cmp_uop,
unsigned int count)
@@ -106,6 +110,10 @@ int compat_grant_table_op(unsigned int cmd,
CASE(swap_grant_ref);
#endif
+#ifndef CHECK_gnttab_unmap_and_duplicate
+ CASE(unmap_and_duplicate);
+#endif
+
#undef CASE
default:
return do_grant_table_op(cmd, cmp_uop, count);
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index f42bc7a..54ee952 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -70,6 +70,7 @@ struct gnttab_unmap_common {
uint64_t dev_bus_addr;
uint64_t new_addr;
grant_handle_t handle;
+ bool_t zero_new_addr;
/* Return */
int16_t status;
@@ -920,7 +921,8 @@ __gnttab_unmap_common(
{
if ( (rc = replace_grant_host_mapping(op->host_addr,
op->frame, op->new_addr,
- op->flags)) < 0 )
+ op->flags,
+ op->zero_new_addr)) < 0 )
goto unmap_out;
ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
@@ -1129,6 +1131,7 @@ __gnttab_unmap_and_replace(
common->host_addr = op->host_addr;
common->new_addr = op->new_addr;
common->handle = op->handle;
+ common->zero_new_addr = 1;
/* Intialise these in case common contains old state */
common->dev_bus_addr = 0;
@@ -1184,6 +1187,70 @@ fault:
return -EFAULT;
}
+static void
+__gnttab_unmap_and_duplicate(
+ struct gnttab_unmap_and_duplicate *op,
+ struct gnttab_unmap_common *common)
+{
+ common->host_addr = op->host_addr;
+ common->new_addr = op->new_addr;
+ common->handle = op->handle;
+ common->zero_new_addr = 0;
+
+ /* Intialise these in case common contains old state */
+ common->dev_bus_addr = 0;
+ common->rd = NULL;
+
+ __gnttab_unmap_common(common);
+ op->status = common->status;
+}
+
+static long
+gnttab_unmap_and_duplicate(
+ XEN_GUEST_HANDLE_PARAM(gnttab_unmap_and_duplicate_t) uop, unsigned int
count)
+{
+ int i, c, partial_done, done = 0;
+ struct gnttab_unmap_and_duplicate op;
+ struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
+
+ while ( count != 0 )
+ {
+ c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
+ partial_done = 0;
+
+ for ( i = 0; i < c; i++ )
+ {
+ if ( unlikely(__copy_from_guest(&op, uop, 1)) )
+ goto fault;
+ __gnttab_unmap_and_duplicate(&op, &(common[i]));
+ ++partial_done;
+ if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
+ goto fault;
+ guest_handle_add_offset(uop, 1);
+ }
+
+ flush_tlb_mask(current->domain->domain_dirty_cpumask);
+
+ for ( i = 0; i < partial_done; i++ )
+ __gnttab_unmap_common_complete(&(common[i]));
+
+ count -= c;
+ done += c;
+
+ if (count && hypercall_preempt_check())
+ return done;
+ }
+
+ return 0;
+
+fault:
+ flush_tlb_mask(current->domain->domain_dirty_cpumask);
+
+ for ( i = 0; i < partial_done; i++ )
+ __gnttab_unmap_common_complete(&(common[i]));
+ return -EFAULT;
+}
+
static int
gnttab_populate_status_frames(struct domain *d, struct grant_table *gt,
unsigned int req_nr_frames)
@@ -2552,6 +2619,20 @@ do_grant_table_op(
}
break;
}
+ case GNTTABOP_unmap_and_duplicate:
+ {
+ XEN_GUEST_HANDLE_PARAM(gnttab_unmap_and_duplicate_t) unmap +
guest_handle_cast(uop, gnttab_unmap_and_duplicate_t);
+ if ( unlikely(!guest_handle_okay(unmap, count)) )
+ goto out;
+ rc = gnttab_unmap_and_duplicate(unmap, count);
+ if ( rc > 0 )
+ {
+ guest_handle_add_offset(unmap, rc);
+ uop = guest_handle_cast(unmap, void);
+ }
+ break;
+ }
default:
rc = -ENOSYS;
break;
diff --git a/xen/include/asm-arm/grant_table.h
b/xen/include/asm-arm/grant_table.h
index 6e0cc59..0410355 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -12,7 +12,7 @@ int create_grant_host_mapping(unsigned long gpaddr,
cache_flags);
#define gnttab_host_mapping_get_page_type(op, d, rd) (0)
int replace_grant_host_mapping(unsigned long gpaddr, unsigned long mfn,
- unsigned long new_gpaddr, unsigned int flags);
+ unsigned long new_gpaddr, unsigned int flags, bool_t zero_new_addr);
void gnttab_mark_dirty(struct domain *d, unsigned long l);
#define gnttab_create_status_page(d, t, i) do {} while (0)
#define gnttab_status_gmfn(d, t, i) (0)
diff --git a/xen/include/asm-x86/grant_table.h
b/xen/include/asm-x86/grant_table.h
index 3013869..e985968 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -16,7 +16,8 @@
int create_grant_host_mapping(uint64_t addr, unsigned long frame,
unsigned int flags, unsigned int cache_flags);
int replace_grant_host_mapping(
- uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags);
+ uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags,
+ bool_t zero_new_addr);
#define gnttab_create_shared_page(d, t, i) \
do { \
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index b8a3d6c..a4ad98d 100644
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -309,6 +309,7 @@ typedef uint16_t grant_status_t;
#define GNTTABOP_get_status_frames 9
#define GNTTABOP_get_version 10
#define GNTTABOP_swap_grant_ref 11
+#define GNTTABOP_unmap_and_duplicate 12
#endif /* __XEN_INTERFACE_VERSION__ */
/* ` } */
@@ -574,6 +575,27 @@ struct gnttab_swap_grant_ref {
typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
+/*
+ * GNTTABOP_unmap_and_duplicate: 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>.
+ * NOTES:
+ * 1. The call may fail in an undefined manner if either mapping is not
+ * tracked by <handle>.
+ * 2. After executing a batch of unmaps, it is guaranteed that no stale
+ * mappings will remain in the device or host TLBs.
+ */
+struct gnttab_unmap_and_duplicate {
+ /* IN parameters. */
+ uint64_t host_addr;
+ uint64_t new_addr;
+ grant_handle_t handle;
+ /* OUT parameters. */
+ int16_t status; /* => enum grant_status */
+};
+typedef struct gnttab_unmap_and_duplicate gnttab_unmap_and_duplicate_t;
+DEFINE_XEN_GUEST_HANDLE(gnttab_unmap_and_duplicate_t);
+
#endif /* __XEN_INTERFACE_VERSION__ */
/*
@@ -631,6 +653,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 { \
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index f00cef3..8b967f1 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -51,6 +51,7 @@
? grant_entry_header grant_table.h
? grant_entry_v2 grant_table.h
? gnttab_swap_grant_ref grant_table.h
+? gnttab_unmap_and_duplicate grant_table.h
? kexec_exec kexec.h
! kexec_image kexec.h
! kexec_range kexec.h
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Jan Beulich
2013-Nov-05 09:40 UTC
Re: [PATCH] xen/grant_table: introduce GNTTABOP_unmap_and_duplicate
>>> On 04.11.13 at 17:30, Roger Pau Monne <roger.pau@citrix.com> wrote: > int replace_grant_host_mapping( > - uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags) > + uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags, > + bool_t zero_new_addr)So why can this flag not be merged into "flags"?> - if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(), > - gl1mfn, curr, 0)) ) > + if ( zero_new_addr ) > { > - 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; > + 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; > + } > + } > + else > + { > + /* Increase refcount */ > + if ( unlikely(!get_page_and_type(l1e_get_page(ol1e), curr->domain, > + PGT_writable_page)) ) > + { > + page_unlock(l1pg); > + put_page(l1pg); > + guest_unmap_l1e(curr, pl1e); > + MEM_LOG("Cannot increase refcount of frame at %"PRI_mfn, > + page_to_mfn(l1e_get_page(ol1e))); > + return GNTST_general_error; > + }The two error paths are identical except for the messages they log. Please avoid unnecessary code duplication like this. That''ll also reduce the churn the patch causes due to the indentation change. (Moving the guest_unmap_l1e() ahead of the issuing of the message is, btw, appreciated.)> +/* > + * GNTTABOP_unmap_and_duplicate: 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>. > + * NOTES: > + * 1. The call may fail in an undefined manner if either mapping is not > + * tracked by <handle>. > + * 2. After executing a batch of unmaps, it is guaranteed that no stale > + * mappings will remain in the device or host TLBs. > + */ > +struct gnttab_unmap_and_duplicate { > + /* IN parameters. */ > + uint64_t host_addr; > + uint64_t new_addr; > + grant_handle_t handle; > + /* OUT parameters. */ > + int16_t status; /* => enum grant_status */ > +};If you re-used the identical in layout gnttab_unmap_and_replace here (perhaps nevertheless creating aliases with the right name), wouldn''t it be possible to also reduce the amount of added code duplication in xen/common/grant_table.c? Jan