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

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

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@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.
>
> 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@xxxxxxx>
>> ---
>>   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@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)
>> +{
>> +    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;
>
> 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,
>