Re: [RFC PATCH v3 1/7] xen/arm: add generic SCI subsystem

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi Stefano,

Thanks for your comments.

On 14.03.25 01:48, Stefano Stabellini wrote:
> On Tue, 11 Mar 2025, Grygorii Strashko wrote:
> > From: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> >
> > This patch adds the basic framework for ARM SCI mediator. SCI is System
> > Control Interface, which is designed to redirect requests from the Domains
> > to ARM specific Firmware (for example SCMI). This will allow the devices,
> > passed-through to the different Domains, to access to the System resources
> > (such as clocks/resets etc) by sending requests to the firmware.
> >
> > ARM SCI subsystem allows to implement different SCI drivers to handle
> > specific ARM firmware interfaces (like ARM SCMI) and mediate requests
> > between the Domains and the Firmware. Also it allows SCI drivers to perform
> > proper action during Domain creation/destruction which is vital for
> > handling use cases like Domain reboot.
> >
> > This patch introduces new DEVICE_ARM_SCI device subclass for probing SCI
> > drivers basing on device tree, SCI drivers register itself with
> > DT_DEVICE_START/END macro. On init - the SCI drivers should register its
> > SCI ops with sci_register(). Only one SCI driver can be supported.
> >
> > At run-time, the following SCI API calls are introduced:
> >
> > - sci_domain_sanitise_config() called from arch_sanitise_domain_config()
> > - sci_domain_init() called from arch_domain_create()
> > - sci_relinquish_resources() called from domain_relinquish_resources()
> > - sci_domain_destroy() called from arch_domain_destroy()
> > - sci_handle_call() called from vsmccc_handle_call()
> > - sci_dt_handle_node()
> >    sci_dt_finalize() called from handle_node() (Dom0 DT)
> >
> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> > Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> > ---
> >   MAINTAINERS                             |   6 +
> >   xen/arch/arm/device.c                   |   5 +
> >   xen/arch/arm/dom0less-build.c           |  13 ++
> >   xen/arch/arm/domain.c                   |  12 +-
> >   xen/arch/arm/domain_build.c             |   8 +
> >   xen/arch/arm/firmware/Kconfig           |   8 +
> >   xen/arch/arm/firmware/Makefile          |   1 +
> >   xen/arch/arm/firmware/sci.c             | 187 +++++++++++++++++++++
> >   xen/arch/arm/include/asm/domain.h       |   5 +
> >   xen/arch/arm/include/asm/firmware/sci.h | 214 ++++++++++++++++++++++++
> >   xen/arch/arm/vsmc.c                     |   3 +
> >   xen/common/domctl.c                     |  13 ++
> >   xen/drivers/passthrough/device_tree.c   |   7 +
> >   xen/include/asm-generic/device.h        |   1 +
> >   xen/include/public/arch-arm.h           |   4 +
> >   15 files changed, 486 insertions(+), 1 deletion(-)
> >   create mode 100644 xen/arch/arm/firmware/sci.c
> >   create mode 100644 xen/arch/arm/include/asm/firmware/sci.h
[...]

> > +/*
> > + * Generic part of the SCI (System Control Interface) subsystem.
> > + *
> > + * Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> > + * Copyright (c) 2025 EPAM Systems
> > + */
> > +
> > +#include <xen/acpi.h>
> > +#include <xen/errno.h>
> > +#include <xen/init.h>
> > +#include <xen/sched.h>
> > +#include <xen/types.h>
> > +
> > +#include <asm/firmware/sci.h>
> > +
> > +static const struct sci_mediator_ops __read_mostly *cur_mediator;
> > +
> > +int sci_register(const struct sci_mediator_ops *ops)
> > +{
> > +    if ( cur_mediator )
> > +        return -EEXIST;
> > +
> > +    if ( !ops->domain_init || !ops->domain_destroy || !ops->handle_call )
> > +        return -EINVAL;
> > +
> > +    cur_mediator = ops;
> > +
> > +    return 0;
> > +};
> > +
> > +bool sci_handle_call(struct cpu_user_regs *args)
> > +{
> > +    if ( unlikely(!cur_mediator) )
> > +        return false;
> > +
> > +    return cur_mediator->handle_call(args);
> > +}
> > +
> > +int sci_domain_init(struct domain *d, struct xen_domctl_createdomain *config)
> > +{
> > +    if ( !cur_mediator )
> > +        return 0;
> > +
> > +    return cur_mediator->domain_init(d, config);
> > +}
> > +
> > +int sci_domain_sanitise_config(struct xen_domctl_createdomain *config)
> > +{
> > +    if ( !cur_mediator )
> > +        return 0;
> > +
> > +    if ( !cur_mediator->domain_sanitise_config )
> > +        return 0;
> > +
> > +    return cur_mediator->domain_sanitise_config(config);
> > +}
> > +
> > +void sci_domain_destroy(struct domain *d)
> > +{
> > +    if ( !cur_mediator )
> > +        return;
> > +
> > +    cur_mediator->domain_destroy(d);
> > +}
> > +
> > +int sci_relinquish_resources(struct domain *d)
> > +{
> > +    if ( !cur_mediator )
> > +        return 0;
> > +
> > +    if ( !cur_mediator->relinquish_resources )
> > +        return 0;
> > +
> > +    return cur_mediator->relinquish_resources(d);
> > +}
> > +
> > +bool sci_dt_handle_node(struct domain *d, struct dt_device_node *node)
> > +{
> > +    if ( !cur_mediator )
> > +        return 0;
> > +
> > +    if ( !cur_mediator->dom0_dt_handle_node )
> > +        return 0;
> > +
> > +    return cur_mediator->dom0_dt_handle_node(d, node);
> > +}
> > +
> > +int sci_dt_finalize(struct domain *d, void *fdt)
> > +{
> > +    if ( !cur_mediator )
> > +        return 0;
> > +
> > +    if ( !cur_mediator->dom0_dt_finalize )
> > +        return 0;
> > +
> > +    return cur_mediator->dom0_dt_finalize(d, fdt);
> > +}
> > +
> > +int sci_assign_dt_device(struct domain *d, struct dt_device_node *dev)
> > +{
> > +    struct dt_phandle_args ac_spec;
> > +    int index = 0;
> > +    int ret;
> > +
> > +    if ( !cur_mediator )
> > +        return 0;
> > +
> > +    if ( !cur_mediator->assign_dt_device )
> > +        return 0;
> > +
> > +    while ( !dt_parse_phandle_with_args(dev, "access-controllers",
> > +                                        "#access-controller-cells", index,
> > +                                        &ac_spec) )
> > +    {
> > +        printk(XENLOG_DEBUG "sci: assign device %s to %pd\n",
> > +               dt_node_full_name(dev), d);
> > +
> > +        ret = cur_mediator->assign_dt_device(d, &ac_spec);
> > +        if ( ret )
> > +            return ret;
> I am confused by this: we are passing a reference to the controller
> rather than to the device to be assigned?
At this moment the Host DT is parsed as specified by access controllers bindings

        access-controllers = <&sci_fw [parameters]>;

and "ac_spec" contains
- np : access controller node, which should correspond SCI FW device node if it 
serves as
  access controller.
- args_count/args : DT device parameters to be passed to access controllers

In case of, SCI SCMI that's enough to perform device assignment - args[0] == 
device_id


> > +        index++;
> > +    }
> > +
> > +    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:
> > +        if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> > +        {
> > +            ret = -EINVAL;
> > +            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;
> > +
> > +        break;
> > +    default:
> > +        /* do not fail here as call is chained with iommu handling */
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int __init sci_init(void)
> > +{
> > +    struct dt_device_node *np;
> > +    unsigned int num_sci = 0;
> > +    int rc;
> > +
> > +    dt_for_each_device_node(dt_host, np)
> > +    {
> > +        rc = device_init(np, DEVICE_ARM_SCI, NULL);
> > +        if ( !rc && num_sci )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "SCMI: Only one SCI controller is supported. found second 
> > %s\n",
> > +                   np->name);
> > +            return -EOPNOTSUPP;
> > +        }
> > +        else if ( !rc )
> > +            num_sci++;
> > +        else if ( rc != -EBADF && rc != -ENODEV )
> > +            return rc;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +__initcall(sci_init);
> > diff --git a/xen/arch/arm/include/asm/domain.h 
> > b/xen/arch/arm/include/asm/domain.h
> > index f1d72c6e48df..fa0898b7cf80 100644
> > --- a/xen/arch/arm/include/asm/domain.h
> > +++ b/xen/arch/arm/include/asm/domain.h
> > @@ -118,6 +118,11 @@ struct arch_domain
> >   #ifdef CONFIG_TEE
> >       void *tee;
> >   #endif
> > +#ifdef CONFIG_ARM_SCI
> > +    bool sci_enabled;
> > +    /* ARM SCI driver's specific data */
> > +    void *sci_data;
> > +#endif
> >   
> >   }  __cacheline_aligned;
> >   
[...]

> > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> > index 62d8117a120c..51b3c0297314 100644
> > --- a/xen/arch/arm/vsmc.c
> > +++ b/xen/arch/arm/vsmc.c
> > @@ -12,6 +12,7 @@
> >   #include <public/arch-arm/smccc.h>
> >   #include <asm/cpuerrata.h>
> >   #include <asm/cpufeature.h>
> > +#include <asm/firmware/sci.h>
> >   #include <asm/monitor.h>
> >   #include <asm/regs.h>
> >   #include <asm/smccc.h>
> > @@ -300,6 +301,8 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
> >               break;
> >           case ARM_SMCCC_OWNER_SIP:
> >               handled = handle_sip(regs);
> > +            if ( !handled )
> > +                handled = sci_handle_call(regs);
> Isn't there a proper funcid range for SCI calls?
Unfortunately no. The ARM DEN 0028B "SMC CALLING CONVENTION" spec only defines 
the
range

Table 6-2
0x82000000-0x8200FFFF SMC32: SiP Service Calls
0xC2000000-0xC200FFFF SMC64: SiP Service Calls

And in Linux Kernel mainline I can see:

./arm64/boot/dts/freescale/s32g2.dtsi:          arm,smc-id = <0xc20000fe>;
./arm64/boot/dts/freescale/s32g3.dtsi:          arm,smc-id = <0xc20000fe>;
./arm64/boot/dts/freescale/imx8ulp.dtsi:        arm,smc-id = <0xc20000fe>;
./arm64/boot/dts/amlogic/amlogic-c3.dtsi:       arm,smc-id = <0x820000C1>;
./arm64/boot/dts/st/stm32mp251.dtsi:            arm,smc-id = <0xb200005a>;
./arm64/boot/dts/blaize/blaize-blzp1600.dtsi:   arm,smc-id = <0x82002000>;
./arm64/boot/dts/rockchip/rk356x-base.dtsi:     arm,smc-id = <0x82000010>;
./arm64/boot/dts/rockchip/rk3588-base.dtsi:     arm,smc-id = <0x82000010>;
./arm64/boot/dts/rockchip/rk3576.dtsi:          arm,smc-id = <0x82000010>;


> >               break;
> >           case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END:
> >           case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END:
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index 05abb581a03d..b48ad20a6e2b 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,18 @@ 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 >= 0 || (ret == -EOPNOTSUPP) || (ret == -ENODEV) )
> > +        {
> > +            /*
> > +             * TODO: RFC
> > +             * This change will allow to pass DT nodes/devices to
> > +             * XEN_DOMCTL_assign_device OP using xl.cfg:"dtdev" property even
> > +             * if those DT nodes/devices even are not behind IOMMU (or IOMMU
> > +             * is disabled) without failure.
> > +             */
> > +            ret = sci_do_domctl(op, d, u_domctl);
> > +        }
> >           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 075fb25a3706..f2ee0a72f541 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -318,6 +318,13 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct 
> > domain *d,
> >               break;
> >           }
> >   
> > +        /* TODO: RFC allow assignment of devices without IOMMU protection. */
> > +        if ( !dt_device_is_protected(dev) )
> > +        {
> > +            ret = 0;
> > +            break;
> > +        }
> This should not be needed, there is a similar check at the beginning of
> iommu_assign_dt_device
Unfortunately no, as iommu_assign_dt_device() checks it as

    if ( !dt_device_is_protected(dev) )
        return -EINVAL;

and returns -EINVAL for DT devices which are not IOMMU-protected,
while iommu_add_dt_device() returns 1 in such cases (few lines above).
Therefore this change which in combination with do_domctl() change allows to
pass DT devices in xl.cfg:"dtdev" for processing.

In general, there are three places where DT IOMMU is configured for ARM

1) arch\arm\device.c handle_device() - used for Dom0/hwdom init and dt-overlays
2) arch\arm\dom0less-build.c handle_passthrough_prop() - used for dom0less DT 
devices pass through
3) drivers/passthrough/device_tree.c iommu_do_dt_domctl() - above

In cases (1) and (2), the code will not fail if DT device is not 
IOMMU-protected and
there is the following calling pattern:

        res = iommu_add_dt_device(dev);
        if ( res < 0 )
            return res;

// if DT device is not IOMMU-protected res == 1
// and dt_device_is_protected(dev) == false

        if ( !dt_device_is_protected(dev) )
                return 0;

        res = iommu_assign_dt_device(d, dev);


The case (3), reviewed here, has different calling pattern:
iommu_do_dt_domctl():
        ret = iommu_add_dt_device(dev);
        if ( ret < 0 )
           return ret;

// if DT device is not IOMMU-protected ret == 1
// and dt_device_is_protected(dev) == false

        ret = iommu_assign_dt_device(d, dev);

and will always fail if DT device is not IOMMU-protected.


 
> >           ret = iommu_assign_dt_device(d, dev);
> >   
> >           if ( ret )
[...]

--
Best regards,
-grygorii