[PATCH v2 0/4] xen/arm: introduce GNTTABOP_cache_flush

[PATCH v2 0/4] 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 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 (4):
      xen/arm: introduce invalidate_xen_dcache_va_range
      xen: introduce grant_map_exists
      xen/arm: introduce GNTTABOP_cache_flush
      Revert "xen/arm: introduce XENFEAT_grant_map_identity"

 xen/common/grant_table.c           |  133 +++++++++++++++++++++++++++++-------
 xen/common/kernel.c                |    2 -
 xen/drivers/passthrough/arm/smmu.c |   33 +++++++++
 xen/include/asm-arm/arm32/page.h   |    3 +
 xen/include/asm-arm/arm64/page.h   |    3 +
 xen/include/asm-arm/grant_table.h  |    3 +-
 xen/include/asm-arm/page.h         |   30 ++++++++
 xen/include/public/features.h      |    4 +-
 xen/include/public/grant_table.h   |   19 ++++++
 9 files changed, 201 insertions(+), 29 deletions(-)

[PATCH v2 1/4] xen/arm: introduce invalidate_xen_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>
---
 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 9740672..6fb2e68 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_xen_dcache_one(R) STORE_CP32(R, DCIMVAC)
+
 /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
 #define __clean_xen_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 bb10164..f181b1b 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_xen_dcache_one(R) "dc ivac, %" #R ";"
+
 /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
 #define __clean_xen_dcache_one(R) "dc cvac, %" #R ";"
 
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d758b61..da02e97 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_xen_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_xen_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_xen_dcache_one(0) : : "r" (end));
+    }
+
+    for ( ; p < end; p += cacheline_bytes )
+        asm volatile (__invalidate_xen_dcache_one(0) : : "r" (p));
+
+    dsb(sy);           /* So we know the flushes happen before continuing */
+}
+
 static inline void clean_xen_dcache_va_range(const void *p, unsigned long size)
 {
     const void *end;
-- 
1.7.10.4

[PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini]

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 v2:
- make the function static;
- remove the spin_lock.
---
 xen/common/grant_table.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 23266c3..7a6399b 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -484,6 +484,36 @@ static int _set_status(unsigned gt_version,
         return _set_status_v2(domid, readonly, mapflag, shah, act, status);
 }
 
+static bool_t grant_map_exists(struct domain *ld,
+                        struct grant_table *rgt,
+                        unsigned long mfn)
+{
+    struct active_grant_entry *act;
+    grant_ref_t ref;
+    bool_t ret = 0;
+
+    ASSERT(&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;
+        
+        ret = 1;
+        break;
+    }
+
+    return ret;
+}
+
 static void mapcount(
     struct grant_table *lgt, struct domain *rd, unsigned long mfn,
     unsigned int *wrc, unsigned int *rdc)
-- 
1.7.10.4

[PATCH v2 3/4] 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.

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

---

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/grant_table.c         |   73 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/grant_table.h |   19 ++++++++++
 2 files changed, 92 insertions(+)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 7a6399b..d5bb4f7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2641,6 +2641,79 @@ do_grant_table_op(
         }
         break;
     }
+    case GNTTABOP_cache_flush:
+    {
+        struct gnttab_cache_flush cflush;
+        struct domain *d, *owner;
+        struct page_info *page;
+        uint64_t mfn;
+        void *v;
+
+        /* currently unimplemented */
+        if ( count != 1 )
+            return -ENOSYS;
+
+        if ( copy_from_guest(&cflush, uop, 1) )
+            return -EFAULT;
+
+        if ( cflush.offset + cflush.size > PAGE_SIZE )
+            return -EINVAL;
+
+        if ( cflush.size == 0 || cflush.op == 0 )
+            return 0;
+
+        if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) )
+            return -EINVAL;
+
+        /* currently unimplemented */
+        if ( cflush.a.ref != 0 )
+            return -ENOSYS;
+
+        d = rcu_lock_current_domain();
+        if ( d == NULL )
+            return -ESRCH;
+
+        mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT;
+
+        if ( !mfn_valid(mfn) )
+        {
+            gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", 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 -EFAULT;
+        }
+
+        spin_lock(&owner->grant_table->lock);
+
+        if ( !grant_map_exists(d, owner->grant_table, mfn) )
+        {
+            spin_unlock(&owner->grant_table->lock);
+            put_page(page);
+            rcu_unlock_domain(d);
+            gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %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 )
+            invalidate_xen_dcache_va_range(v, cflush.size);
+        if ( cflush.op & GNTTAB_CACHE_CLEAN )
+            clean_xen_dcache_va_range(v, cflush.size);
+
+        unmap_domain_page(v);
+        spin_unlock(&owner->grant_table->lock);
+        put_page(page);
+    }
     default:
         rc = -ENOSYS;
         break;
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index b8a3d6c..1833bba 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,24 @@ 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;
+    uint32_t offset; /* offset from start of grant */
+    uint32_t size;   /* size within the grant */
+#define GNTTAB_CACHE_CLEAN      (1<<0)
+#define GNTTAB_CACHE_INVAL      (1<<1)
+    uint32_t op;
+};
+typedef struct gnttab_cache_flush gnttab_cache_flush_t;
+DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
+
 #endif /* __XEN_INTERFACE_VERSION__ */
 
 /*
-- 
1.7.10.4

[PATCH v2 4/4] Revert "xen/arm: introduce XENFEAT_grant_map_identity"

[Stefano Stabellini]

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 d5bb4f7..96a2682 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -768,23 +768,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 )
         {
@@ -981,19 +971,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

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

On Fri, 3 Oct 2014, 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> 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/grant_table.c         |   73 ++++++++++++++++++++++++++++++++++++++
>  xen/include/public/grant_table.h |   19 ++++++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 7a6399b..d5bb4f7 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2641,6 +2641,79 @@ do_grant_table_op(
>          }
>          break;
>      }
> +    case GNTTABOP_cache_flush:
> +    {
> +        struct gnttab_cache_flush cflush;
> +        struct domain *d, *owner;
> +        struct page_info *page;
> +        uint64_t mfn;
> +        void *v;
> +
> +        /* currently unimplemented */
> +        if ( count != 1 )
> +            return -ENOSYS;
> +
> +        if ( copy_from_guest(&cflush, uop, 1) )
> +            return -EFAULT;
> +
> +        if ( cflush.offset + cflush.size > PAGE_SIZE )
> +            return -EINVAL;
> +
> +        if ( cflush.size == 0 || cflush.op == 0 )
> +            return 0;
> +
> +        if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) )
> +            return -EINVAL;
> +
> +        /* currently unimplemented */
> +        if ( cflush.a.ref != 0 )
> +            return -ENOSYS;
> +
> +        d = rcu_lock_current_domain();
> +        if ( d == NULL )
> +            return -ESRCH;
> +
> +        mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT;
> +
> +        if ( !mfn_valid(mfn) )
> +        {
> +            gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", 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 -EFAULT;
> +        }
> +
> +        spin_lock(&owner->grant_table->lock);
> +
> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> +        {
> +            spin_unlock(&owner->grant_table->lock);
> +            put_page(page);
> +            rcu_unlock_domain(d);
> +            gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %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 )
> +            invalidate_xen_dcache_va_range(v, cflush.size);
> +        if ( cflush.op & GNTTAB_CACHE_CLEAN )
> +            clean_xen_dcache_va_range(v, cflush.size);

I realize now that I forgot to introduce empty cache cleaning stubs for x86.


> +        unmap_domain_page(v);
> +        spin_unlock(&owner->grant_table->lock);
> +        put_page(page);
> +    }
>      default:
>          rc = -ENOSYS;
>          break;
> diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
> index b8a3d6c..1833bba 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,24 @@ 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;
> +    uint32_t offset; /* offset from start of grant */
> +    uint32_t size;   /* size within the grant */
> +#define GNTTAB_CACHE_CLEAN      (1<<0)
> +#define GNTTAB_CACHE_INVAL      (1<<1)
> +    uint32_t op;
> +};
> +typedef struct gnttab_cache_flush gnttab_cache_flush_t;
> +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
> +
>  #endif /* __XEN_INTERFACE_VERSION__ */
>  
>  /*
> -- 
> 1.7.10.4
> 

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Andrew Cooper]

On 03/10/14 15:50, Stefano Stabellini wrote:
> 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 v2:
> - make the function static;
> - remove the spin_lock.
> ---
>  xen/common/grant_table.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 23266c3..7a6399b 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -484,6 +484,36 @@ static int _set_status(unsigned gt_version,
>          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> +static bool_t grant_map_exists(struct domain *ld,
> +                        struct grant_table *rgt,
> +                        unsigned long mfn)
> +{
> +    struct active_grant_entry *act;
> +    grant_ref_t ref;
> +    bool_t ret = 0;
> +
> +    ASSERT(&rgt->lock);

I presume you mean spin_is_locked()?  This assertion is currently a
tautology.

~Andrew

> +
> +    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;
> +        
> +        ret = 1;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
>  static void mapcount(
>      struct grant_table *lgt, struct domain *rd, unsigned long mfn,
>      unsigned int *wrc, unsigned int *rdc)

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Andrew Cooper]

On 03/10/14 15:50, 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.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> ---
>
> 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/grant_table.c         |   73 ++++++++++++++++++++++++++++++++++++++
>  xen/include/public/grant_table.h |   19 ++++++++++
>  2 files changed, 92 insertions(+)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 7a6399b..d5bb4f7 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2641,6 +2641,79 @@ do_grant_table_op(
>          }
>          break;
>      }
> +    case GNTTABOP_cache_flush:
> +    {
> +        struct gnttab_cache_flush cflush;
> +        struct domain *d, *owner;
> +        struct page_info *page;
> +        uint64_t mfn;
> +        void *v;
> +
> +        /* currently unimplemented */
> +        if ( count != 1 )
> +            return -ENOSYS;

ENOTSUPP (and elsewhere).

> +
> +        if ( copy_from_guest(&cflush, uop, 1) )
> +            return -EFAULT;
> +
> +        if ( cflush.offset + cflush.size > PAGE_SIZE )
> +            return -EINVAL;

This logic can still be fooled by cleverly crafted overflows, resulting
in garbage being used later, which I am guessing will be a bad thing.

You need to bound check them individually before bounds-checking the
addition of the two.

> +
> +        if ( cflush.size == 0 || cflush.op == 0 )
> +            return 0;
> +
> +        if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) )
> +            return -EINVAL;
> +
> +        /* currently unimplemented */
> +        if ( cflush.a.ref != 0 )
> +            return -ENOSYS;
> +
> +        d = rcu_lock_current_domain();
> +        if ( d == NULL )
> +            return -ESRCH;

rcu_lock_current_domain() cannot possibly fail.

> +
> +        mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT;
> +
> +        if ( !mfn_valid(mfn) )
> +        {
> +            gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", mfn);

PRIx64, as mfn is uint64_t (and elsewhere).

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

EPERM seems better, as it is an ownership check on an existing mfn.

> +        }
> +
> +        spin_lock(&owner->grant_table->lock);

Is this safe without taking the paging lock and doing a dying check on
'owner' ?

> +
> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> +        {
> +            spin_unlock(&owner->grant_table->lock);
> +            put_page(page);
> +            rcu_unlock_domain(d);

These two can be reordered to reduce the length of time the rcu lock is
held.

> +            gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %d\n",
> +                    mfn, owner->domain_id, d->domain_id);

"hasn't been granted by d%d to d%d", to indicate that the numbers are
domids.

> +            return -EINVAL;
> +        }
> +
> +        v = map_domain_page(mfn);
> +        v += cflush.offset;
> +
> +        if ( cflush.op & GNTTAB_CACHE_INVAL )
> +            invalidate_xen_dcache_va_range(v, cflush.size);
> +        if ( cflush.op & GNTTAB_CACHE_CLEAN )
> +            clean_xen_dcache_va_range(v, cflush.size);
> +
> +        unmap_domain_page(v);
> +        spin_unlock(&owner->grant_table->lock);

These two can be reordered to reduce the length of time the grant lock
is held. (especially as it is a remote domains grant lock)

~Andrew

> +        put_page(page);
> +    }
>      default:
>          rc = -ENOSYS;
>          break;
> diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
> index b8a3d6c..1833bba 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,24 @@ 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;
> +    uint32_t offset; /* offset from start of grant */
> +    uint32_t size;   /* size within the grant */
> +#define GNTTAB_CACHE_CLEAN      (1<<0)
> +#define GNTTAB_CACHE_INVAL      (1<<1)
> +    uint32_t op;
> +};
> +typedef struct gnttab_cache_flush gnttab_cache_flush_t;
> +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
> +
>  #endif /* __XEN_INTERFACE_VERSION__ */
>  
>  /*

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini]

On Fri, 3 Oct 2014, Andrew Cooper wrote:
> On 03/10/14 15:50, Stefano Stabellini wrote:
> > 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 v2:
> > - make the function static;
> > - remove the spin_lock.
> > ---
> >  xen/common/grant_table.c |   30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index 23266c3..7a6399b 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -484,6 +484,36 @@ static int _set_status(unsigned gt_version,
> >          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
> >  }
> >  
> > +static bool_t grant_map_exists(struct domain *ld,
> > +                        struct grant_table *rgt,
> > +                        unsigned long mfn)
> > +{
> > +    struct active_grant_entry *act;
> > +    grant_ref_t ref;
> > +    bool_t ret = 0;
> > +
> > +    ASSERT(&rgt->lock);
> 
> I presume you mean spin_is_locked()?  This assertion is currently a
> tautology.

Ooops, you are right.


> ~Andrew
> 
> > +
> > +    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;
> > +        
> > +        ret = 1;
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> >  static void mapcount(
> >      struct grant_table *lgt, struct domain *rd, unsigned long mfn,
> >      unsigned int *wrc, unsigned int *rdc)
> 

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

On Fri, 3 Oct 2014, Andrew Cooper wrote:
> On 03/10/14 15:50, 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.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > ---
> >
> > 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/grant_table.c         |   73 ++++++++++++++++++++++++++++++++++++++
> >  xen/include/public/grant_table.h |   19 ++++++++++
> >  2 files changed, 92 insertions(+)
> >
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index 7a6399b..d5bb4f7 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -2641,6 +2641,79 @@ do_grant_table_op(
> >          }
> >          break;
> >      }
> > +    case GNTTABOP_cache_flush:
> > +    {
> > +        struct gnttab_cache_flush cflush;
> > +        struct domain *d, *owner;
> > +        struct page_info *page;
> > +        uint64_t mfn;
> > +        void *v;
> > +
> > +        /* currently unimplemented */
> > +        if ( count != 1 )
> > +            return -ENOSYS;
> 
> ENOTSUPP (and elsewhere).

ENOTSUPP doesn't seem to be defined in Xen. Should I take the
opportunity to introduce it as 252?


> > +
> > +        if ( copy_from_guest(&cflush, uop, 1) )
> > +            return -EFAULT;
> > +
> > +        if ( cflush.offset + cflush.size > PAGE_SIZE )
> > +            return -EINVAL;
> 
> This logic can still be fooled by cleverly crafted overflows, resulting
> in garbage being used later, which I am guessing will be a bad thing.
> 
> You need to bound check them individually before bounds-checking the
> addition of the two.
> 
> > +
> > +        if ( cflush.size == 0 || cflush.op == 0 )
> > +            return 0;
> > +
> > +        if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) )
> > +            return -EINVAL;
> > +
> > +        /* currently unimplemented */
> > +        if ( cflush.a.ref != 0 )
> > +            return -ENOSYS;
> > +
> > +        d = rcu_lock_current_domain();
> > +        if ( d == NULL )
> > +            return -ESRCH;
> 
> rcu_lock_current_domain() cannot possibly fail.
> 
> > +
> > +        mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT;
> > +
> > +        if ( !mfn_valid(mfn) )
> > +        {
> > +            gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", mfn);
> 
> PRIx64, as mfn is uint64_t (and elsewhere).
> 
> > +            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 -EFAULT;
> 
> EPERM seems better, as it is an ownership check on an existing mfn.
> 
> > +        }
> > +
> > +        spin_lock(&owner->grant_table->lock);
> 
> Is this safe without taking the paging lock and doing a dying check on
> 'owner' ?

In gnttab_release_mappings we do take the grant_table lock before
releasing any entries so it seems to me that it should be sufficient.



> > +
> > +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> > +        {
> > +            spin_unlock(&owner->grant_table->lock);
> > +            put_page(page);
> > +            rcu_unlock_domain(d);
> 
> These two can be reordered to reduce the length of time the rcu lock is
> held.
> 
> > +            gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %d\n",
> > +                    mfn, owner->domain_id, d->domain_id);
> 
> "hasn't been granted by d%d to d%d", to indicate that the numbers are
> domids.
> 
> > +            return -EINVAL;
> > +        }
> > +
> > +        v = map_domain_page(mfn);
> > +        v += cflush.offset;
> > +
> > +        if ( cflush.op & GNTTAB_CACHE_INVAL )
> > +            invalidate_xen_dcache_va_range(v, cflush.size);
> > +        if ( cflush.op & GNTTAB_CACHE_CLEAN )
> > +            clean_xen_dcache_va_range(v, cflush.size);
> > +
> > +        unmap_domain_page(v);
> > +        spin_unlock(&owner->grant_table->lock);
> 
> These two can be reordered to reduce the length of time the grant lock
> is held. (especially as it is a remote domains grant lock)
> 
> ~Andrew
> 
> > +        put_page(page);
> > +    }
> >      default:
> >          rc = -ENOSYS;
> >          break;
> > diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
> > index b8a3d6c..1833bba 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,24 @@ 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;
> > +    uint32_t offset; /* offset from start of grant */
> > +    uint32_t size;   /* size within the grant */
> > +#define GNTTAB_CACHE_CLEAN      (1<<0)
> > +#define GNTTAB_CACHE_INVAL      (1<<1)
> > +    uint32_t op;
> > +};
> > +typedef struct gnttab_cache_flush gnttab_cache_flush_t;
> > +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
> > +
> >  #endif /* __XEN_INTERFACE_VERSION__ */
> >  
> >  /*
> 
> 

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Andrew Cooper]

On 03/10/14 17:33, Stefano Stabellini wrote:
> On Fri, 3 Oct 2014, Andrew Cooper wrote:
>> On 03/10/14 15:50, 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.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>
>>> ---
>>>
>>> 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/grant_table.c         |   73 ++++++++++++++++++++++++++++++++++++++
>>>  xen/include/public/grant_table.h |   19 ++++++++++
>>>  2 files changed, 92 insertions(+)
>>>
>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>>> index 7a6399b..d5bb4f7 100644
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -2641,6 +2641,79 @@ do_grant_table_op(
>>>          }
>>>          break;
>>>      }
>>> +    case GNTTABOP_cache_flush:
>>> +    {
>>> +        struct gnttab_cache_flush cflush;
>>> +        struct domain *d, *owner;
>>> +        struct page_info *page;
>>> +        uint64_t mfn;
>>> +        void *v;
>>> +
>>> +        /* currently unimplemented */
>>> +        if ( count != 1 )
>>> +            return -ENOSYS;
>> ENOTSUPP (and elsewhere).
> ENOTSUPP doesn't seem to be defined in Xen. Should I take the
> opportunity to introduce it as 252?

Sorry. EOPNOTSUPP, 95.

~Andrew

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Ian Campbell]

On Fri, 2014-10-03 at 17:33 +0100, Stefano Stabellini wrote:
> On Fri, 3 Oct 2014, Andrew Cooper wrote:
> > On 03/10/14 15:50, 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.
> > >
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >
> > > ---
> > >
> > > 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/grant_table.c         |   73 ++++++++++++++++++++++++++++++++++++++
> > >  xen/include/public/grant_table.h |   19 ++++++++++
> > >  2 files changed, 92 insertions(+)
> > >
> > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > > index 7a6399b..d5bb4f7 100644
> > > --- a/xen/common/grant_table.c
> > > +++ b/xen/common/grant_table.c
> > > @@ -2641,6 +2641,79 @@ do_grant_table_op(
> > >          }
> > >          break;
> > >      }
> > > +    case GNTTABOP_cache_flush:
> > > +    {
> > > +        struct gnttab_cache_flush cflush;
> > > +        struct domain *d, *owner;
> > > +        struct page_info *page;
> > > +        uint64_t mfn;
> > > +        void *v;
> > > +
> > > +        /* currently unimplemented */
> > > +        if ( count != 1 )
> > > +            return -ENOSYS;
> > 
> > ENOTSUPP (and elsewhere).
> 
> ENOTSUPP doesn't seem to be defined in Xen. Should I take the
> opportunity to introduce it as 252?

Use EOPNOTSUPP I think.

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Jan Beulich]

>>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -484,6 +484,36 @@ static int _set_status(unsigned gt_version,
>          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> +static bool_t grant_map_exists(struct domain *ld,
> +                        struct grant_table *rgt,

Please constify these if possible.

> +                        unsigned long mfn)
> +{
> +    struct active_grant_entry *act;

And this one too.

> +    grant_ref_t ref;
> +    bool_t ret = 0;
> +
> +    ASSERT(&rgt->lock);
> +
> +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )

This loop's worst case iteration count is controlled solely by the
"gnttab_max_nr_frames=" command line option afaict, i.e. for a
large enough specified value this is going to become a security
issue.

> +    {
> +        act = &active_entry(rgt, ref);
> +
> +        if ( !act->pin )
> +            continue;
> +
> +        if ( act->domid != ld->domain_id )
> +            continue;
> +
> +        if ( act->frame != mfn )
> +            continue;
> +        
> +        ret = 1;
> +        break;
> +    }
> +
> +    return ret;
> +}

Apart from it not being very useful to introduce a static function
without consumer, I very much expect this patch on its own to
also cause a build error (unused static function). Please fold this
into the patch adding a consumer of the function.

Jan

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Jan Beulich]

>>> On 03.10.14 at 18:33, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 3 Oct 2014, Andrew Cooper wrote:
>> On 03/10/14 15:50, Stefano Stabellini wrote:
>> > +        }
>> > +
>> > +        spin_lock(&owner->grant_table->lock);
>> 
>> Is this safe without taking the paging lock and doing a dying check on
>> 'owner' ?
> 
> In gnttab_release_mappings we do take the grant_table lock before
> releasing any entries so it seems to me that it should be sufficient.

Now did you check at all when gnttab_release_mappings() gets
called? Only ever on a dying domain (there's even a BUG_ON() to
that effect at the start of the function). Yet here you want to
avoid dying domains, i.e. your point of reference would rather be
gnttab_transfer().

That said, since you take a reference to the page, I don't think
you need to pay attention to whether the page owner is dying.

Jan

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Jan Beulich]

>>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2641,6 +2641,79 @@ do_grant_table_op(
>          }
>          break;
>      }
> +    case GNTTABOP_cache_flush:
> +    {
> +        struct gnttab_cache_flush cflush;
> +        struct domain *d, *owner;
> +        struct page_info *page;
> +        uint64_t mfn;
> +        void *v;
> +
> +        /* currently unimplemented */
> +        if ( count != 1 )
> +            return -ENOSYS;

Didn't you yourself indicate that batching here would be beneficial?

> +
> +        if ( copy_from_guest(&cflush, uop, 1) )
> +            return -EFAULT;
> +
> +        if ( cflush.offset + cflush.size > PAGE_SIZE )
> +            return -EINVAL;
> +
> +        if ( cflush.size == 0 || cflush.op == 0 )
> +            return 0;
> +
> +        if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) )
> +            return -EINVAL;
> +
> +        /* currently unimplemented */
> +        if ( cflush.a.ref != 0 )

You can't check like this - a.ref and a.dev_bus_addr share the same
space in memory. You need a flag indicating whether the union holds
a gref or a busaddr.

> +            return -ENOSYS;
> +
> +        d = rcu_lock_current_domain();
> +        if ( d == NULL )
> +            return -ESRCH;
> +
> +        mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT;
> +
> +        if ( !mfn_valid(mfn) )
> +        {
> +            gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", mfn);

I continue to question the usefulness of this and other printk()s
here.

> +            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 -EFAULT;
> +        }
> +
> +        spin_lock(&owner->grant_table->lock);
> +
> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> +        {
> +            spin_unlock(&owner->grant_table->lock);
> +            put_page(page);
> +            rcu_unlock_domain(d);
> +            gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %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 )
> +            invalidate_xen_dcache_va_range(v, cflush.size);
> +        if ( cflush.op & GNTTAB_CACHE_CLEAN )
> +            clean_xen_dcache_va_range(v, cflush.size);

I already commented on the ordering of these for v1, and it's still
the wrong way round for the case of both flags being set (or at
least you didn't explain why it needs to be in this order).

> @@ -574,6 +575,24 @@ 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;
> +    uint32_t offset; /* offset from start of grant */
> +    uint32_t size;   /* size within the grant */

Elsewhere we use uint16_t for offsets and lengths (and we call
lengths "length", not "size"). Please extend an existing interface
in a consistent way.

> +#define GNTTAB_CACHE_CLEAN      (1<<0)
> +#define GNTTAB_CACHE_INVAL      (1<<1)
> +    uint32_t op;
> +};

The above adjustment will at once make sure the new structure
won't have different size for 32- and 64-bit guests (on x86 at least).
Speaking of which - this patch lacks the necessary adjustment(s) to
common/compat/grant_table.c.

Jan

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -484,6 +484,36 @@ static int _set_status(unsigned gt_version,
> >          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
> >  }
> >  
> > +static bool_t grant_map_exists(struct domain *ld,
> > +                        struct grant_table *rgt,
> 
> Please constify these if possible.
> 
> > +                        unsigned long mfn)
> > +{
> > +    struct active_grant_entry *act;
> 
> And this one too.
> 
> > +    grant_ref_t ref;
> > +    bool_t ret = 0;
> > +
> > +    ASSERT(&rgt->lock);
> > +
> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
> 
> This loop's worst case iteration count is controlled solely by the
> "gnttab_max_nr_frames=" command line option afaict, i.e. for a
> large enough specified value this is going to become a security
> issue.

I am not sure what I could do about this.
Any suggestions?



> > +    {
> > +        act = &active_entry(rgt, ref);
> > +
> > +        if ( !act->pin )
> > +            continue;
> > +
> > +        if ( act->domid != ld->domain_id )
> > +            continue;
> > +
> > +        if ( act->frame != mfn )
> > +            continue;
> > +        
> > +        ret = 1;
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +}
> 
> Apart from it not being very useful to introduce a static function
> without consumer, I very much expect this patch on its own to
> also cause a build error (unused static function). Please fold this
> into the patch adding a consumer of the function.
> 
> Jan
> 

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -484,6 +484,36 @@ static int _set_status(unsigned gt_version,
> >          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
> >  }
> >  
> > +static bool_t grant_map_exists(struct domain *ld,
> > +                        struct grant_table *rgt,
> 
> Please constify these if possible.

rgt is used below as argument for 2 functions that take non-const struct
grant_table *.



> > +                        unsigned long mfn)
> > +{
> > +    struct active_grant_entry *act;
> 
> And this one too.
> 
> > +    grant_ref_t ref;
> > +    bool_t ret = 0;
> > +
> > +    ASSERT(&rgt->lock);
> > +
> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
> 
> This loop's worst case iteration count is controlled solely by the
> "gnttab_max_nr_frames=" command line option afaict, i.e. for a
> large enough specified value this is going to become a security
> issue.
> 
> > +    {
> > +        act = &active_entry(rgt, ref);
> > +
> > +        if ( !act->pin )
> > +            continue;
> > +
> > +        if ( act->domid != ld->domain_id )
> > +            continue;
> > +
> > +        if ( act->frame != mfn )
> > +            continue;
> > +        
> > +        ret = 1;
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +}
> 
> Apart from it not being very useful to introduce a static function
> without consumer, I very much expect this patch on its own to
> also cause a build error (unused static function). Please fold this
> into the patch adding a consumer of the function.
> 
> Jan
> 

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Jan Beulich]

>>> On 06.10.14 at 11:41, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
>> > --- a/xen/common/grant_table.c
>> > +++ b/xen/common/grant_table.c
>> > @@ -484,6 +484,36 @@ static int _set_status(unsigned gt_version,
>> >          return _set_status_v2(domid, readonly, mapflag, shah, act, 
> status);
>> >  }
>> >  
>> > +static bool_t grant_map_exists(struct domain *ld,
>> > +                        struct grant_table *rgt,
>> 
>> Please constify these if possible.
> 
> rgt is used below as argument for 2 functions that take non-const struct
> grant_table *.

nr_grant_entries() could certainly be dealt with, but I guess the
other is the spin_is_locked() that the current version wrongly
omitted. In which case yes, this needs to be kept non-const.

Jan

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Jan Beulich]

>>> On 06.10.14 at 11:37, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
>> > +    grant_ref_t ref;
>> > +    bool_t ret = 0;
>> > +
>> > +    ASSERT(&rgt->lock);
>> > +
>> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
>> 
>> This loop's worst case iteration count is controlled solely by the
>> "gnttab_max_nr_frames=" command line option afaict, i.e. for a
>> large enough specified value this is going to become a security
>> issue.
> 
> I am not sure what I could do about this.
> Any suggestions?

Sadly nothing simple (i.e. other than adding ways to do the reverse
lookup). But I hope you agree that the lack of a good solution can't
be a reason to introduce a security issue. And I'm not really fancying
workarounds (like limiting the maximum) here...

Jan

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 06.10.14 at 11:37, <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
> >> > +    grant_ref_t ref;
> >> > +    bool_t ret = 0;
> >> > +
> >> > +    ASSERT(&rgt->lock);
> >> > +
> >> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
> >> 
> >> This loop's worst case iteration count is controlled solely by the
> >> "gnttab_max_nr_frames=" command line option afaict, i.e. for a
> >> large enough specified value this is going to become a security
> >> issue.
> > 
> > I am not sure what I could do about this.
> > Any suggestions?
> 
> Sadly nothing simple (i.e. other than adding ways to do the reverse
> lookup). But I hope you agree that the lack of a good solution can't
> be a reason to introduce a security issue.

Of course.


> And I'm not really fancying workarounds (like limiting the maximum)
> here...

The other approach to do this, that I avoided because it is slower (2
spinlocks instead of 1), is to use the existing mapcount function.
mapcount is based on:

for ( handle = 0; handle < lgt->maptrack_limit; handle++ )

do you think it is better?
If so, do you think I could avoid taking the rd spin_lock?

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Jan Beulich]

>>> On 06.10.14 at 15:08, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >>> On 06.10.14 at 11:37, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >> >>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
>> >> > +    grant_ref_t ref;
>> >> > +    bool_t ret = 0;
>> >> > +
>> >> > +    ASSERT(&rgt->lock);
>> >> > +
>> >> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
>> >> 
>> >> This loop's worst case iteration count is controlled solely by the
>> >> "gnttab_max_nr_frames=" command line option afaict, i.e. for a
>> >> large enough specified value this is going to become a security
>> >> issue.
>> > 
>> > I am not sure what I could do about this.
>> > Any suggestions?
>> 
>> Sadly nothing simple (i.e. other than adding ways to do the reverse
>> lookup). But I hope you agree that the lack of a good solution can't
>> be a reason to introduce a security issue.
> 
> Of course.
> 
> 
>> And I'm not really fancying workarounds (like limiting the maximum)
>> here...
> 
> The other approach to do this, that I avoided because it is slower (2
> spinlocks instead of 1), is to use the existing mapcount function.
> mapcount is based on:
> 
> for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
> 
> do you think it is better?

No, that's worse: This is the Dom0 side of things, and the loop
bound would still be controlled by that same command line option.

So between the two, the original solution would be better, and
maybe enforcing a lower limit on DomU-s might actually be an
option.

Btw., one more thing I think I forgot in the reply to patch 3:
Since you intentionally only care about remote pages, shouldn't
you check owner != d after page_get_owner_and_reference()?

Jan

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 06.10.14 at 15:08, <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >>> On 06.10.14 at 11:37, <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >> >>> On 03.10.14 at 16:50, <stefano.stabellini@eu.citrix.com> wrote:
> >> >> > +    grant_ref_t ref;
> >> >> > +    bool_t ret = 0;
> >> >> > +
> >> >> > +    ASSERT(&rgt->lock);
> >> >> > +
> >> >> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
> >> >> 
> >> >> This loop's worst case iteration count is controlled solely by the
> >> >> "gnttab_max_nr_frames=" command line option afaict, i.e. for a
> >> >> large enough specified value this is going to become a security
> >> >> issue.
> >> > 
> >> > I am not sure what I could do about this.
> >> > Any suggestions?
> >> 
> >> Sadly nothing simple (i.e. other than adding ways to do the reverse
> >> lookup). But I hope you agree that the lack of a good solution can't
> >> be a reason to introduce a security issue.
> > 
> > Of course.
> > 
> > 
> >> And I'm not really fancying workarounds (like limiting the maximum)
> >> here...
> > 
> > The other approach to do this, that I avoided because it is slower (2
> > spinlocks instead of 1), is to use the existing mapcount function.
> > mapcount is based on:
> > 
> > for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
> > 
> > do you think it is better?
> 
> No, that's worse: This is the Dom0 side of things, and the loop
> bound would still be controlled by that same command line option.
> 
> So between the two, the original solution would be better, and
> maybe enforcing a lower limit on DomU-s might actually be an
> option.

What about keeping track of the highest ref number actually used so far?
I could use that as upper limit instead of the theoretical max. It would
also make the execution faster.


> Btw., one more thing I think I forgot in the reply to patch 3:
> Since you intentionally only care about remote pages, shouldn't
> you check owner != d after page_get_owner_and_reference()?

The problem is that with complex scenarios, such as guest disks over
iscsi, or disk frontend and backend both in dom0, I am afraid that the
"foreign" page could actually end up belonging to the caller domain.
Also it is not really a security issue calling an hypercall to do cache
flush on your own pages, just inefficient. These are the reasons why I
removed that check.

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Jan Beulich]

>>> On 06.10.14 at 15:46, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >>> On 06.10.14 at 15:08, <stefano.stabellini@eu.citrix.com> wrote:
>> > The other approach to do this, that I avoided because it is slower (2
>> > spinlocks instead of 1), is to use the existing mapcount function.
>> > mapcount is based on:
>> > 
>> > for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
>> > 
>> > do you think it is better?
>> 
>> No, that's worse: This is the Dom0 side of things, and the loop
>> bound would still be controlled by that same command line option.
>> 
>> So between the two, the original solution would be better, and
>> maybe enforcing a lower limit on DomU-s might actually be an
>> option.
> 
> What about keeping track of the highest ref number actually used so far?
> I could use that as upper limit instead of the theoretical max. It would
> also make the execution faster.

But the loops are already using a value on the order of the high
watermark, not the theoretical one. I.e. you're not alway looping
for a long time, it is just possible for this to happen.

>> Btw., one more thing I think I forgot in the reply to patch 3:
>> Since you intentionally only care about remote pages, shouldn't
>> you check owner != d after page_get_owner_and_reference()?
> 
> The problem is that with complex scenarios, such as guest disks over
> iscsi, or disk frontend and backend both in dom0, I am afraid that the
> "foreign" page could actually end up belonging to the caller domain.
> Also it is not really a security issue calling an hypercall to do cache
> flush on your own pages, just inefficient. These are the reasons why I
> removed that check.

Makes sense.

Jan

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 06.10.14 at 15:46, <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >>> On 06.10.14 at 15:08, <stefano.stabellini@eu.citrix.com> wrote:
> >> > The other approach to do this, that I avoided because it is slower (2
> >> > spinlocks instead of 1), is to use the existing mapcount function.
> >> > mapcount is based on:
> >> > 
> >> > for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
> >> > 
> >> > do you think it is better?
> >> 
> >> No, that's worse: This is the Dom0 side of things, and the loop
> >> bound would still be controlled by that same command line option.
> >> 
> >> So between the two, the original solution would be better, and
> >> maybe enforcing a lower limit on DomU-s might actually be an
> >> option.
> > 
> > What about keeping track of the highest ref number actually used so far?
> > I could use that as upper limit instead of the theoretical max. It would
> > also make the execution faster.
> 
> But the loops are already using a value on the order of the high
> watermark, not the theoretical one. I.e. you're not alway looping
> for a long time, it is just possible for this to happen.

OK, I understand.

The default max value is 32 pages that lead to 16384 iterations in
grant_map_exists. Should I reduce it?

Given the simple operations in the loop, the function shouldn't actually
be so slow to be a security risk. It should take less that 0.1
milliseconds to complete.

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Jan Beulich]

>>> On 06.10.14 at 16:34, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >>> On 06.10.14 at 15:46, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >> >>> On 06.10.14 at 15:08, <stefano.stabellini@eu.citrix.com> wrote:
>> >> > The other approach to do this, that I avoided because it is slower (2
>> >> > spinlocks instead of 1), is to use the existing mapcount function.
>> >> > mapcount is based on:
>> >> > 
>> >> > for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
>> >> > 
>> >> > do you think it is better?
>> >> 
>> >> No, that's worse: This is the Dom0 side of things, and the loop
>> >> bound would still be controlled by that same command line option.
>> >> 
>> >> So between the two, the original solution would be better, and
>> >> maybe enforcing a lower limit on DomU-s might actually be an
>> >> option.
>> > 
>> > What about keeping track of the highest ref number actually used so far?
>> > I could use that as upper limit instead of the theoretical max. It would
>> > also make the execution faster.
>> 
>> But the loops are already using a value on the order of the high
>> watermark, not the theoretical one. I.e. you're not alway looping
>> for a long time, it is just possible for this to happen.
> 
> OK, I understand.
> 
> The default max value is 32 pages that lead to 16384 iterations in
> grant_map_exists. Should I reduce it?
> 
> Given the simple operations in the loop, the function shouldn't actually
> be so slow to be a security risk. It should take less that 0.1
> milliseconds to complete.

I don't think there's problem with the default (i.e. no reason to reduce
that value). But bumping the limit via command line option should still
result in a secure system. Therefore I think the limit should be split to
separate Dom0 and DomU ones (e.g. along the lines of the
"extra_guest_irqs=" option), and the DomU limit should then get
bounded more strictly. (And I wonder btw whether things work right
even without your patch if passing a sufficiently big value to
"gnttab_max_nr_frames=".)

Jan

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 06.10.14 at 16:34, <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >>> On 06.10.14 at 15:46, <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >> >>> On 06.10.14 at 15:08, <stefano.stabellini@eu.citrix.com> wrote:
> >> >> > The other approach to do this, that I avoided because it is slower (2
> >> >> > spinlocks instead of 1), is to use the existing mapcount function.
> >> >> > mapcount is based on:
> >> >> > 
> >> >> > for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
> >> >> > 
> >> >> > do you think it is better?
> >> >> 
> >> >> No, that's worse: This is the Dom0 side of things, and the loop
> >> >> bound would still be controlled by that same command line option.
> >> >> 
> >> >> So between the two, the original solution would be better, and
> >> >> maybe enforcing a lower limit on DomU-s might actually be an
> >> >> option.
> >> > 
> >> > What about keeping track of the highest ref number actually used so far?
> >> > I could use that as upper limit instead of the theoretical max. It would
> >> > also make the execution faster.
> >> 
> >> But the loops are already using a value on the order of the high
> >> watermark, not the theoretical one. I.e. you're not alway looping
> >> for a long time, it is just possible for this to happen.
> > 
> > OK, I understand.
> > 
> > The default max value is 32 pages that lead to 16384 iterations in
> > grant_map_exists. Should I reduce it?
> > 
> > Given the simple operations in the loop, the function shouldn't actually
> > be so slow to be a security risk. It should take less that 0.1
> > milliseconds to complete.
> 
> I don't think there's problem with the default (i.e. no reason to reduce
> that value). But bumping the limit via command line option should still
> result in a secure system. Therefore I think the limit should be split to
> separate Dom0 and DomU ones (e.g. along the lines of the
> "extra_guest_irqs=" option), and the DomU limit should then get
> bounded more strictly. 

That makes sense.
Would you prefer having the changes as part of this series or separately?


> (And I wonder btw whether things work right
> even without your patch if passing a sufficiently big value to
> "gnttab_max_nr_frames=".)

at the very least mapcount would be badly affected

Re: [PATCH v2 2/4] xen: introduce grant_map_exists

[Jan Beulich]

>>> On 06.10.14 at 16:57, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >>> On 06.10.14 at 16:34, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >> >>> On 06.10.14 at 15:46, <stefano.stabellini@eu.citrix.com> wrote:
>> >> > On Mon, 6 Oct 2014, Jan Beulich wrote:
>> >> >> >>> On 06.10.14 at 15:08, <stefano.stabellini@eu.citrix.com> wrote:
>> >> >> > The other approach to do this, that I avoided because it is slower (2
>> >> >> > spinlocks instead of 1), is to use the existing mapcount function.
>> >> >> > mapcount is based on:
>> >> >> > 
>> >> >> > for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
>> >> >> > 
>> >> >> > do you think it is better?
>> >> >> 
>> >> >> No, that's worse: This is the Dom0 side of things, and the loop
>> >> >> bound would still be controlled by that same command line option.
>> >> >> 
>> >> >> So between the two, the original solution would be better, and
>> >> >> maybe enforcing a lower limit on DomU-s might actually be an
>> >> >> option.
>> >> > 
>> >> > What about keeping track of the highest ref number actually used so far?
>> >> > I could use that as upper limit instead of the theoretical max. It would
>> >> > also make the execution faster.
>> >> 
>> >> But the loops are already using a value on the order of the high
>> >> watermark, not the theoretical one. I.e. you're not alway looping
>> >> for a long time, it is just possible for this to happen.
>> > 
>> > OK, I understand.
>> > 
>> > The default max value is 32 pages that lead to 16384 iterations in
>> > grant_map_exists. Should I reduce it?
>> > 
>> > Given the simple operations in the loop, the function shouldn't actually
>> > be so slow to be a security risk. It should take less that 0.1
>> > milliseconds to complete.
>> 
>> I don't think there's problem with the default (i.e. no reason to reduce
>> that value). But bumping the limit via command line option should still
>> result in a secure system. Therefore I think the limit should be split to
>> separate Dom0 and DomU ones (e.g. along the lines of the
>> "extra_guest_irqs=" option), and the DomU limit should then get
>> bounded more strictly. 
> 
> That makes sense.
> Would you prefer having the changes as part of this series or separately?

It needs to be a prereq to what the series does - including it in the
series would therefore seem reasonable, but not a strict requirement.

Jan

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[David Vrabel]

On 03/10/14 15:50, 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
[...]
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2641,6 +2641,79 @@ do_grant_table_op(
[...]
> +
> +        page = mfn_to_page(mfn);
> +        owner = page_get_owner_and_reference(page);
> +        if ( !owner )
> +        {
> +            rcu_unlock_domain(d);
> +            return -EFAULT;
> +        }
> +
> +        spin_lock(&owner->grant_table->lock);

The grant table lock is already heavily contended,  so you should skip
the lock and the grant_map_exists() check if d == owner.

> +
> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )

Looping over all grant table entries or all maptrack entries looks
expensive to me.

Perhaps consider allowing suitably privileged domains to
clean/invalidate any address without having to check if it's been granted.

Instead of this hypercall, could the guest clean/invalidate by set/way?
 I guess this would need a suitable IPA which could be obtained by some
(offset) 1:1 mapping in the stage 2 tables?

David

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

On Mon, 6 Oct 2014, David Vrabel wrote:
> On 03/10/14 15:50, 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> [...]
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -2641,6 +2641,79 @@ do_grant_table_op(
> [...]
> > +
> > +        page = mfn_to_page(mfn);
> > +        owner = page_get_owner_and_reference(page);
> > +        if ( !owner )
> > +        {
> > +            rcu_unlock_domain(d);
> > +            return -EFAULT;
> > +        }
> > +
> > +        spin_lock(&owner->grant_table->lock);
> 
> The grant table lock is already heavily contended,  so you should skip
> the lock and the grant_map_exists() check if d == owner.

OK, but it is going to make the code uglier.


> > +
> > +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> 
> Looping over all grant table entries or all maptrack entries looks
> expensive to me.
> 
> Perhaps consider allowing suitably privileged domains to
> clean/invalidate any address without having to check if it's been granted.
 
I think that would weaken our security guarantees.


> Instead of this hypercall, could the guest clean/invalidate by set/way?
>  I guess this would need a suitable IPA which could be obtained by some
> (offset) 1:1 mapping in the stage 2 tables?

I wish I could: as the clean/invalidate by set/way is implementation
specific, there is now way to use it to clean a specific range of
addresses.

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[David Vrabel]

On 08/10/14 12:54, Stefano Stabellini wrote:
> On Mon, 6 Oct 2014, David Vrabel wrote:
>> On 03/10/14 15:50, 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.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> [...]
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
[...]
>>> @@ -2641,6 +2641,79 @@ do_grant_table_op(
>>> +
>>> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
>>
>> Looping over all grant table entries or all maptrack entries looks
>> expensive to me.
>>
>> Perhaps consider allowing suitably privileged domains to
>> clean/invalidate any address without having to check if it's been granted.
>  
> I think that would weaken our security guarantees.

If the domain can map the foreign domain's frames by other means (e.g.,
dom0 or a qemu stubdom) then it already has the ability to to
clean/invalidate the cache for arbitrary foreign frames.

David

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Ian Campbell]

On Wed, 2014-10-08 at 12:54 +0100, Stefano Stabellini wrote:
> > Instead of this hypercall, could the guest clean/invalidate by set/way?
> >  I guess this would need a suitable IPA which could be obtained by some
> > (offset) 1:1 mapping in the stage 2 tables?
> 
> I wish I could: as the clean/invalidate by set/way is implementation
> specific, there is now way to use it to clean a specific range of
> addresses.

Clean by set/way is basically unusable except in very specific
circumstances (e.g. tearing down prior to a suspend). It's certainly not
usable on an active SMP system.

http://lists.xen.org/archives/html/xen-devel/2014-10/msg00709.html

Ian.

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[David Vrabel]

On 08/10/14 13:17, Ian Campbell wrote:
> On Wed, 2014-10-08 at 12:54 +0100, Stefano Stabellini wrote:
>>> Instead of this hypercall, could the guest clean/invalidate by set/way?
>>>  I guess this would need a suitable IPA which could be obtained by some
>>> (offset) 1:1 mapping in the stage 2 tables?
>>
>> I wish I could: as the clean/invalidate by set/way is implementation
>> specific, there is now way to use it to clean a specific range of
>> addresses.
> 
> Clean by set/way is basically unusable except in very specific
> circumstances (e.g. tearing down prior to a suspend). It's certainly not
> usable on an active SMP system.
> 
> http://lists.xen.org/archives/html/xen-devel/2014-10/msg00709.html

Yeah, I saw that email after I made my suggestion.

David

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

On Wed, 8 Oct 2014, David Vrabel wrote:
> On 08/10/14 12:54, Stefano Stabellini wrote:
> > On Mon, 6 Oct 2014, David Vrabel wrote:
> >> On 03/10/14 15:50, 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.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >> [...]
> >>> --- a/xen/common/grant_table.c
> >>> +++ b/xen/common/grant_table.c
> [...]
> >>> @@ -2641,6 +2641,79 @@ do_grant_table_op(
> >>> +
> >>> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> >>
> >> Looping over all grant table entries or all maptrack entries looks
> >> expensive to me.
> >>
> >> Perhaps consider allowing suitably privileged domains to
> >> clean/invalidate any address without having to check if it's been granted.
> >  
> > I think that would weaken our security guarantees.
> 
> If the domain can map the foreign domain's frames by other means (e.g.,
> dom0 or a qemu stubdom) then it already has the ability to to
> clean/invalidate the cache for arbitrary foreign frames.

True but the ability of mapping any other domain's memory is something
we could/should get rid of in PV/PVH/ARM only deployments.
Relying on it for page flushing is not going in the right direction.

Re: [PATCH v2 3/4] xen/arm: introduce GNTTABOP_cache_flush

[Ian Campbell]

On Wed, 2014-10-08 at 13:52 +0100, Stefano Stabellini wrote:
> On Wed, 8 Oct 2014, David Vrabel wrote:
> > On 08/10/14 12:54, Stefano Stabellini wrote:
> > > On Mon, 6 Oct 2014, David Vrabel wrote:
> > >> On 03/10/14 15:50, 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.
> > >>>
> > >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >> [...]
> > >>> --- a/xen/common/grant_table.c
> > >>> +++ b/xen/common/grant_table.c
> > [...]
> > >>> @@ -2641,6 +2641,79 @@ do_grant_table_op(
> > >>> +
> > >>> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> > >>
> > >> Looping over all grant table entries or all maptrack entries looks
> > >> expensive to me.
> > >>
> > >> Perhaps consider allowing suitably privileged domains to
> > >> clean/invalidate any address without having to check if it's been granted.
> > >  
> > > I think that would weaken our security guarantees.
> > 
> > If the domain can map the foreign domain's frames by other means (e.g.,
> > dom0 or a qemu stubdom) then it already has the ability to to
> > clean/invalidate the cache for arbitrary foreign frames.
> 
> True but the ability of mapping any other domain's memory is something
> we could/should get rid of in PV/PVH/ARM only deployments.
> Relying on it for page flushing is not going in the right direction.

Agreed.