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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

Hi Jan,

Sorry for a long silence. Please see my answers below:

On 06/11/2025 12:09, Jan Beulich wrote:
> On 01.11.2025 12:56, Oleksii Moisieiev wrote:
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -29,6 +29,7 @@
>>   #include <xen/xvmalloc.h>
>>   
>>   #include <asm/current.h>
>> +#include <asm/firmware/sci.h>
>>   #include <asm/irq.h>
>>   #include <asm/page.h>
>>   #include <asm/p2m.h>
> Does this build at all on non-Arm?
Good finding. Thanks - will fix.
>> @@ -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.

- 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.

BR,

Oleksii
>
> Jan