[PATCH v1 00/10] "Non-shared" IOMMU support on ARM

[PATCH v1 00/10] "Non-shared" IOMMU support on ARM

[Oleksandr Tyshchenko]

Hi, all.

The purpose of this patch series is to create a base for porting
any "Non-shared" IOMMUs to Xen on ARM. Saying "Non-shared" IOMMU I mean
the IOMMU that can't share the page table with the CPU.
Primarily, we are interested in IPMMU-VMSA and I hope that it will be the first candidate.
It is VMSA-compatible IOMMU that integrated in the newest Renesas R-Car Gen3 SoCs (ARM).
I will push IPMMU-VMSA support in a while.

With regard to the patch series, it was rebased on Xen 4.9.0-rc2 and based on RFC patch series
I pushed some time ago[1]: 
- Patches 1 and 3 already have Julien's Rb.
- Patches 2, 4, 9 were reworked.
- Patch 5 was left untouched. 
- Patches 6-8 have light/cosmetic changes. 
- Patch 10 is new.

Not really sure about x86-related changes (especially patch 2) since I had no possibility to check.
Hope that I haven't broken anything for x86, but confirmation is needed.

You can find patch series here.
repo: https://github.com/otyshchenko1/xen.git branch: non_shared_iommu_v1

Thank you.

[1] [RFC PATCH 0/9] "Non-shared" IOMMU support on ARM
https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg01905.html

Oleksandr Tyshchenko (10):
  xen/device-tree: Add dt_count_phandle_with_args helper
  iommu: Add extra order argument to the IOMMU APIs and platform
    callbacks
  xen/arm: p2m: Add helper to convert p2m type to IOMMU flags
  xen/arm: p2m: Update IOMMU mapping whenever possible if page table is
    not shared
  iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share
  iommu: Add extra use_iommu argument to iommu_domain_init()
  iommu/arm: Add alloc_page_table platform callback
  iommu: Split iommu_hwdom_init() into arch specific parts
  xen/arm: Add use_iommu flag to xen_arch_domainconfig
  xen/arm: domain_build: Don't expose the "iommus" property to the guest

 tools/libxl/libxl_arm.c                       | 10 +++++
 xen/arch/arm/domain.c                         |  2 +-
 xen/arch/arm/domain_build.c                   |  4 ++
 xen/arch/arm/p2m.c                            | 10 ++++-
 xen/arch/x86/domain.c                         |  2 +-
 xen/arch/x86/mm.c                             | 11 +++--
 xen/arch/x86/mm/p2m-ept.c                     | 21 +---------
 xen/arch/x86/mm/p2m-pt.c                      | 26 ++----------
 xen/arch/x86/mm/p2m.c                         | 38 +++--------------
 xen/arch/x86/x86_64/mm.c                      |  5 ++-
 xen/common/device_tree.c                      |  7 ++++
 xen/common/grant_table.c                      | 10 ++---
 xen/drivers/passthrough/amd/iommu_map.c       | 49 ++++++++++++++++++++--
 xen/drivers/passthrough/amd/pci_amd_iommu.c   |  8 ++--
 xen/drivers/passthrough/arm/iommu.c           | 12 +++++-
 xen/drivers/passthrough/arm/smmu.c            | 44 +++++++++++++++++++-
 xen/drivers/passthrough/iommu.c               | 60 +++++++++------------------
 xen/drivers/passthrough/vtd/iommu.c           | 48 ++++++++++++++++++++-
 xen/drivers/passthrough/vtd/x86/vtd.c         |  4 +-
 xen/drivers/passthrough/x86/iommu.c           | 42 +++++++++++++++++--
 xen/include/asm-arm/iommu.h                   |  7 +++-
 xen/include/asm-arm/p2m.h                     | 34 +++++++++++++++
 xen/include/asm-x86/hvm/svm/amd-iommu-proto.h |  8 ++--
 xen/include/public/arch-arm.h                 |  5 +++
 xen/include/xen/device_tree.h                 | 19 +++++++++
 xen/include/xen/iommu.h                       | 26 ++++++++----
 26 files changed, 353 insertions(+), 159 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Port Linux helper of_count_phandle_with_args for counting
number of phandles in a property.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>

---
   Changes in v1:
      - Add Julien's reviewed-by
---
 xen/common/device_tree.c      |  7 +++++++
 xen/include/xen/device_tree.h | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 7b009ea..60b0095 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1663,6 +1663,13 @@ int dt_parse_phandle_with_args(const struct dt_device_node *np,
                                         index, out_args);
 }
 
+int dt_count_phandle_with_args(const struct dt_device_node *np,
+                               const char *list_name,
+                               const char *cells_name)
+{
+    return __dt_parse_phandle_with_args(np, list_name, cells_name, 0, -1, NULL);
+}
+
 /**
  * unflatten_dt_node - Alloc and populate a device_node from the flat tree
  * @fdt: The parent device tree blob
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 0aecbe0..738f1b6 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -764,6 +764,25 @@ int dt_parse_phandle_with_args(const struct dt_device_node *np,
                                const char *cells_name, int index,
                                struct dt_phandle_args *out_args);
 
+/**
+ * dt_count_phandle_with_args() - Find the number of phandles references in a property
+ * @np: pointer to a device tree node containing a list
+ * @list_name: property name that contains a list
+ * @cells_name: property name that specifies phandles' arguments count
+ *
+ * Returns the number of phandle + argument tuples within a property. It
+ * is a typical pattern to encode a list of phandle and variable
+ * arguments into a single property. The number of arguments is encoded
+ * by a property in the phandle-target node. For example, a gpios
+ * property would contain a list of GPIO specifies consisting of a
+ * phandle and 1 or more arguments. The number of arguments are
+ * determined by the #gpio-cells property in the node pointed to by the
+ * phandle.
+ */
+int dt_count_phandle_with_args(const struct dt_device_node *np,
+                               const char *list_name,
+                               const char *cells_name);
+
 #ifdef CONFIG_DEVICE_TREE_DEBUG
 #define dt_dprintk(fmt, args...)  \
     printk(XENLOG_DEBUG fmt, ## args)
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Replace existing single-page stuff (IOMMU APIs and platform callbacks)
with the multi-page one followed by modifications of all related parts.

These new map_pages/unmap_pages APIs do almost the same thing
as old map_page/unmap_page ones except the formers have extra
order argument and as the result can handle the number of pages.
So have new platform callbacks.

Although the current behavior was retained in all places (I hope),
it should be noted that the rollback logic was moved from the common code
to the IOMMU drivers. Now the IOMMU drivers are responsible for unmapping
already mapped pages if something went wrong during mapping the number
of pages (order > 0).

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>

---
   Changes in v1:
      - Replace existing single-page IOMMU APIs/platform callbacks with
        multi-page ones instead of just keeping both variants of them.
      - Use order argument instead of page_count.
      - Clarify patch subject/description.
---
 xen/arch/x86/mm.c                             | 11 +++---
 xen/arch/x86/mm/p2m-ept.c                     | 21 ++----------
 xen/arch/x86/mm/p2m-pt.c                      | 26 +++-----------
 xen/arch/x86/mm/p2m.c                         | 38 ++++-----------------
 xen/arch/x86/x86_64/mm.c                      |  5 +--
 xen/common/grant_table.c                      | 10 +++---
 xen/drivers/passthrough/amd/iommu_map.c       | 49 +++++++++++++++++++++++++--
 xen/drivers/passthrough/amd/pci_amd_iommu.c   |  8 ++---
 xen/drivers/passthrough/arm/smmu.c            | 41 ++++++++++++++++++++--
 xen/drivers/passthrough/iommu.c               | 21 ++++++------
 xen/drivers/passthrough/vtd/iommu.c           | 48 ++++++++++++++++++++++++--
 xen/drivers/passthrough/vtd/x86/vtd.c         |  4 +--
 xen/drivers/passthrough/x86/iommu.c           |  6 ++--
 xen/include/asm-x86/hvm/svm/amd-iommu-proto.h |  8 +++--
 xen/include/xen/iommu.h                       | 20 ++++++-----
 15 files changed, 195 insertions(+), 121 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 96bc280..c5bc3a5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2623,11 +2623,14 @@ static int __get_page_type(struct page_info *page, unsigned long type,
         if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
         {
             if ( (x & PGT_type_mask) == PGT_writable_page )
-                iommu_ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
+                iommu_ret = iommu_unmap_pages(d,
+                                              mfn_to_gmfn(d, page_to_mfn(page)),
+                                              0);
             else if ( type == PGT_writable_page )
-                iommu_ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
-                                           page_to_mfn(page),
-                                           IOMMUF_readable|IOMMUF_writable);
+                iommu_ret = iommu_map_pages(d,
+                                            mfn_to_gmfn(d, page_to_mfn(page)),
+                                            page_to_mfn(page), 0,
+                                            IOMMUF_readable|IOMMUF_writable);
         }
     }
 
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index f37a1f2..3a4b6b8 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -869,26 +869,9 @@ out:
         else
         {
             if ( iommu_flags )
-                for ( i = 0; i < (1 << order); i++ )
-                {
-                    rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
-                    if ( unlikely(rc) )
-                    {
-                        while ( i-- )
-                            /* If statement to satisfy __must_check. */
-                            if ( iommu_unmap_page(p2m->domain, gfn + i) )
-                                continue;
-
-                        break;
-                    }
-                }
+                rc = iommu_map_pages(d, gfn, mfn_x(mfn), order, iommu_flags);
             else
-                for ( i = 0; i < (1 << order); i++ )
-                {
-                    ret = iommu_unmap_page(d, gfn + i);
-                    if ( !rc )
-                        rc = ret;
-                }
+                rc = iommu_unmap_pages(d, gfn, order);
         }
     }
 
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 5079b59..51f3e10 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -507,7 +507,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
 {
     /* XXX -- this might be able to be faster iff current->domain == d */
     void *table;
-    unsigned long i, gfn_remainder = gfn;
+    unsigned long gfn_remainder = gfn;
     l1_pgentry_t *p2m_entry, entry_content;
     /* Intermediate table to free if we're replacing it with a superpage. */
     l1_pgentry_t intermediate_entry = l1e_empty();
@@ -718,28 +718,10 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
                 amd_iommu_flush_pages(p2m->domain, gfn, page_order);
         }
         else if ( iommu_pte_flags )
-            for ( i = 0; i < (1UL << page_order); i++ )
-            {
-                rc = iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
-                                    iommu_pte_flags);
-                if ( unlikely(rc) )
-                {
-                    while ( i-- )
-                        /* If statement to satisfy __must_check. */
-                        if ( iommu_unmap_page(p2m->domain, gfn + i) )
-                            continue;
-
-                    break;
-                }
-            }
+            rc = iommu_map_pages(p2m->domain, gfn, mfn_x(mfn), page_order,
+                                 iommu_pte_flags);
         else
-            for ( i = 0; i < (1UL << page_order); i++ )
-            {
-                int ret = iommu_unmap_page(p2m->domain, gfn + i);
-
-                if ( !rc )
-                    rc = ret;
-            }
+            rc = iommu_unmap_pages(p2m->domain, gfn, page_order);
     }
 
     /*
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 1d57e5c..15ba71d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -705,20 +705,9 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
 
     if ( !paging_mode_translate(p2m->domain) )
     {
-        int rc = 0;
-
         if ( need_iommu(p2m->domain) )
-        {
-            for ( i = 0; i < (1 << page_order); i++ )
-            {
-                int ret = iommu_unmap_page(p2m->domain, mfn + i);
-
-                if ( !rc )
-                    rc = ret;
-            }
-        }
-
-        return rc;
+            return iommu_unmap_pages(p2m->domain, mfn, page_order);
+        return 0;
     }
 
     ASSERT(gfn_locked_by_me(p2m, gfn));
@@ -765,23 +754,8 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
     if ( !paging_mode_translate(d) )
     {
         if ( need_iommu(d) && t == p2m_ram_rw )
-        {
-            for ( i = 0; i < (1 << page_order); i++ )
-            {
-                rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)),
-                                    mfn_x(mfn_add(mfn, i)),
-                                    IOMMUF_readable|IOMMUF_writable);
-                if ( rc != 0 )
-                {
-                    while ( i-- > 0 )
-                        /* If statement to satisfy __must_check. */
-                        if ( iommu_unmap_page(d, mfn_x(mfn_add(mfn, i))) )
-                            continue;
-
-                    return rc;
-                }
-            }
-        }
+            return iommu_map_pages(d, mfn_x(mfn), mfn_x(mfn), page_order,
+                                   IOMMUF_readable|IOMMUF_writable);
         return 0;
     }
 
@@ -1134,7 +1108,7 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
     {
         if ( !need_iommu(d) )
             return 0;
-        return iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
+        return iommu_map_pages(d, gfn, gfn, 0, IOMMUF_readable|IOMMUF_writable);
     }
 
     gfn_lock(p2m, gfn, 0);
@@ -1222,7 +1196,7 @@ int clear_identity_p2m_entry(struct domain *d, unsigned long gfn)
     {
         if ( !need_iommu(d) )
             return 0;
-        return iommu_unmap_page(d, gfn);
+        return iommu_unmap_pages(d, gfn, 0);
     }
 
     gfn_lock(p2m, gfn, 0);
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 34f3250..24aaf88 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1443,13 +1443,14 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
     if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) )
     {
         for ( i = spfn; i < epfn; i++ )
-            if ( iommu_map_page(hardware_domain, i, i, IOMMUF_readable|IOMMUF_writable) )
+            if ( iommu_map_pages(hardware_domain, i, i, 0,
+                                 IOMMUF_readable|IOMMUF_writable) )
                 break;
         if ( i != epfn )
         {
             while (i-- > old_max)
                 /* If statement to satisfy __must_check. */
-                if ( iommu_unmap_page(hardware_domain, i) )
+                if ( iommu_unmap_pages(hardware_domain, i, 0) )
                     continue;
 
             goto destroy_m2p;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 4fe9544..ecf8f82 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -964,13 +964,13 @@ __gnttab_map_grant_ref(
              !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
         {
             if ( !(kind & MAPKIND_WRITE) )
-                err = iommu_map_page(ld, frame, frame,
-                                     IOMMUF_readable|IOMMUF_writable);
+                err = iommu_map_pages(ld, frame, frame, 0,
+                                      IOMMUF_readable|IOMMUF_writable);
         }
         else if ( act_pin && !old_pin )
         {
             if ( !kind )
-                err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
+                err = iommu_map_pages(ld, frame, frame, 0, IOMMUF_readable);
         }
         if ( err )
         {
@@ -1190,9 +1190,9 @@ __gnttab_unmap_common(
 
         kind = mapkind(lgt, rd, op->frame);
         if ( !kind )
-            err = iommu_unmap_page(ld, op->frame);
+            err = iommu_unmap_pages(ld, op->frame, 0);
         else if ( !(kind & MAPKIND_WRITE) )
-            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
+            err = iommu_map_pages(ld, op->frame, op->frame, 0, IOMMUF_readable);
 
         double_gt_unlock(lgt, rgt);
 
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index fd2327d..87ab99c 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -631,8 +631,9 @@ static int update_paging_mode(struct domain *d, unsigned long gfn)
     return 0;
 }
 
-int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
-                       unsigned int flags)
+static int __must_check amd_iommu_map_page(struct domain *d, unsigned long gfn,
+                                           unsigned long mfn,
+                                           unsigned int flags)
 {
     bool_t need_flush = 0;
     struct domain_iommu *hd = dom_iommu(d);
@@ -720,7 +721,8 @@ out:
     return 0;
 }
 
-int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
+static int __must_check amd_iommu_unmap_page(struct domain *d,
+                                             unsigned long gfn)
 {
     unsigned long pt_mfn[7];
     struct domain_iommu *hd = dom_iommu(d);
@@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
     return 0;
 }
 
+/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
+int __must_check amd_iommu_map_pages(struct domain *d, unsigned long gfn,
+                                     unsigned long mfn, unsigned int order,
+                                     unsigned int flags)
+{
+    unsigned long i;
+    int rc = 0;
+
+    for ( i = 0; i < (1UL << order); i++ )
+    {
+        rc = amd_iommu_map_page(d, gfn + i, mfn + i, flags);
+        if ( unlikely(rc) )
+        {
+            while ( i-- )
+                /* If statement to satisfy __must_check. */
+                if ( amd_iommu_unmap_page(d, gfn + i) )
+                    continue;
+
+            break;
+        }
+    }
+
+    return rc;
+}
+
+int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn,
+                                       unsigned int order)
+{
+    unsigned long i;
+    int rc = 0;
+
+    for ( i = 0; i < (1UL << order); i++ )
+    {
+        int ret = amd_iommu_unmap_page(d, gfn + i);
+        if ( !rc )
+            rc = ret;
+    }
+
+    return rc;
+}
+
 int amd_iommu_reserve_domain_unity_map(struct domain *domain,
                                        u64 phys_addr,
                                        unsigned long size, int iw, int ir)
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 8c25110..fe744d2 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -296,8 +296,8 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
              */
             if ( mfn_valid(_mfn(pfn)) )
             {
-                int ret = amd_iommu_map_page(d, pfn, pfn,
-                                             IOMMUF_readable|IOMMUF_writable);
+                int ret = amd_iommu_map_pages(d, pfn, pfn, 0,
+                                              IOMMUF_readable|IOMMUF_writable);
 
                 if ( !rc )
                     rc = ret;
@@ -620,8 +620,8 @@ const struct iommu_ops amd_iommu_ops = {
     .remove_device = amd_iommu_remove_device,
     .assign_device  = amd_iommu_assign_device,
     .teardown = amd_iommu_domain_destroy,
-    .map_page = amd_iommu_map_page,
-    .unmap_page = amd_iommu_unmap_page,
+    .map_pages = amd_iommu_map_pages,
+    .unmap_pages = amd_iommu_unmap_pages,
     .free_page_table = deallocate_page_table,
     .reassign_device = reassign_device,
     .get_device_group_id = amd_iommu_group_id,
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 1082fcf..527a592 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2780,6 +2780,43 @@ static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
 	return 0;
 }
 
+/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
+static int __must_check arm_smmu_map_pages(struct domain *d, unsigned long gfn,
+		unsigned long mfn, unsigned int order, unsigned int flags)
+{
+	unsigned long i;
+	int rc = 0;
+
+	for (i = 0; i < (1UL << order); i++) {
+		rc = arm_smmu_map_page(d, gfn + i, mfn + i, flags);
+		if (unlikely(rc)) {
+			while (i--)
+				/* If statement to satisfy __must_check. */
+				if (arm_smmu_unmap_page(d, gfn + i))
+					continue;
+
+			break;
+		}
+	}
+
+	return rc;
+}
+
+static int __must_check arm_smmu_unmap_pages(struct domain *d,
+		unsigned long gfn, unsigned int order)
+{
+	unsigned long i;
+	int rc = 0;
+
+	for (i = 0; i < (1UL << order); i++) {
+		int ret = arm_smmu_unmap_page(d, gfn + i);
+		if (!rc)
+			rc = ret;
+	}
+
+	return rc;
+}
+
 static const struct iommu_ops arm_smmu_iommu_ops = {
     .init = arm_smmu_iommu_domain_init,
     .hwdom_init = arm_smmu_iommu_hwdom_init,
@@ -2788,8 +2825,8 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
     .assign_device = arm_smmu_assign_dev,
     .reassign_device = arm_smmu_reassign_dev,
-    .map_page = arm_smmu_map_page,
-    .unmap_page = arm_smmu_unmap_page,
+    .map_pages = arm_smmu_map_pages,
+    .unmap_pages = arm_smmu_unmap_pages,
 };
 
 static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5e81813..3e9e4c3 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -188,7 +188,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
                   == PGT_writable_page) )
                 mapping |= IOMMUF_writable;
 
-            ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
+            ret = hd->platform_ops->map_pages(d, gfn, mfn, 0, mapping);
             if ( !rc )
                 rc = ret;
 
@@ -249,8 +249,8 @@ void iommu_domain_destroy(struct domain *d)
     arch_iommu_domain_destroy(d);
 }
 
-int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
-                   unsigned int flags)
+int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long mfn,
+                    unsigned int order, unsigned int flags)
 {
     const struct domain_iommu *hd = dom_iommu(d);
     int rc;
@@ -258,13 +258,13 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
+    rc = hd->platform_ops->map_pages(d, gfn, mfn, order, flags);
     if ( unlikely(rc) )
     {
         if ( !d->is_shutting_down && printk_ratelimit() )
             printk(XENLOG_ERR
-                   "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d\n",
-                   d->domain_id, gfn, mfn, rc);
+                   "d%d: IOMMU mapping gfn %#lx to mfn %#lx order %u failed: %d\n",
+                   d->domain_id, gfn, mfn, order, rc);
 
         if ( !is_hardware_domain(d) )
             domain_crash(d);
@@ -273,7 +273,8 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
     return rc;
 }
 
-int iommu_unmap_page(struct domain *d, unsigned long gfn)
+int iommu_unmap_pages(struct domain *d, unsigned long gfn,
+                      unsigned int order)
 {
     const struct domain_iommu *hd = dom_iommu(d);
     int rc;
@@ -281,13 +282,13 @@ int iommu_unmap_page(struct domain *d, unsigned long gfn)
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    rc = hd->platform_ops->unmap_page(d, gfn);
+    rc = hd->platform_ops->unmap_pages(d, gfn, order);
     if ( unlikely(rc) )
     {
         if ( !d->is_shutting_down && printk_ratelimit() )
             printk(XENLOG_ERR
-                   "d%d: IOMMU unmapping gfn %#lx failed: %d\n",
-                   d->domain_id, gfn, rc);
+                   "d%d: IOMMU unmapping gfn %#lx order %u failed: %d\n",
+                   d->domain_id, gfn, order, rc);
 
         if ( !is_hardware_domain(d) )
             domain_crash(d);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index a5c61c6..6c7f4c6 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1816,6 +1816,50 @@ static int __must_check intel_iommu_unmap_page(struct domain *d,
     return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K);
 }
 
+/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
+static int __must_check intel_iommu_map_pages(struct domain *d,
+                                              unsigned long gfn,
+                                              unsigned long mfn,
+                                              unsigned int order,
+                                              unsigned int flags)
+{
+    unsigned long i;
+    int rc = 0;
+
+    for ( i = 0; i < (1UL << order); i++ )
+    {
+        rc = intel_iommu_map_page(d, gfn + i, mfn + i, flags);
+        if ( unlikely(rc) )
+        {
+            while ( i-- )
+                /* If statement to satisfy __must_check. */
+                if ( intel_iommu_unmap_page(d, gfn + i) )
+                    continue;
+
+            break;
+        }
+    }
+
+    return rc;
+}
+
+static int __must_check intel_iommu_unmap_pages(struct domain *d,
+                                                unsigned long gfn,
+                                                unsigned int order)
+{
+    unsigned long i;
+    int rc = 0;
+
+    for ( i = 0; i < (1UL << order); i++ )
+    {
+        int ret = intel_iommu_unmap_page(d, gfn + i);
+        if ( !rc )
+            rc = ret;
+    }
+
+    return rc;
+}
+
 int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
                     int order, int present)
 {
@@ -2639,8 +2683,8 @@ const struct iommu_ops intel_iommu_ops = {
     .remove_device = intel_iommu_remove_device,
     .assign_device  = intel_iommu_assign_device,
     .teardown = iommu_domain_teardown,
-    .map_page = intel_iommu_map_page,
-    .unmap_page = intel_iommu_unmap_page,
+    .map_pages = intel_iommu_map_pages,
+    .unmap_pages = intel_iommu_unmap_pages,
     .free_page_table = iommu_free_page_table,
     .reassign_device = reassign_device_ownership,
     .get_device_group_id = intel_iommu_group_id,
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index 88a60b3..62a6ee6 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -143,8 +143,8 @@ void __hwdom_init vtd_set_hwdom_mapping(struct domain *d)
         tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
         for ( j = 0; j < tmp; j++ )
         {
-            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
-                                     IOMMUF_readable|IOMMUF_writable);
+            int ret = iommu_map_pages(d, pfn * tmp + j, pfn * tmp + j, 0,
+                                      IOMMUF_readable|IOMMUF_writable);
 
             if ( !rc )
                rc = ret;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 0253823..973b72f 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -65,9 +65,9 @@ int arch_iommu_populate_page_table(struct domain *d)
             {
                 ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
                 BUG_ON(SHARED_M2P(gfn));
-                rc = hd->platform_ops->map_page(d, gfn, mfn,
-                                                IOMMUF_readable |
-                                                IOMMUF_writable);
+                rc = hd->platform_ops->map_pages(d, gfn, mfn, 0,
+                                                 IOMMUF_readable |
+                                                 IOMMUF_writable);
             }
             if ( rc )
             {
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
index 99bc21c..8f44489 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -52,9 +52,11 @@ int amd_iommu_init(void);
 int amd_iommu_update_ivrs_mapping_acpi(void);
 
 /* mapping functions */
-int __must_check amd_iommu_map_page(struct domain *d, unsigned long gfn,
-                                    unsigned long mfn, unsigned int flags);
-int __must_check amd_iommu_unmap_page(struct domain *d, unsigned long gfn);
+int __must_check amd_iommu_map_pages(struct domain *d, unsigned long gfn,
+                                     unsigned long mfn, unsigned int order,
+                                     unsigned int flags);
+int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn,
+                                       unsigned int order);
 u64 amd_iommu_get_next_table_from_pte(u32 *entry);
 int __must_check amd_iommu_alloc_root(struct domain_iommu *hd);
 int amd_iommu_reserve_domain_unity_map(struct domain *domain,
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5803e3f..3297998 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -71,14 +71,16 @@ int iommu_construct(struct domain *d);
 /* Function used internally, use iommu_domain_destroy */
 void iommu_teardown(struct domain *d);
 
-/* iommu_map_page() takes flags to direct the mapping operation. */
+/* iommu_map_pages() takes flags to direct the mapping operation. */
 #define _IOMMUF_readable 0
 #define IOMMUF_readable  (1u<<_IOMMUF_readable)
 #define _IOMMUF_writable 1
 #define IOMMUF_writable  (1u<<_IOMMUF_writable)
-int __must_check iommu_map_page(struct domain *d, unsigned long gfn,
-                                unsigned long mfn, unsigned int flags);
-int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn);
+int __must_check iommu_map_pages(struct domain *d, unsigned long gfn,
+                                 unsigned long mfn, unsigned int order,
+                                 unsigned int flags);
+int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn,
+                                   unsigned int order);
 
 enum iommu_feature
 {
@@ -168,9 +170,11 @@ struct iommu_ops {
 #endif /* HAS_PCI */
 
     void (*teardown)(struct domain *d);
-    int __must_check (*map_page)(struct domain *d, unsigned long gfn,
-                                 unsigned long mfn, unsigned int flags);
-    int __must_check (*unmap_page)(struct domain *d, unsigned long gfn);
+    int __must_check (*map_pages)(struct domain *d, unsigned long gfn,
+                                  unsigned long mfn, unsigned int order,
+                                  unsigned int flags);
+    int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
+                                    unsigned int order);
     void (*free_page_table)(struct page_info *);
 #ifdef CONFIG_X86
     void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
@@ -213,7 +217,7 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev);
  * The purpose of the iommu_dont_flush_iotlb optional cpu flag is to
  * avoid unecessary iotlb_flush in the low level IOMMU code.
  *
- * iommu_map_page/iommu_unmap_page must flush the iotlb but somethimes
+ * iommu_map_pages/iommu_unmap_pages must flush the iotlb but somethimes
  * this operation can be really expensive. This flag will be set by the
  * caller to notify the low level IOMMU code to avoid the iotlb flushes.
  * iommu_iotlb_flush/iommu_iotlb_flush_all will be explicitly called by
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 03/10] xen/arm: p2m: Add helper to convert p2m type to IOMMU flags

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The helper has the same purpose as existing for x86 one.
It is used for choosing IOMMU mapping attribute according to
the memory type.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>

---
   Changes in v1:
      - Add Julien's reviewed-by
---
 xen/include/asm-arm/p2m.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 18c57f9..9082ba0 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -5,6 +5,7 @@
 #include <xen/radix-tree.h>
 #include <xen/rwlock.h>
 #include <xen/mem_access.h>
+#include <xen/iommu.h>
 #include <public/vm_event.h> /* for vm_event_response_t */
 #include <public/memory.h>
 #include <xen/p2m-common.h>
@@ -357,6 +358,39 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn, unsigned int order)
     return gfn_add(gfn, 1UL << order);
 }
 
+/*
+ * p2m type to IOMMU flags
+ */
+static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
+{
+    unsigned int flags;
+
+    switch( p2mt )
+    {
+    case p2m_ram_rw:
+    case p2m_iommu_map_rw:
+    case p2m_map_foreign:
+    case p2m_grant_map_rw:
+    case p2m_mmio_direct_dev:
+    case p2m_mmio_direct_nc:
+    case p2m_mmio_direct_c:
+        flags = IOMMUF_readable | IOMMUF_writable;
+        break;
+    case p2m_ram_ro:
+    case p2m_iommu_map_ro:
+    case p2m_grant_map_ro:
+        flags = IOMMUF_readable;
+        break;
+    default:
+        flags = 0;
+        break;
+    }
+
+    /* TODO Do we need to handle access permissions here? */
+
+    return flags;
+}
+
 #endif /* _XEN_P2M_H */
 
 /*
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Update IOMMU mapping if the IOMMU doesn't share page table with the CPU.
The best place to do so on ARM is __p2m_set_entry(). Use mfn as an indicator
of the required action. If mfn is valid call iommu_map_pages(),
otherwise - iommu_unmap_pages().

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Julien Grall <julien.grall@arm.com>

---
   Changes in v1:
      - Update IOMMU mapping in __p2m_set_entry() instead of p2m_set_entry().
      - Pass order argument to IOMMU APIs instead of page_count.
---
 xen/arch/arm/p2m.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 34d5776..9ca491b 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -984,7 +984,15 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
         p2m_free_entry(p2m, orig_pte, level);
 
     if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) )
-        rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
+    {
+        if ( iommu_use_hap_pt(p2m->domain) )
+            rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
+        else if ( !mfn_eq(smfn, INVALID_MFN) )
+            rc = iommu_map_pages(p2m->domain, gfn_x(sgfn), mfn_x(smfn),
+                                 page_order, p2m_get_iommu_flags(t));
+        else
+            rc = iommu_unmap_pages(p2m->domain, gfn_x(sgfn), page_order);
+    }
     else
         rc = 0;
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Not every integrated into ARM SoCs IOMMU can share page tables
with the CPU and as result the iommu_use_hap_pt(d) is not always true.
Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU
page table is shared or not.

Now all IOMMU drivers on ARM are able to change this flag
according to their possibilities like x86-variants do.
Therefore set iommu_hap_pt_share flag for SMMU because it always shares
page table with the CPU.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 xen/drivers/passthrough/arm/smmu.c | 3 +++
 xen/include/asm-arm/iommu.h        | 7 +++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 527a592..86ee12a 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev,
 
 	platform_features &= smmu->features;
 
+	/* Always share P2M table between the CPU and the SMMU */
+	iommu_hap_pt_share = true;
+
 	return 0;
 }
 
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index 57d9b1e..10a6f23 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -20,8 +20,11 @@ struct arch_iommu
     void *priv;
 };
 
-/* Always share P2M Table between the CPU and the IOMMU */
-#define iommu_use_hap_pt(d) (1)
+/*
+ * The ARM domain always has a P2M table, but not every integrated into
+ * ARM SoCs IOMMU can use it as page table.
+ */
+#define iommu_use_hap_pt(d) (iommu_hap_pt_share)
 
 const struct iommu_ops *iommu_get_ops(void);
 void __init iommu_set_ops(const struct iommu_ops *ops);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The presence of this flag lets us know that the guest
has devices which will most likely be used for passthrough
and as the result the use of IOMMU is expected for this domain.
In that case we have to call iommu_construct(), actually
what the real assign_device call usually does.

As iommu_domain_init() is called with use_iommu flag being forced
to false for now, no functional change is intended for both ARM and x86.

Basically, this patch is needed for non-shared IOMMUs on ARM only
since the non-shared IOMMUs on x86 are ok if iommu_construct() is called
later. But, in order to be more generic and for possible future optimization
make this change appliable for both platforms.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Jan Beulich <jbeulich@suse.com>

---
   Changes in v1:
      - Clarify patch subject/description.
      - s/bool_t/bool/
---
 xen/arch/arm/domain.c           |  2 +-
 xen/arch/x86/domain.c           |  2 +-
 xen/drivers/passthrough/iommu.c | 11 +++++++++--
 xen/include/xen/iommu.h         |  2 +-
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 76310ed..ec19310 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     ASSERT(config != NULL);
 
     /* p2m_init relies on some value initialized by the IOMMU subsystem */
-    if ( (rc = iommu_domain_init(d)) != 0 )
+    if ( (rc = iommu_domain_init(d, false)) != 0 )
         goto fail;
 
     if ( (rc = p2m_init(d)) != 0 )
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 90e2b1f..54037af 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -641,7 +641,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
         if ( (rc = init_domain_irq_mapping(d)) != 0 )
             goto fail;
 
-        if ( (rc = iommu_domain_init(d)) != 0 )
+        if ( (rc = iommu_domain_init(d, false)) != 0 )
             goto fail;
     }
     spin_lock_init(&d->arch.e820_lock);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 3e9e4c3..c85f7b4 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -129,7 +129,7 @@ static void __init parse_iommu_param(char *s)
     } while ( ss );
 }
 
-int iommu_domain_init(struct domain *d)
+int iommu_domain_init(struct domain *d, bool use_iommu)
 {
     struct domain_iommu *hd = dom_iommu(d);
     int ret = 0;
@@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d)
         return 0;
 
     hd->platform_ops = iommu_get_ops();
-    return hd->platform_ops->init(d);
+    ret = hd->platform_ops->init(d);
+    if ( ret )
+        return ret;
+
+    if ( use_iommu && !is_hardware_domain(d) )
+        ret = iommu_construct(d);
+
+    return ret;
 }
 
 static void __hwdom_init check_hwdom_reqs(struct domain *d)
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 3297998..3afbc3b 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -56,7 +56,7 @@ int iommu_setup(void);
 int iommu_add_device(struct pci_dev *pdev);
 int iommu_enable_device(struct pci_dev *pdev);
 int iommu_remove_device(struct pci_dev *pdev);
-int iommu_domain_init(struct domain *d);
+int iommu_domain_init(struct domain *d, bool use_iommu);
 void iommu_hwdom_init(struct domain *d);
 void iommu_domain_destroy(struct domain *d);
 int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The alloc_page_table callback is a mandatory thing
for the IOMMUs that don't share page table with the CPU on ARM.
The non-shared IOMMUs have to perform all required actions here
to be ready to handle IOMMU mapping updates right after completing it.

The arch_iommu_populate_page_table() seems an appropriate place
to call newly created callback.
Since we will only be here for the non-shared IOMMUs always
return error if the callback wasn't implemented.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Jan Beulich <jbeulich@suse.com>

---
   Changes in V1:
      - Wrap callback in #ifdef CONFIG_ARM.
---
 xen/drivers/passthrough/arm/iommu.c | 5 +++--
 xen/include/xen/iommu.h             | 3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 95b1abb..f132032 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -70,6 +70,7 @@ void arch_iommu_domain_destroy(struct domain *d)
 
 int arch_iommu_populate_page_table(struct domain *d)
 {
-    /* The IOMMU shares the p2m with the CPU */
-    return -ENOSYS;
+    const struct iommu_ops *ops = iommu_get_ops();
+
+    return ops->alloc_page_table ? ops->alloc_page_table(d) : -ENOSYS;
 }
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 3afbc3b..f5914db 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -175,6 +175,9 @@ struct iommu_ops {
                                   unsigned int flags);
     int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
                                     unsigned int order);
+#ifdef CONFIG_ARM
+    int (*alloc_page_table)(struct domain *d);
+#endif /* CONFIG_ARM */
     void (*free_page_table)(struct page_info *);
 #ifdef CONFIG_X86
     void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The "retrieving mapping" code has never executed since
iommu_use_hap_pt(d) always returned true on ARM so far. But, with
introducing the non-shared IOMMU patch series we can no longer keep
this code as is due to the lack of M2P support.

In order to retain the current behavior for x86 this code was completely
moved to x86 specific part.
For ARM we just need to populate IOMMU page table if need_iommu flag
is already set and the IOMMU is non-shared.

So, the logic on ARM was changed a bit, but no functional change for x86.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>

---
   Changes in V1:
      - Clarify patch description.
---
 xen/drivers/passthrough/arm/iommu.c |  7 +++++++
 xen/drivers/passthrough/iommu.c     | 30 +-----------------------------
 xen/drivers/passthrough/x86/iommu.c | 36 ++++++++++++++++++++++++++++++++++++
 xen/include/xen/iommu.h             |  1 +
 4 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index f132032..2198723 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -19,6 +19,7 @@
 #include <xen/iommu.h>
 #include <xen/device_tree.h>
 #include <asm/device.h>
+#include <xen/sched.h>
 
 static const struct iommu_ops *iommu_ops;
 
@@ -59,6 +60,12 @@ void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
     return;
 }
 
+void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
+{
+    if ( need_iommu(d) && !iommu_use_hap_pt(d) )
+        arch_iommu_populate_page_table(d);
+}
+
 int arch_iommu_domain_init(struct domain *d)
 {
     return iommu_dt_domain_init(d);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index c85f7b4..e66eefb 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -177,36 +177,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
 
     register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0);
     d->need_iommu = !!iommu_dom0_strict;
-    if ( need_iommu(d) && !iommu_use_hap_pt(d) )
-    {
-        struct page_info *page;
-        unsigned int i = 0;
-        int rc = 0;
-
-        page_list_for_each ( page, &d->page_list )
-        {
-            unsigned long mfn = page_to_mfn(page);
-            unsigned long gfn = mfn_to_gmfn(d, mfn);
-            unsigned int mapping = IOMMUF_readable;
-            int ret;
-
-            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
-                 ((page->u.inuse.type_info & PGT_type_mask)
-                  == PGT_writable_page) )
-                mapping |= IOMMUF_writable;
-
-            ret = hd->platform_ops->map_pages(d, gfn, mfn, 0, mapping);
-            if ( !rc )
-                rc = ret;
-
-            if ( !(i++ & 0xfffff) )
-                process_pending_softirqs();
-        }
 
-        if ( rc )
-            printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
-                   d->domain_id, rc);
-    }
+    arch_iommu_hwdom_init(d);
 
     return hd->platform_ops->hwdom_init(d);
 }
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 973b72f..904736b 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -118,6 +118,42 @@ void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
         panic("Presently, iommu must be enabled for PVH hardware domain\n");
 }
 
+void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
+{
+    const struct domain_iommu *hd = dom_iommu(d);
+
+    if ( need_iommu(d) && !iommu_use_hap_pt(d) )
+    {
+        struct page_info *page;
+        unsigned int i = 0;
+        int rc = 0;
+
+        page_list_for_each ( page, &d->page_list )
+        {
+            unsigned long mfn = page_to_mfn(page);
+            unsigned long gfn = mfn_to_gmfn(d, mfn);
+            unsigned int mapping = IOMMUF_readable;
+            int ret;
+
+            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
+                 ((page->u.inuse.type_info & PGT_type_mask)
+                  == PGT_writable_page) )
+                mapping |= IOMMUF_writable;
+
+            ret = hd->platform_ops->map_pages(d, gfn, mfn, 0, mapping);
+            if ( !rc )
+                rc = ret;
+
+            if ( !(i++ & 0xfffff) )
+                process_pending_softirqs();
+        }
+
+        if ( rc )
+            printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
+                   d->domain_id, rc);
+    }
+}
+
 int arch_iommu_domain_init(struct domain *d)
 {
     struct domain_iommu *hd = dom_iommu(d);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index f5914db..be43b28 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -65,6 +65,7 @@ void arch_iommu_domain_destroy(struct domain *d);
 int arch_iommu_domain_init(struct domain *d);
 int arch_iommu_populate_page_table(struct domain *d);
 void arch_iommu_check_autotranslated_hwdom(struct domain *d);
+void arch_iommu_hwdom_init(struct domain *d);
 
 int iommu_construct(struct domain *d);
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This flag is intended to let Xen know that the guest has devices
which will most likely be used for passthrough and as the result
the use of IOMMU is expected for this domain.
The primary aim of this knowledge is to help the IOMMUs that don't
share page tables with the CPU on ARM be ready before P2M code starts
updating IOMMU mapping.
So, if this flag is set the non-shared IOMMUs will populate
their page tables at the domain creation time and thereby will be able
to handle IOMMU mapping updates from *the very beginning*.

In order to retain the current behavior for x86 still call
iommu_domain_init() with use_iommu flag being forced to false.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

---
   Changes in V1:
      - Treat use_iommu flag as the ARM decision only. Don't use
        common domain creation flag for it, use ARM config instead.
      - Clarify patch subject/description.
---
 tools/libxl/libxl_arm.c       | 10 ++++++++++
 xen/arch/arm/domain.c         |  2 +-
 xen/include/public/arch-arm.h |  5 +++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d842d88..9c4705e 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         return ERROR_FAIL;
     }
 
+    /* TODO Are these assumptions enough to make decision about using IOMMU? */
+    if ((d_config->num_dtdevs && d_config->dtdevs) ||
+        (d_config->num_pcidevs && d_config->pcidevs))
+        xc_config->use_iommu = 1;
+    else
+        xc_config->use_iommu = 0;
+
+    LOG(DEBUG, "The use of IOMMU %s expected for this domain",
+        xc_config->use_iommu ? "is" : "isn't");
+
     return 0;
 }
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index ec19310..81c4b90 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     ASSERT(config != NULL);
 
     /* p2m_init relies on some value initialized by the IOMMU subsystem */
-    if ( (rc = iommu_domain_init(d, false)) != 0 )
+    if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != 0 )
         goto fail;
 
     if ( (rc = p2m_init(d)) != 0 )
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index bd974fb..cb33f75 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -322,6 +322,11 @@ struct xen_arch_domainconfig {
      *
      */
     uint32_t clock_frequency;
+    /*
+     * IN
+     * Inform the hypervisor that the use of IOMMU is expected for this domain.
+     */
+    uint8_t use_iommu;
 };
 #endif /* __XEN__ || __XEN_TOOLS__ */
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We don't passthrough IOMMU device to DOM0 even if it is not used by
Xen. Therefore exposing the property that describes the IOMMU
master interfaces of the device does not make any sense.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 xen/arch/arm/domain_build.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3abacc0..2defb60 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -432,6 +432,10 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
             continue;
         }
 
+        /* Don't expose the property "iommus" to the guest */
+        if ( dt_property_name_is_equal(prop, "iommus") )
+            continue;
+
         res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len);
 
         if ( res )
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper

[Jan Beulich]

>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Port Linux helper of_count_phandle_with_args for counting
> number of phandles in a property.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Reviewed-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>    Changes in v1:
>       - Add Julien's reviewed-by
> ---
>  xen/common/device_tree.c      |  7 +++++++
>  xen/include/xen/device_tree.h | 19 +++++++++++++++++++
>  2 files changed, 26 insertions(+)

I'm sure I did point this out to you before: With this diffstat, I don't
see why I've been copied on this patch. Likely the same for a few
more in this series.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 01/10] xen/device-tree: Add dt_count_phandle_with_args helper

[Oleksandr Tyshchenko]

Hi, Jan, all.

On Wed, May 10, 2017 at 5:50 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Port Linux helper of_count_phandle_with_args for counting
>> number of phandles in a property.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>    Changes in v1:
>>       - Add Julien's reviewed-by
>> ---
>>  xen/common/device_tree.c      |  7 +++++++
>>  xen/include/xen/device_tree.h | 19 +++++++++++++++++++
>>  2 files changed, 26 insertions(+)
>
> I'm sure I did point this out to you before: With this diffstat, I don't
> see why I've been copied on this patch. Likely the same for a few
> more in this series.
My apologies for that to everybody who I have mistakenly CCed.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Julien Grall]

Hi Oleksandr,

On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Update IOMMU mapping if the IOMMU doesn't share page table with the CPU.
> The best place to do so on ARM is __p2m_set_entry(). Use mfn as an indicator
> of the required action. If mfn is valid call iommu_map_pages(),
> otherwise - iommu_unmap_pages().
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Julien Grall <julien.grall@arm.com>

Acked-by: Julien Grall <julien.grall@arm.com>

>
> ---
>    Changes in v1:
>       - Update IOMMU mapping in __p2m_set_entry() instead of p2m_set_entry().
>       - Pass order argument to IOMMU APIs instead of page_count.
> ---
>  xen/arch/arm/p2m.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 34d5776..9ca491b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -984,7 +984,15 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>          p2m_free_entry(p2m, orig_pte, level);
>
>      if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) )
> -        rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
> +    {
> +        if ( iommu_use_hap_pt(p2m->domain) )
> +            rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
> +        else if ( !mfn_eq(smfn, INVALID_MFN) )
> +            rc = iommu_map_pages(p2m->domain, gfn_x(sgfn), mfn_x(smfn),
> +                                 page_order, p2m_get_iommu_flags(t));
> +        else
> +            rc = iommu_unmap_pages(p2m->domain, gfn_x(sgfn), page_order);
> +    }
>      else
>          rc = 0;
>
>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share

[Julien Grall]

Hi Oleksandr,

On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Not every integrated into ARM SoCs IOMMU can share page tables
> with the CPU and as result the iommu_use_hap_pt(d) is not always true.
> Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU
> page table is shared or not.
>
> Now all IOMMU drivers on ARM are able to change this flag
> according to their possibilities like x86-variants do.
> Therefore set iommu_hap_pt_share flag for SMMU because it always shares
> page table with the CPU.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 3 +++
>  xen/include/asm-arm/iommu.h        | 7 +++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 527a592..86ee12a 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct dt_device_node *dev,
>
>  	platform_features &= smmu->features;
>
> +	/* Always share P2M table between the CPU and the SMMU */
> +	iommu_hap_pt_share = true;
> +

I would prefer to bail-out if someone try to unshare the page-table 
rather than overriding. This would help us to know if someone are try to 
do that.

So I would do:

if ( !iommu_hap_pt_share )
{
	printk(....)
	return -EINVAL;
}

>  	return 0;
>  }
>
> diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
> index 57d9b1e..10a6f23 100644
> --- a/xen/include/asm-arm/iommu.h
> +++ b/xen/include/asm-arm/iommu.h
> @@ -20,8 +20,11 @@ struct arch_iommu
>      void *priv;
>  };
>
> -/* Always share P2M Table between the CPU and the IOMMU */
> -#define iommu_use_hap_pt(d) (1)
> +/*
> + * The ARM domain always has a P2M table, but not every integrated into
> + * ARM SoCs IOMMU can use it as page table.

The first part: "ARM domain has a P2M table" is pretty obvious. I would 
instead say: "Not every ARM SoCs IOMMU use the same page-table format as 
the processor.".

> + */
> +#define iommu_use_hap_pt(d) (iommu_hap_pt_share)
>
>  const struct iommu_ops *iommu_get_ops(void);
>  void __init iommu_set_ops(const struct iommu_ops *ops);
>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback

[Julien Grall]

Hi Oleksandr,

On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> The alloc_page_table callback is a mandatory thing
> for the IOMMUs that don't share page table with the CPU on ARM.
> The non-shared IOMMUs have to perform all required actions here
> to be ready to handle IOMMU mapping updates right after completing it.
>
> The arch_iommu_populate_page_table() seems an appropriate place
> to call newly created callback.
> Since we will only be here for the non-shared IOMMUs always
> return error if the callback wasn't implemented.

Why do you need a specific callback and not doing it directly in 
iommu_domain_init?

My take here is in the unshare case, we may want to have multiple set of 
page tables (e.g one per device). So this should be left at the 
discretion of the IOMMU itself.

Am I missing something?

>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Jan Beulich <jbeulich@suse.com>
>
> ---
>    Changes in V1:
>       - Wrap callback in #ifdef CONFIG_ARM.
> ---
>  xen/drivers/passthrough/arm/iommu.c | 5 +++--
>  xen/include/xen/iommu.h             | 3 +++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index 95b1abb..f132032 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -70,6 +70,7 @@ void arch_iommu_domain_destroy(struct domain *d)
>
>  int arch_iommu_populate_page_table(struct domain *d)
>  {
> -    /* The IOMMU shares the p2m with the CPU */
> -    return -ENOSYS;
> +    const struct iommu_ops *ops = iommu_get_ops();
> +
> +    return ops->alloc_page_table ? ops->alloc_page_table(d) : -ENOSYS;
>  }
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 3afbc3b..f5914db 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -175,6 +175,9 @@ struct iommu_ops {
>                                    unsigned int flags);
>      int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
>                                      unsigned int order);
> +#ifdef CONFIG_ARM
> +    int (*alloc_page_table)(struct domain *d);
> +#endif /* CONFIG_ARM */
>      void (*free_page_table)(struct page_info *);
>  #ifdef CONFIG_X86
>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig

[Julien Grall]

Hi Oleksandr,

On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> This flag is intended to let Xen know that the guest has devices
> which will most likely be used for passthrough and as the result
> the use of IOMMU is expected for this domain.
> The primary aim of this knowledge is to help the IOMMUs that don't
> share page tables with the CPU on ARM be ready before P2M code starts
> updating IOMMU mapping.
> So, if this flag is set the non-shared IOMMUs will populate
> their page tables at the domain creation time and thereby will be able
> to handle IOMMU mapping updates from *the very beginning*.
>
> In order to retain the current behavior for x86 still call
> iommu_domain_init() with use_iommu flag being forced to false.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
>
> ---
>    Changes in V1:
>       - Treat use_iommu flag as the ARM decision only. Don't use
>         common domain creation flag for it, use ARM config instead.
>       - Clarify patch subject/description.
> ---
>  tools/libxl/libxl_arm.c       | 10 ++++++++++
>  xen/arch/arm/domain.c         |  2 +-
>  xen/include/public/arch-arm.h |  5 +++++
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..9c4705e 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>          return ERROR_FAIL;
>      }
>
> +    /* TODO Are these assumptions enough to make decision about using IOMMU? */
> +    if ((d_config->num_dtdevs && d_config->dtdevs) ||
> +        (d_config->num_pcidevs && d_config->pcidevs))

Checking num_dtdevs and num_pcidevs is enough. It would be a bug if 
dtdevs and pcidevs are not null.

> +        xc_config->use_iommu = 1;
> +    else
> +        xc_config->use_iommu = 0;
> +
> +    LOG(DEBUG, "The use of IOMMU %s expected for this domain",
> +        xc_config->use_iommu ? "is" : "isn't");
> +
>      return 0;
>  }
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index ec19310..81c4b90 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>      ASSERT(config != NULL);
>
>      /* p2m_init relies on some value initialized by the IOMMU subsystem */
> -    if ( (rc = iommu_domain_init(d, false)) != 0 )
> +    if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) != 0 )

!!config->use_iommu is enough.

>          goto fail;
>
>      if ( (rc = p2m_init(d)) != 0 )
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index bd974fb..cb33f75 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -322,6 +322,11 @@ struct xen_arch_domainconfig {
>       *
>       */
>      uint32_t clock_frequency;
> +    /*
> +     * IN
> +     * Inform the hypervisor that the use of IOMMU is expected for this domain.

I would simplify to : "IOMMU is expected to be used for this domain".

> +     */
> +    uint8_t use_iommu;
>  };
>  #endif /* __XEN__ || __XEN_TOOLS__ */
>
>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest

[Julien Grall]

Hi Oleksandr,

On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> We don't passthrough IOMMU device to DOM0 even if it is not used by
> Xen. Therefore exposing the property that describes the IOMMU
> master interfaces of the device does not make any sense.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  xen/arch/arm/domain_build.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3abacc0..2defb60 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -432,6 +432,10 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>              continue;
>          }
>
> +        /* Don't expose the property "iommus" to the guest */
> +        if ( dt_property_name_is_equal(prop, "iommus") )
> +            continue;

It would be useful to have a link to the bindings associated 
(Documentation/devicetree/bindings/iommu/iommu.txt).

Also, whilst you are at it, you likely want to remove all the other 
iommu properties such as iommu-map.

> +
>          res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len);
>
>          if ( res )
>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback

[Oleksandr Tyshchenko]

On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi, Julien

>
> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> The alloc_page_table callback is a mandatory thing
>> for the IOMMUs that don't share page table with the CPU on ARM.
>> The non-shared IOMMUs have to perform all required actions here
>> to be ready to handle IOMMU mapping updates right after completing it.
>>
>> The arch_iommu_populate_page_table() seems an appropriate place
>> to call newly created callback.
>> Since we will only be here for the non-shared IOMMUs always
>> return error if the callback wasn't implemented.
>
>
> Why do you need a specific callback and not doing it directly in
> iommu_domain_init?
>
> My take here is in the unshare case, we may want to have multiple set of
> page tables (e.g one per device). So this should be left at the discretion
> of the IOMMU itself.
>
> Am I missing something?
I was thinking about extra need_iommu argument for init platform
callback as I had done for iommu_domain_init API.
But I had doubts regarding hw_domain. During iommu_domain_init
execution we haven't known yet is the IOMMU expected for domain 0
or not.

Taking into account that I needed to:
- populate page table followed by setting need_iommu flag.
- implement arch_iommu_populate_page_table() on ARM because of
!iommu_use_hap_pt(d).
- find a solution for hw_domain.

I decided to use iommu_construct() and implement alloc_page_table
callback to be called for populating page table.
I thought that it would allow us to keep all required actions in a
single place rather than spreading.

>
>
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>>    Changes in V1:
>>       - Wrap callback in #ifdef CONFIG_ARM.
>> ---
>>  xen/drivers/passthrough/arm/iommu.c | 5 +++--
>>  xen/include/xen/iommu.h             | 3 +++
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/iommu.c
>> b/xen/drivers/passthrough/arm/iommu.c
>> index 95b1abb..f132032 100644
>> --- a/xen/drivers/passthrough/arm/iommu.c
>> +++ b/xen/drivers/passthrough/arm/iommu.c
>> @@ -70,6 +70,7 @@ void arch_iommu_domain_destroy(struct domain *d)
>>
>>  int arch_iommu_populate_page_table(struct domain *d)
>>  {
>> -    /* The IOMMU shares the p2m with the CPU */
>> -    return -ENOSYS;
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +
>> +    return ops->alloc_page_table ? ops->alloc_page_table(d) : -ENOSYS;
>>  }
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 3afbc3b..f5914db 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -175,6 +175,9 @@ struct iommu_ops {
>>                                    unsigned int flags);
>>      int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
>>                                      unsigned int order);
>> +#ifdef CONFIG_ARM
>> +    int (*alloc_page_table)(struct domain *d);
>> +#endif /* CONFIG_ARM */
>>      void (*free_page_table)(struct page_info *);
>>  #ifdef CONFIG_X86
>>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg,
>> unsigned int value);
>>
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 09/10] xen/arm: Add use_iommu flag to xen_arch_domainconfig

[Oleksandr Tyshchenko]

On Thu, May 11, 2017 at 2:42 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi Julien

>
>
> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This flag is intended to let Xen know that the guest has devices
>> which will most likely be used for passthrough and as the result
>> the use of IOMMU is expected for this domain.
>> The primary aim of this knowledge is to help the IOMMUs that don't
>> share page tables with the CPU on ARM be ready before P2M code starts
>> updating IOMMU mapping.
>> So, if this flag is set the non-shared IOMMUs will populate
>> their page tables at the domain creation time and thereby will be able
>> to handle IOMMU mapping updates from *the very beginning*.
>>
>> In order to retain the current behavior for x86 still call
>> iommu_domain_init() with use_iommu flag being forced to false.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Ian Jackson <ian.jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> ---
>>    Changes in V1:
>>       - Treat use_iommu flag as the ARM decision only. Don't use
>>         common domain creation flag for it, use ARM config instead.
>>       - Clarify patch subject/description.
>> ---
>>  tools/libxl/libxl_arm.c       | 10 ++++++++++
>>  xen/arch/arm/domain.c         |  2 +-
>>  xen/include/public/arch-arm.h |  5 +++++
>>  3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index d842d88..9c4705e 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -78,6 +78,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>          return ERROR_FAIL;
>>      }
>>
>> +    /* TODO Are these assumptions enough to make decision about using
>> IOMMU? */
>> +    if ((d_config->num_dtdevs && d_config->dtdevs) ||
>> +        (d_config->num_pcidevs && d_config->pcidevs))
>
>
> Checking num_dtdevs and num_pcidevs is enough. It would be a bug if dtdevs
> and pcidevs are not null.
ok

>
>> +        xc_config->use_iommu = 1;
>> +    else
>> +        xc_config->use_iommu = 0;
>> +
>> +    LOG(DEBUG, "The use of IOMMU %s expected for this domain",
>> +        xc_config->use_iommu ? "is" : "isn't");
>> +
>>      return 0;
>>  }
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index ec19310..81c4b90 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -569,7 +569,7 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>>      ASSERT(config != NULL);
>>
>>      /* p2m_init relies on some value initialized by the IOMMU subsystem
>> */
>> -    if ( (rc = iommu_domain_init(d, false)) != 0 )
>> +    if ( (rc = iommu_domain_init(d, config->use_iommu ? true : false)) !=
>> 0 )
>
>
> !!config->use_iommu is enough.
ok

>
>>          goto fail;
>>
>>      if ( (rc = p2m_init(d)) != 0 )
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index bd974fb..cb33f75 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -322,6 +322,11 @@ struct xen_arch_domainconfig {
>>       *
>>       */
>>      uint32_t clock_frequency;
>> +    /*
>> +     * IN
>> +     * Inform the hypervisor that the use of IOMMU is expected for this
>> domain.
>
>
> I would simplify to : "IOMMU is expected to be used for this domain".
ok

>
>> +     */
>> +    uint8_t use_iommu;
>>  };
>>  #endif /* __XEN__ || __XEN_TOOLS__ */
>>
>>
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest

[Oleksandr Tyshchenko]

On Thu, May 11, 2017 at 2:58 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi Julien

>
> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We don't passthrough IOMMU device to DOM0 even if it is not used by
>> Xen. Therefore exposing the property that describes the IOMMU
>> master interfaces of the device does not make any sense.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  xen/arch/arm/domain_build.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 3abacc0..2defb60 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -432,6 +432,10 @@ static int write_properties(struct domain *d, struct
>> kernel_info *kinfo,
>>              continue;
>>          }
>>
>> +        /* Don't expose the property "iommus" to the guest */
>> +        if ( dt_property_name_is_equal(prop, "iommus") )
>> +            continue;
>
>
> It would be useful to have a link to the bindings associated
> (Documentation/devicetree/bindings/iommu/iommu.txt).
Agree. I will mention it in commit description.

>
> Also, whilst you are at it, you likely want to remove all the other iommu
> properties such as iommu-map.
Excuse me, I have never heard about it. Is it a required property?

>
>> +
>>          res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len);
>>
>>          if ( res )
>>
>
> Cheers,
>
> --
> Julien Grall


-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 04/10] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Oleksandr Tyshchenko]

On Thu, May 11, 2017 at 2:24 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi Julien

>
> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Update IOMMU mapping if the IOMMU doesn't share page table with the CPU.
>> The best place to do so on ARM is __p2m_set_entry(). Use mfn as an
>> indicator
>> of the required action. If mfn is valid call iommu_map_pages(),
>> otherwise - iommu_unmap_pages().
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>
>
> Acked-by: Julien Grall <julien.grall@arm.com>
Great. Thank you.

>
>
>>
>> ---
>>    Changes in v1:
>>       - Update IOMMU mapping in __p2m_set_entry() instead of
>> p2m_set_entry().
>>       - Pass order argument to IOMMU APIs instead of page_count.
>> ---
>>  xen/arch/arm/p2m.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 34d5776..9ca491b 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -984,7 +984,15 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>>          p2m_free_entry(p2m, orig_pte, level);
>>
>>      if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) ||
>> p2m_valid(*entry)) )
>> -        rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL <<
>> page_order);
>> +    {
>> +        if ( iommu_use_hap_pt(p2m->domain) )
>> +            rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL <<
>> page_order);
>> +        else if ( !mfn_eq(smfn, INVALID_MFN) )
>> +            rc = iommu_map_pages(p2m->domain, gfn_x(sgfn), mfn_x(smfn),
>> +                                 page_order, p2m_get_iommu_flags(t));
>> +        else
>> +            rc = iommu_unmap_pages(p2m->domain, gfn_x(sgfn), page_order);
>> +    }
>>      else
>>          rc = 0;
>>
>>
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share

[Oleksandr Tyshchenko]

On Thu, May 11, 2017 at 2:28 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi Julien

>
> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Not every integrated into ARM SoCs IOMMU can share page tables
>> with the CPU and as result the iommu_use_hap_pt(d) is not always true.
>> Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU
>> page table is shared or not.
>>
>> Now all IOMMU drivers on ARM are able to change this flag
>> according to their possibilities like x86-variants do.
>> Therefore set iommu_hap_pt_share flag for SMMU because it always shares
>> page table with the CPU.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  xen/drivers/passthrough/arm/smmu.c | 3 +++
>>  xen/include/asm-arm/iommu.h        | 7 +++++--
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/smmu.c
>> b/xen/drivers/passthrough/arm/smmu.c
>> index 527a592..86ee12a 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct
>> dt_device_node *dev,
>>
>>         platform_features &= smmu->features;
>>
>> +       /* Always share P2M table between the CPU and the SMMU */
>> +       iommu_hap_pt_share = true;
>> +
>
>
> I would prefer to bail-out if someone try to unshare the page-table rather
> than overriding. This would help us to know if someone are try to do that.
>
> So I would do:
>
> if ( !iommu_hap_pt_share )
> {
>         printk(....)
>         return -EINVAL;
> }
I got it for SMMU.

But, for IPMMU we will override since iommu_hap_pt_share is true by
default. Right?

/*
* The IPMMU can't reuse P2M table since it only supports
* stage-1 page tables.
*/
iommu_hap_pt_share = false;

>
>>         return 0;
>>  }
>>
>> diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
>> index 57d9b1e..10a6f23 100644
>> --- a/xen/include/asm-arm/iommu.h
>> +++ b/xen/include/asm-arm/iommu.h
>> @@ -20,8 +20,11 @@ struct arch_iommu
>>      void *priv;
>>  };
>>
>> -/* Always share P2M Table between the CPU and the IOMMU */
>> -#define iommu_use_hap_pt(d) (1)
>> +/*
>> + * The ARM domain always has a P2M table, but not every integrated into
>> + * ARM SoCs IOMMU can use it as page table.
>
>
> The first part: "ARM domain has a P2M table" is pretty obvious. I would
> instead say: "Not every ARM SoCs IOMMU use the same page-table format as the
> processor.".
Agree.

>
>> + */
>> +#define iommu_use_hap_pt(d) (iommu_hap_pt_share)
>>
>>  const struct iommu_ops *iommu_get_ops(void);
>>  void __init iommu_set_ops(const struct iommu_ops *ops);
>>
>
> Cheers,
>
> --
> Julien Grall


-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share

[Julien Grall]

On 11/05/17 15:38, Oleksandr Tyshchenko wrote:
> On Thu, May 11, 2017 at 2:28 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Oleksandr,
> Hi Julien

Hi Oleksandr,

>>
>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>>
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> Not every integrated into ARM SoCs IOMMU can share page tables
>>> with the CPU and as result the iommu_use_hap_pt(d) is not always true.
>>> Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU
>>> page table is shared or not.
>>>
>>> Now all IOMMU drivers on ARM are able to change this flag
>>> according to their possibilities like x86-variants do.
>>> Therefore set iommu_hap_pt_share flag for SMMU because it always shares
>>> page table with the CPU.
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>  xen/drivers/passthrough/arm/smmu.c | 3 +++
>>>  xen/include/asm-arm/iommu.h        | 7 +++++--
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/arm/smmu.c
>>> b/xen/drivers/passthrough/arm/smmu.c
>>> index 527a592..86ee12a 100644
>>> --- a/xen/drivers/passthrough/arm/smmu.c
>>> +++ b/xen/drivers/passthrough/arm/smmu.c
>>> @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct
>>> dt_device_node *dev,
>>>
>>>         platform_features &= smmu->features;
>>>
>>> +       /* Always share P2M table between the CPU and the SMMU */
>>> +       iommu_hap_pt_share = true;
>>> +
>>
>>
>> I would prefer to bail-out if someone try to unshare the page-table rather
>> than overriding. This would help us to know if someone are try to do that.
>>
>> So I would do:
>>
>> if ( !iommu_hap_pt_share )
>> {
>>         printk(....)
>>         return -EINVAL;
>> }
> I got it for SMMU.
>
> But, for IPMMU we will override since iommu_hap_pt_share is true by
> default. Right?
>
> /*
> * The IPMMU can't reuse P2M table since it only supports
> * stage-1 page tables.
> */
> iommu_hap_pt_share = false;

Good point. Looking at the other driver, they print a message to notify 
the override. (See drivers/passthrough/amd/iommu_init.c for instance).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback

[Julien Grall]

Hi Oleksandr,

On 11/05/17 15:00, Oleksandr Tyshchenko wrote:
> On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Oleksandr,
> Hi, Julien
>
>>
>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>>
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> The alloc_page_table callback is a mandatory thing
>>> for the IOMMUs that don't share page table with the CPU on ARM.
>>> The non-shared IOMMUs have to perform all required actions here
>>> to be ready to handle IOMMU mapping updates right after completing it.
>>>
>>> The arch_iommu_populate_page_table() seems an appropriate place
>>> to call newly created callback.
>>> Since we will only be here for the non-shared IOMMUs always
>>> return error if the callback wasn't implemented.
>>
>>
>> Why do you need a specific callback and not doing it directly in
>> iommu_domain_init?
>>
>> My take here is in the unshare case, we may want to have multiple set of
>> page tables (e.g one per device). So this should be left at the discretion
>> of the IOMMU itself.
>>
>> Am I missing something?
> I was thinking about extra need_iommu argument for init platform
> callback as I had done for iommu_domain_init API.
> But I had doubts regarding hw_domain. During iommu_domain_init
> execution we haven't known yet is the IOMMU expected for domain 0
> or not.
>
> Taking into account that I needed to:
> - populate page table followed by setting need_iommu flag.
> - implement arch_iommu_populate_page_table() on ARM because of
> !iommu_use_hap_pt(d).
> - find a solution for hw_domain.
>
> I decided to use iommu_construct() and implement alloc_page_table
> callback to be called for populating page table.
> I thought that it would allow us to keep all required actions in a
> single place rather than spreading.

Looking at your patch #8, you always allocate the page table for 
hardware domain, so this is very similar to set iommu_enable in 
xen_arch_domain_config (see config. in start_xen).

So this does not hold to me. Maybe Jan (IOMMU maintainer) has a 
different view on it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest

[Julien Grall]

On 11/05/17 15:15, Oleksandr Tyshchenko wrote:
> On Thu, May 11, 2017 at 2:58 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Oleksandr,
> Hi Julien

Hi,

>>
>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>>
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> We don't passthrough IOMMU device to DOM0 even if it is not used by
>>> Xen. Therefore exposing the property that describes the IOMMU
>>> master interfaces of the device does not make any sense.
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>  xen/arch/arm/domain_build.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 3abacc0..2defb60 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -432,6 +432,10 @@ static int write_properties(struct domain *d, struct
>>> kernel_info *kinfo,
>>>              continue;
>>>          }
>>>
>>> +        /* Don't expose the property "iommus" to the guest */
>>> +        if ( dt_property_name_is_equal(prop, "iommus") )
>>> +            continue;
>>
>>
>> It would be useful to have a link to the bindings associated
>> (Documentation/devicetree/bindings/iommu/iommu.txt).
> Agree. I will mention it in commit description.
>
>>
>> Also, whilst you are at it, you likely want to remove all the other iommu
>> properties such as iommu-map.
> Excuse me, I have never heard about it. Is it a required property?

This property is used to translate an RID into a Master ID (see the 
documentation in bindings/pci/pci-iommu.txt).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest

[Oleksandr Tyshchenko]

On Thu, May 11, 2017 at 9:07 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 11/05/17 15:15, Oleksandr Tyshchenko wrote:
>>
>> On Thu, May 11, 2017 at 2:58 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi Oleksandr,
>>
>> Hi Julien
>
>
> Hi,
>
>
>>>
>>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>>>
>>>>
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> We don't passthrough IOMMU device to DOM0 even if it is not used by
>>>> Xen. Therefore exposing the property that describes the IOMMU
>>>> master interfaces of the device does not make any sense.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>>  xen/arch/arm/domain_build.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index 3abacc0..2defb60 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -432,6 +432,10 @@ static int write_properties(struct domain *d,
>>>> struct
>>>> kernel_info *kinfo,
>>>>              continue;
>>>>          }
>>>>
>>>> +        /* Don't expose the property "iommus" to the guest */
>>>> +        if ( dt_property_name_is_equal(prop, "iommus") )
>>>> +            continue;
>>>
>>>
>>>
>>> It would be useful to have a link to the bindings associated
>>> (Documentation/devicetree/bindings/iommu/iommu.txt).
>>
>> Agree. I will mention it in commit description.
>>
>>>
>>> Also, whilst you are at it, you likely want to remove all the other iommu
>>> properties such as iommu-map.
>>
>> Excuse me, I have never heard about it. Is it a required property?
>
>
> This property is used to translate an RID into a Master ID (see the
> documentation in bindings/pci/pci-iommu.txt).
So, these two should be skipped too:
- iommu-map
- iommu-map-mask

Right?

>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 10/10] xen/arm: domain_build: Don't expose the "iommus" property to the guest

[Julien Grall]

On 11/05/17 19:19, Oleksandr Tyshchenko wrote:
> On Thu, May 11, 2017 at 9:07 PM, Julien Grall <julien.grall@arm.com> wrote:
>> This property is used to translate an RID into a Master ID (see the
>> documentation in bindings/pci/pci-iommu.txt).
> So, these two should be skipped too:
> - iommu-map
> - iommu-map-mask
>
> Right?

Yep.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 05/10] iommu/arm: Re-define iommu_use_hap_pt(d) as iommu_hap_pt_share

[Oleksandr Tyshchenko]

On Thu, May 11, 2017 at 8:58 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 11/05/17 15:38, Oleksandr Tyshchenko wrote:
>>
>> On Thu, May 11, 2017 at 2:28 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi Oleksandr,
>>
>> Hi Julien
>
>
> Hi Oleksandr,
>
>
>>>
>>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>>>
>>>>
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> Not every integrated into ARM SoCs IOMMU can share page tables
>>>> with the CPU and as result the iommu_use_hap_pt(d) is not always true.
>>>> Reuse x86's iommu_hap_pt_share flag to indicate whether the IOMMU
>>>> page table is shared or not.
>>>>
>>>> Now all IOMMU drivers on ARM are able to change this flag
>>>> according to their possibilities like x86-variants do.
>>>> Therefore set iommu_hap_pt_share flag for SMMU because it always shares
>>>> page table with the CPU.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>>  xen/drivers/passthrough/arm/smmu.c | 3 +++
>>>>  xen/include/asm-arm/iommu.h        | 7 +++++--
>>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/drivers/passthrough/arm/smmu.c
>>>> b/xen/drivers/passthrough/arm/smmu.c
>>>> index 527a592..86ee12a 100644
>>>> --- a/xen/drivers/passthrough/arm/smmu.c
>>>> +++ b/xen/drivers/passthrough/arm/smmu.c
>>>> @@ -2870,6 +2870,9 @@ static __init int arm_smmu_dt_init(struct
>>>> dt_device_node *dev,
>>>>
>>>>         platform_features &= smmu->features;
>>>>
>>>> +       /* Always share P2M table between the CPU and the SMMU */
>>>> +       iommu_hap_pt_share = true;
>>>> +
>>>
>>>
>>>
>>> I would prefer to bail-out if someone try to unshare the page-table
>>> rather
>>> than overriding. This would help us to know if someone are try to do
>>> that.
>>>
>>> So I would do:
>>>
>>> if ( !iommu_hap_pt_share )
>>> {
>>>         printk(....)
>>>         return -EINVAL;
>>> }
>>
>> I got it for SMMU.
>>
>> But, for IPMMU we will override since iommu_hap_pt_share is true by
>> default. Right?
>>
>> /*
>> * The IPMMU can't reuse P2M table since it only supports
>> * stage-1 page tables.
>> */
>> iommu_hap_pt_share = false;
>
>
> Good point. Looking at the other driver, they print a message to notify the
> override. (See drivers/passthrough/amd/iommu_init.c for instance).
I will print a message too.

>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback

[Oleksandr Tyshchenko]

On Thu, May 11, 2017 at 9:06 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi Julien

>
>
> On 11/05/17 15:00, Oleksandr Tyshchenko wrote:
>>
>> On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi Oleksandr,
>>
>> Hi, Julien
>>
>>>
>>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>>>
>>>>
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> The alloc_page_table callback is a mandatory thing
>>>> for the IOMMUs that don't share page table with the CPU on ARM.
>>>> The non-shared IOMMUs have to perform all required actions here
>>>> to be ready to handle IOMMU mapping updates right after completing it.
>>>>
>>>> The arch_iommu_populate_page_table() seems an appropriate place
>>>> to call newly created callback.
>>>> Since we will only be here for the non-shared IOMMUs always
>>>> return error if the callback wasn't implemented.
>>>
>>>
>>>
>>> Why do you need a specific callback and not doing it directly in
>>> iommu_domain_init?
>>>
>>> My take here is in the unshare case, we may want to have multiple set of
>>> page tables (e.g one per device). So this should be left at the
>>> discretion
>>> of the IOMMU itself.
>>>
>>> Am I missing something?
>>
>> I was thinking about extra need_iommu argument for init platform
>> callback as I had done for iommu_domain_init API.
>> But I had doubts regarding hw_domain. During iommu_domain_init
>> execution we haven't known yet is the IOMMU expected for domain 0
>> or not.
>>
>> Taking into account that I needed to:
>> - populate page table followed by setting need_iommu flag.
>> - implement arch_iommu_populate_page_table() on ARM because of
>> !iommu_use_hap_pt(d).
>> - find a solution for hw_domain.
>>
>> I decided to use iommu_construct() and implement alloc_page_table
>> callback to be called for populating page table.
>> I thought that it would allow us to keep all required actions in a
>> single place rather than spreading.
>
>
> Looking at your patch #8, you always allocate the page table for hardware
> domain, so this is very similar to set iommu_enable in
> xen_arch_domain_config (see config. in start_xen).
Indeed.

I allocate page table for hw_domain only if need_iommu flag is set.
The need_iommu flag depends on iommu_dom0_strict.
We force iommu_dom0_strict to true.
This all means that we always use iommu for hw_domain)

So, you proposed to avoid new callback and allocate page table
directly in "init" callback?
This also means we have to add extra need_iommu argument to "init" callback.

>
> So this does not hold to me. Maybe Jan (IOMMU maintainer) has a different
> view on it.
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Jan Beulich]

>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>      return 0;
>  }
>  
> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */

Looking over the titles of the rest of this series it doesn't look like
you're eliminating this TODO later. While I appreciate this not
being done in the already large patch, I don't think such a TODO
should be left around. If need be (e.g. because you can't test
the change), get in touch with the maintainer(s).

> +int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn,
> +                                       unsigned int order)
> +{
> +    unsigned long i;
> +    int rc = 0;
> +
> +    for ( i = 0; i < (1UL << order); i++ )
> +    {
> +        int ret = amd_iommu_unmap_page(d, gfn + i);
> +        if ( !rc )

Blank line between declaration(s) and statement(s) please.

x86 and generic iommu parts (and _only_ those, so please don't
convert this into a blanket R-b)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Jan Beulich]

>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> The presence of this flag lets us know that the guest
> has devices which will most likely be used for passthrough
> and as the result the use of IOMMU is expected for this domain.
> In that case we have to call iommu_construct(), actually
> what the real assign_device call usually does.
> 
> As iommu_domain_init() is called with use_iommu flag being forced
> to false for now, no functional change is intended for both ARM and x86.
> 
> Basically, this patch is needed for non-shared IOMMUs on ARM only
> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called
> later. But, in order to be more generic and for possible future optimization
> make this change appliable for both platforms.

I continue to be unconvinced that this is wanted / needed, as I
continue to not see why shared vs unshared really matters here.
After all we have both modes working on x86 without this flag.

> @@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d)
>          return 0;
>  
>      hd->platform_ops = iommu_get_ops();
> -    return hd->platform_ops->init(d);
> +    ret = hd->platform_ops->init(d);
> +    if ( ret )
> +        return ret;
> +
> +    if ( use_iommu && !is_hardware_domain(d) )
> +        ret = iommu_construct(d);

You don't handle the -ERESTART you may (and likely will) get here
or in the caller.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 07/10] iommu/arm: Add alloc_page_table platform callback

[Jan Beulich]

>>> On 11.05.17 at 20:06, <julien.grall@arm.com> wrote:
> Hi Oleksandr,
> 
> On 11/05/17 15:00, Oleksandr Tyshchenko wrote:
>> On Thu, May 11, 2017 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
>>> Hi Oleksandr,
>> Hi, Julien
>>
>>>
>>> On 10/05/17 15:03, Oleksandr Tyshchenko wrote:
>>>>
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> The alloc_page_table callback is a mandatory thing
>>>> for the IOMMUs that don't share page table with the CPU on ARM.
>>>> The non-shared IOMMUs have to perform all required actions here
>>>> to be ready to handle IOMMU mapping updates right after completing it.
>>>>
>>>> The arch_iommu_populate_page_table() seems an appropriate place
>>>> to call newly created callback.
>>>> Since we will only be here for the non-shared IOMMUs always
>>>> return error if the callback wasn't implemented.
>>>
>>>
>>> Why do you need a specific callback and not doing it directly in
>>> iommu_domain_init?
>>>
>>> My take here is in the unshare case, we may want to have multiple set of
>>> page tables (e.g one per device). So this should be left at the discretion
>>> of the IOMMU itself.
>>>
>>> Am I missing something?
>> I was thinking about extra need_iommu argument for init platform
>> callback as I had done for iommu_domain_init API.
>> But I had doubts regarding hw_domain. During iommu_domain_init
>> execution we haven't known yet is the IOMMU expected for domain 0
>> or not.
>>
>> Taking into account that I needed to:
>> - populate page table followed by setting need_iommu flag.
>> - implement arch_iommu_populate_page_table() on ARM because of
>> !iommu_use_hap_pt(d).
>> - find a solution for hw_domain.
>>
>> I decided to use iommu_construct() and implement alloc_page_table
>> callback to be called for populating page table.
>> I thought that it would allow us to keep all required actions in a
>> single place rather than spreading.
> 
> Looking at your patch #8, you always allocate the page table for 
> hardware domain, so this is very similar to set iommu_enable in 
> xen_arch_domain_config (see config. in start_xen).
> 
> So this does not hold to me. Maybe Jan (IOMMU maintainer) has a 
> different view on it.

Well, I have to admit that I don't really understand the need for
this new callback. But it looks like in a later reply Oleksandr moves
to agreeing with you to drop this new hook. As it's ARM-specific,
I'll leave this to you for the moment.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
> The "retrieving mapping" code has never executed since
> iommu_use_hap_pt(d) always returned true on ARM so far. But, with
> introducing the non-shared IOMMU patch series we can no longer keep
> this code as is due to the lack of M2P support.
> 
> In order to retain the current behavior for x86 this code was completely
> moved to x86 specific part.
> For ARM we just need to populate IOMMU page table if need_iommu flag
> is already set and the IOMMU is non-shared.
> 
> So, the logic on ARM was changed a bit, but no functional change for x86.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> ---
>    Changes in V1:
>       - Clarify patch description.

My prior objection stands: You don't make clear why architecture-
agnostic code needs to be moved into an architecture-specific file.
Of course once you give a proper reason in the description, the
change itself looks mostly fine - just one remark:

> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> +{
> +    const struct domain_iommu *hd = dom_iommu(d);

This isn't used outside of ...

> +    if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> +    {

... this if(), so should be moved here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

Hi, Jan.

On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>> The "retrieving mapping" code has never executed since
>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with
>> introducing the non-shared IOMMU patch series we can no longer keep
>> this code as is due to the lack of M2P support.
>>
>> In order to retain the current behavior for x86 this code was completely
>> moved to x86 specific part.
>> For ARM we just need to populate IOMMU page table if need_iommu flag
>> is already set and the IOMMU is non-shared.
>>
>> So, the logic on ARM was changed a bit, but no functional change for x86.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>    Changes in V1:
>>       - Clarify patch description.
>
> My prior objection stands: You don't make clear why architecture-
> agnostic code needs to be moved into an architecture-specific file.

Is the following description more cleaner?

Although this code looks like architecture-agnostic code, there is
only one thing that prevents it
to be *completely* architecture-agnostic -> mfn_to_gmfn helper.
As we don't have an M2P on ARM, it always returns incoming mfn.
And if domain is not direct mapped we will get into the trouble using that.

We didn't care about this code before because it has never been executed
(iommu_use_hap_pt(d) always returned true on ARM so far).
But, with introducing the non-shared IOMMU patch series we can no longer keep
this code as is since it will be executed for non-shared IOMMU.
So, move this code to x86-specific file in order not to expose a possible bug.

> Of course once you give a proper reason in the description, the
> change itself looks mostly fine - just one remark:
>
>> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>> +{
>> +    const struct domain_iommu *hd = dom_iommu(d);
>
> This isn't used outside of ...
>
>> +    if ( need_iommu(d) && !iommu_use_hap_pt(d) )
>> +    {
>
> ... this if(), so should be moved here.
Agree. Will move.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote:
> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>> The "retrieving mapping" code has never executed since
>>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with
>>> introducing the non-shared IOMMU patch series we can no longer keep
>>> this code as is due to the lack of M2P support.
>>>
>>> In order to retain the current behavior for x86 this code was completely
>>> moved to x86 specific part.
>>> For ARM we just need to populate IOMMU page table if need_iommu flag
>>> is already set and the IOMMU is non-shared.
>>>
>>> So, the logic on ARM was changed a bit, but no functional change for x86.
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>    Changes in V1:
>>>       - Clarify patch description.
>>
>> My prior objection stands: You don't make clear why architecture-
>> agnostic code needs to be moved into an architecture-specific file.
> 
> Is the following description more cleaner?
> 
> Although this code looks like architecture-agnostic code, there is
> only one thing that prevents it
> to be *completely* architecture-agnostic -> mfn_to_gmfn helper.
> As we don't have an M2P on ARM, it always returns incoming mfn.
> And if domain is not direct mapped we will get into the trouble using that.
> 
> We didn't care about this code before because it has never been executed
> (iommu_use_hap_pt(d) always returned true on ARM so far).
> But, with introducing the non-shared IOMMU patch series we can no longer keep
> this code as is since it will be executed for non-shared IOMMU.
> So, move this code to x86-specific file in order not to expose a possible bug.

Yes, this helps.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Oleksandr Tyshchenko]

Hi Jan.

On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>>      return 0;
>>  }
>>
>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
>
> Looking over the titles of the rest of this series it doesn't look like
> you're eliminating this TODO later. While I appreciate this not
> being done in the already large patch, I don't think such a TODO
> should be left around. If need be (e.g. because you can't test
> the change), get in touch with the maintainer(s).
I will drop this TODO everywhere.

>
>> +int __must_check amd_iommu_unmap_pages(struct domain *d, unsigned long gfn,
>> +                                       unsigned int order)
>> +{
>> +    unsigned long i;
>> +    int rc = 0;
>> +
>> +    for ( i = 0; i < (1UL << order); i++ )
>> +    {
>> +        int ret = amd_iommu_unmap_page(d, gfn + i);
>> +        if ( !rc )
>
> Blank line between declaration(s) and statement(s) please.
ok

>
> x86 and generic iommu parts (and _only_ those, so please don't
> convert this into a blanket R-b)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thank you. Sure. I won't put your R-b until I get R-b from ARM folks.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Jan Beulich]

>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote:
> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>>>      return 0;
>>>  }
>>>
>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
>>
>> Looking over the titles of the rest of this series it doesn't look like
>> you're eliminating this TODO later. While I appreciate this not
>> being done in the already large patch, I don't think such a TODO
>> should be left around. If need be (e.g. because you can't test
>> the change), get in touch with the maintainer(s).
> I will drop this TODO everywhere.

By "drop" you mean "address" or really just "drop"?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Oleksandr Tyshchenko]

On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote:
>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>>>>      return 0;
>>>>  }
>>>>
>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
>>>
>>> Looking over the titles of the rest of this series it doesn't look like
>>> you're eliminating this TODO later. While I appreciate this not
>>> being done in the already large patch, I don't think such a TODO
>>> should be left around. If need be (e.g. because you can't test
>>> the change), get in touch with the maintainer(s).
>> I will drop this TODO everywhere.
>
> By "drop" you mean "address" or really just "drop"?
I meant just drop.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Oleksandr Tyshchenko]

Hi, Jan.

On Fri, May 12, 2017 at 5:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> The presence of this flag lets us know that the guest
>> has devices which will most likely be used for passthrough
>> and as the result the use of IOMMU is expected for this domain.
>> In that case we have to call iommu_construct(), actually
>> what the real assign_device call usually does.
>>
>> As iommu_domain_init() is called with use_iommu flag being forced
>> to false for now, no functional change is intended for both ARM and x86.
>>
>> Basically, this patch is needed for non-shared IOMMUs on ARM only
>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called
>> later. But, in order to be more generic and for possible future optimization
>> make this change appliable for both platforms.
>
> I continue to be unconvinced that this is wanted / needed, as I
> continue to not see why shared vs unshared really matters here.
> After all we have both modes working on x86 without this flag.
I know. But, for ARM we need this hint. We can't reuse the "retrieving
mapping" code I moved to x86-specific part in patch #8 (due to lack of
M2P on ARM) .

>
>> @@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d)
>>          return 0;
>>
>>      hd->platform_ops = iommu_get_ops();
>> -    return hd->platform_ops->init(d);
>> +    ret = hd->platform_ops->init(d);
>> +    if ( ret )
>> +        return ret;
>> +
>> +    if ( use_iommu && !is_hardware_domain(d) )
>> +        ret = iommu_construct(d);
>
> You don't handle the -ERESTART you may (and likely will) get here
> or in the caller.
Indeed. I forgot about it.

I most likely rework this patch not to call iommu_construct at all.
But, there are an open questions here and in patch #7 I would like to
clarify the first.
1. Are you against extra arguments at all?
2. If this question still makes sense, Shall I add extra need_iommu
argument to "init" callback too and just pass thought
    incoming flag to IOMMU drivers? This change wouldn't affect x86 at
all since we set this flag for ARM only (see patch #9).
    For x86 this flag would be always treated as unused.

>
> Jan
>


-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 12.05.17 at 17:34, <JBeulich@suse.com> wrote:
>>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote:
>> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>>> The "retrieving mapping" code has never executed since
>>>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with
>>>> introducing the non-shared IOMMU patch series we can no longer keep
>>>> this code as is due to the lack of M2P support.
>>>>
>>>> In order to retain the current behavior for x86 this code was completely
>>>> moved to x86 specific part.
>>>> For ARM we just need to populate IOMMU page table if need_iommu flag
>>>> is already set and the IOMMU is non-shared.
>>>>
>>>> So, the logic on ARM was changed a bit, but no functional change for x86.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> CC: Jan Beulich <jbeulich@suse.com>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>    Changes in V1:
>>>>       - Clarify patch description.
>>>
>>> My prior objection stands: You don't make clear why architecture-
>>> agnostic code needs to be moved into an architecture-specific file.
>> 
>> Is the following description more cleaner?
>> 
>> Although this code looks like architecture-agnostic code, there is
>> only one thing that prevents it
>> to be *completely* architecture-agnostic -> mfn_to_gmfn helper.
>> As we don't have an M2P on ARM, it always returns incoming mfn.
>> And if domain is not direct mapped we will get into the trouble using that.
>> 
>> We didn't care about this code before because it has never been executed
>> (iommu_use_hap_pt(d) always returned true on ARM so far).
>> But, with introducing the non-shared IOMMU patch series we can no longer keep
>> this code as is since it will be executed for non-shared IOMMU.
>> So, move this code to x86-specific file in order not to expose a possible bug.
> 
> Yes, this helps.

Having thought about this some more, what's still missing is a
clear explanation why this new need of a non-stub mfn_to_gmfn()
isn't finally enough of a reason to introduce an M2P on ARM. We
currently have several uses already which ARM fakes one way or
another:
- gnttab_shared_gmfn()
- gnttab_status_gmfn()
- memory_exchange()
- getdomaininfo()
With this I think there's quite a bit of justification needed to keep
going without M2P on ARM.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Jan Beulich]

>>> On 12.05.17 at 18:25, <olekstysh@gmail.com> wrote:
> On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote:
>>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>>>>>      return 0;
>>>>>  }
>>>>>
>>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
>>>>
>>>> Looking over the titles of the rest of this series it doesn't look like
>>>> you're eliminating this TODO later. While I appreciate this not
>>>> being done in the already large patch, I don't think such a TODO
>>>> should be left around. If need be (e.g. because you can't test
>>>> the change), get in touch with the maintainer(s).
>>> I will drop this TODO everywhere.
>>
>> By "drop" you mean "address" or really just "drop"?
> I meant just drop.

Then I'm sorry, but no, this is not a way to address the comment I've
made.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Jan Beulich]

>>> On 12.05.17 at 19:00, <olekstysh@gmail.com> wrote:
> On Fri, May 12, 2017 at 5:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> The presence of this flag lets us know that the guest
>>> has devices which will most likely be used for passthrough
>>> and as the result the use of IOMMU is expected for this domain.
>>> In that case we have to call iommu_construct(), actually
>>> what the real assign_device call usually does.
>>>
>>> As iommu_domain_init() is called with use_iommu flag being forced
>>> to false for now, no functional change is intended for both ARM and x86.
>>>
>>> Basically, this patch is needed for non-shared IOMMUs on ARM only
>>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called
>>> later. But, in order to be more generic and for possible future optimization
>>> make this change appliable for both platforms.
>>
>> I continue to be unconvinced that this is wanted / needed, as I
>> continue to not see why shared vs unshared really matters here.
>> After all we have both modes working on x86 without this flag.
> I know. But, for ARM we need this hint. We can't reuse the "retrieving
> mapping" code I moved to x86-specific part in patch #8 (due to lack of
> M2P on ARM) .

Well, see the reply to 08/10 I've sent a minute ago.

>>> @@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d)
>>>          return 0;
>>>
>>>      hd->platform_ops = iommu_get_ops();
>>> -    return hd->platform_ops->init(d);
>>> +    ret = hd->platform_ops->init(d);
>>> +    if ( ret )
>>> +        return ret;
>>> +
>>> +    if ( use_iommu && !is_hardware_domain(d) )
>>> +        ret = iommu_construct(d);
>>
>> You don't handle the -ERESTART you may (and likely will) get here
>> or in the caller.
> Indeed. I forgot about it.
> 
> I most likely rework this patch not to call iommu_construct at all.
> But, there are an open questions here and in patch #7 I would like to
> clarify the first.
> 1. Are you against extra arguments at all?

No, extra arguments aren't the point. The kind of information passed
is the questionable thing.

> 2. If this question still makes sense, Shall I add extra need_iommu
> argument to "init" callback too and just pass thought
>     incoming flag to IOMMU drivers? This change wouldn't affect x86 at
> all since we set this flag for ARM only (see patch #9).
>     For x86 this flag would be always treated as unused.

I won't give a firm answer here considering the wider question on
M2P, but I'd like to say that always-unused flags on one architecture
are often (but not always, namely not when there's a hardware
feature one architecture has but others don't) a sign of a design/
abstraction problem.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Julien Grall]

Hi Jan,

On 15/05/2017 08:20, Jan Beulich wrote:
>>>> On 12.05.17 at 17:34, <JBeulich@suse.com> wrote:
>>>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote:
>>> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>>>> The "retrieving mapping" code has never executed since
>>>>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with
>>>>> introducing the non-shared IOMMU patch series we can no longer keep
>>>>> this code as is due to the lack of M2P support.
>>>>>
>>>>> In order to retain the current behavior for x86 this code was completely
>>>>> moved to x86 specific part.
>>>>> For ARM we just need to populate IOMMU page table if need_iommu flag
>>>>> is already set and the IOMMU is non-shared.
>>>>>
>>>>> So, the logic on ARM was changed a bit, but no functional change for x86.
>>>>>
>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>> CC: Jan Beulich <jbeulich@suse.com>
>>>>> CC: Julien Grall <julien.grall@arm.com>
>>>>>
>>>>> ---
>>>>>    Changes in V1:
>>>>>       - Clarify patch description.
>>>>
>>>> My prior objection stands: You don't make clear why architecture-
>>>> agnostic code needs to be moved into an architecture-specific file.
>>>
>>> Is the following description more cleaner?
>>>
>>> Although this code looks like architecture-agnostic code, there is
>>> only one thing that prevents it
>>> to be *completely* architecture-agnostic -> mfn_to_gmfn helper.
>>> As we don't have an M2P on ARM, it always returns incoming mfn.
>>> And if domain is not direct mapped we will get into the trouble using that.
>>>
>>> We didn't care about this code before because it has never been executed
>>> (iommu_use_hap_pt(d) always returned true on ARM so far).
>>> But, with introducing the non-shared IOMMU patch series we can no longer keep
>>> this code as is since it will be executed for non-shared IOMMU.
>>> So, move this code to x86-specific file in order not to expose a possible bug.
>>
>> Yes, this helps.
>
> Having thought about this some more, what's still missing is a
> clear explanation why this new need of a non-stub mfn_to_gmfn()
> isn't finally enough of a reason to introduce an M2P on ARM. We
> currently have several uses already which ARM fakes one way or
> another:
> - gnttab_shared_gmfn()

This does not use mfn_to_gmfn on ARM.

> - gnttab_status_gmfn()

gnttab_status_gmfn() returns 0 so far. I have to look at this one.

> - memory_exchange()

Memory exchange does not work on ARM today and will require more work 
than that. When I looked at the code a couple of years ago, it was 
possible to drop the call to mfn_to_gmfn().

> - getdomaininfo()

We could rework to store the gmfn in arch_domain.

> With this I think there's quite a bit of justification needed to keep
> going without M2P on ARM.

As said in a previous thread, I made a quick calculation, ARM32 supports 
up 40-bit PA and IPA (e.g guest address), which means 28-bits of 
MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so 
we would need 2^28 * 4 = 1GiB of virtual address space and potentially 
physical memory. We don't have 1GB of VA space free on 32-bit right now.

ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means 
36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for 
alignment, so we would need 2^36 * 8 = 512GiB of virtual address space 
and potentially physical memory. While virtual address space is not a 
problem, the memory is a problem for embedded platform. We want Xen to 
be as lean as possible.

So the M2P is not a solution on ARM. A better approach is to drop those 
calls from common code and replace by something different (as we did for 
gnttab_shared_mfn).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote:
> On 15/05/2017 08:20, Jan Beulich wrote:
>> Having thought about this some more, what's still missing is a
>> clear explanation why this new need of a non-stub mfn_to_gmfn()
>> isn't finally enough of a reason to introduce an M2P on ARM. We
>> currently have several uses already which ARM fakes one way or
>> another:
>> - gnttab_shared_gmfn()
> 
> This does not use mfn_to_gmfn on ARM.

Right, at the price of maintaining some other helper data.

>> - gnttab_status_gmfn()
> 
> gnttab_status_gmfn() returns 0 so far. I have to look at this one.
> 
>> - memory_exchange()
> 
> Memory exchange does not work on ARM today and will require more work 
> than that. When I looked at the code a couple of years ago, it was 
> possible to drop the call to mfn_to_gmfn().
> 
>> - getdomaininfo()
> 
> We could rework to store the gmfn in arch_domain.

Which again would mean you maintain extra data in order to avoid
the more general M2P.

>> With this I think there's quite a bit of justification needed to keep
>> going without M2P on ARM.
> 
> As said in a previous thread, I made a quick calculation, ARM32 supports 
> up 40-bit PA and IPA (e.g guest address), which means 28-bits of 
> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so 
> we would need 2^28 * 4 = 1GiB of virtual address space and potentially 
> physical memory. We don't have 1GB of VA space free on 32-bit right now.

How come? You don't share address spaces with guests.

> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means 
> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for 
> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space 
> and potentially physical memory. While virtual address space is not a 
> problem, the memory is a problem for embedded platform. We want Xen to 
> be as lean as possible.

You don't need to allocate all 512Gb, the table can be as sparse as
present memory permits.

> So the M2P is not a solution on ARM. A better approach is to drop those 
> calls from common code and replace by something different (as we did for 
> gnttab_shared_mfn).

I'm of the exact opposite opinion. Or at the very least, have a mode
(read: config or command line option) where ARM maintains M2P and
make features like the IOMMU one here depend on being in that mode.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Oleksandr Tyshchenko]

Hi, Jan

On Mon, May 15, 2017 at 10:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 12.05.17 at 18:25, <olekstysh@gmail.com> wrote:
>> On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote:
>>>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>>>>>>      return 0;
>>>>>>  }
>>>>>>
>>>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
>>>>>
>>>>> Looking over the titles of the rest of this series it doesn't look like
>>>>> you're eliminating this TODO later. While I appreciate this not
>>>>> being done in the already large patch, I don't think such a TODO
>>>>> should be left around. If need be (e.g. because you can't test
>>>>> the change), get in touch with the maintainer(s).
>>>> I will drop this TODO everywhere.
>>>
>>> By "drop" you mean "address" or really just "drop"?
>> I meant just drop.
>
> Then I'm sorry, but no, this is not a way to address the comment I've
> made.

Indeed, there was some misunderstanding from my side on this.
Let me elaborate a bit more on this:
1. Yes, this TODO shouldn't be just dropped, but needs to be
addressed, so at least I will have them back in the patch
2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
it makes me lots of work to do this change
properly, so this is not only the question of testing the code, but rather
having it written.
3. Please correct me if I'm wrong, but these are all *optimizations* which
I am mentioning in that TODO, not something that breaks x86 or affects it
in any way.

That being said, can we postpone implementation of the *optimizations*
in question
and have those as a separate activity?
Or if these *optimizations* must be present in the current patch
series, could you, please, provide me with some hints how
these TODO should be properly implemented?

>
> Jan
>


-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Julien Grall]

Hi Jan,

On 05/15/2017 09:19 AM, Jan Beulich wrote:
>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote:
>> On 15/05/2017 08:20, Jan Beulich wrote:
>>> Having thought about this some more, what's still missing is a
>>> clear explanation why this new need of a non-stub mfn_to_gmfn()
>>> isn't finally enough of a reason to introduce an M2P on ARM. We
>>> currently have several uses already which ARM fakes one way or
>>> another:
>>> - gnttab_shared_gmfn()
>>
>> This does not use mfn_to_gmfn on ARM.
>
> Right, at the price of maintaining some other helper data.

saving few MB of memory in small board and hundreds in server if we use 
an M2P. The choice is very easy here.

>
>>> - gnttab_status_gmfn()
>>
>> gnttab_status_gmfn() returns 0 so far. I have to look at this one.
>>
>>> - memory_exchange()
>>
>> Memory exchange does not work on ARM today and will require more work
>> than that. When I looked at the code a couple of years ago, it was
>> possible to drop the call to mfn_to_gmfn().
>>
>>> - getdomaininfo()
>>
>> We could rework to store the gmfn in arch_domain.
>
> Which again would mean you maintain extra data in order to avoid
> the more general M2P.

Yes saving MBs as above.

>
>>> With this I think there's quite a bit of justification needed to keep
>>> going without M2P on ARM.
>>
>> As said in a previous thread, I made a quick calculation, ARM32 supports
>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of
>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so
>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially
>> physical memory. We don't have 1GB of VA space free on 32-bit right now.
>
> How come? You don't share address spaces with guests.

Below the layout for ARM32:


  *   0  -  12M   <COMMON>
  *
  *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
  * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
  *                    space
  *
  *   1G -   2G   Xenheap: always-mapped memory
  *   2G -   4G   Domheap: on-demand-mapped
  *


>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means
>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for
>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space
>> and potentially physical memory. While virtual address space is not a
>> problem, the memory is a problem for embedded platform. We want Xen to
>> be as lean as possible.
>
> You don't need to allocate all 512Gb, the table can be as sparse as
> present memory permits.

I am aware about that... The main point is reducing the footprint of 
Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P.

Likely this will increase the footprint of Xen ARM. For what benefits? 
Avoiding to store few byte in a non-generic way when we need it...

I will comment about the IOMMU below.

>
>> So the M2P is not a solution on ARM. A better approach is to drop those
>> calls from common code and replace by something different (as we did for
>> gnttab_shared_mfn).
>
> I'm of the exact opposite opinion. Or at the very least, have a mode
> (read: config or command line option) where ARM maintains M2P and
> make features like the IOMMU one here depend on being in that mode.

Well, in embedded platform you will know in advance that you will 
passthrough devices to a guest. So there is no point of deferring the 
creation of the page-tables until a device is been assigned.

In server side, I would expect page-table to be shared most of the time. 
We might have to unshare some part of the page-tables but not everything 
as it is currently done on x86.

So far, you didn't convince me the M2P is the right solution for ARM. We 
use more memory for benefits of, AFAICT, of device hotpluging (?) and 
been "generic".

Anyway, I will let Stefano give his opinion on it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Jan Beulich]

>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote:
> On Mon, May 15, 2017 at 10:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 12.05.17 at 18:25, <olekstysh@gmail.com> wrote:
>>> On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote:
>>>>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>>>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>>>>>>>      return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
>>>>>>
>>>>>> Looking over the titles of the rest of this series it doesn't look like
>>>>>> you're eliminating this TODO later. While I appreciate this not
>>>>>> being done in the already large patch, I don't think such a TODO
>>>>>> should be left around. If need be (e.g. because you can't test
>>>>>> the change), get in touch with the maintainer(s).
>>>>> I will drop this TODO everywhere.
>>>>
>>>> By "drop" you mean "address" or really just "drop"?
>>> I meant just drop.
>>
>> Then I'm sorry, but no, this is not a way to address the comment I've
>> made.
> 
> Indeed, there was some misunderstanding from my side on this.
> Let me elaborate a bit more on this:
> 1. Yes, this TODO shouldn't be just dropped, but needs to be
> addressed, so at least I will have them back in the patch
> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
> it makes me lots of work to do this change
> properly, so this is not only the question of testing the code, but rather
> having it written.
> 3. Please correct me if I'm wrong, but these are all *optimizations* which
> I am mentioning in that TODO, not something that breaks x86 or affects it
> in any way.
> 
> That being said, can we postpone implementation of the *optimizations*
> in question
> and have those as a separate activity?
> Or if these *optimizations* must be present in the current patch
> series, could you, please, provide me with some hints how
> these TODO should be properly implemented?

I'm puzzled. When I first commented on these TODOs I did say
"While I appreciate this not being done in the already large patch,
I don't think such a TODO should be left around. If need be (e.g.
because you can't test the change), get in touch with the
maintainer(s)." Of course the "e.g." extends to the actual
implementation. IOW I'm not saying you need to do this work
immediately and all by yourself, but there should be a clear plan
on getting these items addressed. We shouldn't ship several
releases with them still present. I'm sorry this hits you, but we've
had too bad experience in the past with people leaving todo or
fixme notes in the code, perhaps even promising to address them
without ever doing so.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote:
> On 05/15/2017 09:19 AM, Jan Beulich wrote:
>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote:
>>> On 15/05/2017 08:20, Jan Beulich wrote:
>>>> With this I think there's quite a bit of justification needed to keep
>>>> going without M2P on ARM.
>>>
>>> As said in a previous thread, I made a quick calculation, ARM32 supports
>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of
>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so
>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially
>>> physical memory. We don't have 1GB of VA space free on 32-bit right now.
>>
>> How come? You don't share address spaces with guests.
> 
> Below the layout for ARM32:
> 
> 
>   *   0  -  12M   <COMMON>
>   *
>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>   *                    space
>   *
>   *   1G -   2G   Xenheap: always-mapped memory
>   *   2G -   4G   Domheap: on-demand-mapped

Since Domheap hardly covers all memory, the obvious thing would
seem to be to take part of that region, just like on x86 we also
had to reduce the direct mapping area in order to support systems
with more than 5Tb.

>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means
>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for
>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space
>>> and potentially physical memory. While virtual address space is not a
>>> problem, the memory is a problem for embedded platform. We want Xen to
>>> be as lean as possible.
>>
>> You don't need to allocate all 512Gb, the table can be as sparse as
>> present memory permits.
> 
> I am aware about that... The main point is reducing the footprint of 
> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P.
> 
> Likely this will increase the footprint of Xen ARM. For what benefits? 
> Avoiding to store few byte in a non-generic way when we need it...

But that's the point: Generic code becomes more and more clumsy
if non-generic mechanisms need to be introduced. Quite a few we've
had the discussion of saving a few Mb here or there, and I've almost
always been the one to ask for not wasting memory even if we have
plenty, so I'm all with you on that aspect. Nevertheless there is a
point where the trade-off between memory overhead and generic
(i.e. easier to maintain) code crosses a boundary, and I'm simply
wondering whether we aren't at that point.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Oleksandr Tyshchenko]

Hi, Jan

On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote:
>> On Mon, May 15, 2017 at 10:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 12.05.17 at 18:25, <olekstysh@gmail.com> wrote:
>>>> On Fri, May 12, 2017 at 7:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 12.05.17 at 17:50, <olekstysh@gmail.com> wrote:
>>>>>> On Fri, May 12, 2017 at 5:23 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>>>>>>> @@ -771,6 +773,47 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long gfn)
>>>>>>>>      return 0;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */
>>>>>>>
>>>>>>> Looking over the titles of the rest of this series it doesn't look like
>>>>>>> you're eliminating this TODO later. While I appreciate this not
>>>>>>> being done in the already large patch, I don't think such a TODO
>>>>>>> should be left around. If need be (e.g. because you can't test
>>>>>>> the change), get in touch with the maintainer(s).
>>>>>> I will drop this TODO everywhere.
>>>>>
>>>>> By "drop" you mean "address" or really just "drop"?
>>>> I meant just drop.
>>>
>>> Then I'm sorry, but no, this is not a way to address the comment I've
>>> made.
>>
>> Indeed, there was some misunderstanding from my side on this.
>> Let me elaborate a bit more on this:
>> 1. Yes, this TODO shouldn't be just dropped, but needs to be
>> addressed, so at least I will have them back in the patch
>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
>> it makes me lots of work to do this change
>> properly, so this is not only the question of testing the code, but rather
>> having it written.
>> 3. Please correct me if I'm wrong, but these are all *optimizations* which
>> I am mentioning in that TODO, not something that breaks x86 or affects it
>> in any way.
>>
>> That being said, can we postpone implementation of the *optimizations*
>> in question
>> and have those as a separate activity?
>> Or if these *optimizations* must be present in the current patch
>> series, could you, please, provide me with some hints how
>> these TODO should be properly implemented?
>
> I'm puzzled. When I first commented on these TODOs I did say
> "While I appreciate this not being done in the already large patch,
> I don't think such a TODO should be left around. If need be (e.g.
> because you can't test the change), get in touch with the
> maintainer(s)." Of course the "e.g." extends to the actual
> implementation. IOW I'm not saying you need to do this work
> immediately and all by yourself, but there should be a clear plan
> on getting these items addressed. We shouldn't ship several
> releases with them still present. I'm sorry this hits you, but we've
> had too bad experience in the past with people leaving todo or
> fixme notes in the code, perhaps even promising to address them
> without ever doing so.
I see. You are right about leaving TODO)
Don't mind to get these items addressed *within current patch series*
as separate patch or patches.
So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will
leave SMMU for myself.

Could you, please, provide me with some hints how these TODO should be
properly implemented?
Or
I was thinking I can even just squash *pages with *page and send you a
draft as we need to start from somewhere.
What do you think?

>
> Jan
>

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Jan Beulich]

>>> On 16.05.17 at 14:48, <olekstysh@gmail.com> wrote:
> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote:
>>> Indeed, there was some misunderstanding from my side on this.
>>> Let me elaborate a bit more on this:
>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be
>>> addressed, so at least I will have them back in the patch
>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
>>> it makes me lots of work to do this change
>>> properly, so this is not only the question of testing the code, but rather
>>> having it written.
>>> 3. Please correct me if I'm wrong, but these are all *optimizations* which
>>> I am mentioning in that TODO, not something that breaks x86 or affects it
>>> in any way.
>>>
>>> That being said, can we postpone implementation of the *optimizations*
>>> in question
>>> and have those as a separate activity?
>>> Or if these *optimizations* must be present in the current patch
>>> series, could you, please, provide me with some hints how
>>> these TODO should be properly implemented?
>>
>> I'm puzzled. When I first commented on these TODOs I did say
>> "While I appreciate this not being done in the already large patch,
>> I don't think such a TODO should be left around. If need be (e.g.
>> because you can't test the change), get in touch with the
>> maintainer(s)." Of course the "e.g." extends to the actual
>> implementation. IOW I'm not saying you need to do this work
>> immediately and all by yourself, but there should be a clear plan
>> on getting these items addressed. We shouldn't ship several
>> releases with them still present. I'm sorry this hits you, but we've
>> had too bad experience in the past with people leaving todo or
>> fixme notes in the code, perhaps even promising to address them
>> without ever doing so.
> I see. You are right about leaving TODO)
> Don't mind to get these items addressed *within current patch series*
> as separate patch or patches.
> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will
> leave SMMU for myself.
> 
> Could you, please, provide me with some hints how these TODO should be
> properly implemented?

I have to admit that I don't really understand the request. Quite
clearly we want to use large pages in the case that hardware
supports them.

> I was thinking I can even just squash *pages with *page and send you a
> draft as we need to start from somewhere.

I'm afraid I've lost too much of the context to see what you mean
here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Oleksandr Tyshchenko]

Hi, Jan.

On Tue, May 16, 2017 at 4:11 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.05.17 at 14:48, <olekstysh@gmail.com> wrote:
>> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote:
>>>> Indeed, there was some misunderstanding from my side on this.
>>>> Let me elaborate a bit more on this:
>>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be
>>>> addressed, so at least I will have them back in the patch
>>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
>>>> it makes me lots of work to do this change
>>>> properly, so this is not only the question of testing the code, but rather
>>>> having it written.
>>>> 3. Please correct me if I'm wrong, but these are all *optimizations* which
>>>> I am mentioning in that TODO, not something that breaks x86 or affects it
>>>> in any way.
>>>>
>>>> That being said, can we postpone implementation of the *optimizations*
>>>> in question
>>>> and have those as a separate activity?
>>>> Or if these *optimizations* must be present in the current patch
>>>> series, could you, please, provide me with some hints how
>>>> these TODO should be properly implemented?
>>>
>>> I'm puzzled. When I first commented on these TODOs I did say
>>> "While I appreciate this not being done in the already large patch,
>>> I don't think such a TODO should be left around. If need be (e.g.
>>> because you can't test the change), get in touch with the
>>> maintainer(s)." Of course the "e.g." extends to the actual
>>> implementation. IOW I'm not saying you need to do this work
>>> immediately and all by yourself, but there should be a clear plan
>>> on getting these items addressed. We shouldn't ship several
>>> releases with them still present. I'm sorry this hits you, but we've
>>> had too bad experience in the past with people leaving todo or
>>> fixme notes in the code, perhaps even promising to address them
>>> without ever doing so.
>> I see. You are right about leaving TODO)
>> Don't mind to get these items addressed *within current patch series*
>> as separate patch or patches.
>> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will
>> leave SMMU for myself.
>>
>> Could you, please, provide me with some hints how these TODO should be
>> properly implemented?
>
> I have to admit that I don't really understand the request. Quite
> clearly we want to use large pages in the case that hardware
> supports them.
>
>> I was thinking I can even just squash *pages with *page and send you a
>> draft as we need to start from somewhere.
>
> I'm afraid I've lost too much of the context to see what you mean
> here.
Sorry if I was unclear.

At the moment patch contains three TODOs in the following files:
1. a/xen/drivers/passthrough/vtd/iommu.c
2. a/xen/drivers/passthrough/amd/iommu_map.c
3. a/xen/drivers/passthrough/arm/smmu.c

And the *optimization* which I mentioned in that TODO is same for all
three files.
+/* TODO: Optimize by squashing map_pages/unmap_pages with
map_page/unmap_page */

I think that I could try to address this TODO by myself as I imagine
it should be addressed and send you a draft or post RFC patch.
As the result of this RFC patch we would have map_pages/unmap_pages
callbacks only, but still iterate 4K pages.

We need to start from somewhere and this patch would be a base point
for continue optimizing.
What do you think? Or you have another opinion?

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Jan Beulich]

>>> On 17.05.17 at 17:28, <olekstysh@gmail.com> wrote:
> Hi, Jan.
> 
> On Tue, May 16, 2017 at 4:11 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 16.05.17 at 14:48, <olekstysh@gmail.com> wrote:
>>> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote:
>>>>> Indeed, there was some misunderstanding from my side on this.
>>>>> Let me elaborate a bit more on this:
>>>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be
>>>>> addressed, so at least I will have them back in the patch
>>>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
>>>>> it makes me lots of work to do this change
>>>>> properly, so this is not only the question of testing the code, but rather
>>>>> having it written.
>>>>> 3. Please correct me if I'm wrong, but these are all *optimizations* which
>>>>> I am mentioning in that TODO, not something that breaks x86 or affects it
>>>>> in any way.
>>>>>
>>>>> That being said, can we postpone implementation of the *optimizations*
>>>>> in question
>>>>> and have those as a separate activity?
>>>>> Or if these *optimizations* must be present in the current patch
>>>>> series, could you, please, provide me with some hints how
>>>>> these TODO should be properly implemented?
>>>>
>>>> I'm puzzled. When I first commented on these TODOs I did say
>>>> "While I appreciate this not being done in the already large patch,
>>>> I don't think such a TODO should be left around. If need be (e.g.
>>>> because you can't test the change), get in touch with the
>>>> maintainer(s)." Of course the "e.g." extends to the actual
>>>> implementation. IOW I'm not saying you need to do this work
>>>> immediately and all by yourself, but there should be a clear plan
>>>> on getting these items addressed. We shouldn't ship several
>>>> releases with them still present. I'm sorry this hits you, but we've
>>>> had too bad experience in the past with people leaving todo or
>>>> fixme notes in the code, perhaps even promising to address them
>>>> without ever doing so.
>>> I see. You are right about leaving TODO)
>>> Don't mind to get these items addressed *within current patch series*
>>> as separate patch or patches.
>>> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will
>>> leave SMMU for myself.
>>>
>>> Could you, please, provide me with some hints how these TODO should be
>>> properly implemented?
>>
>> I have to admit that I don't really understand the request. Quite
>> clearly we want to use large pages in the case that hardware
>> supports them.
>>
>>> I was thinking I can even just squash *pages with *page and send you a
>>> draft as we need to start from somewhere.
>>
>> I'm afraid I've lost too much of the context to see what you mean
>> here.
> Sorry if I was unclear.
> 
> At the moment patch contains three TODOs in the following files:
> 1. a/xen/drivers/passthrough/vtd/iommu.c
> 2. a/xen/drivers/passthrough/amd/iommu_map.c
> 3. a/xen/drivers/passthrough/arm/smmu.c
> 
> And the *optimization* which I mentioned in that TODO is same for all
> three files.
> +/* TODO: Optimize by squashing map_pages/unmap_pages with
> map_page/unmap_page */
> 
> I think that I could try to address this TODO by myself as I imagine
> it should be addressed and send you a draft or post RFC patch.
> As the result of this RFC patch we would have map_pages/unmap_pages
> callbacks only, but still iterate 4K pages.
> 
> We need to start from somewhere and this patch would be a base point
> for continue optimizing.
> What do you think? Or you have another opinion?

Well, yes, this would reduce the scope of the TODO, but no, it
wouldn't eliminate it. After all the primary goal (from my
perspective) of adding the order parameter is to make use of
large pages whenever possible. And as said before - it's not like
it has to be you who does the work, but I'd sort of expect that
whoever introduces TODOs at least tries to arrange for them
being taken care of (unless e.g. they affect exotic situations
only), for example by pulling in the respective maintainers.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

Hi, Jan.

On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote:
>> On 05/15/2017 09:19 AM, Jan Beulich wrote:
>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote:
>>>> On 15/05/2017 08:20, Jan Beulich wrote:
>>>>> With this I think there's quite a bit of justification needed to keep
>>>>> going without M2P on ARM.
>>>>
>>>> As said in a previous thread, I made a quick calculation, ARM32 supports
>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of
>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so
>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially
>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now.
>>>
>>> How come? You don't share address spaces with guests.
>>
>> Below the layout for ARM32:
>>
>>
>>   *   0  -  12M   <COMMON>
>>   *
>>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>>   *                    space
>>   *
>>   *   1G -   2G   Xenheap: always-mapped memory
>>   *   2G -   4G   Domheap: on-demand-mapped
>
> Since Domheap hardly covers all memory, the obvious thing would
> seem to be to take part of that region, just like on x86 we also
> had to reduce the direct mapping area in order to support systems
> with more than 5Tb.
>
>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means
>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for
>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space
>>>> and potentially physical memory. While virtual address space is not a
>>>> problem, the memory is a problem for embedded platform. We want Xen to
>>>> be as lean as possible.
>>>
>>> You don't need to allocate all 512Gb, the table can be as sparse as
>>> present memory permits.
>>
>> I am aware about that... The main point is reducing the footprint of
>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P.
>>
>> Likely this will increase the footprint of Xen ARM. For what benefits?
>> Avoiding to store few byte in a non-generic way when we need it...
>
> But that's the point: Generic code becomes more and more clumsy
> if non-generic mechanisms need to be introduced. Quite a few we've
> had the discussion of saving a few Mb here or there, and I've almost
> always been the one to ask for not wasting memory even if we have
> plenty, so I'm all with you on that aspect. Nevertheless there is a
> point where the trade-off between memory overhead and generic
> (i.e. easier to maintain) code crosses a boundary, and I'm simply
> wondering whether we aren't at that point.

Is the lack of M2P support on ARM a blocker for this patch to be accepted?
This patch I think is only prevents us from possible bugs in a future.
Please correct me if I am wrong.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote:
> On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote:
>>> On 05/15/2017 09:19 AM, Jan Beulich wrote:
>>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote:
>>>>> On 15/05/2017 08:20, Jan Beulich wrote:
>>>>>> With this I think there's quite a bit of justification needed to keep
>>>>>> going without M2P on ARM.
>>>>>
>>>>> As said in a previous thread, I made a quick calculation, ARM32 supports
>>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of
>>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so
>>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially
>>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now.
>>>>
>>>> How come? You don't share address spaces with guests.
>>>
>>> Below the layout for ARM32:
>>>
>>>
>>>   *   0  -  12M   <COMMON>
>>>   *
>>>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>>>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>>>   *                    space
>>>   *
>>>   *   1G -   2G   Xenheap: always-mapped memory
>>>   *   2G -   4G   Domheap: on-demand-mapped
>>
>> Since Domheap hardly covers all memory, the obvious thing would
>> seem to be to take part of that region, just like on x86 we also
>> had to reduce the direct mapping area in order to support systems
>> with more than 5Tb.
>>
>>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means
>>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for
>>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space
>>>>> and potentially physical memory. While virtual address space is not a
>>>>> problem, the memory is a problem for embedded platform. We want Xen to
>>>>> be as lean as possible.
>>>>
>>>> You don't need to allocate all 512Gb, the table can be as sparse as
>>>> present memory permits.
>>>
>>> I am aware about that... The main point is reducing the footprint of
>>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P.
>>>
>>> Likely this will increase the footprint of Xen ARM. For what benefits?
>>> Avoiding to store few byte in a non-generic way when we need it...
>>
>> But that's the point: Generic code becomes more and more clumsy
>> if non-generic mechanisms need to be introduced. Quite a few we've
>> had the discussion of saving a few Mb here or there, and I've almost
>> always been the one to ask for not wasting memory even if we have
>> plenty, so I'm all with you on that aspect. Nevertheless there is a
>> point where the trade-off between memory overhead and generic
>> (i.e. easier to maintain) code crosses a boundary, and I'm simply
>> wondering whether we aren't at that point.
> 
> Is the lack of M2P support on ARM a blocker for this patch to be accepted?

Well, if the ARM maintainers insist on baking their own thing every
time we'd use the M2P if it was there, I think I can't reasonably
block this patch. Otoh I'd prefer to not see the non-x86-specific
code move to x86/, so perhaps the whole patch wants
re-structuring using either a manifest definition in the ARM headers
or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't).

> This patch I think is only prevents us from possible bugs in a future.
> Please correct me if I am wrong.

Avoiding possible bugs in the future I didn't connect to this patch so
far.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Oleksandr Tyshchenko]

Hi, all.

On Wed, May 17, 2017 at 6:39 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 17.05.17 at 17:28, <olekstysh@gmail.com> wrote:
>> Hi, Jan.
>>
>> On Tue, May 16, 2017 at 4:11 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 16.05.17 at 14:48, <olekstysh@gmail.com> wrote:
>>>> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 15.05.17 at 12:43, <olekstysh@gmail.com> wrote:
>>>>>> Indeed, there was some misunderstanding from my side on this.
>>>>>> Let me elaborate a bit more on this:
>>>>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be
>>>>>> addressed, so at least I will have them back in the patch
>>>>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
>>>>>> it makes me lots of work to do this change
>>>>>> properly, so this is not only the question of testing the code, but rather
>>>>>> having it written.
>>>>>> 3. Please correct me if I'm wrong, but these are all *optimizations* which
>>>>>> I am mentioning in that TODO, not something that breaks x86 or affects it
>>>>>> in any way.
>>>>>>
>>>>>> That being said, can we postpone implementation of the *optimizations*
>>>>>> in question
>>>>>> and have those as a separate activity?
>>>>>> Or if these *optimizations* must be present in the current patch
>>>>>> series, could you, please, provide me with some hints how
>>>>>> these TODO should be properly implemented?
>>>>>
>>>>> I'm puzzled. When I first commented on these TODOs I did say
>>>>> "While I appreciate this not being done in the already large patch,
>>>>> I don't think such a TODO should be left around. If need be (e.g.
>>>>> because you can't test the change), get in touch with the
>>>>> maintainer(s)." Of course the "e.g." extends to the actual
>>>>> implementation. IOW I'm not saying you need to do this work
>>>>> immediately and all by yourself, but there should be a clear plan
>>>>> on getting these items addressed. We shouldn't ship several
>>>>> releases with them still present. I'm sorry this hits you, but we've
>>>>> had too bad experience in the past with people leaving todo or
>>>>> fixme notes in the code, perhaps even promising to address them
>>>>> without ever doing so.
>>>> I see. You are right about leaving TODO)
>>>> Don't mind to get these items addressed *within current patch series*
>>>> as separate patch or patches.
>>>> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will
>>>> leave SMMU for myself.
>>>>
>>>> Could you, please, provide me with some hints how these TODO should be
>>>> properly implemented?
>>>
>>> I have to admit that I don't really understand the request. Quite
>>> clearly we want to use large pages in the case that hardware
>>> supports them.
>>>
>>>> I was thinking I can even just squash *pages with *page and send you a
>>>> draft as we need to start from somewhere.
>>>
>>> I'm afraid I've lost too much of the context to see what you mean
>>> here.
>> Sorry if I was unclear.
>>
>> At the moment patch contains three TODOs in the following files:
>> 1. a/xen/drivers/passthrough/vtd/iommu.c
>> 2. a/xen/drivers/passthrough/amd/iommu_map.c
>> 3. a/xen/drivers/passthrough/arm/smmu.c
>>
>> And the *optimization* which I mentioned in that TODO is same for all
>> three files.
>> +/* TODO: Optimize by squashing map_pages/unmap_pages with
>> map_page/unmap_page */
>>
>> I think that I could try to address this TODO by myself as I imagine
>> it should be addressed and send you a draft or post RFC patch.
>> As the result of this RFC patch we would have map_pages/unmap_pages
>> callbacks only, but still iterate 4K pages.
>>
>> We need to start from somewhere and this patch would be a base point
>> for continue optimizing.
>> What do you think? Or you have another opinion?
>
> Well, yes, this would reduce the scope of the TODO, but no, it
> wouldn't eliminate it. After all the primary goal (from my
> perspective) of adding the order parameter is to make use of
> large pages whenever possible. And as said before - it's not like
> it has to be you who does the work, but I'd sort of expect that
> whoever introduces TODOs at least tries to arrange for them
> being taken care of (unless e.g. they affect exotic situations
> only), for example by pulling in the respective maintainers.

Jan,
I understand what you are taking about. CCed respective maintainers.

Kevin, Suravee,
First of all my apologies for the fact that I haven't CCed your
earlier. I added changes
to files you are the maintainers of. My bad.

A bit of context.
This patch touches Intel/AMD IOMMUs as well as other IOMMUs since it adds extra
order argument. The purpose of adding extra order argument is to make use of
super pages whenever possible. Being honest I am interested in adding
IPMMU support
on ARM and this kind of IOMMU does support super pages. And as we
don't want to keep
separate single-page and multi-page stuff together it was decided to
rename existing APIs/callbacks
and add order argument.
In order not to brake existing x86-specific drivers (to retain current behavior)
I had to introduce additional helpers inside them and leave some TODO
which describe that
some optimization is needed.

I can try to reduce the scope of these TODO (to have
map_pages/unmap_pages callbacks only,
but still iterate 4K pages even if hardware supports large pages), but
I am sure that I won't be able to eliminate them completely
(to use large pages in the case that hardware supports them) due to
the several reasons.
I am neither familiar with x86 nor even have x86 boards, excuse me,
but I don't even know support these hardware super pages or not.

I want this patch to be accepted, so some common ground should be
found on getting these items addressed. Maybe you already have
some plan regarding adding such support?

>
> Jan

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

Hi, all.

On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote:
>> On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote:
>>>> On 05/15/2017 09:19 AM, Jan Beulich wrote:
>>>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote:
>>>>>> On 15/05/2017 08:20, Jan Beulich wrote:
>>>>>>> With this I think there's quite a bit of justification needed to keep
>>>>>>> going without M2P on ARM.
>>>>>>
>>>>>> As said in a previous thread, I made a quick calculation, ARM32 supports
>>>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of
>>>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so
>>>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially
>>>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now.
>>>>>
>>>>> How come? You don't share address spaces with guests.
>>>>
>>>> Below the layout for ARM32:
>>>>
>>>>
>>>>   *   0  -  12M   <COMMON>
>>>>   *
>>>>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>>>>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>>>>   *                    space
>>>>   *
>>>>   *   1G -   2G   Xenheap: always-mapped memory
>>>>   *   2G -   4G   Domheap: on-demand-mapped
>>>
>>> Since Domheap hardly covers all memory, the obvious thing would
>>> seem to be to take part of that region, just like on x86 we also
>>> had to reduce the direct mapping area in order to support systems
>>> with more than 5Tb.
>>>
>>>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means
>>>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for
>>>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space
>>>>>> and potentially physical memory. While virtual address space is not a
>>>>>> problem, the memory is a problem for embedded platform. We want Xen to
>>>>>> be as lean as possible.
>>>>>
>>>>> You don't need to allocate all 512Gb, the table can be as sparse as
>>>>> present memory permits.
>>>>
>>>> I am aware about that... The main point is reducing the footprint of
>>>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P.
>>>>
>>>> Likely this will increase the footprint of Xen ARM. For what benefits?
>>>> Avoiding to store few byte in a non-generic way when we need it...
>>>
>>> But that's the point: Generic code becomes more and more clumsy
>>> if non-generic mechanisms need to be introduced. Quite a few we've
>>> had the discussion of saving a few Mb here or there, and I've almost
>>> always been the one to ask for not wasting memory even if we have
>>> plenty, so I'm all with you on that aspect. Nevertheless there is a
>>> point where the trade-off between memory overhead and generic
>>> (i.e. easier to maintain) code crosses a boundary, and I'm simply
>>> wondering whether we aren't at that point.
>>
>> Is the lack of M2P support on ARM a blocker for this patch to be accepted?
>
> Well, if the ARM maintainers insist on baking their own thing every
> time we'd use the M2P if it was there, I think I can't reasonably
> block this patch. Otoh I'd prefer to not see the non-x86-specific
> code move to x86/, so perhaps the whole patch wants
> re-structuring using either a manifest definition in the ARM headers
> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't).
Jan, I am afraid but I didn't get what you meant here:
"manifest definition in the ARM headers"

Julien, Stefano what do you think in general?

>
>> This patch I think is only prevents us from possible bugs in a future.
>> Please correct me if I am wrong.
>
> Avoiding possible bugs in the future I didn't connect to this patch so
> far.
>
> Jan
>

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Julien Grall]

Hi Jan,

On 05/12/2017 03:31 PM, Jan Beulich wrote:
>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> The presence of this flag lets us know that the guest
>> has devices which will most likely be used for passthrough
>> and as the result the use of IOMMU is expected for this domain.
>> In that case we have to call iommu_construct(), actually
>> what the real assign_device call usually does.
>>
>> As iommu_domain_init() is called with use_iommu flag being forced
>> to false for now, no functional change is intended for both ARM and x86.
>>
>> Basically, this patch is needed for non-shared IOMMUs on ARM only
>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called
>> later. But, in order to be more generic and for possible future optimization
>> make this change appliable for both platforms.
>
> I continue to be unconvinced that this is wanted / needed, as I
> continue to not see why shared vs unshared really matters here.
> After all we have both modes working on x86 without this flag.

Well on x86 you allocate the page table on the fly in the unsharing 
case. This is only useful if you don't know whether a domain will have 
device assigned (e.g hotplug case).

When you know that the domain will have device pass-throughed, you can 
populate the IOMMU page tables before hand avoiding to have to go 
through the list of page at the first assigned device.

In embedded platform hotplug is likely to be inexistent. For servers, I 
don't know but likely page tables are going to be shared (or as I 
mentioned earlier partially shared).

So I don't see any benefit of the current code over populating the IOMMU 
page tables from the beginning.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Julien Grall]

On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote:
> Hi, all.

Hi Oleksandr,

> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote:
>>> On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote:
>>>>> On 05/15/2017 09:19 AM, Jan Beulich wrote:
>>>>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote:
>>>>>>> On 15/05/2017 08:20, Jan Beulich wrote:
>>>>>>>> With this I think there's quite a bit of justification needed to keep
>>>>>>>> going without M2P on ARM.
>>>>>>>
>>>>>>> As said in a previous thread, I made a quick calculation, ARM32 supports
>>>>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of
>>>>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so
>>>>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially
>>>>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now.
>>>>>>
>>>>>> How come? You don't share address spaces with guests.
>>>>>
>>>>> Below the layout for ARM32:
>>>>>
>>>>>
>>>>>   *   0  -  12M   <COMMON>
>>>>>   *
>>>>>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>>>>>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>>>>>   *                    space
>>>>>   *
>>>>>   *   1G -   2G   Xenheap: always-mapped memory
>>>>>   *   2G -   4G   Domheap: on-demand-mapped
>>>>
>>>> Since Domheap hardly covers all memory, the obvious thing would
>>>> seem to be to take part of that region, just like on x86 we also
>>>> had to reduce the direct mapping area in order to support systems
>>>> with more than 5Tb.
>>>>
>>>>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means
>>>>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for
>>>>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space
>>>>>>> and potentially physical memory. While virtual address space is not a
>>>>>>> problem, the memory is a problem for embedded platform. We want Xen to
>>>>>>> be as lean as possible.
>>>>>>
>>>>>> You don't need to allocate all 512Gb, the table can be as sparse as
>>>>>> present memory permits.
>>>>>
>>>>> I am aware about that... The main point is reducing the footprint of
>>>>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P.
>>>>>
>>>>> Likely this will increase the footprint of Xen ARM. For what benefits?
>>>>> Avoiding to store few byte in a non-generic way when we need it...
>>>>
>>>> But that's the point: Generic code becomes more and more clumsy
>>>> if non-generic mechanisms need to be introduced. Quite a few we've
>>>> had the discussion of saving a few Mb here or there, and I've almost
>>>> always been the one to ask for not wasting memory even if we have
>>>> plenty, so I'm all with you on that aspect. Nevertheless there is a
>>>> point where the trade-off between memory overhead and generic
>>>> (i.e. easier to maintain) code crosses a boundary, and I'm simply
>>>> wondering whether we aren't at that point.
>>>
>>> Is the lack of M2P support on ARM a blocker for this patch to be accepted?
>>
>> Well, if the ARM maintainers insist on baking their own thing every
>> time we'd use the M2P if it was there, I think I can't reasonably
>> block this patch. Otoh I'd prefer to not see the non-x86-specific
>> code move to x86/, so perhaps the whole patch wants
>> re-structuring using either a manifest definition in the ARM headers
>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't).
> Jan, I am afraid but I didn't get what you meant here:
> "manifest definition in the ARM headers"

I think he meant to have either a define in the header mentioning the 
absence/presence of M2P. But my preference would be using the Kconfig here.

>
> Julien, Stefano what do you think in general?

My point stands here. The M2P sounds a real waste of memory for a 
limited benefit. There are only a few place in common code that care 
about that and the only one where would potentially really need it (e.g 
iommu_hwdom_init()) can be replaced by allocating page table in advance 
(for justification see my answer on patch #6).

I will take an action to replace the few mfn_to_gmfn by wrapper and 
proper implementation for ARM.

So Jan suggestion for CONFIG_M2P (or maybe CONFIG_HAS_M2P) would be a 
good solution.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Jan Beulich]

>>> On 17.05.17 at 21:52, <julien.grall@arm.com> wrote:
> On 05/12/2017 03:31 PM, Jan Beulich wrote:
>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> The presence of this flag lets us know that the guest
>>> has devices which will most likely be used for passthrough
>>> and as the result the use of IOMMU is expected for this domain.
>>> In that case we have to call iommu_construct(), actually
>>> what the real assign_device call usually does.
>>>
>>> As iommu_domain_init() is called with use_iommu flag being forced
>>> to false for now, no functional change is intended for both ARM and x86.
>>>
>>> Basically, this patch is needed for non-shared IOMMUs on ARM only
>>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called
>>> later. But, in order to be more generic and for possible future optimization
>>> make this change appliable for both platforms.
>>
>> I continue to be unconvinced that this is wanted / needed, as I
>> continue to not see why shared vs unshared really matters here.
>> After all we have both modes working on x86 without this flag.
> 
> Well on x86 you allocate the page table on the fly in the unsharing 
> case. This is only useful if you don't know whether a domain will have 
> device assigned (e.g hotplug case).
> 
> When you know that the domain will have device pass-throughed, you can 
> populate the IOMMU page tables before hand avoiding to have to go 
> through the list of page at the first assigned device.
> 
> In embedded platform hotplug is likely to be inexistent. For servers, I 
> don't know but likely page tables are going to be shared (or as I 
> mentioned earlier partially shared).
> 
> So I don't see any benefit of the current code over populating the IOMMU 
> page tables from the beginning.

Interesting. To me, the primary benefit is that we wouldn't need to
introduce new code to handle yet another case specially. Anyway,
the changes in this patch are simple enough, so I don't mean to
block it despite being unconvinced of the basic idea.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote:
> On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote:
>> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>> Well, if the ARM maintainers insist on baking their own thing every
>>> time we'd use the M2P if it was there, I think I can't reasonably
>>> block this patch. Otoh I'd prefer to not see the non-x86-specific
>>> code move to x86/, so perhaps the whole patch wants
>>> re-structuring using either a manifest definition in the ARM headers
>>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't).
>> Jan, I am afraid but I didn't get what you meant here:
>> "manifest definition in the ARM headers"
> 
> I think he meant to have either a define in the header mentioning the 
> absence/presence of M2P.

Yes, at least in a way.

> But my preference would be using the Kconfig here.

Depends on the symbol used: If such a symbol solely _indicates_
the presence, Kconfig would be better indeed. If, however, the
symbol is e.g. a macro resolving to a per-arch implementation,
with common code providing a default definition when the arch
doesn't provide any, then the non-Kconfig variant may be
preferable.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Oleksandr Tyshchenko]

Hi, all.

On Thu, May 18, 2017 at 11:38 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 17.05.17 at 21:52, <julien.grall@arm.com> wrote:
>> On 05/12/2017 03:31 PM, Jan Beulich wrote:
>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> The presence of this flag lets us know that the guest
>>>> has devices which will most likely be used for passthrough
>>>> and as the result the use of IOMMU is expected for this domain.
>>>> In that case we have to call iommu_construct(), actually
>>>> what the real assign_device call usually does.
>>>>
>>>> As iommu_domain_init() is called with use_iommu flag being forced
>>>> to false for now, no functional change is intended for both ARM and x86.
>>>>
>>>> Basically, this patch is needed for non-shared IOMMUs on ARM only
>>>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called
>>>> later. But, in order to be more generic and for possible future optimization
>>>> make this change appliable for both platforms.
>>>
>>> I continue to be unconvinced that this is wanted / needed, as I
>>> continue to not see why shared vs unshared really matters here.
>>> After all we have both modes working on x86 without this flag.
>>
>> Well on x86 you allocate the page table on the fly in the unsharing
>> case. This is only useful if you don't know whether a domain will have
>> device assigned (e.g hotplug case).
>>
>> When you know that the domain will have device pass-throughed, you can
>> populate the IOMMU page tables before hand avoiding to have to go
>> through the list of page at the first assigned device.
>>
>> In embedded platform hotplug is likely to be inexistent. For servers, I
>> don't know but likely page tables are going to be shared (or as I
>> mentioned earlier partially shared).
>>
>> So I don't see any benefit of the current code over populating the IOMMU
>> page tables from the beginning.
>
> Interesting. To me, the primary benefit is that we wouldn't need to
> introduce new code to handle yet another case specially. Anyway,
> the changes in this patch are simple enough, so I don't mean to
> block it despite being unconvinced of the basic idea.

Thank you for your comments.
I would like to say that I share Julien's opinion, but understand
Jan's points too.
I think some mutually agreeable solution should be worked out.

It is not completely clear to me what I have to do with patches #6-#8.
So, I will try to summarize thoughts regarding them. Please, correct
me if I am wrong.

patch #6: As for the current patch, likely the "init" platform
callback should be extended with
extra "use_iommu" argument as well as the "iommu_domain_init" API. In
that case we
would just pass thought incoming flag to the IOMMU drivers followed by
updating "need_iommu" domain flag.

Something like this:
...
int iommu_domain_init(struct domain *d, bool use_iommu)
{
    struct domain_iommu *hd = dom_iommu(d);
    int ret = 0;

    ret = arch_iommu_domain_init(d);
    if ( ret )
        return ret;

    if ( !iommu_enabled )
        return 0;

    hd->platform_ops = iommu_get_ops();
    ret = hd->platform_ops->init(d, use_iommu);
    if ( ret )
        return ret;

    d->need_iommu = !!use_iommu;

    return 0;
}
...

patch #7: This patch should be just dropped.

patch #8: As we always allocate the page table for hardware domain,
this patch should be reworked.
The use_iommu flag should be set for both archs in case of hardware
domain. Having d->need_iommu set at the early stage we won't skip
IOMMU
mapping updates anymore. And as the result the existing in
iommu_hwdom_init() code that goes through the list of page and tries
to retrieve mapping could be just dropped
instead of moving it to the arch-specific part.

So, what we would have as the final result:
1. In case of hardware domain "use_iommu" flag is always set for both
ARM and x86.
2. For other domains the "use_iommu" flag is always unset for x86
only, but the real value is passed for ARM
according to the libxl expectation about IOMMU usage.
This would allow us to allocate the page table in advance on ARM and
retain the current behavior for x86 (allocating the page table
on-demand).

What do you think?

>
> Jan
>

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

Hi, all.

On Thu, May 18, 2017 at 11:53 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote:
>> On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote:
>>> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> Well, if the ARM maintainers insist on baking their own thing every
>>>> time we'd use the M2P if it was there, I think I can't reasonably
>>>> block this patch. Otoh I'd prefer to not see the non-x86-specific
>>>> code move to x86/, so perhaps the whole patch wants
>>>> re-structuring using either a manifest definition in the ARM headers
>>>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't).
>>> Jan, I am afraid but I didn't get what you meant here:
>>> "manifest definition in the ARM headers"
>>
>> I think he meant to have either a define in the header mentioning the
>> absence/presence of M2P.
>
> Yes, at least in a way.
>
>> But my preference would be using the Kconfig here.
>
> Depends on the symbol used: If such a symbol solely _indicates_
> the presence, Kconfig would be better indeed. If, however, the
> symbol is e.g. a macro resolving to a per-arch implementation,
> with common code providing a default definition when the arch
> doesn't provide any, then the non-Kconfig variant may be
> preferable.
>
> Jan
>

Thank you for your comments.
I have already posted a common comment regarding several patches in
the current series
as they are interrelated (please see patch #6), but I duplicate here
only related to this patch part.

...
patch #8: As we always allocate the page table for hardware domain,
this patch should be reworked.
The use_iommu flag should be set for both archs in case of hardware
domain. Having d->need_iommu set at the early stage we won't skip
IOMMU mapping updates anymore. And as the result the existing in
iommu_hwdom_init() code that goes through the list of page and tries
to retrieve mapping could be just dropped
instead of moving it to the arch-specific part.
...

Does the described above make sense?

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Jan Beulich]

>>> On 18.05.17 at 19:41, <olekstysh@gmail.com> wrote:
> patch #6: As for the current patch, likely the "init" platform
> callback should be extended with
> extra "use_iommu" argument as well as the "iommu_domain_init" API. In
> that case we
> would just pass thought incoming flag to the IOMMU drivers followed by
> updating "need_iommu" domain flag.
> 
> Something like this:
> ...
> int iommu_domain_init(struct domain *d, bool use_iommu)
> {
>     struct domain_iommu *hd = dom_iommu(d);
>     int ret = 0;
> 
>     ret = arch_iommu_domain_init(d);
>     if ( ret )
>         return ret;
> 
>     if ( !iommu_enabled )
>         return 0;
> 
>     hd->platform_ops = iommu_get_ops();
>     ret = hd->platform_ops->init(d, use_iommu);
>     if ( ret )
>         return ret;
> 
>     d->need_iommu = !!use_iommu;
> 
>     return 0;
> }
> ...

The final shape of this primarily depends on ARM side needs.
However, you need to be careful to make sure the final setting
of d->need_iommu then is no different than it is today on at
least x86. I'm mentioning this in particular because of e.g.

    d->need_iommu = !!iommu_dom0_strict;

in iommu_hwdom_init().

Also as a minor remark note that in your new code the !! would
not be needed.

> patch #7: This patch should be just dropped.
> 
> patch #8: As we always allocate the page table for hardware domain,
> this patch should be reworked.
> The use_iommu flag should be set for both archs in case of hardware
> domain. Having d->need_iommu set at the early stage we won't skip
> IOMMU
> mapping updates anymore. And as the result the existing in
> iommu_hwdom_init() code that goes through the list of page and tries
> to retrieve mapping could be just dropped
> instead of moving it to the arch-specific part.

And again, careful here: There are three command line options
influencing which pages do actually get mapped, and in which way
(iommu=dom0-passthrough, iommu=dom0-strict, and VT-d's
iommu_inclusive_mapping). The behavior after your change must
not differ from current behavior regardless of which of these
options may be used.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 18.05.17 at 20:06, <olekstysh@gmail.com> wrote:
> On Thu, May 18, 2017 at 11:53 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote:
>>> On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote:
>>>> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> Well, if the ARM maintainers insist on baking their own thing every
>>>>> time we'd use the M2P if it was there, I think I can't reasonably
>>>>> block this patch. Otoh I'd prefer to not see the non-x86-specific
>>>>> code move to x86/, so perhaps the whole patch wants
>>>>> re-structuring using either a manifest definition in the ARM headers
>>>>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't).
>>>> Jan, I am afraid but I didn't get what you meant here:
>>>> "manifest definition in the ARM headers"
>>>
>>> I think he meant to have either a define in the header mentioning the
>>> absence/presence of M2P.
>>
>> Yes, at least in a way.
>>
>>> But my preference would be using the Kconfig here.
>>
>> Depends on the symbol used: If such a symbol solely _indicates_
>> the presence, Kconfig would be better indeed. If, however, the
>> symbol is e.g. a macro resolving to a per-arch implementation,
>> with common code providing a default definition when the arch
>> doesn't provide any, then the non-Kconfig variant may be
>> preferable.
> 
> Thank you for your comments.
> I have already posted a common comment regarding several patches in
> the current series
> as they are interrelated (please see patch #6), but I duplicate here
> only related to this patch part.
> 
> ...
> patch #8: As we always allocate the page table for hardware domain,
> this patch should be reworked.
> The use_iommu flag should be set for both archs in case of hardware
> domain. Having d->need_iommu set at the early stage we won't skip
> IOMMU mapping updates anymore. And as the result the existing in
> iommu_hwdom_init() code that goes through the list of page and tries
> to retrieve mapping could be just dropped
> instead of moving it to the arch-specific part.
> ...
> 
> Does the described above make sense?

As just said in the other reply - only if there weren't all these extra
overrides (one of which is even on by default, albeit we've been
discussing recently whether that's actually [still] appropriate).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Oleksandr Tyshchenko]

Hi, Jan

On Fri, May 19, 2017 at 9:30 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 18.05.17 at 19:41, <olekstysh@gmail.com> wrote:
>> patch #6: As for the current patch, likely the "init" platform
>> callback should be extended with
>> extra "use_iommu" argument as well as the "iommu_domain_init" API. In
>> that case we
>> would just pass thought incoming flag to the IOMMU drivers followed by
>> updating "need_iommu" domain flag.
>>
>> Something like this:
>> ...
>> int iommu_domain_init(struct domain *d, bool use_iommu)
>> {
>>     struct domain_iommu *hd = dom_iommu(d);
>>     int ret = 0;
>>
>>     ret = arch_iommu_domain_init(d);
>>     if ( ret )
>>         return ret;
>>
>>     if ( !iommu_enabled )
>>         return 0;
>>
>>     hd->platform_ops = iommu_get_ops();
>>     ret = hd->platform_ops->init(d, use_iommu);
>>     if ( ret )
>>         return ret;
>>
>>     d->need_iommu = !!use_iommu;
>>
>>     return 0;
>> }
>> ...
>
> The final shape of this primarily depends on ARM side needs.
> However, you need to be careful to make sure the final setting
> of d->need_iommu then is no different than it is today on at
> least x86. I'm mentioning this in particular because of e.g.
>
>     d->need_iommu = !!iommu_dom0_strict;
>
> in iommu_hwdom_init().
Yes, sure, I will take it into the account.

>
> Also as a minor remark note that in your new code the !! would
> not be needed.
>
>> patch #7: This patch should be just dropped.
>>
>> patch #8: As we always allocate the page table for hardware domain,
>> this patch should be reworked.
>> The use_iommu flag should be set for both archs in case of hardware
>> domain. Having d->need_iommu set at the early stage we won't skip
>> IOMMU
>> mapping updates anymore. And as the result the existing in
>> iommu_hwdom_init() code that goes through the list of page and tries
>> to retrieve mapping could be just dropped
>> instead of moving it to the arch-specific part.
>
> And again, careful here: There are three command line options
> influencing which pages do actually get mapped, and in which way
> (iommu=dom0-passthrough, iommu=dom0-strict, and VT-d's
> iommu_inclusive_mapping). The behavior after your change must
> not differ from current behavior regardless of which of these
> options may be used.
Yes, sure. This is my target not to brake things.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel