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
