On 14.01.2026 19:29, Oleksii Moisieiev wrote:
> From: Grygorii Strashko <[email protected]>
Nit on the patch title: "not only iommu" can mean pretty much anything. Imo
SCI should somehow appear in the title.
> Add chained handling of assigned DT devices to support access-controller
> functionality through SCI framework, so a DT device assign request can be
> passed to firmware for processing and enabling VM access to the requested
> device (for example, device power management through SCMI).
>
> The SCI access-controller DT device processing is called before the IOMMU
> path. It runs for any DT-described device (protected or not, and even when
> the IOMMU is disabled). The IOMMU path remains unchanged for PCI devices;
> only the DT path is relaxed to permit non-IOMMU devices.
>
> This lets xl.cfg:"dtdev" list both IOMMU-protected and non-protected DT
> devices:
>
> dtdev = [
> "/soc/video@e6ef0000", <- IOMMU protected device
> "/soc/i2c@e6508000", <- not IOMMU protected device
> ]
>
> The change is done in two parts:
> 1) call sci_do_domctl() in do_domctl() before IOMMU processing and propagate
> its error if it fails while the IOMMU path succeeds (unhandled cases return
> -ENXIO and are ignored);
> 2) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
> not fail if DT device is not protected by IOMMU. iommu_do_pci_domctl
> doesn't need to be updated because iommu_do_domctl first tries
> iommu_do_pci_domctl (when CONFIG_HAS_PCI) and falls back to
> iommu_do_dt_domctl only if PCI returns -ENODEV.
Given how the line above ends early, you likely want to have a paragraph
(empty line) past here, to aid the reader?
> The new dt_device_is_protected() bypass in iommu_do_dt_domctl only
> applies to DT-described devices; SCI parameters are carried via DT nodes.
> PCI devices handled by iommu_do_pci_domctl do not carry DT/SCI
> metadata in this path, so there is no notion of “SCI parameters on a
> non-IOMMU-protected PCI device” for it to interpret or to skip. The
> PCI path should continue to report errors if assignment cannot be
> performed by the IOMMU layer.
It's less clear here, as ...
> So we should leave iommu_do_pci_domctl unchanged; the SCI/DT-specific
> relaxations belong only in the DT path.
... this still looks to pertain to the earlier paragraph. Perhaps that
sentence should start on the earlier line?
A more general nit: Please try to be consistent with line wrapping.
Don't go past 75 chars, but at the same time use available line length
instead of wrapping a sentence early.
Also please be consistent with adding () after function names.
> @@ -827,7 +830,37 @@ 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:
> + if ( IS_ENABLED(CONFIG_ARM) )
> + {
> + /*
> + * Add chained handling of assigned DT devices to support
Why "Add"? The comment should be describing what the code does, not what is
being changed by this patch.
> + * 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 called 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).
> + */
Is there perhaps one (or more) comma(s) missing here? One after "processing",
and maybe another one after "code". Assuming of course I infer correctly what
is intended to be said.
> + ret1 = sci_do_domctl(op, d, u_domctl);
> + if ( ret1 < 0 )
> + return ret1;
This leaves ret1 >= 0 for the code further down. Then ...
> + }
> + else
> + ret1 = -ENXIO;
> +
> ret = iommu_do_domctl(op, d, u_domctl);
> + if ( ret < 0 )
> + return ret;
> +
> + /*
> + * If IOMMU processing was successful, check for SCI processing
> return
> + * code and if it was failed then overwrite the return code to
> propagate
> + * SCI failure back to caller.
> + */
> + if ( ret1 != -ENXIO )
> + ret = ret1;
..., with ret being 0 when we make it here, what use is this? I don't think
we can or should be returning positive values?
Nit for the comment: Drop the latter "was".
Jan
