From: Grygorii Strashko <grygorii_strashko@epam.com>

The introduced SCI (System Control Interface) subsystem provides unified
interface to integrate in Xen SCI drivers which adds support for ARM
firmware (EL3, SCP) based software interfaces (like SCMI) that are used in
system management. The SCI subsystem allows to add drivers for different FW
interfaces or have different drivers for the same FW interface (for example,
SCMI with different transports).

This patch updates SCMI over SMC calls handling layer, introduced by
commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI over SMC calls handling
layer"), to be SCI driver:
- convert to DT device;
- convert to SCI Xen interface.

There are no functional changes in general, the driver is just adopted
to the SCI interface.

Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---

Changes in v6:
- add R-b tag

 xen/arch/arm/firmware/Kconfig                | 13 ++-
 xen/arch/arm/firmware/scmi-smc.c             | 93 +++++++++++---------
 xen/arch/arm/include/asm/firmware/scmi-smc.h | 41 ---------
 xen/arch/arm/vsmc.c                          |  5 +-
 xen/include/public/arch-arm.h                |  1 +
 5 files changed, 64 insertions(+), 89 deletions(-)
 delete mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h

diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig
index fc7918c7fc..bbf88fbb9a 100644
--- a/xen/arch/arm/firmware/Kconfig
+++ b/xen/arch/arm/firmware/Kconfig
@@ -8,9 +8,18 @@ config ARM_SCI
 
 menu "Firmware Drivers"
 
+choice
+	prompt "ARM SCI driver type"
+	default SCMI_SMC
+	help
+	Choose which ARM SCI driver to enable.
+
+config ARM_SCI_NONE
+	bool "none"
+
 config SCMI_SMC
 	bool "Forward SCMI over SMC calls from hwdom to EL3 firmware"
-	default y
+	select ARM_SCI
 	help
 	  This option enables basic awareness for SCMI calls using SMC as
 	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
@@ -18,4 +27,6 @@ config SCMI_SMC
 	  firmware node is used to trap and forward corresponding SCMI SMCs
 	  to firmware running at EL3, for calls coming from the hardware domain.
 
+endchoice
+
 endmenu
diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c
index 33473c04b1..13d1137592 100644
--- a/xen/arch/arm/firmware/scmi-smc.c
+++ b/xen/arch/arm/firmware/scmi-smc.c
@@ -9,6 +9,7 @@
  * Copyright 2024 NXP
  */
 
+#include <asm/device.h>
 #include <xen/acpi.h>
 #include <xen/device_tree.h>
 #include <xen/errno.h>
@@ -16,12 +17,11 @@
 #include <xen/sched.h>
 #include <xen/types.h>
 
+#include <asm/firmware/sci.h>
 #include <asm/smccc.h>
-#include <asm/firmware/scmi-smc.h>
 
 #define SCMI_SMC_ID_PROP   "arm,smc-id"
 
-static bool __ro_after_init scmi_enabled;
 static uint32_t __ro_after_init scmi_smc_id;
 
 /*
@@ -41,14 +41,11 @@ static bool scmi_is_valid_smc_id(uint32_t fid)
  *
  * Returns true if SMC was handled (regardless of response), false otherwise.
  */
-bool scmi_handle_smc(struct cpu_user_regs *regs)
+static bool scmi_handle_smc(struct cpu_user_regs *regs)
 {
     uint32_t fid = (uint32_t)get_user_reg(regs, 0);
     struct arm_smccc_res res;
 
-    if ( !scmi_enabled )
-        return false;
-
     if ( !scmi_is_valid_smc_id(fid) )
         return false;
 
@@ -78,49 +75,45 @@ bool scmi_handle_smc(struct cpu_user_regs *regs)
     return true;
 }
 
-static int __init scmi_check_smccc_ver(void)
+static int scmi_smc_domain_init(struct domain *d,
+                                struct xen_domctl_createdomain *config)
 {
-    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
-    {
-        printk(XENLOG_WARNING
-               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
-        return -ENOSYS;
-    }
+    if ( !is_hardware_domain(d) )
+        return 0;
 
+    d->arch.sci_enabled = true;
+    printk(XENLOG_DEBUG "SCMI: %pd init\n", d);
     return 0;
 }
 
-static int __init scmi_dt_init_smccc(void)
+static void scmi_smc_domain_destroy(struct domain *d)
 {
-    static const struct dt_device_match scmi_ids[] __initconst =
-    {
-        /* We only support "arm,scmi-smc" binding for now */
-        DT_MATCH_COMPATIBLE("arm,scmi-smc"),
-        { /* sentinel */ },
-    };
-    const struct dt_device_node *scmi_node;
-    int ret;
+    if ( !is_hardware_domain(d) )
+        return;
 
-    /* If no SCMI firmware node found, fail silently as it's not mandatory */
-    scmi_node = dt_find_matching_node(NULL, scmi_ids);
-    if ( !scmi_node )
-        return -EOPNOTSUPP;
+    printk(XENLOG_DEBUG "SCMI: %pd destroy\n", d);
+}
 
-    ret = dt_property_read_u32(scmi_node, SCMI_SMC_ID_PROP, &scmi_smc_id);
-    if ( !ret )
+static int __init scmi_check_smccc_ver(void)
+{
+    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
     {
-        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
-               SCMI_SMC_ID_PROP, scmi_node->full_name);
-        return -ENOENT;
+        printk(XENLOG_WARNING
+               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
+        return -ENOSYS;
     }
 
-    scmi_enabled = true;
-
     return 0;
 }
 
+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,
+};
+
 /* Initialize the SCMI layer based on SMCs and Device-tree */
-static int __init scmi_init(void)
+static int __init scmi_dom0_init(struct dt_device_node *dev, const void *data)
 {
     int ret;
 
@@ -134,22 +127,36 @@ static int __init scmi_init(void)
     if ( ret )
         return ret;
 
-    ret = scmi_dt_init_smccc();
-    if ( ret == -EOPNOTSUPP )
-        return ret;
+    ret = dt_property_read_u32(dev, SCMI_SMC_ID_PROP, &scmi_smc_id);
+    if ( !ret )
+    {
+        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
+               SCMI_SMC_ID_PROP, dt_node_full_name(dev));
+        return -ENOENT;
+    }
+
+    ret = sci_register(&scmi_smc_ops);
     if ( ret )
-        goto err;
+    {
+        printk(XENLOG_ERR "SCMI: mediator already registered (ret = %d)\n",
+               ret);
+        return ret;
+    }
 
     printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
 
     return 0;
-
- err:
-    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
-    return ret;
 }
 
-__initcall(scmi_init);
+static const struct dt_device_match scmi_smc_match[] __initconst = {
+    DT_MATCH_COMPATIBLE("arm,scmi-smc"),
+    { /* sentinel */ },
+};
+
+DT_DEVICE_START(scmi_smc, "SCMI SMC DOM0", DEVICE_FIRMWARE)
+    .dt_match = scmi_smc_match,
+    .init = scmi_dom0_init,
+DT_DEVICE_END
 
 /*
  * Local variables:
diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h b/xen/arch/arm/include/asm/firmware/scmi-smc.h
deleted file mode 100644
index 6b1a164a40..0000000000
--- a/xen/arch/arm/include/asm/firmware/scmi-smc.h
+++ /dev/null
@@ -1,41 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * xen/arch/arm/include/asm/firmware/scmi-smc.h
- *
- * ARM System Control and Management Interface (SCMI) over SMC
- * Generic handling layer
- *
- * Andrei Cherechesu <andrei.cherechesu@nxp.com>
- * Copyright 2024 NXP
- */
-
-#ifndef __ASM_SCMI_SMC_H__
-#define __ASM_SCMI_SMC_H__
-
-#include <xen/types.h>
-
-struct cpu_user_regs;
-
-#ifdef CONFIG_SCMI_SMC
-
-bool scmi_handle_smc(struct cpu_user_regs *regs);
-
-#else
-
-static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
-{
-    return false;
-}
-
-#endif /* CONFIG_SCMI_SMC */
-
-#endif /* __ASM_SCMI_H__ */
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 2469738fcc..78d5bdf56f 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -21,7 +21,6 @@
 #include <asm/traps.h>
 #include <asm/vpsci.h>
 #include <asm/platform.h>
-#include <asm/firmware/scmi-smc.h>
 
 /* Number of functions currently supported by Hypervisor Service. */
 #define XEN_SMCCC_FUNCTION_COUNT 3
@@ -233,7 +232,7 @@ static bool handle_sip(struct cpu_user_regs *regs)
     if ( platform_smc(regs) )
         return true;
 
-    return scmi_handle_smc(regs);
+    return sci_handle_call(regs);
 }
 
 /*
@@ -301,8 +300,6 @@ 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);
             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/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 55eed9992c..095b1a23e3 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -328,6 +328,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
 #define XEN_DOMCTL_CONFIG_TEE_FFA       2
 
 #define XEN_DOMCTL_CONFIG_ARM_SCI_NONE      0
+#define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC  1
 
 struct xen_arch_domainconfig {
     /* IN/OUT */
-- 
2.34.1

On 28.08.25 19:40, Oleksii Moisieiev wrote:


Hello Oleksii

the patch lgtm, just some comments

> From: Grygorii Strashko <grygorii_strashko@epam.com>
> 
> The introduced SCI (System Control Interface) subsystem provides unified
> interface to integrate in Xen SCI drivers which adds support for ARM
> firmware (EL3, SCP) based software interfaces (like SCMI) that are used in
> system management. The SCI subsystem allows to add drivers for different FW
> interfaces or have different drivers for the same FW interface (for example,
> SCMI with different transports).
> 
> This patch updates SCMI over SMC calls handling layer, introduced by
> commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI over SMC calls handling
> layer"), to be SCI driver:
> - convert to DT device;
> - convert to SCI Xen interface.
> 
> There are no functional changes in general, the driver is just adopted
> to the SCI interface.
> 
> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> 
> Changes in v6:
> - add R-b tag
> 
>   xen/arch/arm/firmware/Kconfig                | 13 ++-
>   xen/arch/arm/firmware/scmi-smc.c             | 93 +++++++++++---------
>   xen/arch/arm/include/asm/firmware/scmi-smc.h | 41 ---------
>   xen/arch/arm/vsmc.c                          |  5 +-
>   xen/include/public/arch-arm.h                |  1 +
>   5 files changed, 64 insertions(+), 89 deletions(-)
>   delete mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h
> 
> diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig
> index fc7918c7fc..bbf88fbb9a 100644
> --- a/xen/arch/arm/firmware/Kconfig
> +++ b/xen/arch/arm/firmware/Kconfig
> @@ -8,9 +8,18 @@ config ARM_SCI
>   
>   menu "Firmware Drivers"
>   
> +choice
> +	prompt "ARM SCI driver type"
> +	default SCMI_SMC
> +	help
> +	Choose which ARM SCI driver to enable.
> +
> +config ARM_SCI_NONE
> +	bool "none"
> +
>   config SCMI_SMC
>   	bool "Forward SCMI over SMC calls from hwdom to EL3 firmware"
> -	default y
> +	select ARM_SCI
>   	help
>   	  This option enables basic awareness for SCMI calls using SMC as
>   	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
> @@ -18,4 +27,6 @@ config SCMI_SMC
>   	  firmware node is used to trap and forward corresponding SCMI SMCs
>   	  to firmware running at EL3, for calls coming from the hardware domain.
>   
> +endchoice
> +
>   endmenu
> diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c
> index 33473c04b1..13d1137592 100644
> --- a/xen/arch/arm/firmware/scmi-smc.c
> +++ b/xen/arch/arm/firmware/scmi-smc.c
> @@ -9,6 +9,7 @@
>    * Copyright 2024 NXP
>    */
>   
> +#include <asm/device.h>

NIT: Should be included after "xen" headers
(could probably be updated on commit)

>   #include <xen/acpi.h>
>   #include <xen/device_tree.h>
>   #include <xen/errno.h>
> @@ -16,12 +17,11 @@
>   #include <xen/sched.h>
>   #include <xen/types.h>
>   
> +#include <asm/firmware/sci.h>
>   #include <asm/smccc.h>
> -#include <asm/firmware/scmi-smc.h>
>   
>   #define SCMI_SMC_ID_PROP   "arm,smc-id"
>   
> -static bool __ro_after_init scmi_enabled;
>   static uint32_t __ro_after_init scmi_smc_id;
>   
>   /*
> @@ -41,14 +41,11 @@ static bool scmi_is_valid_smc_id(uint32_t fid)
>    *
>    * Returns true if SMC was handled (regardless of response), false otherwise.
>    */
> -bool scmi_handle_smc(struct cpu_user_regs *regs)
> +static bool scmi_handle_smc(struct cpu_user_regs *regs)
>   {
>       uint32_t fid = (uint32_t)get_user_reg(regs, 0);
>       struct arm_smccc_res res;
>   
> -    if ( !scmi_enabled )
> -        return false;
> -
>       if ( !scmi_is_valid_smc_id(fid) )
>           return false;
>   
> @@ -78,49 +75,45 @@ bool scmi_handle_smc(struct cpu_user_regs *regs)
>       return true;
>   }
>   
> -static int __init scmi_check_smccc_ver(void)
> +static int scmi_smc_domain_init(struct domain *d,
> +                                struct xen_domctl_createdomain *config)
>   {
> -    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
> -    {
> -        printk(XENLOG_WARNING
> -               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
> -        return -ENOSYS;
> -    }
> +    if ( !is_hardware_domain(d) )
> +        return 0;
>   
> +    d->arch.sci_enabled = true;
> +    printk(XENLOG_DEBUG "SCMI: %pd init\n", d);
>       return 0;
>   }
>   
> -static int __init scmi_dt_init_smccc(void)
> +static void scmi_smc_domain_destroy(struct domain *d)
>   {
> -    static const struct dt_device_match scmi_ids[] __initconst =
> -    {
> -        /* We only support "arm,scmi-smc" binding for now */
> -        DT_MATCH_COMPATIBLE("arm,scmi-smc"),
> -        { /* sentinel */ },
> -    };
> -    const struct dt_device_node *scmi_node;
> -    int ret;
> +    if ( !is_hardware_domain(d) )
> +        return;
>   
> -    /* If no SCMI firmware node found, fail silently as it's not mandatory */
> -    scmi_node = dt_find_matching_node(NULL, scmi_ids);
> -    if ( !scmi_node )
> -        return -EOPNOTSUPP;
> +    printk(XENLOG_DEBUG "SCMI: %pd destroy\n", d);
> +}
>   
> -    ret = dt_property_read_u32(scmi_node, SCMI_SMC_ID_PROP, &scmi_smc_id);
> -    if ( !ret )
> +static int __init scmi_check_smccc_ver(void)
> +{
> +    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
>       {
> -        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
> -               SCMI_SMC_ID_PROP, scmi_node->full_name);
> -        return -ENOENT;
> +        printk(XENLOG_WARNING
> +               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
> +        return -ENOSYS;
>       }
>   
> -    scmi_enabled = true;
> -
>       return 0;
>   }
>   
> +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,
> +};
> +
>   /* Initialize the SCMI layer based on SMCs and Device-tree */
> -static int __init scmi_init(void)
> +static int __init scmi_dom0_init(struct dt_device_node *dev, const void *data)
>   {
>       int ret;
>   
> @@ -134,22 +127,36 @@ static int __init scmi_init(void)
>       if ( ret )
>           return ret;
>   
> -    ret = scmi_dt_init_smccc();
> -    if ( ret == -EOPNOTSUPP )
> -        return ret;
> +    ret = dt_property_read_u32(dev, SCMI_SMC_ID_PROP, &scmi_smc_id);
> +    if ( !ret )
> +    {
> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
> +               SCMI_SMC_ID_PROP, dt_node_full_name(dev));
> +        return -ENOENT;
> +    }

I know that you are just moving the code, but I wonder whether it makes 
sense to validate that retrieved value at least corresponds to SiP 
Service Calls (for the future hardening)?


> +
> +    ret = sci_register(&scmi_smc_ops);
>       if ( ret )
> -        goto err;
> +    {
> +        printk(XENLOG_ERR "SCMI: mediator already registered (ret = %d)\n",
> +               ret);
> +        return ret;
> +    }
>   
>       printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
>   
>       return 0;
> -
> - err:
> -    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
> -    return ret;
>   }
>   
> -__initcall(scmi_init);
> +static const struct dt_device_match scmi_smc_match[] __initconst = {
> +    DT_MATCH_COMPATIBLE("arm,scmi-smc"),
> +    { /* sentinel */ },
> +};
> +
> +DT_DEVICE_START(scmi_smc, "SCMI SMC DOM0", DEVICE_FIRMWARE)
> +    .dt_match = scmi_smc_match,
> +    .init = scmi_dom0_init,
> +DT_DEVICE_END
>   
>   /*
>    * Local variables:
> diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h b/xen/arch/arm/include/asm/firmware/scmi-smc.h
> deleted file mode 100644
> index 6b1a164a40..0000000000
> --- a/xen/arch/arm/include/asm/firmware/scmi-smc.h
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * xen/arch/arm/include/asm/firmware/scmi-smc.h
> - *
> - * ARM System Control and Management Interface (SCMI) over SMC
> - * Generic handling layer
> - *
> - * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> - * Copyright 2024 NXP
> - */
> -
> -#ifndef __ASM_SCMI_SMC_H__
> -#define __ASM_SCMI_SMC_H__
> -
> -#include <xen/types.h>
> -
> -struct cpu_user_regs;
> -
> -#ifdef CONFIG_SCMI_SMC
> -
> -bool scmi_handle_smc(struct cpu_user_regs *regs);
> -
> -#else
> -
> -static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
> -{
> -    return false;
> -}
> -
> -#endif /* CONFIG_SCMI_SMC */
> -
> -#endif /* __ASM_SCMI_H__ */
> -
> -/*
> - * Local variables:
> - * mode: C
> - * c-file-style: "BSD"
> - * c-basic-offset: 4
> - * indent-tabs-mode: nil
> - * End:
> - */
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 2469738fcc..78d5bdf56f 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -21,7 +21,6 @@
>   #include <asm/traps.h>
>   #include <asm/vpsci.h>
>   #include <asm/platform.h>
> -#include <asm/firmware/scmi-smc.h>
>   
>   /* Number of functions currently supported by Hypervisor Service. */
>   #define XEN_SMCCC_FUNCTION_COUNT 3
> @@ -233,7 +232,7 @@ static bool handle_sip(struct cpu_user_regs *regs)
>       if ( platform_smc(regs) )
>           return true;
>   
> -    return scmi_handle_smc(regs);
> +    return sci_handle_call(regs);
>   }
>   
>   /*
> @@ -301,8 +300,6 @@ 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);

These two lines where added by [PATCH v6 1/4] xen/arm: add generic SCI 
subsystem, but here they are removed. This is not a request for an extra 
work right now, but ideally the series should be splitted without an 
extra temporarily changes.

>               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/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 55eed9992c..095b1a23e3 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -328,6 +328,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>   #define XEN_DOMCTL_CONFIG_TEE_FFA       2
>   
>   #define XEN_DOMCTL_CONFIG_ARM_SCI_NONE      0
> +#define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC  1
>   
>   struct xen_arch_domainconfig {
>       /* IN/OUT */

On 29/08/2025 17:42, Oleksandr Tyshchenko wrote:
>
>
> On 28.08.25 19:40, Oleksii Moisieiev wrote:
>
>
> Hello Oleksii
>
> the patch lgtm, just some comments
>
>> From: Grygorii Strashko <grygorii_strashko@epam.com>
>>
>> The introduced SCI (System Control Interface) subsystem provides unified
>> interface to integrate in Xen SCI drivers which adds support for ARM
>> firmware (EL3, SCP) based software interfaces (like SCMI) that are 
>> used in
>> system management. The SCI subsystem allows to add drivers for 
>> different FW
>> interfaces or have different drivers for the same FW interface (for 
>> example,
>> SCMI with different transports).
>>
>> This patch updates SCMI over SMC calls handling layer, introduced by
>> commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI over SMC calls 
>> handling
>> layer"), to be SCI driver:
>> - convert to DT device;
>> - convert to SCI Xen interface.
>>
>> There are no functional changes in general, the driver is just adopted
>> to the SCI interface.
>>
>> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> ---
>>
>> Changes in v6:
>> - add R-b tag
>>
>>   xen/arch/arm/firmware/Kconfig                | 13 ++-
>>   xen/arch/arm/firmware/scmi-smc.c             | 93 +++++++++++---------
>>   xen/arch/arm/include/asm/firmware/scmi-smc.h | 41 ---------
>>   xen/arch/arm/vsmc.c                          |  5 +-
>>   xen/include/public/arch-arm.h                |  1 +
>>   5 files changed, 64 insertions(+), 89 deletions(-)
>>   delete mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h
>>
>> diff --git a/xen/arch/arm/firmware/Kconfig 
>> b/xen/arch/arm/firmware/Kconfig
>> index fc7918c7fc..bbf88fbb9a 100644
>> --- a/xen/arch/arm/firmware/Kconfig
>> +++ b/xen/arch/arm/firmware/Kconfig
>> @@ -8,9 +8,18 @@ config ARM_SCI
>>     menu "Firmware Drivers"
>>   +choice
>> +    prompt "ARM SCI driver type"
>> +    default SCMI_SMC
>> +    help
>> +    Choose which ARM SCI driver to enable.
>> +
>> +config ARM_SCI_NONE
>> +    bool "none"
>> +
>>   config SCMI_SMC
>>       bool "Forward SCMI over SMC calls from hwdom to EL3 firmware"
>> -    default y
>> +    select ARM_SCI
>>       help
>>         This option enables basic awareness for SCMI calls using SMC as
>>         doorbell mechanism and Shared Memory for transport 
>> ("arm,scmi-smc"
>> @@ -18,4 +27,6 @@ config SCMI_SMC
>>         firmware node is used to trap and forward corresponding SCMI 
>> SMCs
>>         to firmware running at EL3, for calls coming from the 
>> hardware domain.
>>   +endchoice
>> +
>>   endmenu
>> diff --git a/xen/arch/arm/firmware/scmi-smc.c 
>> b/xen/arch/arm/firmware/scmi-smc.c
>> index 33473c04b1..13d1137592 100644
>> --- a/xen/arch/arm/firmware/scmi-smc.c
>> +++ b/xen/arch/arm/firmware/scmi-smc.c
>> @@ -9,6 +9,7 @@
>>    * Copyright 2024 NXP
>>    */
>>   +#include <asm/device.h>
[snip]
>> +
>>   /* Initialize the SCMI layer based on SMCs and Device-tree */
>> -static int __init scmi_init(void)
>> +static int __init scmi_dom0_init(struct dt_device_node *dev, const 
>> void *data)
>>   {
>>       int ret;
>>   @@ -134,22 +127,36 @@ static int __init scmi_init(void)
>>       if ( ret )
>>           return ret;
>>   -    ret = scmi_dt_init_smccc();
>> -    if ( ret == -EOPNOTSUPP )
>> -        return ret;
>> +    ret = dt_property_read_u32(dev, SCMI_SMC_ID_PROP, &scmi_smc_id);
>> +    if ( !ret )
>> +    {
>> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" 
>> DT node\n",
>> +               SCMI_SMC_ID_PROP, dt_node_full_name(dev));
>> +        return -ENOENT;
>> +    }
>
> I know that you are just moving the code, but I wonder whether it 
> makes sense to validate that retrieved value at least corresponds to 
> SiP Service Calls (for the future hardening)?
>
>
I don't see any reason for that. I couldn't find any requirement about 
SiP call in [1]
Also, according to DEN0028D [0] 0x05000000 mask can be used as long as 
0x02000000

[0] DEN0028D 
https://documentation-service.arm.com/static/6013e5faeee5236980d08619?token=
[1] DEN0056 https://developer.arm.com/documentation/den0056/latest/
>> +
>> +    ret = sci_register(&scmi_smc_ops);
>>       if ( ret )
>> -        goto err;
>> +    {
>> +        printk(XENLOG_ERR "SCMI: mediator already registered (ret = 
>> %d)\n",
>> +               ret);
>> +        return ret;
>> +    }
>>         printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", 
>> scmi_smc_id);
>>         return 0;
>> -
>> - err:
>> -    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
>> -    return ret;
>>   }
>>   -__initcall(scmi_init);
>> +static const struct dt_device_match scmi_smc_match[] __initconst = {
>> +    DT_MATCH_COMPATIBLE("arm,scmi-smc"),
>> +    { /* sentinel */ },
>> +};
>> +
>> +DT_DEVICE_START(scmi_smc, "SCMI SMC DOM0", DEVICE_FIRMWARE)
>> +    .dt_match = scmi_smc_match,
>> +    .init = scmi_dom0_init,
>> +DT_DEVICE_END
>>     /*
>>    * Local variables:
>> diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h 
>> b/xen/arch/arm/include/asm/firmware/scmi-smc.h
>> deleted file mode 100644
>> index 6b1a164a40..0000000000
>> --- a/xen/arch/arm/include/asm/firmware/scmi-smc.h
>> +++ /dev/null
>> @@ -1,41 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0-only */
>> -/*
>> - * xen/arch/arm/include/asm/firmware/scmi-smc.h
>> - *
>> - * ARM System Control and Management Interface (SCMI) over SMC
>> - * Generic handling layer
>> - *
>> - * Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> - * Copyright 2024 NXP
>> - */
>> -
>> -#ifndef __ASM_SCMI_SMC_H__
>> -#define __ASM_SCMI_SMC_H__
>> -
>> -#include <xen/types.h>
>> -
>> -struct cpu_user_regs;
>> -
>> -#ifdef CONFIG_SCMI_SMC
>> -
>> -bool scmi_handle_smc(struct cpu_user_regs *regs);
>> -
>> -#else
>> -
>> -static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
>> -{
>> -    return false;
>> -}
>> -
>> -#endif /* CONFIG_SCMI_SMC */
>> -
>> -#endif /* __ASM_SCMI_H__ */
>> -
>> -/*
>> - * Local variables:
>> - * mode: C
>> - * c-file-style: "BSD"
>> - * c-basic-offset: 4
>> - * indent-tabs-mode: nil
>> - * End:
>> - */
>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>> index 2469738fcc..78d5bdf56f 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -21,7 +21,6 @@
>>   #include <asm/traps.h>
>>   #include <asm/vpsci.h>
>>   #include <asm/platform.h>
>> -#include <asm/firmware/scmi-smc.h>
>>     /* Number of functions currently supported by Hypervisor Service. */
>>   #define XEN_SMCCC_FUNCTION_COUNT 3
>> @@ -233,7 +232,7 @@ static bool handle_sip(struct cpu_user_regs *regs)
>>       if ( platform_smc(regs) )
>>           return true;
>>   -    return scmi_handle_smc(regs);
>> +    return sci_handle_call(regs);
>>   }
>>     /*
>> @@ -301,8 +300,6 @@ 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);
>
> These two lines where added by [PATCH v6 1/4] xen/arm: add generic SCI 
> subsystem, but here they are removed. This is not a request for an 
> extra work right now, but ideally the series should be splitted 
> without an extra temporarily changes.
>
>>               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/include/public/arch-arm.h 
>> b/xen/include/public/arch-arm.h
>> index 55eed9992c..095b1a23e3 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -328,6 +328,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>>   #define XEN_DOMCTL_CONFIG_TEE_FFA       2
>>     #define XEN_DOMCTL_CONFIG_ARM_SCI_NONE      0
>> +#define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC  1
>>     struct xen_arch_domainconfig {
>>       /* IN/OUT */
>

On Thu, 28 Aug 2025, Oleksii Moisieiev wrote:
> From: Grygorii Strashko <grygorii_strashko@epam.com>
> 
> The introduced SCI (System Control Interface) subsystem provides unified
> interface to integrate in Xen SCI drivers which adds support for ARM
> firmware (EL3, SCP) based software interfaces (like SCMI) that are used in
> system management. The SCI subsystem allows to add drivers for different FW
> interfaces or have different drivers for the same FW interface (for example,
> SCMI with different transports).
> 
> This patch updates SCMI over SMC calls handling layer, introduced by
> commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI over SMC calls handling
> layer"), to be SCI driver:
> - convert to DT device;
> - convert to SCI Xen interface.
> 
> There are no functional changes in general, the driver is just adopted
> to the SCI interface.
> 
> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Also I wanted to write down that the R-b is OK from me