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

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

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.

>
> Cheers,
>

Regards,
Andrei Cherechesu