Re: [PATCH v6 1/5] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.01.2026 16:54, Oleksii Moisieiev wrote:
> 
> 
> On 12/01/2026 17:40, Jan Beulich wrote:
>> On 12.01.2026 16:16, Oleksii Moisieiev wrote:
>>> On 06/11/2025 12:09, Jan Beulich wrote:
>>>> On 01.11.2025 12:56, Oleksii Moisieiev wrote:
>>>>> @@ -827,7 +828,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>>>> u_domctl)
>>>>>        case XEN_DOMCTL_test_assign_device:
>>>>>        case XEN_DOMCTL_deassign_device:
>>>>>        case XEN_DOMCTL_get_device_group:
>>>>> +        int ret1;
>>>>> +
>>>>> +        /*
>>>>> +         * 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.
>>>>> +         * The access-controller DT device processing is chained before 
>>>>> IOMMU
>>>>> +         * processing preserving return code and expected to be executed 
>>>>> for
>>>>> +         * any DT device regardless if DT device is protected by IOMMU or
>>>>> +         * not (or IOMMU is disabled).
>>>>> +         */
>>>>> +        ret1 = sci_do_domctl(op, d, u_domctl);
>>>> Why would this not be the initializer of the new variable? (I also don't 
>>>> think
>>>> that we've decided to permit variable declarations at other than the top of
>>>> scopes or within e.g. a for() loop control construct.)
>>>>
>>> +
>>>>>            ret = iommu_do_domctl(op, d, u_domctl);
>>>>> +        if ( ret < 0 )
>>>>> +            return ret;
>>>> Why would you invoke both in all cases? If sci_do_domctl() handled the 
>>>> request,
>>>> there isn't any point in also invoking iommu_do_domctl(), is there? Or 
>>>> else is
>>>> there maybe some crucial aspect missing from the description (or not 
>>>> explicit
>>>> enough there for a non-SCI person like me)?
>>>>
>>>> Also this doesn't look to fit the description saying "The SCI 
>>>> access-controller
>>>> DT device processing is chained after IOMMU processing ..."
>>>>
>>> We call both because SCI and IOMMU cover different concerns and a DT
>>> device may need
>>> both: SCI for FW-mediated access control (power/clocks/reset) and IOMMU
>>> for DMA isolation.
>>> SCI returning success does not mean the IOMMU work is redundant.
>> Can the comment then please be updated to properly call out this dual
>> requirement?
> Yes, for sure.
>>> - sci_do_domctl() returns -ENXIO when it has nothing to do (non-DT, no
>>> mediator, mediator lacks assign hook).
>>> That is the “not handled by SCI” sentinel; in that case the code
>>> proceeds to IOMMU normally.
>>> -  When sci_do_domctl() succeeds (0), the device may still require IOMMU
>>> programming
>>> (e.g., DT device has an iommus property). Skipping iommu_do_domctl()
>>> would leave DMA isolation unprogrammed.
>>>
>>> The final if (ret1 != -ENXIO) ret = ret1; ensures that if both paths ran
>>> and IOMMU succeeded,
>>> an SCI failure is still reported to the caller.
>>>
>>> Device-tree examples to illustrate the dual roles:
>>> 1. Access-controlled DT device (not necessarily IOMMU-protected):
>>>
>>> i2c3: i2c@e6508000 {
>>>       compatible = "renesas,rcar-gen3-i2c";
>>>       reg = <0 0xe6508000 0 0x40>;
>>>       power-domains = <&scmi_pd 5>;      // FW-managed power domain
>>>       clocks = <&scmi_clk 12>;
>>>       clock-names = "fck";
>>>       access-controllers = <&scmi_xen 0>;
>>>       // no iommus property: SCI may need to authorize/power this device;
>>> IOMMU has nothing to do
>>> };
>>>
>>> 2. IOMMU-protected DT device that still may need SCI mediation:
>>> vpu: video@e6ef0000 {
>>>       compatible = "renesas,rcar-vpu";
>>>       reg = <0 0xe6ef0000 0 0x10000>;
>>>       iommus = <&ipmmu 0 0>;             // needs IOMMU mapping for DMA
>>> isolation
>>>       power-domains = <&scmi_pd 7>;      // FW-managed power/clock/reset
>>>       clocks = <&scmi_clk 34>;
>>>       access-controllers = <&scmi_xen 0>;
>>>       clock-names = "vpu";
>>> };
>>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>>> @@ -379,6 +379,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, 
>>>>> struct domain *d,
>>>>>                break;
>>>>>            }
>>>>>    
>>>>> +        if ( !dt_device_is_protected(dev) )
>>>>> +        {
>>>>> +            ret = 0;
>>>>> +            break;
>>>>> +        }
>>>>> +
>>>>>            ret = iommu_assign_dt_device(d, dev);
>>>>>    
>>>>>            if ( ret )
>>>> How are DT and PCI different in this regard?
>>> Please find examples above.
>> Sorry, but I can't spot anything PCI-ish in the examples above. Then again I
>> also no longer recall why I compared with PCI here. Oh, perhaps because the
>> PCI side isn't being modified at all.
>>
> Correct, pci code wasn't touched by these changes.

Yet my question boils down to "why", not "whether".

Jan