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

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

Hi Julien,

On 03/10/2024 19:07, Julien Grall wrote:
>
>
> On 03/10/2024 16:27, Andrei Cherechesu wrote:
>> Hi Julien,
>
> Hi Andrei,
>
>> On 01/10/2024 12:35, Julien Grall wrote:
>>>
>>>> , 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.
>
> To clarify, I meant that it would be possible to have an hardware domain on 
> Arm. This is not used/implemented today but most of the code is adhering to 
> the language. The only reason where we would use dom0 is when we explicitely 
> check for domain_id 0 in the code.
>
> [...]

Thank you for the clarification.

>
>>>> +        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.
>
> Correct me if I am wrong, but from the doc, I understood that the property 
> arm,smc-id is only necessary if the transport is SMC/HVC.
>
> So I don't think we should print an error if it is not found as this is 
> effectively optional.

I believe your understanding is correct, the 'arm,smc-id' property
is needed if the SCMI firmware node has either "arm,scmi-smc" or
"arm,scmi-smc-param" compatibles [0]. We only support the
"arm,scmi-smc" compatible with our implementation, though, as you
mentioned there should not be any addresses passed in the regs
alongside the SMC call. That would need to happen with the
"arm,scmi-smc-param" compatible.

But, by "if we do find the SCMI firmware node" I effectively meant
"if we find the node in Host DT with 'arm,scmi-smc' compatible",
since we're only implementing this specific variant. And that's
why I implied that a valid 'arm,smc-id' property is equivalent to
having a correctly defined SCMI firmware node. Having found the
"arm,scmi-smc" compatible node (which IMO means we should commit to
initializing the SCMI layer), shouldn't we print if we tried to init
the SCMI layer and failed (due to incorrect formatting of the node)?

I hope I explained it better now. If there's still anything unclear,
let me know.
 
Anyway, I'm fine either way regarding the prints. If we consider SCMI
layer initialization optional we could also not print any errors if
it fails (no matter why). Correct me if I'm wrong, but IIUC that is
what you're suggesting.

[0] 
https://elixir.bootlin.com/linux/v6.11/source/Documentation/devicetree/bindings/firmware/arm,scmi.yaml#L359

>
> I agree...
>
>>
>> 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.
>
> ... with the two other points.
>
> Cheers,
>

Regards,
Andrei C