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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

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@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.
> 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
>>> >