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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

On 26/06/2025 17:41, Jan Beulich wrote:
> 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 was talking about the following code in iommu_do_pci_domctl:

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

Sorry, I misinterpreted this.

>> 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.
>
I think I could use -ENXIO since it has the following comment:
     /* No such device or address */
It seems to be suitable for our case.

WBR,

Oleksii.