[RFC PATCH 0/9] "Non-shared" IOMMU support on ARM

[RFC PATCH 0/9] "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).
And this IOMMU can't share the page table with the CPU since it uses
stage-1 page table unlike the CPU that uses stage-2 therefore I name it
"Non-shared" IOMMU.

I have already raised disscusion [2] about some generic problems I had faced
during porting IPMMU-VMSA Linux driver to Xen. And on this basis I made
patches you can see it in this request. Only the first patch is not related to 
IOMMU. But, I decided to ship it with the current request since it is a generic change
which we will need in a moment.

The reason for this patch series to be RFC is that I still have some doubts about generic code I touched.
I hope that I haven't broken anything for x86, but confirmation is needed.

I didn't include IPMMU-VMSA driver in this request. Although, I am still in progress, I want to say
that passthrough use-cases (actually what this all are firstly needed for) work for me with some limitations.
I tested on Salvator-X board (R-Car H3) with the next devices that have DMA IPs
connected to IPMMU uTLBs (using current master branch):
1. AUDMA is assigned to dom0 (protected by IOMMU)
2. SDHC0 is assigned to dom1 (passthrough to domain)
3. SDHC3 is assigned to dom2 (passthrough to domain)

During porting IPMMU-VMSA driver to Xen I was trying to be as close as possible to Linux [1]. But,
it was a little bit difficult). It would be really nice to have some feedback and get your feeling regarding this driver.
I am also interested in if I took the right direction or there are some other ideas on doing the same.
So, is it a right direction to move on?

You can find IPMMU-VMSA driver here.
repo: https://github.com/otyshchenko1/xen.git branch: ipmmu_ml
or
https://github.com/otyshchenko1/xen/commits/ipmmu_ml
It is located on top of "Unshared" IOMMU patch series and consist of 6 patches.

Thank you.

[1] Question about porting IPMMU-VMSA Linux driver to XEN
https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg00992.html
[2] Unshared IOMMU issues 
https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg01781.html

Oleksandr Tyshchenko (9):
  xen/device-tree: Add dt_count_phandle_with_args helper
  iommu: Add ability to map/unmap the number of pages
  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: Pass additional 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: Add use_iommu flag to createdomain domctl

 tools/libxl/libxl_create.c          |  5 ++
 xen/arch/arm/domain.c               |  4 +-
 xen/arch/arm/p2m.c                  | 40 +++++++++++++++-
 xen/arch/x86/domain.c               |  4 +-
 xen/common/device_tree.c            |  7 +++
 xen/common/domctl.c                 |  5 +-
 xen/drivers/passthrough/arm/iommu.c | 12 ++++-
 xen/drivers/passthrough/arm/smmu.c  |  3 ++
 xen/drivers/passthrough/iommu.c     | 91 ++++++++++++++++++++-----------------
 xen/drivers/passthrough/x86/iommu.c | 36 +++++++++++++++
 xen/include/asm-arm/iommu.h         |  7 ++-
 xen/include/asm-arm/p2m.h           | 34 ++++++++++++++
 xen/include/public/domctl.h         |  3 ++
 xen/include/xen/device_tree.h       | 19 ++++++++
 xen/include/xen/iommu.h             | 20 ++++++--
 xen/include/xen/sched.h             |  3 ++
 16 files changed, 239 insertions(+), 54 deletions(-)

-- 
2.7.4


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

[RFC PATCH 1/9] 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>
---
 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

Re: [RFC PATCH 1/9] xen/device-tree: Add dt_count_phandle_with_args helper

[Julien Grall]

Hi Oleksandr,

On 03/15/2017 08:05 PM, Oleksandr Tyshchenko 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>

Regards,

-- 
Julien Grall

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

Re: [RFC PATCH 1/9] xen/device-tree: Add dt_count_phandle_with_args helper

[Oleksandr Tyshchenko]

On Thu, Mar 16, 2017 at 5:39 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,

Hi Julien

>
> On 03/15/2017 08:05 PM, Oleksandr Tyshchenko 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>

Thank you.

>
> Regards,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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

[RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Extend the IOMMU code with new APIs and platform callbacks.
These new map_pages/unmap_pages API do almost the same thing
as existing map_page/unmap_page ones except the formers can
handle the number of pages. So do new platform callbacks.

Currently, this patch requires to modify neither
existing IOMMU drivers nor P2M code.
But, the patch might be rewritten to replace existing
single-page stuff with the multi-page one followed by modifications
of all related parts.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 xen/drivers/passthrough/iommu.c | 50 ++++++++++++++++++++++++++++++++---------
 xen/include/xen/iommu.h         | 16 ++++++++++---
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5e81813..115698f 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -249,22 +249,37 @@ 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 long page_count, unsigned int flags)
 {
     const struct domain_iommu *hd = dom_iommu(d);
-    int rc;
+    int rc = 0;
+    unsigned long i;
 
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
+    if ( hd->platform_ops->map_pages )
+        rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags);
+    else
+    {
+        for ( i = 0; i < page_count; i++ )
+        {
+            rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags);
+            if ( unlikely(rc) )
+            {
+                /* TODO Do we need to unmap if map failed? */
+                break;
+            }
+        }
+    }
+
     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 page count %lu failed: %d\n",
+                   d->domain_id, gfn, mfn, page_count, rc);
 
         if ( !is_hardware_domain(d) )
             domain_crash(d);
@@ -273,21 +288,34 @@ 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 long page_count)
 {
     const struct domain_iommu *hd = dom_iommu(d);
-    int rc;
+    int ret, rc = 0;
+    unsigned long i;
 
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    rc = hd->platform_ops->unmap_page(d, gfn);
+    if ( hd->platform_ops->unmap_pages )
+        rc = hd->platform_ops->unmap_pages(d, gfn, page_count);
+    else
+    {
+        for ( i = 0; i < page_count; i++ )
+        {
+            ret = hd->platform_ops->unmap_page(d, gfn + i);
+            if ( likely(!rc) )
+                rc = ret;
+        }
+    }
+
     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 page count %lu failed: %d\n",
+                   d->domain_id, gfn, page_count, rc);
 
         if ( !is_hardware_domain(d) )
             domain_crash(d);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5803e3f..0446ed3 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d);
 #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 long page_count,
+                                 unsigned int flags);
+int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn,
+                                   unsigned long page_count);
+
+#define iommu_map_page(d,gfn,mfn,flags) (iommu_map_pages(d,gfn,mfn,1,flags))
+#define iommu_unmap_page(d,gfn)         (iommu_unmap_pages(d,gfn,1))
 
 enum iommu_feature
 {
@@ -170,7 +175,12 @@ struct iommu_ops {
     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 (*map_pages)(struct domain *d, unsigned long gfn,
+                                  unsigned long mfn, unsigned long page_count,
+                                  unsigned int flags);
     int __must_check (*unmap_page)(struct domain *d, unsigned long gfn);
+    int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
+                                    unsigned long page_count);
     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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Jan Beulich]

>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Extend the IOMMU code with new APIs and platform callbacks.
> These new map_pages/unmap_pages API do almost the same thing
> as existing map_page/unmap_page ones except the formers can
> handle the number of pages. So do new platform callbacks.
> 
> Currently, this patch requires to modify neither
> existing IOMMU drivers nor P2M code.
> But, the patch might be rewritten to replace existing
> single-page stuff with the multi-page one followed by modifications
> of all related parts.

Yes please. However, unless you strictly need a page count, please
instead use an "order" parameter. Doing that has been on my todo
list for quite a while.

Jan


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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko]

Hi Jan

On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Extend the IOMMU code with new APIs and platform callbacks.
>> These new map_pages/unmap_pages API do almost the same thing
>> as existing map_page/unmap_page ones except the formers can
>> handle the number of pages. So do new platform callbacks.
>>
>> Currently, this patch requires to modify neither
>> existing IOMMU drivers nor P2M code.
>> But, the patch might be rewritten to replace existing
>> single-page stuff with the multi-page one followed by modifications
>> of all related parts.
>
> Yes please. However, unless you strictly need a page count, please
> instead use an "order" parameter. Doing that has been on my todo
> list for quite a while.
>
> Jan
>

The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well
as new map_pages/unmap_pages platform callbacks.

So, we just need to replace single-page APIs with multi-page ones, but
leave both "new" (map_pages/unmap_pages) and "old"
(map_page/unmap_page) platform callbacks.
This way doesn't require to modify existing IOMMU drivers right now,
just P2M code. Or we need to replace platform callbacks too?

Don't mind to use order instead of page count.

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Jan Beulich]

>>> On 22.03.17 at 19:01, <olekstysh@gmail.com> wrote:
> Hi Jan
> 
> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> Extend the IOMMU code with new APIs and platform callbacks.
>>> These new map_pages/unmap_pages API do almost the same thing
>>> as existing map_page/unmap_page ones except the formers can
>>> handle the number of pages. So do new platform callbacks.
>>>
>>> Currently, this patch requires to modify neither
>>> existing IOMMU drivers nor P2M code.
>>> But, the patch might be rewritten to replace existing
>>> single-page stuff with the multi-page one followed by modifications
>>> of all related parts.
>>
>> Yes please. However, unless you strictly need a page count, please
>> instead use an "order" parameter. Doing that has been on my todo
>> list for quite a while.
> 
> The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well
> as new map_pages/unmap_pages platform callbacks.
> 
> So, we just need to replace single-page APIs with multi-page ones, but
> leave both "new" (map_pages/unmap_pages) and "old"
> (map_page/unmap_page) platform callbacks.
> This way doesn't require to modify existing IOMMU drivers right now,
> just P2M code. Or we need to replace platform callbacks too?

That was the "yes please" part of my answer: I don't see why we
would want two API / callback flavors, with one being a strict
superset of the other.

Jan


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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko]

On Thu, Mar 23, 2017 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.03.17 at 19:01, <olekstysh@gmail.com> wrote:
>> Hi Jan
>>
>> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> Extend the IOMMU code with new APIs and platform callbacks.
>>>> These new map_pages/unmap_pages API do almost the same thing
>>>> as existing map_page/unmap_page ones except the formers can
>>>> handle the number of pages. So do new platform callbacks.
>>>>
>>>> Currently, this patch requires to modify neither
>>>> existing IOMMU drivers nor P2M code.
>>>> But, the patch might be rewritten to replace existing
>>>> single-page stuff with the multi-page one followed by modifications
>>>> of all related parts.
>>>
>>> Yes please. However, unless you strictly need a page count, please
>>> instead use an "order" parameter. Doing that has been on my todo
>>> list for quite a while.
>>
>> The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well
>> as new map_pages/unmap_pages platform callbacks.
>>
>> So, we just need to replace single-page APIs with multi-page ones, but
>> leave both "new" (map_pages/unmap_pages) and "old"
>> (map_page/unmap_page) platform callbacks.
>> This way doesn't require to modify existing IOMMU drivers right now,
>> just P2M code. Or we need to replace platform callbacks too?
>
> That was the "yes please" part of my answer: I don't see why we
> would want two API / callback flavors, with one being a strict
> superset of the other.

Agree. I'll be back if I have questions that need to be clarified.

>
> Jan
>


-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko]

[-- Attachment #1.1: Type: text/plain, Size: 4560 bytes --]

Hi, Jan.

There are the questions I would like to clarify.

On Thu, Mar 23, 2017 at 2:47 PM, Oleksandr Tyshchenko <olekstysh@gmail.com>
wrote:
> On Thu, Mar 23, 2017 at 11:07 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 22.03.17 at 19:01, <olekstysh@gmail.com> wrote:
>>> Hi Jan
>>>
>>> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>
>>>>> Extend the IOMMU code with new APIs and platform callbacks.
>>>>> These new map_pages/unmap_pages API do almost the same thing
>>>>> as existing map_page/unmap_page ones except the formers can
>>>>> handle the number of pages. So do new platform callbacks.
>>>>>
>>>>> Currently, this patch requires to modify neither
>>>>> existing IOMMU drivers nor P2M code.
>>>>> But, the patch might be rewritten to replace existing
>>>>> single-page stuff with the multi-page one followed by modifications
>>>>> of all related parts.
>>>>
>>>> Yes please. However, unless you strictly need a page count, please
>>>> instead use an "order" parameter. Doing that has been on my todo
>>>> list for quite a while.
>>>
>>> The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well
>>> as new map_pages/unmap_pages platform callbacks.
>>>
>>> So, we just need to replace single-page APIs with multi-page ones, but
>>> leave both "new" (map_pages/unmap_pages) and "old"
>>> (map_page/unmap_page) platform callbacks.
>>> This way doesn't require to modify existing IOMMU drivers right now,
>>> just P2M code. Or we need to replace platform callbacks too?
>>
>> That was the "yes please" part of my answer: I don't see why we
>> would want two API / callback flavors, with one being a strict
>> superset of the other.
>
> Agree. I'll be back if I have questions that need to be clarified.

Now I am trying to replace single-page stuff with the multi-page one.
Currently, with the single-page API, if map fails we always try to unmap
already mapped pages.

This is an example of generic code I am speaking about:
...
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;
    }
}
...

After modification the generic code will look like:
...
rc = iommu_map_pages(d, gfn, mfn_x(mfn), 1 << order, iommu_flags);
...
In case of an error we won't know how many pages have been already mapped
and
as the result we won't be able to unmap them in order to restore the
initial state.
Therefore I will move the rollback logic to the IOMMU drivers code. So, the
IOMMU drivers code
will take care of rollback mapping if something goes wrong. Am I right?

If all described above make sense, then there are several places I am
trying to optimize, but I am not familiar with)

1. xen/arch/x86/x86_64/mm.c:1443:
...
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)
)
            break;
    if ( i != epfn )
    {
        while (i-- > old_max) // <--- [1]
            /* If statement to satisfy __must_check. */
            if ( iommu_unmap_page(hardware_domain, i) )
                continue;

        goto destroy_m2p;
    }
}
...

[1] Shouldn't we unmap already mapped pages only?  I mean to use "while
(i-- > spfn)" instead.
And if the use of "old_max" here has a special meaning, I think, that this
place of code won't be optimized
by passing the whole memory block (epfn - spfn) to the IOMMU. Just keep it
as is (handle one page at time).

2. xen/drivers/passthrough/vtd/x86/vtd.c:143:
...
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);

    if ( !rc )
       rc = ret;
}
...

Here we don't try to rollback mapping at all. Was the idea to do so? Or was
this place simply missed?

P.S. As for using "order" parameter instead of page_count.
There are *few* places where "order" doesn't fit. I can introduce something
like this:

#define __iommu_map_pages(d,gfn,mfn,order,flags)
(iommu_map_pages(d,gfn,mfn,1U << (order),flags))

>
>>
>> Jan
>>
>
>
> --
> Regards,
>
> Oleksandr Tyshchenko

-- 
Regards,

Oleksandr Tyshchenko

[-- Attachment #1.2: Type: text/html, Size: 6304 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Jan Beulich]

>>> On 27.04.17 at 18:56, <olekstysh@gmail.com> wrote:
> Now I am trying to replace single-page stuff with the multi-page one.
> Currently, with the single-page API, if map fails we always try to unmap
> already mapped pages.
> 
> This is an example of generic code I am speaking about:
> ...
> 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;
>     }
> }
> ...
> 
> After modification the generic code will look like:
> ...
> rc = iommu_map_pages(d, gfn, mfn_x(mfn), 1 << order, iommu_flags);
> ...
> In case of an error we won't know how many pages have been already mapped
> and
> as the result we won't be able to unmap them in order to restore the
> initial state.
> Therefore I will move the rollback logic to the IOMMU drivers code. So, the
> IOMMU drivers code
> will take care of rollback mapping if something goes wrong. Am I right?

Yes, it should be iommu_map_pages() (or its descendants) to clean
up after itself upon error.

> If all described above make sense, then there are several places I am
> trying to optimize, but I am not familiar with)
> 
> 1. xen/arch/x86/x86_64/mm.c:1443:
> ...
> 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)
> )
>             break;
>     if ( i != epfn )
>     {
>         while (i-- > old_max) // <--- [1]
>             /* If statement to satisfy __must_check. */
>             if ( iommu_unmap_page(hardware_domain, i) )
>                 continue;
> 
>         goto destroy_m2p;
>     }
> }
> ...
> 
> [1] Shouldn't we unmap already mapped pages only?  I mean to use "while
> (i-- > spfn)" instead.

Both should have the same effect, considering what old_max
represents, at least as long as there's no MMIO in between. But
yes, generally it would be more logical to unmap using spfn.

> And if the use of "old_max" here has a special meaning, I think, that this
> place of code won't be optimized
> by passing the whole memory block (epfn - spfn) to the IOMMU. Just keep it
> as is (handle one page at time).

Right, that would have been my general advice: If in doubt, keep
the code as is rather than trying to optimize it.

> 2. xen/drivers/passthrough/vtd/x86/vtd.c:143:
> ...
> 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);
> 
>     if ( !rc )
>        rc = ret;
> }
> ...
> 
> Here we don't try to rollback mapping at all. Was the idea to do so? Or was
> this place simply missed?

Did you consider both the context this is in (establishing hwdom
mappings) and the code's history? Both would tell you that yes,
this is a best effort mapping attempt deliberately continuing
despite errors (but reporting the first one). This behavior will
need to be retained. Plus I'm sure you've noticed that this
effectively is a single page mapping only anyway (due to
PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this
"clever" code was used.

And as a side note - the way you quote code (by line number and
without naming the function it's in) makes it somewhat more
complicated to answer your questions. In both cases, had I known
the function the code is in, I wouldn't have had a need at all to go
look up the context.

> P.S. As for using "order" parameter instead of page_count.
> There are *few* places where "order" doesn't fit.

Well, I'd prefer the "few places" to then break up their requests to
fit the "order" parameter. Especially for the purpose of possibly
using large pages, an order input is quite a bit more sensible.

> I can introduce something like this:
> 
> #define __iommu_map_pages(d,gfn,mfn,order,flags)
> (iommu_map_pages(d,gfn,mfn,1U << (order),flags))

I'd prefer if you didn't. In no case should this be an identifier
starting with an underscore.

Jan

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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko]

Hi, Jan.

Thank you for your reply.

On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 27.04.17 at 18:56, <olekstysh@gmail.com> wrote:
> > Now I am trying to replace single-page stuff with the multi-page one.
> > Currently, with the single-page API, if map fails we always try to unmap
> > already mapped pages.
> >
> > This is an example of generic code I am speaking about:
> > ...
> > 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;
> >     }
> > }
> > ...
> >
> > After modification the generic code will look like:
> > ...
> > rc = iommu_map_pages(d, gfn, mfn_x(mfn), 1 << order, iommu_flags);
> > ...
> > In case of an error we won't know how many pages have been already mapped
> > and
> > as the result we won't be able to unmap them in order to restore the
> > initial state.
> > Therefore I will move the rollback logic to the IOMMU drivers code. So, the
> > IOMMU drivers code
> > will take care of rollback mapping if something goes wrong. Am I right?
>
> Yes, it should be iommu_map_pages() (or its descendants) to clean
> up after itself upon error.
>
> > If all described above make sense, then there are several places I am
> > trying to optimize, but I am not familiar with)
> >
> > 1. xen/arch/x86/x86_64/mm.c:1443:
> > ...
> > 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)
> > )
> >             break;
> >     if ( i != epfn )
> >     {
> >         while (i-- > old_max) // <--- [1]
> >             /* If statement to satisfy __must_check. */
> >             if ( iommu_unmap_page(hardware_domain, i) )
> >                 continue;
> >
> >         goto destroy_m2p;
> >     }
> > }
> > ...
> >
> > [1] Shouldn't we unmap already mapped pages only?  I mean to use "while
> > (i-- > spfn)" instead.
>
> Both should have the same effect, considering what old_max
> represents, at least as long as there's no MMIO in between. But
> yes, generally it would be more logical to unmap using spfn.
>
> > And if the use of "old_max" here has a special meaning, I think, that this
> > place of code won't be optimized
> > by passing the whole memory block (epfn - spfn) to the IOMMU. Just keep it
> > as is (handle one page at time).
>
> Right, that would have been my general advice: If in doubt, keep
> the code as is rather than trying to optimize it.
OK

>
>
> > 2. xen/drivers/passthrough/vtd/x86/vtd.c:143:
> > ...
> > 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);
> >
> >     if ( !rc )
> >        rc = ret;
> > }
> > ...
> >
> > Here we don't try to rollback mapping at all. Was the idea to do so? Or was
> > this place simply missed?
>
> Did you consider both the context this is in (establishing hwdom
> mappings) and the code's history? Both would tell you that yes,
> this is a best effort mapping attempt deliberately continuing
> despite errors (but reporting the first one). This behavior will
> need to be retained. Plus I'm sure you've noticed that this
> effectively is a single page mapping only anyway (due to
> PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this
> "clever" code was used.
So, if I understand what your meant I don't even need to try to
optimize this code.
Despite the fact that this code would become much more simple:
...
rc = iommu_map_pages(d, pfn, pfn, 1,
                  IOMMUF_readable|IOMMUF_writable);
...
Right?

>
> And as a side note - the way you quote code (by line number and
> without naming the function it's in) makes it somewhat more
> complicated to answer your questions. In both cases, had I known
> the function the code is in, I wouldn't have had a need at all to go
> look up the context.
Sorry for that. Next time I will provide more detailed snippet.

>
> > P.S. As for using "order" parameter instead of page_count.
> > There are *few* places where "order" doesn't fit.
>
> Well, I'd prefer the "few places" to then break up their requests to
> fit the "order" parameter. Especially for the purpose of possibly
> using large pages, an order input is quite a bit more sensible.
OK

>
> > I can introduce something like this:
> >
> > #define __iommu_map_pages(d,gfn,mfn,order,flags)
> > (iommu_map_pages(d,gfn,mfn,1U << (order),flags))
>
> I'd prefer if you didn't. In no case should this be an identifier
> starting with an underscore.
I got it. I proposed because I had seen analogy with
__map_domain_page/map_domain_page.

>
> Jan

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Jan Beulich]

>>> On 28.04.17 at 12:16, <olekstysh@gmail.com> wrote:
> On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 27.04.17 at 18:56, <olekstysh@gmail.com> wrote:
>> > 2. xen/drivers/passthrough/vtd/x86/vtd.c:143:
>> > ...
>> > 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);
>> >
>> >     if ( !rc )
>> >        rc = ret;
>> > }
>> > ...
>> >
>> > Here we don't try to rollback mapping at all. Was the idea to do so? Or was
>> > this place simply missed?
>>
>> Did you consider both the context this is in (establishing hwdom
>> mappings) and the code's history? Both would tell you that yes,
>> this is a best effort mapping attempt deliberately continuing
>> despite errors (but reporting the first one). This behavior will
>> need to be retained. Plus I'm sure you've noticed that this
>> effectively is a single page mapping only anyway (due to
>> PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this
>> "clever" code was used.
> So, if I understand what your meant I don't even need to try to
> optimize this code.
> Despite the fact that this code would become much more simple:
> ...
> rc = iommu_map_pages(d, pfn, pfn, 1,
>                   IOMMUF_readable|IOMMUF_writable);
> ...
> Right?

Well, the actual simplification is entirely independent of you patch;
what you'd add is just the extra order argument (which ought to
be zero, btw). I am, however, of the opinion that the simplification
would be good to do, but independent of your patch, and only
unless the VT-d maintainers actually think there is a reason for this
"cleverness".

>> > I can introduce something like this:
>> >
>> > #define __iommu_map_pages(d,gfn,mfn,order,flags)
>> > (iommu_map_pages(d,gfn,mfn,1U << (order),flags))
>>
>> I'd prefer if you didn't. In no case should this be an identifier
>> starting with an underscore.
> I got it. I proposed because I had seen analogy with
> __map_domain_page/map_domain_page.

Well, there are quite a few things in long standing code which
we wouldn't allow in new instances of anymore, one of which
being the non-standard-conforming use of leading underscores.
Eventually old code would be nice to be cleaned up too, but
that's tedious and time consuming, and most if not all of us
have better uses for their time. So commonly we do such
cleanup only when respective code needs touching anyway.

Jan


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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko]

On Fri, Apr 28, 2017 at 1:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 28.04.17 at 12:16, <olekstysh@gmail.com> wrote:
>> On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> >>> On 27.04.17 at 18:56, <olekstysh@gmail.com> wrote:
>>> > 2. xen/drivers/passthrough/vtd/x86/vtd.c:143:
>>> > ...
>>> > 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);
>>> >
>>> >     if ( !rc )
>>> >        rc = ret;
>>> > }
>>> > ...
>>> >
>>> > Here we don't try to rollback mapping at all. Was the idea to do so? Or was
>>> > this place simply missed?
>>>
>>> Did you consider both the context this is in (establishing hwdom
>>> mappings) and the code's history? Both would tell you that yes,
>>> this is a best effort mapping attempt deliberately continuing
>>> despite errors (but reporting the first one). This behavior will
>>> need to be retained. Plus I'm sure you've noticed that this
>>> effectively is a single page mapping only anyway (due to
>>> PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this
>>> "clever" code was used.
>> So, if I understand what your meant I don't even need to try to
>> optimize this code.
>> Despite the fact that this code would become much more simple:
>> ...
>> rc = iommu_map_pages(d, pfn, pfn, 1,
>>                   IOMMUF_readable|IOMMUF_writable);
>> ...
>> Right?
>
> Well, the actual simplification is entirely independent of you patch;
> what you'd add is just the extra order argument (which ought to
> be zero, btw). I am, however, of the opinion that the simplification
> would be good to do, but independent of your patch, and only
> unless the VT-d maintainers actually think there is a reason for this
> "cleverness".
Clear enough.

>
>>> > I can introduce something like this:
>>> >
>>> > #define __iommu_map_pages(d,gfn,mfn,order,flags)
>>> > (iommu_map_pages(d,gfn,mfn,1U << (order),flags))
>>>
>>> I'd prefer if you didn't. In no case should this be an identifier
>>> starting with an underscore.
>> I got it. I proposed because I had seen analogy with
>> __map_domain_page/map_domain_page.
>
> Well, there are quite a few things in long standing code which
> we wouldn't allow in new instances of anymore, one of which
> being the non-standard-conforming use of leading underscores.
> Eventually old code would be nice to be cleaned up too, but
> that's tedious and time consuming, and most if not all of us
> have better uses for their time. So commonly we do such
> cleanup only when respective code needs touching anyway.
Clear enough.

>
> Jan
>

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Julien Grall]

Hi Oleksandr,

On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> Extend the IOMMU code with new APIs and platform callbacks.
> These new map_pages/unmap_pages API do almost the same thing
> as existing map_page/unmap_page ones except the formers can
> handle the number of pages. So do new platform callbacks.
>
> Currently, this patch requires to modify neither
> existing IOMMU drivers nor P2M code.
> But, the patch might be rewritten to replace existing
> single-page stuff with the multi-page one followed by modifications
> of all related parts.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  xen/drivers/passthrough/iommu.c | 50 ++++++++++++++++++++++++++++++++---------
>  xen/include/xen/iommu.h         | 16 ++++++++++---
>  2 files changed, 52 insertions(+), 14 deletions(-)
>
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 5e81813..115698f 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -249,22 +249,37 @@ 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 long page_count, unsigned int flags)

It would be nice if you can use mfn_t and gfn_t instead of unsigned long 
for any new functions. They are typesafe and avoid confusion between gfn 
and mfn.

>  {
>      const struct domain_iommu *hd = dom_iommu(d);
> -    int rc;
> +    int rc = 0;
> +    unsigned long i;
>
>      if ( !iommu_enabled || !hd->platform_ops )
>          return 0;
>
> -    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
> +    if ( hd->platform_ops->map_pages )
> +        rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags);
> +    else
> +    {
> +        for ( i = 0; i < page_count; i++ )
> +        {
> +            rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags);
> +            if ( unlikely(rc) )
> +            {
> +                /* TODO Do we need to unmap if map failed? */
> +                break;
> +            }
> +        }
> +    }
> +
>      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 page count %lu failed: %d\n",
> +                   d->domain_id, gfn, mfn, page_count, rc);
>
>          if ( !is_hardware_domain(d) )
>              domain_crash(d);
> @@ -273,21 +288,34 @@ 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 long page_count)
>  {
>      const struct domain_iommu *hd = dom_iommu(d);
> -    int rc;
> +    int ret, rc = 0;
> +    unsigned long i;
>
>      if ( !iommu_enabled || !hd->platform_ops )
>          return 0;
>
> -    rc = hd->platform_ops->unmap_page(d, gfn);
> +    if ( hd->platform_ops->unmap_pages )
> +        rc = hd->platform_ops->unmap_pages(d, gfn, page_count);
> +    else
> +    {
> +        for ( i = 0; i < page_count; i++ )
> +        {
> +            ret = hd->platform_ops->unmap_page(d, gfn + i);
> +            if ( likely(!rc) )
> +                rc = ret;
> +        }
> +    }
> +
>      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 page count %lu failed: %d\n",
> +                   d->domain_id, gfn, page_count, rc);
>
>          if ( !is_hardware_domain(d) )
>              domain_crash(d);
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 5803e3f..0446ed3 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d);
>  #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 long page_count,
> +                                 unsigned int flags);
> +int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn,
> +                                   unsigned long page_count);
> +
> +#define iommu_map_page(d,gfn,mfn,flags) (iommu_map_pages(d,gfn,mfn,1,flags))
> +#define iommu_unmap_page(d,gfn)         (iommu_unmap_pages(d,gfn,1))
>
>  enum iommu_feature
>  {
> @@ -170,7 +175,12 @@ struct iommu_ops {
>      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 (*map_pages)(struct domain *d, unsigned long gfn,
> +                                  unsigned long mfn, unsigned long page_count,
> +                                  unsigned int flags);
>      int __must_check (*unmap_page)(struct domain *d, unsigned long gfn);
> +    int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
> +                                    unsigned long page_count);
>      void (*free_page_table)(struct page_info *);
>  #ifdef CONFIG_X86
>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
>

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko]

On Wed, Apr 19, 2017 at 8:31 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi, Julien

>
>
> On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Extend the IOMMU code with new APIs and platform callbacks.
>> These new map_pages/unmap_pages API do almost the same thing
>> as existing map_page/unmap_page ones except the formers can
>> handle the number of pages. So do new platform callbacks.
>>
>> Currently, this patch requires to modify neither
>> existing IOMMU drivers nor P2M code.
>> But, the patch might be rewritten to replace existing
>> single-page stuff with the multi-page one followed by modifications
>> of all related parts.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  xen/drivers/passthrough/iommu.c | 50
>> ++++++++++++++++++++++++++++++++---------
>>  xen/include/xen/iommu.h         | 16 ++++++++++---
>>  2 files changed, 52 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/iommu.c
>> b/xen/drivers/passthrough/iommu.c
>> index 5e81813..115698f 100644
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -249,22 +249,37 @@ 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 long page_count, unsigned int flags)
>
>
> It would be nice if you can use mfn_t and gfn_t instead of unsigned long for
> any new functions. They are typesafe and avoid confusion between gfn and
> mfn.
ok

>
>
>>  {
>>      const struct domain_iommu *hd = dom_iommu(d);
>> -    int rc;
>> +    int rc = 0;
>> +    unsigned long i;
>>
>>      if ( !iommu_enabled || !hd->platform_ops )
>>          return 0;
>>
>> -    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
>> +    if ( hd->platform_ops->map_pages )
>> +        rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags);
>> +    else
>> +    {
>> +        for ( i = 0; i < page_count; i++ )
>> +        {
>> +            rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags);
>> +            if ( unlikely(rc) )
>> +            {
>> +                /* TODO Do we need to unmap if map failed? */
>> +                break;
>> +            }
>> +        }
>> +    }
>> +
>>      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 page count
>> %lu failed: %d\n",
>> +                   d->domain_id, gfn, mfn, page_count, rc);
>>
>>          if ( !is_hardware_domain(d) )
>>              domain_crash(d);
>> @@ -273,21 +288,34 @@ 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 long page_count)
>>  {
>>      const struct domain_iommu *hd = dom_iommu(d);
>> -    int rc;
>> +    int ret, rc = 0;
>> +    unsigned long i;
>>
>>      if ( !iommu_enabled || !hd->platform_ops )
>>          return 0;
>>
>> -    rc = hd->platform_ops->unmap_page(d, gfn);
>> +    if ( hd->platform_ops->unmap_pages )
>> +        rc = hd->platform_ops->unmap_pages(d, gfn, page_count);
>> +    else
>> +    {
>> +        for ( i = 0; i < page_count; i++ )
>> +        {
>> +            ret = hd->platform_ops->unmap_page(d, gfn + i);
>> +            if ( likely(!rc) )
>> +                rc = ret;
>> +        }
>> +    }
>> +
>>      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 page count %lu failed:
>> %d\n",
>> +                   d->domain_id, gfn, page_count, rc);
>>
>>          if ( !is_hardware_domain(d) )
>>              domain_crash(d);
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 5803e3f..0446ed3 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d);
>>  #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 long
>> page_count,
>> +                                 unsigned int flags);
>> +int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn,
>> +                                   unsigned long page_count);
>> +
>> +#define iommu_map_page(d,gfn,mfn,flags)
>> (iommu_map_pages(d,gfn,mfn,1,flags))
>> +#define iommu_unmap_page(d,gfn)         (iommu_unmap_pages(d,gfn,1))
>>
>>  enum iommu_feature
>>  {
>> @@ -170,7 +175,12 @@ struct iommu_ops {
>>      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 (*map_pages)(struct domain *d, unsigned long gfn,
>> +                                  unsigned long mfn, unsigned long
>> page_count,
>> +                                  unsigned int flags);
>>      int __must_check (*unmap_page)(struct domain *d, unsigned long gfn);
>> +    int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
>> +                                    unsigned long page_count);
>>      void (*free_page_table)(struct page_info *);
>>  #ifdef CONFIG_X86
>>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg,
>> unsigned int value);
>>
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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

[RFC PATCH 3/9] 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>
---
 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 0899523..4a93ba8 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>
@@ -354,6 +355,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

Re: [RFC PATCH 3/9] xen/arm: p2m: Add helper to convert p2m type to IOMMU flags

[Julien Grall]

Hi Oleksandr,

On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
> 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>

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 3/9] xen/arm: p2m: Add helper to convert p2m type to IOMMU flags

[Oleksandr Tyshchenko]

On Wed, Apr 19, 2017 at 8:28 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi, Julien

>
> On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
>>
>> 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>
Thank you

>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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

[RFC PATCH 4/9] 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>
---
 xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 1fc6ca3..84d3a09 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
     if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
         p2m_free_entry(p2m, orig_pte, level);
 
-    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) )
+    if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) &&
+         (p2m_valid(orig_pte) || p2m_valid(*entry)) )
         rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
     else
         rc = 0;
@@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m,
                   p2m_type_t t,
                   p2m_access_t a)
 {
+    unsigned long orig_nr = nr;
+    gfn_t orig_sgfn = sgfn;
+    mfn_t orig_smfn = smfn;
     int rc = 0;
 
     while ( nr )
@@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m,
         nr -= (1 << order);
     }
 
+    if ( likely(!rc) )
+    {
+        /*
+         * It's time to update IOMMU mapping if the latter doesn't
+         * share page table with the CPU. Pass the whole memory block to let
+         * the IOMMU code decide what to do with it.
+         * The reason to update IOMMU mapping outside "while loop" is that
+         * the IOMMU might not support the pages/superpages the CPU can deal
+         * with (4K, 2M, 1G) and as result this will lead to non-optimal
+         * mapping.
+         * Also we assume that the IOMMU mapping should be updated only
+         * if CPU mapping passed successfully.
+         */
+        if ( need_iommu(p2m->domain) && !iommu_use_hap_pt(p2m->domain) )
+        {
+            if ( !mfn_eq(orig_smfn, INVALID_MFN) )
+            {
+                unsigned int flags = p2m_get_iommu_flags(t);
+
+                rc = iommu_map_pages(p2m->domain,
+                                     gfn_x(orig_sgfn),
+                                     mfn_x(orig_smfn),
+                                     orig_nr,
+                                     flags);
+            }
+            else
+            {
+                rc = iommu_unmap_pages(p2m->domain,
+                                       gfn_x(orig_sgfn),
+                                       orig_nr);
+            }
+        }
+    }
+
     return rc;
 }
 
-- 
2.7.4


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

Re: [RFC PATCH 4/9] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Julien Grall]

Hi Oleksandr,

On 15/03/17 20:05, 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().

Can you explain in the commit message why you do this change in 
p2m_set_entry and not __p2m_set_entry?

>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 1fc6ca3..84d3a09 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>      if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
>          p2m_free_entry(p2m, orig_pte, level);
>
> -    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) || p2m_valid(*entry)) )
> +    if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) &&
> +         (p2m_valid(orig_pte) || p2m_valid(*entry)) )
>          rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
>      else
>          rc = 0;
> @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m,
>                    p2m_type_t t,
>                    p2m_access_t a)
>  {
> +    unsigned long orig_nr = nr;
> +    gfn_t orig_sgfn = sgfn;
> +    mfn_t orig_smfn = smfn;
>      int rc = 0;
>
>      while ( nr )
> @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m,
>          nr -= (1 << order);
>      }
>
> +    if ( likely(!rc) )

I would do

if ( unlikely(rc) )
   return rc;

/* IOMMU map/unmap ... */

This would remove one indentation layer of if.

> +    {
> +        /*
> +         * It's time to update IOMMU mapping if the latter doesn't
> +         * share page table with the CPU. Pass the whole memory block to let
> +         * the IOMMU code decide what to do with it.
> +         * The reason to update IOMMU mapping outside "while loop" is that
> +         * the IOMMU might not support the pages/superpages the CPU can deal
> +         * with (4K, 2M, 1G) and as result this will lead to non-optimal
> +         * mapping.

My worry with this solution is someone may decide to use __p2m_set_entry 
(see relinquish) directly because he knows the correct order. So the 
IOMMU would be correctly handled when page table are shared and not when 
they are "unshared".

I would probably extend __p2m_get_entry with a new parameter indication 
whether we want to map the page in the IOMMU directly or not.

Also, do you expect the IOMMU to support a page granularity bigger than 
the one supported by Xen? If not, we should probably have a check 
somewhere, to prevent potential security issue as we would map more than 
expected.

> +         * Also we assume that the IOMMU mapping should be updated only
> +         * if CPU mapping passed successfully.
> +         */
> +        if ( need_iommu(p2m->domain) && !iommu_use_hap_pt(p2m->domain) )
> +        {
> +            if ( !mfn_eq(orig_smfn, INVALID_MFN) )
> +            {
> +                unsigned int flags = p2m_get_iommu_flags(t);
> +
> +                rc = iommu_map_pages(p2m->domain,
> +                                     gfn_x(orig_sgfn),
> +                                     mfn_x(orig_smfn),
> +                                     orig_nr,
> +                                     flags);
> +            }
> +            else
> +            {
> +                rc = iommu_unmap_pages(p2m->domain,
> +                                       gfn_x(orig_sgfn),
> +                                       orig_nr);
> +            }
> +        }
> +    }
> +
>      return rc;
>  }
>
>

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 4/9] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Oleksandr Tyshchenko]

On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi, Julien

>
> On 15/03/17 20:05, 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().
>
>
> Can you explain in the commit message why you do this change in
> p2m_set_entry and not __p2m_set_entry?
ok

>
>
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 1fc6ca3..84d3a09 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>>      if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
>>          p2m_free_entry(p2m, orig_pte, level);
>>
>> -    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) ||
>> p2m_valid(*entry)) )
>> +    if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) &&
>> +         (p2m_valid(orig_pte) || p2m_valid(*entry)) )
>>          rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL <<
>> page_order);
>>      else
>>          rc = 0;
>> @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m,
>>                    p2m_type_t t,
>>                    p2m_access_t a)
>>  {
>> +    unsigned long orig_nr = nr;
>> +    gfn_t orig_sgfn = sgfn;
>> +    mfn_t orig_smfn = smfn;
>>      int rc = 0;
>>
>>      while ( nr )
>> @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m,
>>          nr -= (1 << order);
>>      }
>>
>> +    if ( likely(!rc) )
>
>
> I would do
>
> if ( unlikely(rc) )
>   return rc;
>
> /* IOMMU map/unmap ... */
>
> This would remove one indentation layer of if.
Agree.

>
>> +    {
>> +        /*
>> +         * It's time to update IOMMU mapping if the latter doesn't
>> +         * share page table with the CPU. Pass the whole memory block to
>> let
>> +         * the IOMMU code decide what to do with it.
>> +         * The reason to update IOMMU mapping outside "while loop" is
>> that
>> +         * the IOMMU might not support the pages/superpages the CPU can
>> deal
>> +         * with (4K, 2M, 1G) and as result this will lead to non-optimal
>> +         * mapping.
>
>
> My worry with this solution is someone may decide to use __p2m_set_entry
> (see relinquish) directly because he knows the correct order. So the IOMMU
> would be correctly handled when page table are shared and not when they are
> "unshared".
As for relinquish_p2m_mapping(), I was thinking about it, but I
decided not to remove IOMMU mapping here since
the whole IOMMU page table would be removed during complete_domain_destroy().
But, I agree that nothing prevent someone to use __p2m_set_entry
directly in future.

>
> I would probably extend __p2m_get_entry with a new parameter indication
> whether we want to map the page in the IOMMU directly or not.
Sorry, I didn't get your point here. Did you mean __p2m_set_entry?

>
> Also, do you expect the IOMMU to support a page granularity bigger than the
> one supported by Xen? If not, we should probably have a check somewhere, to
> prevent potential security issue as we would map more than expected.
At the moment I keep in mind IPMMU only. And it supports the same page
granularity as the CPU
(4K, 2M, 1G). Could you, please, explain what a proposed check should check?
With the current solution we pass the whole memory block to the IOMMU
code and the IOMMU driver should handle this properly.
If we pass, for example 1,1 GB, but the IOMMU driver supports 4K pages
only then it has to split the memory block into 4K pages and process
them.

>
>
>> +         * Also we assume that the IOMMU mapping should be updated only
>> +         * if CPU mapping passed successfully.
>> +         */
>> +        if ( need_iommu(p2m->domain) && !iommu_use_hap_pt(p2m->domain) )
>> +        {
>> +            if ( !mfn_eq(orig_smfn, INVALID_MFN) )
>> +            {
>> +                unsigned int flags = p2m_get_iommu_flags(t);
>> +
>> +                rc = iommu_map_pages(p2m->domain,
>> +                                     gfn_x(orig_sgfn),
>> +                                     mfn_x(orig_smfn),
>> +                                     orig_nr,
>> +                                     flags);
>> +            }
>> +            else
>> +            {
>> +                rc = iommu_unmap_pages(p2m->domain,
>> +                                       gfn_x(orig_sgfn),
>> +                                       orig_nr);
>> +            }
>> +        }
>> +    }
>> +
>>      return rc;
>>  }
>>
>>
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 4/9] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Julien Grall]

On 21/04/17 15:18, Oleksandr Tyshchenko wrote:
> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Oleksandr,
> Hi, Julien

Hi Oleksandr,

>>
>> On 15/03/17 20:05, 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().
>>
>>
>> Can you explain in the commit message why you do this change in
>> p2m_set_entry and not __p2m_set_entry?
> ok
>
>>
>>
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>  xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 1fc6ca3..84d3a09 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>>>      if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
>>>          p2m_free_entry(p2m, orig_pte, level);
>>>
>>> -    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) ||
>>> p2m_valid(*entry)) )
>>> +    if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) &&
>>> +         (p2m_valid(orig_pte) || p2m_valid(*entry)) )
>>>          rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL <<
>>> page_order);
>>>      else
>>>          rc = 0;
>>> @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m,
>>>                    p2m_type_t t,
>>>                    p2m_access_t a)
>>>  {
>>> +    unsigned long orig_nr = nr;
>>> +    gfn_t orig_sgfn = sgfn;
>>> +    mfn_t orig_smfn = smfn;
>>>      int rc = 0;
>>>
>>>      while ( nr )
>>> @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m,
>>>          nr -= (1 << order);
>>>      }
>>>
>>> +    if ( likely(!rc) )
>>
>>
>> I would do
>>
>> if ( unlikely(rc) )
>>   return rc;
>>
>> /* IOMMU map/unmap ... */
>>
>> This would remove one indentation layer of if.
> Agree.
>
>>
>>> +    {
>>> +        /*
>>> +         * It's time to update IOMMU mapping if the latter doesn't
>>> +         * share page table with the CPU. Pass the whole memory block to
>>> let
>>> +         * the IOMMU code decide what to do with it.
>>> +         * The reason to update IOMMU mapping outside "while loop" is
>>> that
>>> +         * the IOMMU might not support the pages/superpages the CPU can
>>> deal
>>> +         * with (4K, 2M, 1G) and as result this will lead to non-optimal
>>> +         * mapping.
>>
>>
>> My worry with this solution is someone may decide to use __p2m_set_entry
>> (see relinquish) directly because he knows the correct order. So the IOMMU
>> would be correctly handled when page table are shared and not when they are
>> "unshared".
> As for relinquish_p2m_mapping(), I was thinking about it, but I
> decided not to remove IOMMU mapping here since
> the whole IOMMU page table would be removed during complete_domain_destroy().
> But, I agree that nothing prevent someone to use __p2m_set_entry
> directly in future.
>
>>
>> I would probably extend __p2m_get_entry with a new parameter indication
>> whether we want to map the page in the IOMMU directly or not.
> Sorry, I didn't get your point here. Did you mean __p2m_set_entry?

Yes sorry.

>
>>
>> Also, do you expect the IOMMU to support a page granularity bigger than the
>> one supported by Xen? If not, we should probably have a check somewhere, to
>> prevent potential security issue as we would map more than expected.
> At the moment I keep in mind IPMMU only. And it supports the same page
> granularity as the CPU
> (4K, 2M, 1G). Could you, please, explain what a proposed check should check?

We should check that the smallest granularity supported by the IOMMU is 
inferior or equal to PAGE_SIZE. If not, then there will be more rework 
required in Xen to support correctly those IOMMUs.

For instance, Xen PAGE_SIZE is currently 4KB. If the IOMMUs only support 
64KB, then you will end up to map a bigger chunk than expected leading a 
guest device to access memory it should not access.

Supporting such IOMMU will require much more work in Xen than this small 
changes.


> With the current solution we pass the whole memory block to the IOMMU
> code and the IOMMU driver should handle this properly.
> If we pass, for example 1,1 GB, but the IOMMU driver supports 4K pages
> only then it has to split the memory block into 4K pages and process
> them.

The IOMMU driver will likely duplicate the same logic as in 
p2m_set_entry and today does not seem to be necessary. By that I mean 
splitting into smaller chunk.

You may need to split again those chunks if the IOMMU does not support 
the granularity. But this is less likely on current hardware.

My main concern is to make sure __p2m_get_entry works as expected with 
all the configurations. Currently, what you describe will require much 
more work than this series. I will be ok whether you rework 
__p2m_get_entry or not.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 4/9] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Oleksandr Tyshchenko]

On Fri, Apr 21, 2017 at 7:27 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 21/04/17 15:18, Oleksandr Tyshchenko wrote:
>>
>> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> Hi Oleksandr,
>>
>> Hi, Julien
>
>
> Hi Oleksandr,
Hi, Julien

>
>
>>>
>>> On 15/03/17 20:05, 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().
>>>
>>>
>>>
>>> Can you explain in the commit message why you do this change in
>>> p2m_set_entry and not __p2m_set_entry?
>>
>> ok
>>
>>>
>>>
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>>  xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index 1fc6ca3..84d3a09 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>>>>      if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
>>>>          p2m_free_entry(p2m, orig_pte, level);
>>>>
>>>> -    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) ||
>>>> p2m_valid(*entry)) )
>>>> +    if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) &&
>>>> +         (p2m_valid(orig_pte) || p2m_valid(*entry)) )
>>>>          rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL <<
>>>> page_order);
>>>>      else
>>>>          rc = 0;
>>>> @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m,
>>>>                    p2m_type_t t,
>>>>                    p2m_access_t a)
>>>>  {
>>>> +    unsigned long orig_nr = nr;
>>>> +    gfn_t orig_sgfn = sgfn;
>>>> +    mfn_t orig_smfn = smfn;
>>>>      int rc = 0;
>>>>
>>>>      while ( nr )
>>>> @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m,
>>>>          nr -= (1 << order);
>>>>      }
>>>>
>>>> +    if ( likely(!rc) )
>>>
>>>
>>>
>>> I would do
>>>
>>> if ( unlikely(rc) )
>>>   return rc;
>>>
>>> /* IOMMU map/unmap ... */
>>>
>>> This would remove one indentation layer of if.
>>
>> Agree.
>>
>>>
>>>> +    {
>>>> +        /*
>>>> +         * It's time to update IOMMU mapping if the latter doesn't
>>>> +         * share page table with the CPU. Pass the whole memory block
>>>> to
>>>> let
>>>> +         * the IOMMU code decide what to do with it.
>>>> +         * The reason to update IOMMU mapping outside "while loop" is
>>>> that
>>>> +         * the IOMMU might not support the pages/superpages the CPU can
>>>> deal
>>>> +         * with (4K, 2M, 1G) and as result this will lead to
>>>> non-optimal
>>>> +         * mapping.
>>>
>>>
>>>
>>> My worry with this solution is someone may decide to use __p2m_set_entry
>>> (see relinquish) directly because he knows the correct order. So the
>>> IOMMU
>>> would be correctly handled when page table are shared and not when they
>>> are
>>> "unshared".
>>
>> As for relinquish_p2m_mapping(), I was thinking about it, but I
>> decided not to remove IOMMU mapping here since
>> the whole IOMMU page table would be removed during
>> complete_domain_destroy().
>> But, I agree that nothing prevent someone to use __p2m_set_entry
>> directly in future.
>>
>>>
>>> I would probably extend __p2m_get_entry with a new parameter indication
>>> whether we want to map the page in the IOMMU directly or not.
>>
>> Sorry, I didn't get your point here. Did you mean __p2m_set_entry?
>
>
> Yes sorry.
>
>>
>>>
>>> Also, do you expect the IOMMU to support a page granularity bigger than
>>> the
>>> one supported by Xen? If not, we should probably have a check somewhere,
>>> to
>>> prevent potential security issue as we would map more than expected.
>>
>> At the moment I keep in mind IPMMU only. And it supports the same page
>> granularity as the CPU
>> (4K, 2M, 1G). Could you, please, explain what a proposed check should
>> check?
>
>
> We should check that the smallest granularity supported by the IOMMU is
> inferior or equal to PAGE_SIZE. If not, then there will be more rework
> required in Xen to support correctly those IOMMUs.
>
> For instance, Xen PAGE_SIZE is currently 4KB. If the IOMMUs only support
> 64KB, then you will end up to map a bigger chunk than expected leading a
> guest device to access memory it should not access.
>
> Supporting such IOMMU will require much more work in Xen than this small
> changes.

Aha. Seems, I understand what you meant. I already have a check in IPMMU driver:

/*
* both the virtual address and the physical one, as well as
* the size of the mapping, must be aligned (at least) to the
* size of the smallest page supported by the hardware
*/
if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) {
printk("unaligned: iova 0x%lx pa 0x%"PRIx64" size 0x%zx min_pagesz 0x%x\n",
      iova, paddr, size, min_pagesz);
return -EINVAL;
}

where min_pagesz - is a minimum page size supported by hardware.
Hope, that this check can catch such case.

>
>
>> With the current solution we pass the whole memory block to the IOMMU
>> code and the IOMMU driver should handle this properly.
>> If we pass, for example 1,1 GB, but the IOMMU driver supports 4K pages
>> only then it has to split the memory block into 4K pages and process
>> them.
>
>
> The IOMMU driver will likely duplicate the same logic as in p2m_set_entry
> and today does not seem to be necessary. By that I mean splitting into
> smaller chunk.
>
> You may need to split again those chunks if the IOMMU does not support the
> granularity. But this is less likely on current hardware.
>
> My main concern is to make sure __p2m_get_entry works as expected with all
> the configurations. Currently, what you describe will require much more work
> than this series. I will be ok whether you rework __p2m_get_entry or not.
As we have already found common ground:
I will rework the patch by adding updating IOMMU mapping to
__p2m_set_entry and add new argument to it to
to show whether we have to update IOMMU mapping or not.
So, if we call  __p2m_set_entry directly, we have to force it to
update IOMMU mapping.

>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 4/9] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Julien Grall]

Hi Oleksandr,

On 21/04/17 19:44, Oleksandr Tyshchenko wrote:
> On Fri, Apr 21, 2017 at 7:27 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 21/04/17 15:18, Oleksandr Tyshchenko wrote:
>>>
>>> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com>
>>> wrote:
>
> Aha. Seems, I understand what you meant. I already have a check in IPMMU driver:
>
> /*
> * both the virtual address and the physical one, as well as
> * the size of the mapping, must be aligned (at least) to the
> * size of the smallest page supported by the hardware
> */
> if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) {
> printk("unaligned: iova 0x%lx pa 0x%"PRIx64" size 0x%zx min_pagesz 0x%x\n",
>       iova, paddr, size, min_pagesz);
> return -EINVAL;
> }
>
> where min_pagesz - is a minimum page size supported by hardware.
> Hope, that this check can catch such case.

I think will cover what I meant. Although, can't we do this check when 
the IOMMU is initialized at boot time rather than for every mapping?

For instance you know that if the mimimum page granularity supported  by 
the IOMMU is bigger than PAGE_SIZE, than you will likely get into 
trouble later on.

So rather than randomly crashing at runtime, you can disable the 
IOMMU/panic at boot time.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 4/9] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Oleksandr Tyshchenko]

On Mon, Apr 24, 2017 at 2:41 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi, Julien

>
> On 21/04/17 19:44, Oleksandr Tyshchenko wrote:
>>
>> On Fri, Apr 21, 2017 at 7:27 PM, Julien Grall <julien.grall@arm.com>
>> wrote:
>>>
>>> On 21/04/17 15:18, Oleksandr Tyshchenko wrote:
>>>>
>>>>
>>>> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@arm.com>
>>>> wrote:
>>
>>
>> Aha. Seems, I understand what you meant. I already have a check in IPMMU
>> driver:
>>
>> /*
>> * both the virtual address and the physical one, as well as
>> * the size of the mapping, must be aligned (at least) to the
>> * size of the smallest page supported by the hardware
>> */
>> if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) {
>> printk("unaligned: iova 0x%lx pa 0x%"PRIx64" size 0x%zx min_pagesz
>> 0x%x\n",
>>       iova, paddr, size, min_pagesz);
>> return -EINVAL;
>> }
>>
>> where min_pagesz - is a minimum page size supported by hardware.
>> Hope, that this check can catch such case.
>
>
> I think will cover what I meant. Although, can't we do this check when the
> IOMMU is initialized at boot time rather than for every mapping?
I think, yes.

>
> For instance you know that if the mimimum page granularity supported  by the
> IOMMU is bigger than PAGE_SIZE, than you will likely get into trouble later
> on.
>
> So rather than randomly crashing at runtime, you can disable the IOMMU/panic
> at boot time.
Agree.

>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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

[RFC PATCH 5/9] 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 1082fcf..b2bb41f 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2833,6 +2833,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

[RFC PATCH 6/9] iommu: Pass additional 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.
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 forced to false use_iommu flag
for now, no functional change is intended.

Basically, this patch is needed for unshared IOMMUs on ARM only
since the unshared 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 applicable for both platforms.
So, the patch target is to make ARM happy and not to brake x86.
Confirmation from x86 guys is needed.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 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 bb327da..bab62ee 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -550,7 +550,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 479aee6..8ef4160 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -646,7 +646,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 115698f..6c17c59 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_t 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 0446ed3..ab68ae2 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_t 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

Re: [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to iommu_domain_init()

[Jan Beulich]

>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
> --- 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_t use_iommu)

You properly use "false" above, so why bool_t (instead of just bool)
here?

Jan


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

Re: [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to iommu_domain_init()

[Oleksandr Tyshchenko]

On Wed, Mar 22, 2017 at 5:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>> --- 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_t use_iommu)
>
> You properly use "false" above, so why bool_t (instead of just bool)
> here?

Indeed, will use bool.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

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

[RFC PATCH 7/9] 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 unshared 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 unshared IOMMUs always
return error if the callback wasn't implemented.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 xen/drivers/passthrough/arm/iommu.c | 5 +++--
 xen/include/xen/iommu.h             | 1 +
 2 files changed, 4 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 ab68ae2..3150d7b 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -181,6 +181,7 @@ struct iommu_ops {
     int __must_check (*unmap_page)(struct domain *d, unsigned long gfn);
     int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
                                     unsigned long page_count);
+    int (*alloc_page_table)(struct domain *d);
     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

Re: [RFC PATCH 7/9] iommu/arm: Add alloc_page_table platform callback

[Jan Beulich]

>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -181,6 +181,7 @@ struct iommu_ops {
>      int __must_check (*unmap_page)(struct domain *d, unsigned long gfn);
>      int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
>                                      unsigned long page_count);
> +    int (*alloc_page_table)(struct domain *d);
>      void (*free_page_table)(struct page_info *);
>  #ifdef CONFIG_X86
>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);

As you can see in patch context here, we have x86-specific callbacks.
If the new one is used on ARM only, it should be made ARM-only.

Jan



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

Re: [RFC PATCH 7/9] iommu/arm: Add alloc_page_table platform callback

[Oleksandr Tyshchenko]

On Wed, Mar 22, 2017 at 5:49 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -181,6 +181,7 @@ struct iommu_ops {
>>      int __must_check (*unmap_page)(struct domain *d, unsigned long gfn);
>>      int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
>>                                      unsigned long page_count);
>> +    int (*alloc_page_table)(struct domain *d);
>>      void (*free_page_table)(struct page_info *);
>>  #ifdef CONFIG_X86
>>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
>
> As you can see in patch context here, we have x86-specific callbacks.
> If the new one is used on ARM only, it should be made ARM-only.

Agree. Will do.

-- 
Regards,

Oleksandr Tyshchenko

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

[RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Logic on ARM was changed a bit.
Taking into account that we are here because we have the IOMMU
that doesn't share page table with the CPU and need_iommu flag is set
just call arch_iommu_populate_page_table() to allow unshared IOMMU
to allocate resources.

No functional change for x86 part.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 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 6c17c59..cfe3bd1 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_page(d, gfn, mfn, 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 69cd6c5..b353449 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_page(d, gfn, mfn, 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 3150d7b..43cbb80 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

Re: [RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
> --- 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_page(d, gfn, mfn, 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);

The code being moved from here has survived the previous separation
of arch-independent from arch-specific code, and looking at the code
I also can't immediately see what's x86-specific here, so please make
the description explain why the code as is can't be used.

Jan


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

Re: [RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>> --- 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_page(d, gfn, mfn, 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);
>
> The code being moved from here has survived the previous separation
> of arch-independent from arch-specific code, and looking at the code
> I also can't immediately see what's x86-specific here, so please make
> the description explain why the code as is can't be used.
>
> Jan
>

You are right, there is nothing x86-specific here at first sight.
The reason why I moved this code to x86 folder is that we don't need
to retrieve IOMMU mappings on ARM this way. With need_iommu being
explicitly set at the hardware domain creation time
we just need to ask unshared IOMMU driver to allocate its page table
to be ready to receive and process IOMMU mappings from P2M code.

Other points that had prevented me from using it as is.
If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we
can find mfn by gfn for particular domain on ARM, but not vise versa.
Also this iommu_hwdom_init() is being called before allocating domain
memory on ARM. What is the point?
So, the d->page_list is empty during it execution.

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote:
> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>> --- 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_page(d, gfn, mfn, 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);
>>
>> The code being moved from here has survived the previous separation
>> of arch-independent from arch-specific code, and looking at the code
>> I also can't immediately see what's x86-specific here, so please make
>> the description explain why the code as is can't be used.
> 
> You are right, there is nothing x86-specific here at first sight.
> The reason why I moved this code to x86 folder is that we don't need
> to retrieve IOMMU mappings on ARM this way. With need_iommu being
> explicitly set at the hardware domain creation time
> we just need to ask unshared IOMMU driver to allocate its page table
> to be ready to receive and process IOMMU mappings from P2M code.
> 
> Other points that had prevented me from using it as is.
> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we
> can find mfn by gfn for particular domain on ARM, but not vise versa.
> Also this iommu_hwdom_init() is being called before allocating domain
> memory on ARM. What is the point?
> So, the d->page_list is empty during it execution.

In which case the question even more so is - why move the code
if it simply does nothing in your case?

Jan


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

Re: [RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote:
>> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>> --- 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_page(d, gfn, mfn, 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);
>>>
>>> The code being moved from here has survived the previous separation
>>> of arch-independent from arch-specific code, and looking at the code
>>> I also can't immediately see what's x86-specific here, so please make
>>> the description explain why the code as is can't be used.
>>
>> You are right, there is nothing x86-specific here at first sight.
>> The reason why I moved this code to x86 folder is that we don't need
>> to retrieve IOMMU mappings on ARM this way. With need_iommu being
>> explicitly set at the hardware domain creation time
>> we just need to ask unshared IOMMU driver to allocate its page table
>> to be ready to receive and process IOMMU mappings from P2M code.
>>
>> Other points that had prevented me from using it as is.
>> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we
>> can find mfn by gfn for particular domain on ARM, but not vise versa.
>> Also this iommu_hwdom_init() is being called before allocating domain
>> memory on ARM. What is the point?
>> So, the d->page_list is empty during it execution.
>
> In which case the question even more so is - why move the code
> if it simply does nothing in your case?

Reasonable question. Before answering you I would like to clarify the reason
why the iommu_hwdom_init() is being called before allocating domain
memory on ARM.
Is it a bug or there is an explanation for doing this.

I think that even if I don't move this code to x86 part I will still
need to call
arch_iommu_hwdom_init() after. In such case arch_iommu_hwdom_init()
would do what it is supposed to do on ARM and
would be a stub on x86. What do you think?

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Jan Beulich]

>>> On 23.03.17 at 13:40, <olekstysh@gmail.com> wrote:
> On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote:
>>> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>>> --- 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_page(d, gfn, mfn, 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);
>>>>
>>>> The code being moved from here has survived the previous separation
>>>> of arch-independent from arch-specific code, and looking at the code
>>>> I also can't immediately see what's x86-specific here, so please make
>>>> the description explain why the code as is can't be used.
>>>
>>> You are right, there is nothing x86-specific here at first sight.
>>> The reason why I moved this code to x86 folder is that we don't need
>>> to retrieve IOMMU mappings on ARM this way. With need_iommu being
>>> explicitly set at the hardware domain creation time
>>> we just need to ask unshared IOMMU driver to allocate its page table
>>> to be ready to receive and process IOMMU mappings from P2M code.
>>>
>>> Other points that had prevented me from using it as is.
>>> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we
>>> can find mfn by gfn for particular domain on ARM, but not vise versa.
>>> Also this iommu_hwdom_init() is being called before allocating domain
>>> memory on ARM. What is the point?
>>> So, the d->page_list is empty during it execution.
>>
>> In which case the question even more so is - why move the code
>> if it simply does nothing in your case?
> 
> Reasonable question. Before answering you I would like to clarify the reason
> why the iommu_hwdom_init() is being called before allocating domain
> memory on ARM.
> Is it a bug or there is an explanation for doing this.

Well, as that's different from x86 I'm afraid I'm the wrong one to ask
- the ARM maintainers would likely be in a much better position to
explain. The code sequence in each arch's construct_dom0() is
pretty clearly very different in this regard (ARM calls the function
almost first, while x86 calls it almost last.

Jan


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

Re: [RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Julien Grall]

Hi,

Sorry for the late answer.

On 23/03/17 12:40, Oleksandr Tyshchenko wrote:
> On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote:
>>> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>> The code being moved from here has survived the previous separation
>>>> of arch-independent from arch-specific code, and looking at the code
>>>> I also can't immediately see what's x86-specific here, so please make
>>>> the description explain why the code as is can't be used.
>>>
>>> You are right, there is nothing x86-specific here at first sight.
>>> The reason why I moved this code to x86 folder is that we don't need
>>> to retrieve IOMMU mappings on ARM this way. With need_iommu being
>>> explicitly set at the hardware domain creation time
>>> we just need to ask unshared IOMMU driver to allocate its page table
>>> to be ready to receive and process IOMMU mappings from P2M code.
>>>
>>> Other points that had prevented me from using it as is.
>>> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we
>>> can find mfn by gfn for particular domain on ARM, but not vise versa.
>>> Also this iommu_hwdom_init() is being called before allocating domain
>>> memory on ARM. What is the point?
>>> So, the d->page_list is empty during it execution.
>>
>> In which case the question even more so is - why move the code
>> if it simply does nothing in your case?

I didn't move the code, because back then we were only planning to 
support shared page table (iommu_use_hap_pt(...) always returns true on 
ARM so far).

With more perspective, this code cannot work on ARM because of 
mfn_to_gmfn (we don't have an M2P). If we keep the code like that after 
this series, this will at least expose a bug (the helper always assume a 
direct mapping).

So I think moving the code is the right solution.

>
> Reasonable question. Before answering you I would like to clarify the reason
> why the iommu_hwdom_init() is being called before allocating domain
> memory on ARM.
> Is it a bug or there is an explanation for doing this.

No real reason, I didn't see a reason to call this function later on. I 
would be interested to know whether there is a latent bug.

Anyway, I think this is a good idea to fully initialize the IOMMU early 
for DOM0 as the builder will take care of assigning the non-PCI device 
protected.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 8/9] iommu: Split iommu_hwdom_init() into arch specific parts

[Oleksandr Tyshchenko]

On Wed, Apr 19, 2017 at 9:09 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi,
Hi, Julien.

>
> Sorry for the late answer.
>
> On 23/03/17 12:40, Oleksandr Tyshchenko wrote:
>>
>> On Thu, Mar 23, 2017 at 11:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>
>>>>>> On 22.03.17 at 19:40, <olekstysh@gmail.com> wrote:
>>>>
>>>> On Wed, Mar 22, 2017 at 5:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>
>>>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>>>
>>>>> The code being moved from here has survived the previous separation
>>>>> of arch-independent from arch-specific code, and looking at the code
>>>>> I also can't immediately see what's x86-specific here, so please make
>>>>> the description explain why the code as is can't be used.
>>>>
>>>>
>>>> You are right, there is nothing x86-specific here at first sight.
>>>> The reason why I moved this code to x86 folder is that we don't need
>>>> to retrieve IOMMU mappings on ARM this way. With need_iommu being
>>>> explicitly set at the hardware domain creation time
>>>> we just need to ask unshared IOMMU driver to allocate its page table
>>>> to be ready to receive and process IOMMU mappings from P2M code.
>>>>
>>>> Other points that had prevented me from using it as is.
>>>> If the hardware domain isn't 1:1 mapped we won't know gfn. IIRC, we
>>>> can find mfn by gfn for particular domain on ARM, but not vise versa.
>>>> Also this iommu_hwdom_init() is being called before allocating domain
>>>> memory on ARM. What is the point?
>>>> So, the d->page_list is empty during it execution.
>>>
>>>
>>> In which case the question even more so is - why move the code
>>> if it simply does nothing in your case?
>
>
> I didn't move the code, because back then we were only planning to support
> shared page table (iommu_use_hap_pt(...) always returns true on ARM so far).
>
> With more perspective, this code cannot work on ARM because of mfn_to_gmfn
> (we don't have an M2P). If we keep the code like that after this series,
> this will at least expose a bug (the helper always assume a direct mapping).
Agree.

>
> So I think moving the code is the right solution.
>
>>
>> Reasonable question. Before answering you I would like to clarify the
>> reason
>> why the iommu_hwdom_init() is being called before allocating domain
>> memory on ARM.
>> Is it a bug or there is an explanation for doing this.
>
>
> No real reason, I didn't see a reason to call this function later on. I
> would be interested to know whether there is a latent bug.
>
> Anyway, I think this is a good idea to fully initialize the IOMMU early for
> DOM0 as the builder will take care of assigning the non-PCI device
> protected.
Does this patch do right things or I have missed something?

>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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

[RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[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.
The primary aim of this knowledge is to help the IOMMUs that don't
share page tables with the CPU be ready before P2M code starts
updating IOMMU mapping.
So, if this flag is set the unshared 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*.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 tools/libxl/libxl_create.c  | 5 +++++
 xen/arch/arm/domain.c       | 4 +++-
 xen/arch/x86/domain.c       | 4 +++-
 xen/common/domctl.c         | 5 ++++-
 xen/include/public/domctl.h | 3 +++
 xen/include/xen/sched.h     | 3 +++
 6 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e741b9a..4393fa2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         flags |= XEN_DOMCTL_CDF_hap;
     }
 
+    /* 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))
+        flags |= XEN_DOMCTL_CDF_use_iommu;
+
     /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
     libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid);
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bab62ee..940bb98 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
                        struct xen_arch_domainconfig *config)
 {
     int rc, count = 0;
+    bool_t use_iommu;
 
     BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
     d->arch.relmem = RELMEM_not_started;
@@ -550,7 +551,8 @@ 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 )
+    use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
+    if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
         goto fail;
 
     if ( (rc = p2m_init(d)) != 0 )
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8ef4160..7d634ff 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
 {
     bool paging_initialised = false;
     int rc = -ENOMEM;
+    bool_t use_iommu;
 
     if ( config == NULL && !is_idle_domain(d) )
         return -EINVAL;
@@ -646,7 +647,8 @@ 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, false)) != 0 )
+        use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
+        if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
             goto fail;
     }
     spin_lock_init(&d->arch.e820_lock);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 93e3029..56c4d38 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                | XEN_DOMCTL_CDF_hap
                | XEN_DOMCTL_CDF_s3_integrity
                | XEN_DOMCTL_CDF_oos_off
-               | XEN_DOMCTL_CDF_xs_domain)) )
+               | XEN_DOMCTL_CDF_xs_domain
+               | XEN_DOMCTL_CDF_use_iommu)) )
             break;
 
         dom = op->domain;
@@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             domcr_flags |= DOMCRF_oos_off;
         if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
             domcr_flags |= DOMCRF_xs_domain;
+        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu )
+            domcr_flags |= DOMCRF_use_iommu;
 
         d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
                           &op->u.createdomain.config);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 85cbb7c..a37a566 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
  /* Is this a xenstore domain? */
 #define _XEN_DOMCTL_CDF_xs_domain     5
 #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
+ /* Should IOMMU page tables be populated at the domain creation time? */
+#define _XEN_DOMCTL_CDF_use_iommu     6
+#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
     uint32_t flags;
     struct xen_arch_domainconfig config;
 };
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 0929c0b..80e6fdc 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -561,6 +561,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
  /* DOMCRF_xs_domain: xenstore domain */
 #define _DOMCRF_xs_domain       6
 #define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
+ /* DOMCRF_use_iommu: Populate IOMMU page tables at the domain creation time */
+#define _DOMCRF_use_iommu       7
+#define DOMCRF_use_iommu        (1U<<_DOMCRF_use_iommu)
 
 /*
  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
-- 
2.7.4


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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Jan Beulich]

>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>   /* Is this a xenstore domain? */
>  #define _XEN_DOMCTL_CDF_xs_domain     5
>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
> + /* Should IOMMU page tables be populated at the domain creation time? */
> +#define _XEN_DOMCTL_CDF_use_iommu     6
> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>      uint32_t flags;

The need for this to be done via domain creation flag (rather than
as a separate, later step) needs to be well explained. Generally
what to add here should only be things which can't be done later
in a reasonable way.

Jan


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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>   /* Is this a xenstore domain? */
>>  #define _XEN_DOMCTL_CDF_xs_domain     5
>>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
>> + /* Should IOMMU page tables be populated at the domain creation time? */
>> +#define _XEN_DOMCTL_CDF_use_iommu     6
>> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>      uint32_t flags;
>
> The need for this to be done via domain creation flag (rather than
> as a separate, later step) needs to be well explained. Generally
> what to add here should only be things which can't be done later
> in a reasonable way.

Well, the non-shared IOMMU should populate its page table by the time
the P2M code will have started update mappings. Theoretically, it
might happen right after p2m_init has been completed,
that is, even during createdomain domctl execution. For example, I see
that domain_vgic_init() inserts mapping to P2M table, because of
map_mmio_regions() is being called during VGIC initialization.
Not sure that GIC mmio ranges must be present in the IOMMU page table,
but anyway, it might be the per-domain initialization of other IPs,
co-processors that mapping mustn't be skipped because of IOMMU is not
ready.
So, as both iommu_domain_init() and p2m_init() are called from
arch_domain_create(), i.e. during createdomain domctl execution, we
have to know exactly should we use IOMMU for this domain to pass
proper value to iommu_domain_init().

If don't take into account everything I wrote above, yes, it is
possible to introduce new domctl for this purpose that will be called
later, but before any operations with guest_physmap.

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Jan Beulich]

>>> On 23.03.17 at 17:36, <olekstysh@gmail.com> wrote:
> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>>   /* Is this a xenstore domain? */
>>>  #define _XEN_DOMCTL_CDF_xs_domain     5
>>>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
>>> + /* Should IOMMU page tables be populated at the domain creation time? */
>>> +#define _XEN_DOMCTL_CDF_use_iommu     6
>>> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>>      uint32_t flags;
>>
>> The need for this to be done via domain creation flag (rather than
>> as a separate, later step) needs to be well explained. Generally
>> what to add here should only be things which can't be done later
>> in a reasonable way.
> 
> Well, the non-shared IOMMU should populate its page table by the time
> the P2M code will have started update mappings. Theoretically, it
> might happen right after p2m_init has been completed,
> that is, even during createdomain domctl execution. For example, I see
> that domain_vgic_init() inserts mapping to P2M table, because of
> map_mmio_regions() is being called during VGIC initialization.
> Not sure that GIC mmio ranges must be present in the IOMMU page table,
> but anyway, it might be the per-domain initialization of other IPs,
> co-processors that mapping mustn't be skipped because of IOMMU is not
> ready.
> So, as both iommu_domain_init() and p2m_init() are called from
> arch_domain_create(), i.e. during createdomain domctl execution, we
> have to know exactly should we use IOMMU for this domain to pass
> proper value to iommu_domain_init().

Well, no, iommu_domain_init() is not supposed to do any table
setup, so it shouldn't need to know.

> If don't take into account everything I wrote above, yes, it is
> possible to introduce new domctl for this purpose that will be called
> later, but before any operations with guest_physmap.

Exactly. Any other things needing syncing, but being done during
domain creation may then need syncing over. One might question
whether some of those things then actually are being done too
early (and quite possibly have been done that way just for simplicity).

Jan


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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

Hi Jan

On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 23.03.17 at 17:36, <olekstysh@gmail.com> wrote:
>> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>> --- a/xen/include/public/domctl.h
>>>> +++ b/xen/include/public/domctl.h
>>>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>>>   /* Is this a xenstore domain? */
>>>>  #define _XEN_DOMCTL_CDF_xs_domain     5
>>>>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
>>>> + /* Should IOMMU page tables be populated at the domain creation time? */
>>>> +#define _XEN_DOMCTL_CDF_use_iommu     6
>>>> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>>>      uint32_t flags;
>>>
>>> The need for this to be done via domain creation flag (rather than
>>> as a separate, later step) needs to be well explained. Generally
>>> what to add here should only be things which can't be done later
>>> in a reasonable way.
>>
>> Well, the non-shared IOMMU should populate its page table by the time
>> the P2M code will have started update mappings. Theoretically, it
>> might happen right after p2m_init has been completed,
>> that is, even during createdomain domctl execution. For example, I see
>> that domain_vgic_init() inserts mapping to P2M table, because of
>> map_mmio_regions() is being called during VGIC initialization.
>> Not sure that GIC mmio ranges must be present in the IOMMU page table,
>> but anyway, it might be the per-domain initialization of other IPs,
>> co-processors that mapping mustn't be skipped because of IOMMU is not
>> ready.
>> So, as both iommu_domain_init() and p2m_init() are called from
>> arch_domain_create(), i.e. during createdomain domctl execution, we
>> have to know exactly should we use IOMMU for this domain to pass
>> proper value to iommu_domain_init().
>
> Well, no, iommu_domain_init() is not supposed to do any table
> setup, so it shouldn't need to know.

So, does this mean that you disagree to what this patch does as well
as the preceding patch
[RFC PATCH 6/9] iommu: Pass additional use_iommu argument to
iommu_domain_init().
Am I right?

As we need this use_iommu flag on ARM only, the possible solution
might be to hide it in struct xen_arch_domainconfig for ARM.
In such case we would always call iommu_domain_init() with use_iommu
forced to false on x86.

>
>> If don't take into account everything I wrote above, yes, it is
>> possible to introduce new domctl for this purpose that will be called
>> later, but before any operations with guest_physmap.
>
> Exactly. Any other things needing syncing, but being done during
> domain creation may then need syncing over. One might question
> whether some of those things then actually are being done too
> early (and quite possibly have been done that way just for simplicity).

Sorry, I'm afraid I don't entirely understand you here.

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Jan Beulich]

>>> On 24.03.17 at 12:19, <olekstysh@gmail.com> wrote:
> Hi Jan
> 
> On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 23.03.17 at 17:36, <olekstysh@gmail.com> wrote:
>>> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>>> --- a/xen/include/public/domctl.h
>>>>> +++ b/xen/include/public/domctl.h
>>>>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>>>>   /* Is this a xenstore domain? */
>>>>>  #define _XEN_DOMCTL_CDF_xs_domain     5
>>>>>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
>>>>> + /* Should IOMMU page tables be populated at the domain creation time? */
>>>>> +#define _XEN_DOMCTL_CDF_use_iommu     6
>>>>> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>>>>      uint32_t flags;
>>>>
>>>> The need for this to be done via domain creation flag (rather than
>>>> as a separate, later step) needs to be well explained. Generally
>>>> what to add here should only be things which can't be done later
>>>> in a reasonable way.
>>>
>>> Well, the non-shared IOMMU should populate its page table by the time
>>> the P2M code will have started update mappings. Theoretically, it
>>> might happen right after p2m_init has been completed,
>>> that is, even during createdomain domctl execution. For example, I see
>>> that domain_vgic_init() inserts mapping to P2M table, because of
>>> map_mmio_regions() is being called during VGIC initialization.
>>> Not sure that GIC mmio ranges must be present in the IOMMU page table,
>>> but anyway, it might be the per-domain initialization of other IPs,
>>> co-processors that mapping mustn't be skipped because of IOMMU is not
>>> ready.
>>> So, as both iommu_domain_init() and p2m_init() are called from
>>> arch_domain_create(), i.e. during createdomain domctl execution, we
>>> have to know exactly should we use IOMMU for this domain to pass
>>> proper value to iommu_domain_init().
>>
>> Well, no, iommu_domain_init() is not supposed to do any table
>> setup, so it shouldn't need to know.
> 
> So, does this mean that you disagree to what this patch does as well
> as the preceding patch
> [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to
> iommu_domain_init().

Yes.

> As we need this use_iommu flag on ARM only, the possible solution
> might be to hide it in struct xen_arch_domainconfig for ARM.
> In such case we would always call iommu_domain_init() with use_iommu
> forced to false on x86.

If that won't involve a domain creation flag addition anymore,
I'd be fine and leave this to the ARM maintainers.

>>> If don't take into account everything I wrote above, yes, it is
>>> possible to introduce new domctl for this purpose that will be called
>>> later, but before any operations with guest_physmap.
>>
>> Exactly. Any other things needing syncing, but being done during
>> domain creation may then need syncing over. One might question
>> whether some of those things then actually are being done too
>> early (and quite possibly have been done that way just for simplicity).
> 
> Sorry, I'm afraid I don't entirely understand you here.

You had mentioned a couple of things where you think you need
to know ahead of the time whether IOMMU use will actually be
needed. In turn I question whether these things can't be done
later, i.e. whether they're being done in their current ordering
just for convenience of the original code authors.

Jan


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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

On Fri, Mar 24, 2017 at 1:38 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 24.03.17 at 12:19, <olekstysh@gmail.com> wrote:
>> Hi Jan
>>
>> On Thu, Mar 23, 2017 at 7:05 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 23.03.17 at 17:36, <olekstysh@gmail.com> wrote:
>>>> On Wed, Mar 22, 2017 at 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 15.03.17 at 21:05, <olekstysh@gmail.com> wrote:
>>>>>> --- a/xen/include/public/domctl.h
>>>>>> +++ b/xen/include/public/domctl.h
>>>>>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>>>>>   /* Is this a xenstore domain? */
>>>>>>  #define _XEN_DOMCTL_CDF_xs_domain     5
>>>>>>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
>>>>>> + /* Should IOMMU page tables be populated at the domain creation time? */
>>>>>> +#define _XEN_DOMCTL_CDF_use_iommu     6
>>>>>> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>>>>>      uint32_t flags;
>>>>>
>>>>> The need for this to be done via domain creation flag (rather than
>>>>> as a separate, later step) needs to be well explained. Generally
>>>>> what to add here should only be things which can't be done later
>>>>> in a reasonable way.
>>>>
>>>> Well, the non-shared IOMMU should populate its page table by the time
>>>> the P2M code will have started update mappings. Theoretically, it
>>>> might happen right after p2m_init has been completed,
>>>> that is, even during createdomain domctl execution. For example, I see
>>>> that domain_vgic_init() inserts mapping to P2M table, because of
>>>> map_mmio_regions() is being called during VGIC initialization.
>>>> Not sure that GIC mmio ranges must be present in the IOMMU page table,
>>>> but anyway, it might be the per-domain initialization of other IPs,
>>>> co-processors that mapping mustn't be skipped because of IOMMU is not
>>>> ready.
>>>> So, as both iommu_domain_init() and p2m_init() are called from
>>>> arch_domain_create(), i.e. during createdomain domctl execution, we
>>>> have to know exactly should we use IOMMU for this domain to pass
>>>> proper value to iommu_domain_init().
>>>
>>> Well, no, iommu_domain_init() is not supposed to do any table
>>> setup, so it shouldn't need to know.
>>
>> So, does this mean that you disagree to what this patch does as well
>> as the preceding patch
>> [RFC PATCH 6/9] iommu: Pass additional use_iommu argument to
>> iommu_domain_init().
>
> Yes.
>
>> As we need this use_iommu flag on ARM only, the possible solution
>> might be to hide it in struct xen_arch_domainconfig for ARM.
>> In such case we would always call iommu_domain_init() with use_iommu
>> forced to false on x86.
>
> If that won't involve a domain creation flag addition anymore,
> I'd be fine and leave this to the ARM maintainers.

OK.

>
>>>> If don't take into account everything I wrote above, yes, it is
>>>> possible to introduce new domctl for this purpose that will be called
>>>> later, but before any operations with guest_physmap.
>>>
>>> Exactly. Any other things needing syncing, but being done during
>>> domain creation may then need syncing over. One might question
>>> whether some of those things then actually are being done too
>>> early (and quite possibly have been done that way just for simplicity).
>>
>> Sorry, I'm afraid I don't entirely understand you here.
>
> You had mentioned a couple of things where you think you need
> to know ahead of the time whether IOMMU use will actually be
> needed. In turn I question whether these things can't be done
> later, i.e. whether they're being done in their current ordering
> just for convenience of the original code authors.

Now I got it, but I don't have the right answer at the moment.

-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Julien Grall]

Hi Oleksandr,

Please CC the appropriate maintainers for all the components you modify.

On 15/03/17 20:05, 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.
> The primary aim of this knowledge is to help the IOMMUs that don't
> share page tables with the CPU be ready before P2M code starts
> updating IOMMU mapping.
> So, if this flag is set the unshared 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*.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  tools/libxl/libxl_create.c  | 5 +++++
>  xen/arch/arm/domain.c       | 4 +++-
>  xen/arch/x86/domain.c       | 4 +++-
>  xen/common/domctl.c         | 5 ++++-
>  xen/include/public/domctl.h | 3 +++
>  xen/include/xen/sched.h     | 3 +++
>  6 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e741b9a..4393fa2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>          flags |= XEN_DOMCTL_CDF_hap;
>      }
>
> +    /* 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))
> +        flags |= XEN_DOMCTL_CDF_use_iommu;

Regardless Jan's comment about the flag, I believe we still want to keep 
the current behavior for x86 (e.g allocating the page table on-demand).

So I think this should be per architecture decision rather than a common 
change. Maybe in, libxl__arch_domain_prepare_config. This also means we 
would switch to xen_arch_domainconfig.

> +
>      /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
>      libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid);
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index bab62ee..940bb98 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>                         struct xen_arch_domainconfig *config)
>  {
>      int rc, count = 0;
> +    bool_t use_iommu;

s/bool_t/bool/

>
>      BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
>      d->arch.relmem = RELMEM_not_started;
> @@ -550,7 +551,8 @@ 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 )
> +    use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
> +    if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
>          goto fail;
>
>      if ( (rc = p2m_init(d)) != 0 )
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 8ef4160..7d634ff 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>  {
>      bool paging_initialised = false;
>      int rc = -ENOMEM;
> +    bool_t use_iommu;

Ditto.

>
>      if ( config == NULL && !is_idle_domain(d) )
>          return -EINVAL;
> @@ -646,7 +647,8 @@ 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, false)) != 0 )
> +        use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
> +        if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
>              goto fail;
>      }
>      spin_lock_init(&d->arch.e820_lock);
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 93e3029..56c4d38 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>                 | XEN_DOMCTL_CDF_hap
>                 | XEN_DOMCTL_CDF_s3_integrity
>                 | XEN_DOMCTL_CDF_oos_off
> -               | XEN_DOMCTL_CDF_xs_domain)) )
> +               | XEN_DOMCTL_CDF_xs_domain
> +               | XEN_DOMCTL_CDF_use_iommu)) )
>              break;
>
>          dom = op->domain;
> @@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              domcr_flags |= DOMCRF_oos_off;
>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
>              domcr_flags |= DOMCRF_xs_domain;
> +        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu )
> +            domcr_flags |= DOMCRF_use_iommu;
>
>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
>                            &op->u.createdomain.config);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 85cbb7c..a37a566 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>   /* Is this a xenstore domain? */
>  #define _XEN_DOMCTL_CDF_xs_domain     5
>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
> + /* Should IOMMU page tables be populated at the domain creation time? */
> +#define _XEN_DOMCTL_CDF_use_iommu     6
> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>      uint32_t flags;
>      struct xen_arch_domainconfig config;
>  };
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 0929c0b..80e6fdc 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -561,6 +561,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>   /* DOMCRF_xs_domain: xenstore domain */
>  #define _DOMCRF_xs_domain       6
>  #define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
> + /* DOMCRF_use_iommu: Populate IOMMU page tables at the domain creation time */
> +#define _DOMCRF_use_iommu       7
> +#define DOMCRF_use_iommu        (1U<<_DOMCRF_use_iommu)
>
>  /*
>   * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
>

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

On Wed, Apr 19, 2017 at 9:26 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi, Julien

>
> Please CC the appropriate maintainers for all the components you modify.
Sorry, sure.

>
>
> On 15/03/17 20:05, 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.
>> The primary aim of this knowledge is to help the IOMMUs that don't
>> share page tables with the CPU be ready before P2M code starts
>> updating IOMMU mapping.
>> So, if this flag is set the unshared 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*.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  tools/libxl/libxl_create.c  | 5 +++++
>>  xen/arch/arm/domain.c       | 4 +++-
>>  xen/arch/x86/domain.c       | 4 +++-
>>  xen/common/domctl.c         | 5 ++++-
>>  xen/include/public/domctl.h | 3 +++
>>  xen/include/xen/sched.h     | 3 +++
>>  6 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index e741b9a..4393fa2 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -546,6 +546,11 @@ int libxl__domain_make(libxl__gc *gc,
>> libxl_domain_config *d_config,
>>          flags |= XEN_DOMCTL_CDF_hap;
>>      }
>>
>> +    /* 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))
>> +        flags |= XEN_DOMCTL_CDF_use_iommu;
>
>
> Regardless Jan's comment about the flag, I believe we still want to keep the
> current behavior for x86 (e.g allocating the page table on-demand).
>
> So I think this should be per architecture decision rather than a common
> change. Maybe in, libxl__arch_domain_prepare_config. This also means we
> would switch to xen_arch_domainconfig.
Agree. Will do. So, the use_iommu flag will be included to xen_arch_domainconfig
and passed to iommu_domain_init() on ARM. On x86 we will always pass
the "false" value to
iommu_domain_init(). Right?

>
>> +
>>      /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
>>      libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid);
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index bab62ee..940bb98 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -539,6 +539,7 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>>                         struct xen_arch_domainconfig *config)
>>  {
>>      int rc, count = 0;
>> +    bool_t use_iommu;
>
>
> s/bool_t/bool/
ok

>
>>
>>      BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
>>      d->arch.relmem = RELMEM_not_started;
>> @@ -550,7 +551,8 @@ 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 )
>> +    use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
>> +    if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
>>          goto fail;
>>
>>      if ( (rc = p2m_init(d)) != 0 )
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 8ef4160..7d634ff 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -525,6 +525,7 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>>  {
>>      bool paging_initialised = false;
>>      int rc = -ENOMEM;
>> +    bool_t use_iommu;
>
>
> Ditto.
ok

>
>
>>
>>      if ( config == NULL && !is_idle_domain(d) )
>>          return -EINVAL;
>> @@ -646,7 +647,8 @@ 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, false)) != 0 )
>> +        use_iommu = !!(domcr_flags & DOMCRF_use_iommu);
>> +        if ( (rc = iommu_domain_init(d, use_iommu)) != 0 )
>>              goto fail;
>>      }
>>      spin_lock_init(&d->arch.e820_lock);
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 93e3029..56c4d38 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -505,7 +505,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>> u_domctl)
>>                 | XEN_DOMCTL_CDF_hap
>>                 | XEN_DOMCTL_CDF_s3_integrity
>>                 | XEN_DOMCTL_CDF_oos_off
>> -               | XEN_DOMCTL_CDF_xs_domain)) )
>> +               | XEN_DOMCTL_CDF_xs_domain
>> +               | XEN_DOMCTL_CDF_use_iommu)) )
>>              break;
>>
>>          dom = op->domain;
>> @@ -549,6 +550,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>> u_domctl)
>>              domcr_flags |= DOMCRF_oos_off;
>>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
>>              domcr_flags |= DOMCRF_xs_domain;
>> +        if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_use_iommu )
>> +            domcr_flags |= DOMCRF_use_iommu;
>>
>>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
>>                            &op->u.createdomain.config);
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 85cbb7c..a37a566 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -66,6 +66,9 @@ struct xen_domctl_createdomain {
>>   /* Is this a xenstore domain? */
>>  #define _XEN_DOMCTL_CDF_xs_domain     5
>>  #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
>> + /* Should IOMMU page tables be populated at the domain creation time? */
>> +#define _XEN_DOMCTL_CDF_use_iommu     6
>> +#define XEN_DOMCTL_CDF_use_iommu      (1U<<_XEN_DOMCTL_CDF_use_iommu)
>>      uint32_t flags;
>>      struct xen_arch_domainconfig config;
>>  };
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 0929c0b..80e6fdc 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -561,6 +561,9 @@ struct domain *domain_create(domid_t domid, unsigned
>> int domcr_flags,
>>   /* DOMCRF_xs_domain: xenstore domain */
>>  #define _DOMCRF_xs_domain       6
>>  #define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
>> + /* DOMCRF_use_iommu: Populate IOMMU page tables at the domain creation
>> time */
>> +#define _DOMCRF_use_iommu       7
>> +#define DOMCRF_use_iommu        (1U<<_DOMCRF_use_iommu)
>>
>>  /*
>>   * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
>>
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Wei Liu]

On Wed, Apr 19, 2017 at 07:26:44PM +0100, Julien Grall wrote:
> Hi Oleksandr,
> 
> Please CC the appropriate maintainers for all the components you modify.
> 
> On 15/03/17 20:05, 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.
> > The primary aim of this knowledge is to help the IOMMUs that don't
> > share page tables with the CPU be ready before P2M code starts
> > updating IOMMU mapping.
> > So, if this flag is set the unshared 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*.
> > 

If it is not able to do so since the very beginning, what will happen?

Let me explain where I'm coming from:

1. if not populating the iommu page table causes Xen to malfunction
   (crash?), it is a bug. It should be fixed without involvement
   from toolstack.
2. if this is an optimasation, can't we not always populate iommu pt
   hence no new flag is needed?

Overall I'm not too convinced a new flag is needed.

Wei.

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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

Hi, Wei

On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, Apr 19, 2017 at 07:26:44PM +0100, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> Please CC the appropriate maintainers for all the components you modify.
>>
>> On 15/03/17 20:05, 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.
>> > The primary aim of this knowledge is to help the IOMMUs that don't
>> > share page tables with the CPU be ready before P2M code starts
>> > updating IOMMU mapping.
>> > So, if this flag is set the unshared 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*.
>> >
>
> If it is not able to do so since the very beginning, what will happen?
IOMMU page faults will occur if passthroughed/protected devices try to
do DMA since
the corresponding memory mapping won't be present in IOMMU page table.
If we don't populate IOMMU page table (with setting need_iommu flag)
during domain creation time we will lose memory mapping (at least
domain RAM).
Unfortunately, we can't restore this memory mapping on ARM unlike x86.
I would like to say that it is true for non-shared IOMMUs only. For
shared IOMMU (SMMU) we don't have such problem.

>
> Let me explain where I'm coming from:
>
> 1. if not populating the iommu page table causes Xen to malfunction
>    (crash?), it is a bug. It should be fixed without involvement
>    from toolstack.
> 2. if this is an optimasation, can't we not always populate iommu pt
>    hence no new flag is needed?
>
> Overall I'm not too convinced a new flag is needed.
We don't want to always populate IOMMU page table since it might be
just wasting CPU and memory resources if no devices are assigned to
domain. That's why we need this hint.

>
> Wei.



-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Ian Jackson]

Oleksandr Tyshchenko writes ("Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl"):
> On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > Let me explain where I'm coming from:
> >
> > 1. if not populating the iommu page table causes Xen to malfunction
> >    (crash?), it is a bug. It should be fixed without involvement
> >    from toolstack.
> > 2. if this is an optimasation, can't we not always populate iommu pt
> >    hence no new flag is needed?
> >
> > Overall I'm not too convinced a new flag is needed.
>
> We don't want to always populate IOMMU page table since it might be
> just wasting CPU and memory resources if no devices are assigned to
> domain. That's why we need this hint.

I thought we already had a way to tell libxl that pci passthrough is
expected for this domain.  There are other things that want to know
this in advance.  I can't seem to find it now at least in the docs,
though.

Ian.

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

Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl

[Oleksandr Tyshchenko]

Hi, Ian

On Wed, Apr 26, 2017 at 1:05 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> Oleksandr Tyshchenko writes ("Re: [RFC PATCH 9/9] xen: Add use_iommu flag to createdomain domctl"):
>> On Tue, Apr 25, 2017 at 6:23 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> > Let me explain where I'm coming from:
>> >
>> > 1. if not populating the iommu page table causes Xen to malfunction
>> >    (crash?), it is a bug. It should be fixed without involvement
>> >    from toolstack.
>> > 2. if this is an optimasation, can't we not always populate iommu pt
>> >    hence no new flag is needed?
>> >
>> > Overall I'm not too convinced a new flag is needed.
>>
>> We don't want to always populate IOMMU page table since it might be
>> just wasting CPU and memory resources if no devices are assigned to
>> domain. That's why we need this hint.
>
> I thought we already had a way to tell libxl that pci passthrough is
> expected for this domain.  There are other things that want to know
> this in advance.  I can't seem to find it now at least in the docs,
> though.
Did mean a way different from the guest configuration file?

>
> Ian.



-- 
Regards,

Oleksandr Tyshchenko

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

Re: [RFC PATCH 0/9] "Non-shared" IOMMU support on ARM

[Julien Grall]

On 03/15/2017 08:05 PM, Oleksandr Tyshchenko wrote:
> Hi, all.

Hi Oleksandr,

Thank you for looking at the support of "non-shared" page table IOMMU 
support.

>
> 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).
> And this IOMMU can't share the page table with the CPU since it uses
> stage-1 page table unlike the CPU that uses stage-2 therefore I name it
> "Non-shared" IOMMU.
>
> I have already raised disscusion [2] about some generic problems I had faced
> during porting IPMMU-VMSA Linux driver to Xen. And on this basis I made
> patches you can see it in this request. Only the first patch is not related to
> IOMMU. But, I decided to ship it with the current request since it is a generic change
> which we will need in a moment.
>
> The reason for this patch series to be RFC is that I still have some doubts about generic code I touched.
> I hope that I haven't broken anything for x86, but confirmation is needed.
>
> I didn't include IPMMU-VMSA driver in this request. Although, I am still in progress, I want to say
> that passthrough use-cases (actually what this all are firstly needed for) work for me with some limitations.
> I tested on Salvator-X board (R-Car H3) with the next devices that have DMA IPs
> connected to IPMMU uTLBs (using current master branch):
> 1. AUDMA is assigned to dom0 (protected by IOMMU)
> 2. SDHC0 is assigned to dom1 (passthrough to domain)
> 3. SDHC3 is assigned to dom2 (passthrough to domain)

I am looking forward to see the support in Xen :).

> During porting IPMMU-VMSA driver to Xen I was trying to be as close as possible to Linux [1]. But,
> it was a little bit difficult). It would be really nice to have some feedback and get your feeling regarding this driver.
> I am also interested in if I took the right direction or there are some other ideas on doing the same.
> So, is it a right direction to move on?

I would be interested to know what was the difficulties to port Linux 
driver in Xen and how much you diverge from it.

My biggest concern is maintainability. It is fair to say that the Linux 
driver will be more exercised than the Xen driver. By keeping our own, 
we would potentially have hidden bug that may never be fixed.

By the way, I am not saying we should stick to Linux. I am happy to 
consider both solutions :).

>
> You can find IPMMU-VMSA driver here.
> repo: https://github.com/otyshchenko1/xen.git branch: ipmmu_ml
> or
> https://github.com/otyshchenko1/xen/commits/ipmmu_ml
> It is located on top of "Unshared" IOMMU patch series and consist of 6 patches.

I will have a look.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 0/9] "Non-shared" IOMMU support on ARM

[Oleksandr Tyshchenko]

On Thu, Mar 16, 2017 at 5:31 PM, Julien Grall <julien.grall@arm.com> wrote:
> On 03/15/2017 08:05 PM, Oleksandr Tyshchenko wrote:
>>
>> Hi, all.
>
>
> Hi Oleksandr,

Hi Julien

>
> Thank you for looking at the support of "non-shared" page table IOMMU
> support.
>
>>
>> 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).
>> And this IOMMU can't share the page table with the CPU since it uses
>> stage-1 page table unlike the CPU that uses stage-2 therefore I name it
>> "Non-shared" IOMMU.
>>
>> I have already raised disscusion [2] about some generic problems I had
>> faced
>> during porting IPMMU-VMSA Linux driver to Xen. And on this basis I made
>> patches you can see it in this request. Only the first patch is not
>> related to
>> IOMMU. But, I decided to ship it with the current request since it is a
>> generic change
>> which we will need in a moment.
>>
>> The reason for this patch series to be RFC is that I still have some
>> doubts about generic code I touched.
>> I hope that I haven't broken anything for x86, but confirmation is needed.
>>
>> I didn't include IPMMU-VMSA driver in this request. Although, I am still
>> in progress, I want to say
>> that passthrough use-cases (actually what this all are firstly needed for)
>> work for me with some limitations.
>> I tested on Salvator-X board (R-Car H3) with the next devices that have
>> DMA IPs
>> connected to IPMMU uTLBs (using current master branch):
>> 1. AUDMA is assigned to dom0 (protected by IOMMU)
>> 2. SDHC0 is assigned to dom1 (passthrough to domain)
>> 3. SDHC3 is assigned to dom2 (passthrough to domain)
>
>
> I am looking forward to see the support in Xen :).

Yes, It would be really great.

>
>> During porting IPMMU-VMSA driver to Xen I was trying to be as close as
>> possible to Linux [1]. But,
>> it was a little bit difficult). It would be really nice to have some
>> feedback and get your feeling regarding this driver.
>> I am also interested in if I took the right direction or there are some
>> other ideas on doing the same.
>> So, is it a right direction to move on?
>
>
> I would be interested to know what was the difficulties to port Linux driver
> in Xen and how much you diverge from it.

From my point of view the difficulty is:
The IPMMU-VMSA Linux driver relies on some Linux functional
(IOMMU/DMA/io-pgtable frameworks)
the Xen doesn't have (it is expected). So, we have to dig into them
and try to understand how they works,
how they depend on each other, how they interact with the driver.
What we should get to Xen and how they will be integrated to it, or
does the Xen have alternative mechanisms we need to reuse,
or what functional won't be used in Xen at all and should be hidden
under #if 0.
So, we have to pull some of this stuff, make stubs/wrappers to make
driver happy inside Xen.
These all lead to the driver looks complicated a bit).

>
> My biggest concern is maintainability. It is fair to say that the Linux
> driver will be more exercised than the Xen driver. By keeping our own, we
> would potentially have hidden bug that may never be fixed.
>
> By the way, I am not saying we should stick to Linux. I am happy to consider
> both solutions :).

OK, the first possible solution (let's say direct port from Linux)
already exists in github,
and while I am in progress I can continue to follow this direction.
And I think I will be able to make the second variant (where I will
drop all Linux related stuff and
modify code not to rely on it) after I finish developing. Then we can
compare both variants and choose
one.
I would like to notice that I agree with both variants despite the
complexity of the first variant.

>
>>
>> You can find IPMMU-VMSA driver here.
>> repo: https://github.com/otyshchenko1/xen.git branch: ipmmu_ml
>> or
>> https://github.com/otyshchenko1/xen/commits/ipmmu_ml
>> It is located on top of "Unshared" IOMMU patch series and consist of 6
>> patches.
>
>
> I will have a look.

Looking forward to your and other guys comment.

>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

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