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 17:10, Oleksii Moisieiev wrote:
> 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".
>>
> 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.

Quite possible, yet does that somehow eliminate the need to address it? I'm
trying to understand why the respective PCI code isn't being touched.

Jan