All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush
@ 2014-10-13 14:57 Stefano Stabellini
  2014-10-13 14:58 ` [PATCH v5 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options Stefano Stabellini
                   ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:57 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Ian Campbell, Stefano Stabellini

Hi all,
this patch series introduces a new hypercall to perform cache
maintenance operations on behalf of the guest. It is useful for dom0 to
be able to cache flush pages involved in a dma operation with
non-coherent devices.

It also removes XENFEAT_grant_map_identity as the feature is no longer
necessary: it was used to achieve the same goal but the guest can now
use the hypercall instead. Keeping the flag would also have a
significant performance impact as a new p2m mapping gets created and
then destroyed for every grant that is mapped and unmapped in dom0.


Changes in v5:
- /max_nr_maptrack_frames/max_maptrack_frames/g;
- deprecate gnttab_max_nr_frames;
- introduce gnttab_max_frames;
- rename the max_nr_grant_frames variable to max_grant_frames;
- return int from flush_page_to_ram and *_dcache_va_range;
- return int from invalidate_dcache_va_range;
- make order an unsigned int;
- add a comment on sub-page granularity support;
- cache operations return error;
- move the functions to xen/include/asm-x86/flushtlb.h;
- make mfn unsigned long;
- remove unhelpful error message;
- handle errors returned by cache maintenance functions.

Changes in v4:
- add patch "xen: introduce gnttab_max_nr_maptrack_frames command line option";
- remove patch "introduce two different max_nr_dom0/domU_grant_frames
parameters";
- rename *_xen_dcache_* operations to *_dcache_*;
- implement the x86 cache maintenance functions using flush_area_local;
- 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;
- revert "xen: introduce arch_grant_(un)map_page_identity"

Changes in v3:
- introduce two different  max_nr_dom0/domU_grant_frames parameters;
- x86: introduce more cache maintenance operations;
- 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:
- make grant_map_exists static;
- remove the spin_lock in grant_map_exists;
- move the hypercall to GNTTABOP;
- do not check for mfn_to_page errors in GNTTABOP_cache_flush;
- take a reference to the page in GNTTABOP_cache_flush;
- replace printk with gdprintk in GNTTABOP_cache_flush;
- split long line in GNTTABOP_cache_flush;
- remove out label in GNTTABOP_cache_flush;
- move rcu_lock_current_domain down before the loop in
GNTTABOP_cache_flush;
- take a spin_lock before calling grant_map_exists in
GNTTABOP_cache_flush.



Stefano Stabellini (8):
      xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options
      xen/arm: rename *_xen_dcache_* operations to *_dcache_*
      xen/arm: return int from flush_page_to_ram and *_dcache_va_range
      xen/arm: introduce invalidate_dcache_va_range
      xen/x86: introduce more cache maintenance operations
      xen/arm: introduce GNTTABOP_cache_flush
      Revert "xen/arm: introduce XENFEAT_grant_map_identity"
      Revert "xen: introduce arch_grant_(un)map_page_identity"

 docs/misc/xen-command-line.markdown |   15 +++
 xen/arch/arm/domain.c               |    2 +-
 xen/arch/arm/guestcopy.c            |    2 +-
 xen/arch/arm/kernel.c               |    2 +-
 xen/arch/arm/mm.c                   |   30 ++---
 xen/arch/arm/p2m.c                  |   30 +----
 xen/arch/arm/smpboot.c              |    2 +-
 xen/arch/x86/mm.c                   |    2 +-
 xen/common/compat/grant_table.c     |   20 +++-
 xen/common/grant_table.c            |  209 ++++++++++++++++++++++++++++-------
 xen/common/kernel.c                 |    2 -
 xen/drivers/passthrough/arm/smmu.c  |   42 +++++++
 xen/include/asm-arm/arm32/page.h    |    7 +-
 xen/include/asm-arm/arm64/page.h    |    7 +-
 xen/include/asm-arm/grant_table.h   |    5 +-
 xen/include/asm-arm/p2m.h           |    4 -
 xen/include/asm-arm/page.h          |   56 ++++++++--
 xen/include/asm-x86/flushtlb.h      |   16 +++
 xen/include/asm-x86/p2m.h           |    4 -
 xen/include/asm-x86/page.h          |    4 +-
 xen/include/public/features.h       |    4 +-
 xen/include/public/grant_table.h    |   20 ++++
 xen/include/xen/grant_table.h       |    2 +-
 xen/include/xlat.lst                |    1 +
 24 files changed, 361 insertions(+), 127 deletions(-)

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v5 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options
  2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
@ 2014-10-13 14:58 ` Stefano Stabellini
  2014-10-14 10:50   ` Jan Beulich
  2014-10-13 14:58 ` [PATCH v5 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_* Stefano Stabellini
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:58 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini

Introduce gnttab_max_maptrack_frames: a new Xen command line option to
specify the max number of maptrack frames per domain.
Deprecate the old gnttab_max_nr_frames and introduce gnttab_max_frames
instead, that doesn't affect the maptrack. Keep gnttab_max_nr_frames for
compatibility.

Rename internally max_nr_grant_frames to max_grant_frames to avoid
confusions.

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

---
Changes in v5:

- /max_nr_maptrack_frames/max_maptrack_frames/g;
- deprecate gnttab_max_nr_frames;
- introduce gnttab_max_frames;
- rename the max_nr_grant_frames variable to max_grant_frames.
---
 docs/misc/xen-command-line.markdown |   15 ++++++++++++
 xen/arch/arm/domain.c               |    2 +-
 xen/arch/arm/mm.c                   |    2 +-
 xen/arch/x86/mm.c                   |    2 +-
 xen/common/compat/grant_table.c     |   12 ++++-----
 xen/common/grant_table.c            |   46 ++++++++++++++++++++++-------------
 xen/include/asm-arm/grant_table.h   |    2 +-
 xen/include/xen/grant_table.h       |    2 +-
 8 files changed, 55 insertions(+), 28 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 389701a..9960ea1 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -611,8 +611,23 @@ Specify the serial parameters for the GDB stub.
 ### gnttab\_max\_nr\_frames
 > `= <integer>`
 
+*Deprecated*
+Use gnttab\_max\_frames and gnttab\_max\_maptrack\_frames instead.
+
+Specify the maximum number of frames per grant table operation and the
+maximum number of maptrack frames domain.
+
+### gnttab\_max\_frames
+> `= <integer>`
+
 Specify the maximum number of frames per grant table operation.
 
+### gnttab\_max\_maptrack\_frames
+> `= <integer>`
+
+Specify the maximum number of maptrack frames domain.
+The default value is 8 times gnttab_max_nr_frames.
+
 ### guest\_loglvl
 > `= <level>[/<rate-limited level>]` where level is `none | error | warning | info | debug | all`
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2b53931..57f09c3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -409,7 +409,7 @@ struct domain *alloc_domain_struct(void)
         return NULL;
 
     clear_page(d);
-    d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_nr_grant_frames);
+    d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_grant_frames);
     return d;
 }
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c5b48ef..675a51b 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1040,7 +1040,7 @@ int xenmem_add_to_physmap_one(
         else
         {
             if ( (idx >= nr_grant_frames(d->grant_table)) &&
-                    (idx < max_nr_grant_frames) )
+                    (idx < max_grant_frames) )
                 gnttab_grow_table(d, idx + 1);
 
             if ( idx < nr_grant_frames(d->grant_table) )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5b3f06f..fe6a6e3 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4539,7 +4539,7 @@ int xenmem_add_to_physmap_one(
             else
             {
                 if ( (idx >= nr_grant_frames(d->grant_table)) &&
-                     (idx < max_nr_grant_frames) )
+                     (idx < max_grant_frames) )
                     gnttab_grow_table(d, idx + 1);
 
                 if ( idx < nr_grant_frames(d->grant_table) )
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 7ebbbc1..2dc1e44 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -146,11 +146,11 @@ int compat_grant_table_op(unsigned int cmd,
                 unsigned int max_frame_list_size_in_page =
                     (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
                     sizeof(*nat.setup->frame_list.p);
-                if ( max_frame_list_size_in_page < max_nr_grant_frames )
+                if ( max_frame_list_size_in_page < max_grant_frames )
                 {
                     gdprintk(XENLOG_WARNING,
-                             "max_nr_grant_frames is too large (%u,%u)\n",
-                             max_nr_grant_frames, max_frame_list_size_in_page);
+                             "max_grant_frames is too large (%u,%u)\n",
+                             max_grant_frames, max_frame_list_size_in_page);
                     rc = -EINVAL;
                 }
                 else
@@ -284,11 +284,11 @@ int compat_grant_table_op(unsigned int cmd,
                 break;
             }
             if ( max_frame_list_size_in_pages <
-                 grant_to_status_frames(max_nr_grant_frames) )
+                 grant_to_status_frames(max_grant_frames) )
             {
                 gdprintk(XENLOG_WARNING,
-                         "grant_to_status_frames(max_nr_grant_frames) is too large (%u,%u)\n",
-                         grant_to_status_frames(max_nr_grant_frames),
+                         "grant_to_status_frames(max_grant_frames) is too large (%u,%u)\n",
+                         grant_to_status_frames(max_grant_frames),
                          max_frame_list_size_in_pages);
                 rc = -EINVAL;
                 break;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 23266c3..49efe10 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -24,6 +24,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <xen/ctype.h>
 #include <xen/err.h>
 #include <xen/iocap.h>
 #include <xen/lib.h>
@@ -41,8 +42,8 @@
 #include <asm/flushtlb.h>
 
 #ifndef max_nr_grant_frames
-unsigned int max_nr_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
-integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
+unsigned int max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+integer_param("gnttab_max_frames", max_grant_frames);
 #endif
 
 /* The maximum number of grant mappings is defined as a multiplier of the
@@ -51,6 +52,22 @@ integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
  */
 #define MAX_MAPTRACK_TO_GRANTS_RATIO 8
 
+static unsigned int max_maptrack_frames = DEFAULT_MAX_NR_GRANT_FRAMES *
+                                             MAX_MAPTRACK_TO_GRANTS_RATIO;
+integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
+
+/* this option is deprecated, use gnttab_max_frames and
+ * gnttab_max_maptrack_frames instead */
+static void __init parse_gnttab_max_nr_frames(const char *s)
+{
+    if ( isdigit(*s) )
+    {
+        max_grant_frames = simple_strtoul(s, &s, 0);
+        max_maptrack_frames = max_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO;
+    }
+}
+custom_param("gnttab_max_nr_frames", parse_gnttab_max_nr_frames);
+
 /*
  * The first two members of a grant entry are updated as a combined pair.
  * The following union allows that to happen in an endian-neutral fashion.
@@ -102,11 +119,6 @@ nr_maptrack_frames(struct grant_table *t)
     return t->maptrack_limit / MAPTRACK_PER_PAGE;
 }
 
-static unsigned inline int max_nr_maptrack_frames(void)
-{
-    return (max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO);
-}
-
 #define MAPTRACK_TAIL (~0u)
 
 #define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
@@ -164,7 +176,7 @@ num_act_frames_from_sha_frames(const unsigned int num)
 }
 
 #define max_nr_active_grant_frames \
-    num_act_frames_from_sha_frames(max_nr_grant_frames)
+    num_act_frames_from_sha_frames(max_grant_frames)
 
 static inline unsigned int
 nr_active_grant_frames(struct grant_table *gt)
@@ -271,7 +283,7 @@ get_maptrack_handle(
     while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
     {
         nr_frames = nr_maptrack_frames(lgt);
-        if ( nr_frames >= max_nr_maptrack_frames() )
+        if ( nr_frames >= max_maptrack_frames )
             break;
 
         new_mt = alloc_xenheap_page();
@@ -1265,7 +1277,7 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
     struct grant_table *gt = d->grant_table;
     unsigned int i;
 
-    ASSERT(req_nr_frames <= max_nr_grant_frames);
+    ASSERT(req_nr_frames <= max_grant_frames);
 
     gdprintk(XENLOG_INFO,
             "Expanding dom (%d) grant table from (%d) to (%d) frames.\n",
@@ -1338,11 +1350,11 @@ gnttab_setup_table(
         return -EFAULT;
     }
 
-    if ( unlikely(op.nr_frames > max_nr_grant_frames) )
+    if ( unlikely(op.nr_frames > max_grant_frames) )
     {
         gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table frames"
                 " per domain.\n",
-                max_nr_grant_frames);
+                max_grant_frames);
         op.status = GNTST_general_error;
         goto out1;
     }
@@ -1377,7 +1389,7 @@ gnttab_setup_table(
     {
         gdprintk(XENLOG_INFO,
                  "Expand grant table to %u failed. Current: %u Max: %u\n",
-                 op.nr_frames, nr_grant_frames(gt), max_nr_grant_frames);
+                 op.nr_frames, nr_grant_frames(gt), max_grant_frames);
         op.status = GNTST_general_error;
         goto out3;
     }
@@ -1438,7 +1450,7 @@ gnttab_query_size(
     spin_lock(&d->grant_table->lock);
 
     op.nr_frames     = nr_grant_frames(d->grant_table);
-    op.max_nr_frames = max_nr_grant_frames;
+    op.max_nr_frames = max_grant_frames;
     op.status        = GNTST_okay;
 
     spin_unlock(&d->grant_table->lock);
@@ -2659,7 +2671,7 @@ grant_table_create(
 
     /* Tracking of mapped foreign frames table */
     if ( (t->maptrack = xzalloc_array(struct grant_mapping *,
-                                      max_nr_maptrack_frames())) == NULL )
+                                      max_maptrack_frames)) == NULL )
         goto no_mem_2;
     if ( (t->maptrack[0] = alloc_xenheap_page()) == NULL )
         goto no_mem_3;
@@ -2670,7 +2682,7 @@ grant_table_create(
     t->maptrack[0][i - 1].ref = MAPTRACK_TAIL;
 
     /* Shared grant table. */
-    if ( (t->shared_raw = xzalloc_array(void *, max_nr_grant_frames)) == NULL )
+    if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
         goto no_mem_3;
     for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
     {
@@ -2681,7 +2693,7 @@ grant_table_create(
     
     /* Status pages for grant table - for version 2 */
     t->status = xzalloc_array(grant_status_t *,
-                              grant_to_status_frames(max_nr_grant_frames));
+                              grant_to_status_frames(max_grant_frames));
     if ( t->status == NULL )
         goto no_mem_4;
 
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 47147ce..e798880 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -31,7 +31,7 @@ static inline int replace_grant_supported(void)
 
 #define gnttab_shared_gmfn(d, t, i)                                      \
     ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
-     (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
+     (i < max_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
 
 #define gnttab_need_iommu_mapping(d)           (is_domain_direct_mapped(d))
 
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 5941191..9f3a1f3 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -51,7 +51,7 @@
 #endif
 #ifndef max_nr_grant_frames /* to allow arch to override */
 /* The maximum size of a grant table. */
-extern unsigned int max_nr_grant_frames;
+extern unsigned int max_grant_frames;
 #endif
 
 /*
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_*
  2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
  2014-10-13 14:58 ` [PATCH v5 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options Stefano Stabellini
@ 2014-10-13 14:58 ` Stefano Stabellini
  2014-10-13 15:36   ` Julien Grall
  2014-10-13 14:58 ` [PATCH v5 3/8] xen/arm: return int from flush_page_to_ram and *_dcache_va_range Stefano Stabellini
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:58 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/guestcopy.c         |    2 +-
 xen/arch/arm/kernel.c            |    2 +-
 xen/arch/arm/mm.c                |   24 ++++++++++++------------
 xen/arch/arm/p2m.c               |    4 ++--
 xen/arch/arm/smpboot.c           |    2 +-
 xen/include/asm-arm/arm32/page.h |    4 ++--
 xen/include/asm-arm/arm64/page.h |    4 ++--
 xen/include/asm-arm/page.h       |   20 ++++++++++----------
 8 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 0173597..7dbaeca 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -27,7 +27,7 @@ static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
         p += offset;
         memcpy(p, from, size);
         if ( flush_dcache )
-            clean_xen_dcache_va_range(p, size);
+            clean_dcache_va_range(p, size);
 
         unmap_domain_page(p - offset);
         put_page(page);
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 1b8ac9a..209c3dd 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -57,7 +57,7 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
 
         set_fixmap(FIXMAP_MISC, p, BUFFERABLE);
         memcpy(dst, src + s, l);
-        clean_xen_dcache_va_range(dst, l);
+        clean_dcache_va_range(dst, l);
 
         paddr += l;
         dst += l;
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 675a51b..996687b 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -381,7 +381,7 @@ void flush_page_to_ram(unsigned long mfn)
 {
     void *v = map_domain_page(mfn);
 
-    clean_and_invalidate_xen_dcache_va_range(v, PAGE_SIZE);
+    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
     unmap_domain_page(v);
 }
 
@@ -504,17 +504,17 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
     /* Clear the copy of the boot pagetables. Each secondary CPU
      * rebuilds these itself (see head.S) */
     memset(boot_pgtable, 0x0, PAGE_SIZE);
-    clean_and_invalidate_xen_dcache(boot_pgtable);
+    clean_and_invalidate_dcache(boot_pgtable);
 #ifdef CONFIG_ARM_64
     memset(boot_first, 0x0, PAGE_SIZE);
-    clean_and_invalidate_xen_dcache(boot_first);
+    clean_and_invalidate_dcache(boot_first);
     memset(boot_first_id, 0x0, PAGE_SIZE);
-    clean_and_invalidate_xen_dcache(boot_first_id);
+    clean_and_invalidate_dcache(boot_first_id);
 #endif
     memset(boot_second, 0x0, PAGE_SIZE);
-    clean_and_invalidate_xen_dcache(boot_second);
+    clean_and_invalidate_dcache(boot_second);
     memset(boot_third, 0x0, PAGE_SIZE);
-    clean_and_invalidate_xen_dcache(boot_third);
+    clean_and_invalidate_dcache(boot_third);
 
     /* Break up the Xen mapping into 4k pages and protect them separately. */
     for ( i = 0; i < LPAE_ENTRIES; i++ )
@@ -552,7 +552,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
 
     /* Make sure it is clear */
     memset(this_cpu(xen_dommap), 0, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
-    clean_xen_dcache_va_range(this_cpu(xen_dommap),
+    clean_dcache_va_range(this_cpu(xen_dommap),
                               DOMHEAP_SECOND_PAGES*PAGE_SIZE);
 #endif
 }
@@ -563,7 +563,7 @@ int init_secondary_pagetables(int cpu)
     /* Set init_ttbr for this CPU coming up. All CPus share a single setof
      * pagetables, but rewrite it each time for consistency with 32 bit. */
     init_ttbr = (uintptr_t) xen_pgtable + phys_offset;
-    clean_xen_dcache(init_ttbr);
+    clean_dcache(init_ttbr);
     return 0;
 }
 #else
@@ -598,15 +598,15 @@ int init_secondary_pagetables(int cpu)
         write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
     }
 
-    clean_xen_dcache_va_range(first, PAGE_SIZE);
-    clean_xen_dcache_va_range(domheap, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
+    clean_dcache_va_range(first, PAGE_SIZE);
+    clean_dcache_va_range(domheap, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
 
     per_cpu(xen_pgtable, cpu) = first;
     per_cpu(xen_dommap, cpu) = domheap;
 
     /* Set init_ttbr for this CPU coming up */
     init_ttbr = __pa(first);
-    clean_xen_dcache(init_ttbr);
+    clean_dcache(init_ttbr);
 
     return 0;
 }
@@ -1273,7 +1273,7 @@ void clear_and_clean_page(struct page_info *page)
     void *p = __map_domain_page(page);
 
     clear_page(p);
-    clean_xen_dcache_va_range(p, PAGE_SIZE);
+    clean_dcache_va_range(p, PAGE_SIZE);
     unmap_domain_page(p);
 }
 
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 70929fc..6ff8c17 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -302,7 +302,7 @@ static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool_t flush_cache)
 {
     write_pte(p, pte);
     if ( flush_cache )
-        clean_xen_dcache(*p);
+        clean_dcache(*p);
 }
 
 /*
@@ -362,7 +362,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
         clear_page(p);
 
     if ( flush_cache )
-        clean_xen_dcache_va_range(p, PAGE_SIZE);
+        clean_dcache_va_range(p, PAGE_SIZE);
 
     unmap_domain_page(p);
 
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index ee395a1..14054ae 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -374,7 +374,7 @@ int __cpu_up(unsigned int cpu)
 
     /* Open the gate for this CPU */
     smp_up_cpu = cpu_logical_map(cpu);
-    clean_xen_dcache(smp_up_cpu);
+    clean_dcache(smp_up_cpu);
 
     rc = arch_cpu_up(cpu);
 
diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index 9740672..20a6a7f 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -20,11 +20,11 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
 }
 
 /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
-#define __clean_xen_dcache_one(R) STORE_CP32(R, DCCMVAC)
+#define __clean_dcache_one(R) STORE_CP32(R, DCCMVAC)
 
 /* Inline ASM to clean and invalidate dcache on register R (may be an
  * inline asm operand) */
-#define __clean_and_invalidate_xen_dcache_one(R) STORE_CP32(R, DCCIMVAC)
+#define __clean_and_invalidate_dcache_one(R) STORE_CP32(R, DCCIMVAC)
 
 /*
  * Flush all hypervisor mappings from the TLB and branch predictor of
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index bb10164..101d7a8 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -15,11 +15,11 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
 }
 
 /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
-#define __clean_xen_dcache_one(R) "dc cvac, %" #R ";"
+#define __clean_dcache_one(R) "dc cvac, %" #R ";"
 
 /* Inline ASM to clean and invalidate dcache on register R (may be an
  * inline asm operand) */
-#define __clean_and_invalidate_xen_dcache_one(R) "dc  civac, %" #R ";"
+#define __clean_and_invalidate_dcache_one(R) "dc  civac, %" #R ";"
 
 /*
  * Flush all hypervisor mappings from the TLB of the local processor.
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d758b61..1327b00 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -268,48 +268,48 @@ extern size_t cacheline_bytes;
 /* Functions for flushing medium-sized areas.
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */
-static inline void clean_xen_dcache_va_range(const void *p, unsigned long size)
+static inline void clean_dcache_va_range(const void *p, unsigned long size)
 {
     const void *end;
     dsb(sy);           /* So the CPU issues all writes to the range */
     for ( end = p + size; p < end; p += cacheline_bytes )
-        asm volatile (__clean_xen_dcache_one(0) : : "r" (p));
+        asm volatile (__clean_dcache_one(0) : : "r" (p));
     dsb(sy);           /* So we know the flushes happen before continuing */
 }
 
-static inline void clean_and_invalidate_xen_dcache_va_range
+static inline void clean_and_invalidate_dcache_va_range
     (const void *p, unsigned long size)
 {
     const void *end;
     dsb(sy);         /* So the CPU issues all writes to the range */
     for ( end = p + size; p < end; p += cacheline_bytes )
-        asm volatile (__clean_and_invalidate_xen_dcache_one(0) : : "r" (p));
+        asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
     dsb(sy);         /* So we know the flushes happen before continuing */
 }
 
 /* Macros for flushing a single small item.  The predicate is always
  * compile-time constant so this will compile down to 3 instructions in
  * the common case. */
-#define clean_xen_dcache(x) do {                                        \
+#define clean_dcache(x) do {                                        \
     typeof(x) *_p = &(x);                                               \
     if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) )    \
-        clean_xen_dcache_va_range(_p, sizeof(x));                       \
+        clean_dcache_va_range(_p, sizeof(x));                       \
     else                                                                \
         asm volatile (                                                  \
             "dsb sy;"   /* Finish all earlier writes */                 \
-            __clean_xen_dcache_one(0)                                   \
+            __clean_dcache_one(0)                                   \
             "dsb sy;"   /* Finish flush before continuing */            \
             : : "r" (_p), "m" (*_p));                                   \
 } while (0)
 
-#define clean_and_invalidate_xen_dcache(x) do {                         \
+#define clean_and_invalidate_dcache(x) do {                         \
     typeof(x) *_p = &(x);                                               \
     if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) )    \
-        clean_and_invalidate_xen_dcache_va_range(_p, sizeof(x));        \
+        clean_and_invalidate_dcache_va_range(_p, sizeof(x));        \
     else                                                                \
         asm volatile (                                                  \
             "dsb sy;"   /* Finish all earlier writes */                 \
-            __clean_and_invalidate_xen_dcache_one(0)                    \
+            __clean_and_invalidate_dcache_one(0)                    \
             "dsb sy;"   /* Finish flush before continuing */            \
             : : "r" (_p), "m" (*_p));                                   \
 } while (0)
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 3/8] xen/arm: return int from flush_page_to_ram and *_dcache_va_range
  2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
  2014-10-13 14:58 ` [PATCH v5 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options Stefano Stabellini
  2014-10-13 14:58 ` [PATCH v5 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_* Stefano Stabellini
@ 2014-10-13 14:58 ` Stefano Stabellini
  2014-10-13 15:35   ` Julien Grall
  2014-10-13 14:58 ` [PATCH v5 4/8] xen/arm: introduce invalidate_dcache_va_range Stefano Stabellini
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:58 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini

These functions cannot really fail on ARM, but their x86 equivalents can
(-EOPNOTSUPP). Change the prototype to return int.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/mm.c          |    6 ++++--
 xen/include/asm-arm/page.h |    8 +++++---
 xen/include/asm-x86/page.h |    3 ++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 996687b..f6f20aa 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -377,12 +377,14 @@ unsigned long domain_page_map_to_mfn(const void *ptr)
 }
 #endif
 
-void flush_page_to_ram(unsigned long mfn)
+int flush_page_to_ram(unsigned long mfn)
 {
+    int ret;
     void *v = map_domain_page(mfn);
 
-    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
+    ret = clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
     unmap_domain_page(v);
+    return ret;
 }
 
 void __init arch_init_memory(void)
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 1327b00..6265c45 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -268,16 +268,17 @@ extern size_t cacheline_bytes;
 /* Functions for flushing medium-sized areas.
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */
-static inline void clean_dcache_va_range(const void *p, unsigned long size)
+static inline int clean_dcache_va_range(const void *p, unsigned long size)
 {
     const void *end;
     dsb(sy);           /* So the CPU issues all writes to the range */
     for ( end = p + size; p < end; p += cacheline_bytes )
         asm volatile (__clean_dcache_one(0) : : "r" (p));
     dsb(sy);           /* So we know the flushes happen before continuing */
+    return 0;
 }
 
-static inline void clean_and_invalidate_dcache_va_range
+static inline int clean_and_invalidate_dcache_va_range
     (const void *p, unsigned long size)
 {
     const void *end;
@@ -285,6 +286,7 @@ static inline void clean_and_invalidate_dcache_va_range
     for ( end = p + size; p < end; p += cacheline_bytes )
         asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
     dsb(sy);         /* So we know the flushes happen before continuing */
+    return 0;
 }
 
 /* Macros for flushing a single small item.  The predicate is always
@@ -353,7 +355,7 @@ static inline void flush_xen_data_tlb_range_va(unsigned long va,
 }
 
 /* Flush the dcache for an entire page. */
-void flush_page_to_ram(unsigned long mfn);
+int flush_page_to_ram(unsigned long mfn);
 
 /*
  * Print a walk of a page table or p2m
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 9aa780e..006e3fa 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -21,6 +21,7 @@
 #endif
 
 #include <asm/x86_64/page.h>
+#include <xen/errno.h>
 
 /* Read a pte atomically from memory. */
 #define l1e_read_atomic(l1ep) \
@@ -345,7 +346,7 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
 }
 
 /* No cache maintenance required on x86 architecture. */
-static inline void flush_page_to_ram(unsigned long mfn) {}
+static inline int flush_page_to_ram(unsigned long mfn) { return -EOPNOTSUPP; }
 
 /* return true if permission increased */
 static inline bool_t
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 4/8] xen/arm: introduce invalidate_dcache_va_range
  2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
                   ` (2 preceding siblings ...)
  2014-10-13 14:58 ` [PATCH v5 3/8] xen/arm: return int from flush_page_to_ram and *_dcache_va_range Stefano Stabellini
@ 2014-10-13 14:58 ` Stefano Stabellini
  2014-10-13 14:58 ` [PATCH v5 5/8] xen/x86: introduce more cache maintenance operations Stefano Stabellini
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:58 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini

Take care of handling non-cacheline aligned addresses and sizes.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---

Changes in v5:
- return int from invalidate_dcache_va_range.

Changes in v4:
- rename invalidate_xen_dcache_va_range to invalidate_dcache_va_range.
---
 xen/include/asm-arm/arm32/page.h |    3 +++
 xen/include/asm-arm/arm64/page.h |    3 +++
 xen/include/asm-arm/page.h       |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index 20a6a7f..a07e217 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -19,6 +19,9 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
         : : "r" (pte.bits), "r" (p) : "memory");
 }
 
+/* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */
+#define __invalidate_dcache_one(R) STORE_CP32(R, DCIMVAC)
+
 /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
 #define __clean_dcache_one(R) STORE_CP32(R, DCCMVAC)
 
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index 101d7a8..1fd416d 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -14,6 +14,9 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
         : : "r" (pte.bits), "r" (p) : "memory");
 }
 
+/* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */
+#define __invalidate_dcache_one(R) "dc ivac, %" #R ";"
+
 /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
 #define __clean_dcache_one(R) "dc cvac, %" #R ";"
 
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 6265c45..33fbe66 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -268,6 +268,38 @@ extern size_t cacheline_bytes;
 /* Functions for flushing medium-sized areas.
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */
+
+static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
+{
+    size_t off;
+    const void *end = p + size;
+
+    dsb(sy);           /* So the CPU issues all writes to the range */
+
+    off = (unsigned long)p % cacheline_bytes;
+    if ( off )
+    {
+        p -= off;
+        asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
+        p += cacheline_bytes;
+        size -= cacheline_bytes - off;
+    }
+    off = (unsigned long)end % cacheline_bytes;
+    if ( off )
+    {
+        end -= off;
+        size -= off;
+        asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (end));
+    }
+
+    for ( ; p < end; p += cacheline_bytes )
+        asm volatile (__invalidate_dcache_one(0) : : "r" (p));
+
+    dsb(sy);           /* So we know the flushes happen before continuing */
+
+    return 0;
+}
+
 static inline int clean_dcache_va_range(const void *p, unsigned long size)
 {
     const void *end;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 5/8] xen/x86: introduce more cache maintenance operations
  2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
                   ` (3 preceding siblings ...)
  2014-10-13 14:58 ` [PATCH v5 4/8] xen/arm: introduce invalidate_dcache_va_range Stefano Stabellini
@ 2014-10-13 14:58 ` Stefano Stabellini
  2014-10-14 10:54   ` Jan Beulich
  2014-10-13 14:58 ` [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:58 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini

Move the existing flush_page_to_ram flushtlb.h.

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

---

Changes in v5:
- make order an unsigned int;
- add a comment on sub-page granularity support;
- cache operations return error;
- move the functions to xen/include/asm-x86/flushtlb.h.

Changes in v4:
- remove _xen in the function names;
- implement the functions using existing x86 flushing functions.
---
 xen/include/asm-x86/flushtlb.h |   16 ++++++++++++++++
 xen/include/asm-x86/page.h     |    3 ---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-x86/flushtlb.h b/xen/include/asm-x86/flushtlb.h
index 7f46632..3ced2fb 100644
--- a/xen/include/asm-x86/flushtlb.h
+++ b/xen/include/asm-x86/flushtlb.h
@@ -11,6 +11,7 @@
 #define __FLUSHTLB_H__
 
 #include <xen/config.h>
+#include <xen/mm.h>
 #include <xen/percpu.h>
 #include <xen/smp.h>
 #include <xen/types.h>
@@ -115,4 +116,19 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags);
 #define flush_tlb_one_all(v)                    \
     flush_tlb_one_mask(&cpu_online_map, v)
 
+static inline int flush_page_to_ram(unsigned long mfn) { return -EOPNOTSUPP; }
+static inline int invalidate_dcache_va_range(const void *p, unsigned long size) { return -EOPNOTSUPP; }
+static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
+{
+    unsigned int order = get_order_from_bytes(size);
+    /* sub-page granularity support needs to be added if necessary */
+    flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
+    return 0;
+}
+static inline int clean_dcache_va_range(const void *p, unsigned long size)
+{
+    clean_and_invalidate_dcache_va_range(p, size);
+    return 0;
+}
+
 #endif /* __FLUSHTLB_H__ */
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 006e3fa..8ca6923 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -345,9 +345,6 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
     return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3);
 }
 
-/* No cache maintenance required on x86 architecture. */
-static inline int flush_page_to_ram(unsigned long mfn) { return -EOPNOTSUPP; }
-
 /* return true if permission increased */
 static inline bool_t
 perms_strictly_increased(uint32_t old_flags, uint32_t new_flags)
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush
  2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
                   ` (4 preceding siblings ...)
  2014-10-13 14:58 ` [PATCH v5 5/8] xen/x86: introduce more cache maintenance operations Stefano Stabellini
@ 2014-10-13 14:58 ` Stefano Stabellini
  2014-10-14 11:00   ` Jan Beulich
  2014-10-13 14:58 ` [PATCH v5 7/8] Revert "xen/arm: introduce XENFEAT_grant_map_identity" Stefano Stabellini
  2014-10-13 14:58 ` [PATCH v5 8/8] Revert "xen: introduce arch_grant_(un)map_page_identity" Stefano Stabellini
  7 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:58 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini

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.

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

---

Changes in v5:
- make 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  |    8 +++
 xen/common/grant_table.c         |  133 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/grant_table.h |   20 ++++++
 xen/include/xlat.lst             |    1 +
 4 files changed, 162 insertions(+)

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 2dc1e44..5130c35 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_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)
@@ -106,6 +110,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);
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 49efe10..1371e6d 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -496,6 +496,34 @@ static int _set_status(unsigned gt_version,
         return _set_status_v2(domid, readonly, mapflag, shah, act, status);
 }
 
+static bool_t grant_map_exists(const struct domain *ld,
+                        struct grant_table *rgt,
+                        unsigned long mfn)
+{
+    const struct active_grant_entry *act;
+    grant_ref_t ref;
+
+    ASSERT(spin_is_locked(&rgt->lock));
+
+    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
+    {
+        act = &active_entry(rgt, ref);
+
+        if ( !act->pin )
+            continue;
+
+        if ( act->domid != ld->domain_id )
+            continue;
+
+        if ( act->frame != mfn )
+            continue;
+
+        return 1;
+    }
+
+    return 0;
+}
+
 static void mapcount(
     struct grant_table *lgt, struct domain *rd, unsigned long mfn,
     unsigned int *wrc, unsigned int *rdc)
@@ -2494,6 +2522,97 @@ 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)
+{
+    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);
+
+        if ( !grant_map_exists(d, owner->grant_table, mfn) )
+        {
+            spin_unlock(&owner->grant_table->lock);
+            rcu_unlock_domain(d);
+            put_page(page);
+            return -EINVAL;
+        }
+    }
+
+    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,
+                      unsigned int count)
+{
+    int ret;
+    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;
+        ret = __gnttab_cache_flush(&op);
+        if ( ret < 0 )
+            return ret;
+        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)
@@ -2623,6 +2742,20 @@ do_grant_table_op(
         }
         break;
     }
+    case GNTTABOP_cache_flush:
+    {
+        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, count);
+        if ( rc > 0 )
+        {
+            guest_handle_add_offset(cflush, rc);
+            uop = guest_handle_cast(cflush, void);
+        }
+        break;
+    }
     default:
         rc = -ENOSYS;
         break;
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index b8a3d6c..20d4e77 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_cache_flush	      12
 #endif /* __XEN_INTERFACE_VERSION__ */
 /* ` } */
 
@@ -574,6 +575,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/xlat.lst b/xen/include/xlat.lst
index 9a35dd7..3822b00 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
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 7/8] Revert "xen/arm: introduce XENFEAT_grant_map_identity"
  2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
                   ` (5 preceding siblings ...)
  2014-10-13 14:58 ` [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
@ 2014-10-13 14:58 ` Stefano Stabellini
  2014-10-13 14:58 ` [PATCH v5 8/8] Revert "xen: introduce arch_grant_(un)map_page_identity" Stefano Stabellini
  7 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:58 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini

Revert commit id 8d09ef6906ca0a9957e21334ad2c3eed626abe63.
Just keep the definition of XENFEAT_grant_map_identity.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

---

Changes in v2:

- comment out the definition of XENFEAT_grant_map_identity.
---
 xen/common/grant_table.c           |   30 +++++-------------------------
 xen/common/kernel.c                |    2 --
 xen/drivers/passthrough/arm/smmu.c |   33 +++++++++++++++++++++++++++++++++
 xen/include/asm-arm/grant_table.h  |    3 ++-
 xen/include/public/features.h      |    4 +++-
 5 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 1371e6d..9cdd422 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -778,23 +778,13 @@ __gnttab_map_grant_ref(
              !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
         {
             if ( wrc == 0 )
-            {
-                if ( is_domain_direct_mapped(ld) )
-                    err = arch_grant_map_page_identity(ld, frame, 1);
-                else
-                    err = iommu_map_page(ld, frame, frame,
-                            IOMMUF_readable|IOMMUF_writable);
-            }
+                err = iommu_map_page(ld, frame, frame,
+                                     IOMMUF_readable|IOMMUF_writable);
         }
         else if ( act_pin && !old_pin )
         {
             if ( (wrc + rdc) == 0 )
-            {
-                if ( is_domain_direct_mapped(ld) )
-                    err = arch_grant_map_page_identity(ld, frame, 0);
-                else
-                    err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
-            }
+                err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
         }
         if ( err )
         {
@@ -991,19 +981,9 @@ __gnttab_unmap_common(
         int err = 0;
         mapcount(lgt, rd, op->frame, &wrc, &rdc);
         if ( (wrc + rdc) == 0 )
-        {
-            if ( is_domain_direct_mapped(ld) )
-                err = arch_grant_unmap_page_identity(ld, op->frame);
-            else
-                err = iommu_unmap_page(ld, op->frame);
-        }
+            err = iommu_unmap_page(ld, op->frame);
         else if ( wrc == 0 )
-        {
-            if ( is_domain_direct_mapped(ld) )
-                err = arch_grant_map_page_identity(ld, op->frame, 0);
-            else
-                err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
-        }
+            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
         if ( err )
         {
             rc = GNTST_general_error;
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index ce65486..d23c422 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -332,8 +332,6 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                 break;
             }
 #endif
-            if ( is_domain_direct_mapped(d) )
-                fi.submap |= 1U << XENFEAT_grant_map_identity;
             break;
         default:
             return -EINVAL;
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 3cbd206..9a95ac9 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1536,6 +1536,37 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
     xfree(smmu_domain);
 }
 
+static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
+                             unsigned long mfn, unsigned int flags)
+{
+    /* Grant mappings can be used for DMA requests. The dev_bus_addr returned by
+     * the hypercall is the MFN (not the IPA). For device protected by
+     * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
+     * allow DMA request to work.
+     * This is only valid when the domain is directed mapped. Hence this
+     * function should only be used by gnttab code with gfn == mfn.
+     */
+    BUG_ON(!is_domain_direct_mapped(d));
+    BUG_ON(mfn != gfn);
+
+    /* We only support readable and writable flags */
+    if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
+        return -EINVAL;
+
+    return arch_grant_map_page_identity(d, mfn, flags & IOMMUF_writable);
+}
+
+static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
+{
+    /* This function should only be used by gnttab code when the domain
+     * is direct mapped
+     */
+    if ( !is_domain_direct_mapped(d) )
+        return -EINVAL;
+
+    return arch_grant_unmap_page_identity(d, gfn);
+}
+
 static const struct iommu_ops arm_smmu_iommu_ops = {
     .init = arm_smmu_iommu_domain_init,
     .hwdom_init = arm_smmu_iommu_hwdom_init,
@@ -1544,6 +1575,8 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
     .assign_dt_device = arm_smmu_attach_dev,
     .reassign_dt_device = arm_smmu_reassign_dt_dev,
+    .map_page = arm_smmu_map_page,
+    .unmap_page = arm_smmu_unmap_page,
 };
 
 static int __init smmu_init(struct dt_device_node *dev,
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index e798880..0edad67 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -33,7 +33,8 @@ static inline int replace_grant_supported(void)
     ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
      (i < max_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
 
-#define gnttab_need_iommu_mapping(d)           (is_domain_direct_mapped(d))
+#define gnttab_need_iommu_mapping(d)                    \
+    (is_domain_direct_mapped(d) && need_iommu(d))
 
 #endif /* __ASM_GRANT_TABLE_H__ */
 /*
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index b7bf83f..16d92aa 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -94,8 +94,10 @@
 /* operation as Dom0 is supported */
 #define XENFEAT_dom0                      11
 
-/* Xen also maps grant references at pfn = mfn */
+/* Xen also maps grant references at pfn = mfn.
+ * This feature flag is deprecated and should not be used.
 #define XENFEAT_grant_map_identity        12
+ */
 
 #define XENFEAT_NR_SUBMAPS 1
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v5 8/8] Revert "xen: introduce arch_grant_(un)map_page_identity"
  2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
                   ` (6 preceding siblings ...)
  2014-10-13 14:58 ` [PATCH v5 7/8] Revert "xen/arm: introduce XENFEAT_grant_map_identity" Stefano Stabellini
@ 2014-10-13 14:58 ` Stefano Stabellini
  7 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-13 14:58 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini

This reverts commit e25a5f4d8cf3b55718048abdd21c7d0de64ae54c.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/p2m.c                 |   26 --------------------------
 xen/drivers/passthrough/arm/smmu.c |   13 +++++++++++--
 xen/include/asm-arm/p2m.h          |    4 ----
 xen/include/asm-x86/p2m.h          |    4 ----
 4 files changed, 11 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6ff8c17..acad2f2 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -931,32 +931,6 @@ void guest_physmap_remove_page(struct domain *d,
                       pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
 }
 
-int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
-                                 bool_t writeable)
-{
-    p2m_type_t t;
-
-    ASSERT(is_domain_direct_mapped(d));
-
-    /* This is not an IOMMU mapping but it is not a regular RAM p2m type
-     * either. We are using IOMMU p2m types here to prevent the pages
-     * from being used as grants. */
-    if ( writeable )
-        t = p2m_iommu_map_rw;
-    else
-        t = p2m_iommu_map_ro;
-
-    return guest_physmap_add_entry(d, frame, frame, 0, t);
-}
-
-int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame)
-{
-    ASSERT(is_domain_direct_mapped(d));
-
-    guest_physmap_remove_page(d, frame, frame, 0);
-    return 0;
-}
-
 int p2m_alloc_table(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 9a95ac9..42bde75 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1539,6 +1539,8 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
 static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
                              unsigned long mfn, unsigned int flags)
 {
+    p2m_type_t t;
+
     /* Grant mappings can be used for DMA requests. The dev_bus_addr returned by
      * the hypercall is the MFN (not the IPA). For device protected by
      * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
@@ -1553,7 +1555,12 @@ static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
     if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
         return -EINVAL;
 
-    return arch_grant_map_page_identity(d, mfn, flags & IOMMUF_writable);
+    t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
+
+    /* The function guest_physmap_add_entry replaces the current mapping
+     * if there is already one...
+     */
+    return guest_physmap_add_entry(d, gfn, mfn, 0, t);
 }
 
 static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
@@ -1564,7 +1571,9 @@ static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
     if ( !is_domain_direct_mapped(d) )
         return -EINVAL;
 
-    return arch_grant_unmap_page_identity(d, gfn);
+    guest_physmap_remove_page(d, gfn, gfn, 0);
+
+    return 0;
 }
 
 static const struct iommu_ops arm_smmu_iommu_ops = {
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 10bf111..da36504 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -214,10 +214,6 @@ static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
-int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
-                                 bool_t writeable);
-int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame);
-
 /* get host p2m table */
 #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 90ddd15..5f7fe71 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -709,10 +709,6 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
     return flags;
 }
 
-extern int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
-                                 bool_t writeable);
-extern int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame);
-
 #endif /* _XEN_P2M_H */
 
 /*
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/8] xen/arm: return int from flush_page_to_ram and *_dcache_va_range
  2014-10-13 14:58 ` [PATCH v5 3/8] xen/arm: return int from flush_page_to_ram and *_dcache_va_range Stefano Stabellini
@ 2014-10-13 15:35   ` Julien Grall
  2014-10-16 11:03     ` Stefano Stabellini
  0 siblings, 1 reply; 19+ messages in thread
From: Julien Grall @ 2014-10-13 15:35 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: julien.grall, Ian.Campbell

On 10/13/2014 03:58 PM, Stefano Stabellini wrote:
> These functions cannot really fail on ARM, but their x86 equivalents can
> (-EOPNOTSUPP). Change the prototype to return int.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/mm.c          |    6 ++++--
>  xen/include/asm-arm/page.h |    8 +++++---
>  xen/include/asm-x86/page.h |    3 ++-
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 996687b..f6f20aa 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -377,12 +377,14 @@ unsigned long domain_page_map_to_mfn(const void *ptr)
>  }
>  #endif
>  
> -void flush_page_to_ram(unsigned long mfn)
> +int flush_page_to_ram(unsigned long mfn)

If you decide to change the prototype of flush_page_to_ram, you need to
check the return on every call-site.

IHMO, as this function is implemented as a no-op on x86, I would keep
the void and add an ASSERT below to check ret.

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_*
  2014-10-13 14:58 ` [PATCH v5 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_* Stefano Stabellini
@ 2014-10-13 15:36   ` Julien Grall
  0 siblings, 0 replies; 19+ messages in thread
From: Julien Grall @ 2014-10-13 15:36 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: julien.grall, Ian.Campbell

Hi Stefano,

On 10/13/2014 03:58 PM, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

I guess this patch is just mechanical changes and has been build tested
on both arm32 and arm64, so:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

> ---
>  xen/arch/arm/guestcopy.c         |    2 +-
>  xen/arch/arm/kernel.c            |    2 +-
>  xen/arch/arm/mm.c                |   24 ++++++++++++------------
>  xen/arch/arm/p2m.c               |    4 ++--
>  xen/arch/arm/smpboot.c           |    2 +-
>  xen/include/asm-arm/arm32/page.h |    4 ++--
>  xen/include/asm-arm/arm64/page.h |    4 ++--
>  xen/include/asm-arm/page.h       |   20 ++++++++++----------
>  8 files changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 0173597..7dbaeca 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -27,7 +27,7 @@ static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
>          p += offset;
>          memcpy(p, from, size);
>          if ( flush_dcache )
> -            clean_xen_dcache_va_range(p, size);
> +            clean_dcache_va_range(p, size);
>  
>          unmap_domain_page(p - offset);
>          put_page(page);
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 1b8ac9a..209c3dd 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -57,7 +57,7 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
>  
>          set_fixmap(FIXMAP_MISC, p, BUFFERABLE);
>          memcpy(dst, src + s, l);
> -        clean_xen_dcache_va_range(dst, l);
> +        clean_dcache_va_range(dst, l);
>  
>          paddr += l;
>          dst += l;
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 675a51b..996687b 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -381,7 +381,7 @@ void flush_page_to_ram(unsigned long mfn)
>  {
>      void *v = map_domain_page(mfn);
>  
> -    clean_and_invalidate_xen_dcache_va_range(v, PAGE_SIZE);
> +    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
>      unmap_domain_page(v);
>  }
>  
> @@ -504,17 +504,17 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>      /* Clear the copy of the boot pagetables. Each secondary CPU
>       * rebuilds these itself (see head.S) */
>      memset(boot_pgtable, 0x0, PAGE_SIZE);
> -    clean_and_invalidate_xen_dcache(boot_pgtable);
> +    clean_and_invalidate_dcache(boot_pgtable);
>  #ifdef CONFIG_ARM_64
>      memset(boot_first, 0x0, PAGE_SIZE);
> -    clean_and_invalidate_xen_dcache(boot_first);
> +    clean_and_invalidate_dcache(boot_first);
>      memset(boot_first_id, 0x0, PAGE_SIZE);
> -    clean_and_invalidate_xen_dcache(boot_first_id);
> +    clean_and_invalidate_dcache(boot_first_id);
>  #endif
>      memset(boot_second, 0x0, PAGE_SIZE);
> -    clean_and_invalidate_xen_dcache(boot_second);
> +    clean_and_invalidate_dcache(boot_second);
>      memset(boot_third, 0x0, PAGE_SIZE);
> -    clean_and_invalidate_xen_dcache(boot_third);
> +    clean_and_invalidate_dcache(boot_third);
>  
>      /* Break up the Xen mapping into 4k pages and protect them separately. */
>      for ( i = 0; i < LPAE_ENTRIES; i++ )
> @@ -552,7 +552,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>  
>      /* Make sure it is clear */
>      memset(this_cpu(xen_dommap), 0, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
> -    clean_xen_dcache_va_range(this_cpu(xen_dommap),
> +    clean_dcache_va_range(this_cpu(xen_dommap),
>                                DOMHEAP_SECOND_PAGES*PAGE_SIZE);
>  #endif
>  }
> @@ -563,7 +563,7 @@ int init_secondary_pagetables(int cpu)
>      /* Set init_ttbr for this CPU coming up. All CPus share a single setof
>       * pagetables, but rewrite it each time for consistency with 32 bit. */
>      init_ttbr = (uintptr_t) xen_pgtable + phys_offset;
> -    clean_xen_dcache(init_ttbr);
> +    clean_dcache(init_ttbr);
>      return 0;
>  }
>  #else
> @@ -598,15 +598,15 @@ int init_secondary_pagetables(int cpu)
>          write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
>      }
>  
> -    clean_xen_dcache_va_range(first, PAGE_SIZE);
> -    clean_xen_dcache_va_range(domheap, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
> +    clean_dcache_va_range(first, PAGE_SIZE);
> +    clean_dcache_va_range(domheap, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
>  
>      per_cpu(xen_pgtable, cpu) = first;
>      per_cpu(xen_dommap, cpu) = domheap;
>  
>      /* Set init_ttbr for this CPU coming up */
>      init_ttbr = __pa(first);
> -    clean_xen_dcache(init_ttbr);
> +    clean_dcache(init_ttbr);
>  
>      return 0;
>  }
> @@ -1273,7 +1273,7 @@ void clear_and_clean_page(struct page_info *page)
>      void *p = __map_domain_page(page);
>  
>      clear_page(p);
> -    clean_xen_dcache_va_range(p, PAGE_SIZE);
> +    clean_dcache_va_range(p, PAGE_SIZE);
>      unmap_domain_page(p);
>  }
>  
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 70929fc..6ff8c17 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -302,7 +302,7 @@ static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool_t flush_cache)
>  {
>      write_pte(p, pte);
>      if ( flush_cache )
> -        clean_xen_dcache(*p);
> +        clean_dcache(*p);
>  }
>  
>  /*
> @@ -362,7 +362,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
>          clear_page(p);
>  
>      if ( flush_cache )
> -        clean_xen_dcache_va_range(p, PAGE_SIZE);
> +        clean_dcache_va_range(p, PAGE_SIZE);
>  
>      unmap_domain_page(p);
>  
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index ee395a1..14054ae 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -374,7 +374,7 @@ int __cpu_up(unsigned int cpu)
>  
>      /* Open the gate for this CPU */
>      smp_up_cpu = cpu_logical_map(cpu);
> -    clean_xen_dcache(smp_up_cpu);
> +    clean_dcache(smp_up_cpu);
>  
>      rc = arch_cpu_up(cpu);
>  
> diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
> index 9740672..20a6a7f 100644
> --- a/xen/include/asm-arm/arm32/page.h
> +++ b/xen/include/asm-arm/arm32/page.h
> @@ -20,11 +20,11 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
>  }
>  
>  /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
> -#define __clean_xen_dcache_one(R) STORE_CP32(R, DCCMVAC)
> +#define __clean_dcache_one(R) STORE_CP32(R, DCCMVAC)
>  
>  /* Inline ASM to clean and invalidate dcache on register R (may be an
>   * inline asm operand) */
> -#define __clean_and_invalidate_xen_dcache_one(R) STORE_CP32(R, DCCIMVAC)
> +#define __clean_and_invalidate_dcache_one(R) STORE_CP32(R, DCCIMVAC)
>  
>  /*
>   * Flush all hypervisor mappings from the TLB and branch predictor of
> diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
> index bb10164..101d7a8 100644
> --- a/xen/include/asm-arm/arm64/page.h
> +++ b/xen/include/asm-arm/arm64/page.h
> @@ -15,11 +15,11 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
>  }
>  
>  /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
> -#define __clean_xen_dcache_one(R) "dc cvac, %" #R ";"
> +#define __clean_dcache_one(R) "dc cvac, %" #R ";"
>  
>  /* Inline ASM to clean and invalidate dcache on register R (may be an
>   * inline asm operand) */
> -#define __clean_and_invalidate_xen_dcache_one(R) "dc  civac, %" #R ";"
> +#define __clean_and_invalidate_dcache_one(R) "dc  civac, %" #R ";"
>  
>  /*
>   * Flush all hypervisor mappings from the TLB of the local processor.
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index d758b61..1327b00 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -268,48 +268,48 @@ extern size_t cacheline_bytes;
>  /* Functions for flushing medium-sized areas.
>   * if 'range' is large enough we might want to use model-specific
>   * full-cache flushes. */
> -static inline void clean_xen_dcache_va_range(const void *p, unsigned long size)
> +static inline void clean_dcache_va_range(const void *p, unsigned long size)
>  {
>      const void *end;
>      dsb(sy);           /* So the CPU issues all writes to the range */
>      for ( end = p + size; p < end; p += cacheline_bytes )
> -        asm volatile (__clean_xen_dcache_one(0) : : "r" (p));
> +        asm volatile (__clean_dcache_one(0) : : "r" (p));
>      dsb(sy);           /* So we know the flushes happen before continuing */
>  }
>  
> -static inline void clean_and_invalidate_xen_dcache_va_range
> +static inline void clean_and_invalidate_dcache_va_range
>      (const void *p, unsigned long size)
>  {
>      const void *end;
>      dsb(sy);         /* So the CPU issues all writes to the range */
>      for ( end = p + size; p < end; p += cacheline_bytes )
> -        asm volatile (__clean_and_invalidate_xen_dcache_one(0) : : "r" (p));
> +        asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
>      dsb(sy);         /* So we know the flushes happen before continuing */
>  }
>  
>  /* Macros for flushing a single small item.  The predicate is always
>   * compile-time constant so this will compile down to 3 instructions in
>   * the common case. */
> -#define clean_xen_dcache(x) do {                                        \
> +#define clean_dcache(x) do {                                        \
>      typeof(x) *_p = &(x);                                               \
>      if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) )    \
> -        clean_xen_dcache_va_range(_p, sizeof(x));                       \
> +        clean_dcache_va_range(_p, sizeof(x));                       \
>      else                                                                \
>          asm volatile (                                                  \
>              "dsb sy;"   /* Finish all earlier writes */                 \
> -            __clean_xen_dcache_one(0)                                   \
> +            __clean_dcache_one(0)                                   \
>              "dsb sy;"   /* Finish flush before continuing */            \
>              : : "r" (_p), "m" (*_p));                                   \
>  } while (0)
>  
> -#define clean_and_invalidate_xen_dcache(x) do {                         \
> +#define clean_and_invalidate_dcache(x) do {                         \
>      typeof(x) *_p = &(x);                                               \
>      if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) )    \
> -        clean_and_invalidate_xen_dcache_va_range(_p, sizeof(x));        \
> +        clean_and_invalidate_dcache_va_range(_p, sizeof(x));        \
>      else                                                                \
>          asm volatile (                                                  \
>              "dsb sy;"   /* Finish all earlier writes */                 \
> -            __clean_and_invalidate_xen_dcache_one(0)                    \
> +            __clean_and_invalidate_dcache_one(0)                    \
>              "dsb sy;"   /* Finish flush before continuing */            \
>              : : "r" (_p), "m" (*_p));                                   \
>  } while (0)
> 


-- 
Julien Grall

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options
  2014-10-13 14:58 ` [PATCH v5 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options Stefano Stabellini
@ 2014-10-14 10:50   ` Jan Beulich
  0 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2014-10-14 10:50 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell

>>> On 13.10.14 at 16:58, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -611,8 +611,23 @@ Specify the serial parameters for the GDB stub.
>  ### gnttab\_max\_nr\_frames
>  > `= <integer>`
>  
> +*Deprecated*
> +Use gnttab\_max\_frames and gnttab\_max\_maptrack\_frames instead.
> +
> +Specify the maximum number of frames per grant table operation and the
> +maximum number of maptrack frames domain.
> +
> +### gnttab\_max\_frames
> +> `= <integer>`
> +
>  Specify the maximum number of frames per grant table operation.
>  
> +### gnttab\_max\_maptrack\_frames
> +> `= <integer>`
> +
> +Specify the maximum number of maptrack frames domain.
> +The default value is 8 times gnttab_max_nr_frames.
> +

Please don't break the alphabetical sorting in this file (i.e. the two
new options should go ahead of the deprecated old one).

I also question this latter new option referencing the deprecated
one.

> +/* this option is deprecated, use gnttab_max_frames and
> + * gnttab_max_maptrack_frames instead */
> +static void __init parse_gnttab_max_nr_frames(const char *s)
> +{
> +    if ( isdigit(*s) )
> +    {
> +        max_grant_frames = simple_strtoul(s, &s, 0);
> +        max_maptrack_frames = max_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO;
> +    }
> +}
> +custom_param("gnttab_max_nr_frames", parse_gnttab_max_nr_frames);

Hmm, that's not really how I think (and outlined) this should work.
Considering that setting both of the to be controlled variables to
zero is meaningless, have _3_ variables each controlled by the
respective integer_param() command line option, and each zero
initialized. Then in an __initcall() handler (I think that's early
enough, but please double check) check which of the two new
variables are still zero, and set them according to the deprecated
option's value if so. And of course warn if the deprecated option
is being used.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 5/8] xen/x86: introduce more cache maintenance operations
  2014-10-13 14:58 ` [PATCH v5 5/8] xen/x86: introduce more cache maintenance operations Stefano Stabellini
@ 2014-10-14 10:54   ` Jan Beulich
  0 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2014-10-14 10:54 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell

>>> On 13.10.14 at 16:58, <stefano.stabellini@eu.citrix.com> wrote:
> Move the existing flush_page_to_ram flushtlb.h.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
provided you ...

> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
> +{
> +    unsigned int order = get_order_from_bytes(size);
> +    /* sub-page granularity support needs to be added if necessary */
> +    flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
> +    return 0;
> +}
> +static inline int clean_dcache_va_range(const void *p, unsigned long size)
> +{
> +    clean_and_invalidate_dcache_va_range(p, size);
> +    return 0;

... either make this return the called function's return value or make
the whole thing a simple, object-like macro.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush
  2014-10-13 14:58 ` [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
@ 2014-10-14 11:00   ` Jan Beulich
  2014-10-16 10:55     ` Stefano Stabellini
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2014-10-14 11:00 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell

>>> On 13.10.14 at 16:58, <stefano.stabellini@eu.citrix.com> wrote:
> +static bool_t grant_map_exists(const struct domain *ld,
> +                        struct grant_table *rgt,
> +                        unsigned long mfn)
> +{
> +    const struct active_grant_entry *act;
> +    grant_ref_t ref;
> +
> +    ASSERT(spin_is_locked(&rgt->lock));
> +
> +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )

Unless I overlooked something here or in an earlier patch I think
you're _still_ not enforcing a suitably low upper limit on the
variable that controls the maximum iteration count here. Whether
you enforce this the hard way (by just capping an eventual
command line specified value) or the soft way (by issuing a
warning that such a setting may hamper security, plus
documenting the supported value range) is up to you.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush
  2014-10-14 11:00   ` Jan Beulich
@ 2014-10-16 10:55     ` Stefano Stabellini
  2014-10-16 12:05       ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-16 10:55 UTC (permalink / raw)
  To: Jan Beulich; +Cc: julien.grall, xen-devel, Ian.Campbell, Stefano Stabellini

On Tue, 14 Oct 2014, Jan Beulich wrote:
> >>> On 13.10.14 at 16:58, <stefano.stabellini@eu.citrix.com> wrote:
> > +static bool_t grant_map_exists(const struct domain *ld,
> > +                        struct grant_table *rgt,
> > +                        unsigned long mfn)
> > +{
> > +    const struct active_grant_entry *act;
> > +    grant_ref_t ref;
> > +
> > +    ASSERT(spin_is_locked(&rgt->lock));
> > +
> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
> 
> Unless I overlooked something here or in an earlier patch I think
> you're _still_ not enforcing a suitably low upper limit on the
> variable that controls the maximum iteration count here. Whether
> you enforce this the hard way (by just capping an eventual
> command line specified value) or the soft way (by issuing a
> warning that such a setting may hamper security, plus
> documenting the supported value range) is up to you.

What value range do you suggest?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 3/8] xen/arm: return int from flush_page_to_ram and *_dcache_va_range
  2014-10-13 15:35   ` Julien Grall
@ 2014-10-16 11:03     ` Stefano Stabellini
  0 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-16 11:03 UTC (permalink / raw)
  To: Julien Grall; +Cc: julien.grall, xen-devel, Ian.Campbell, Stefano Stabellini

On Mon, 13 Oct 2014, Julien Grall wrote:
> On 10/13/2014 03:58 PM, Stefano Stabellini wrote:
> > These functions cannot really fail on ARM, but their x86 equivalents can
> > (-EOPNOTSUPP). Change the prototype to return int.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/mm.c          |    6 ++++--
> >  xen/include/asm-arm/page.h |    8 +++++---
> >  xen/include/asm-x86/page.h |    3 ++-
> >  3 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 996687b..f6f20aa 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -377,12 +377,14 @@ unsigned long domain_page_map_to_mfn(const void *ptr)
> >  }
> >  #endif
> >  
> > -void flush_page_to_ram(unsigned long mfn)
> > +int flush_page_to_ram(unsigned long mfn)
> 
> If you decide to change the prototype of flush_page_to_ram, you need to
> check the return on every call-site.
> 
> IHMO, as this function is implemented as a no-op on x86, I would keep
> the void and add an ASSERT below to check ret.

I think you are right, I'll do that.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush
  2014-10-16 10:55     ` Stefano Stabellini
@ 2014-10-16 12:05       ` Jan Beulich
  2014-10-16 14:19         ` Stefano Stabellini
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2014-10-16 12:05 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell

>>> On 16.10.14 at 12:55, <stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 14 Oct 2014, Jan Beulich wrote:
>> >>> On 13.10.14 at 16:58, <stefano.stabellini@eu.citrix.com> wrote:
>> > +static bool_t grant_map_exists(const struct domain *ld,
>> > +                        struct grant_table *rgt,
>> > +                        unsigned long mfn)
>> > +{
>> > +    const struct active_grant_entry *act;
>> > +    grant_ref_t ref;
>> > +
>> > +    ASSERT(spin_is_locked(&rgt->lock));
>> > +
>> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
>> 
>> Unless I overlooked something here or in an earlier patch I think
>> you're _still_ not enforcing a suitably low upper limit on the
>> variable that controls the maximum iteration count here. Whether
>> you enforce this the hard way (by just capping an eventual
>> command line specified value) or the soft way (by issuing a
>> warning that such a setting may hamper security, plus
>> documenting the supported value range) is up to you.
> 
> What value range do you suggest?

Whatever is suitable to limit the loop to no more than a few thousand
iterations.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush
  2014-10-16 12:05       ` Jan Beulich
@ 2014-10-16 14:19         ` Stefano Stabellini
  2014-10-16 15:39           ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: Stefano Stabellini @ 2014-10-16 14:19 UTC (permalink / raw)
  To: Jan Beulich; +Cc: julien.grall, xen-devel, Ian.Campbell, Stefano Stabellini

On Thu, 16 Oct 2014, Jan Beulich wrote:
> >>> On 16.10.14 at 12:55, <stefano.stabellini@eu.citrix.com> wrote:
> > On Tue, 14 Oct 2014, Jan Beulich wrote:
> >> >>> On 13.10.14 at 16:58, <stefano.stabellini@eu.citrix.com> wrote:
> >> > +static bool_t grant_map_exists(const struct domain *ld,
> >> > +                        struct grant_table *rgt,
> >> > +                        unsigned long mfn)
> >> > +{
> >> > +    const struct active_grant_entry *act;
> >> > +    grant_ref_t ref;
> >> > +
> >> > +    ASSERT(spin_is_locked(&rgt->lock));
> >> > +
> >> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
> >> 
> >> Unless I overlooked something here or in an earlier patch I think
> >> you're _still_ not enforcing a suitably low upper limit on the
> >> variable that controls the maximum iteration count here. Whether
> >> you enforce this the hard way (by just capping an eventual
> >> command line specified value) or the soft way (by issuing a
> >> warning that such a setting may hamper security, plus
> >> documenting the supported value range) is up to you.
> > 
> > What value range do you suggest?
> 
> Whatever is suitable to limit the loop to no more than a few thousand
> iterations.

To limit the number of iterations to 5000, we need 10 grant frames max.
I'll use that as limit, that also means changing
DEFAULT_MAX_NR_GRANT_FRAMES to 10.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush
  2014-10-16 14:19         ` Stefano Stabellini
@ 2014-10-16 15:39           ` Jan Beulich
  0 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2014-10-16 15:39 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell

>>> On 16.10.14 at 16:19, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 16 Oct 2014, Jan Beulich wrote:
>> >>> On 16.10.14 at 12:55, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Tue, 14 Oct 2014, Jan Beulich wrote:
>> >> >>> On 13.10.14 at 16:58, <stefano.stabellini@eu.citrix.com> wrote:
>> >> > +static bool_t grant_map_exists(const struct domain *ld,
>> >> > +                        struct grant_table *rgt,
>> >> > +                        unsigned long mfn)
>> >> > +{
>> >> > +    const struct active_grant_entry *act;
>> >> > +    grant_ref_t ref;
>> >> > +
>> >> > +    ASSERT(spin_is_locked(&rgt->lock));
>> >> > +
>> >> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
>> >> 
>> >> Unless I overlooked something here or in an earlier patch I think
>> >> you're _still_ not enforcing a suitably low upper limit on the
>> >> variable that controls the maximum iteration count here. Whether
>> >> you enforce this the hard way (by just capping an eventual
>> >> command line specified value) or the soft way (by issuing a
>> >> warning that such a setting may hamper security, plus
>> >> documenting the supported value range) is up to you.
>> > 
>> > What value range do you suggest?
>> 
>> Whatever is suitable to limit the loop to no more than a few thousand
>> iterations.
> 
> To limit the number of iterations to 5000, we need 10 grant frames max.
> I'll use that as limit, that also means changing
> DEFAULT_MAX_NR_GRANT_FRAMES to 10.

This may be fine (provided that constant doesn't affect Dom0's
maptrack size anymore), but I'm not really certain going below the
current default is a good thing to do.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2014-10-16 15:39 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-13 14:57 [PATCH v5 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
2014-10-13 14:58 ` [PATCH v5 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options Stefano Stabellini
2014-10-14 10:50   ` Jan Beulich
2014-10-13 14:58 ` [PATCH v5 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_* Stefano Stabellini
2014-10-13 15:36   ` Julien Grall
2014-10-13 14:58 ` [PATCH v5 3/8] xen/arm: return int from flush_page_to_ram and *_dcache_va_range Stefano Stabellini
2014-10-13 15:35   ` Julien Grall
2014-10-16 11:03     ` Stefano Stabellini
2014-10-13 14:58 ` [PATCH v5 4/8] xen/arm: introduce invalidate_dcache_va_range Stefano Stabellini
2014-10-13 14:58 ` [PATCH v5 5/8] xen/x86: introduce more cache maintenance operations Stefano Stabellini
2014-10-14 10:54   ` Jan Beulich
2014-10-13 14:58 ` [PATCH v5 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
2014-10-14 11:00   ` Jan Beulich
2014-10-16 10:55     ` Stefano Stabellini
2014-10-16 12:05       ` Jan Beulich
2014-10-16 14:19         ` Stefano Stabellini
2014-10-16 15:39           ` Jan Beulich
2014-10-13 14:58 ` [PATCH v5 7/8] Revert "xen/arm: introduce XENFEAT_grant_map_identity" Stefano Stabellini
2014-10-13 14:58 ` [PATCH v5 8/8] Revert "xen: introduce arch_grant_(un)map_page_identity" Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.