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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 19 May 2025, Oleksii Moisieiev wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> 
> 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
> (for example, device power management through FW interface like SCMI).
> 
> The SCI access-controller DT device processing is chained after IOMMU
> processing and expected to be executed for any DT device regardless of its
> protection by IOMMU (or if IOMMU is disabled).
> 
> This allows to pass not only IOMMU protected DT device through
> xl.cfg:"dtdev" property for processing:
> 
> dtdev = [
>     "/soc/video@e6ef0000", <- IOMMU protected device
>     "/soc/i2c@e6508000", <- not IOMMU protected device
> ]
> 
> The change is done in two parts:
> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
> not fail if DT device is not protected by IOMMU
> 2) add chained call to sci_do_domctl() in do_domctl()
> 
> Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> ---
> 
> 
> 
>  xen/arch/arm/firmware/sci.c             | 37 +++++++++++++++++++++++++
>  xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
>  xen/common/domctl.c                     | 19 +++++++++++++
>  xen/drivers/passthrough/device_tree.c   |  6 ++++
>  4 files changed, 76 insertions(+)
> 
> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
> index e1522e10e2..8efd541c4f 100644
> --- 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;

Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?


> +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> +            break;

this one

> +        if ( !cur_mediator )
> +            break;

this one

> +        if ( !cur_mediator->assign_dt_device )
> +            break;

and also this one? It seems more like an -EINVAL as the caller used a
wrong parameter?


> +        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;
> +
> +        break;
> +    default:
> +        /* do not fail here as call is chained with iommu handling */

It looks like this should be an error


> +        break;
> +    }
> +
> +    return ret;
> +}
> +
>  static int __init sci_init(void)
>  {
>      struct dt_device_node *np;
> diff --git a/xen/arch/arm/include/asm/firmware/sci.h 
> b/xen/arch/arm/include/asm/firmware/sci.h
> index 71fb54852e..b8d1bc8a62 100644
> --- 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 */
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 05abb581a0..a74ee92067 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -27,6 +27,7 @@
>  #include <xen/vm_event.h>
>  #include <xen/monitor.h>
>  #include <asm/current.h>
> +#include <asm/firmware/sci.h>
>  #include <asm/irq.h>
>  #include <asm/page.h>
>  #include <asm/p2m.h>
> @@ -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 )

It is better to invert the check:

if ( ret < 0 && ret != -EOPNOTSUPP )
    return ret;


> +        {
> +            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);
> +            if ( ret1 != -EOPNOTSUPP )
> +                ret = ret1;
> +        }
>          break;
>  
>      case XEN_DOMCTL_get_paging_mempool_size:
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 075fb25a37..2624767e51 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -318,6 +318,12 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct 
> domain *d,
>              break;
>          }
>  
> +        if ( !dt_device_is_protected(dev) )
> +        {
> +            ret = 0;
> +            break;
> +        }

I am concerned about this: previously we would call
iommu_assign_dt_device and the same check at the beginning of
iommu_assign_dt_device would return -EINVAL. Now it is a success.

I am not sure this is appropriate. I wonder if instead we should:

- remove this chunk from the patch
- change the return error for !dt_device_is_protected at the top of
  iommu_assign_dt_device from -EINVAL to -EOPNOTSUPP
- this would fall into the same ret != -EOPNOTSUPP check after
  iommu_do_domctl


>          ret = iommu_assign_dt_device(d, dev);
>  
>          if ( ret )
> -- 
> 2.34.1
>