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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

On 12/01/2026 17:56, Jan Beulich wrote:
> 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
I have reviewed the previous versions of the patch serie and could not 
find any questions related to PCI prior to this series. Therefore, "How 
are DT and PCI different in this regard?" appears to be the first 
question concerning PCI.

BR,
Oleksii.