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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

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