[V5,7/8] iommu/arm: Introduce iommu_add_dt_device API

Commit Message

Oleksandr Tyshchenko Sept. 24, 2019, 3:30 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

The main puprose of this patch is to add a way to register DT device
(which is behind the IOMMU) using the generic IOMMU DT bindings [1]
before assigning that device to a domain.

So, this patch adds new "iommu_add_dt_device" API for adding DT device
to the IOMMU using generic IOMMU DT bindings and previously added
"iommu_fwspec" support. As devices can be assigned to the hardware domain
and other domains this function is called from two places: handle_device()
and iommu_do_dt_domctl().

Besides that, this patch adds new "dt_xlate" callback (borrowed from
Linux "of_xlate") for providing the driver with DT IOMMU specifier
which describes the IOMMU master interfaces of that device (device IDs, etc).
According to the generic IOMMU DT bindings the context of required
properties for IOMMU device/master node (#iommu-cells, iommus) depends
on many factors and is really driver depended thing.

Please note, all IOMMU drivers which support generic IOMMU DT bindings
should use "dt_xlate" and "add_device" callbacks.

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt

Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
CC: Julien Grall <[e-mail]>
CC: Jan Beulich <[e-mail]>

---
Changes V4 -> V5:
     - added "const" to struct dt_phandle_args *args in dt_xlate
     - moved iommu_add_dt_device() to xen/passthrough/device_tree.c
     - modified logic, don't try to add "all" devices to the IOMMU
       when constructing Dom0, but only devices which are going to be
       assigned to hwdom
     - updated iommu_do_dt_domctl() to call iommu_add_dt_device()
     - clarified patch description
     - removed "__init" from iommu_add_dt_device()

Changes V3 -> V4:
     - squashed with "iommu: Add of_xlate callback" patch
     - renamed "of_xlate" to "dt_xlate"
     - reworked patch description
     - clarified comments in code, removed confusing word
       "initialize device", etc
     - updated debug message in handle_device()
     - modified to check ops->of_xlate and ops->add_device
       only if "iommus" property is exists

Changes V2 -> V3:
    - clarified patch description
    - clarified comments in code
    - modified to provide DT IOMMU specifier to the driver
      using "of_xlate" callback
    - documented function usage
    - modified to return an error if ops is not present/implemented,
    - added ability to return a possitive value to indicate
      that device doesn't need to be protected
    - removed check for the "iommu" property presence
      in the common code
    - included <asm/iommu_fwspec.h> directly
---
 xen/arch/arm/domain_build.c           | 22 +++++++----
 xen/drivers/passthrough/device_tree.c | 71 +++++++++++++++++++++++++++++++++++
 xen/include/xen/iommu.h               | 21 +++++++++++
 3 files changed, 107 insertions(+), 7 deletions(-)

Comments

Julien Grall Sept. 24, 2019, 3:57 p.m. UTC | #1

Hi,

On 9/24/19 4:30 PM, Oleksandr Tyshchenko wrote:
> @@ -1263,15 +1264,22 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
>       dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
>                  dt_node_full_name(dev), need_mapping, nirq, naddr);
>   
> -    if ( dt_device_is_protected(dev) && need_mapping )
> +    if ( need_mapping )
>       {
> -        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
> -        res = iommu_assign_dt_device(d, dev);
> -        if ( res )
> -        {
> -            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
> +        dt_dprintk("Check if %s is behind the IOMMU and add it\n",
>                      dt_node_full_name(dev));
> -            return res;
> +
> +        iommu_add_dt_device(dev);

Return value should always be checked or explain why this is not done.

[...]

>   int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>   {
> @@ -177,6 +241,13 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>               break;
>           }
>   
> +        iommu_add_dt_device(dev);

Same here.

> +        if ( !dt_device_is_protected(dev) )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }

This is already checked in iommu_assign_dt_device().

Cheers,

Oleksandr Tyshchenko Sept. 24, 2019, 4:22 p.m. UTC | #2

On 24.09.19 18:57, Julien Grall wrote:
> Hi,

Hi Julien


>
> On 9/24/19 4:30 PM, Oleksandr Tyshchenko wrote:
>> @@ -1263,15 +1264,22 @@ static int __init handle_device(struct domain 
>> *d, struct dt_device_node *dev,
>>       dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
>>                  dt_node_full_name(dev), need_mapping, nirq, naddr);
>>   -    if ( dt_device_is_protected(dev) && need_mapping )
>> +    if ( need_mapping )
>>       {
>> -        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
>> -        res = iommu_assign_dt_device(d, dev);
>> -        if ( res )
>> -        {
>> -            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
>> +        dt_dprintk("Check if %s is behind the IOMMU and add it\n",
>>                      dt_node_full_name(dev));
>> -            return res;
>> +
>> +        iommu_add_dt_device(dev);
>
> Return value should always be checked or explain why this is not done.

Yes, I will add a check. The positive result for us is non-negative 
(either "device is protected" or "device doesn't need to be protected").


>
>
> [...]
>
>>   int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>   {
>> @@ -177,6 +241,13 @@ int iommu_do_dt_domctl(struct xen_domctl 
>> *domctl, struct domain *d,
>>               break;
>>           }
>>   +        iommu_add_dt_device(dev);
>
> Same here.

Yes, I think, we don't need to check for return value, because the only 
one positive result "here" is the fact that "device is protected" (which 
is checked below).

What is more, if we add a check for the return value to be strictly 0, 
we will get an error after guest's reboot (since iommu_add_dt_device() 
will return -EEXIST).

So, I will add a comment explaining why we don't check. What do you think?


>
>> +        if ( !dt_device_is_protected(dev) )
>> +        {
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>
> This is already checked in iommu_assign_dt_device().

Will drop here.

Julien Grall Sept. 24, 2019, 5:21 p.m. UTC | #3

Hi Oleksandr,

On 9/24/19 5:22 PM, Oleksandr wrote:
> 
> On 24.09.19 18:57, Julien Grall wrote:
>> Hi,
> 
> Hi Julien
> 
> 
>>
>> On 9/24/19 4:30 PM, Oleksandr Tyshchenko wrote:
>>> @@ -1263,15 +1264,22 @@ static int __init handle_device(struct domain 
>>> *d, struct dt_device_node *dev,
>>>       dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
>>>                  dt_node_full_name(dev), need_mapping, nirq, naddr);
>>>   -    if ( dt_device_is_protected(dev) && need_mapping )
>>> +    if ( need_mapping )
>>>       {
>>> -        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
>>> -        res = iommu_assign_dt_device(d, dev);
>>> -        if ( res )
>>> -        {
>>> -            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
>>> +        dt_dprintk("Check if %s is behind the IOMMU and add it\n",
>>>                      dt_node_full_name(dev));
>>> -            return res;
>>> +
>>> +        iommu_add_dt_device(dev);
>>
>> Return value should always be checked or explain why this is not done.
> 
> Yes, I will add a check. The positive result for us is non-negative 
> (either "device is protected" or "device doesn't need to be protected").
> 
> 
>>
>>
>> [...]
>>
>>>   int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>   {
>>> @@ -177,6 +241,13 @@ int iommu_do_dt_domctl(struct xen_domctl 
>>> *domctl, struct domain *d,
>>>               break;
>>>           }
>>>   +        iommu_add_dt_device(dev);
>>
>> Same here.
> 
> Yes, I think, we don't need to check for return value, because the only 
> one positive result "here" is the fact that "device is protected" (which 
> is checked below).
> 
> What is more, if we add a check for the return value to be strictly 0, 
> we will get an error after guest's reboot (since iommu_add_dt_device() 
> will return -EEXIST).
> 
> So, I will add a comment explaining why we don't check. What do you think?

Why don't you do the following code?

if ( ret < 0 && ret != -EEXIST )

This would allow the code to return the corrrect error to the upper 
layer. A suitable comment on top explaing the check would also be useful.

Cheers,

Oleksandr Tyshchenko Sept. 24, 2019, 5:30 p.m. UTC | #4

On 24.09.19 20:21, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien.


>
>>>
>>>
>>> [...]
>>>
>>>>   int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>> XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>   {
>>>> @@ -177,6 +241,13 @@ int iommu_do_dt_domctl(struct xen_domctl 
>>>> *domctl, struct domain *d,
>>>>               break;
>>>>           }
>>>>   +        iommu_add_dt_device(dev);
>>>
>>> Same here.
>>
>> Yes, I think, we don't need to check for return value, because the 
>> only one positive result "here" is the fact that "device is 
>> protected" (which is checked below).
>>
>> What is more, if we add a check for the return value to be strictly 
>> 0, we will get an error after guest's reboot (since 
>> iommu_add_dt_device() will return -EEXIST).
>>
>> So, I will add a comment explaining why we don't check. What do you 
>> think?
>
> Why don't you do the following code?
>
> if ( ret < 0 && ret != -EEXIST )
>
> This would allow the code to return the corrrect error to the upper 
> layer. A suitable comment on top explaing the check would also be useful.

Being honest, I was thinking about the similar, but rejected this. I 
thought, all what we wanted to know "here" was whether the particular 
device protected or not. But, I agree now, the upper layer should be 
informed about the exact error reason.

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a0fee1e..8048789 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1243,6 +1243,7 @@  static int __init map_device_children(struct domain *d,
  *  - Give permission to the guest to manage IRQ and MMIO range
  *  - Retrieve the IRQ configuration (i.e edge/level) from device tree
  * When the device is not marked for guest passthrough:
+ *  - Try to call iommu_add_dt_device to protect the device by an IOMMU
  *  - Assign the device to the guest if it's protected by an IOMMU
  *  - Map the IRQs and iomem regions to DOM0
  */
@@ -1263,15 +1264,22 @@  static int __init handle_device(struct domain *d, struct dt_device_node *dev,
     dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
                dt_node_full_name(dev), need_mapping, nirq, naddr);
 
-    if ( dt_device_is_protected(dev) && need_mapping )
+    if ( need_mapping )
     {
-        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
-        res = iommu_assign_dt_device(d, dev);
-        if ( res )
-        {
-            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
+        dt_dprintk("Check if %s is behind the IOMMU and add it\n",
                    dt_node_full_name(dev));
-            return res;
+
+        iommu_add_dt_device(dev);
+        if ( dt_device_is_protected(dev) )
+        {
+            dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
+            res = iommu_assign_dt_device(d, dev);
+            if ( res )
+            {
+                printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
+                       dt_node_full_name(dev));
+                return res;
+            }
         }
     }
 
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 3f328f4..0fd2488 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -22,6 +22,8 @@ 
 #include <xen/sched.h>
 #include <xsm/xsm.h>
 
+#include <asm/iommu_fwspec.h>
+
 static spinlock_t dtdevs_lock = SPIN_LOCK_UNLOCKED;
 
 int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
@@ -136,6 +138,68 @@  int iommu_release_dt_devices(struct domain *d)
     return 0;
 }
 
+int iommu_add_dt_device(struct dt_device_node *np)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    struct dt_phandle_args iommu_spec;
+    struct device *dev = dt_to_dev(np);
+    int rc = 1, index = 0;
+
+    if ( !iommu_enabled )
+        return 1;
+
+    if ( !ops )
+        return -EINVAL;
+
+    if ( dev_iommu_fwspec_get(dev) )
+        return -EEXIST;
+
+    /*
+     * According to the Documentation/devicetree/bindings/iommu/iommu.txt
+     * from Linux.
+     */
+    while ( !dt_parse_phandle_with_args(np, "iommus", "#iommu-cells",
+                                        index, &iommu_spec) )
+    {
+        /*
+         * The driver which supports generic IOMMU DT bindings must have
+         * these callback implemented.
+         */
+        if ( !ops->add_device || !ops->dt_xlate )
+            return -EINVAL;
+
+        if ( !dt_device_is_available(iommu_spec.np) )
+            break;
+
+        rc = iommu_fwspec_init(dev, &iommu_spec.np->dev);
+        if ( rc )
+            break;
+
+        /*
+         * Provide DT IOMMU specifier which describes the IOMMU master
+         * interfaces of that device (device IDs, etc) to the driver.
+         * The driver is responsible to decide how to interpret them.
+         */
+        rc = ops->dt_xlate(dev, &iommu_spec);
+        if ( rc )
+            break;
+
+        index++;
+    }
+
+    /*
+     * Add master device to the IOMMU if latter is present and available.
+     * The driver is responsible to mark that device as protected.
+     */
+    if ( !rc )
+        rc = ops->add_device(0, dev);
+
+    if ( rc < 0 )
+        iommu_fwspec_free(dev);
+
+    return rc;
+}
+
 int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
@@ -177,6 +241,13 @@  int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
             break;
         }
 
+        iommu_add_dt_device(dev);
+        if ( !dt_device_is_protected(dev) )
+        {
+            ret = -EINVAL;
+            break;
+        }
+
         ret = iommu_assign_dt_device(d, dev);
 
         if ( ret )
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index c5ed7ef..2d1cc6e 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -186,6 +186,17 @@  int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
 int iommu_dt_domain_init(struct domain *d);
 int iommu_release_dt_devices(struct domain *d);
 
+/*
+ * Helper to add master device to the IOMMU using generic IOMMU DT bindings.
+ *
+ * Return values:
+ *  0 : device is protected by an IOMMU
+ * <0 : device is not protected by an IOMMU, but must be (error condition)
+ * >0 : device doesn't need to be protected by an IOMMU
+ *      (IOMMU is not enabled/present or device is not connected to it).
+ */
+int iommu_add_dt_device(struct dt_device_node *np);
+
 int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
 
@@ -254,6 +265,16 @@  struct iommu_ops {
     int __must_check (*iotlb_flush_all)(struct domain *d);
     int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
     void (*dump_p2m_table)(struct domain *d);
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+    /*
+     * All IOMMU drivers which support generic IOMMU DT bindings should use
+     * this callback. This is a way for the framework to provide the driver
+     * with DT IOMMU specifier which describes the IOMMU master interfaces of
+     * that device (device IDs, etc).
+     */
+    int (*dt_xlate)(device_t *dev, const struct dt_phandle_args *args);
+#endif
 };
 
 #include <asm/iommu.h>