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

[Jan Beulich <jbeulich@suse.com>]

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