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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.06.2025 10:42, Oleksii Moisieiev wrote:
> Adding Roger and Jan to the conversation.
> 
> Please see below.

Why is this? I did answer that question at the bottom already.

Jan

> On 23/06/2025 00:30, Stefano Stabellini wrote:
>> On Thu, 19 Jun 2025, Oleksii Moisieiev wrote:
>>> On 18/06/2025 03:04, Stefano Stabellini wrote:
>>>> On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
>>>>> Hi Stefano,
>>>>>
>>>>> I'm very sorry for a long silence. Please see my answers below:
>>>>>
>>>>> On 22/05/2025 03:25, Stefano Stabellini wrote:
>>>>>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
>>>>>>> From: Grygorii Strashko<grygorii_strashko@xxxxxxxx>
>>>>>>>
>>>>>>> Add chained handling of assigned DT devices to support access-controller
>>>>>>> functionality through SCI framework, so DT device assign request can be
>>>>>>> passed to FW for processing and enabling VM access to requested device
>>>>>>> (for example, device power management through FW interface like SCMI).
>>>>>>>
>>>>>>> The SCI access-controller DT device processing is chained after IOMMU
>>>>>>> processing and expected to be executed for any DT device regardless of 
>>>>>>> its
>>>>>>> protection by IOMMU (or if IOMMU is disabled).
>>>>>>>
>>>>>>> This allows to pass not only IOMMU protected DT device through
>>>>>>> xl.cfg:"dtdev" property for processing:
>>>>>>>
>>>>>>> dtdev = [
>>>>>>>        "/soc/video@e6ef0000", <- IOMMU protected device
>>>>>>>        "/soc/i2c@e6508000", <- not IOMMU protected device
>>>>>>> ]
>>>>>>>
>>>>>>> The change is done in two parts:
>>>>>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
>>>>>>> not fail if DT device is not protected by IOMMU
>>>>>>> 2) add chained call to sci_do_domctl() in do_domctl()
>>>>>>>
>>>>>>> Signed-off-by: Grygorii Strashko<grygorii_strashko@xxxxxxxx>
>>>>>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisieiev@xxxxxxxx>
>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>     xen/arch/arm/firmware/sci.c             | 37 
>>>>>>> +++++++++++++++++++++++++
>>>>>>>     xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>>>>>>>     xen/common/domctl.c                     | 19 +++++++++++++
>>>>>>>     xen/drivers/passthrough/device_tree.c   |  6 ++++
>>>>>>>     4 files changed, 76 insertions(+)
>>>>>>>
>>>>>>> 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.
>>>
>>> Maybe we should add commit author?
>> Roger and Jan might know