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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

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@xxxxxxxx>
>>
>> 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@xxxxxxxx>
>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@xxxxxxxx>
>> ---
>>
>>
>>
>>   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
>>