From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Introduce the SCMI layer to have some basic degree of awareness
about SMC calls that are based on the ARM System Control and
Management Interface (SCMI) specification (DEN0056E).

The SCMI specification includes various protocols for managing
system-level resources, such as: clocks, pins, reset, system power,
power domains, performance domains, etc. The clients are named
"SCMI agents" and the server is named "SCMI platform".

Only support the shared-memory based transport with SMCs as
the doorbell mechanism for notifying the platform. Also, this
implementation only handles the "arm,scmi-smc" compatible,
requiring the following properties:
	- "arm,smc-id" (unique SMC ID)
	- "shmem" (one or more phandles pointing to shmem zones
	for each channel)

The initialization is done as 'presmp_initcall', since we need
SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
If no "arm,scmi-smc" compatible node is found in Dom0's
DT, the initialization fails silently, as it's not mandatory.
Otherwise, we get the 'arm,smc-id' DT property from the node,
to know the SCMI SMC ID we handle. The 'shmem' memory ranges
are not validated, as the SMC calls are only passed through
to EL3 FW if coming from Dom0 and as if Dom0 would be natively
running.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 xen/arch/arm/Kconfig                |  10 ++
 xen/arch/arm/Makefile               |   1 +
 xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
 xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
 4 files changed, 226 insertions(+)
 create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
 create mode 100644 xen/arch/arm/scmi-smc.c

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 323c967361..adf53e2de1 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -245,6 +245,16 @@ config PARTIAL_EMULATION
 	  not been emulated to their complete functionality. Enabling this might
 	  result in unwanted/non-spec compliant behavior.
 
+config SCMI_SMC
+	bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
+	default y
+	help
+	  This option enables basic awareness for SCMI calls using SMC as
+	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
+	  compatible only). The value of "arm,smc-id" DT property from SCMI
+	  firmware node is used to trap and forward corresponding SCMI SMCs
+	  to firmware running at EL3, if the call comes from Dom0.
+
 endmenu
 
 menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7792bff597..b85ad9c13f 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
 obj-y += physdev.o
 obj-y += processor.o
 obj-y += psci.o
+obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
 obj-y += setup.o
 obj-y += shutdown.o
 obj-y += smp.o
diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
new file mode 100644
index 0000000000..c6c0079e86
--- /dev/null
+++ b/xen/arch/arm/include/asm/scmi-smc.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * xen/arch/arm/include/asm/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>
+#include <asm/regs.h>
+
+#ifdef CONFIG_SCMI_SMC
+
+bool scmi_is_enabled(void);
+bool scmi_is_valid_smc_id(uint32_t fid);
+bool scmi_handle_smc(struct cpu_user_regs *regs);
+
+#else
+
+static inline bool scmi_is_enabled(void)
+{
+    return false;
+}
+
+static inline bool scmi_is_valid_smc_id(uint32_t fid)
+{
+    return false;
+}
+
+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/scmi-smc.c b/xen/arch/arm/scmi-smc.c
new file mode 100644
index 0000000000..373ca7ba5f
--- /dev/null
+++ b/xen/arch/arm/scmi-smc.c
@@ -0,0 +1,163 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * xen/arch/arm/scmi-smc.c
+ *
+ * ARM System Control and Management Interface (SCMI) over SMC
+ * Generic handling layer
+ *
+ * Andrei Cherechesu <andrei.cherechesu@nxp.com>
+ * Copyright 2024 NXP
+ */
+
+#include <xen/acpi.h>
+#include <xen/device_tree.h>
+#include <xen/errno.h>
+#include <xen/init.h>
+#include <xen/sched.h>
+#include <xen/types.h>
+
+#include <asm/scmi-smc.h>
+#include <asm/smccc.h>
+
+#define SCMI_SMC_ID_PROP   "arm,smc-id"
+
+static bool scmi_support;
+static uint32_t scmi_smc_id;
+
+/* Check if SCMI layer correctly initialized and can be used. */
+bool scmi_is_enabled(void)
+{
+    return scmi_support;
+}
+
+/*
+ * Check if provided SMC Function Identifier matches the one known by the SCMI
+ * layer, as read from DT prop 'arm,smc-id' during initialiation.
+ */
+bool scmi_is_valid_smc_id(uint32_t fid)
+{
+    return (fid == scmi_smc_id);
+}
+
+/*
+ * Generic handler for SCMI-SMC requests, currently only forwarding the
+ * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
+ * layer for SiP SMCs, since SCMI calls are usually provided this way.
+ * Can also be called from `platform_smc()` plat-specific callback.
+ *
+ * Returns true if SMC was handled (regardless of response), false otherwise.
+ */
+bool scmi_handle_smc(struct cpu_user_regs *regs)
+{
+    struct arm_smccc_res res;
+
+    /* Only the hardware domain should use SCMI calls */
+    if ( !is_hardware_domain(current->domain) )
+    {
+        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
+                current->domain->domain_id);
+        return false;
+    }
+
+    /* For the moment, forward the SCMI Request to FW running at EL3 */
+    arm_smccc_1_1_smc(scmi_smc_id,
+                      get_user_reg(regs, 1),
+                      get_user_reg(regs, 2),
+                      get_user_reg(regs, 3),
+                      get_user_reg(regs, 4),
+                      get_user_reg(regs, 5),
+                      get_user_reg(regs, 6),
+                      get_user_reg(regs, 7),
+                      &res);
+
+    set_user_reg(regs, 0, res.a0);
+    set_user_reg(regs, 1, res.a1);
+    set_user_reg(regs, 2, res.a2);
+    set_user_reg(regs, 3, res.a3);
+
+    return true;
+}
+
+static int __init scmi_check_smccc_ver(void)
+{
+    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
+    {
+        printk(XENLOG_ERR
+               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
+        return -ENOSYS;
+    }
+
+    return 0;
+}
+
+static int __init scmi_dt_init_smccc(void)
+{
+    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;
+    const char *smc_id_prop = SCMI_SMC_ID_PROP;
+    int ret;
+
+    /* 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;
+
+    ret = dt_property_read_u32(scmi_node, smc_id_prop, &scmi_smc_id);
+    if ( !ret )
+    {
+        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
+               smc_id_prop, scmi_node->full_name);
+        return -ENOENT;
+    }
+
+    scmi_support = true;
+
+    return 0;
+}
+
+/* Initialize the SCMI layer based on SMCs and Device-tree */
+static int __init scmi_init(void)
+{
+    int ret;
+
+    if ( !acpi_disabled )
+    {
+        printk("SCMI is not supported when using ACPI\n");
+        return -EINVAL;
+    }
+
+    ret = scmi_check_smccc_ver();
+    if ( ret )
+        goto err;
+
+    ret = scmi_dt_init_smccc();
+    if ( ret == -EOPNOTSUPP )
+        return ret;
+    if ( ret )
+        goto err;
+
+    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;
+}
+
+presmp_initcall(scmi_init);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.45.2

Hi

I'd be apprcieated if could consider my comments below.

On 30.09.24 14:47, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Introduce the SCMI layer to have some basic degree of awareness
> about SMC calls that are based on the ARM System Control and
> Management Interface (SCMI) specification (DEN0056E).
> 
> The SCMI specification includes various protocols for managing
> system-level resources, such as: clocks, pins, reset, system power,
> power domains, performance domains, etc. The clients are named
> "SCMI agents" and the server is named "SCMI platform".
> 
> Only support the shared-memory based transport with SMCs as
> the doorbell mechanism for notifying the platform. Also, this
> implementation only handles the "arm,scmi-smc" compatible,
> requiring the following properties:
> 	- "arm,smc-id" (unique SMC ID)
> 	- "shmem" (one or more phandles pointing to shmem zones
> 	for each channel)
> 
> The initialization is done as 'presmp_initcall', since we need
> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
> If no "arm,scmi-smc" compatible node is found in Dom0's
> DT, the initialization fails silently, as it's not mandatory.
> Otherwise, we get the 'arm,smc-id' DT property from the node,
> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
> are not validated, as the SMC calls are only passed through
> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
> running.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>   xen/arch/arm/Kconfig                |  10 ++
>   xen/arch/arm/Makefile               |   1 +
>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++

Could it be moved in separate folder - for example "sci" or "firmware"?
There are definitely more SCMI specific code will be added in the future
as this solution is little bit too simplified.

>   4 files changed, 226 insertions(+)
>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>   create mode 100644 xen/arch/arm/scmi-smc.c
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 323c967361..adf53e2de1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>   	  not been emulated to their complete functionality. Enabling this might
>   	  result in unwanted/non-spec compliant behavior.
>   
> +config SCMI_SMC

Could you please rename it to clearly specify that it is only dom0/hwdom
specific? Like SCMI_SMC_DOM0 or SCMI_SMC_HW_DOM.

> +	bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
> +	default y
> +	help
> +	  This option enables basic awareness for SCMI calls using SMC as
> +	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
> +	  compatible only). The value of "arm,smc-id" DT property from SCMI
> +	  firmware node is used to trap and forward corresponding SCMI SMCs
> +	  to firmware running at EL3, if the call comes from Dom0.
> +
>   endmenu
>   
>   menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7792bff597..b85ad9c13f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>   obj-y += physdev.o
>   obj-y += processor.o
>   obj-y += psci.o
> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>   obj-y += setup.o
>   obj-y += shutdown.o
>   obj-y += smp.o
> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
> new file mode 100644
> index 0000000000..c6c0079e86
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/scmi-smc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/include/asm/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>
> +#include <asm/regs.h>
> +
> +#ifdef CONFIG_SCMI_SMC
> +
> +bool scmi_is_enabled(void);
> +bool scmi_is_valid_smc_id(uint32_t fid);
> +bool scmi_handle_smc(struct cpu_user_regs *regs);
> +
> +#else
> +
> +static inline bool scmi_is_enabled(void)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)

I propose to add "struct domain *d" as the first parameter to make it
more abstract from Xen internals.

> +{
> +    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/scmi-smc.c b/xen/arch/arm/scmi-smc.c
> new file mode 100644
> index 0000000000..373ca7ba5f
> --- /dev/null
> +++ b/xen/arch/arm/scmi-smc.c
> @@ -0,0 +1,163 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/scmi-smc.c
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#include <xen/acpi.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <asm/scmi-smc.h>
> +#include <asm/smccc.h>
> +
> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
> +
> +static bool scmi_support;
> +static uint32_t scmi_smc_id;
> +
> +/* Check if SCMI layer correctly initialized and can be used. */
> +bool scmi_is_enabled(void)
> +{
> +    return scmi_support;
> +}
> +
> +/*
> + * Check if provided SMC Function Identifier matches the one known by the SCMI
> + * layer, as read from DT prop 'arm,smc-id' during initialiation.
> + */
> +bool scmi_is_valid_smc_id(uint32_t fid)

I do not think this API is required at all as it's driver's internals.

> +{
> +    return (fid == scmi_smc_id);
> +}
> +
> +/*
> + * Generic handler for SCMI-SMC requests, currently only forwarding the
> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
> + * Can also be called from `platform_smc()` plat-specific callback.
> + *
> + * Returns true if SMC was handled (regardless of response), false otherwise.
> + */
> +bool scmi_handle_smc(struct cpu_user_regs *regs)

[...]

I'd propose to squash patch "[v2,4/8] xen/arm: vsmc: Enable handling
SiP-owned SCMI SMC calls" to clearly show how API interface is going to
be used.


BR,
-Grygorii

Hi,

On 01/11/2024 15:22, Grygorii Strashko wrote:
> Hi
> 
> I'd be apprcieated if could consider my comments below.
> 
> On 30.09.24 14:47, Andrei Cherechesu (OSS) wrote:
>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>
>> Introduce the SCMI layer to have some basic degree of awareness
>> about SMC calls that are based on the ARM System Control and
>> Management Interface (SCMI) specification (DEN0056E).
>>
>> The SCMI specification includes various protocols for managing
>> system-level resources, such as: clocks, pins, reset, system power,
>> power domains, performance domains, etc. The clients are named
>> "SCMI agents" and the server is named "SCMI platform".
>>
>> Only support the shared-memory based transport with SMCs as
>> the doorbell mechanism for notifying the platform. Also, this
>> implementation only handles the "arm,scmi-smc" compatible,
>> requiring the following properties:
>>     - "arm,smc-id" (unique SMC ID)
>>     - "shmem" (one or more phandles pointing to shmem zones
>>     for each channel)
>>
>> The initialization is done as 'presmp_initcall', since we need
>> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
>> If no "arm,scmi-smc" compatible node is found in Dom0's
>> DT, the initialization fails silently, as it's not mandatory.
>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>> are not validated, as the SMC calls are only passed through
>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>> running.
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> ---
>>   xen/arch/arm/Kconfig                |  10 ++
>>   xen/arch/arm/Makefile               |   1 +
>>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
> 
> Could it be moved in separate folder - for example "sci" or "firmware"?
> There are definitely more SCMI specific code will be added in the future
> as this solution is little bit too simplified.
> 
>>   4 files changed, 226 insertions(+)
>>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>   create mode 100644 xen/arch/arm/scmi-smc.c
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 323c967361..adf53e2de1 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>         not been emulated to their complete functionality. Enabling 
>> this might
>>         result in unwanted/non-spec compliant behavior.
>> +config SCMI_SMC
> 
> Could you please rename it to clearly specify that it is only dom0/hwdom
> specific? Like SCMI_SMC_DOM0 or SCMI_SMC_HW_DOM.

I expect this series to be just a stop gap until we support SCMI for the 
VMs. Once this is merge, I don't expect we would want to keep a Kconfig 
to allow SCMI just for dom0. Therefore, I am not entirely convinced the 
proposed new name is a good idea.

> 
>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 
>> firmware"
>> +    default y
>> +    help
>> +      This option enables basic awareness for SCMI calls using SMC as
>> +      doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
>> +      compatible only). The value of "arm,smc-id" DT property from SCMI
>> +      firmware node is used to trap and forward corresponding SCMI SMCs
>> +      to firmware running at EL3, if the call comes from Dom0.
>> +
>>   endmenu
>>   menu "ARM errata workaround via the alternative framework"
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 7792bff597..b85ad9c13f 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>>   obj-y += physdev.o
>>   obj-y += processor.o
>>   obj-y += psci.o
>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>>   obj-y += setup.o
>>   obj-y += shutdown.o
>>   obj-y += smp.o
>> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/ 
>> include/asm/scmi-smc.h
>> new file mode 100644
>> index 0000000000..c6c0079e86
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/scmi-smc.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * xen/arch/arm/include/asm/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>
>> +#include <asm/regs.h>
>> +
>> +#ifdef CONFIG_SCMI_SMC
>> +
>> +bool scmi_is_enabled(void);
>> +bool scmi_is_valid_smc_id(uint32_t fid);
>> +bool scmi_handle_smc(struct cpu_user_regs *regs);
>> +
>> +#else
>> +
>> +static inline bool scmi_is_enabled(void)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
> 
> I propose to add "struct domain *d" as the first parameter to make it
> more abstract from Xen internals.

I am not sure to understand why we would want the call to be more 
abstract. This function should *only* act on the vCPU currently loaded. 
So it makes sense to use "current->domain".

Cheers,

-- 
Julien Grall

Hi Andrei,

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> +/*
> + * Generic handler for SCMI-SMC requests, currently only forwarding the
> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
> + * Can also be called from `platform_smc()` plat-specific callback.
> + *
> + * Returns true if SMC was handled (regardless of response), false otherwise.
> + */
> +bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    struct arm_smccc_res res;
> +
> +    /* Only the hardware domain should use SCMI calls */
> +    if ( !is_hardware_domain(current->domain) )
> +    {
> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
> +                current->domain->domain_id);
> +        return false;
> +    }
> +
> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
> +    arm_smccc_1_1_smc(scmi_smc_id,

Actually, shouldn't this be get_user_reg(regs, 0) so we don't rely on 
scmi_handle_smc() to be called only when this is equal to scmi_smc_id?

The other option is to move the check for scmi_smc_id in this function.

Cheers,

-- 
Julien Grall

Hi Julien,


On 01/10/2024 12:59, Julien Grall wrote:
> Hi Andrei,
>
> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>> +/*
>> + * Generic handler for SCMI-SMC requests, currently only forwarding the
>> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
>> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
>> + * Can also be called from `platform_smc()` plat-specific callback.
>> + *
>> + * Returns true if SMC was handled (regardless of response), false otherwise.
>> + */
>> +bool scmi_handle_smc(struct cpu_user_regs *regs)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    /* Only the hardware domain should use SCMI calls */
>> +    if ( !is_hardware_domain(current->domain) )
>> +    {
>> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
>> +                current->domain->domain_id);
>> +        return false;
>> +    }
>> +
>> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
>> +    arm_smccc_1_1_smc(scmi_smc_id,
>
> Actually, shouldn't this be get_user_reg(regs, 0) so we don't rely on scmi_handle_smc() to be called only when this is equal to scmi_smc_id?
>
> The other option is to move the check for scmi_smc_id in this function.

I'll move the check for scmi_smc_id in this function and make it static,
to avoid exposing unnecessary complexity to the users of this layer.

Thanks for the suggestion & review.

>
> Cheers,
>

Regards,
Andrei Cherechesu

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Introduce the SCMI layer to have some basic degree of awareness
> about SMC calls that are based on the ARM System Control and
> Management Interface (SCMI) specification (DEN0056E).
> 
> The SCMI specification includes various protocols for managing
> system-level resources, such as: clocks, pins, reset, system power,
> power domains, performance domains, etc. The clients are named
> "SCMI agents" and the server is named "SCMI platform".
> 
> Only support the shared-memory based transport with SMCs as
> the doorbell mechanism for notifying the platform. Also, this
> implementation only handles the "arm,scmi-smc" compatible,
> requiring the following properties:
> 	- "arm,smc-id" (unique SMC ID)
> 	- "shmem" (one or more phandles pointing to shmem zones
> 	for each channel)
> 
> The initialization is done as 'presmp_initcall', since we need
> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.

presmp_initcall() should only be used when we really need to initialize 
a subsytem very early. But it is not clear why this can't be called in 
initcall. Can you clarify?

> If no "arm,scmi-smc" compatible node is found in Dom0's
> DT

You are checking the host device tree. Dom0's DT will be created by Xen 
based on the host device tree when the domain is ceated.

>, the initialization fails silently, as it's not mandatory.
> Otherwise, we get the 'arm,smc-id' DT property from the node,
> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
> are not validated, as the SMC calls are only passed through
> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
> running.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> ---
>   xen/arch/arm/Kconfig                |  10 ++
>   xen/arch/arm/Makefile               |   1 +
>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>   4 files changed, 226 insertions(+)
>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>   create mode 100644 xen/arch/arm/scmi-smc.c
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 323c967361..adf53e2de1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>   	  not been emulated to their complete functionality. Enabling this might
>   	  result in unwanted/non-spec compliant behavior.
>   
> +config SCMI_SMC
> +	bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"

Strictly speaking you are forwarding SMC from the hardware domain. For 
Arm, it is dom0 but it doesn't need to.

> +	default y
 > +	help> +	  This option enables basic awareness for SCMI calls using 
SMC as
> +	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
> +	  compatible only). The value of "arm,smc-id" DT property from SCMI
> +	  firmware node is used to trap and forward corresponding SCMI SMCs
> +	  to firmware running at EL3, if the call comes from Dom0.

Same here.

 > +>   endmenu
>   
>   menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7792bff597..b85ad9c13f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>   obj-y += physdev.o
>   obj-y += processor.o
>   obj-y += psci.o
> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>   obj-y += setup.o
>   obj-y += shutdown.o
>   obj-y += smp.o
> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
> new file mode 100644
> index 0000000000..c6c0079e86
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/scmi-smc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/include/asm/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>
> +#include <asm/regs.h>
> +
> +#ifdef CONFIG_SCMI_SMC
> +
> +bool scmi_is_enabled(void);
> +bool scmi_is_valid_smc_id(uint32_t fid);
> +bool scmi_handle_smc(struct cpu_user_regs *regs);
> +
> +#else
> +
> +static inline bool scmi_is_enabled(void)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return false;
> +}
> +
> +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/scmi-smc.c b/xen/arch/arm/scmi-smc.c
> new file mode 100644
> index 0000000000..373ca7ba5f
> --- /dev/null
> +++ b/xen/arch/arm/scmi-smc.c
> @@ -0,0 +1,163 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/scmi-smc.c
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#include <xen/acpi.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <asm/scmi-smc.h>
> +#include <asm/smccc.h>
> +
> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
> +
> +static bool scmi_support;
> +static uint32_t scmi_smc_id;

AFAICT, the two variables should not change after boot. So they should 
be marked __ro_after_init.

> +
> +/* Check if SCMI layer correctly initialized and can be used. */
> +bool scmi_is_enabled(void)
> +{
> +    return scmi_support;
> +}
> +
> +/*
> + * Check if provided SMC Function Identifier matches the one known by the SCMI
> + * layer, as read from DT prop 'arm,smc-id' during initialiation.
> + */
> +bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return (fid == scmi_smc_id);
> +}
> +
> +/*
> + * Generic handler for SCMI-SMC requests, currently only forwarding the
> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC

s/dom0/hardware domain/

> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
> + * Can also be called from `platform_smc()` plat-specific callback.

I am not entirely sure I understand the value of calling the function 
from platform_smc(). I also don't see any use of it in this series, so 
probably best to remove the sentence for now.

 > + *> + * Returns true if SMC was handled (regardless of response), 
false otherwise.
> + */
> +bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    struct arm_smccc_res res;
> +
> +    /* Only the hardware domain should use SCMI calls */
> +    if ( !is_hardware_domain(current->domain) )
> +    {
> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",

Please use %pd instead d%d. With that you can simply use...

> +                current->domain->domain_id);

... current->domain here.

 > +        return false;> +    }
> +
> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
> +    arm_smccc_1_1_smc(scmi_smc_id,
> +                      get_user_reg(regs, 1),
> +                      get_user_reg(regs, 2),
> +                      get_user_reg(regs, 3),
> +                      get_user_reg(regs, 4),
> +                      get_user_reg(regs, 5),
> +                      get_user_reg(regs, 6),
> +                      get_user_reg(regs, 7),
> +                      &res);
> +
> +    set_user_reg(regs, 0, res.a0);
> +    set_user_reg(regs, 1, res.a1);
> +    set_user_reg(regs, 2, res.a2);
> +    set_user_reg(regs, 3, res.a3);
> +
> +    return true;
> +}
> +
> +static int __init scmi_check_smccc_ver(void)
> +{
> +    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
> +    {
> +        printk(XENLOG_ERR
 > +               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding 
disabled\n");> +        return -ENOSYS;
> +    }
> +
> +    return 0;
> +}
> +
> +static int __init scmi_dt_init_smccc(void)
> +{
> +    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;
> +    const char *smc_id_prop = SCMI_SMC_ID_PROP;

NIT: I would remove smc_id_prop and use SCMI_SMC_ID_PROP directly.

> +    int ret;
> +
> +    /* 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;
> +
> +    ret = dt_property_read_u32(scmi_node, smc_id_prop, &scmi_smc_id);
> +    if ( !ret )
> +    {
> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
> +               smc_id_prop, scmi_node->full_name);
> +        return -ENOENT;
> +    }
> +
> +    scmi_support = true;
> +
> +    return 0;
> +}
> +
> +/* Initialize the SCMI layer based on SMCs and Device-tree */
> +static int __init scmi_init(void)
> +{
> +    int ret;
> +
> +    if ( !acpi_disabled )
> +    {
> +        printk("SCMI is not supported when using ACPI\n");

NIT: Can you use XENLOG_WARN in front?

> +        return -EINVAL;
> +    }
> +
> +    ret = scmi_check_smccc_ver();
> +    if ( ret )
> +        goto err;
> +
> +    ret = scmi_dt_init_smccc();
> +    if ( ret == -EOPNOTSUPP )
> +        return ret;
> +    if ( ret )
> +        goto err;
> +
> +    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);

In the commit message, you said the SCMI subsystem was optional. But 
here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?

> +    return ret;
> +}
> +
> +presmp_initcall(scmi_init);

See my question above about using __initcall.

> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Cheers,

-- 
Julien Grall

Hi Julien,


On 01/10/2024 12:35, Julien Grall wrote:
>
>
> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>
>> Introduce the SCMI layer to have some basic degree of awareness
>> about SMC calls that are based on the ARM System Control and
>> Management Interface (SCMI) specification (DEN0056E).
>>
>> The SCMI specification includes various protocols for managing
>> system-level resources, such as: clocks, pins, reset, system power,
>> power domains, performance domains, etc. The clients are named
>> "SCMI agents" and the server is named "SCMI platform".
>>
>> Only support the shared-memory based transport with SMCs as
>> the doorbell mechanism for notifying the platform. Also, this
>> implementation only handles the "arm,scmi-smc" compatible,
>> requiring the following properties:
>>     - "arm,smc-id" (unique SMC ID)
>>     - "shmem" (one or more phandles pointing to shmem zones
>>     for each channel)
>>
>> The initialization is done as 'presmp_initcall', since we need
>> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
>
> presmp_initcall() should only be used when we really need to initialize a subsytem very early. But it is not clear why this can't be called in initcall. Can you clarify?

Right, I will change it to __initcall(), as there's no specific reason
to do it as a presmp_initcall().

The commit message was meant to imply that PSCI initialization is
a prerequisite (because it also initializes SMC capabilities,
which IMO should be decoupled from PSCI).

But of course there shouldn't be any hard deadline to initializing
SCMI, if obviously we're doing it before Dom0 starts booting.

>
>> If no "arm,scmi-smc" compatible node is found in Dom0's
>> DT
>
> You are checking the host device tree. Dom0's DT will be created by Xen based on the host device tree when the domain is ceated.

Indeed, I will change it to Host DT.

My choice of words came from the train of thought that Dom0
is the closest to a native Linux, which would have the SCMI
firmware node as part of its DT. And Dom0 basically gets
most of the original host device tree by default, except
some special nodes. But I will change it.

Thanks for the suggestion.

>
>> , the initialization fails silently, as it's not mandatory.
>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>> are not validated, as the SMC calls are only passed through
>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>> running.
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> ---
>>   xen/arch/arm/Kconfig                |  10 ++
>>   xen/arch/arm/Makefile               |   1 +
>>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>   4 files changed, 226 insertions(+)
>>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>   create mode 100644 xen/arch/arm/scmi-smc.c
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 323c967361..adf53e2de1 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>         not been emulated to their complete functionality. Enabling this might
>>         result in unwanted/non-spec compliant behavior.
>>   +config SCMI_SMC
>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>
> Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it is dom0 but it doesn't need to.

Well, SCMI is Arm-specific and so are SMCs, but I agree to change
to "hardware domain" / "hwdom" in order to keep the language generic.

Will update everywhere.

>
>> +    default y
> > +    help> +      This option enables basic awareness for SCMI calls using SMC as
>> +      doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
>> +      compatible only). The value of "arm,smc-id" DT property from SCMI
>> +      firmware node is used to trap and forward corresponding SCMI SMCs
>> +      to firmware running at EL3, if the call comes from Dom0.
>
> Same here.

Will update.

>
> > +>   endmenu
>>     menu "ARM errata workaround via the alternative framework"
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 7792bff597..b85ad9c13f 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>>   obj-y += physdev.o
>>   obj-y += processor.o
>>   obj-y += psci.o
>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>>   obj-y += setup.o
>>   obj-y += shutdown.o
>>   obj-y += smp.o
>> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
>> new file mode 100644
>> index 0000000000..c6c0079e86
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/scmi-smc.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * xen/arch/arm/include/asm/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>
>> +#include <asm/regs.h>
>> +
>> +#ifdef CONFIG_SCMI_SMC
>> +
>> +bool scmi_is_enabled(void);
>> +bool scmi_is_valid_smc_id(uint32_t fid);
>> +bool scmi_handle_smc(struct cpu_user_regs *regs);
>> +
>> +#else
>> +
>> +static inline bool scmi_is_enabled(void)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
>> +{
>> +    return false;
>> +}
>> +
>> +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/scmi-smc.c b/xen/arch/arm/scmi-smc.c
>> new file mode 100644
>> index 0000000000..373ca7ba5f
>> --- /dev/null
>> +++ b/xen/arch/arm/scmi-smc.c
>> @@ -0,0 +1,163 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * xen/arch/arm/scmi-smc.c
>> + *
>> + * ARM System Control and Management Interface (SCMI) over SMC
>> + * Generic handling layer
>> + *
>> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> + * Copyright 2024 NXP
>> + */
>> +
>> +#include <xen/acpi.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/errno.h>
>> +#include <xen/init.h>
>> +#include <xen/sched.h>
>> +#include <xen/types.h>
>> +
>> +#include <asm/scmi-smc.h>
>> +#include <asm/smccc.h>
>> +
>> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
>> +
>> +static bool scmi_support;
>> +static uint32_t scmi_smc_id;
>
> AFAICT, the two variables should not change after boot. So they should be marked __ro_after_init.

Right, will update.

>
>
>> +
>> +/* Check if SCMI layer correctly initialized and can be used. */
>> +bool scmi_is_enabled(void)
>> +{
>> +    return scmi_support;
>> +}
>> +
>> +/*
>> + * Check if provided SMC Function Identifier matches the one known by the SCMI
>> + * layer, as read from DT prop 'arm,smc-id' during initialiation.
>> + */
>> +bool scmi_is_valid_smc_id(uint32_t fid)
>> +{
>> +    return (fid == scmi_smc_id);
>> +}
>> +
>> +/*
>> + * Generic handler for SCMI-SMC requests, currently only forwarding the
>> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
>
> s/dom0/hardware domain/

Will update.

>
>
>> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
>> + * Can also be called from `platform_smc()` plat-specific callback.
>
> I am not entirely sure I understand the value of calling the function from platform_smc(). I also don't see any use of it in this series, so probably best to remove the sentence for now.

Will remove. It doesn't need to be called from platform_smc(),
since it's a call chain.

>
>
> > + *> + * Returns true if SMC was handled (regardless of response), false otherwise.
>> + */
>> +bool scmi_handle_smc(struct cpu_user_regs *regs)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    /* Only the hardware domain should use SCMI calls */
>> +    if ( !is_hardware_domain(current->domain) )
>> +    {
>> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
>
> Please use %pd instead d%d. With that you can simply use...

Will update.

>
>> +                current->domain->domain_id);
>
> ... current->domain here.

Will update.

>
> > +        return false;> +    }
>> +
>> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
>> +    arm_smccc_1_1_smc(scmi_smc_id,
>> +                      get_user_reg(regs, 1),
>> +                      get_user_reg(regs, 2),
>> +                      get_user_reg(regs, 3),
>> +                      get_user_reg(regs, 4),
>> +                      get_user_reg(regs, 5),
>> +                      get_user_reg(regs, 6),
>> +                      get_user_reg(regs, 7),
>> +                      &res);
>> +
>> +    set_user_reg(regs, 0, res.a0);
>> +    set_user_reg(regs, 1, res.a1);
>> +    set_user_reg(regs, 2, res.a2);
>> +    set_user_reg(regs, 3, res.a3);
>> +
>> +    return true;
>> +}
>> +
>> +static int __init scmi_check_smccc_ver(void)
>> +{
>> +    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
>> +    {
>> +        printk(XENLOG_ERR
> > +               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");> +        return -ENOSYS;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int __init scmi_dt_init_smccc(void)
>> +{
>> +    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;
>> +    const char *smc_id_prop = SCMI_SMC_ID_PROP;
>
> NIT: I would remove smc_id_prop and use SCMI_SMC_ID_PROP directly.

Sure, will update.

>
>
>> +    int ret;
>> +
>> +    /* 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;
>> +
>> +    ret = dt_property_read_u32(scmi_node, smc_id_prop, &scmi_smc_id);
>> +    if ( !ret )
>> +    {
>> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
>> +               smc_id_prop, scmi_node->full_name);
>> +        return -ENOENT;
>> +    }
>> +
>> +    scmi_support = true;
>> +
>> +    return 0;
>> +}
>> +
>> +/* Initialize the SCMI layer based on SMCs and Device-tree */
>> +static int __init scmi_init(void)
>> +{
>> +    int ret;
>> +
>> +    if ( !acpi_disabled )
>> +    {
>> +        printk("SCMI is not supported when using ACPI\n");
>
> NIT: Can you use XENLOG_WARN in front?

Will add.

>
>
>> +        return -EINVAL;
>> +    }
>> +
>> +    ret = scmi_check_smccc_ver();
>> +    if ( ret )
>> +        goto err;
>> +
>> +    ret = scmi_dt_init_smccc();
>> +    if ( ret == -EOPNOTSUPP )
>> +        return ret;
>> +    if ( ret )
>> +        goto err;
>> +
>> +    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);
>
> In the commit message, you said the SCMI subsystem was optional. But here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?

Well, SCMI is optional, in the sense that if we don't find the
SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
and we return right away (no error printed).

But if we do find the SCMI node, it means that we should initialize
the SCMI subsystem, right? And if we're trying to do that and we
find that the DT node is not correctly formatted (i.e. the
'arm,smc-id' property), I think we should print an error.

However, I think we shouldn't print an error if we return
with an error code from scmi_check_smccc_ver(). And the print
inside scmi_check_smccc_ver() should be a XENLOG_WARN.

So, I think we should print a XENLOG_ERR only if we figured
we need to initialize, and we started to do it but it failed.

What do you think?

>
>> +    return ret;
>> +}
>> +
>> +presmp_initcall(scmi_init);
>
> See my question above about using __initcall.

As mentioned, will change to __initcall.

>
>
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>
> Cheers,
>

On 03/10/2024 16:27, Andrei Cherechesu wrote:
> Hi Julien,

Hi Andrei,

> On 01/10/2024 12:35, Julien Grall wrote:
>>
>>> , the initialization fails silently, as it's not mandatory.
>>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>>> are not validated, as the SMC calls are only passed through
>>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>>> running.
>>>
>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>> ---
>>>    xen/arch/arm/Kconfig                |  10 ++
>>>    xen/arch/arm/Makefile               |   1 +
>>>    xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>>    xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>>    4 files changed, 226 insertions(+)
>>>    create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>>    create mode 100644 xen/arch/arm/scmi-smc.c
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index 323c967361..adf53e2de1 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>>          not been emulated to their complete functionality. Enabling this might
>>>          result in unwanted/non-spec compliant behavior.
>>>    +config SCMI_SMC
>>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>>
>> Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it is dom0 but it doesn't need to.
> 
> Well, SCMI is Arm-specific and so are SMCs, but I agree to change
> to "hardware domain" / "hwdom" in order to keep the language generic.

To clarify, I meant that it would be possible to have an hardware domain 
on Arm. This is not used/implemented today but most of the code is 
adhering to the language. The only reason where we would use dom0 is 
when we explicitely check for domain_id 0 in the code.

[...]

>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    ret = scmi_check_smccc_ver();
>>> +    if ( ret )
>>> +        goto err;
>>> +
>>> +    ret = scmi_dt_init_smccc();
>>> +    if ( ret == -EOPNOTSUPP )
>>> +        return ret;
>>> +    if ( ret )
>>> +        goto err;
>>> +
>>> +    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);
>>
>> In the commit message, you said the SCMI subsystem was optional. But here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?
> 
> Well, SCMI is optional, in the sense that if we don't find the
> SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
> and we return right away (no error printed).
> 
> But if we do find the SCMI node, it means that we should initialize
> the SCMI subsystem, right? And if we're trying to do that and we
> find that the DT node is not correctly formatted (i.e. the
> 'arm,smc-id' property), I think we should print an error.

Correct me if I am wrong, but from the doc, I understood that the 
property arm,smc-id is only necessary if the transport is SMC/HVC.

So I don't think we should print an error if it is not found as this is 
effectively optional.

I agree...

> 
> However, I think we shouldn't print an error if we return
> with an error code from scmi_check_smccc_ver(). And the print
> inside scmi_check_smccc_ver() should be a XENLOG_WARN.
> 
> So, I think we should print a XENLOG_ERR only if we figured
> we need to initialize, and we started to do it but it failed.

... with the two other points.

Cheers,

-- 
Julien Grall

Hi Julien,

On 03/10/2024 19:07, Julien Grall wrote:
>
>
> On 03/10/2024 16:27, Andrei Cherechesu wrote:
>> Hi Julien,
>
> Hi Andrei,
>
>> On 01/10/2024 12:35, Julien Grall wrote:
>>>
>>>> , the initialization fails silently, as it's not mandatory.
>>>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>>>> are not validated, as the SMC calls are only passed through
>>>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>>>> running.
>>>>
>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>> ---
>>>>    xen/arch/arm/Kconfig                |  10 ++
>>>>    xen/arch/arm/Makefile               |   1 +
>>>>    xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>>>    xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>>>    4 files changed, 226 insertions(+)
>>>>    create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>>>    create mode 100644 xen/arch/arm/scmi-smc.c
>>>>
>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>> index 323c967361..adf53e2de1 100644
>>>> --- a/xen/arch/arm/Kconfig
>>>> +++ b/xen/arch/arm/Kconfig
>>>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>>>          not been emulated to their complete functionality. Enabling this might
>>>>          result in unwanted/non-spec compliant behavior.
>>>>    +config SCMI_SMC
>>>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>>>
>>> Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it is dom0 but it doesn't need to.
>>
>> Well, SCMI is Arm-specific and so are SMCs, but I agree to change
>> to "hardware domain" / "hwdom" in order to keep the language generic.
>
> To clarify, I meant that it would be possible to have an hardware domain on Arm. This is not used/implemented today but most of the code is adhering to the language. The only reason where we would use dom0 is when we explicitely check for domain_id 0 in the code.
>
> [...]

Thank you for the clarification.

>
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    ret = scmi_check_smccc_ver();
>>>> +    if ( ret )
>>>> +        goto err;
>>>> +
>>>> +    ret = scmi_dt_init_smccc();
>>>> +    if ( ret == -EOPNOTSUPP )
>>>> +        return ret;
>>>> +    if ( ret )
>>>> +        goto err;
>>>> +
>>>> +    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);
>>>
>>> In the commit message, you said the SCMI subsystem was optional. But here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?
>>
>> Well, SCMI is optional, in the sense that if we don't find the
>> SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
>> and we return right away (no error printed).
>>
>> But if we do find the SCMI node, it means that we should initialize
>> the SCMI subsystem, right? And if we're trying to do that and we
>> find that the DT node is not correctly formatted (i.e. the
>> 'arm,smc-id' property), I think we should print an error.
>
> Correct me if I am wrong, but from the doc, I understood that the property arm,smc-id is only necessary if the transport is SMC/HVC.
>
> So I don't think we should print an error if it is not found as this is effectively optional.

I believe your understanding is correct, the 'arm,smc-id' property
is needed if the SCMI firmware node has either "arm,scmi-smc" or
"arm,scmi-smc-param" compatibles [0]. We only support the
"arm,scmi-smc" compatible with our implementation, though, as you
mentioned there should not be any addresses passed in the regs
alongside the SMC call. That would need to happen with the
"arm,scmi-smc-param" compatible.

But, by "if we do find the SCMI firmware node" I effectively meant
"if we find the node in Host DT with 'arm,scmi-smc' compatible",
since we're only implementing this specific variant. And that's
why I implied that a valid 'arm,smc-id' property is equivalent to
having a correctly defined SCMI firmware node. Having found the
"arm,scmi-smc" compatible node (which IMO means we should commit to
initializing the SCMI layer), shouldn't we print if we tried to init
the SCMI layer and failed (due to incorrect formatting of the node)?

I hope I explained it better now. If there's still anything unclear,
let me know.
 
Anyway, I'm fine either way regarding the prints. If we consider SCMI
layer initialization optional we could also not print any errors if
it fails (no matter why). Correct me if I'm wrong, but IIUC that is
what you're suggesting.

[0] https://elixir.bootlin.com/linux/v6.11/source/Documentation/devicetree/bindings/firmware/arm,scmi.yaml#L359

>
> I agree...
>
>>
>> However, I think we shouldn't print an error if we return
>> with an error code from scmi_check_smccc_ver(). And the print
>> inside scmi_check_smccc_ver() should be a XENLOG_WARN.
>>
>> So, I think we should print a XENLOG_ERR only if we figured
>> we need to initialize, and we started to do it but it failed.
>
> ... with the two other points.
>
> Cheers,
>

Regards,
Andrei C

Hi,

On 03/10/2024 17:40, Andrei Cherechesu wrote:
> Hi Julien,
> 
> On 03/10/2024 19:07, Julien Grall wrote:
>>
>>
>> On 03/10/2024 16:27, Andrei Cherechesu wrote:
>>> Hi Julien,
>>
>> Hi Andrei,
>>
>>> On 01/10/2024 12:35, Julien Grall wrote:
>>>>
>>>>> , the initialization fails silently, as it's not mandatory.
>>>>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>>>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>>>>> are not validated, as the SMC calls are only passed through
>>>>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>>>>> running.
>>>>>
>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>> ---
>>>>>     xen/arch/arm/Kconfig                |  10 ++
>>>>>     xen/arch/arm/Makefile               |   1 +
>>>>>     xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>>>>     xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>>>>     4 files changed, 226 insertions(+)
>>>>>     create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>>>>     create mode 100644 xen/arch/arm/scmi-smc.c
>>>>>
>>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>>> index 323c967361..adf53e2de1 100644
>>>>> --- a/xen/arch/arm/Kconfig
>>>>> +++ b/xen/arch/arm/Kconfig
>>>>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>>>>           not been emulated to their complete functionality. Enabling this might
>>>>>           result in unwanted/non-spec compliant behavior.
>>>>>     +config SCMI_SMC
>>>>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>>>>
>>>> Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it is dom0 but it doesn't need to.
>>>
>>> Well, SCMI is Arm-specific and so are SMCs, but I agree to change
>>> to "hardware domain" / "hwdom" in order to keep the language generic.
>>
>> To clarify, I meant that it would be possible to have an hardware domain on Arm. This is not used/implemented today but most of the code is adhering to the language. The only reason where we would use dom0 is when we explicitely check for domain_id 0 in the code.
>>
>> [...]
> 
> Thank you for the clarification.
> 
>>
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    ret = scmi_check_smccc_ver();
>>>>> +    if ( ret )
>>>>> +        goto err;
>>>>> +
>>>>> +    ret = scmi_dt_init_smccc();
>>>>> +    if ( ret == -EOPNOTSUPP )
>>>>> +        return ret;
>>>>> +    if ( ret )
>>>>> +        goto err;
>>>>> +
>>>>> +    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);
>>>>
>>>> In the commit message, you said the SCMI subsystem was optional. But here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?
>>>
>>> Well, SCMI is optional, in the sense that if we don't find the
>>> SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
>>> and we return right away (no error printed).
>>>
>>> But if we do find the SCMI node, it means that we should initialize
>>> the SCMI subsystem, right? And if we're trying to do that and we
>>> find that the DT node is not correctly formatted (i.e. the
>>> 'arm,smc-id' property), I think we should print an error.
>>
>> Correct me if I am wrong, but from the doc, I understood that the property arm,smc-id is only necessary if the transport is SMC/HVC.
>>
>> So I don't think we should print an error if it is not found as this is effectively optional.
> 
> I believe your understanding is correct, the 'arm,smc-id' property
> is needed if the SCMI firmware node has either "arm,scmi-smc" or
> "arm,scmi-smc-param" compatibles [0]. We only support the
> "arm,scmi-smc" compatible with our implementation, though, as you
> mentioned there should not be any addresses passed in the regs
> alongside the SMC call. That would need to happen with the
> "arm,scmi-smc-param" compatible.
> 
> But, by "if we do find the SCMI firmware node" I effectively meant
> "if we find the node in Host DT with 'arm,scmi-smc' compatible",
> since we're only implementing this specific variant. And that's
> why I implied that a valid 'arm,smc-id' property is equivalent to
> having a correctly defined SCMI firmware node. Having found the
> "arm,scmi-smc" compatible node (which IMO means we should commit to
> initializing the SCMI layer), shouldn't we print if we tried to init
> the SCMI layer and failed (due to incorrect formatting of the node)?
> 
> I hope I explained it better now.

Yes. Thanks for the clarification!

  If there's still anything unclear,
> let me know.

It is now clear. For I agree this should be an error if we can't find 
the property arm,smc-id.

Cheers,

-- 
Julien Grall

On Mon, 30 Sep 2024, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Introduce the SCMI layer to have some basic degree of awareness
> about SMC calls that are based on the ARM System Control and
> Management Interface (SCMI) specification (DEN0056E).
> 
> The SCMI specification includes various protocols for managing
> system-level resources, such as: clocks, pins, reset, system power,
> power domains, performance domains, etc. The clients are named
> "SCMI agents" and the server is named "SCMI platform".
> 
> Only support the shared-memory based transport with SMCs as
> the doorbell mechanism for notifying the platform. Also, this
> implementation only handles the "arm,scmi-smc" compatible,
> requiring the following properties:
> 	- "arm,smc-id" (unique SMC ID)
> 	- "shmem" (one or more phandles pointing to shmem zones
> 	for each channel)
> 
> The initialization is done as 'presmp_initcall', since we need
> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
> If no "arm,scmi-smc" compatible node is found in Dom0's
> DT, the initialization fails silently, as it's not mandatory.
> Otherwise, we get the 'arm,smc-id' DT property from the node,
> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
> are not validated, as the SMC calls are only passed through
> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
> running.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/Kconfig                |  10 ++
>  xen/arch/arm/Makefile               |   1 +
>  xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>  xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>  4 files changed, 226 insertions(+)
>  create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>  create mode 100644 xen/arch/arm/scmi-smc.c
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 323c967361..adf53e2de1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>  	  not been emulated to their complete functionality. Enabling this might
>  	  result in unwanted/non-spec compliant behavior.
>  
> +config SCMI_SMC
> +	bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
> +	default y
> +	help
> +	  This option enables basic awareness for SCMI calls using SMC as
> +	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
> +	  compatible only). The value of "arm,smc-id" DT property from SCMI
> +	  firmware node is used to trap and forward corresponding SCMI SMCs
> +	  to firmware running at EL3, if the call comes from Dom0.
> +
>  endmenu
>  
>  menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7792bff597..b85ad9c13f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>  obj-y += physdev.o
>  obj-y += processor.o
>  obj-y += psci.o
> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>  obj-y += setup.o
>  obj-y += shutdown.o
>  obj-y += smp.o
> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
> new file mode 100644
> index 0000000000..c6c0079e86
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/scmi-smc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/include/asm/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>
> +#include <asm/regs.h>
> +
> +#ifdef CONFIG_SCMI_SMC
> +
> +bool scmi_is_enabled(void);
> +bool scmi_is_valid_smc_id(uint32_t fid);
> +bool scmi_handle_smc(struct cpu_user_regs *regs);
> +
> +#else
> +
> +static inline bool scmi_is_enabled(void)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return false;
> +}
> +
> +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/scmi-smc.c b/xen/arch/arm/scmi-smc.c
> new file mode 100644
> index 0000000000..373ca7ba5f
> --- /dev/null
> +++ b/xen/arch/arm/scmi-smc.c
> @@ -0,0 +1,163 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/scmi-smc.c
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#include <xen/acpi.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <asm/scmi-smc.h>
> +#include <asm/smccc.h>
> +
> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
> +
> +static bool scmi_support;
> +static uint32_t scmi_smc_id;
> +
> +/* Check if SCMI layer correctly initialized and can be used. */
> +bool scmi_is_enabled(void)
> +{
> +    return scmi_support;
> +}
> +
> +/*
> + * Check if provided SMC Function Identifier matches the one known by the SCMI
> + * layer, as read from DT prop 'arm,smc-id' during initialiation.
> + */
> +bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return (fid == scmi_smc_id);
> +}
> +
> +/*
> + * Generic handler for SCMI-SMC requests, currently only forwarding the
> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
> + * Can also be called from `platform_smc()` plat-specific callback.
> + *
> + * Returns true if SMC was handled (regardless of response), false otherwise.
> + */
> +bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    struct arm_smccc_res res;
> +
> +    /* Only the hardware domain should use SCMI calls */
> +    if ( !is_hardware_domain(current->domain) )
> +    {
> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
> +                current->domain->domain_id);
> +        return false;
> +    }
> +
> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
> +    arm_smccc_1_1_smc(scmi_smc_id,
> +                      get_user_reg(regs, 1),
> +                      get_user_reg(regs, 2),
> +                      get_user_reg(regs, 3),
> +                      get_user_reg(regs, 4),
> +                      get_user_reg(regs, 5),
> +                      get_user_reg(regs, 6),
> +                      get_user_reg(regs, 7),
> +                      &res);
> +
> +    set_user_reg(regs, 0, res.a0);
> +    set_user_reg(regs, 1, res.a1);
> +    set_user_reg(regs, 2, res.a2);
> +    set_user_reg(regs, 3, res.a3);
> +
> +    return true;
> +}
> +
> +static int __init scmi_check_smccc_ver(void)
> +{
> +    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
> +    {
> +        printk(XENLOG_ERR
> +               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
> +        return -ENOSYS;
> +    }
> +
> +    return 0;
> +}
> +
> +static int __init scmi_dt_init_smccc(void)
> +{
> +    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;
> +    const char *smc_id_prop = SCMI_SMC_ID_PROP;
> +    int ret;
> +
> +    /* 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;
> +
> +    ret = dt_property_read_u32(scmi_node, smc_id_prop, &scmi_smc_id);
> +    if ( !ret )
> +    {
> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
> +               smc_id_prop, scmi_node->full_name);
> +        return -ENOENT;
> +    }
> +
> +    scmi_support = true;
> +
> +    return 0;
> +}
> +
> +/* Initialize the SCMI layer based on SMCs and Device-tree */
> +static int __init scmi_init(void)
> +{
> +    int ret;
> +
> +    if ( !acpi_disabled )
> +    {
> +        printk("SCMI is not supported when using ACPI\n");
> +        return -EINVAL;
> +    }
> +
> +    ret = scmi_check_smccc_ver();
> +    if ( ret )
> +        goto err;
> +
> +    ret = scmi_dt_init_smccc();
> +    if ( ret == -EOPNOTSUPP )
> +        return ret;
> +    if ( ret )
> +        goto err;
> +
> +    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;
> +}
> +
> +presmp_initcall(scmi_init);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.45.2
>