From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Move code for processing DT IOMMU specifier to a separate helper.
This helper will be re-used for adding PCI devices by the subsequent
patches as we will need exact the same actions for processing
DT PCI-IOMMU specifier.

While at it introduce DT_NO_IOMMU to avoid magic "1".

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v7->v8:
* explain NO_IOMMU better and rename to DT_NO_IOMMU

v6->v7:
* explained NO_IOMMU in comments

v5->v6:
* pass ops parameter to iommu_dt_xlate()
* add Julien's R-b

v4->v5:
* rebase on top of "dynamic node programming using overlay dtbo" series
* move #define NO_IOMMU 1 to header
* s/these/this/ inside comment

v3->v4:
* make dt_phandle_args *iommu_spec const
* move !ops->add_device check to helper

v2->v3:
* no change

v1->v2:
* no change

downstream->v1:
* trivial rebase
* s/dt_iommu_xlate/iommu_dt_xlate/

(cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from
 the downstream branch poc/pci-passthrough from
 https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
---
 xen/drivers/passthrough/device_tree.c | 48 +++++++++++++++++----------
 xen/include/xen/iommu.h               |  8 +++++
 2 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 075fb25a37..fe2aaef2df 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -137,6 +137,30 @@ int iommu_release_dt_devices(struct domain *d)
     return 0;
 }
 
+static int iommu_dt_xlate(struct device *dev,
+                          const struct dt_phandle_args *iommu_spec,
+                          const struct iommu_ops *ops)
+{
+    int rc;
+
+    if ( !ops->dt_xlate )
+        return -EINVAL;
+
+    if ( !dt_device_is_available(iommu_spec->np) )
+        return DT_NO_IOMMU;
+
+    rc = iommu_fwspec_init(dev, &iommu_spec->np->dev);
+    if ( rc )
+        return rc;
+
+    /*
+     * 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.
+     */
+    return ops->dt_xlate(dev, iommu_spec);
+}
+
 int iommu_remove_dt_device(struct dt_device_node *np)
 {
     const struct iommu_ops *ops = iommu_get_ops();
@@ -146,7 +170,7 @@ int iommu_remove_dt_device(struct dt_device_node *np)
     ASSERT(rw_is_locked(&dt_host_lock));
 
     if ( !iommu_enabled )
-        return 1;
+        return DT_NO_IOMMU;
 
     if ( !ops )
         return -EOPNOTSUPP;
@@ -187,12 +211,12 @@ 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;
+    int rc = DT_NO_IOMMU, index = 0;
 
     ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock));
 
     if ( !iommu_enabled )
-        return 1;
+        return DT_NO_IOMMU;
 
     if ( !ops )
         return -EINVAL;
@@ -215,27 +239,15 @@ int iommu_add_dt_device(struct dt_device_node *np)
     {
         /*
          * The driver which supports generic IOMMU DT bindings must have
-         * these callback implemented.
+         * this callback implemented.
          */
-        if ( !ops->add_device || !ops->dt_xlate )
+        if ( !ops->add_device )
         {
             rc = -EINVAL;
             goto fail;
         }
 
-        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);
+        rc = iommu_dt_xlate(dev, &iommu_spec, ops);
         if ( rc )
             break;
 
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index b928c67e19..2b6e6e8a3f 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -238,6 +238,14 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
  */
 int iommu_remove_dt_device(struct dt_device_node *np);
 
+/*
+ * Status code indicating that DT device cannot be added to the IOMMU
+ * or removed from it because the IOMMU is disabled or the device is not
+ * connected to it.
+ */
+
+#define DT_NO_IOMMU    1
+
 #endif /* HAS_DEVICE_TREE */
 
 struct page_info;
-- 
2.34.1

On 10.02.2025 11:30, Mykyta Poturai wrote:
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -238,6 +238,14 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>   */
>  int iommu_remove_dt_device(struct dt_device_node *np);
>  
> +/*
> + * Status code indicating that DT device cannot be added to the IOMMU
> + * or removed from it because the IOMMU is disabled or the device is not
> + * connected to it.
> + */
> +
> +#define DT_NO_IOMMU    1

While an improvement, it still isn't clear whose "status code" this is.
The #define is effectively hanging in the air, from all I can tell. And
from it not being a normal error code it is pretty clear that it better
would have only very narrow use.

Also can you please omit an interspersing blank line when the comment
is specifically tied to a definition or declaration?

Jan

On 10.02.25 12:46, Jan Beulich wrote:
> On 10.02.2025 11:30, Mykyta Poturai wrote:
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -238,6 +238,14 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>    */
>>   int iommu_remove_dt_device(struct dt_device_node *np);
>>   
>> +/*
>> + * Status code indicating that DT device cannot be added to the IOMMU
>> + * or removed from it because the IOMMU is disabled or the device is not
>> + * connected to it.
>> + */
>> +
>> +#define DT_NO_IOMMU    1
> 
> While an improvement, it still isn't clear whose "status code" this is.
> The #define is effectively hanging in the air, from all I can tell. And
> from it not being a normal error code it is pretty clear that it better
> would have only very narrow use.
> 
> Also can you please omit an interspersing blank line when the comment
> is specifically tied to a definition or declaration?
> 
> Jan

Hi Jan

What would you say about removing this status code entirely and 
returning something like -ENODEV instead, with adding special handling 
for this return to the callers where needed?

-- 
Mykyta

On 25.02.2025 12:05, Mykyta Poturai wrote:
> On 10.02.25 12:46, Jan Beulich wrote:
>> On 10.02.2025 11:30, Mykyta Poturai wrote:
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -238,6 +238,14 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>    */
>>>   int iommu_remove_dt_device(struct dt_device_node *np);
>>>   
>>> +/*
>>> + * Status code indicating that DT device cannot be added to the IOMMU
>>> + * or removed from it because the IOMMU is disabled or the device is not
>>> + * connected to it.
>>> + */
>>> +
>>> +#define DT_NO_IOMMU    1
>>
>> While an improvement, it still isn't clear whose "status code" this is.
>> The #define is effectively hanging in the air, from all I can tell. And
>> from it not being a normal error code it is pretty clear that it better
>> would have only very narrow use.
>>
>> Also can you please omit an interspersing blank line when the comment
>> is specifically tied to a definition or declaration?
> 
> What would you say about removing this status code entirely and 
> returning something like -ENODEV instead, with adding special handling 
> for this return to the callers where needed?

I'd be okay with that; Arm folks also need to be, though.

Jan

On 25.02.2025 12:08, Jan Beulich wrote:
> On 25.02.2025 12:05, Mykyta Poturai wrote:
>> On 10.02.25 12:46, Jan Beulich wrote:
>>> On 10.02.2025 11:30, Mykyta Poturai wrote:
>>>> --- a/xen/include/xen/iommu.h
>>>> +++ b/xen/include/xen/iommu.h
>>>> @@ -238,6 +238,14 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>    */
>>>>   int iommu_remove_dt_device(struct dt_device_node *np);
>>>>   
>>>> +/*
>>>> + * Status code indicating that DT device cannot be added to the IOMMU
>>>> + * or removed from it because the IOMMU is disabled or the device is not
>>>> + * connected to it.
>>>> + */
>>>> +
>>>> +#define DT_NO_IOMMU    1
>>>
>>> While an improvement, it still isn't clear whose "status code" this is.
>>> The #define is effectively hanging in the air, from all I can tell. And
>>> from it not being a normal error code it is pretty clear that it better
>>> would have only very narrow use.
>>>
>>> Also can you please omit an interspersing blank line when the comment
>>> is specifically tied to a definition or declaration?
>>
>> What would you say about removing this status code entirely and 
>> returning something like -ENODEV instead, with adding special handling 
>> for this return to the callers where needed?
> 
> I'd be okay with that; Arm folks also need to be, though.

Oh, and: Of course it then needs to be pretty clear / obvious that -ENODEV
cannot come into play for other reasons / from other origins.

Jan

Hi,

On 25/02/2025 11:10, Jan Beulich wrote:
> On 25.02.2025 12:08, Jan Beulich wrote:
>> On 25.02.2025 12:05, Mykyta Poturai wrote:
>>> On 10.02.25 12:46, Jan Beulich wrote:
>>>> On 10.02.2025 11:30, Mykyta Poturai wrote:
>>>>> --- a/xen/include/xen/iommu.h
>>>>> +++ b/xen/include/xen/iommu.h
>>>>> @@ -238,6 +238,14 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>     */
>>>>>    int iommu_remove_dt_device(struct dt_device_node *np);
>>>>>    
>>>>> +/*
>>>>> + * Status code indicating that DT device cannot be added to the IOMMU
>>>>> + * or removed from it because the IOMMU is disabled or the device is not
>>>>> + * connected to it.
>>>>> + */
>>>>> +
>>>>> +#define DT_NO_IOMMU    1
>>>>
>>>> While an improvement, it still isn't clear whose "status code" this is.
>>>> The #define is effectively hanging in the air, from all I can tell. And
>>>> from it not being a normal error code it is pretty clear that it better
>>>> would have only very narrow use.
>>>>
>>>> Also can you please omit an interspersing blank line when the comment
>>>> is specifically tied to a definition or declaration?
>>>
>>> What would you say about removing this status code entirely and
>>> returning something like -ENODEV instead, with adding special handling
>>> for this return to the callers where needed?
>>
>> I'd be okay with that; Arm folks also need to be, though.
> 
> Oh, and: Of course it then needs to be pretty clear / obvious that -ENODEV
> cannot come into play for other reasons / from other origins.

It would be difficult to guarantee that all the callbacks will never 
return -ENODEV. So I am quite reluctant to use -ENODEV to indicate the 
IOMMU is not available or the device is not behind an IOMMU.

Anyway, I can't fully remember the previous discussion. Can someone 
remind me what we are trying to solve with introducing DT_NO_IOMMU? The 
meaning of the value is already properly documented on each function 
that can return the value:

* >0 : device doesn't need to be protected by an IOMMU
*      (IOMMU is not enabled/present or device is not connected to it).

It seems to me it would be easier to open-code the value because there 
is no question of how the define is going to be used.

Cheers,

-- 
Julien Grall