[PATCH v4 0/7] xen/arm: introduce GNTTABOP_cache_flush

[PATCH v4 0/7] 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 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 (7):
      xen: introduce gnttab_max_nr_maptrack_frames command line option
      xen/arm: rename *_xen_dcache_* operations to *_dcache_*
      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 |    6 ++
 xen/arch/arm/guestcopy.c            |    2 +-
 xen/arch/arm/kernel.c               |    2 +-
 xen/arch/arm/mm.c                   |   24 ++---
 xen/arch/arm/p2m.c                  |   30 +-----
 xen/arch/arm/smpboot.c              |    2 +-
 xen/arch/x86/flushtlb.c             |    6 ++
 xen/common/compat/grant_table.c     |    8 ++
 xen/common/grant_table.c            |  175 ++++++++++++++++++++++++++++-------
 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   |    3 +-
 xen/include/asm-arm/p2m.h           |    4 -
 xen/include/asm-arm/page.h          |   50 ++++++++--
 xen/include/asm-x86/p2m.h           |    4 -
 xen/include/asm-x86/page.h          |    7 +-
 xen/include/public/features.h       |    4 +-
 xen/include/public/grant_table.h    |   20 ++++
 xen/include/xlat.lst                |    1 +
 21 files changed, 305 insertions(+), 101 deletions(-)

[PATCH v4 1/7] xen: introduce gnttab_max_nr_maptrack_frames command line option

[Stefano Stabellini]

Introduce a new Xen command line option to specify the max number of
maptrack frames per domain.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 docs/misc/xen-command-line.markdown |    6 ++++++
 xen/common/grant_table.c            |   11 +++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 389701a..acf6208 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -613,6 +613,12 @@ Specify the serial parameters for the GDB stub.
 
 Specify the maximum number of frames per grant table operation.
 
+### gnttab\_max\_nr\_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/common/grant_table.c b/xen/common/grant_table.c
index 23266c3..5451c75 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -102,10 +102,9 @@ 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);
-}
+static unsigned int max_nr_maptrack_frames = DEFAULT_MAX_NR_GRANT_FRAMES *
+                                             MAX_MAPTRACK_TO_GRANTS_RATIO;
+integer_param("gnttab_max_nr_maptrack_frames", max_nr_maptrack_frames);
 
 #define MAPTRACK_TAIL (~0u)
 
@@ -271,7 +270,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_nr_maptrack_frames )
             break;
 
         new_mt = alloc_xenheap_page();
@@ -2659,7 +2658,7 @@ grant_table_create(
 
     /* Tracking of mapped foreign frames table */
     if ( (t->maptrack = xzalloc_array(struct grant_mapping *,
-                                      max_nr_maptrack_frames())) == NULL )
+                                      max_nr_maptrack_frames)) == NULL )
         goto no_mem_2;
     if ( (t->maptrack[0] = alloc_xenheap_page()) == NULL )
         goto no_mem_3;
-- 
1.7.10.4

[PATCH v4 2/7] xen/arm: rename *_xen_dcache_* operations to *_dcache_*

[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 c5b48ef..4bef62a 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

[PATCH v4 3/7] 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 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       |   30 ++++++++++++++++++++++++++++++
 3 files changed, 36 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 1327b00..d88a586 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -268,6 +268,36 @@ 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 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 */
+}
+
 static inline void clean_dcache_va_range(const void *p, unsigned long size)
 {
     const void *end;
-- 
1.7.10.4

[PATCH v4 4/7] xen/x86: introduce more cache maintenance operations

[Stefano Stabellini]

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

---

Changes in v4:
- remove _xen in the function names;
- implement the functions using existing x86 flushing functions.
---
 xen/arch/x86/flushtlb.c    |    6 ++++++
 xen/include/asm-x86/page.h |    7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
index 5d5d79c..3e5e287 100644
--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -152,3 +152,9 @@ void flush_area_local(const void *va, unsigned int flags)
 
     local_irq_restore(irqfl);
 }
+
+void clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
+{
+    int order = get_order_from_bytes(size);
+    flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
+}
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 9aa780e..e56be8a 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -344,8 +344,13 @@ 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) {}
+static inline void invalidate_dcache_va_range(const void *p, unsigned long size) {}
+void clean_and_invalidate_dcache_va_range(const void *p, unsigned long size);
+static inline void clean_dcache_va_range(const void *p, unsigned long size)
+{
+    clean_and_invalidate_dcache_va_range(p, size);
+}
 
 /* return true if permission increased */
 static inline bool_t
-- 
1.7.10.4

[PATCH v4 5/7] 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.

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

---

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         |  134 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/grant_table.h |   20 ++++++
 xen/include/xlat.lst             |    1 +
 4 files changed, 163 insertions(+)

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 7ebbbc1..0ffab97 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 5451c75..a989d81 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -483,6 +483,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)
@@ -2481,6 +2509,98 @@ 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;
+    uint64_t mfn;
+    void *v;
+
+    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);
+            gdprintk(XENLOG_G_ERR, "mfn %"PRIx64" hasn't been granted by d%d to d%d\n",
+                    mfn, owner->domain_id, d->domain_id);
+            return -EINVAL;
+        }
+    }
+
+    v = map_domain_page(mfn);
+    v += cflush->offset;
+
+    if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op & GNTTAB_CACHE_CLEAN) )
+        clean_and_invalidate_dcache_va_range(v, cflush->length);
+    else if ( cflush->op & GNTTAB_CACHE_INVAL )
+        invalidate_dcache_va_range(v, cflush->length);
+    else if ( cflush->op & GNTTAB_CACHE_CLEAN )
+        clean_dcache_va_range(v, cflush->length);
+
+    if ( d != owner )
+        spin_unlock(&owner->grant_table->lock);
+    unmap_domain_page(v);
+    put_page(page);
+
+    return 0;
+}
+
+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)
@@ -2610,6 +2730,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

[PATCH v4 6/7] 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>

---

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 a989d81..118d8c7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -765,23 +765,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 )
         {
@@ -978,19 +968,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 47147ce..eac8a70 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_nr_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 v4 7/7] 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>
---
 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

Re: [PATCH v4 6/7] Revert "xen/arm: introduce XENFEAT_grant_map_identity"

[Julien Grall]

Hi Stefano,

On 10/10/2014 12:43, Stefano Stabellini wrote:
> 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>

Regards,

>
> ---
>
> 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 a989d81..118d8c7 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -765,23 +765,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 )
>           {
> @@ -978,19 +968,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 47147ce..eac8a70 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_nr_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
>
>

-- 
Julien Grall

Re: [PATCH v4 7/7] Revert "xen: introduce arch_grant_(un)map_page_identity"

[Julien Grall]

Hi Stefano,

On 10/10/2014 12:44, Stefano Stabellini wrote:
> This reverts commit e25a5f4d8cf3b55718048abdd21c7d0de64ae54c.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

> ---
>   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 */
>
>   /*
>

-- 
Julien Grall

Re: [PATCH v4 5/7] xen/arm: introduce GNTTABOP_cache_flush

[Julien Grall]

Hi Stefano,

On 10/10/2014 12:43, Stefano Stabellini wrote:
> +static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush)
> +{
> +    struct domain *d, *owner;
> +    struct page_info *page;
> +    uint64_t mfn;
> +    void *v;
> +
> +    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;

I may have miss a part of the discussion. What is the purpose of 
introducing a flag that is not supported? Do we expect to implement on 
Xen 4.6?

-- 
Julien Grall

Re: [PATCH v4 1/7] xen: introduce gnttab_max_nr_maptrack_frames command line option

[Jan Beulich]

>>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -613,6 +613,12 @@ Specify the serial parameters for the GDB stub.
>  
>  Specify the maximum number of frames per grant table operation.
>  
> +### gnttab\_max\_nr\_maptrack\_frames

Do we really need the "nr_" in here?

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -102,10 +102,9 @@ 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);
> -}
> +static unsigned int max_nr_maptrack_frames = DEFAULT_MAX_NR_GRANT_FRAMES *
> +                                             MAX_MAPTRACK_TO_GRANTS_RATIO;
> +integer_param("gnttab_max_nr_maptrack_frames", max_nr_maptrack_frames);

I'm not sure: As said before, the primary goal must be that existing
setups don't suddenly start failing. I.e. the other options no longer
controlling the maptrack table size may badly affect Dom0. One
possibility would be to honor the other option in the original way if
the new option wasn't made use of (and perhaps issue a warning
to that effect).

Jan

Re: [PATCH v4 4/7] xen/x86: introduce more cache maintenance operations

[Jan Beulich]

>>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
> +void clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
> +{
> +    int order = get_order_from_bytes(size);

unsigned int if the variable is really needed.

> +    flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
> +}

Overall I wonder whether not making this an inline function is really
appropriate.

Oh, and then I may have slightly misguided you: The caller you're
adding for this wants to be able to clean/invalidate sub-page chunks,
which this interface doesn't allow. This isn't a problem unless the
operation gets done frequently and always degenerates to wbinvd().
At the very least I'd suggest adding a comment saying that sub-page
granularity support would need to be added if necessary here.

> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -344,8 +344,13 @@ 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) {}
> +static inline void invalidate_dcache_va_range(const void *p, unsigned long size) {}

This should fail, not appear to succeed. We just can't do what is being
requested.

Jan

Re: [PATCH v4 5/7] xen/arm: introduce GNTTABOP_cache_flush

[Jan Beulich]

>>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -483,6 +483,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++ )

While you added a way to restrict the time this loop takes, you
still don't enforce a sane upper bound on the value specified
for "gnttab_max_nr_frames=". Perhaps that could be done
together with the compatibility thing I mentioned in reply to one
of the earlier patches?

> +static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush)
> +{
> +    struct domain *d, *owner;
> +    struct page_info *page;
> +    uint64_t mfn;

unsigned long

> +    void *v;
> +
> +    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);
> +            gdprintk(XENLOG_G_ERR, "mfn %"PRIx64" hasn't been granted by d%d to d%d\n",
> +                    mfn, owner->domain_id, d->domain_id);

I'm afraid without a GPFN this message isn't going to be very useful
other than to indicate there is _some_ problem.

Jan

Re: [PATCH v4 1/7] xen: introduce gnttab_max_nr_maptrack_frames command line option

[Stefano Stabellini]

On Fri, 10 Oct 2014, Jan Beulich wrote:
> >>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -613,6 +613,12 @@ Specify the serial parameters for the GDB stub.
> >  
> >  Specify the maximum number of frames per grant table operation.
> >  
> > +### gnttab\_max\_nr\_maptrack\_frames
> 
> Do we really need the "nr_" in here?

I guess not


> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -102,10 +102,9 @@ 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);
> > -}
> > +static unsigned int max_nr_maptrack_frames = DEFAULT_MAX_NR_GRANT_FRAMES *
> > +                                             MAX_MAPTRACK_TO_GRANTS_RATIO;
> > +integer_param("gnttab_max_nr_maptrack_frames", max_nr_maptrack_frames);
> 
> I'm not sure: As said before, the primary goal must be that existing
> setups don't suddenly start failing. I.e. the other options no longer
> controlling the maptrack table size may badly affect Dom0. One
> possibility would be to honor the other option in the original way if
> the new option wasn't made use of (and perhaps issue a warning
> to that effect).

Honestly I think that it would be a bad idea to try to second guess what
the user really meant with the options she passed. Also I think it is a
bad idea to have options do different things depending on the presence
or absence of other options.

I think we should either:

- leave things as they are
- add this new option and as a consequence also change the effect of
gnttab_max_nr_frames

Re: [PATCH v4 4/7] xen/x86: introduce more cache maintenance operations

[Stefano Stabellini]

On Fri, 10 Oct 2014, Jan Beulich wrote:
> >>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
> > +void clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
> > +{
> > +    int order = get_order_from_bytes(size);
> 
> unsigned int if the variable is really needed.
> 
> > +    flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
> > +}
> 
> Overall I wonder whether not making this an inline function is really
> appropriate.

Introducing this function as static inline in page.h would cause the
number of included files to grow a bit too much I think.


> Oh, and then I may have slightly misguided you: The caller you're
> adding for this wants to be able to clean/invalidate sub-page chunks,
> which this interface doesn't allow. This isn't a problem unless the
> operation gets done frequently and always degenerates to wbinvd().
> At the very least I'd suggest adding a comment saying that sub-page
> granularity support would need to be added if necessary here.

Good point.


> > --- a/xen/include/asm-x86/page.h
> > +++ b/xen/include/asm-x86/page.h
> > @@ -344,8 +344,13 @@ 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) {}
> > +static inline void invalidate_dcache_va_range(const void *p, unsigned long size) {}
> 
> This should fail, not appear to succeed. We just can't do what is being
> requested.

You are right, however this will cause all the arm cache flushing
functions to change prototype too.

Re: [PATCH v4 1/7] xen: introduce gnttab_max_nr_maptrack_frames command line option

[Jan Beulich]

>>> On 13.10.14 at 11:59, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 10 Oct 2014, Jan Beulich wrote:
>> >>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
>> > --- a/xen/common/grant_table.c
>> > +++ b/xen/common/grant_table.c
>> > @@ -102,10 +102,9 @@ 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);
>> > -}
>> > +static unsigned int max_nr_maptrack_frames = DEFAULT_MAX_NR_GRANT_FRAMES *
>> > +                                             MAX_MAPTRACK_TO_GRANTS_RATIO;
>> > +integer_param("gnttab_max_nr_maptrack_frames", max_nr_maptrack_frames);
>> 
>> I'm not sure: As said before, the primary goal must be that existing
>> setups don't suddenly start failing. I.e. the other options no longer
>> controlling the maptrack table size may badly affect Dom0. One
>> possibility would be to honor the other option in the original way if
>> the new option wasn't made use of (and perhaps issue a warning
>> to that effect).
> 
> Honestly I think that it would be a bad idea to try to second guess what
> the user really meant with the options she passed. Also I think it is a
> bad idea to have options do different things depending on the presence
> or absence of other options.

I agree this is not optimal, but I suppose you also agree that it is
not really acceptable to break working systems.

> I think we should either:
> 
> - leave things as they are

Only as long as this doesn't result in (perceived) regressions. I.e.
not an option here afaict.

> - add this new option and as a consequence also change the effect of
> gnttab_max_nr_frames

We could deprecate the old option altogether (and have it control
both values unless one of the new options got used), and introduce
a new option "gnttab_max_frames" controlling just the one table's
size. Does that sound any better?

Jan

Re: [PATCH v4 1/7] xen: introduce gnttab_max_nr_maptrack_frames command line option

[Stefano Stabellini]

On Mon, 13 Oct 2014, Jan Beulich wrote:
> >>> On 13.10.14 at 11:59, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 10 Oct 2014, Jan Beulich wrote:
> >> >>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
> >> > --- a/xen/common/grant_table.c
> >> > +++ b/xen/common/grant_table.c
> >> > @@ -102,10 +102,9 @@ 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);
> >> > -}
> >> > +static unsigned int max_nr_maptrack_frames = DEFAULT_MAX_NR_GRANT_FRAMES *
> >> > +                                             MAX_MAPTRACK_TO_GRANTS_RATIO;
> >> > +integer_param("gnttab_max_nr_maptrack_frames", max_nr_maptrack_frames);
> >> 
> >> I'm not sure: As said before, the primary goal must be that existing
> >> setups don't suddenly start failing. I.e. the other options no longer
> >> controlling the maptrack table size may badly affect Dom0. One
> >> possibility would be to honor the other option in the original way if
> >> the new option wasn't made use of (and perhaps issue a warning
> >> to that effect).
> > 
> > Honestly I think that it would be a bad idea to try to second guess what
> > the user really meant with the options she passed. Also I think it is a
> > bad idea to have options do different things depending on the presence
> > or absence of other options.
> 
> I agree this is not optimal, but I suppose you also agree that it is
> not really acceptable to break working systems.
> 
> > I think we should either:
> > 
> > - leave things as they are
> 
> Only as long as this doesn't result in (perceived) regressions. I.e.
> not an option here afaict.
> 
> > - add this new option and as a consequence also change the effect of
> > gnttab_max_nr_frames
> 
> We could deprecate the old option altogether (and have it control
> both values unless one of the new options got used), and introduce
> a new option "gnttab_max_frames" controlling just the one table's
> size. Does that sound any better?
 
Yes, that's better.

Re: [PATCH v4 4/7] xen/x86: introduce more cache maintenance operations

[Jan Beulich]

>>> On 13.10.14 at 12:35, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 10 Oct 2014, Jan Beulich wrote:
>> >>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
>> > +void clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
>> > +{
>> > +    int order = get_order_from_bytes(size);
>> 
>> unsigned int if the variable is really needed.
>> 
>> > +    flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
>> > +}
>> 
>> Overall I wonder whether not making this an inline function is really
>> appropriate.
> 
> Introducing this function as static inline in page.h would cause the
> number of included files to grow a bit too much I think.

Or maybe page.h is simply the wrong home for them, at least on
x86?

>> > --- a/xen/include/asm-x86/page.h
>> > +++ b/xen/include/asm-x86/page.h
>> > @@ -344,8 +344,13 @@ 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) {}
>> > +static inline void invalidate_dcache_va_range(const void *p, unsigned long size) {}
>> 
>> This should fail, not appear to succeed. We just can't do what is being
>> requested.
> 
> You are right, however this will cause all the arm cache flushing
> functions to change prototype too.

But that's a small price to pay compared to introducing an interface
that can fail, but failure of which doesn't get reported correctly.

Jan

Re: [PATCH v4 4/7] xen/x86: introduce more cache maintenance operations

[Stefano Stabellini]

On Mon, 13 Oct 2014, Jan Beulich wrote:
> >>> On 13.10.14 at 12:35, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 10 Oct 2014, Jan Beulich wrote:
> >> >>> On 10.10.14 at 13:43, <stefano.stabellini@eu.citrix.com> wrote:
> >> > +void clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
> >> > +{
> >> > +    int order = get_order_from_bytes(size);
> >> 
> >> unsigned int if the variable is really needed.
> >> 
> >> > +    flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
> >> > +}
> >> 
> >> Overall I wonder whether not making this an inline function is really
> >> appropriate.
> > 
> > Introducing this function as static inline in page.h would cause the
> > number of included files to grow a bit too much I think.
> 
> Or maybe page.h is simply the wrong home for them, at least on
> x86?

good point, I'll move them to flushtlb.h