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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 09.12.24 19:37, Andrei Cherechesu wrote:
> Hi Julien, Grygorii,
>
> On 11/11/2024 12:33, Julien Grall wrote:
> > 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.
> AFAIU, Julien, you don't agree with renaming the config, nor with moving the
> support to a separate folder since it's something temporary? That's my view
> as well.
>
> These changes do not introduce support for a layer of mediators for
> interacting with system firmware, but only for one simplified implementation.
> So I suppose the patch set that adds that support also creates the folder
> (named 'sci' - per Gregorii's proposal - or 'firmware' to align with Linux),
> and the required config.
>
> But I'm up for doing whatever you consider more suitable.
>
> > > > +    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".
> So this should stay the same, right?
>
> @Grygorii,
>
> Regarding `scmi_is_valid_smc_id()`, I will make it private to the
> SCMI-SMC driver.
>
> And regarding squashing [v2,4/8] to [v2,3/8], IMO it is clearer
> this way: to have the implementation of the driver in a different
> commit than the usage/refactorings needed to accomodate it. And
> this makes it easier to revert the behaviour too, eventually. But
> I don't have a strong preference on this and I'm open to squash
> the commits if you believe it is better that way.
I'm ok with other comments - means it's up to you and maintainers.

But in my opinion it'll be better to consolidate the scmi code in standalone 
folder from
the beginning to avoid possible future files moving/renaming. It's optional, 
new features
and it looks not right to mix it with generic Arm code.

Best regards,
Grygorii