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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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