[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





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.

[...]

+        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 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,

--
Julien Grall




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.