diff mbox

[Xen-devel,RFC,10/19] xen/passthrough: Introduce iommu_buildup

Message ID 1402935486-29136-11-git-send-email-julien.grall@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall June 16, 2014, 4:17 p.m. UTC 
This new function will correctly initialize the IOMMU page table for the
current domain.

Also use it in iommu_assign_dt_device even though the current IOMMU
implementation on ARM shares P2M with the processor.
---
 xen/drivers/passthrough/arm/iommu.c   |    6 ++++++
 xen/drivers/passthrough/device_tree.c |    7 +++++++
 xen/drivers/passthrough/iommu.c       |   25 +++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c         |   12 ++++--------
 xen/include/xen/iommu.h               |    2 ++
 5 files changed, 44 insertions(+), 8 deletions(-)

Comments

Ian Campbell July 3, 2014, 11:45 a.m. UTC | #1 
On Mon, 2014-06-16 at 17:17 +0100, Julien Grall wrote:
> This new function will correctly initialize the IOMMU page table for the
> current domain.

_setup, _configure, _initialise, _construct etc would be more normal
names I think.

"buildup" is the half hour of mindless blather from pundits that you
have to watch before a televised sporting event begins ;-)

> Also use it in iommu_assign_dt_device even though the current IOMMU
> implementation on ARM shares P2M with the processor.
> ---
>  xen/drivers/passthrough/arm/iommu.c   |    6 ++++++
>  xen/drivers/passthrough/device_tree.c |    7 +++++++
>  xen/drivers/passthrough/iommu.c       |   25 +++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c         |   12 ++++--------
>  xen/include/xen/iommu.h               |    2 ++
>  5 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
> index 3007b99..de4ed64 100644
> --- a/xen/drivers/passthrough/arm/iommu.c
> +++ b/xen/drivers/passthrough/arm/iommu.c
> @@ -68,3 +68,9 @@ void arch_iommu_domain_destroy(struct domain *d)
>  {
>      iommu_dt_domain_destroy(d);
>  }
> +
> +int arch_iommu_populate_page_table(struct domain *d)
> +{
> +    /* The IOMMU share the p2m with the CPU */

"shares"

> +    return -ENOSYS;
> +}
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 3e47df5..afb4dfc 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -41,6 +41,13 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>      if ( !list_empty(&dev->domain_list) )
>          goto fail;
>  
> +    if ( need_iommu(d) <= 0 )
> +    {
> +        rc = iommu_buildup(d);
> +        if ( rc )
> +            goto fail;
> +    }
> +
>      rc = hd->platform_ops->assign_dt_device(d, dev);
>  
>      if ( rc )
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index cc12735..2e9b48d 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -187,6 +187,31 @@ void iommu_teardown(struct domain *d)
>      tasklet_schedule(&iommu_pt_cleanup_tasklet);
>  }
>  
> +int iommu_buildup(struct domain *d)

Is there really not such a function already? How does x86 setup the
iommu then?

Ian.
Julien Grall July 3, 2014, 11:55 a.m. UTC | #2 
On 07/03/2014 12:45 PM, Ian Campbell wrote:
> On Mon, 2014-06-16 at 17:17 +0100, Julien Grall wrote:
>> This new function will correctly initialize the IOMMU page table for the
>> current domain.
> 
> _setup, _configure, _initialise, _construct etc would be more normal
> names I think.

I will rename to iommu_construct. The function is used to construct the
page table and set to 1 need_iommu(d).

> "buildup" is the half hour of mindless blather from pundits that you
> have to watch before a televised sporting event begins ;-)
> 
>> Also use it in iommu_assign_dt_device even though the current IOMMU
>> implementation on ARM shares P2M with the processor.
>> ---
>>  xen/drivers/passthrough/arm/iommu.c   |    6 ++++++
>>  xen/drivers/passthrough/device_tree.c |    7 +++++++
>>  xen/drivers/passthrough/iommu.c       |   25 +++++++++++++++++++++++++
>>  xen/drivers/passthrough/pci.c         |   12 ++++--------
>>  xen/include/xen/iommu.h               |    2 ++
>>  5 files changed, 44 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
>> index 3007b99..de4ed64 100644
>> --- a/xen/drivers/passthrough/arm/iommu.c
>> +++ b/xen/drivers/passthrough/arm/iommu.c
>> @@ -68,3 +68,9 @@ void arch_iommu_domain_destroy(struct domain *d)
>>  {
>>      iommu_dt_domain_destroy(d);
>>  }
>> +
>> +int arch_iommu_populate_page_table(struct domain *d)
>> +{
>> +    /* The IOMMU share the p2m with the CPU */
> 
> "shares"

Ok.

>> +    return -ENOSYS;
>> +}

[..]

>> +int iommu_buildup(struct domain *d)
> 
> Is there really not such a function already? How does x86 setup the
> iommu then?

There is a function to initialize the IOMMU but the activation and the
setup of page table are down later, I guess to avoid using memory for
nothing.

Regards,
diff mbox

Patch

diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 3007b99..de4ed64 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -68,3 +68,9 @@  void arch_iommu_domain_destroy(struct domain *d)
 {
     iommu_dt_domain_destroy(d);
 }
+
+int arch_iommu_populate_page_table(struct domain *d)
+{
+    /* The IOMMU share the p2m with the CPU */
+    return -ENOSYS;
+}
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 3e47df5..afb4dfc 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -41,6 +41,13 @@  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
     if ( !list_empty(&dev->domain_list) )
         goto fail;
 
+    if ( need_iommu(d) <= 0 )
+    {
+        rc = iommu_buildup(d);
+        if ( rc )
+            goto fail;
+    }
+
     rc = hd->platform_ops->assign_dt_device(d, dev);
 
     if ( rc )
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index cc12735..2e9b48d 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -187,6 +187,31 @@  void iommu_teardown(struct domain *d)
     tasklet_schedule(&iommu_pt_cleanup_tasklet);
 }
 
+int iommu_buildup(struct domain *d)
+{
+    int rc = 0;
+
+    /*
+     * The caller should check we effectively need to set up the IOMMMU
+     * for this domain.
+     */
+    ASSERT(need_iommu(d) <= 0);
+
+    if ( need_iommu(d) > 0 )
+        return 0;
+
+    if ( !iommu_use_hap_pt(d) )
+    {
+        rc = arch_iommu_populate_page_table(d);
+        if ( rc )
+            return rc;
+    }
+
+    d->need_iommu = 1;
+
+    return rc;
+}
+
 void iommu_domain_destroy(struct domain *d)
 {
     struct hvm_iommu *hd = domain_hvm_iommu(d);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 1eba833..e31dcfb 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1342,14 +1342,10 @@  static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
 
     if ( need_iommu(d) <= 0 )
     {
-        if ( !iommu_use_hap_pt(d) )
-        {
-            rc = arch_iommu_populate_page_table(d);
-            if ( rc )
-            {
-                spin_unlock(&pcidevs_lock);
-                return rc;
-            }
+        rc = iommu_buildup(d);
+        if ( rc ) {
+            spin_unlock(&pcidevs_lock);
+            return rc;
         }
         d->need_iommu = 1;
     }
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 8eb764a..9b2af51 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -64,6 +64,8 @@  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);
 
+int iommu_buildup(struct domain *d);
+
 /* Function used internally, use iommu_domain_destroy */
 void iommu_teardown(struct domain *d);