Re: [RFC PATCH v3 3/7] xen/arm: scmi-smc: passthrough SCMI SMC to guest domain

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 14.03.25 01:48, Stefano Stabellini wrote:
> On Tue, 11 Mar 2025, Grygorii Strashko wrote:
> > The commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI over SMC calls
> > handling layer") introduces simple driver which forwards SCMI over SMC
> > calls from hwdom/dom0 to EL3 firmware (TF-A) with a single SCMI OSPM agent
> > support. While it working gracefully for hwdom/dom0 use case it doesn't
> > cover "thin Dom0 with guest domain, which serves as Driver domain"
> > use-case. In this case HW need to be enable in Driver domain and dom0 is
> > performing only control functions.
> >
> > The EL3 SCMI firmware (TF-A) with a single SCMI OSPM agent support is
> > pretty generic case for the default vendors SDK and new platforms.
> >
> > This patch enables passthrough of SCMI SMC single agent interface to the
> > Driver domain which can be enabled in the following way:
> >
> >   - dom0: add dom0_scmi_smc_passthrough to the Xen Command Line
> >   - domD: xl.cfg add "arm_sci" option as below
> >
> >     arm_sci = "type=scmi_smc"
> >
> >   - domD: xl.cfg enable access to the "arm,scmi-shmem"
> >
> > iomem = [
> >      "47ff0,1@22001",
> > ]
> >
> >   - domD: add scmi nodes to the Driver domain partial device tree as in the
> >   below example:
> >
> > passthrough {
> >     scmi_shm_0: sram@22001000 {
> >         compatible = "arm,scmi-shmem";
> >         reg = <0x0 0x22001000 0x0 0x1000>;
> >     };
> >
> >     firmware {
> >          compatible = "simple-bus";
> >              scmi: scmi {
> >                  compatible = "arm,scmi-smc";
> >                  shmem = <&scmi_shm_0>;
> >                  ...
> >              }
> >      }
> > }
> >
> > The SCMI SMC single agent interface can be enabled for one and only one
> > domain. In general, the configuration is similar to any other HW
> > passthrough, except explicitly enabling SCMI with "arm_sci" xl.cfg option.
> >
> > Note that SCMI and "arm,scmi-shmem" nodes will be removed from
> > dom0 DT.
> >
> > Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> > ---
> >   docs/man/xl.cfg.5.pod.in          | 36 ++++++++++++++
> >   docs/misc/xen-command-line.pandoc |  9 ++++
> >   tools/include/libxl.h             |  5 ++
> >   tools/libs/light/libxl_arm.c      | 13 ++++++
> >   tools/libs/light/libxl_types.idl  | 10 ++++
> >   tools/xl/xl_parse.c               | 66 ++++++++++++++++++++++++++
> >   xen/arch/arm/firmware/Kconfig     |  4 +-
> >   xen/arch/arm/firmware/scmi-smc.c  | 78 +++++++++++++++++++++++++++++--
> >   8 files changed, 217 insertions(+), 4 deletions(-)
[...]

> >   /*
> >    * Check if provided SMC Function Identifier matches the one known by the SCMI
> > @@ -50,7 +56,7 @@ static bool scmi_handle_smc(struct cpu_user_regs *regs)
> >           return false;
> >   
> >       /* Only the hardware domain should use SCMI calls */
> > -    if ( !is_hardware_domain(current->domain) )
> > +    if ( scmi_dom != current->domain )
> >       {
> >           gdprintk(XENLOG_WARNING, "SCMI: Unprivileged access attempt\n");
> >           return false;
> > @@ -78,9 +84,18 @@ static bool scmi_handle_smc(struct cpu_user_regs *regs)
> >   static int scmi_smc_domain_init(struct domain *d,
> >                                   struct xen_domctl_createdomain *config)
> >   {
> > -    if ( !is_hardware_domain(d) )
> > +    if ( !opt_dom0_scmi_smc_passthrough && !is_hardware_domain(d) )
> > +        return 0;
> > +
> > +    if ( opt_dom0_scmi_smc_passthrough &&
> > +         (config->arch.arm_sci_type != XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC ||
> I am confused by the check "config->arch.arm_sci_type !=
> XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC". If the check was true, this
> function should not be called?
I'll tried to hide all SCI specifics inside SCI code, so outside SCi only API 
calls
are present (Patch 1).

> > +          is_hardware_domain(d)) )
> >           return 0;
> >   
> > +    if ( scmi_dom )
> > +        return -EEXIST;
> > +
> > +    scmi_dom = d;
> >       d->arch.sci_enabled = true;
> >       printk(XENLOG_DEBUG "SCMI: %pd init\n", d);
> >       return 0;
> > @@ -88,12 +103,68 @@ static int scmi_smc_domain_init(struct domain *d,
> >   
> >   static void scmi_smc_domain_destroy(struct domain *d)
> >   {
> > -    if ( !is_hardware_domain(d) )
> > +    if ( scmi_dom && scmi_dom != d )
> >           return;
> >   
> > +    scmi_dom = NULL;
> > +    d->arch.sci_enabled = false;
> >       printk(XENLOG_DEBUG "SCMI: %pd destroy\n", d);
> >   }
> >   
> > +/*
> > + * Handle Dom0 SCMI SMC specific DT nodes
> > + *
> > + * Copy SCMI nodes into Dom0 device tree if dom0_scmi_smc_passthrough=false.
> I am confused by this: shouldn't scmi_smc_dt_handle_node be part of the
> previous patch? Otherwise, how can it work for dom0?
Previous patch doesn't change functionality introduced by original
commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI over SMC calls handling
layer"). Previous patch just makes it part of SCI subsystem without changing
major functionality.

The original functionality:
- if there is SCMI node in the Host DT then enable it for dom0/hwdom
- SCMI DT node will be copied to Dom0/hwdom in generic way as any other DT 
devices.


This patch enables coping of SCMI nodes in dom0/hwdom DT only if 
dom0_scmi_smc_passthrough=false,
otherwise scmi and scmi_shmem modes are removed.

> > + */
> > +static bool scmi_smc_dt_handle_node(struct domain *d,
> > +                                    struct dt_device_node *node)
> > +{
> > +    static const struct dt_device_match shmem_matches[] __initconst = {
> > +        DT_MATCH_COMPATIBLE("arm,scmi-shmem"),
> > +        { /* sentinel */ },
> > +    };
> > +    static const struct dt_device_match scmi_matches[] __initconst = {
> > +        DT_MATCH_PATH("/firmware/scmi"),
> > +        { /* sentinel */ },
> > +    };
> > +
> > +    if ( dt_match_node(shmem_matches, node) && !sci_domain_is_enabled(d) )
> > +    {
> > +        dt_dprintk("  Skip scmi shmem node\n");
> > +        return true;
> > +    }
> > +
> > +    if ( dt_match_node(scmi_matches, node) && !sci_domain_is_enabled(d) )
It is correct...

> This seems wrong: we are allowing access to the shmem region if
> !sci_domain_is_enabled(d). Shouldn't it be:
>
>    if ( dt_match_node(scmi_matches, node) && sci_domain_is_enabled(d) )
>
>
> > +    {
> > +        struct dt_device_node *shmem_node;
> > +        const __be32 *prop;
> > +        u64 paddr, size;
> > +        int ret;
> > +
> > +        dt_dprintk("  Skip scmi node\n");
(this is confusing left over, sorry)

> > +
> > +        prop = dt_get_property(node, "shmem", NULL);
> > +        if ( !prop )
> > +            return true;
> > +
> > +        shmem_node = dt_find_node_by_phandle(be32_to_cpup(prop));
> > +        if ( !shmem_node )
> > +            return true;
> > +
> > +        ret = dt_device_get_address(shmem_node, 0, &paddr, &size);
> > +        if ( ret )
> > +            return true;
> > +
> > +        ret = iomem_permit_access(d, paddr_to_pfn(paddr),
> > +                                  paddr_to_pfn(paddr + size - 1));
> > +
... even if SCMI node removed from dom0/hwdom DT - it's required to add
scmi_shmem IO range to dom0/hwdom to allow scmi_shmem configuration from
xl.cfg for other domain.

> > +        return true;
... and here node removed

> > +    }
> > +
> > +    return false;
> > +}
> > +
> >   static int __init scmi_check_smccc_ver(void)
> >   {
> >       if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
> > @@ -110,6 +181,7 @@ static const struct sci_mediator_ops scmi_smc_ops = {
> >       .handle_call = scmi_handle_smc,
> >       .domain_init = scmi_smc_domain_init,
> >       .domain_destroy = scmi_smc_domain_destroy,
> > +    .dom0_dt_handle_node = scmi_smc_dt_handle_node,
> >   };
> >   
> >   /* Initialize the SCMI layer based on SMCs and Device-tree */
> > --
> > 2.34.1
--
Best regards,
-grygorii