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.)
>
> Jan
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.
I think -EIO or -ENXIO aren’t suitable here since we’re planning to send
this message when the IOMMU is disabled.
What do you think?
WBR,
Oleksii.
