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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2025 15:07, Oleksii Moisieiev wrote:
> 
> On 26/06/2025 09:10, Jan Beulich wrote:
>> 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.)
> 
> Maybe -ENODEV is a better choice because iommu_do_pci_domctl and
> iommu_do_dt_domctl return this
> 
> code when some feature is not supported.

What feature are you talking about? All I see in the former is

        ret = -ENODEV;
        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
            break;

and there -ENODEV is quite appropriate.

> I think -EIO or -ENXIO aren’t suitable here since we’re planning to send
> this message when the IOMMU is disabled.

Well, I don't like those two very much either for the use here, but I still
view them as better than ENOENT.

Jan