Re: [RFC PATCH v4 5/8] xen/domctl: extend XEN_DOMCTL_assign_device to handle not only iommu

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.05.2025 17:50, Oleksii Moisieiev wrote:
> --- a/xen/arch/arm/firmware/sci.c
> +++ b/xen/arch/arm/firmware/sci.c
> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct 
> dt_device_node *dev)
>      return 0;
>  }
>  
> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    struct dt_device_node *dev;
> +    int ret = 0;
> +
> +    switch ( domctl->cmd )
> +    {
> +    case XEN_DOMCTL_assign_device:
> +        ret = -EOPNOTSUPP;
> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> +            break;
> +
> +        if ( !cur_mediator )
> +            break;
> +
> +        if ( !cur_mediator->assign_dt_device )
> +            break;
> +
> +        ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
> +                                    domctl->u.assign_device.u.dt.size, &dev);
> +        if ( ret )
> +            return ret;
> +
> +        ret = sci_assign_dt_device(d, dev);
> +        if ( ret )
> +            break;

These two lines are pointless when directly followed by ...

> +
> +        break;

... this. Misra calls such "dead code" iirc.

> --- a/xen/arch/arm/include/asm/firmware/sci.h
> +++ b/xen/arch/arm/include/asm/firmware/sci.h
> @@ -146,6 +146,14 @@ int sci_dt_finalize(struct domain *d, void *fdt);
>   * control" functionality.
>   */
>  int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev);
> +
> +/*
> + * SCI domctl handler
> + *
> + * Only XEN_DOMCTL_assign_device is handled for now.
> + */
> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>  #else
>  
>  static inline bool sci_domain_is_enabled(struct domain *d)
> @@ -195,6 +203,12 @@ static inline int sci_assign_dt_device(struct domain *d,
>      return 0;
>  }
>  
> +static inline int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                                XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
> +{
> +    return 0;
> +}
> +
>  #endif /* CONFIG_ARM_SCI */
>  
>  #endif /* __ASM_ARM_SCI_H */

This being an Arm-specific header, how does ...

> @@ -851,6 +852,24 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>      case XEN_DOMCTL_deassign_device:
>      case XEN_DOMCTL_get_device_group:
>          ret = iommu_do_domctl(op, d, u_domctl);
> +
> +        if ( !ret || ret == -EOPNOTSUPP )
> +        {
> +            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 after 
> IOMMU
> +             * processing 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);

... this compile on non-Arm? I think I said so before: I don't like this
sitting in common code anyway. Is there really no way to put it in Arm-
specific code?

Jan