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

[Julien Grall <julien@xxxxxxx>]

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@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.
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@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.
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