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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

Jan