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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.01.2026 19:29, Oleksii Moisieiev wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>

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