[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer
Hi, On 03/10/2024 17:40, Andrei Cherechesu wrote: 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. Yes. Thanks for the clarification! If there's still anything unclear, let me know. It is now clear. For I agree this should be an error if we can't find the property arm,smc-id. Cheers, -- Julien Grall
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |