[PATCH v6 0/8] xen/arm: introduce GNTTABOP_cache_flush

[PATCH v6 0/8] xen/arm: introduce GNTTABOP_cache_flush

[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 v6:
- change ordering of new options in xen-command-line.markdown;
- initialize all options to 0, then set the defaults on an initcall
depending on what options are used;
- do not return int from flush_page_to_ram;
- set DEFAULT_MAX_NR_GRANT_FRAMES to 10;
- warn if max_grant_frames > 10.

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 *_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 |   17 ++-
 xen/arch/arm/domain.c               |    2 +-
 xen/arch/arm/guestcopy.c            |    2 +-
 xen/arch/arm/kernel.c               |    2 +-
 xen/arch/arm/mm.c                   |   26 ++--
 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            |  222 ++++++++++++++++++++++++++++-------
 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          |   54 +++++++--
 xen/include/asm-x86/flushtlb.h      |   16 +++
 xen/include/asm-x86/p2m.h           |    4 -
 xen/include/asm-x86/page.h          |    3 -
 xen/include/public/features.h       |    4 +-
 xen/include/public/grant_table.h    |   20 ++++
 xen/include/xen/grant_table.h       |    4 +-
 xen/include/xlat.lst                |    1 +
 24 files changed, 372 insertions(+), 126 deletions(-)

[PATCH v6 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options

[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 v6:
- change ordering of options in xen-command-line.markdown;
- initialize all options to 0, then set the defaults on an initcall
depending on what options are used.

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 |   17 ++++++++++-
 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            |   55 +++++++++++++++++++++++++----------
 xen/include/asm-arm/grant_table.h   |    2 +-
 xen/include/xen/grant_table.h       |    2 +-
 8 files changed, 66 insertions(+), 28 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 28bbaaf..00416af 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -608,11 +608,26 @@ does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
 
 Specify the serial parameters for the GDB stub.
 
-### gnttab\_max\_nr\_frames
+### 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_frames.
+
+### 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.
+
 ### 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 97e5bc39..dd70d81 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1054,7 +1054,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 6cd7f45..edd1923 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4564,7 +4564,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..345be82 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>
@@ -40,17 +41,26 @@
 #include <xsm/xsm.h>
 #include <asm/flushtlb.h>
 
+/* this option is deprecated, use gnttab_max_frames and
+ * gnttab_max_maptrack_frames instead */
 #ifndef max_nr_grant_frames
-unsigned int max_nr_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+unsigned int max_nr_grant_frames = 0;
 integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
 #endif
 
+unsigned int max_grant_frames = 0;
+integer_param("gnttab_max_frames", max_grant_frames);
+
 /* The maximum number of grant mappings is defined as a multiplier of the
  * maximum number of grant table entries. This defines the multiplier used.
  * Pretty arbitrary. [POLICY]
  */
 #define MAX_MAPTRACK_TO_GRANTS_RATIO 8
 
+static unsigned int max_maptrack_frames = 0;
+integer_param("gnttab_max_maptrack_frames", max_maptrack_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 +112,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 +169,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 +276,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 +1270,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 +1343,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 +1382,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 +1443,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 +2664,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 +2675,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 +2686,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;
 
@@ -2930,6 +2935,24 @@ static struct keyhandler gnttab_usage_print_all_keyhandler = {
 
 static int __init gnttab_usage_init(void)
 {
+    if ( max_nr_grant_frames )
+    {
+        printk(XENLOG_WARNING "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");
+        if ( !max_grant_frames && !max_maptrack_frames )
+        {
+            max_grant_frames = max_nr_grant_frames;
+            max_maptrack_frames = max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO;
+        } else {
+            printk(XENLOG_WARNING "dropping gnttab_max_nr_frames as superseding options are present\n");
+        }
+    }
+
+    if ( !max_grant_frames )
+        max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+
+    if ( !max_maptrack_frames )
+        max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * max_grant_frames;
+
     register_keyhandler('g', &gnttab_usage_print_all_keyhandler);
     return 0;
 }
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

[PATCH v6 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_*

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 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 dd70d81..e43499a 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -388,7 +388,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);
 }
 
@@ -511,17 +511,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++ )
@@ -559,7 +559,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
 }
@@ -570,7 +570,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
@@ -605,15 +605,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;
 }
@@ -1287,7 +1287,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 1585d35..20bcc9e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -343,7 +343,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);
 }
 
 /*
@@ -403,7 +403,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

[PATCH v6 3/8] xen/arm: return int *_dcache_va_range

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

---

Changes in v6:
- do not return int from flush_page_to_ram.
---
 xen/arch/arm/mm.c          |    2 +-
 xen/include/asm-arm/page.h |    6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index e43499a..8e989bf 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
 {
     void *v = map_domain_page(mfn);
 
-    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
+    ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
     unmap_domain_page(v);
 }
 
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 1327b00..26c5856 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
-- 
1.7.10.4

[PATCH v6 4/8] xen/arm: introduce invalidate_dcache_va_range

[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 26c5856..fdcc173 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

[PATCH v6 5/8] xen/x86: introduce more cache maintenance operations

[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..32228e1 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 void flush_page_to_ram(unsigned long mfn) {}
+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 9aa780e..a8bc999 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -344,9 +344,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 void flush_page_to_ram(unsigned long mfn) {}
-
 /* return true if permission increased */
 static inline bool_t
 perms_strictly_increased(uint32_t old_flags, uint32_t new_flags)
-- 
1.7.10.4

[PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

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

As grant_map_exists loops over all the guest grant table entries, limit
DEFAULT_MAX_NR_GRANT_FRAMES to 10 to cap the loop to 5000 iterations
max. Warn if the user sets max_grant_frames higher than 10.

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

---

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  |    8 +++
 xen/common/grant_table.c         |  137 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/grant_table.h |   20 ++++++
 xen/include/xen/grant_table.h    |    2 +-
 xen/include/xlat.lst             |    1 +
 5 files changed, 167 insertions(+), 1 deletion(-)

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 345be82..2124444 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -489,6 +489,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)
@@ -2487,6 +2515,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)
@@ -2616,6 +2735,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;
@@ -2953,6 +3086,10 @@ static int __init gnttab_usage_init(void)
     if ( !max_maptrack_frames )
         max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * max_grant_frames;
 
+    if ( max_grant_frames > DEFAULT_MAX_NR_GRANT_FRAMES )
+        printk(XENLOG_WARNING "An high gnttab_max_frames can seriously slow down the hypervisor.\n"
+                "It is recommended to set gnttab_max_frames to %d max.\n", DEFAULT_MAX_NR_GRANT_FRAMES);
+
     register_keyhandler('g', &gnttab_usage_print_all_keyhandler);
     return 0;
 }
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/xen/grant_table.h b/xen/include/xen/grant_table.h
index 9f3a1f3..77328cd 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -47,7 +47,7 @@
 
 #ifndef DEFAULT_MAX_NR_GRANT_FRAMES /* to allow arch to override */
 /* Default maximum size of a grant table. [POLICY] */
-#define DEFAULT_MAX_NR_GRANT_FRAMES   32
+#define DEFAULT_MAX_NR_GRANT_FRAMES   10
 #endif
 #ifndef max_nr_grant_frames /* to allow arch to override */
 /* The maximum size of a grant table. */
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
-- 
1.7.10.4

[PATCH v6 7/8] Revert "xen/arm: introduce XENFEAT_grant_map_identity"

[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 2124444..e2db5ce 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -771,23 +771,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 )
         {
@@ -984,19 +974,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

[PATCH v6 8/8] Revert "xen: introduce arch_grant_(un)map_page_identity"

[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 20bcc9e..6b7569a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -982,32 +982,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

Re: [PATCH v6 3/8] xen/arm: return int *_dcache_va_range

[Julien Grall]

Hi Stefano,

On 10/16/2014 03:45 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>

I was wondering if we should add a comment in the helpers in case
someone decides to return a non-zero value. If so we will have to check
the return in the caller.

But I guess it will never happen. So either way:

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

Regards,

-- 
Julien Grall

Re: [PATCH v6 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options

[Jan Beulich]

>>> On 16.10.14 at 16:45, <stefano.stabellini@eu.citrix.com> wrote:
> @@ -40,17 +41,26 @@
>  #include <xsm/xsm.h>
>  #include <asm/flushtlb.h>
>  
> +/* this option is deprecated, use gnttab_max_frames and
> + * gnttab_max_maptrack_frames instead */
>  #ifndef max_nr_grant_frames
> -unsigned int max_nr_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
> +unsigned int max_nr_grant_frames = 0;

I suppose this could now be static? And its as well as the other zero
initializers can be dropped too.

>  integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
>  #endif
>  
> +unsigned int max_grant_frames = 0;
> +integer_param("gnttab_max_frames", max_grant_frames);
> +
>  /* The maximum number of grant mappings is defined as a multiplier of the
>   * maximum number of grant table entries. This defines the multiplier used.
>   * Pretty arbitrary. [POLICY]
>   */
>  #define MAX_MAPTRACK_TO_GRANTS_RATIO 8
>  
> +static unsigned int max_maptrack_frames = 0;
> +integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
> +
> +

No double blank lines please.

> @@ -2930,6 +2935,24 @@ static struct keyhandler 
> gnttab_usage_print_all_keyhandler = {
>  
>  static int __init gnttab_usage_init(void)
>  {
> +    if ( max_nr_grant_frames )
> +    {
> +        printk(XENLOG_WARNING "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");

Long line (only keep the string literal together).

> +        if ( !max_grant_frames && !max_maptrack_frames )
> +        {
> +            max_grant_frames = max_nr_grant_frames;
> +            max_maptrack_frames = max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO;

Each unspecified field should be set independently when the
respective new option wasn't present, yet the deprecated one
was.

> +        } else {

Coding style.

> +            printk(XENLOG_WARNING "dropping gnttab_max_nr_frames as superseding options are present\n");

"ignoring ..."

Also please make sure to issue at most one warning message.

> +        }
> +    }
> +
> +    if ( !max_grant_frames )
> +        max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
> +
> +    if ( !max_maptrack_frames )
> +        max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * max_grant_frames;

As said in the earlier reply, this is specifically what you shouldn't do
when you lower the current default. This has to remain at no lower
a default than what we currently have (and should also no longer
be tied to max_grant_frames). I.e. you'll need a separate new
default value here if you really want to reduce the other one (which,
as also said, I'm not certain is a good idea).

Jan

Re: [PATCH v6 5/8] xen/x86: introduce more cache maintenance operations

[Jan Beulich]

>>> On 16.10.14 at 16:45, <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>
> 
> ---
> 
> Changes in v5:

No changes in v6? I gave an ack on v5 with a tiny change request:

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

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[Jan Beulich]

>>> On 16.10.14 at 16:45, <stefano.stabellini@eu.citrix.com> wrote:
> @@ -2953,6 +3086,10 @@ static int __init gnttab_usage_init(void)
>      if ( !max_maptrack_frames )
>          max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * max_grant_frames;
>  
> +    if ( max_grant_frames > DEFAULT_MAX_NR_GRANT_FRAMES )
> +        printk(XENLOG_WARNING "An high gnttab_max_frames can seriously slow down the hypervisor.\n"
> +                "It is recommended to set gnttab_max_frames to %d max.\n", DEFAULT_MAX_NR_GRANT_FRAMES);

It is certainly odd for the default being the maximum, but okay - if
others consider this fine, I'm not going to object.

Also, again - long lines. And the message should start with "A high"
I think, while the second line of the message needs another
XENLOG_WARNING prefixed.

Jan

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

On Thu, 16 Oct 2014, Jan Beulich wrote:
> >>> On 16.10.14 at 16:45, <stefano.stabellini@eu.citrix.com> wrote:
> > @@ -2953,6 +3086,10 @@ static int __init gnttab_usage_init(void)
> >      if ( !max_maptrack_frames )
> >          max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * max_grant_frames;
> >  
> > +    if ( max_grant_frames > DEFAULT_MAX_NR_GRANT_FRAMES )
> > +        printk(XENLOG_WARNING "An high gnttab_max_frames can seriously slow down the hypervisor.\n"
> > +                "It is recommended to set gnttab_max_frames to %d max.\n", DEFAULT_MAX_NR_GRANT_FRAMES);
> 
> It is certainly odd for the default being the maximum, but okay - if
> others consider this fine, I'm not going to object.

I think it is OK for a variable that defines the maximum.


> Also, again - long lines.

I thought that long lines were acceptable for messages and warnings?


> And the message should start with "A high"
> I think, while the second line of the message needs another
> XENLOG_WARNING prefixed.
> 
> Jan
> 

Re: [PATCH v6 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options

[Stefano Stabellini]

On Thu, 16 Oct 2014, Jan Beulich wrote:
> > @@ -2930,6 +2935,24 @@ static struct keyhandler 
> > gnttab_usage_print_all_keyhandler = {
> >  
> >  static int __init gnttab_usage_init(void)
> >  {
> > +    if ( max_nr_grant_frames )
> > +    {
> > +        printk(XENLOG_WARNING "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");
> 
> Long line (only keep the string literal together).

Are you telling me that you actually prefer:

printk(XENLOG_WARNING
       "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");

?

> > +        if ( !max_grant_frames && !max_maptrack_frames )
> > +        {
> > +            max_grant_frames = max_nr_grant_frames;
> > +            max_maptrack_frames = max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO;
> 
> Each unspecified field should be set independently when the
> respective new option wasn't present, yet the deprecated one
> was.

Sorry, I don't understand what you mean.


> > +        } else {
> 
> Coding style.
> 
> > +            printk(XENLOG_WARNING "dropping gnttab_max_nr_frames as superseding options are present\n");
> 
> "ignoring ..."
> 
> Also please make sure to issue at most one warning message.

OK


> > +        }
> > +    }
> > +
> > +    if ( !max_grant_frames )
> > +        max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
> > +
> > +    if ( !max_maptrack_frames )
> > +        max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * max_grant_frames;
> 
> As said in the earlier reply, this is specifically what you shouldn't do
> when you lower the current default.

I am not lowering the current default, not on this patch.
This patch only introduces new options and keep the defaults as before.


> This has to remain at no lower
> a default than what we currently have (and should also no longer
> be tied to max_grant_frames).
>
> I.e. you'll need a separate new
> default value here if you really want to reduce the other one (which,
> as also said, I'm not certain is a good idea).

I understand now. You are saying that for the sake of the following
patch lowering the default, I should turn MAX_MAPTRACK_TO_GRANTS_RATIO
into

#define MAX_MAPTRACK_TO_GRANTS_RATIO 256 

OK, I can do that.

Re: [PATCH v6 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options

[Stefano Stabellini]

On Thu, 16 Oct 2014, Stefano Stabellini wrote:
> On Thu, 16 Oct 2014, Jan Beulich wrote:
> > > @@ -2930,6 +2935,24 @@ static struct keyhandler 
> > > gnttab_usage_print_all_keyhandler = {
> > >  
> > >  static int __init gnttab_usage_init(void)
> > >  {
> > > +    if ( max_nr_grant_frames )
> > > +    {
> > > +        printk(XENLOG_WARNING "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");
> > 
> > Long line (only keep the string literal together).
> 
> Are you telling me that you actually prefer:
> 
> printk(XENLOG_WARNING
>        "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");
> 
> ?
> 
> > > +        if ( !max_grant_frames && !max_maptrack_frames )
> > > +        {
> > > +            max_grant_frames = max_nr_grant_frames;
> > > +            max_maptrack_frames = max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO;
> > 
> > Each unspecified field should be set independently when the
> > respective new option wasn't present, yet the deprecated one
> > was.
> 
> Sorry, I don't understand what you mean.
> 
> 
> > > +        } else {
> > 
> > Coding style.
> > 
> > > +            printk(XENLOG_WARNING "dropping gnttab_max_nr_frames as superseding options are present\n");
> > 
> > "ignoring ..."
> > 
> > Also please make sure to issue at most one warning message.
> 
> OK
> 
> 
> > > +        }
> > > +    }
> > > +
> > > +    if ( !max_grant_frames )
> > > +        max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
> > > +
> > > +    if ( !max_maptrack_frames )
> > > +        max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * max_grant_frames;
> > 
> > As said in the earlier reply, this is specifically what you shouldn't do
> > when you lower the current default.
> 
> I am not lowering the current default, not on this patch.
> This patch only introduces new options and keep the defaults as before.
> 
> 
> > This has to remain at no lower
> > a default than what we currently have (and should also no longer
> > be tied to max_grant_frames).
> >
> > I.e. you'll need a separate new
> > default value here if you really want to reduce the other one (which,
> > as also said, I'm not certain is a good idea).
> 
> I understand now. You are saying that for the sake of the following
> patch lowering the default, I should turn MAX_MAPTRACK_TO_GRANTS_RATIO
> into
> 
> #define MAX_MAPTRACK_TO_GRANTS_RATIO 256 
> 
> OK, I can do that.

With a changed name of course, maybe MAX_MAPTRACK_FRAMES.

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[David Vrabel]

On 16/10/14 15:45, Stefano Stabellini wrote:
> 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.
> 
> As grant_map_exists loops over all the guest grant table entries, limit
> DEFAULT_MAX_NR_GRANT_FRAMES to 10 to cap the loop to 5000 iterations
> max. Warn if the user sets max_grant_frames higher than 10.

No.  This is much too low.

A netfront with 4 queues wants 4 * 2 * 256 = 2048 grant references. So
this limit would only allow for two VIFs which is completely unacceptable.

blkfront would be similarly constrained.

I think you're going to have to add continuations somehow or you are
going to have abandon this approach and use the SWIOTLB in the guest.

David

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

On Thu, 16 Oct 2014, David Vrabel wrote:
> On 16/10/14 15:45, Stefano Stabellini wrote:
> > 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.
> > 
> > As grant_map_exists loops over all the guest grant table entries, limit
> > DEFAULT_MAX_NR_GRANT_FRAMES to 10 to cap the loop to 5000 iterations
> > max. Warn if the user sets max_grant_frames higher than 10.
> 
> No.  This is much too low.
> 
> A netfront with 4 queues wants 4 * 2 * 256 = 2048 grant references. So
> this limit would only allow for two VIFs which is completely unacceptable.
> 
> blkfront would be similarly constrained.

10 is too low, that is a good point, thanks!


> I think you're going to have to add continuations somehow or you are
> going to have abandon this approach and use the SWIOTLB in the guest.

Actually the latest version already supports continuations (even though
I admit I didn't explicitly test it).

In any case the default max (32 frames) means 16000 iterations, that
would take around 32000ns on a not very modern system. I don't think is
that bad. And of course it is just the theoretical worst case.

Maybe we should leave the default max as is?

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[David Vrabel]

On 16/10/14 18:29, Stefano Stabellini wrote:
> On Thu, 16 Oct 2014, David Vrabel wrote:
>> On 16/10/14 15:45, Stefano Stabellini wrote:
>>> 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.
>>>
>>> As grant_map_exists loops over all the guest grant table entries, limit
>>> DEFAULT_MAX_NR_GRANT_FRAMES to 10 to cap the loop to 5000 iterations
>>> max. Warn if the user sets max_grant_frames higher than 10.
>>
>> No.  This is much too low.
>>
>> A netfront with 4 queues wants 4 * 2 * 256 = 2048 grant references. So
>> this limit would only allow for two VIFs which is completely unacceptable.
>>
>> blkfront would be similarly constrained.
> 
> 10 is too low, that is a good point, thanks!
> 
> 
>> I think you're going to have to add continuations somehow or you are
>> going to have abandon this approach and use the SWIOTLB in the guest.
> 
> Actually the latest version already supports continuations (even though
> I admit I didn't explicitly test it).

I meant pre-empting the hypercall when it is in the middle of iterating
through the grant table.  I think it would be safe to drop the grant
table lock and resume the scan part way through since the relevant grant
should not change during the cache flush call (if it does then the flush
wiil at worst safely fail).

> In any case the default max (32 frames) means 16000 iterations, that
> would take around 32000ns on a not very modern system. I don't think is
> that bad. And of course it is just the theoretical worst case.
> 
> Maybe we should leave the default max as is?

At a bare minimum, yes.  But I really don't think we should be /adding/
hard scalability limits to Xen -- we should be removing them!

David

Re: [PATCH v6 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options

[Jan Beulich]

>>> On 16.10.14 at 18:56, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 16 Oct 2014, Jan Beulich wrote:
>> > @@ -2930,6 +2935,24 @@ static struct keyhandler 
>> > gnttab_usage_print_all_keyhandler = {
>> >  
>> >  static int __init gnttab_usage_init(void)
>> >  {
>> > +    if ( max_nr_grant_frames )
>> > +    {
>> > +        printk(XENLOG_WARNING "gnttab_max_nr_frames is deprecated, use 
> gnttab_max_frames instead\n");
>> 
>> Long line (only keep the string literal together).
> 
> Are you telling me that you actually prefer:
> 
> printk(XENLOG_WARNING
>        "gnttab_max_nr_frames is deprecated, use gnttab_max_frames 
> instead\n");
> 
> ?

Yes.

>> > +        if ( !max_grant_frames && !max_maptrack_frames )
>> > +        {
>> > +            max_grant_frames = max_nr_grant_frames;
>> > +            max_maptrack_frames = max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO;
>> 
>> Each unspecified field should be set independently when the
>> respective new option wasn't present, yet the deprecated one
>> was.
> 
> Sorry, I don't understand what you mean.

        if ( !max_grant_frames )
            max_grant_frames = max_nr_grant_frames;
        if ( !max_maptrack_frames )
            max_maptrack_frames = max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO;

>> > +        }
>> > +    }
>> > +
>> > +    if ( !max_grant_frames )
>> > +        max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
>> > +
>> > +    if ( !max_maptrack_frames )
>> > +        max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * max_grant_frames;
>> 
>> As said in the earlier reply, this is specifically what you shouldn't do
>> when you lower the current default.
> 
> I am not lowering the current default, not on this patch.
> This patch only introduces new options and keep the defaults as before.

But the patch that does the lowering doesn't take care of this either.

>> This has to remain at no lower
>> a default than what we currently have (and should also no longer
>> be tied to max_grant_frames).
>>
>> I.e. you'll need a separate new
>> default value here if you really want to reduce the other one (which,
>> as also said, I'm not certain is a good idea).
> 
> I understand now. You are saying that for the sake of the following
> patch lowering the default, I should turn MAX_MAPTRACK_TO_GRANTS_RATIO
> into
> 
> #define MAX_MAPTRACK_TO_GRANTS_RATIO 256 

No. The ratio should be left unchanged too, as it's being used above
(not sure about elsewhere). You should have a standalone new
default for the maptrack table size.

Jan

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[Jan Beulich]

>>> On 16.10.14 at 18:30, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 16 Oct 2014, Jan Beulich wrote:
>> >>> On 16.10.14 at 16:45, <stefano.stabellini@eu.citrix.com> wrote:
>> > @@ -2953,6 +3086,10 @@ static int __init gnttab_usage_init(void)
>> >      if ( !max_maptrack_frames )
>> >          max_maptrack_frames = MAX_MAPTRACK_TO_GRANTS_RATIO * 
> max_grant_frames;
>> >  
>> > +    if ( max_grant_frames > DEFAULT_MAX_NR_GRANT_FRAMES )
>> > +        printk(XENLOG_WARNING "An high gnttab_max_frames can seriously 
> slow down the hypervisor.\n"
>> > +                "It is recommended to set gnttab_max_frames to %d max.\n", 
> DEFAULT_MAX_NR_GRANT_FRAMES);
>> 
>> It is certainly odd for the default being the maximum, but okay - if
>> others consider this fine, I'm not going to object.
> 
> I think it is OK for a variable that defines the maximum.

I don't think I understand what you're stating here.

>> Also, again - long lines.
> 
> I thought that long lines were acceptable for messages and warnings?

Yes, the textual parts of them. XENLOG_* prefixes as well as
arguments to fill the %<format> fields should play by the no-
long-lines rule though.

Jan

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[Jan Beulich]

>>> On 16.10.14 at 19:29, <stefano.stabellini@eu.citrix.com> wrote:
> In any case the default max (32 frames) means 16000 iterations, that
> would take around 32000ns on a not very modern system. I don't think is
> that bad. And of course it is just the theoretical worst case.

2ns per iteration when there are 3 conditionals in the loop body
seems rather optimistic to me.

Jan

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

On Thu, 16 Oct 2014, David Vrabel wrote:
> On 16/10/14 18:29, Stefano Stabellini wrote:
> > On Thu, 16 Oct 2014, David Vrabel wrote:
> >> On 16/10/14 15:45, Stefano Stabellini wrote:
> >>> 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.
> >>>
> >>> As grant_map_exists loops over all the guest grant table entries, limit
> >>> DEFAULT_MAX_NR_GRANT_FRAMES to 10 to cap the loop to 5000 iterations
> >>> max. Warn if the user sets max_grant_frames higher than 10.
> >>
> >> No.  This is much too low.
> >>
> >> A netfront with 4 queues wants 4 * 2 * 256 = 2048 grant references. So
> >> this limit would only allow for two VIFs which is completely unacceptable.
> >>
> >> blkfront would be similarly constrained.
> > 
> > 10 is too low, that is a good point, thanks!
> > 
> > 
> >> I think you're going to have to add continuations somehow or you are
> >> going to have abandon this approach and use the SWIOTLB in the guest.
> > 
> > Actually the latest version already supports continuations (even though
> > I admit I didn't explicitly test it).
> 
> I meant pre-empting the hypercall when it is in the middle of iterating
> through the grant table.  I think it would be safe to drop the grant
> table lock and resume the scan part way through since the relevant grant
> should not change during the cache flush call (if it does then the flush
> wiil at worst safely fail).

Dropping the lock and taking it again every 1000 iterations is doable
and beneficial.
However a real continuation is difficult because we already have a
continuation on the count hypercall argument.

Re: [PATCH v6 6/8] xen/arm: introduce GNTTABOP_cache_flush

[Jan Beulich]

>>> On 17.10.14 at 12:01, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 16 Oct 2014, David Vrabel wrote:
>> On 16/10/14 18:29, Stefano Stabellini wrote:
>> > On Thu, 16 Oct 2014, David Vrabel wrote:
>> >> On 16/10/14 15:45, Stefano Stabellini wrote:
>> >>> 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.
>> >>>
>> >>> As grant_map_exists loops over all the guest grant table entries, limit
>> >>> DEFAULT_MAX_NR_GRANT_FRAMES to 10 to cap the loop to 5000 iterations
>> >>> max. Warn if the user sets max_grant_frames higher than 10.
>> >>
>> >> No.  This is much too low.
>> >>
>> >> A netfront with 4 queues wants 4 * 2 * 256 = 2048 grant references. So
>> >> this limit would only allow for two VIFs which is completely unacceptable.
>> >>
>> >> blkfront would be similarly constrained.
>> > 
>> > 10 is too low, that is a good point, thanks!
>> > 
>> > 
>> >> I think you're going to have to add continuations somehow or you are
>> >> going to have abandon this approach and use the SWIOTLB in the guest.
>> > 
>> > Actually the latest version already supports continuations (even though
>> > I admit I didn't explicitly test it).
>> 
>> I meant pre-empting the hypercall when it is in the middle of iterating
>> through the grant table.  I think it would be safe to drop the grant
>> table lock and resume the scan part way through since the relevant grant
>> should not change during the cache flush call (if it does then the flush
>> wiil at worst safely fail).
> 
> Dropping the lock and taking it again every 1000 iterations is doable
> and beneficial.
> However a real continuation is difficult because we already have a
> continuation on the count hypercall argument.

We've got the upper bits of cmd left to do further encoding of
continuations. If you split every 1024 iterations and used the top
16 bits of cmd, that would leave ample room for future GNTTABOP-s
and still allow handling tables sizes up to 2^26 entries. We could
even deal with the full 2^32 range (removing the need to
enforce some upper limit) if we reserved only 10 bits for the actual
cmd encoding, which should still be plenty.

Jan