[PATCH 0/4] xen/arm: introduce XENMEM_cache_flush

[PATCH 0/4] xen/arm: introduce XENMEM_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.



Stefano Stabellini (4):
      xen/arm: introduce invalidate_xen_dcache_va_range
      xen: introduce grant_map_exists
      xen/arm: introduce XENMEM_cache_flush
      Revert "xen/arm: introduce XENFEAT_grant_map_identity"

 xen/arch/arm/mm.c                  |   92 ++++++++++++++++++++++++++++++++++++
 xen/common/grant_table.c           |   62 ++++++++++++++----------
 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/memory.h        |   17 +++++++
 xen/include/xen/grant_table.h      |    4 ++
 10 files changed, 221 insertions(+), 28 deletions(-)

[PATCH 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>
---
 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 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>
CC: jbeulich@suse.com
CC: tim@xen.org
---
 xen/common/grant_table.c      |   32 ++++++++++++++++++++++++++++++++
 xen/include/xen/grant_table.h |    4 ++++
 2 files changed, 36 insertions(+)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 23266c3..e1dc330 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -484,6 +484,38 @@ static int _set_status(unsigned gt_version,
         return _set_status_v2(domid, readonly, mapflag, shah, act, status);
 }
 
+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;
+
+    spin_lock(&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;
+    }
+
+    spin_unlock(&rgt->lock);
+
+    return ret;
+}
+
 static void mapcount(
     struct grant_table *lgt, struct domain *rd, unsigned long mfn,
     unsigned int *wrc, unsigned int *rdc)
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 5941191..2df390f 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -97,6 +97,10 @@ int grant_table_create(
 void grant_table_destroy(
     struct domain *d);
 
+bool_t grant_map_exists(struct domain *ld,
+                        struct grant_table *rgt,
+                        unsigned long mfn);
+
 /* Domain death release of granted mappings of other domains' memory. */
 void
 gnttab_release_mappings(
-- 
1.7.10.4

[PATCH 3/4] xen/arm: introduce XENMEM_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>
---
 xen/arch/arm/mm.c           |   92 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/memory.h |   17 ++++++++
 2 files changed, 109 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c5b48ef..f6139bd 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1134,6 +1134,98 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_get_sharing_shared_pages:
     case XENMEM_get_sharing_freed_pages:
         return 0;
+    case XENMEM_cache_flush:
+    {
+        struct xen_cache_flush cflush;
+        struct domain *d, *owner;
+        struct page_info *page;
+        uint64_t mfn, end;
+        uint64_t offset, size;
+        void *v;
+        int ret = 0;
+
+        if ( copy_from_guest(&cflush, arg, 1) )
+            return -EFAULT;
+
+        d = rcu_lock_current_domain();
+        if ( d == NULL )
+            return -ESRCH;
+
+        if ( (cflush.size >> PAGE_SHIFT) > (1U<<MAX_ORDER) )
+        {
+            printk(XENLOG_G_ERR "invalid size %llx\n", cflush.size);
+            ret = -EINVAL;
+            goto out;
+        }
+
+        if ( cflush.size == 0 || cflush.op == 0 )
+        {
+            ret = 0;
+            goto out;
+        }
+
+        if ( cflush.op & ~(XENMEM_CACHE_INVAL|XENMEM_CACHE_CLEAN) )
+        {
+            printk(XENLOG_G_ERR "invalid op %x\n", cflush.op);
+            ret = -EINVAL;
+            goto out;
+        }
+
+        end = cflush.addr + cflush.size;
+        while ( cflush.addr < end )
+        {
+            mfn = cflush.addr >> PAGE_SHIFT;
+            offset = cflush.addr & ~PAGE_MASK;
+
+            if ( !mfn_valid(mfn) )
+            {
+                printk(XENLOG_G_ERR "mfn=%llx is not valid\n", mfn);
+                ret = -EINVAL;
+                goto out;
+            }
+
+            page = mfn_to_page(mfn);
+            if ( !page )
+            {
+                printk(XENLOG_G_ERR "couldn't get page for mfn %llx\n", mfn);
+                ret =  -EFAULT;
+                goto out;
+            }
+
+            owner = page_get_owner(page);
+            if ( !owner )
+            {
+                printk(XENLOG_G_ERR "couldn't get owner for mfn %llx\n", mfn);
+                ret = -EFAULT;
+                goto out;
+            }
+
+            if ( owner != d && !grant_map_exists(d, owner->grant_table, mfn) )
+            {
+                printk(XENLOG_G_ERR "mfn %llx hasn't been granted by %d to %d\n", mfn, owner->domain_id, d->domain_id);
+                ret = -EINVAL;
+                goto out;
+            }
+
+            v = map_domain_page(mfn);
+            v += offset;
+            size = cflush.size - cflush.addr;
+            if ( size > PAGE_SIZE - offset )
+                size = PAGE_SIZE - offset;
+
+            if ( cflush.op & XENMEM_CACHE_INVAL )
+                invalidate_xen_dcache_va_range(v, size);
+            if ( cflush.op & XENMEM_CACHE_CLEAN )
+                clean_xen_dcache_va_range(v, size);
+            unmap_domain_page(v);
+
+            cflush.addr += PAGE_SIZE - offset;
+        }
+
+out:
+        rcu_unlock_domain(d);
+        return ret;
+    }
 
     default:
         return -ENOSYS;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index db961ec..4a6641d 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -571,6 +571,23 @@ DEFINE_XEN_GUEST_HANDLE(vnuma_topology_info_t);
  */
 #define XENMEM_get_vnumainfo               26
 
+/*
+ * Issue one or more cache maintenance operations on a memory range
+ * owned by the calling domain or granted to the calling domain by a
+ * foreign domain.
+ */
+#define XENMEM_cache_flush                 27
+struct xen_cache_flush {
+    /* addr is the machine address at the start of the memory range */
+    uint64_t addr;
+    uint64_t size;
+#define XENMEM_CACHE_CLEAN      (1<<0)
+#define XENMEM_CACHE_INVAL      (1<<1)
+    uint32_t op;
+};
+typedef struct xen_cache_flush xen_cache_flush_t;
+DEFINE_XEN_GUEST_HANDLE(xen_cache_flush_t);
+
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
 /* Next available subop number is 27 */
-- 
1.7.10.4

[PATCH 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>
---
 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 ++-
 4 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index e1dc330..47a3495 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -770,23 +770,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 )
         {
@@ -983,19 +973,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__ */
 /*
-- 
1.7.10.4

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

[Tim Deegan]

Hi,

At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
> Check whether an mfn has been granted to a given domain on a target
> grant table.

The implementation looks good but I'm concerned about the need for it,
since linear scans of busy grant tables could get fairly expensive.

I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
range.  Should that not take a gfn range?  That would be the idiomatic
interface, and the p2m lookup would tell you whether the guest had
appropiate rights.

I suspect, from reading 0/4, that I'm missing something about the
tangle of non-IOMMU dom0 operations on ARM. :)

Cheers,

Tim

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

[Stefano Stabellini]

On Thu, 2 Oct 2014, Tim Deegan wrote:
> Hi,
> 
> At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
> > Check whether an mfn has been granted to a given domain on a target
> > grant table.
> 
> The implementation looks good but I'm concerned about the need for it,
> since linear scans of busy grant tables could get fairly expensive.
> 
> I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
> range.  Should that not take a gfn range?  That would be the idiomatic
> interface, and the p2m lookup would tell you whether the guest had
> appropiate rights.
> 
> I suspect, from reading 0/4, that I'm missing something about the
> tangle of non-IOMMU dom0 operations on ARM. :)

The hypercall is going to be used to flush foreign pages grant mapped in
Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address
(==mfn).

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

[Jan Beulich]

>>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -484,6 +484,38 @@ static int _set_status(unsigned gt_version,
>          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> +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;
> +
> +    spin_lock(&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;
> +    }
> +
> +    spin_unlock(&rgt->lock);
> +
> +    return ret;

By the time you get here the information you return is stale. Is that
not a problem for the caller? And if it's not, why would it check in the
first place?

Jan

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Jan Beulich]

>>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1134,6 +1134,98 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case XENMEM_get_sharing_shared_pages:
>      case XENMEM_get_sharing_freed_pages:
>          return 0;
> +    case XENMEM_cache_flush:

Blank line missing above this one.

> +    {
> +        struct xen_cache_flush cflush;
> +        struct domain *d, *owner;
> +        struct page_info *page;
> +        uint64_t mfn, end;
> +        uint64_t offset, size;
> +        void *v;
> +        int ret = 0;
> +
> +        if ( copy_from_guest(&cflush, arg, 1) )
> +            return -EFAULT;
> +
> +        d = rcu_lock_current_domain();
> +        if ( d == NULL )
> +            return -ESRCH;

I think this should be the last check prior to the loop below,
simplifying error handling (break instead of goto).

> +
> +        if ( (cflush.size >> PAGE_SHIFT) > (1U<<MAX_ORDER) )

The why is size a uint64_t in the first place?

> +        {
> +            printk(XENLOG_G_ERR "invalid size %llx\n", cflush.size);
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        if ( cflush.size == 0 || cflush.op == 0 )
> +        {
> +            ret = 0;
> +            goto out;
> +        }
> +
> +        if ( cflush.op & ~(XENMEM_CACHE_INVAL|XENMEM_CACHE_CLEAN) )
> +        {
> +            printk(XENLOG_G_ERR "invalid op %x\n", cflush.op);
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        end = cflush.addr + cflush.size;

As said in various contexts before: Such an addition can overflow,
and (even if this doesn't matter here due to the earlier check) you
can't express "the entire address space" with a (base,size) pair
either. Representing arbitrary ranges is better done using inclusive
pairs of addresses/MFNs/whatever.

> +        while ( cflush.addr < end )
> +        {
> +            mfn = cflush.addr >> PAGE_SHIFT;
> +            offset = cflush.addr & ~PAGE_MASK;
> +
> +            if ( !mfn_valid(mfn) )
> +            {
> +                printk(XENLOG_G_ERR "mfn=%llx is not valid\n", mfn);
> +                ret = -EINVAL;
> +                goto out;
> +            }
> +
> +            page = mfn_to_page(mfn);
> +            if ( !page )
> +            {
> +                printk(XENLOG_G_ERR "couldn't get page for mfn %llx\n", mfn);
> +                ret =  -EFAULT;
> +                goto out;
> +            }
> +
> +            owner = page_get_owner(page);
> +            if ( !owner )
> +            {
> +                printk(XENLOG_G_ERR "couldn't get owner for mfn %llx\n", mfn);
> +                ret = -EFAULT;
> +                goto out;
> +            }
> +
> +            if ( owner != d && !grant_map_exists(d, owner->grant_table, mfn) )
> +            {
> +                printk(XENLOG_G_ERR "mfn %llx hasn't been granted by %d to %d\n", mfn, owner->domain_id, d->domain_id);

Long line. But - do you really need all these printk()s?

> +                ret = -EINVAL;
> +                goto out;
> +            }
> +
> +            v = map_domain_page(mfn);
> +            v += offset;
> +            size = cflush.size - cflush.addr;
> +            if ( size > PAGE_SIZE - offset )
> +                size = PAGE_SIZE - offset;
> +
> +            if ( cflush.op & XENMEM_CACHE_INVAL )
> +                invalidate_xen_dcache_va_range(v, size);
> +            if ( cflush.op & XENMEM_CACHE_CLEAN )
> +                clean_xen_dcache_va_range(v, size);

Is this really the right sequence when both flags are set? Of course
I can only guess a what "clean" and "invalidate" here mean (nothing
more specific is being said alongside their definition), but I'd expect
you to want to clean (flush) cache contents _before_ invalidating.

> +            unmap_domain_page(v);
> +
> +            cflush.addr += PAGE_SIZE - offset;
> +        }
> +
> +out:

Labels should be indented by at least one space. But you don't
really need the label anyway if you follow the suggestion above.

Jan

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

[Jan Beulich]

>>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> Just keep the definition of XENFEAT_grant_map_identity.

With the goal of? (I.e. I'd recommend converting its definition to a
comment.)

Jan

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

[Jan Beulich]

>>> On 02.10.14 at 12:42, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 2 Oct 2014, Tim Deegan wrote:
>> At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
>> > Check whether an mfn has been granted to a given domain on a target
>> > grant table.
>> 
>> The implementation looks good but I'm concerned about the need for it,
>> since linear scans of busy grant tables could get fairly expensive.
>> 
>> I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
>> range.  Should that not take a gfn range?  That would be the idiomatic
>> interface, and the p2m lookup would tell you whether the guest had
>> appropiate rights.
>> 
>> I suspect, from reading 0/4, that I'm missing something about the
>> tangle of non-IOMMU dom0 operations on ARM. :)
> 
> The hypercall is going to be used to flush foreign pages grant mapped in
> Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address
> (==mfn).

Don't grant maps get entered into the P2M as they get established?

Jan

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

[Stefano Stabellini]

On Thu, 2 Oct 2014, Jan Beulich wrote:
> >>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -484,6 +484,38 @@ static int _set_status(unsigned gt_version,
> >          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
> >  }
> >  
> > +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;
> > +
> > +    spin_lock(&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;
> > +    }
> > +
> > +    spin_unlock(&rgt->lock);
> > +
> > +    return ret;
> 
> By the time you get here the information you return is stale. Is that
> not a problem for the caller? And if it's not, why would it check in the
> first place?

This is just a security check to make sure that the caller is allowed to
cache flush the memory range. Even if it is a bit stale I think is OK:
it is not possible for the memory to have already been reused somewhere
else.

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Jan Beulich]

>>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1134,6 +1134,98 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case XENMEM_get_sharing_shared_pages:
>      case XENMEM_get_sharing_freed_pages:
>          return 0;
> +    case XENMEM_cache_flush:

Two further notes/questions:

Shouldn't this rather be an arch-independent op right away? I
suppose x86 will need this the moment it gains 32-bit PVH Dom0
support (which will then include non-PAE).

With that, wouldn't the whole operation be a better fit with
the other MMUEXT_* ones instead of being XENMEM_*?

Jan

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

[Stefano Stabellini]

On Thu, 2 Oct 2014, Jan Beulich wrote:
> >>> On 02.10.14 at 12:42, <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 2 Oct 2014, Tim Deegan wrote:
> >> At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
> >> > Check whether an mfn has been granted to a given domain on a target
> >> > grant table.
> >> 
> >> The implementation looks good but I'm concerned about the need for it,
> >> since linear scans of busy grant tables could get fairly expensive.
> >> 
> >> I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
> >> range.  Should that not take a gfn range?  That would be the idiomatic
> >> interface, and the p2m lookup would tell you whether the guest had
> >> appropiate rights.
> >> 
> >> I suspect, from reading 0/4, that I'm missing something about the
> >> tangle of non-IOMMU dom0 operations on ARM. :)
> > 
> > The hypercall is going to be used to flush foreign pages grant mapped in
> > Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address
> > (==mfn).
> 
> Don't grant maps get entered into the P2M as they get established?

Yes, they do. Still dom0 only has an mfn in its hands. Tracking the mfn
to pfn relationship in Linux has been tried unsuccessfully before. Not
only it is slow but it is also difficult as the same page can be granted
multiple times simultaneously.

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Stefano Stabellini]

On Thu, 2 Oct 2014, Jan Beulich wrote:
> >>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1134,6 +1134,98 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
> >      case XENMEM_get_sharing_shared_pages:
> >      case XENMEM_get_sharing_freed_pages:
> >          return 0;
> > +    case XENMEM_cache_flush:
> 
> Two further notes/questions:
> 
> Shouldn't this rather be an arch-independent op right away?

May be a good idea


> I suppose x86 will need this the moment it gains 32-bit PVH Dom0
> support (which will then include non-PAE).

Only if non-coherent devices start appearing on x86 too: coherent
devices don't need cache flush operations. So in theory it could be
useful on x86 but in practice I don't think it would be used, at least
today.


> With that, wouldn't the whole operation be a better fit with
> the other MMUEXT_* ones instead of being XENMEM_*?

Or maybe a GNTTABOP_?
After all we are only interested in flushing foreign pages with the
hypercall. It would still need to take an mfn (or better a dev_bus_addr)
because dom0 doesn't know the grant_ref either.

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

[Tim Deegan]

At 11:42 +0100 on 02 Oct (1412246555), Stefano Stabellini wrote:
> On Thu, 2 Oct 2014, Tim Deegan wrote:
> > Hi,
> > 
> > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
> > > Check whether an mfn has been granted to a given domain on a target
> > > grant table.
> > 
> > The implementation looks good but I'm concerned about the need for it,
> > since linear scans of busy grant tables could get fairly expensive.
> > 
> > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
> > range.  Should that not take a gfn range?  That would be the idiomatic
> > interface, and the p2m lookup would tell you whether the guest had
> > appropiate rights.
> > 
> > I suspect, from reading 0/4, that I'm missing something about the
> > tangle of non-IOMMU dom0 operations on ARM. :)
> 
> The hypercall is going to be used to flush foreign pages grant mapped in
> Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address
> (==mfn).

Blargh.  Silly linux, forgetting its grant handles. :)

So after some IRL discussion with Stefano and IanC, I'm convinced that
the alternatives (linux maintaining per-map lookup tables to get back
from DMA address to grant handle) aren't going to work.  But we can
make this API a bit neater.  I think the design we came up with is:

 - the cache-flush hypercall becomes a grant-table op instead of a
   memop.
 - the argument becomes a 'dev_bus_addr' instead of an MFN.  That is,
   it must be the address returned in the dev_bus_addr field of a
   grant map operation.  If not, it will return EINVAL.
 - the interals are shuffled so that the grant code calls out to
   arch-specific code to do the cache flush, which solves the locking
   issue.  Unlocking before the flush is probably the right thing to
   do anyway, but we can avoid exposing this new grant_map_exists()
   function that might encourage people to write racy code.

Cheers,

Tim.

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

[Jan Beulich]

>>> On 02.10.14 at 13:34, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 2 Oct 2014, Jan Beulich wrote:
>> >>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
>> > --- a/xen/common/grant_table.c
>> > +++ b/xen/common/grant_table.c
>> > @@ -484,6 +484,38 @@ static int _set_status(unsigned gt_version,
>> >          return _set_status_v2(domid, readonly, mapflag, shah, act, 
> status);
>> >  }
>> >  
>> > +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;
>> > +
>> > +    spin_lock(&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;
>> > +    }
>> > +
>> > +    spin_unlock(&rgt->lock);
>> > +
>> > +    return ret;
>> 
>> By the time you get here the information you return is stale. Is that
>> not a problem for the caller? And if it's not, why would it check in the
>> first place?
> 
> This is just a security check to make sure that the caller is allowed to
> cache flush the memory range. Even if it is a bit stale I think is OK:
> it is not possible for the memory to have already been reused somewhere
> else.

Why not? Especially when Xen itself runs virtualized, the time
window is arbitrarily large.

Jan

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

[Jan Beulich]

>>> On 02.10.14 at 13:37, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 2 Oct 2014, Jan Beulich wrote:
>> >>> On 02.10.14 at 12:42, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Thu, 2 Oct 2014, Tim Deegan wrote:
>> >> At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
>> >> > Check whether an mfn has been granted to a given domain on a target
>> >> > grant table.
>> >> 
>> >> The implementation looks good but I'm concerned about the need for it,
>> >> since linear scans of busy grant tables could get fairly expensive.
>> >> 
>> >> I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
>> >> range.  Should that not take a gfn range?  That would be the idiomatic
>> >> interface, and the p2m lookup would tell you whether the guest had
>> >> appropiate rights.
>> >> 
>> >> I suspect, from reading 0/4, that I'm missing something about the
>> >> tangle of non-IOMMU dom0 operations on ARM. :)
>> > 
>> > The hypercall is going to be used to flush foreign pages grant mapped in
>> > Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address
>> > (==mfn).
>> 
>> Don't grant maps get entered into the P2M as they get established?
> 
> Yes, they do. Still dom0 only has an mfn in its hands. Tracking the mfn
> to pfn relationship in Linux has been tried unsuccessfully before. Not
> only it is slow but it is also difficult as the same page can be granted
> multiple times simultaneously.

It can't be that difficult for Dom0 to track at which PFN is maps a
particular grant. And as long as Dom0 serializes properly wrt unmaps,
which GFN of the perhaps multiple ones it picks for doing the cache
flush doesn't even matter.

Jan

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Jan Beulich]

>>> On 02.10.14 at 13:41, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 2 Oct 2014, Jan Beulich wrote:
>> With that, wouldn't the whole operation be a better fit with
>> the other MMUEXT_* ones instead of being XENMEM_*?
> 
> Or maybe a GNTTABOP_?
> After all we are only interested in flushing foreign pages with the
> hypercall. It would still need to take an mfn (or better a dev_bus_addr)
> because dom0 doesn't know the grant_ref either.

If the op is really meant to only be used on grants, then yes,
XENMEM_ as well as MMUEXT_ would seem the wrong ones (but
then you also incorrectly implemented it, as right now it permits
flushes on non-foreign pages).

But again I don't see why Dom0 can't track state for the mappings
it establishes - after all, a grant-ref would be the most natural
thing to pass in here.

Jan

Re: [PATCH 1/4] xen/arm: introduce invalidate_xen_dcache_va_range

[Julien Grall]

Hi Stefano,

n 10/02/2014 11:02 AM, Stefano Stabellini wrote:
> +
> +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 */

I'm wondering if we could relax the dsb(sy) to dsb(ish)?

In any case:

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

Regards,

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Stefano Stabellini]

On Thu, 2 Oct 2014, Jan Beulich wrote:
> >>> On 02.10.14 at 13:41, <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 2 Oct 2014, Jan Beulich wrote:
> >> With that, wouldn't the whole operation be a better fit with
> >> the other MMUEXT_* ones instead of being XENMEM_*?
> > 
> > Or maybe a GNTTABOP_?
> > After all we are only interested in flushing foreign pages with the
> > hypercall. It would still need to take an mfn (or better a dev_bus_addr)
> > because dom0 doesn't know the grant_ref either.
> 
> If the op is really meant to only be used on grants, then yes,
> XENMEM_ as well as MMUEXT_ would seem the wrong ones (but
> then you also incorrectly implemented it, as right now it permits
> flushes on non-foreign pages).
> 
> But again I don't see why Dom0 can't track state for the mappings
> it establishes - after all, a grant-ref would be the most natural
> thing to pass in here.

I agree it would be more natural to pass the grant-ref, but if Linux
knew the grant-ref it wouldn't need the hypercall: it would also know
the pfn and could just perform the flush on the pseudo-physical address.

See drivers/xen/swiotlb-xen.c: the dma_map_ops api only gives us an mfn
at unmap time. Maintaining an mfn_to_pfn (or to grant_ref) tree with
multiple entries per mfn is expensive. The tree would need to be global:
maintained for every grant map/unmap operation, that nowadays happen
extremely often with netfront/netback. In addition the lookup is not
very fast either. We would also need to take a lock to be able to handle
the multiple grants for the same page scenario.

On the other hand we need to call the hypercall only at the end of dma
operations performed on foreign grants with non-coherent devices.
On a well designed board, having SMMUs and/or coherent devices
everywhere, the hypercall would never need to be called.

In the SoCs we have today network cards tend to be coherent and SATA
controllers not to be: I wouldn't want to penalize the network
performance.

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

[Jan Beulich]

>>> On 02.10.14 at 13:41, <tim@xen.org> wrote:
> At 11:42 +0100 on 02 Oct (1412246555), Stefano Stabellini wrote:
>> On Thu, 2 Oct 2014, Tim Deegan wrote:
>> > Hi,
>> > 
>> > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
>> > > Check whether an mfn has been granted to a given domain on a target
>> > > grant table.
>> > 
>> > The implementation looks good but I'm concerned about the need for it,
>> > since linear scans of busy grant tables could get fairly expensive.
>> > 
>> > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
>> > range.  Should that not take a gfn range?  That would be the idiomatic
>> > interface, and the p2m lookup would tell you whether the guest had
>> > appropiate rights.
>> > 
>> > I suspect, from reading 0/4, that I'm missing something about the
>> > tangle of non-IOMMU dom0 operations on ARM. :)
>> 
>> The hypercall is going to be used to flush foreign pages grant mapped in
>> Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address
>> (==mfn).
> 
> Blargh.  Silly linux, forgetting its grant handles. :)
> 
> So after some IRL discussion with Stefano and IanC, I'm convinced that
> the alternatives (linux maintaining per-map lookup tables to get back
> from DMA address to grant handle) aren't going to work.  But we can
> make this API a bit neater.  I think the design we came up with is:
> 
>  - the cache-flush hypercall becomes a grant-table op instead of a
>    memop.
>  - the argument becomes a 'dev_bus_addr' instead of an MFN.  That is,
>    it must be the address returned in the dev_bus_addr field of a
>    grant map operation.  If not, it will return EINVAL.

Then at the very least let's allow for either a dev_bus_addr or
a grant-ref (even if for the moment the latter may yield
-EOPNOTSUPP in order to not spend more time on this than
immediately necessary). That way guests remembering what
they did a few microseconds back can do this in a more well
behaved fashion.

Jan

>  - the interals are shuffled so that the grant code calls out to
>    arch-specific code to do the cache flush, which solves the locking
>    issue.  Unlocking before the flush is probably the right thing to
>    do anyway, but we can avoid exposing this new grant_map_exists()
>    function that might encourage people to write racy code.
> 
> Cheers,
> 
> Tim.

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Jan Beulich]

>>> On 02.10.14 at 13:57, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 2 Oct 2014, Jan Beulich wrote:
>> But again I don't see why Dom0 can't track state for the mappings
>> it establishes - after all, a grant-ref would be the most natural
>> thing to pass in here.
> 
> I agree it would be more natural to pass the grant-ref, but if Linux
> knew the grant-ref it wouldn't need the hypercall: it would also know
> the pfn and could just perform the flush on the pseudo-physical address.
> 
> See drivers/xen/swiotlb-xen.c: the dma_map_ops api only gives us an mfn
> at unmap time. Maintaining an mfn_to_pfn (or to grant_ref) tree with
> multiple entries per mfn is expensive. The tree would need to be global:
> maintained for every grant map/unmap operation, that nowadays happen
> extremely often with netfront/netback. In addition the lookup is not
> very fast either. We would also need to take a lock to be able to handle
> the multiple grants for the same page scenario.

I don't follow: The first thing xen_unmap_single() does is get
the physical address for the passed in DMA one. Furthermore,
if only SWIOTLB code is of concern, then this is limited size,
and hence you could have a xen_io_tlb_nslabs-element array
tracking whatever additional information you may need for
each of the slabs.

Jan

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Julien Grall]

Hi Stefano,

On 10/02/2014 11:02 AM, 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>
> ---
>  xen/arch/arm/mm.c           |   92 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/memory.h |   17 ++++++++
>  2 files changed, 109 insertions(+)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index c5b48ef..f6139bd 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1134,6 +1134,98 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      case XENMEM_get_sharing_shared_pages:
>      case XENMEM_get_sharing_freed_pages:
>          return 0;
> +    case XENMEM_cache_flush:
> +    {
> +        struct xen_cache_flush cflush;
> +        struct domain *d, *owner;
> +        struct page_info *page;
> +        uint64_t mfn, end;
> +        uint64_t offset, size;
> +        void *v;
> +        int ret = 0;
> +
> +        if ( copy_from_guest(&cflush, arg, 1) )
> +            return -EFAULT;
> +
> +        d = rcu_lock_current_domain();
> +        if ( d == NULL )
> +            return -ESRCH;
> +
> +        if ( (cflush.size >> PAGE_SHIFT) > (1U<<MAX_ORDER) )
> +        {
> +            printk(XENLOG_G_ERR "invalid size %llx\n", cflush.size);

This hypercall is only used with the current domain. So I would replace
all printk by gdprintk.

> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        if ( cflush.size == 0 || cflush.op == 0 )
> +        {
> +            ret = 0;
> +            goto out;
> +        }
> +
> +        if ( cflush.op & ~(XENMEM_CACHE_INVAL|XENMEM_CACHE_CLEAN) )
> +        {
> +            printk(XENLOG_G_ERR "invalid op %x\n", cflush.op);
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        end = cflush.addr + cflush.size;
> +        while ( cflush.addr < end )
> +        {
> +            mfn = cflush.addr >> PAGE_SHIFT;
> +            offset = cflush.addr & ~PAGE_MASK;
> +
> +            if ( !mfn_valid(mfn) )
> +            {
> +                printk(XENLOG_G_ERR "mfn=%llx is not valid\n", mfn);
> +                ret = -EINVAL;
> +                goto out;
> +            }
> +
> +            page = mfn_to_page(mfn);
> +            if ( !page )
> +            {
> +                printk(XENLOG_G_ERR "couldn't get page for mfn %llx\n", mfn);
> +                ret =  -EFAULT;
> +                goto out;
> +            }

This check is not necessary as mfn_to_page will never return NULL.

> +
> +            owner = page_get_owner(page);

Don't you need to take a reference on the page?

The foreign guest may decide to drop the mapping while Xen is using it
to clean. I think this could invalidate the owner pointer (for instance
when a guest that is crashing).

I'm also wondering if we need to take a reference on the foreign domain...

[..]

> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index db961ec..4a6641d 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -571,6 +571,23 @@ DEFINE_XEN_GUEST_HANDLE(vnuma_topology_info_t);
>   */
>  #define XENMEM_get_vnumainfo               26
>  
> +/*
> + * Issue one or more cache maintenance operations on a memory range
> + * owned by the calling domain or granted to the calling domain by a
> + * foreign domain.
> + */
> +#define XENMEM_cache_flush                 27
> +struct xen_cache_flush {
> +    /* addr is the machine address at the start of the memory range */
> +    uint64_t addr;
> +    uint64_t size;
> +#define XENMEM_CACHE_CLEAN      (1<<0)
> +#define XENMEM_CACHE_INVAL      (1<<1)
> +    uint32_t op;
> +};
> +typedef struct xen_cache_flush xen_cache_flush_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_cache_flush_t);
> +
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  
>  /* Next available subop number is 27 */

You forgot to update the comment.

Regards,

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Stefano Stabellini]

On Thu, 2 Oct 2014, Jan Beulich wrote:
> >>> On 02.10.14 at 13:57, <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 2 Oct 2014, Jan Beulich wrote:
> >> But again I don't see why Dom0 can't track state for the mappings
> >> it establishes - after all, a grant-ref would be the most natural
> >> thing to pass in here.
> > 
> > I agree it would be more natural to pass the grant-ref, but if Linux
> > knew the grant-ref it wouldn't need the hypercall: it would also know
> > the pfn and could just perform the flush on the pseudo-physical address.
> > 
> > See drivers/xen/swiotlb-xen.c: the dma_map_ops api only gives us an mfn
> > at unmap time. Maintaining an mfn_to_pfn (or to grant_ref) tree with
> > multiple entries per mfn is expensive. The tree would need to be global:
> > maintained for every grant map/unmap operation, that nowadays happen
> > extremely often with netfront/netback. In addition the lookup is not
> > very fast either. We would also need to take a lock to be able to handle
> > the multiple grants for the same page scenario.
> 
> I don't follow: The first thing xen_unmap_single() does is get
> the physical address for the passed in DMA one.

On ARM that would just return the mfn again, because Linux doesn't track
mfn to pfn anymore.


> Furthermore, if only SWIOTLB code is of concern,
  
It is not: the swiotlb-xen functions are called in all cases but the
swiotlb internal buffer is only used as slow path when strictly necessary.
See for example xen_swiotlb_map_page:

if (dma_capable(dev, dev_addr, size) &&
    !range_straddles_page_boundary(phys, size) && !swiotlb_force) {

this check triggers the fast path.


    
> then this is limited
> size, and hence you could have a xen_io_tlb_nslabs-element array
> tracking whatever additional information you may need for each of the
> slabs.
> 
> Jan
> 

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

[Tim Deegan]

At 12:59 +0100 on 02 Oct (1412251189), Jan Beulich wrote:
> >>> On 02.10.14 at 13:41, <tim@xen.org> wrote:
> > Blargh.  Silly linux, forgetting its grant handles. :)
> > 
> > So after some IRL discussion with Stefano and IanC, I'm convinced that
> > the alternatives (linux maintaining per-map lookup tables to get back
> > from DMA address to grant handle) aren't going to work.  But we can
> > make this API a bit neater.  I think the design we came up with is:
> > 
> >  - the cache-flush hypercall becomes a grant-table op instead of a
> >    memop.
> >  - the argument becomes a 'dev_bus_addr' instead of an MFN.  That is,
> >    it must be the address returned in the dev_bus_addr field of a
> >    grant map operation.  If not, it will return EINVAL.
> 
> Then at the very least let's allow for either a dev_bus_addr or
> a grant-ref (even if for the moment the latter may yield
> -EOPNOTSUPP in order to not spend more time on this than
> immediately necessary). That way guests remembering what
> they did a few microseconds back can do this in a more well
> behaved fashion.

Good idea.

Another thing that occurs to me: this should handle transitive grants
as well if possible.

Cheers,

Tim.

Re: [PATCH 1/4] xen/arm: introduce invalidate_xen_dcache_va_range

[Ian Campbell]

On Thu, 2014-10-02 at 11:02 +0100, Stefano Stabellini wrote:
> Take care of handling non-cacheline aligned addresses and sizes.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 3/4] xen/arm: introduce XENMEM_cache_flush

[Ian Campbell]

On Thu, 2014-10-02 at 11:02 +0100, Stefano Stabellini wrote:
> +            if ( owner != d && !grant_map_exists(d, owner->grant_table, mfn) )
> +            {
> +                printk(XENLOG_G_ERR "mfn %llx hasn't been granted by %d to %d\n", mfn, owner->domain_id, d->domain_id);
> +                ret = -EINVAL;
> +                goto out;
> +            }

I know you are rewriting this as a gnttab op but this made me think to
mention: Be sure to consider the case of a loop back grant, i.e.
granting something to yourself. We do occasionally have call to loopback
attach e.g. vbd devices to dom0.

Ian.

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

[Ian Campbell]

On Thu, 2014-10-02 at 12:02 +0100, Jan Beulich wrote:
> >>> On 02.10.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> > Just keep the definition of XENFEAT_grant_map_identity.
> 
> With the goal of? (I.e. I'd recommend converting its definition to a
> comment.)

and probably adding a "Do Not Use" comment.

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

[Stefano Stabellini]

On Thu, 2 Oct 2014, Jan Beulich wrote:
> >>> On 02.10.14 at 13:41, <tim@xen.org> wrote:
> > At 11:42 +0100 on 02 Oct (1412246555), Stefano Stabellini wrote:
> >> On Thu, 2 Oct 2014, Tim Deegan wrote:
> >> > Hi,
> >> > 
> >> > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
> >> > > Check whether an mfn has been granted to a given domain on a target
> >> > > grant table.
> >> > 
> >> > The implementation looks good but I'm concerned about the need for it,
> >> > since linear scans of busy grant tables could get fairly expensive.
> >> > 
> >> > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
> >> > range.  Should that not take a gfn range?  That would be the idiomatic
> >> > interface, and the p2m lookup would tell you whether the guest had
> >> > appropiate rights.
> >> > 
> >> > I suspect, from reading 0/4, that I'm missing something about the
> >> > tangle of non-IOMMU dom0 operations on ARM. :)
> >> 
> >> The hypercall is going to be used to flush foreign pages grant mapped in
> >> Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address
> >> (==mfn).
> > 
> > Blargh.  Silly linux, forgetting its grant handles. :)
> > 
> > So after some IRL discussion with Stefano and IanC, I'm convinced that
> > the alternatives (linux maintaining per-map lookup tables to get back
> > from DMA address to grant handle) aren't going to work.  But we can
> > make this API a bit neater.  I think the design we came up with is:
> > 
> >  - the cache-flush hypercall becomes a grant-table op instead of a
> >    memop.
> >  - the argument becomes a 'dev_bus_addr' instead of an MFN.  That is,
> >    it must be the address returned in the dev_bus_addr field of a
> >    grant map operation.  If not, it will return EINVAL.
> 
> Then at the very least let's allow for either a dev_bus_addr or
> a grant-ref (even if for the moment the latter may yield
> -EOPNOTSUPP in order to not spend more time on this than
> immediately necessary). That way guests remembering what
> they did a few microseconds back can do this in a more well
> behaved fashion.

I can add the gref as a possible parameter in a union, but I would like
to keep the interface multi-page capable. So the gref case is going to
work implicitly only on a single page.

Re: [PATCH 1/4] xen/arm: introduce invalidate_xen_dcache_va_range

[Ian Campbell]

On Thu, 2014-10-02 at 12:57 +0100, Julien Grall wrote:
> Hi Stefano,
> 
> n 10/02/2014 11:02 AM, Stefano Stabellini wrote:
> > +
> > +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 */
> 
> I'm wondering if we could relax the dsb(sy) to dsb(ish)?

We would need to know which cache level the device we are talking to is
coherent with, which we don't know so we have to be conservative.

Also since you only need this hypercall for incoherent devices I suppose
it needs to push things all the way down.

Ian.

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

[Stefano Stabellini]

On Fri, 3 Oct 2014, Stefano Stabellini wrote:
> On Thu, 2 Oct 2014, Jan Beulich wrote:
> > >>> On 02.10.14 at 13:41, <tim@xen.org> wrote:
> > > At 11:42 +0100 on 02 Oct (1412246555), Stefano Stabellini wrote:
> > >> On Thu, 2 Oct 2014, Tim Deegan wrote:
> > >> > Hi,
> > >> > 
> > >> > At 11:02 +0100 on 02 Oct (1412244156), Stefano Stabellini wrote:
> > >> > > Check whether an mfn has been granted to a given domain on a target
> > >> > > grant table.
> > >> > 
> > >> > The implementation looks good but I'm concerned about the need for it,
> > >> > since linear scans of busy grant tables could get fairly expensive.
> > >> > 
> > >> > I see in patch 3/4 you add a hypercall to flush caches, taking an mfn
> > >> > range.  Should that not take a gfn range?  That would be the idiomatic
> > >> > interface, and the p2m lookup would tell you whether the guest had
> > >> > appropiate rights.
> > >> > 
> > >> > I suspect, from reading 0/4, that I'm missing something about the
> > >> > tangle of non-IOMMU dom0 operations on ARM. :)
> > >> 
> > >> The hypercall is going to be used to flush foreign pages grant mapped in
> > >> Dom0. Unfortunately dom0 doesn't know the gfn, only the dma address
> > >> (==mfn).
> > > 
> > > Blargh.  Silly linux, forgetting its grant handles. :)
> > > 
> > > So after some IRL discussion with Stefano and IanC, I'm convinced that
> > > the alternatives (linux maintaining per-map lookup tables to get back
> > > from DMA address to grant handle) aren't going to work.  But we can
> > > make this API a bit neater.  I think the design we came up with is:
> > > 
> > >  - the cache-flush hypercall becomes a grant-table op instead of a
> > >    memop.
> > >  - the argument becomes a 'dev_bus_addr' instead of an MFN.  That is,
> > >    it must be the address returned in the dev_bus_addr field of a
> > >    grant map operation.  If not, it will return EINVAL.
> > 
> > Then at the very least let's allow for either a dev_bus_addr or
> > a grant-ref (even if for the moment the latter may yield
> > -EOPNOTSUPP in order to not spend more time on this than
> > immediately necessary). That way guests remembering what
> > they did a few microseconds back can do this in a more well
> > behaved fashion.
> 
> I can add the gref as a possible parameter in a union, but I would like
> to keep the interface multi-page capable. So the gref case is going to
> work implicitly only on a single page.

I take it back, the hypercall working on a single grant is OK as
otherwise pages wouldn't be contiguous in mfn space and couldn't be used
in a single dma_op.

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

[Jan Beulich]

>>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 10/03/14 3:50 PM >>>
>On Thu, 2 Oct 2014, Jan Beulich wrote:
>> Then at the very least let's allow for either a dev_bus_addr or
>> a grant-ref (even if for the moment the latter may yield
>> -EOPNOTSUPP in order to not spend more time on this than
>> immediately necessary). That way guests remembering what
>> they did a few microseconds back can do this in a more well
>> behaved fashion.
>
>I can add the gref as a possible parameter in a union, but I would like
>to keep the interface multi-page capable. So the gref case is going to
>work implicitly only on a single page.

Imo that's not a reasonable design choice when this is to become a gnttab
op - this should instead use the usual built-in batching most gnttab ops
have to deal with multi-page extents.

Jan