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

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

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?

But even if we're not exporting the internal status of the
SCMI layer (enabled/disabled), the driver itself needs to
have an internal state to to know if it's been correctly
initialized or not, right? Otherwise, how would we know
internally if we can actually handle a SCMI request or not?

Well, we could manage to base this mechanism entirely on the
`scmi_smc_id` variable (i.e., if `scmi_smc_id` is not 0,
then we know the SCMI-SMC layer is initialized). This could
work as long as this handler is not called in a situation
where the ID of the SMC we're trapping could actually be 0.
And it currently would be not, since FID == 0 is reserved
for Arm Architecture SMCs.

So, I agree that it makes the code simpler in some sense,
but it will add some underlying assumptions and make it
less explicit.

I'm happy to implement your suggested changes. But I need to
know that I understood correctly your comments and that you
agree with the implications I've stated above.

>
>> +err:
> CODING_STYLE: this should be indented with one space.

Will fix.

>
> ~Michal
>

Regards,
Andrei Cherechesu