Re: [PATCH v3 1/6] xen/arm: firmware: Add SCMI over SMC calls handling layer

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

Hi Michal,

On 19/12/2024 10:35, Michal Orzel wrote:
>
> On 18/12/2024 15:58, Andrei Cherechesu wrote:
>>
>> Hi Michal,
>>
>> Thank you for the review.
>>
>> On 18/12/2024 16:26, Michal Orzel wrote:
>>> On 18/12/2024 11:11, Andrei Cherechesu (OSS) wrote:
>>>> From: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>>>>
>>>> Introduce the SCMI-SMC layer to have some basic degree of
>>>> awareness about SCMI 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 initcall, since we need
>>>> SMCs, and PSCI should already probe EL3 FW for SMCCC support.
>>>> If no "arm,scmi-smc" compatible node is found in the host
>>>> 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 the hardware domain.
>>>>
>>>> Create a new 'firmware' folder to keep the SCMI code separate
>>>> from the generic ARM code.
>>>>
>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>>>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>> ---
>>>>  xen/arch/arm/Kconfig                         |   2 +
>>>>  xen/arch/arm/Makefile                        |   1 +
>>>>  xen/arch/arm/firmware/Kconfig                |  13 ++
>>>>  xen/arch/arm/firmware/Makefile               |   1 +
>>>>  xen/arch/arm/firmware/scmi-smc.c             | 166 +++++++++++++++++++
>>>>  xen/arch/arm/include/asm/firmware/scmi-smc.h |  46 +++++
>>>>  6 files changed, 229 insertions(+)
>>>>  create mode 100644 xen/arch/arm/firmware/Kconfig
>>>>  create mode 100644 xen/arch/arm/firmware/Makefile
>>>>  create mode 100644 xen/arch/arm/firmware/scmi-smc.c
>>>>  create mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h
>>>>
>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>> index 604aba4996..23dc7162a7 100644
>>>> --- a/xen/arch/arm/Kconfig
>>>> +++ b/xen/arch/arm/Kconfig
>>>> @@ -271,6 +271,8 @@ config PARTIAL_EMULATION
>>>>           not been emulated to their complete functionality. Enabling this 
>>>> might
>>>>           result in unwanted/non-spec compliant behavior.
>>>>
>>>> +source "arch/arm/firmware/Kconfig"
>>>> +
>>>>  endmenu
>>>>
>>>>  menu "ARM errata workaround via the alternative framework"
>>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>>>> index e4ad1ce851..8c696c2011 100644
>>>> --- a/xen/arch/arm/Makefile
>>>> +++ b/xen/arch/arm/Makefile
>>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_HAS_PCI) += pci/
>>>>  ifneq ($(CONFIG_NO_PLAT),y)
>>>>  obj-y += platforms/
>>>>  endif
>>>> +obj-y += firmware/
>>>>  obj-$(CONFIG_TEE) += tee/
>>>>  obj-$(CONFIG_HAS_VPCI) += vpci.o
>>>>
>>>> diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig
>>>> new file mode 100644
>>>> index 0000000000..817da745fd
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/firmware/Kconfig
>>>> @@ -0,0 +1,13 @@
>>>> +menu "Firmware Drivers"
>>>> +
>>>> +config SCMI_SMC
>>>> +       bool "Forward SCMI over SMC calls from hwdom 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, for calls coming from the hardware 
>>>> domain.
>>>> +
>>>> +endmenu
>>>> diff --git a/xen/arch/arm/firmware/Makefile 
>>>> b/xen/arch/arm/firmware/Makefile
>>>> new file mode 100644
>>>> index 0000000000..a5e4542666
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/firmware/Makefile
>>>> @@ -0,0 +1 @@
>>>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>>>> diff --git a/xen/arch/arm/firmware/scmi-smc.c 
>>>> b/xen/arch/arm/firmware/scmi-smc.c
>>>> new file mode 100644
>>>> index 0000000000..62657308d6
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/firmware/scmi-smc.c
>>>> @@ -0,0 +1,166 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * xen/arch/arm/firmware/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/smccc.h>
>>>> +#include <asm/firmware/scmi-smc.h>
>>>> +
>>>> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
>>>> +
>>>> +static bool __ro_after_init scmi_support;
>>> I don't understand the need for this variable. IMO it's useless, given that 
>>> in next patch you do:
>>>
>>> ...
>>> if ( scmi_is_enabled() )
>>>     return scmi_handle_smc(regs);
>>>
>>> return false;
>>>
>>> which could simply be changed to:
>>> ...
>>> return scmi_handle_smc(regs);
>>>
>>> and the variable, functions for it, etc. could be removed which would 
>>> simplify the code.
>>>
>>> [...]
>> Well, I agree that the code would maybe be simpler, but
>> that means we would call `scmi_handle_smc()` both when
>> SCMI-SMC layer is initialized and when it is not.
>>
>> That then means that if `scmi_handle_smc()` returns false,
>> we won't know in the caller if the SCMI-SMC layer is not
>> initialized at all or if it is initialized, but the SMC
>> request is invalid (it does not have the SMC ID we expect).
>> Do we need to, though?
> Let me explain more:
> scmi_handle_smc() is called from within handle_sip() that can result 
> true/false.
> If SCMI is disabled, we need to return false. If SCMI is enabled but request 
> is
> invalid we need to return false as well. If SCMI is enabled but not 
> initialized
> we also need to return false. 

Right, that's what I expressed my concern about too: that we're grouping up all
of these cases under a single return code of the same function. I'm fine with
that, though.

> I suggest to drop scmi_is_enabled() as exported
> function and only use scmi_handle_smc() as global like I did in my example.
> Now, this solves the part where SCMI is disabled since you have a stub 
> returning
> false and also the part where SCMI request is invalid. However, this does not
> solve the part where SCMI is enabled but layer not initialized. To fix it, you
> simply need to check inside scmi_handle_smc() if it's initialized. That's much
> simpler than requiring to use another global function which is not nice.

Got it, my understanding was that you suggested to completely drop the 
`scmi_support`
variable and its associated function (`scmi_is_enabled()`), and decide 
internally
whether the SCMI layer is initialized based on another mechanism. That's why I 
suggested
`scmi_smc_id != 0` as mechanism and said that it simplifies the code, but
requires maybe some non-trivial assumptions (if `scmi_smc_id == 0` => SCMI not
initialized yet).
...

>
> Diff below:
> diff --git a/xen/arch/arm/firmware/scmi-smc.c 
> b/xen/arch/arm/firmware/scmi-smc.c
> index 62657308d61d..b3f34bdbb89b 100644
> --- a/xen/arch/arm/firmware/scmi-smc.c
> +++ b/xen/arch/arm/firmware/scmi-smc.c
> @@ -24,12 +24,6 @@
>  static bool __ro_after_init scmi_support;
>  static uint32_t __ro_after_init 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.
> @@ -52,6 +46,9 @@ bool scmi_handle_smc(struct cpu_user_regs *regs)
>      uint32_t fid = (uint32_t)get_user_reg(regs, 0);
>      struct arm_smccc_res res;
>
> +    if ( !scmi_support )
> +        return false;
> +
>      if ( !scmi_is_valid_smc_id(fid) )
>          return false;
>
> diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h 
> b/xen/arch/arm/include/asm/firmware/scmi-smc.h
> index 57cc9eef8676..58730a8037c5 100644
> --- a/xen/arch/arm/include/asm/firmware/scmi-smc.h
> +++ b/xen/arch/arm/include/asm/firmware/scmi-smc.h
> @@ -17,16 +17,10 @@
>
>  #ifdef CONFIG_SCMI_SMC
>
> -bool scmi_is_enabled(void);
>  bool scmi_handle_smc(struct cpu_user_regs *regs);
>
>  #else
>
> -static inline bool scmi_is_enabled(void)
> -{
> -    return false;
> -}
> -
>  static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
>  {
>      return false;
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index c4d225c45cd3..62d8117a120c 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -232,10 +232,7 @@ static bool handle_sip(struct cpu_user_regs *regs)
>      if ( platform_smc(regs) )
>          return true;
>
> -    if ( scmi_is_enabled() )
> -        return scmi_handle_smc(regs);
> -
> -    return false;
> +    return scmi_handle_smc(regs);
>  }
>
>  /*

...
I understand now that your suggestions were not that restrictive :).

Thank you for the clarification!
v4 is on the way.
> ~Michal
>

Regards,
Andrei Cherechesu