[v4] xen/arm: scmi: introduce SCI SCMI SMC multi-agent support

[RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 5 months, 2 weeks ago

From: Grygorii Strashko <grygorii_strashko@epam.com>

Add chained handling of assigned DT devices to support access-controller
functionality through SCI framework, so DT device assign request can be
passed to FW for processing and enabling VM access to requested device
(for example, device power management through FW interface like SCMI).

The SCI access-controller DT device processing is chained after IOMMU
processing and expected to be executed for any DT device regardless of its
protection by IOMMU (or if IOMMU is disabled).

This allows to pass not only IOMMU protected DT device through
xl.cfg:"dtdev" property for processing:

dtdev = [
    "/soc/video@e6ef0000", <- IOMMU protected device
    "/soc/i2c@e6508000", <- not IOMMU protected device
]

The change is done in two parts:
1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
not fail if DT device is not protected by IOMMU
2) add chained call to sci_do_domctl() in do_domctl()

Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
---



 xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
 xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
 xen/common/domctl.c                     | 19 +++++++++++++
 xen/drivers/passthrough/device_tree.c   |  6 ++++
 4 files changed, 76 insertions(+)

diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
index e1522e10e2..8efd541c4f 100644
--- a/xen/arch/arm/firmware/sci.c
+++ b/xen/arch/arm/firmware/sci.c
@@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
     return 0;
 }
 
+int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
+                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+    struct dt_device_node *dev;
+    int ret = 0;
+
+    switch ( domctl->cmd )
+    {
+    case XEN_DOMCTL_assign_device:
+        ret = -EOPNOTSUPP;
+        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
+            break;
+
+        if ( !cur_mediator )
+            break;
+
+        if ( !cur_mediator->assign_dt_device )
+            break;
+
+        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
+                                    domctl->u.assign_device.u.dt.size, &dev);
+        if ( ret )
+            return ret;
+
+        ret = sci_assign_dt_device(d, dev);
+        if ( ret )
+            break;
+
+        break;
+    default:
+        /* do not fail here as call is chained with iommu handling */
+        break;
+    }
+
+    return ret;
+}
+
 static int __init sci_init(void)
 {
     struct dt_device_node *np;
diff --git a/xen/arch/arm/include/asm/firmware/sci.h b/xen/arch/arm/include/asm/firmware/sci.h
index 71fb54852e..b8d1bc8a62 100644
--- a/xen/arch/arm/include/asm/firmware/sci.h
+++ b/xen/arch/arm/include/asm/firmware/sci.h
@@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
  * control" functionality.
  */
 int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
+
+/*
+ * SCI domctl handler
+ *
+ * Only XEN_DOMCTL_assign_device is handled for now.
+ */
+int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
+                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
 #else
 
 static inline bool sci_domain_is_enabled(struct domain *d)
@@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
     return 0;
 }
 
+static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
+                                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+    return 0;
+}
+
 #endif /* CONFIG_ARM_SCI */
 
 #endif /* __ASM_ARM_SCI_H */
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 05abb581a0..a74ee92067 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -27,6 +27,7 @@
 #include <xen/vm_event.h>
 #include <xen/monitor.h>
 #include <asm/current.h>
+#include <asm/firmware/sci.h>
 #include <asm/irq.h>
 #include <asm/page.h>
 #include <asm/p2m.h>
@@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     case XEN_DOMCTL_deassign_device:
     case XEN_DOMCTL_get_device_group:
         ret = iommu_do_domctl(op, d, u_domctl);
+
+        if ( !ret || ret == -EOPNOTSUPP )
+        {
+            int ret1;
+            /*
+             * Add chained handling of assigned DT devices to support
+             * access-controller functionality through SCI framework, so
+             * DT device assign request can be passed to FW for processing and
+             * enabling VM access to requested device.
+             * The access-controller DT device processing is chained after IOMMU
+             * processing and expected to be executed for any DT device
+             * regardless if DT device is protected by IOMMU or not (or IOMMU
+             * is disabled).
+             */
+            ret1 = sci_do_domctl(op, d, u_domctl);
+            if ( ret1 != -EOPNOTSUPP )
+                ret = ret1;
+        }
         break;
 
     case XEN_DOMCTL_get_paging_mempool_size:
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 075fb25a37..2624767e51 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -318,6 +318,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
             break;
         }
 
+        if ( !dt_device_is_protected(dev) )
+        {
+            ret = 0;
+            break;
+        }
+
         ret = iommu_assign_dt_device(d, dev);
 
         if ( ret )
-- 
2.34.1

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Stefano Stabellini 5 months, 1 week ago

On Mon, 19 May 2025, Oleksii Moisieiev wrote:
> From: Grygorii Strashko <grygorii_strashko@epam.com>
> 
> Add chained handling of assigned DT devices to support access-controller
> functionality through SCI framework, so DT device assign request can be
> passed to FW for processing and enabling VM access to requested device
> (for example, device power management through FW interface like SCMI).
> 
> The SCI access-controller DT device processing is chained after IOMMU
> processing and expected to be executed for any DT device regardless of its
> protection by IOMMU (or if IOMMU is disabled).
> 
> This allows to pass not only IOMMU protected DT device through
> xl.cfg:"dtdev" property for processing:
> 
> dtdev = [
>     "/soc/video@e6ef0000", <- IOMMU protected device
>     "/soc/i2c@e6508000", <- not IOMMU protected device
> ]
> 
> The change is done in two parts:
> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
> not fail if DT device is not protected by IOMMU
> 2) add chained call to sci_do_domctl() in do_domctl()
> 
> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> ---
> 
> 
> 
>  xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>  xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>  xen/common/domctl.c                     | 19 +++++++++++++
>  xen/drivers/passthrough/device_tree.c   |  6 ++++
>  4 files changed, 76 insertions(+)
> 
> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
> index e1522e10e2..8efd541c4f 100644
> --- a/xen/arch/arm/firmware/sci.c
> +++ b/xen/arch/arm/firmware/sci.c
> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>      return 0;
>  }
>  
> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    struct dt_device_node *dev;
> +    int ret = 0;
> +
> +    switch ( domctl->cmd )
> +    {
> +    case XEN_DOMCTL_assign_device:
> +        ret = -EOPNOTSUPP;

Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?


> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> +            break;

this one

> +        if ( !cur_mediator )
> +            break;

this one

> +        if ( !cur_mediator->assign_dt_device )
> +            break;

and also this one? It seems more like an -EINVAL as the caller used a
wrong parameter?


> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
> +                                    domctl->u.assign_device.u.dt.size, &dev);
> +        if ( ret )
> +            return ret;
> +
> +        ret = sci_assign_dt_device(d, dev);
> +        if ( ret )
> +            break;
> +
> +        break;
> +    default:
> +        /* do not fail here as call is chained with iommu handling */

It looks like this should be an error


> +        break;
> +    }
> +
> +    return ret;
> +}
> +
>  static int __init sci_init(void)
>  {
>      struct dt_device_node *np;
> diff --git a/xen/arch/arm/include/asm/firmware/sci.h b/xen/arch/arm/include/asm/firmware/sci.h
> index 71fb54852e..b8d1bc8a62 100644
> --- a/xen/arch/arm/include/asm/firmware/sci.h
> +++ b/xen/arch/arm/include/asm/firmware/sci.h
> @@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
>   * control" functionality.
>   */
>  int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
> +
> +/*
> + * SCI domctl handler
> + *
> + * Only XEN_DOMCTL_assign_device is handled for now.
> + */
> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>  #else
>  
>  static inline bool sci_domain_is_enabled(struct domain *d)
> @@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
>      return 0;
>  }
>  
> +static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    return 0;
> +}
> +
>  #endif /* CONFIG_ARM_SCI */
>  
>  #endif /* __ASM_ARM_SCI_H */
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 05abb581a0..a74ee92067 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -27,6 +27,7 @@
>  #include <xen/vm_event.h>
>  #include <xen/monitor.h>
>  #include <asm/current.h>
> +#include <asm/firmware/sci.h>
>  #include <asm/irq.h>
>  #include <asm/page.h>
>  #include <asm/p2m.h>
> @@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      case XEN_DOMCTL_deassign_device:
>      case XEN_DOMCTL_get_device_group:
>          ret = iommu_do_domctl(op, d, u_domctl);
> +
> +        if ( !ret || ret == -EOPNOTSUPP )

It is better to invert the check:

if ( ret < 0 && ret != -EOPNOTSUPP )
    return ret;


> +        {
> +            int ret1;
> +            /*
> +             * Add chained handling of assigned DT devices to support
> +             * access-controller functionality through SCI framework, so
> +             * DT device assign request can be passed to FW for processing and
> +             * enabling VM access to requested device.
> +             * The access-controller DT device processing is chained after IOMMU
> +             * processing and expected to be executed for any DT device
> +             * regardless if DT device is protected by IOMMU or not (or IOMMU
> +             * is disabled).
> +             */
> +            ret1 = sci_do_domctl(op, d, u_domctl);
> +            if ( ret1 != -EOPNOTSUPP )
> +                ret = ret1;
> +        }
>          break;
>  
>      case XEN_DOMCTL_get_paging_mempool_size:
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 075fb25a37..2624767e51 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -318,6 +318,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>              break;
>          }
>  
> +        if ( !dt_device_is_protected(dev) )
> +        {
> +            ret = 0;
> +            break;
> +        }

I am concerned about this: previously we would call
iommu_assign_dt_device and the same check at the beginning of
iommu_assign_dt_device would return -EINVAL. Now it is a success.

I am not sure this is appropriate. I wonder if instead we should:

- remove this chunk from the patch
- change the return error for !dt_device_is_protected at the top of
  iommu_assign_dt_device from -EINVAL to -EOPNOTSUPP
- this would fall into the same ret != -EOPNOTSUPP check after
  iommu_do_domctl


>          ret = iommu_assign_dt_device(d, dev);
>  
>          if ( ret )
> -- 
> 2.34.1
>

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 4 months, 2 weeks ago

Hi Stefano,

I'm very sorry for a long silence. Please see my answers below:

On 22/05/2025 03:25, Stefano Stabellini wrote:
> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>> From: Grygorii Strashko<grygorii_strashko@epam.com>
>>
>> Add chained handling of assigned DT devices to support access-controller
>> functionality through SCI framework, so DT device assign request can be
>> passed to FW for processing and enabling VM access to requested device
>> (for example, device power management through FW interface like SCMI).
>>
>> The SCI access-controller DT device processing is chained after IOMMU
>> processing and expected to be executed for any DT device regardless of its
>> protection by IOMMU (or if IOMMU is disabled).
>>
>> This allows to pass not only IOMMU protected DT device through
>> xl.cfg:"dtdev" property for processing:
>>
>> dtdev = [
>>      "/soc/video@e6ef0000", <- IOMMU protected device
>>      "/soc/i2c@e6508000", <- not IOMMU protected device
>> ]
>>
>> The change is done in two parts:
>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>> not fail if DT device is not protected by IOMMU
>> 2) add chained call to sci_do_domctl() in do_domctl()
>>
>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
>> ---
>>
>>
>>
>>   xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>>   xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>   xen/common/domctl.c                     | 19 +++++++++++++
>>   xen/drivers/passthrough/device_tree.c   |  6 ++++
>>   4 files changed, 76 insertions(+)
>>
>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>> index e1522e10e2..8efd541c4f 100644
>> --- a/xen/arch/arm/firmware/sci.c
>> +++ b/xen/arch/arm/firmware/sci.c
>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>       return 0;
>>   }
>>   
>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>> +{
>> +    struct dt_device_node *dev;
>> +    int ret = 0;
>> +
>> +    switch ( domctl->cmd )
>> +    {
>> +    case XEN_DOMCTL_assign_device:
>> +        ret = -EOPNOTSUPP;
> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?

The -EOPNOTSUPP code is used because this is part of a chained call after
iommu_do_domctl, as stated in xen/common/domctl.c:859. The 
XEN_DOMCTL_assign_device
call is expected to handle any DT device, regardless of whether the DT 
device is
protected by an IOMMU or if the IOMMU is disabled.
The following cases are considered:

1. IOMMU Protected Device (Success)

If the device is protected by the IOMMU and iommu_do_domctl returns 0, 
we continue
processing the DT device by calling sci_do_domctl.

2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)

If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is 
disabled,
we still proceed to call sci_do_domctl.

3. Error from iommu_do_domctl (Fail State)

If iommu_do_domctl returns any error, the system enters a fail state, and
sci_do_domctl is not called.

4. -EOPNOTSUPP from sci_do_domctl

If sci_do_domctl returns -EOPNOTSUPP, this indicates one of the following:
- The provided device is not a DT device.
- There is no cur_mediator available (indicating that the SCI subsystem 
is enabled
in the configuration, but no mediator was provided).
- The current mediator does not support assign_dt_device (this is 
expected to be changed;
see below for details).
In this case, -EOPNOTSUPP is returned but will be ignored, and the 
original return value from iommu_do_domctl will be used as the final result.

5. Return Code from sci_do_domctl

If sci_do_domctl returns 0 (success) or an error code (failure),
the return value from iommu_do_domctl is overridden, and the result from 
sci_do_domctl is returned.
Note: -EOPNOTSUPP from iommu_do_domctl will also be overridden since
step 2 was successfully completed (or failed).
>> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
>> +            break;
> this one
>
>> +        if ( !cur_mediator )
>> +            break;
> this one
>
>> +        if ( !cur_mediator->assign_dt_device )
>> +            break;
> and also this one? It seems more like an -EINVAL as the caller used a
> wrong parameter?

I think you are right that this case should return -EINVAL because we 
should fail if mediator

without implemented mandatory features was provided. Will be fixed.

>> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>> +                                    domctl->u.assign_device.u.dt.size, &dev);
>> +        if ( ret )
>> +            return ret;
>> +
>> +        ret = sci_assign_dt_device(d, dev);
>> +        if ( ret )
>> +            break;
>> +
>> +        break;
>> +    default:
>> +        /* do not fail here as call is chained with iommu handling */
> It looks like this should be an error
>
>
>> +        break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static int __init sci_init(void)
>>   {
>>       struct dt_device_node *np;
>> diff --git a/xen/arch/arm/include/asm/firmware/sci.h b/xen/arch/arm/include/asm/firmware/sci.h
>> index 71fb54852e..b8d1bc8a62 100644
>> --- a/xen/arch/arm/include/asm/firmware/sci.h
>> +++ b/xen/arch/arm/include/asm/firmware/sci.h
>> @@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
>>    * control" functionality.
>>    */
>>   int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>> +
>> +/*
>> + * SCI domctl handler
>> + *
>> + * Only XEN_DOMCTL_assign_device is handled for now.
>> + */
>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>   #else
>>   
>>   static inline bool sci_domain_is_enabled(struct domain *d)
>> @@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
>>       return 0;
>>   }
>>   
>> +static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> +                                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>> +{
>> +    return 0;
>> +}
>> +
>>   #endif /* CONFIG_ARM_SCI */
>>   
>>   #endif /* __ASM_ARM_SCI_H */
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 05abb581a0..a74ee92067 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -27,6 +27,7 @@
>>   #include <xen/vm_event.h>
>>   #include <xen/monitor.h>
>>   #include <asm/current.h>
>> +#include <asm/firmware/sci.h>
>>   #include <asm/irq.h>
>>   #include <asm/page.h>
>>   #include <asm/p2m.h>
>> @@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>       case XEN_DOMCTL_deassign_device:
>>       case XEN_DOMCTL_get_device_group:
>>           ret = iommu_do_domctl(op, d, u_domctl);
>> +
>> +        if ( !ret || ret == -EOPNOTSUPP )
> It is better to invert the check:
>
> if ( ret < 0 && ret != -EOPNOTSUPP )
>      return ret;
+
>> +        {
>> +            int ret1;
>> +            /*
>> +             * Add chained handling of assigned DT devices to support
>> +             * access-controller functionality through SCI framework, so
>> +             * DT device assign request can be passed to FW for processing and
>> +             * enabling VM access to requested device.
>> +             * The access-controller DT device processing is chained after IOMMU
>> +             * processing and expected to be executed for any DT device
>> +             * regardless if DT device is protected by IOMMU or not (or IOMMU
>> +             * is disabled).
>> +             */
>> +            ret1 = sci_do_domctl(op, d, u_domctl);
>> +            if ( ret1 != -EOPNOTSUPP )
>> +                ret = ret1;
>> +        }
>>           break;
>>   
>>       case XEN_DOMCTL_get_paging_mempool_size:
>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>> index 075fb25a37..2624767e51 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -318,6 +318,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>               break;
>>           }
>>   
>> +        if ( !dt_device_is_protected(dev) )
>> +        {
>> +            ret = 0;
>> +            break;
>> +        }
> I am concerned about this: previously we would call
> iommu_assign_dt_device and the same check at the beginning of
> iommu_assign_dt_device would return -EINVAL. Now it is a success.
>
> I am not sure this is appropriate. I wonder if instead we should:
>
> - remove this chunk from the patch
> - change the return error for !dt_device_is_protected at the top of
>    iommu_assign_dt_device from -EINVAL to -EOPNOTSUPP
> - this would fall into the same ret != -EOPNOTSUPP check after
>    iommu_do_domctl

That's a good point. I think we should do the same for

 > if ( !is_iommu_enabled(d) )

 >  return -EINVAL;

because in this case we should process sci as well. I will do the change

>>           ret = iommu_assign_dt_device(d, dev);
>>   
>>           if ( ret )
>> -- 
>> 2.34.1
>>

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Stefano Stabellini 4 months, 2 weeks ago

On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
> Hi Stefano,
> 
> I'm very sorry for a long silence. Please see my answers below:
> 
> On 22/05/2025 03:25, Stefano Stabellini wrote:
> > On Mon, 19 May 2025, Oleksii Moisieiev wrote:
> >> From: Grygorii Strashko<grygorii_strashko@epam.com>
> >>
> >> Add chained handling of assigned DT devices to support access-controller
> >> functionality through SCI framework, so DT device assign request can be
> >> passed to FW for processing and enabling VM access to requested device
> >> (for example, device power management through FW interface like SCMI).
> >>
> >> The SCI access-controller DT device processing is chained after IOMMU
> >> processing and expected to be executed for any DT device regardless of its
> >> protection by IOMMU (or if IOMMU is disabled).
> >>
> >> This allows to pass not only IOMMU protected DT device through
> >> xl.cfg:"dtdev" property for processing:
> >>
> >> dtdev = [
> >>      "/soc/video@e6ef0000", <- IOMMU protected device
> >>      "/soc/i2c@e6508000", <- not IOMMU protected device
> >> ]
> >>
> >> The change is done in two parts:
> >> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
> >> not fail if DT device is not protected by IOMMU
> >> 2) add chained call to sci_do_domctl() in do_domctl()
> >>
> >> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
> >> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
> >> ---
> >>
> >>
> >>
> >>   xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
> >>   xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
> >>   xen/common/domctl.c                     | 19 +++++++++++++
> >>   xen/drivers/passthrough/device_tree.c   |  6 ++++
> >>   4 files changed, 76 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
> >> index e1522e10e2..8efd541c4f 100644
> >> --- a/xen/arch/arm/firmware/sci.c
> >> +++ b/xen/arch/arm/firmware/sci.c
> >> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
> >>       return 0;
> >>   }
> >>   
> >> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> >> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >> +{
> >> +    struct dt_device_node *dev;
> >> +    int ret = 0;
> >> +
> >> +    switch ( domctl->cmd )
> >> +    {
> >> +    case XEN_DOMCTL_assign_device:
> >> +        ret = -EOPNOTSUPP;
> > Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
> 
> The -EOPNOTSUPP code is used because this is part of a chained call after
> iommu_do_domctl, as stated in xen/common/domctl.c:859. The 
> XEN_DOMCTL_assign_device
> call is expected to handle any DT device, regardless of whether the DT 
> device is
> protected by an IOMMU or if the IOMMU is disabled.
> The following cases are considered:
> 
> 1. IOMMU Protected Device (Success)
> 
> If the device is protected by the IOMMU and iommu_do_domctl returns 0, 
> we continue
> processing the DT device by calling sci_do_domctl.
> 
> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
> 
> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is 
> disabled,
> we still proceed to call sci_do_domctl.

OK this makes sense.  I think it is OK to have a special error code to
say "the IOMMU is disabled" but I don't know if it is a good idea to try
to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
configuration with domctl disabled, for instance.

It might be wiser to use a different error code. Maybe ENOENT?


> 3. Error from iommu_do_domctl (Fail State)
> 
> If iommu_do_domctl returns any error, the system enters a fail state, and
> sci_do_domctl is not called.
> 
> 4. -EOPNOTSUPP from sci_do_domctl
> 
> If sci_do_domctl returns -EOPNOTSUPP, this indicates one of the following:
> - The provided device is not a DT device.
> - There is no cur_mediator available (indicating that the SCI subsystem 
> is enabled
> in the configuration, but no mediator was provided).
> - The current mediator does not support assign_dt_device (this is 
> expected to be changed;
> see below for details).
> In this case, -EOPNOTSUPP is returned but will be ignored, and the 
> original return value from iommu_do_domctl will be used as the final result.

Same comment as before. We need to be careful not confuse this case you
described with other cases where sci_do_domctl is simply not
implemented.


> 5. Return Code from sci_do_domctl
> 
> If sci_do_domctl returns 0 (success) or an error code (failure),
> the return value from iommu_do_domctl is overridden, and the result from 
> sci_do_domctl is returned.
> Note: -EOPNOTSUPP from iommu_do_domctl will also be overridden since
> step 2 was successfully completed (or failed).
> >> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> >> +            break;
> > this one
> >
> >> +        if ( !cur_mediator )
> >> +            break;
> > this one
> >
> >> +        if ( !cur_mediator->assign_dt_device )
> >> +            break;
> > and also this one? It seems more like an -EINVAL as the caller used a
> > wrong parameter?
> 
> I think you are right that this case should return -EINVAL because we 
> should fail if mediator
> 
> without implemented mandatory features was provided. Will be fixed.
> 
> >> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
> >> +                                    domctl->u.assign_device.u.dt.size, &dev);
> >> +        if ( ret )
> >> +            return ret;
> >> +
> >> +        ret = sci_assign_dt_device(d, dev);
> >> +        if ( ret )
> >> +            break;
> >> +
> >> +        break;
> >> +    default:
> >> +        /* do not fail here as call is chained with iommu handling */
> > It looks like this should be an error
> >
> >
> >> +        break;
> >> +    }
> >> +
> >> +    return ret;
> >> +}
> >> +
> >>   static int __init sci_init(void)
> >>   {
> >>       struct dt_device_node *np;
> >> diff --git a/xen/arch/arm/include/asm/firmware/sci.h b/xen/arch/arm/include/asm/firmware/sci.h
> >> index 71fb54852e..b8d1bc8a62 100644
> >> --- a/xen/arch/arm/include/asm/firmware/sci.h
> >> +++ b/xen/arch/arm/include/asm/firmware/sci.h
> >> @@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
> >>    * control" functionality.
> >>    */
> >>   int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
> >> +
> >> +/*
> >> + * SCI domctl handler
> >> + *
> >> + * Only XEN_DOMCTL_assign_device is handled for now.
> >> + */
> >> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> >> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
> >>   #else
> >>   
> >>   static inline bool sci_domain_is_enabled(struct domain *d)
> >> @@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
> >>       return 0;
> >>   }
> >>   
> >> +static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> >> +                                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >> +{
> >> +    return 0;
> >> +}
> >> +
> >>   #endif /* CONFIG_ARM_SCI */
> >>   
> >>   #endif /* __ASM_ARM_SCI_H */
> >> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> >> index 05abb581a0..a74ee92067 100644
> >> --- a/xen/common/domctl.c
> >> +++ b/xen/common/domctl.c
> >> @@ -27,6 +27,7 @@
> >>   #include <xen/vm_event.h>
> >>   #include <xen/monitor.h>
> >>   #include <asm/current.h>
> >> +#include <asm/firmware/sci.h>
> >>   #include <asm/irq.h>
> >>   #include <asm/page.h>
> >>   #include <asm/p2m.h>
> >> @@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >>       case XEN_DOMCTL_deassign_device:
> >>       case XEN_DOMCTL_get_device_group:
> >>           ret = iommu_do_domctl(op, d, u_domctl);
> >> +
> >> +        if ( !ret || ret == -EOPNOTSUPP )
> > It is better to invert the check:
> >
> > if ( ret < 0 && ret != -EOPNOTSUPP )
> >      return ret;
> +
> >> +        {
> >> +            int ret1;
> >> +            /*
> >> +             * Add chained handling of assigned DT devices to support
> >> +             * access-controller functionality through SCI framework, so
> >> +             * DT device assign request can be passed to FW for processing and
> >> +             * enabling VM access to requested device.
> >> +             * The access-controller DT device processing is chained after IOMMU
> >> +             * processing and expected to be executed for any DT device
> >> +             * regardless if DT device is protected by IOMMU or not (or IOMMU
> >> +             * is disabled).
> >> +             */
> >> +            ret1 = sci_do_domctl(op, d, u_domctl);
> >> +            if ( ret1 != -EOPNOTSUPP )
> >> +                ret = ret1;
> >> +        }
> >>           break;
> >>   
> >>       case XEN_DOMCTL_get_paging_mempool_size:
> >> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> >> index 075fb25a37..2624767e51 100644
> >> --- a/xen/drivers/passthrough/device_tree.c
> >> +++ b/xen/drivers/passthrough/device_tree.c
> >> @@ -318,6 +318,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> >>               break;
> >>           }
> >>   
> >> +        if ( !dt_device_is_protected(dev) )
> >> +        {
> >> +            ret = 0;
> >> +            break;
> >> +        }
> > I am concerned about this: previously we would call
> > iommu_assign_dt_device and the same check at the beginning of
> > iommu_assign_dt_device would return -EINVAL. Now it is a success.
> >
> > I am not sure this is appropriate. I wonder if instead we should:
> >
> > - remove this chunk from the patch
> > - change the return error for !dt_device_is_protected at the top of
> >    iommu_assign_dt_device from -EINVAL to -EOPNOTSUPP
> > - this would fall into the same ret != -EOPNOTSUPP check after
> >    iommu_do_domctl
> 
> That's a good point. I think we should do the same for
> 
>  > if ( !is_iommu_enabled(d) )
> 
>  >  return -EINVAL;
> 
> because in this case we should process sci as well. I will do the change
> 
> >>           ret = iommu_assign_dt_device(d, dev);
> >>   
> >>           if ( ret )
> >> -- 
> >> 2.34.1
> >>

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 4 months, 1 week ago

On 18/06/2025 03:04, Stefano Stabellini wrote:
> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>> Hi Stefano,
>>
>> I'm very sorry for a long silence. Please see my answers below:
>>
>> On 22/05/2025 03:25, Stefano Stabellini wrote:
>>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>>>> From: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>
>>>> Add chained handling of assigned DT devices to support access-controller
>>>> functionality through SCI framework, so DT device assign request can be
>>>> passed to FW for processing and enabling VM access to requested device
>>>> (for example, device power management through FW interface like SCMI).
>>>>
>>>> The SCI access-controller DT device processing is chained after IOMMU
>>>> processing and expected to be executed for any DT device regardless of its
>>>> protection by IOMMU (or if IOMMU is disabled).
>>>>
>>>> This allows to pass not only IOMMU protected DT device through
>>>> xl.cfg:"dtdev" property for processing:
>>>>
>>>> dtdev = [
>>>>       "/soc/video@e6ef0000", <- IOMMU protected device
>>>>       "/soc/i2c@e6508000", <- not IOMMU protected device
>>>> ]
>>>>
>>>> The change is done in two parts:
>>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>>>> not fail if DT device is not protected by IOMMU
>>>> 2) add chained call to sci_do_domctl() in do_domctl()
>>>>
>>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
>>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
>>>> ---
>>>>
>>>>
>>>>
>>>>    xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>>>>    xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>>>    xen/common/domctl.c                     | 19 +++++++++++++
>>>>    xen/drivers/passthrough/device_tree.c   |  6 ++++
>>>>    4 files changed, 76 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>> index e1522e10e2..8efd541c4f 100644
>>>> --- a/xen/arch/arm/firmware/sci.c
>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>        return 0;
>>>>    }
>>>>    
>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>> +{
>>>> +    struct dt_device_node *dev;
>>>> +    int ret = 0;
>>>> +
>>>> +    switch ( domctl->cmd )
>>>> +    {
>>>> +    case XEN_DOMCTL_assign_device:
>>>> +        ret = -EOPNOTSUPP;
>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>> The -EOPNOTSUPP code is used because this is part of a chained call after
>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>> XEN_DOMCTL_assign_device
>> call is expected to handle any DT device, regardless of whether the DT
>> device is
>> protected by an IOMMU or if the IOMMU is disabled.
>> The following cases are considered:
>>
>> 1. IOMMU Protected Device (Success)
>>
>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>> we continue
>> processing the DT device by calling sci_do_domctl.
>>
>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>
>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>> disabled,
>> we still proceed to call sci_do_domctl.
> OK this makes sense.  I think it is OK to have a special error code to
> say "the IOMMU is disabled" but I don't know if it is a good idea to try
> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
> configuration with domctl disabled, for instance.
>
> It might be wiser to use a different error code. Maybe ENOENT?
>
I see that in the following commit:

71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)

-ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.

It's not clear to me why this was done from the commit description.

Maybe we should add commit author?

>> 3. Error from iommu_do_domctl (Fail State)
>>
>> If iommu_do_domctl returns any error, the system enters a fail state, and
>> sci_do_domctl is not called.
>>
>> 4. -EOPNOTSUPP from sci_do_domctl
>>
>> If sci_do_domctl returns -EOPNOTSUPP, this indicates one of the following:
>> - The provided device is not a DT device.
>> - There is no cur_mediator available (indicating that the SCI subsystem
>> is enabled
>> in the configuration, but no mediator was provided).
>> - The current mediator does not support assign_dt_device (this is
>> expected to be changed;
>> see below for details).
>> In this case, -EOPNOTSUPP is returned but will be ignored, and the
>> original return value from iommu_do_domctl will be used as the final result.
> Same comment as before. We need to be careful not confuse this case you
> described with other cases where sci_do_domctl is simply not
> implemented.
>
I was trying to mimic iommu_do_domctl logic...
>> 5. Return Code from sci_do_domctl
>>
>> If sci_do_domctl returns 0 (success) or an error code (failure),
>> the return value from iommu_do_domctl is overridden, and the result from
>> sci_do_domctl is returned.
>> Note: -EOPNOTSUPP from iommu_do_domctl will also be overridden since
>> step 2 was successfully completed (or failed).
>>>> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
>>>> +            break;
>>> this one
>>>
>>>> +        if ( !cur_mediator )
>>>> +            break;
>>> this one
>>>
>>>> +        if ( !cur_mediator->assign_dt_device )
>>>> +            break;
>>> and also this one? It seems more like an -EINVAL as the caller used a
>>> wrong parameter?
>> I think you are right that this case should return -EINVAL because we
>> should fail if mediator
>>
>> without implemented mandatory features was provided. Will be fixed.
>>
>>>> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>>>> +                                    domctl->u.assign_device.u.dt.size, &dev);
>>>> +        if ( ret )
>>>> +            return ret;
>>>> +
>>>> +        ret = sci_assign_dt_device(d, dev);
>>>> +        if ( ret )
>>>> +            break;
>>>> +
>>>> +        break;
>>>> +    default:
>>>> +        /* do not fail here as call is chained with iommu handling */
>>> It looks like this should be an error
>>>
>>>
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    static int __init sci_init(void)
>>>>    {
>>>>        struct dt_device_node *np;
>>>> diff --git a/xen/arch/arm/include/asm/firmware/sci.h b/xen/arch/arm/include/asm/firmware/sci.h
>>>> index 71fb54852e..b8d1bc8a62 100644
>>>> --- a/xen/arch/arm/include/asm/firmware/sci.h
>>>> +++ b/xen/arch/arm/include/asm/firmware/sci.h
>>>> @@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
>>>>     * control" functionality.
>>>>     */
>>>>    int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>> +
>>>> +/*
>>>> + * SCI domctl handler
>>>> + *
>>>> + * Only XEN_DOMCTL_assign_device is handled for now.
>>>> + */
>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>>>    #else
>>>>    
>>>>    static inline bool sci_domain_is_enabled(struct domain *d)
>>>> @@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
>>>>        return 0;
>>>>    }
>>>>    
>>>> +static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>> +                                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    #endif /* CONFIG_ARM_SCI */
>>>>    
>>>>    #endif /* __ASM_ARM_SCI_H */
>>>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>>>> index 05abb581a0..a74ee92067 100644
>>>> --- a/xen/common/domctl.c
>>>> +++ b/xen/common/domctl.c
>>>> @@ -27,6 +27,7 @@
>>>>    #include <xen/vm_event.h>
>>>>    #include <xen/monitor.h>
>>>>    #include <asm/current.h>
>>>> +#include <asm/firmware/sci.h>
>>>>    #include <asm/irq.h>
>>>>    #include <asm/page.h>
>>>>    #include <asm/p2m.h>
>>>> @@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>        case XEN_DOMCTL_deassign_device:
>>>>        case XEN_DOMCTL_get_device_group:
>>>>            ret = iommu_do_domctl(op, d, u_domctl);
>>>> +
>>>> +        if ( !ret || ret == -EOPNOTSUPP )
>>> It is better to invert the check:
>>>
>>> if ( ret < 0 && ret != -EOPNOTSUPP )
>>>       return ret;
>> +
>>>> +        {
>>>> +            int ret1;
>>>> +            /*
>>>> +             * Add chained handling of assigned DT devices to support
>>>> +             * access-controller functionality through SCI framework, so
>>>> +             * DT device assign request can be passed to FW for processing and
>>>> +             * enabling VM access to requested device.
>>>> +             * The access-controller DT device processing is chained after IOMMU
>>>> +             * processing and expected to be executed for any DT device
>>>> +             * regardless if DT device is protected by IOMMU or not (or IOMMU
>>>> +             * is disabled).
>>>> +             */
>>>> +            ret1 = sci_do_domctl(op, d, u_domctl);
>>>> +            if ( ret1 != -EOPNOTSUPP )
>>>> +                ret = ret1;
>>>> +        }
>>>>            break;
>>>>    
>>>>        case XEN_DOMCTL_get_paging_mempool_size:
>>>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>>>> index 075fb25a37..2624767e51 100644
>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>> @@ -318,6 +318,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>                break;
>>>>            }
>>>>    
>>>> +        if ( !dt_device_is_protected(dev) )
>>>> +        {
>>>> +            ret = 0;
>>>> +            break;
>>>> +        }
>>> I am concerned about this: previously we would call
>>> iommu_assign_dt_device and the same check at the beginning of
>>> iommu_assign_dt_device would return -EINVAL. Now it is a success.
>>>
>>> I am not sure this is appropriate. I wonder if instead we should:
>>>
>>> - remove this chunk from the patch
>>> - change the return error for !dt_device_is_protected at the top of
>>>     iommu_assign_dt_device from -EINVAL to -EOPNOTSUPP
>>> - this would fall into the same ret != -EOPNOTSUPP check after
>>>     iommu_do_domctl
>> That's a good point. I think we should do the same for
>>
>>   > if ( !is_iommu_enabled(d) )
>>
>>   >  return -EINVAL;
>>
>> because in this case we should process sci as well. I will do the change
>>
>>>>            ret = iommu_assign_dt_device(d, dev);
>>>>    
>>>>            if ( ret )
>>>> -- 
>>>> 2.34.1
>>> >

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Jan Beulich 4 months, 1 week ago

On 19.06.2025 18:15, Oleksii Moisieiev wrote:
> 
> On 18/06/2025 03:04, Stefano Stabellini wrote:
>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>> Hi Stefano,
>>>
>>> I'm very sorry for a long silence. Please see my answers below:
>>>
>>> On 22/05/2025 03:25, Stefano Stabellini wrote:
>>>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>>>>> From: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>
>>>>> Add chained handling of assigned DT devices to support access-controller
>>>>> functionality through SCI framework, so DT device assign request can be
>>>>> passed to FW for processing and enabling VM access to requested device
>>>>> (for example, device power management through FW interface like SCMI).
>>>>>
>>>>> The SCI access-controller DT device processing is chained after IOMMU
>>>>> processing and expected to be executed for any DT device regardless of its
>>>>> protection by IOMMU (or if IOMMU is disabled).
>>>>>
>>>>> This allows to pass not only IOMMU protected DT device through
>>>>> xl.cfg:"dtdev" property for processing:
>>>>>
>>>>> dtdev = [
>>>>>       "/soc/video@e6ef0000", <- IOMMU protected device
>>>>>       "/soc/i2c@e6508000", <- not IOMMU protected device
>>>>> ]
>>>>>
>>>>> The change is done in two parts:
>>>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>>>>> not fail if DT device is not protected by IOMMU
>>>>> 2) add chained call to sci_do_domctl() in do_domctl()
>>>>>
>>>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
>>>>> ---
>>>>>
>>>>>
>>>>>
>>>>>    xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>>>>>    xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>>>>    xen/common/domctl.c                     | 19 +++++++++++++
>>>>>    xen/drivers/passthrough/device_tree.c   |  6 ++++
>>>>>    4 files changed, 76 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>> index e1522e10e2..8efd541c4f 100644
>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>        return 0;
>>>>>    }
>>>>>    
>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>> +{
>>>>> +    struct dt_device_node *dev;
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    switch ( domctl->cmd )
>>>>> +    {
>>>>> +    case XEN_DOMCTL_assign_device:
>>>>> +        ret = -EOPNOTSUPP;
>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>> XEN_DOMCTL_assign_device
>>> call is expected to handle any DT device, regardless of whether the DT
>>> device is
>>> protected by an IOMMU or if the IOMMU is disabled.
>>> The following cases are considered:
>>>
>>> 1. IOMMU Protected Device (Success)
>>>
>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>> we continue
>>> processing the DT device by calling sci_do_domctl.
>>>
>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>
>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>> disabled,
>>> we still proceed to call sci_do_domctl.
>> OK this makes sense.  I think it is OK to have a special error code to
>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>> configuration with domctl disabled, for instance.
>>
>> It might be wiser to use a different error code. Maybe ENOENT?
>>
> I see that in the following commit:
> 
> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
> 
> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
> 
> It's not clear to me why this was done from the commit description.

This has been discussed many times elsewhere. Many of our ENOSYS uses are
simply wrong. ENOSYS has very limited applicability: Unavailability of a
top-level hypercall (originally: syscall).

> Maybe we should add commit author?

You might, but Paul hasn't been active in Xen for quite some time now.

Jan

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 4 months, 1 week ago

On 23/06/2025 10:15, Jan Beulich wrote:
> On 19.06.2025 18:15, Oleksii Moisieiev wrote:
>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>         return 0;
>>>>>>     }
>>>>>>     
>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>> +{
>>>>>> +    struct dt_device_node *dev;
>>>>>> +    int ret = 0;
>>>>>> +
>>>>>> +    switch ( domctl->cmd )
>>>>>> +    {
>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>> +        ret = -EOPNOTSUPP;
>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>> XEN_DOMCTL_assign_device
>>>> call is expected to handle any DT device, regardless of whether the DT
>>>> device is
>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>> The following cases are considered:
>>>>
>>>> 1. IOMMU Protected Device (Success)
>>>>
>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>> we continue
>>>> processing the DT device by calling sci_do_domctl.
>>>>
>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>
>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>> disabled,
>>>> we still proceed to call sci_do_domctl.
>>> OK this makes sense.  I think it is OK to have a special error code to
>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>> configuration with domctl disabled, for instance.
>>>
>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>
>> I see that in the following commit:
>>
>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>
>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>
>> It's not clear to me why this was done from the commit description.
> This has been discussed many times elsewhere. Many of our ENOSYS uses are
> simply wrong. ENOSYS has very limited applicability: Unavailability of a
> top-level hypercall (originally: syscall).
>
What is your opinion about changing it to -ENOENT to say "the IOMMU is 
disabled" as Stefano suggested in [0]?

[0]: https://lists.xen.org/archives/html/xen-devel/2025-06/msg01233.html

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Jan Beulich 4 months, 1 week ago

On 25.06.2025 21:56, Oleksii Moisieiev wrote:
> 
> On 23/06/2025 10:15, Jan Beulich wrote:
>> On 19.06.2025 18:15, Oleksii Moisieiev wrote:
>>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>>         return 0;
>>>>>>>     }
>>>>>>>     
>>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>>> +{
>>>>>>> +    struct dt_device_node *dev;
>>>>>>> +    int ret = 0;
>>>>>>> +
>>>>>>> +    switch ( domctl->cmd )
>>>>>>> +    {
>>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>>> +        ret = -EOPNOTSUPP;
>>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>>> XEN_DOMCTL_assign_device
>>>>> call is expected to handle any DT device, regardless of whether the DT
>>>>> device is
>>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>>> The following cases are considered:
>>>>>
>>>>> 1. IOMMU Protected Device (Success)
>>>>>
>>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>>> we continue
>>>>> processing the DT device by calling sci_do_domctl.
>>>>>
>>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>>
>>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>>> disabled,
>>>>> we still proceed to call sci_do_domctl.
>>>> OK this makes sense.  I think it is OK to have a special error code to
>>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>>> configuration with domctl disabled, for instance.
>>>>
>>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>>
>>> I see that in the following commit:
>>>
>>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>>
>>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>>
>>> It's not clear to me why this was done from the commit description.
>> This has been discussed many times elsewhere. Many of our ENOSYS uses are
>> simply wrong. ENOSYS has very limited applicability: Unavailability of a
>> top-level hypercall (originally: syscall).
>>
> What is your opinion about changing it to -ENOENT to say "the IOMMU is 
> disabled" as Stefano suggested in [0]?
> 
> [0]: https://lists.xen.org/archives/html/xen-devel/2025-06/msg01233.html

To me, ENOENT is closer to ENODEV, and hence not overly applicable here.
If you want to avoid EOPNOTSUPP for whatever reason, how about ENXIO or
EIO? (EPERM might also be an option, but we assign that a different
meaning generally.)

Jan

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 4 months ago

On 26/06/2025 09:10, Jan Beulich wrote:
> On 25.06.2025 21:56, Oleksii Moisieiev wrote:
>> On 23/06/2025 10:15, Jan Beulich wrote:
>>> On 19.06.2025 18:15, Oleksii Moisieiev wrote:
>>>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>>>          return 0;
>>>>>>>>      }
>>>>>>>>
>>>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>>>> +{
>>>>>>>> +    struct dt_device_node *dev;
>>>>>>>> +    int ret = 0;
>>>>>>>> +
>>>>>>>> +    switch ( domctl->cmd )
>>>>>>>> +    {
>>>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>>>> +        ret = -EOPNOTSUPP;
>>>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>>>> XEN_DOMCTL_assign_device
>>>>>> call is expected to handle any DT device, regardless of whether the DT
>>>>>> device is
>>>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>>>> The following cases are considered:
>>>>>>
>>>>>> 1. IOMMU Protected Device (Success)
>>>>>>
>>>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>>>> we continue
>>>>>> processing the DT device by calling sci_do_domctl.
>>>>>>
>>>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>>>
>>>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>>>> disabled,
>>>>>> we still proceed to call sci_do_domctl.
>>>>> OK this makes sense.  I think it is OK to have a special error code to
>>>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>>>> configuration with domctl disabled, for instance.
>>>>>
>>>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>>>
>>>> I see that in the following commit:
>>>>
>>>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>>>
>>>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>>>
>>>> It's not clear to me why this was done from the commit description.
>>> This has been discussed many times elsewhere. Many of our ENOSYS uses are
>>> simply wrong. ENOSYS has very limited applicability: Unavailability of a
>>> top-level hypercall (originally: syscall).
>>>
>> What is your opinion about changing it to -ENOENT to say "the IOMMU is
>> disabled" as Stefano suggested in [0]?
>>
>> [0]: https://lists.xen.org/archives/html/xen-devel/2025-06/msg01233.html
> To me, ENOENT is closer to ENODEV, and hence not overly applicable here.
> If you want to avoid EOPNOTSUPP for whatever reason, how about ENXIO or
> EIO? (EPERM might also be an option, but we assign that a different
> meaning generally.)
>
> Jan

Maybe -ENODEV is a better choice because iommu_do_pci_domctl and
iommu_do_dt_domctl return this

code when some feature is not supported.

I think -EIO or -ENXIO aren’t suitable here since we’re planning to send
this message when the IOMMU is disabled.

What do you think?

WBR,

Oleksii.

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Jan Beulich 4 months ago

On 26.06.2025 15:07, Oleksii Moisieiev wrote:
> 
> On 26/06/2025 09:10, Jan Beulich wrote:
>> On 25.06.2025 21:56, Oleksii Moisieiev wrote:
>>> On 23/06/2025 10:15, Jan Beulich wrote:
>>>> On 19.06.2025 18:15, Oleksii Moisieiev wrote:
>>>>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>>>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>>>>          return 0;
>>>>>>>>>      }
>>>>>>>>>
>>>>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>>>>> +{
>>>>>>>>> +    struct dt_device_node *dev;
>>>>>>>>> +    int ret = 0;
>>>>>>>>> +
>>>>>>>>> +    switch ( domctl->cmd )
>>>>>>>>> +    {
>>>>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>>>>> +        ret = -EOPNOTSUPP;
>>>>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>>>>> XEN_DOMCTL_assign_device
>>>>>>> call is expected to handle any DT device, regardless of whether the DT
>>>>>>> device is
>>>>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>>>>> The following cases are considered:
>>>>>>>
>>>>>>> 1. IOMMU Protected Device (Success)
>>>>>>>
>>>>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>>>>> we continue
>>>>>>> processing the DT device by calling sci_do_domctl.
>>>>>>>
>>>>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>>>>
>>>>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>>>>> disabled,
>>>>>>> we still proceed to call sci_do_domctl.
>>>>>> OK this makes sense.  I think it is OK to have a special error code to
>>>>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>>>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>>>>> configuration with domctl disabled, for instance.
>>>>>>
>>>>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>>>>
>>>>> I see that in the following commit:
>>>>>
>>>>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>>>>
>>>>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>>>>
>>>>> It's not clear to me why this was done from the commit description.
>>>> This has been discussed many times elsewhere. Many of our ENOSYS uses are
>>>> simply wrong. ENOSYS has very limited applicability: Unavailability of a
>>>> top-level hypercall (originally: syscall).
>>>>
>>> What is your opinion about changing it to -ENOENT to say "the IOMMU is
>>> disabled" as Stefano suggested in [0]?
>>>
>>> [0]: https://lists.xen.org/archives/html/xen-devel/2025-06/msg01233.html
>> To me, ENOENT is closer to ENODEV, and hence not overly applicable here.
>> If you want to avoid EOPNOTSUPP for whatever reason, how about ENXIO or
>> EIO? (EPERM might also be an option, but we assign that a different
>> meaning generally.)
> 
> Maybe -ENODEV is a better choice because iommu_do_pci_domctl and
> iommu_do_dt_domctl return this
> 
> code when some feature is not supported.

What feature are you talking about? All I see in the former is

        ret = -ENODEV;
        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
            break;

and there -ENODEV is quite appropriate.

> I think -EIO or -ENXIO aren’t suitable here since we’re planning to send
> this message when the IOMMU is disabled.

Well, I don't like those two very much either for the use here, but I still
view them as better than ENOENT.

Jan

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 4 months ago

On 26/06/2025 17:41, Jan Beulich wrote:
> On 26.06.2025 15:07, Oleksii Moisieiev wrote:
>> On 26/06/2025 09:10, Jan Beulich wrote:
>>> On 25.06.2025 21:56, Oleksii Moisieiev wrote:
>>>> On 23/06/2025 10:15, Jan Beulich wrote:
>>>>> On 19.06.2025 18:15, Oleksii Moisieiev wrote:
>>>>>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>>>>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>>>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>>>>>           return 0;
>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>>>>>> +{
>>>>>>>>>> +    struct dt_device_node *dev;
>>>>>>>>>> +    int ret = 0;
>>>>>>>>>> +
>>>>>>>>>> +    switch ( domctl->cmd )
>>>>>>>>>> +    {
>>>>>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>>>>>> +        ret = -EOPNOTSUPP;
>>>>>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>>>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>>>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>>>>>> XEN_DOMCTL_assign_device
>>>>>>>> call is expected to handle any DT device, regardless of whether the DT
>>>>>>>> device is
>>>>>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>>>>>> The following cases are considered:
>>>>>>>>
>>>>>>>> 1. IOMMU Protected Device (Success)
>>>>>>>>
>>>>>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>>>>>> we continue
>>>>>>>> processing the DT device by calling sci_do_domctl.
>>>>>>>>
>>>>>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>>>>>
>>>>>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>>>>>> disabled,
>>>>>>>> we still proceed to call sci_do_domctl.
>>>>>>> OK this makes sense.  I think it is OK to have a special error code to
>>>>>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>>>>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>>>>>> configuration with domctl disabled, for instance.
>>>>>>>
>>>>>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>>>>>
>>>>>> I see that in the following commit:
>>>>>>
>>>>>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>>>>>
>>>>>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>>>>>
>>>>>> It's not clear to me why this was done from the commit description.
>>>>> This has been discussed many times elsewhere. Many of our ENOSYS uses are
>>>>> simply wrong. ENOSYS has very limited applicability: Unavailability of a
>>>>> top-level hypercall (originally: syscall).
>>>>>
>>>> What is your opinion about changing it to -ENOENT to say "the IOMMU is
>>>> disabled" as Stefano suggested in [0]?
>>>>
>>>> [0]: https://lists.xen.org/archives/html/xen-devel/2025-06/msg01233.html
>>> To me, ENOENT is closer to ENODEV, and hence not overly applicable here.
>>> If you want to avoid EOPNOTSUPP for whatever reason, how about ENXIO or
>>> EIO? (EPERM might also be an option, but we assign that a different
>>> meaning generally.)
>> Maybe -ENODEV is a better choice because iommu_do_pci_domctl and
>> iommu_do_dt_domctl return this
>>
>> code when some feature is not supported.
> What feature are you talking about? All I see in the former is
>
>          ret = -ENODEV;
>          if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
>              break;
>
> and there -ENODEV is quite appropriate.

I was talking about the following code in iommu_do_pci_domctl:

     ret = -ENODEV;
         if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
             break;

Sorry, I misinterpreted this.

>> I think -EIO or -ENXIO aren’t suitable here since we’re planning to send
>> this message when the IOMMU is disabled.
> Well, I don't like those two very much either for the use here, but I still
> view them as better than ENOENT.
>
I think I could use -ENXIO since it has the following comment:
     /* No such device or address */
It seems to be suitable for our case.

WBR,

Oleksii.

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 4 months, 1 week ago

Adding Paul to the conversation.


On 23/06/2025 10:15, Jan Beulich wrote:
> On 19.06.2025 18:15, Oleksii Moisieiev wrote:
>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>> Hi Stefano,
>>>>
>>>> I'm very sorry for a long silence. Please see my answers below:
>>>>
>>>> On 22/05/2025 03:25, Stefano Stabellini wrote:
>>>>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>>>>>> From: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>>
>>>>>> Add chained handling of assigned DT devices to support access-controller
>>>>>> functionality through SCI framework, so DT device assign request can be
>>>>>> passed to FW for processing and enabling VM access to requested device
>>>>>> (for example, device power management through FW interface like SCMI).
>>>>>>
>>>>>> The SCI access-controller DT device processing is chained after IOMMU
>>>>>> processing and expected to be executed for any DT device regardless of its
>>>>>> protection by IOMMU (or if IOMMU is disabled).
>>>>>>
>>>>>> This allows to pass not only IOMMU protected DT device through
>>>>>> xl.cfg:"dtdev" property for processing:
>>>>>>
>>>>>> dtdev = [
>>>>>>        "/soc/video@e6ef0000", <- IOMMU protected device
>>>>>>        "/soc/i2c@e6508000", <- not IOMMU protected device
>>>>>> ]
>>>>>>
>>>>>> The change is done in two parts:
>>>>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>>>>>> not fail if DT device is not protected by IOMMU
>>>>>> 2) add chained call to sci_do_domctl() in do_domctl()
>>>>>>
>>>>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
>>>>>> ---
>>>>>>
>>>>>>
>>>>>>
>>>>>>     xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>>>>>>     xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>>>>>     xen/common/domctl.c                     | 19 +++++++++++++
>>>>>>     xen/drivers/passthrough/device_tree.c   |  6 ++++
>>>>>>     4 files changed, 76 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>         return 0;
>>>>>>     }
>>>>>>     
>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>> +{
>>>>>> +    struct dt_device_node *dev;
>>>>>> +    int ret = 0;
>>>>>> +
>>>>>> +    switch ( domctl->cmd )
>>>>>> +    {
>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>> +        ret = -EOPNOTSUPP;
>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>> XEN_DOMCTL_assign_device
>>>> call is expected to handle any DT device, regardless of whether the DT
>>>> device is
>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>> The following cases are considered:
>>>>
>>>> 1. IOMMU Protected Device (Success)
>>>>
>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>> we continue
>>>> processing the DT device by calling sci_do_domctl.
>>>>
>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>
>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>> disabled,
>>>> we still proceed to call sci_do_domctl.
>>> OK this makes sense.  I think it is OK to have a special error code to
>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>> configuration with domctl disabled, for instance.
>>>
>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>
>> I see that in the following commit:
>>
>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>
>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>
>> It's not clear to me why this was done from the commit description.
> This has been discussed many times elsewhere. Many of our ENOSYS uses are
> simply wrong. ENOSYS has very limited applicability: Unavailability of a
> top-level hypercall (originally: syscall).
>
>> Maybe we should add commit author?
> You might, but Paul hasn't been active in Xen for quite some time now.
>
> Jan

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Stefano Stabellini 4 months, 1 week ago

On Thu, 19 Jun 2025, Oleksii Moisieiev wrote:
> On 18/06/2025 03:04, Stefano Stabellini wrote:
> > On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
> >> Hi Stefano,
> >>
> >> I'm very sorry for a long silence. Please see my answers below:
> >>
> >> On 22/05/2025 03:25, Stefano Stabellini wrote:
> >>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
> >>>> From: Grygorii Strashko<grygorii_strashko@epam.com>
> >>>>
> >>>> Add chained handling of assigned DT devices to support access-controller
> >>>> functionality through SCI framework, so DT device assign request can be
> >>>> passed to FW for processing and enabling VM access to requested device
> >>>> (for example, device power management through FW interface like SCMI).
> >>>>
> >>>> The SCI access-controller DT device processing is chained after IOMMU
> >>>> processing and expected to be executed for any DT device regardless of its
> >>>> protection by IOMMU (or if IOMMU is disabled).
> >>>>
> >>>> This allows to pass not only IOMMU protected DT device through
> >>>> xl.cfg:"dtdev" property for processing:
> >>>>
> >>>> dtdev = [
> >>>>       "/soc/video@e6ef0000", <- IOMMU protected device
> >>>>       "/soc/i2c@e6508000", <- not IOMMU protected device
> >>>> ]
> >>>>
> >>>> The change is done in two parts:
> >>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
> >>>> not fail if DT device is not protected by IOMMU
> >>>> 2) add chained call to sci_do_domctl() in do_domctl()
> >>>>
> >>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
> >>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
> >>>> ---
> >>>>
> >>>>
> >>>>
> >>>>    xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
> >>>>    xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
> >>>>    xen/common/domctl.c                     | 19 +++++++++++++
> >>>>    xen/drivers/passthrough/device_tree.c   |  6 ++++
> >>>>    4 files changed, 76 insertions(+)
> >>>>
> >>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
> >>>> index e1522e10e2..8efd541c4f 100644
> >>>> --- a/xen/arch/arm/firmware/sci.c
> >>>> +++ b/xen/arch/arm/firmware/sci.c
> >>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
> >>>>        return 0;
> >>>>    }
> >>>>    
> >>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> >>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >>>> +{
> >>>> +    struct dt_device_node *dev;
> >>>> +    int ret = 0;
> >>>> +
> >>>> +    switch ( domctl->cmd )
> >>>> +    {
> >>>> +    case XEN_DOMCTL_assign_device:
> >>>> +        ret = -EOPNOTSUPP;
> >>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
> >> The -EOPNOTSUPP code is used because this is part of a chained call after
> >> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
> >> XEN_DOMCTL_assign_device
> >> call is expected to handle any DT device, regardless of whether the DT
> >> device is
> >> protected by an IOMMU or if the IOMMU is disabled.
> >> The following cases are considered:
> >>
> >> 1. IOMMU Protected Device (Success)
> >>
> >> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
> >> we continue
> >> processing the DT device by calling sci_do_domctl.
> >>
> >> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
> >>
> >> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
> >> disabled,
> >> we still proceed to call sci_do_domctl.
> > OK this makes sense.  I think it is OK to have a special error code to
> > say "the IOMMU is disabled" but I don't know if it is a good idea to try
> > to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
> > configuration with domctl disabled, for instance.
> >
> > It might be wiser to use a different error code. Maybe ENOENT?
> >
> I see that in the following commit:
> 
> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
> 
> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
> 
> It's not clear to me why this was done from the commit description.
> 
> Maybe we should add commit author?

Roger and Jan might know

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 4 months, 1 week ago

Adding Roger and Jan to the conversation.

Please see below.

On 23/06/2025 00:30, Stefano Stabellini wrote:
> On Thu, 19 Jun 2025, Oleksii Moisieiev wrote:
>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>> Hi Stefano,
>>>>
>>>> I'm very sorry for a long silence. Please see my answers below:
>>>>
>>>> On 22/05/2025 03:25, Stefano Stabellini wrote:
>>>>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>>>>>> From: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>>
>>>>>> Add chained handling of assigned DT devices to support access-controller
>>>>>> functionality through SCI framework, so DT device assign request can be
>>>>>> passed to FW for processing and enabling VM access to requested device
>>>>>> (for example, device power management through FW interface like SCMI).
>>>>>>
>>>>>> The SCI access-controller DT device processing is chained after IOMMU
>>>>>> processing and expected to be executed for any DT device regardless of its
>>>>>> protection by IOMMU (or if IOMMU is disabled).
>>>>>>
>>>>>> This allows to pass not only IOMMU protected DT device through
>>>>>> xl.cfg:"dtdev" property for processing:
>>>>>>
>>>>>> dtdev = [
>>>>>>        "/soc/video@e6ef0000", <- IOMMU protected device
>>>>>>        "/soc/i2c@e6508000", <- not IOMMU protected device
>>>>>> ]
>>>>>>
>>>>>> The change is done in two parts:
>>>>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>>>>>> not fail if DT device is not protected by IOMMU
>>>>>> 2) add chained call to sci_do_domctl() in do_domctl()
>>>>>>
>>>>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
>>>>>> ---
>>>>>>
>>>>>>
>>>>>>
>>>>>>     xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>>>>>>     xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>>>>>     xen/common/domctl.c                     | 19 +++++++++++++
>>>>>>     xen/drivers/passthrough/device_tree.c   |  6 ++++
>>>>>>     4 files changed, 76 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>         return 0;
>>>>>>     }
>>>>>>     
>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>> +{
>>>>>> +    struct dt_device_node *dev;
>>>>>> +    int ret = 0;
>>>>>> +
>>>>>> +    switch ( domctl->cmd )
>>>>>> +    {
>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>> +        ret = -EOPNOTSUPP;
>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>> XEN_DOMCTL_assign_device
>>>> call is expected to handle any DT device, regardless of whether the DT
>>>> device is
>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>> The following cases are considered:
>>>>
>>>> 1. IOMMU Protected Device (Success)
>>>>
>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>> we continue
>>>> processing the DT device by calling sci_do_domctl.
>>>>
>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>
>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>> disabled,
>>>> we still proceed to call sci_do_domctl.
>>> OK this makes sense.  I think it is OK to have a special error code to
>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>> configuration with domctl disabled, for instance.
>>>
>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>
>> I see that in the following commit:
>>
>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>
>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>
>> It's not clear to me why this was done from the commit description.
>>
>> Maybe we should add commit author?
> Roger and Jan might know

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Jan Beulich 4 months, 1 week ago

On 24.06.2025 10:42, Oleksii Moisieiev wrote:
> Adding Roger and Jan to the conversation.
> 
> Please see below.

Why is this? I did answer that question at the bottom already.

Jan

> On 23/06/2025 00:30, Stefano Stabellini wrote:
>> On Thu, 19 Jun 2025, Oleksii Moisieiev wrote:
>>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>>> Hi Stefano,
>>>>>
>>>>> I'm very sorry for a long silence. Please see my answers below:
>>>>>
>>>>> On 22/05/2025 03:25, Stefano Stabellini wrote:
>>>>>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>>>>>>> From: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>>>
>>>>>>> Add chained handling of assigned DT devices to support access-controller
>>>>>>> functionality through SCI framework, so DT device assign request can be
>>>>>>> passed to FW for processing and enabling VM access to requested device
>>>>>>> (for example, device power management through FW interface like SCMI).
>>>>>>>
>>>>>>> The SCI access-controller DT device processing is chained after IOMMU
>>>>>>> processing and expected to be executed for any DT device regardless of its
>>>>>>> protection by IOMMU (or if IOMMU is disabled).
>>>>>>>
>>>>>>> This allows to pass not only IOMMU protected DT device through
>>>>>>> xl.cfg:"dtdev" property for processing:
>>>>>>>
>>>>>>> dtdev = [
>>>>>>>        "/soc/video@e6ef0000", <- IOMMU protected device
>>>>>>>        "/soc/i2c@e6508000", <- not IOMMU protected device
>>>>>>> ]
>>>>>>>
>>>>>>> The change is done in two parts:
>>>>>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>>>>>>> not fail if DT device is not protected by IOMMU
>>>>>>> 2) add chained call to sci_do_domctl() in do_domctl()
>>>>>>>
>>>>>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>     xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>>>>>>>     xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>>>>>>     xen/common/domctl.c                     | 19 +++++++++++++
>>>>>>>     xen/drivers/passthrough/device_tree.c   |  6 ++++
>>>>>>>     4 files changed, 76 insertions(+)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>>         return 0;
>>>>>>>     }
>>>>>>>     
>>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>>> +{
>>>>>>> +    struct dt_device_node *dev;
>>>>>>> +    int ret = 0;
>>>>>>> +
>>>>>>> +    switch ( domctl->cmd )
>>>>>>> +    {
>>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>>> +        ret = -EOPNOTSUPP;
>>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>>> XEN_DOMCTL_assign_device
>>>>> call is expected to handle any DT device, regardless of whether the DT
>>>>> device is
>>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>>> The following cases are considered:
>>>>>
>>>>> 1. IOMMU Protected Device (Success)
>>>>>
>>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>>> we continue
>>>>> processing the DT device by calling sci_do_domctl.
>>>>>
>>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>>
>>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>>> disabled,
>>>>> we still proceed to call sci_do_domctl.
>>>> OK this makes sense.  I think it is OK to have a special error code to
>>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>>> configuration with domctl disabled, for instance.
>>>>
>>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>>
>>> I see that in the following commit:
>>>
>>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>>
>>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>>
>>> It's not clear to me why this was done from the commit description.
>>>
>>> Maybe we should add commit author?
>> Roger and Jan might know

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Oleksii Moisieiev 4 months, 1 week ago

On 24/06/2025 11:47, Jan Beulich wrote:
> On 24.06.2025 10:42, Oleksii Moisieiev wrote:
>> Adding Roger and Jan to the conversation.
>>
>> Please see below.
> Why is this? I did answer that question at the bottom already.
>
> Jan
Oh sorry, missed this email..
>> On 23/06/2025 00:30, Stefano Stabellini wrote:
>>> On Thu, 19 Jun 2025, Oleksii Moisieiev wrote:
>>>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>>>> Hi Stefano,
>>>>>>
>>>>>> I'm very sorry for a long silence. Please see my answers below:
>>>>>>
>>>>>> On 22/05/2025 03:25, Stefano Stabellini wrote:
>>>>>>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>>>>>>>> From: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>>>>
>>>>>>>> Add chained handling of assigned DT devices to support access-controller
>>>>>>>> functionality through SCI framework, so DT device assign request can be
>>>>>>>> passed to FW for processing and enabling VM access to requested device
>>>>>>>> (for example, device power management through FW interface like SCMI).
>>>>>>>>
>>>>>>>> The SCI access-controller DT device processing is chained after IOMMU
>>>>>>>> processing and expected to be executed for any DT device regardless of its
>>>>>>>> protection by IOMMU (or if IOMMU is disabled).
>>>>>>>>
>>>>>>>> This allows to pass not only IOMMU protected DT device through
>>>>>>>> xl.cfg:"dtdev" property for processing:
>>>>>>>>
>>>>>>>> dtdev = [
>>>>>>>>         "/soc/video@e6ef0000", <- IOMMU protected device
>>>>>>>>         "/soc/i2c@e6508000", <- not IOMMU protected device
>>>>>>>> ]
>>>>>>>>
>>>>>>>> The change is done in two parts:
>>>>>>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>>>>>>>> not fail if DT device is not protected by IOMMU
>>>>>>>> 2) add chained call to sci_do_domctl() in do_domctl()
>>>>>>>>
>>>>>>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
>>>>>>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>      xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>>>>>>>>      xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>>>>>>>      xen/common/domctl.c                     | 19 +++++++++++++
>>>>>>>>      xen/drivers/passthrough/device_tree.c   |  6 ++++
>>>>>>>>      4 files changed, 76 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>>>>>>> index e1522e10e2..8efd541c4f 100644
>>>>>>>> --- a/xen/arch/arm/firmware/sci.c
>>>>>>>> +++ b/xen/arch/arm/firmware/sci.c
>>>>>>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>>>>>>          return 0;
>>>>>>>>      }
>>>>>>>>      
>>>>>>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>>>> +{
>>>>>>>> +    struct dt_device_node *dev;
>>>>>>>> +    int ret = 0;
>>>>>>>> +
>>>>>>>> +    switch ( domctl->cmd )
>>>>>>>> +    {
>>>>>>>> +    case XEN_DOMCTL_assign_device:
>>>>>>>> +        ret = -EOPNOTSUPP;
>>>>>>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
>>>>>> The -EOPNOTSUPP code is used because this is part of a chained call after
>>>>>> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
>>>>>> XEN_DOMCTL_assign_device
>>>>>> call is expected to handle any DT device, regardless of whether the DT
>>>>>> device is
>>>>>> protected by an IOMMU or if the IOMMU is disabled.
>>>>>> The following cases are considered:
>>>>>>
>>>>>> 1. IOMMU Protected Device (Success)
>>>>>>
>>>>>> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
>>>>>> we continue
>>>>>> processing the DT device by calling sci_do_domctl.
>>>>>>
>>>>>> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
>>>>>>
>>>>>> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
>>>>>> disabled,
>>>>>> we still proceed to call sci_do_domctl.
>>>>> OK this makes sense.  I think it is OK to have a special error code to
>>>>> say "the IOMMU is disabled" but I don't know if it is a good idea to try
>>>>> to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
>>>>> configuration with domctl disabled, for instance.
>>>>>
>>>>> It might be wiser to use a different error code. Maybe ENOENT?
>>>>>
>>>> I see that in the following commit:
>>>>
>>>> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>>>>
>>>> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>>>>
>>>> It's not clear to me why this was done from the commit description.
>>>>
>>>> Maybe we should add commit author?
>>> Roger and Jan might know

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Grygorii Strashko 4 months, 2 weeks ago


On 12.06.25 14:42, Oleksii Moisieiev wrote:
> Hi Stefano,
> 
> I'm very sorry for a long silence. Please see my answers below:
> 
> On 22/05/2025 03:25, Stefano Stabellini wrote:
>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>>> From: Grygorii Strashko<grygorii_strashko@epam.com>
>>>
>>> Add chained handling of assigned DT devices to support access-controller
>>> functionality through SCI framework, so DT device assign request can be
>>> passed to FW for processing and enabling VM access to requested device
>>> (for example, device power management through FW interface like SCMI).
>>>
>>> The SCI access-controller DT device processing is chained after IOMMU
>>> processing and expected to be executed for any DT device regardless of its
>>> protection by IOMMU (or if IOMMU is disabled).
>>>
>>> This allows to pass not only IOMMU protected DT device through
>>> xl.cfg:"dtdev" property for processing:
>>>
>>> dtdev = [
>>>       "/soc/video@e6ef0000", <- IOMMU protected device
>>>       "/soc/i2c@e6508000", <- not IOMMU protected device
>>> ]
>>>
>>> The change is done in two parts:
>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>>> not fail if DT device is not protected by IOMMU
>>> 2) add chained call to sci_do_domctl() in do_domctl()
>>>
>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@epam.com>
>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@epam.com>
>>> ---
>>>
>>>
>>>
>>>    xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>>>    xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>>    xen/common/domctl.c                     | 19 +++++++++++++
>>>    xen/drivers/passthrough/device_tree.c   |  6 ++++
>>>    4 files changed, 76 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
>>> index e1522e10e2..8efd541c4f 100644
>>> --- a/xen/arch/arm/firmware/sci.c
>>> +++ b/xen/arch/arm/firmware/sci.c
>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>>>        return 0;
>>>    }
>>>    
>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>> +{
>>> +    struct dt_device_node *dev;
>>> +    int ret = 0;
>>> +
>>> +    switch ( domctl->cmd )
>>> +    {
>>> +    case XEN_DOMCTL_assign_device:
>>> +        ret = -EOPNOTSUPP;
>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
> 
> The -EOPNOTSUPP code is used because this is part of a chained call after
> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
> XEN_DOMCTL_assign_device
> call is expected to handle any DT device, regardless of whether the DT
> device is
> protected by an IOMMU or if the IOMMU is disabled.
> The following cases are considered:
> 
> 1. IOMMU Protected Device (Success)
> 
> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
> we continue
> processing the DT device by calling sci_do_domctl.
> 
> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
> 
> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
> disabled,
> we still proceed to call sci_do_domctl.
> 
> 3. Error from iommu_do_domctl (Fail State)
> 
> If iommu_do_domctl returns any error, the system enters a fail state, and
> sci_do_domctl is not called.
> 
> 4. -EOPNOTSUPP from sci_do_domctl
> 
> If sci_do_domctl returns -EOPNOTSUPP, this indicates one of the following:
> - The provided device is not a DT device.
> - There is no cur_mediator available (indicating that the SCI subsystem
> is enabled
> in the configuration, but no mediator was provided).
> - The current mediator does not support assign_dt_device (this is
> expected to be changed;
> see below for details).
> In this case, -EOPNOTSUPP is returned but will be ignored, and the
> original return value from iommu_do_domctl will be used as the final result.
> 
> 5. Return Code from sci_do_domctl
> 
> If sci_do_domctl returns 0 (success) or an error code (failure),
> the return value from iommu_do_domctl is overridden, and the result from
> sci_do_domctl is returned.
> Note: -EOPNOTSUPP from iommu_do_domctl will also be overridden since
> step 2 was successfully completed (or failed).
>>> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
>>> +            break;
>> this one
>>
>>> +        if ( !cur_mediator )
>>> +            break;
>> this one
>>
>>> +        if ( !cur_mediator->assign_dt_device )
>>> +            break;
>> and also this one? It seems more like an -EINVAL as the caller used a
>> wrong parameter?
> 
> I think you are right that this case should return -EINVAL because we
> should fail if mediator
> 
> without implemented mandatory features was provided. Will be fixed.
> 
>>> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>>> +                                    domctl->u.assign_device.u.dt.size, &dev);
>>> +        if ( ret )
>>> +            return ret;
>>> +
>>> +        ret = sci_assign_dt_device(d, dev);
>>> +        if ( ret )
>>> +            break;
>>> +
>>> +        break;
>>> +    default:
>>> +        /* do not fail here as call is chained with iommu handling */
>> It looks like this should be an error
>>
>>
>>> +        break;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>    static int __init sci_init(void)
>>>    {
>>>        struct dt_device_node *np;
>>> diff --git a/xen/arch/arm/include/asm/firmware/sci.h b/xen/arch/arm/include/asm/firmware/sci.h
>>> index 71fb54852e..b8d1bc8a62 100644
>>> --- a/xen/arch/arm/include/asm/firmware/sci.h
>>> +++ b/xen/arch/arm/include/asm/firmware/sci.h
>>> @@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
>>>     * control" functionality.
>>>     */
>>>    int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>>> +
>>> +/*
>>> + * SCI domctl handler
>>> + *
>>> + * Only XEN_DOMCTL_assign_device is handled for now.
>>> + */
>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>>    #else
>>>    
>>>    static inline bool sci_domain_is_enabled(struct domain *d)
>>> @@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
>>>        return 0;
>>>    }
>>>    
>>> +static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>> +                                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>>    #endif /* CONFIG_ARM_SCI */
>>>    
>>>    #endif /* __ASM_ARM_SCI_H */
>>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>>> index 05abb581a0..a74ee92067 100644
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -27,6 +27,7 @@
>>>    #include <xen/vm_event.h>
>>>    #include <xen/monitor.h>
>>>    #include <asm/current.h>
>>> +#include <asm/firmware/sci.h>
>>>    #include <asm/irq.h>
>>>    #include <asm/page.h>
>>>    #include <asm/p2m.h>
>>> @@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>        case XEN_DOMCTL_deassign_device:
>>>        case XEN_DOMCTL_get_device_group:
>>>            ret = iommu_do_domctl(op, d, u_domctl);
>>> +
>>> +        if ( !ret || ret == -EOPNOTSUPP )
>> It is better to invert the check:
>>
>> if ( ret < 0 && ret != -EOPNOTSUPP )
>>       return ret;
> +
>>> +        {
>>> +            int ret1;
>>> +            /*
>>> +             * Add chained handling of assigned DT devices to support
>>> +             * access-controller functionality through SCI framework, so
>>> +             * DT device assign request can be passed to FW for processing and
>>> +             * enabling VM access to requested device.
>>> +             * The access-controller DT device processing is chained after IOMMU
>>> +             * processing and expected to be executed for any DT device
>>> +             * regardless if DT device is protected by IOMMU or not (or IOMMU
>>> +             * is disabled).
>>> +             */
>>> +            ret1 = sci_do_domctl(op, d, u_domctl);
>>> +            if ( ret1 != -EOPNOTSUPP )
>>> +                ret = ret1;
>>> +        }
>>>            break;
>>>    
>>>        case XEN_DOMCTL_get_paging_mempool_size:
>>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>>> index 075fb25a37..2624767e51 100644
>>> --- a/xen/drivers/passthrough/device_tree.c
>>> +++ b/xen/drivers/passthrough/device_tree.c
>>> @@ -318,6 +318,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>                break;
>>>            }
>>>    
>>> +        if ( !dt_device_is_protected(dev) )
>>> +        {
>>> +            ret = 0;
>>> +            break;
>>> +        }
>> I am concerned about this: previously we would call
>> iommu_assign_dt_device and the same check at the beginning of
>> iommu_assign_dt_device would return -EINVAL. Now it is a success.
>>
>> I am not sure this is appropriate. I wonder if instead we should:
>>
>> - remove this chunk from the patch
>> - change the return error for !dt_device_is_protected at the top of
>>     iommu_assign_dt_device from -EINVAL to -EOPNOTSUPP
>> - this would fall into the same ret != -EOPNOTSUPP check after
>>     iommu_do_domctl

We need to be careful here :(

The reason for this change, in it's current form is to make this part of
the code to work the same way as other parts [1] and [2] where IOMMU is configured (yep, and get what we need):

[1] xen/arch/arm/device.c handle_device()
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/device.c;h=5e1c1cc326ac04d30e7158905fead9b41c719fc0;hb=HEAD#l287

which is used to create hwdom(dom0) or apply overlays and called for every "own" node (!"xen,passthrough").
The "own" node may not be IOMMU protected.


[2] xen/common/device-tree/dom0less-build.c handle_passthrough_prop()
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/device-tree/dom0less-build.c;h=3d503c697337a4cc6df35403b9a2595b40089dae;hb=HEAD#l224

which is used for "dom0less" devices pass-through (arm/ppc) and called for every node with "xen,path" property,
but this path is intended to perform not only IOMMU, but also IRQs configuration.
The DT node, pointed by "xen,path", may not be IOMMU protected.

In both above cases call pattern is:

     res = iommu_add_dt_device(node);
     if ( res < 0 )
	return res;

     if ( !dt_device_is_protected(node) )
          return 0; //no failure

     res =  iommu_assign_dt_device(domain, node);
     if ( res )
	return res;

And only when XEN_DOMCTL_assign_device (iommu_do_dt_domctl()) is handled the call pattern is
different and there is no check for dt_device_is_protected().

> 
> That's a good point. I think we should do the same for
> 
>   > if ( !is_iommu_enabled(d) )
> 
>   >  return -EINVAL;
> 
> because in this case we should process sci as well. I will do the change

-- 
Best regards,
-grygorii

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Jan Beulich 5 months, 1 week ago

On 22.05.2025 02:25, Stefano Stabellini wrote:
> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>> +                                    domctl->u.assign_device.u.dt.size, &dev);
>> +        if ( ret )
>> +            return ret;
>> +
>> +        ret = sci_assign_dt_device(d, dev);
>> +        if ( ret )
>> +            break;
>> +
>> +        break;
>> +    default:
>> +        /* do not fail here as call is chained with iommu handling */
> 
> It looks like this should be an error

The way the hooking is done at the call site, I think it can't be an error
here. But I previously questioned the way that hooking is done.

Jan

Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

Posted by Jan Beulich 5 months, 2 weeks ago

On 19.05.2025 17:50, Oleksii Moisieiev wrote:
> --- a/xen/arch/arm/firmware/sci.c
> +++ b/xen/arch/arm/firmware/sci.c
> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
>      return 0;
>  }
>  
> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    struct dt_device_node *dev;
> +    int ret = 0;
> +
> +    switch ( domctl->cmd )
> +    {
> +    case XEN_DOMCTL_assign_device:
> +        ret = -EOPNOTSUPP;
> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> +            break;
> +
> +        if ( !cur_mediator )
> +            break;
> +
> +        if ( !cur_mediator->assign_dt_device )
> +            break;
> +
> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
> +                                    domctl->u.assign_device.u.dt.size, &dev);
> +        if ( ret )
> +            return ret;
> +
> +        ret = sci_assign_dt_device(d, dev);
> +        if ( ret )
> +            break;

These two lines are pointless when directly followed by ...

> +
> +        break;

... this. Misra calls such "dead code" iirc.

> --- a/xen/arch/arm/include/asm/firmware/sci.h
> +++ b/xen/arch/arm/include/asm/firmware/sci.h
> @@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
>   * control" functionality.
>   */
>  int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
> +
> +/*
> + * SCI domctl handler
> + *
> + * Only XEN_DOMCTL_assign_device is handled for now.
> + */
> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>  #else
>  
>  static inline bool sci_domain_is_enabled(struct domain *d)
> @@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
>      return 0;
>  }
>  
> +static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    return 0;
> +}
> +
>  #endif /* CONFIG_ARM_SCI */
>  
>  #endif /* __ASM_ARM_SCI_H */

This being an Arm-specific header, how does ...

> @@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      case XEN_DOMCTL_deassign_device:
>      case XEN_DOMCTL_get_device_group:
>          ret = iommu_do_domctl(op, d, u_domctl);
> +
> +        if ( !ret || ret == -EOPNOTSUPP )
> +        {
> +            int ret1;
> +            /*
> +             * Add chained handling of assigned DT devices to support
> +             * access-controller functionality through SCI framework, so
> +             * DT device assign request can be passed to FW for processing and
> +             * enabling VM access to requested device.
> +             * The access-controller DT device processing is chained after IOMMU
> +             * processing and expected to be executed for any DT device
> +             * regardless if DT device is protected by IOMMU or not (or IOMMU
> +             * is disabled).
> +             */
> +            ret1 = sci_do_domctl(op, d, u_domctl);

... this compile on non-Arm? I think I said so before: I don't like this
sitting in common code anyway. Is there really no way to put it in Arm-
specific code?

Jan

[RFC PATCH v4 1/8] xen/arm: add generic SCI subsystem
[RFC PATCH v4 2/8] xen/arm: scmi-smc: update to be used under sci subsystem
[RFC PATCH v4 3/8] xen/arm: scmi-smc: passthrough SCMI SMC to domain, single agent
[RFC PATCH v4 4/8] docs: arm: add docs for SCMI over SMC calls forwarding driver
[RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu
[RFC PATCH v4 6/8] xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver
[RFC PATCH v4 7/8] docs: arm: add SCI SCMI SMC multi-agent driver docs
[RFC PATCH v4 8/8] docs: arm: proposal to add separate SCMI node for Xen agent