diff mbox

[Xen-devel,v8,6/8] xen/arm: introduce GNTTABOP_cache_flush

Message ID 1413798499-28314-6-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini Oct. 20, 2014, 9:48 a.m. UTC
Introduce a new hypercall to perform cache maintenance operation on
behalf of the guest. The argument is a machine address and a size. The
implementation checks that the memory range is owned by the guest or the
guest has been granted access to it by another domain.

Introduce grant_map_exists: an internal grant table function to check
whether an mfn has been granted to a given domain on a target grant
table. Check hypercall_preempt_check() every 4096 iterations in the
implementation of grant_map_exists.
Use the top 20 bits of the GNTTABOP cmd encoding to save the last ref
across the hypercall continuation.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v8:
- avoid security issues, use two separate opaque variables to store the
input argument and the output argument;
- fix return values of grant_map_exists;
- rename CMD_MASK to GNTTABOP_CMD_MASK;
- rename OPAQUE_CONTINUATION_ARG_SHIFT to
GNTTABOP_CONTINUATION_ARG_SHIFT;
- save in the opaque argument the shifted ref_count;
- set GNTTABOP_CONTINUATION_ARG_SHIFT to 20 and
MAX_GRANT_ENTRIES_ITER_SHIFT to 12, to cover the full grant_ref_t value
range;
- move GNTTABOP_CONTINUATION_ARG_SHIFT and GNTTABOP_CMD_MASK to
include/xen/grant_table.h;
- cmd &= GNTTABOP_CMD_MASK in the compat wrapper.

Changes in v7:
- remove warning message;
- prefix second line of the warning with XENLOG_WARNING;
- do not lower DEFAULT_MAX_NR_GRANT_FRAMES;
- no long lines;
- interrupt loops in grant_map_exists with more than 2048 iterations,
  create an hypercall continuation if necessary.

Changes in v6:
- set DEFAULT_MAX_NR_GRANT_FRAMES to 10;
- warn if max_grant_frames > 10.

Changes in v5:
- make mfn mfn unsigned long;
- remove unhelpful error message;
- handle errors returned by cache maintenance functions.

Changes in v4:
- ASSERT(spin_is_locked);
- return instead of break in grant_map_exists;
- pass a pointer to __gnttab_cache_flush;
- code style;
- unsigned int iterator in gnttab_cache_flush;
- return ret instead -ret;
- cflush.offset >= PAGE_SIZE return -EINVAL.

Changes in v3:
- reduce the time the grant_table lock is held;
- fix warning message;
- s/EFAULT/EPERM;
- s/llx/PRIx64;
- check offset and size independetly before checking their sum;
- rcu_lock_current_domain cannot fail;
- s/ENOSYS/EOPNOTSUPP;
- use clean_and_invalidate_xen_dcache_va_range to do both operations at
once;
- fold grant_map_exists in this patch;
- support "count" argument;
- make correspondent changes to compat/grant_table.c;
- introduce GNTTAB_CACHE_SOURCE_GREF to select the type of input in the
union;
- rename size field to length;
- make length and offset uint16_t;
- only take spin_lock if d != owner.

Changes in v2:
- do not check for mfn_to_page errors;
- take a reference to the page;
- replace printk with gdprintk;
- split long line;
- remove out label;
- move rcu_lock_current_domain down before the loop.
- move the hypercall to GNTTABOP;
- take a spin_lock before calling grant_map_exists.
---
 xen/common/compat/grant_table.c  |   15 +++-
 xen/common/grant_table.c         |  166 +++++++++++++++++++++++++++++++++++++-
 xen/include/public/grant_table.h |   21 +++++
 xen/include/xen/grant_table.h    |    3 +
 xen/include/xlat.lst             |    1 +
 5 files changed, 200 insertions(+), 6 deletions(-)

Comments

Ian Campbell Oct. 20, 2014, 3 p.m. UTC | #1
On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:

Despite the title I don't think this is arm specific any more, is it? I
trust Jan's judgement more than my own on this area of code so I intend
to defer to him unless someone suggests I do otherwise.
Ian Campbell Oct. 20, 2014, 3:24 p.m. UTC | #2
On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
>  
> @@ -574,6 +576,25 @@ 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);
>  
> +/*
> + * Issue one or more cache maintenance operations on a portion of a
> + * page granted to the calling domain by a foreign domain.
> + */
> +struct gnttab_cache_flush {
> +    union {
> +        uint64_t dev_bus_addr;
> +        grant_ref_t ref;
> +    } a;
> +    uint16_t offset; /* offset from start of grant */
> +    uint16_t length; /* size within the grant */

So are these not valid when used with dev_bus_addr? I can see that being
the case for offset, but length too?

> +#define GNTTAB_CACHE_CLEAN          (1<<0)
> +#define GNTTAB_CACHE_INVAL          (1<<1)
> +#define GNTTAB_CACHE_SOURCE_GREF    (1<<31)
> +    uint32_t op;
> +};
> +typedef struct gnttab_cache_flush gnttab_cache_flush_t;
> +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
> +
>  #endif /* __XEN_INTERFACE_VERSION__ */
>  
>  /*
Jan Beulich Oct. 20, 2014, 3:30 p.m. UTC | #3
>>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
>>  
>> @@ -574,6 +576,25 @@ 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);
>>  
>> +/*
>> + * Issue one or more cache maintenance operations on a portion of a
>> + * page granted to the calling domain by a foreign domain.
>> + */
>> +struct gnttab_cache_flush {
>> +    union {
>> +        uint64_t dev_bus_addr;
>> +        grant_ref_t ref;
>> +    } a;
>> +    uint16_t offset; /* offset from start of grant */
>> +    uint16_t length; /* size within the grant */
> 
> So are these not valid when used with dev_bus_addr? I can see that being
> the case for offset, but length too?

Both ought to be valid, and dev_bus_addr should be page aligned.

Jan
Stefano Stabellini Oct. 20, 2014, 4:31 p.m. UTC | #4
On Mon, 20 Oct 2014, Jan Beulich wrote:
> >>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
> >>  
> >> @@ -574,6 +576,25 @@ 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);
> >>  
> >> +/*
> >> + * Issue one or more cache maintenance operations on a portion of a
> >> + * page granted to the calling domain by a foreign domain.
> >> + */
> >> +struct gnttab_cache_flush {
> >> +    union {
> >> +        uint64_t dev_bus_addr;
> >> +        grant_ref_t ref;
> >> +    } a;
> >> +    uint16_t offset; /* offset from start of grant */
> >> +    uint16_t length; /* size within the grant */
> > 
> > So are these not valid when used with dev_bus_addr? I can see that being
> > the case for offset, but length too?
> 
> Both ought to be valid, and dev_bus_addr should be page aligned.

That's right.
Ian Campbell Oct. 21, 2014, 9:04 a.m. UTC | #5
On Mon, 2014-10-20 at 17:31 +0100, Stefano Stabellini wrote:
> On Mon, 20 Oct 2014, Jan Beulich wrote:
> > >>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
> > > On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
> > >>  
> > >> @@ -574,6 +576,25 @@ 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);
> > >>  
> > >> +/*
> > >> + * Issue one or more cache maintenance operations on a portion of a
> > >> + * page granted to the calling domain by a foreign domain.
> > >> + */
> > >> +struct gnttab_cache_flush {
> > >> +    union {
> > >> +        uint64_t dev_bus_addr;
> > >> +        grant_ref_t ref;
> > >> +    } a;
> > >> +    uint16_t offset; /* offset from start of grant */
> > >> +    uint16_t length; /* size within the grant */
> > > 
> > > So are these not valid when used with dev_bus_addr? I can see that being
> > > the case for offset, but length too?
> > 
> > Both ought to be valid, and dev_bus_addr should be page aligned.
> 
> That's right.

I would have expected you to correct the comment in the repost then
(which I notice also still says arm in the subject)

Ian.
diff mbox

Patch

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 2dc1e44..0368289 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -51,16 +51,21 @@  CHECK_gnttab_get_version;
 CHECK_gnttab_swap_grant_ref;
 #undef xen_gnttab_swap_grant_ref
 
+#define xen_gnttab_cache_flush gnttab_cache_flush
+CHECK_gnttab_cache_flush;
+#undef xen_gnttab_cache_flush
+
 int compat_grant_table_op(unsigned int cmd,
                           XEN_GUEST_HANDLE_PARAM(void) cmp_uop,
                           unsigned int count)
 {
     int rc = 0;
-    unsigned int i;
+    unsigned int i, cmd_op;
     XEN_GUEST_HANDLE_PARAM(void) cnt_uop;
 
     set_xen_guest_handle(cnt_uop, NULL);
-    switch ( cmd )
+    cmd_op = cmd & GNTTABOP_CMD_MASK;
+    switch ( cmd_op )
     {
 #define CASE(name) \
     case GNTTABOP_##name: \
@@ -106,6 +111,10 @@  int compat_grant_table_op(unsigned int cmd,
     CASE(swap_grant_ref);
 #endif
 
+#ifndef CHECK_gnttab_cache_flush
+    CASE(cache_flush);
+#endif
+
 #undef CASE
     default:
         return do_grant_table_op(cmd, cmp_uop, count);
@@ -132,7 +141,7 @@  int compat_grant_table_op(unsigned int cmd,
         } cmp;
 
         set_xen_guest_handle(nat.uop, COMPAT_ARG_XLAT_VIRT_BASE);
-        switch ( cmd )
+        switch ( cmd_op )
         {
         case GNTTABOP_setup_table:
             if ( unlikely(count > 1) )
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 784bbc6..c56ad5e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -488,6 +488,43 @@  static int _set_status(unsigned gt_version,
         return _set_status_v2(domid, readonly, mapflag, shah, act, status);
 }
 
+#define MAX_GRANT_ENTRIES_ITER_SHIFT 12
+static int grant_map_exists(const struct domain *ld,
+                            struct grant_table *rgt,
+                            unsigned long mfn,
+                            grant_ref_t *ref_count)
+{
+    const struct active_grant_entry *act;
+    grant_ref_t ref;
+
+    ASSERT(spin_is_locked(&rgt->lock));
+
+    for ( ref = *ref_count; ref < nr_grant_entries(rgt); ref++ )
+    {
+        if ( (ref % (1 << MAX_GRANT_ENTRIES_ITER_SHIFT)) == 0 &&
+             ref != *ref_count )
+        {
+            *ref_count = ref;
+            return 1;
+        }
+
+        act = &active_entry(rgt, ref);
+
+        if ( !act->pin )
+            continue;
+
+        if ( act->domid != ld->domain_id )
+            continue;
+
+        if ( act->frame != mfn )
+            continue;
+
+        return 0;
+    }
+
+    return -EINVAL;
+}
+
 static void mapcount(
     struct grant_table *lgt, struct domain *rd, unsigned long mfn,
     unsigned int *wrc, unsigned int *rdc)
@@ -2486,16 +2523,119 @@  gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
     return 0;
 }
 
+static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush,
+                                grant_ref_t *ref_count)
+{
+    struct domain *d, *owner;
+    struct page_info *page;
+    unsigned long mfn;
+    void *v;
+    int ret = 0;
+
+    if ( (cflush->offset >= PAGE_SIZE) ||
+         (cflush->length > PAGE_SIZE) ||
+         (cflush->offset + cflush->length > PAGE_SIZE) )
+        return -EINVAL;
+
+    if ( cflush->length == 0 || cflush->op == 0 )
+        return 0;
+
+    /* currently unimplemented */
+    if ( cflush->op & GNTTAB_CACHE_SOURCE_GREF )
+        return -EOPNOTSUPP;
+
+    d = rcu_lock_current_domain();
+    mfn = cflush->a.dev_bus_addr >> PAGE_SHIFT;
+
+    if ( !mfn_valid(mfn) )
+    {
+        rcu_unlock_domain(d);
+        return -EINVAL;
+    }
+
+    page = mfn_to_page(mfn);
+    owner = page_get_owner_and_reference(page);
+    if ( !owner )
+    {
+        rcu_unlock_domain(d);
+        return -EPERM;
+    }
+
+    if ( d != owner )
+    {
+        spin_lock(&owner->grant_table->lock);
+
+        ret = grant_map_exists(d, owner->grant_table, mfn, ref_count);
+        if ( ret != 0 )
+        {
+            spin_unlock(&owner->grant_table->lock);
+            rcu_unlock_domain(d);
+            put_page(page);
+            return ret;
+        }
+    }
+
+    v = map_domain_page(mfn);
+    v += cflush->offset;
+
+    if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op & GNTTAB_CACHE_CLEAN) )
+        ret = clean_and_invalidate_dcache_va_range(v, cflush->length);
+    else if ( cflush->op & GNTTAB_CACHE_INVAL )
+        ret = invalidate_dcache_va_range(v, cflush->length);
+    else if ( cflush->op & GNTTAB_CACHE_CLEAN )
+        ret = clean_dcache_va_range(v, cflush->length);
+
+    if ( d != owner )
+        spin_unlock(&owner->grant_table->lock);
+    unmap_domain_page(v);
+    put_page(page);
+
+    return ret;
+}
+
+static long
+gnttab_cache_flush(XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) uop,
+                      grant_ref_t *ref_count,
+                      unsigned int count)
+{
+    int ret = 0;
+    unsigned int i;
+    gnttab_cache_flush_t op;
+
+    for ( i = 0; i < count; i++ )
+    {
+        if ( i && hypercall_preempt_check() )
+            return i;
+        if ( unlikely(__copy_from_guest(&op, uop, 1)) )
+            return -EFAULT;
+        do {
+            ret = __gnttab_cache_flush(&op, ref_count);
+            if ( ret < 0 )
+                return ret;
+            if ( ret > 0 && hypercall_preempt_check() )
+                return i;
+        } while ( ret > 0 );
+        *ref_count = 0;
+        guest_handle_add_offset(uop, 1);
+    }
+    return 0;
+}
+
+
 long
 do_grant_table_op(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
     long rc;
+    unsigned int opaque_in = 0, opaque_out = 0;
     
     if ( (int)count < 0 )
         return -EINVAL;
     
     rc = -EFAULT;
+
+    opaque_in = cmd >> GNTTABOP_CONTINUATION_ARG_SHIFT;
+    cmd &= GNTTABOP_CMD_MASK;
     switch ( cmd )
     {
     case GNTTABOP_map_grant_ref:
@@ -2615,17 +2755,37 @@  do_grant_table_op(
         }
         break;
     }
+    case GNTTABOP_cache_flush:
+    {
+        grant_ref_t ref_count = opaque_in << MAX_GRANT_ENTRIES_ITER_SHIFT;
+        XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
+            guest_handle_cast(uop, gnttab_cache_flush_t);
+        if ( unlikely(!guest_handle_okay(cflush, count)) )
+            goto out;
+        rc = gnttab_cache_flush(cflush, &ref_count, count);
+        if ( rc > 0 )
+        {
+            guest_handle_add_offset(cflush, rc);
+            uop = guest_handle_cast(cflush, void);
+        }
+        opaque_out = ref_count >> MAX_GRANT_ENTRIES_ITER_SHIFT;
+        break;
+    }
     default:
         rc = -ENOSYS;
         break;
     }
     
   out:
-    if ( rc > 0 )
+    if ( rc > 0 || opaque_out != 0 )
     {
         ASSERT(rc < count);
-        rc = hypercall_create_continuation(__HYPERVISOR_grant_table_op,
-                                           "ihi", cmd, uop, count - rc);
+        ASSERT(opaque_out <
+               (1 << (sizeof(cmd)*8 - GNTTABOP_CONTINUATION_ARG_SHIFT)));
+        rc = hypercall_create_continuation(__HYPERVISOR_grant_table_op, "ihi",
+                                           (opaque_out <<
+                                            GNTTABOP_CONTINUATION_ARG_SHIFT) | cmd,
+                                           uop, count - rc);
     }
     
     return rc;
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index b8a3d6c..2dfb5a2 100644
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -309,6 +309,8 @@  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_cache_flush	      12
+/* max GNTTABOP is 4095 */
 #endif /* __XEN_INTERFACE_VERSION__ */
 /* ` } */
 
@@ -574,6 +576,25 @@  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);
 
+/*
+ * Issue one or more cache maintenance operations on a portion of a
+ * page granted to the calling domain by a foreign domain.
+ */
+struct gnttab_cache_flush {
+    union {
+        uint64_t dev_bus_addr;
+        grant_ref_t ref;
+    } a;
+    uint16_t offset; /* offset from start of grant */
+    uint16_t length; /* size within the grant */
+#define GNTTAB_CACHE_CLEAN          (1<<0)
+#define GNTTAB_CACHE_INVAL          (1<<1)
+#define GNTTAB_CACHE_SOURCE_GREF    (1<<31)
+    uint32_t op;
+};
+typedef struct gnttab_cache_flush gnttab_cache_flush_t;
+DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
+
 #endif /* __XEN_INTERFACE_VERSION__ */
 
 /*
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 32f5786..0b80be7 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -127,4 +127,7 @@  static inline unsigned int grant_to_status_frames(int grant_frames)
         GRANT_STATUS_PER_PAGE;
 }
 
+#define GNTTABOP_CONTINUATION_ARG_SHIFT 20
+#define GNTTABOP_CMD_MASK               ((1<<GNTTABOP_CONTINUATION_ARG_SHIFT)-1)
+
 #endif /* __XEN_GRANT_TABLE_H__ */
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 234b668..9ce9fee 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_cache_flush		grant_table.h
 ?	kexec_exec			kexec.h
 !	kexec_image			kexec.h
 !	kexec_range			kexec.h