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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
> Hi Stefano,
> 
> I'm very sorry for a long silence. Please see my answers below:
> 
> On 22/05/2025 03:25, Stefano Stabellini wrote:
> > 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?
> 
> The -EOPNOTSUPP code is used because this is part of a chained call after
> iommu_do_domctl, as stated in xen/common/domctl.c:859. The 
> XEN_DOMCTL_assign_device
> call is expected to handle any DT device, regardless of whether the DT 
> device is
> protected by an IOMMU or if the IOMMU is disabled.
> The following cases are considered:
> 
> 1. IOMMU Protected Device (Success)
> 
> If the device is protected by the IOMMU and iommu_do_domctl returns 0, 
> we continue
> processing the DT device by calling sci_do_domctl.
> 
> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
> 
> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is 
> disabled,
> we still proceed to call sci_do_domctl.

OK this makes sense.  I think it is OK to have a special error code to
say "the IOMMU is disabled" but I don't know if it is a good idea to try
to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
configuration with domctl disabled, for instance.

It might be wiser to use a different error code. Maybe ENOENT?


> 3. Error from iommu_do_domctl (Fail State)
> 
> If iommu_do_domctl returns any error, the system enters a fail state, and
> sci_do_domctl is not called.
> 
> 4. -EOPNOTSUPP from sci_do_domctl
> 
> If sci_do_domctl returns -EOPNOTSUPP, this indicates one of the following:
> - The provided device is not a DT device.
> - There is no cur_mediator available (indicating that the SCI subsystem 
> is enabled
> in the configuration, but no mediator was provided).
> - The current mediator does not support assign_dt_device (this is 
> expected to be changed;
> see below for details).
> In this case, -EOPNOTSUPP is returned but will be ignored, and the 
> original return value from iommu_do_domctl will be used as the final result.

Same comment as before. We need to be careful not confuse this case you
described with other cases where sci_do_domctl is simply not
implemented.


> 5. Return Code from sci_do_domctl
> 
> If sci_do_domctl returns 0 (success) or an error code (failure),
> the return value from iommu_do_domctl is overridden, and the result from 
> sci_do_domctl is returned.
> Note: -EOPNOTSUPP from iommu_do_domctl will also be overridden since
> step 2 was successfully completed (or failed).
> >> +        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?
> 
> I think you are right that this case should return -EINVAL because we 
> should fail if mediator
> 
> without implemented mandatory features was provided. Will be fixed.
> 
> >> +        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
> 
> That's a good point. I think we should do the same for
> 
>  > if ( !is_iommu_enabled(d) )
> 
>  >  return -EINVAL;
> 
> because in this case we should process sci as well. I will do the change
> 
> >>           ret = iommu_assign_dt_device(d, dev);
> >>   
> >>           if ( ret )
> >> -- 
> >> 2.34.1
> >>