Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

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@xxxxxxx>
>
> 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@xxxxxxx>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> ---
>   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@xxxxxxx>
> + * 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@xxxxxxx>
> + * 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