Re: [PATCH v6 2/4] xen/arm: scmi-smc: update to be used under sci subsystem

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

On 29/08/2025 17:42, Oleksandr Tyshchenko wrote:
>
>
> On 28.08.25 19:40, Oleksii Moisieiev wrote:
>
>
> Hello Oleksii
>
> the patch lgtm, just some comments
>
>> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>>
>> The introduced SCI (System Control Interface) subsystem provides unified
>> interface to integrate in Xen SCI drivers which adds support for ARM
>> firmware (EL3, SCP) based software interfaces (like SCMI) that are 
>> used in
>> system management. The SCI subsystem allows to add drivers for 
>> different FW
>> interfaces or have different drivers for the same FW interface (for 
>> example,
>> SCMI with different transports).
>>
>> This patch updates SCMI over SMC calls handling layer, introduced by
>> commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI over SMC calls 
>> handling
>> layer"), to be SCI driver:
>> - convert to DT device;
>> - convert to SCI Xen interface.
>>
>> There are no functional changes in general, the driver is just adopted
>> to the SCI interface.
>>
>> Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> ---
>>
>> Changes in v6:
>> - add R-b tag
>>
>>   xen/arch/arm/firmware/Kconfig                | 13 ++-
>>   xen/arch/arm/firmware/scmi-smc.c             | 93 +++++++++++---------
>>   xen/arch/arm/include/asm/firmware/scmi-smc.h | 41 ---------
>>   xen/arch/arm/vsmc.c                          |  5 +-
>>   xen/include/public/arch-arm.h                |  1 +
>>   5 files changed, 64 insertions(+), 89 deletions(-)
>>   delete mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h
>>
>> diff --git a/xen/arch/arm/firmware/Kconfig 
>> b/xen/arch/arm/firmware/Kconfig
>> index fc7918c7fc..bbf88fbb9a 100644
>> --- a/xen/arch/arm/firmware/Kconfig
>> +++ b/xen/arch/arm/firmware/Kconfig
>> @@ -8,9 +8,18 @@ config ARM_SCI
>>     menu "Firmware Drivers"
>>   +choice
>> +    prompt "ARM SCI driver type"
>> +    default SCMI_SMC
>> +    help
>> +    Choose which ARM SCI driver to enable.
>> +
>> +config ARM_SCI_NONE
>> +    bool "none"
>> +
>>   config SCMI_SMC
>>       bool "Forward SCMI over SMC calls from hwdom to EL3 firmware"
>> -    default y
>> +    select ARM_SCI
>>       help
>>         This option enables basic awareness for SCMI calls using SMC as
>>         doorbell mechanism and Shared Memory for transport 
>> ("arm,scmi-smc"
>> @@ -18,4 +27,6 @@ config SCMI_SMC
>>         firmware node is used to trap and forward corresponding SCMI 
>> SMCs
>>         to firmware running at EL3, for calls coming from the 
>> hardware domain.
>>   +endchoice
>> +
>>   endmenu
>> diff --git a/xen/arch/arm/firmware/scmi-smc.c 
>> b/xen/arch/arm/firmware/scmi-smc.c
>> index 33473c04b1..13d1137592 100644
>> --- a/xen/arch/arm/firmware/scmi-smc.c
>> +++ b/xen/arch/arm/firmware/scmi-smc.c
>> @@ -9,6 +9,7 @@
>>    * Copyright 2024 NXP
>>    */
>>   +#include <asm/device.h>
[snip]
>> +
>>   /* Initialize the SCMI layer based on SMCs and Device-tree */
>> -static int __init scmi_init(void)
>> +static int __init scmi_dom0_init(struct dt_device_node *dev, const 
>> void *data)
>>   {
>>       int ret;
>>   @@ -134,22 +127,36 @@ static int __init scmi_init(void)
>>       if ( ret )
>>           return ret;
>>   -    ret = scmi_dt_init_smccc();
>> -    if ( ret == -EOPNOTSUPP )
>> -        return ret;
>> +    ret = dt_property_read_u32(dev, SCMI_SMC_ID_PROP, &scmi_smc_id);
>> +    if ( !ret )
>> +    {
>> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" 
>> DT node\n",
>> +               SCMI_SMC_ID_PROP, dt_node_full_name(dev));
>> +        return -ENOENT;
>> +    }
>
> I know that you are just moving the code, but I wonder whether it 
> makes sense to validate that retrieved value at least corresponds to 
> SiP Service Calls (for the future hardening)?
>
>
I don't see any reason for that. I couldn't find any requirement about 
SiP call in [1]
Also, according to DEN0028D [0] 0x05000000 mask can be used as long as 
0x02000000

[0] DEN0028D 
https://documentation-service.arm.com/static/6013e5faeee5236980d08619?token=
[1] DEN0056 https://developer.arm.com/documentation/den0056/latest/
>> +
>> +    ret = sci_register(&scmi_smc_ops);
>>       if ( ret )
>> -        goto err;
>> +    {
>> +        printk(XENLOG_ERR "SCMI: mediator already registered (ret = 
>> %d)\n",
>> +               ret);
>> +        return ret;
>> +    }
>>         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);
>> -    return ret;
>>   }
>>   -__initcall(scmi_init);
>> +static const struct dt_device_match scmi_smc_match[] __initconst = {
>> +    DT_MATCH_COMPATIBLE("arm,scmi-smc"),
>> +    { /* sentinel */ },
>> +};
>> +
>> +DT_DEVICE_START(scmi_smc, "SCMI SMC DOM0", DEVICE_FIRMWARE)
>> +    .dt_match = scmi_smc_match,
>> +    .init = scmi_dom0_init,
>> +DT_DEVICE_END
>>     /*
>>    * Local variables:
>> diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h 
>> b/xen/arch/arm/include/asm/firmware/scmi-smc.h
>> deleted file mode 100644
>> index 6b1a164a40..0000000000
>> --- a/xen/arch/arm/include/asm/firmware/scmi-smc.h
>> +++ /dev/null
>> @@ -1,41 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0-only */
>> -/*
>> - * xen/arch/arm/include/asm/firmware/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>
>> -
>> -struct cpu_user_regs;
>> -
>> -#ifdef CONFIG_SCMI_SMC
>> -
>> -bool scmi_handle_smc(struct cpu_user_regs *regs);
>> -
>> -#else
>> -
>> -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/vsmc.c b/xen/arch/arm/vsmc.c
>> index 2469738fcc..78d5bdf56f 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -21,7 +21,6 @@
>>   #include <asm/traps.h>
>>   #include <asm/vpsci.h>
>>   #include <asm/platform.h>
>> -#include <asm/firmware/scmi-smc.h>
>>     /* Number of functions currently supported by Hypervisor Service. */
>>   #define XEN_SMCCC_FUNCTION_COUNT 3
>> @@ -233,7 +232,7 @@ static bool handle_sip(struct cpu_user_regs *regs)
>>       if ( platform_smc(regs) )
>>           return true;
>>   -    return scmi_handle_smc(regs);
>> +    return sci_handle_call(regs);
>>   }
>>     /*
>> @@ -301,8 +300,6 @@ static bool vsmccc_handle_call(struct 
>> cpu_user_regs *regs)
>>               break;
>>           case ARM_SMCCC_OWNER_SIP:
>>               handled = handle_sip(regs);
>> -            if ( !handled )
>> -                handled = sci_handle_call(regs);
>
> These two lines where added by [PATCH v6 1/4] xen/arm: add generic SCI 
> subsystem, but here they are removed. This is not a request for an 
> extra work right now, but ideally the series should be splitted 
> without an extra temporarily changes.
>
>>               break;
>>           case ARM_SMCCC_OWNER_TRUSTED_APP ... 
>> ARM_SMCCC_OWNER_TRUSTED_APP_END:
>>           case ARM_SMCCC_OWNER_TRUSTED_OS ... 
>> ARM_SMCCC_OWNER_TRUSTED_OS_END:
>> diff --git a/xen/include/public/arch-arm.h 
>> b/xen/include/public/arch-arm.h
>> index 55eed9992c..095b1a23e3 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -328,6 +328,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>>   #define XEN_DOMCTL_CONFIG_TEE_FFA       2
>>     #define XEN_DOMCTL_CONFIG_ARM_SCI_NONE      0
>> +#define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC  1
>>     struct xen_arch_domainconfig {
>>       /* IN/OUT */
>